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

Using a custom CameraProjection and directional light shadows crash #9077

Closed
Selene-Amanita opened this issue Jul 8, 2023 · 0 comments · Fixed by #9226
Closed

Using a custom CameraProjection and directional light shadows crash #9077

Selene-Amanita opened this issue Jul 8, 2023 · 0 comments · Fixed by #9226
Labels
A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior P-Crash A sudden unexpected crash S-Needs-Design This issue requires design work to think about how it would best be accomplished
Milestone
0.12

Comments

@Selene-Amanita
Copy link
Member

Selene-Amanita commented Jul 8, 2023 •
edited
Loading

Bevy version

0.10.1 but I'm pretty sure it's the case for 0.11/main too

What you did

To opt-out of automatic Frustum definition, in bevy_basic_portals, I defined a custom component implementing the trait CameraProjection.
I added the plugin CameraProjectionPlugin::<PortalProjection>::default(), I voluntarily didn't add the system update_frusta::<PortalProjection>.
But I couldn't do anything about update_directional_light_cascades and extract_taa_settings because they're not generic over the projection.

How to trigger the bug

For directional light, predictively, if you have a DirectionalLight with shadows_enabled = true and a camera with a custom CameraProjection component, Bevy crashes.

More specifically, it happens for an entity with a Camera3d component but no Projection, could happen by using PerspectiveProjection or OrthographicProjection directly too, not using Camera3dBundle.

What happens exactly

What happens is that update_directional_light_cascades doesn't add our camera to Cascades's cascades (of type HashMap<Entity, Vec<Cascade>>, the entities are cameras), this cascades is given to ExtractedDirectionalLight, then in prepare_lights (line 1106 of bevy_pbr/src/render/light.rs on main), it tries to get our camera from this hashset, unwraps, and panics.

A similar thing might happen for TAA, since extract_taa_settings is not generic either, I didn't test it.

Why I think this should be fixed

Even if defining a custom projection is not something usual, it has its uses.
For my previous usage of defining a custom Frustum I could still use Bevy's Projection and just fight against update_frusta, but I also wanted to use a custom projection in the future of bevy_basic_portals to define a projection to the portal destination directly instead of doing the "render to image + masking" technique, I will need a custom projection for that.
Bevy is supposed to fit all usages and be modular.

How to fix it

Here are some possibilities/thoughts to fix this:

  1. add a generic method to calculate the corners of the Frustum in the trait CameraProjection to do it generically in update_directional_light_cascades and replace frustum_corners/frustum_corners_ortho
  2. Make update_directional_light_cascades generic like update_frusta and camera_system, this is a bit annoying since this system clears the Cascades, but it may be possible to get around this using a resource to only clear once per tick or a different system for clearing which wouldn't be generic and run before update_directional_light_cascades.
  3. Make PbrPlugin (which adds update_directional_light_cascades) generic like CameraProjectionPlugin? Probably a bad idea, but maybe it can be possible to make a sub-plugin that would be generic to add update_directional_light_cascades, but it's probably easier for people who want a custom projection to add update_directional_light_cascades directly anyway (?)
  4. Instead of adding a component that implements CameraProjection directly, we could add a component which is a box of a CameraProjection, then do everything on that component, this would remove the need to make all the systems generic over CameraProjection and then having to add the systems for every kind of projection one by one, at the cost of doing dynamic typing (which is probably less optimized)
  5. Same as 4., but only for custom projections, like either you use Bevy's default projection, or you use a custom projection in a box
  6. Don't do an unwrap in prepare_lights. If no entity is found, just ignore it and continue.

