Fix simultaneous usage of clearcoat and tangent data

[Oletus]

Clearcoat normals need to use temporary variable names that are different from regular normals so that there's no name conflict. This also moves sampling of the normal map outside of the normal perturbation helper function so that there isn't as much duplicate code.

[Oletus]

@Mugen87 @mrdoob Please review this bug fix!

[Oletus]

Not sure what's up with the CI, looks like it maybe timed out due to some unrelated condition?

[Mugen87]

Current (https://threejs.org/examples/webgl_materials_physical_clearcoat):

PR:

It seems the actual normalMap is now visible.

[Oletus]

Looks like the previous code also had another bug - in case per-vertex tangents were being used, clearcoat would use the regular normal map. I used that as a reference so I accidentally broke the case that was previously working correctly. Added a commit to always use the right normal map now.

[WestLangley]

It seems the actual normalMap is now visible.

@Mugen87 No, that is a bug in the PR.

[WestLangley]

@Oletus said

Clearcoat normals need to use temporary variable names that are different from regular normals so that there's no name conflict.

Can you please point to the bug you were referring to -- and presumably fixed?

//

I spent about an hour studying your PR and I agree your refactoring is an improvement. Thanks!

[Oletus]

I didn't file a separate bug report for the bugs.

Bug 1: there's a shader name conflict when vertexTangents are enabled on a material with clearcoat. You can reproduce this by using BufferGeometryUtils.computeTangents and enabling vertexTangents in the clearcoat example.

Bug 2: clearcoat shader code under the USE_TANGENT define was also referring to regular normal map instead of the clearcoat normal map , this line in the old code:

vec3 mapN = texture2D( normalMap, vUv ).xyz * 2.0 - 1.0;`

[WestLangley]

@Oletus This looks good to me. Thank you.

It is not your fault, but IMO, the shader code with the chunks is very fragile. Logical errors and edge cases are easy to overlook. I found the bug you introduced prior to you mentioning it (bug 2), but I did not notice the redeclaration bug you fixed (bug 1) until I asked.

I think it would be beneficial if @arobertson0 would review these changes, too.

[arobertson0]

I think it would be beneficial if @arobertson0 would review these changes, too.

On it =)

[arobertson0]

The examples are running correctly, and the code is clear and seemingly correct =)

I'm noticing that the contents of clearcoat_normal_fragment_maps and normal_fragment_maps are very similar.
Would it be worthwhile to abstract the logic as a function? It would probably land in normal_fragment_begin, right where vTBN is being defined.

[WestLangley]

The examples are running correctly

The examples worked before...

The related examples don't use tangents. Testing requires a lot of effort, and numerous use cases.

[Oletus]

Should we add something to examples to have at least a basic test for this? It would be easy enough to add variations of geometry that use computed per-vertex tangents to the webgl_materials_physical_clearcoat example.

[WestLangley]

Should we add something to examples

That is up to @mrdoob.

For the record, there are 8 cases:

• with/without normal map
• with/without clearcoat normal map
• with/without tangents

[Mugen87]

I don't think the mentioned examples should block this PR from being merged. They can also be added at a later point, too.

[mrdoob]

Looks good to me. We can improve things as we go.

[mrdoob]

Thanks!

[WestLangley]

/ping @sunag FYI. :-)