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

Normal is not correct when switching Variant in webgl_loader_gltf_variants #21136

Closed
cx20 opened this issue Jan 24, 2021 · 8 comments · Fixed by #21148
Closed

Normal is not correct when switching Variant in webgl_loader_gltf_variants #21136

cx20 opened this issue Jan 24, 2021 · 8 comments · Fixed by #21148

Comments

@cx20
Copy link
Contributor

cx20 commented Jan 24, 2021

Describe the bug

I tried the sample KHR_materials_variants extension of glTF.
However, when I switch the Variant from midnight to beach, Normal does not seem to be correct.

To Reproduce

  1. Go to https://threejs.org/examples/?q=variant#webgl_loader_gltf_variants
  2. Switch the Variant from midnight to beach
  3. Look at the logo on the side of the shoe.

Expected behavior

Since this model refers to the same normal.jpg, the expected behavior is that the unevenness will not change.

Screenshots

Variant image
midnight image
beach image

Platform:

  • Device: MacBook Air
  • OS: Windows 10 (BootCamp)
  • Browser: Chrome 87.0.4280.141
  • Three.js version: r124
@Mugen87
Copy link
Collaborator

Mugen87 commented Jan 25, 2021

For some reasons, the normalScale value of midnight is 1, -1 whereas the other two material variants have a value of 1, 1.

It seems the material adjustments from assignFinalMaterial() are not done for beach and street material.

if ( material.normalScale && ! useVertexTangents ) {
material.normalScale.y = - material.normalScale.y;
}

@mrdoob
Copy link
Owner

mrdoob commented Jan 25, 2021

is this an issue on the khronos model?

/cc @donmccurdy

@cx20
Copy link
Contributor Author

cx20 commented Jan 25, 2021

The original models used here can be found in Khronos glTF-Sample-Models repository.
https://github.com/KhronosGroup/glTF-Sample-Models/tree/master/2.0/MaterialsVariantsShoe

If I remember correctly, I reduced the texture size to save space. but I think the rest of the model structure is the same.

For reference, the normal problem does not seem to occur when displaying this model in Babylon.js.
Babylon.js + MaterialsVariantsShoe.gltf result:

Variant image
midnight image
beach image

@donmccurdy
Copy link
Collaborator

As @Mugen87 points out, the assignFinalMaterial would normally do some adjustments to materials based on the mesh they're attached to, as we need to know whether the geometry has tangents or not. See #11438 (comment). Variants are applied well after the model was loaded, outside of GLTFLoader, and the example wouldn't know about these details.

Ideally this model would contain vertex tangents... Blender can generate them, but won't read/write the variants, so unfortunately that's difficult to add at the moment. With vertex tangents this would not be an issue.

Perhaps #20789 would also help fix this, but I'm still worried about several parts of that.

@mrdoob
Copy link
Owner

mrdoob commented Jan 25, 2021

/cc @elalish

@elalish
Copy link
Contributor

elalish commented Jan 25, 2021

Looks like it might have been a simple oversight. assignFinalMaterial should probably be called right after getDependency when loading a new variant. It seems assignFinalMaterial isn't included in GLTFLoader.d.ts, but I guess it should be?

@donmccurdy
Copy link
Collaborator

That could work I think — might try to think of a more descriptive name before we consider it a public API, but sounds like a good solution.

@elalish
Copy link
Contributor

elalish commented Jan 25, 2021

Well, I gave it a try and no such luck. assignFinalMaterial negates normalScale.y every time, so it ends up switching back and forth as you swap back to already-loaded variants. I think that line needs to move inside the cached ClonedMaterial block. I'll work up a PR.

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

Successfully merging a pull request may close this issue.

5 participants