Skip to content

Conversation

ryantrem
Copy link
Member

There were two issues with skipMaterials:

  1. If a mesh in the glTF has a material asigned but skipMaterials is on, no material is loaded (correct behavior). If a mesh in the glTF has no material assigned and skipMaterials is on, the flag is ignored and a default material is still created and assigned.
  2. PBRMaterial or OpenPBRMaterial are dynamically loaded, depending on useOpenPBR flag. However, if skipMaterials is on, then we won't be loading any materials anyway, so we should skip this dynamic import.

After these changes, I could see with some testing that when using skipMaterials (with either a glb with materials and material references and without), PBRMaterial was neither instantiated nor imported (statically or dynamically).

@ryantrem ryantrem requested review from sebavan and bghgary September 25, 2025 22:27
@ryantrem ryantrem added the bug label Sep 25, 2025
@ryantrem ryantrem enabled auto-merge (squash) September 25, 2025 22:29
Copy link
Contributor

@bghgary bghgary left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@bghgary
Copy link
Contributor

bghgary commented Sep 25, 2025

Maybe we can add a unit test for this case? Not sure it'll be worth it.

@bjsplat
Copy link
Collaborator

bjsplat commented Sep 25, 2025

Please make sure to label your PR with "bug", "new feature" or "breaking change" label(s).
To prevent this PR from going to the changelog marked it with the "skip changelog" label.

@ryantrem ryantrem disabled auto-merge September 25, 2025 22:41
@bjsplat
Copy link
Collaborator

bjsplat commented Sep 25, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Sep 25, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Sep 25, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Sep 25, 2025

@bjsplat
Copy link
Collaborator

bjsplat commented Sep 25, 2025

@ryantrem ryantrem marked this pull request as draft September 26, 2025 00:16
@ryantrem
Copy link
Member Author

Not quite ready to merge, some discussion still under way.

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

Successfully merging this pull request may close these issues.

4 participants