Skip to content

fix distinct directional lights per view #19147

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

robtfm
Copy link
Contributor

@robtfm robtfm commented May 9, 2025

Objective

after #15156 it seems like using distinct directional lights on different views is broken (and will probably break spotlights too). fix them

Solution

the reason is a bit hairy so with an example:

  • camera 0 on layer 0
  • camera 1 on layer 1
  • dir light 0 on layer 0 (2 cascades)
  • dir light 1 on layer 1 (2 cascades)

in render/lights.rs:

  • outside of any view loop,

    • we count the total number of shadow casting directional light cascades (4) and assign an incrementing depth_texture_base_index for each (0-1 for one light, 2-3 for the other, depending on iteration order) (line 1034)
    • allocate a texture array for the total number of cascades plus spotlight maps (4) (line 1106)
  • in the view loop, for directional lights we

    • skip lights that don't intersect on renderlayers (line 1440)
    • assign an incrementing texture layer to each light/cascade starting from 0 (resets to 0 per view) (assigning 0 and 1 each time for the 2 cascades of the intersecting light) (line 1509, init at 1421)

then in the rendergraph:

  • camera 0 renders the shadow map for light 0 to texture indices 0 and 1

  • camera 0 renders using shadows from the depth_texture_base_index (maybe 0-1, maybe 2-3 depending on the iteration order)

  • camera 1 renders the shadow map for light 1 to texture indices 0 and 1

  • camera 0 renders using shadows from the depth_texture_base_index (maybe 0-1, maybe 2-3 depending on the iteration order)

issues:

  • one of the views uses empty shadow maps (bug)
  • we allocated a texture layer per cascade per light, even though not all lights are used on all views (just inefficient)
  • I think we're allocating texture layers even for lights with shadows_enabled: false (just inefficient)

solution:

  • calculate upfront the view with the largest number of directional cascades
  • allocate this many layers (plus layers for spotlights) in the texture array
  • keep using texture layers 0..n in the per-view loop, but build GpuLights.gpu_directional_lights within the loop too so it refers to the same layers we render to

nice side effects:

  • we can now use max_texture_array_layers / MAX_CASCADES_PER_LIGHT shadow-casting directional lights per view, rather than overall.
  • we can remove the GpuDirectionalLight::skip field, since the gpu lights struct is constructed per view

a simpler approach would be to keep everything the same, and just increment the texture layer index in the view loop even for non-intersecting lights. this pr reduces the total shadowmap vram used as well and isn't much extra complexity. but if we want something less risky/intrusive for 16.1 that would be the way.

Testing

i edited the split screen example to put separate lights on layer 1 and layer 2, and put the plane and fox on both layers (using lots of unrelated code for render layer propagation from #17575).
without the fix the directional shadows will only render on one of the top 2 views even though there are directional lights on both layers.

//! Renders two cameras to the same window to accomplish "split screen".

use std::f32::consts::PI;

use bevy::{
    pbr::CascadeShadowConfigBuilder, prelude::*, render::camera::Viewport, window::WindowResized,
};
use bevy_render::view::RenderLayers;

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .add_plugins(HierarchyPropagatePlugin::<RenderLayers>::default())
        .add_systems(Startup, setup)
        .add_systems(Update, (set_camera_viewports, button_system))
        .run();
}

