reflection: what to do about dynamic types

[soqb]

bevy_reflect is a great tool for the gamedev usecase, but there are still a number of pain points in its design which lead both to bugs and to inefficiencies and to me, the most painful area is dealing with dynamic types (or proxy types). here is an attempt to compile exactly what is wrong with them, and how we might fix them:

pain points

1. proxies are a poor substitute for concrete values.

• Serialization of Name component may not be working correctly #12001 (fix is reflect: treat proxy types correctly when serializing #12024 which requires just forgetting about proxy types and converting using FromReflect)
• bevy_reflect: reflect_hash and reflect_partial_eq inconsistent on Dynamic types #6601
• DynamicTypePath impl for dynamic types returns the non-proxied type path (e.g. "bevy_reflect::DynamicStruct"), rather than the type path of the proxied type.
  • we (i) chose this option to be consistent, since if the dynamic type doesn't proxy any type it doesn't have access to any other TypePath implementation but it definitely can cause issues.
  • this extends also to Any::type_id (which is already otherwise confusing since Box<dyn Reflect>: Any but at least that case requires Any to be in scope).

2. Reflect doesn't always accurately model dynamic types and sometimes it has to stretch to support them.

• first-class downcasting for dynamic types is a footgun. downcasting a proxied value is likely an attempt by the user to get a concrete value, so we should probably direct users elsewhere. if we need this behavior (it can probably enable some specific but notable performance improvements) we should provide an external escape hatch.
• the values of dynamic types don't have a fixed structure
  • this means if something is passed to a function like fn(&mut dyn Reflect), we can't assume the structure is the same.
  • this also stops us from having a consistent access to TypeInfo from &dyn Reflect, we could have a Cow<'static, TypeInfo> but this would be extremely slow for dynamic types, having to regenerate them whenever their structure changes.
• bevy_reflect: reflect_hash and reflect_partial_eq inconsistent on Dynamic types #6601 is also very relevant here.
• reflect_hash is completely useless outside of DynamicMap and i would love to remove it completely.

3. overreliance on proxies in the wider engine leads to both poor performance and unfriendly design.

