Skip to content

Commit

Permalink
bevy_reflect: Relax bounds on Option<T> (#5658)
Browse files Browse the repository at this point in the history
# Objective

The reflection impls on `Option<T>` have the bound `T: Reflect + Clone`. This means that using `FromReflect` requires `Clone` even though we can normally get away with just `FromReflect`.

## Solution

Update the bounds on `Option<T>` to match that of `Vec<T>`, where `T: FromReflect`. 

This helps remove a `Clone` implementation that may be undesired but added for the sole purpose of getting the code to compile.

---

## Changelog

* Reflection on `Option<T>` now has `T` bound by `FromReflect` rather than `Reflect + Clone`
* Added a `FromReflect` impl for `Instant`

## Migration Guide

If using `Option<T>` with Bevy's reflection API, `T` now needs to implement `FromReflect` rather than just `Clone`. This can be achieved easily by simply deriving `FromReflect`:

```rust

// OLD
#[derive(Reflect, Clone)]
struct Foo;

let reflected: Box<dyn Reflect> = Box::new(Some(Foo));

// NEW
#[derive(Reflect, FromReflect)]
struct Foo;

let reflected: Box<dyn Reflect> = Box::new(Some(Foo));
```
> Note: You can still derive `Clone`, but it's not required in order to compile.
  • Loading branch information
MrGVSV committed Aug 17, 2022
1 parent 3221e56 commit aed3232
Show file tree
Hide file tree
Showing 3 changed files with 35 additions and 13 deletions.
39 changes: 30 additions & 9 deletions crates/bevy_reflect/src/impls/std.rs
Expand Up @@ -76,6 +76,7 @@ impl_from_reflect_value!(String);
impl_from_reflect_value!(HashSet<T: Hash + Eq + Clone + Send + Sync + 'static>);
impl_from_reflect_value!(Range<T: Clone + Send + Sync + 'static>);
impl_from_reflect_value!(Duration);
impl_from_reflect_value!(Instant);
impl_from_reflect_value!(NonZeroI128);
impl_from_reflect_value!(NonZeroU128);
impl_from_reflect_value!(NonZeroIsize);
Expand Down Expand Up @@ -585,13 +586,13 @@ impl Reflect for Cow<'static, str> {
}
}

impl<T: Reflect + Clone> GetTypeRegistration for Option<T> {
impl<T: FromReflect> GetTypeRegistration for Option<T> {
fn get_type_registration() -> TypeRegistration {
TypeRegistration::of::<Option<T>>()
}
}

impl<T: Reflect + Clone> Enum for Option<T> {
impl<T: FromReflect> Enum for Option<T> {
fn field(&self, _name: &str) -> Option<&dyn Reflect> {
None
}
Expand Down Expand Up @@ -655,7 +656,7 @@ impl<T: Reflect + Clone> Enum for Option<T> {
}
}

impl<T: Reflect + Clone> Reflect for Option<T> {
impl<T: FromReflect> Reflect for Option<T> {
#[inline]
fn type_name(&self) -> &str {
std::any::type_name::<Self>()
Expand Down Expand Up @@ -741,7 +742,7 @@ impl<T: Reflect + Clone> Reflect for Option<T> {

#[inline]
fn clone_value(&self) -> Box<dyn Reflect> {
Box::new(self.clone())
Box::new(Enum::clone_dynamic(self))
}

fn reflect_hash(&self) -> Option<u64> {
Expand All @@ -753,7 +754,7 @@ impl<T: Reflect + Clone> Reflect for Option<T> {
}
}

impl<T: Reflect + Clone> FromReflect for Option<T> {
impl<T: FromReflect> FromReflect for Option<T> {
fn from_reflect(reflect: &dyn Reflect) -> Option<Self> {
if let ReflectRef::Enum(dyn_enum) = reflect.reflect_ref() {
match dyn_enum.variant_name() {
Expand All @@ -762,7 +763,7 @@ impl<T: Reflect + Clone> FromReflect for Option<T> {
.field_at(0)
.expect("Field at index 0 should exist")
.clone_value();
let field = field.take::<T>().unwrap_or_else(|_| {
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>()
Expand All @@ -783,7 +784,7 @@ impl<T: Reflect + Clone> FromReflect for Option<T> {
}
}

impl<T: Reflect + Clone> Typed for Option<T> {
impl<T: FromReflect> Typed for Option<T> {
fn type_info() -> &'static TypeInfo {
static CELL: GenericTypeInfoCell = GenericTypeInfoCell::new();
CELL.get_or_insert::<Self, _>(|| {
Expand Down Expand Up @@ -827,10 +828,12 @@ impl FromReflect for Cow<'static, str> {

#[cfg(test)]
mod tests {
use crate as bevy_reflect;
use crate::{
Enum, Reflect, ReflectSerialize, TypeInfo, TypeRegistry, Typed, VariantInfo, VariantType,
Enum, FromReflect, Reflect, ReflectSerialize, TypeInfo, TypeRegistry, Typed, VariantInfo,
VariantType,
};
use bevy_utils::HashMap;
use bevy_utils::{HashMap, Instant};
use std::f32::consts::{PI, TAU};

#[test]
Expand Down Expand Up @@ -939,6 +942,17 @@ mod tests {
assert_eq!(Some(321), value);
}

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

let expected = Some(Foo(123));
let output = <Option<Foo> as FromReflect>::from_reflect(&expected).unwrap();

assert_eq!(expected, output);
}

#[test]
fn option_should_impl_typed() {
type MyOption = Option<i32>;
Expand Down Expand Up @@ -979,4 +993,11 @@ mod tests {
let forty_two: std::num::NonZeroUsize = crate::FromReflect::from_reflect(a).unwrap();
assert_eq!(forty_two, std::num::NonZeroUsize::new(42).unwrap());
}

#[test]
fn instant_should_from_reflect() {
let expected = Instant::now();
let output = <Instant as FromReflect>::from_reflect(&expected).unwrap();
assert_eq!(expected, output);
}
}
5 changes: 3 additions & 2 deletions crates/bevy_render/src/camera/camera.rs
Expand Up @@ -19,6 +19,7 @@ use bevy_ecs::{
};
use bevy_math::{Mat4, UVec2, Vec2, Vec3};
use bevy_reflect::prelude::*;
use bevy_reflect::FromReflect;
use bevy_transform::components::GlobalTransform;
use bevy_utils::HashSet;
use bevy_window::{WindowCreated, WindowId, WindowResized, Windows};
Expand All @@ -32,7 +33,7 @@ use wgpu::Extent3d;
/// You can overlay multiple cameras in a single window using viewports to create effects like
/// split screen, minimaps, and character viewers.
// TODO: remove reflect_value when possible
#[derive(Reflect, Debug, Clone, Serialize, Deserialize)]
#[derive(Reflect, FromReflect, Debug, Clone, Serialize, Deserialize)]
#[reflect_value(Default, Serialize, Deserialize)]
pub struct Viewport {
/// The physical position to render this viewport to within the [`RenderTarget`] of this [`Camera`].
Expand Down Expand Up @@ -71,7 +72,7 @@ pub struct ComputedCameraValues {
target_info: Option<RenderTargetInfo>,
}

#[derive(Component, Debug, Reflect, Clone)]
#[derive(Component, Debug, Reflect, FromReflect, Clone)]
#[reflect(Component)]
pub struct Camera {
/// If set, this camera will render to the given [`Viewport`] rectangle within the configured [`RenderTarget`].
Expand Down
4 changes: 2 additions & 2 deletions crates/bevy_time/src/time.rs
@@ -1,9 +1,9 @@
use bevy_ecs::{reflect::ReflectResource, system::Resource};
use bevy_reflect::Reflect;
use bevy_reflect::{FromReflect, Reflect};
use bevy_utils::{Duration, Instant};

/// Tracks elapsed time since the last update and since the App has started
#[derive(Resource, Reflect, Debug, Clone)]
#[derive(Resource, Reflect, FromReflect, Debug, Clone)]
#[reflect(Resource)]
pub struct Time {
delta: Duration,
Expand Down

0 comments on commit aed3232

Please sign in to comment.