Three.js: Normals flipped on some parts of the mesh that have double sided materials

[pushmatrix]

Description of the problem

Upgrading from 107 to 108, I've noticed that parts of my model now look like they have flipped normals. This only occurs with double sided materials.
See here: https://vaulted-jonquil.glitch.me/

This is what it should look like, with the pattern being indented on both wheels. That's how it was in 107

In 108 though one of the wheels has the pattern looking like it's popping out:

Three.js version

• [x] r108

Browser

• [x] All of them

OS

• [x] All of them
• [ ] Windows
• [ ] macOS
• [ ] Linux
• [ ] Android
• [ ] iOS

[elalish]

@pushmatrix Can you confirm if this model does or does not supply its own tangents? I'm curious if this may be related to #11438.

[pushmatrix]

I don't believe it specifies its own tangents

[WestLangley]

Using a different test case, git bisect points to #17586 as the PR that initiated the problem if the mesh is double-sided and has negative scale X. That was in r.109, not r.108, however.

This was not tested on the model from this PR because that model is not available.

[pushmatrix]

Here's the test glb
wheels.glb.zip

[WestLangley]

From 3 years ago: https://github.com/mrdoob/three.js/issues/10331#issue-194807426

Basically it seems that ... gl_FrontFace is not working properly with double sided materials on Adreno GPUs.

Is this _still_ an issue that we need to accommodate?

//

This model appears to render correctly on my machine if we remove the Adreno workaround in normalmap_pars_fragment:

#ifdef DOUBLE_SIDED

    // Workaround for Adreno GPUs gl_FrontFacing bug. See #15850 and #10331

    bool frontFacing = dot( cross( S, T ), N ) > 0.0;

    mapN.xy *= ( float( frontFacing ) * 2.0 - 1.0 );

#else

    mapN.xy *= ( float( gl_FrontFacing ) * 2.0 - 1.0 );

#endif

[elalish]

Hmm, I remember reading that comment too; I think I came across it while looking at this bug, which was apparently fixed in r108 (at least upgrading fixed it for us): https://github.com/GoogleWebComponents/model-viewer/issues/740

Maybe it's related? FWIW, I have gotten some feedback about old Adreno GPUs, so apparently they're still fairly common.

[mrdoob]

Just to confirm, this still happens in dev:

[WestLangley]

Just to confirm, this still happens in dev:

Right. But not if you remove the Adreno bug workaround.

[Mugen87]

But not if you remove the Adreno bug workaround.

How about removing the workaround for now and then search for a different solution? The current implementation leads to wrong visuals no matter what platform you are using.

[WestLangley]

@Mugen87 In this particular case, I believe the geometries are mirrors of each other, but they share the same normal map.

I agree with you. I have invested many hours trying to find an Adreno-compatible workaround that is correct in every use case.

[pushmatrix]

Inspecting the wheel model model, each wheel has its own UV shell. It doesn't share one.
And the vertex order is opposite.

However the face orientation is the same, at least in Blender.

So I'm not sure whether it's a bug in r108, or if r108 is now doing things properly. ¯_(ツ)_/¯

[mrdoob]

@pushmatrix can you compare with Unity or Blender?

[pushmatrix]

@mrdoob

Blender 2.8

Unity

Sketchfab

[mrdoob]

@pushmatrix

Unity is hard to see because the light is in the same direction the camera is, but looks like Unity is wrong too? 🤔

[pushmatrix]

@mrdoob

Yeah it does depend a bit on the lighting it ends up being an illusion and looking like it's popped up in some places. But when you rotate around, it disappears.

Here it is in other light, where it appears correct

[WestLangley]

@mrdoob Front-faces have counter-clockwise winding order in three.js. Typically, the UVs of the 3 vertices of each face also have counter-clockwise winding order on the UV map.

In this example, my _conjecture_ is the model on the left has counter-clockwise UVs and the model on the right has _clockwise_ UVs. The model the right is rendering incorrectly in three.js when doubleSide is true.

AFAIK, removing the Adreno-workaround fixes the problem for non-Adreno GPUs.

[WestLangley]

@pushmatrix Your Unity example appears to have directional lights from two directions. That makes it difficult to discern what is going on. A single light may be preferable.

[pushmatrix]

@WestLangley I am using the same environment map the model-viewer is using, but likely it doesn't have the same rotation. I will try with a single light.

[pushmatrix]

@WestLangley Single directional light rotating around. I feel like I'm looking at an optical illusion

[pushmatrix]

Seems like Unity makes the grooves pop in and out depending on how the light hits it, but at least it's consistent for both.

Threejs is also consistent, but one definitely looks always popped out:

[mrdoob]

I feel like there is something wrong in the Unity one...
Like it needs something like. material.normalMapScale.x = -1.

