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

GLTFLoader: record association between glTF elements and Three.js objects #19257

Closed
3 of 13 tasks
cdata opened this issue Apr 29, 2020 · 27 comments · Fixed by #19359
Closed
3 of 13 tasks

GLTFLoader: record association between glTF elements and Three.js objects #19257

cdata opened this issue Apr 29, 2020 · 27 comments · Fixed by #19359

Comments

@cdata
Copy link
Contributor

cdata commented Apr 29, 2020

Note: I don't want to bury the lede for this request, so I moved a brief summary to the top of the issue. Please refer to the second and third headings for a more detailed background and rationale!

Proposal

When loading models with GLTFLoader, it would be nice if we could retain associations between glTF scene graph elements and the Three.js objects that correspond to them. I briefly discussed this problem with @donmccurdy and he suggested that one of the following approaches might be acceptable:

  1. Decorate Three.js objects via userData e.g., material.userData.gltfMeta.index = 3
  2. Expose a lookup table somewhere e.g., parser.getDefinition( material: THREE.Material ): GLTF.Material

What do others think about this problem? Would you be willing to consider a change that makes either of the suggested enhancements?

Background

Three.js' GLTFLoader currently produces an artifact that contains a Three.js scene graph as well as a GLTFParser instance that holds the original, deserialized glTF.

The GLTFParser holds associations between the elements in the original glTF and the Three.js scene graph. These associations are expressed both by GLTFParser.prototype.getDependencies and the cache object on the GLTFParser instance.

In some cases, the relationship between the original glTF element and the associated Three.js objects is obscured and cannot be trivially re-established. For example, materials associated with a skinned mesh are cloned, and the resulting clones are cached against a key that cannot be reconstructed easily:

cachedMaterial = material.clone();
if ( useSkinning ) cachedMaterial.skinning = true;
if ( useVertexTangents ) cachedMaterial.vertexTangents = true;
if ( useVertexColors ) cachedMaterial.vertexColors = true;
if ( useFlatShading ) cachedMaterial.flatShading = true;
if ( useMorphTargets ) cachedMaterial.morphTargets = true;
if ( useMorphNormals ) cachedMaterial.morphNormals = true;
this.cache.add( cacheKey, cachedMaterial );

Description of the problem

Consider a model editor that allows a user to:

  1. Load a glTF with any shape
  2. Interactively change the value of any field of any element of the glTF
  3. See the result of the change rendered in real time
  4. Export a new glTF

In an ideal world, the editor should faithfully re-export the model with its original hierarchy and shape. The only meaningful change in the exported file should be the one made interactively by the user.

In the pursuit of this goal, it is very useful (and probably required) that an association between the source glTF scene graph and the Three.js scene graph be made, so that for any change we make to a glTF element, we can reflect that change in the rendered Three.js scene in real-time without completely re-loading the glTF.

Some of the information required to correctly correlate the glTF scene graph with the Three.js scene graph is lost or obscured, and this makes the above described editor scenario very difficult (perhaps impossible) to achieve.

Three.js version
  • Dev
  • r115
  • ...
Browser
  • All of them
  • Chrome
  • Firefox
  • Internet Explorer
OS
  • All of them
  • Windows
  • macOS
  • Linux
  • Android
  • iOS
Hardware Requirements (graphics card, VR Device, ...)
@mrdoob mrdoob added this to the rXXX milestone Apr 29, 2020
@cdata
Copy link
Contributor Author

cdata commented May 1, 2020

If this request is broadly acceptable, I would like to suggest that approach (1) suggested by @donmccurdy seems preferable because it will be easier to retain associations when making clones of the scene graph.

@mrdoob
Copy link
Owner

mrdoob commented May 2, 2020

@cdata approach (1) sounds good to me 👌

@donmccurdy
Copy link
Collaborator

@drcmda and @fms-cat: I think you've both asked about variations of this in the past. Would either of these approaches be helpful for you?

@cdata
Copy link
Contributor Author

cdata commented May 2, 2020 via email

@drcmda
Copy link
Contributor

drcmda commented May 2, 2020

i would most definitively prefer a lookup table. react-three-fibers useLoader adds "nodes" and "materials" to the gltf data. with these two the entire graph can be represented.

this enables full automation, for instance this is how a file from sketchfab looks like: https://codesandbox.io/s/romantic-tharp-0buje?file=/src/Model.js

in plain threejs it could look like this:

new GLTFLoader().load(url, ({ nodes, materials }) => {
  nodes.cube.receiveShadow = true
  materials.metal.roughness = 0.5
  ...
})

