-
-
Notifications
You must be signed in to change notification settings - Fork 35.9k
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
Conversation
|
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.
@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.
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? |
Good point! Without the additional validation in |
@aaronfranke Do you mind sharing a glTF asset using the |
@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.
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.
All that will happen is it will have an extra
Sure! boom_box.zip With this PR, the imported Note that in this glTF file, the root node has the 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 |
In the abstract – I'm OK with three.js supporting vendor-prefixed extensions like 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 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. |
Lately, I've been thinking that the core parts of As for optimization, that can be done after running Whether scene flattening is useful really depends on the use case. 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 Regarding vendor extension support in the core 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. |
Interesting! I wasn't aware of Maybe support of @aaronfranke Are you interesting in updating the PR according to this suggestion? This is probably the best chance to get |
If this can be achieved using 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. |
@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. |
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 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. |
How would you measure a high adoption rate though? This is somewhat a subjective call. That aside, is something like A more distinct policy would be easier to understand and follow. Like only supporting How about stating if |
Yeah, I think using promotion to an 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:
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. |
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. |
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:
... 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 aGroup
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:
... then the exported glTF JSON data will have the
GODOT_single_root
flag set, becausegenerated.scene
is a single non-Scene
object. This also applies to other objects regardless of if they came from aGODOT_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 toGLTFLoader
, at most you could have code that wraps aroundGLTFLoader
. 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 theGroup
, it returns (in the callback) an object holding a bunch of generated data, which includes the single root node (which isGroup
without the single root flag, or anotherObject3D
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.