Render point and spot light shadow maps only once, regardless of the number of cameras.#23713
Conversation
pcwalton
added
the
P-Regression
pcwalton
added
C-Bug
number of cameras. The intention has long been to render shadow maps for point and spot lights only once, regardless of the number of views. This is reflected in the fact that `RetainedViewEntity::auxiliary_entity` is `Entity::PLACEHOLDER` for them. Unfortunately, this is currently inconsistently implemented, and a separate `ExtractedView` is presently spawned and rendered to for every point and spot light shadow map. The behavior of these views is inconsistent because they violate the invariant that there must only be one render-world view per `RetainedViewEntity`. This patch changes Bevy's behavior to spawn only one `ExtractedView` for point and spot lights. This required some significant rearchitecting of the render schedule because the render schedule is currently driven off cameras. Driving the rendering off cameras is incorrect for point and spot light shadow maps, which aren't associated with any camera. This PR fixes the regression on the `render_layers` test in `testbed_3d` in PR bevyengine#23481, in that it renders the way it rendered before that PR. Note, however, that the rendering isn't what may have been intended: the shadows don't match the visible objects. That's because the shadows come from point lights, which aren't associated with cameras, and therefore shadows are rendered using the default set of `RenderLayers`. A future patch may want to add flags to cameras that specify that they should have their own point light and spot light shadow maps that inherit the render layer (and HLOD) behavior of their associated cameras. As this patch is fairly large, though, and because my immediate goal is to fix the regression in bevyengine#23481, I think those flags are best implemented in a follow-up.
pcwalton
force-pushed
the
only-render-point-and-spot-light-shadows-once
branch
from
51e96a2 to
87971f0
Compare
|
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! 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. |
1 similar comment
|
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! 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. |
…-spot-light-shadows-once
|
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! 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. |
|
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! 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. |
pcwalton
added
the
M-Deliberate-Rendering-Change
atlv24
left a comment
atlv24
left a comment
There was a problem hiding this comment.
Tthanks for fixing this! I think we should explore moving from camera-driven rendering to view-driven rendering. RootNonCameraView is kinda weird and hacky, the fact that we need this is evidence that our current abstraction is not very good i feel.
| shadow_render_phases.prepare_for_new_frame( | ||
| retained_view_entity, | ||
| gpu_preprocessing_support.max_supported_mode, | ||
| ); | ||
| live_shadow_mapping_lights.insert(retained_view_entity); |
There was a problem hiding this comment.
This is already done unconditionally below, right?
| shadow_render_phases.prepare_for_new_frame( | |
| retained_view_entity, | |
| gpu_preprocessing_support.max_supported_mode, | |
| ); | |
| live_shadow_mapping_lights.insert(retained_view_entity); |
There was a problem hiding this comment.
Removed the duplicated code.
| // We couldn't fetch the material, probably because the | ||
| // material hasn't been loaded yet. Add the entity to | ||
| // the list of pending shadows and bail. | ||
| view_pending_shadow_queues | ||
| .current_frame | ||
| .insert((*render_entity, *visible_entity)); | ||
| continue; | ||
| }; | ||
|
|
||
| let Some(mesh_instance) = | ||
| render_mesh_instances.render_mesh_queue_data(*visible_entity) | ||
| else { | ||
| // We couldn't fetch the mesh, probably because it | ||
| // hasn't loaded yet. Add the entity to the list of | ||
| // pending shadows and bail. | ||
| view_pending_shadow_queues | ||
| .current_frame | ||
| .insert((*render_entity, *visible_entity)); | ||
| continue; | ||
| }; | ||
| let Some(material) = render_materials.get(material_instance.asset_id) else { | ||
| // We couldn't fetch the material, probably because the | ||
| // material hasn't been loaded yet. Add the entity to | ||
| // the list of pending shadows and bail. | ||
| view_pending_shadow_queues | ||
| .current_frame | ||
| .insert((*render_entity, *visible_entity)); | ||
| continue; | ||
| }; | ||
| if !material.properties.shadows_enabled { | ||
| // If the material is not a shadow caster, we don't need to specialize it. | ||
| continue; | ||
| } | ||
| if !mesh_instance | ||
| .flags() | ||
| .contains(RenderMeshInstanceFlags::SHADOW_CASTER) | ||
| { | ||
| continue; | ||
| } | ||
| let Some(mesh) = render_meshes.get(mesh_instance.mesh_asset_id()) else { | ||
| continue; | ||
| }; | ||
| let Some(mesh_instance) = | ||
| render_mesh_instances.render_mesh_queue_data(*visible_entity) | ||
| else { | ||
| // We couldn't fetch the mesh, probably because it | ||
| // hasn't loaded yet. Add the entity to the list of | ||
| // pending shadows and bail. | ||
| view_pending_shadow_queues | ||
| .current_frame | ||
| .insert((*render_entity, *visible_entity)); | ||
| continue; | ||
| }; | ||
| let Some(material) = render_materials.get(material_instance.asset_id) else { | ||
| // We couldn't fetch the material, probably because the | ||
| // material hasn't been loaded yet. Add the entity to | ||
| // the list of pending shadows and bail. |
There was a problem hiding this comment.
I know this is pre-existing code just re-indented, just wanna note that we should have this comment just once, at the top of these three blocks. This is quite repetitive
There was a problem hiding this comment.
I reworded the comment and also grepped for other instances of the same comment that were duplicated multiple times in the same place and replaced them too.
| @@ -108,6 +111,16 @@ impl Core2d { | |||
| #[derive(Resource)] | |||
There was a problem hiding this comment.
Driving the rendering off cameras is incorrect for point and spot light shadow maps, which aren't associated with any camera.
This essentially means that if a camera doesn't "see" the same things as each individual light, then rendering will be broken (as-in rendering artifacts will be present). A single light on its own is not a "complete" model of what should be seen in the final render. If a Camera "sees" a light, it should render as configured according to the logical model:
- Define what is globally configured to be visible
- Filter that down to what "exists" on the various layers: lights, meshes, etc
- Define what layers a given camera "sees"
- Produce the final render
I'll note that RenderLayers are currently our only tool for drawing two isolated scenes in the same World. The RenderLayers behavior (as it was originally implemented), is how people will expect it to work, and how it works in other engines like Godot.
The onus should not be on content authors to do the manual work of ensuring that for a given camera, every light that contributes to its final render is manually configured with the exact same visibility configuration.
I see this change as a major functional step backward in the interest of defending an implementation that chose to ignore the logical model. This is not "more correct". In this new "lights (as in, high-level user-defined lights) are their own standalone view of the world", every single light needs to be manually configured to exactly match the scene that a given Camera sees, otherwise rendering errors will occur. And that will come at the cost of breaking rendering for every other Camera with a different view configuration.
This is not any more "configurable". It only introduces the ability for developers to accidentally introduce render breakage, forces them to solve the problem on a per-light basis, with the process being: (1) ensure that the light's render layer settings match the current camera's render layer settings, to avoid render artifacts (2) duplicate the light each time it has conflicting requirements across cameras.
Driving the rendering off cameras is incorrect for point and spot light shadow maps, which aren't associated with any camera
By forcing lights to be "blind" to what a given camera needs / have its own configuration, you actually force a tighter user-facing coupling of cameras to lights, and you make that the user's problem to solve.
I want a solid vision for the "visibility" UX here before moving forward with this.
I believe the motivation is largely performance oriented (ex: avoid re-rendering lights for cameras that "see" the same thing). From my perspective the correct fix is to continue to drive light views from cameras, but dedupe when we identify cases that can be shared.
There was a problem hiding this comment.
There's a misunderstanding here. The main motivation here isn't performance; it's correctness. Currently, our behavior is inconsistent. We render light shadow maps multiple times, because we drive render schedule systems off cameras. But we only bin once, because bins are driven off extracted views. In practice this means that things will randomly appear and disappear from shadow maps based on whichever view the binning got to first.
I would like to make the decision to render shadow maps per camera or not more configurable. Whether we use automatic configuration as you're in favor of or manual configuration isn't my primary concern at the moment. We can certainly switch to automatic configuration in the future. I'm just trying to get our behavior consistent, which in my view is a necessary prerequisite to making it configurable.
I'm working on mechanism here, not policy. The decision to render point light shadow maps only once was made a long time ago. I'm happy to change that, but we need the mechanism working properly first. The status quo is just broken and doesn't implement any semantics properly.
|
Since this touches point and spot light shadows, I was wondering the effect it would have on #23769. Unfortunately it does not fix it. I applied the change I have in my draft fix (#23790 but it’s probably not the best or correct fix, so take this statement with a grain of salt!) on top of this, and it seems this PR breaks the shadows when toggling to spotlight. Caveat: I haven’t read in depth if I need to add anything extra to what I’ve drafted based on changes in this PR so there’s a second grain of salt) |
tychedelia
left a comment
tychedelia
left a comment
There was a problem hiding this comment.
I agree that I don't like RootNonCameraView. I concur with @atlv24 that we should have a more holistic change here to generalize views by demoting cameras.
I also really would like us to follow up with options to un-break multi-cam with render layers. I think this is the right call for fixing the regression but I would like to flag that as another critical follow-up.
I think restoring the previous behavior is important but that this leaves us in a worse state overall. I'm going to approve in favor of moving 0.19 forward but really think this is a critical follow up.
|
I fully agree that the current model isn't good. This is basically the minimum possible change I could think of in order to make our semantics consistent. That is, I wanted to get things into a non-broken state as quickly as I could, and even then this patch is big. We should fix this in the future, and also come up with a sensible policy as @cart wishes. |
atlv24
added
S-Ready-For-Final-Review
…-spot-light-shadows-once
alice-i-cecile
removed
the
S-Ready-For-Final-Review
alice-i-cecile
added
the
S-Waiting-on-Author
pcwalton
added
S-Needs-Review
|
I believe I've addressed all the review comments. |
kfc35
added
S-Ready-For-Final-Review
…lated components to SyncComponent for lights (bevyengine#23790) # Objective - Fixes bevyengine#23769 ! ## Solution I `git bisect`’d the issue to bbcc1e6 which has to do with `SyncComponent`s. The problem was that not all the relevant components were being despawned when toggling an entity between different Light components within the example. Expanding `SyncComponents` for each light ensures that the lights clean up properly between toggling. I also had to review bevyengine#23713 and I connected the dots that de-spawning the light should also despawn the light view entity. Duh! ## Testing - `cargo run --example pcss --features=“experimental_pbr_pcss”`: toggling works!
…lated components to SyncComponent for lights (bevyengine#23790) # Objective - Fixes bevyengine#23769 ! ## Solution I `git bisect`’d the issue to bbcc1e6 which has to do with `SyncComponent`s. The problem was that not all the relevant components were being despawned when toggling an entity between different Light components within the example. Expanding `SyncComponents` for each light ensures that the lights clean up properly between toggling. I also had to review bevyengine#23713 and I connected the dots that de-spawning the light should also despawn the light view entity. Duh! ## Testing - `cargo run --example pcss --features=“experimental_pbr_pcss”`: toggling works!
6 participants
The intention has long been to render shadow maps for point and spot lights only once, regardless of the number of views. This is reflected in the fact that
RetainedViewEntity::auxiliary_entityisEntity::PLACEHOLDERfor them. Unfortunately, this is currently inconsistently implemented, and a separateExtractedViewis presently spawned and rendered to for every point and spot light shadow map. The behavior of these views is inconsistent because they violate the invariant that there must only be one render-world view perRetainedViewEntity.This patch changes Bevy's behavior to spawn only one
ExtractedViewfor point and spot lights. This required some significant rearchitecting of the render schedule because the render schedule is currently driven off cameras. Driving the rendering off cameras is incorrect for point and spot light shadow maps, which aren't associated with any camera.This PR fixes the regression on the
render_layerstest intestbed_3din PR #23481, in that it renders the way it rendered before that PR. Note, however, that the rendering isn't what may have been intended: the shadows don't match the visible objects. That's because the shadows come from point lights, which aren't associated with cameras, and therefore shadows are rendered using the default set ofRenderLayers. A future patch may want to add flags to cameras that specify that they should have their own point light and spot light shadow maps that inherit the render layer (and HLOD) behavior of their associated cameras. As this patch is fairly large, though, and because my immediate goal is to fix the regression in #23481, I think those flags are best implemented in a follow-up.