Fix panic when specializing materials for entities spawned in PostUpdate

[grind086]

Objective

If an entity requiring specialization is spawned during PostUpdate after its material's check_entities_needing_specialization, but before CheckVisibility, a panic occurs during material specialization. This happens because the view assumes that every visible entity will be present in the specialization ticks map, but in this scenario the entity won't be added to the map until the next frame.

Fixes #19048. This may also fix the related #18980, but I wasn't able to reproduce that one and it could be unrelated.

Edit by Alice: Fixes #18980 too!

Solution

Move check_entities_needing_specialization systems to Last. This ensures that they always runs after CheckVisibility.

Testing

Confirmed the reproduction from #19048 is fixed, and ran several 3d and 2d examples with no apparent change.

[alice-i-cecile]

(edited your PR description to avoid accidentally closing the related issue)

[Henauxg]

I'm not sure that just changing the schedule of check_entities_needing_specialization to Last is correct. Some systems expect it to run in PostUpdate
As an example, in material.rs in bevy_pbr:

       if self.shadows_enabled {
            app.add_systems(
                PostUpdate,
                check_light_entities_needing_specialization::<M>
                    .after(check_entities_needing_specialization::<M>),
            );
        }

check_light_entities_needing_specialization writes in EntitiesNeedingSpecialization after the clear done by check_entities_needing_specialization

1. We could maybe? move also these systems to the Last schedule, but there may be other reasons for those to be in PostUpdate that I did not deduce yet.
2. But we could instead explicitly specify .after(VisibilitySystems::CheckVisibility) as a condition for check_entities_needing_specialization

@tychedelia, maybe you would know what decision would be the best ?

Notes:

• AssetChanged doc has an error, it says that it runs in Last, while it runs in PostUpdate. l opened a PR to fix it.

[Henauxg]

Going with option 2 as described above

Suggested change

	.add_systems(Last, check_entities_needing_specialization::<M>);
	.add_systems(
	PostUpdate,
	check_entities_needing_specialization::<M>
	.after(AssetEvents)
	.after(VisibilitySystems::CheckVisibility),
	);

[grind086]

This was actually my initial choice for a fix, but @tychedelia mentioned on discord that moving the system to Last might be safer if it turns out there's no benefit to keeping it in LastUpdate. I think they were going to take a look when they had time.

[Henauxg]

(Then we should at least move check_light_entities_needing_specialization to Last in bevy_pbr)

[Henauxg]

With option 2. described above, this would look like:

       .add_systems(
                PostUpdate,
                (
                    mark_3d_meshes_as_changed_if_their_assets_changed,
                    mark_meshes_as_changed_if_their_materials_changed::<M>.ambiguous_with_all(),
                    check_entities_needing_specialization::<M>,
                )
                    .chain()
                    .after(AssetEvents)
                    .after(VisibilitySystems::CheckVisibility),
            );

The chaining is not strictly necessary, we could just add the new aftercondition to check_entities_needing_specialization.
But that's also a point I'd like to discuss. Currently check_entities_needing_specialization needlessly? checks for

 Or<(
                Changed<Mesh3d>,
                AssetChanged<Mesh3d>,
                Changed<MeshMaterial3d<M>>,
                AssetChanged<MeshMaterial3d<M>>,
            )>,

But that is literally what mark_3d_meshes_as_changed_if_their_assets_changed and mark_meshes_as_changed_if_their_materials_changed seem to be there for (marking Mesh3d as changed) ? So chaining, and just checking for Changed<Mesh3d> in check_entities_needing_specialization would probably be simpler and achieve the expected goal.

Also:

• with the current PR mark_meshes_as_changed_if_their_materials_changed and mark_3d_meshes_as_changed_if_their_assets_changed are not properly scheduled after AssetEvents
• on main, mark_meshes_as_changed_if_their_materials_changed is not either, which could expain why all the Or parameters were introduced in check_entities_needing_specialization

[grind086]

I think you may have misread something here. mark_3d_meshes_as_changed_if_their_assets_changed is from bevy_render, and isn't added here. The only ordering constraint related to it in either main or this PR should be mark_meshes_as_changed_if_their_materials_changed.after(mark_3d_meshes_as_changed_if_their_assets_changed) unless there's some other interaction happening.

The minimal change from main to ensure ordering relative to CheckVisibility would just be:

.add_systems(
    PostUpdate,
    (
        mark_meshes_as_changed_if_their_materials_changed::<M>.ambiguous_with_all(),
        check_entities_needing_specialization::<M>
            .after(AssetEventSystems)
            .after(VisibilitySystems::CheckVisibility),
    )
        .after(mark_3d_meshes_as_changed_if_their_assets_changed)
);

