Recursive Type Registration #4154

james7132 · 2022-03-08T17:09:41Z

What problem does this solve or what need does it fill?

Currently registering a type only creates a registration for itself. Registering a type will not register the types of its fields. This means every intermediate field type must be manually registered. This would make deserialization difficult to manage for deeply nested structures.

What solution would you like?

Type registration should be recursive. Upon registering a type, it's field types should be registered. This should continue recursively until a value type has been reached, or if a type had already been registered.

This may increase app load times, but provide a much better developer UX when using the TypeRegistry to handle serialization.

What alternative(s) have you considered?

Register every type manually.

alice-i-cecile · 2022-03-08T17:13:20Z

I'm strongly in favor of this: this will improve ergonomics significantly.

It's also required for automatic registration of component and resource types to work properly.

james7132 · 2022-03-08T17:16:14Z

We could also likely remove the default type registrations in bevy_core too, since they would be transitively registered by any type that needs them.

MrGVSV · 2022-03-08T17:35:30Z

Also in favor of this. Just as long as the duplication logic is handled in the registry itself and not by the macro (if that was even an option haha). I just know users— including myself— are going to be accidentally re-registering things that got picked up by a field registration.

james7132 · 2022-03-09T02:43:24Z

This is probably doable as a follow-up to #4042 by just inverting the flow of control of GetTypeRegistration to have a type register itself and its fields into a TypeRegistry.

cart · 2022-03-09T21:16:08Z

I'm open to considering this, especially if we can show that this doesn't meaningfully affect startup times in complicated apps. Only recursing when a type isn't registered yet will avoid truly bad time complexity, but this still forces an extra hashmap lookup of every type id of every field of every registered struct.

james7132 · 2022-03-09T21:33:36Z

Ideally we could/should do this at compile time, but both const Any/TypeId functions both aren't const yet, nor is inventory available for this kind of use.

I'm also not convinced that repeatedly fetching the TypeRegistry resource, getting a write lock, and inserting registrations for each type is going to be any faster than doing a HashMap::contains_key check for every field in a type, which would eventually amortize to a one layer check since most of the constituent fields will already have been registered.

) # Objective Reflected tuples do not implement `GetTypeRegistration`, preventing us from registering our tuples, like: ```rust app.register_type::<(i32, i32)>(); ``` This is especially important for things like using #4042 to improve the scene format or implementing #4154 to recursively register fields. ## Solution Added an implementation to the tuple macro: ```rust impl<$($name: Reflect + for<'de> Deserialize<'de>),*> GetTypeRegistration for ($($name,)*) { fn get_type_registration() -> TypeRegistration { let mut registration = TypeRegistration::of::<($($name,)*)>(); registration.insert::<ReflectDeserialize>(FromType::<($($name,)*)>::from_type()); registration } } ``` This requires that the tuple's types implement `Deserialize`. This is exactly how `Vec` and `HashMap` handle it: ```rust impl<T: FromReflect + for<'de> Deserialize<'de>> GetTypeRegistration for Vec<T> { fn get_type_registration() -> TypeRegistration { let mut registration = TypeRegistration::of::<Vec<T>>(); registration.insert::<ReflectDeserialize>(FromType::<Vec<T>>::from_type()); registration } } ```

…vyengine#4226) # Objective Reflected tuples do not implement `GetTypeRegistration`, preventing us from registering our tuples, like: ```rust app.register_type::<(i32, i32)>(); ``` This is especially important for things like using bevyengine#4042 to improve the scene format or implementing bevyengine#4154 to recursively register fields. ## Solution Added an implementation to the tuple macro: ```rust impl<$($name: Reflect + for<'de> Deserialize<'de>),*> GetTypeRegistration for ($($name,)*) { fn get_type_registration() -> TypeRegistration { let mut registration = TypeRegistration::of::<($($name,)*)>(); registration.insert::<ReflectDeserialize>(FromType::<($($name,)*)>::from_type()); registration } } ``` This requires that the tuple's types implement `Deserialize`. This is exactly how `Vec` and `HashMap` handle it: ```rust impl<T: FromReflect + for<'de> Deserialize<'de>> GetTypeRegistration for Vec<T> { fn get_type_registration() -> TypeRegistration { let mut registration = TypeRegistration::of::<Vec<T>>(); registration.insert::<ReflectDeserialize>(FromType::<Vec<T>>::from_type()); registration } } ```

# Objective Resolves #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 #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 #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 <[email protected]> Co-authored-by: radiish <[email protected]> Co-authored-by: Carter Anderson <[email protected]>

# Objective Fix #12304. Remove unnecessary type registrations thanks to #4154. ## Solution Conservatively remove type registrations. Keeping the top level components, resources, and events, but dropping everything else that is a type of a member of those types.

# 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 <[email protected]> Co-authored-by: radiish <[email protected]> Co-authored-by: Carter Anderson <[email protected]>

