Skip to content

bevy_ecs: flush entities after running observers and hooks in despawn #15398

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

Merged
merged 3 commits into from
Sep 24, 2024
Merged

bevy_ecs: flush entities after running observers and hooks in despawn #15398

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
313c896
bevy_ecs: add failing test for spawn-in-despawn-observer
jrobsonchase Sep 23, 2024
0669dd1
bevy_ecs: flush entities after running observers and hooks in despawn
jrobsonchase Sep 23, 2024
12c5e3e
remove the first world.flush_entities() in despawn
jrobsonchase Sep 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 22 additions & 0 deletions crates/bevy_ecs/src/observer/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1160,4 +1160,26 @@ mod tests {
world.flush();
assert_eq!(vec!["event", "event"], world.resource::<Order>().0);
}

// Regression test for https://github.com/bevyengine/bevy/issues/14467
// Fails prior to https://github.com/bevyengine/bevy/pull/15398
#[test]
fn observer_on_remove_during_despawn_spawn_empty() {
let mut world = World::new();

// Observe the removal of A - this will run during despawn
world.observe(|_: Trigger<OnRemove, A>, mut cmd: Commands| {
// Spawn a new entity - this reserves a new ID and requires a flush
// afterward before Entities::free can be called.
cmd.spawn_empty();
});

let ent = world.spawn(A).id();

// Despawn our entity, which runs the OnRemove observer and allocates a
// new Entity.
// Should not panic - if it does, then Entities was not flushed properly
// after the observer's spawn_empty.
world.despawn(ent);
}
}
5 changes: 4 additions & 1 deletion crates/bevy_ecs/src/world/entity_ref.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1297,7 +1297,6 @@ impl<'w> EntityWorldMut<'w> {
/// See [`World::despawn`] for more details.
pub fn despawn(self) {
let world = self.world;
world.flush_entities();
let archetype = &world.archetypes[self.location.archetype_id];

// SAFETY: Archetype cannot be mutably aliased by DeferredWorld
Expand All @@ -1323,6 +1322,10 @@ impl<'w> EntityWorldMut<'w> {
world.removed_components.send(component_id, self.entity);
}

// Observers and on_remove hooks may reserve new entities, which
// requires a flush before Entities::free may be called.
world.flush_entities();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's also a call to world.flush_entities(); at the top of this method. Do we need both, or did that one just need to get moved down here?

It looks like prior to #10756 the flush() call was right before the free() call, and the hooks and observers code got added in between. The calls to deferred_world.trigger_on_foo in bundle.rs don't have flush() calls. That makes me think that we don't need to flush before triggering things, and we only need the single call here, right before the free().

It doesn't look like anything bad can happen if we flush twice, though, so it should be fine to leave both even if they're not both needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, so if I remove the first world.flush_entities(), the tests all pass and everything appears to work fine. As long as observers and hooks are unable to call any methods on Entities that verify_flushed(), it's safe to move. Since all of those require mutable access to Entities and thus exclusive World access as far as I'm aware, we should be OK to remove the first one.


let location = world
.entities
.free(self.entity)
Expand Down