Skip to content

[Merged by Bors] - perf: only recalculate frusta of changed lights#4086

Closed
dataphract wants to merge 1 commit intobevyengine:mainfrom
dataphract:perf/changed-light-frusta
Closed

[Merged by Bors] - perf: only recalculate frusta of changed lights#4086
dataphract wants to merge 1 commit intobevyengine:mainfrom
dataphract:perf/changed-light-frusta

Conversation

@dataphract
Copy link
Copy Markdown
Member

@dataphract dataphract commented Mar 3, 2022

Objective

Currently, all directional and point lights have their viewing frusta recalculated every frame, even if they have not moved or been disabled/enabled.

Solution

The relevant systems now make use of change detection to only update those lights whose viewing frusta may have changed.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Mar 3, 2022
@james7132 james7132 added A-Rendering Drawing game state to the screen C-Performance A change motivated by improving speed, memory usage or compile times and removed S-Needs-Triage This issue needs to be labelled labels Mar 3, 2022
@alice-i-cecile alice-i-cecile 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 Mar 3, 2022
@james7132 james7132 requested a review from mockersf March 3, 2022 20:46
@dataphract dataphract force-pushed the perf/changed-light-frusta branch from e07fd6e to 516b2ef Compare March 7, 2022 22:44
@mockersf
Copy link
Copy Markdown
Member

mockersf commented Mar 8, 2022

bors r+

