Skip to content

Commit

Permalink
bevy_reflect: Simplify take-or-else-from_reflect operation (bevye…
Browse files Browse the repository at this point in the history
…ngine#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::<T>().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::<T>()
    )
  })
});
```

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::<T>()
  )
});
```

Based on suggestion from @soqb on [Discord](https://discord.com/channels/691052431525675048/1002362493634629796/1041046880316043374).

---

## Changelog

- Add `FromReflect::take_from_reflect` method
MrGVSV authored and alradish committed Jan 22, 2023
1 parent 041823e commit a72ee9a
Showing 2 changed files with 65 additions and 53 deletions.
20 changes: 20 additions & 0 deletions crates/bevy_reflect/src/from_reflect.rs
Original file line number Diff line number Diff line change
@@ -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<Self>;

/// 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<Self>`)
/// 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<dyn Reflect>) -> Result<Self, Box<dyn Reflect>> {
match reflect.take::<Self>() {
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.
98 changes: 45 additions & 53 deletions crates/bevy_reflect/src/impls/std.rs
Original file line number Diff line number Diff line change
@@ -230,13 +230,11 @@ macro_rules! impl_reflect_for_veclike {
}

fn push(&mut self, value: Box<dyn Reflect>) {
let value = value.take::<T>().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<K: FromReflect + Eq + Hash, V: FromReflect> Map for HashMap<K, V> {
key: Box<dyn Reflect>,
value: Box<dyn Reflect>,
) -> Option<Box<dyn Reflect>> {
let key = key.take::<K>().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::<V>().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<dyn Reflect>)
@@ -829,25 +823,24 @@ impl<T: FromReflect> Reflect for Option<T> {
// 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::<Option<T>>()
)
})
.clone_value()
.take::<T>()
.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::<Option<T>>(),
std::any::type_name::<T>()
"Field in `Some` variant of {} should exist",
std::any::type_name::<Option<T>>()
)
})
});
.clone_value(),
)
.unwrap_or_else(|_| {
panic!(
"Field in `Some` variant of {} should be of type {}",
std::any::type_name::<Option<T>>(),
std::any::type_name::<T>()
)
});
*self = Some(field);
}
"None" => {
@@ -896,25 +889,24 @@ impl<T: FromReflect> FromReflect for Option<T> {
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::<Option<T>>()
)
})
.clone_value()
.take::<T>()
.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::<Option<T>>(),
std::any::type_name::<T>()
"Field in `Some` variant of {} should exist",
std::any::type_name::<Option<T>>()
)
})
});
.clone_value(),
)
.unwrap_or_else(|_| {
panic!(
"Field in `Some` variant of {} should be of type {}",
std::any::type_name::<Option<T>>(),
std::any::type_name::<T>()
)
});
Some(Some(field))
}
"None" => Some(None),

0 comments on commit a72ee9a

Please sign in to comment.