pmndrs / react-three-fiber

🇨🇭 A React renderer for Three.js
https://docs.pmnd.rs/react-three-fiber
MIT License
27.59k stars 1.6k forks source link

useLoader incorrectly merges materials that have the same name (used in useGLTF) #3358

Open DennisSmolek opened 2 months ago

DennisSmolek commented 2 months ago

When working on setting up loading material variants I kept finding that useGLTF was only returning a single material when the gltf has three different ones: image

The reason for this is that useGLTF uses useLoader which uses the buildGraph util. buildGraph has a section specifically for materials where traverses the object and set's the materials based on the material name property.

This means that in the case of phong1SG it hits the first result only and skips the rest.

The issue is material name is often optional. Some software just adds the name 'material' or 'undefined' as the material name. Or in the case of variants, names all the variants the same material name.

I think there are 3 paths:

  1. use the material uuid which will be unique to put the material in the material object. This will keep the code as simple but be less human readable.
  2. Add some logic for if a material with the name already exists it adds a suffix materialName-alt or even fancier an index: materialName-3 This keeps it human readable but maintains uniqueness.
  3. if multiple materials exist with the same name create an array and push each one into it. Simpler than the second option, but slightly less readable as you'd have to go through the array.

My vote is 2 or 3. 2 is better for devs that have a system of simply listing materials from the material object without having to build logic to parse a potential array.

  1. is easier to implement.
CodyJasonBennett commented 2 months ago

Have you seen the discussion in #2498? There's some work we can do upstream still. No. 2 was my kneejerk suggestion since we can do it here with uuid/name to find collisions.

DennisSmolek commented 2 months ago

Have you seen the discussion in #2498? There's some work we can do upstream still. No. 2 was my kneejerk suggestion since we can do it here with uuid/name to find collisions.

I forgot I was even a part of that thread lol.

I didn't even consider what happens when meshes in an object have the same material name but are different.

I think the issue has to get resolved and updated on the GLTF and then Threejs side before a permanent solution goes into place but I think we should also do something in the meantime.

Right now what happens if someone uses a material named phong1SG it would simply ignore the second and third versions of this.

So adding phong1SG-2 wouldn't hurt anything backwards compatibility wise.

Part of me wonders Isn't the "correct" way to handle this (especially in gltfJSX) just to respect the original materials array? Load materials[0] instead of materials['phong1SG']? In the GLTF Json itself that's how it identifies the material.