Fix spotlight basis

[atlv24]

Objective

• Fix a visual regression introduced by Deduplicate orthonormal basis construction #20191 where theres a slight green line in lighting example between cube and plane

bestRanar:

I know some minor shadow differences were expected after
#20191
But the new green line below the front face of the cube in lighting seems interesting. Is that expected?
https://pixel-eagle.com/project/b25a040a-a980-4602-b90c-d480ab84076d/run/11500/compare/11494?screenshot=3D+Rendering/lighting.png

Solution

It turns out, i accidentally unflipped the x and y axis in the basis construction of spotlights to actually match the JCGT paper. This wasn't something i realized at the time, but its just a handedness flip. This means that the handedness flip Aceeri was asking about in #20191 which I just extracted from the orthonormalize implementation was just there to correct for a mistranslation in the original implementation, probably.

So this means we can just yeet it, because two handedness flips means none at all. And indeed, removing the extra flip fixes the regression. So now the code is more straightforward and understandable and it works :D

Most of the diff is updating the comments to reflect this new knowledge.

Testing

• lighting example

[github-actions]

Your PR caused a change in the graphical output of an example or rendering test. This might be intentional, but it could also mean that something broke!
You can review it at https://pixel-eagle.com/project/B04F67C0-C054-4A6F-92EC-F599FEC2FD1D?filter=PR-20272

If it's expected, please add the M-Deliberate-Rendering-Change label.

If this change seems unrelated to your PR, you can consider updating your PR to target the latest main branch, either by rebasing or merging main into it.

[superdump]

I like this as long as there are no unintended side effects. I’d like if @robtfm could take a look before merging.

[robtfm]

one possible reason for the handedness switch might be to make the shadow texture look like a normal view from the light? i guess without the switch it will be mirrored. this isn't very important but might have been useful for debugging when i was originally writing the code.

i also can't see any reason why the handedness switch should cause this seam in the lighting example though. i fear that this change is masking the issue rather than fixing it.

the only significant change looks to be the shader copysign function vs the previous >= 0.0 test, and the lighting example's green spotlight does have a direction with z==0. perhaps the equality test was yielding the same as the rust code, where the copysign function isn't?

[atlv24]

the original code had no handedness switch despite claiming it did: there was a mistranslation where the x and y in the orthonormal basis were flipped. it seems the negation of the x axis was added to correct for this, and it was documented as a handedness flip, but thats a bandage over a mistranslation and not really a handedness flip

i tested the copysign stuff separately and it makes no difference, i dont think we have a negative zero in the example

[robtfm]

the original code was copied from glam which seems to still have the same layout so i don't think it's a "mistranslation", just glam's choice of orthonormal basis was different to what i wanted.

can you think why else this change might affect the example? annoyingly i don't see the green seam so i can't really investigate.

[atlv24]

Current glam:
https://github.com/bitshifter/glam-rs/blob/main/templates/vec.rs.tera#L2355

(
    Self::new(1.0 + sign * self.x * self.x * a, sign * b, -sign * self.x),
    Self::new(b, sign + self.y * self.y * a, -self.y),
)

PR that introduced it: https://github.com/bitshifter/glam-rs/pull/129/files#diff-d0026d63cc96b573d3205664c75e79d503f4602b6d588b79a56d5e100e49ce2bR235
(same as above)

