bevy_reflect: `TypeData` and registration callbacks by MrGVSV · Pull Request #9777 · bevyengine/bevy

MrGVSV · 2023-09-12T04:32:30Z

Objective

Background

Many libraries are powered by Bevy's reflection crate. Oftentimes, they will define what's known as "type data" in order to call trait methods in a dynamic context.

For example:

trait UiButton {
  fn on_press(&self, ui: &mut UiContext);
}

#[derive(Clone)]
struct ReflectUiButton {
  on_press: fn(&dyn Reflect, &mut UiContext)
}

impl<T: Reflect + UiButton> FromType<T> for ReflectUiButton {
  fn from_type() -> Self {
    Self {
      on_press: |value, ui| {
        value.downcast_ref::<T>.unwrap().on_press(ui);
      }
    }
  }
}

Users can then register this type data on their types like so:

#[derive(Reflect, UiButton)]
#[reflect(UiButton)]
struct MyButton;

Now when we register MyButton, it will automatically register ReflectUiButton.

Problem

While this is great for simple type data, it's not always enough for more complex traits and type data.

For example, let's modify the UiButton trait so that we can save and load its state. We'll do this by specifying an associated type which we can serialize into and deserialize from.

trait UiButton {
  type State: FromReflect

  // ...
}

Now the user needs to not only register their MyButton type, but also the type they use for State.

As the library author, we can slightly reduce this pain point by creating a helper function or extension trait:

trait UiButtonAppExt {
  fn register_ui_button<T: UiButton>(&mut self) -> &mut Self;
}

impl UiButtonAppExt for App {
  fn register_ui_button<T: UiButton>(&mut self) -> &mut Self {
    self.register_type::<T>()
      .register_type_data::<T, ReflectUiButton>()
      .register_type::<T::State>()
      // We may also need to register other types as well:
      .register_type::<Option<T::State>>()
      .register_type::<UiButtonManager<T>>()
  }
}

While this works, it isn't very clean and requires a bit more effort on both the library author and the user. It would be nicer if we could inject these hidden registrations into the registration for ReflectUiButton.

Solution

Added a callback for when type data is registered. This allows the type data to register other kinds of type data when it itself is registered.

This callback is added to FromType, which has been renamed TypeData to make it clear what it's for.

Now we can define our ReflectUiButton type data like so:

impl<T: Reflect + UiButton> TypeData<T> for ReflectUiButton {
  fn create_type_data() -> Self {
    Self {
      on_press: |value, ui| {
        value.downcast_ref::<T>.unwrap().on_press(ui);
      }
    }
  }

  fn on_register(registry: &mut TypeRegistry) {
    registry
      .register_type_data::<T, ReflectUiButton>()
      .register::<T::State>()
      .register::<Option<T::State>>()
      .register::<UiButtonManager<T>>()
  }
}

The associated on_register function will be called whenever MyButton is registered (or whenever ReflectUiButton is manually registered via register_type_data).

`TypeRegistration`

One issue with this approach is that users can currently get a mutable reference to the TypeRegistration and insert data that way. Unfortunately, this means we can't automatically dispatch the on_register callbacks when that happens.

To solve this issue, this PR makes it so that adding type data on a TypeRegistration requires ownership of that registration. This changes how type data can be added to TypeRegistration and also increases the number of hash lookups needed (since, presumably, users can now only manually add new type data via TypeRegistry::register_type_data).

Because of this...

Should we keep this behavior? Should we revert back to the original and just leave a note telling implementors to manually dispatch these callbacks? Any other solutions?

Another option would be to have a dedicated TypeRegistrationMut which blocks access to those methods and then add a TypeRegistry::scope method to access the full &mut TypeRegistration.

Changelog

Renamed trait FromType<T> to TypeData<T>
- Renamed method FromType::<T>::from_type to TypeData::<T>::create_type_data
- Added method TypeData::<T>::on_register
Type data can now register registration callbacks
TypeRegistration::insert split into TypeRegistration::insert and TypeRegistration::register
- Both TypeRegistration::insert and TypeRegistration::register take ownership

Migration Guide

FromType<T> and has been renamed to TypeData<T> along with its methods. Implementors and callers will need to update their code.

// BEFORE
impl<T: Reflect + Foo> FromType<T> for ReflectFoo {
  fn from_type() -> Self {
    // ...
  }
}

// AFTER
impl<T: Reflect + Foo> TypeData<T> for ReflectFoo {
  fn create_type_data() -> Self {
    // ...
  }
}

// BEFORE
let data = <ReflectFoo as FromType<MyStruct>>::from_type();

// AFTER
let data = <ReflectFoo as TypeData<MyStruct>>::create_type_data();

Additionally, TypeRegistration::insert has been split into two methods: TypeRegistration::insert and TypeRegistration::register. TypeRegistration::register is the preferred way of registering data. Both methods now also require ownership of the TypeRegistration.

// BEFORE
let mut registration = TypeRegistration::of::<MyStruct>();
registration.insert<ReflectFoo>(FromType::<MyStruct>::from_type());
registration.insert(SomeOtherTypeData::new());

