From a29d328fe5cd4d6a5ec3dfd39e97e8e24409c0f3 Mon Sep 17 00:00:00 2001 From: Testare Date: Mon, 1 May 2023 14:40:19 -0700 Subject: [PATCH] Rename map_entities and map_specific_entities (#7570) # Objective After fixing dynamic scene to only map specific entities, we want map_entities to default to the less error prone behavior and have the previous behavior renamed to "map_all_entities." As this is a breaking change, it could not be pushed out with the bug fix. ## Solution Simple rename and refactor. ## Changelog ### Changed - `map_entities` now accepts a list of entities to apply to, with `map_all_entities` retaining previous behavior of applying to all entities in the map. ## Migration Guide - In `bevy_ecs`, `ReflectMapEntities::map_entites` now requires an additional `entities` parameter to specify which entities it applies to. To keep the old behavior, use the new `ReflectMapEntities::map_all_entities`, but consider if passing the entities in specifically might be better for your use case to avoid bugs. --- crates/bevy_ecs/src/reflect.rs | 28 +++++++++++--------------- crates/bevy_scene/src/dynamic_scene.rs | 2 +- crates/bevy_scene/src/scene.rs | 2 +- 3 files changed, 14 insertions(+), 18 deletions(-) diff --git a/crates/bevy_ecs/src/reflect.rs b/crates/bevy_ecs/src/reflect.rs index c80fb9ae586fa..62be9c9ae4198 100644 --- a/crates/bevy_ecs/src/reflect.rs +++ b/crates/bevy_ecs/src/reflect.rs @@ -412,8 +412,8 @@ impl_from_reflect_value!(Entity); #[derive(Clone)] pub struct ReflectMapEntities { - map_entities: fn(&mut World, &mut EntityMapper), - map_specific_entities: fn(&mut World, &mut EntityMapper, &[Entity]), + map_all_entities: fn(&mut World, &mut EntityMapper), + map_entities: fn(&mut World, &mut EntityMapper, &[Entity]), } impl ReflectMapEntities { @@ -424,25 +424,21 @@ impl ReflectMapEntities { /// by the [`EntityMap`] 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_entities`](Self::map_entities), those `Parent` + /// 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_entities(&self, world: &mut World, entity_map: &mut EntityMap) { - entity_map.world_scope(world, self.map_entities); + pub fn map_all_entities(&self, world: &mut World, entity_map: &mut EntityMap) { + entity_map.world_scope(world, self.map_all_entities); } - /// This is like [`map_entities`](Self::map_entities), but only applied to specific entities, not all values + /// A general method for applying [`MapEntities`] behavior to elements in an [`EntityMap`]. Unlike + /// [`map_all_entities`](Self::map_all_entities), this is applied to specific entities, not all values /// in the [`EntityMap`]. /// /// This is useful mostly for when you need to be careful not to update components that already contain valid entity - /// values. See [`map_entities`](Self::map_entities) for more details. - pub fn map_specific_entities( - &self, - world: &mut World, - entity_map: &mut EntityMap, - entities: &[Entity], - ) { + /// values. See [`map_all_entities`](Self::map_all_entities) for more details. + pub fn map_entities(&self, world: &mut World, entity_map: &mut EntityMap, entities: &[Entity]) { entity_map.world_scope(world, |world, mapper| { - (self.map_specific_entities)(world, mapper, entities); + (self.map_entities)(world, mapper, entities); }); } } @@ -450,14 +446,14 @@ impl ReflectMapEntities { impl FromType for ReflectMapEntities { fn from_type() -> Self { ReflectMapEntities { - map_specific_entities: |world, entity_mapper, entities| { + 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_entities: |world, entity_mapper| { + map_all_entities: |world, entity_mapper| { let entities = entity_mapper.get_map().values().collect::>(); for entity in &entities { if let Some(mut component) = world.get_mut::(*entity) { diff --git a/crates/bevy_scene/src/dynamic_scene.rs b/crates/bevy_scene/src/dynamic_scene.rs index 7bbdb07cb9f20..4acf375d1288f 100644 --- a/crates/bevy_scene/src/dynamic_scene.rs +++ b/crates/bevy_scene/src/dynamic_scene.rs @@ -142,7 +142,7 @@ impl DynamicScene { "we should be getting TypeId from this TypeRegistration in the first place", ); if let Some(map_entities_reflect) = registration.data::() { - map_entities_reflect.map_specific_entities(world, entity_map, &entities); + map_entities_reflect.map_entities(world, entity_map, &entities); } } diff --git a/crates/bevy_scene/src/scene.rs b/crates/bevy_scene/src/scene.rs index 2e5a342e40e77..741f570314004 100644 --- a/crates/bevy_scene/src/scene.rs +++ b/crates/bevy_scene/src/scene.rs @@ -120,7 +120,7 @@ impl Scene { for registration in type_registry.iter() { if let Some(map_entities_reflect) = registration.data::() { - map_entities_reflect.map_entities(world, &mut instance_info.entity_map); + map_entities_reflect.map_all_entities(world, &mut instance_info.entity_map); } }