bevy_reflect: `FromReflect` Ergonomics Implementation

[MrGVSV]

Objective

This is a proof-of-concept implementation of https://github.com/bevyengine/rfcs/pull/59.

It is currently in draft form as the RFC is not merged yet. However, reviews are very much welcomed as that will help inform the future of the RFC and get this closer to being merged!

---

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 #[from_reflect(auto_derive = false)] attribute.

#[derive(Reflect)]
struct Foo;

#[derive(Reflect)]
#[from_reflect(auto_derive = false)]
struct Bar;

fn test<T: FromReflect>(value: T) {}

test(Foo); // <-- OK
test(Bar); // <-- Panic! Bar does not implement trait `FromReflect`

ReflectFromReflect

This PR introduces the ReflectFromReflect type data for dynamic FromReflect conversions. This was previously explored in #4147, but was abandoned as it was a pain to use. However, now that we control FromReflect as part of the Reflect derive, we can easily register ReflectFromReflect without sacrificing ergonomics.

This type data allows the FromReflect conversion to occur without having access to the concrete type.

let obj: &dyn Reflect = /* ... */;
let rfr = registry.get_type_data::<ReflectFromReflect>(type_id).unwrap();

// We don't know the concrete type of `obj`, but can still convert it so that
// we get back a `Box<dyn Reflect>` containing the Real type rather than the Dynamic one
let output: Box<dyn Reflect> = rfr.from_reflect(obj).unwrap();

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 the Box'd Real type.

[Un]TypedReflectDeserializer::new_dynamic will work like what we have now and simply return the Box'd Dynamic type.

// Returns the Real type
let reflect_deserializer = UntypedReflectDeserializer::new(&registry);
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(&registry);
let mut deserializer = ron::de::Deserializer::from_str(input)?;

let output: DynamicStruct = reflect_deserializer.deserialize(&mut deserializer)?.take()?;

---

Changelog

• FromReflect is now automatically derived within the Reflect derive macro
• Added ReflectFromReflect type data
• Renamed TypedReflectDeserializer::new and UntypedReflectDeserializer::new to TypedReflectDeserializer::new_dynamic and UntypedReflectDeserializer::new_dynamic, respectively
• Changed TypedReflectDeserializer::new and UntypedReflectDeserializer::new to automatically convert the deserialized output using FromReflect

Migration Guide

• FromReflect is now automatically derived within the Reflect derive macro. Items with both derives will need to remove the FromReflect one.

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

  // OLD
  #[derive(Reflect)]
  struct Foo;
  
  impl FromReflect for Foo {/* ... */}
  
  // NEW
  #[derive(Reflect)]
  #[from_reflect(auto_derive = false)]
  struct Foo;
  
  impl FromReflect for Foo {/* ... */}
  

• 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>).

  let reflect_deserializer = UntypedReflectDeserializer::new_dynamic(&registry);
  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:

  // OLD
  let reflect_deserializer = UntypedReflectDeserializer::new(&registry);
  
  // NEW
  let reflect_deserializer = UntypedReflectDeserializer::new_dynamic(&registry);
  

[MrGVSV]

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 #[reflect(from_reflect = false)] or #[reflect(derive_from_reflect = false)]?

[tguichaoua]

That's a good point. The only thing is it's a bit long haha. Maybe #[reflect(from_reflect = false)] or #[reflect(derive_from_reflect = false)]?

The shorter the better. IMHO, #[reflect(from_reflect = false)] is clear enough if we consider the whole code (with the derive attribute just above) and if this is mentioned in the doc of Reflect and FromReflect.

[soqb]

what about #[reflect(not(FromReflect))] (like cfg attribute) or (my favourite) #[reflect(!FromReflect)] (like negative impls)

[Zeenobit]

I've been looking at this PR and #6245 and been reading the Related RFC and I'm not a fan of automatically deriving FromReflect. While I agree with you that it's very useful, I specifically don't agree with this stance:

Chances are, if a type implements Reflect, it should also be implementing FromReflect.

From experience, the only time FromReflect is required is when the user is dealing with nested or generic types. Both of these cases are not (hopefully should not) be common in higher level game logic. Rather, it should be reserved for mid/low-level game systems, and in those cases, using #[derive(FromReflect)] helps readability by declaring intent.

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.

[MrGVSV]

I've been looking at this PR and #6245 and been reading the Related RFC and I'm not a fan of automatically deriving FromReflect. While I agree with you that it's very useful, I specifically don't agree with this stance:

Chances are, if a type implements Reflect, it should also be implementing FromReflect.

From experience, the only time FromReflect is required is when the user is dealing with nested or generic types. Both of these cases are not (hopefully should not) be common in higher level game logic. Rather, it should be reserved for mid/low-level game systems, and in those cases, using #[derive(FromReflect)] helps readability by declaring intent.

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 #[reflect_trait]), and certain other type data. We can't take it's value as the concrete type. We also can't put non-FromReflect types in Vec<T>, HashMap<K, V>, or any enum and expect Reflect::apply to work like it does for other types.

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 FromReflect overall.

