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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - Add orthographic camera support back to directional shadows #7796

Closed
wants to merge 15 commits into from
Closed

[Merged by Bors] - Add orthographic camera support back to directional shadows #7796

wants to merge 15 commits into from

Conversation

rparrett
Copy link
Contributor

@rparrett rparrett commented Feb 23, 2023 •
edited
Loading

Objective

Fixes #7797

Solution

This seems like a simple fix, but I'm not 100% confident and I may have messed up the math in some way. In particular, I'm not sure what I should be using for an FOV value.

However, this seems to be producing similar results to 0.9.

Here's the orthographic example with a default directional light.

edit: better screen grab below.

@rparrett
Copy link
Contributor Author

rparrett commented Feb 23, 2023 •
edited
Loading

One moment, this does not work, I had accidentally worked from an old commit...

@mockersf mockersf added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen labels Feb 23, 2023
@alice-i-cecile alice-i-cecile added the P-Regression Functionality that used to work but no longer does. Add a test for this! label Feb 23, 2023
@alice-i-cecile alice-i-cecile added this to the 0.10 milestone Feb 23, 2023
@rparrett rparrett force-pushed the orthographic-directional branch from 25c3636 to bd50e4d Compare February 23, 2023 17:12
@rparrett
Copy link
Contributor Author

rparrett commented Feb 23, 2023 •
edited
Loading

Actually, I'm 99% confident that the value I'm using for the tan_half_fov is nonsensical and that this isn't working properly. It seems like calculate_cascade likely needs the camera's projection bounds, or we need a separate calculate_cascade_orthographic.

I'm guessing that it's enough to build frustum_corners from the projection bounds and the cascade near/far and the rest of the math would be the same. (but could probably be reduced)

I'll dig into it more later or hopefully a rendering expert can chime in.

Please feel free to open an alternative PR if that's easier.

@rparrett
Copy link
Contributor Author

Maybe something along these lines, breaking the "frustum corners" math into a separate function: https://github.com/rparrett/bevy/tree/orthographic-directional-two

@rparrett
Copy link
Contributor Author

rparrett commented Feb 23, 2023 •
edited
Loading

I merged that other experimental branch into this one, because testing on the naive solution I first attempted showed that it was obviously wrong with very unstable shadows.

edit: Actually, it seems like rotating a directional light with a low res shadow map is just sort of janky in bevy 0.9, orthographic, perspective, etc. But picking a random FOV is clearly wrong...

This seems to be behaving much better, but needs more testing.

@rparrett
Copy link
Contributor Author

rparrett commented Feb 24, 2023 •
edited
Loading

ortho-shadows.mov

I think this is working properly.

Note: Unlike with perspective projections, maximum_distance seems very effective at increasing shadow quality.

danchia
danchia suggested changes Feb 26, 2023
View reviewed changes
Copy link
Contributor

@danchia danchia left a comment

Choose a reason for hiding this comment

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

Thank you for doing this! Calculating the frustum corners differently for orthographic was exactly how I was going to approach this.

crates/bevy_pbr/src/light.rs Show resolved Hide resolved
crates/bevy_pbr/src/light.rs Outdated Show resolved Hide resolved
crates/bevy_pbr/src/light.rs Outdated Show resolved Hide resolved
examples/3d/orthographic.rs Outdated Show resolved Hide resolved
crates/bevy_pbr/src/light.rs Outdated Show resolved Hide resolved
crates/bevy_pbr/src/light.rs Outdated Show resolved Hide resolved
danchia
danchia suggested changes Feb 26, 2023
View reviewed changes
crates/bevy_pbr/src/light.rs Outdated Show resolved Hide resolved
crates/bevy_pbr/src/light.rs Outdated Show resolved Hide resolved
@rparrett
Copy link
Contributor Author

If you don't mind, could you double check those corners again? After applying the last two suggestions they seemed obviously wrong/different for the far plane.

@danchia
Copy link
Contributor

danchia commented Feb 26, 2023

If you don't mind, could you double check those corners again? After applying the last two suggestions they seemed obviously wrong/different for the far plane.

Looks good to me! I wonder if github suggestions was doing some weird with them

@james7132 james7132 added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Feb 27, 2023
@cart
Copy link
Member

cart commented Mar 2, 2023

bors r+

bors bot pushed a commit that referenced this pull request Mar 2, 2023
@rparrett
Add orthographic camera support back to directional shadows (#7796)
a39c223 
# Objective

Fixes #7797 

## Solution

This **seems** like a simple fix, but I'm not 100% confident and I may have messed up the math in some way. In particular, I'm not sure what I should be using for an FOV value.

However, this seems to be producing similar results to 0.9.

Here's the `orthographic` example with a default directional light.

edit: better screen grab below.
@bors
Copy link
Contributor

bors bot commented Mar 2, 2023

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Add orthographic camera support back to directional shadows [Merged by Bors] - Add orthographic camera support back to directional shadows Mar 2, 2023
@bors bors bot closed this Mar 2, 2023
Shfty pushed a commit to shfty-rust/bevy that referenced this pull request Mar 19, 2023
@rparrett @Shfty
Add orthographic camera support back to directional shadows (bevyengi…
9b7a206 
…ne#7796)

# Objective

Fixes bevyengine#7797 

## Solution

This **seems** like a simple fix, but I'm not 100% confident and I may have messed up the math in some way. In particular, I'm not sure what I should be using for an FOV value.

However, this seems to be producing similar results to 0.9.

Here's the `orthographic` example with a default directional light.

edit: better screen grab below.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mockersf mockersf mockersf approved these changes

@cart cart cart approved these changes

@danchia danchia danchia approved these changes

@superdump superdump Awaiting requested review from superdump

Assignees
No one assigned
Labels
A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior P-Regression Functionality that used to work but no longer does. Add a test for this! S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Milestone
0.10
Development

Successfully merging this pull request may close these issues.

Crash when having a Directional Light with shadows enabled and a Orthographic Camera
6 participants
@rparrett @danchia @cart @mockersf @james7132 @alice-i-cecile