bevy_scene: Stabilize entity order in DynamicSceneBuilder (bevyengi…

…ne#6382)

# Objective

Currently, `DynamicSceneBuilder` keeps track of entities via a `HashMap`. This has an unintended side-effect in that, when building the full `DynamicScene`, we aren't guaranteed any particular order. 

In other words, inserting Entity A then Entity B can result in either `[A, B]` or `[B, A]`. This can be rather annoying when running tests on scenes generated via the builder as it will work sometimes but not other times. There's also the potential that this might unnecessarily clutter up VCS diffs for scene files (assuming they had an intentional order).

## Solution

Store `DynamicSceneBuilder`'s entities in a `Vec` rather than a `HashMap`.

---

## Changelog

* Stablized entity order in `DynamicSceneBuilder` (0.9.0-dev)

crates/bevy_scene/src/dynamic_scene_builder.rs

@@ -1,10 +1,18 @@
 use crate::{DynamicEntity, DynamicScene};
 use bevy_app::AppTypeRegistry;
 use bevy_ecs::{prelude::Entity, reflect::ReflectComponent, world::World};
-use bevy_utils::{default, HashMap};
+use bevy_utils::default;
+use std::collections::BTreeMap;
 
 /// A [`DynamicScene`] builder, used to build a scene from a [`World`] by extracting some entities.
 ///
+/// # Entity Order
+///
+/// Extracted entities will always be stored in ascending order based on their [id](Entity::id).
+/// This means that inserting `Entity(1v0)` then `Entity(0v0)` will always result in the entities
+/// being ordered as `[Entity(0v0), Entity(1v0)]`.
+///
+/// # Example
 /// ```
 /// # use bevy_scene::DynamicSceneBuilder;
 /// # use bevy_app::AppTypeRegistry;
@@ -23,7 +31,7 @@ use bevy_utils::{default, HashMap};
 /// let dynamic_scene = builder.build();
 /// ```
 pub struct DynamicSceneBuilder<'w> {
-    scene: HashMap<u32, DynamicEntity>,
+    entities: BTreeMap<u32, DynamicEntity>,
     type_registry: AppTypeRegistry,
     world: &'w World,
 }