/// set up a simple 3D scene
fn setup(
    mut commands: Commands,
    asset_server: Res<AssetServer>,
    mut meshes: ResMut<Assets<Mesh>>,
    mut materials: ResMut<Assets<StandardMaterial>>,
) {
    let all_layers = RenderLayers::layer(1).with(2).with(3).with(4);

    // plane
    commands.spawn((
        Mesh3d(meshes.add(Plane3d::default().mesh().size(100.0, 100.0))),
        MeshMaterial3d(materials.add(Color::srgb(0.3, 0.5, 0.3))),
        all_layers.clone()
    ));

    commands.spawn((
        SceneRoot(
            asset_server.load(GltfAssetLabel::Scene(0).from_asset("models/animated/Fox.glb")),
        ),
        Propagate(all_layers.clone()),
    ));

    // Light
    commands.spawn((
        Transform::from_rotation(Quat::from_euler(EulerRot::ZYX, 0.0, 1.0, -PI / 4.)),
        DirectionalLight {
            shadows_enabled: true,
            ..default()
        },
        CascadeShadowConfigBuilder {
            num_cascades: if cfg!(all(
                feature = "webgl2",
                target_arch = "wasm32",
                not(feature = "webgpu")
            )) {
                // Limited to 1 cascade in WebGL
                1
            } else {
                2
            },
            first_cascade_far_bound: 200.0,
            maximum_distance: 280.0,
            ..default()
        }
        .build(),
        RenderLayers::layer(1),
    ));

    commands.spawn((
        Transform::from_rotation(Quat::from_euler(EulerRot::ZYX, 0.0, 1.0, -PI / 4.)),
        DirectionalLight {
            shadows_enabled: true,
            ..default()
        },
        CascadeShadowConfigBuilder {
            num_cascades: if cfg!(all(
                feature = "webgl2",
                target_arch = "wasm32",
                not(feature = "webgpu")
            )) {
                // Limited to 1 cascade in WebGL
                1
            } else {
                2
            },
            first_cascade_far_bound: 200.0,
            maximum_distance: 280.0,
            ..default()
        }
        .build(),
        RenderLayers::layer(2),
    ));

    // Cameras and their dedicated UI
    for (index, (camera_name, camera_pos)) in [
        ("Player 1", Vec3::new(0.0, 200.0, -150.0)),
        ("Player 2", Vec3::new(150.0, 150., 50.0)),
        ("Player 3", Vec3::new(100.0, 150., -150.0)),
        ("Player 4", Vec3::new(-100.0, 80., 150.0)),
    ]
    .iter()
    .enumerate()
    {
        let camera = commands
            .spawn((
                Camera3d::default(),
                Transform::from_translation(*camera_pos).looking_at(Vec3::ZERO, Vec3::Y),
                Camera {
                    // Renders cameras with different priorities to prevent ambiguities
                    order: index as isize,
                    ..default()
                },
                CameraPosition {
                    pos: UVec2::new((index % 2) as u32, (index / 2) as u32),
                },
                RenderLayers::layer(index+1)
            ))
            .id();

        // Set up UI
        commands
            .spawn((
                UiTargetCamera(camera),
                Node {
                    width: Val::Percent(100.),
                    height: Val::Percent(100.),
                    ..default()
                },
            ))
            .with_children(|parent| {
                parent.spawn((
                    Text::new(*camera_name),
                    Node {
                        position_type: PositionType::Absolute,
                        top: Val::Px(12.),
                        left: Val::Px(12.),
                        ..default()
                    },
                ));
                buttons_panel(parent);
            });
    }

    fn buttons_panel(parent: &mut ChildSpawnerCommands) {
        parent
            .spawn(Node {
                position_type: PositionType::Absolute,
                width: Val::Percent(100.),
                height: Val::Percent(100.),
                display: Display::Flex,
                flex_direction: FlexDirection::Row,
                justify_content: JustifyContent::SpaceBetween,
                align_items: AlignItems::Center,
                padding: UiRect::all(Val::Px(20.)),
                ..default()
            })
            .with_children(|parent| {
                rotate_button(parent, "<", Direction::Left);
                rotate_button(parent, ">", Direction::Right);
            });
    }

    fn rotate_button(parent: &mut ChildSpawnerCommands, caption: &str, direction: Direction) {
        parent
            .spawn((
                RotateCamera(direction),
                Button,
                Node {
                    width: Val::Px(40.),
                    height: Val::Px(40.),
                    border: UiRect::all(Val::Px(2.)),
                    justify_content: JustifyContent::Center,
                    align_items: AlignItems::Center,
                    ..default()
                },
                BorderColor(Color::WHITE),
                BackgroundColor(Color::srgb(0.25, 0.25, 0.25)),
            ))
            .with_children(|parent| {
                parent.spawn(Text::new(caption));
            });
    }
}

#[derive(Component)]
struct CameraPosition {
    pos: UVec2,
}

#[derive(Component)]
struct RotateCamera(Direction);

enum Direction {
    Left,
    Right,
}

fn set_camera_viewports(
    windows: Query<&Window>,
    mut resize_events: EventReader<WindowResized>,
    mut query: Query<(&CameraPosition, &mut Camera)>,
) {
    // We need to dynamically resize the camera's viewports whenever the window size changes
    // so then each camera always takes up half the screen.
    // A resize_event is sent when the window is first created, allowing us to reuse this system for initial setup.
    for resize_event in resize_events.read() {
        let window = windows.get(resize_event.window).unwrap();
        let size = window.physical_size() / 2;

        for (camera_position, mut camera) in &mut query {
            camera.viewport = Some(Viewport {
                physical_position: camera_position.pos * size,
                physical_size: size,
                ..default()
            });
        }
    }
}

