From 9b82d3da7578e3363fc5974003373bfe993ee454 Mon Sep 17 00:00:00 2001 From: Brice DAVIER Date: Sun, 16 Jan 2022 18:17:59 +0100 Subject: [PATCH 1/4] bevy_reflect: support map insertion --- crates/bevy_reflect/src/impls/std.rs | 49 ++++++++++++------ crates/bevy_reflect/src/lib.rs | 3 ++ crates/bevy_reflect/src/map.rs | 74 +++++++++++++++++++--------- crates/bevy_reflect/src/reflect.rs | 5 +- crates/bevy_reflect/src/serde/de.rs | 2 +- 5 files changed, 92 insertions(+), 41 deletions(-) diff --git a/crates/bevy_reflect/src/impls/std.rs b/crates/bevy_reflect/src/impls/std.rs index 801ce2c0329b6..31985bf8fe83a 100644 --- a/crates/bevy_reflect/src/impls/std.rs +++ b/crates/bevy_reflect/src/impls/std.rs @@ -1,8 +1,8 @@ use crate as bevy_reflect; use crate::{ - map_partial_eq, serde::Serializable, DynamicMap, FromReflect, FromType, GetTypeRegistration, - List, ListIter, Map, MapIter, Reflect, ReflectDeserialize, ReflectMut, ReflectRef, - TypeRegistration, + map_apply, map_partial_eq, serde::Serializable, DynamicMap, FromReflect, FromType, + GetTypeRegistration, List, ListIter, Map, MapIter, Reflect, ReflectDeserialize, ReflectMut, + ReflectRef, TypeRegistration, }; use bevy_reflect_derive::{impl_from_reflect_value, impl_reflect_value}; @@ -166,7 +166,7 @@ impl FromReflect for Vec { } } -impl Map for HashMap { +impl Map for HashMap { fn get(&self, key: &dyn Reflect) -> Option<&dyn Reflect> { key.downcast_ref::() .and_then(|key| HashMap::get(self, key)) @@ -204,10 +204,35 @@ impl Map for HashMap { } dynamic_map } + + fn insert_boxed( + &mut self, + key: Box, + value: Box, + ) -> Option> { + let key = key.take::().unwrap_or_else(|key| { + K::from_reflect(&*key).unwrap_or_else(|| { + panic!( + "Attempted to insert invalid key of type {}.", + key.type_name() + ) + }) + }); + let value = value.take::().unwrap_or_else(|value| { + V::from_reflect(&*value).unwrap_or_else(|| { + panic!( + "Attempted to push invalid value of type {}.", + value.type_name() + ) + }) + }); + self.insert(key, value) + .map(|old_value| Box::new(old_value) as Box) + } } // SAFE: any and any_mut both return self -unsafe impl Reflect for HashMap { +unsafe impl Reflect for HashMap { fn type_name(&self) -> &str { std::any::type_name::() } @@ -221,15 +246,7 @@ unsafe impl Reflect for HashMap { } fn apply(&mut self, value: &dyn Reflect) { - if let ReflectRef::Map(map_value) = value.reflect_ref() { - for (key, value) in map_value.iter() { - if let Some(v) = Map::get_mut(self, key) { - v.apply(value) - } - } - } else { - panic!("Attempted to apply a non-map type to a map type."); - } + map_apply(self, value) } fn set(&mut self, value: Box) -> Result<(), Box> { @@ -264,8 +281,8 @@ unsafe impl Reflect for HashMap { impl GetTypeRegistration for HashMap where - K: Reflect + Clone + Eq + Hash + for<'de> Deserialize<'de>, - V: Reflect + Clone + for<'de> Deserialize<'de>, + K: FromReflect + Clone + Eq + Hash + for<'de> Deserialize<'de>, + V: FromReflect + Clone + for<'de> Deserialize<'de>, { fn get_type_registration() -> TypeRegistration { let mut registration = TypeRegistration::of::>(); diff --git a/crates/bevy_reflect/src/lib.rs b/crates/bevy_reflect/src/lib.rs index cd479d1e07dec..cec68a56438ca 100644 --- a/crates/bevy_reflect/src/lib.rs +++ b/crates/bevy_reflect/src/lib.rs @@ -247,6 +247,7 @@ mod tests { let mut map = DynamicMap::default(); map.insert(2usize, 3i8); + map.insert(3usize, 4i8); foo_patch.insert("d", map); let mut bar_patch = DynamicStruct::default(); @@ -285,6 +286,7 @@ mod tests { let mut hash_map = HashMap::default(); hash_map.insert(1, 1); hash_map.insert(2, 3); + hash_map.insert(3, 4); let mut hash_map_baz = HashMap::default(); hash_map_baz.insert(1, Bar { x: 7 }); @@ -306,6 +308,7 @@ mod tests { let mut hash_map = HashMap::default(); hash_map.insert(2, 3); + hash_map.insert(3, 4); let expected_new_foo = Foo { a: 2, diff --git a/crates/bevy_reflect/src/map.rs b/crates/bevy_reflect/src/map.rs index f2ad17d6671c1..a9248bf51ed8f 100644 --- a/crates/bevy_reflect/src/map.rs +++ b/crates/bevy_reflect/src/map.rs @@ -41,6 +41,16 @@ pub trait Map: Reflect { /// Clones the map, producing a [`DynamicMap`]. fn clone_dynamic(&self) -> DynamicMap; + + /// Inserts a key-value pair into the map. + /// + /// If the map did not have this key present, `None` is returned. + /// If the map did have this key present, the value is updated, and the old value is returned. + fn insert_boxed( + &mut self, + key: Box, + value: Box, + ) -> Option>; } const HASH_ERROR: &str = "the given key does not support hashing"; @@ -74,19 +84,6 @@ impl DynamicMap { pub fn insert(&mut self, key: K, value: V) { self.insert_boxed(Box::new(key), Box::new(value)); } - - /// Inserts a key-value pair of [`Reflect`] values into the map. - pub fn insert_boxed(&mut self, key: Box, value: Box) { - match self.indices.entry(key.reflect_hash().expect(HASH_ERROR)) { - Entry::Occupied(entry) => { - self.values[*entry.get()] = (key, value); - } - Entry::Vacant(entry) => { - entry.insert(self.values.len()); - self.values.push((key, value)); - } - } - } } impl Map for DynamicMap { @@ -131,6 +128,25 @@ impl Map for DynamicMap { .get(index) .map(|(key, value)| (&**key, &**value)) } + + fn insert_boxed( + &mut self, + key: Box, + mut value: Box, + ) -> Option> { + match self.indices.entry(key.reflect_hash().expect(HASH_ERROR)) { + Entry::Occupied(entry) => { + let (_old_key, old_value) = self.values.get_mut(*entry.get()).unwrap(); + std::mem::swap(old_value, &mut value); + Some(value) + } + Entry::Vacant(entry) => { + entry.insert(self.values.len()); + self.values.push((key, value)); + None + } + } + } } // SAFE: any and any_mut both return self @@ -148,15 +164,7 @@ unsafe impl Reflect for DynamicMap { } fn apply(&mut self, value: &dyn Reflect) { - if let ReflectRef::Map(map_value) = value.reflect_ref() { - for (key, value) in map_value.iter() { - if let Some(v) = self.get_mut(key) { - v.apply(value) - } - } - } else { - panic!("Attempted to apply a non-map type to a map type."); - } + map_apply(self, value); } fn set(&mut self, value: Box) -> Result<(), Box> { @@ -243,3 +251,25 @@ pub fn map_partial_eq(a: &M, b: &dyn Reflect) -> Option { Some(true) } + +/// Applies the elements of `b` to the corresponding elements of `a`. +/// +/// If a key from `b` does not exist in `a`, the value is cloned and inserted. +/// +/// # Panics +/// +/// This function panics if `b` is not a map. +#[inline] +pub fn map_apply(a: &mut M, b: &dyn Reflect) { + if let ReflectRef::Map(map_value) = b.reflect_ref() { + for (key, b_value) in map_value.iter() { + if let Some(a_value) = a.get_mut(key) { + a_value.apply(b_value); + } else { + a.insert_boxed(key.clone_value(), b_value.clone_value()); + } + } + } else { + panic!("Attempted to apply a non-map type to a map type."); + } +} diff --git a/crates/bevy_reflect/src/reflect.rs b/crates/bevy_reflect/src/reflect.rs index 4362068df918e..4a45cb88361ea 100644 --- a/crates/bevy_reflect/src/reflect.rs +++ b/crates/bevy_reflect/src/reflect.rs @@ -79,10 +79,11 @@ pub unsafe trait Reflect: Any + Send + Sync { /// Note that `Reflect` must be implemented manually for [`List`]s and /// [`Map`]s in order to achieve the correct semantics, as derived /// implementations will have the semantics for [`Struct`], [`TupleStruct`] - /// or none of the above depending on the kind of type. For lists, use the - /// [`list_apply`] helper function when implementing this method. + /// or none of the above depending on the kind of type. For lists and maps, use the + /// [`list_apply`] and [`map_apply`] helper functions when implementing this method. /// /// [`list_apply`]: crate::list_apply + /// [`map_apply`]: crate::map_apply /// /// # Panics /// diff --git a/crates/bevy_reflect/src/serde/de.rs b/crates/bevy_reflect/src/serde/de.rs index 216b25e106f40..f8118fbfd6a02 100644 --- a/crates/bevy_reflect/src/serde/de.rs +++ b/crates/bevy_reflect/src/serde/de.rs @@ -1,6 +1,6 @@ use crate::{ serde::type_fields, DynamicList, DynamicMap, DynamicStruct, DynamicTuple, DynamicTupleStruct, - Reflect, ReflectDeserialize, TypeRegistry, + Map, Reflect, ReflectDeserialize, TypeRegistry, }; use erased_serde::Deserializer; use serde::de::{self, DeserializeSeed, MapAccess, SeqAccess, Visitor}; From 0defec3238516a4b77f700a2566328dbf7f7775c Mon Sep 17 00:00:00 2001 From: Brice DAVIER Date: Sun, 16 Jan 2022 18:28:33 +0100 Subject: [PATCH 2/4] fix documentation --- crates/bevy_reflect/src/reflect.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_reflect/src/reflect.rs b/crates/bevy_reflect/src/reflect.rs index 4a45cb88361ea..e4ab37fd63d37 100644 --- a/crates/bevy_reflect/src/reflect.rs +++ b/crates/bevy_reflect/src/reflect.rs @@ -72,7 +72,7 @@ pub unsafe trait Reflect: Any + Send + Sync { /// and excess elements in `value` are appended to `self`. /// - If `T` is a [`Map`], then for each key in `value`, the associated /// value is applied to the value associated with the same key in `self`. - /// Keys which are not present in both maps are ignored. + /// Keys which are not present in `self` are inserted. /// - If `T` is none of these, then `value` is downcast to `T`, cloned, and /// assigned to `self`. /// From c2fd7985fa5cf2b1a625bad2bf21b21885b19203 Mon Sep 17 00:00:00 2001 From: james7132 Date: Fri, 1 Jul 2022 16:43:52 -0700 Subject: [PATCH 3/4] Shut up clippy --- crates/bevy_reflect/src/impls/std.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_reflect/src/impls/std.rs b/crates/bevy_reflect/src/impls/std.rs index 1389e3132ce8c..b7f0b2710dcad 100644 --- a/crates/bevy_reflect/src/impls/std.rs +++ b/crates/bevy_reflect/src/impls/std.rs @@ -288,7 +288,7 @@ impl Reflect for HashMap { } fn apply(&mut self, value: &dyn Reflect) { - map_apply(self, value) + map_apply(self, value); } fn set(&mut self, value: Box) -> Result<(), Box> { From f0213cd57d660f1046e4f3c79014fadd40e6598e Mon Sep 17 00:00:00 2001 From: james7132 Date: Sat, 2 Jul 2022 03:42:11 -0700 Subject: [PATCH 4/4] Apply suggestions from MrGVSV --- crates/bevy_reflect/src/impls/std.rs | 2 +- crates/bevy_reflect/src/map.rs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/bevy_reflect/src/impls/std.rs b/crates/bevy_reflect/src/impls/std.rs index b7f0b2710dcad..61352302fa67b 100644 --- a/crates/bevy_reflect/src/impls/std.rs +++ b/crates/bevy_reflect/src/impls/std.rs @@ -248,7 +248,7 @@ impl Map for HashMap { let value = value.take::().unwrap_or_else(|value| { V::from_reflect(&*value).unwrap_or_else(|| { panic!( - "Attempted to push invalid value of type {}.", + "Attempted to insert invalid value of type {}.", value.type_name() ) }) diff --git a/crates/bevy_reflect/src/map.rs b/crates/bevy_reflect/src/map.rs index fd1df210c58d2..5824ba05a82d9 100644 --- a/crates/bevy_reflect/src/map.rs +++ b/crates/bevy_reflect/src/map.rs @@ -395,13 +395,13 @@ pub fn map_debug(dyn_map: &dyn Map, f: &mut std::fmt::Formatter<'_>) -> std::fmt debug.finish() } -/// Applies the elements of `b` to the corresponding elements of `a`. +/// Applies the elements of reflected map `b` to the corresponding elements of map `a`. /// /// If a key from `b` does not exist in `a`, the value is cloned and inserted. /// /// # Panics /// -/// This function panics if `b` is not a map. +/// This function panics if `b` is not a reflected map. #[inline] pub fn map_apply(a: &mut M, b: &dyn Reflect) { if let ReflectRef::Map(map_value) = b.reflect_ref() {