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

Left-handed y-up cubemap coordinates #8122

Merged
merged 3 commits into from
Mar 18, 2023
Merged

Conversation

danchia
Copy link
Contributor

@danchia danchia commented Mar 18, 2023

Objective

  • Texture cubemaps, such as used by point light shadows, inherently use left-handed y-up coordinate axes. Prior to this PR, I understand that we attempted to keep cubemaps mostly right-handed. However, the solution has sharp edges, such as requiring the sampling vector to still be negated, as well as hard-to-figure reasons for why the cubemap faces use a specific target/up axis.
  • Solves the same problem as bevy_pbr: Use left-handed coordinates for point light shadow cubemaps #4242, but with a slightly different approach.

Solution

  • We embrace texture cubemaps being left-handed y-up:
    • When it comes time to sample them we flip the z coordinate to transform bevy's right-handed y-up coordinates to left-handed y-up coordinates.
    • When rendering the cubemap, carefully pick the right Bevy coordinate-space target/up axis to produce the correct cubemap faces accounting for the fact that the cubemap faces are specified in the left-hand coordinate space.
    • Note that the in prior PR bevy_pbr: Use left-handed coordinates for point light shadow cubemaps #4242, we converted the entire shadow rendering pipeline for the cubemap to left-handed coordinate space. I felt that carefully mapping the cubemap faces to the right axes led to a simpler solution overall, even if the cubemap face mapping had to be done carefully.

Changelog

  • Bevy point light shadow cubemaps now use a left-handed y-up coordinate space.

Migration Guide

  • When sampling from the point light shadow cubemap, use the (expected) light to fragment direction vector but negate the z coordinate. Previously, you would have used the fragment to light direction vector.

@danchia danchia requested a review from superdump March 18, 2023 07:14
Copy link
Contributor

@superdump superdump left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simple, clean, good stuff.

@superdump superdump requested a review from robtfm March 18, 2023 07:38
@james7132 james7132 added C-Feature A new feature, making something new possible A-Rendering Drawing game state to the screen labels Mar 18, 2023
@james7132 james7132 added this to the 0.11 milestone Mar 18, 2023
Copy link
Contributor

@robtfm robtfm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm apart form 1 nit

crates/bevy_pbr/src/render/shadows.wgsl Outdated Show resolved Hide resolved
@danchia danchia requested a review from robtfm March 18, 2023 16:17
@danchia
Copy link
Contributor Author

danchia commented Mar 18, 2023

Thanks for the reviews!

@superdump superdump added this pull request to the merge queue Mar 18, 2023
@superdump superdump merged commit 2010164 into bevyengine:main Mar 18, 2023
Shfty pushed a commit to shfty-rust/bevy that referenced this pull request Mar 19, 2023
Shfty pushed a commit to shfty-rust/bevy that referenced this pull request Mar 19, 2023
@Selene-Amanita Selene-Amanita added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Jul 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Feature A new feature, making something new possible M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants