bevy_reflect: `FromReflect` Ergonomics Implementation #6056

MrGVSV · 2022-09-22T06:15:39Z

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.

#[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.

Improved Deserialization

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.

// 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()?;

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.

// 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.

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

impl FromReflect for Foo {/* ... */}

// NEW
#[derive(Reflect)]
#[reflect(from_reflect = false)]
struct Foo;

impl FromReflect for Foo {/* ... */}

Removed Migrations

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>).

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:

// OLD
let reflect_deserializer = UntypedReflectDeserializer::new(&registry);

// NEW
let reflect_deserializer = UntypedReflectDeserializer::new_dynamic(&registry);

jakobhellermann

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

Wouldn't it make more sense to have this as #[reflect(auto_derive_from_reflect = false)]? Because whether FromReflect is emitted is not a property of the FromReflect derive, but the Reflect one.

MrGVSV · 2022-10-03T15:07:57Z

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

Wouldn't it make more sense to have this as #[reflect(auto_derive_from_reflect = false)]? Because whether FromReflect is emitted is not a property of the FromReflect derive, but the Reflect one.

That's a good point. The only thing is it's a bit long haha. Maybe #[reflect(from_reflect = false)] or #[reflect(derive_from_reflect = false)]?

tguichaoua · 2022-10-03T15:55:36Z

That's a good point. The only thing is it's a bit long haha. Maybe #[reflect(from_reflect = false)] or #[reflect(derive_from_reflect = false)]?

The shorter the better.
IMHO, #[reflect(from_reflect = false)] is clear enough if we consider the whole code (with the derive attribute just above) and if this is mentioned in the doc of Reflect and FromReflect.

soqb · 2022-10-12T07:18:53Z

what about #[reflect(not(FromReflect))] (like cfg attribute) or (my favourite) #[reflect(!FromReflect)] (like negative impls)

Zeenobit · 2022-12-11T04:35:45Z

I've been looking at this PR and #6245 and been reading the Related RFC and I'm not a fan of automatically deriving FromReflect. While I agree with you that it's very useful, I specifically don't agree with this stance:

Chances are, if a type implements Reflect, it should also be implementing FromReflect.

From experience, the only time FromReflect is required is when the user is dealing with nested or generic types. Both of these cases are not (hopefully should not) be common in higher level game logic. Rather, it should be reserved for mid/low-level game systems, and in those cases, using #[derive(FromReflect)] helps readability by declaring intent.

Deriving this automatically would add complexity where it's not needed, in my opinion, especially given the unconventional nature of "opting out" of a derive attribute.

MrGVSV · 2022-12-11T05:41:53Z

I've been looking at this PR and #6245 and been reading the Related RFC and I'm not a fan of automatically deriving FromReflect. While I agree with you that it's very useful, I specifically don't agree with this stance:

Chances are, if a type implements Reflect, it should also be implementing FromReflect.

From experience, the only time FromReflect is required is when the user is dealing with nested or generic types. Both of these cases are not (hopefully should not) be common in higher level game logic. Rather, it should be reserved for mid/low-level game systems, and in those cases, using #[derive(FromReflect)] helps readability by declaring intent.

Deriving this automatically would add complexity where it's not needed, in my opinion, especially given the unconventional nature of "opting out" of a derive attribute.

Two of the main pain points here are deserialization and cloning. Both of these almost always generate Dynamic types, which means we lose the ability to use that value as the concrete type. This means we're unable to use these values with reflected traits (made with #[reflect_trait]), and certain other type data. We can't take it's value as the concrete type. We also can't put non-FromReflect types in Vec<T>, HashMap<K, V>, or any enum and expect Reflect::apply to work like it does for other types.

While I do think the stance is strong, I think there are a lot of good reasons to make this behavior the default rather than an easily-forgotten opt-in thing (especially considering this mainly affects people really diving into reflection rather than the average third-party crate developer).

Obviously I'm open to hearing dissenting opinions haha, but I really do think we need to figure out a good solution for FromReflect overall.

Zeenobit · 2022-12-11T05:58:28Z

