From 7983ac9aea5ed86bb02cd01325805257253a150e Mon Sep 17 00:00:00 2001 From: Azorlogh Date: Tue, 24 Jun 2025 15:38:12 +0200 Subject: [PATCH 1/7] fix map apply, remove get_at/_mut & add retain to Map --- .../src/impls/alloc/collections/btree/map.rs | 28 +- crates/bevy_reflect/src/impls/macros/map.rs | 23 +- crates/bevy_reflect/src/impls/macros/set.rs | 4 + crates/bevy_reflect/src/map.rs | 292 +++++------------- crates/bevy_reflect/src/set.rs | 11 +- 5 files changed, 103 insertions(+), 255 deletions(-) diff --git a/crates/bevy_reflect/src/impls/alloc/collections/btree/map.rs b/crates/bevy_reflect/src/impls/alloc/collections/btree/map.rs index e579ace206eb2..df68b425f6914 100644 --- a/crates/bevy_reflect/src/impls/alloc/collections/btree/map.rs +++ b/crates/bevy_reflect/src/impls/alloc/collections/btree/map.rs @@ -2,7 +2,7 @@ use crate::{ error::ReflectCloneError, generics::{Generics, TypeParamInfo}, kind::{ReflectKind, ReflectMut, ReflectOwned, ReflectRef}, - map::{map_apply, map_partial_eq, map_try_apply, Map, MapInfo, MapIter}, + map::{map_apply, map_partial_eq, map_try_apply, Map, MapInfo}, prelude::*, reflect::{impl_full_reflect, ApplyError}, type_info::{MaybeTyped, TypeInfo, Typed}, @@ -31,27 +31,15 @@ where .map(|value| value as &mut dyn PartialReflect) } - fn get_at(&self, index: usize) -> Option<(&dyn PartialReflect, &dyn PartialReflect)> { - self.iter() - .nth(index) - .map(|(key, value)| (key as &dyn PartialReflect, value as &dyn PartialReflect)) - } - - fn get_at_mut( - &mut self, - index: usize, - ) -> Option<(&dyn PartialReflect, &mut dyn PartialReflect)> { - self.iter_mut() - .nth(index) - .map(|(key, value)| (key as &dyn PartialReflect, value as &mut dyn PartialReflect)) - } - fn len(&self) -> usize { Self::len(self) } - fn iter(&self) -> MapIter { - MapIter::new(self) + fn iter(&self) -> Box + '_> { + Box::new( + self.iter() + .map(|(k, v)| (k as &dyn PartialReflect, v as &dyn PartialReflect)), + ) } fn drain(&mut self) -> Vec<(Box, Box)> { @@ -68,6 +56,10 @@ where result } + fn retain(&mut self, f: &mut dyn FnMut(&dyn PartialReflect, &mut dyn PartialReflect) -> bool) { + self.retain(move |k, v| f(k, v)); + } + fn insert_boxed( &mut self, key: Box, diff --git a/crates/bevy_reflect/src/impls/macros/map.rs b/crates/bevy_reflect/src/impls/macros/map.rs index ce621b7f78e89..d356ddba587a2 100644 --- a/crates/bevy_reflect/src/impls/macros/map.rs +++ b/crates/bevy_reflect/src/impls/macros/map.rs @@ -19,27 +19,12 @@ macro_rules! impl_reflect_for_hashmap { .map(|value| value as &mut dyn $crate::reflect::PartialReflect) } - fn get_at(&self, index: usize) -> Option<(&dyn $crate::reflect::PartialReflect, &dyn $crate::reflect::PartialReflect)> { - self.iter() - .nth(index) - .map(|(key, value)| (key as &dyn $crate::reflect::PartialReflect, value as &dyn $crate::reflect::PartialReflect)) - } - - fn get_at_mut( - &mut self, - index: usize, - ) -> Option<(&dyn $crate::reflect::PartialReflect, &mut dyn $crate::reflect::PartialReflect)> { - self.iter_mut().nth(index).map(|(key, value)| { - (key as &dyn $crate::reflect::PartialReflect, value as &mut dyn $crate::reflect::PartialReflect) - }) - } - fn len(&self) -> usize { Self::len(self) } - fn iter(&self) -> $crate::map::MapIter { - $crate::map::MapIter::new(self) + fn iter(&self) -> bevy_platform::prelude::Box + '_> { + bevy_platform::prelude::Box::new(self.iter().map(|(k, v)| (k as &dyn $crate::reflect::PartialReflect, v as &dyn $crate::reflect::PartialReflect))) } fn drain(&mut self) -> bevy_platform::prelude::Vec<(bevy_platform::prelude::Box, bevy_platform::prelude::Box)> { @@ -53,6 +38,10 @@ macro_rules! impl_reflect_for_hashmap { .collect() } + fn retain(&mut self, f: &mut dyn FnMut(&dyn $crate::reflect::PartialReflect, &mut dyn $crate::reflect::PartialReflect) -> bool) { + self.retain(move |key, value| f(key, value)); + } + fn to_dynamic_map(&self) -> $crate::map::DynamicMap { let mut dynamic_map = $crate::map::DynamicMap::default(); dynamic_map.set_represented_type($crate::reflect::PartialReflect::get_represented_type_info(self)); diff --git a/crates/bevy_reflect/src/impls/macros/set.rs b/crates/bevy_reflect/src/impls/macros/set.rs index e00e764e17110..599ec1c0c8481 100644 --- a/crates/bevy_reflect/src/impls/macros/set.rs +++ b/crates/bevy_reflect/src/impls/macros/set.rs @@ -28,6 +28,10 @@ macro_rules! impl_reflect_for_hashset { .collect() } + fn retain(&mut self, f: &mut dyn FnMut(&dyn $crate::reflect::PartialReflect) -> bool) { + self.retain(move |value| f(value)); + } + fn insert_boxed(&mut self, value: bevy_platform::prelude::Box) -> bool { let value = V::take_from_reflect(value).unwrap_or_else(|value| { panic!( diff --git a/crates/bevy_reflect/src/map.rs b/crates/bevy_reflect/src/map.rs index 20531569e8e17..918803ee4cf17 100644 --- a/crates/bevy_reflect/src/map.rs +++ b/crates/bevy_reflect/src/map.rs @@ -56,15 +56,6 @@ pub trait Map: PartialReflect { /// If no value is associated with `key`, returns `None`. fn get_mut(&mut self, key: &dyn PartialReflect) -> Option<&mut dyn PartialReflect>; - /// Returns the key-value pair at `index` by reference, or `None` if out of bounds. - fn get_at(&self, index: usize) -> Option<(&dyn PartialReflect, &dyn PartialReflect)>; - - /// Returns the key-value pair at `index` by reference where the value is a mutable reference, or `None` if out of bounds. - fn get_at_mut( - &mut self, - index: usize, - ) -> Option<(&dyn PartialReflect, &mut dyn PartialReflect)>; - /// Returns the number of elements in the map. fn len(&self) -> usize; @@ -74,13 +65,18 @@ pub trait Map: PartialReflect { } /// Returns an iterator over the key-value pairs of the map. - fn iter(&self) -> MapIter; + fn iter(&self) -> Box + '_>; /// Drain the key-value pairs of this map to get a vector of owned values. /// /// After calling this function, `self` will be empty. fn drain(&mut self) -> Vec<(Box, Box)>; + /// Retain only the elements specified by the predicate. + /// + /// In other words, remove all pairs `(k, v)` such that `f(&k, &mut v)` returns `false`. + fn retain(&mut self, f: &mut dyn FnMut(&dyn PartialReflect, &mut dyn PartialReflect) -> bool); + /// Creates a new [`DynamicMap`] from this map. fn to_dynamic_map(&self) -> DynamicMap { let mut map = DynamicMap::default(); @@ -218,12 +214,11 @@ macro_rules! hash_error { }} } -/// An ordered mapping between reflected values. +/// An unordered mapping between reflected values. #[derive(Default)] pub struct DynamicMap { represented_type: Option<&'static TypeInfo>, - values: Vec<(Box, Box)>, - indices: HashTable, + hash_table: HashTable<(Box, Box)>, } impl DynamicMap { @@ -254,13 +249,12 @@ impl DynamicMap { value.reflect_hash().expect(&hash_error!(value)) } - fn internal_eq<'a>( - value: &'a dyn PartialReflect, - values: &'a [(Box, Box)], - ) -> impl FnMut(&usize) -> bool + 'a { - |&index| { - value - .reflect_partial_eq(&*values[index].0) + fn internal_eq( + key: &dyn PartialReflect, + ) -> impl FnMut(&(Box, Box)) -> bool + '_ { + |(other, _)| { + key + .reflect_partial_eq(&**other) .expect("underlying type does not reflect `PartialEq` and hence doesn't support equality checks") } } @@ -268,46 +262,33 @@ impl DynamicMap { impl Map for DynamicMap { fn get(&self, key: &dyn PartialReflect) -> Option<&dyn PartialReflect> { - let hash = Self::internal_hash(key); - let eq = Self::internal_eq(key, &self.values); - self.indices - .find(hash, eq) - .map(|&index| &*self.values[index].1) + self.hash_table + .find(Self::internal_hash(key), Self::internal_eq(key)) + .map(|(_, value)| &**value) } fn get_mut(&mut self, key: &dyn PartialReflect) -> Option<&mut dyn PartialReflect> { - let hash = Self::internal_hash(key); - let eq = Self::internal_eq(key, &self.values); - self.indices - .find(hash, eq) - .map(|&index| &mut *self.values[index].1) - } - - fn get_at(&self, index: usize) -> Option<(&dyn PartialReflect, &dyn PartialReflect)> { - self.values - .get(index) - .map(|(key, value)| (&**key, &**value)) - } - - fn get_at_mut( - &mut self, - index: usize, - ) -> Option<(&dyn PartialReflect, &mut dyn PartialReflect)> { - self.values - .get_mut(index) - .map(|(key, value)| (&**key, &mut **value)) + self.hash_table + .find_mut(Self::internal_hash(key), Self::internal_eq(key)) + .map(|(_, value)| &mut **value) } fn len(&self) -> usize { - self.values.len() + self.hash_table.len() } - fn iter(&self) -> MapIter { - MapIter::new(self) + fn iter(&self) -> Box + '_> { + let iter = self.hash_table.iter().map(|(k, v)| (&**k, &**v)); + Box::new(iter) } fn drain(&mut self) -> Vec<(Box, Box)> { - self.values.drain(..).collect() + self.hash_table.drain().collect() + } + + fn retain(&mut self, f: &mut dyn FnMut(&dyn PartialReflect, &mut dyn PartialReflect) -> bool) { + self.hash_table + .retain(move |(key, value)| f(&**key, &mut **value)); } fn insert_boxed( @@ -322,20 +303,15 @@ impl Map for DynamicMap { ); let hash = Self::internal_hash(&*key); - let eq = Self::internal_eq(&*key, &self.values); - match self.indices.find(hash, eq) { - Some(&index) => { - let (key_ref, value_ref) = &mut self.values[index]; - *key_ref = key; - let old_value = core::mem::replace(value_ref, value); - Some(old_value) - } + let eq = Self::internal_eq(&*key); + match self.hash_table.find_mut(hash, eq) { + Some((_, old)) => Some(std::mem::replace(old, value)), None => { - let index = self.values.len(); - self.values.push((key, value)); - self.indices.insert_unique(hash, index, |&index| { - Self::internal_hash(&*self.values[index].0) - }); + self.hash_table.insert_unique( + Self::internal_hash(key.as_ref()), + (key, value), + |(key, _)| Self::internal_hash(&**key), + ); None } } @@ -343,26 +319,10 @@ impl Map for DynamicMap { fn remove(&mut self, key: &dyn PartialReflect) -> Option> { let hash = Self::internal_hash(key); - let eq = Self::internal_eq(key, &self.values); - match self.indices.find_entry(hash, eq) { + let eq = Self::internal_eq(key); + match self.hash_table.find_entry(hash, eq) { Ok(entry) => { - let (index, _) = entry.remove(); - let (_, old_value) = self.values.swap_remove(index); - - // The `swap_remove` might have moved the last element of `values` - // to `index`, so we might need to fix up its index in `indices`. - // If the removed element was also the last element there's nothing to - // fixup and this will return `None`, otherwise it returns the key - // whose index needs to be fixed up. - if let Some((moved_key, _)) = self.values.get(index) { - let hash = Self::internal_hash(&**moved_key); - let moved_index = self - .indices - .find_mut(hash, |&moved_index| moved_index == self.values.len()) - .expect("key inserted in a `DynamicMap` is no longer present, this means its reflected `Hash` might be incorrect"); - *moved_index = index; - } - + let ((_, old_value), _) = entry.remove(); Some(old_value) } Err(_) => None, @@ -451,35 +411,6 @@ impl Debug for DynamicMap { } } -/// An iterator over the key-value pairs of a [`Map`]. -pub struct MapIter<'a> { - map: &'a dyn Map, - index: usize, -} - -impl MapIter<'_> { - /// Creates a new [`MapIter`]. - #[inline] - pub const fn new(map: &dyn Map) -> MapIter { - MapIter { map, index: 0 } - } -} - -impl<'a> Iterator for MapIter<'a> { - type Item = (&'a dyn PartialReflect, &'a dyn PartialReflect); - - fn next(&mut self) -> Option { - let value = self.map.get_at(self.index); - self.index += value.is_some() as usize; - value - } - - fn size_hint(&self) -> (usize, Option) { - let size = self.map.len(); - (size, Some(size)) - } -} - impl FromIterator<(Box, Box)> for DynamicMap { fn from_iter, Box)>>( items: I, @@ -504,24 +435,30 @@ impl FromIterator<(K, V)> for DynamicMap { impl IntoIterator for DynamicMap { type Item = (Box, Box); - type IntoIter = alloc::vec::IntoIter; + type IntoIter = bevy_platform::collections::hash_table::IntoIter; fn into_iter(self) -> Self::IntoIter { - self.values.into_iter() + self.hash_table.into_iter() } } impl<'a> IntoIterator for &'a DynamicMap { type Item = (&'a dyn PartialReflect, &'a dyn PartialReflect); - type IntoIter = MapIter<'a>; + type IntoIter = core::iter::Map< + bevy_platform::collections::hash_table::Iter< + 'a, + (Box, Box), + >, + fn(&'a (Box, Box)) -> Self::Item, + >; fn into_iter(self) -> Self::IntoIter { - self.iter() + self.hash_table + .iter() + .map(|(k, v)| (k.as_ref(), v.as_ref())) } } -impl<'a> ExactSizeIterator for MapIter<'a> {} - /// Compares a [`Map`] with a [`PartialReflect`] value. /// /// Returns true if and only if all of the following are true: @@ -615,117 +552,17 @@ pub fn map_try_apply(a: &mut M, b: &dyn PartialReflect) -> Result<(), Ap a.insert_boxed(key.to_dynamic(), b_value.to_dynamic()); } } + a.retain(&mut |key, _| map_value.get(key).is_some()); Ok(()) } #[cfg(test)] mod tests { - use super::{DynamicMap, Map}; - use alloc::{ - borrow::ToOwned, - string::{String, ToString}, - }; - #[test] - fn test_into_iter() { - let expected = ["foo", "bar", "baz"]; + use crate::PartialReflect; - let mut map = DynamicMap::default(); - map.insert(0usize, expected[0].to_string()); - map.insert(1usize, expected[1].to_string()); - map.insert(2usize, expected[2].to_string()); - - for (index, item) in map.into_iter().enumerate() { - let key = item - .0 - .try_take::() - .expect("couldn't downcast to usize"); - let value = item - .1 - .try_take::() - .expect("couldn't downcast to String"); - assert_eq!(index, key); - assert_eq!(expected[index], value); - } - } - - #[test] - fn test_map_get_at() { - let values = ["first", "second", "third"]; - let mut map = DynamicMap::default(); - map.insert(0usize, values[0].to_string()); - map.insert(1usize, values[1].to_string()); - map.insert(1usize, values[2].to_string()); - - let (key_r, value_r) = map.get_at(1).expect("Item wasn't found"); - let value = value_r - .try_downcast_ref::() - .expect("Couldn't downcast to String"); - let key = key_r - .try_downcast_ref::() - .expect("Couldn't downcast to usize"); - assert_eq!(key, &1usize); - assert_eq!(value, &values[2].to_owned()); - - assert!(map.get_at(2).is_none()); - map.remove(&1usize); - assert!(map.get_at(1).is_none()); - } - - #[test] - fn test_map_get_at_mut() { - let values = ["first", "second", "third"]; - let mut map = DynamicMap::default(); - map.insert(0usize, values[0].to_string()); - map.insert(1usize, values[1].to_string()); - map.insert(1usize, values[2].to_string()); - - let (key_r, value_r) = map.get_at_mut(1).expect("Item wasn't found"); - let value = value_r - .try_downcast_mut::() - .expect("Couldn't downcast to String"); - let key = key_r - .try_downcast_ref::() - .expect("Couldn't downcast to usize"); - assert_eq!(key, &1usize); - assert_eq!(value, &mut values[2].to_owned()); - - value.clone_from(&values[0].to_owned()); - - assert_eq!( - map.get(&1usize) - .expect("Item wasn't found") - .try_downcast_ref::() - .expect("Couldn't downcast to String"), - &values[0].to_owned() - ); - - assert!(map.get_at(2).is_none()); - } - - #[test] - fn next_index_increment() { - let values = ["first", "last"]; - let mut map = DynamicMap::default(); - map.insert(0usize, values[0]); - map.insert(1usize, values[1]); - - let mut iter = map.iter(); - let size = iter.len(); - - for _ in 0..2 { - let prev_index = iter.index; - assert!(iter.next().is_some()); - assert_eq!(prev_index, iter.index - 1); - } - - // When None we should no longer increase index - for _ in 0..2 { - assert!(iter.next().is_none()); - assert_eq!(size, iter.index); - } - } + use super::{DynamicMap, Map}; #[test] fn remove() { @@ -743,4 +580,21 @@ mod tests { assert!(map.remove(&1).is_none()); assert!(map.get(&1).is_none()); } + + #[test] + fn apply() { + let mut map_a = DynamicMap::default(); + map_a.insert(0, 0); + map_a.insert(1, 1); + + let mut map_b = DynamicMap::default(); + map_b.insert(10, 10); + map_b.insert(1, 5); + + map_a.apply(&map_b); + + assert!(map_a.get(&0).is_none()); + assert_eq!(map_a.get(&1).unwrap().try_downcast_ref(), Some(&5)); + assert_eq!(map_a.get(&10).unwrap().try_downcast_ref(), Some(&10)); + } } diff --git a/crates/bevy_reflect/src/set.rs b/crates/bevy_reflect/src/set.rs index 01888e7825dc1..1052650ea67b9 100644 --- a/crates/bevy_reflect/src/set.rs +++ b/crates/bevy_reflect/src/set.rs @@ -67,6 +67,11 @@ pub trait Set: PartialReflect { /// After calling this function, `self` will be empty. fn drain(&mut self) -> Vec>; + /// Retain only the elements specified by the predicate. + /// + /// In other words, remove all elements `e` for which `f(&e)` returns `false`. + fn retain(&mut self, f: &mut dyn FnMut(&dyn PartialReflect) -> bool); + /// Creates a new [`DynamicSet`] from this set. fn to_dynamic_set(&self) -> DynamicSet { let mut set = DynamicSet::default(); @@ -139,7 +144,7 @@ impl SetInfo { impl_generic_info_methods!(generics); } -/// An ordered set of reflected values. +/// An unordered set of reflected values. #[derive(Default)] pub struct DynamicSet { represented_type: Option<&'static TypeInfo>, @@ -205,6 +210,10 @@ impl Set for DynamicSet { self.hash_table.drain().collect::>() } + fn retain(&mut self, f: &mut dyn FnMut(&dyn PartialReflect) -> bool) { + self.hash_table.retain(move |value| f(&**value)); + } + fn insert_boxed(&mut self, value: Box) -> bool { assert_eq!( value.reflect_partial_eq(&*value), From 7f1011fc571a2a4a9ff7b9a91f0692fa44b354bc Mon Sep 17 00:00:00 2001 From: Azorlogh Date: Tue, 24 Jun 2025 19:48:49 +0200 Subject: [PATCH 2/7] fix wrong expected result, fix apply for sets & add test, fix docs --- crates/bevy_reflect/src/lib.rs | 1 - crates/bevy_reflect/src/map.rs | 2 ++ crates/bevy_reflect/src/reflect.rs | 4 ++-- crates/bevy_reflect/src/set.rs | 36 ++++++++++++++++++++++-------- 4 files changed, 31 insertions(+), 12 deletions(-) diff --git a/crates/bevy_reflect/src/lib.rs b/crates/bevy_reflect/src/lib.rs index 8b50c4b5b2273..eaf601ef0d353 100644 --- a/crates/bevy_reflect/src/lib.rs +++ b/crates/bevy_reflect/src/lib.rs @@ -1583,7 +1583,6 @@ mod tests { foo.apply(&foo_patch); let mut hash_map = >::default(); - hash_map.insert(1, 1); hash_map.insert(2, 3); hash_map.insert(3, 4); diff --git a/crates/bevy_reflect/src/map.rs b/crates/bevy_reflect/src/map.rs index 918803ee4cf17..65be81134eea9 100644 --- a/crates/bevy_reflect/src/map.rs +++ b/crates/bevy_reflect/src/map.rs @@ -521,6 +521,7 @@ pub fn map_debug(dyn_map: &dyn Map, f: &mut Formatter<'_>) -> core::fmt::Result /// 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. +/// If a key from `a` does not exist in `b`, the value is removed. /// /// # Panics /// @@ -536,6 +537,7 @@ pub fn map_apply(a: &mut M, b: &dyn PartialReflect) { /// and returns a Result. /// /// If a key from `b` does not exist in `a`, the value is cloned and inserted. +/// If a key from `a` does not exist in `b`, the value is removed. /// /// # Errors /// diff --git a/crates/bevy_reflect/src/reflect.rs b/crates/bevy_reflect/src/reflect.rs index c1e283a5f4753..1bd1795066a55 100644 --- a/crates/bevy_reflect/src/reflect.rs +++ b/crates/bevy_reflect/src/reflect.rs @@ -167,10 +167,10 @@ where /// and excess elements in `value` are appended to `self`. /// - If `Self` 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 `self` are inserted. + /// Keys which are not present in `self` are inserted, and keys from `self` which are not present in `value` are removed. /// - If `Self` is a [`Set`], then each element of `value` is applied to the corresponding /// element of `Self`. If an element of `value` does not exist in `Self` then it is - /// cloned and inserted. + /// cloned and inserted. If an element from `self` is not present in `value` then it is removed. /// - If `Self` is none of these, then `value` is downcast to `Self`, cloned, and /// assigned to `self`. /// diff --git a/crates/bevy_reflect/src/set.rs b/crates/bevy_reflect/src/set.rs index 1052650ea67b9..887deb7b36492 100644 --- a/crates/bevy_reflect/src/set.rs +++ b/crates/bevy_reflect/src/set.rs @@ -450,27 +450,23 @@ pub fn set_debug(dyn_set: &dyn Set, f: &mut Formatter<'_>) -> core::fmt::Result /// Applies the elements of reflected set `b` to the corresponding elements of set `a`. /// /// If a value from `b` does not exist in `a`, the value is cloned and inserted. +/// If a value from `a` does not exist in `b`, the value is removed. /// /// # Panics /// /// This function panics if `b` is not a reflected set. #[inline] pub fn set_apply(a: &mut M, b: &dyn PartialReflect) { - if let ReflectRef::Set(set_value) = b.reflect_ref() { - for b_value in set_value.iter() { - if a.get(b_value).is_none() { - a.insert_boxed(b_value.to_dynamic()); - } - } - } else { - panic!("Attempted to apply a non-set type to a set type."); + if let Err(err) = set_try_apply(a, b) { + panic!("{err}"); } } /// Tries to apply the elements of reflected set `b` to the corresponding elements of set `a` /// and returns a Result. /// -/// If a key from `b` does not exist in `a`, the value is cloned and inserted. +/// If a value from `b` does not exist in `a`, the value is cloned and inserted. +/// If a value from `a` does not exist in `b`, the value is removed. /// /// # Errors /// @@ -485,12 +481,15 @@ pub fn set_try_apply(a: &mut S, b: &dyn PartialReflect) -> Result<(), Ap a.insert_boxed(b_value.to_dynamic()); } } + a.retain(&mut |value| set_value.get(value).is_some()); Ok(()) } #[cfg(test)] mod tests { + use crate::{PartialReflect, Set}; + use super::DynamicSet; use alloc::string::{String, ToString}; @@ -514,4 +513,23 @@ mod tests { assert_eq!(expected[index], value); } } + + #[test] + fn apply() { + let mut map_a = DynamicSet::default(); + map_a.insert(0); + map_a.insert(1); + + let mut map_b = DynamicSet::default(); + map_b.insert(1); + map_b.insert(2); + + map_a.apply(&map_b); + + std::println!("{:?}", map_a); + + assert!(map_a.get(&0).is_none()); + assert_eq!(map_a.get(&1).unwrap().try_downcast_ref(), Some(&1)); + assert_eq!(map_a.get(&2).unwrap().try_downcast_ref(), Some(&2)); + } } From 0da9a30ab6e461db3ebb8ed4cd1747a96884e120 Mon Sep 17 00:00:00 2001 From: Azorlogh Date: Tue, 24 Jun 2025 19:52:39 +0200 Subject: [PATCH 3/7] use `core::mem::replace` instead of `std::mem::replace` --- crates/bevy_reflect/src/map.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_reflect/src/map.rs b/crates/bevy_reflect/src/map.rs index 65be81134eea9..e7178692023e2 100644 --- a/crates/bevy_reflect/src/map.rs +++ b/crates/bevy_reflect/src/map.rs @@ -305,7 +305,7 @@ impl Map for DynamicMap { let hash = Self::internal_hash(&*key); let eq = Self::internal_eq(&*key); match self.hash_table.find_mut(hash, eq) { - Some((_, old)) => Some(std::mem::replace(old, value)), + Some((_, old)) => Some(core::mem::replace(old, value)), None => { self.hash_table.insert_unique( Self::internal_hash(key.as_ref()), From a5db38e50f75cbeb2ae1779e147c527f9e00fe06 Mon Sep 17 00:00:00 2001 From: Azorlogh Date: Tue, 24 Jun 2025 20:01:35 +0200 Subject: [PATCH 4/7] cleanup --- crates/bevy_reflect/src/set.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/crates/bevy_reflect/src/set.rs b/crates/bevy_reflect/src/set.rs index 887deb7b36492..e464ee4aab699 100644 --- a/crates/bevy_reflect/src/set.rs +++ b/crates/bevy_reflect/src/set.rs @@ -526,8 +526,6 @@ mod tests { map_a.apply(&map_b); - std::println!("{:?}", map_a); - assert!(map_a.get(&0).is_none()); assert_eq!(map_a.get(&1).unwrap().try_downcast_ref(), Some(&1)); assert_eq!(map_a.get(&2).unwrap().try_downcast_ref(), Some(&2)); From 97f8cdfb2881414f89860d6994462ce0a99efa8d Mon Sep 17 00:00:00 2001 From: Azorlogh Date: Wed, 25 Jun 2025 15:13:47 +0200 Subject: [PATCH 5/7] migration guide --- release-content/migration-guides/map_set_apply.md | 8 ++++++++ 1 file changed, 8 insertions(+) create mode 100644 release-content/migration-guides/map_set_apply.md diff --git a/release-content/migration-guides/map_set_apply.md b/release-content/migration-guides/map_set_apply.md new file mode 100644 index 0000000000000..63095136ef90d --- /dev/null +++ b/release-content/migration-guides/map_set_apply.md @@ -0,0 +1,8 @@ +--- +title: `DynamicMap` is now unordered and `Map::get_at` and `Map::get_at_mut` are removed. +pull_requests: [19802] +--- + +`DynamicMap` is now unordered, and the `Map` trait no longer assumes implementors to be ordered. If you previously relied on them being ordered, you should now store a list of keys (`Vec>`) separately. + +`Map::get_at` and `Map::get_at_mut` are now removed. You should no longer use `usize` to index into the map, and instead use `&dyn PartialReflect` with `Map::get` and `Map::get_mut`. From d151250df044da61b2b8bf33cdea1e944a50c6d1 Mon Sep 17 00:00:00 2001 From: Azorlogh Date: Wed, 25 Jun 2025 15:15:06 +0200 Subject: [PATCH 6/7] migration guide (2) --- release-content/migration-guides/map_set_apply.md | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/release-content/migration-guides/map_set_apply.md b/release-content/migration-guides/map_set_apply.md index 63095136ef90d..3677b2a1b8861 100644 --- a/release-content/migration-guides/map_set_apply.md +++ b/release-content/migration-guides/map_set_apply.md @@ -1,8 +1,11 @@ --- -title: `DynamicMap` is now unordered and `Map::get_at` and `Map::get_at_mut` are removed. +title: `DynamicMap` is now unordered, `Map::get_at` and `Map::get_at_mut` are now removed, and `apply` removes excess entries from reflected maps. pull_requests: [19802] --- `DynamicMap` is now unordered, and the `Map` trait no longer assumes implementors to be ordered. If you previously relied on them being ordered, you should now store a list of keys (`Vec>`) separately. `Map::get_at` and `Map::get_at_mut` are now removed. You should no longer use `usize` to index into the map, and instead use `&dyn PartialReflect` with `Map::get` and `Map::get_mut`. + +`PartialReflect::apply(self, other)` for maps now removes excess entries (entries present in `self` that are not present in `other`). +If you need those entries to be preserved, you will need to re-insert them manually. From 612db3af6005396c053f8697e4ebdfb9c042fa17 Mon Sep 17 00:00:00 2001 From: Azorlogh Date: Wed, 25 Jun 2025 15:15:35 +0200 Subject: [PATCH 7/7] migration guide (2) --- release-content/migration-guides/map_set_apply.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/release-content/migration-guides/map_set_apply.md b/release-content/migration-guides/map_set_apply.md index 3677b2a1b8861..c9fadc1c0c857 100644 --- a/release-content/migration-guides/map_set_apply.md +++ b/release-content/migration-guides/map_set_apply.md @@ -7,5 +7,5 @@ pull_requests: [19802] `Map::get_at` and `Map::get_at_mut` are now removed. You should no longer use `usize` to index into the map, and instead use `&dyn PartialReflect` with `Map::get` and `Map::get_mut`. -`PartialReflect::apply(self, other)` for maps now removes excess entries (entries present in `self` that are not present in `other`). +`PartialReflect::apply(self, other)` for maps now removes excess entries (entries present in `self` which are not present in `other`). If you need those entries to be preserved, you will need to re-insert them manually.