From 40e88dceff62d695869893c9f846bbcf59ab6eb0 Mon Sep 17 00:00:00 2001 From: Josh Robson Chase Date: Tue, 1 Oct 2024 14:34:09 -0400 Subject: [PATCH] Change ReflectMapEntities to operate on components before insertion (#15422) Previous PR https://github.com/bevyengine/bevy/pull/14549 was closed in error and couldn't be reopened since I had updated the branch :crying_cat_face: # Objective Fixes #14465 ## Solution `ReflectMapEntities` now works similarly to `MapEntities` in that it works on the reflected value itself rather than the component in the world after insertion. This makes it so that observers see the remapped entities on insertion rather than the entity IDs from the scene. `ReflectMapEntities` now works for both components and resources, so we only need the one. ## Testing * New unit test for `Observer`s + `DynamicScene`s * New unit test for `Observer`s + `Scene`s * Open to suggestions for other tests! --- ## Migration Guide - Consumers of `ReflectMapEntities` will need to call `map_entities` on values prior to inserting them into the world. - Implementors of `MapEntities` will need to remove the `mappings` method, which is no longer needed for `ReflectMapEntities` and has been removed from the trait. --------- Co-authored-by: Alice Cecile Co-authored-by: Hennadii Chernyshchyk --- crates/bevy_ecs/src/entity/map_entities.rs | 94 ++--------------- crates/bevy_ecs/src/reflect/map_entities.rs | 106 ++++---------------- crates/bevy_ecs/src/reflect/mod.rs | 2 +- crates/bevy_scene/src/dynamic_scene.rs | 88 +++++++--------- crates/bevy_scene/src/scene.rs | 61 +++++++---- 5 files changed, 104 insertions(+), 247 deletions(-) diff --git a/crates/bevy_ecs/src/entity/map_entities.rs b/crates/bevy_ecs/src/entity/map_entities.rs index 5bc0be1725774..5b0de2359d6b8 100644 --- a/crates/bevy_ecs/src/entity/map_entities.rs +++ b/crates/bevy_ecs/src/entity/map_entities.rs @@ -15,9 +15,16 @@ use super::{EntityHashMap, VisitEntitiesMut}; /// (usually by using an [`EntityHashMap`] between source entities and entities in the /// current world). /// +/// This trait is similar to [`VisitEntitiesMut`]. They differ in that [`VisitEntitiesMut`] operates +/// on `&mut Entity` and allows for in-place modification, while this trait makes no assumption that +/// such in-place modification is occurring, which is impossible for types such as [`HashSet`] +/// and [`EntityHashMap`] which must be rebuilt when their contained [`Entity`]s are remapped. +/// /// Implementing this trait correctly is required for properly loading components /// with entity references from scenes. /// +/// [`HashSet`]: bevy_utils::HashSet +/// /// ## Example /// /// ``` @@ -60,9 +67,6 @@ impl MapEntities for T { /// /// More generally, this can be used to map [`Entity`] references between any two [`Worlds`](World). /// -/// Note that this trait is _not_ [object safe](https://doc.rust-lang.org/reference/items/traits.html#object-safety). -/// Please see [`DynEntityMapper`] for an object safe alternative. -/// /// ## Example /// /// ``` @@ -79,64 +83,16 @@ impl MapEntities for T { /// fn map_entity(&mut self, entity: Entity) -> Entity { /// self.map.get(&entity).copied().unwrap_or(entity) /// } -/// -/// fn mappings(&self) -> impl Iterator { -/// self.map.iter().map(|(&source, &target)| (source, target)) -/// } /// } /// ``` pub trait EntityMapper { /// Map an entity to another entity fn map_entity(&mut self, entity: Entity) -> Entity; - - /// Iterate over all entity to entity mappings. - /// - /// # Examples - /// - /// ```rust - /// # use bevy_ecs::entity::{Entity, EntityMapper}; - /// # fn example(mapper: impl EntityMapper) { - /// for (source, target) in mapper.mappings() { - /// println!("Will map from {source} to {target}"); - /// } - /// # } - /// ``` - fn mappings(&self) -> impl Iterator; -} - -/// An [object safe](https://doc.rust-lang.org/reference/items/traits.html#object-safety) version -/// of [`EntityMapper`]. This trait is automatically implemented for type that implements `EntityMapper`. -pub trait DynEntityMapper { - /// Map an entity to another entity. - /// - /// This is an [object safe](https://doc.rust-lang.org/reference/items/traits.html#object-safety) - /// alternative to [`EntityMapper::map_entity`]. - fn dyn_map_entity(&mut self, entity: Entity) -> Entity; - - /// Iterate over all entity to entity mappings. - /// - /// This is an [object safe](https://doc.rust-lang.org/reference/items/traits.html#object-safety) - /// alternative to [`EntityMapper::mappings`]. - fn dyn_mappings(&self) -> Vec<(Entity, Entity)>; -} - -impl DynEntityMapper for T { - fn dyn_map_entity(&mut self, entity: Entity) -> Entity { - ::map_entity(self, entity) - } - - fn dyn_mappings(&self) -> Vec<(Entity, Entity)> { - ::mappings(self).collect() - } } -impl<'a> EntityMapper for &'a mut dyn DynEntityMapper { +impl EntityMapper for &mut dyn EntityMapper { fn map_entity(&mut self, entity: Entity) -> Entity { - (*self).dyn_map_entity(entity) - } - - fn mappings(&self) -> impl Iterator { - (*self).dyn_mappings().into_iter() + (*self).map_entity(entity) } } @@ -160,10 +116,6 @@ impl EntityMapper for SceneEntityMapper<'_> { new } - - fn mappings(&self) -> impl Iterator { - self.map.iter().map(|(&source, &target)| (source, target)) - } } /// A wrapper for [`EntityHashMap`], augmenting it with the ability to allocate new [`Entity`] references in a destination @@ -242,10 +194,9 @@ impl<'m> SceneEntityMapper<'m> { #[cfg(test)] mod tests { use crate::{ - entity::{DynEntityMapper, Entity, EntityHashMap, EntityMapper, SceneEntityMapper}, + entity::{Entity, EntityHashMap, EntityMapper, SceneEntityMapper}, world::World, }; - use bevy_utils::assert_object_safe; #[test] fn entity_mapper() { @@ -292,26 +243,6 @@ mod tests { assert!(entity.generation() > dead_ref.generation()); } - #[test] - fn entity_mapper_iteration() { - let mut old_world = World::new(); - let mut new_world = World::new(); - - let mut map = EntityHashMap::default(); - let mut mapper = SceneEntityMapper::new(&mut map, &mut new_world); - - assert_eq!(mapper.mappings().collect::>(), vec![]); - - let old_entity = old_world.spawn_empty().id(); - - let new_entity = mapper.map_entity(old_entity); - - assert_eq!( - mapper.mappings().collect::>(), - vec![(old_entity, new_entity)] - ); - } - #[test] fn entity_mapper_no_panic() { let mut world = World::new(); @@ -328,9 +259,4 @@ mod tests { // The SceneEntityMapper should leave `Entities` in a flushed state. assert!(!world.entities.needs_flush()); } - - #[test] - fn dyn_entity_mapper_object_safe() { - assert_object_safe::(); - } } diff --git a/crates/bevy_ecs/src/reflect/map_entities.rs b/crates/bevy_ecs/src/reflect/map_entities.rs index dcf8c3ae86f30..49c018f50e00c 100644 --- a/crates/bevy_ecs/src/reflect/map_entities.rs +++ b/crates/bevy_ecs/src/reflect/map_entities.rs @@ -1,107 +1,37 @@ -use crate::{ - component::Component, - entity::{Entity, EntityHashMap, MapEntities, SceneEntityMapper}, - world::World, -}; -use bevy_reflect::FromType; +use crate::entity::{EntityMapper, MapEntities}; +use bevy_reflect::{FromReflect, FromType, PartialReflect}; -/// For a specific type of component, this maps any fields with values of type [`Entity`] to a new world. +/// For a specific type of value, this maps any fields with values of type [`Entity`] to a new world. /// /// Since a given `Entity` ID is only valid for the world it came from, when performing deserialization /// any stored IDs need to be re-allocated in the destination world. /// -/// See [`SceneEntityMapper`] and [`MapEntities`] for more information. +/// See [`EntityMapper`] and [`MapEntities`] for more information. +/// +/// [`Entity`]: crate::entity::Entity +/// [`EntityMapper`]: crate::entity::EntityMapper #[derive(Clone)] pub struct ReflectMapEntities { - map_all_entities: fn(&mut World, &mut SceneEntityMapper), - map_entities: fn(&mut World, &mut SceneEntityMapper, &[Entity]), + map_entities: fn(&mut dyn PartialReflect, &mut dyn EntityMapper), } impl ReflectMapEntities { - /// A general method for applying [`MapEntities`] behavior to all elements in an [`EntityHashMap`]. - /// - /// Be mindful in its usage: Works best in situations where the entities in the [`EntityHashMap`] are newly - /// created, before systems have a chance to add new components. If some of the entities referred to - /// by the [`EntityHashMap`] might already contain valid entity references, you should use [`map_entities`](Self::map_entities). - /// - /// An example of this: A scene can be loaded with `Parent` components, but then a `Parent` component can be added - /// to these entities after they have been loaded. If you reload the scene using [`map_all_entities`](Self::map_all_entities), those `Parent` - /// components with already valid entity references could be updated to point at something else entirely. - pub fn map_all_entities(&self, world: &mut World, entity_map: &mut EntityHashMap) { - SceneEntityMapper::world_scope(entity_map, world, self.map_all_entities); - } - - /// A general method for applying [`MapEntities`] behavior to elements in an [`EntityHashMap`]. Unlike - /// [`map_all_entities`](Self::map_all_entities), this is applied to specific entities, not all values - /// in the [`EntityHashMap`]. + /// A general method for remapping entities in a reflected value via an [`EntityMapper`]. /// - /// This is useful mostly for when you need to be careful not to update components that already contain valid entity - /// values. See [`map_all_entities`](Self::map_all_entities) for more details. - pub fn map_entities( - &self, - world: &mut World, - entity_map: &mut EntityHashMap, - entities: &[Entity], - ) { - SceneEntityMapper::world_scope(entity_map, world, |world, mapper| { - (self.map_entities)(world, mapper, entities); - }); + /// # Panics + /// Panics if the the type of the reflected value doesn't match. + pub fn map_entities(&self, reflected: &mut dyn PartialReflect, mapper: &mut dyn EntityMapper) { + (self.map_entities)(reflected, mapper); } } -impl FromType for ReflectMapEntities { +impl FromType for ReflectMapEntities { fn from_type() -> Self { ReflectMapEntities { - map_entities: |world, entity_mapper, entities| { - for &entity in entities { - if let Some(mut component) = world.get_mut::(entity) { - component.map_entities(entity_mapper); - } - } - }, - map_all_entities: |world, entity_mapper| { - let entities = entity_mapper - .get_map() - .values() - .copied() - .collect::>(); - for entity in &entities { - if let Some(mut component) = world.get_mut::(*entity) { - component.map_entities(entity_mapper); - } - } - }, - } - } -} - -/// For a specific type of resource, this maps any fields with values of type [`Entity`] to a new world. -/// -/// Since a given `Entity` ID is only valid for the world it came from, when performing deserialization -/// any stored IDs need to be re-allocated in the destination world. -/// -/// See [`SceneEntityMapper`] and [`MapEntities`] for more information. -#[derive(Clone)] -pub struct ReflectMapEntitiesResource { - map_entities: fn(&mut World, &mut SceneEntityMapper), -} - -impl ReflectMapEntitiesResource { - /// A method for applying [`MapEntities`] behavior to elements in an [`EntityHashMap`]. - pub fn map_entities(&self, world: &mut World, entity_map: &mut EntityHashMap) { - SceneEntityMapper::world_scope(entity_map, world, |world, mapper| { - (self.map_entities)(world, mapper); - }); - } -} - -impl FromType for ReflectMapEntitiesResource { - fn from_type() -> Self { - ReflectMapEntitiesResource { - map_entities: |world, entity_mapper| { - if let Some(mut resource) = world.get_resource_mut::() { - resource.map_entities(entity_mapper); - } + map_entities: |reflected, mut mapper| { + let mut concrete = C::from_reflect(reflected).expect("reflected type should match"); + concrete.map_entities(&mut mapper); + reflected.apply(&concrete); }, } } diff --git a/crates/bevy_ecs/src/reflect/mod.rs b/crates/bevy_ecs/src/reflect/mod.rs index 8b7f0ada6edb4..ba27538d2ade9 100644 --- a/crates/bevy_ecs/src/reflect/mod.rs +++ b/crates/bevy_ecs/src/reflect/mod.rs @@ -24,7 +24,7 @@ pub use bundle::{ReflectBundle, ReflectBundleFns}; pub use component::{ReflectComponent, ReflectComponentFns}; pub use entity_commands::ReflectCommandExt; pub use from_world::{ReflectFromWorld, ReflectFromWorldFns}; -pub use map_entities::{ReflectMapEntities, ReflectMapEntitiesResource}; +pub use map_entities::ReflectMapEntities; pub use resource::{ReflectResource, ReflectResourceFns}; pub use visit_entities::{ReflectVisitEntities, ReflectVisitEntitiesMut}; diff --git a/crates/bevy_scene/src/dynamic_scene.rs b/crates/bevy_scene/src/dynamic_scene.rs index 6649ef8ac521f..48ee6493543f0 100644 --- a/crates/bevy_scene/src/dynamic_scene.rs +++ b/crates/bevy_scene/src/dynamic_scene.rs @@ -1,16 +1,15 @@ use crate::{ron, DynamicSceneBuilder, Scene, SceneSpawnError}; +use bevy_asset::Asset; +use bevy_ecs::reflect::ReflectResource; use bevy_ecs::{ - entity::{Entity, EntityHashMap}, + entity::{Entity, EntityHashMap, SceneEntityMapper}, reflect::{AppTypeRegistry, ReflectComponent, ReflectMapEntities}, world::World, }; use bevy_reflect::{PartialReflect, TypePath, TypeRegistry}; -use bevy_utils::TypeIdMap; #[cfg(feature = "serialize")] use crate::serde::SceneSerializer; -use bevy_asset::Asset; -use bevy_ecs::reflect::{ReflectMapEntitiesResource, ReflectResource}; #[cfg(feature = "serialize")] use serde::Serialize; @@ -70,23 +69,26 @@ impl DynamicScene { ) -> Result<(), SceneSpawnError> { let type_registry = type_registry.read(); - // For each component types that reference other entities, we keep track - // of which entities in the scene use that component. - // This is so we can update the scene-internal references to references - // of the actual entities in the world. - let mut scene_mappings: TypeIdMap> = Default::default(); - + // First ensure that every entity in the scene has a corresponding world + // entity in the entity map. for scene_entity in &self.entities { // Fetch the entity with the given entity id from the `entity_map` // or spawn a new entity with a transiently unique id if there is // no corresponding entry. - let entity = *entity_map + entity_map .entry(scene_entity.entity) .or_insert_with(|| world.spawn_empty().id()); - let entity_mut = &mut world.entity_mut(entity); + } + + for scene_entity in &self.entities { + // Fetch the entity with the given entity id from the `entity_map`. + let entity = *entity_map + .get(&scene_entity.entity) + .expect("should have previously spawned an empty entity"); // Apply/ add each component to the given entity. for component in &scene_entity.components { + let mut component = component.clone_value(); let type_info = component.get_represented_type_info().ok_or_else(|| { SceneSpawnError::NoRepresentedType { type_path: component.reflect_type_path().to_string(), @@ -104,39 +106,26 @@ impl DynamicScene { } })?; - // If this component references entities in the scene, track it - // so we can update it to the entity in the world. - if registration.data::().is_some() { - scene_mappings - .entry(registration.type_id()) - .or_default() - .push(entity); + // If this component references entities in the scene, update + // them to the entities in the world. + if let Some(map_entities) = registration.data::() { + SceneEntityMapper::world_scope(entity_map, world, |_, mapper| { + map_entities.map_entities(component.as_partial_reflect_mut(), mapper); + }); } - // If the entity already has the given component attached, - // just apply the (possibly) new value, otherwise add the - // component to the entity. reflect_component.apply_or_insert( - entity_mut, + &mut world.entity_mut(entity), component.as_partial_reflect(), &type_registry, ); } } - // Updates references to entities in the scene to entities in the world - for (type_id, entities) in scene_mappings.into_iter() { - let registration = type_registry.get(type_id).expect( - "we should be getting TypeId from this TypeRegistration in the first place", - ); - if let Some(map_entities_reflect) = registration.data::() { - map_entities_reflect.map_entities(world, entity_map, &entities); - } - } - // Insert resources after all entities have been added to the world. // This ensures the entities are available for the resources to reference during mapping. for resource in &self.resources { + let mut resource = resource.clone_value(); let type_info = resource.get_represented_type_info().ok_or_else(|| { SceneSpawnError::NoRepresentedType { type_path: resource.reflect_type_path().to_string(), @@ -153,14 +142,17 @@ impl DynamicScene { } })?; + // If this component references entities in the scene, update + // them to the entities in the world. + if let Some(map_entities) = registration.data::() { + SceneEntityMapper::world_scope(entity_map, world, |_, mapper| { + map_entities.map_entities(resource.as_partial_reflect_mut(), mapper); + }); + } + // If the world already contains an instance of the given resource // just apply the (possibly) new value, otherwise insert the resource - reflect_resource.apply_or_insert(world, &**resource, &type_registry); - - // Map entities in the resource if it implements [`MapEntities`]. - if let Some(map_entities_reflect) = registration.data::() { - map_entities_reflect.map_entities(world, entity_map); - } + reflect_resource.apply_or_insert(world, resource.as_partial_reflect(), &type_registry); } Ok(()) @@ -210,11 +202,10 @@ where mod tests { use bevy_ecs::{ component::Component, - entity::{Entity, EntityHashMap, EntityMapper, MapEntities, VisitEntities}, - reflect::{ - AppTypeRegistry, ReflectComponent, ReflectMapEntities, ReflectMapEntitiesResource, - ReflectResource, + entity::{ + Entity, EntityHashMap, EntityMapper, MapEntities, VisitEntities, VisitEntitiesMut, }, + reflect::{AppTypeRegistry, ReflectComponent, ReflectMapEntities, ReflectResource}, system::Resource, world::{Command, World}, }; @@ -224,20 +215,13 @@ mod tests { use crate::dynamic_scene::DynamicScene; use crate::dynamic_scene_builder::DynamicSceneBuilder; - #[derive(Resource, Reflect, Debug, VisitEntities)] - #[reflect(Resource, MapEntitiesResource)] + #[derive(Resource, Reflect, Debug, VisitEntities, VisitEntitiesMut)] + #[reflect(Resource, MapEntities)] struct TestResource { entity_a: Entity, entity_b: Entity, } - impl MapEntities for TestResource { - fn map_entities(&mut self, entity_mapper: &mut M) { - self.entity_a = entity_mapper.map_entity(self.entity_a); - self.entity_b = entity_mapper.map_entity(self.entity_b); - } - } - #[test] fn resource_entity_map_maps_entities() { let type_registry = AppTypeRegistry::default(); diff --git a/crates/bevy_scene/src/scene.rs b/crates/bevy_scene/src/scene.rs index 5d0cf57639201..f3a44bf666c3c 100644 --- a/crates/bevy_scene/src/scene.rs +++ b/crates/bevy_scene/src/scene.rs @@ -1,11 +1,11 @@ use crate::{DynamicScene, SceneSpawnError}; use bevy_asset::Asset; use bevy_ecs::{ - entity::{Entity, EntityHashMap}, + entity::{Entity, EntityHashMap, SceneEntityMapper}, reflect::{AppTypeRegistry, ReflectComponent, ReflectMapEntities, ReflectResource}, world::World, }; -use bevy_reflect::TypePath; +use bevy_reflect::{PartialReflect, TypePath}; /// To spawn a scene, you can use either: /// * [`SceneSpawner::spawn`](crate::SceneSpawner::spawn) @@ -90,11 +90,22 @@ impl Scene { reflect_resource.copy(&self.world, world, &type_registry); } + // Ensure that all scene entities have been allocated in the destination + // world before handling components that may contain references that need mapping. for archetype in self.world.archetypes().iter() { for scene_entity in archetype.entities() { - let entity = entity_map + entity_map .entry(scene_entity.id()) .or_insert_with(|| world.spawn_empty().id()); + } + } + + for archetype in self.world.archetypes().iter() { + for scene_entity in archetype.entities() { + let entity = *entity_map + .get(&scene_entity.id()) + .expect("should have previously spawned an entity"); + for component_id in archetype.components() { let component_info = self .world @@ -102,35 +113,41 @@ impl Scene { .get_info(component_id) .expect("component_ids in archetypes should have ComponentInfo"); - let reflect_component = type_registry + let registration = type_registry .get(component_info.type_id().unwrap()) .ok_or_else(|| SceneSpawnError::UnregisteredType { std_type_name: component_info.name().to_string(), - }) - .and_then(|registration| { - registration.data::().ok_or_else(|| { - SceneSpawnError::UnregisteredComponent { - type_path: registration.type_info().type_path().to_string(), - } - }) })?; - reflect_component.copy( - &self.world, - world, - scene_entity.id(), - *entity, + let reflect_component = + registration.data::().ok_or_else(|| { + SceneSpawnError::UnregisteredComponent { + type_path: registration.type_info().type_path().to_string(), + } + })?; + + let Some(mut component) = reflect_component + .reflect(self.world.entity(scene_entity.id())) + .map(PartialReflect::clone_value) + else { + continue; + }; + + // If this component references entities in the scene, + // update them to the entities in the world. + if let Some(map_entities) = registration.data::() { + SceneEntityMapper::world_scope(entity_map, world, |_, mapper| { + map_entities.map_entities(component.as_partial_reflect_mut(), mapper); + }); + } + reflect_component.apply_or_insert( + &mut world.entity_mut(entity), + component.as_partial_reflect(), &type_registry, ); } } } - for registration in type_registry.iter() { - if let Some(map_entities_reflect) = registration.data::() { - map_entities_reflect.map_all_entities(world, entity_map); - } - } - Ok(()) } }