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

Proposal: nodes + material lookup tables for GLTFLoader #18530

Closed
drcmda opened this issue Feb 1, 2020 · 15 comments
Closed

Proposal: nodes + material lookup tables for GLTFLoader #18530

drcmda opened this issue Feb 1, 2020 · 15 comments
Assignees

Comments

@drcmda
Copy link
Contributor

drcmda commented Feb 1, 2020

Description of the problem

Currently when we want to change things inside a GLTF, for instance making meshes drop shadows, material props, etc, we have to do messy deep traversals.

GLTF loader already returns scenes, cameras and assets, but looking though the source it still knows about nodes and materials internally. If this could be exposed it would be very efficient for us to make changes:

new GLTFLoader().load(url, gltf => {
  gltf.materials["base"].roughness = 0.5
  gltf.nodes["plane"].castShadow = true
})

i have made a tool for gltf-jsx exports that does something similar, and i've started implementing the above variant in a test, it looks like this now:

Screenshot 2020-02-01 at 18 35 42

would it be possible to add these two collections officially to the gltf result?

@donmccurdy
Copy link
Collaborator

donmccurdy commented Feb 4, 2020

@drcmda you can access all of the materials and nodes listed by the glTF asset as follows:

new GLTFLoader().load(url, gltf => {
  const materials = await gltf.parser.getDependencies( 'material' );
  const nodes = await gltf.parser.getDependencies( 'node' );
})

... however, note that if a glTF file contains 1 material and reuses it for 3 meshes, that does not necessarily mean the loaded scene has only one THREE.Material instance. For a variety of reasons, GLTFLoader may need to assign copies of that material to meshes that use it, and modifying the source material returned by getDependency or getDependencies will not affect those copies. We're efficient about not making more copies than necessary, but it's still a consideration.

I think you'll find that .traverse may be as clean an option as anything else, but open to ideas here.

@drcmda
Copy link
Contributor Author

drcmda commented Feb 4, 2020

a look up table just would expose the actual objects that have been created, it wouldn't change if they are being reused or how they're rendered. it just gives users the ability to access a named material or node directly.

