[Merged by Bors] - Always update clusters and remove per-frame allocations #4169

cart · 2022-03-10T05:03:01Z

Refactor assign_lights_to_clusters to always clear + update clusters, even if the screen size isn't available yet / is zero. This fixes multiple_windows example is broken #4167. We still avoid the "expensive" per-light work when the screen size isn't available yet. I also consolidated some logic to eliminate some redundancies.
Removed a ton of (potentially very large) per-frame reallocations
- Removed Res<VisiblePointLights> (a vec) in favor of Res<GlobalVisiblePointLights> (a hashmap). We were allocating a new hashmap every frame, the collecting it into a vec every frame, then in another system re-generating the hashmap. It is always used like a hashmap, might as well embrace that. We now reuse the same hashmap every frame and dont use any intermediate collections.
- We were re-allocating Clusters aabb and light vectors every frame by re-constructing Clusters every frame. We now re-use the existing collections.
- Reuse per-camera VisiblePointLight vecs when possible instead of allocating them every frame. We now only insert VisiblePointLights if the component doesn't exist yet.

alice-i-cecile · 2022-03-10T05:48:18Z

I'm curious about the perf impact; do we have a good benchmark for this?

robtfm

sorry for the regression. doing serial PRs on this area feels a bit like trying to replace the wheels on a car while driving it... but i hope i'll learn and improve as we go.

is it possible to get examples to run as part of CI?

crates/bevy_pbr/src/light.rs

bjorn3 · 2022-03-10T11:10:45Z

is it possible to get examples to run as part of CI?

Part of them already run using swiftshader.

superdump

Not a full review.

crates/bevy_pbr/src/light.rs

cart · 2022-03-10T20:49:42Z

I'm curious about the perf impact; do we have a good benchmark for this?

I benchmarked lighting.rs prior to submitting this pr and the difference was in the noise. But that makes sense because there aren't that many lights in the scene (and therefore we have a smaller number of allocations + copies). We'd want to test on a scene with a large number of lights.

…quested_cluster_dimensions

cart · 2022-03-10T23:23:26Z

I think this is good to go now. Feel free to leave more feedback, but I'll merge this in the next few days if i dont hear back from anyone.

robtfm · 2022-03-10T23:52:22Z

looks good. in testing i found an unrelated issue, if you want to include it that would be great, otherwise i'll open another pr.

the obb test is occasionally failing in Single mode due to f32 precision issues. we can fix it by removing the 1e9s and calculating the actual depth we need:

