Skip to content

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

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
Closed
dataphract wants to merge 1 commit into from
Closed

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

dataphract wants to merge 1 commit into from

Conversation

@dataphract
Copy link
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
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
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
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