bevy_reflect: Recursive registration #5781

MrGVSV · 2022-08-24T06:26:00Z

Objective

Resolves #4154

Currently, registration must all be done manually:

#[derive(Reflect)]
struct Foo(Bar);

#[derive(Reflect)]
struct Bar(Baz);

#[derive(Reflect)]
struct Baz(usize);

fn main() {
  // ...
  app
    .register_type::<Foo>()
    .register_type::<Bar>()
    .register_type::<Baz>()
    // .register_type::<usize>() <- This one is handled by Bevy, thankfully
  // ...
}

This can grow really quickly and become very annoying to add, remove, and update as types change. It would be great if we could help reduce the number of types that a user must manually implement themselves.

Solution

As suggested in #4154, this PR adds automatic recursive registration. Essentially, when a type is registered, it may now also choose to register additional types along with it using the new GetTypeRegistration::register_type_dependencies trait method.

The Reflect derive macro now automatically does this for all fields in structs, tuple structs, struct variants, and tuple variants. This is also done for tuples, arrays, Vec<T>, HashMap<K, V>, and Option<T>.

This allows us to simplify the code above like:

#[derive(Reflect)]
struct Foo(Bar);

#[derive(Reflect)]
struct Bar(Baz);

#[derive(Reflect)]
struct Baz(usize);

fn main() {
  // ...
  app.register_type::<Foo>()
  // ...
}

This automatic registration only occurs if the type has not yet been registered. If it has been registered, we simply skip it and move to the next one. This reduces the cost of registration and prevents overwriting customized registrations.

Considerations

While this does improve ergonomics on one front, it's important to look at some of the arguments against adopting a PR like this.

Generic Bounds

Since we need to be able to register the fields individually, we need those fields to implement GetTypeRegistration. This forces users to then add this trait as a bound on their generic arguments. This annoyance could be relieved with something like #5772.

This is no longer a major issue as the Reflect derive now adds the GetTypeRegistration bound by default. This should technically be okay, since we already add the Reflect bound.

However, this can also be considered a breaking change for manual implementations that left out a GetTypeRegistration impl ~~or for items that contain dynamic types (e.g. DynamicStruct) since those also do not implement GetTypeRegistration~~.

Registration Assumptions

By automatically registering fields, users might inadvertently be relying on certain types to be automatically registered. If Foo auto-registers Bar, but Foo is later removed from the code, then anywhere that previously used or relied on Bar's registration would now fail.

Changelog

Added recursive type registration to structs, tuple structs, struct variants, tuple variants, tuples, arrays, Vec<T>, HashMap<K, V>, and Option<T>
Added a new trait in the hidden bevy_reflect::__macro_exports module called RegisterForReflection
Added GetTypeRegistration impl for bevy_render::render_asset::RenderAssetUsages

Migration Guide

All types that derive Reflect will now automatically add GetTypeRegistration as a bound on all (unignored) fields. This means that all reflected fields will need to also implement GetTypeRegistration.

If all fields derive Reflect or are implemented in bevy_reflect, this should not cause any issues. However, manual implementations of Reflect that excluded a GetTypeRegistration impl for their type will need to add one.

#[derive(Reflect)]
struct Foo<T: FromReflect> {
  data: MyCustomType<T>
}

// OLD
impl<T: FromReflect> Reflect for MyCustomType<T> {/* ... */}

// NEW
impl<T: FromReflect + GetTypeRegistration> Reflect for MyCustomType<T> {/* ... */}
impl<T: FromReflect + GetTypeRegistration> GetTypeRegistration for MyCustomType<T> {/* ... */}

crates/bevy_reflect/src/type_registry.rs

jakobhellermann · 2022-09-24T11:50:13Z

examples/reflection/generic_reflection.rs

@@ -13,7 +14,7 @@ fn main() {
 }

 #[derive(Reflect)]
