From 894334b51e8fcab57a4c58fe3a45018c21c0a887 Mon Sep 17 00:00:00 2001 From: Gino Valente Date: Thu, 27 Oct 2022 01:46:33 +0000 Subject: [PATCH] bevy_scene: Use map for scene `components` (#6345) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit # Objective Currently scenes define components using a list: ```rust [ ( entity: 0, components: [ { "bevy_transform::components::transform::Transform": ( translation: ( x: 0.0, y: 0.0, z: 0.0 ), rotation: (0.0, 0.0, 0.0, 1.0), scale: ( x: 1.0, y: 1.0, z: 1.0 ), ), }, { "my_crate::Foo": ( text: "Hello World", ), }, { "my_crate::Bar": ( baz: 123, ), }, ], ), ] ``` However, this representation has some drawbacks (as pointed out by @Metadorius in [this](https://github.com/bevyengine/bevy/pull/4561#issuecomment-1202215565) comment): 1. Increased nesting and more characters (minor effect on overall size) 2. More importantly, by definition, entities cannot have more than one instance of any given component. Therefore, such data is best stored as a map— where all values are meant to have unique keys. ## Solution Change `components` to store a map of components rather than a list: ```rust [ ( entity: 0, components: { "bevy_transform::components::transform::Transform": ( translation: ( x: 0.0, y: 0.0, z: 0.0 ), rotation: (0.0, 0.0, 0.0, 1.0), scale: ( x: 1.0, y: 1.0, z: 1.0 ), ), "my_crate::Foo": ( text: "Hello World", ), "my_crate::Bar": ( baz: 123 ), }, ), ] ``` #### Code Representation This change only affects the scene format itself. `DynamicEntity` still stores its components as a list. The reason for this is that storing such data as a map is not really needed since: 1. The "key" of each value is easily found by just calling `Reflect::type_name` on it 2. We should be generating such structs using the `World` itself which upholds the one-component-per-entity rule One could in theory create manually create a `DynamicEntity` with duplicate components, but this isn't something I think we should focus on in this PR. `DynamicEntity` can be broken in other ways (i.e. storing a non-component in the components list), and resolving its issues can be done in a separate PR. --- ## Changelog * The scene format now uses a map to represent the collection of components rather than a list ## Migration Guide The scene format now uses a map to represent the collection of components. Scene files will need to update from the old list format.
Example Code ```rust // OLD [ ( entity: 0, components: [ { "bevy_transform::components::transform::Transform": ( translation: ( x: 0.0, y: 0.0, z: 0.0 ), rotation: (0.0, 0.0, 0.0, 1.0), scale: ( x: 1.0, y: 1.0, z: 1.0 ), ), }, { "my_crate::Foo": ( text: "Hello World", ), }, { "my_crate::Bar": ( baz: 123, ), }, ], ), ] // NEW [ ( entity: 0, components: { "bevy_transform::components::transform::Transform": ( translation: ( x: 0.0, y: 0.0, z: 0.0 ), rotation: (0.0, 0.0, 0.0, 1.0), scale: ( x: 1.0, y: 1.0, z: 1.0 ), ), "my_crate::Foo": ( text: "Hello World", ), "my_crate::Bar": ( baz: 123 ), }, ), ] ```
--- assets/scenes/load_scene_example.scn.ron | 58 ++++++++++-------------- crates/bevy_scene/src/serde.rs | 53 ++++++++++++++++------ 2 files changed, 64 insertions(+), 47 deletions(-) diff --git a/assets/scenes/load_scene_example.scn.ron b/assets/scenes/load_scene_example.scn.ron index 696f0c79274f1..85783856d13e6 100644 --- a/assets/scenes/load_scene_example.scn.ron +++ b/assets/scenes/load_scene_example.scn.ron @@ -2,45 +2,37 @@ entities: [ ( entity: 0, - components: [ - { - "bevy_transform::components::transform::Transform": ( - translation: ( - x: 0.0, - y: 0.0, - z: 0.0 - ), - rotation: (0.0, 0.0, 0.0, 1.0), - scale: ( - x: 1.0, - y: 1.0, - z: 1.0 - ), + components: { + "bevy_transform::components::transform::Transform": ( + translation: ( + x: 0.0, + y: 0.0, + z: 0.0 ), - }, - { - "scene::ComponentB": ( - value: "hello", - ), - }, - { - "scene::ComponentA": ( + rotation: (0.0, 0.0, 0.0, 1.0), + scale: ( x: 1.0, - y: 2.0, + y: 1.0, + z: 1.0 ), - }, - ], + ), + "scene::ComponentB": ( + value: "hello", + ), + "scene::ComponentA": ( + x: 1.0, + y: 2.0, + ), + }, ), ( entity: 1, - components: [ - { - "scene::ComponentA": ( - x: 3.0, - y: 4.0, - ), - }, - ], + components: { + "scene::ComponentA": ( + x: 3.0, + y: 4.0, + ), + }, ), ] ) diff --git a/crates/bevy_scene/src/serde.rs b/crates/bevy_scene/src/serde.rs index f15e989e7d6e8..996157c009339 100644 --- a/crates/bevy_scene/src/serde.rs +++ b/crates/bevy_scene/src/serde.rs @@ -1,9 +1,9 @@ use crate::{DynamicEntity, DynamicScene}; use anyhow::Result; -use bevy_reflect::{ - serde::{ReflectSerializer, UntypedReflectDeserializer}, - Reflect, TypeRegistry, TypeRegistryArc, -}; +use bevy_reflect::serde::{TypedReflectDeserializer, TypedReflectSerializer}; +use bevy_reflect::{serde::UntypedReflectDeserializer, Reflect, TypeRegistry, TypeRegistryArc}; +use bevy_utils::HashSet; +use serde::ser::SerializeMap; use serde::{ de::{DeserializeSeed, Error, MapAccess, SeqAccess, Visitor}, ser::{SerializeSeq, SerializeStruct}, @@ -100,10 +100,12 @@ impl<'a> Serialize for ComponentsSerializer<'a> { where S: serde::Serializer, { - let mut state = serializer.serialize_seq(Some(self.components.len()))?; + let mut state = serializer.serialize_map(Some(self.components.len()))?; for component in self.components { - state - .serialize_element(&ReflectSerializer::new(&**component, &self.registry.read()))?; + state.serialize_entry( + component.type_name(), + &TypedReflectSerializer::new(&**component, &*self.registry.read()), + )?; } state.end() } @@ -285,7 +287,7 @@ impl<'a, 'de> Visitor<'de> for SceneEntityVisitor<'a> { return Err(Error::duplicate_field(ENTITY_FIELD_COMPONENTS)); } - components = Some(map.next_value_seed(ComponentVecDeserializer { + components = Some(map.next_value_seed(ComponentDeserializer { registry: self.registry, })?); } @@ -306,32 +308,55 @@ impl<'a, 'de> Visitor<'de> for SceneEntityVisitor<'a> { } } -pub struct ComponentVecDeserializer<'a> { +pub struct ComponentDeserializer<'a> { pub registry: &'a TypeRegistry, } -impl<'a, 'de> DeserializeSeed<'de> for ComponentVecDeserializer<'a> { +impl<'a, 'de> DeserializeSeed<'de> for ComponentDeserializer<'a> { type Value = Vec>; fn deserialize(self, deserializer: D) -> Result where D: serde::Deserializer<'de>, { - deserializer.deserialize_seq(ComponentSeqVisitor { + deserializer.deserialize_map(ComponentVisitor { registry: self.registry, }) } } -struct ComponentSeqVisitor<'a> { +struct ComponentVisitor<'a> { pub registry: &'a TypeRegistry, } -impl<'a, 'de> Visitor<'de> for ComponentSeqVisitor<'a> { +impl<'a, 'de> Visitor<'de> for ComponentVisitor<'a> { type Value = Vec>; fn expecting(&self, formatter: &mut std::fmt::Formatter) -> std::fmt::Result { - formatter.write_str("list of components") + formatter.write_str("map of components") + } + + fn visit_map(self, mut map: A) -> std::result::Result + where + A: MapAccess<'de>, + { + let mut added = HashSet::new(); + let mut components = Vec::new(); + while let Some(key) = map.next_key::<&str>()? { + if !added.insert(key) { + return Err(Error::custom(format!("duplicate component: `{}`", key))); + } + + let registration = self + .registry + .get_with_name(key) + .ok_or_else(|| Error::custom(format!("no registration found for `{}`", key)))?; + components.push( + map.next_value_seed(TypedReflectDeserializer::new(registration, self.registry))?, + ); + } + + Ok(components) } fn visit_seq(self, mut seq: A) -> Result