-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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: Recursive registration #5781
bevy_reflect: Recursive registration #5781
Conversation
82108df
to
5bc61bb
Compare
5bc61bb
to
9dd48e1
Compare
@@ -13,7 +14,7 @@ fn main() { | |||
} | |||
|
|||
#[derive(Reflect)] | |||
struct MyType<T: Reflect> { | |||
struct MyType<T: Reflect + GetTypeRegistration> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But it still introduces another trait. It would be nice if T: Reflect
just worked.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
9dd48e1
to
0cc73be
Compare
0cc73be
to
162c3d0
Compare
# 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>`
# 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>`
79fa0d6
to
813e889
Compare
…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>`
813e889
to
196b4d8
Compare
…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>`
…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>`
196b4d8
to
3ac6285
Compare
3ac6285
to
a2798d7
Compare
We might actually be able to improve the usability here by adding Tasks
|
3541b65
to
2212a96
Compare
a857552
to
f40347e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just pushed my register() optimization changes. Please review them before merging.
if self.add_registration(T::get_type_registration()) { | ||
T::register_type_dependencies(self); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 thefn
only when actually needed internally - Call
TypeId::of::<T>()
directly inregister()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay I added register_internal
to do just that!
@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? |
Great: removing the controversial label for now so I don't miss it 😅 |
One final thing that came to mind was maybe to annotate |
@james7132 Annotate just the default trait method or add it to the generated |
The empty default is fine, but the ones that have an actual body need to be annotated to avoid inlining the recursion. |
LGTM, although the |
# 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 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 - 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 <[email protected]> Co-authored-by: Gino Valente <[email protected]>
Objective
Resolves #4154
Currently, registration must all be done manually:
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>
, andOption<T>
.This allows us to simplify the code above like:
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 implementGetTypeRegistration
. 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 theGetTypeRegistration
bound by default. This should technically be okay, since we already add theReflect
bound.However, this can also be considered a breaking change for manual implementations that left out a
GetTypeRegistration
implor for items that contain dynamic types (e.g..DynamicStruct
) since those also do not implementGetTypeRegistration
Registration Assumptions
By automatically registering fields, users might inadvertently be relying on certain types to be automatically registered. If
Foo
auto-registersBar
, butFoo
is later removed from the code, then anywhere that previously used or relied onBar
's registration would now fail.Changelog
Vec<T>
,HashMap<K, V>
, andOption<T>
bevy_reflect::__macro_exports
module calledRegisterForReflection
GetTypeRegistration
impl forbevy_render::render_asset::RenderAssetUsages
Migration Guide
All types that derive
Reflect
will now automatically addGetTypeRegistration
as a bound on all (unignored) fields. This means that all reflected fields will need to also implementGetTypeRegistration
.If all fields derive
Reflect
or are implemented inbevy_reflect
, this should not cause any issues. However, manual implementations ofReflect
that excluded aGetTypeRegistration
impl for their type will need to add one.