-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
GODOT_single_root #2329
GODOT_single_root #2329
Conversation
Just as a cross-reference: #1542 |
Yes, the restrictions here are very much related to #1542. For further explanation of the problem and proposed solution, see my comment at the bottom of that issue: #1542 (comment) |
I'm curious about the requirement for the root node to be at index 0 specifically, without any transform placed on it. Other than the terminology, how is this type of un-transformed Put another way, if you interpret If an extra blank root node is appearing on some round-trip, perhaps the importer/exporter code needs better alignment between its two halves to better understand the role of a glTF "scene" as being the sole root node. So, why not use glTF's "scene" as a node instead of a scene? Then all existing single-scene glTF assets automatically become single-root-node assets. |
Note: As of the last week, this extension is now a part of Godot 4.2 stable and is actively being used in production.
Being at index 0 ensures that the Having no transform is required to ensure the behavior does not change compared to importing without support for this extension. Aside from that, it's a bad practice to have the root node be transformed.
The idea is that they are the same, such that
Separating these is explicitly not desired. Everything valid for a sub-node should also be valid for the root node. Doing otherwise is silly and adds complications for exporters for no good reason.
No, see below.
The problem is that this would require duplicating all data schemas and parsing logic for nodes to also exist on scenes. glTF extensions that apply to An even bigger problem is that this would break compatibility importing in tools that do not support reading node data from the scene. With what you propose, an implementation that does not support the extension would result in the scene data being discarded. With this PR, an implementation that does not support the extension would result in all desired nodes still existing but just an extra root may be generated. We simply must ensure that there is no breakage when a glTF reader without support for this extension encounters a file with it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this is a vendor extension used in production vendor software, we should probably merge this (and generally any other PRs in similar condition). My approval here is not an endorsement for use of this in other software, just an acknowledgement that we should allow vendor-specific documentation of its use.
What is the Khronos stance on merging vendor extensions? This is finalized and actively used in production. |
Maybe an explicit ping @lexaknyazev can help. (I skimmed over the requested changes, and saw that they had been marked as 'resolved', but not by the reviewer - so this PR will at least need another approval, to make sure that the changes actually are resolved from the standpoint of the reviewer). (No opinion from my side. The goal of the extension itself sounds reasonable, as a somewhat standardized, non-breaking way of solving #1542 , and the goal of ~"making |
Hey all, I feel it's important that vendor extensions are approved and merged regularly (in the absence of egregious quality issues). If there's not bandwidth for a thorough review from working group members, that's entirely understandable! But perhaps the default in that case should be to merge extensions as-is, and perhaps even automatically. GitHub "labels" on extension PRs could be helpful as well, to indicate general PR status and next steps. |
/ping @lexaknyazev This PR is actually blocked by your earlier request for changes. None of us have GitHub permission to merge this with that request still present here. Can you remove the block? |
@lexaknyazev Can you reply and remove the block or at least state reasoning for keeping the block? |
extensions/2.0/Vendor/GODOT_single_root/examples/dynamic_body_root.gltf
Outdated
Show resolved
Hide resolved
The example asset has been removed as requested. |
@lexaknyazev This PR is blocked from merging by your request for changes. |
@lexaknyazev Updated per your latest comments. Clarified that nodes are allowed to not be in the scene.
Those are all prerequisite conditions to being compliant with
With
In this case, we have no choice but to make "glTF Scene" be the root. We don't want the existence or non-existence of "glTF Node C" to affect the behavior of "glTF Node A" as the scene root or not. Therefore, we make an explicit flag for files to ask for "glTF Node A" to be the root, and this flag is only valid when the prerequisite conditions are true. |
So the goal is to explicitly flag assets that should be imported without creating an extra "Scene" node assuming that they are compatible with such workflows. In other words, the main point of the extension is not to add some restrictions but to signal the intended asset usage that needs those restrictions. Following the example above, the behaviour should then be:
The first sentence of the extension overview should highlight this key detail (feel free to tweak the language):
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM since the extension intention is clearly stated now.
Preview: https://github.com/aaronfranke/glTF/tree/GODOT_single_root/extensions/2.0/Vendor/GODOT_single_root
Many games engines import glTF scenes as objects with a single root node. Unity has prefabs with a single root GameObject, and Godot has scenes with a single root node. Since glTF allows defining multiple root nodes, engines will insert the glTF root nodes as children of the "real" root node, which makes it difficult for a glTF file to define the properties of the "real" root node. This is important for things like physics bodies and character controllers, which are defined on the root node, so that all child nodes are moved with the body. Aside from physics, a single root node would avoid an extra node in the tree when round-tripping between glTF and these engines.
The
GODOT_single_root
glTF extension solves this problem by imposing additional constraints on the glTF scene to ensure it can be parsed into one of these objects with a single root node. Implementations can detectGODOT_single_root
and import the single root node as the object's "real" root node in the scene/prefab/etc. The extension contains no data, it only restricts behavior and gives a hint to importers.If there is interest from other vendors, this could be made into a multi-vendor extension. The restrictions imposed here could also be added to the base glTF spec in a future version (could be, say, "2.1" since this can be done without breaking compatibility as long as we require the scene/scenes boilerplate).
EDIT: Also note, the restrictions imposed here are a superset of #1542.