Correctly transform normals for non-uniform scales

[CptPotato]

This changes the way normals are transformed in the vertex shader of bevy_pbr. In order to correctly transform normals, the scaling actually has to be applied inversely (see this explanation for details). Otherwise non-uniform scales will produces incorrect normals.

My proposed change comes with the assumption that the Model matrix is orthogonal (i.e. does not contain shearing). This allows for some optimization, I can explain more if needed.

Here's a pancaked version of load_gltf as comparison:

screenshot

Since this is just a minor change I didn't bother creating an issue. Feel free to close if the solution doesn't seem fit.

[alice-i-cecile]

Three naive questions:

1. Will this method be called repeatedly? Aka, do we care about optimizing here?
2. Are there any realistic scenarios where users may want a non-orthogonal model matrix?
3. Can we include some sort of assert or warning if this assumption is violated without compromising performance?

[CptPotato]

Three naive questions:

1. Will this method be called repeatedly? Aka, do we care about optimizing here?
2. Are there any realistic scenarios where users may want a non-orthogonal model matrix?
3. Can we include some sort of assert or warning if this assumption is violated without compromising performance?

1. Kind of? It's executed for all vertices every frame. It's running on the GPU so it's not a huge impact, but using the "optimized" version I went with does maybe save 20-30 or so arithmetic operations.
   Alternatively we could also just use the more general transpose(inverse(m)) and take that (minor) performance hit.
2. Currently I don't think that's even possible in bevy to my knowledge. At least not without major changes from the user. The Transform component only stores position, rotation and scale which makes it impossible to produce shearing.
   From what I know, shearing is usually even something developers try to avoid, because it can get tricky to handle in many scenarios (collision detection, physics, culling, etc.)
3. (see 2.): Not really applicable because it's currently not possible to produce that case.

[alice-i-cecile]

Great, sounds like a perfectly reasonable optimization then. Thanks for the explanation :)

[CptPotato]

There's a scenario that I haven't considered, yet, sorry.
I'll keep this as a draft until I tested and verified it.

[cart]

It would also be worth investigating how other popular engines approach this calculation. It seems like they must be doing it by default, so its probably just a matter of peeking at their shaders. I'm familiar with godot's code, so I can just link to the relevant bit:

https://github.com/godotengine/godot/blob/4a7679e4dd9f68270b3d27797146f88491f182b8/servers/rendering/renderer_rd/shaders/scene_forward_clustered.glsl#L119

[CptPotato]

@cart thanks for the input and link. I did take a look at Godot's shader code but to be honest, it doesn't look quite right to me. Taking things to the editor seems to confirm this (unless the bitflag of the shader needs to be enabled manually somehow):

screenshot

As mentioned in my comment above, to my knowledge transpose(inverse(m)) should be the general approach of inverting the scale of a mat3 and keeping everything else.
I went for the (non-shearing) optimization because invert(m) is a quite hefty operation. It's not too bad here since it's just a mat3 but I still opted for performance given that shearing can't even happen currently. Godot's approach of branching and passing it as a bitflag is interesting, but I don't have what it takes to bring that into bevy.

Also, I'll likely keep this as a draft until some things around #1755 are cleared up.

[CptPotato]

Closing, since this is for the old renderer and the new one already does this correctly.