bevy_pbr: Use left-handed coordinates for point light shadow cubemaps

[superdump]

Objective

• Point light shadow cubemaps (and cubemaps in general) use left-handed coordinates. Trying to force it to work using right-handed coordinates makes the code look very confusing, and makes it very very difficult to reason about the code.

Solution

• Frustum culling is still done using right-handed coordinates, but the +z/-z view rotations are swapped to correspond to those view directions being opposite in left-handed
• Point light view, and projection matrices are left-handed
• Cubemap texture sampling can now correctly use the direction from the light to the fragment, instead of the other way around
• The up and target directions of the cube map faces now follow left-handed x-right, y-up, z-forward conventions and make sense
• An example was added with a light in a box with some floating small boxes between the light and the walls. These floating boxes are slightly offset in the positive axis direction to allow inspection of the point light shadow cubemap textures in a debugger such as RenderDoc or Xcode to verify that they are correct

[superdump]

I think I'll need this PR for the next steps of Percentage-Closer Filtering and making The Witness approach work for point light shadows as well as directional light shadows.

[aloucks]

This seems to add a lot of complexity so that the light direction doesn't have to be negated. I would think that keeping everything in the same coordinate system would make things easier to reason about, not less, but I'm certainly not an expert here. I'm assuming that there's something that I'm missing. Do "correct" cubemap textures look wrong in the debugger when they are using a right-handed coordinate system (i.e. is that the motivation for the change)?

[superdump]

This seems to add a lot of complexity so that the light direction doesn't have to be negated. I would think that keeping everything in the same coordinate system would make things easier to reason about, not less, but I'm certainly not an expert here. I'm assuming that there's something that I'm missing. Do "correct" cubemap textures look wrong in the debugger when they are using a right-handed coordinate system (i.e. is that the motivation for the change)?

A note - I may sound harsh in my tone in what follows. It is definitely not directed at you. Your question is legitimate and it is my responsibility to make the motivation for this change clear.

The reason my tone may be harsh is that I have built up frustration about this as I have spent many days if not multiple weeks in total debugging cubemap sampling over the past year. I eventually discovered that the root problem making it not possible to reason about what was going on, and making it seemingly impossible to have simple shader code (that is executed for every pixel of every frame) that addresses all cases was that cubemapping in graphics drivers and hardware uses left-handed coordinates as it was designed that way.

So, the motivation for the change is that for point light shadow cubemap filtering, one needs to be able to offset from the sample position (the vector from the light to the fragment) by shadow map texels around the sample point. This involves converting the light to fragment vector to shadow map texel coordinates, applying the offsets, and then converting back.

Because we are currently forcing the cubemap generation and sampling to be right-handed when it is absolutely left-handed in graphics drivers and hardware, we're doing multiple hacks that somehow work but are incomprehensible when debugging or trying to add features. For example, the CUBE_MAP_FACES target and up vectors make no sense before this PR. When sampling a cubemap, which is like a cube as observed from the centre of the cube, you are supposed to use the vector from the centre of the cube pointing in the direction you want to sample, which makes a lot of sense, but we have to hack it to use the vector pointing from the direction we want to sample to the centre, and reasoning about what gets sampled by that ends up being nonsense, especially when trying to figure out what should happen as it moves from one cube face to another.

As such, in my opinion as a person who has and will be doing development using cubemaps, a small amount of complexity to convert to the correct coordinate space for cubemaps is a necessary change.

tl;dr trying to handle cubemaps, which are left-handed, using only right-handed coordinates, is practically unmaintainable / not extensible.

[superdump]

I moved some unrelated view z calculation fixes to #4330

[superdump]

I noticed that this is actually broken. I'll try to fix it.

[aevyrie]

A note - I may sound harsh in my tone in what follows. It is definitely not directed at you. Your question is legitimate and it is my responsibility to make the motivation for this change clear.

FWIW, I don't detect any harshness.

tl;dr trying to handle cubemaps, which are left-handed, using only right-handed coordinates, is practically unmaintainable / not extensible.

