Skip to content

GLTFLoader: Add support for importing single root files #31112

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

Closed
wants to merge 7 commits into from

Conversation

aaronfranke
Copy link
Contributor

@aaronfranke aaronfranke commented May 15, 2025

Description

This PR adds support for importing files with the GODOT_single_root extension to GLTFLoader. This vendor extension is recognized by Khronos in their repository's vendor extensions folder (but not developed by or officially endorsed by Khronos).

Old text collapsed, I removed the export code so this is no longer correct:

With this PR, if you round-trip a non-scene Three.JS object like this:

const loader = new GLTFLoader();
const exporter = new GLTFExporter();

exporter.parse(myobject, function (gltfJson) {
	// We need to make a URL so that GLTFLoader can load the data.
	const blob = new Blob([JSON.stringify(gltfJson)], { type: "application/json" });
	const url = URL.createObjectURL(blob);
	loader.load(url, function (generated) {
		console.log(generated.scene);
	});
]);

... then the generated.scene object corresponds to the same object that was exported, with the same name, and with any data that can survive a round-trip through glTF node extensions. Without this PR, in the current dev branch, it will instead return a Group named "AuxScene", and the exported node will be a child of that, needlessly creating an extra node.

Also with this PR, if you round-trip a glTF file with the GODOT_single_root flag:

const loader = new GLTFLoader();
const exporter = new GLTFExporter();

loader.load("/path/to/my/file.gltf", function (generated) {
	exporter.parse(generated.scene, function (gltfJson) {
		console.log(gltfJson);
	});
]);

... then the exported glTF JSON data will have the GODOT_single_root flag set, because generated.scene is a single non-Scene object. This also applies to other objects regardless of if they came from a GODOT_single_root input file.

This improves interoperability with Godot Engine, including exporting files from Godot to Three.JS. I could imagine it being useful to use Godot as a visual tool to make objects for Three.JS. Other apps and engines may choose to support GODOT_single_root if they wish, such as a potential future Blender extension geared towards game development.

The code for this is heavily integrated into loadScene() so it can't be done with an extension to GLTFLoader, at most you could have code that wraps around GLTFLoader. Also, the code for this is quite minimal, most of the diffs are from indenting already existing code one scope further in.

Note: The "GODOT_single_root" extension name is prefixed with "GODOT_" because that is the context in which that extension was developed, however this does not mean it is only intended to be used for Godot. Quite the opposite actually, this is designed to improve interoperability between engines for glTF files containing "one object". The "EXT_" prefix is reserved for extensions developed by multiple vendors, it does not prescribe which apps can use it.

Also, I noticed the docs for GLTFLoader.parse said "Parses the given FBX data and returns the resulting group.". This is wrong in two ways: it's glTF not FBX, and it doesn't return the Group, it returns (in the callback) an object holding a bunch of generated data, which includes the single root node (which is Group without the single root flag, or another Object3D generated from the glTF root node with the single root flag). I've updated the description of this function.

This is my first contribution to Three.JS. I've tested this and I've done my best to follow the existing code style. If any changes are needed I will be happy to update the PR.

@aaronfranke
Copy link
Contributor Author

aaronfranke commented May 15, 2025

The CI failures seem to be unrelated to this PR. If I run npm run test-e2e on the dev branch, I also get lots of errors. This PR does not change behavior when files do not have GODOT_single_root (const isSingleRoot being false causes it to run the same code as in dev, the only other thing is I moved const nodeIds up).

@gkjohnson gkjohnson requested a review from donmccurdy May 15, 2025 05:00
@mrdoob mrdoob added this to the r177 milestone May 15, 2025
@aaronfranke aaronfranke changed the title GLTFLoader/GLTFExporter: Add support for single root files GLTFLoader: Add support for single root files May 17, 2025
@aaronfranke aaronfranke changed the title GLTFLoader: Add support for single root files GLTFLoader: Add support for importing single root files May 17, 2025
Mugen87
Mugen87 previously approved these changes May 18, 2025
Copy link
Collaborator