PR that introduced spotlights #4715 :

    let up_dir = Vec4::new(
        1.0 + sign * fwd_dir.x * fwd_dir.x * a,
        sign * b,
        -sign * fwd_dir.x,
        0.0,
    );
    let right_dir = Vec4::new(-b, -sign - fwd_dir.y * fwd_dir.y * a, fwd_dir.y, 0.0);

    Mat4::from_cols(
        right_dir,
        up_dir,
        fwd_dir,

Bevy main (after the change i did that broke it, but conforms to glam):

   let x_basis = Vec3::new(
        1.0 + sign * z_basis.x * z_basis.x * a,
        sign * b,
        -sign * z_basis.x,
    );
    let y_basis = Vec3::new(b, sign + z_basis.y * z_basis.y * a, -z_basis.y);
    Mat3::from_cols(x_basis, y_basis, z_basis.into())

Notice that the original spotlight pr up_dir starts with 1.0 + and right_dir starts with b. The basis that glam constructs is x: 1.0 + ..., y: b, .... The basis that bevy constructs has them flipped: x (right_dir): b, ..., y (up_dir): 1.0 + ...

This is a mistranslation, idk how else to explain it. The other thing to note is that in bevy the right_dir is negated compared to glam, this is the handedness flip pointed out in the docs, and most likely is there to correct the flip.

[atlv24]

Also I have a PR open in glam to make the handedness of any_orthonormal_pair explicit: bitshifter/glam-rs#660

The JCGT paper doesn't specify handedness anywhere, but it does specify it is of consistent handedness. I then read Frisvad's paper, which also didnt specify handedness, and then dug into the source accompanying the paper, and found that it was just using a right-handed (standard) cross product to get the third basis vector, after choosing the second one. So its a right handed basis that glam has, assuming you interpret the tuple as (x, y) given a z as input

[atlv24]

I had concerns about for example a light cookie on a spotlight being mirrored, but i don't think thats something that we can even worry about realistically with this code, as consistent rotation of the light cookie would be far more important to get right, and this code cannot specify rotation as it just uses the z direction and reconstructs the rest. I believe the light texture implementation we have just takes a different approach using clustering, which has a different basis. So this basis is only relevant for shadowmap sampling. I believe the reason the handedness flip makes things weird is that the shadow bias factors would be inverted, causing them to be applied in the wrong direction, but thats just conjecture.

[robtfm]

Notice that the up_dir starts with 1.0 + and right_dir starts with b.

ok i see. this isn't really important, but glam doesn't label these here as "x" and "y", they are just any old orthonormal pair... if i had chosen to use them as "right" and "up" instead of "up" and "right" then i agree the flip wouldn't have been necessary. and since you changed it to interpret them as x and y instead of y and x, i agree that removing the flip makes sense / recovers the original behaviour (and should also not be mirrored on the shadowmap texture).

light texture implementation we have just takes a different approach using clustering

yes there's no issue there, they use the transform rotation instead of constructing a basis.

shadow bias factors would be inverted

i managed to reproduce, and it's the same with biases set to zero. here's an interesting image with the cube texture set to semi-transparent:

the top plane of the cube is not being rendered to the shadow view at all. this suggests some issue with the rust matrix. i tried just flipping that one, and (while the x-axis of the shadows is then inverted) that does fix the top plane issue.

unfortunately if we apply this fix of negating the x-axis but then also scale the cube by vec3::splat(-1), we end up back in the same situation where the top plane fails to cast shadows (with double_sided: true on the material so that the sides and bottom do cast shadows, and the top should do).

i rechecked in bevy 0.16 and this also happened there so it's not a regression. it is weird though...

anyway i agree with this change but would love to understand what the root of the issue is.

[robtfm]

ah i understand ... with the x_axis flip, the the cube is casting shadows from the inside only. this is more-or-less ok for a cube and explains why it looks like the top plane is missing. setting cull_mode: Some(Face::Front) makes it behave as expected with the flip.

so i'm happy this is the right fix, and that there's no further issue.

[atlv24]

ok i see. this isn't really important, but glam doesn't label these here as "x" and "y", they are just any old orthonormal pair... if i had chosen to use them as "right" and "up" instead of "up" and "right" then i agree the flip wouldn't have been necessary. and since you changed it to interpret them as x and y instead of y and x, i agree that removing the flip makes sense / recovers the original behaviour (and should also not be mirrored on the shadowmap texture).

i more am just following what the paper specifies, glam lost the semantic there. It also discarded the semantic of it even being a consistently handed basis. "Any orthonormal pair" could be any handedness, but its not. Its a good point that the API is not explicitly x,y: i should update my PR bitshifter/glam-rs#660 to make that explicit in the return type somehow.

with the x_axis flip, the the cube is casting shadows from the inside only

yeah thats what i figured, im not sure it would be correct to automatically "correct" this though, as the semantics of negative scale arent clear. The inside should be the outside if you flip it, no?

[robtfm]

im not sure it would be correct to automatically "correct" this though

the fix you're doing here stops the spotlight view from ivnerting winding and that's why it fixes this problem. we shouldn't do anything else

[rparrett]

It looks like this also fixed the volumetric_fog example, which I never got around to investigating.

https://pixel-eagle.com/project/b25a040a-a980-4602-b90c-d480ab84076d/run/11563/compare/11560?screenshot=3D%20Rendering/volumetric_fog.png

At least, this changed the example to how it used to render, which looks better to me.