• setting the values of components via ReflectComponent (and similar APIs), even despite the fact they are nearly always called with the correct concrete type (in trait object form), still must use Reflect::apply which is slow as described below.
  • there has been pushback against this design (see this longstanding partial solution (#6060) and this Discord discussion)
• for every reflected component/resource we use in a scene, we convert them to a proxy, only to use them by reference which results in much unavoidable, wasted work.
  • this has caused (and still is causing) loads of misserializations.
  • also, under the fix above (#12024), if the type registers a bespoke ReflectSerialize implementation, we'll convert back to the concrete type, wasting a whole bunch more work.
  • ideally, we'd roll values into serialization via DynamicScene by reference, which would remove both conversions.
• a similar philosophy applies to deserialization: we lazily convert our deserialized proxies into concrete values, only doing so if needed. this is good for Reflect::apply-based workflows but it might be worth defaulting to eager FromReflect::from_reflect conversions.
• compounding this with our extensive use of trait objects severely impedes optimization.

potential solutions

note these are not necessarily mutually exclusive.

Unique Reflect

Note

see the RFC and #7207 for details.

in the words of the RFC's author, @nicopap:

you can think of the Unique Reflect RFC as a way to move the "is the underlying type the canonical type" check into the type system.

so we would:

• introduce a new trait: PartialReflect (although i am starting to prefer the name Introspect), encapsulating the behavior of reading and writing to values (e.g. Struct::field would return Option<&dyn PartialReflect>).
• make Reflect a subtrait of PartialReflect.
• provide fallible conversions from PartialReflect to Reflect and infallible conversions in the other direction.
• make dynamic types implement PartialReflect but not Reflect, since they don't support any non-introspective operations.
• essentially, the new philosophy is if you have a T: Reflect it must be the type you want it, i.e. we won't bother helping you out via FromReflect, but a T: PartialReflect is just "some data" 🤷.

and then future aspects include:

• introducing a new Reflectable supertrait of Reflect which mediates things like FromReflect, Typed, TypePath (maybe GetTypeRegistration) to simplify bounds, improve future compatibility somewhat and widely improve consistency/predictability between all the reflection traits.
• try making PartialReflect: ?Any to allow it to be put on more types.
• in combination with some other ideas, yeet get_represented_type_info and require instead if a typed trait object is needed, dyn Reflect (not dyn PartialReflect) must be used.

together, this handily solves (2) and does some for (1), but nothing for (3).

investigate how to better structure downstream APIs.

building on unique reflect, we would reconsider APIs which consume Reflect and PartialReflect (like ReflectComponent and bevy_scene).

i have thoughts about how to put this together but i'm not confident enough about them to recommend anything although i am completely sure this is something we need to change as soon as possible.

this would definitely alleviate (3) somewhat and might block solutions to (2).

the DynamicValue proposition

Warning

this is an idea i had at the weekend so its not yet fully formed.

inspired in part by mirror_mirror::Value, we could introduce one dynamic type to rule them all and reconfigure DynamicStruct and friends to contain DynamicValues rather than Box<dyn Reflect>s. to make this worthwhile we could add a FromReflect::from_value method which would let us specialize (and optimize) the common case where things are deeply dynamically-reflected.

• its possible too we could have a Other(Box<dyn Reflect>) variant which lets us store non-dynamic data in DynamicValue if we really want to.

this would (i assume) improve (3) measurably as well and does a lot to reduce the impact of (1).

strictly separate dynamically-reflected values from concrete ones.

Warning

more weekend ideas: read at your own risk.

its possible we might want to disallow altogether dynamic values inside concrete ones and concrete (non-leaf) types in dynamic ones. this is largely because it has caused just so many issues in the past.

in this way, we would disallow all of the following:

// concrete in partial:
// ...

let foo: Foo = todo!();
let foo_boxed: Box<dyn Reflect> = Box::new(foo);

let mut struct_ = DynamicStruct::new();
struct_.insert_boxed(foo_boxed);

// partial in concrete:
// ...

#[derive(Reflect)]
struct Bar {
    very_dynamically_typed: DynamicStruct,
}

// and with unique reflect:
#[derive(Reflect)]
struct Baz {
    very_very_dynamically_typed: Box<dyn PartialReflect>,
}

// and with the `DynamicValue` proposition:
#[derive(Reflect)]
struct Quux {
	dynamically_typed: DynamicValue,
}

its easy to see how DynamicValue makes the top section easy and natural to enforce, and the bottom is easy to enforce with unique reflect but whether this is the direction we want to go in at all is harder to answer.

this mostly solves (1) and (2) combined with other suggestions.

comments

please leave below your thoughts on the solutions i proposed, and suggest more inconvenient aspects of dynamic types/parallel issues which you think might need a cohesive solution.

[MrGVSV]

Agreed to pretty much everything! The whole dynamic vs proxy vs concrete/canonical thing is really frustrating and confusing (especially for newcomers to Bevy's reflection).

I have some comments/questions/thoughts. I'll spread them out over multiple replies to make it a bit easier, though!

[MrGVSV]

• although i am starting to prefer the name Introspect

Could you elaborate as to why? I'm still of the mindset that PartialReflect and Reflect, while not strictly true or informative, do have that familiarity edge (i.e. "PartialReflect must be like Reflect but not fully Reflect"). Of course, bikeshedding this is always an option!

[cBournhonesque]

So PartialReflect is basically for the dynamic types (DynamicEnum, etc.); while Reflect is for the actual type?
I'm not a fan of the name either, I thought initially it was for reflecting only a subset of fields

Maybe ReflectProxy, DynamicProxy, StructuredReflect?
Just to make it clear that the role PartialReflect (the only role, if I understood the RFC correctly) is to have access to those structured functions (from DynamicList, DynamicMap, etc.) when in reflection

[MrGVSV]

So PartialReflect is basically for the dynamic types (DynamicEnum, etc.); while Reflect is for the actual type?

Sorta. PartialReflect is used to indicate that the underlying type may be a dynamic type or it may be a concrete type. So it's not necessary that the underlying type is actually a dynamic type and/or a proxy.

Maybe ReflectProxy, DynamicProxy, StructuredReflect?

StructuredReflect is decent. One concern, is that StructuredReflect sounds a lot more concrete than just Reflect (at least to me lol). I think to combat this, we could make Reflect sound a lot stronger. The term CanonicalReflect has been used before, which might help if we did want to go that route.

Another thing to consider wrt naming is that there may be a time when we want to split the subtraits in a similar way. For example, Enum into Enum and PartialEnum. Whatever naming scheme we end up with might influence how we name these splits, so it should be one that works for them as well.

Just to make it clear that the role PartialReflect (the only role, if I understood the RFC correctly) is to have access to those structured functions (from DynamicList, DynamicMap, etc.) when in reflection

That is its primary role. However, it also has methods such as PartialReflect::apply which I'm not sure count as "structured functions" (although, I suppose it relies on the structure of the type to apply the changes).

[MrGVSV]

• provide fallible conversions from PartialReflect to Reflect and infallible conversions in the other direction.

This means that FromReflect will be a requirement to go from a PartialReflect to a Reflect, correct?

[MrGVSV]

• introducing a new Reflectable supertrait of Reflect which mediates things like FromReflect, Typed, TypePath (maybe GetTypeRegistration) to simplify bounds, improve future compatibility somewhat and widely improve consistency/predictability between all the reflection traits.

Yeah I think this will be really helpful for compatibility. See #5772

[MrGVSV]

• try making PartialReflect: ?Any to allow it to be put on more types.

I imagine this is doable, but we lose out on things that require Any, like TypeId and such, right? How feasible do you think this would be?

[MrGVSV]

• in combination with some other ideas, yeet get_represented_type_info and require instead if a typed trait object is needed, dyn Reflect (not dyn PartialReflect) must be used.

Only for PartialReflect, though, right? Renamed back to get_type_info, I think we'd still want to keep it on Reflect.

[MrGVSV]

together, this handily solves (2) and does some for (1), but nothing for (3).

I agree that (3) isn't ideal, but I have a feeling Reflect::apply is still going to be a valuable part of Bevy's reflection system. For example, to facilitate undo/redo or for networking (where you might not need to send the full type).

But I do agree that wherever it makes sense to use Reflect over PartialReflect, we should.

[MrGVSV]

its possible we might want to disallow altogether dynamic values inside concrete ones and concrete (non-leaf) types in dynamic ones. this is largely because it has caused just so many issues in the past.

It is pretty annoying to deal with but I don't think we want to prevent either case. I imagine concrete-inside-dynamic has a chance to provide better performance in places where it does happen. And dynamic-inside-concrete is potentially too useful.

Especially in game dev where there's definitely a lot more use cases for reflection, I don't think it'd be great to limit reflection as a tool. By "coloring" reflection types, users wouldn't be able to store dynamic data inside a reflectable component.

[MrGVSV]

Another piece to this puzzle might also be improving Reflect::clone_value. I think it would make sense to provide a way for users to register their Clone impls so we can avoid having to always return a PartialReflect (I imagine we could return an enum containing either a Reflect or PartialReflect based on this registration).

This might also help with some of the issues in (3) since part of the problem is also in wanting to take a reference but needing an owned value.

[MrGVSV]

Regarding the DynamicValue solution, I think that would come as a result of Unique Reflect anyways, right? PartialReflect would probably have some partial_reflect_ref method to get an enum containing the various dyn Struct types (btw would we need to use a PartialStruct instead or will Struct only be a subtrait of PartialReflect?).

So I think Unique Reflect still seems like the more "complete" solution.

[Brezak]

Totally unrelated but the link to the rfc in the note under Unique Reflect title has the tld misspelled as .cm instead of .com.

[soqb]

fixed!