From 0bd4ea7cede2a54d8278e85795c6d5e66f0aeb9a Mon Sep 17 00:00:00 2001 From: Nicola Papale Date: Mon, 18 Sep 2023 15:41:51 +0200 Subject: [PATCH] Provide getters for fields of ReflectFromPtr (#9748) # Objective The reasoning is similar to #8687. I'm building a dynamic query. Currently, I store the ReflectFromPtr in my dynamic `Fetch` type. [See relevant code](https://github.com/nicopap/bevy_mod_dynamic_query/blob/97ba68ae1e13f15cabfac1dcd58c2483396dfd3f/src/fetches.rs#L14-L17) However, `ReflectFromPtr` is: - 16 bytes for TypeId - 8 bytes for the non-mutable function pointer - 8 bytes for the mutable function pointer It's a lot, it adds 32 bytes to my base `Fetch` which is only `ComponendId` (8 bytes) for a total of 40 bytes. I only need one function per fetch, reducing the total dynamic fetch size to 16 bytes. Since I'm querying the components by the ComponendId associated with the function pointer I'm using, I don't need the TypeId, it's a redundant check. In fact, I've difficulties coming up with situations where checking the TypeId beforehand is relevant. So to me, if ReflectFromPtr makes sense as a public API, exposing the function pointers also makes sense. ## Solution - Make the fields public through methods. --- ## Changelog - Add `from_ptr` and `from_ptr_mut` methods to `ReflectFromPtr` to access the underlying function pointers - `ReflectFromPtr::as_reflect_ptr` is now `ReflectFromPtr::as_reflect` - `ReflectFromPtr::as_reflect_ptr_mut` is now `ReflectFromPtr::as_reflect_mut` ## Migration guide - `ReflectFromPtr::as_reflect_ptr` is now `ReflectFromPtr::as_reflect` - `ReflectFromPtr::as_reflect_ptr_mut` is now `ReflectFromPtr::as_reflect_mut` --- crates/bevy_ecs/src/change_detection.rs | 4 +- crates/bevy_reflect/src/type_registry.rs | 58 +++++++++++++++++------- 2 files changed, 44 insertions(+), 18 deletions(-) diff --git a/crates/bevy_ecs/src/change_detection.rs b/crates/bevy_ecs/src/change_detection.rs index 01284c067e..0db970ba58 100644 --- a/crates/bevy_ecs/src/change_detection.rs +++ b/crates/bevy_ecs/src/change_detection.rs @@ -833,7 +833,7 @@ impl<'a> MutUntyped<'a> { /// # let mut_untyped: MutUntyped = unimplemented!(); /// # let reflect_from_ptr: bevy_reflect::ReflectFromPtr = unimplemented!(); /// // SAFETY: from the context it is known that `ReflectFromPtr` was made for the type of the `MutUntyped` - /// mut_untyped.map_unchanged(|ptr| unsafe { reflect_from_ptr.as_reflect_ptr_mut(ptr) }); + /// mut_untyped.map_unchanged(|ptr| unsafe { reflect_from_ptr.as_reflect_mut(ptr) }); /// ``` pub fn map_unchanged(self, f: impl FnOnce(PtrMut<'a>) -> &'a mut T) -> Mut<'a, T> { Mut { @@ -1168,7 +1168,7 @@ mod tests { let mut new = value.map_unchanged(|ptr| { // SAFETY: The underlying type of `ptr` matches `reflect_from_ptr`. - let value = unsafe { reflect_from_ptr.as_reflect_ptr_mut(ptr) }; + let value = unsafe { reflect_from_ptr.as_reflect_mut(ptr) }; value }); diff --git a/crates/bevy_reflect/src/type_registry.rs b/crates/bevy_reflect/src/type_registry.rs index aeaba79a2f..b7ce14d830 100644 --- a/crates/bevy_reflect/src/type_registry.rs +++ b/crates/bevy_reflect/src/type_registry.rs @@ -521,37 +521,63 @@ impl Deserialize<'a> + Reflect> FromType for ReflectDeserialize { /// let reflect_data = type_registry.get(std::any::TypeId::of::()).unwrap(); /// let reflect_from_ptr = reflect_data.data::().unwrap(); /// // SAFE: `value` is of type `Reflected`, which the `ReflectFromPtr` was created for -/// let value = unsafe { reflect_from_ptr.as_reflect_ptr(value) }; +/// let value = unsafe { reflect_from_ptr.as_reflect(value) }; /// /// assert_eq!(value.downcast_ref::().unwrap().0, "Hello world!"); /// ``` #[derive(Clone)] pub struct ReflectFromPtr { type_id: TypeId, - to_reflect: for<'a> unsafe fn(Ptr<'a>) -> &'a dyn Reflect, - to_reflect_mut: for<'a> unsafe fn(PtrMut<'a>) -> &'a mut dyn Reflect, + from_ptr: unsafe fn(Ptr) -> &dyn Reflect, + from_ptr_mut: unsafe fn(PtrMut) -> &mut dyn Reflect, } impl ReflectFromPtr { - /// Returns the [`TypeId`] that the [`ReflectFromPtr`] was constructed for + /// Returns the [`TypeId`] that the [`ReflectFromPtr`] was constructed for. pub fn type_id(&self) -> TypeId { self.type_id } + /// Convert `Ptr` into `&dyn Reflect`. + /// /// # Safety /// /// `val` must be a pointer to value of the type that the [`ReflectFromPtr`] was constructed for. /// This can be verified by checking that the type id returned by [`ReflectFromPtr::type_id`] is the expected one. - pub unsafe fn as_reflect_ptr<'a>(&self, val: Ptr<'a>) -> &'a dyn Reflect { - (self.to_reflect)(val) + pub unsafe fn as_reflect<'a>(&self, val: Ptr<'a>) -> &'a dyn Reflect { + (self.from_ptr)(val) } + /// Convert `PtrMut` into `&mut dyn Reflect`. + /// /// # Safety /// /// `val` must be a pointer to a value of the type that the [`ReflectFromPtr`] was constructed for /// This can be verified by checking that the type id returned by [`ReflectFromPtr::type_id`] is the expected one. - pub unsafe fn as_reflect_ptr_mut<'a>(&self, val: PtrMut<'a>) -> &'a mut dyn Reflect { - (self.to_reflect_mut)(val) + pub unsafe fn as_reflect_mut<'a>(&self, val: PtrMut<'a>) -> &'a mut dyn Reflect { + (self.from_ptr_mut)(val) + } + /// Get a function pointer to turn a `Ptr` into `&dyn Reflect` for + /// the type this [`ReflectFromPtr`] was constructed for. + /// + /// # Safety + /// + /// When calling the unsafe function returned by this method you must ensure that: + /// - The input `Ptr` points to the `Reflect` type this `ReflectFromPtr` + /// was constructed for. + pub fn from_ptr(&self) -> unsafe fn(Ptr) -> &dyn Reflect { + self.from_ptr + } + /// Get a function pointer to turn a `PtrMut` into `&mut dyn Reflect` for + /// the type this [`ReflectFromPtr`] was constructed for. + /// + /// # Safety + /// + /// When calling the unsafe function returned by this method you must ensure that: + /// - The input `PtrMut` points to the `Reflect` type this `ReflectFromPtr` + /// was constructed for. + pub fn from_ptr_mut(&self) -> unsafe fn(PtrMut) -> &mut dyn Reflect { + self.from_ptr_mut } } @@ -559,14 +585,14 @@ impl FromType for ReflectFromPtr { fn from_type() -> Self { ReflectFromPtr { type_id: std::any::TypeId::of::(), - to_reflect: |ptr| { - // SAFE: only called from `as_reflect`, where the `ptr` is guaranteed to be of type `T`, - // and `as_reflect_ptr`, where the caller promises to call it with type `T` + from_ptr: |ptr| { + // SAFETY: `from_ptr_mut` is either called in `ReflectFromPtr::as_reflect` + // or returned by `ReflectFromPtr::from_ptr`, both lay out the invariants + // required by `deref` unsafe { ptr.deref::() as &dyn Reflect } }, - to_reflect_mut: |ptr| { - // SAFE: only called from `as_reflect_mut`, where the `ptr` is guaranteed to be of type `T`, - // and `as_reflect_ptr_mut`, where the caller promises to call it with type `T` + from_ptr_mut: |ptr| { + // SAFETY: same as above, but foor `as_reflect_mut`, `from_ptr_mut` and `deref_mut`. unsafe { ptr.deref_mut::() as &mut dyn Reflect } }, } @@ -601,7 +627,7 @@ mod test { { let value = PtrMut::from(&mut value); // SAFETY: reflect_from_ptr was constructed for the correct type - let dyn_reflect = unsafe { reflect_from_ptr.as_reflect_ptr_mut(value) }; + let dyn_reflect = unsafe { reflect_from_ptr.as_reflect_mut(value) }; match dyn_reflect.reflect_mut() { bevy_reflect::ReflectMut::Struct(strukt) => { strukt.field_mut("a").unwrap().apply(&2.0f32); @@ -612,7 +638,7 @@ mod test { { // SAFETY: reflect_from_ptr was constructed for the correct type - let dyn_reflect = unsafe { reflect_from_ptr.as_reflect_ptr(Ptr::from(&value)) }; + let dyn_reflect = unsafe { reflect_from_ptr.as_reflect(Ptr::from(&value)) }; match dyn_reflect.reflect_ref() { bevy_reflect::ReflectRef::Struct(strukt) => { let a = strukt.field("a").unwrap().downcast_ref::().unwrap();