From 247d8857798dc459094ea00ecf5125e3d46048f5 Mon Sep 17 00:00:00 2001 From: Gino Valente Date: Mon, 13 Feb 2023 21:07:53 +0000 Subject: [PATCH] bevy_reflect: Decouple `List` and `Array` traits (#7467) Resolves #7121 Decouples `List` and `Array` by removing `Array` as a supertrait of `List`. Additionally, similar methods from `Array` have been added to `List` so that their usages can remain largely unchanged. My guess for why we originally made `List` a subtrait of `Array` is that they share a lot of common operations. We could potentially move these overlapping methods to a `Sequence` (name taken from #7059) trait and make that a supertrait of both. This would allow functions to contain logic that simply operates on a sequence rather than "list vs array". However, this means that we'd need to add methods for converting to a `dyn Sequence`. It also might be confusing since we wouldn't add a `ReflectRef::Sequence` or anything like that. Is such a trait worth adding (either in this PR or a followup one)? --- - Removed `Array` as supertrait of `List` - Added methods to `List` that were previously provided by `Array` The `List` trait is no longer dependent on `Array`. Implementors of `List` can remove the `Array` impl and move its methods into the `List` impl (with only a couple tweaks). ```rust // BEFORE impl Array for Foo { fn get(&self, index: usize) -> Option<&dyn Reflect> {/* ... */} fn get_mut(&mut self, index: usize) -> Option<&mut dyn Reflect> {/* ... */} fn len(&self) -> usize {/* ... */} fn is_empty(&self) -> bool {/* ... */} fn iter(&self) -> ArrayIter {/* ... */} fn drain(self: Box) -> Vec> {/* ... */} fn clone_dynamic(&self) -> DynamicArray {/* ... */} } impl List for Foo { fn insert(&mut self, index: usize, element: Box) {/* ... */} fn remove(&mut self, index: usize) -> Box {/* ... */} fn push(&mut self, value: Box) {/* ... */} fn pop(&mut self) -> Option> {/* ... */} fn clone_dynamic(&self) -> DynamicList {/* ... */} } // AFTER impl List for Foo { fn get(&self, index: usize) -> Option<&dyn Reflect> {/* ... */} fn get_mut(&mut self, index: usize) -> Option<&mut dyn Reflect> {/* ... */} fn insert(&mut self, index: usize, element: Box) {/* ... */} fn remove(&mut self, index: usize) -> Box {/* ... */} fn push(&mut self, value: Box) {/* ... */} fn pop(&mut self) -> Option> {/* ... */} fn len(&self) -> usize {/* ... */} fn is_empty(&self) -> bool {/* ... */} fn iter(&self) -> ListIter {/* ... */} fn drain(self: Box) -> Vec> {/* ... */} fn clone_dynamic(&self) -> DynamicList {/* ... */} } ``` Some other small tweaks that will need to be made include: - Use `ListIter` for `List::iter` instead of `ArrayIter` (the return type from `Array::iter`) - Replace `array_hash` with `list_hash` in `Reflect::reflect_hash` for implementors of `List` --- crates/bevy_reflect/src/array.rs | 10 +- crates/bevy_reflect/src/impls/smallvec.rs | 41 ++++---- crates/bevy_reflect/src/impls/std.rs | 42 ++++---- crates/bevy_reflect/src/lib.rs | 6 +- crates/bevy_reflect/src/list.rs | 123 ++++++++++++++++------ crates/bevy_render/src/globals.wgsl | 16 --- 6 files changed, 138 insertions(+), 100 deletions(-) delete mode 100644 crates/bevy_render/src/globals.wgsl diff --git a/crates/bevy_reflect/src/array.rs b/crates/bevy_reflect/src/array.rs index 942e437c33ca5d..4b7b91c09b1589 100644 --- a/crates/bevy_reflect/src/array.rs +++ b/crates/bevy_reflect/src/array.rs @@ -21,19 +21,25 @@ use std::{ pub trait Array: Reflect { /// Returns a reference to the element at `index`, or `None` if out of bounds. fn get(&self, index: usize) -> Option<&dyn Reflect>; + /// Returns a mutable reference to the element at `index`, or `None` if out of bounds. fn get_mut(&mut self, index: usize) -> Option<&mut dyn Reflect>; - /// Returns the number of elements in the collection. + + /// Returns the number of elements in the array. fn len(&self) -> usize; + /// Returns `true` if the collection contains no elements. fn is_empty(&self) -> bool { self.len() == 0 } - /// Returns an iterator over the collection. + + /// Returns an iterator over the array. fn iter(&self) -> ArrayIter; + /// Drain the elements of this array to get a vector of owned values. fn drain(self: Box) -> Vec>; + /// Clones the list, producing a [`DynamicArray`]. fn clone_dynamic(&self) -> DynamicArray { DynamicArray { name: self.type_name().to_string(), diff --git a/crates/bevy_reflect/src/impls/smallvec.rs b/crates/bevy_reflect/src/impls/smallvec.rs index c911653bd695c4..769c541d9d6cf4 100644 --- a/crates/bevy_reflect/src/impls/smallvec.rs +++ b/crates/bevy_reflect/src/impls/smallvec.rs @@ -3,11 +3,11 @@ use std::any::Any; use crate::utility::GenericTypeInfoCell; use crate::{ - Array, ArrayIter, FromReflect, FromType, GetTypeRegistration, List, ListInfo, Reflect, - ReflectFromPtr, ReflectMut, ReflectOwned, ReflectRef, TypeInfo, TypeRegistration, Typed, + FromReflect, FromType, GetTypeRegistration, List, ListInfo, ListIter, Reflect, ReflectFromPtr, + ReflectMut, ReflectOwned, ReflectRef, TypeInfo, TypeRegistration, Typed, }; -impl Array for SmallVec +impl List for SmallVec where T::Item: FromReflect, { @@ -27,25 +27,6 @@ where } } - fn len(&self) -> usize { - >::len(self) - } - - fn iter(&self) -> ArrayIter { - ArrayIter::new(self) - } - - fn drain(self: Box) -> Vec> { - self.into_iter() - .map(|value| Box::new(value) as Box) - .collect() - } -} - -impl List for SmallVec -where - T::Item: FromReflect, -{ fn insert(&mut self, index: usize, value: Box) { let value = value.take::().unwrap_or_else(|value| { ::Item::from_reflect(&*value).unwrap_or_else(|| { @@ -77,6 +58,20 @@ where fn pop(&mut self) -> Option> { self.pop().map(|value| Box::new(value) as Box) } + + fn len(&self) -> usize { + >::len(self) + } + + fn iter(&self) -> ListIter { + ListIter::new(self) + } + + fn drain(self: Box) -> Vec> { + self.into_iter() + .map(|value| Box::new(value) as Box) + .collect() + } } impl Reflect for SmallVec @@ -137,7 +132,7 @@ where } fn clone_value(&self) -> Box { - Box::new(List::clone_dynamic(self)) + Box::new(self.clone_dynamic()) } fn reflect_partial_eq(&self, value: &dyn Reflect) -> Option { diff --git a/crates/bevy_reflect/src/impls/std.rs b/crates/bevy_reflect/src/impls/std.rs index 4555447255bc7f..efae9fdc8070cc 100644 --- a/crates/bevy_reflect/src/impls/std.rs +++ b/crates/bevy_reflect/src/impls/std.rs @@ -180,7 +180,7 @@ impl_from_reflect_value!(NonZeroI8); macro_rules! impl_reflect_for_veclike { ($ty:ty, $insert:expr, $remove:expr, $push:expr, $pop:expr, $sub:ty) => { - impl Array for $ty { + impl List for $ty { #[inline] fn get(&self, index: usize) -> Option<&dyn Reflect> { <$sub>::get(self, index).map(|value| value as &dyn Reflect) @@ -191,25 +191,6 @@ macro_rules! impl_reflect_for_veclike { <$sub>::get_mut(self, index).map(|value| value as &mut dyn Reflect) } - #[inline] - fn len(&self) -> usize { - <$sub>::len(self) - } - - #[inline] - fn iter(&self) -> ArrayIter { - ArrayIter::new(self) - } - - #[inline] - fn drain(self: Box) -> Vec> { - self.into_iter() - .map(|value| Box::new(value) as Box) - .collect() - } - } - - impl List for $ty { fn insert(&mut self, index: usize, value: Box) { let value = value.take::().unwrap_or_else(|value| { T::from_reflect(&*value).unwrap_or_else(|| { @@ -239,6 +220,23 @@ macro_rules! impl_reflect_for_veclike { fn pop(&mut self) -> Option> { $pop(self).map(|value| Box::new(value) as Box) } + + #[inline] + fn len(&self) -> usize { + <$sub>::len(self) + } + + #[inline] + fn iter(&self) -> $crate::ListIter { + $crate::ListIter::new(self) + } + + #[inline] + fn drain(self: Box) -> Vec> { + self.into_iter() + .map(|value| Box::new(value) as Box) + .collect() + } } impl Reflect for $ty { @@ -296,11 +294,11 @@ macro_rules! impl_reflect_for_veclike { } fn clone_value(&self) -> Box { - Box::new(List::clone_dynamic(self)) + Box::new(self.clone_dynamic()) } fn reflect_hash(&self) -> Option { - crate::array_hash(self) + crate::list_hash(self) } fn reflect_partial_eq(&self, value: &dyn Reflect) -> Option { diff --git a/crates/bevy_reflect/src/lib.rs b/crates/bevy_reflect/src/lib.rs index d0bf81058442ed..2a80f501dd67ba 100644 --- a/crates/bevy_reflect/src/lib.rs +++ b/crates/bevy_reflect/src/lib.rs @@ -394,7 +394,7 @@ mod tests { list.push(3isize); list.push(4isize); list.push(5isize); - foo_patch.insert("c", List::clone_dynamic(&list)); + foo_patch.insert("c", list.clone_dynamic()); let mut map = DynamicMap::default(); map.insert(2usize, 3i8); @@ -607,11 +607,11 @@ mod tests { #[test] fn dynamic_names() { let list = Vec::::new(); - let dyn_list = List::clone_dynamic(&list); + let dyn_list = list.clone_dynamic(); assert_eq!(dyn_list.type_name(), std::any::type_name::>()); let array = [b'0'; 4]; - let dyn_array = Array::clone_dynamic(&array); + let dyn_array = array.clone_dynamic(); assert_eq!(dyn_array.type_name(), std::any::type_name::<[u8; 4]>()); let map = HashMap::::default(); diff --git a/crates/bevy_reflect/src/list.rs b/crates/bevy_reflect/src/list.rs index 922102e4f3b925..1dd1a8b183cd85 100644 --- a/crates/bevy_reflect/src/list.rs +++ b/crates/bevy_reflect/src/list.rs @@ -1,16 +1,19 @@ use std::any::{Any, TypeId}; use std::fmt::{Debug, Formatter}; +use std::hash::{Hash, Hasher}; use crate::utility::NonGenericTypeInfoCell; use crate::{ - Array, ArrayIter, DynamicArray, DynamicInfo, FromReflect, Reflect, ReflectMut, ReflectOwned, - ReflectRef, TypeInfo, Typed, ValueInfo, + DynamicInfo, FromReflect, Reflect, ReflectMut, ReflectOwned, ReflectRef, TypeInfo, Typed, + ValueInfo, }; /// An ordered, mutable list of [Reflect] items. This corresponds to types like [`std::vec::Vec`]. /// -/// This is a sub-trait of [`Array`], however as it implements [insertion](List::insert) and [removal](List::remove), -/// it's internal size may change. +/// Unlike the [`Array`](crate::Array) trait, implementors of this type are not expected to +/// maintain a constant length. +/// Methods like [insertion](List::insert) and [removal](List::remove) explicitly allow for their +/// internal size to change. /// /// This trait expects index 0 to contain the _front_ element. /// The _back_ element must refer to the element with the largest index. @@ -18,7 +21,13 @@ use crate::{ /// /// [`push`](List::push) and [`pop`](List::pop) have default implementations, /// however it may be faster to implement them manually. -pub trait List: Reflect + Array { +pub trait List: Reflect { + /// Returns a reference to the element at `index`, or `None` if out of bounds. + fn get(&self, index: usize) -> Option<&dyn Reflect>; + + /// Returns a mutable reference to the element at `index`, or `None` if out of bounds. + fn get_mut(&mut self, index: usize) -> Option<&mut dyn Reflect>; + /// Inserts an element at position `index` within the list, /// shifting all elements after it towards the back of the list. /// @@ -47,6 +56,20 @@ pub trait List: Reflect + Array { } } + /// Returns the number of elements in the list. + fn len(&self) -> usize; + + /// Returns `true` if the collection contains no elements. + fn is_empty(&self) -> bool { + self.len() == 0 + } + + /// Returns an iterator over the list. + fn iter(&self) -> ListIter; + + /// Drain the elements of this list to get a vector of owned values. + fn drain(self: Box) -> Vec>; + /// Clones the list, producing a [`DynamicList`]. fn clone_dynamic(&self) -> DynamicList { DynamicList { @@ -158,7 +181,7 @@ impl DynamicList { } } -impl Array for DynamicList { +impl List for DynamicList { fn get(&self, index: usize) -> Option<&dyn Reflect> { self.values.get(index).map(|value| &**value) } @@ -167,31 +190,6 @@ impl Array for DynamicList { self.values.get_mut(index).map(|value| &mut **value) } - fn len(&self) -> usize { - self.values.len() - } - - fn iter(&self) -> ArrayIter { - ArrayIter::new(self) - } - - fn drain(self: Box) -> Vec> { - self.values - } - - fn clone_dynamic(&self) -> DynamicArray { - DynamicArray { - name: self.name.clone(), - values: self - .values - .iter() - .map(|value| value.clone_value()) - .collect(), - } - } -} - -impl List for DynamicList { fn insert(&mut self, index: usize, element: Box) { self.values.insert(index, element); } @@ -208,6 +206,18 @@ impl List for DynamicList { self.values.pop() } + fn len(&self) -> usize { + self.values.len() + } + + fn iter(&self) -> ListIter { + ListIter::new(self) + } + + fn drain(self: Box) -> Vec> { + self.values + } + fn clone_dynamic(&self) -> DynamicList { DynamicList { name: self.name.clone(), @@ -288,12 +298,12 @@ impl Reflect for DynamicList { #[inline] fn clone_value(&self) -> Box { - Box::new(List::clone_dynamic(self)) + Box::new(self.clone_dynamic()) } #[inline] fn reflect_hash(&self) -> Option { - crate::array_hash(self) + list_hash(self) } fn reflect_partial_eq(&self, value: &dyn Reflect) -> Option { @@ -329,6 +339,51 @@ impl IntoIterator for DynamicList { } } +/// An iterator over an [`List`]. +pub struct ListIter<'a> { + list: &'a dyn List, + index: usize, +} + +impl<'a> ListIter<'a> { + /// Creates a new [`ListIter`]. + #[inline] + pub const fn new(list: &'a dyn List) -> ListIter { + ListIter { list, index: 0 } + } +} + +impl<'a> Iterator for ListIter<'a> { + type Item = &'a dyn Reflect; + + #[inline] + fn next(&mut self) -> Option { + let value = self.list.get(self.index); + self.index += 1; + value + } + + #[inline] + fn size_hint(&self) -> (usize, Option) { + let size = self.list.len(); + (size, Some(size)) + } +} + +impl<'a> ExactSizeIterator for ListIter<'a> {} + +/// Returns the `u64` hash of the given [list](List). +#[inline] +pub fn list_hash(list: &L) -> Option { + let mut hasher = crate::ReflectHasher::default(); + std::any::Any::type_id(list).hash(&mut hasher); + list.len().hash(&mut hasher); + for value in list.iter() { + hasher.write_u64(value.reflect_hash()?); + } + Some(hasher.finish()) +} + /// Applies the elements of `b` to the corresponding elements of `a`. /// /// If the length of `b` is greater than that of `a`, the excess elements of `b` @@ -346,7 +401,7 @@ pub fn list_apply(a: &mut L, b: &dyn Reflect) { v.apply(value); } } else { - List::push(a, value.clone_value()); + a.push(value.clone_value()); } } } else { diff --git a/crates/bevy_render/src/globals.wgsl b/crates/bevy_render/src/globals.wgsl deleted file mode 100644 index 6e0f271982b201..00000000000000 --- a/crates/bevy_render/src/globals.wgsl +++ /dev/null @@ -1,16 +0,0 @@ -#define_import_path bevy_render::globals - -struct Globals { - // The time since startup in seconds - // Wraps to 0 after 1 hour. - time: f32, - // The delta time since the previous frame in seconds - delta_time: f32, - // Frame count since the start of the app. - // It wraps to zero when it reaches the maximum value of a u32. - frame_count: u32, -#ifdef SIXTEEN_BYTE_ALIGNMENT - // WebGL2 structs must be 16 byte aligned. - _webgl2_padding: f32 -#endif -};