@@ -323,7 +323,7 @@ impl ClusterConfig {
     fn first_slice_depth(&self) -> f32 {
         match self {
             ClusterConfig::None => 0.0,
-            ClusterConfig::Single => 1.0e9, // FIXME note can't use f32::MAX as the aabb explodes
+            ClusterConfig::Single => 0.0,
             ClusterConfig::XYZ { z_config, .. } | ClusterConfig::FixedZ { z_config, .. } => {
                 z_config.first_slice_depth
             }
@@ -333,7 +333,7 @@ impl ClusterConfig {
     fn far_z_mode(&self) -> ClusterFarZMode {
         match self {
             ClusterConfig::None => ClusterFarZMode::Constant(0.0),
-            ClusterConfig::Single => ClusterFarZMode::Constant(1.0e9), // FIXME note can't use f32::MAX as the aabb explodes
+            ClusterConfig::Single => ClusterFarZMode::MaxLightRange,
             ClusterConfig::XYZ { z_config, .. } | ClusterConfig::FixedZ { z_config, .. } => {
                 z_config.far_z_mode
             }

superdump

LGTM. It would be good to add in robtfm's noted fix too.

superdump · 2022-03-23T00:20:21Z

@cart ping? I want to get this in as I have a couple more PRs that will come on top of this. I’ll add some many lights example while I’m at it I guess so we have something to test with. I’ve used Sponza locally but it’s not really necessary. :)

cart · 2022-03-24T00:07:01Z

Ill update this now and get it merged!

…ent-bug

cart · 2022-03-24T00:20:11Z

bors r+

* Refactor assign_lights_to_clusters to always clear + update clusters, even if the screen size isn't available yet / is zero. This fixes #4167. We still avoid the "expensive" per-light work when the screen size isn't available yet. I also consolidated some logic to eliminate some redundancies. * Removed _a ton_ of (potentially very large) per-frame reallocations * Removed `Res<VisiblePointLights>` (a vec) in favor of `Res<GlobalVisiblePointLights>` (a hashmap). We were allocating a new hashmap every frame, the collecting it into a vec every frame, then in another system _re-generating the hashmap_. It is always used like a hashmap, might as well embrace that. We now reuse the same hashmap every frame and dont use any intermediate collections. * We were re-allocating Clusters aabb and light vectors every frame by re-constructing Clusters every frame. We now re-use the existing collections. * Reuse per-camera VisiblePointLight vecs when possible instead of allocating them every frame. We now only insert VisiblePointLights if the component doesn't exist yet.

bors · 2022-03-24T00:34:46Z

Pull request successfully merged into main.

Build succeeded:

) * Refactor assign_lights_to_clusters to always clear + update clusters, even if the screen size isn't available yet / is zero. This fixes bevyengine#4167. We still avoid the "expensive" per-light work when the screen size isn't available yet. I also consolidated some logic to eliminate some redundancies. * Removed _a ton_ of (potentially very large) per-frame reallocations * Removed `Res<VisiblePointLights>` (a vec) in favor of `Res<GlobalVisiblePointLights>` (a hashmap). We were allocating a new hashmap every frame, the collecting it into a vec every frame, then in another system _re-generating the hashmap_. It is always used like a hashmap, might as well embrace that. We now reuse the same hashmap every frame and dont use any intermediate collections. * We were re-allocating Clusters aabb and light vectors every frame by re-constructing Clusters every frame. We now re-use the existing collections. * Reuse per-camera VisiblePointLight vecs when possible instead of allocating them every frame. We now only insert VisiblePointLights if the component doesn't exist yet.

Always update clusters and remove per-frame allocations

d9140f3

cart added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen labels Mar 10, 2022

github-actions bot added the S-Needs-Triage This issue needs to be labelled label Mar 10, 2022

cart removed the S-Needs-Triage This issue needs to be labelled label Mar 10, 2022

alice-i-cecile added the C-Performance A change motivated by improving speed, memory usage or compile times label Mar 10, 2022

robtfm reviewed Mar 10, 2022

View reviewed changes

crates/bevy_pbr/src/light.rs Outdated Show resolved Hide resolved

crates/bevy_pbr/src/light.rs Outdated Show resolved Hide resolved

crates/bevy_pbr/src/light.rs Outdated Show resolved Hide resolved

superdump reviewed Mar 10, 2022

View reviewed changes

crates/bevy_pbr/src/light.rs Show resolved Hide resolved

crates/bevy_pbr/src/light.rs Outdated Show resolved Hide resolved

crates/bevy_pbr/src/light.rs Show resolved Hide resolved

cart added 2 commits March 10, 2022 12:43

properly set clusters near and far

e401319

remove old todo

e86b529

cart added 2 commits March 10, 2022 15:10

clusters.axis_slices -> clusters.dimensions, cluster_dimensions -> re…

e8110d1

…quested_cluster_dimensions

Replace constructor with Default. Move debug_asserts

52b9e9b

superdump approved these changes Mar 11, 2022

View reviewed changes

cart added 2 commits March 23, 2022 17:18

Merge remote-tracking branch 'upstream/main' into fix-cluster-assignm…

2d4674b

…ent-bug

Fix ClusterConfig::Single behavior

787a788

bors bot changed the title ~~Always update clusters and remove per-frame allocations~~ [Merged by Bors] - Always update clusters and remove per-frame allocations Mar 24, 2022

bors bot closed this Mar 24, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Always update clusters and remove per-frame allocations #4169

[Merged by Bors] - Always update clusters and remove per-frame allocations #4169

cart commented Mar 10, 2022

alice-i-cecile commented Mar 10, 2022

robtfm left a comment

bjorn3 commented Mar 10, 2022

superdump left a comment •

edited

Loading

cart commented Mar 10, 2022

cart commented Mar 10, 2022

robtfm commented Mar 10, 2022

superdump left a comment

superdump commented Mar 23, 2022

cart commented Mar 24, 2022

cart commented Mar 24, 2022

bors bot commented Mar 24, 2022

[Merged by Bors] - Always update clusters and remove per-frame allocations #4169

[Merged by Bors] - Always update clusters and remove per-frame allocations #4169

Conversation

cart commented Mar 10, 2022

alice-i-cecile commented Mar 10, 2022

robtfm left a comment

Choose a reason for hiding this comment

bjorn3 commented Mar 10, 2022

superdump left a comment • edited Loading

Choose a reason for hiding this comment

cart commented Mar 10, 2022

cart commented Mar 10, 2022

robtfm commented Mar 10, 2022

superdump left a comment

Choose a reason for hiding this comment

superdump commented Mar 23, 2022

cart commented Mar 24, 2022

cart commented Mar 24, 2022

bors bot commented Mar 24, 2022

superdump left a comment •

edited

Loading