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

Normalize normal, tangent, and binormal before interpolating in the mobile renderer to avoid precision errors on heavily scaled meshes #99163

Merged
merged 1 commit into from
Nov 14, 2024

Conversation

clayjohn
Copy link
Member

Fixes: #89101
Fixes: #86275
Fixes: #86707

We interpolate normals using half precision in the Mobile renderer. As a result, very small normals can become corrupted. We can end up with very small normals when using a small scale in a MeshInstance's transform. In all three bug reports large meshes are used with a tiny scale (0.01 or smaller), the tiny scale transforms the normals to also be tiny, then the conversion to half float breaks them.

There are two options to solve that problem:

  1. Interpolate with full precision (this is what we do in the Forward+ renderer which is why it doesn't have issues)
  2. Normalize the normal before interpolating so it is in a good range

I chose the second option since a single normalize in the vertex shader will be relatively cheap and we need to minimize the amount of varying data that we use as it can easily be a bottleneck on Mobile devices.

@clayjohn clayjohn added this to the 4.4 milestone Nov 13, 2024
@clayjohn clayjohn requested a review from a team as a code owner November 13, 2024 06:30
@clayjohn clayjohn force-pushed the Mobile-normal-interp branch from 6148b25 to 1a9263f Compare November 13, 2024 06:31
@TCROC
Copy link
Contributor

TCROC commented Nov 13, 2024

mrp_custom_shader2.zip

@clayjohn This fixes everything for us except 1 shader! :) So with that news we can celebrate! I have attached an mrp of our one shader that still has this issue:

Desktop

image

Android

Screenshot_20241113-140007_Game MRP

This shader was created using Godot's shader graph. Here is the shader for quick reference:

image

We are so close though! The fix is almost there and this is most definitely a HUGE improvement! :)

@TCROC
Copy link
Contributor

TCROC commented Nov 13, 2024

And you may be wondering why we would create such a simple shader in the shader graph. That actually isn't the whole shader. That is just a stripped down version of the shader where this issue occurs. Aka, the mrp shader 😎. The real shader we use is animated so that the Blocky Ball logo moves across the arch in the background. But I figured an mrp shader would be easier to debug :)

@clayjohn
Copy link
Member Author

@TCROC Thank you for the MRP! Looks like we need to do the same fix for tangents and binormals (which are needed when using anisotropy). Just testing now, but I should be able to push a fix in a few minutes

…ile renderer to avoid precision errors on heavily scaled meshes
@clayjohn clayjohn force-pushed the Mobile-normal-interp branch from 1a9263f to 2c158c3 Compare November 13, 2024 20:25
@clayjohn
Copy link
Member Author

@TCROC I just pushed a change that fixes this for your new MRP too! Should be good to go now

@TCROC
Copy link
Contributor

TCROC commented Nov 13, 2024

@TCROC I just pushed a change that fixes this for your new MRP too! Should be good to go now

Brilliant! I'll go try it out now! :)

@TCROC
Copy link
Contributor

TCROC commented Nov 13, 2024

@clayjohn I can confirm that it works! Thank you so much for the quick fix on this issue! :) Just in case you weren't aware, I just want you to know that you are indeed the man! 😎

@TCROC
Copy link
Contributor

TCROC commented Nov 13, 2024

And the Godot community in general! You all never fail to impress me! :)

@akien-mga akien-mga changed the title Normalize normal before interpolating in the mobile renderer to avoid precision errors on heavily scaled meshes Normalize normal, tangent, and binormal before interpolating in the mobile renderer to avoid precision errors on heavily scaled meshes Nov 14, 2024
@roalyr
Copy link

roalyr commented Nov 14, 2024

Does it mean that after this PR is merged I should roll back the change in my fork's scene_forward_mobile.glsl precision for normal, tangent and binormal back to mediump? cb99e40#diff-94158bc3d510819bb8d7ebd62a8890eeebad49608d9e934d148b73b6b65aeff9

@Calinou Calinou added topic:3d cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Nov 14, 2024
@clayjohn
Copy link
Member Author

Does it mean that after this PR is merged I should roll back the change in my fork's scene_forward_mobile.glsl precision for normal, tangent and binormal back to mediump?

Yes, mediump will be slightly faster in some situations on mobile devices

@Repiteo Repiteo merged commit 7f6b272 into godotengine:master Nov 14, 2024
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Nov 14, 2024

Thanks!

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