// AFTER
let registration = TypeRegistration::of::<MyStruct>()
  .register<MyStruct, ReflectFoo>()
  .insert(SomeOtherTypeData::new(), None);

nicopap

I'm doubtful the current implementation is any improvement.

Splitting FromType into its own module and adding an example is great (although the example itself isn't so), but the rest of the proposed change adds complexity with benefits that are not proportional.

nicopap · 2023-09-12T06:38:19Z

+///
+/// [`TypeRegistration`]: crate::TypeRegistration
+/// [crate-level documentation]: crate
+pub trait BaseTypeData: Downcast + Send + Sync {


I prefer the old naming of TypeData for this and FromType for what is called "TypeData" now. I was very confused when reviewed and not being familiar with the old names.

IMO it doesn't make sense to call this "BaseTypeData"

FromType<T>: expresses it's an interface to get something from T

TypeData: expresses it's some data, usually meant to be stored.

BaseTypeData is a bad name.

Yeah I really don't like BaseTypeData haha. If you have any other names, I'm all ears!

For me, the issue is that FromType feels disconnected from the concept of type data. If a user saw a FromType impl in code, they likely wouldn't know it's related unless they have prior knowledge that type data is created using FromType.

And the current TypeData (aka BaseTypeData) is also a little odd. We talk about TypeData a lot in regards to reflection, but the eponymous trait is not one you can implement. The only time a user ever interacts with it is seeing it as a type param in the registration methods.

So my reasoning for the naming changes was to try and solve both these issues. TypeData is the trait you implement when you want something to be "type data" (generally). We'll be able to directly point to that trait and users unfamiliar with FromType should have an easier time understanding the point of those impls.

i agree with @nicopap that TypeData nicely encapsulates what BaseTypeData represents. perhaps a better name for the new 'BaseTypeData' is TypeDataFactory?

Hm, TypeDataFactory sorta implies that it's what creates the type data. This is basically just an object-safe Clone trait.

What about one of these?

CloneableTypeData

GenericTypeData

AnyTypeData

TypeDataValue (I think this one is my favorite)

DynamicTypeData

sorry that's a typo on my part. i meant to suggest TypeData -> TypeDataFactory, BaseTypeData -> TypeData.

Oh gotcha. Then yeah I think I'd be okay with that naming convention. I'll go ahead with that unless @nicopap (or anyone else) has any other ideas!

nicopap · 2023-09-12T06:43:37Z

+/// trait Animal {
+///   fn speak(&self) -> &'static str;
+/// }


Can we use something else than the notoriously misleading OOP inheritance example?

Something that would make sense to use trait reflection for.

As a hint, the rust book's chapter on trait: https://doc.rust-lang.org/stable/book/ch10-02-traits.html. Though I think in bevy we should use a game-oriented example

Haha I don't think it's that bad (Rust by Example uses it), but I can understand why we might want to avoid it. I'll replace it with a game-oriented example like you suggested!

i don't mind this example because its recognizable and very simple but it really annoys me that it uses 2 spaces instead of 4.

nicopap · 2023-09-12T07:00:39Z

+    /// A queue of callbacks to dispatch whenever this registration is registered
+    /// into a [`TypeRegistry`].
+    register_queue: Vec<fn(&mut TypeRegistry)>,


Why store each on_register callbacks into the registration?

Wouldn't it be possible to:

amend the Reflect macro to call .on_register() with the reflected traits

amend TypeRegistry::register_type_data to call it?

Now we don't need this awkward handling of callbacks.

I don't like at all this solution. It makes very difficult to trace execution of code:

How many times and when is this callback ran?

Several callbacks on the same registration can behave in very confusing ways

Yeah this is the part I was struggling with because it definitely is super awkward. We could definitely just add it to those two places. It just means that on_register isn't "guaranteed" to be called (which might be something we just accept).

And the reason for that is because a user could do registry.get_mut(type_id).unwrap().insert::<ReflectFoo>(FromType::<MyStruct>::from_type()) without realizing they need to call the on_register function afterwards.

@soqb do you have any thoughts on this? I think I agree with @nicopap that the best solution might be to just modify the callers rather than the registration API. It's probably not often someone is manually editing the TypeRegistration directly anyways, and we can leave a note in the documentation that on_register should be called for TypeData.

Hm, I was beginning to implement this when I ran into a problem: GetTypeRegistration doesn't have access to the registry (so we can't call this function in the Reflect derive). We could add it as a parameter, but that starts to encroach on the work done in #5781.

Maybe this needs to happen after that PR gets merged?

i don't think moving the on_register calls into the derived macro is really better. it's absolutely conceivable that someone would build their own TypeRegistration and would not think to call on_register because that's not how these kinds of api tend to work.

however it's also true that backtraces and quite possibly performance are left on the table because of the dynamic way we call the hooks.

i think the cleanest solution would involve simplifying the GetTypeRegistration trait with a total redesign like the following:

trait Registerable { fn register(registry: &mut TypeRegistry); }

but that's a pretty sweeping change that should probably be left until both this PR and the recursive registration implementation, so i'm fine to leave this PR's implementation the way it was initially implemented, @MrGVSV

i don't think moving the on_register calls into the derived macro is really better. it's absolutely conceivable that someone would build their own TypeRegistration and would not think to call on_register because that's not how these kinds of api tend to work.

Yeah this was my original concern and why I made insert take self, so as to prevent users from accidentally not calling on_register after registration.

think the cleanest solution would involve simplifying the GetTypeRegistration trait with a total redesign like the following:

trait Registerable { fn register(registry: &mut TypeRegistry); }

Yeah, I generally agree. The recursive registration PR actually adds something like this. It keeps the current get_type_registration method, but also adds a fn register_type_dependencies(registry: &mut TypeRegistry) method.

If we wanted to move the on_register call into the GetTypeRegistration impl, that would probably be a good place to do so.

But again, this means we leave the API in a position where users could register type data without calling on_register. While I think that's okay, perhaps a better solution would be rethinking how we create, manage, and expose a TypeRegistration in the first place.

nicopap · 2023-09-12T07:02:36Z

+    ///
+    /// [`TypeRegistration`]: crate::TypeRegistration
+    #[allow(unused_variables)]
+    fn on_register(registry: &mut TypeRegistry) {}


What about renaming on_register to register and accept a self and the default impl just calls registry.register_type_data

i think perhaps the signature should be fn(&self, registry: &mut TypeRegistry), but i think taking self would make this API quite unpredictable. currently, its relationship with the methods on the type registry is clearly defined, and i wouldn't want to muddy the waters.

I don't think the default impl should call registry.register_type_data, since that method will likely need to call this one.

MrGVSV · 2023-09-12T08:38:05Z

I'm doubtful the current implementation is any improvement.

Splitting FromType into its own module and adding an example is great (although the example itself isn't so), but the rest of the proposed change adds complexity with benefits that are not proportional.

Yeah I think that's fair. It seems like the only way this seems worth doing is if we barely change the logic of TypeRegistry and TypeRegistration. The changes there are a little awkward and seem to do more harm than good.

I do think the motivation behind this PR is good, though. It's mainly for reducing clutter in library APIs, where the alternatives are to force consumers to register additional types manually or force them to register their types through a separate app method.

It's a small benefit, but one that could be worth it (if we can find a way to clean up the logic here haha).

soqb

i like the new behavior and consistency changes a lot, but i think there's still some kinks to iron out.

soqb · 2023-09-12T15:54:39Z

+///
+/// [`TypeRegistration`]: crate::TypeRegistration
+/// [crate-level documentation]: crate
+pub trait BaseTypeData: Downcast + Send + Sync {


i agree with @nicopap that TypeData nicely encapsulates what BaseTypeData represents. perhaps a better name for the new 'BaseTypeData' is TypeDataFactory?

soqb · 2023-09-12T15:56:54Z

+/// trait Animal {
+///   fn speak(&self) -> &'static str;
+/// }


i don't mind this example because its recognizable and very simple but it really annoys me that it uses 2 spaces instead of 4.

soqb · 2023-09-12T16:07:48Z

+    ///
+    /// [`TypeRegistration`]: crate::TypeRegistration
+    #[allow(unused_variables)]
+    fn on_register(registry: &mut TypeRegistry) {}


i think perhaps the signature should be fn(&self, registry: &mut TypeRegistry), but i think taking self would make this API quite unpredictable. currently, its relationship with the methods on the type registry is clearly defined, and i wouldn't want to muddy the waters.

soqb · 2023-09-12T16:18:07Z

-    pub fn insert<T: TypeData>(&mut self, data: T) {
-        self.data.insert(TypeId::of::<T>(), Box::new(data));
+    /// If another instance of `D` was previously inserted, it is replaced.
+    pub fn register<T: 'static, D: TypeData<T>>(self) -> Self {


i dislike these signature changes quite a lot. i would suggest instead we make the signature fn<T: TypeData>(&mut self, data: T) -> &mut Self so that chaining operations still works. this is what bevy_app::App does as well.

By this, do you mean that we should revert the self change but keep the return Self change (as a &mut Self instead)?

MrGVSV · 2026-06-02T20:34:48Z

Closing in favor of #24518

Modified TypeData logic

2a96eed

MrGVSV added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Sep 12, 2023

MrGVSV marked this pull request as ready for review September 12, 2023 04:36

nicopap reviewed Sep 12, 2023

View reviewed changes

soqb reviewed Sep 12, 2023

View reviewed changes

BenjaminBrienen added D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jan 23, 2025

cart closed this May 5, 2026

cart reopened this May 5, 2026

MrGVSV mentioned this pull request Jun 2, 2026

bevy_reflect: Type data registration callbacks #24518

Open

MrGVSV closed this Jun 2, 2026

Uh oh!

Conversation

MrGVSV commented Sep 12, 2023

Objective

Background

Problem

Solution

TypeRegistration

Changelog

Migration Guide

Uh oh!

nicopap left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MrGVSV commented Sep 12, 2023

Uh oh!

soqb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MrGVSV commented Jun 2, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

`TypeRegistration`