Skip to content

Commit

Permalink
bevy_reflect: Fix apply method for Option<T> (bevyengine#5780)
Browse files Browse the repository at this point in the history
# Objective

bevyengine#5658 made it so that `FromReflect` was used as the bound for `T` in `Option<T>`. However, it did not use this change effectively for the implementation of `Reflect::apply` (it was still using `take`, which would fail for Dynamic types).

Additionally, the changes were not consistent with other methods within the file, such as the ones for `Vec<T>` and `HashMap<K, V>`.

## Solution

Update `Option<T>` to fallback on `FromReflect` if `take` fails, instead of wholly relying on one or the other.

I also chose to update the error messages, as they weren't all too descriptive before.

---

## Changelog

- Use `FromReflect::from_reflect` as a fallback in the `Reflect::apply` implementation for `Option<T>`
  • Loading branch information
MrGVSV authored and ItsDoot committed Feb 1, 2023
1 parent 2e0e67b commit 7f10d53
Showing 1 changed file with 69 additions and 18 deletions.
87 changes: 69 additions & 18 deletions crates/bevy_reflect/src/impls/std.rs
Expand Up @@ -696,8 +696,7 @@ impl<T: FromReflect> Reflect for Option<T> {
if self.variant_name() == value.variant_name() {
// Same variant -> just update fields
for (index, field) in value.iter_fields().enumerate() {
let name = value.name_at(index).unwrap();
if let Some(v) = self.field_mut(name) {
if let Some(v) = self.field_at_mut(index) {
v.apply(field.value());
}
}
Expand All @@ -707,14 +706,23 @@ impl<T: FromReflect> Reflect for Option<T> {
"Some" => {
let field = value
.field_at(0)
.expect("Field at index 0 should exist")
.clone_value();
let field = field.take::<T>().unwrap_or_else(|_| {
panic!(
"Field at index 0 should be of type {}",
std::any::type_name::<T>()
)
});
.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(|| {
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" => {
Expand Down Expand Up @@ -761,14 +769,23 @@ impl<T: FromReflect> FromReflect for Option<T> {
"Some" => {
let field = dyn_enum
.field_at(0)
.expect("Field at index 0 should exist")
.clone_value();
let field = T::from_reflect(field.as_ref()).unwrap_or_else(|| {
panic!(
"Field at index 0 should be of type {}",
std::any::type_name::<T>()
)
});
.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(|| {
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),
Expand Down Expand Up @@ -953,6 +970,40 @@ mod tests {
assert_eq!(expected, output);
}

#[test]
fn option_should_apply() {
#[derive(Reflect, FromReflect, PartialEq, Debug)]
struct Foo(usize);

// === None on None === //
let patch = None::<Foo>;
let mut value = None;
Reflect::apply(&mut value, &patch);

assert_eq!(patch, value, "None apply onto None");

// === Some on None === //
let patch = Some(Foo(123));
let mut value = None;
Reflect::apply(&mut value, &patch);

assert_eq!(patch, value, "Some apply onto None");

// === None on Some === //
let patch = None::<Foo>;
let mut value = Some(Foo(321));
Reflect::apply(&mut value, &patch);

assert_eq!(patch, value, "None apply onto Some");

// === Some on Some === //
let patch = Some(Foo(123));
let mut value = Some(Foo(321));
Reflect::apply(&mut value, &patch);

assert_eq!(patch, value, "Some apply onto Some");
}

#[test]
fn option_should_impl_typed() {
type MyOption = Option<i32>;
Expand Down

0 comments on commit 7f10d53

Please sign in to comment.