Have you considered gating this behind a feature? i.e. only auto derive FromReflect if feature = "xyz".

I fully understand the rationale behind this CL. I think it makes perfect sense for more generic game systems that do a lot of exactly what you're describing. I'm just skeptical about it being turned on by default for everything unless specified otherwise.

Gating it behind a feature might allow us to opt-in/out of this at a crate level. This would eliminate the need for #[reflect(from_reflect = false)] as well.

MrGVSV · 2022-12-11T06:26:28Z

Have you considered gating this behind a feature? i.e. only auto derive FromReflect if feature = "xyz".

Hm, that might be a fair compromise 🤔

I mean, this PR doesn't enforce that FromReflect exists on everything anyway, so we could potentially put it behind a feature without breaking any "guarantees". So would you be opposed to making this a default feature?

Again, I think this is an ergonomics benefit for users and crate-authors who don't know or care about reflection— especially if some of their dependencies really do care and rely on FromReflect heavily behind the scenes.

This would eliminate the need for #[reflect(from_reflect = false)] as well.

I don't think it would since you may want to have everything derive FromReflect automatically except for a handful of types (e.g. ones that are unable to derive FromReflect).

Zeenobit · 2022-12-11T06:42:43Z

So in my opinion it wouldn't be a default feature and the user would have to opt-in at the crate level. However, to counter my own argument, there are 2 issues here:

I do agree with you that we might still need #[reflect(from_reflect = false)], for the exceptional cases where the feature is enabled on the crate, but user still wants to override the auto-derive, or opt-out. So then if we still need to implement this, then we're still adding the complexity, and the crate feature support would be even more complexity in addition to that.
I'm having a hard time thinking of the cost of deriving FromReflect for all reflected types to the user. If it's there, and the user isn't using it, is it really hurting anyone?

So yah, thinking this over again, your approach is probably the ideal approach.

# Objective Resolves #4597 (based on the work from #6056 and a refresh of #4147) When using reflection, we may often end up in a scenario where we have a Dynamic representing a certain type. Unfortunately, we can't just call `MyType::from_reflect` as we do not have knowledge of the concrete type (`MyType`) at runtime. Such scenarios happen when we call `Reflect::clone_value`, use the reflection deserializers, or create the Dynamic type ourselves. ## Solution Add a `ReflectFromReflect` type data struct. This struct allows us to easily convert Dynamic representations of our types into their respective concrete instances. ```rust #[derive(Reflect, FromReflect)] #[reflect(FromReflect)] // <- Register `ReflectFromReflect` struct MyStruct(String); let type_id = TypeId::of::<MyStruct>(); // Register our type let mut registry = TypeRegistry::default(); registry.register::<MyStruct>(); // Create a concrete instance let my_struct = MyStruct("Hello world".to_string()); // `Reflect::clone_value` will generate a `DynamicTupleStruct` for tuple struct types let dynamic_value: Box<dyn Reflect> = my_struct.clone_value(); assert!(!dynamic_value.is::<MyStruct>()); // Get the `ReflectFromReflect` type data from the registry let rfr: &ReflectFromReflect = registry .get_type_data::<ReflectFromReflect>(type_id) .unwrap(); // Call `FromReflect::from_reflect` on our Dynamic value let concrete_value: Box<dyn Reflect> = rfr.from_reflect(&dynamic_value); assert!(concrete_value.is::<MyStruct>()); ``` ### Why this PR? ###### Why now? The three main reasons I closed #4147 were that: 1. Registering `ReflectFromReflect` is clunky (deriving `FromReflect` *and* registering `ReflectFromReflect`) 2. The ecosystem and Bevy itself didn't seem to pay much attention to deriving `FromReflect` 3. I didn't see a lot of desire from the community for such a feature However, as time has passed it seems 2 and 3 are not really true anymore. Bevy is internally adding lots more `FromReflect` derives, which should make this feature all the more useful. Additionally, I have seen a growing number of people look for something like `ReflectFromReflect`. I think 1 is still an issue, but not a horrible one. Plus it could be made much, much better using #6056. And I think splitting this feature out of #6056 could lead to #6056 being adopted sooner (or at least make the need more clear to users). ###### Why not just re-open #4147? The main reason is so that this PR can garner more attention than simply re-opening the old one. This helps bring fresh eyes to the PR for potentially more perspectives/reviews. --- ## Changelog * Added `ReflectFromReflect` Co-authored-by: Gino Valente <49806985+MrGVSV@users.noreply.github.com>

