Materials returned by buildGraph are not reliable

[AlaricBaraou]

While using the GltfLoader from three.js through drei useGltf hook, I noticed that the render varied from time to time after reloading.
After investigating I found out that the order of objects returned by GltfLoader in the imported scene may vary from one load to the other.

Since buildGraph will group material by name, it'll use the first material with a specific name as the "reference" for other materials using the same name. Since the order may vary on load, the material returned for a particular name might vary too.
That's also an expectation that might need to be discussed, the Gltf specs specify that a material name isn't necessarily unique.

https://registry.khronos.org/glTF/specs/2.0/glTF-2.0.html

Any top-level glTF object MAY 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.

And three.js also doesn't expect a material name to be unique
https://threejs.org/docs/#api/en/materials/Material.name

Optional name of the object (doesn't need to be unique). Default is an empty string.

Therefore, I believe buildGraph should group by uuid, since three.js already reuse material in the GltfLoader
https://github.com/utsuboco/three.js/blob/dev/examples/jsm/loaders/GLTFLoader.js#L3145

Materials could still return an object containing the names of the materials but with a constant unique identifier to it to differentiate them. Maybe something like the cacheKey that three.js use in GltfLoader
https://github.com/utsuboco/three.js/blob/dev/examples/jsm/loaders/GLTFLoader.js#L3138

Doing so, there wouldn't be any "randomness" in which material gets to be returned by buildGraph under the common name.
But this would be a breaking change and should probably be targeted for v9.

That said, here is a simple example.

1. Loading a GLTf containing two mesh with these two materials
2. Mesh1 has a material with the name 'cloth'
   Mesh2 has a similar material also using the name 'cloth' but with a vertexColors property set to true
3. After loading, buildGraph traverse the scene, since the order isn't guaranteed, Mesh1 or Mesh2 can come up first.
4. The first will assign see its material assigned to materials.cloth and the next one will be ignored
5. Tools like gltfjsx will then assign the same cloth material to both mesh while they don't share it initially.

I hope I could make the issue clear enough, otherwise, let me know if something isn't clear.

If/once everyone agrees with this and on the best way to solve this, I'd be happy to make a PR myself. 👍

[drcmda]

@AlaricBaraou yes, gltfloader is async, the order is not guaranteed. it wasn't always like that and previously useGraph/gltfjsx was order based, but threejs had a breaking change.

if meshes have the same name, that's too bad. :-/ uuid, ... i don't know how that would work since the uuid is runtime generated, no? so how could gltfjsx create an object index when the uuid's will be different on load?

what could work is that we use unique names by adding "-[index]" for duplicates. would be relatively easy, too. if you want, go ahead with a pr.

[AlaricBaraou]

Thanks for sharing more info on this.
I'm not sure why I suggested uuid to fix this, I remember I mostly felt like material should be grouped the same way three GLTFLoader group them.
Like here if I remember correctly
https://github.com/mrdoob/three.js/blob/master/examples/jsm/loaders/GLTFLoader.js#L3199

I'll get back to this one in a few days, I'm a bit busy with the Sougen sale coming

[CodyJasonBennett]

Can you share that link but pointed to https://github.com/mrdoob/three.js? The Utsubo fork is private.

[AlaricBaraou]

Updated it, forgot I was on the Utsubo fork my bad!

[DennisSmolek]

if meshes have the same name, that's too bad. :-/ uuid, ... i don't know how that would work since the uuid is runtime generated, no? so how could gltfjsx create an object index when the uuid's will be different on load?

Just to clarify, he wasn't talking about meshes having the same name but materials.

A GLTF of a chess board for example would have pieces that may both have the material named "Body" but be totally opposite colors.

Here is a link to relative material loading in the core three loader:
https://github.com/mrdoob/three.js/blob/master/examples/jsm/loaders/GLTFLoader.js#L3374

It shows there is a cacheKey for the material and there is also a UUID...

I'm not familiar enough with the buildGraph to point to relevant code yet though..

[CodyJasonBennett]

Is this solvable without a modification in GLTFLoader and/or glTF? KhronosGroup/glTF#2337

[CodyJasonBennett]

See mrdoob/three.js#27052.

[donmccurdy]

With materials specifically, there's another problem. Even if your glTF model has only ONE material, three.js might need to clone that material to configure it for use by incompatible meshes. Vertex colors would be one example, but there are others:

https://github.com/mrdoob/three.js/blob/273eb1a4e0b506998438698dbb4c38ff6cd39b3f/examples/jsm/loaders/GLTFLoader.js#L3323-L3339

I don't think this is solvable by KhronosGroup/glTF#2337. At best that would ensure that every name occurring in the glTF file is unique. But the issue above remains - glTF objects cannot translate 1:1 into three.js objects, and we must clone materials and meshes at runtime in order to construct a valid three.js scene graph.

Ultimately runtime-generated IDs cannot be stable across all three.js releases, it's an unworkable goal for GLTFLoader to meet. We do intend for names to be deterministic and stable within any given release – if that's not the case, a bug could be filed.

I realize stable names are really necessary for JSX, though, and I think sanitizing the glTF file — at the time the JSX is generated — is the safer approach. See:

• Prepass to de-duplicate object names at JSX generation time gltfjsx#231

[krispya]

R3F will follow Three's lead here. Thanks for the discussion everyone. The current functionality will maintain in v9.