Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Merged by Bors] - bevy_pbr: Normalize skinned normals #6543

Conversation

superdump
Copy link
Contributor

@superdump superdump commented Nov 10, 2022

Objective

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.

This was forgotten in bevyengine#5666 when normalization of normals was moved from the
fragment to vertex stage.
@superdump superdump requested a review from cart November 10, 2022 22:08
@superdump superdump added this to the Bevy 0.9 milestone Nov 10, 2022
@superdump superdump added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen C-Regression Functionality that used to work but no longer does. Add a test for this! labels Nov 10, 2022
@superdump
Copy link
Contributor Author

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
Copy link
Contributor Author

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
Copy link
Contributor Author

superdump commented Nov 10, 2022

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():
Screenshot 2022-11-10 at 23 41 15

With it:
Screenshot 2022-11-10 at 23 41 22

NVIDIA FLIP diff:
flip Screenshot 2022-11-10 at 23 41 Screenshot 2022-11-10 at 23 41 67ppd ldr

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
Copy link
Contributor Author

This is outputting vec4<f32>(vec3<f32>(length(pbr_input.N) - 1.0), 1.0):
Screenshot 2022-11-10 at 23 51 53
So I think it's a good idea to add the normalize() at the end of apply_normal_map() as well.

The interpolated world normals were not being re-normalized for the non-normal
mapping case.
@Elabajaba
Copy link
Contributor

This fixes it for me.

@cart
Copy link
Member

cart commented Nov 11, 2022

bors r+

bors bot pushed a commit that referenced this pull request Nov 11, 2022
# Objective

- Make the many foxes not unnecessarily bright. Broken since #5666.
- Fixes #6528 

## Solution

- In #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.
@bors bors bot changed the title bevy_pbr: Normalize skinned normals [Merged by Bors] - bevy_pbr: Normalize skinned normals Nov 11, 2022
@bors bors bot closed this Nov 11, 2022
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

- Make the many foxes not unnecessarily bright. Broken since bevyengine#5666.
- Fixes bevyengine#6528 

## Solution

- In bevyengine#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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior C-Regression Functionality that used to work but no longer does. Add a test for this!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

many_foxes are all black
4 participants