fn button_system(
    interaction_query: Query<
        (&Interaction, &ComputedNodeTarget, &RotateCamera),
        (Changed<Interaction>, With<Button>),
    >,
    mut camera_query: Query<&mut Transform, With<Camera>>,
) {
    for (interaction, computed_target, RotateCamera(direction)) in &interaction_query {
        if let Interaction::Pressed = *interaction {
            // Since TargetCamera propagates to the children, we can use it to find
            // which side of the screen the button is on.
            if let Some(mut camera_transform) = computed_target
                .camera()
                .and_then(|camera| camera_query.get_mut(camera).ok())
            {
                let angle = match direction {
                    Direction::Left => -0.1,
                    Direction::Right => 0.1,
                };
                camera_transform.rotate_around(Vec3::ZERO, Quat::from_axis_angle(Vec3::Y, angle));
            }
        }
    }
}








use std::marker::PhantomData;

use bevy::{
    app::{App, Plugin, Update},
    ecs::query::QueryFilter,
    prelude::{
        Changed, Children, Commands, Component, Entity, Local, Query,
        RemovedComponents, SystemSet, With, Without,
    },
};

/// Causes the inner component to be added to this entity and all children.
/// A child with a Propagate<C> component of it's own will override propagation from
/// that point in the tree
#[derive(Component, Clone, PartialEq)]
pub struct Propagate<C: Component + Clone + PartialEq>(pub C);

/// Internal struct for managing propagation
#[derive(Component, Clone, PartialEq)]
pub struct Inherited<C: Component + Clone + PartialEq>(pub C);

/// Stops the output component being added to this entity.
/// Children will still inherit the component from this entity or its parents
#[derive(Component, Default)]
pub struct PropagateOver<C: Component + Clone + PartialEq>(PhantomData<fn() -> C>);

/// Stops the propagation at this entity. Children will not inherit the component.
#[derive(Component, Default)]
pub struct PropagateStop<C: Component + Clone + PartialEq>(PhantomData<fn() -> C>);

pub struct HierarchyPropagatePlugin<C: Component + Clone + PartialEq, F: QueryFilter = ()> {
    _p: PhantomData<fn() -> (C, F)>,
}

impl<C: Component + Clone + PartialEq, F: QueryFilter> Default for HierarchyPropagatePlugin<C, F> {
    fn default() -> Self {
        Self {
            _p: Default::default(),
        }
    }
}

#[derive(SystemSet, Clone, PartialEq, PartialOrd, Ord)]
pub struct PropagateSet<C: Component + Clone + PartialEq> {
    _p: PhantomData<fn() -> C>,
}

impl<C: Component + Clone + PartialEq> std::fmt::Debug for PropagateSet<C> {
    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
        f.debug_struct("PropagateSet")
            .field("_p", &self._p)
            .finish()
    }
}

impl<C: Component + Clone + PartialEq> Eq for PropagateSet<C> {}
impl<C: Component + Clone + PartialEq> std::hash::Hash for PropagateSet<C> {
    fn hash<H: std::hash::Hasher>(&self, state: &mut H) {
        self._p.hash(state);
    }
}

impl<C: Component + Clone + PartialEq> Default for PropagateSet<C> {
    fn default() -> Self {
        Self {
            _p: Default::default(),
        }
    }
}

impl<C: Component + Clone + PartialEq, F: QueryFilter + 'static> Plugin
    for HierarchyPropagatePlugin<C, F>
{
    fn build(&self, app: &mut App) {
        app.add_systems(
            Update,
            (
                update_source::<C, F>,
                update_stopped::<C, F>,
                update_reparented::<C, F>,
                propagate_inherited::<C, F>,
                propagate_output::<C, F>,
            )
                .chain()
                .in_set(PropagateSet::<C>::default()),
        );
    }
}

pub fn update_source<C: Component + Clone + PartialEq, F: QueryFilter>(
    mut commands: Commands,
    changed: Query<(Entity, &Propagate<C>), (Changed<Propagate<C>>, Without<PropagateStop<C>>)>,
    mut removed: RemovedComponents<Propagate<C>>,
) {
    for (entity, source) in &changed {
        commands
            .entity(entity)
            .try_insert(Inherited(source.0.clone()));
    }

    for removed in removed.read() {
        if let Ok(mut commands) = commands.get_entity(removed) {
            commands.remove::<(Inherited<C>, C)>();
        }
    }
}