# Objective Fix bevyengine#12304. Remove unnecessary type registrations thanks to bevyengine#4154. ## Solution Conservatively remove type registrations. Keeping the top level components, resources, and events, but dropping everything else that is a type of a member of those types.

# 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 <[email protected]> Co-authored-by: radiish <[email protected]> Co-authored-by: Carter Anderson <[email protected]>

# Objective When deriving `Reflect`, users will notice that their generic arguments also need to implement `Reflect`: ```rust #[derive(Reflect)] struct Foo<T: Reflect> { value: T } ``` This works well for now. However, as we want to do more with `Reflect`, these bounds might need to change. For example, to get #4154 working, we likely need to enforce the `GetTypeRegistration` trait. So now we have: ```rust #[derive(Reflect)] struct Foo<T: Reflect + GetTypeRegistration> { value: T } ``` Not great, but not horrible. However, we might then want to do something as suggested in [this](#5745 (comment)) comment and add a `ReflectTypeName` trait for stable type name support. Well now we have: ```rust #[derive(Reflect)] struct Foo<T: Reflect + GetTypeRegistration + ReflectTypeName> { value: T } ``` Now imagine that for even two or three generic types. Yikes! As the API changes it would be nice if users didn't need to manually migrate their generic type bounds like this. A lot of these traits are (or will/might be) core to the entire reflection API. And although `Reflect` can't add them as supertraits for object-safety reasons, they are still indirectly required for things to function properly (manual implementors will know how easy it is to forget to implement `GetTypeRegistration`). And they should all be automatically implemented for user types anyways as long they're using `#[derive(Reflect)]`. ## Solution Add a "catch-all" trait called `Reflectable` whose supertraits are a select handful of core reflection traits. This allows us to consolidate all the examples above into this: ```rust #[derive(Reflect)] struct Foo<T: Reflectable> { value: T } ``` And as we experiment with the API, users can rest easy knowing they don't need to migrate dozens upon dozens of types. It should all be automatic! ## Discussion 1. Thoughts on the name `Reflectable`? Is it too easily confused with `Reflect`? Or does it at least accurately describe that this contains the core traits? If not, maybe `BaseReflect`? --- ## Changelog - Added the `Reflectable` trait --------- Co-authored-by: Alice Cecile <[email protected]>

james7132 added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Mar 8, 2022

alice-i-cecile added A-Reflection Runtime information about types C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed S-Needs-Triage This issue needs to be labelled C-Feature A new feature, making something new possible labels Mar 8, 2022

MrGVSV mentioned this issue Mar 16, 2022

[Merged by Bors] - bevy_reflect: Add GetTypeRegistration impl for reflected tuples #4226

Closed

alice-i-cecile mentioned this issue Mar 21, 2022

Bundle Registering #60

Closed

MrGVSV moved this to Controversial in Reflection Jun 4, 2022

MrGVSV added this to Reflection Jun 4, 2022

MrGVSV self-assigned this Aug 23, 2022

This was referenced Aug 23, 2022

bevy_reflect: Add Reflectable trait #5772

Merged

bevy_reflect: Recursive registration #5781

Merged

MrGVSV moved this from Done to In Progress in Reflection Oct 15, 2022

MrGVSV mentioned this issue Oct 18, 2022

bevy_reflect: Require type registrations #6289

Open

SludgePhD mentioned this issue Sep 12, 2023

Implement Reflect for Mesh #9779

Merged

alice-i-cecile mentioned this issue Oct 25, 2023

Allow registering multiple types at once using tuple types #10263

Open

james7132 mentioned this issue Feb 6, 2024

Missing registrations #11736

Merged

alice-i-cecile closed this as completed in #5781 Mar 4, 2024

github-project-automation bot moved this from In Progress to Done in Reflection Mar 4, 2024

This was referenced Mar 4, 2024

Clean up type registrations #12304

Closed

Clean up type registrations #12314

Merged

Automatic registration of ECS types #12332

Open

james7132 mentioned this issue Mar 8, 2024

Automatically register types for Reflect #3936

Open

james7132 mentioned this issue Mar 23, 2024

Cannot Serialize Simple Bevy World Into a Scene #9060

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Recursive Type Registration #4154

Recursive Type Registration #4154

james7132 commented Mar 8, 2022

alice-i-cecile commented Mar 8, 2022

james7132 commented Mar 8, 2022

MrGVSV commented Mar 8, 2022

james7132 commented Mar 9, 2022

cart commented Mar 9, 2022

james7132 commented Mar 9, 2022

Recursive Type Registration #4154

Recursive Type Registration #4154

Comments

james7132 commented Mar 8, 2022

What problem does this solve or what need does it fill?

What solution would you like?

What alternative(s) have you considered?

alice-i-cecile commented Mar 8, 2022

james7132 commented Mar 8, 2022

MrGVSV commented Mar 8, 2022

james7132 commented Mar 9, 2022

cart commented Mar 9, 2022

james7132 commented Mar 9, 2022