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

Fix CameraProjection panic and improve CameraProjectionPlugin #11808

Merged
merged 4 commits into from
Apr 27, 2024

Conversation

doonv
Copy link
Contributor

@doonv doonv commented Feb 10, 2024

Objective

Fix #11799 and improve CameraProjectionPlugin

Solution

CameraProjectionPlugin is now an all-in-one plugin for adding a custom CameraProjection. I also added PbrProjectionPlugin which is like CameraProjectionPlugin but for PBR.

P.S. I'd like to get this merged after #11766.


Changelog

  • Changed CameraProjectionPlugin to be an all-in-one plugin for adding a CameraProjection
  • Removed VisibilitySystems::{UpdateOrthographicFrusta, UpdatePerspectiveFrusta, UpdateProjectionFrusta}, now replaced with VisibilitySystems::UpdateFrusta
  • Added PbrProjectionPlugin for projection-specific PBR functionality.

Migration Guide

VisibilitySystems's UpdateOrthographicFrusta, UpdatePerspectiveFrusta, and UpdateProjectionFrusta variants were removed, they were replaced with VisibilitySystems::UpdateFrusta

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior C-Enhancement A new feature A-Rendering Drawing game state to the screen S-Blocked This cannot move forward until something else changes labels Feb 10, 2024
@doonv
Copy link
Contributor Author

doonv commented Feb 26, 2024

This is now unblocked since #11766 has been merged.

@NthTensor NthTensor removed the S-Blocked This cannot move forward until something else changes label Apr 23, 2024
Copy link
Contributor

@NthTensor NthTensor left a comment

Choose a reason for hiding this comment

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

Code looks like a big improvement. Will approve later after I get the chance to evaluate this on examples.

@Zoomulator
Copy link
Contributor

Zoomulator commented Apr 23, 2024

@doonv I did a rebase and fixed conflicts: doonv#3

Notable conflicts:

  • CameraProjectionPlugin<T> got new trait bounds T: CameraProjection + Component + GetTypeRegistration, which conflicted with a doc comment above it.
  • CameraUpdateSystem struct definition was moved to the line below CameraProjectionPlugin
  • check_visibility is now generic check_visibility::<WithMesh> (view/visibility/mod.rs)

Copy link
Contributor

@Zoomulator Zoomulator left a comment

Choose a reason for hiding this comment

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

Been running this in my own bevy fork for my game and it's been working well with my custom projection + pbr shadows. No complaints :)

@BD103 BD103 added the C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide label Apr 23, 2024
@doonv doonv requested a review from Zoomulator April 25, 2024 21:25
Copy link
Contributor

@Zoomulator Zoomulator left a comment

Choose a reason for hiding this comment

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

Looks good! Well spotted to not need the check_visibility.after given the recently added .before(CheckVisibility) above.

@NthTensor NthTensor 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 Apr 26, 2024
@mockersf mockersf added this pull request to the merge queue Apr 26, 2024
Merged via the queue into bevyengine:main with commit de9dc9c Apr 27, 2024
28 checks passed
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-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide C-Bug An unexpected or incorrect behavior C-Enhancement A new feature 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
Development

Successfully merging this pull request may close these issues.

Panic upon using a CameraProjection with DirectionalLight shadows
6 participants