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

GODOT_single_root #2329

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

aaronfranke
Copy link
Contributor

@aaronfranke aaronfranke commented Sep 18, 2023

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 detect GODOT_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.

@javagl
Copy link
Contributor

javagl commented Sep 18, 2023

Just as a cross-reference: #1542

@aaronfranke
Copy link
Contributor Author

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)

@emackey
Copy link
Member

emackey commented Dec 6, 2023

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 node[0] any different from scene[0]?

Put another way, if you interpret scene[0] as being not a scene but a special node, the sole root node, and then you interpret all entries in the nodes array as being child/descendant nodes, isn't that the same idea? It even separates the kinds of extensions that one is allowed to place on the root node (so-called "scene" extensions) vs the child nodes.

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.

@aaronfranke
Copy link
Contributor Author

aaronfranke commented Dec 6, 2023

Note: As of the last week, this extension is now a part of Godot 4.2 stable and is actively being used in production.

I'm curious about the requirement for the root node to be at index 0 specifically, without any transform placed on it.

Being at index 0 ensures that the "scene" and "scenes" properties are fully redundant, so that they are not required to correctly parse the file (but should still exist for compatibility with existing glTF implementations).

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.

Other than the terminology, how is this type of un-transformed node[0] any different from scene[0]?

The idea is that they are the same, such that scene[0] is fully redundant. The scene of a valid GODOT_single_root file can be fully described without reading the "scene" or "scenes" properties.

It even separates the kinds of extensions that one is allowed to place on the root node (so-called "scene" extensions) vs the child nodes.

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.

Put another way, if you interpret scene[0] as being not a scene but a special node, the sole root node, and then you interpret all entries in the nodes array as being child/descendant nodes, isn't that the same idea?

No, see below.

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?

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 node.schema.json do not automatically apply to scene.schema.json.

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.

Copy link
Member

@emackey emackey left a 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.

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

Successfully merging this pull request may close these issues.

None yet

4 participants