Skip to content
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

multiple_windows example is broken #4167

Closed
cart opened this issue Mar 9, 2022 · 5 comments
Closed

multiple_windows example is broken #4167

cart opened this issue Mar 9, 2022 · 5 comments
Labels
A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior

Comments

@cart
Copy link
Member

cart commented Mar 9, 2022

It panics with:

thread 'main' panicked at 'index out of bounds: the len is 0 but the index is 0', crates/bevy_pbr/src/render/light.rs:996:43

This is a regression introduced recently by #3968
@robtfm @superdump
@cart cart added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen labels Mar 9, 2022
@cart
Copy link
Member Author

cart commented Mar 9, 2022

I'm thinking this is because the window doesn't exist yet for the first frame that the secondary camera is spawned, which results in a "default" screen size of (0, 0) in the cluster assignment algorithm / cluster assignments being skipped. Seems like a "corner case" that we missed in #3968. I'm guessing this would also be a problem for single window scenarios that behave the same way.

@cart
Copy link
Member Author

cart commented Mar 10, 2022

Skipping cluster prep when there are no clusters to assign to resolves the problem.

@superdump
Copy link
Contributor

Ouch!

@superdump
Copy link
Contributor

Did you see:

and does that fix it?

@cart
Copy link
Member Author

cart commented Mar 10, 2022

I would not expect it to, as that only touches update() and the "core" issue is that update() isn't called because we "skip" it.

@cart cart mentioned this issue Mar 10, 2022
Closed
@bors bors bot closed this as completed in 207ebde Mar 24, 2022
aevyrie pushed a commit to aevyrie/bevy that referenced this issue Jun 7, 2022
@cart @aevyrie
Always update clusters and remove per-frame allocations (bevyengine#4169
e4a9b1c 
)

 * 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.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this issue Feb 1, 2023
@cart @ItsDoot
Always update clusters and remove per-frame allocations (bevyengine#4169
576db01 
)

 * 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-Rendering Drawing game state to the screen C-Bug An unexpected or incorrect behavior
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

[Merged by Bors] - Always update clusters and remove per-frame allocations cart/bevy
2 participants
@superdump @cart