new GLTFLoader().load(url, gltf => {
  // gltf.scene, scenes, assets, animations, cameras <- these are exposed by default
  gltf.materials.waterMaterial.opactity = 0.2
  gltf.nodes.rock.visible = false

i published my version yesterday https://twitter.com/0xca0a/status/1224335000755146753 if you enlarge the video you can see the comfort of being able to quickly manipulate stuff without having to search for it. it translates 1:1 to vanilla threejs as well. but i'm fine with exposing it myself.

@donmccurdy
Copy link
Collaborator

What do you do if there are many materials with the same name, though? A lookup table would be handy if names were unique, I agree, but often they aren't, and we tend to get complaints if we change user-provided names without a very good reason.

The node lookup has an alternative available:

var rock = gltf.scene.getObjectByName('rock');

I think these are useful patterns you're describing, I'm just not sure we can provide a one-size-fits-all solution that fits well within the loader for a specific file format.

@drcmda
Copy link
Contributor Author

drcmda commented Feb 5, 2020

From what i see it doesn't seem to be allowed, Blender for instance will automatically adapt to "name.001" if "name" is already present. The GLTF spec says it's indexed, so it might still be possible in theory, maybe Maya or something else allows this, who knows. But that's alright, it's no trouble at all creating these maps in userland.

@drcmda drcmda closed this as completed Feb 5, 2020
@donmccurdy
Copy link
Collaborator

Blender doesn't allow duplicate names. glTF and threejs do, although it's typical (and best practice) for users to choose unique names in both. The much more common constraints are:

  1. Nodes and materials may be unnamed.
  2. A single material in the glTF file may be split into multiple threejs materials (see Proposal: nodes + material lookup tables for GLTFLoader #18530 (comment)) because it's reused by meshes that have skinning, vertex colors, and so on. In this case you'll end up with materials sharing the same name, and it's relatively common.

@drcmda
Copy link
Contributor Author

drcmda commented Feb 5, 2020

interesting, i guess i need to come up with something to prevent mixups. the problem seems to be that gltfloader is async, which renders indices useless. although the actual gltf data descriptors are fixed, if i find a way to know if an objects belongs to a certain node that would be a good fix.

@donmccurdy
Copy link
Collaborator

Give a node index, var object = await gltf.parser.getDependency( 'node', index ); will give you the threejs object associated with that index. Still async, so I don't know if that fits your needs. Or the getDependencies result should also be ordered to match the indices. There will be a few extra objects in the hierarchy not associated with any glTF node: a glTF mesh can contain multiple primitives, threejs lights may contain a target object. That can probably be ignored for these purposes though.

@drcmda
Copy link
Contributor Author

drcmda commented Feb 6, 2020

thanks! will try that out.

@drcmda
Copy link
Contributor Author

drcmda commented Feb 9, 2020

@donmccurdy i think i bumped into a GLTFLoader bug trying to fix unnamed geometries.

gltfloader will set default names if the node does not have a name:

https://github.com/mrdoob/three.js/blob/dev/examples/js/loaders/GLTFLoader.js#L2597-L2599

mesh.name = meshDef.name || ( 'mesh_' + meshIndex );
if ( geometries.length > 1 ) mesh.name += '_' + i;

later that name is accidentally removed, leaving the mesh nameless and the connection to the original node is lost:

https://github.com/mrdoob/three.js/blob/dev/examples/js/loaders/GLTFLoader.js#L2997-L3002

if ( nodeDef.name !== undefined ) {
  node.userData.name = nodeDef.name;
  node.name = THREE.PropertyBinding.sanitizeNodeName( nodeDef.name );
}

nodeDef.name is never undefined, it is a typeof string === '', hence it goes into that bracket always, even if the name is empty, and wipes out the generated name.

Screenshot 2020-02-09 at 11 58 21

here's my initial readout trying to follow what's going on. in loadMesh the names are still there (where it says "loader"), then the meshes loose it again inside loadNode (where it says "node").

@drcmda
Copy link
Contributor Author

drcmda commented Feb 9, 2020

I think it should say:

if (nodeDef.name) { ...

when i make the change, all is working as it should:

before fix:

<mesh material={materials['Scene_-_Root']} geometry={nodes[''].geometry} />
<mesh material={materials['Scene_-_Root']} geometry={nodes[''].geometry} />
<mesh material={materials['Scene_-_Root']} geometry={nodes[''].geometry} />
<mesh material={materials['Scene_-_Root']} geometry={nodes[''].geometry} />

after fix:

<mesh material={materials['Scene_-_Root']} geometry={nodes['mesh_0'].geometry} />
<mesh material={materials['Scene_-_Root']} geometry={nodes['mesh_1'].geometry} />
<mesh material={materials['Scene_-_Root']} geometry={nodes['mesh_2'].geometry} />
<mesh material={materials['Scene_-_Root']} geometry={nodes['mesh_3'].geometry} />

@donmccurdy
Copy link
Collaborator

Agreed, thanks for the PR!

@drcmda
Copy link
Contributor Author

drcmda commented Nov 1, 2020

@donmccurdy @mrdoob now that threejs dedupes names and there are no unnamed objects, could we look at this proposal again? we've been working with this for a long while now, as an extension, and it has proven very useful. i would prefer if this were official. again, having look-up tables removes the need for traversal once and for all.

@mrdoob
Copy link
Owner

mrdoob commented Nov 2, 2020

@donmccurdy Is there anything the engine should change for this? Or is this a GLTFLoader thing?

@drcmda
Copy link
Contributor Author

drcmda commented Nov 2, 2020

i believe at this point there would be no changes. just this somewhere in the gltfloader, once loading has finished:

data.nodes = {}
data.materials = {}
data.scene.traverse((obj) => {
  if (obj.name) data.nodes[obj.name] = obj
  if (obj.material && !data.materials[obj.material.name]) data.materials[obj.material.name] = obj.material
})

and then we can do this:

new GLTFLoader().load(url, ({ nodes, materials }) => {
  materials.leaf.color.set("green")
  nodes.tree.receiveShadow = true
  scene.add(nodes.tree)
})

@donmccurdy
Copy link
Collaborator

... now that threejs dedupes names and there are no unnamed objects, ...

I don't think either of these are true, though. We don't guarantee that things have names, we just guarantee that for any Object3D subclass with a name, that name will be unique. Even if we did start generating names for everything, I'm not comfortable guaranteeing that those generated names will be stable over future changes (e.g. performance improvements, support for lazy loading) in GLTFLoader.

Since the ~7 lines of code above will work just as well outside of GLTFLoader as inside of it, I'm still not sure it's a good idea for GLTFLoader to take responsibility for what is inherently not a very stable API. If the user would like to guarantee stable names for everything in their scene, the user may need to generate those stable names in advance. For example:

import { NodeIO } from '@gltf-transform/core';

const io = new NodeIO();
const doc = io.read('input.glb');

doc.getRoot().listMaterials().forEach((material, index) => {
  if (!material.getName()) material.setName(`Material_${index}`);
});
  
io.write('output.glb', doc);

Even after each glTF material has a unique name, a single glTF material may generate multiple three.js materials, if it's reused by multiple meshes with different shader requirements. But for this use case that seems OK?

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

No branches or pull requests

4 participants