Skip to content

Render point and spot light shadow maps only once, regardless of the number of cameras.#23713

Open
pcwalton wants to merge 4 commits intobevyengine:mainfrom
pcwalton:only-render-point-and-spot-light-shadows-once
Open

Render point and spot light shadow maps only once, regardless of the number of cameras.#23713
pcwalton wants to merge 4 commits intobevyengine:mainfrom
pcwalton:only-render-point-and-spot-light-shadows-once

Conversation

@pcwalton
Copy link
Copy Markdown
Contributor

@pcwalton pcwalton commented Apr 8, 2026

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 #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 #23481, I think those flags are best implemented in a follow-up.

Screenshot 2026-04-07 215221

@pcwalton pcwalton added the A-Rendering Drawing game state to the screen label Apr 8, 2026
@pcwalton pcwalton added the P-Regression Functionality that used to work but no longer does. Add a test for this! label Apr 8, 2026
@github-project-automation github-project-automation bot moved this to Needs SME Triage in Rendering Apr 8, 2026
@pcwalton pcwalton added C-Bug An unexpected or incorrect behavior S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Apr 8, 2026
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 pcwalton force-pushed the only-render-point-and-spot-light-shadows-once branch from 51e96a2 to 87971f0 Compare April 8, 2026 04:55
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 8, 2026

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!
You can review it at https://pixel-eagle.com/project/B04F67C0-C054-4A6F-92EC-F599FEC2FD1D?filter=PR-23713

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
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 8, 2026

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!
You can review it at https://pixel-eagle.com/project/B04F67C0-C054-4A6F-92EC-F599FEC2FD1D?filter=PR-23713

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.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 9, 2026

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!
You can review it at https://pixel-eagle.com/project/B04F67C0-C054-4A6F-92EC-F599FEC2FD1D?filter=PR-23713

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.

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Apr 9, 2026

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!
You can review it at https://pixel-eagle.com/project/B04F67C0-C054-4A6F-92EC-F599FEC2FD1D?filter=PR-23713

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 pcwalton added the M-Deliberate-Rendering-Change An intentional change to how tests and examples are rendered label Apr 9, 2026
Copy link
Copy Markdown
Contributor

@atlv24 atlv24 left a comment

Choose a reason for hiding this comment

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

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.

Comment on lines +1746 to +1750
shadow_render_phases.prepare_for_new_frame(
retained_view_entity,
gpu_preprocessing_support.max_supported_mode,
);
live_shadow_mapping_lights.insert(retained_view_entity);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is already done unconditionally below, right?

Suggested change
shadow_render_phases.prepare_for_new_frame(
retained_view_entity,
gpu_preprocessing_support.max_supported_mode,
);
live_shadow_mapping_lights.insert(retained_view_entity);

Comment on lines +2326 to +2349
// 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.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

@atlv24 atlv24 added this to the 0.19 milestone Apr 13, 2026
@@ -108,6 +111,16 @@ impl Core2d {
#[derive(Resource)]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

@kfc35
Copy link
Copy Markdown
Contributor

kfc35 commented Apr 13, 2026

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)

Copy link
Copy Markdown
Member

@tychedelia tychedelia left a comment

Choose a reason for hiding this comment

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

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.

@pcwalton
Copy link
Copy Markdown
Contributor Author

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 atlv24 added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Apr 13, 2026
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-Bug An unexpected or incorrect behavior M-Deliberate-Rendering-Change An intentional change to how tests and examples are rendered 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

Status: Needs SME Triage

Development

Successfully merging this pull request may close these issues.

5 participants