[Merged by Bors] - bevy_pbr: Normalize skinned normals

[superdump]

Objective

• Make the many foxes not unnecessarily bright. Broken since [Merged by Bors] - bevy_pbr: Fix tangent and normal normalization #5666.
• Fixes many_foxes are all black #6528

Solution

• In [Merged by Bors] - bevy_pbr: Fix tangent and normal normalization #5666 normalisation of normals was moved from the fragment stage to the vertex stage. However, it was not added to the vertex stage for skinned normals. The many foxes are skinned and their skinned normals were not unit normals. which made them brighter. Normalising the skinned normals fixes this.

---

Changelog

• Fixed: Non-unit length skinned normals are now normalized.

[superdump]

How does this relate to #6540?

This should be the correct fix. But I want it to see some testing. As noted in the comment from the code, normalization is not meant to be applied to all normals in the fragment shader before normal mapping. It may be that we should make sure to normalise all normals at the end of the apply_normal_mapping() function though and that may be more difficult to find errors with so maybe I'll add that...

[superdump]

So the remaining bit that I'm unsure of is whether perspective correct interpolation causes normals to be significantly scaled such that they are no longer unit normals. The safe bet then would be:

diff --git a/crates/bevy_pbr/src/render/pbr_functions.wgsl b/crates/bevy_pbr/src/render/pbr_functions.wgsl
index 58eb7023e..3e9046353 100644
--- a/crates/bevy_pbr/src/render/pbr_functions.wgsl
+++ b/crates/bevy_pbr/src/render/pbr_functions.wgsl
@@ -91,12 +91,12 @@ fn apply_normal_mapping(
     // calculates the normal maps so there is no error introduced. Do not change this code
     // unless you really know what you are doing.
     // http://www.mikktspace.com/
-    N = normalize(Nt.x * T + Nt.y * B + Nt.z * N);
+    N = Nt.x * T + Nt.y * B + Nt.z * N;
 #endif
 #endif
 #endif
 
-    return N;
+    return normalize(N);
 }
 
 // NOTE: Correctly calculates the view vector depending on whether

[superdump]

I figured what I could do was make the main pass pbr function return vec4<f32>(vec3<f32>(length(in.N)), 1.0) and compare the lengths of the vectors visually. Though I realise now that the conversion to srgb and quantisation to 8-bit will probably hide a lot of the error...

Without the normalisation diff from above that normalises at the end of apply_normal_map():

With it:

NVIDIA FLIP diff:

NVIDIA FLIP stats:

Invoking LDR-FLIP
     Pixels per degree: 67

FLIP between reference image <Screenshot 2022-11-10 at 23.41.15.png> and test image <Screenshot 2022-11-10 at 23.41.22.png>
     Mean: 0.003883
     Weighted median: 0.031568
     1st weighted quartile: 0.026942
     3rd weighted quartile: 0.033410
     Min: 0.000000
     Max: 0.070265
     Evaluation time: 1.0133 seconds

[superdump]

This is outputting vec4<f32>(vec3<f32>(length(pbr_input.N) - 1.0), 1.0):

So I think it's a good idea to add the normalize() at the end of apply_normal_map() as well.

[Elabajaba]

This fixes it for me.

[cart]

bors r+

[bors]

Pull request successfully merged into main.

Build succeeded:

• build-and-install-on-iOS
• build-android
• build (macos-latest)
• build (ubuntu-latest)
• build-wasm
• build (windows-latest)
• build-without-default-features (bevy)
• build-without-default-features (bevy_ecs)
• build-without-default-features (bevy_reflect)
• check-compiles
• check-doc
• check-missing-examples-in-docs
• ci
• markdownlint
• run-examples
• run-examples-on-wasm
• run-examples-on-windows-dx12