@@ -33,7 +41,7 @@ impl<'w> DynamicSceneBuilder<'w> {
     /// All components registered in that world's [`AppTypeRegistry`] resource will be extracted.
     pub fn from_world(world: &'w World) -> Self {
         Self {
-            scene: default(),
+            entities: default(),
             type_registry: world.resource::<AppTypeRegistry>().clone(),
             world,
         }
@@ -43,7 +51,7 @@ impl<'w> DynamicSceneBuilder<'w> {
     /// Only components registered in the given [`AppTypeRegistry`] will be extracted.
     pub fn from_world_with_type_registry(world: &'w World, type_registry: AppTypeRegistry) -> Self {
         Self {
-            scene: default(),
+            entities: default(),
             type_registry,
             world,
         }
@@ -52,7 +60,7 @@ impl<'w> DynamicSceneBuilder<'w> {
     /// Consume the builder, producing a [`DynamicScene`].
     pub fn build(self) -> DynamicScene {
         DynamicScene {
-            entities: self.scene.into_values().collect(),
+            entities: self.entities.into_values().collect(),
         }
     }
 
@@ -92,12 +100,14 @@ impl<'w> DynamicSceneBuilder<'w> {
         let type_registry = self.type_registry.read();
 
         for entity in entities {
-            if self.scene.contains_key(&entity.id()) {
+            let id = entity.id();
+
+            if self.entities.contains_key(&id) {
                 continue;
             }
 
             let mut entry = DynamicEntity {
-                entity: entity.id(),
+                entity: id,
                 components: Vec::new(),
             };
 
@@ -116,7 +126,7 @@ impl<'w> DynamicSceneBuilder<'w> {
                 }
             }
 
-            self.scene.insert(entity.id(), entry);
+            self.entities.insert(id, entry);
         }
 
         drop(type_registry);
@@ -208,6 +218,33 @@ mod tests {
         assert!(scene.entities[0].components[1].represents::<ComponentB>());
     }
 
+    #[test]
+    fn extract_entity_order() {
+        let mut world = World::default();
+        world.init_resource::<AppTypeRegistry>();
+
+        // Spawn entities in order
+        let entity_a = world.spawn_empty().id();
+        let entity_b = world.spawn_empty().id();
+        let entity_c = world.spawn_empty().id();
+        let entity_d = world.spawn_empty().id();
+
+        let mut builder = DynamicSceneBuilder::from_world(&world);
+
+        // Insert entities out of order
+        builder.extract_entity(entity_b);
+        builder.extract_entities([entity_d, entity_a].into_iter());
+        builder.extract_entity(entity_c);
+
+        let mut entities = builder.build().entities.into_iter();
+
+        // Assert entities are ordered
+        assert_eq!(entity_a.id(), entities.next().map(|e| e.entity).unwrap());
+        assert_eq!(entity_b.id(), entities.next().map(|e| e.entity).unwrap());
+        assert_eq!(entity_c.id(), entities.next().map(|e| e.entity).unwrap());
+        assert_eq!(entity_d.id(), entities.next().map(|e| e.entity).unwrap());
+    }
+
     #[test]
     fn extract_query() {
         let mut world = World::default();

crates/bevy_scene/src/serde.rs

@@ -373,3 +373,130 @@ impl<'a, 'de> Visitor<'de> for ComponentVisitor<'a> {
         Ok(dynamic_properties)
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use crate::serde::SceneDeserializer;
+    use crate::DynamicSceneBuilder;
+    use bevy_app::AppTypeRegistry;
+    use bevy_ecs::entity::EntityMap;
+    use bevy_ecs::prelude::{Component, ReflectComponent, World};
+    use bevy_reflect::Reflect;
+    use serde::de::DeserializeSeed;
+
+    #[derive(Component, Reflect, Default)]
+    #[reflect(Component)]
+    struct Foo(i32);
+    #[derive(Component, Reflect, Default)]
+    #[reflect(Component)]
+    struct Bar(i32);
+    #[derive(Component, Reflect, Default)]
+    #[reflect(Component)]
+    struct Baz(i32);
+
+    fn create_world() -> World {
+        let mut world = World::new();
+        let registry = AppTypeRegistry::default();
+        {
+            let mut registry = registry.write();
+            registry.register::<Foo>();
+            registry.register::<Bar>();
+            registry.register::<Baz>();
+        }
+        world.insert_resource(registry);
+        world
+    }
+
+    #[test]
+    fn should_serialize() {
+        let mut world = create_world();
+
+        let a = world.spawn(Foo(123)).id();
+        let b = world.spawn((Foo(123), Bar(345))).id();
+        let c = world.spawn((Foo(123), Bar(345), Baz(789))).id();
+
+        let mut builder = DynamicSceneBuilder::from_world(&world);
+        builder.extract_entities([a, b, c].into_iter());
+        let scene = builder.build();
+
+        let expected = r#"(
+  entities: [
+    (
+      entity: 0,
+      components: {
+        "bevy_scene::serde::tests::Foo": (123),
+      },
+    ),
+    (
+      entity: 1,
+      components: {
+        "bevy_scene::serde::tests::Foo": (123),
+        "bevy_scene::serde::tests::Bar": (345),
+      },
+    ),
+    (
+      entity: 2,
+      components: {
+        "bevy_scene::serde::tests::Foo": (123),
+        "bevy_scene::serde::tests::Bar": (345),
+        "bevy_scene::serde::tests::Baz": (789),
+      },
+    ),
+  ],
+)"#;
+        let output = scene
+            .serialize_ron(&world.resource::<AppTypeRegistry>().0)
+            .unwrap();
+        assert_eq!(expected, output);
+    }
+
+    #[test]
+    fn should_deserialize() {
+        let world = create_world();
+
+        let input = r#"(
+  entities: [
+    (
+      entity: 0,
+      components: {
+        "bevy_scene::serde::tests::Foo": (123),
+      },
+    ),
+    (
+      entity: 1,
+      components: {
+        "bevy_scene::serde::tests::Foo": (123),
+        "bevy_scene::serde::tests::Bar": (345),
+      },
+    ),
+    (
+      entity: 2,
+      components: {
+        "bevy_scene::serde::tests::Foo": (123),
+        "bevy_scene::serde::tests::Bar": (345),
+        "bevy_scene::serde::tests::Baz": (789),
+      },
+    ),
+  ],
+)"#;
+        let mut deserializer = ron::de::Deserializer::from_str(input).unwrap();
+        let scene_deserializer = SceneDeserializer {
+            type_registry: &world.resource::<AppTypeRegistry>().read(),
+        };
+        let scene = scene_deserializer.deserialize(&mut deserializer).unwrap();
+
+        assert_eq!(
+            3,
+            scene.entities.len(),
+            "expected `entities` to contain 3 entities"
+        );
+
+        let mut map = EntityMap::default();
+        let mut dst_world = create_world();
+        scene.write_to_world(&mut dst_world, &mut map).unwrap();
+
+        assert_eq!(3, dst_world.query::<&Foo>().iter(&dst_world).count());
+        assert_eq!(2, dst_world.query::<&Bar>().iter(&dst_world).count());
+        assert_eq!(1, dst_world.query::<&Baz>().iter(&dst_world).count());
+    }
+}