@Mugen87 Mugen87 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@donmccurdy At least implementation-wise the PR looks good now. Not sure it makes sense to move the code to a separate extension class since the changes are so minor.

@takahirox
Copy link
Collaborator

Hmm, personally, I’m hesitant to support vendor extensions directly in the core glTF loader. Even if support for this extension only takes a few lines of code, accepting every vendor-specific extension just because “it’s only a few lines” quickly becomes unsustainable and unmaintainable.

In other words, if this vendor extension is to be included in the core glTF loader, I’d want to see a strong motivation for it — such as the extension being widely used and recognized in the broader ecosystem.

If it’s not widely adopted, then I think we should treat it the same way as other vendor extensions: start by exploring whether it can be handled through the plugin system. If that’s not currently possible, then we should first focus on improving the plugin system itself.

Also, what happens when you try to load a glTF file that uses this extension without applying this change? Does it fail to load entirely, or just produce incorrect results?

@Mugen87
Copy link
Collaborator

Mugen87 commented May 19, 2025

start by exploring whether it can be handled through the plugin system.

Good point! Without the additional validation in loadScene() it's only a single place where code needs to be changed. We should indeed try to move the extensions specific code into a separate class.

@Mugen87
Copy link
Collaborator

Mugen87 commented May 19, 2025

@aaronfranke Do you mind sharing a glTF asset using the GODOT_single_root extension in this thread?

@aaronfranke
Copy link
Contributor Author

aaronfranke commented May 19, 2025

@takahirox What about the feature itself? Is it valuable to be able to use glTF files to represent a "single object", and get that object as the returned Object3D directly?

Worth noting that this is already the implicit behavior of UnityGLTF, it skips generating an extra root node if the glTF file contains only one root node. This extension makes that behavior explicit.

Would this be easier to accept if it was simply named differently (KHR or EXT prefix)? I'm of the opinion that the name shouldn't matter if the feature is good, and that I am concerned with the idea of one organization (Khronos) gatekeeping what is allowed to exist in the ecosystem.

If it’s not widely adopted, then I think we should treat it the same way as other vendor extensions: start by exploring whether it can be handled through the plugin system. If that’s not currently possible, then we should first focus on improving the plugin system itself.

Ok, but how? Most glTF extensions are about changing the types or properties of returned objects. This extension very unique because it changes which object gets returned as the root.

Also, what happens when you try to load a glTF file that uses this extension without applying this change? Does it fail to load entirely, or just produce incorrect results?

All that will happen is it will have an extra Group root node generated, which needs to be handled by the user. The user could just ignore it and keep the extra Group object in the tree, or if they want the root node from the glTF, they would need to remove the Group via a heuristic decision, like checking for a Group with one child. But that has the problem of different behavior, where adding an additional root node actually changes the returned node and adds two generated nodes. The extension aims to make this explicit to avoid such surprise behavior.

Do you mind sharing a glTF asset using the GODOT_single_root extension in this thread?

Sure! boom_box.zip

With this PR, the imported .scene has the { name: "BoomBox", type: "Object3D" } properties. Without this PR, or if removing the flag from the file, the imported .scene has the { name: "", type: "Group" } properties.

Note that in this glTF file, the root node has the "OMI_physics_body" extension, defining the motion properties if the implementation chooses to treat it as a physics object. Three.js doesn't import that data yet (nor should it, at least not officially, since the physics extensions are still WIP, and Khronos has one under their own name which will very likely be the one used by the ecosystem).

The major use case of promoting the glTF root node to the returned root node is that for physics objects, their transform is meant to represent the transform of the entire object, since all child nodes follow that parent. Setting the "position" of the Group is not really a meaningful operation, but setting the position of the physics object is meaningful. Therefore, the Group is an obstacle in the way of the desired operation. If you kept the Group in the tree, code that sets positions of objects would have to deal with this as a special case.

