Hoping for further feedback from Michael and others on this feature proposal, as I'm hoping to work on a draft PR soon.
Thanks! On Fri, Aug 23, 2024 at 2:03 PM Knee Snap <kneeste...@gmail.com> wrote: > Thanks for the clarification on how the design would work. > > However, this design is separate/unrelated to the goal of this feature > proposal. > Instead of extending TriangleMesh, you imagine a new separate mesh which > can eventually be used to support user-supplied shaders. > I do hope to propose such a feature at a future date (support user-defined > shaders), but until such a proposal this system isn't super relevant / > doesn't have much relation to the current proposal. > > > *The future we both see for the future of working with meshes is a > scenario with two (or potentially more) mesh classes:* > *#1) *TriangleMesh (No dealing with shaders, buffers, and other advanced > capabilities) > *#2) *VertexMesh (or name it ShaderMesh, etc), which allows the user to > do more advanced capabilities and lets the user define their own > buffers, which could end up looking like the design you've shown. > > But critical to this design is understanding that only TriangleMesh needs > explicit vertex color support. > VertexMesh/ShaderMesh/etc would be able to support vertex colors > implicitly due to its ability to have the user supply arbitrary buffers and > shaders. > So the whole purpose of my proposal is that this feature belongs in > TriangleMesh (or an extension of TriangleMesh), but is currently missing. > The example you've linked however does not extend TriangleMesh, instead > it's starting work on the future proposal, ignoring the need for this > feature in the existing TriangleMesh. > > I hope this helps clarify! > > On Fri, Aug 23, 2024 at 12:53 AM Michael Strauß <michaelstr...@gmail.com> > wrote: > >> I've created a rough prototype to illustrate what I mean: >> https://github.com/mstr2/jfx/tree/experiments/vertexmesh >> >> This is how you would use VertexMesh in an application: >> >> var mesh = new VertexMesh<>(Vertex.PositionTexCoord.class); >> >> mesh.getVertices().addAll( >> new Vertex.PositionTexCoord( >> new Point3D(0, 0, 0), >> new Point2D(0, 0)), >> >> new Vertex.PositionTexCoord( >> new Point3D(100, 0, 0), >> new Point2D(1, 0)), >> >> new Vertex.PositionTexCoord( >> new Point3D(0, 100, 0), >> new Point2D(0, 1)) >> ); >> >> mesh.getIndices().addAll(0, 2, 1); >> >> var meshView = new MeshView(mesh); >> meshView.setMaterial(new PhongMaterial(Color.RED)); >> meshView.setCullFace(CullFace.NONE); >> >> stage.setScene(new Scene(new Group(meshView))); >> stage.show(); >> >> >> In addition to PositionTexCoord, we could then also offer >> PositionNormal, PositionNormalTexCoord, PositionColor, >> PositionNormalColor, and PositionNormalColorTexCoord. These objects >> are supposed to be data carriers for vertices, and could be >> user-definable in the future. >> >> Note that this is by no means a well thought-out proposal, it's just a >> rough sketch to get the basic idea across. Most likely, this API is >> deficient in many ways, so take it as a discussion point rather than a >> serious API proposal. >> >> >> >> On Fri, Aug 23, 2024 at 6:49 AM Knee Snap <kneeste...@gmail.com> wrote: >> > >> > Gottcha, >> > >> > That helps give the context I need to better elaborate. And to be clear >> I'm not suggesting you've done anything wrong, I realized maybe I had >> implied that I was upset, so I just wanted to say explicitly that is not >> the case. >> > >> > Anywho, regarding CustomMesh<TVertex> it would be impossible to inherit >> from TriangleMesh.java without breaking the existing API specification. At >> least, when I assume TVertex is the representation of a single vertex. If >> this assumption was wrong, and it intended to be the definition of the >> vertex, that scenario will also be addressed. >> > >> > TriangleMesh.java does not currently use vertex objects, and making >> such a TVertex to represent each individual vertex is incompatible without >> changing the current public TriangleMesh API specification. If the idea of >> a vertex object only exists within CustomMesh<TVertex> and not >> TriangleMesh, then it's a second-class way of writing to mesh data since it >> would only work on a subset of available mesh types, whereas writing >> directly to the buffers (as it works now) would have worked in all cases. >> If I (someone using JavaFX) want to make utilities for creating meshes, it >> rules out using TVertex unless I commit to never using the base >> TriangleMesh. >> > >> > Additionally, using individual vertex objects provides no utility, but >> requires a decent amount of added code complexity, as now there needs to >> be a way to correlate vertex objects with buffer positions, and keep them >> up to date as the buffer also changes. (What does it mean when a vertex is >> moved in the buffer, but the ObservableFloatArray wasn't told that and it >> was just given a new full array replacement? This is currently the only way >> to update an ObservableFloatArray. Let's consider this vertex object for a >> second. What is it? Is it a wrapper around the underlying buffer? If so, >> every single time the array changes, all vertex objects would become >> invalidated as there's no way to ensure the objects point to the correct >> data in the array, or even to know if that data even exists anymore. If >> it's not a wrapper around the array then we'd need to make changes to the >> array backport to the object. Which has the same problem since the main way >> to update the array is to provide a fully new array, meaning we would have >> no way to associate the new array contents with the old objects. The only >> solution would be to break the API spec and make these new vertex objects >> the authoritative data source and not the arrays, which breaks existing >> code. >> > >> > But I'd like to drive home the final nail. There's pretty much no >> benefit to be had by having vertices as objects anyways. The 3D/GPU >> paradigm is easiest to work with when treating vertex data as arrays and >> not individual vertex objects. (Can refer to OpenInventor, Ogre, etc, to >> confirm this design choice is standard across other object-oriented 3D >> frameworks). This is because at the end of the day, this is what gets >> passed directly to the GPU. Adding layers of abstraction is helpful for >> creating/modifying the array, but not for representing it in memory. >> > >> > In other words, while TVertex might intuitively make sense from a >> general object-oriented perspective, array buffers are almost always >> preferable to vertex objects, even in object-oriented projects. And when >> individual objects are desired, they can exist / act as wrappers in >> user-code, which benefits of objects we cannot provide automatically, as it >> requires information only the user knows about the organization of the >> arrays. >> > >> > But what about if TVertex is not a vertex, but instead a definition of >> what buffers the mesh has? Well, we already have that, and it's called >> VertexFormat. Making it a generic parameter also wouldn't really provide >> any benefit anyways. Instead of making CustomMesh<TriangleMesh>, I propose >> expanding VertexFormat to allow for additional arbitrarily defined buffers. >> However, I do not think we need to expose this functionality publicly yet, >> which is why I've not documented it after the suggestion. We can keep it as >> internal implementation details until it's time to add user-supplied >> shaders. Doing so will give us maximum flexibility when it is time to make >> it public. >> > >> > This way also has the benefit of us being able to retroactively include >> TriangleMesh's points/texCoords/normals arrays in the shader system with >> very little complexity, as they are already part of VertexFormat. >> > >> > Also thanks for the suggestion about the JEPs, I'll keep this in mind >> making future proposals, and it sounds like I should follow-up discussing >> various different implementation options and why I've chosen the one I've >> chosen instead. I suspect the reason this feels somewhat underdeveloped >> from the API perspective is because it's the simplest option I came up with >> that had the best API outcome and I didn't elaborate as much as I could >> have on why others I thought about weren't satisfactory. >> > >> > Thanks again for the feedback, I look forward to hearing back again 😁 >> > >> > On Thu, Aug 22, 2024, 8:08 PM Michael Strauß <michaelstr...@gmail.com> >> wrote: >> >> >> >> I understand that you propose to add a special-purpose mesh >> >> (GouraudShadedTriangleMesh) instead of adding yet another buffer to >> >> the existing TriangleMesh. That might be a valid idea if the goal is >> >> to not overload the TriangleMesh class with special-purpose stuff. >> >> >> >> However, I still feel that the solution space in terms of API isn't >> >> explored in enough detail here. It might be the case that >> >> CustomMesh<TVertex> is not implementable (and it might also be the >> >> case that CustomMesh<TVertex> isn't a good idea to begin with). But at >> >> this point, none of this is obvious to me. >> >> >> >> Usually, when you propose a new feature, you should explain the >> >> motivation, goals and non-goals, alternatives, and so on (you can use >> >> a JEP template for that if you like). You adequately addressed the >> >> motivation for your proposed enhancement, but I feel that the >> >> discussion of different approaches should be expanded upon. I'm not >> >> convinced that CustomMesh<TVertex> is impossible to implement: if >> >> TVertex can only ever be PositionTexCoord, PositionNormalTexCoord, >> >> PositionColorTexCoord, and PositionNormalColorTexCoord (and this is >> >> enforced, for example using sealed interfaces), then why wouldn't we >> >> be able to connect this to our existing shaders? >> >> >> >> Again, I'm not saying that this is a good idea; it might not work for >> >> any number of reasons. But I think these alternative approaches should >> >> at least be explored a little bit before dismissing them. Maybe it >> >> will be GouraudShadedTriangleMesh in the end. >> >> >> >> >> >> On Fri, Aug 23, 2024 at 4:45 AM Knee Snap <kneeste...@gmail.com> >> wrote: >> >> > >> >> > Was hoping to get feedback on my suggestion instead, but another >> suggestion doesn't work. >> >> > >> >> > The idea of a CustomMesh<TVertex> is impossible to implement until >> after we have fully user-supplied shader support, which I've already >> addressed as being not the scope of this change (but instead it is a >> separate future change which is not impacted by this) it also feels like >> this point may have been missed as well. >> >