-struct MyType<T: Reflect> {
+struct MyType<T: Reflect + GetTypeRegistration> {


I don't really like that GetTypeRegistration now becomes something people need to think about for generic type parameters. I thought about adding it as a supertrait to Reflect but DynamicX types don't implement it of course.
With bevyengine/rfcs#56 we can add it as a supertrait, right? (with a where Self: Sized bound to keep object safety)

This is why I’d really like #5772 to get merged. It would combine all these core traits into one that does not need to worry about object-safety

But it still introduces another trait. It would be nice if T: Reflect just worked.

Yeah but it means we keep it to two traits that a user must know (Reflect or Reflectable), even as we add more and more core traits.

Separating traits is also about readability imo so if we did combine everything then we'd eventually end up with a potentially massive trait

# Objective Many types in `bevy_render` implemented `Reflect` but were not registered. ## Solution Register all types in `bevy_render` that impl `Reflect`. This also registers additional dependent types (i.e. field types). > Note: Adding these dependent types would not be needed using something like #5781 😉 --- ## Changelog - Register missing `bevy_render` types in the `TypeRegistry`: - `camera::RenderTarget` - `globals::GlobalsUniform` - `texture::Image` - `view::ComputedVisibility` - `view::Visibility` - `view::VisibleEntities` - Register additional dependent types: - `view::ComputedVisibilityFlags` - `Vec<Entity>`

…yengine#6725) Many types in `bevy_render` implemented `Reflect` but were not registered. Register all types in `bevy_render` that impl `Reflect`. This also registers additional dependent types (i.e. field types). > Note: Adding these dependent types would not be needed using something like bevyengine#5781 😉 --- - Register missing `bevy_render` types in the `TypeRegistry`: - `camera::RenderTarget` - `globals::GlobalsUniform` - `texture::Image` - `view::ComputedVisibility` - `view::Visibility` - `view::VisibleEntities` - Register additional dependent types: - `view::ComputedVisibilityFlags` - `Vec<Entity>`

…yengine#6725) # Objective Many types in `bevy_render` implemented `Reflect` but were not registered. ## Solution Register all types in `bevy_render` that impl `Reflect`. This also registers additional dependent types (i.e. field types). > Note: Adding these dependent types would not be needed using something like bevyengine#5781 😉 --- ## Changelog - Register missing `bevy_render` types in the `TypeRegistry`: - `camera::RenderTarget` - `globals::GlobalsUniform` - `texture::Image` - `view::ComputedVisibility` - `view::Visibility` - `view::VisibleEntities` - Register additional dependent types: - `view::ComputedVisibilityFlags` - `Vec<Entity>`

MrGVSV · 2023-03-25T06:41:45Z

We might actually be able to improve the usability here by adding GetTypeRegistration to the where clause of the impl like we do with Reflect 🤔

Tasks

Do this

cart

I just pushed my register() optimization changes. Please review them before merging.

MrGVSV · 2024-03-03T22:15:35Z

crates/bevy_reflect/src/type_registry.rs

+        if self.add_registration(T::get_type_registration()) {
+            T::register_type_dependencies(self);
+        }


@cart Would this possibly incur more TypeId hashes than the previous version?

This will call T::get_type_registration() every time. And most GetTypeRegistration impls will have at least 1 TypeId hash via TypeRegistration::insert (ReflectFromPtr) but more often at least 2 (ReflectFromReflect).

That's the minimum, but most types are registering other type data, such as ReflectComponent, ReflectDefault, ReflectSerialize, etc.

So each call to register now will typically require a few extra hashes rather than just two.

Ew yeah I wasn't thinking about the cost of get_type_registration() at all.

I think this would work:

Rework add_registration to accept TypeId and a fn() -> TypeRegistration. Call the fn only when actually needed internally

Call TypeId::of::<T>() directly in register()

I agree, eagerly computing T::get_type_registration is likely not good for performance. Maybe it would be better to add a variant of add_registration that takes a closure and only calls it if the registration is not already present?

I'll try it out. I think it would make sense to make this a private function so we don't allow add_registration to be abused (i.e. registering types that are not allowed to be registered).

Okay I added register_internal to do just that!

alice-i-cecile · 2024-03-03T23:53:38Z

@MrGVSV say the word and I'll merge this in :) @SkiFire13 or @soqb your review on the last batch of changes would be nice too.

MrGVSV · 2024-03-03T23:54:56Z

@MrGVSV say the word and I'll merge this in :) @SkiFire13 or @soqb your review on the last batch of changes would be nice too.

I mean I’m happy with it haha. We could maybe schedule it to be merged as part of the merge train tomorrow in case anyone has any last minute concerns?

alice-i-cecile · 2024-03-04T00:01:56Z

Great: removing the controversial label for now so I don't miss it 😅

james7132 · 2024-03-04T00:11:43Z

One final thing that came to mind was maybe to annotate register_type_dependencies as #[inline(never)] to avoid excessive code generation at compile time.

MrGVSV · 2024-03-04T01:00:45Z

One final thing that came to mind was maybe to annotate register_type_dependencies as #[inline(never)] to avoid excessive code generation at compile time.

@james7132 Annotate just the default trait method or add it to the generated GetTypeRegistration impls?

james7132 · 2024-03-04T01:31:16Z

@james7132 Annotate just the default trait method or add it to the generated GetTypeRegistration impls?

The empty default is fine, but the ones that have an actual body need to be annotated to avoid inlining the recursion.

SkiFire13 · 2024-03-04T07:38:20Z

LGTM, although the #[inline(never)] feels like premature optimizations. Is it certain that LLVM will inline that method normally (and that this will actually be bad, since it seems it's only used in one function)?

# Objective Resolves bevyengine#4154 Currently, registration must all be done manually: ```rust #[derive(Reflect)] struct Foo(Bar); #[derive(Reflect)] struct Bar(Baz); #[derive(Reflect)] struct Baz(usize); fn main() { // ... app .register_type::<Foo>() .register_type::<Bar>() .register_type::<Baz>() // .register_type::<usize>() <- This one is handled by Bevy, thankfully // ... } ``` This can grow really quickly and become very annoying to add, remove, and update as types change. It would be great if we could help reduce the number of types that a user must manually implement themselves. ## Solution As suggested in bevyengine#4154, this PR adds automatic recursive registration. Essentially, when a type is registered, it may now also choose to register additional types along with it using the new `GetTypeRegistration::register_type_dependencies` trait method. The `Reflect` derive macro now automatically does this for all fields in structs, tuple structs, struct variants, and tuple variants. This is also done for tuples, arrays, `Vec<T>`, `HashMap<K, V>`, and `Option<T>`. This allows us to simplify the code above like: ```rust #[derive(Reflect)] struct Foo(Bar); #[derive(Reflect)] struct Bar(Baz); #[derive(Reflect)] struct Baz(usize); fn main() { // ... app.register_type::<Foo>() // ... } ``` This automatic registration only occurs if the type has not yet been registered. If it has been registered, we simply skip it and move to the next one. This reduces the cost of registration and prevents overwriting customized registrations. ## Considerations While this does improve ergonomics on one front, it's important to look at some of the arguments against adopting a PR like this. #### Generic Bounds ~~Since we need to be able to register the fields individually, we need those fields to implement `GetTypeRegistration`. This forces users to then add this trait as a bound on their generic arguments. This annoyance could be relieved with something like bevyengine#5772.~~ This is no longer a major issue as the `Reflect` derive now adds the `GetTypeRegistration` bound by default. This should technically be okay, since we already add the `Reflect` bound. However, this can also be considered a breaking change for manual implementations that left out a `GetTypeRegistration` impl ~~or for items that contain dynamic types (e.g. `DynamicStruct`) since those also do not implement `GetTypeRegistration`~~. #### Registration Assumptions By automatically registering fields, users might inadvertently be relying on certain types to be automatically registered. If `Foo` auto-registers `Bar`, but `Foo` is later removed from the code, then anywhere that previously used or relied on `Bar`'s registration would now fail. --- ## Changelog - Added recursive type registration to structs, tuple structs, struct variants, tuple variants, tuples, arrays, `Vec<T>`, `HashMap<K, V>`, and `Option<T>` - Added a new trait in the hidden `bevy_reflect::__macro_exports` module called `RegisterForReflection` - Added `GetTypeRegistration` impl for `bevy_render::render_asset::RenderAssetUsages` ## Migration Guide All types that derive `Reflect` will now automatically add `GetTypeRegistration` as a bound on all (unignored) fields. This means that all reflected fields will need to also implement `GetTypeRegistration`. If all fields **derive** `Reflect` or are implemented in `bevy_reflect`, this should not cause any issues. However, manual implementations of `Reflect` that excluded a `GetTypeRegistration` impl for their type will need to add one. ```rust #[derive(Reflect)] struct Foo<T: FromReflect> { data: MyCustomType<T> } // OLD impl<T: FromReflect> Reflect for MyCustomType<T> {/* ... */} // NEW impl<T: FromReflect + GetTypeRegistration> Reflect for MyCustomType<T> {/* ... */} impl<T: FromReflect + GetTypeRegistration> GetTypeRegistration for MyCustomType<T> {/* ... */} ``` --------- Co-authored-by: James Liu <contact@jamessliu.com> Co-authored-by: radiish <cb.setho@gmail.com> Co-authored-by: Carter Anderson <mcanders1@gmail.com>

# Objective - Implements the [Unique Reflect RFC](https://github.com/nicopap/rfcs/blob/bevy-reflect-api/rfcs/56-better-reflect.md). ## Solution - Implements the RFC. - This implementation differs in some ways from the RFC: - In the RFC, it was suggested `Reflect: Any` but `PartialReflect: ?Any`. During initial implementation I tried this, but we assume the `PartialReflect: 'static` in a lot of places and the changes required crept out of the scope of this PR. - `PartialReflect::try_into_reflect` originally returned `Option<Box<dyn Reflect>>` but i changed this to `Result<Box<dyn Reflect>, Box<dyn PartialReflect>>` since the method takes by value and otherwise there would be no way to recover the type. `as_full` and `as_full_mut` both still return `Option<&(mut) dyn Reflect>`. --- ## Changelog - Added `PartialReflect`. - `Reflect` is now a subtrait of `PartialReflect`. - Moved most methods on `Reflect` to the new `PartialReflect`. - Added `PartialReflect::{as_partial_reflect, as_partial_reflect_mut, into_partial_reflect}`. - Added `PartialReflect::{try_as_reflect, try_as_reflect_mut, try_into_reflect}`. - Added `<dyn PartialReflect>::{try_downcast_ref, try_downcast_mut, try_downcast, try_take}` supplementing the methods on `dyn Reflect`. ## Migration Guide - Most instances of `dyn Reflect` should be changed to `dyn PartialReflect` which is less restrictive, however trait bounds should generally stay as `T: Reflect`. - The new `PartialReflect::{as_partial_reflect, as_partial_reflect_mut, into_partial_reflect, try_as_reflect, try_as_reflect_mut, try_into_reflect}` methods as well as `Reflect::{as_reflect, as_reflect_mut, into_reflect}` will need to be implemented for manual implementors of `Reflect`. ## Future Work - This PR is designed to be followed up by another "Unique Reflect Phase 2" that addresses the following points: - Investigate making serialization revolve around `Reflect` instead of `PartialReflect`. - [Remove the `try_*` methods on `dyn PartialReflect` since they are stop gaps](#7207 (comment)). - Investigate usages like `ReflectComponent`. In the places they currently use `PartialReflect`, should they be changed to use `Reflect`? - Merging this opens the door to lots of reflection features we haven't been able to implement. - We could re-add [the `Reflectable` trait](https://github.com/bevyengine/bevy/blob/8e3488c88065a94a4f72199587e59341c9b6553d/crates/bevy_reflect/src/reflect.rs#L337-L342) and make `FromReflect` a requirement to improve [`FromReflect` ergonomics](bevyengine/rfcs#59). This is currently not possible because dynamic types cannot sensibly be `FromReflect`. - Since this is an alternative to #5772, #5781 would be made cleaner. --------- Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com> Co-authored-by: Gino Valente <49806985+MrGVSV@users.noreply.github.com>

MrGVSV added C-Usability A simple quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types X-Controversial There is active debate or serious implications around merging this PR labels Aug 24, 2022

MrGVSV force-pushed the reflect-auto-registration branch 2 times, most recently from 82108df to 5bc61bb Compare August 24, 2022 21:08

MrGVSV marked this pull request as ready for review August 24, 2022 21:34

MrGVSV force-pushed the reflect-auto-registration branch from 5bc61bb to 9dd48e1 Compare August 28, 2022 05:39

mockersf mentioned this pull request Sep 22, 2022

add test checking that types are registered (as far as possible) #6061

Open

jakobhellermann reviewed Sep 24, 2022

View reviewed changes

MrGVSV force-pushed the reflect-auto-registration branch from 9dd48e1 to 0cc73be Compare October 12, 2022 05:41

MrGVSV force-pushed the reflect-auto-registration branch from 0cc73be to 162c3d0 Compare November 10, 2022 06:43

MrGVSV mentioned this pull request Nov 22, 2022

[Merged by Bors] - bevy_reflect: Register missing reflected types for bevy_render #6725

Closed

MrGVSV force-pushed the reflect-auto-registration branch from 79fa0d6 to 813e889 Compare December 3, 2022 03:22

MrGVSV force-pushed the reflect-auto-registration branch from 813e889 to 196b4d8 Compare December 18, 2022 21:30

soqb mentioned this pull request Jan 22, 2023

reflect: implement the unique reflect rfc #7207

Merged

MrGVSV force-pushed the reflect-auto-registration branch from 196b4d8 to 3ac6285 Compare February 7, 2023 05:33

MrGVSV force-pushed the reflect-auto-registration branch from 3ac6285 to a2798d7 Compare March 25, 2023 06:37

MrGVSV mentioned this pull request Apr 17, 2023

Scene serialization doesn't work without field types being registered #8415

Closed

james7132 mentioned this pull request Apr 17, 2023

Register some extra types to type registry #8430

Merged

MrGVSV force-pushed the reflect-auto-registration branch from 3541b65 to 2212a96 Compare April 17, 2023 19:57

james7132 self-requested a review April 17, 2023 20:06

MrGVSV force-pushed the reflect-auto-registration branch 2 times, most recently from a857552 to f40347e Compare April 21, 2023 04:44

Remove redundant hashing in register()

726857f

cart approved these changes Mar 3, 2024

View reviewed changes

MrGVSV commented Mar 3, 2024

View reviewed changes

Add TypeRegistry::register_internal

e9faa79

alice-i-cecile approved these changes Mar 3, 2024

View reviewed changes

alice-i-cecile added this pull request to the merge queue Mar 3, 2024

alice-i-cecile removed this pull request from the merge queue due to a manual request Mar 3, 2024

alice-i-cecile removed the X-Controversial There is active debate or serious implications around merging this PR label Mar 4, 2024

Add #[inline(never)]

a14793a

SkiFire13 approved these changes Mar 4, 2024

View reviewed changes

soqb approved these changes Mar 4, 2024

View reviewed changes

alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Mar 4, 2024

alice-i-cecile added this pull request to the merge queue Mar 4, 2024

Merged via the queue into bevyengine:main with commit ccb9d05 Mar 4, 2024
24 of 26 checks passed

MrGVSV deleted the reflect-auto-registration branch March 4, 2024 21:52

s-puig mentioned this pull request Mar 10, 2024

CI test mismatch on local runner #12404

Closed

This was referenced May 30, 2024

Write release notes for PR #5781: bevy_reflect: Recursive registration bevyengine/bevy-website#1236

Closed

Write release notes for PR #5781: bevy_reflect: Recursive registration bevyengine/bevy-website#1256

Closed

eugineerd mentioned this pull request Sep 3, 2024

Reflect auto registration #15030

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bevy_reflect: Recursive registration #5781

bevy_reflect: Recursive registration #5781

MrGVSV commented Aug 24, 2022 •

edited

Loading

jakobhellermann Sep 24, 2022

MrGVSV Sep 24, 2022

jakobhellermann Sep 24, 2022

MrGVSV Sep 24, 2022 •

edited

Loading

MrGVSV commented Mar 25, 2023 •

edited

Loading

cart left a comment

MrGVSV Mar 3, 2024

cart Mar 3, 2024 •

edited

Loading

SkiFire13 Mar 3, 2024

MrGVSV Mar 3, 2024

MrGVSV Mar 3, 2024

alice-i-cecile commented Mar 3, 2024 •

edited

Loading

MrGVSV commented Mar 3, 2024

alice-i-cecile commented Mar 4, 2024

james7132 commented Mar 4, 2024

MrGVSV commented Mar 4, 2024 •

edited

Loading

james7132 commented Mar 4, 2024

SkiFire13 commented Mar 4, 2024

bevy_reflect: Recursive registration #5781

bevy_reflect: Recursive registration #5781

Conversation

MrGVSV commented Aug 24, 2022 • edited Loading

Objective

Solution

Considerations

Generic Bounds

Registration Assumptions

Changelog

Migration Guide

jakobhellermann Sep 24, 2022

Choose a reason for hiding this comment

MrGVSV Sep 24, 2022

Choose a reason for hiding this comment

jakobhellermann Sep 24, 2022

Choose a reason for hiding this comment

MrGVSV Sep 24, 2022 • edited Loading

Choose a reason for hiding this comment

MrGVSV commented Mar 25, 2023 • edited Loading

Tasks

cart left a comment

Choose a reason for hiding this comment

MrGVSV Mar 3, 2024

Choose a reason for hiding this comment

cart Mar 3, 2024 • edited Loading

Choose a reason for hiding this comment

SkiFire13 Mar 3, 2024

Choose a reason for hiding this comment

MrGVSV Mar 3, 2024

Choose a reason for hiding this comment

MrGVSV Mar 3, 2024

Choose a reason for hiding this comment

alice-i-cecile commented Mar 3, 2024 • edited Loading

MrGVSV commented Mar 3, 2024

alice-i-cecile commented Mar 4, 2024

james7132 commented Mar 4, 2024

MrGVSV commented Mar 4, 2024 • edited Loading

james7132 commented Mar 4, 2024

SkiFire13 commented Mar 4, 2024

MrGVSV commented Aug 24, 2022 •

edited

Loading

MrGVSV Sep 24, 2022 •

edited

Loading

MrGVSV commented Mar 25, 2023 •

edited

Loading

cart Mar 3, 2024 •

edited

Loading

alice-i-cecile commented Mar 3, 2024 •

edited

Loading

MrGVSV commented Mar 4, 2024 •

edited

Loading