bors bot pushed a commit that referenced this pull request Mar 8, 2022
@dataphract
perf: only recalculate frusta of changed lights (#4086)
b4483db 
## Objective

Currently, all directional and point lights have their viewing frusta recalculated every frame, even if they have not moved or been disabled/enabled.

## Solution

The relevant systems now make use of change detection to only update those lights whose viewing frusta may have changed.
@bors
Copy link
Copy Markdown
Contributor

bors bot commented Mar 8, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title perf: only recalculate frusta of changed lights [Merged by Bors] - perf: only recalculate frusta of changed lights Mar 8, 2022
@bors bors bot closed this Mar 8, 2022
aevyrie pushed a commit to aevyrie/bevy that referenced this pull request Jun 7, 2022
@dataphract @aevyrie
perf: only recalculate frusta of changed lights (bevyengine#4086)
94a5a32 
## Objective

Currently, all directional and point lights have their viewing frusta recalculated every frame, even if they have not moved or been disabled/enabled.

## Solution

The relevant systems now make use of change detection to only update those lights whose viewing frusta may have changed.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
@dataphract @ItsDoot
perf: only recalculate frusta of changed lights (bevyengine#4086)
a81eba4 
## Objective

Currently, all directional and point lights have their viewing frusta recalculated every frame, even if they have not moved or been disabled/enabled.

## Solution

The relevant systems now make use of change detection to only update those lights whose viewing frusta may have changed.
@HugoPeters1024 HugoPeters1024 mentioned this pull request Mar 24, 2025
Closed
@HugoPeters1024
Copy link
Copy Markdown
Contributor

HugoPeters1024 commented Mar 24, 2025 •
edited
Loading

I believe this optimization was wrong for not considering that changes to global_light would interact with unchanged PointLights as well: #11682 (comment)

@dataphract do you remember how much extra performance this actually gives? Can you help me think of a potential solution perhaps?

EDIT: I believe I found a compromise that fixes the bug but keeps the performance benefit: #18519

@HugoPeters1024 HugoPeters1024 mentioned this pull request Mar 24, 2025
Merged
github-merge-queue bot pushed a commit that referenced this pull request Mar 25, 2025
@HugoPeters1024 @alice-i-cecile @JMS55
bugfix(frustra of point lights were not recalculated when a camera ch…
a58a8bd 
…anges) (#18519)

# Objective

- Fixes #11682

## Solution

- #4086 introduced an
optimization to not do redundant calculations, but did not take into
account changes to the resource `global_lights`. I believe that my patch
includes the optimization benefit but adds the required nuance to fix
said bug.

## Testing

The example originally given by
[@kirillsurkov](https://github.com/kirillsurkov) and then updated by me
to bevy 15.3 here:
#11682 (comment)
will not have shadows without this patch:

```rust
use bevy::prelude::*;

#[derive(Resource)]
struct State {
    x: f32,
}

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .add_systems(Startup, setup)
        .add_systems(Update, update)
        .insert_resource(State { x: -40.0 })
        .run();
}

fn setup(
    mut commands: Commands,
    mut meshes: ResMut<Assets<Mesh>>,
    mut materials: ResMut<Assets<StandardMaterial>>,
) {
    commands.spawn((
        Mesh3d(meshes.add(Circle::new(4.0))),
        MeshMaterial3d(materials.add(Color::WHITE)),
    ));
    commands.spawn((
        Mesh3d(meshes.add(Cuboid::new(1.0, 1.0, 1.0))),
        MeshMaterial3d(materials.add(Color::linear_rgb(0.0, 1.0, 0.0))),
    ));
    commands.spawn((
        PointLight {
            shadows_enabled: true,
            ..default()
        },
        Transform::from_xyz(4.0, 8.0, 4.0),
    ));
    commands.spawn(Camera3d::default());
}

fn update(mut state: ResMut<State>, mut camera: Query<&mut Transform, With<Camera3d>>) {
    let mut camera = camera.single_mut().unwrap();

    let t = Vec3::new(state.x, 0.0, 10.0);
    camera.translation = t;
    camera.look_at(t - Vec3::Z, Vec3::Y);

    state.x = 0.0;
}
```

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: JMS55 <47158642+JMS55@users.noreply.github.com>
mockersf pushed a commit that referenced this pull request Mar 25, 2025
@HugoPeters1024 @alice-i-cecile
@JMS55 @mockersf
bugfix(frustra of point lights were not recalculated when a camera ch…
df26bc3 
…anges) (#18519)

# Objective

- Fixes #11682

## Solution

- #4086 introduced an
optimization to not do redundant calculations, but did not take into
account changes to the resource `global_lights`. I believe that my patch
includes the optimization benefit but adds the required nuance to fix
said bug.

## Testing

The example originally given by
[@kirillsurkov](https://github.com/kirillsurkov) and then updated by me
to bevy 15.3 here:
#11682 (comment)
will not have shadows without this patch:

```rust
use bevy::prelude::*;

#[derive(Resource)]
struct State {
    x: f32,
}

fn main() {
    App::new()
        .add_plugins(DefaultPlugins)
        .add_systems(Startup, setup)
        .add_systems(Update, update)
        .insert_resource(State { x: -40.0 })
        .run();
}

fn setup(
    mut commands: Commands,
    mut meshes: ResMut<Assets<Mesh>>,
    mut materials: ResMut<Assets<StandardMaterial>>,
) {
    commands.spawn((
        Mesh3d(meshes.add(Circle::new(4.0))),
        MeshMaterial3d(materials.add(Color::WHITE)),
    ));
    commands.spawn((
        Mesh3d(meshes.add(Cuboid::new(1.0, 1.0, 1.0))),
        MeshMaterial3d(materials.add(Color::linear_rgb(0.0, 1.0, 0.0))),
    ));
    commands.spawn((
        PointLight {
            shadows_enabled: true,
            ..default()
        },
        Transform::from_xyz(4.0, 8.0, 4.0),
    ));
    commands.spawn(Camera3d::default());
}

fn update(mut state: ResMut<State>, mut camera: Query<&mut Transform, With<Camera3d>>) {
    let mut camera = camera.single_mut().unwrap();

    let t = Vec3::new(state.x, 0.0, 10.0);
    camera.translation = t;
    camera.look_at(t - Vec3::Z, Vec3::Y);

    state.x = 0.0;
}
```

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
Co-authored-by: JMS55 <47158642+JMS55@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@superdump superdump superdump approved these changes

@james7132 james7132 james7132 approved these changes

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@mockersf mockersf mockersf approved these changes

Assignees

No one assigned

Labels

A-Rendering Drawing game state to the screen C-Performance A change motivated by improving speed, memory usage or compile times 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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@dataphract @mockersf @HugoPeters1024 @superdump @james7132 @alice-i-cecile