Skip to content

Commit

Permalink
bevy_reflect: FromReflect Ergonomics Implementation (#6056)
Browse files Browse the repository at this point in the history
# Objective

**This implementation is based on
bevyengine/rfcs#59

---

Resolves #4597

Full details and motivation can be found in the RFC, but here's a brief
summary.

`FromReflect` is a very powerful and important trait within the
reflection API. It allows Dynamic types (e.g., `DynamicList`, etc.) to
be formed into Real ones (e.g., `Vec<i32>`, etc.).

This mainly comes into play concerning deserialization, where the
reflection deserializers both return a `Box<dyn Reflect>` that almost
always contain one of these Dynamic representations of a Real type. To
convert this to our Real type, we need to use `FromReflect`.

It also sneaks up in other ways. For example, it's a required bound for
`T` in `Vec<T>` so that `Vec<T>` as a whole can be made `FromReflect`.
It's also required by all fields of an enum as it's used as part of the
`Reflect::apply` implementation.

So in other words, much like `GetTypeRegistration` and `Typed`, it is
very much a core reflection trait.

The problem is that it is not currently treated like a core trait and is
not automatically derived alongside `Reflect`. This makes using it a bit
cumbersome and easy to forget.

## Solution

Automatically derive `FromReflect` when deriving `Reflect`.

Users can then choose to opt-out if needed using the
`#[reflect(from_reflect = false)]` attribute.

```rust
#[derive(Reflect)]
struct Foo;

#[derive(Reflect)]
#[reflect(from_reflect = false)]
struct Bar;

fn test<T: FromReflect>(value: T) {}

test(Foo); // <-- OK
test(Bar); // <-- Panic! Bar does not implement trait `FromReflect`
```

#### `ReflectFromReflect`

This PR also automatically adds the `ReflectFromReflect` (introduced in
#6245) registration to the derived `GetTypeRegistration` impl— if the
type hasn't opted out of `FromReflect` of course.

<details>
<summary><h4>Improved Deserialization</h4></summary>

> **Warning**
> This section includes changes that have since been descoped from this
PR. They will likely be implemented again in a followup PR. I am mainly
leaving these details in for archival purposes, as well as for reference
when implementing this logic again.

And since we can do all the above, we might as well improve
deserialization. We can now choose to deserialize into a Dynamic type or
automatically convert it using `FromReflect` under the hood.

`[Un]TypedReflectDeserializer::new` will now perform the conversion and
return the `Box`'d Real type.

`[Un]TypedReflectDeserializer::new_dynamic` will work like what we have
now and simply return the `Box`'d Dynamic type.

```rust
// Returns the Real type
let reflect_deserializer = UntypedReflectDeserializer::new(&registry);
let mut deserializer = ron::de::Deserializer::from_str(input)?;

let output: SomeStruct = reflect_deserializer.deserialize(&mut deserializer)?.take()?;

// Returns the Dynamic type
let reflect_deserializer = UntypedReflectDeserializer::new_dynamic(&registry);
let mut deserializer = ron::de::Deserializer::from_str(input)?;

let output: DynamicStruct = reflect_deserializer.deserialize(&mut deserializer)?.take()?;
```

</details>

---

## Changelog

* `FromReflect` is now automatically derived within the `Reflect` derive
macro
* This includes auto-registering `ReflectFromReflect` in the derived
`GetTypeRegistration` impl
* ~~Renamed `TypedReflectDeserializer::new` and
`UntypedReflectDeserializer::new` to
`TypedReflectDeserializer::new_dynamic` and
`UntypedReflectDeserializer::new_dynamic`, respectively~~ **Descoped**
* ~~Changed `TypedReflectDeserializer::new` and
`UntypedReflectDeserializer::new` to automatically convert the
deserialized output using `FromReflect`~~ **Descoped**

## Migration Guide

* `FromReflect` is now automatically derived within the `Reflect` derive
macro. Items with both derives will need to remove the `FromReflect`
one.

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

If using a manual implementation of `FromReflect` and the `Reflect`
derive, users will need to opt-out of the automatic implementation.

  ```rust
  // OLD
  #[derive(Reflect)]
  struct Foo;
  
  impl FromReflect for Foo {/* ... */}
  
  // NEW
  #[derive(Reflect)]
  #[reflect(from_reflect = false)]
  struct Foo;
  
  impl FromReflect for Foo {/* ... */}
  ```

<details>
<summary><h4>Removed Migrations</h4></summary>

> **Warning**
> This section includes changes that have since been descoped from this
PR. They will likely be implemented again in a followup PR. I am mainly
leaving these details in for archival purposes, as well as for reference
when implementing this logic again.

* The reflect deserializers now perform a `FromReflect` conversion
internally. The expected output of `TypedReflectDeserializer::new` and
`UntypedReflectDeserializer::new` is no longer a Dynamic (e.g.,
`DynamicList`), but its Real counterpart (e.g., `Vec<i32>`).

  ```rust
let reflect_deserializer =
UntypedReflectDeserializer::new_dynamic(&registry);
  let mut deserializer = ron::de::Deserializer::from_str(input)?;
  
  // OLD
let output: DynamicStruct = reflect_deserializer.deserialize(&mut
deserializer)?.take()?;
  
  // NEW
let output: SomeStruct = reflect_deserializer.deserialize(&mut
deserializer)?.take()?;
  ```

Alternatively, if this behavior isn't desired, use the
`TypedReflectDeserializer::new_dynamic` and
`UntypedReflectDeserializer::new_dynamic` methods instead:

  ```rust
  // OLD
  let reflect_deserializer = UntypedReflectDeserializer::new(&registry);
  
  // NEW
let reflect_deserializer =
UntypedReflectDeserializer::new_dynamic(&registry);
  ```

</details>

---------

Co-authored-by: Carter Anderson <mcanders1@gmail.com>
  • Loading branch information
MrGVSV and cart authored Jun 29, 2023
1 parent a6fffc5 commit ba72a33
Show file tree
Hide file tree
Showing 94 changed files with 922 additions and 779 deletions.
10 changes: 5 additions & 5 deletions crates/bevy_animation/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@ use bevy_core::Name;
use bevy_ecs::prelude::*;
use bevy_hierarchy::{Children, Parent};
use bevy_math::{Quat, Vec3};
use bevy_reflect::{FromReflect, Reflect, TypeUuid};
use bevy_reflect::{Reflect, TypeUuid};
use bevy_render::mesh::morph::MorphWeights;
use bevy_time::Time;
use bevy_transform::{prelude::Transform, TransformSystem};
Expand All @@ -27,7 +27,7 @@ pub mod prelude {
}

/// List of keyframes for one of the attribute of a [`Transform`].
#[derive(Reflect, FromReflect, Clone, Debug)]
#[derive(Reflect, Clone, Debug)]
pub enum Keyframes {
/// Keyframes for rotation.
Rotation(Vec<Quat>),
Expand All @@ -49,7 +49,7 @@ pub enum Keyframes {
/// Describes how an attribute of a [`Transform`] or [`MorphWeights`] should be animated.
///
/// `keyframe_timestamps` and `keyframes` should have the same length.
#[derive(Reflect, FromReflect, Clone, Debug)]
#[derive(Reflect, Clone, Debug)]
pub struct VariableCurve {
/// Timestamp for each of the keyframes.
pub keyframe_timestamps: Vec<f32>,
Expand All @@ -58,14 +58,14 @@ pub struct VariableCurve {
}

/// Path to an entity, with [`Name`]s. Each entity in a path must have a name.
#[derive(Reflect, FromReflect, Clone, Debug, Hash, PartialEq, Eq, Default)]
#[derive(Reflect, Clone, Debug, Hash, PartialEq, Eq, Default)]
pub struct EntityPath {
/// Parts of the path
pub parts: Vec<Name>,
}

/// A list of [`VariableCurve`], and the [`EntityPath`] to which they apply.
#[derive(Reflect, FromReflect, Clone, TypeUuid, Debug, Default)]
#[derive(Reflect, Clone, TypeUuid, Debug, Default)]
#[uuid = "d81b7179-0448-4eb0-89fe-c067222725bf"]
pub struct AnimationClip {
curves: Vec<Vec<VariableCurve>>,
Expand Down
24 changes: 6 additions & 18 deletions crates/bevy_asset/src/handle.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,28 +10,14 @@ use crate::{
Asset, Assets,
};
use bevy_ecs::{component::Component, reflect::ReflectComponent};
use bevy_reflect::{
std_traits::ReflectDefault, FromReflect, Reflect, ReflectDeserialize, ReflectFromReflect,
ReflectSerialize,
};
use bevy_reflect::{std_traits::ReflectDefault, Reflect, ReflectDeserialize, ReflectSerialize};
use bevy_utils::Uuid;
use crossbeam_channel::{Receiver, Sender};
use serde::{Deserialize, Serialize};

/// A unique, stable asset id.
#[derive(
Debug,
Clone,
Copy,
Eq,
PartialEq,
Hash,
Ord,
PartialOrd,
Serialize,
Deserialize,
Reflect,
FromReflect,
Debug, Clone, Copy, Eq, PartialEq, Hash, Ord, PartialOrd, Serialize, Deserialize, Reflect,
)]
#[reflect_value(Serialize, Deserialize, PartialEq, Hash)]
pub enum HandleId {
Expand Down Expand Up @@ -103,8 +89,8 @@ impl HandleId {
/// handle to the unloaded asset, but it will not be able to retrieve the image data, resulting in
/// collisions no longer being detected for that entity.
///
#[derive(Component, Reflect, FromReflect)]
#[reflect(Component, Default, FromReflect)]
#[derive(Component, Reflect)]
#[reflect(Component, Default)]
pub struct Handle<T>
where
T: Asset,
Expand All @@ -117,7 +103,9 @@ where
marker: PhantomData<fn() -> T>,
}

#[derive(Default)]
enum HandleType {
#[default]
Weak,
Strong(Sender<RefChange>),
}
Expand Down
38 changes: 7 additions & 31 deletions crates/bevy_asset/src/path.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,4 @@
use bevy_reflect::{
FromReflect, Reflect, ReflectDeserialize, ReflectFromReflect, ReflectSerialize,
};
use bevy_reflect::{Reflect, ReflectDeserialize, ReflectSerialize};
use bevy_utils::{AHasher, RandomState};
use serde::{Deserialize, Serialize};
use std::{
Expand All @@ -10,8 +8,8 @@ use std::{
};

/// Represents a path to an asset in the file system.
#[derive(Debug, Eq, PartialEq, Hash, Clone, Serialize, Deserialize, Reflect, FromReflect)]
#[reflect(Debug, PartialEq, Hash, Serialize, Deserialize, FromReflect)]
#[derive(Debug, Eq, PartialEq, Hash, Clone, Serialize, Deserialize, Reflect)]
#[reflect(Debug, PartialEq, Hash, Serialize, Deserialize)]
pub struct AssetPath<'a> {
path: Cow<'a, Path>,
label: Option<Cow<'a, str>>,
Expand Down Expand Up @@ -69,38 +67,16 @@ impl<'a> AssetPath<'a> {

/// An unique identifier to an asset path.
#[derive(
Debug,
Clone,
Copy,
Eq,
PartialEq,
Hash,
Ord,
PartialOrd,
Serialize,
Deserialize,
Reflect,
FromReflect,
Debug, Clone, Copy, Eq, PartialEq, Hash, Ord, PartialOrd, Serialize, Deserialize, Reflect,
)]
#[reflect_value(PartialEq, Hash, Serialize, Deserialize, FromReflect)]
#[reflect_value(PartialEq, Hash, Serialize, Deserialize)]
pub struct AssetPathId(SourcePathId, LabelId);

/// An unique identifier to the source path of an asset.
#[derive(
Debug,
Clone,
Copy,
Eq,
PartialEq,
Hash,
Ord,
PartialOrd,
Serialize,
Deserialize,
Reflect,
FromReflect,
Debug, Clone, Copy, Eq, PartialEq, Hash, Ord, PartialOrd, Serialize, Deserialize, Reflect,
)]
#[reflect_value(PartialEq, Hash, Serialize, Deserialize, FromReflect)]
#[reflect_value(PartialEq, Hash, Serialize, Deserialize)]
pub struct SourcePathId(u64);

/// An unique identifier to a sub-asset label.
Expand Down
4 changes: 2 additions & 2 deletions crates/bevy_asset/src/reflect.rs
Original file line number Diff line number Diff line change
Expand Up @@ -255,11 +255,11 @@ mod tests {

use bevy_app::App;
use bevy_ecs::reflect::AppTypeRegistry;
use bevy_reflect::{FromReflect, Reflect, ReflectMut, TypeUuid};
use bevy_reflect::{Reflect, ReflectMut, TypeUuid};

use crate::{AddAsset, AssetPlugin, HandleUntyped, ReflectAsset};

#[derive(Reflect, FromReflect, TypeUuid)]
#[derive(Reflect, TypeUuid)]
#[uuid = "09191350-1238-4736-9a89-46f04bda6966"]
struct AssetType {
field: String,
Expand Down
8 changes: 4 additions & 4 deletions crates/bevy_core/src/name.rs
Original file line number Diff line number Diff line change
@@ -1,8 +1,8 @@
use bevy_ecs::{
component::Component, entity::Entity, query::WorldQuery, reflect::ReflectComponent,
};
use bevy_reflect::{std_traits::ReflectDefault, FromReflect};
use bevy_reflect::{Reflect, ReflectFromReflect};
use bevy_reflect::std_traits::ReflectDefault;
use bevy_reflect::Reflect;
use bevy_utils::AHasher;
use std::{
borrow::Cow,
Expand All @@ -17,8 +17,8 @@ use std::{
/// [`Name`] should not be treated as a globally unique identifier for entities,
/// as multiple entities can have the same name. [`bevy_ecs::entity::Entity`] should be
/// used instead as the default unique identifier.
#[derive(Reflect, FromReflect, Component, Clone)]
#[reflect(Component, Default, Debug, FromReflect)]
#[derive(Reflect, Component, Clone)]
#[reflect(Component, Default, Debug)]
pub struct Name {
hash: u64, // TODO: Shouldn't be serialized
name: Cow<'static, str>,
Expand Down
5 changes: 2 additions & 3 deletions crates/bevy_core_pipeline/src/bloom/settings.rs
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
use super::downsampling_pipeline::BloomUniforms;
use bevy_ecs::{prelude::Component, query::QueryItem, reflect::ReflectComponent};
use bevy_math::{UVec4, Vec4};
use bevy_reflect::{std_traits::ReflectDefault, FromReflect, Reflect, ReflectFromReflect};
use bevy_reflect::{std_traits::ReflectDefault, Reflect};
use bevy_render::{extract_component::ExtractComponent, prelude::Camera};

/// Applies a bloom effect to an HDR-enabled 2d or 3d camera.
Expand Down Expand Up @@ -160,8 +160,7 @@ impl Default for BloomSettings {
/// * Changing these settings creates a physically inaccurate image
/// * Changing these settings makes it easy to make the final result look worse
/// * Non-default prefilter settings should be used in conjuction with [`BloomCompositeMode::Additive`]
#[derive(Default, Clone, Reflect, FromReflect)]
#[reflect(FromReflect)]
#[derive(Default, Clone, Reflect)]
pub struct BloomPrefilterSettings {
/// Baseline of the quadratic threshold curve (default: 0.0).
///
Expand Down
12 changes: 5 additions & 7 deletions crates/bevy_core_pipeline/src/clear_color.rs
Original file line number Diff line number Diff line change
@@ -1,13 +1,11 @@
use bevy_derive::{Deref, DerefMut};
use bevy_ecs::prelude::*;
use bevy_reflect::{
FromReflect, Reflect, ReflectDeserialize, ReflectFromReflect, ReflectSerialize,
};
use bevy_reflect::{Reflect, ReflectDeserialize, ReflectSerialize};
use bevy_render::{color::Color, extract_resource::ExtractResource};
use serde::{Deserialize, Serialize};

#[derive(Reflect, FromReflect, Serialize, Deserialize, Clone, Debug, Default)]
#[reflect(Serialize, Deserialize, FromReflect)]
#[derive(Reflect, Serialize, Deserialize, Clone, Debug, Default)]
#[reflect(Serialize, Deserialize)]
pub enum ClearColorConfig {
#[default]
Default,
Expand All @@ -19,8 +17,8 @@ pub enum ClearColorConfig {
///
/// This color appears as the "background" color for simple apps,
/// when there are portions of the screen with nothing rendered.
#[derive(Resource, Clone, Debug, Deref, DerefMut, ExtractResource, Reflect, FromReflect)]
#[reflect(Resource, FromReflect)]
#[derive(Resource, Clone, Debug, Deref, DerefMut, ExtractResource, Reflect)]
#[reflect(Resource)]
pub struct ClearColor(pub Color);

impl Default for ClearColor {
Expand Down
6 changes: 3 additions & 3 deletions crates/bevy_core_pipeline/src/core_2d/camera_2d.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ use crate::{
tonemapping::{DebandDither, Tonemapping},
};
use bevy_ecs::prelude::*;
use bevy_reflect::{FromReflect, Reflect, ReflectFromReflect};
use bevy_reflect::Reflect;
use bevy_render::{
camera::{Camera, CameraProjection, CameraRenderGraph, OrthographicProjection},
extract_component::ExtractComponent,
Expand All @@ -12,9 +12,9 @@ use bevy_render::{
};
use bevy_transform::prelude::{GlobalTransform, Transform};

#[derive(Component, Default, Reflect, FromReflect, Clone, ExtractComponent)]
#[derive(Component, Default, Reflect, Clone, ExtractComponent)]
#[extract_component_filter(With<Camera>)]
#[reflect(Component, FromReflect)]
#[reflect(Component)]
pub struct Camera2d {
pub clear_color: ClearColorConfig,
}
Expand Down
15 changes: 6 additions & 9 deletions crates/bevy_core_pipeline/src/core_3d/camera_3d.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,7 @@ use crate::{
tonemapping::{DebandDither, Tonemapping},
};
use bevy_ecs::prelude::*;
use bevy_reflect::{
FromReflect, Reflect, ReflectDeserialize, ReflectFromReflect, ReflectSerialize,
};
use bevy_reflect::{Reflect, ReflectDeserialize, ReflectSerialize};
use bevy_render::{
camera::{Camera, CameraRenderGraph, Projection},
extract_component::ExtractComponent,
Expand All @@ -17,9 +15,9 @@ use bevy_transform::prelude::{GlobalTransform, Transform};
use serde::{Deserialize, Serialize};

/// Configuration for the "main 3d render graph".
#[derive(Component, Reflect, FromReflect, Clone, ExtractComponent)]
#[derive(Component, Reflect, Clone, ExtractComponent)]
#[extract_component_filter(With<Camera>)]
#[reflect(Component, FromReflect)]
#[reflect(Component)]
pub struct Camera3d {
/// The clear color operation to perform for the main 3d pass.
pub clear_color: ClearColorConfig,
Expand All @@ -39,8 +37,7 @@ impl Default for Camera3d {
}
}

#[derive(Clone, Copy, Reflect, FromReflect)]
#[reflect(FromReflect)]
#[derive(Clone, Copy, Reflect)]
pub struct Camera3dDepthTextureUsage(u32);

impl From<TextureUsages> for Camera3dDepthTextureUsage {
Expand All @@ -55,8 +52,8 @@ impl From<Camera3dDepthTextureUsage> for TextureUsages {
}

/// The depth clear operation to perform for the main 3d pass.
#[derive(Reflect, FromReflect, Serialize, Deserialize, Clone, Debug)]
#[reflect(Serialize, Deserialize, FromReflect)]
#[derive(Reflect, Serialize, Deserialize, Clone, Debug)]
#[reflect(Serialize, Deserialize)]
pub enum Camera3dDepthLoadOp {
/// Clear with a specified value.
/// Note that 0.0 is the far plane due to bevy's use of reverse-z projections.
Expand Down
12 changes: 5 additions & 7 deletions crates/bevy_core_pipeline/src/fxaa/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,7 @@ use bevy_app::prelude::*;
use bevy_asset::{load_internal_asset, HandleUntyped};
use bevy_derive::Deref;
use bevy_ecs::prelude::*;
use bevy_reflect::{
std_traits::ReflectDefault, FromReflect, Reflect, ReflectFromReflect, TypeUuid,
};
use bevy_reflect::{std_traits::ReflectDefault, Reflect, TypeUuid};
use bevy_render::{
extract_component::{ExtractComponent, ExtractComponentPlugin},
prelude::Camera,
Expand All @@ -26,8 +24,8 @@ mod node;

pub use node::FxaaNode;

#[derive(Reflect, FromReflect, Eq, PartialEq, Hash, Clone, Copy)]
#[reflect(FromReflect, PartialEq, Hash)]
#[derive(Reflect, Eq, PartialEq, Hash, Clone, Copy)]
#[reflect(PartialEq, Hash)]
pub enum Sensitivity {
Low,
Medium,
Expand All @@ -48,8 +46,8 @@ impl Sensitivity {
}
}

#[derive(Reflect, FromReflect, Component, Clone, ExtractComponent)]
#[reflect(Component, FromReflect, Default)]
#[derive(Reflect, Component, Clone, ExtractComponent)]
#[reflect(Component, Default)]
#[extract_component_filter(With<Camera>)]
pub struct Fxaa {
/// Enable render passes for FXAA.
Expand Down
11 changes: 4 additions & 7 deletions crates/bevy_core_pipeline/src/prepass/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ pub mod node;
use std::cmp::Reverse;

use bevy_ecs::prelude::*;
use bevy_reflect::{FromReflect, Reflect, ReflectFromReflect};
use bevy_reflect::Reflect;
use bevy_render::{
render_phase::{CachedRenderPipelinePhaseItem, DrawFunctionId, PhaseItem},
render_resource::{CachedRenderPipelineId, Extent3d, TextureFormat},
Expand All @@ -43,19 +43,16 @@ pub const NORMAL_PREPASS_FORMAT: TextureFormat = TextureFormat::Rgb10a2Unorm;
pub const MOTION_VECTOR_PREPASS_FORMAT: TextureFormat = TextureFormat::Rg16Float;

/// If added to a [`crate::prelude::Camera3d`] then depth values will be copied to a separate texture available to the main pass.
#[derive(Component, Default, Reflect, FromReflect)]
#[reflect(FromReflect)]
#[derive(Component, Default, Reflect)]
pub struct DepthPrepass;

/// If added to a [`crate::prelude::Camera3d`] then vertex world normals will be copied to a separate texture available to the main pass.
/// Normals will have normal map textures already applied.
#[derive(Component, Default, Reflect, FromReflect)]
#[reflect(FromReflect)]
#[derive(Component, Default, Reflect)]
pub struct NormalPrepass;

/// If added to a [`crate::prelude::Camera3d`] then screen space motion vectors will be copied to a separate texture available to the main pass.
#[derive(Component, Default, Reflect, FromReflect)]
#[reflect(FromReflect)]
#[derive(Component, Default, Reflect)]
pub struct MotionVectorPrepass;

/// Textures that are written to by the prepass.
Expand Down
Loading

0 comments on commit ba72a33

Please sign in to comment.