[Merged by Bors] - Dynamic light clusters #3968

robtfm · 2022-02-17T11:52:47Z

Objective

provide some customisation for default cluster setup
avoid "cluster index lists is full" in all cases (using a strategy outlined by @superdump)

Solution

Add ClusterConfig enum (which can be inserted into a view at any time) to allow specifying cluster setup with variants:

None (do not do any light assignment - for views which do not require light info, e.g. minimaps etc)
Single (one cluster)
XYZ (explicit cluster counts in each dimension)
FixedZ (most similar to current - specify Z-slices and total, then x and y counts are dynamically determined to give approximately square clusters based on current aspect ratio)
Defaults to FixedZ { total: 4096, z: 24 } which is similar to the current setup.

Per frame, estimate the number of indices that would be required for the current config and decrease the cluster counts / increase the cluster sizes in the x and y dimensions if the index list would be too small.

notes:

I didn't put ClusterConfig in the camera bundles to avoid introducing a dependency from bevy_render to bevy_pbr. the ClusterConfig enum comes with a pbr-centric impl block so i didn't want to move that into bevy_render either.
~~Might want to add None variant to cluster config for views that don't care about lights?~~
Not well tested for orthographic
~~there's a cluster_muck branch on my repo which includes some diagnostics / a modified lighting example which may be useful for tyre-kicking~~ (outdated, i will bring it up to date if required)

anecdotal timings:

FPS on the lighting demo is negligibly better (~5%), maybe due to a small optimisation constraining the light aabb to be in front of the camera
FPS on the lighting demo with 100 extra lights added is ~33% faster, and also renders correctly as the cluster index count is no longer exceeded

superdump · 2022-02-17T15:09:41Z

With my Sponza 256 point light test scene, I used to not be able to use larger point light ranges, so I used 1m. With that configuration I observe no significant loss in performance just looking at FPS. If I used light ranges of 2m, I'd hit the cluster light limits. Now I can increase it to 2m and it will continue to work. The frame rate drops significantly but then more lights are affecting more fragments on the screen so that's not surprising. Here's a video with the cluster coherency debug enabled so you can see what is going on:
https://user-images.githubusercontent.com/302146/154508450-310d81fe-eea1-4a79-b859-1619a24e482e.mp4

Chatting on Discord, @robtfm suggested bringing the camera far plane closer. I noted that the camera far plane made sense to use as the far bound of the furthest depth slice, but it only has to be as far as the furthest light sphere. We could evaluate the near/far based on where the closest/furthest light sphere bounds are and then get better use of the available clusters which should improve performance a bit. Unless it hurts the performance due to causing more light sphere - cluster intersections. But that can probably be optimised some more.

robtfm · 2022-02-19T01:17:59Z

brought in the optimisations from [Merged by Bors] - bevy_pbr: Optimize assign_lights_to_clusters #3984
added some further config options on depths so we can specify the first slice depth, and choose between max light depth / camera far plane / constant for the far Z-depth.
also tweaked the estimate slightly and added some more comments.
made a bit of a mess with all the commits... not used to github yet, sorry

superdump

I left some suggestions for improvements to the documentation so that they show up in rustdoc output. I'll go over the code changes more thoroughly when #3916 and #3984 have been merged.

crates/bevy_pbr/src/light.rs

Co-authored-by: Robert Swain <robert.swain@gmail.com>

superdump

Adding a suggestion for top-down games.

crates/bevy_pbr/src/light.rs

Co-authored-by: Robert Swain <robert.swain@gmail.com>

superdump

The handling of and comments about view z for orthographic look wrong to me. It mostly looks good though. :)

superdump · 2022-03-05T15:33:37Z

crates/bevy_pbr/src/light.rs

@@ -479,6 +590,98 @@ fn ndc_position_to_cluster(
        .clamp(UVec3::ZERO, cluster_dimensions - UVec3::ONE)
 }

+// Calculate an AABB for the light in view space, returns a (Vec3, Vec3) containing min and max with
+// - x and y in view space with range [-1, 1]
+// - z in world space with view orientation, with range [-inf, -0.0001] for perspective, and [1.0000, inf] for orthographic


Suggested change

// - z in world space with view orientation, with range [-inf, -0.0001] for perspective, and [1.0000, inf] for orthographic

// z in view space with range [-inf, -f32::EPSILON] for perspective, and [1.0000, inf] for orthographic

Also, the difference in ranges between perspective and orthographic is confusing to me as the coordinate space axis orientation is the same (i.e. view z is not flipped.) View z in front of the camera is always negative.