@donmccurdy
Copy link
Collaborator

donmccurdy commented May 20, 2025

In the abstract – I'm OK with three.js supporting vendor-prefixed extensions like GODOT_*. I think of implementation as a "vote" by three.js for that extension eventually being further standardized as an EXT_ or KHR_ extension. So I agree that main question is whether the feature itself is a net positive, and we feel comfortable maintaining it. On that question, I'm conflicted:

Pros: On one hand a "flatter" result would indeed be nice; most three.js users are conceptually loading a model into an existing scene, and probably don't need a representation of a scene in the file. It's a pretty simple change – even simpler if we remove the nodeIds.length !== 1 validation warning, which I'd prefer.

Cons: trying to flatten the scene graph conditionally is how we got #29768 and several related issues. I'm hesitant to create more conditions we'd need to explain to users about how loader output will be structured.

@takahirox
Copy link
Collaborator

Lately, I've been thinking that the core parts of GLTFLoader and GLTFExporter are better off being as compact as possible, doing only the minimal necessary work. In my view, GLTFLoader should simply convert glTF data into Three.js structures as directly and accurately as possible, and GLTFExporter should do the reverse. This would simplify both tools and improve their maintainability.

As for optimization, that can be done after running GLTFLoader or GLTFExporter. If you want reusable, Three.js-friendly post-processing, you can use the plugin system. By keeping the conversion between glTF and Three.js data as straightforward and lossless as possible, we can avoid issues like #29768

Whether scene flattening is useful really depends on the use case. GLTFLoader could output with a Group node in between, which might seem redundant in some cases, but for use cases where flattening is beneficial, that could be handled as a post-process step. I think this approach would allow us to cover a wider range of use cases. In fact, I've been considering writing such a plugin for scene hierarchy optimization to do just that — though I haven’t had the time to get to it yet...

That's why I’m also cautious about adopting this extension as part of the core. If this were to be handled via a plugin, I wonder if the afterRoot hook could be used?

Regarding vendor extension support in the core GLTFLoader, I understand Don’s perspective. But at the very least, I’d like to see some prospect of the extension being promoted to an EXT_ or KHR_ standard. For example, if it’s being discussed within the glTF working group.

I think the overhead of debating every single vendor extension is non-trivial, and having some sign that an extension might eventually become standardized would help reduce that cost. At least, that’s how I feel personally.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 3, 2025

If this were to be handled via a plugin, I wonder if the afterRoot hook could be used?

Interesting! I wasn't aware of afterRoot(). Probably because no existing extension in GLTFLoader is using it so far.

Maybe support of GODOT_SINGLE_ROOT could be accepted if the extension is implemented as a plugin utilizing afterRoot()? It would post-process the loaded scene graph by removing the intermediate instance of THREE.Group.

@aaronfranke Are you interesting in updating the PR according to this suggestion? This is probably the best chance to get GODOT_SINGLE_ROOT supported in three.js.

@takahirox
Copy link
Collaborator

If this can be achieved using afterRoot(), to be honest, I think it would be better for this kind of extension to be handled by an external plugin.

I think following a strategy like the one below would allow us to cover a wider range of use cases: the core logic of the Three.js glTF loader should avoid flattening the hierarchy and generate Three.js objects without losing any information as much as possible. If users want optimized objects, they can apply their own optimizations using external plugins.

In my experience, whether to prioritize preserving glTF structure or optimizing the scene really depends on the use case. That’s why I think the Three.js glTF loader should aim to support both use cases.

Restoring lost information from an already optimized structure is either impossible or very difficult. On the other hand, starting with unoptimized data—even if somewhat redundant—and optimizing it later would be much easier. That’s why I think the Three.js glTF loader should focus on faithfully reproducing the glTF data without aggressive optimization, and let external plugins handle any needed optimizations.

