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

Scene Shaders - TBN Vector Fixes #100441

Merged
merged 1 commit into from
Dec 20, 2024

Conversation

StaydMcMuffin
Copy link
Contributor

@StaydMcMuffin StaydMcMuffin commented Dec 15, 2024

Part two of: #100348

Supersedes #78339 sans the anisotropic fixes, which I leave for a future PR dedicated to anisotropy.

Addresses two errors found in the scene shader files that relate to the TBN basis vectors, ie. the vertex normals, tangents, and bitangents/binormals.

First and most important, the normalization of these vectors was reversed from what MikkTSpace expects.
That is to say, the vectors were left unnormalized by the end of the vertex shader, and were normalized early in the fragment shader, whereas MikkTSpace is designed to work with TBN vectors that are normalized prior to vertex interpolation, then left unnormalized for the final tangent-space transformation of the shading normal (which is, itself, subsequently normalized).
Not only is this what's officially advised, as a result it also matches the behaviour of Blender, ensuring that assets made in Blender have their normals reconstructed exactly in Godot.

Second, while the interpolated vertex normal was properly negated on backfaces, this only applied to the subsequent shading normal. Other uses of the vertex normal, such as for applying Normal Bias to shadows, were not inverted, leading to shading errors like the one shown below.

Before After
image image

DirectionalLight3D with Bias = 0.01 and Normal Bias = 1.0
Note: The left square is a front-facing quad, the right square is back-facing.

bugsquad edit: Fixes: #78343

@StaydMcMuffin StaydMcMuffin requested a review from a team as a code owner December 15, 2024 17:03
@StaydMcMuffin StaydMcMuffin force-pushed the scene-shaders-tbn-fixes branch 2 times, most recently from f42fad2 to 4d1656b Compare December 15, 2024 17:30
@hpvb hpvb added this to the 4.x milestone Dec 15, 2024
@clayjohn
Copy link
Member

Supersedes: #78339

#78339 additionally recreates the TBN frame in the fragment shader, but I feel like that part was overkill.

The path to getting this merged is twofold:

  1. Confirm that using geo_normal doesn't hurt performance. You can also check VGPR usage with either the mali offline compiler or the Radeon GPU analyzer (both are available on shader playground). I suspect that overall improving our VGPR usage will be more valuable than saving the cost of the normalize calls
  2. Remove the first commit with the style changes. Ideally this PR should just contain the substantive fixes so we can merge the fix without having to merge 1,300 lines of style changes first. Style changes are very low priority for us and invite more bikeshedding because style changes are often subjective, so they take longer to merge.

@StaydMcMuffin StaydMcMuffin force-pushed the scene-shaders-tbn-fixes branch from 4d1656b to e447898 Compare December 17, 2024 03:25
Fixes two errors related to the normal, tangent,
and bitangent vectors, namely normals not always
being inverted on backfaces, and normalization
being reversed from what MikkTSpace expects.
@StaydMcMuffin StaydMcMuffin force-pushed the scene-shaders-tbn-fixes branch from e447898 to ed0e3d7 Compare December 17, 2024 05:37
@StaydMcMuffin
Copy link
Contributor Author

Supersedes: #78339

#78339 additionally recreates the TBN frame in the fragment shader, but I feel like that part was overkill.

The path to getting this merged is twofold:

1. Confirm that using `geo_normal` doesn't hurt performance. You can also check VGPR usage with either the mali offline compiler or the Radeon GPU analyzer (both are available on shader playground). I suspect that overall improving our VGPR usage will be more valuable than saving the cost of the normalize calls

2. Remove the first commit with the style changes. Ideally this PR should just contain the substantive fixes so we can merge the fix without having to merge 1,300 lines of style changes first. Style changes are very low priority for us and invite more bikeshedding because style changes are often subjective, so they take longer to merge.

With help from @hpvb, we've ascertained that VGPR usage actually seems to be the same between this branch and master.
Assuming I understand how VGPRs and input variables work, I presume this is because normal_interp is no longer used anywhere after geo_normal is constructed, allowing the latter to take the former's place in the registers, so to speak.
From my (limited) testing, there doesn't seem to be a performance difference between this branch and master either way, though if anyone with a better setup wants to test more thoroughly it would be appreciated.

I've also rebased away from the style fixes PR, so this is now mergeable as a standalone fix.

@clayjohn
Copy link
Member

Taking a look at the results in Radeon GPU Analyzer, I can see that this does indeed consume an extra VGPR, however, the VGPR is released before we hit our worst-case allocation section so the number of allocated VGPRs doesn't change (remember, VGPR usage is based on a worst-case allocation strategy).

Overall though, you are right, I forgot about the side check. Fixing CULL_DISABLED and CULL_BACK is necessary and this is the way to do it.

Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me!

@clayjohn clayjohn modified the milestones: 4.x, 4.4 Dec 18, 2024
@Repiteo Repiteo merged commit 1536e0e into godotengine:master Dec 20, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Dec 20, 2024

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tangent space sync issues on normal mapped meshes
4 participants