From ec9ec876ae80a4458bb63450678bc042720d0fde Mon Sep 17 00:00:00 2001 From: Gino Valente Date: Thu, 27 Oct 2022 23:38:03 +0000 Subject: [PATCH] bevy_scene: Stabilize entity order in `DynamicSceneBuilder` (#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) --- .../bevy_scene/src/dynamic_scene_builder.rs | 53 ++++++-- crates/bevy_scene/src/serde.rs | 127 ++++++++++++++++++ 2 files changed, 172 insertions(+), 8 deletions(-) diff --git a/crates/bevy_scene/src/dynamic_scene_builder.rs b/crates/bevy_scene/src/dynamic_scene_builder.rs index a456138a3f8d2f..4a8438c2bb44cc 100644 --- a/crates/bevy_scene/src/dynamic_scene_builder.rs +++ b/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, + entities: BTreeMap, 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::().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::()); } + #[test] + fn extract_entity_order() { + let mut world = World::default(); + world.init_resource::(); + + // 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(); diff --git a/crates/bevy_scene/src/serde.rs b/crates/bevy_scene/src/serde.rs index 996157c0093396..3cae74c3c3bbfa 100644 --- a/crates/bevy_scene/src/serde.rs +++ b/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::(); + registry.register::(); + registry.register::(); + } + 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::().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::().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()); + } +}