users now could go ahead and build their own graphs:

const mesh = new THREE.Mesh(nodes.cube.geometry, materials.metal)
scene.add(mesh)

as for userData, i think object mutation isn't the best choice imo. i don't like mutation in general, and it still needs traversal in the end, which is the very thing that makes handling gltf so hard in threejs. if you add a lookup table with gltf indices that would be nice. official support for nodes/materials would be welcome, too - then i can remove it from r3f.

if you go for a index table alone, pretty please dont name it "nodes" 👀

@0b5vr
Copy link
Collaborator

0b5vr commented May 2, 2020

My original intention of the idea 1 was to load the custom material from the glTF and that still should be achieved using plugins.
This one sounds like a totally different purpose and also a valid idea 👍

End developer have to delete the userData field when they try to clone a gltf derived materials in this case, and that sounds weird a bit 🤔

@donmccurdy
Copy link
Collaborator

donmccurdy commented May 4, 2020

One hitch with approach (1) is that some of the objects created by Three.js do not have a userData property. For example, AnimationClip and BufferAttribute.

I'm not inclined to include every possible object type, until we have concrete reasons to do so. If the solution happens to handle everything easily, neat, but it can probably wait.

new GLTFLoader().load(url, ({ nodes, materials }) => {
nodes.cube.receiveShadow = true
 materials.metal.roughness = 0.5
...
})

...as for userData, i think object mutation isn't the best choice imo

I'd be OK with exposing some kind of lookup table, either on GLTFLoader's result object or on the GLTFParser it already returns. It probably should not rely on unique names, which are not guaranteed, but indices or a Map would work.

if you go for a index table alone, pretty please dont name it "nodes"

I don't understand — the name conflicts with something you need?

@drcmda
Copy link
Contributor

drcmda commented May 4, 2020

It probably should not rely on unique names, which are not guaranteed, but indices or a Map would work.

the names though is what makes it usable - and typesafe - , otherwise to find out where "robotLeg" is you traverse again, as opposed to:

const { nodes: { robotLeg } } = await new GLTFLoader<Robot>().loadAsync(url)
robotLeg.receiveShadows = true

someone could hack into a gltf with a text editor and use one name multiple times, it isn't possible in modeling software like blender and unnamed objects are named by three with an additive index. all it would do is overwrite a named slot, no harm done. but we're forcing all users to traverse in order to find back their assets because of it.

I don't understand — the name conflicts with something you need?

if you do decide to add something that's working differently than a name:value collection, please consider naming it something else than "nodes" and "materials" if it's in any way possible. i know that's a lot to ask for, but it would throw us into major breaking changes.

@cdata
Copy link
Contributor Author

cdata commented May 4, 2020

@drcmda @donmccurdy I have taken to referring to the glTF descriptors as glTF "elements." Not sure if that helps the discussion, but worth considering in case you want another word to refer to those things collectively 💁

It sounds like a look-up table is the preferred approach. I took a quick stab at this last week. Perhaps I could put up a PR for considering?

I'm not inclined to include every possible object type, until we have concrete reasons to do so.

Is there a reason we should keep in mind for why you might be disinclined to include some object types? Subtle complexity or anticipated changes in the implementation, perhaps?

@donmccurdy
Copy link
Collaborator

someone could hack into a gltf with a text editor and use one name multiple times, it isn't possible in modeling software like blender and unnamed objects are named by three with an additive index.

We do see objects with duplicate names periodically; there have been multiple bug reports against three.js as a result. That's solvable: I think GLTFLoader should automatically rename duplicates. But more importantly, mapping from a unique name (possibly generated by GLTFLoader) to a three.js object does not provide the mapping back to the glTF definition that @cdata presumably needs. You would need to also return an annotated version of the original glTF JSON, with new names. 😕

Is there a reason we should keep in mind for why you might be disinclined to include some object types?

Mostly I'm nervous of modifying three.js' API just to match glTF, if that change doesn't also bring some real benefit to the API itself. And, there are various objects (samplers, textures vs images) that exist in glTF but don't obviously map to a three.js object in the final scene. Mapping nodes, meshes, and materials is a much more tractable problem, but still has plenty of edge cases.

@cdata
Copy link
Contributor Author

cdata commented May 4, 2020

I'm definitely in agreement about not using names to map these things. The glTF spec is really explicit about not being able to rely on name uniqueness:

Any top-level glTF object can have a name string property for this purpose. These property values are not guaranteed to be unique as they are intended to contain values created when the asset was authored.

Source: https://github.com/KhronosGroup/glTF/tree/master/specification/2.0#indices-and-names

