-
-
Notifications
You must be signed in to change notification settings - Fork 35.8k
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
base: dev
Are you sure you want to change the base?
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. |
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.