From a72ee9aa91e8e48b2a7aebc990264afe18922a67 Mon Sep 17 00:00:00 2001 From: Gino Valente Date: Wed, 11 Jan 2023 16:25:37 +0000 Subject: [PATCH] bevy_reflect: Simplify `take`-or-else-`from_reflect` operation (#6566) # Objective There are times where we want to simply take an owned `dyn Reflect` and cast it to a type `T`. Currently, this involves doing: ```rust let value = value.take::().unwrap_or_else(|value| { T::from_reflect(&*value).unwrap_or_else(|| { panic!( "expected value of type {} to convert to type {}.", value.type_name(), std::any::type_name::() ) }) }); ``` This is a common operation that could be easily be simplified. ## Solution Add the `FromReflect::take_from_reflect` method. This first tries to `take` the value, calling `from_reflect` iff that fails. ```rust let value = T::take_from_reflect(value).unwrap_or_else(|value| { panic!( "expected value of type {} to convert to type {}.", value.type_name(), std::any::type_name::() ) }); ``` Based on suggestion from @soqb on [Discord](https://discord.com/channels/691052431525675048/1002362493634629796/1041046880316043374). --- ## Changelog - Add `FromReflect::take_from_reflect` method --- crates/bevy_reflect/src/from_reflect.rs | 20 +++++ crates/bevy_reflect/src/impls/std.rs | 98 ++++++++++++------------- 2 files changed, 65 insertions(+), 53 deletions(-) diff --git a/crates/bevy_reflect/src/from_reflect.rs b/crates/bevy_reflect/src/from_reflect.rs index e9df08aefc459..41c4c3582bd3a 100644 --- a/crates/bevy_reflect/src/from_reflect.rs +++ b/crates/bevy_reflect/src/from_reflect.rs @@ -13,6 +13,26 @@ use crate::{FromType, Reflect}; pub trait FromReflect: Reflect + Sized { /// Constructs a concrete instance of `Self` from a reflected value. fn from_reflect(reflect: &dyn Reflect) -> Option; + + /// Attempts to downcast the given value to `Self` using, + /// constructing the value using [`from_reflect`] if that fails. + /// + /// This method is more efficient than using [`from_reflect`] for cases where + /// the given value is likely a boxed instance of `Self` (i.e. `Box`) + /// rather than a boxed dynamic type (e.g. [`DynamicStruct`], [`DynamicList`], etc.). + /// + /// [`from_reflect`]: Self::from_reflect + /// [`DynamicStruct`]: crate::DynamicStruct + /// [`DynamicList`]: crate::DynamicList + fn take_from_reflect(reflect: Box) -> Result> { + match reflect.take::() { + Ok(value) => Ok(value), + Err(value) => match Self::from_reflect(value.as_ref()) { + None => Err(value), + Some(value) => Ok(value), + }, + } + } } /// Type data that represents the [`FromReflect`] trait and allows it to be used dynamically. diff --git a/crates/bevy_reflect/src/impls/std.rs b/crates/bevy_reflect/src/impls/std.rs index e80859ddbd286..9b7bbf8ff77fa 100644 --- a/crates/bevy_reflect/src/impls/std.rs +++ b/crates/bevy_reflect/src/impls/std.rs @@ -230,13 +230,11 @@ macro_rules! impl_reflect_for_veclike { } fn push(&mut self, value: Box) { - let value = value.take::().unwrap_or_else(|value| { - T::from_reflect(&*value).unwrap_or_else(|| { - panic!( - "Attempted to push invalid value of type {}.", - value.type_name() - ) - }) + let value = T::take_from_reflect(value).unwrap_or_else(|value| { + panic!( + "Attempted to push invalid value of type {}.", + value.type_name() + ) }); $push(self, value); } @@ -409,21 +407,17 @@ impl Map for HashMap { 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 key = K::take_from_reflect(key).unwrap_or_else(|key| { + 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 insert invalid value of type {}.", - value.type_name() - ) - }) + let value = V::take_from_reflect(value).unwrap_or_else(|value| { + panic!( + "Attempted to insert invalid value of type {}.", + value.type_name() + ) }); self.insert(key, value) .map(|old_value| Box::new(old_value) as Box) @@ -829,25 +823,24 @@ impl Reflect for Option { // New variant -> perform a switch match value.variant_name() { "Some" => { - let field = value - .field_at(0) - .unwrap_or_else(|| { - panic!( - "Field in `Some` variant of {} should exist", - std::any::type_name::>() - ) - }) - .clone_value() - .take::() - .unwrap_or_else(|value| { - T::from_reflect(&*value).unwrap_or_else(|| { + let field = T::take_from_reflect( + value + .field_at(0) + .unwrap_or_else(|| { panic!( - "Field in `Some` variant of {} should be of type {}", - std::any::type_name::>(), - std::any::type_name::() + "Field in `Some` variant of {} should exist", + std::any::type_name::>() ) }) - }); + .clone_value(), + ) + .unwrap_or_else(|_| { + panic!( + "Field in `Some` variant of {} should be of type {}", + std::any::type_name::>(), + std::any::type_name::() + ) + }); *self = Some(field); } "None" => { @@ -896,25 +889,24 @@ impl FromReflect for Option { if let ReflectRef::Enum(dyn_enum) = reflect.reflect_ref() { match dyn_enum.variant_name() { "Some" => { - let field = dyn_enum - .field_at(0) - .unwrap_or_else(|| { - panic!( - "Field in `Some` variant of {} should exist", - std::any::type_name::>() - ) - }) - .clone_value() - .take::() - .unwrap_or_else(|value| { - T::from_reflect(&*value).unwrap_or_else(|| { + let field = T::take_from_reflect( + dyn_enum + .field_at(0) + .unwrap_or_else(|| { panic!( - "Field in `Some` variant of {} should be of type {}", - std::any::type_name::>(), - std::any::type_name::() + "Field in `Some` variant of {} should exist", + std::any::type_name::>() ) }) - }); + .clone_value(), + ) + .unwrap_or_else(|_| { + panic!( + "Field in `Some` variant of {} should be of type {}", + std::any::type_name::>(), + std::any::type_name::() + ) + }); Some(Some(field)) } "None" => Some(None),