mark_3d_meshes_as_changed_if_their_assets_changed is also already explicitly ordered before asset events here:

bevy/crates/bevy_render/src/mesh/mod.rs

Lines 74 to 76 in 4051465

	mark_3d_meshes_as_changed_if_their_assets_changed
	.ambiguous_with(VisibilitySystems::CalculateBounds)
	.before(AssetEventSystems),

[Henauxg]

You're right about mark_3d_meshes_as_changed_if_their_assets_changed, it's only used for ordering in bevy_pbr, my bad.
I'm still surprised though that mark_3d_meshes_as_changed_if_their_assets_changed is ordered before AssetEvents all the while reading the asset events. If anyone knows why I'd gladly take an explanation.
And, even a bit worse, mark_meshes_as_changed_if_their_materials_changed is also reading asset events. Its execution order is not deterministic as its only ordered after mark_3d_meshes_as_changed_if_their_assets_changed which itself is before AssetEvents. So mark_meshes_as_changed_if_their_materials_changed can end up running before or after AssetEvents depending on scheduling (and as such, marking meshes as modifed or not depending on scheduling).

[janhohenheim]

Looks to me like you just uncovered another bug

[tychedelia]

Thanks for thinking about this deeply. These indeed seem to be sketchy to me. I think that both these systems were implemented before the AssetChanged filter was added and so they are ad-hoc implementations of the same thing. We should be able to simply add that filter in extract_meshes_for_gpu_building for mark_3d_meshes_as_changed_if_their_assets_changed. mark_meshes_as_changed_if_their_materials_changed is a bit more complicated because it requires the material type, so needs to stay in place with the potential for fixing post #18075 potentially.

[janhohenheim]

I think there's a chance that this scheduling ambiguity I just ran into is also caused by this.

[janhohenheim]

From reading this, I'm fairly sure this fixes my issue #18980 as well, so let's close that one on merging :)

[tychedelia]

For context, the reason these were added to PostUpdate instead of Last (including moving AssetEvents to PostUpdate) was to hide some latency of the change detection table scans behind the check_visibility system and any other engine internal bookkeeping we tend to do there. Benchmarking at the time showed an improvement.

Last is the safer choice in some ways, but it still creates the possibility of this bug if people spawn things in Last without ordering relative to AssetEvents (i.e. if we were to move AssetEvents back to Last). I originally chose the unwrap to enforce the invariant that we always have a detected change tick for new entities and have defended that choice recently precisely because it helps uncover issues like this. However, I hadn't fully considered that users might in practice spawn new mesh/materials even in Last. It's a little weird, but the schedule exists for a reason.

Given we've rooted out a lot of the cold specialization bugs, I could be convinced that we maybe could swap the unwrap with a very noisey error log that points out the user is likely doing something wrong.

[tychedelia]

If we move these to Last we should also revert the change to move AssetEvents into PostUpdate. I'd like to see some benchmarking too if possible.

[alice-i-cecile]

Hi @grind086, do you have time to do the requested changes right now? This has ended up in the 0.17 milestone, but I'm tempted to cut it unless it's fixed up in the next couple of days.

[mirsella]

hey, I would really love this being in 0.17, as this crash is prominent in my wasm builds.

[alice-i-cecile]

@mirsella, are you up for adopting this PR then? I'd like to get this in too, but we need help finishing this work.

[mirsella]

as much i would like to contribute to this level, sadly my knowledge of internal bevy things are nowhere near enough to understand the implications of each internal system ordering.

also, i remember now when i tried using this fix it was still happening, so i instead removed the panic instead to be sure:

            let Some(entity_tick) = entity_specialization_ticks.get(visible_entity) else {
                continue;
            };

now, ive tried using 0.16.1 again without these patch to try to get it to crash again but couldn't reproduce it this time :(

but i can help if its about moving back AssetEvents to Last or

Given we've rooted out a lot of the cold specialization bugs, I could be convinced that we maybe could swap the unwrap with a very noisey error log that points out the user is likely doing something wrong.

which is basically what i did in my fork minus the log

[mirsella]

what about i do a small PR just to remove the panic and instead error! log for 0.17, and this PR solving the source issue can be completed in a future time ?

[alice-i-cecile]

what about i do a small PR just to remove the panic and instead error! log for 0.17, and this PR solving the source issue can be completed in a future time ?

I would be very happy to review that PR :)