Fix black spots appearing due to NANs when SSAO is enabled

[Elabajaba]

Objective

Fixes #8925

Solution

Clamp the bad values.

Normalize the prepass normals when we get them in the prepass_normal() function.

More Info

The issue is that NdotV is sometimes very slightly greater than 1 (maybe FP rounding issues?), which caused F_Schlick() to return NANs in pow(1.0 - NdotV, 5.0) (call stack looked likepbr() -> directional_light() -> Fd_Burley() -> F_Schlick())

[Elabajaba]

Another option that fixes it is to wrap prepass_normal() here

bevy/crates/bevy_pbr/src/render/pbr.wgsl

Line 121 in bb59509

pbr_input.N = prepass_normal(in.frag_coord, 0u);

with normalize()

[Elabajaba]

I changed it to just normalize the prepass_normal() return value to avoid any potential loss of data from clamping.

We do this for apply_normal_mapping() when the prepass normals aren't used here

bevy/crates/bevy_pbr/src/render/pbr_functions.wgsl

Line 107 in c6170d4

return normalize(N);

[superdump]

As mentioned on Discord, I think this solution is probably not the best choice.

The prepass normals are stored in an Rgb10a2Unorm texture. That means the values must be in the range [0,1] in the texture. I’m not certain but I think any values outside of that range would be clamped to that range.

Clamping is a problem because it changes the direction of the normal, which is undesirable for lighting. Normalising is probably a better choice.

Mikktspace says that when using normal mapping, you are not supposed to normalise the vertex normal, nor the interpolated vertex normal in order to obtain an exact inverse of the normal map baking process. You only normalise the normal-mapped normal which is the one used for lighting. This is done at the end of apply_normal_mapping.

So, the problem here is that mikktspace would potentially allow normals that don’t fit in Rgb10a2Unorm, and would be clamped, and then perhaps also that the world normal is not normalised.

I think then maybe the best thing to do is to go against the mikktspace recommendations and normalise the world normal before writing to the texture. I’m not certain of this but I’d say until we have clarity on what to do, this is possibly a bit controversial.

On the other hand, it is better than stuff doesn’t look broken so maybe we should add a FIXME comment to this, merge it, and continue figuring out how best to address it.

[Elabajaba]

@superdump sorry, I forgot to update the PR description.

I removed all the clamps, and this just normalizes the prepass_normal() call.

[IceSentry]

We do this for apply_normal_mapping() when the prepass normals aren't used here

Unless something else changed, the prepass already calls apply_normal_mapping() so the results should already be normalized the exact same way. If they aren't it might mean there's a missing shader def in the prepass or something.

[IceSentry]

Also, I think this shouldn't be done in prepass_normal() but should be done at the callsite. The normal texture can be generated by a custom material and normalizing there might not be what user expects. So I'd just do it directly in pbr.wgsl.

[IceSentry]

It was pointed out on discord that this happens because of precision issues even if the normals are normalized in the prepass. So it makes sense to do it like this. So you can ignore my comments.

[alice-i-cecile]

@superdump are you okay if we merge this?

[fintelia]

The saturate method is defined as:

Returns clamp(e, 0.0, 1.0). Component-wise when T is a vector.

So it doesn't seem to make much sense to do clamp(saturate(...), 0.0, 1.0)...

Edit: Seems that was removed in some intermediate commit

[superdump]

After some more discussions on discord and here, I’m fine with this solution.