Mostly I'm nervous of modifying three.js' API just to match glTF, if that change doesn't also bring some real benefit to the API itself.

FWIW all that matters to me at the end of the day is that it is possible to cross-associate between the two hierarchies. The relative stability of this association over time is not nearly as important. That said, I appreciate that any nature of association will be considered an implicit contract in the the API.

To look at this somewhat differently, if it were possible for a GLTFLoader user to participate in the parsing process, this might give Three.js more conceptual flexibility and still enable someone like me to satisfy the original stated goals. The GLTFParser already has a fairly well-factored implementation that separates loading of each distinct part of the glTF:

/**
* Requests the specified dependency asynchronously, with caching.
* @param {string} type
* @param {number} index
* @return {Promise<Object3D|Material|THREE.Texture|AnimationClip|ArrayBuffer|Object>}
*/
GLTFParser.prototype.getDependency = function ( type, index ) {
var cacheKey = type + ':' + index;
var dependency = this.cache.get( cacheKey );
if ( ! dependency ) {
switch ( type ) {
case 'scene':
dependency = this.loadScene( index );
break;
case 'node':
dependency = this.loadNode( index );
break;
case 'mesh':
dependency = this.loadMesh( index );
break;
case 'accessor':
dependency = this.loadAccessor( index );
break;
case 'bufferView':
dependency = this.loadBufferView( index );
break;
case 'buffer':
dependency = this.loadBuffer( index );
break;
case 'material':
dependency = this.loadMaterial( index );
break;
case 'texture':
dependency = this.loadTexture( index );
break;
case 'skin':
dependency = this.loadSkin( index );
break;
case 'animation':
dependency = this.loadAnimation( index );
break;
case 'camera':
dependency = this.loadCamera( index );
break;
case 'light':
dependency = this.extensions[ EXTENSIONS.KHR_LIGHTS_PUNCTUAL ].loadLight( index );
break;
default:
throw new Error( 'Unknown type: ' + type );
}
this.cache.add( cacheKey, dependency );
}
return dependency;
};

It might be sufficient to support a callback that is invoked at the tail-end of loading each dependency. For example:

GLTFParser.prototype.getDependency = function ( type, index ) {
  // ...

  if ( ! dependency ) {
    switch ( type ) {
      case 'scene':
        dependency = this.loadScene( index );
        break;
      // ...
    }
    
    if ( this.dependencyCallback ) {
      this.dependencyCallback( type, index, dependency );
    }
  }

  // ...
};

The nice thing about this approach is that it is simple to implement and the structure of glTF is the primary driver of the API (rather than the ways in which the glTF is interpreted in terms of Three.js). This perhaps conceptually lightens the burden on Three.js implementation. WDYT @donmccurdy ?

@cdata
Copy link
Contributor Author

cdata commented May 7, 2020

Although I should note that the use case is orthogonal to the current feature request, it is worth considering that the dependencyCallback approach could help folks when prototyping support for experimental or soon-to-be-ratified glTF extensions. To support this case, it might make sense for the dependencyCallback to be designed as a mapping function.

This isn't to suggest we should support this, but it is an interesting capability that is potentially unlocked with the right design. The idea came up in discussion with some folks from 3D Commerce today while we were speculating about how we might experiment with KHR_materials_variant support in Three.js without forking anything.

@pushmatrix
Copy link
Contributor

Big 👍 for this. I built a glTF compression tool in three.js (lets you optimize the mesh + textures) and it was a pain having to maintain all those references manually.

@donmccurdy
Copy link
Collaborator

That use case is meant to be addressed by #19144. Feedback and testing on the PR would be welcome, although we'd like to keep it fairly simple to start.

I think providing a lookup table would probably be independent of that extension API, but open to suggestions.

@donmccurdy
Copy link
Collaborator

@pushmatrix this is still in progress, but for use cases like optimizing a model you may find https://github.com/donmccurdy/glTF-Transform helpful.

@cdata
Copy link
Contributor Author

cdata commented May 7, 2020

@donmccurdy in the fullness of time, we will need to participate in parsing of the entire scene graph. I want to make sure that if we begin to build some cross-association between the glTF scene graph and the Three.js one, that it is coherent for all parts of the glTF (eventually).

I'm happy to implement that in whatever form makes sense to you, while being sensitive to your feedback in #19257 (comment)

The dependency callback approach seems like a nice middle-ground, as it doesn't explicitly require some specific Three.js output, but does give us enough information to know what part of a glTF a skinnedMesh.material (for example) corresponds to (because we will know the GLTF.Mesh, and thus the GLTF.Material, and we will have already "visited" the originating THREE.Material ahead of it being cloned, so the information is all there).

