[Merged by Bors] - fix issues with too many point lights by robtfm · Pull Request #3916 · bevyengine/bevy

robtfm · 2022-02-11T13:14:11Z

Objective

fix #3915

Solution

the issues are caused by

lights are assigned to clusters before being filtered down to MAX_POINT_LIGHTS, leading to cluster counts potentially being too high
after fixing the above, packing the count into 8 bits still causes overflow with exactly 256 lights affecting a cluster

to fix:

assign_lights_to_clusters

limit extracted lights to MAX_POINT_LIGHTS, selecting based on shadow-caster & intensity (if required)
warn if MAX_POINT_LIGHT count is exceeded

prepare_lights

limit the lights assigned to a cluster to CLUSTER_COUNT_MASK (which is 1 less than MAX_POINT_LIGHTS) to avoid overflowing into the offset bits

notes:

a better solution to the overflow may be to use more than 8 bits for cluster_count (the comment states only 14 of the remaining 24 bits are used for the offset). this would touch more of the code base but i'm happy to try if it has some benefit.
intensity is only one way to select lights. it may be worth allowing user configuration of the light filtering, but i can't see a clean way to do that

robtfm · 2022-02-14T15:57:39Z

after reviewing with @superdump,

removed the intensity step from the light filter - choosing lights that have most effect on the scene is a hard problem and using intensity is bad enough to be not worth doing
increased the CLUSTER_COUNT_SIZE so that the current max lights fit with some room to spare

this means that users should be filtering lights themselves based on their own preferred strategy. to be feasible this will mean adding a visiblity filter to point lights. this is not included here since there is ongoing discussion about bools vs marker components that we would like @cart to weigh in on.

superdump

Just the one question, otherwise LGTM. When the question is answered, I'll mark it as ready for final review.

crates/bevy_pbr/src/light.rs

superdump

A couple of small changes then LGTM.

crates/bevy_pbr/src/light.rs

mockersf · 2022-02-25T15:20:22Z

crates/bevy_pbr/src/light.rs

+            .collect();
+
+        if lights.len() > MAX_POINT_LIGHTS {
+            warn!("MAX_POINT_LIGHTS ({}) exceeded", MAX_POINT_LIGHTS);


not a fan of logs that will potentially happen on every frame

i agree, but it should definitely be emitted once.

is it reasonable to add a Local to check if the warning has been emitted already?

The problem is that if there are other logs then it may get lost. I think we need a general pattern for this. Perhaps something like a Local<> that holds the last time we logged, and then just log once every 10s or something?

no, I would keep it that way for now rather than add an additional parameter... there are a few other places with similar issues, we don't have the good solution for that yet

ideally the logger should be able to detect duplicate logs and only log them once in a while 🤔

For something that should be "fixed" i'm cool with warning every frame (until we can suppress more intelligently). But in this case, complex scenes could easily go over the max point light limit. This isn't "wrong" and I dont think we should be aggressively pushing people to hide lights that go over the limit.

Manually suppressing duplicate warnings is important to do here imo.

@cart what do you think of my suggestion above as an approach to 'rate limiting' the log?

limited to warning once for now

@superdump yeah i like that, although I'm cool with logging once for now. "rate limiting" is an option, but we could also consider detecting fluctuations in light counts (ex: if you go under the limit and then back over, we print the warning again).

crates/bevy_pbr/src/light.rs

cart · 2022-03-01T01:07:01Z

crates/bevy_pbr/src/light.rs

+
+        // check each light against each view's frustum, keep only those that affect at least one of our views
+        let frusta: Vec<_> = views.iter().map(|(_, _, _, frustum, _)| *frustum).collect();
+        *lights = (*lights)


We could use retain here to avoid the extra allocation (combine the filter predicate with a captured counter to limit to MAX_POINT_LIGHTS + 1 and remove the remaining lights)

good idea, thanks. the checks are failing now but i think it's not my fault?

Yup its a github linux-vm CI problem. Bleh :)

cart · 2022-03-01T10:17:24Z

bors r+

# Objective fix #3915 ## Solution the issues are caused by - lights are assigned to clusters before being filtered down to MAX_POINT_LIGHTS, leading to cluster counts potentially being too high - after fixing the above, packing the count into 8 bits still causes overflow with exactly 256 lights affecting a cluster to fix: ```assign_lights_to_clusters``` - limit extracted lights to MAX_POINT_LIGHTS, selecting based on shadow-caster & intensity (if required) - warn if MAX_POINT_LIGHT count is exceeded ```prepare_lights``` - limit the lights assigned to a cluster to CLUSTER_COUNT_MASK (which is 1 less than MAX_POINT_LIGHTS) to avoid overflowing into the offset bits notes: - a better solution to the overflow may be to use more than 8 bits for cluster_count (the comment states only 14 of the remaining 24 bits are used for the offset). this would touch more of the code base but i'm happy to try if it has some benefit. - intensity is only one way to select lights. it may be worth allowing user configuration of the light filtering, but i can't see a clean way to do that

bors · 2022-03-01T10:34:42Z

Pull request successfully merged into main.

Build succeeded:

# Objective Add Visibility for lights ## Solution - add Visibility to PointLightBundle and DirectionLightBundle - filter lights used by Visibility.is_visible note: includes changes from #3916 due to overlap, will be cleaner after that is merged

# Objective fix bevyengine#3915 ## Solution the issues are caused by - lights are assigned to clusters before being filtered down to MAX_POINT_LIGHTS, leading to cluster counts potentially being too high - after fixing the above, packing the count into 8 bits still causes overflow with exactly 256 lights affecting a cluster to fix: ```assign_lights_to_clusters``` - limit extracted lights to MAX_POINT_LIGHTS, selecting based on shadow-caster & intensity (if required) - warn if MAX_POINT_LIGHT count is exceeded ```prepare_lights``` - limit the lights assigned to a cluster to CLUSTER_COUNT_MASK (which is 1 less than MAX_POINT_LIGHTS) to avoid overflowing into the offset bits notes: - a better solution to the overflow may be to use more than 8 bits for cluster_count (the comment states only 14 of the remaining 24 bits are used for the offset). this would touch more of the code base but i'm happy to try if it has some benefit. - intensity is only one way to select lights. it may be worth allowing user configuration of the light filtering, but i can't see a clean way to do that

# Objective Add Visibility for lights ## Solution - add Visibility to PointLightBundle and DirectionLightBundle - filter lights used by Visibility.is_visible note: includes changes from bevyengine#3916 due to overlap, will be cleaner after that is merged

robtfm added 4 commits February 11, 2022 11:01

lights

94967d0

format

d4d8e0c

omit lights that don't affect the scene

a9ec744

change warning from cluster to extraction

92e261e

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

superdump added A-Rendering Drawing game state to the screen and removed S-Needs-Triage This issue needs to be labelled labels Feb 11, 2022

alice-i-cecile added the C-Bug An unexpected or incorrect behavior label Feb 11, 2022

robtfm marked this pull request as draft February 11, 2022 22:59

robtfm added 3 commits February 14, 2022 10:36

Merge branch 'bevyengine:main' into main

20a68a4

filter lights based on impact to any view

460ccc4

revert shadow-breaking changes

edd7217

robtfm marked this pull request as ready for review February 14, 2022 11:27

robtfm added 2 commits February 14, 2022 15:52

increase CLUSTER_COUNT_SIZE

5d1a327

format

ab47c5f

robtfm mentioned this pull request Feb 15, 2022

[Merged by Bors] - add Visibility for lights #3958

Closed

superdump reviewed Feb 17, 2022

View reviewed changes

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

superdump requested changes Feb 25, 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

add MAX_POINT_LIGHTS value to warning

c0cdb26

superdump approved these changes Feb 25, 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 Feb 25, 2022

mockersf reviewed Feb 25, 2022

View reviewed changes

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

mockersf reviewed Feb 25, 2022

View reviewed changes

extract translation before collecting lights

86c4bd3

mockersf approved these changes Feb 25, 2022

View reviewed changes

cart reviewed Feb 28, 2022

View reviewed changes

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

robtfm added 3 commits February 28, 2022 23:06

use local vec for storing light data

00bbe60

suppress duplicate MAX_POINT_LIGHT warnings

3274e4d

Merge remote-tracking branch 'upstream/main'

6b4f309

cart reviewed Mar 1, 2022

View reviewed changes

robtfm added 2 commits March 1, 2022 09:28

iter.filter.copied.collect -> retain

0e5293b

clippy

4ed538a

bors bot changed the title ~~fix issues with too many point lights~~ [Merged by Bors] - fix issues with too many point lights Mar 1, 2022

bors bot closed this Mar 1, 2022

Uh oh!

Conversation

robtfm commented Feb 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Uh oh!

robtfm commented Feb 14, 2022

Uh oh!

superdump left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

superdump left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mockersf Feb 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cart Mar 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cart commented Mar 1, 2022

Uh oh!

bors bot commented Mar 1, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

robtfm commented Feb 11, 2022 •

edited

Loading

mockersf Feb 25, 2022 •

edited

Loading

cart Mar 1, 2022 •

edited

Loading