-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
bevy_reflect: FromReflect
Ergonomics Implementation
#6056
Conversation
25684f0
to
c907c92
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
The shorter the better. |
what about |
c7bda64
to
c63f746
Compare
I've been looking at this PR and #6245 and been reading the Related RFC and I'm not a fan of automatically deriving
From experience, the only time 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 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 |
Have you considered gating this behind a feature? i.e. only auto derive 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 |
Hm, that might be a fair compromise 🤔 I mean, this PR doesn't enforce that 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
I don't think it would since you may want to have everything derive |
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:
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>
c63f746
to
880c415
Compare
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? ❓ |
880c415
to
57c952b
Compare
If this is put behind a feature it should definitely be enabled by default. 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 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>
# 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>
95cb9e3
to
e4d34aa
Compare
This reverts commit 15b7aef8525f6f1b4ba01481a61c260f2e183f00.
e4d34aa
to
6c96cca
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no blocking issues as far as i can see!
a couple of minor nits which can quickly be solved now or in a followup.
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); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hahaha! yes!
if existing.value() != new.value() { | ||
return Err(syn::Error::new( | ||
new.span(), | ||
format!("`from_reflect` already set to {}", existing.value()), | ||
)); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 {}
@@ -182,7 +232,7 @@ pub fn derive_reflect(input: TokenStream) -> TokenStream { | |||
pub fn derive_from_reflect(input: TokenStream) -> TokenStream { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a deprecation notice at least would be nice here
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 | ||
}, | ||
), | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
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(), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
/// If you choose to ignore a field, you need to let the automatically-derived `FromReflect` implementation | ||
/// how to handle the field. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/// 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. |
😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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) |
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 useFromReflect
.It also sneaks up in other ways. For example, it's a required bound for
T
inVec<T>
so thatVec<T>
as a whole can be madeFromReflect
. It's also required by all fields of an enum as it's used as part of theReflect::apply
implementation.So in other words, much like
GetTypeRegistration
andTyped
, 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 derivingReflect
.Users can then choose to opt-out if needed using the
#[reflect(from_reflect = false)]
attribute.ReflectFromReflect
This PR also automatically adds the
ReflectFromReflect
(introduced in #6245) registration to the derivedGetTypeRegistration
impl— if the type hasn't opted out ofFromReflect
of course.Improved Deserialization
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 theBox
'd Real type.[Un]TypedReflectDeserializer::new_dynamic
will work like what we have now and simply return theBox
'd Dynamic type.Changelog
FromReflect
is now automatically derived within theReflect
derive macroReflectFromReflect
in the derivedGetTypeRegistration
implRenamedDescopedTypedReflectDeserializer::new
andUntypedReflectDeserializer::new
toTypedReflectDeserializer::new_dynamic
andUntypedReflectDeserializer::new_dynamic
, respectivelyChangedDescopedTypedReflectDeserializer::new
andUntypedReflectDeserializer::new
to automatically convert the deserialized output usingFromReflect
Migration Guide
FromReflect
is now automatically derived within theReflect
derive macro. Items with both derives will need to remove theFromReflect
one.If using a manual implementation of
FromReflect
and theReflect
derive, users will need to opt-out of the automatic implementation.Removed Migrations
The reflect deserializers now perform a
FromReflect
conversion internally. The expected output ofTypedReflectDeserializer::new
andUntypedReflectDeserializer::new
is no longer a Dynamic (e.g.,DynamicList
), but its Real counterpart (e.g.,Vec<i32>
).Alternatively, if this behavior isn't desired, use the
TypedReflectDeserializer::new_dynamic
andUntypedReflectDeserializer::new_dynamic
methods instead: