Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

glTF 2.0: Unecessary Inconsistency, Node references Mesh but Skin references Nodes #889

Closed
ParticlePeter opened this issue Mar 21, 2017 · 12 comments

Comments

@ParticlePeter
Copy link

So once more:
Node references Mesh
Node references Camera
But:
Skin references Nodes

Why not keep the Node as flat as possible, with only transformation data and let all other objects that require a transformation reference Node IDs?

Mesh references Node (Nodes for instancing)
Camera references Node
Skin references Nodes

@javagl
Copy link
Contributor

javagl commented Mar 25, 2017

Related (or rather a special case) : #836

I think that the concept of a node referencing to the mesh (for instancing) makes sense. If I understood this correctly, the alternative that you suggested would roughly be

mesh : {
    nodesInstancingThisMesh : [ 3,6,7,8 ]
    ...
}

Personally, I think that the idea of having a "scene graph" with the (leaf) nodes to which the meshes are attached is far more intuitive. But I've learned that (my) intuition may not be in line with other goals.

For skins, the situation is a bit different. They are not "instanced" by being "attached" to a node. Instead, the node provides the information that is rather part of the skin information itself (namely, the skeleton).

@ParticlePeter
Copy link
Author

ParticlePeter commented Mar 28, 2017

First of all I always need to remind myself that we are talking about a data format and not about the internal workings of an engine. However I also think that its not a bad idea to have the format be close to efficient engine design as well as easy parsing.
In the former case AFAIK its more efficient to upload an array of transforms once per frame and index into this array with e.g. instance id and the newer draw id, instead setting one uniform (transform) per draw call.
In the latter case I feel its easier to traverse arrays of meshes, skins and cameras referencing only one element type, the node, instead of traversing the whole node hierarchy, and figuring out which element type is attached. But I guess this is an opinion and personal taste.

Personally, I think that the idea of having a "scene graph" with the (leaf) nodes to which the meshes are attached is far more intuitive. But I've learned that (my) intuition may not be in line with other goals.

That's the way its most often implemented in 3D Apps like Maya or Blender, and in that case I agree, especially for artists manipulating the transform hierarchy, but this is actually bad design for computers. Tom Forsyth's article "Scene Graphs - just say no" (search the page for the title) is actually a good read about that, and can do a much better job explaining then me if you're interested. Similar but with historical evolution of scene graphs is here.

For skins, the situation is a bit different. They are not "instanced" by being "attached" to a node. Instead, the node provides the information that is rather part of the skin information itself (namely, the skeleton).

Yes you're right, but that does not mean that the operation of getting data from the node hierarchy has to be specified in two distinct ways. Essentially there is no difference between joints and transforms, both do nothing more then transform vertices. How they transform, rigid or weighted, is a property of either mesh or skin (and vertex weights of course). So why access them differently?
Think about an instanced and animated flock of birds. Skin vertices must be transformed weighted by nodes as joints and the resulting geometry positioned rigidly and instanced by nodes as transforms. Add node ids to skin as "transform" attribute and you maintain clean consistency.

@lexaknyazev
Copy link
Member

@ParticlePeter

In the former case AFAIK its more efficient to upload an array of transforms once per frame and index into this array with e.g. instance id and the newer draw id, instead setting one uniform (transform) per draw call.
...
Think about an instanced and animated flock of birds. Skin vertices must be transformed weighted by nodes as joints and the resulting geometry positioned rigidly and instanced by nodes as transforms. Add node ids to skin as "transform" attribute and you maintain clean consistency.

This sounds like a breaking change so it's possible only with major version update (i.e. 2.0 or 3.0). Since we're very close to 2.0 release, could you provide full JSON example with skins, instancing, and rigid transforms? This could help making a decision there.


CC @pjcozzi

@pjcozzi
Copy link
Member

pjcozzi commented Mar 29, 2017

There's potential here as an optimization, but it is unlikely that we could scope this for 2.0. We should consider this for 3.0.

@ParticlePeter
Copy link
Author

ParticlePeter commented Mar 29, 2017

flat_nodes_proposal.zip

I edited the SimpleSkin example to show my proposal: gltf_2_skin_example.json
The file has some explanatory comments, removing them should result in valid json. AFAIK it was not valid gltf 2.0, but I tried to follow the 2.0 spec as far as I understand it.
Additionally I edited several schemes (node, mesh, skin) and added two related new ones (morph, morph.target). The later ones are not part of the example, but would be necessary in the context of my proposal as all the morph attributes are removed from node and mesh. As a side effect the morphing mechanism is nicely encapsulated.

I understand if you think that those changes are too late for gltf 2.0. However, the change in schemes does not seem to be huge and moreover simplifies some of them. The conceptual change is a different thing.

@ParticlePeter
Copy link
Author

Did anyone get a chance to look into the files? Any thoughts or comments?

@donmccurdy
Copy link
Contributor

For my $0.02, the v2.0 spec is consistent enough, with nodes referencing meshes, skins, cameras, and lights alike.

Trying to get rid of the scene graph could also dramatically complicate things like animation, where animations that once targeted a single parent node now must be applied to (potentially many) child nodes, we need to add rotation pivots to the spec, etc...

The possible optimizations of flattening the tree make some sense, but given that it’s not how most authoring tools or most engines work, it’s not clear how much benefit we’d see, and seems like a risky change for us to make even in a major version increment IMO.

@ParticlePeter
Copy link
Author

ParticlePeter commented Dec 26, 2017

Trying to get rid of the scene graph could also dramatically complicate things like animation, where animations that once targeted a single parent node now must be applied to (potentially many) child nodes, we need to add rotation pivots to the spec, etc...

I am NOT arguing about the removal of the transformation graph, but rather make the references point in the opposite directions (mesh references node instead of node references mesh). Please reread my first post for details. There is no difference in evaluating and applying animation at all.

... but given that it’s not how most authoring tools or most engines work ...

Did you have a chance to read Tom Forsyth's article "Scene Graphs - just say no" (search the page for the title, and sorry for repeating myself)? Transformation graphs are not the only graphs an engine uses and definitely not the most important ones (drawing meshes in transform graph traversal order is not the smartest idea). But they are the only ones (I can currently think of) which must be part of the file.

Tom Forsyth's "Scene Graphs - just say no" article bottom line (hope it's o.k. to quote him here):

I'm not saying there aren't some good reasons for using one, but after writing a fair number of engines for a fair number of games, I have yet to find any of those reasons particularly compelling. They invented a lot of cool stuff in the 70s and 80s, and there's plenty of it that we continue to ignore at our peril. But some of it was purely an artifact of its time, and should be thwacked over the head and dumped in a shallow grave with a stake through its heart.

@donmccurdy
Copy link
Contributor

I am NOT arguing about the removal of the transformation graph, but rather make the references point in the opposite directions...

Ok, I misunderstood here.

The article makes points that seem relevant to someone implementing an engine, but the proposed syntax changes sound like more indirection for a loader... if the loader needs scene[2], it now has to scan all descendant nodes of scene[2], then scan all meshes for any that point to those nodes to pass data into the engine.

I think you're describing a case where the engine uses the glTF structure in place and makes draw calls directly from that data structure, whereas I'm loading the glTF into an engine's internal scene graph... in my use case, a significant breaking syntax change from 2.0 to 3.0 means more work to maintain backwards compatibility, and the runtime performance is the same.

@ParticlePeter
Copy link
Author

Ok, thanks. You pointed out some flaws while some of them might actually be a merit. Suppose we have a big file with a deep and wide tree hierarchy and scene[2] would point to exactly:

In your case one leaf node. You would then traverse the node hierarchy upwards while applying each nodes local transform.

In my case one mesh. I would then look up its transform index and, based on that index, traverse the hierarchy upwards while applying each nodes local transform.

Three flaws from my side: Firstly my assumption was that the whole per file node tree would be evaluated and local matrices converted to global ones. This would in your case be unnecessary work, but with using the one indirection we would do exactly the same as in your case.
Secondly, I have not updated and uploaded scene.schema.json, which would require arrays of meshes, skins, morphs, cameras and eventually lights.
Thirdly grouping facility is lost. We would have to list all meshes of a multi-mesh hierarchical object in a scene, in contrast to just the objects root node. This one is severe and I need to invest some thoughts on it.

On the other hand there is also merit in the updated scene scheme, we would immediately see which camera (and eventually lights) should be used. In your case you must scan the node hierarchy for a camera and, if your one node is not a leaf node, you need to traverse downwards as well.

I think you're describing a case where the engine uses the glTF structure in place and makes draw calls directly from that data structure, whereas I'm loading the glTF into an engine's internal scene graph...

Almost directly, we still need to evaluate T, R, S nodes and convert local to global transforms. But then yes, you have a nice indexed structure to be accessed form UBOs/TBOs in particular with the instance index.

in my use case, a significant breaking syntax change from 2.0 to 3.0 means more work to maintain backwards compatibility, and the runtime performance is the same.

I am not sure, but isn't this also true for 1.0 and 2.0 ? Are there no braking changes? I admit that having file version conversion/upgrade tools is a small comfort. But I also think lets get it right and break stuff instead of accumulating old flawed cruft.

Proposal advantages:

  • Sane instance syntax
  • Unified transform and skin-joint access
  • Morph encapsulation (born out of necessity)
  • Easier parsing due to single responsibility pattern, contrary to jack-of-all trades node structure
  • Affix new schemes (e.g. lights) without having to touch implementations node parser

@greggman
Copy link
Contributor

I agree with @ParticlePeter entirely. I wrote my first gLTF parser recently and was surprised by all the special cases need. I hope for 3.0 it would be considered trying to make things more generic, less special case. For all the benefits @ParticlePeter mentioned. It becomes easier to parse and new data often just works without having to add more code.

I also think lets get it right and break stuff instead of accumulating old flawed cruft.

Agreed 100%

@donmccurdy
Copy link
Contributor

This sort of change would be possible in a future version of glTF, but we'll need to discuss fairly concrete proposals to make a decision one way or the other there. @greggman has made some helpful suggestions elsewhere (#1527, #1518) so to keep discussion focused let's continue the discussion in those threads.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants