Fix the crash in Bistro by avoiding binning mesh instances multiple times.

[pcwalton]

At the moment, DirtySpecializations::iter_to_queue attempts to avoid yielding the same mesh instance multiple times in a complex way. Unfortunately, this logic is incorrect for mesh instances that couldn't be queued because their materials haven't loaded yet (i.e. the contents of last_frame_view_pending_queues). The Bistro example tests this code path, as it has many materials that take a significant amount of time to load.

Code exists that tries to handle the case in which a mesh is re-binned, and this is why no crash occurred before PR #23662. PR #23662 made this incorrect code more incorrect because it bumps the instance count regardless of whether the mesh instance was already present in the bin; this causes a crash later with an out-of-bounds array access. However, it's always illegal to try to bin a binned entity; the entity must be removed from its bin first. Therefore, moving the instance count bump to the case in which the entity wasn't already in a bin is the wrong fix.

This commit contains the correct fix, which is to change DirtySpecializations::iter_to_queue so that it doesn't yield duplicate entities. This PR removes the complex logic that failed to account for all cases of duplicate mesh instances in favor of a simple hash set that tracks the mesh instances yielded so far. Additionally, this PR deletes the code path that tried to handle the case in which a mesh that's already binned is binned again and replaces it with a panic in debug mode.

I verified that this change fixes the crash in Bistro via cargo run --features=bevy/debug -p bistro --bin bistro. Note that on my NVIDIA GeForce RTX 2060, I still get an OOM error on that test case, but that's a separate issue.

Fixes #24990.

[pcwalton]

This doesn’t need to be in 0.19.1 as PR #23662 isn’t in 0.19.

[kfc35]

This fixes the startup errors for mixed_lighting and clustered_decal_maps, but switching light types in pcss (cargo run --example pcss —features="experimental_pbr_pcss") still errors when switching light types, now with the new error. That example removes a Light component from an entity and sticks a different Light on the same entity when the light type is switched. It results in the new debug_assert! failing in render_phase/mod.rs:

thread 'Compute Task Pool (1)' (3041233) panicked at crates/bevy_render/src/render_phase/mod.rs:295:9:
assertion failed: maybe_previous_binned_mesh_instance_buffer_index.is_none()
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace

thread 'Compute Task Pool (1)' (3041233) panicked at crates/bevy_ecs/src/error/handler.rs:141:1:
Encountered an error in system `bevy_pbr::render::light::queue_shadows`: System panicked

Encountered a panic in system `bevy_pbr::render::light::queue_shadows`!

thread 'Render thread' (3041323) panicked at crates/bevy_ecs/src/error/handler.rs:141:1:
Encountered an error in system `bevy_render::run_render_schedule`: Exclusive system panicked

Encountered a panic in system `bevy_render::run_render_schedule`!

I’m approving since this does fix several crashes. This probably supercedes #24980 but the issue with pcss still needs to be resolved.

[stuartparmenter]

Is removing the entity_pair_is_visible check intentional here? It seems like the check could be kept. iter_to_specialize continues to keep it

[pcwalton]

It shouldn’t be needed anymore because mesh_instances_queued_this_frame handles that case. i.e. it existed before to avoid yielding a mesh instance twice, but now we have a more general solution for that, so there’s no need for the check.

[pcwalton]

Oh, and also iter_to_specialize doesn’t have mesh_instances_queued_this_frame, so it still needs the check since it has no other way to avoid yielding a mesh instance twice.

[stuartparmenter]

nit: mesh_instances_queued_this_frame is maybe misleading name given it is cleared on every iter_to_queue call -- is more of a queued scratch space

[pcwalton]

Renamed it to mesh_instances_queued_this_iteration_scratch_space.

[alice-i-cecile]

Waiting on brief replies to @stuartparmenter then I'll merge :)

[ChristopherBiscardi]

fixes world_serialization and custom_skinned_mesh as well. thus closes #24990

[alice-i-cecile]

@pcwalton lemme know when you'd like me to merge this and I'll press the button for you :)

[pcwalton]

Merged on top of #24980. I kept the change from #24980 that avoids panicking or corrupting the bins if we yield the same mesh instance twice, but we now log an error.

Additionally, I fixed the crash in PCSS at the source. The problem in that case was that RenderExtractedShadowMapVisibleEntities and RenderShadowMapVisibleEntities were part of the SyncComponent::Target for DirectionalLight, PointLight, and SpotLight. So if a light changes from one type to another, the system that handles SyncComponent will run and delete the RenderExtractedShadowMapVisibleEntities and RenderShadowMapVisibleEntities components. That system runs during extraction, and the scheduler might decide to schedule it after extract_lights. If it does, then the RenderExtractedShadowMapVisibleEntities and RenderShadowMapVisibleEntities components that extract_lights updated will be deleted right after the light changes type. Because those components must be retained in order to detect whether entities are newly visible or retained from the previous frame, this ultimately causes us to bin mesh instances in the shadow map twice. The solution was to remove RenderExtractedShadowMapVisibleEntities and RenderShadowMapVisibleEntities from the SyncComponent::Target associated type for DirectionalLight, PointLight, and SpotLight and to centralize the management of those components in extract_lights.