bevy_reflect: Convert dynamic types to concrete instances

[MrGVSV]

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 DynamicStruct here for simplicity). They know that this DynamicStruct contains an object that implements— or likely implements— their custom trait SomeTrait.

They made their SomeTrait object-safe so it can be passed around as dyn SomeTrait, but the problem is that they don't know the underlying type. If they did, they might be able to convert this DynamicStruct into that type like:

let my_struct = <SomeStruct as FromReflect>::from_reflect(&dyn_struct);

But, again, we don't know SomeStruct exists.

Okay so maybe we just get the reflected trait from the registry:

let reflect_some_trait = type_registry
  .get_type_data::<ReflectSomeTrait>(concrete_type_id)
  .unwrap();

let my_struct: &dyn SomeTrait = reflect_some_trait.get(&dyn_struct).unwrap();

This seems to work, but that last line will panic. Why? It's because we're passing in our DynamicStruct instead of a concrete instance of SomeStruct.

So the question is, how do we get SomeStruct from DynamicStruct when we don't know SomeStruct exists?

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 return Dynamic*** 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:

1. Simple. The methods used to access the concrete type are easily approachable. The reflection crate is already a bit confusing and can be somewhat complex so the simpler, the better.
2. Safe. The public API should be safe to use for those who maybe aren't as technically skilled (e.g. user-facing pointer shenanigans).
3. Just Works. It would be nice if the system can work without the users needing to do anything on their part. As far as I can tell, this is probably not possible. But if it is, we should aim for it.

In regards to that last point, it would be great if we also didn't have to rely on FromReflect since not all types implement it. However, if the usefulness/ergonomics of it improves or it's added back into the Reflect derive, 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:

let my_struct: Box<dyn Reflect> = dyn_struct.convert();
// OR
let my_struct: Box<dyn Reflect> = dyn_struct.convert(&type_registry);

Obviously, this isn't a requirement and the actual API can look quite different.

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 the DynamicStruct to a reflected concrete instance:

#[derive(Reflect, FromReflect)]
#[reflect(FromReflect)]
struct SomeStruct { value: usize }

let rfr = registry.get_type_data::<ReflectFromReflect>(type_id).unwrap();
let my_struct: Box<dyn Reflect> = rfr.from_reflect(&dyn_struct).unwrap();

However, this was abandoned for a few reasons:

• It adds boilerplate/confusion (we have to write FromReflect twice)
• It relies on FromReflect which requires users derive it along with Reflect itself
• It feels like we can do better

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

[oddfacade]

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 ReflectComponent. My main complaint with your proposed API is that if a user absolutely needs a concrete type, they would have to speculatively call convert on everything, which seems like it would be yet another counter-intuitive detail in the already conceptually challenging reflect crate.

The fundamental problem, as I see it, is that users have no way to know whether the Reflect trait objects they're dealing with are "dynamic" or not. This issue derives from the fact that "being dynamic" is purely semantic. As far as the code is concerned, there's nothing special about dynamic structs. There is a certain elegance to this; the user is given a unified interface and encouraged to think of dynamic and concrete objects as being basically the same thing. The problem is that if the user actually takes the bait and thinks of them this way it's only a matter of time before they step on one of the numerous landmines present in the api. If the dynamic/concrete distinction doesn't matter, I should be able to downcast to the concrete type, right? I should be able to use this trait object's type ID to look up its registrations?

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) Dynamic trait

Let something like your convert method be defined on this trait, and implement it only for DynamicStruct and friends. The result would be a bit like ReflectFromReflect but with a narrower scope. Users can then do stuff like

   // suppose "component" is of type &dyn Reflect
   let definitely_the_right_typeid = registry.get_type_data::<ReflectDynamic>(component.type_id())
      .and_then(|dynamic_methods| dynamic_methods.reify(component))
      .unwrap_or(component)
      .type_id();

(untested and not entirely thought through, but you get the idea). Unfortunately, this has a lot of the same problems as you mention in your "alternatives" section, and introduces more hash table lookup boilerplate.

Make converting to/from dynamic objects a more explicit part of the Reflect trait

As a starting point, an associated IS_DYNAMIC const would go a long way. Rename clone_value to make_dynamic and add a from_dynamic constructor (perhaps as a secondary trait with a blanket implementation over FromReflect + Dynamic). Figure out a way to have dynamic types be aware of which type they're ostensibly reflecting, then tighten up apply and friends so that they only work for the type they're supposed to be defined on, rather than anything that has the right fields. Use these new methods in the TypeRegistry api to let it do the reification for you when it makes sense to do so.

I feel like your #4042 proposal might integrate well with this second approach, but I haven't properly reviewed it yet.

[MrGVSV]

My main complaint with your proposed API is that if a user absolutely needs a concrete type, they would have to speculatively call convert on everything, which seems like it would be yet another counter-intuitive detail in the already conceptually challenging reflect crate.

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.

The fundamental problem, as I see it, is that users have no way to know whether the Reflect trait objects they're dealing with are "dynamic" or not. This issue derives from the fact that "being dynamic" is purely semantic.

