GLTFLoader: Fixed variants normal map scale #21148
Conversation
|
@donmccurdy @mrdoob Getting a bit close for the release, but it'd be nice to make r125. |
|
FYI @takahirox |
|
|
||
| // https://github.com/mrdoob/three.js/issues/11438#issuecomment-507003995 | ||
| if ( material.normalScale ) material.normalScale.y *= -1; | ||
| if ( material.clearcoatNormalScale ) material.clearcoatNormalScale.y *= -1; |
There was a problem hiding this comment.
These two lines are needed only if vertex tangents are not present — that's the issue in #21136 as well.
There was a problem hiding this comment.
No, because I reversed them by default above, so now they need to be re-reversed if vertexTangents are used, which can work because vertexTangents create a new cacheKey.
There was a problem hiding this comment.
Oops, I see. Seems a little more confusing to have inversions in three places, was there a specific reason you prefer that? If we keep it that way, we should include a link to the github issue above in all three places.
There was a problem hiding this comment.
As I say, it was necessary in order to make the switch happen only inside a cached constructor, instead of every time assignFinalMaterial was called. That was the bug. I'll put in the comments.
There was a problem hiding this comment.
It seems like this will introduce the opposite problem if the mesh did have vertex tangents with variants, no?
There was a problem hiding this comment.
No, I don't think so. Did you find that to be true in testing?
There was a problem hiding this comment.
We have exactly one sample using variants, and it doesn't have vertex tangents, so I'm limited to guessing at this point. I think I see what you mean though.
mrdoob
changed the title
|
@donmccurdy How do you feel about this PR? Do you think it's ready to merge (r125 due tomorrow) or should we leave it for r126? |
There was a problem hiding this comment.
Hm, sorry, I'm not sure what to do here. This fixes the reported issue, but creates a new, identical issue if the model does have vertex tangents, I think?
If it's a choice between the two, it would probably be better to ensure correct behavior for the users who have gone to the trouble of including vertex tangents.
Calling the assignFinalMaterial method from the application would handle either case, so then I guess the question is whether the remaining logic should still be changed?
|
If the model does have vertex tangents then the first time the material is created, the normals will be flipped once, then flipped back again in assignFinalMaterial. If the variant is switched back, it will use the cached version in assignFinalMaterial instead of re-flipping the normals. This should solve it both ways, no? |
|
Thanks! 🤞 |
3 participants
Fixes #21136
This makes
assignFinalMaterial()public (well, it sort of was already, just not in the types file). I couldn't think of a better name, andgetDependency()is already kind of confusing to me, so I feel like the bar isn't too high. 😂I verified this works on both the three.js example and in model-viewer. If anyone has a model with vertex tangents and variants, that would be good to test, but I'm pretty confident this will work. Basically the sign-flip is now the default behavior and flipping it back is inside of a cache check, so it does't happen more than once.