Skip to content
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

bevy_reflect: Add casting traits to support Box and other wrapper types #15532

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

MrGVSV
Copy link
Member

@MrGVSV MrGVSV commented Sep 29, 2024

Objective

Closes #3392
Closes #3400
Closes #6098
Closes #9929
Closes #14776

Here we go again...

I won't go into detail about the problem space as it has been covered in all those issues/PRs above (see #14776 for a high-level overview).

Essentially, we want to enable this:

#[derive(Reflect)]
struct Player {
    weapon: Box<dyn Reflect>
}

without directly implementing Reflect for Box<dyn Reflect>, as this makes it very easy to accidentally "double-box" your trait object. And though those double-boxed values would still behave as though it were just the inner type, it would really be potentially dozens of unnecessary pointer dereferences and heap allocations depending on how deep the double-boxing goes.

So what's the solution?

#14776 attempted to solve the problem by using remote reflection and introducing a RemoteBox<T> type, as suggested by @soqb.

While that approach works and saves us from the double-boxing problem, it still has some limitations.

The biggest limitation is the inability to use it as a type parameter to another type (e.g. the T in Foo<T>), since remote reflection doesn't support remotely reflecting a type parameter.

This is a real pain point as it significantly blocks us from supporting things like Vec<Box<dyn Reflect>> and HashMap<String, Box<dyn Reflect>>—arguably some of the most common cases for this feature.

So this PR attempts to support Box<dyn Reflect> without implementing Reflect on it and without using remote reflection.

How? By taking another idea from @soqb!

Solution

This PR introduces two new traits: CastPartialReflect and CastReflect. These are used to cast to dyn PartialReflect and dyn Reflect, respectively. In fact, they're supertraits of those traits!

By introducing these traits, we can alter the behavior of the logic used in the derive macro and custom implementations to rely on them. In other words, rather than requiring a type that is PartialReflect, we require one that casts to PartialReflect.

This means we can support any wrapper type around a reflected type and almost act as if it doesn't exist!

Obviously, this is meant to work with Box<dyn Reflect> and Box<dyn PartialReflect>, but it also supports Box<T: PartialReflect> (if you need that for some reason) as well as Box<dyn CustomTrait>, where CustomTrait: CastPartialReflect.

Future Work

To keep these PRs smaller and tighter in scope, I decided to split this feature across multiple PRs.

This is the primary one, that lays down the groundwork for those to come.

The following PRs may potentially be somewhat controversial, and they may modify the API proposed here in less ergonomic ways in order to achieve their goals.

FromReflect

One thing we may want to do is support FromReflect for these types so their container types can also be FromReflect.

Now we could just implement it and return None or attempt to clone and cast the data (which is also likely to return None), but this almost makes the problem worse: types containing these ones will almost always fail.

Instead, we should look into making ReflectFromReflect available to these types. One way will be through a global TypeRegistry. However, since that may be a ways out, another way is through storing it on TypeInfo, which is the solution I'm planning to look into.

Serialization

With FromReflect solved, we're going to want a way to easily serialize and deserialize these types.

Luckily, #15131 sorta already solved this problem. I'll work on rebasing that PR so it can be reviewed and merged, making support for Box<dyn Reflect> serialization almost trivial.

However, it still needs review. If we don't like the approach or can't agree on the format for the dynamic types, then that affects Box<dyn Reflect> as well (seeing as it is partially a dynamic type).

Modification

Currently, the wrapper type is completely hidden away (apart from TypeInfo and TypeRegistration) once thrown behind a dyn PartialReflect or dyn Reflect. This works fine, but it does mean that there is no way to modify the wrapper itself.

As suggested by @SkiFire13, it would be nice if there was a way to Reflect::set or PartialReflect::apply the Box<dyn Reflect> itself so it's stored value could be changed at runtime.

This is a trickier problem to solve and may require a larger change. I think it might be possible by moving the apply and set methods to these new traits (or rather, introduce new traits for modifying PartialReflect and Reflect types). That should allow wrapper types like Box to control how they are modified.

On top of another big change, this will potentially also affect the ergonomics of supporting custom trait objects. If so, it's not the end of the world, but all of these nuances are better off being explored in a separate PR.

Testing

You can test locally by running:

cargo test --package bevy_reflect --all-features

Showcase

A common occurrence when reflecting a type is wanting to store reflected data within a reflected type. This wasn't possible before, but now it is!

#[derive(Reflect)]
struct Sword {
    damage: u32
}

#[derive(Reflect)]
// Keep in mind that `Box<dyn Reflect>` does not implement `FromReflect`.
// Because of this, we will need to opt-out of the automatic `FromReflect` impl:
#[reflect(from_reflect = false)]
struct Player {
    weapon: Box<dyn Reflect>
}

let player = Player {
    // We can create our `Box<dyn Reflect>` as normal:
    weapon: Box::new(Sword { damage: 100 })
};

// Now we can reflect `weapon`!
let weapon: &dyn Reflect = player.weapon.as_reflect();
assert!(weapon.reflect_partial_eq(&Sword { damage: 100 }).unwrap_or_default());

This works thanks to changes in bevy_reflect that makes the reflection logic now use the new CastPartialReflect and CastReflect traits. This means our fields no longer need to implement Reflect themselves, but can instead delegate their reflection behavior to another type at runtime!

Migration Guide

The Reflect bound has been removed from FromReflect. If you were relying on that bound existing, you will now need to add it yourself:

// BEFORE
impl<T: FromReflect> MyType<T> {/* ... */}

// AFTER
impl<T: FromReflect + Reflect> MyType<T> {/* ... */}

The Reflect bound has been removed from Typed. If you were relying on that bound existing, you will now need to add it yourself:

// BEFORE
impl<T: Typed> MyType<T> {/* ... */}

// AFTER
impl<T: Typed + Reflect> MyType<T> {/* ... */}

The following trait methods have been moved:

  • Reflect::as_reflectCastReflect::as_reflect
  • Reflect::as_reflect_mutCastReflect::as_reflect_mut
  • Reflect::into_reflectCastReflect::into_reflect

Reflect now also requires CastReflect.

@MrGVSV MrGVSV added C-Feature A new feature, making something new possible A-Reflection Runtime information about types D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 29, 2024
@MrGVSV MrGVSV added this to the 0.15 milestone Sep 29, 2024
@MrGVSV MrGVSV added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Sep 29, 2024
Copy link
Contributor

@pablo-lua pablo-lua left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Big PR. I have some questions for now

crates/bevy_reflect/src/from_reflect.rs Show resolved Hide resolved
crates/bevy_reflect/src/impls/smallvec.rs Show resolved Hide resolved
@alice-i-cecile alice-i-cecile added the X-Contentious There are nontrivial implications that should be thought through label Oct 2, 2024
@alice-i-cecile alice-i-cecile modified the milestones: 0.15, 0.16 Oct 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Reflection Runtime information about types C-Feature A new feature, making something new possible D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Needs-Review Needs reviewer attention (from anyone!) to move forward X-Contentious There are nontrivial implications that should be thought through
Projects
Status: In Progress
Development

Successfully merging this pull request may close these issues.

Implement Reflect for Box<dyn Reflect>
3 participants