pub fn update_stopped<C: Component + Clone + PartialEq, F: QueryFilter>(
    mut commands: Commands,
    q: Query<Entity, (With<Inherited<C>>, F, With<PropagateStop<C>>)>,
) {
    for entity in q.iter() {
        let mut cmds = commands.entity(entity);
        cmds.remove::<Inherited<C>>();
    }
}

pub fn update_reparented<C: Component + Clone + PartialEq, F: QueryFilter>(
    mut commands: Commands,
    moved: Query<
        (Entity, &ChildOf, Option<&Inherited<C>>),
        (
            Changed<ChildOf>,
            Without<Propagate<C>>,
            Without<PropagateStop<C>>,
            F,
        ),
    >,
    parents: Query<&Inherited<C>>,
) {
    for (entity, parent, maybe_inherited) in &moved {
        if let Ok(inherited) = parents.get(parent.parent()) {
            commands.entity(entity).try_insert(inherited.clone());
        } else if maybe_inherited.is_some() {
            commands.entity(entity).remove::<(Inherited<C>, C)>();
        }
    }
}

pub fn propagate_inherited<C: Component + Clone + PartialEq, F: QueryFilter>(
    mut commands: Commands,
    changed: Query<
        (&Inherited<C>, &Children),
        (Changed<Inherited<C>>, Without<PropagateStop<C>>, F),
    >,
    recurse: Query<
        (Option<&Children>, Option<&Inherited<C>>),
        (Without<Propagate<C>>, Without<PropagateStop<C>>, F),
    >,
    mut to_process: Local<Vec<(Entity, Option<Inherited<C>>)>>,
    mut removed: RemovedComponents<Inherited<C>>,
) {
    // gather changed
    for (inherited, children) in &changed {
        to_process.extend(
            children
                .iter()
                .map(|child| (child, Some(inherited.clone()))),
        );
    }

    // and removed
    for entity in removed.read() {
        if let Ok((Some(children), _)) = recurse.get(entity) {
            to_process.extend(children.iter().map(|child| (child, None)))
        }
    }

    // propagate
    while let Some((entity, maybe_inherited)) = (*to_process).pop() {
        let Ok((maybe_children, maybe_current)) = recurse.get(entity) else {
            continue;
        };

        if maybe_current == maybe_inherited.as_ref() {
            continue;
        }

        if let Some(children) = maybe_children {
            to_process.extend(
                children
                    .iter()
                    .map(|child| (child, maybe_inherited.clone())),
            );
        }

        if let Some(inherited) = maybe_inherited {
            commands.entity(entity).try_insert(inherited.clone());
        } else {
            commands.entity(entity).remove::<(Inherited<C>, C)>();
        }
    }
}

pub fn propagate_output<C: Component + Clone + PartialEq, F: QueryFilter>(
    mut commands: Commands,
    changed: Query<
        (Entity, &Inherited<C>, Option<&C>),
        (Changed<Inherited<C>>, Without<PropagateOver<C>>, F),
    >,
) {
    for (entity, inherited, maybe_current) in &changed {
        if maybe_current.is_some_and(|c| &inherited.0 == c) {
            continue;
        }

        commands.entity(entity).try_insert(inherited.0.clone());
    }
}

@robtfm robtfm added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen labels May 9, 2025
@alice-i-cecile alice-i-cecile added this to the 0.16.1 milestone May 9, 2025
@alice-i-cecile alice-i-cecile added the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label May 9, 2025
@alice-i-cecile
Copy link
Member

Maybe not in this PR, but can we add this to our rendering tests to catch regressions in this sort of setup in the future?

@mcobzarenco
Copy link
Contributor

mcobzarenco commented May 9, 2025

Amazing, not a review, but thank you for fixing this, I've noticed the issue too and I've been using only one directional light in 0.16 as a workaround.

@mcobzarenco
Copy link
Contributor

mcobzarenco commented May 9, 2025

Maybe not in this PR, but can we add this to our rendering tests to catch regressions in this sort of setup in the future?

Not familiar with CI infra for screenshot tests, but if we take screenshots of the examples, could we keep these changes in:

i edited the split screen example to put separate lights on layer 1 and layer 2, and put the plane and fox on both layers (using lots of unrelated code for render layer propagation from #17575).
without the fix the directional shadows will only render on one of the top 2 views even though there are directional lights on both layers.

In which case we'd have a test for the issue

@robtfm
Copy link
Contributor Author

robtfm commented May 9, 2025

could we keep these changes in

I think I’d make a separate test, particularly since it introduces 100 lines of unrelated code that people looking into split screen won’t care about

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 S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

3 participants