I might edit later if I have other ideas.
I didn't check how TAA works.
I can probably do it myself if nobody else wants to but I would appreciate insights before jumping into the code.
@Selene-Amanita Selene-Amanita added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen P-Crash A sudden unexpected crash S-Needs-Design This issue requires design work to think about how it would best be accomplished labels Jul 8, 2023
@Selene-Amanita Selene-Amanita mentioned this issue Jul 10, 2023
Merged
@Selene-Amanita Selene-Amanita added this to the 0.12 milestone Jul 10, 2023
@Selene-Amanita Selene-Amanita mentioned this issue Jul 21, 2023
Merged
github-merge-queue bot pushed a commit that referenced this issue Aug 10, 2023
@Selene-Amanita
Update Camera's Frustum only when its GlobalTransform or `Camer…
77824b9 
…aProjection` changed (#9092)

# Objective

Update a camera's frustum only when needed.

- Maybe a performance gain from not having to compute frusta when not
needed, at the cost of change detection (?)
- Making "fighting" with `update_frusta` less tedious, see
#9077 and
https://discord.com/channels/691052431525675048/743663924229963868/1127566087966433322

## Solution

Add change detection filter for `GlobalTransform` or `T:
CameraProjection` in `update_frusta`, since those are the cases when the
frustum needs to be updated.

## Note

I don't think a migration guide and changelog are needed, but I'm not
100% sure, I could put something like "if you're fighting against
`update_frusta`, you can do it only when there is a change to
`GlobalTransform` or `CameraProjection` now", what do you think? It's
not really a breaking change with a normal use case.
cart pushed a commit that referenced this issue Aug 10, 2023
@Selene-Amanita @cart
Update Camera's Frustum only when its GlobalTransform or `Camer…
9d1a102 
…aProjection` changed (#9092)

# Objective

Update a camera's frustum only when needed.

- Maybe a performance gain from not having to compute frusta when not
needed, at the cost of change detection (?)
- Making "fighting" with `update_frusta` less tedious, see
#9077 and
https://discord.com/channels/691052431525675048/743663924229963868/1127566087966433322

## Solution

Add change detection filter for `GlobalTransform` or `T:
CameraProjection` in `update_frusta`, since those are the cases when the
frustum needs to be updated.

## Note

I don't think a migration guide and changelog are needed, but I'm not
100% sure, I could put something like "if you're fighting against
`update_frusta`, you can do it only when there is a change to
`GlobalTransform` or `CameraProjection` now", what do you think? It's
not really a breaking change with a normal use case.
@cart cart closed this as completed in #9226 Nov 3, 2023
github-merge-queue bot pushed a commit that referenced this issue Nov 3, 2023
@Selene-Amanita @cart
Make DirectionalLight Cascades computation generic over `CameraPr…
c376954 
…ojection` (#9226)

# Objective

Fixes #9077 (see this issue for
motivations)

## Solution

Implement 1 and 2 of the "How to fix it" section of
#9077

`update_directional_light_cascades` is split into
`clear_directional_light_cascades` and a generic
`build_directional_light_cascades`, to clear once and potentially insert
many times.

---

## Changelog

`DirectionalLight`'s computation is now generic over `CameraProjection`
and can work with custom camera projections.

## Migration Guide

If you have a component `MyCustomProjection` that implements
`CameraProjection`:
- You need to implement a new required associated method,
`get_frustum_corners`, returning an array of the corners of a subset of
the frustum with given `z_near` and `z_far`, in local camera space.
- You can now add the
`build_directional_light_cascades::<MyCustomProjection>` system in
`SimulationLightSystems::UpdateDirectionalLightCascades` after
`clear_directional_light_cascades` for your projection to work with
directional lights.

---------

Co-authored-by: Carter Anderson <[email protected]>
aevyrie added a commit to aevyrie/bevy that referenced this issue Nov 4, 2023
@aevyrie
Review feedback
1523014 
Fix branding inconsistencies

don't Implement `Display` for `Val` (bevyengine#10345)

- Revert bevyengine#10296

- Avoid implementing `Display` without a justification
- `Display` implementation is a guarantee without a direct use, takes
additional time to compile and require work to maintain
- `Debug`, `Reflect` or `Serialize` should cover all needs

Combine visibility queries in check_visibility_system (bevyengine#10196)

Alternative to bevyengine#7310

Implemented the suggestion from
bevyengine#7310 (comment)

I am guessing that these were originally split as an optimization, but I
am not sure since I believe the original author of the code is the one
speculating about combining them up there.

I ran three benchmarks to compare main, this PR, and the approach from
([updated](https://github.com/rparrett/bevy/commits/rebased-parallel-check-visibility)
to the same commit on main).

This seems to perform slightly better than main in scenarios where most
entities have AABBs, and a bit worse when they don't (`many_lights`).
That seems to make sense to me.

Either way, the difference is ~-20 microseconds in the more common
scenarios or ~+100 microseconds in the less common scenario. I would
speculate that this might perform **very slightly** worse in
single-threaded scenarios.

Benches were run in release mode for 2000 frames while capturing a trace
with tracy.

| bench | commit | check_visibility_system mean μs |
| -- | -- | -- |
| many_cubes | main | 929.5 |
| many_cubes | this | 914.0 |
| many_cubes | 7310 | 1003.5 |
| | |
| many_foxes | main | 191.6 |
| many_foxes | this | 173.2 |
| many_foxes | 7310 | 167.9 |
| | |
| many_lights | main | 619.3 |
| many_lights | this | 703.7 |
| many_lights | 7310 | 842.5 |

Technically this behaves slightly differently -- prior to this PR, view
visibility was determined even for entities without `GlobalTransform`. I
don't think this has any practical impact though.

IMO, I don't think we need to do this. But I opened a PR because it
seemed like the handiest way to share the code / benchmarks.

I have done some rudimentary testing with the examples above, but I can
do some screenshot diffing if it seems like we want to do this.

Make VERTEX_COLORS usable in prepass shader, if available (bevyengine#10341)

I was working with forward rendering prepass fragment shaders and ran
into an issue of not being able to access vertex colors in the prepass.
I was able to access vertex colors in regular fragment shaders as well
as in deferred shaders.

It seems like this `if` was nested unintentionally as moving it outside
of the `deferred` block works.

---

Enable vertex colors in forward rendering prepass fragment shaders

allow DeferredPrepass to work without other prepass markers (bevyengine#10223)

fix crash / misbehaviour when `DeferredPrepass` is used without
`DepthPrepass`.

- Deferred lighting requires the depth prepass texture to be present, so
that the depth texture is available for binding. without it the deferred
lighting pass will use 0 for depth of all meshes.
- When `DeferredPrepass` is used without other prepass markers, and with
any materials that use `OpaqueRenderMode::Forward`, those entities will
try to queue to the `Opaque3dPrepass` render phase, which doesn't exist,
causing a crash.

- check if the prepass phases exist before queueing
- generate prepass textures if `Opaque3dDeferred` is present
- add a note to the DeferredPrepass marker to note that DepthPrepass is
also required by the default deferred lighting pass
- also changed some `With<T>.is_some()`s to `Has<T>`s

UI batching Fix (bevyengine#9610)

Reimplement bevyengine#8793 on top of the recent rendering changes.

The batch creation logic is quite convoluted, but I tested it on enough
examples to convince myself that it works.

The initial value of `batch_image_handle` is changed from
`HandleId::Id(Uuid::nil(), u64::MAX)` to `DEFAULT_IMAGE_HANDLE.id()`,
which allowed me to make the if-block simpler I think.

The default image from `DEFAULT_IMAGE_HANDLE` is always inserted into
`UiImageBindGroups` even if it's not used. I tried to add a check so
that it would be only inserted when there is only one batch using the
default image but this crashed.

---

`prepare_uinodes`
* Changed the initial value of `batch_image_handle` to
`DEFAULT_IMAGE_HANDLE.id()`.
* The default image is added to the UI image bind groups before
assembling the batches.
* A new `UiBatch` isn't created when the next `ExtractedUiNode`s image
is set to `DEFAULT_IMAGE_HANDLE` (unless it is the first item in the UI
phase items list).

Increase default normal bias to avoid common artifacts (bevyengine#10346)

Bevy's default bias values for directional and spot lights currently
cause significant artifacts. We should fix that so shadows look good by
default!

This is a less controversial/invasive alternative to bevyengine#10188, which might
enable us to keep the default bias value low, but also has its own sets
of concerns and caveats that make it a risky choice for Bevy 0.12.

Bump the default normal bias from `0.6` to `1.8`. There is precedent for
values in this general area as Godot has a default normal bias of `2.0`.

![image](https://github.com/superdump/bevy/assets/2694663/a5828011-33fc-4427-90ed-f093d7389053)

![image](https://github.com/bevyengine/bevy/assets/2694663/0f2b16b0-c116-41ab-9886-1ace9e00efd6)

The default `shadow_normal_bias` value for `DirectionalLight` and
`SpotLight` has changed to accommodate artifacts introduced with the new
shadow PCF changes. It is unlikely (especially given the new PCF shadow
behaviors with these values), but you might need to manually tweak this
value if your scene requires a lower bias and it relied on the previous
default value.

Make `DirectionalLight` `Cascades` computation generic over `CameraProjection` (bevyengine#9226)

Fixes bevyengine#9077 (see this issue for
motivations)

Implement 1 and 2 of the "How to fix it" section of
bevyengine#9077

`update_directional_light_cascades` is split into
`clear_directional_light_cascades` and a generic
`build_directional_light_cascades`, to clear once and potentially insert
many times.

---

`DirectionalLight`'s computation is now generic over `CameraProjection`
and can work with custom camera projections.

If you have a component `MyCustomProjection` that implements
`CameraProjection`:
- You need to implement a new required associated method,
`get_frustum_corners`, returning an array of the corners of a subset of
the frustum with given `z_near` and `z_far`, in local camera space.
- You can now add the
`build_directional_light_cascades::<MyCustomProjection>` system in
`SimulationLightSystems::UpdateDirectionalLightCascades` after
`clear_directional_light_cascades` for your projection to work with
directional lights.

---------

Co-authored-by: Carter Anderson <[email protected]>

Update default `ClearColor` to better match Bevy's branding (bevyengine#10339)

- Changes the default clear color to match the code block color on
Bevy's website.

- Changed the clear color, updated text in examples to ensure adequate
contrast. Inconsistent usage of white text color set to use the default
color instead, which is already white.
- Additionally, updated the `3d_scene` example to make it look a bit
better, and use bevy's branding colors.

![image](https://github.com/bevyengine/bevy/assets/2632925/540a22c0-826c-4c33-89aa-34905e3e313a)

Corrected incorrect doc comment on read_asset_bytes (bevyengine#10352)

Fixes bevyengine#10302

- Removed the incorrect comment.

Allow AccessKit to react to WindowEvents before they reach the engine (bevyengine#10356)

- Adopt bevyengine#10239 to get it in time for the release
- Fix accessibility on macOS and linux

- call `on_event` from AcccessKit adapter on winit events

---------

Co-authored-by: Nolan Darilek <[email protected]>
Co-authored-by: Alice Cecile <[email protected]>
Co-authored-by: Alice Cecile <[email protected]>

Fix typo in window.rs (bevyengine#10358)

Fixes a small typo in `bevy_window/src/window.rs`

Change `Should be used instead 'scale_factor' when set.` to `Should be
used instead of 'scale_factor' when set.`

Add UI Materials (bevyengine#9506)

- Add Ui Materials so that UI can render more complex and animated
widgets.
- Fixes bevyengine#5607

- Create a UiMaterial trait for specifying a Shader Asset and Bind Group
Layout/Data.
- Create a pipeline for rendering these Materials inside the Ui
layout/tree.
- Create a MaterialNodeBundle for simple spawning.

- Created a `UiMaterial` trait for specifying a Shader asset and Bind
Group.
- Created a `UiMaterialPipeline` for rendering said Materials.
- Added Example [`ui_material`
](https://github.com/MarkusTheOrt/bevy/blob/ui_material/examples/ui/ui_material.rs)
for example usage.
- Created
[`UiVertexOutput`](https://github.com/MarkusTheOrt/bevy/blob/ui_material/crates/bevy_ui/src/render/ui_vertex_output.wgsl)
export as VertexData for shaders.
- Created
[`material_ui`](https://github.com/MarkusTheOrt/bevy/blob/ui_material/crates/bevy_ui/src/render/ui_material.wgsl)
shader as default for both Vertex and Fragment shaders.

---------

Co-authored-by: ickshonpe <[email protected]>
Co-authored-by: François <[email protected]>

support file operations in single threaded context (bevyengine#10312)

- Fixes bevyengine#10209
- Assets should work in single threaded

- In single threaded mode, don't use `async_fs` but fallback on
`std::fs` with a thin layer to mimic the async API
- file `file_asset.rs` is the async imps from `mod.rs`
- file `sync_file_asset.rs` is the same with `async_fs` APIs replaced by
`std::fs`
- which module is used depends on the `multi-threaded` feature

---------

Co-authored-by: Carter Anderson <[email protected]>

Fix gizmo crash when prepass enabled (bevyengine#10360)

- Fix gizmo crash when prepass enabled

- Add the prepass to the view key

Fixes: bevyengine#10347
ameknite pushed a commit to ameknite/bevy that referenced this issue Nov 6, 2023
@Selene-Amanita @cart @ameknite
Make DirectionalLight Cascades computation generic over `CameraPr…
e76cbb4 
…ojection` (bevyengine#9226)

# Objective

Fixes bevyengine#9077 (see this issue for
motivations)

## Solution

Implement 1 and 2 of the "How to fix it" section of
bevyengine#9077

`update_directional_light_cascades` is split into
`clear_directional_light_cascades` and a generic
`build_directional_light_cascades`, to clear once and potentially insert
many times.

---

## Changelog

`DirectionalLight`'s computation is now generic over `CameraProjection`
and can work with custom camera projections.

## Migration Guide

If you have a component `MyCustomProjection` that implements
`CameraProjection`:
- You need to implement a new required associated method,
`get_frustum_corners`, returning an array of the corners of a subset of
the frustum with given `z_near` and `z_far`, in local camera space.
- You can now add the
`build_directional_light_cascades::<MyCustomProjection>` system in
`SimulationLightSystems::UpdateDirectionalLightCascades` after
`clear_directional_light_cascades` for your projection to work with
directional lights.

---------

Co-authored-by: Carter Anderson <[email protected]>
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this issue Jan 9, 2024
@Selene-Amanita @cart
Make DirectionalLight Cascades computation generic over `CameraPr…
90eedea 
…ojection` (bevyengine#9226)

# Objective

Fixes bevyengine#9077 (see this issue for
motivations)

## Solution

Implement 1 and 2 of the "How to fix it" section of
bevyengine#9077

`update_directional_light_cascades` is split into
`clear_directional_light_cascades` and a generic
`build_directional_light_cascades`, to clear once and potentially insert
many times.

---

## Changelog

`DirectionalLight`'s computation is now generic over `CameraProjection`
and can work with custom camera projections.

## Migration Guide

If you have a component `MyCustomProjection` that implements
`CameraProjection`:
- You need to implement a new required associated method,
`get_frustum_corners`, returning an array of the corners of a subset of
the frustum with given `z_near` and `z_far`, in local camera space.
- You can now add the
`build_directional_light_cascades::<MyCustomProjection>` system in
`SimulationLightSystems::UpdateDirectionalLightCascades` after
`clear_directional_light_cascades` for your projection to work with
directional lights.

---------

Co-authored-by: Carter Anderson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior P-Crash A sudden unexpected crash S-Needs-Design This issue requires design work to think about how it would best be accomplished
Projects
None yet
Milestone
0.12
Development

Successfully merging a pull request may close this issue.

Make DirectionalLight Cascades computation generic over CameraProjection Selene-Amanita/bevy
1 participant
@Selene-Amanita