[Zeenobit]

Have you considered gating this behind a feature? i.e. only auto derive FromReflect if feature = "xyz".

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 #[reflect(from_reflect = false)] as well.

[MrGVSV]

Have you considered gating this behind a feature? i.e. only auto derive FromReflect if feature = "xyz".

Hm, that might be a fair compromise 🤔

I mean, this PR doesn't enforce that FromReflect exists on everything anyway, so we could potentially put it behind a feature without breaking any "guarantees". So would you be opposed to making this a default feature?

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 FromReflect heavily behind the scenes.

This would eliminate the need for #[reflect(from_reflect = false)] as well.

I don't think it would since you may want to have everything derive FromReflect automatically except for a handful of types (e.g. ones that are unable to derive FromReflect).

[Zeenobit]

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:

1. I do agree with you that we might still need #[reflect(from_reflect = false)], for the exceptional cases where the feature is enabled on the crate, but user still wants to override the auto-derive, or opt-out. So then if we still need to implement this, then we're still adding the complexity, and the crate feature support would be even more complexity in addition to that.

2. I'm having a hard time thinking of the cost of deriving FromReflect for all reflected types to the user. If it's there, and the user isn't using it, is it really hurting anyone?

So yah, thinking this over again, your approach is probably the ideal approach.

[MrGVSV]

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? ❓

[tim-blackbird]

If this is put behind a feature it should definitely be enabled by default. Scenes are a core pillar of game development. Even games that are heavily procedural use scenes for rooms/prefabs.

If it is behind a feature, disabling the feature is likely to prevent that project from making use of the future Bevy editor.

[MrGVSV]

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 FromReflect (or ReflectDefault but that's a lot more restrictive).

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)

[raffaeleragni]

Hi, I am currently trying to deserialize some bevy native types (eg Transform) but they are lacking ReflectFromReflect. What is the status of this, is it planned for the next version?

[MrGVSV]

Update June 3, 2023

What happened to the RFC?

After some thought, I'm closing out the RFC in favor of this PR.

The RFC was meant to gain feedback about the design around improving the user experience vis-a-vis FromReflect. However, the implementation was fairly straightforward— with ReflectFromReflect even getting merged in its own PR (https://github.com/bevyengine/bevy/pull/6245).

Because of the simplicity of the implementation, it makes more sense to continue the discussion here, with real-life code that can be tested and reviewed.

In terms of learnings for other devs, I would suggest choosing wisely between creating an RFC or a Discussion. Since the RFC was meant more as a means for discussing concerns and possible design choices, and lacked the need for a complex implementation strategy, it probably would have made more sense as a Discussion post. This would have allowed discussion to go as long as needed before the implementation without adding another item that needed to be merged.

What changed in this rebase?

Deserialization

In order to keep the focus on merging the FromReflect derive, I've removed commits related to the final goal of improving the deserializer with ReflectFromReflect. I will create a followup PR when/if this one gets merged that will reimplement that logic.

Bounds

One thing that changed since this was first implemented, was the addition of new bounds to the where clause of the generated impls. The rebased changes now make full use of this updated logic to enforce that all fields have the necessary trait bounds.

For example, all active fields must now implement FromReflect unless the container opts out of the automatic FromReflect derive, in which case the bound for all fields is broadened to just Reflect. Additionally, all ignored fields are now given the bound of Default unless the container opts out of the automatic FromReflect derive.

Note that these changes are effectively the same as if we were deriving FromReflect separately. We are now just adding the trait bounds in the where clause instead of relying on their usage within the generated impls so that we can enforce these requirements at the definition (rather than at the usages where the author may not even be the one to hit the compiler error).

Example

#[derive(Reflect)]
struct Foo<T, U> {
  a: T,
  #[reflect(ignore)]
  b: U
}

error[E0277]: the trait bound `U: Default` is not satisfied
   --> crates/bevy_reflect/src/lib.rs:755:18
    |
755 |         #[derive(Reflect)]
    |                  ^^^^^^^ the trait `Default` is not implemented for `U`
    |
note: required for `tests::from_reflect_should_use_default_variant_field_attributes::Foo<T, U>` to implement `from_reflect::FromReflect`

Opt-out Attribute

Lastly, I went ahead and changed the syntax for opting out from #[from_reflect(auto_derive = false)] to #[reflect(from_reflect = false)].

It's shorter, clearer, and removes a mental split between #[reflect] and #[from_reflect] attributes.

I haven't yet put this behind a feature flag. I agree that, with the growth of scenes and editor support, this will almost always be desired. And the Unique Reflect work (https://github.com/bevyengine/rfcs/pull/56) may even require it. So I'd like to hear more thoughts on whether or not we should put this behind a feature.

[MrGVSV]

@soqb did bring up some good points, but I think those can be addressed in followups.

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)