Fix for SceneEntityMapper + hooks panic (#15405)

# Objective

- Add a test case for #14300 

Fixes #14300

## Solution

`SceneEntityMapper` relies on operations on `Entities` that require
flushing in advance, such as `alloc` and `free`. Previously, it wasn't
calling `world.flush_entities()` itself and relied on its caller having
flushed beforehand. This wasn't an issue before observers and hooks were
released, since entity reservation was happening at expected times. Now
that hooks and observers are a thing, they can introduce a need to
flush.

We have a few options:
* Flush after each observer/hook run
* Flush between each paired observer/hook and operation that requires a
flush
* Flush before operations requiring it

The first option for this case seemed trickier to reason about than I
wanted, since it involved the `BundleInserter` and its
`UnsafeWorldCell`, and the second is generally harder to track down. The
third seemed the most straightforward and conventional, since we can see
a flush occurring at the start of a number of `World` methods.
Therefore, we're letting `SceneEntityMapper` be in charge of upholding
its own invariants and calling `flush_entities` when it's created.

## Testing

Added a new test case modeled after #14300
This commit is contained in:
Josh Robson Chase 2024-09-24 13:37:23 -04:00 committed by GitHub
parent fb9aaa1527
commit fcddb54ce5
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 73 additions and 1 deletions

View file

@ -183,6 +183,9 @@ impl<'m> SceneEntityMapper<'m> {
/// Creates a new [`SceneEntityMapper`], spawning a temporary base [`Entity`] in the provided [`World`]
pub fn new(map: &'m mut EntityHashMap<Entity>, world: &mut World) -> Self {
// We're going to be calling methods on `Entities` that require advance
// flushing, such as `alloc` and `free`.
world.flush_entities();
Self {
map,
// SAFETY: Entities data is kept in a valid state via `EntityMapper::world_scope`
@ -292,6 +295,23 @@ mod tests {
);
}
#[test]
fn entity_mapper_no_panic() {
let mut world = World::new();
// "Dirty" the `Entities`, requiring a flush afterward.
world.entities.reserve_entity();
assert!(world.entities.needs_flush());
// Create and exercise a SceneEntityMapper - should not panic because it flushes
// `Entities` first.
SceneEntityMapper::world_scope(&mut Default::default(), &mut world, |_, m| {
m.map_entity(Entity::PLACEHOLDER);
});
// The SceneEntityMapper should leave `Entities` in a flushed state.
assert!(!world.entities.needs_flush());
}
#[test]
fn dyn_entity_mapper_object_safe() {
assert_object_safe::<dyn DynEntityMapper>();

View file

@ -209,14 +209,19 @@ where
#[cfg(test)]
mod tests {
use bevy_ecs::{
component::Component,
entity::{Entity, EntityHashMap, EntityMapper, MapEntities},
reflect::{AppTypeRegistry, ReflectMapEntitiesResource, ReflectResource},
reflect::{
AppTypeRegistry, ReflectComponent, ReflectMapEntities, ReflectMapEntitiesResource,
ReflectResource,
},
system::Resource,
world::{Command, World},
};
use bevy_hierarchy::{AddChild, Parent};
use bevy_reflect::Reflect;
use crate::dynamic_scene::DynamicScene;
use crate::dynamic_scene_builder::DynamicSceneBuilder;
#[derive(Resource, Reflect, Debug)]
@ -348,4 +353,51 @@ mod tests {
"something is wrong with the this test or the code reloading scenes since the relationship between scene entities is broken"
);
}
// Regression test for https://github.com/bevyengine/bevy/issues/14300
// Fails before the fix in https://github.com/bevyengine/bevy/pull/15405
#[test]
fn no_panic_in_map_entities_after_pending_entity_in_hook() {
#[derive(Default, Component, Reflect)]
#[reflect(Component)]
struct A;
#[derive(Component, Reflect)]
#[reflect(Component, MapEntities)]
struct B(pub Entity);
impl MapEntities for B {
fn map_entities<M: EntityMapper>(&mut self, entity_mapper: &mut M) {
self.0 = entity_mapper.map_entity(self.0);
}
}
let reg = AppTypeRegistry::default();
{
let mut reg_write = reg.write();
reg_write.register::<A>();
reg_write.register::<B>();
}
let mut scene_world = World::new();
scene_world.insert_resource(reg.clone());
scene_world.spawn((B(Entity::PLACEHOLDER), A));
let scene = DynamicScene::from_world(&scene_world);
let mut dst_world = World::new();
dst_world
.register_component_hooks::<A>()
.on_add(|mut world, _, _| {
world.commands().spawn_empty();
});
dst_world.insert_resource(reg.clone());
// Should not panic.
// Prior to fix, the `Entities::alloc` call in
// `EntityMapper::map_entity` would panic due to pending entities from the observer
// not having been flushed.
scene
.write_to_world(&mut dst_world, &mut Default::default())
.unwrap();
}
}