With regards to the plug-in system, that seems relatively complex and doesn't seem to cover most of the glTF scene graph yet. With your feedback in mind, would you prefer that we focus on giving that PR feedback rather then implement cross-associations in some other way?

@cdata
Copy link
Contributor Author

cdata commented May 7, 2020

cc @takahirox I may be overestimating the utility of having a single dependency callback; maybe you can advise me 🙏

@donmccurdy
Copy link
Collaborator

donmccurdy commented May 9, 2020

A per-dependency callback doesn't provide enough flexibility to implement existing extensions, which the targeted hooks in #19144 would allow. We can certainly add more hooks, especially following the loadFoo pattern there. But complete flexibility to reinterpret any part of the glTF schema is not a goal of GLTFLoader; I think that will always be the domain of custom code. Our goal for now is to enable most known use cases, but not to get bogged down with hypothetical ones.

If the addition of loadNode (or similar) to #19144 would provide what you need, let's use that? If you expect to also need hooks for accessors, samplers, buffer views, and other minutiae of the glTF spec, I'm not sure GLTFLoader can provide this.

@cdata
Copy link
Contributor Author

cdata commented May 9, 2020 via email

@donmccurdy
Copy link
Collaborator

Typically the term "scene graph" refers only to objects with positions in the scene. Materials, accessors, animations, samplers, etc. are not typically considered part of the scene graph.

Mapping the relationship between the three.js scene graph and the glTF scene graph, we certainly can do, and perhaps a bit more (e.g. materials). I'm not sure if that's what you're asking here, or if you need a mapping for everything that comes out of the glTF file?

@cdata
Copy link
Contributor Author

cdata commented May 9, 2020 via email

@donmccurdy
Copy link
Collaborator

Sorry for the wording nit; I'm trying to understand the scope of what you're asking for, and in that sense the scene graph and the glTF dependency graph are very different.

I do not think we should provide mappings for all top-level constructs in the glTF spec from GLTFLoader. Mappings for nodes and materials would be fine, sure. But your longer-term goal here is likely to require a custom loader.

@cdata
Copy link
Contributor Author

cdata commented May 11, 2020

I do not think we should provide mappings for all top-level constructs in the glTF spec from GLTFLoader.

You already provide this mapping via the GLTFParser's cache. The main problem is that it loses information in a handful of cases. My request is that those cases be fixed so that the relevant information is retained.

If we can fix the ambiguity with materials to start, that should be fine.

But your longer-term goal here is likely to require a custom loader.

I don't want to put words in anyone's mouth, so if you can please clarify for me:

The implication of this seems to be that the example use case in the original issue (the ability to import a glTF, make discrete edits, and export that glTF with only those discrete edits applied) is only to be supported for nodes and materials. Is that a fair expansion of this comment?

@donmccurdy
Copy link
Collaborator

donmccurdy commented May 11, 2020

GLTFParser's internal cache was never intended as a public API. It may change over time, and I would not expect those changes to appear in the changelog. The goal here is to arrive at a public API that is not at risk of changing as a result of internal refactoring.

I'm not sure how to answer a question about supporting discrete edits, the full implications of that question are unclear to me. But as for mappings/associations: I would strongly prefer that GLTFLoader not be burdened with maintaining mappings to low-level (but still top-level, as far as schema is concerned) glTF elements like buffers, buffer views, accessors, and samplers. The other elements are open for discussion, but nodes and materials seem like the place to start.

@cdata
Copy link
Contributor Author

cdata commented May 11, 2020

I have been operating on the assumption that being able to map glTF dependencies to their associated Three.js constructs is a prerequisite to enabling discrete edits to the glTF. This might not be correct, and I would probably benefit from the wisdom of others if that is the case.

It sounds like there is some agreement to disambiguate materials and nodes, and that is a reasonable starting place to me.

I reviewed react-three-fiber's implementation. Although it uses names (which seems perilous), we could probably substitute indexes and get similar complexity when correlating the two trees.

Thank you @donmccurdy for the clarifying discussion. At this point I would like to proceed with a proposal that implements something along the lines of (2) as described in the original issue text, specifically: a GLTFParser-level mapping that accounts for nodes and materials.

@cdata
Copy link
Contributor Author

cdata commented May 14, 2020

I have opened a PR for your consideration: #19359

@mrdoob mrdoob removed this from the rXXX milestone May 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants
@mrdoob @pushmatrix @cdata @donmccurdy @drcmda @0b5vr @Mugen87 and others