[Feature Proposal]: TriangleMesh - Vertex Color Support

[Knee Snap]

👍

Knee reacted via Gmail
<https://www.google.com/gmail/about/?utm_source=gmail-in-product&utm_medium=et&utm_campaign=emojireactionemail#app>

On Wed, Oct 16, 2024, 1:57 AM Nir Lisker <nlisker at 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 <kneester77 at 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 <nlisker at 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 <kneester77 at 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 <nlisker at 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 <kneester77 at 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 <nlisker at 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ß <
>>>>>>> michaelstrau2 at 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.
>>>>>>>>
>>>>>>>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: text/vnd.google.email-reaction+json
Size: 37 bytes
Desc: not available
URL: <https://mail.openjdk.org/pipermail/openjfx-dev/attachments/20241016/e8125823/attachment-0001.bin>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/openjfx-dev/attachments/20241016/e8125823/attachment-0001.htm>