search

bevyengine / bevy

A refreshingly simple data-driven game engine built in Rust
https://bevyengine.org
Apache License 2.0
35.17k stars 3.47k forks source link

Change Detection Logic Bug in Point/Spot Light Frustra Update #14959

Open coreh opened 2 weeks ago

coreh commented 2 weeks ago

Bevy version

e4b740840f17d0ff3fa15d979d0ed2c65fa693c2

What you did

Spawn point/spot lights with shadows enabled far away from the camera, and don't change their transform or light components. Move the camera closer to them.

What went wrong

They will have no shadows, because their frustra isn't updated.

Additional information

The issue is taking place here:

https://github.com/bevyengine/bevy/blob/36c6f29832ddeb2ce4880f2db90efde00f2609ee/crates/bevy_pbr/src/light/mod.rs#L565-L571

https://github.com/bevyengine/bevy/blob/36c6f29832ddeb2ce4880f2db90efde00f2609ee/crates/bevy_pbr/src/light/mod.rs#L609-L615

We rely on Or<Changed<Transform>, Changed<PointLight> to update the frustra of only lights that changed. However, we bail early here if the lights are not in any clusters:

https://github.com/bevyengine/bevy/blob/36c6f29832ddeb2ce4880f2db90efde00f2609ee/crates/bevy_pbr/src/light/mod.rs#L585

https://github.com/bevyengine/bevy/blob/36c6f29832ddeb2ce4880f2db90efde00f2609ee/crates/bevy_pbr/src/light/mod.rs#L622

Once they're added to the clusters, they end up not getting their frustra updated until they either move or have the light component changed.

Relevant discord conversation: https://discord.com/channels/691052431525675048/743663924229963868/1277413598859100223

Potential Solutions

  1. Remove the GlobalVisibleClusterableObjects check entirely. This will result in some light frustra being updated needlessly, but doing so should be fast enough that it won't be a problem realistically.
  2. Add a GloballyClustered component (or similar) that gets added/removed from the lights, and react to changes to that. This has performance implications due to archetype moves, and would go against the grain of moving rendering stuff out of components and into resources for perf.
  3. Add a ClusteringStatus component that is updated with the lights (not added/removed), and react to changes to that. This should give us the benefit of not having false updated, without the drawbacks of false positives, but will also imply in an extra component laying around for all lights, regardless of clustering status.
coreh commented 2 weeks ago

If we chose solution number 1, I can open a PR immediately, as I already have it working locally. Either way these are low complexity.

alice-i-cecile commented 2 weeks ago

Solution 1 is my preference here: simplicity wins over unwarranted optimization.

JMS55 commented 2 weeks ago

I actually think 2. or 3. wouldn't be bad either. We can use sparse components that won't affect the archetype, no?

alice-i-cecile commented 2 weeks ago

Yeah, sparse set components should be faster here now.