Replace perturbNormal implementation with a more robust version #21299
Conversation
This change switches to a slightly different formulation using a cotangent frame described by Christian Schüler in "Normal Mapping Without Precomputed Tangents" follow-up blog post. This implementation is nicer as it has fewer opportunities to produce a NaN output given a degenerate input; it contains one division and one normalize at the end, and only division needs to be guarded against. As a result, when the UV mapping is degenerate within a given triangle, the resulting determinant is 0 and scale is set to 0 as well. Using Mali Offline Shader Compiler on a simple shader that samples a normal map and converts it to object space using this function, the resulting code is also slightly faster than before - 20.5 cycles vs 22.3 cycles. The resulting performance on a complete three.js shader is likely to be unaffected.
|
I've tested this on some artificial examples like the one in the linked issue as well as on three.js normal map example, but any other pointers for scenes / situations to test would be appreciated as this is a rather fundamental function in the shader code :) |
|
The |
|
FWIW some validation for why this change can be important - when the input model has triangles with degenerate mapping - Before this change:
After this change:
|
|
@donmccurdy Thanks! This is a truly fantastic test model. I've confirmed that the model looks correct after this change from both sides on all orientations:
|
|
Profiling results: http://shader-playground.timjones.io/d229083f2340912fbf707e6f23d8bf63 highp: mediump: Radeon Graphics Analyzer results on shader playground sadly don't show estimated cycle counts, but the new variant has ~10% fewer instructions so that looks like a general rule of thumb wrt new variant across multiple architectures. |
|
Excellent stuff! |
|
Thanks! |
|
On a side note... (I didn't know about http://shader-playground.timjones.io/) Seems like this: float faceDirection = gl_FrontFacing ? 1.0 : - 1.0;produces less instructions than this: float faceDirection = float( gl_FrontFacing ) * 2.0 - 1.0;Do you have any recommendations? |
|
@mrdoob Nice catch - I didn't look at this part since it was generic to all shaders. It does seem that on Mali, conditional select is a bit faster. On PowerVR and AMD it looks like the compiler compiles both to more or less identical code. My usual rule of thumb is that conditional selects are preferable to attempts to emulate them using multiplications since all GPUs have dedicated instructions for this that don't require branching, and multiplications can result in more instructions / cycles in some cases - but in this case it mostly appears to be a wash. Still, since there does appear to be a tiny difference on one vendor it may be worth changing this to |
|
@zeux Thanks for doing this!
Did you try a non-uniform scale test case? Also, models having mirrored UVs must be verified (they have backwards winding order.). I think 'DamagedHelmet.gltf' has mirrored UVs. And back-sided faces -- as opposed to double-sided ones. And, of course, the Adreno hardware should be tested, which has been a problem for us. |
|
@WestLangley I've tried a case when UV mapping was degenerate along only one axis and that also worked fine. If by "back-sided faces" you mean models with disabled culling then yeah, the NormalTangentTest covers that. I'll double check wrt mirrored UVs |
|
Mirrored UVs look correct as well - I've tested using NormalTangentMirrorTest.gltf with manually patched gltf file to remove TANGENT attribute. |
If this PR had broken DamagedHelmet.gltf the e2e tests would have caught it.
The problem with Adreno was |
I believe the Adreno workarounds here previously included dFdx(vec3) which I've kept as is to not hit this :) Other than that this is mostly just vector math, |
Will do, thanks! Also, I think Babylon.js does the same. |
|
@mrdoob Indeed - Babylon.js uses the exact same function which I didn't know before. This is great since it also equalizes behavior between the two engines :) The only difference there is that they handle back-facing triangles by negating the UV coordinate instead of negating T & B which is mathematically (and numerically) equivalent. |
Yep!
What's faster? 🤓 |
|
@mrdoob Our approach looks like it's 1 cycle faster on Mali :) Same on PowerVR. Also I guess less prone to bugs with gl_FrontFacing... |
|
@zeux @mrdoob It looks like our shader math is wrong. The normal map was designed to be applied to the normal -- not to the normal after the normal matrix has been applied. We apply the normal matrix first, in the vertex shader. In theory, we should apply the normal map first. Under uniform scale -- the base case -- it does not matter. It might make sense to see how other engines handle this. Maybe we just live with it. |
Yeah that's an interesting problem. In particular, if you have a non-uniform scale along axes that are orthogonal to the vertex normal, the vertex normal stays the same! So any approach that just computes TBN from the interpolated normal in the fragment shader is doomed. I'm not sure if there's a great way to deal with this. On some level I want to say that if an application requires perfect tangent space normal mapping it really needs to store tangents in the vertex data - indeed, canonically when one bakes tangent space normal maps from a high/low-res geometry, the bake results depend on the specific tangent space calculation (for which there's no single standard, although MikkTS is becoming a de-facto one which glTF also recommends). We could of course deal with this by correcting the TBN matrix using the normal matrix in the fragment shader, but that's rather expensive and has additional issues with instanced geometry (where it's even more expensive since we compute the correctly scaled normal in the shader atm). |
|
@zeux We are in agreement. @WestLangley wrote:
It makes no sense to invest time doing that, since (as I noted above) the math in previous steps is incorrect. But at least the issue is documented here. |
This change switches to a slightly different formulation using a
cotangent frame described by Christian Schüler in "Normal Mapping
Without Precomputed Tangents" follow-up blog post.
This implementation is nicer as it has fewer opportunities to produce a
NaN output given a degenerate input; it contains one division and one
normalize at the end, and only division needs to be guarded against.
As a result, when the UV mapping is degenerate within a given triangle,
the resulting determinant is 0 and scale is set to 0 as well.
I believe this also handles non-uniform mapping better than the method
we used to use, as it preserves the relative length of T & B. It's worth noting
that this method is also more closely aligned with bump perturbation
(see perturbNormalArb in bumppar_pars_fragment.glsl.js).
Using Mali Offline Shader Compiler on a simple shader that samples a
normal map and converts it to object space using this function, the
resulting code is also slightly faster than before - 20.5 cycles vs 22.3
cycles. The resulting performance on a complete three.js shader is
likely to be unaffected.
Replaces #18871 (which was not merged)
Fixes #19727