MrGVSV · 2022-12-11T22:56:29Z

Yeah I think if we went ahead and put this behind a feature, it'd be best to make it a default one. But I'd like to hear other people's thoughts.

❓ Should we (1) put this behind a feature and (2) should that feature be enabled by default? ❓

tim-blackbird · 2022-12-11T23:46:32Z

If this is put behind a feature it should definitely be enabled by default.
Scenes are a core pillar of game development. Even games that are heavily procedural use scenes for rooms/prefabs.

If it is behind a feature, disabling the feature is likely to prevent that project from making use of the future Bevy editor.

MrGVSV · 2022-12-13T17:19:44Z

If it is behind a feature, disabling the feature is likely to prevent that project from making use of the future Bevy editor.

True. Even beyond scenes, the future editor will likely need to be able to create data, which is only possible through FromReflect (or ReflectDefault but that's a lot more restrictive).

I'll try to put this behind a feature and update the RFC with this new approach (if it works nicely/doesn't result in spaghetti lol)

# Objective Resolves bevyengine#4597 (based on the work from bevyengine#6056 and a refresh of bevyengine#4147) When using reflection, we may often end up in a scenario where we have a Dynamic representing a certain type. Unfortunately, we can't just call `MyType::from_reflect` as we do not have knowledge of the concrete type (`MyType`) at runtime. Such scenarios happen when we call `Reflect::clone_value`, use the reflection deserializers, or create the Dynamic type ourselves. ## Solution Add a `ReflectFromReflect` type data struct. This struct allows us to easily convert Dynamic representations of our types into their respective concrete instances. ```rust #[derive(Reflect, FromReflect)] #[reflect(FromReflect)] // <- Register `ReflectFromReflect` struct MyStruct(String); let type_id = TypeId::of::<MyStruct>(); // Register our type let mut registry = TypeRegistry::default(); registry.register::<MyStruct>(); // Create a concrete instance let my_struct = MyStruct("Hello world".to_string()); // `Reflect::clone_value` will generate a `DynamicTupleStruct` for tuple struct types let dynamic_value: Box<dyn Reflect> = my_struct.clone_value(); assert!(!dynamic_value.is::<MyStruct>()); // Get the `ReflectFromReflect` type data from the registry let rfr: &ReflectFromReflect = registry .get_type_data::<ReflectFromReflect>(type_id) .unwrap(); // Call `FromReflect::from_reflect` on our Dynamic value let concrete_value: Box<dyn Reflect> = rfr.from_reflect(&dynamic_value); assert!(concrete_value.is::<MyStruct>()); ``` ### Why this PR? ###### Why now? The three main reasons I closed bevyengine#4147 were that: 1. Registering `ReflectFromReflect` is clunky (deriving `FromReflect` *and* registering `ReflectFromReflect`) 2. The ecosystem and Bevy itself didn't seem to pay much attention to deriving `FromReflect` 3. I didn't see a lot of desire from the community for such a feature However, as time has passed it seems 2 and 3 are not really true anymore. Bevy is internally adding lots more `FromReflect` derives, which should make this feature all the more useful. Additionally, I have seen a growing number of people look for something like `ReflectFromReflect`. I think 1 is still an issue, but not a horrible one. Plus it could be made much, much better using bevyengine#6056. And I think splitting this feature out of bevyengine#6056 could lead to bevyengine#6056 being adopted sooner (or at least make the need more clear to users). ###### Why not just re-open bevyengine#4147? The main reason is so that this PR can garner more attention than simply re-opening the old one. This helps bring fresh eyes to the PR for potentially more perspectives/reviews. --- ## Changelog * Added `ReflectFromReflect` Co-authored-by: Gino Valente <49806985+MrGVSV@users.noreply.github.com>

