bevy_reflect: Convert dynamic types to concrete instances #4597
Comments
MrGVSV
added
C-Feature
alice-i-cecile
added
A-Reflection
|
I completely agree with the basic premise that we need some way to "reify" dynamic types. This would alleviate the need for some of the weird bespoke "trait reflection" workarounds like The fundamental problem, as I see it, is that users have no way to know whether the I don't know the best way to solve this, but here are a couple of approaches I've been thinking about: Add a (reflectable)
|
Hm, yeah, I think you're right that this could just be called for anything and add to confusion. But I'm not sure I see a way to do this without that and I don't think it's the worst thing. This whole issue is probably most relevant to plugin authors and deserialized data where they really don't know if something is "dynamic" or not. So I think it's okay that they perform some sort of pre-processing/assertion within their logic.
Yeah, this is why I like the
I think this is definitely a step up from what I had with
Maybe I'm missing something, but isn't this already what
I think the difficulty is that we might not know the user's intentions. Maybe they actually do want to |
I'm not exactly taking issue with the existence of runtime type checking logic. That's par for the course with dynamic type systems. It's more that I think making this logic explicit (if doing so is feasible) is worth the extra keystrokes. However, I think I've come around to your position. I don't think what I was suggesting is feasible. We could, hypothetically, add some
Perhaps the method could be defined only on some
I think something along these lines would probably be worth the complexity if you could have it tell you what the underlying "real" type is without actually doing the conversion. That would probably help streamline and formalize this "pull the real type's registration by name and then call 'apply' with a reference to the dynamic type" pattern that gets used in a few different places. I might be putting the cart before the horse here though.
I don't think users can reflect new traits on |
Yeah my thought was that it performs the check and acts accordingly similar to how fn do_something(mut value: Box<dyn Reflect>, registry: &TypeRegistry) {
let value = value.as_concrete(registry).unwrap();
// Use `value`...
}And I agree that reifying shouldn't result in an error. Probably just return an
They can if they do any manual type data insertions. Very unlikely, but possible haha.
Definitely agree. We should probably favor a clear, consistent, and useful API over one that is highly configurable. |
|
Perhaps I can offer a different perspective here. In my opinion It appears to me that this issue is trying to solve the problem of how to convert dynamic types of concrete types back into concrete types. We must ask ourselves, why are there dynamic types when we could immediately create a concrete type? To me the answer appears to be, because they did not implement This could conceivably be solved by implementing This suggestion is made in the context of the new work in #5723. In this context I would like to ask why did #5723 remove |
Great question! There's a couple reasons for this.
|
|
And for reference, there is some work being done in this space to help solve this issue. We have the Unique |
# 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>
# 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 <mcanders1@gmail.com>
What problem does this solve or what need does it fill?
TL;DR
Currently, there is not a good way to retrieve a concrete instance from a
Dynamic***type if the type is not known at compile-time. This is especially an issue in cases where a trait object is expected (and likely the only "known property" about the type).The Issue
There are cases where a user or plugin author has a dynamic structure (we'll just use a
DynamicStructhere for simplicity). They know that thisDynamicStructcontains an object that implements— or likely implements— their custom traitSomeTrait.They made their
SomeTraitobject-safe so it can be passed around asdyn SomeTrait, but the problem is that they don't know the underlying type. If they did, they might be able to convert thisDynamicStructinto that type like:But, again, we don't know
SomeStructexists.Okay so maybe we just get the reflected trait from the registry:
This seems to work, but that last line will panic. Why? It's because we're passing in our
DynamicStructinstead of a concrete instance ofSomeStruct.So the question is, how do we get
SomeStructfromDynamicStructwhen we don't knowSomeStructexists?Why?
It might seem obvious that if we need this functionality, just don't use
DynamicStruct. However, this is not always possible. For example, deserialization using reflection will almost always returnDynamic***data (for most types).For plugin authors who might rely on some serialized data, this could be very important.
What solution would you like?
Ideally, a solution would meet at least some of the following (generic and obvious) criteria:
In regards to that last point, it would be great if we also didn't have to rely on
FromReflectsince not all types implement it. However, if the usefulness/ergonomics of it improves or it's added back into theReflectderive, then it would be a lot nicer to use here.If I had to give an example API, something like the following would probably be most ideal:
What alternative(s) have you considered?
#4147 went about this by registering a new type:
ReflectFromReflect.When used on a struct as
#[reflect(FromReflect)], it would be registered as type data for the type in the type registry. When retrieved, it could be used to convert theDynamicStructto a reflected concrete instance:However, this was abandoned for a few reasons:
FromReflecttwice)FromReflectwhich requires users derive it along withReflectitselfIf this is the best we can do and/or the community prefers this method, it can be re-opened. However, for now it might be good to see what other solutions can be found.
The text was updated successfully, but these errors were encountered: