-
Notifications
You must be signed in to change notification settings - Fork 66
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
FromReflect
Ergonomics
#59
Conversation
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.
Love this! I am fully behind and have wanted every feature here for a while.
|
||
## Unresolved questions | ||
|
||
- Should `ReflectFromReflect` be replaced by a method on `TypeRegistration`, such as `TypeRegistration::construct`? |
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 don't think so, but I wouldn't be opposed to adding construct()
as a helper function which wraps over ReflectFromReflect
.
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.
Yeah I agree that we should keep RFR. I decided to leave out the Future Possibilities
section, but in it I originally described adding a ReflectFromWorldReflect
type data to support a proposed FromWorldReflect
trait. And my thought was that this would pair nicely with RFR without needing to create another method on TypeRegistration
. (Left out that section because it seemed a bit off-topic, though, haha.)
And also, by keeping RFR we keep open the possibility of cloning and storing the type data outside the registry if needed.
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.
Please try to split your lines at the sentence / clause level; it makes reviewing and suggestions much more pleasant :)
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.
Have you considered using the “static type” instead of “real type”? It seems to be a direct translation from dynamic vs static typing.
I did, but chose not to use it. My reasoning was that "static" feels like we should know the exact type at compile time. However, a |
Okay I split bascially every sentence across multiple lines (where I could). Typora hates me now lol, but hopefully that's more readable/reviewable 🙂 |
Any update on this? I am trying to deserialize some bevy basic types (eg Transform) but can't rewire the type from DynamicStruct, because it doesn't have ReflectFromReflect. |
Unfortunately, there hasn't been much progress. While One reason I haven't updated it is because after coding the initial implementation in bevyengine/bevy#6056, I just found the RFC to be overkill. The actual changes are only a few hundred lines and most of it is cleanup. On top of that, So I could update this, but I may just close it out in favor of just using bevyengine/bevy#6056 to respond to any feedback. I'll probably rebase and update that PR relatively soon. Thoughts?
If you want to make a PR adding in a In the meantime, however, you can just manually register it for all your known types: app.register_type_data::<Transform, ReflectFromReflect>(); |
Didn't know there was a workaround, that's actually great, thanks. |
Closing this out. See bevyengine/bevy#6056 (comment) for details. |
# 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(®istry); 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(®istry); 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(®istry); 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(®istry); // NEW let reflect_deserializer = UntypedReflectDeserializer::new_dynamic(®istry); ``` </details> --------- Co-authored-by: Carter Anderson <[email protected]>
# 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(®istry); 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(®istry); 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(®istry); 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(®istry); // NEW let reflect_deserializer = UntypedReflectDeserializer::new_dynamic(®istry); ``` </details> --------- Co-authored-by: Carter Anderson <[email protected]>
# 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(®istry); 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(®istry); 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(®istry); 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(®istry); // NEW let reflect_deserializer = UntypedReflectDeserializer::new_dynamic(®istry); ``` </details> --------- Co-authored-by: Carter Anderson <[email protected]>
# Objective - Implements the [Unique Reflect RFC](https://github.com/nicopap/rfcs/blob/bevy-reflect-api/rfcs/56-better-reflect.md). ## Solution - Implements the RFC. - This implementation differs in some ways from the RFC: - In the RFC, it was suggested `Reflect: Any` but `PartialReflect: ?Any`. During initial implementation I tried this, but we assume the `PartialReflect: 'static` in a lot of places and the changes required crept out of the scope of this PR. - `PartialReflect::try_into_reflect` originally returned `Option<Box<dyn Reflect>>` but i changed this to `Result<Box<dyn Reflect>, Box<dyn PartialReflect>>` since the method takes by value and otherwise there would be no way to recover the type. `as_full` and `as_full_mut` both still return `Option<&(mut) dyn Reflect>`. --- ## Changelog - Added `PartialReflect`. - `Reflect` is now a subtrait of `PartialReflect`. - Moved most methods on `Reflect` to the new `PartialReflect`. - Added `PartialReflect::{as_partial_reflect, as_partial_reflect_mut, into_partial_reflect}`. - Added `PartialReflect::{try_as_reflect, try_as_reflect_mut, try_into_reflect}`. - Added `<dyn PartialReflect>::{try_downcast_ref, try_downcast_mut, try_downcast, try_take}` supplementing the methods on `dyn Reflect`. ## Migration Guide - Most instances of `dyn Reflect` should be changed to `dyn PartialReflect` which is less restrictive, however trait bounds should generally stay as `T: Reflect`. - The new `PartialReflect::{as_partial_reflect, as_partial_reflect_mut, into_partial_reflect, try_as_reflect, try_as_reflect_mut, try_into_reflect}` methods as well as `Reflect::{as_reflect, as_reflect_mut, into_reflect}` will need to be implemented for manual implementors of `Reflect`. ## Future Work - This PR is designed to be followed up by another "Unique Reflect Phase 2" that addresses the following points: - Investigate making serialization revolve around `Reflect` instead of `PartialReflect`. - [Remove the `try_*` methods on `dyn PartialReflect` since they are stop gaps](#7207 (comment)). - Investigate usages like `ReflectComponent`. In the places they currently use `PartialReflect`, should they be changed to use `Reflect`? - Merging this opens the door to lots of reflection features we haven't been able to implement. - We could re-add [the `Reflectable` trait](https://github.com/bevyengine/bevy/blob/8e3488c88065a94a4f72199587e59341c9b6553d/crates/bevy_reflect/src/reflect.rs#L337-L342) and make `FromReflect` a requirement to improve [`FromReflect` ergonomics](bevyengine/rfcs#59). This is currently not possible because dynamic types cannot sensibly be `FromReflect`. - Since this is an alternative to #5772, #5781 would be made cleaner. --------- Co-authored-by: Alice Cecile <[email protected]> Co-authored-by: Gino Valente <[email protected]>
RENDERED
Derive
FromReflect
by default when derivingReflect
, addReflectFromReflect
type data, and makeReflectDeserializer
easier to use.