This reverts commit 15b7aef8525f6f1b4ba01481a61c260f2e183f00.

crates/bevy_reflect/bevy_reflect_derive/src/derive_data.rs

alice-i-cecile

A fairly simple change with substantial improvements to serious use-cases.

Docs are great, code quality is solid, and there are some nice new compile-fail tests. We definitely need more realistic uses of reflection in the engine and the examples: it's hard to see the ultimate impact in the diff.

I'd really like to see the dead_code cleaned up one way or another, but I have no other objections.

soqb

no blocking issues as far as i can see!
a couple of minor nits which can quickly be solved now or in a followup.

soqb · 2023-06-23T11:07:04Z

crates/bevy_reflect/src/impls/std.rs

-impl_from_reflect_value!(bool);
-impl_from_reflect_value!(char);
-impl_from_reflect_value!(u8);
-impl_from_reflect_value!(u16);
-impl_from_reflect_value!(u32);
-impl_from_reflect_value!(u64);
-impl_from_reflect_value!(u128);
-impl_from_reflect_value!(usize);
-impl_from_reflect_value!(i8);
-impl_from_reflect_value!(i16);
-impl_from_reflect_value!(i32);
-impl_from_reflect_value!(i64);
-impl_from_reflect_value!(i128);
-impl_from_reflect_value!(isize);
-impl_from_reflect_value!(f32);
-impl_from_reflect_value!(f64);
-impl_from_reflect_value!(String);
-impl_from_reflect_value!(PathBuf);
-impl_from_reflect_value!(OsString);
-impl_from_reflect_value!(HashSet<T: TypePath + Hash + Eq + Clone + Send + Sync>);
-impl_from_reflect_value!(Range<T: TypePath + Clone + Send + Sync>);
-impl_from_reflect_value!(RangeInclusive<T: TypePath + Clone + Send + Sync>);
-impl_from_reflect_value!(RangeFrom<T: TypePath + Clone + Send + Sync>);
-impl_from_reflect_value!(RangeTo<T: TypePath + Clone + Send + Sync>);
-impl_from_reflect_value!(RangeToInclusive<T: TypePath + Clone + Send + Sync>);
-impl_from_reflect_value!(RangeFull);
-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);
-impl_from_reflect_value!(NonZeroUsize);
-impl_from_reflect_value!(NonZeroI64);
-impl_from_reflect_value!(NonZeroU64);
-impl_from_reflect_value!(NonZeroU32);
-impl_from_reflect_value!(NonZeroI32);
-impl_from_reflect_value!(NonZeroI16);
-impl_from_reflect_value!(NonZeroU16);
-impl_from_reflect_value!(NonZeroU8);
-impl_from_reflect_value!(NonZeroI8);
-


hahaha! yes!

soqb · 2023-06-23T11:21:27Z

crates/bevy_reflect/bevy_reflect_derive/src/container_attributes.rs

+                if existing.value() != new.value() {
+                    return Err(syn::Error::new(
+                        new.span(),
+                        format!("`from_reflect` already set to {}", existing.value()),
+                    ));
+                }


should we report an error if there are two matching instances of the attribute, for example:

#[derive(Reflect)] #[reflect(from_reflect = false)] #[reflect(from_reflect = false)] // ERROR: multiple uses of `from_reflect` struct A {}

this is what the standard library does.

soqb · 2023-06-23T11:32:49Z

crates/bevy_reflect/bevy_reflect_derive/src/lib.rs