worded the comment slightly differently, and removed the distinction for orthographic in this function.

crates/bevy_pbr/src/light.rs

superdump · 2022-03-05T19:00:16Z

crates/bevy_pbr/src/light.rs

+        // constraint z to be negative - i.e. in front of the camera
+        light_aabb_view_min.z = light_aabb_view_min.z.min(-0.0001);
+        light_aabb_view_max.z = light_aabb_view_max.z.min(-0.0001);


Suggested change

// constraint z to be negative - i.e. in front of the camera

light_aabb_view_min.z = light_aabb_view_min.z.min(-0.0001);

light_aabb_view_max.z = light_aabb_view_max.z.min(-0.0001);

// Constrain view z to be negative - i.e. in front of the camera

// When view z is >= 0.0 and we're using a perspective projection, bad things happen.

// At view z == 0.0, ndc x,y are mathematically undefined. At view z > 0.0, i.e. behind the camera,

// the perspective projection flips the directions of the axes. This breaks assumptions about

// use of min/max operations as something that was to the left in view space is now returning a

// coordinate that for view z in front of the camera would be on the right, but at view z behind the

// camera is on the left. So, we just constrain view z to be < 0.0 and necessarily in front of the camera.

light_aabb_view_min.z = light_aabb_view_min.z.min(-f32::EPSILON);

light_aabb_view_max.z = light_aabb_view_max.z.min(-f32::EPSILON);

This made sense after I'd understood (a tautology) but I wanted to see it in code too so I quickly hacked up this:

use bevy::{ math::{Vec3Swizzles, Vec4Swizzles}, prelude::*, }; fn main() { let view_pos = Vec3::new(-1.0, -1.0, -1.0); let projection = Mat4::perspective_infinite_reverse_rh(std::f32::consts::FRAC_PI_4, 1.0, 0.1); let in_front_clip = projection * view_pos.extend(1.0); let in_front_ndc = in_front_clip.xyz() / in_front_clip.w; let behind_clip = projection * view_pos.xy().extend(1.0).extend(1.0); let behind_ndc = behind_clip.xyz() / behind_clip.w; dbg!(in_front_clip); dbg!(in_front_ndc); dbg!(behind_clip); dbg!(behind_ndc); }

gives

[examples/3d/3d_scene.rs:13] in_front_clip = Vec4( -2.4142134, -2.4142134, 0.1, 1.0, ) [examples/3d/3d_scene.rs:14] in_front_ndc = Vec3( -2.4142134, -2.4142134, 0.1, ) [examples/3d/3d_scene.rs:15] behind_clip = Vec4( -2.4142134, -2.4142134, 0.1, -1.0, ) [examples/3d/3d_scene.rs:16] behind_ndc = Vec3( 2.4142134, 2.4142134, -0.1, )

superdump

Just one change left I think, depending on what you think.

crates/bevy_pbr/src/light.rs

Co-authored-by: Robert Swain <robert.swain@gmail.com>

cart

On first pass this looks reasonable. Haven't validated all of the math yet or tested on large scenes, but I'm on board with the concept and the general implementation.
I'll do another pass after lunch :)

crates/bevy_pbr/src/light.rs

cart · 2022-03-08T04:42:00Z

Looks good to me! Couldn't get it to produce anything "obviously wrong" when spamming a scene with various moving lights. Great job!

cart · 2022-03-08T04:42:05Z

bors r+

@superdump

# Objective provide some customisation for default cluster setup avoid "cluster index lists is full" in all cases (using a strategy outlined by @superdump) ## Solution Add ClusterConfig enum (which can be inserted into a view at any time) to allow specifying cluster setup with variants: - None (do not do any light assignment - for views which do not require light info, e.g. minimaps etc) - Single (one cluster) - XYZ (explicit cluster counts in each dimension) - FixedZ (most similar to current - specify Z-slices and total, then x and y counts are dynamically determined to give approximately square clusters based on current aspect ratio) Defaults to FixedZ { total: 4096, z: 24 } which is similar to the current setup. Per frame, estimate the number of indices that would be required for the current config and decrease the cluster counts / increase the cluster sizes in the x and y dimensions if the index list would be too small. notes: - I didn't put ClusterConfig in the camera bundles to avoid introducing a dependency from bevy_render to bevy_pbr. the ClusterConfig enum comes with a pbr-centric impl block so i didn't want to move that into bevy_render either. - ~Might want to add None variant to cluster config for views that don't care about lights?~ - Not well tested for orthographic - ~there's a cluster_muck branch on my repo which includes some diagnostics / a modified lighting example which may be useful for tyre-kicking~ (outdated, i will bring it up to date if required) anecdotal timings: FPS on the lighting demo is negligibly better (~5%), maybe due to a small optimisation constraining the light aabb to be in front of the camera FPS on the lighting demo with 100 extra lights added is ~33% faster, and also renders correctly as the cluster index count is no longer exceeded

bors · 2022-03-08T05:13:41Z

Pull request successfully merged into main.

Build succeeded:

@superdump

# Objective provide some customisation for default cluster setup avoid "cluster index lists is full" in all cases (using a strategy outlined by @superdump) ## Solution Add ClusterConfig enum (which can be inserted into a view at any time) to allow specifying cluster setup with variants: - None (do not do any light assignment - for views which do not require light info, e.g. minimaps etc) - Single (one cluster) - XYZ (explicit cluster counts in each dimension) - FixedZ (most similar to current - specify Z-slices and total, then x and y counts are dynamically determined to give approximately square clusters based on current aspect ratio) Defaults to FixedZ { total: 4096, z: 24 } which is similar to the current setup. Per frame, estimate the number of indices that would be required for the current config and decrease the cluster counts / increase the cluster sizes in the x and y dimensions if the index list would be too small. notes: - I didn't put ClusterConfig in the camera bundles to avoid introducing a dependency from bevy_render to bevy_pbr. the ClusterConfig enum comes with a pbr-centric impl block so i didn't want to move that into bevy_render either. - ~Might want to add None variant to cluster config for views that don't care about lights?~ - Not well tested for orthographic - ~there's a cluster_muck branch on my repo which includes some diagnostics / a modified lighting example which may be useful for tyre-kicking~ (outdated, i will bring it up to date if required) anecdotal timings: FPS on the lighting demo is negligibly better (~5%), maybe due to a small optimisation constraining the light aabb to be in front of the camera FPS on the lighting demo with 100 extra lights added is ~33% faster, and also renders correctly as the cluster index count is no longer exceeded

@superdump

# Objective provide some customisation for default cluster setup avoid "cluster index lists is full" in all cases (using a strategy outlined by @superdump) ## Solution Add ClusterConfig enum (which can be inserted into a view at any time) to allow specifying cluster setup with variants: - None (do not do any light assignment - for views which do not require light info, e.g. minimaps etc) - Single (one cluster) - XYZ (explicit cluster counts in each dimension) - FixedZ (most similar to current - specify Z-slices and total, then x and y counts are dynamically determined to give approximately square clusters based on current aspect ratio) Defaults to FixedZ { total: 4096, z: 24 } which is similar to the current setup. Per frame, estimate the number of indices that would be required for the current config and decrease the cluster counts / increase the cluster sizes in the x and y dimensions if the index list would be too small. notes: - I didn't put ClusterConfig in the camera bundles to avoid introducing a dependency from bevy_render to bevy_pbr. the ClusterConfig enum comes with a pbr-centric impl block so i didn't want to move that into bevy_render either. - ~Might want to add None variant to cluster config for views that don't care about lights?~ - Not well tested for orthographic - ~there's a cluster_muck branch on my repo which includes some diagnostics / a modified lighting example which may be useful for tyre-kicking~ (outdated, i will bring it up to date if required) anecdotal timings: FPS on the lighting demo is negligibly better (~5%), maybe due to a small optimisation constraining the light aabb to be in front of the camera FPS on the lighting demo with 100 extra lights added is ~33% faster, and also renders correctly as the cluster index count is no longer exceeded

robtfm added 5 commits February 16, 2022 16:27

variable x/y clusters

5009e1d

bug fixes

234ba1c

support orthographic

a4f5165

cleanup

9b4e34c

remove debugging changes

924ef46

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

robtfm added 4 commits February 17, 2022 12:04

clippy fix

88b84f0

clippy fix

1c73e44

improve estimate

9fbbfce

improve estimate

b142337

superdump added A-Rendering Drawing game state to the screen C-Feature A new feature, making something new possible C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed S-Needs-Triage This issue needs to be labelled labels Feb 17, 2022

robtfm added 7 commits February 18, 2022 14:48

more config options

c1a8cc4

improve estimate

416e258

improve estimate

d8f4216

optimisations from bevyengine#3984 + comments

fe967ae

more config options

7530323

Merge branch 'cluster_muck' into dynamic_clusters

6e2a7e8

format

0d97170

robtfm added 2 commits February 25, 2022 11:09

bug fixes, improve estimate

09fd4d0

add None config option

77a072b

superdump requested changes Feb 25, 2022

View reviewed changes

Apply suggestions from code review

019d25d

Co-authored-by: Robert Swain <robert.swain@gmail.com>

superdump requested changes Feb 25, 2022

View reviewed changes

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

robtfm and others added 2 commits February 25, 2022 14:42

Apply suggestions from code review

3782ecb

Co-authored-by: Robert Swain <robert.swain@gmail.com>

Merge branch 'main' into dynamic_clusters

47d91c8

robtfm added 3 commits March 4, 2022 14:53

format

6d235bc

Merge branch 'main' into dynamic_clusters

52f0281

clippy

d025799

superdump requested changes Mar 5, 2022

View reviewed changes

superdump reviewed Mar 5, 2022

View reviewed changes

apply review suggestions

9129857

superdump requested changes Mar 6, 2022

View reviewed changes

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

Update crates/bevy_pbr/src/light.rs

14f9ce8

Co-authored-by: Robert Swain <robert.swain@gmail.com>

superdump approved these changes Mar 7, 2022

View reviewed changes

superdump 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 7, 2022

skip assignment with screensize 0

862efca

cart reviewed Mar 8, 2022

View reviewed changes

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

bors bot changed the title ~~Dynamic light clusters~~ [Merged by Bors] - Dynamic light clusters Mar 8, 2022

bors bot closed this Mar 8, 2022

cart mentioned this pull request Mar 9, 2022

multiple_windows example is broken #4167

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Merged by Bors] - Dynamic light clusters #3968

[Merged by Bors] - Dynamic light clusters #3968

robtfm commented Feb 17, 2022 •

edited

Loading

superdump commented Feb 17, 2022

robtfm commented Feb 19, 2022 •

edited

Loading

superdump left a comment

superdump left a comment

superdump left a comment

superdump Mar 5, 2022 •

edited

Loading

robtfm Mar 5, 2022

superdump Mar 5, 2022

superdump Mar 5, 2022

superdump left a comment

cart left a comment

cart commented Mar 8, 2022

cart commented Mar 8, 2022

bors bot commented Mar 8, 2022

	// - z in world space with view orientation, with range [-inf, -0.0001] for perspective, and [1.0000, inf] for orthographic
	// z in view space with range [-inf, -f32::EPSILON] for perspective, and [1.0000, inf] for orthographic

-        // constraint z to be negative - i.e. in front of the camera
-        light_aabb_view_min.z = light_aabb_view_min.z.min(-0.0001);
-        light_aabb_view_max.z = light_aabb_view_max.z.min(-0.0001);
+        // Constrain view z to be negative - i.e. in front of the camera
+        // When view z is >= 0.0 and we're using a perspective projection, bad things happen.
+        // At view z == 0.0, ndc x,y are mathematically undefined. At view z > 0.0, i.e. behind the camera,
+        // the perspective projection flips the directions of the axes. This breaks assumptions about
+        // use of min/max operations as something that was to the left in view space is now returning a
+        // coordinate that for view z in front of the camera would be on the right, but at view z behind the
+        // camera is on the left. So, we just constrain view z to be < 0.0 and necessarily in front of the camera.
+        light_aabb_view_min.z = light_aabb_view_min.z.min(-f32::EPSILON);
+        light_aabb_view_max.z = light_aabb_view_max.z.min(-f32::EPSILON);

[Merged by Bors] - Dynamic light clusters #3968

[Merged by Bors] - Dynamic light clusters #3968

Conversation

robtfm commented Feb 17, 2022 • edited Loading

Objective

Solution

superdump commented Feb 17, 2022

robtfm commented Feb 19, 2022 • edited Loading

superdump left a comment

Choose a reason for hiding this comment

superdump left a comment

Choose a reason for hiding this comment

superdump left a comment

Choose a reason for hiding this comment

superdump Mar 5, 2022 • edited Loading

Choose a reason for hiding this comment

robtfm Mar 5, 2022

Choose a reason for hiding this comment

superdump Mar 5, 2022

Choose a reason for hiding this comment

superdump Mar 5, 2022

Choose a reason for hiding this comment

superdump left a comment

Choose a reason for hiding this comment

cart left a comment

Choose a reason for hiding this comment

cart commented Mar 8, 2022

cart commented Mar 8, 2022

bors bot commented Mar 8, 2022

robtfm commented Feb 17, 2022 •

edited

Loading

robtfm commented Feb 19, 2022 •

edited

Loading

superdump Mar 5, 2022 •

edited

Loading