[mrdoob]

@WestLangley

@mrdoob Front-faces have counter-clockwise winding order in three.js. Typically, the UVs of the 3 vertices of each face also have counter-clockwise winding order on the UV map.

In this example, my _conjecture_ is the model on the left has counter-clockwise UVs and the model on the right has _clockwise_ UVs. The model the right is rendering incorrectly in three.js when doubleSide is true.

AFAIK, removing the Adreno-workaround fixes the problem for non-Adreno GPUs.

Thanks, that makes sense. I think it still deserves more investigation though. If Sketchfab looks correct in all devices we may have to look at their shaders.

[Mugen87]

I had a look at the respective shader code today and it seems they are not using "Per-Pixel Tangent Space Normal Mapping".

According to this post, Sketchfab expects tangent definitions in assets or these data are
generate based on uv coordinates. That corresponds to what I've seen in the GLSL code.

If I add tangents to the model via BufferGeometryUtils.computeTangents() and set Material.vertexTangents to true, the result seems fine:

BTW: Is it okay to share code snippets from Sketchfab in this post^^? After all, it's not open-source.

[pushmatrix]

For consistency, here is Sketchfab with 1 directional light moving back and forth:

Yeah one thing to keep in mind is Sketchfab does do processing on their end when you upload a model, so very likely they could be generating things / changing things. What you're viewing is not a glTF but a glTF file converted to their own format.

[WestLangley]

@pushmatrix Can you _please_ demonstrate that #17958 works for your model?

[WestLangley]

three.js computes tangents in the fragment shader. I believe it works correctly if a hack to accommodate certain buggy Adreno GPUs is removed. See #17958.

//

Sketchfab computes tangents on the CPU when tangents are required and are not provided by the model. (@donmccurdy This is what the glTF spec requires.)

three.js can do that too, but it will mean adding correct tangents to all built-in geometries. three.js will also have to implement the MIKKTSpace algorithm to replace ComputeTangents(). Since three.js supports both indexed and non-indexed geometries, I expect this will require considerable effort.

[pushmatrix]

Fixes it!

[mrdoob]

Okay, so seems like the solution here is to revert the Adreno workaround (#17958) and recommend people to use BufferGeometryUtils.computeTangents() when the model doesn't have tangents and they're aiming to support Adreno GPUs (#15850).

Sounds good?

[mrdoob]

Another solution is to call BufferGeometryUtils.computeTangents() in the engine automatically when is not provided and remove all the code that computes tangents in the fragment shader... 🤔

[Mugen87]

BufferGeometryUtils.computeTangents() currently requires an index so it can't be use in the core. However, I think it would be preferable if three.js could generate tangents according to MIKKTSpace at some point and then remove perturbNormal2Arb().

[WestLangley]

@WestLangley wrote:

it will mean adding correct tangents to all built-in geometries.

Changing my mind... Tangents are not necessarily required. Instead, I would restore the method BufferGeometry.computeTangents() and "override" that method for all built-in geometries for which we can set exact tangents analytically.

//

Another solution is to call BufferGeometryUtils.computeTangents() in the engine automatically when is not provided and remove all the code that computes tangents in the fragment shader...

That sounds right to me.

BufferGeometryUtils.computeTangents() currently requires an index so it can't be use in the core.

I think that is easily fixed. And it will have to be fixed to support https://github.com/mrdoob/three.js/issues/17804#issuecomment-557135610.

[donmccurdy]

it would be preferable if three.js could generate tangents according to MIKKTSpace at some point and then remove perturbNormal2Arb()...

I don't know if MikkTSpace should necessarily replace the fragment shader computation. The shader has almost no performance cost, and works fine for a majority of models. Precomputing tangents has a per-vertex upfront cost every time, and no benefit except in these specific cases. 😕 Despite what the glTF spec says, I'm having a hard time justifying doing that by default.

It would be nice if BufferGeometryUtils.computeTangents could implement the MikkTSpace approach — that is the best algorithm, if you must precompute them. But it would probably be easier to put MikkTSpace in tools like glTF-Pipeline or gltfpack, which can use the existing native implementation, and do the calculation in advance, offline.

[mrdoob]

@donmccurdy

But, say... in the <model-viewer> use case, I don't think there is a reliable way to know if the user has a Adreno GPU but we would want to look right (https://github.com/GoogleWebComponents/model-viewer/issues/740).

Should <model-viewer> (and others projects that want x-gpu support) call BufferGeometryUtils.computeTangents when tangents are not provided, or should Three?

[mrdoob]

@pushmatrix

Haha, I've just realized where these wheels are from 😁

https://bumbleride.com/products/era?variant=20719969599599

[pushmatrix]

@mrdoob 🕵

[mrdoob]

Okay then, lets revert the workaround for now.