@aaronfranke
Copy link
Contributor Author

aaronfranke commented Jul 4, 2025

@takahirox With this extension in place, this is the most faithful way to interpret the glTF data, because it respects the rules of the extension. The existence of the extension effectively instructs importers on how to import it. I think you are referring specifically to faithfully reproducing just the data in the base glTF spec.

@takahirox
Copy link
Collaborator

takahirox commented Jul 4, 2025

Hmm, if we go down that road, we’d end up having to account for every vendor extension with similar intent in the future.

This particular extension likely won’t be used often as extensionsRequired, and I think ignoring it wouldn’t cause issues in most common use cases. To be honest, I haven’t yet seen a strong enough reason to justify handling a relatively uncommon extension—meant for a specific workflow—as a built-in feature.

How about first implementing and publishing it as an external plugin, and then considering integrating it as a built-in feature only if a compelling reason arises—like high adoption by users?

Some might argue that since the changes are minimal, it would be fine to handle this as a built-in feature. However, once we start doing that, we could end up with requests to support other vendor extensions as well, which could quickly get out of hand and negatively impact maintainability.

Even if the spec itself is simple, once something is built into the glTF loader, it introduces ongoing maintenance costs—such as ensuring it doesn’t cause issues with future changes to the loader.

So personally, I think that if we’re going to support a vendor extension as a built-in feature, there needs to be a solid reason and strong motivation for doing so.

@Mugen87
Copy link
Collaborator

Mugen87 commented Jul 4, 2025

like high adoption by users?

How would you measure a high adoption rate though? This is somewhat a subjective call. That aside, is something like EXT_MESH_GPU_INSTANCING considered as a commonly used extension? One could argue in a similar fashion against EXT_MESH_GPU_INSTANCING which is supported in GLTFLoader.

A more distinct policy would be easier to understand and follow. Like only supporting KHR_ and EXT_ extensions like you have suggested in #31112 (comment). That sounds good to me.

How about stating if GODOT_SINGLE_ROOT becomes to EXT_SINGLE_ROOT, we support it in GLTFLoader as a built-in plugin. Until that, a plugin for GODOT_SINGLE_ROOT should be part of a third-party repository.

@takahirox
Copy link
Collaborator

takahirox commented Jul 10, 2025

Yeah, I think using promotion to an EXT_ or KHR_ extension as the baseline for starting a discussion on whether to support this extension as built-in makes sense in this case.

Personally, I don’t think we need a rigid set of rules to decide whether an extension should be supported as built-in. I believe it’s fine to evaluate each case individually, on its own merits. That said, having some general guidelines helps smooth out discussions. For me, the following criteria often serve as a reference:

  • How universally or standard the extension is used (the presence of an EXT_ or KHR_ prefix is a meaningful signal).
  • How broadly beneficial the extension is across different use cases.
  • Whether the required logic for the extension can be maintained in a clean and sustainable way.
  • How well the extension’s logic can integrate with Three.js and the GLTFLoader architecture.
  • How well the extension aligns with the overall design and development goals of the GLTFLoader.

Just to reiterate, I think that once we start supporting vendor extensions, there’s no end to it—so in the case of vendor extensions, I’d want to see stronger justification in terms of how standard they are and the value they provide.

Also, I think it would be helpful to create a GitHub issue specifically for discussing whether a new extension should be added to the GLTFLoader/Exporter as built-in support. When proposing new built-in support, the first step could be to have the discussion in that issue before starting on implementation. That way, we can reduce the risk of spending time on an implementation that ends up getting rejected.

@Mugen87 Mugen87 closed this Jul 10, 2025
@Mugen87 Mugen87 added this to the r179 milestone Jul 10, 2025
@takahirox
Copy link
Collaborator

If this extension can’t be handled well with the current Plugin API, I’d really appreciate any feedback. It would help us evaluate potential improvements to the API.

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.

5 participants