From fffc645c881e2822ae3436e694bc2cdc85361846 Mon Sep 17 00:00:00 2001
From: Testare <testare.i9z@gmail.com>
Date: Tue, 7 Feb 2023 19:05:26 -0800
Subject: [PATCH 01/13] Bugfix: Scene reloading corruption

When a scene was reloaded, it was corrupting components that
weren't native to the scene itself. In particular, when a DynamicScene
was created and Parent components added to entities without parents so
that they were children of the entity that spawne them, if that scene
was reloaded and the ID of the DyanmicSceneSpawner entity was in the
scene, that parent component was corrupted, causing the
hierarchy to become malformed and bevy to panic.
---
 crates/bevy_ecs/src/reflect.rs         | 17 +++++++++++++----
 crates/bevy_scene/src/dynamic_scene.rs | 21 +++++++++++++++++++--
 2 files changed, 32 insertions(+), 6 deletions(-)

diff --git a/crates/bevy_ecs/src/reflect.rs b/crates/bevy_ecs/src/reflect.rs
index 927973dea28bc..7718bddff3f35 100644
--- a/crates/bevy_ecs/src/reflect.rs
+++ b/crates/bevy_ecs/src/reflect.rs
@@ -412,7 +412,7 @@ impl_from_reflect_value!(Entity);
 
 #[derive(Clone)]
 pub struct ReflectMapEntities {
-    map_entities: fn(&mut World, &EntityMap) -> Result<(), MapEntitiesError>,
+    map_entities: fn(&mut World, &EntityMap, Vec<Entity>) -> Result<(), MapEntitiesError>,
 }
 
 impl ReflectMapEntities {
@@ -421,15 +421,24 @@ impl ReflectMapEntities {
         world: &mut World,
         entity_map: &EntityMap,
     ) -> Result<(), MapEntitiesError> {
-        (self.map_entities)(world, entity_map)
+        (self.map_entities)(world, entity_map, entity_map.values().collect())
+    }
+
+    pub fn map_specific_entities(
+        &self,
+        world: &mut World,
+        entity_map: &EntityMap,
+        entities: Vec<Entity>
+    )  -> Result<(), MapEntitiesError> {
+        (self.map_entities)(world, entity_map, entities)
     }
 }
 
 impl<C: Component + MapEntities> FromType<C> for ReflectMapEntities {
     fn from_type() -> Self {
         ReflectMapEntities {
-            map_entities: |world, entity_map| {
-                for entity in entity_map.values() {
+            map_entities: |world, entity_map, entities| {
+                for entity in entities {
                     if let Some(mut component) = world.get_mut::<C>(entity) {
                         component.map_entities(entity_map)?;
                     }
diff --git a/crates/bevy_scene/src/dynamic_scene.rs b/crates/bevy_scene/src/dynamic_scene.rs
index adac64c425c83..9da65827751df 100644
--- a/crates/bevy_scene/src/dynamic_scene.rs
+++ b/crates/bevy_scene/src/dynamic_scene.rs
@@ -7,6 +7,7 @@ use bevy_ecs::{
     world::World,
 };
 use bevy_reflect::{Reflect, TypeRegistryArc, TypeUuid};
+use bevy_utils::HashMap;
 
 #[cfg(feature = "serialize")]
 use crate::serde::SceneSerializer;
@@ -86,6 +87,11 @@ impl DynamicScene {
             reflect_resource.apply_or_insert(world, &**resource);
         }
 
+        // Collection of entities that have references to entities in the scene,
+        // that need to be updated to entities in the world.
+        // Keyed by Component's TypeId.
+        let mut entity_mapped_entities = HashMap::default();
+
         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
@@ -109,6 +115,15 @@ 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::<ReflectMapEntities>().is_some() {
+                    entity_mapped_entities
+                        .entry(registration.type_id())
+                        .or_insert(Vec::new())
+                        .push(entity);
+                }
+
                 // If the entity already has the given component attached,
                 // just apply the (possibly) new value, otherwise add the
                 // component to the entity.
@@ -116,10 +131,12 @@ impl DynamicScene {
             }
         }
 
-        for registration in type_registry.iter() {
+        // Updates references to entities in the scene to entities in the world
+        for (type_id, entities) in entity_mapped_entities.into_iter() {
+            let registration = type_registry.get(type_id).unwrap();
             if let Some(map_entities_reflect) = registration.data::<ReflectMapEntities>() {
                 map_entities_reflect
-                    .map_entities(world, entity_map)
+                    .map_specific_entities(world, entity_map, entities)
                     .unwrap();
             }
         }

From a58df059b76fa0b53f56bcbebc7f24ab166504d0 Mon Sep 17 00:00:00 2001
From: Testare <testare.i9z@gmail.com>
Date: Wed, 8 Feb 2023 11:05:09 -0800
Subject: [PATCH 02/13] fix formatting

---
 crates/bevy_ecs/src/reflect.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/crates/bevy_ecs/src/reflect.rs b/crates/bevy_ecs/src/reflect.rs
index 7718bddff3f35..9943bcea2a33b 100644
--- a/crates/bevy_ecs/src/reflect.rs
+++ b/crates/bevy_ecs/src/reflect.rs
@@ -428,8 +428,8 @@ impl ReflectMapEntities {
         &self,
         world: &mut World,
         entity_map: &EntityMap,
-        entities: Vec<Entity>
-    )  -> Result<(), MapEntitiesError> {
+        entities: Vec<Entity>,
+    ) -> Result<(), MapEntitiesError> {
         (self.map_entities)(world, entity_map, entities)
     }
 }

From 94f57c2892d0f8a17b1b96671c7d571af0303905 Mon Sep 17 00:00:00 2001
From: Testare <testare.i9z@gmail.com>
Date: Tue, 14 Feb 2023 18:45:37 -0800
Subject: [PATCH 03/13] Added unit test

---
 crates/bevy_scene/src/dynamic_scene.rs | 81 ++++++++++++++++++++++++++
 1 file changed, 81 insertions(+)

diff --git a/crates/bevy_scene/src/dynamic_scene.rs b/crates/bevy_scene/src/dynamic_scene.rs
index 9da65827751df..d9829b7e156ea 100644
--- a/crates/bevy_scene/src/dynamic_scene.rs
+++ b/crates/bevy_scene/src/dynamic_scene.rs
@@ -177,3 +177,84 @@ where
         .new_line("\n".to_string());
     ron::ser::to_string_pretty(&serialize, pretty_config)
 }
+
+#[cfg(test)]
+mod tests {
+    use bevy_app::AppTypeRegistry;
+    use bevy_ecs::{entity::EntityMap, system::Command, world::World};
+    use bevy_hierarchy::{AddChild, Parent};
+
+    use crate::dynamic_scene_builder::DynamicSceneBuilder;
+
+    #[test]
+    fn components_not_defined_in_scene_should_not_be_effected_by_scene_entity_map() {
+        // Testing that scene reloading applies EntitiyMap correctly to MapEntities components.
+
+        // First, we create a simple world with a parent and a child relationship
+        let mut world = World::new();
+        world.init_resource::<AppTypeRegistry>();
+        world
+            .resource_mut::<AppTypeRegistry>()
+            .write()
+            .register::<Parent>();
+        let gen_0_entity = world.spawn_empty().id();
+        let gen_1_entity = world.spawn_empty().id();
+        AddChild {
+            parent: gen_0_entity,
+            child: gen_1_entity,
+        }
+        .write(&mut world);
+
+        // We then write this relationship to a new scene, and then write that scene back to the world to create another parent and child relationship
+        let mut scene_builder = DynamicSceneBuilder::from_world(&world);
+        scene_builder.extract_entity(gen_0_entity);
+        scene_builder.extract_entity(gen_1_entity);
+        let scene = scene_builder.build();
+        let mut entity_map = EntityMap::default();
+        scene.write_to_world(&mut world, &mut entity_map).unwrap();
+
+        // We then add the parent in the scene relationship (gen_2) as a child of the first child relationship (gen_1)
+        let gen_2_entity = entity_map.get(gen_0_entity).unwrap();
+        let gen_3_entity = entity_map.get(gen_1_entity).unwrap();
+        AddChild {
+            parent: gen_1_entity,
+            child: gen_2_entity,
+        }
+        .write(&mut world);
+
+        // We then reload the scene to make sure that gen_2_entity's parent component isn't updated with the entity map, since this component isn't defined in the scene.
+        // With bevy_hierarchy, this can cause serious errors and malformed hierarchies.
+        scene.write_to_world(&mut world, &mut entity_map).unwrap();
+
+        assert_eq!(
+            gen_0_entity,
+            world
+                .get_entity(gen_1_entity)
+                .unwrap()
+                .get::<Parent>()
+                .unwrap()
+                .get(),
+            "Something about reloading the scene is touching entities with the same scene Ids"
+        );
+        assert_eq!(
+            gen_1_entity,
+            world
+                .get_entity(gen_2_entity)
+                .unwrap()
+                .get::<Parent>()
+                .unwrap()
+                .get(),
+            "Something about reloading the scene is touching components not defined in the scene but on entities defined in the scene"
+        );
+        assert_eq!(
+            gen_2_entity,
+            world
+                .get_entity(gen_3_entity)
+                .unwrap()
+                .get::<Parent>()
+                .expect("Something is wrong with this test, and the scene components don't have a parent/child relationship")
+                .get(),
+            "Something is wrong with the this test or the code reloading scenes since the relationship between scene entities is broken"
+        );
+    }
+}

From 09c64d93bdaeae7c2f91eadf53bbe46ca93f6416 Mon Sep 17 00:00:00 2001
From: Testare <testare.i9z@gmail.com>
Date: Mon, 20 Feb 2023 09:07:43 -0800
Subject: [PATCH 04/13] Corrections from feedback

---
 crates/bevy_ecs/src/reflect.rs         |  6 +++
 crates/bevy_scene/src/dynamic_scene.rs | 63 +++++++++++++++-----------
 2 files changed, 42 insertions(+), 27 deletions(-)

diff --git a/crates/bevy_ecs/src/reflect.rs b/crates/bevy_ecs/src/reflect.rs
index 9943bcea2a33b..a4e80755bded5 100644
--- a/crates/bevy_ecs/src/reflect.rs
+++ b/crates/bevy_ecs/src/reflect.rs
@@ -424,6 +424,12 @@ impl ReflectMapEntities {
         (self.map_entities)(world, entity_map, entity_map.values().collect())
     }
 
+    /// This is like map_entities, but only applied to specific entities, not all values in the EntityMap.
+    ///
+    /// This is useful for when you only want to apply the MapEntity logic to specific entities. For example,
+    /// 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 and used plain `map_entities`, those Parent components
+    /// with already valid entity references could be updated to point at something else entirely.
     pub fn map_specific_entities(
         &self,
         world: &mut World,
diff --git a/crates/bevy_scene/src/dynamic_scene.rs b/crates/bevy_scene/src/dynamic_scene.rs
index d9829b7e156ea..5550b5b3b2ecf 100644
--- a/crates/bevy_scene/src/dynamic_scene.rs
+++ b/crates/bevy_scene/src/dynamic_scene.rs
@@ -1,8 +1,11 @@
+use std::any::TypeId;
+
 use crate::{DynamicSceneBuilder, Scene, SceneSpawnError};
 use anyhow::Result;
 use bevy_app::AppTypeRegistry;
 use bevy_ecs::{
     entity::EntityMap,
+    prelude::Entity,
     reflect::{ReflectComponent, ReflectMapEntities},
     world::World,
 };
@@ -87,10 +90,11 @@ impl DynamicScene {
             reflect_resource.apply_or_insert(world, &**resource);
         }
 
-        // Collection of entities that have references to entities in the scene,
-        // that need to be updated to entities in the world.
-        // Keyed by Component's TypeId.
-        let mut entity_mapped_entities = HashMap::default();
+        // 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: HashMap<TypeId, Vec<Entity>> = HashMap::default();
 
         for scene_entity in &self.entities {
             // Fetch the entity with the given entity id from the `entity_map`
@@ -118,7 +122,7 @@ 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::<ReflectMapEntities>().is_some() {
-                    entity_mapped_entities
+                    scene_mappings
                         .entry(registration.type_id())
                         .or_insert(Vec::new())
                         .push(entity);
@@ -132,7 +136,7 @@ impl DynamicScene {
         }
 
         // Updates references to entities in the scene to entities in the world
-        for (type_id, entities) in entity_mapped_entities.into_iter() {
+        for (type_id, entities) in scene_mappings.into_iter() {
             let registration = type_registry.get(type_id).unwrap();
             if let Some(map_entities_reflect) = registration.data::<ReflectMapEntities>() {
                 map_entities_reflect
@@ -187,8 +191,8 @@ mod tests {
     use crate::dynamic_scene_builder::DynamicSceneBuilder;
 
     #[test]
-    fn components_not_defined_in_scene_should_not_be_effected_by_scene_entity_map() {
-        // Testing that scene reloading applies EntitiyMap correctly to MapEntities components.
+    fn components_not_defined_in_scene_should_not_be_affected_by_scene_entity_map() {
+        // Testing that scene reloading applies EntityMap correctly to MapEntities components.
 
         // First, we create a simple world with a parent and a child relationship
         let mut world = World::new();
@@ -197,39 +201,44 @@ mod tests {
             .resource_mut::<AppTypeRegistry>()
             .write()
             .register::<Parent>();
-        let gen_0_entity = world.spawn_empty().id();
-        let gen_1_entity = world.spawn_empty().id();
+        let original_parent_entity = world.spawn_empty().id();
+        let original_child_entity = world.spawn_empty().id();
         AddChild {
-            parent: gen_0_entity,
-            child: gen_1_entity,
+            parent: original_parent_entity,
+            child: original_child_entity,
         }
         .write(&mut world);
 
-        // We then write this relationship to a new scene, and then write that scene back to the world to create another parent and child relationship
+        // We then write this relationship to a new scene, and then write that scene back to the
+        // world to create another parent and child relationship
         let mut scene_builder = DynamicSceneBuilder::from_world(&world);
-        scene_builder.extract_entity(gen_0_entity);
-        scene_builder.extract_entity(gen_1_entity);
+        scene_builder.extract_entity(original_parent_entity);
+        scene_builder.extract_entity(original_child_entity);
         let scene = scene_builder.build();
         let mut entity_map = EntityMap::default();
         scene.write_to_world(&mut world, &mut entity_map).unwrap();
 
-        // We then add the parent in the scene relationship (gen_2) as a child of the first child relationship (gen_1)
-        let gen_2_entity = entity_map.get(gen_0_entity).unwrap();
-        let gen_3_entity = entity_map.get(gen_1_entity).unwrap();
+        let from_scene_parent_entity = entity_map.get(original_parent_entity).unwrap();
+        let from_scene_child_entity = entity_map.get(original_child_entity).unwrap();
+
+        // We then add the parent from the scene as a child of the original child
+        // Hierarchy should look like:
+        // Original Parent <- Original Child <- Scene Parent <- Scene Child
         AddChild {
-            parent: gen_1_entity,
-            child: gen_2_entity,
+            parent: original_child_entity,
+            child: from_scene_parent_entity,
         }
         .write(&mut world);
 
-        // We then reload the scene to make sure that gen_2_entity's parent component isn't updated with the entity map, since this component isn't defined in the scene.
+        // We then reload the scene to make sure that from_scene_parent_entity's parent component
+        // isn't updated with the entity map, since this component isn't defined in the scene.
         // With bevy_hierarchy, this can cause serious errors and malformed hierarchies.
         scene.write_to_world(&mut world, &mut entity_map).unwrap();
 
         assert_eq!(
-            gen_0_entity,
+            original_parent_entity,
             world
-                .get_entity(gen_1_entity)
+                .get_entity(original_child_entity)
                 .unwrap()
                 .get::<Parent>()
                 .unwrap()
@@ -237,9 +246,9 @@ mod tests {
             "Something about reloading the scene is touching entities with the same scene Ids"
         );
         assert_eq!(
-            gen_1_entity,
+            original_child_entity,
             world
-                .get_entity(gen_2_entity)
+                .get_entity(from_scene_parent_entity)
                 .unwrap()
                 .get::<Parent>()
                 .unwrap()
@@ -247,9 +256,9 @@ mod tests {
             "Something about reloading the scene is touching components not defined in the scene but on entities defined in the scene"
         );
         assert_eq!(
-            gen_2_entity,
+            from_scene_parent_entity,
             world
-                .get_entity(gen_3_entity)
+                .get_entity(from_scene_child_entity)
                 .unwrap()
                 .get::<Parent>()
                 .expect("Something is wrong with this test, and the scene components don't have a parent/child relationship")

From f0382b90595a0ecaf62632f2fb931f3dcb45c441 Mon Sep 17 00:00:00 2001
From: Testare <testare.i9z@gmail.com>
Date: Mon, 20 Feb 2023 09:14:45 -0800
Subject: [PATCH 05/13] missing backticks

---
 crates/bevy_ecs/src/reflect.rs | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/crates/bevy_ecs/src/reflect.rs b/crates/bevy_ecs/src/reflect.rs
index a4e80755bded5..8cec091d075a5 100644
--- a/crates/bevy_ecs/src/reflect.rs
+++ b/crates/bevy_ecs/src/reflect.rs
@@ -424,11 +424,11 @@ impl ReflectMapEntities {
         (self.map_entities)(world, entity_map, entity_map.values().collect())
     }
 
-    /// This is like map_entities, but only applied to specific entities, not all values in the EntityMap.
+    /// This is like `map_entities`, but only applied to specific entities, not all values in the `EntityMap`.
     ///
-    /// This is useful for when you only want to apply the MapEntity logic to specific entities. For example,
-    /// 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 and used plain `map_entities`, those Parent components
+    /// This is useful for when you only want to apply the `MapEntity` logic to specific entities. For example,
+    /// 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 and used plain `map_entities`, those `Parent` components
     /// with already valid entity references could be updated to point at something else entirely.
     pub fn map_specific_entities(
         &self,

From 1ac25ac93fe8070726336ffaa2c1b71e0e095f5f Mon Sep 17 00:00:00 2001
From: Testare <testare.i9z@gmail.com>
Date: Tue, 21 Feb 2023 08:37:31 -0800
Subject: [PATCH 06/13] no more mr. map_specific_entities

---
 crates/bevy_ecs/src/reflect.rs         | 20 +++-----------------
 crates/bevy_scene/src/dynamic_scene.rs |  2 +-
 crates/bevy_scene/src/scene.rs         |  4 +++-
 3 files changed, 7 insertions(+), 19 deletions(-)

diff --git a/crates/bevy_ecs/src/reflect.rs b/crates/bevy_ecs/src/reflect.rs
index 8cec091d075a5..4e2f9033f817a 100644
--- a/crates/bevy_ecs/src/reflect.rs
+++ b/crates/bevy_ecs/src/reflect.rs
@@ -412,7 +412,7 @@ impl_from_reflect_value!(Entity);
 
 #[derive(Clone)]
 pub struct ReflectMapEntities {
-    map_entities: fn(&mut World, &EntityMap, Vec<Entity>) -> Result<(), MapEntitiesError>,
+    map_entities: fn(&mut World, &EntityMap, &Vec<Entity>) -> Result<(), MapEntitiesError>,
 }
 
 impl ReflectMapEntities {
@@ -420,21 +420,7 @@ impl ReflectMapEntities {
         &self,
         world: &mut World,
         entity_map: &EntityMap,
-    ) -> Result<(), MapEntitiesError> {
-        (self.map_entities)(world, entity_map, entity_map.values().collect())
-    }
-
-    /// This is like `map_entities`, but only applied to specific entities, not all values in the `EntityMap`.
-    ///
-    /// This is useful for when you only want to apply the `MapEntity` logic to specific entities. For example,
-    /// 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 and used plain `map_entities`, those `Parent` components
-    /// with already valid entity references could be updated to point at something else entirely.
-    pub fn map_specific_entities(
-        &self,
-        world: &mut World,
-        entity_map: &EntityMap,
-        entities: Vec<Entity>,
+        entities: &Vec<Entity>,
     ) -> Result<(), MapEntitiesError> {
         (self.map_entities)(world, entity_map, entities)
     }
@@ -444,7 +430,7 @@ impl<C: Component + MapEntities> FromType<C> for ReflectMapEntities {
     fn from_type() -> Self {
         ReflectMapEntities {
             map_entities: |world, entity_map, entities| {
-                for entity in entities {
+                for &entity in entities {
                     if let Some(mut component) = world.get_mut::<C>(entity) {
                         component.map_entities(entity_map)?;
                     }
diff --git a/crates/bevy_scene/src/dynamic_scene.rs b/crates/bevy_scene/src/dynamic_scene.rs
index 5550b5b3b2ecf..fb6137459aaa5 100644
--- a/crates/bevy_scene/src/dynamic_scene.rs
+++ b/crates/bevy_scene/src/dynamic_scene.rs
@@ -140,7 +140,7 @@ impl DynamicScene {
             let registration = type_registry.get(type_id).unwrap();
             if let Some(map_entities_reflect) = registration.data::<ReflectMapEntities>() {
                 map_entities_reflect
-                    .map_specific_entities(world, entity_map, entities)
+                    .map_entities(world, entity_map, &entities)
                     .unwrap();
             }
         }
diff --git a/crates/bevy_scene/src/scene.rs b/crates/bevy_scene/src/scene.rs
index 460f7cf8b2f3a..d41f1685b746e 100644
--- a/crates/bevy_scene/src/scene.rs
+++ b/crates/bevy_scene/src/scene.rs
@@ -117,10 +117,12 @@ impl Scene {
                 }
             }
         }
+
+        let entities_from_scene: Vec<_> = instance_info.entity_map.values().collect();
         for registration in type_registry.iter() {
             if let Some(map_entities_reflect) = registration.data::<ReflectMapEntities>() {
                 map_entities_reflect
-                    .map_entities(world, &instance_info.entity_map)
+                    .map_entities(world, &instance_info.entity_map, &entities_from_scene)
                     .unwrap();
             }
         }

From db3503732b4acdd802b5d1fbe8fc0f21f301a165 Mon Sep 17 00:00:00 2001
From: Testare <testare.i9z@gmail.com>
Date: Tue, 21 Feb 2023 19:59:42 -0800
Subject: [PATCH 07/13] Welcome Mr. map_all_entities

---
 crates/bevy_ecs/src/reflect.rs | 44 ++++++++++++++++++++++++++++------
 crates/bevy_scene/src/scene.rs |  3 +--
 2 files changed, 38 insertions(+), 9 deletions(-)

diff --git a/crates/bevy_ecs/src/reflect.rs b/crates/bevy_ecs/src/reflect.rs
index 4e2f9033f817a..a3dbb0a5251bb 100644
--- a/crates/bevy_ecs/src/reflect.rs
+++ b/crates/bevy_ecs/src/reflect.rs
@@ -412,27 +412,57 @@ impl_from_reflect_value!(Entity);
 
 #[derive(Clone)]
 pub struct ReflectMapEntities {
-    map_entities: fn(&mut World, &EntityMap, &Vec<Entity>) -> Result<(), MapEntitiesError>,
+    map_entities: fn(&mut World, &EntityMap, Option<&[Entity]>) -> Result<(), MapEntitiesError>,
 }
 
 impl ReflectMapEntities {
+    /// A general method for applying MapEntity behavior to all elements in an [`EntityMap`].
+    ///
+    /// Be mindful in its usage: Works best in situations where the entities in the [`EntityMap`] are newly
+    /// created, before systems have a chance to add new components. If some of the entities referred to
+    /// 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_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: &EntityMap,
+    ) -> Result<(), MapEntitiesError> {
+        (self.map_entities)(world, entity_map, None)
+    }
+
+    /// This is like [`map_all_entities`](Self::map_all_entities), but only 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_all_entities`](Self::map_all_entities) for more details.
     pub fn map_entities(
         &self,
         world: &mut World,
         entity_map: &EntityMap,
-        entities: &Vec<Entity>,
+        entities: &[Entity],
     ) -> Result<(), MapEntitiesError> {
-        (self.map_entities)(world, entity_map, entities)
+        (self.map_entities)(world, entity_map, Some(entities))
     }
 }
 
 impl<C: Component + MapEntities> FromType<C> for ReflectMapEntities {
     fn from_type() -> Self {
         ReflectMapEntities {
-            map_entities: |world, entity_map, entities| {
-                for &entity in entities {
-                    if let Some(mut component) = world.get_mut::<C>(entity) {
-                        component.map_entities(entity_map)?;
+            map_entities: |world, entity_map, entities_opt| {
+                if let Some(entities) = entities_opt {
+                    for &entity in entities {
+                        if let Some(mut component) = world.get_mut::<C>(entity) {
+                            component.map_entities(entity_map)?;
+                        }
+                    }
+                } else {
+                    for entity in entity_map.values() {
+                        if let Some(mut component) = world.get_mut::<C>(entity) {
+                            component.map_entities(entity_map)?;
+                        }
                     }
                 }
                 Ok(())
diff --git a/crates/bevy_scene/src/scene.rs b/crates/bevy_scene/src/scene.rs
index d41f1685b746e..36d8cf9e593af 100644
--- a/crates/bevy_scene/src/scene.rs
+++ b/crates/bevy_scene/src/scene.rs
@@ -118,11 +118,10 @@ impl Scene {
             }
         }
 
-        let entities_from_scene: Vec<_> = instance_info.entity_map.values().collect();
         for registration in type_registry.iter() {
             if let Some(map_entities_reflect) = registration.data::<ReflectMapEntities>() {
                 map_entities_reflect
-                    .map_entities(world, &instance_info.entity_map, &entities_from_scene)
+                    .map_all_entities(world, &instance_info.entity_map)
                     .unwrap();
             }
         }

From af8709b48fb93604b882fdc1475a88454ce7f053 Mon Sep 17 00:00:00 2001
From: Testare <testare.i9z@gmail.com>
Date: Tue, 21 Feb 2023 20:44:36 -0800
Subject: [PATCH 08/13] docfix

---
 crates/bevy_ecs/src/reflect.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/crates/bevy_ecs/src/reflect.rs b/crates/bevy_ecs/src/reflect.rs
index a3dbb0a5251bb..03277f1f875ca 100644
--- a/crates/bevy_ecs/src/reflect.rs
+++ b/crates/bevy_ecs/src/reflect.rs
@@ -416,7 +416,7 @@ pub struct ReflectMapEntities {
 }
 
 impl ReflectMapEntities {
-    /// A general method for applying MapEntity behavior to all elements in an [`EntityMap`].
+    /// A general method for applying [`MapEntity`] behavior to all elements in an [`EntityMap`].
     ///
     /// Be mindful in its usage: Works best in situations where the entities in the [`EntityMap`] are newly
     /// created, before systems have a chance to add new components. If some of the entities referred to

From 82f8299a9c7e35d5b991e45f9af36daec0cdf880 Mon Sep 17 00:00:00 2001
From: Testare <testare.i9z@gmail.com>
Date: Tue, 21 Feb 2023 20:53:21 -0800
Subject: [PATCH 09/13] docfix 2

---
 crates/bevy_ecs/src/reflect.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/crates/bevy_ecs/src/reflect.rs b/crates/bevy_ecs/src/reflect.rs
index 03277f1f875ca..81dcabe5f830f 100644
--- a/crates/bevy_ecs/src/reflect.rs
+++ b/crates/bevy_ecs/src/reflect.rs
@@ -416,14 +416,14 @@ pub struct ReflectMapEntities {
 }
 
 impl ReflectMapEntities {
-    /// A general method for applying [`MapEntity`] behavior to all elements in an [`EntityMap`].
+    /// A general method for applying [`MapEntities`] behavior to all elements in an [`EntityMap`].
     ///
     /// Be mindful in its usage: Works best in situations where the entities in the [`EntityMap`] are newly
     /// created, before systems have a chance to add new components. If some of the entities referred to
     /// 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_all_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_all_entities(
         &self,

From e279c732d0e6f5cf61b571a560ef18c9cde3351d Mon Sep 17 00:00:00 2001
From: Testare <testare.i9z@gmail.com>
Date: Wed, 22 Feb 2023 08:07:31 -0800
Subject: [PATCH 10/13] expect and two closures

---
 crates/bevy_ecs/src/reflect.rs         | 29 +++++++++++++-------------
 crates/bevy_scene/src/dynamic_scene.rs |  3 ++-
 2 files changed, 17 insertions(+), 15 deletions(-)

diff --git a/crates/bevy_ecs/src/reflect.rs b/crates/bevy_ecs/src/reflect.rs
index 81dcabe5f830f..1e623068f69ad 100644
--- a/crates/bevy_ecs/src/reflect.rs
+++ b/crates/bevy_ecs/src/reflect.rs
@@ -412,7 +412,8 @@ impl_from_reflect_value!(Entity);
 
 #[derive(Clone)]
 pub struct ReflectMapEntities {
-    map_entities: fn(&mut World, &EntityMap, Option<&[Entity]>) -> Result<(), MapEntitiesError>,
+    map_all_entities: fn(&mut World, &EntityMap) -> Result<(), MapEntitiesError>,
+    map_entities: fn(&mut World, &EntityMap, &[Entity]) -> Result<(), MapEntitiesError>,
 }
 
 impl ReflectMapEntities {
@@ -430,7 +431,7 @@ impl ReflectMapEntities {
         world: &mut World,
         entity_map: &EntityMap,
     ) -> Result<(), MapEntitiesError> {
-        (self.map_entities)(world, entity_map, None)
+        (self.map_all_entities)(world, entity_map)
     }
 
     /// This is like [`map_all_entities`](Self::map_all_entities), but only applied to specific entities, not all values
@@ -444,25 +445,25 @@ impl ReflectMapEntities {
         entity_map: &EntityMap,
         entities: &[Entity],
     ) -> Result<(), MapEntitiesError> {
-        (self.map_entities)(world, entity_map, Some(entities))
+        (self.map_entities)(world, entity_map, entities)
     }
 }
 
 impl<C: Component + MapEntities> FromType<C> for ReflectMapEntities {
     fn from_type() -> Self {
         ReflectMapEntities {
-            map_entities: |world, entity_map, entities_opt| {
-                if let Some(entities) = entities_opt {
-                    for &entity in entities {
-                        if let Some(mut component) = world.get_mut::<C>(entity) {
-                            component.map_entities(entity_map)?;
-                        }
+            map_entities: |world, entity_map, entities| {
+                for &entity in entities {
+                    if let Some(mut component) = world.get_mut::<C>(entity) {
+                        component.map_entities(entity_map)?;
                     }
-                } else {
-                    for entity in entity_map.values() {
-                        if let Some(mut component) = world.get_mut::<C>(entity) {
-                            component.map_entities(entity_map)?;
-                        }
+                }
+                Ok(())
+            },
+            map_all_entities: |world, entity_map| {
+                for entity in entity_map.values() {
+                    if let Some(mut component) = world.get_mut::<C>(entity) {
+                        component.map_entities(entity_map)?;
                     }
                 }
                 Ok(())
diff --git a/crates/bevy_scene/src/dynamic_scene.rs b/crates/bevy_scene/src/dynamic_scene.rs
index fb6137459aaa5..1f725df23eca9 100644
--- a/crates/bevy_scene/src/dynamic_scene.rs
+++ b/crates/bevy_scene/src/dynamic_scene.rs
@@ -137,7 +137,8 @@ impl DynamicScene {
 
         // 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).unwrap();
+            let registration = type_registry.get(type_id)
+                .expect("This TypeRegistration was where we got the TypeId initially, should be safe to unwrap");
             if let Some(map_entities_reflect) = registration.data::<ReflectMapEntities>() {
                 map_entities_reflect
                     .map_entities(world, entity_map, &entities)

From ffd5e048da397ea2f6538d935ae18243eaf57342 Mon Sep 17 00:00:00 2001
From: Testare <testare.i9z@gmail.com>
Date: Wed, 22 Feb 2023 08:32:20 -0800
Subject: [PATCH 11/13] Bugfix: Scene reload corruption (Non-breaking)

---
 crates/bevy_ecs/src/reflect.rs         | 22 +++++++++++-----------
 crates/bevy_scene/src/dynamic_scene.rs |  7 ++++---
 crates/bevy_scene/src/scene.rs         |  2 +-
 3 files changed, 16 insertions(+), 15 deletions(-)

diff --git a/crates/bevy_ecs/src/reflect.rs b/crates/bevy_ecs/src/reflect.rs
index 1e623068f69ad..91781e74fc90e 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_all_entities: fn(&mut World, &EntityMap) -> Result<(), MapEntitiesError>,
-    map_entities: fn(&mut World, &EntityMap, &[Entity]) -> Result<(), MapEntitiesError>,
+    map_entities: fn(&mut World, &EntityMap) -> Result<(), MapEntitiesError>,
+    map_specific_entities: fn(&mut World, &EntityMap, &[Entity]) -> Result<(), MapEntitiesError>,
 }
 
 impl ReflectMapEntities {
@@ -424,35 +424,35 @@ 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_all_entities`](Self::map_all_entities), those `Parent`
+    /// to these entities after they have been loaded. If you reload the scene using [`map_entities`](Self::map_entities), those `Parent`
     /// components with already valid entity references could be updated to point at something else entirely.
-    pub fn map_all_entities(
+    pub fn map_entities(
         &self,
         world: &mut World,
         entity_map: &EntityMap,
     ) -> Result<(), MapEntitiesError> {
-        (self.map_all_entities)(world, entity_map)
+        (self.map_entities)(world, entity_map)
     }
 
-    /// This is like [`map_all_entities`](Self::map_all_entities), but only applied to specific entities, not all values
+    /// This is like [`map_entities`](Self::map_entities), but only 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_all_entities`](Self::map_all_entities) for more details.
-    pub fn map_entities(
+    /// values. See [`map_entities`](Self::map_entities) for more details.
+    pub fn map_specific_entities(
         &self,
         world: &mut World,
         entity_map: &EntityMap,
         entities: &[Entity],
     ) -> Result<(), MapEntitiesError> {
-        (self.map_entities)(world, entity_map, entities)
+        (self.map_specific_entities)(world, entity_map, entities)
     }
 }
 
 impl<C: Component + MapEntities> FromType<C> for ReflectMapEntities {
     fn from_type() -> Self {
         ReflectMapEntities {
-            map_entities: |world, entity_map, entities| {
+            map_specific_entities: |world, entity_map, entities| {
                 for &entity in entities {
                     if let Some(mut component) = world.get_mut::<C>(entity) {
                         component.map_entities(entity_map)?;
@@ -460,7 +460,7 @@ impl<C: Component + MapEntities> FromType<C> for ReflectMapEntities {
                 }
                 Ok(())
             },
-            map_all_entities: |world, entity_map| {
+            map_entities: |world, entity_map| {
                 for entity in entity_map.values() {
                     if let Some(mut component) = world.get_mut::<C>(entity) {
                         component.map_entities(entity_map)?;
diff --git a/crates/bevy_scene/src/dynamic_scene.rs b/crates/bevy_scene/src/dynamic_scene.rs
index 1f725df23eca9..5a090176f4d3c 100644
--- a/crates/bevy_scene/src/dynamic_scene.rs
+++ b/crates/bevy_scene/src/dynamic_scene.rs
@@ -137,11 +137,12 @@ impl DynamicScene {
 
         // 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("This TypeRegistration was where we got the TypeId initially, should be safe to unwrap");
+            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::<ReflectMapEntities>() {
                 map_entities_reflect
-                    .map_entities(world, entity_map, &entities)
+                    .map_specific_entities(world, entity_map, &entities)
                     .unwrap();
             }
         }
diff --git a/crates/bevy_scene/src/scene.rs b/crates/bevy_scene/src/scene.rs
index 36d8cf9e593af..462c1ec41e49f 100644
--- a/crates/bevy_scene/src/scene.rs
+++ b/crates/bevy_scene/src/scene.rs
@@ -121,7 +121,7 @@ impl Scene {
         for registration in type_registry.iter() {
             if let Some(map_entities_reflect) = registration.data::<ReflectMapEntities>() {
                 map_entities_reflect
-                    .map_all_entities(world, &instance_info.entity_map)
+                    .map_entities(world, &instance_info.entity_map)
                     .unwrap();
             }
         }

From 923f9b8b83622e15d7230062a9cdb73f92debbd7 Mon Sep 17 00:00:00 2001
From: Testare <testare.i9z@gmail.com>
Date: Tue, 7 Mar 2023 08:41:45 -0800
Subject: [PATCH 12/13] idiomatic asserts

---
 crates/bevy_scene/src/dynamic_scene.rs | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/crates/bevy_scene/src/dynamic_scene.rs b/crates/bevy_scene/src/dynamic_scene.rs
index 5a090176f4d3c..069e6786df82e 100644
--- a/crates/bevy_scene/src/dynamic_scene.rs
+++ b/crates/bevy_scene/src/dynamic_scene.rs
@@ -245,7 +245,7 @@ mod tests {
                 .get::<Parent>()
                 .unwrap()
                 .get(),
-            "Something about reloading the scene is touching entities with the same scene Ids"
+            "something about reloading the scene is touching entities with the same scene Ids"
         );
         assert_eq!(
             original_child_entity,
@@ -255,7 +255,7 @@ mod tests {
                 .get::<Parent>()
                 .unwrap()
                 .get(),
-            "Something about reloading the scene is touching components not defined in the scene but on entities defined in the scene"
+            "something about reloading the scene is touching components not defined in the scene but on entities defined in the scene"
         );
         assert_eq!(
             from_scene_parent_entity,
@@ -265,7 +265,7 @@ mod tests {
                 .get::<Parent>()
                 .expect("Something is wrong with this test, and the scene components don't have a parent/child relationship")
                 .get(),
-            "Something is wrong with the this test or the code reloading scenes since the relationship between scene entities is broken"
+            "something is wrong with the this test or the code reloading scenes since the relationship between scene entities is broken"
         );
     }
 }

From 2a1043dcf3cce309c4c0002a39694047425e25ae Mon Sep 17 00:00:00 2001
From: Testare <testare.i9z@gmail.com>
Date: Tue, 7 Mar 2023 08:56:52 -0800
Subject: [PATCH 13/13] idiomatic expectations

---
 crates/bevy_scene/src/dynamic_scene.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/crates/bevy_scene/src/dynamic_scene.rs b/crates/bevy_scene/src/dynamic_scene.rs
index 069e6786df82e..23a033c969751 100644
--- a/crates/bevy_scene/src/dynamic_scene.rs
+++ b/crates/bevy_scene/src/dynamic_scene.rs
@@ -138,7 +138,7 @@ impl DynamicScene {
         // 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",
+                "we should be getting TypeId from this TypeRegistration in the first place",
             );
             if let Some(map_entities_reflect) = registration.data::<ReflectMapEntities>() {
                 map_entities_reflect
@@ -263,7 +263,7 @@ mod tests {
                 .get_entity(from_scene_child_entity)
                 .unwrap()
                 .get::<Parent>()
-                .expect("Something is wrong with this test, and the scene components don't have a parent/child relationship")
+                .expect("something is wrong with this test, and the scene components don't have a parent/child relationship")
                 .get(),
             "something is wrong with the this test or the code reloading scenes since the relationship between scene entities is broken"
         );