The argument you've laid out makes a lot of sense to me. Can you outline any potential negative consequences you can foresee, other that the aforementioned added complexity?

The only thing I can suggest for this PR is to add some module-level documentation on why LH coordinates are needed. Adding some reasoning for this choice seems important enough to include in the bevy_pbr/src/render/light docs. The "why" isn't very obvious to me looking at the code without the context you added above.

[superdump]

tl;dr trying to handle cubemaps, which are left-handed, using only right-handed coordinates, is practically unmaintainable / not extensible.

The argument you've laid out makes a lot of sense to me. Can you outline any potential negative consequences you can foresee, other that the aforementioned added complexity?

I’m not aware of anything other than that one needs to be aware of the change in coordinate systems that happens for cubemaps. I’m thinking that it should be in generic, reusable code for handling cubemaps as it’s not specific to point light shadow maps. But I would have to try to build that to figure out how to share it and that’s not my goal here.

The only thing I can suggest for this PR is to add some module-level documentation on why LH coordinates are needed. Adding some reasoning for this choice seems important enough to include in the bevy_pbr/src/render/light docs. The "why" isn't very obvious to me looking at the code without the context you added above.

Yup. I could definitely do that.

[superdump]

Removing from the 0.7 milestone as I am unlikely to be able to fix it in time.

[superdump]

I fixed the bug. I was multiplying a Mat4::from_translation(left-handed translation) * Mat4::look_at_lh() and because Mat4::look_at_*() seem to produce inverse view matrices, I was inverting that combined rotation + translation matrix, which then incorrectly inverted the translation part. As such, I moved the inversion of Mat4::look_at_*() to where they are first called and added notes. It makes more sense now.

As for the documentation part, I was adding this at the top of bevy_pbr/src/render/light.rs but it's not visible in the docs as far as I can tell, I would guess because the light module is included privately in its parent, and pub use light::*; instead. So, I'm not sure where to put this documentation but I'll not it here so it can be reviewed and then it should be quick to put it wherever we think is necessary/appropriate:

diff --git a/crates/bevy_pbr/src/render/light.rs b/crates/bevy_pbr/src/render/light.rs
index f9caf6f64..b94ef0821 100644
--- a/crates/bevy_pbr/src/render/light.rs
+++ b/crates/bevy_pbr/src/render/light.rs
@@ -1,3 +1,15 @@
+//! Lights and shadows
+//!
+//! This module supports directional and point lights, and depth-buffer based shadow maps.
+//!
+//! Point light shadow maps use cubemaps with a left-handed, x-right, y-up, z-forward coordinate
+//! system, which is contrary to bevy's convention of right-handed, x-right, y-up, z-back for world
+//! coordinates. The reason for this exception is because cubemaps are implemented in graphics
+//! drivers and hardware using a left-handed coordinate system. Trying to force a right-handed
+//! convention only causes hacks, bugs, and confusion. Developers of this module (Rob Swain) have
+//! lost many hours trying to debug shadow cubemap bugs only to learn that cubemaps use left-handed
+//! coordinates. Let those hours not be in vain. :)
+
 use crate::{
     point_light_order, AmbientLight, Clusters, CubemapVisibleEntities, DirectionalLight,
     DirectionalLightShadowMap, DrawMesh, GlobalVisiblePointLights, MeshPipeline, NotShadowCaster,

[superdump]

I chatted with @alice-i-cecile on Discord and they suggested making the module chain public. So, I did.

[Nilirad]

This PR adds a new example. Adding module and item level doc comments, as described in:

• Module-style doc strings for examples #3951 and
• [Merged by Bors] - Doc/module style doc blocks for examples #4438,

would be really useful to those who will browse examples.

[superdump]

OK, I think this is in OK shape again now. I decided to stick with right-handed coordinates except for the view and projection matrices, and then when sampling from the shadow cubemap, switch to left-handed at the appropriate point. This allows more code to use the regular right-handed y-up convention, and just the code that actually has to do with the cubemap sampling uses left-handed y-up.

[superdump]

Closing in favour of #8122 which is a simpler solution.