Yeah, this is why I like the IS_DYNAMIC const idea. Otherwise, there's no way of knowing (besides maybe comparing TypeId?). And this would obviously help with determining if conversion is necessary (probably through a convenience Reflect::is_dynamic() method).

Let something like your convert method be defined on this trait, and implement it only for DynamicStruct and friends. The result would be a bit like ReflectFromReflect but with a narrower scope.

I think this is definitely a step up from what I had with ReflectFromReflect. But I think your API could still be condensed into a single possibly_dynamic.convert(&type_registry) for user convenience. Again, I agree it's confusing haha but I think it beats boilerplate (and the confusion that comes along with it there).

add a from_dynamic constructor (perhaps as a secondary trait with a blanket implementation over FromReflect + Dynamic)

Maybe I'm missing something, but isn't this already what FromReflect::from_reflect(...) does?

Use these new methods in the TypeRegistry api to let it do the reification for you when it makes sense to do so.

I think the difficulty is that we might not know the user's intentions. Maybe they actually do want to ReflectTrait on a DynamicStruct or whatever. Or maybe the type data is meant for use with dynamic types. I think it makes more sense to put this power in the hands of the users (and more importantly the hands of plugin authors). Others may disagree with that sentiment though haha.

[oddfacade]

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.

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 is_dynamic method to Reflect and tell users to check it before they call convert or whatever, but it just doesn't make sense to do so. If convert is part of Reflect it has to have an implementation regardless of whether the underlying type is dynamic. So your options are either

1. make it a no-op if the type isn't dynamic (which means the check is redundant), or
2. make it panic/return an error (which introduces a completely unnecessary failure state).

Perhaps the method could be defined only on some Dynamic trait, but then functions which can't tolerate the trait bound (because they need to be able to work on any dyn Reflect) would have to pull it out of the type registry. Short of some more comprehensive (and extremely controversial) overhaul like trying to make it so that DynamicStruct isn't Reflect I think your suggestion is probably the best way forward.

And this would obviously help with determining if conversion is necessary (probably through a convenience Reflect::is_dynamic() method).

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.

Maybe they actually do want to ReflectTrait on a DynamicStruct or whatever.

I don't think users can reflect new traits on DynamicStruct, because they don't control its GetTypeRegistration implementation. That being said, I could imagine a use case where you might want access to DynamicStruct's serialize implementation or something, but I feel like something has to give here. As it stands, we have an API which is dedicated to erasing any distinction between these two types of objects, except there's a handful of circumstances where it's super important that you know exactly what you're dealing with, and you can only really figure out which is which by reading the source code.

[MrGVSV]

So your options are either

1. make it a no-op if the type isn't dynamic (which means the check is redundant), or
2. make it panic/return an error (which introduces a completely unnecessary failure state).

Yeah my thought was that it performs the check and acts accordingly similar to how Handle::as_weak(...) works— where you could check if it's weak beforehand or just call the function directly. If it's already in the correct state, then it's a no-op. Maybe it would be better to mimic that method for consistency. As an example:

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 Option<Box<dyn Reflect>>.

I don't think users can reflect new traits on DynamicStruct, because they don't control its GetTypeRegistration implementation.

They can if they do any manual type data insertions. Very unlikely, but possible haha.

That being said, I could imagine a use case where you might want access to DynamicStruct's serialize implementation or something, but I feel like something has to give here. As it stands, we have an API which is dedicated to erasing any distinction between these two types of objects, except there's a handful of circumstances where it's super important that you know exactly what you're dealing with, and you can only really figure out which is which by reading the source code.

Definitely agree. We should probably favor a clear, consistent, and useful API over one that is highly configurable.

[Suficio]

Perhaps I can offer a different perspective here.

In my opinion ReflectDeserialize already indirectly provides the functionality required by this issue.

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 ReflectDeserialize during construction and therefore had to be represented by a dynamic type.

This could conceivably be solved by implementing Deserializer on one of our existing deserializers.

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 Deserialize derives on glam types when additionally deriving ReflectDeserialize would allow the types to be deserialized into concrete types, avoiding this issue.

[MrGVSV]

In this context I would like to ask why did #5723 remove Deserialize derives on glam types when additionally deriving ReflectDeserialize would allow the types to be deserialized into concrete types, avoiding this issue.

Great question! There's a couple reasons for this.

1. One of the overarching goals of bevy_reflect is to remove the need for deriving Serialize/Deserialize on user types. This helps reduce compile times and generated lines of code (Deserialize can generate upwards of 1000 lines of code, while Reflect only needs about 500).

2. It's also probably not great to use ReflectDeserialize to solve this as it is intended for deserialization. If we just have a Dynamic*** type lying around, we'd need to serialize then deserialize it (using ReflectDeserialize) in order to get the concrete type. This can work, but will likely be slow and inefficient (I'm looking at you JSON.parse(JSON.stringify(...))). Not to mention, it means we need a lock on the TypeRegistry every time we want to convert.

[MrGVSV]

And for reference, there is some work being done in this space to help solve this issue. We have the Unique Reflect RFC which splits Reflect into Reflect (for concrete types) and PartialReflect (for Dynamic/proxy types). We also have #6056 which is an implementation of the FromReflect Ergonomics RFC and provides us with a better version of ReflectFromReflect.