>
> I've got no objections on refactoring the validateXyz() methods in
> TriangleMesh to better reflect their names, I had been following the
> previous pattern, and considered doing this change myself, but decided
> against it since the contribution guide discourages refactoring.
>
That's true. I would leave the current code as-is and deal with it later.
Regarding not changing the VertexFormat at all, and only handling colors if
> they exist, I do have one concern, and it's that we should either:
A) Specify in documentation that the color array having more than 0
> elements will cause the color array to be used for rendering.
> B) Add a public method returning a boolean which allows testing if color
> data is present.
>
I'm in favor of option A since I don't think this behavior is likely to
> change (which would be the benefit of a method), and it avoids clutter for
> a simple operation.
Documenting this behavior of the color array is definitely a must, it won't
pass code review otherwise :)
The color array documentation should also detail the "internal structure"
of its data, like the docs do for the other arrays (e.g., "groups of 3").
I see no reason to add a method to check if the color data is used since
colorArray.length == 0 does that.
I'm happy with this approach, shall I go ahead and make those changes?
>
I would say yes. See if it works without any unexpected quirks. Then I
would say the PR will be ready for review.
On Wed, Oct 16, 2024 at 11:14 AM Knee Snap <kneeste...@gmail.com> wrote:
> I've got no objections on refactoring the validateXyz() methods in
> TriangleMesh to better reflect their names, I had been following the
> previous pattern, and considered doing this change myself, but decided
> against it since the contribution guide discourages refactoring.
>
> Regarding not changing the VertexFormat at all, and only handling colors
> if they exist, I do have one concern, and it's that we should either:
> A) Specify in documentation that the color array having more than 0
> elements will cause the color array to be used for rendering.
> B) Add a public method returning a boolean which allows testing if color
> data is present.
>
> I'm in favor of option A since I don't think this behavior is likely to
> change (which would be the benefit of a method), and it avoids clutter for
> a simple operation.
>
> I'm happy with this approach, shall I go ahead and make those changes?
>
> On Wed, Oct 9, 2024, 2:24 PM Nir Lisker <nlis...@gmail.com> wrote:
>
>> The code snippet returns true because if the VertexFormat specifies no
>>> normal data, then the contents of the normals array are valid no matter
>>> what they are, because they are unused. validateNormals() is called
>>> regardless of if normals are used by the VertexFormat which is why it
>>> explicitly includes that check.
>>>
>>> validateNormals() is only returning whether the mesh data is valid, and
>>> if the VertexFormat does not include normal data, then the normal array is
>>> always treated as valid (as normals are not used and thus the mesh is valid
>>> regardless of the contents of the array).
>>>
>>
>> In that case, returning 'false' for an array of length 0 is wrong since
>> there are no normals to be validated.
>>
>> Regarding the last sentence of the previous email, yes, that is the same
>>> as option #2 in my last email. This would break the existing design choices
>>> (as you've noticed with normals), but it is one of the paths I can see
>>> working.
>>>
>>
>> Your option 2 says to make VertexFormat read-only and deprecate the
>> constructor. My suggestion is to keep it as is (for now), and add a color
>> array. Determine if colors are used by a length == 0 check. The vertex
>> format has no impact on this.
>>
>> On Tue, Oct 1, 2024 at 2:01 AM Knee Snap <kneeste...@gmail.com> wrote:
>>
>>> The code snippet returns true because if the VertexFormat specifies no
>>> normal data, then the contents of the normals array are valid no matter
>>> what they are, because they are unused. validateNormals() is called
>>> regardless of if normals are used by the VertexFormat which is why it
>>> explicitly includes that check.
>>>
>>> validateNormals() is only returning whether the mesh data is valid, and
>>> if the VertexFormat does not include normal data, then the normal array is
>>> always treated as valid (as normals are not used and thus the mesh is valid
>>> regardless of the contents of the array).
>>>
>>> Regarding the last sentence of the previous email, yes, that is the same
>>> as option #2 in my last email. This would break the existing design choices
>>> (as you've noticed with normals), but it is one of the paths I can see
>>> working.
>>>
>>>
>>> On Mon, Sep 30, 2024, 2:58 AM Nir Lisker <nlis...@gmail.com> wrote:
>>>
>>>> I would first try to figure out if the code in the snippet I gave is
>>>> actually correct. If it is, then the vertex format has a functional use. If
>>>> it's not, then we can think about what to do with it. I didn't look into
>>>> it, but on the face of it, it doesn't look like the unused normals should
>>>> be validated.
>>>>
>>>> The last sentence in my previous mail suggested a different solution:
>>>> keep the vertex format as it is (for now) to dictate how points and normals
>>>> are read, but add color without interaction with the vertex format. The
>>>> color will be used if it's not empty/null.
>>>> This keeps everything as it is, avoids multiplications of formats, and
>>>> doesn't hinder possible future additions that will also not use the vertex
>>>> format. On the slightly confusing side, it means that vertex format doesn't
>>>> define the format for *all* the data of the vertex, just for its positional
>>>> components that are mandatory (at the very least, points should be
>>>> supplied).
>>>>
>>>> On Mon, Sep 30, 2024 at 3:12 AM Knee Snap <kneeste...@gmail.com> wrote:
>>>>
>>>>> These are some great points!
>>>>>
>>>>> Nir mentioned a builder-style pattern for TriangleMesh, but I struggle
>>>>> to think of how this could work well, as one critical feature of the
>>>>> current API is to be able to dynamically update mesh data, which is very
>>>>> important for any kind of animated mesh. I'd love to see any designs that
>>>>> get prototyped though (regardless of if they are builder-like).
>>>>>
>>>>> Let's discuss the big question though, VertexFormat's design.
>>>>>
>>>>> To be honest, light maps and shadow maps seem unrealistic right now.
>>>>> They are way too intensive to generate in JavaFX (we'd need a LOT of work
>>>>> to do this efficiently, only for the user to not like the algorithm we
>>>>> selected). And even if the user gets this far, they'll be faced with a
>>>>> significant performance penalty. Light maps usually contain data for many
>>>>> 3D objects, but due to JavaFX rendering order being based on the scene
>>>>> graph order, which is going to cause a significant amount of large texture
>>>>> swaps. I'd be open to changing that and do hope to fix the performance
>>>>> issues as much as we can, but overall it strikes me as a very far-fetched
>>>>> feature.
>>>>>
>>>>> Regarding bones/skeletal rigs, that's a really good idea actually. I'm
>>>>> kind of shocked I didn't think of that sooner.
>>>>>
>>>>> I've got a few ideas for how we can tackle these problems, but none
>>>>> particularly jump out as the perfect option to me, so I'm hoping for
>>>>> feedback on what you guys think.
>>>>>
>>>>> *1) Why not let users define their own VertexFormats? If our concern
>>>>> is the explosion of definitions we'd be keeping statically, we could just
>>>>> define a handful of built-in formats but let users define formats as they
>>>>> see fit. Similar to the Cesium Project Nir linked.*
>>>>>
>>>>> *Pros:*
>>>>> - This would avoid the explosion of built-in formats, since we
>>>>> wouldn't need to explicitly define/support every single format, but
>>>>> instead
>>>>> let the user define the format themselves.
>>>>>
>>>>> *Cons:*
>>>>> - Doesn't address the concern of TriangleMesh properties not serving
>>>>> any purpose depending on what VertexFormat it has.
>>>>>
>>>>> *2) Have TriangleMesh automatically determine format based on which
>>>>> buffers contain/do not contain data.*
>>>>> This is extremely similar to #1, except it
>>>>>
>>>>> We could keep VertexFormat around, but make it read-only, so it only
>>>>> serves as a reflection of the data in TriangleMesh, instead of something
>>>>> the user can apply. We'd deprecate the constructor
>>>>> TriangleMesh(VertexFormat) and TriangleMesh.setFormat(VertexFormat) but
>>>>> keep them around (they would do nothing) thus keeping code compatibility
>>>>> (and maybe also binary compatibility?).
>>>>>
>>>>> *Pros:*
>>>>> - Avoid bloat.
>>>>> - Extensible without ever leaving any functionality both unusable and
>>>>> accessible. (IDEA: What if it's not fully removed, but instead is a
>>>>> read-only property obtainable inside TriangleMesh, and the constructor
>>>>> which accepts a VertexFormat deprecated instead? It still feels
>>>>> appropriate
>>>>> to get vertex format data from VertexFormat instead of the mesh itself)
>>>>>
>>>>> *Cons:*
>>>>> - Breaking API change. (If we deprecate/remove VertexFormat)
>>>>> - The user is still responsible for supplying the faces array in the
>>>>> correct format, and making the VertexFormat calculated implicitly behind
>>>>> the scenes (something the user must respond to or the mesh will either not
>>>>> render or display corrupted if we can't tell), would be hidden from the
>>>>> user. Currently the user has to explicitly say they're changing the
>>>>> format,
>>>>> but this automatic behavior is less clear to the user. Not a huge deal but
>>>>> worth considering.
>>>>>
>>>>> The overall outcome of this seems pretty similar to #1, but it loses
>>>>> the benefit of explicitness in exchange for not ever having buffers which
>>>>> are unusable but still accessible. I'm personally torn on if this
>>>>> trade-off
>>>>> is worth it.
>>>>>
>>>>> *3) Add Vertex Colors to VertexFormat, but future features use either
>>>>> #1 or #2.*
>>>>> Bump map tangents/light map uvs/skeleton seem significantly less
>>>>> ubiquitous than vertex colors even if they're still important. Picking
>>>>> this
>>>>> option would expand the already questionable normals design, but
>>>>> considering these future features may never be added, perhaps it makes
>>>>> sense to keep the existing design until we're sure we're even going to add
>>>>> the others. And at that point we'd be in exactly the same position as now
>>>>> with exactly the same options for implementing it. In other words, it buys
>>>>> us time while still letting us scrap VertexFormat if the time ever comes.
>>>>>
>>>>> *Pros:*
>>>>> - Keeps all existing features consistent with existing public API
>>>>> design.
>>>>> - Avoids bloat
>>>>> - Avoids breaking consistency with the existing public API up until
>>>>> the moment we start adding less common data fields.
>>>>>
>>>>> *Cons:*
>>>>> - If Option #1 is picked for future features, the same cons from #1
>>>>> are present.
>>>>> - If Option #2 is picked for future features, the same cons from #2
>>>>> are present.
>>>>> - If we're going to deprecate VertexFormat, it might be a valid point
>>>>> that that we don't actually want to delay that, and would rather do it
>>>>> earlier rather than later.
>>>>>
>>>>> I'm torn between all of the options I've listed here. What do you
>>>>> guys think?
>>>>>
>>>>> We should also discuss how other primitive topologies (POINT, LINE,
>>>>> TRIANGLE, TRIANGLE_STRIP) could work and how this might factor into our
>>>>> VertexFormat discussions. (Should LINE really even be represented by
>>>>> TriangleMesh and not a new LineMesh? Would it make sense to still use
>>>>> VertexFormat?)
>>>>>
>>>>> On Mon, Sep 23, 2024, 9:23 AM Nir Lisker <nlis...@gmail.com> wrote:
>>>>>
>>>>>> Having gone through some sources, I found the additional properties
>>>>>> that can be added as per-vertex data:
>>>>>> * Additional texture coordinates (mentioned by Michael). These can be
>>>>>> used for detailed textures.
>>>>>> * Bone indices and weights. These are for GPU skinning.
>>>>>> Interestingly, the D3D vertex shader constants file has some preparation
>>>>>> for skinning and bones [1].
>>>>>> * Tangents. Used similarly to normals in some lighting-related
>>>>>> calculations.
>>>>>>
>>>>>> While it's not clear that we'll need these, the proliferation of
>>>>>> vertex formats will be unmanageable with even one of these added (in
>>>>>> addition to color).
>>>>>>
>>>>>> Looking at the use of VertexFormat in TriangleMesh with regards to
>>>>>> normals:
>>>>>>
>>>>>> private boolean validateNormals() {
>>>>>> // Only validate normals if vertex format has normal component
>>>>>> if (getVertexFormat() != VertexFormat.POINT_NORMAL_TEXCOORD)
>>>>>> return true;
>>>>>>
>>>>>> if (normals.size() == 0) { // Valid but meaningless for picking
>>>>>> or rendering.
>>>>>> return false;
>>>>>> ...
>>>>>> }
>>>>>>
>>>>>> I'm confused by this. If the normals should be validated only when
>>>>>> the vertex format defines that normals are used, why does the method
>>>>>> return
>>>>>> true when normals aren't used?
>>>>>> I would think, along with what Michael said, that either if the
>>>>>> vertex format doesn't declare normals, or the user doesn't declare normal
>>>>>> (size ==0), then the normals wouldn't be validated. That would also mean
>>>>>> that the vertex format is redundant since it's implied by the
>>>>>> existence/length of the array.
>>>>>>
>>>>>> Perhaps it's worth looking at the color implementation this way.
>>>>>> Additional vertex formats should not be added, and the use of vertex
>>>>>> colors
>>>>>> should be dictated by looking at the array.
>>>>>>
>>>>>> [1]
>>>>>> https://github.com/openjdk/jfx/blob/master/modules/javafx.graphics/src/main/native-prism-d3d/hlsl/vsConstants.h
>>>>>>
>>>>>> On Sat, Sep 21, 2024 at 5:17 AM Michael Strauß <
>>>>>> michaelstr...@gmail.com> wrote:
>>>>>>
>>>>>>> > I'm not yet convinced that new vertex formats are the way to go.
>>>>>>>
>>>>>>> To be fair, I'm not sure why VertexFormat even exists. It serves no
>>>>>>> purpose when creating a TriangleMesh, as the vertex format could
>>>>>>> easily be inferred by the presence (or absence) of vertex data.
>>>>>>> Having
>>>>>>> users specify the VertexFormat explicitly is probably not a good API,
>>>>>>> as it now makes the other data components be dependent on the choice
>>>>>>> of the vertex format.
>>>>>>>
>>>>>>> Of course, not all combinations of vertex data are valid, but this
>>>>>>> could be specified in documentation and be validated by the
>>>>>>> implementation. I think it might be best to deprecate VertexFormat
>>>>>>> for
>>>>>>> removal. This would also prevent a potential explosion of vertex
>>>>>>> formats in the public API.
>>>>>>>
>>>>>>> I have no objection to the choice to add vertex colors as an
>>>>>>> additional data component to TriangleMesh. It adds to the API, but
>>>>>>> this is not a slippery slope as there's only a very finite set of
>>>>>>> features we might be tempted to add next:
>>>>>>>
>>>>>>> The most obvious thing that JavaFX 3D really can't do is shadows.
>>>>>>> These come in static form (light mapping) and dynamic form (shadow
>>>>>>> mapping). Light mapping requires a second set of texcoords, while
>>>>>>> shadow mapping does not.
>>>>>>>
>>>>>>> Adding light mapping would make JavaFX 3D competitive with late-90's
>>>>>>> graphics, and adding shadow mapping would make it competitive with
>>>>>>> early-2000's graphics.
>>>>>>>
>>>>>>
