Fix panic when specializing materials for entities spawned in `PostUpdate` #19064

grind086 · 2025-05-05T03:08:19Z

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 · 2025-05-05T03:19:39Z

(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

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.
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 · 2025-05-06T10:52:33Z

crates/bevy_sprite/src/mesh2d/material.rs

-                PostUpdate,
-                check_entities_needing_specialization::<M>.after(AssetEvents),
-            );
+            .add_systems(Last, check_entities_needing_specialization::<M>);


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),

);

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.

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

Henauxg · 2025-05-06T11:23:09Z

crates/bevy_pbr/src/material.rs

            .add_systems(
                PostUpdate,
-                (
-                    mark_meshes_as_changed_if_their_materials_changed::<M>.ambiguous_with_all(),
-                    check_entities_needing_specialization::<M>.after(AssetEvents),
-                )
+                mark_meshes_as_changed_if_their_materials_changed::<M>
+                    .ambiguous_with_all()
                    .after(mark_3d_meshes_as_changed_if_their_assets_changed),
-            );
+            )
+            .add_systems(Last, check_entities_needing_specialization::<M>);


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

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),

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).

Looks to me like you just uncovered another bug

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.

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

janhohenheim · 2025-05-06T19:56:58Z

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

tychedelia · 2025-05-07T18:22:03Z

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 · 2025-08-19T17:38:39Z

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 · 2025-08-19T18:28:52Z

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

alice-i-cecile · 2025-08-19T18:50:43Z

@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 · 2025-08-20T16:18:17Z

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 · 2025-08-20T19:19:41Z

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 · 2025-08-20T19:26:09Z

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 :)

# Objective prevent panics talked about in #19064 this a “temporary” fix to get in 0.17, until the above PR is finalized, and the source problem is fixed. ## Solution remove the unwrap ## Testing - anything is better than panic - been using this patch in my own games for months without any apparent issue ## Note please improve the actual error missing if needed, i doubt its perfect --------- Co-authored-by: Alice Cecile <[email protected]>

janhohenheim · 2025-08-24T20:36:22Z

Punting this to 0.18 because we have landed github.com//pull/20677

move material check_entities_needing_specialization system to Last

003dd70

alice-i-cecile added this to the 0.16.1 milestone May 5, 2025

github-project-automation bot added this to Rendering May 5, 2025

Henauxg suggested changes May 6, 2025

View reviewed changes

tychedelia suggested changes May 7, 2025

View reviewed changes

Trashtalk217 added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels May 16, 2025

xiyuoh mentioned this pull request May 27, 2025

Add Bevy patch to fix panic when spawning entities open-rmf/rmf_site#285

Merged

alice-i-cecile modified the milestones: 0.16.1, 0.16.2 May 30, 2025

luca-della-vedova mentioned this pull request Aug 7, 2025

Remove dependency on custom bevy open-rmf/rmf_site#362

Merged

2 tasks

mockersf modified the milestones: 0.16.2, 0.17 Aug 18, 2025

mirsella mentioned this pull request Aug 20, 2025

fix(mesh2d): replace unwrap by error log in specialize system #20677

Merged

janhohenheim removed this from the 0.17 milestone Aug 24, 2025

janhohenheim added this to the 0.18 milestone Aug 24, 2025

hukasu mentioned this pull request Sep 30, 2025

MeshMaterial3d added in PostUpdate sometimes do not get sent to the RenderApp #21290

Closed

alice-i-cecile added the X-Contentious There are nontrivial implications that should be thought through label Dec 10, 2025

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

Uh oh!

Fix panic when specializing materials for entities spawned in PostUpdate #19064

Are you sure you want to change the base?

Fix panic when specializing materials for entities spawned in PostUpdate #19064

Conversation

grind086 commented May 5, 2025 • edited by alice-i-cecile Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Testing

Uh oh!

alice-i-cecile commented May 5, 2025

Uh oh!

Henauxg left a comment • 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!

janhohenheim May 8, 2025 • 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!

janhohenheim commented May 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tychedelia commented May 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tychedelia left a comment

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile commented Aug 19, 2025

Uh oh!

mirsella commented Aug 19, 2025

Uh oh!

alice-i-cecile commented Aug 19, 2025

Uh oh!

mirsella commented Aug 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mirsella commented Aug 20, 2025

Uh oh!

alice-i-cecile commented Aug 20, 2025

Uh oh!

janhohenheim commented Aug 24, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Fix panic when specializing materials for entities spawned in `PostUpdate` #19064

Fix panic when specializing materials for entities spawned in `PostUpdate` #19064

grind086 commented May 5, 2025 •

edited by alice-i-cecile

Loading

Henauxg left a comment •

edited

Loading

janhohenheim May 8, 2025 •

edited

Loading

janhohenheim commented May 6, 2025 •

edited

Loading

tychedelia commented May 7, 2025 •

edited

Loading

mirsella commented Aug 20, 2025 •

edited

Loading