After having started making the changes, I realize that this method breaks
the existing specification of VertexFormat.java.

VertexFormat.java defines getVertexIndexSize() as "the number of component
indices that represents a vertex."
So, for VertexFormat.POINT_TEXCOORD, that number will be 2, and for
VertexFormat.POINT_NORMAL_TEXCOORD that number will be 3.
When colors are added, we are adding another component index to the vertex,
but VertexFormat will not reflect this change, but since VertexFormat will
be unable to reflect this change, its spec is no longer correct.
In this case I do think we need to add this getVertexIndexSize() to
TriangleMesh as well, since there would be no way to get this information
anymore.

Should I continue with this approach, or should we discuss further?

On Wed, Oct 16, 2024 at 1:57 AM Nir Lisker <nlis...@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.
>>
>
> 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.
>>>>>>>>
>>>>>>>

Reply via email to