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

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

Open
DennisSmolek opened this issue Sep 15, 2024 · 2 comments

Comments

@DennisSmolek
Copy link
Contributor

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.
3. is easier to implement.

@CodyJasonBennett
Copy link
Member

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
Copy link
Contributor Author

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.

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

No branches or pull requests

2 participants