@@ -182,7 +232,7 @@ pub fn derive_reflect(input: TokenStream) -> TokenStream {
 pub fn derive_from_reflect(input: TokenStream) -> TokenStream {


is there any reason we want to keep this derive/macro around? this feels to me like api clutter since it is useless without Reflect and iiuc there is no difference between the FromReflect auto-derive and the manual derive.

a deprecation notice at least would be nice here

soqb · 2023-06-23T11:46:44Z

crates/bevy_reflect/bevy_reflect_derive/src/lib.rs

+    let (reflect_impls, from_reflect_impl) = match derive_data {
+        ReflectDerive::Struct(struct_data) | ReflectDerive::UnitStruct(struct_data) => (
+            impls::impl_struct(&struct_data),
+            if struct_data.meta().from_reflect().should_auto_derive() {
+                Some(from_reflect::impl_struct(&struct_data))
+            } else {
+                None
+            },
+        ),
+        ReflectDerive::TupleStruct(struct_data) => (
+            impls::impl_tuple_struct(&struct_data),
+            if struct_data.meta().from_reflect().should_auto_derive() {
+                Some(from_reflect::impl_tuple_struct(&struct_data))
+            } else {
+                None
+            },
+        ),
+        ReflectDerive::Enum(enum_data) => (
+            impls::impl_enum(&enum_data),
+            if enum_data.meta().from_reflect().should_auto_derive() {
+                Some(from_reflect::impl_enum(&enum_data))
+            } else {
+                None
+            },
+        ),
+        ReflectDerive::Value(meta) => (
+            impls::impl_value(&meta),
+            if meta.from_reflect().should_auto_derive() {
+                Some(from_reflect::impl_value(&meta))
+            } else {
+                None
+            },
+        ),
+    };


imo the FromReflect impls would make more sense placed within the impls::impl_struct, impls::impl_value etc. functions since this is where we already put the other traits like Typed and TypePath.

soqb · 2023-06-23T11:52:04Z

crates/bevy_reflect/bevy_reflect_derive/src/utility.rs

+    pub fn new_with_bounds<'a: 'b, 'b>(
+        meta: &ReflectMeta,
+        active_fields: impl Iterator<Item = &'b StructField<'a>>,
+        ignored_fields: impl Iterator<Item = &'b StructField<'a>>,
+        active_bounds: impl Fn(&StructField<'a>) -> Option<proc_macro2::TokenStream>,
+        ignored_bounds: impl Fn(&StructField<'a>) -> Option<proc_macro2::TokenStream>,
+    ) -> Self {
+        let bevy_reflect_path = meta.bevy_reflect_path();
+        let is_from_reflect = meta.from_reflect().should_auto_derive();
+
+        let (active_types, active_trait_bounds): (Vec<_>, Vec<_>) = active_fields
+            .map(|field| {
+                let ty = field.data.ty.clone();
+
+                let custom_bounds = active_bounds(field).map(|bounds| quote!(+ #bounds));
+
+                let bounds = if is_from_reflect {
+                    quote!(#bevy_reflect_path::FromReflect #custom_bounds)
+                } else {
+                    quote!(#bevy_reflect_path::Reflect #custom_bounds)
+                };
+
+                (ty, bounds)
+            })
+            .unzip();
+
+        let (ignored_types, ignored_trait_bounds): (Vec<_>, Vec<_>) = ignored_fields
+            .map(|field| {
+                let ty = field.data.ty.clone();
+
+                let custom_bounds = ignored_bounds(field).map(|bounds| quote!(+ #bounds));
+                let bounds = quote!(#FQAny + #FQSend + #FQSync #custom_bounds);
+
+                (ty, bounds)
+            })
+            .unzip();
+
+        let (parameter_types, parameter_trait_bounds): (Vec<_>, Vec<_>) = meta
+            .type_path()
+            .generics()
+            .type_params()
+            .map(|param| {
+                let ident = param.ident.clone();
+                let bounds = quote!(#bevy_reflect_path::TypePath);
+                (ident, bounds)
+            })
+            .unzip();
+
+        Self {
+            active_types: active_types.into_boxed_slice(),
+            active_trait_bounds: active_trait_bounds.into_boxed_slice(),
+            ignored_types: ignored_types.into_boxed_slice(),
+            ignored_trait_bounds: ignored_trait_bounds.into_boxed_slice(),
+            parameter_types: parameter_types.into_boxed_slice(),
+            parameter_trait_bounds: parameter_trait_bounds.into_boxed_slice(),


these parameters are pretty complex and i wonder if it would make more sense to use a builder pattern where xxx_fields and xxx_bounds are specified together with a method on WhereClauseOptions.

the function is sparsely used so its okay to leave it as is, though.

soqb · 2023-06-23T11:58:51Z

examples/reflection/reflection.rs

+/// If you choose to ignore a field, you need to let the automatically-derived `FromReflect` implementation
+/// how to handle the field.


Suggested change

/// If you choose to ignore a field, you need to let the automatically-derived `FromReflect` implementation

/// how to handle the field.

/// If you choose to ignore a field, you need to let the automatically-derived `FromReflect` implementation

/// know how to handle the field.

😉

cart

I'm on board! Love the improved ergonomics. Just resolved merge conflicts.

@soqb did bring up some good points, but I think those can be addressed in followups. Given the impeding 0.11 release, its worth getting this in now.

MrGVSV · 2023-06-29T01:42:04Z

@soqb did bring up some good points, but I think those can be addressed in followups.

Yeah I was meaning to get to those but just got busy. Out of town now, but I'll try to find time to get the follow ups in PR form (either for 0.11 or immediately after)

MrGVSV added C-Feature A new feature, making something new possible A-Reflection Runtime information about types X-Controversial There is active debate or serious implications around merging this PR labels Sep 22, 2022

MrGVSV force-pushed the reflect-from-reflect-ergonomics branch 3 times, most recently from 25684f0 to c907c92 Compare September 22, 2022 07:14

Nilirad added the S-Needs-Design-Doc This issue or PR is particularly complex, and needs an approved design doc before it can be merged label Sep 22, 2022

MrGVSV mentioned this pull request Sep 24, 2022

bevy_reflect: Unique Reflect bevyengine/rfcs#56

Merged

5 tasks

jakobhellermann reviewed Oct 3, 2022

View reviewed changes

jakobhellermann mentioned this pull request Oct 6, 2022

[Merged by Bors] - add ReflectAsset and ReflectHandle #5923

Closed

MrGVSV mentioned this pull request Oct 7, 2022

bevy_reflect: Convert dynamic types to concrete instances #4597

Closed

MrGVSV mentioned this pull request Oct 12, 2022

[Merged by Bors] - bevy_reflect: Add ReflectFromReflect (v2) #6245

Closed

MrGVSV force-pushed the reflect-from-reflect-ergonomics branch 2 times, most recently from c7bda64 to c63f746 Compare November 10, 2022 07:14

MrGVSV force-pushed the reflect-from-reflect-ergonomics branch from c63f746 to 880c415 Compare December 11, 2022 22:46

MrGVSV force-pushed the reflect-from-reflect-ergonomics branch from 880c415 to 57c952b Compare December 11, 2022 23:10

MrGVSV force-pushed the reflect-from-reflect-ergonomics branch 2 times, most recently from 95cb9e3 to e4d34aa Compare June 22, 2023 07:56

MrGVSV added 6 commits June 22, 2023 14:18

Automatically derive FromReflect with Reflect

2c9b06f

Add #[reflect(from_reflect = false)] attribute

74bca48

Auto-register ReflectFromReflect

36fa637

Use FromReflect when deserializing

ed88e68

Revert "Use FromReflect when deserializing"

09039d4

This reverts commit 15b7aef8525f6f1b4ba01481a61c260f2e183f00.

Fixed bounds for normal FromReflect derive

6c96cca

MrGVSV force-pushed the reflect-from-reflect-ergonomics branch from e4d34aa to 6c96cca Compare June 22, 2023 21:19

alice-i-cecile added this to the 0.11 milestone Jun 22, 2023

alice-i-cecile reviewed Jun 22, 2023

View reviewed changes

crates/bevy_reflect/bevy_reflect_derive/src/derive_data.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Jun 22, 2023

View reviewed changes

Remove deadened code

de83756

alice-i-cecile approved these changes Jun 22, 2023

View reviewed changes

soqb approved these changes Jun 23, 2023

View reviewed changes

Merge remote-tracking branch 'origin/main' into pr/MrGVSV/6056

bf39a86

cart approved these changes Jun 29, 2023

View reviewed changes

cart enabled auto-merge June 29, 2023 01:18

cart added this pull request to the merge queue Jun 29, 2023

Merged via the queue into bevyengine:main with commit aeeb20e Jun 29, 2023

Nilirad mentioned this pull request Jun 29, 2023

Fix CI Nilirad/bevy_prototype_lyon#218

Merged

cart mentioned this pull request Jun 29, 2023

News: Release 0.11 bevyengine/bevy-website#657

Merged

MrGVSV mentioned this pull request Jul 4, 2023

bevy_reflect: Recursive registration #5781

Merged

MrGVSV deleted the reflect-from-reflect-ergonomics branch July 31, 2023 01:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bevy_reflect: `FromReflect` Ergonomics Implementation #6056

bevy_reflect: `FromReflect` Ergonomics Implementation #6056

MrGVSV commented Sep 22, 2022 •

edited

Loading

jakobhellermann left a comment

MrGVSV commented Oct 3, 2022

tguichaoua commented Oct 3, 2022

soqb commented Oct 12, 2022

Zeenobit commented Dec 11, 2022 •

edited

Loading

MrGVSV commented Dec 11, 2022

Zeenobit commented Dec 11, 2022

MrGVSV commented Dec 11, 2022

Zeenobit commented Dec 11, 2022

MrGVSV commented Dec 11, 2022

tim-blackbird commented Dec 11, 2022 •

edited

Loading

MrGVSV commented Dec 13, 2022 •

edited

Loading

alice-i-cecile left a comment

soqb left a comment

soqb Jun 23, 2023

soqb Jun 23, 2023

soqb Jun 23, 2023

soqb Jun 23, 2023

soqb Jun 23, 2023

soqb Jun 23, 2023

soqb Jun 23, 2023

cart left a comment

MrGVSV commented Jun 29, 2023

		@@ -182,7 +232,7 @@ pub fn derive_reflect(input: TokenStream) -> TokenStream {
		pub fn derive_from_reflect(input: TokenStream) -> TokenStream {

		/// If you choose to ignore a field, you need to let the automatically-derived `FromReflect` implementation
		/// how to handle the field.

bevy_reflect: FromReflect Ergonomics Implementation #6056

bevy_reflect: FromReflect Ergonomics Implementation #6056

Conversation

MrGVSV commented Sep 22, 2022 • edited Loading

Objective

Solution

ReflectFromReflect

Improved Deserialization

Changelog

Migration Guide

Removed Migrations

jakobhellermann left a comment

Choose a reason for hiding this comment

MrGVSV commented Oct 3, 2022

tguichaoua commented Oct 3, 2022

soqb commented Oct 12, 2022

Zeenobit commented Dec 11, 2022 • edited Loading

MrGVSV commented Dec 11, 2022

Zeenobit commented Dec 11, 2022

MrGVSV commented Dec 11, 2022

Zeenobit commented Dec 11, 2022

MrGVSV commented Dec 11, 2022

tim-blackbird commented Dec 11, 2022 • edited Loading

MrGVSV commented Dec 13, 2022 • edited Loading

alice-i-cecile left a comment

Choose a reason for hiding this comment

soqb left a comment

Choose a reason for hiding this comment

soqb Jun 23, 2023

Choose a reason for hiding this comment

soqb Jun 23, 2023

Choose a reason for hiding this comment

soqb Jun 23, 2023

Choose a reason for hiding this comment

soqb Jun 23, 2023

Choose a reason for hiding this comment

soqb Jun 23, 2023

Choose a reason for hiding this comment

soqb Jun 23, 2023

Choose a reason for hiding this comment

soqb Jun 23, 2023

Choose a reason for hiding this comment

cart left a comment

Choose a reason for hiding this comment

MrGVSV commented Jun 29, 2023

bevy_reflect: `FromReflect` Ergonomics Implementation #6056

bevy_reflect: `FromReflect` Ergonomics Implementation #6056

MrGVSV commented Sep 22, 2022 •

edited

Loading

`ReflectFromReflect`

Zeenobit commented Dec 11, 2022 •

edited

Loading

tim-blackbird commented Dec 11, 2022 •

edited

Loading

MrGVSV commented Dec 13, 2022 •

edited

Loading