bevy_reflect: Add `Reflectable` trait #5772

MrGVSV · 2022-08-23T06:30:14Z

Objective

When deriving Reflect, users will notice that their generic arguments also need to implement Reflect:

#[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:

#[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 comment and add a ReflectTypeName trait for stable type name support. Well now we have:

#[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:

#[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

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

examples/reflection/generic_reflection.rs

crates/bevy_reflect/src/lib.rs

alice-i-cecile · 2022-08-31T03:01:20Z

I'm in favor of this design: I've seen this sort of supertrait be super useful with Num.

Name is fine. Only blocking request is that this should be linked to from the Reflect docs (and any other traits that this subsumes).

MrGVSV · 2022-08-31T04:47:28Z

crates/bevy_reflect/src/type_info.rs

@@ -8,6 +8,8 @@ use std::any::{Any, TypeId};
 /// This trait is automatically implemented by the `#[derive(Reflect)]` macro
 /// and allows type information to be processed without an instance of that type.
 ///
+/// As a core trait to the reflection system, it is a supertrait of [`Reflectable`].


@alice-i-cecile I'm not really sure I love this. Any thoughts or suggestions?

afonsolage

Does advising users to make use of Reflectable will likely increas the number of breaking changes, whenever we update Reflectable? I mean, if I add a new trait to Reflectable, and it isn't an auto impl trait, this means the user will have to add a new #[derive] for all types that uses the generic bounds, right?

Also, should we consider using Reflectable as temporary until trait_alias stabilize (which may take years)?

MrGVSV · 2022-08-31T07:33:47Z

Does advising users to make use of Reflectable will likely increas the number of breaking changes, whenever we update Reflectable? I mean, if I add a new trait to Reflectable, and it isn't an auto impl trait, this means the user will have to add a new #[derive] for all types that uses the generic bounds, right?

Reflect has the restriction that it needs to be object-safe. Because of this, we have a lot of other required (or partially-required) traits to provide other functionality. Typed is definitely required due to Reflect::get_type_info, while GetTypeRegistration is required if you need to use the registry. However, not implementing GetTypeRegistration can lead to subtle footguns.

Reflectable is meant to unify these core traits. And because these traits are so heavily utilized within bevy_reflect, they are automatically derived via #[derive(Reflect)].

I don't think Reflectable should contain anything that isn't auto-implemented via that derive macro. Therefore, adding a trait should only be a breaking change for those subset of users that implement Reflect (et al.) manually— which probably isn't many haha.

What could be a breaking change, though, is removing a trait. Anyone that used Reflectable to ensure a trait impl existed would suddenly not have that guarantee anymore. But if we removed a trait from Reflectable, we likely also removed it from the derive macro, meaning that this would still be a breaking change with-or-without Reflectable.

Also, should we consider using Reflectable as temporary until trait_alias stabilize (which may take years)?

I don't know much about it, but it looks like pretty much what we're trying to do. However, I'm not too sure what advantages it has over the blanket impl approach.

alice-i-cecile · 2022-08-31T16:07:29Z

Reflectable is meant to unify these core traits. And because these traits are so heavily utilized within bevy_reflect, they are automatically derived via #[derive(Reflect)].

We should consider making the derive name the same as the name of the blanket trait. IMO that means:

Reflect trait name is used for this Reflectable trait.
The current Reflect functionality becomes something like BasicReflect.

MrGVSV · 2022-08-31T16:43:44Z

We should consider making the derive name the same as the name of the blanket trait. IMO that means:

Reflect trait name is used for this Reflectable trait.

The current Reflect functionality becomes something like BasicReflect.

I don't mind this as it makes it slightly clearer that it will be deriving multiple traits (at least, if a user knows what Reflectable is). Or, at any rate I suppose it brings the trait to the attention of users.

However, I don't really like the idea of renaming Reflect to BaseReflect. All the other supertraits of Reflectable help drive the other parts of reflection. But most of that is utilized within bevy_reflect. Apart from generic types and functions where those bounds are necessary, users generally won't need to worry about them.

Most often, they'll just be working with dyn Reflect as opposed to a concrete generic type. As it stands right now, this PR shouldn't break anything on its own. If we changed to BaseReflect then the only thing users will see from it is that they need to use dyn BaseReflect now.

If we wanted to change anything, I feel it should at most be in your suggestion to make this the trait we "derive" via #[derive(Reflectable)]. But I think that could be saved for a more controversial followup PR haha.

@nicopap

# Objective Fixes #8965. #### Background For convenience and to ensure everything is setup properly, we automatically add certain bounds to the derived types. The current implementation does this by taking the types from all active fields and adding them to the where-clause of the generated impls. I believe this method was chosen because it won't add bounds to types that are otherwise ignored. ```rust #[derive(Reflect)] struct Foo<T, U: SomeTrait, V> { t: T, u: U::Assoc, #[reflect(ignore)] v: [V; 2] } // Generates something like: impl<T, U: SomeTrait, V> for Foo<T, U, V> where // Active: T: Reflect, U::Assoc: Reflect, // Ignored: [V; 2]: Send + Sync + Any { // ... } ``` The self-referential type fails because it ends up using _itself_ as a type bound due to being one of its own active fields. ```rust #[derive(Reflect)] struct Foo { foo: Vec<Foo> } // Foo where Vec<Foo>: Reflect -> Vec<T> where T: Reflect -> Foo where Vec<Foo>: Reflect -> ... ``` ## Solution We can't simply parse all field types for the name of our type. That would be both complex and prone to errors and false-positives. And even if it wasn't, what would we replace the bound with? Instead, I opted to go for a solution that only adds the bounds to what really needs it: the type parameters. While the bounds on concrete types make errors a bit cleaner, they aren't strictly necessary. This means we can change our generated where-clause to only add bounds to generic type parameters. Doing this, though, returns us back to the problem of over-bounding parameters that don't need to be bounded. To solve this, I added a new container attribute (based on [this](dtolnay/syn#422 (comment)) comment and @nicopap's [comment](#9046 (comment))) that allows us to pass in a custom where clause to modify what bounds are added to these type parameters. This allows us to do stuff like: ```rust trait Trait { type Assoc; } // We don't need `T` to be reflectable since we only care about `T::Assoc`. #[derive(Reflect)] #[reflect(where T::Assoc: FromReflect)] struct Foo<T: Trait>(T::Assoc); #[derive(TypePath)] struct Bar; impl Trait for Bar { type Assoc = usize; } #[derive(Reflect)] struct Baz { a: Foo<Bar>, } ``` > **Note** > I also [tried](dc139ea) allowing `#[reflect(ignore)]` to be used on the type parameters themselves, but that proved problematic since the derive macro does not consume the attribute. This is why I went with the container attribute approach. ### Alternatives One alternative could possibly be to just not add reflection bounds automatically (i.e. only add required bounds like `Send`, `Sync`, `Any`, and `TypePath`). The downside here is we add more friction to using reflection, which already comes with its own set of considerations. This is a potentially viable option, but we really need to consider whether or not the ergonomics hit is worth it. If we did decide to go the more manual route, we should at least consider something like #5772 to make it easier for users to add the right bounds (although, this could still become tricky with `FromReflect` also being automatically derived). ### Open Questions 1. Should we go with this approach or the manual alternative? 2. ~~Should we add a `skip_params` attribute to avoid the `T: 'static` trick?~~ ~~Decided to go with `custom_where()` as it's the simplest~~ Scratch that, went with a normal where clause 3. ~~`custom_where` bikeshedding?~~ No longer needed since we are using a normal where clause ### TODO - [x] Add compile-fail tests --- ## Changelog - Fixed issue preventing recursive types from deriving `Reflect` - Changed how where-clause bounds are generated by the `Reflect` derive macro - They are now only applied to the type parameters, not to all active fields - Added `#[reflect(where T: Trait, U::Assoc: Trait, ...)]` container attribute ## Migration Guide When deriving `Reflect`, generic type params that do not need the automatic reflection bounds (such as `Reflect`) applied to them will need to opt-out using a custom where clause like: `#[reflect(where T: Trait, U::Assoc: Trait, ...)]`. The attribute can define custom bounds only used by the reflection impls. To simply opt-out all the type params, we can pass in an empty where clause: `#[reflect(where)]`. ```rust // BEFORE: #[derive(Reflect)] struct Foo<T>(#[reflect(ignore)] T); // AFTER: #[derive(Reflect)] #[reflect(where)] struct Foo<T>(#[reflect(ignore)] T); ``` --------- Co-authored-by: Nicola Papale <nicopap@users.noreply.github.com>

@nicopap

# Objective Fixes bevyengine#8965. #### Background For convenience and to ensure everything is setup properly, we automatically add certain bounds to the derived types. The current implementation does this by taking the types from all active fields and adding them to the where-clause of the generated impls. I believe this method was chosen because it won't add bounds to types that are otherwise ignored. ```rust #[derive(Reflect)] struct Foo<T, U: SomeTrait, V> { t: T, u: U::Assoc, #[reflect(ignore)] v: [V; 2] } // Generates something like: impl<T, U: SomeTrait, V> for Foo<T, U, V> where // Active: T: Reflect, U::Assoc: Reflect, // Ignored: [V; 2]: Send + Sync + Any { // ... } ``` The self-referential type fails because it ends up using _itself_ as a type bound due to being one of its own active fields. ```rust #[derive(Reflect)] struct Foo { foo: Vec<Foo> } // Foo where Vec<Foo>: Reflect -> Vec<T> where T: Reflect -> Foo where Vec<Foo>: Reflect -> ... ``` ## Solution We can't simply parse all field types for the name of our type. That would be both complex and prone to errors and false-positives. And even if it wasn't, what would we replace the bound with? Instead, I opted to go for a solution that only adds the bounds to what really needs it: the type parameters. While the bounds on concrete types make errors a bit cleaner, they aren't strictly necessary. This means we can change our generated where-clause to only add bounds to generic type parameters. Doing this, though, returns us back to the problem of over-bounding parameters that don't need to be bounded. To solve this, I added a new container attribute (based on [this](dtolnay/syn#422 (comment)) comment and @nicopap's [comment](bevyengine#9046 (comment))) that allows us to pass in a custom where clause to modify what bounds are added to these type parameters. This allows us to do stuff like: ```rust trait Trait { type Assoc; } // We don't need `T` to be reflectable since we only care about `T::Assoc`. #[derive(Reflect)] #[reflect(where T::Assoc: FromReflect)] struct Foo<T: Trait>(T::Assoc); #[derive(TypePath)] struct Bar; impl Trait for Bar { type Assoc = usize; } #[derive(Reflect)] struct Baz { a: Foo<Bar>, } ``` > **Note** > I also [tried](bevyengine@dc139ea) allowing `#[reflect(ignore)]` to be used on the type parameters themselves, but that proved problematic since the derive macro does not consume the attribute. This is why I went with the container attribute approach. ### Alternatives One alternative could possibly be to just not add reflection bounds automatically (i.e. only add required bounds like `Send`, `Sync`, `Any`, and `TypePath`). The downside here is we add more friction to using reflection, which already comes with its own set of considerations. This is a potentially viable option, but we really need to consider whether or not the ergonomics hit is worth it. If we did decide to go the more manual route, we should at least consider something like bevyengine#5772 to make it easier for users to add the right bounds (although, this could still become tricky with `FromReflect` also being automatically derived). ### Open Questions 1. Should we go with this approach or the manual alternative? 2. ~~Should we add a `skip_params` attribute to avoid the `T: 'static` trick?~~ ~~Decided to go with `custom_where()` as it's the simplest~~ Scratch that, went with a normal where clause 3. ~~`custom_where` bikeshedding?~~ No longer needed since we are using a normal where clause ### TODO - [x] Add compile-fail tests --- ## Changelog - Fixed issue preventing recursive types from deriving `Reflect` - Changed how where-clause bounds are generated by the `Reflect` derive macro - They are now only applied to the type parameters, not to all active fields - Added `#[reflect(where T: Trait, U::Assoc: Trait, ...)]` container attribute ## Migration Guide When deriving `Reflect`, generic type params that do not need the automatic reflection bounds (such as `Reflect`) applied to them will need to opt-out using a custom where clause like: `#[reflect(where T: Trait, U::Assoc: Trait, ...)]`. The attribute can define custom bounds only used by the reflection impls. To simply opt-out all the type params, we can pass in an empty where clause: `#[reflect(where)]`. ```rust // BEFORE: #[derive(Reflect)] struct Foo<T>(#[reflect(ignore)] T); // AFTER: #[derive(Reflect)] #[reflect(where)] struct Foo<T>(#[reflect(ignore)] T); ``` --------- Co-authored-by: Nicola Papale <nicopap@users.noreply.github.com>

# 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 <contact@jamessliu.com> Co-authored-by: radiish <cb.setho@gmail.com> Co-authored-by: Carter Anderson <mcanders1@gmail.com>

# 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>

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>

brandon-reinhart

This looks straightforward to me.

Is FromReflect also needed?

At the moment, I need:

pub trait EntityLocator:
    GetTypeRegistration + Typed + FromReflect + Reflect + TypePath

To do this:

    fn register_entity_locator<Locator: EntityLocator>(&mut self) -> &mut Self {
        self.register_type::<MapLocations<Locator>>();
        
       // other stuff
      }

Without which I get:

error[E0277]: `Locator` can not be created through reflection
   --> game\tier1\src\ai\locations\map_locations.rs:11:30
    |
11  |         self.register_type::<MapLocations<Locator>>();
    |              -------------   ^^^^^^^^^^^^^^^^^^^^^ the trait `bevy::prelude::FromReflect` is not implemented for `Locator`, which is required by `MapLocations<Locator>: bevy::bevy_reflect::GetTypeRegistration`
    |              |
    |              required by a bound introduced by this call
    |
    = note: consider annotating `Locator` with `#[derive(FromReflect)]`

MrGVSV · 2024-09-17T23:57:27Z

This looks straightforward to me.

Is FromReflect also needed?

Good question! I chose to leave FromReflect out since it's not strictly required by reflection. It can be opted out of by the derive macro, which would make Reflectable a lot more restrictive than would probably be ideal.

So in the cases you need FromReflect, I'd suggest just making that part explicit:

pub trait EntityLocator: Reflectable + FromReflect {}

crates/bevy_reflect/src/reflectable.rs

alice-i-cecile

I have a wording suggestion, but I think this is useful and clear.

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>

MrGVSV added C-Usability A simple quality-of-life change that makes Bevy easier to use A-Reflection Runtime information about types labels Aug 23, 2022

This was referenced Aug 24, 2022

bevy_reflect: Recursive registration #5781

Merged

stable reflect type name #5805

Closed

alice-i-cecile reviewed Aug 31, 2022

View reviewed changes

examples/reflection/generic_reflection.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Aug 31, 2022

View reviewed changes

crates/bevy_reflect/src/lib.rs Outdated Show resolved Hide resolved

MrGVSV force-pushed the reflect-reflectable branch from 3e09a4e to 94814c3 Compare August 31, 2022 04:16

MrGVSV commented Aug 31, 2022

View reviewed changes

MrGVSV force-pushed the reflect-reflectable branch from edbc673 to 0344bad Compare August 31, 2022 05:07

afonsolage reviewed Aug 31, 2022

View reviewed changes

MrGVSV force-pushed the reflect-reflectable branch from 0344bad to be039c6 Compare September 21, 2022 19:27

soqb mentioned this pull request Jan 15, 2023

reflect: implement the unique reflect rfc #7207

Merged

MrGVSV mentioned this pull request Jul 5, 2023

bevy_reflect: Type parameter bounds #9046

Merged

1 task

SkiFire13 mentioned this pull request Mar 24, 2024

Make Typed, TypePath, GetTypeRegistration, etc etc supertraits of Reflect #12691

Open

MrGVSV and others added 2 commits September 17, 2024 15:38

Added Reflectable trait

936dd9b

Apply suggestions from code review

88cd4a2

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>

MrGVSV force-pushed the reflect-reflectable branch from be039c6 to 355f71a Compare September 17, 2024 22:55

alice-i-cecile added S-Needs-Review Needs reviewer attention (from anyone!) to move forward C-Needs-Release-Note Work that should be called out in the blog due to impact labels Sep 17, 2024

brandon-reinhart reviewed Sep 17, 2024

View reviewed changes

Update documentation for Reflectable

d64ecb8

MrGVSV force-pushed the reflect-reflectable branch from 355f71a to d64ecb8 Compare September 17, 2024 23:59

brandon-reinhart approved these changes Sep 18, 2024

View reviewed changes

alice-i-cecile added this to the 0.15 milestone Sep 18, 2024

alice-i-cecile reviewed Sep 18, 2024

View reviewed changes

crates/bevy_reflect/src/reflectable.rs Outdated Show resolved Hide resolved

alice-i-cecile approved these changes Sep 18, 2024

View reviewed changes

alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 18, 2024

Update crates/bevy_reflect/src/reflectable.rs

2d179e1

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>

alice-i-cecile enabled auto-merge September 18, 2024 00:31

alice-i-cecile added this pull request to the merge queue Sep 18, 2024

Merged via the queue into bevyengine:main with commit 6954146 Sep 18, 2024
26 checks passed

MrGVSV deleted the reflect-reflectable branch September 18, 2024 00:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bevy_reflect: Add `Reflectable` trait #5772

bevy_reflect: Add `Reflectable` trait #5772

MrGVSV commented Aug 23, 2022

alice-i-cecile commented Aug 31, 2022

MrGVSV Aug 31, 2022

afonsolage left a comment

MrGVSV commented Aug 31, 2022

alice-i-cecile commented Aug 31, 2022

MrGVSV commented Aug 31, 2022 •

edited

Loading

brandon-reinhart left a comment •

edited

Loading

MrGVSV commented Sep 17, 2024 •

edited

Loading

alice-i-cecile left a comment

bevy_reflect: Add Reflectable trait #5772

bevy_reflect: Add Reflectable trait #5772

Conversation

MrGVSV commented Aug 23, 2022

Objective

Solution

Discussion

Changelog

alice-i-cecile commented Aug 31, 2022

MrGVSV Aug 31, 2022

Choose a reason for hiding this comment

afonsolage left a comment

Choose a reason for hiding this comment

MrGVSV commented Aug 31, 2022

alice-i-cecile commented Aug 31, 2022

MrGVSV commented Aug 31, 2022 • edited Loading

brandon-reinhart left a comment • edited Loading

Choose a reason for hiding this comment

MrGVSV commented Sep 17, 2024 • edited Loading

alice-i-cecile left a comment

Choose a reason for hiding this comment

bevy_reflect: Add `Reflectable` trait #5772

bevy_reflect: Add `Reflectable` trait #5772

MrGVSV commented Aug 31, 2022 •

edited

Loading

brandon-reinhart left a comment •

edited

Loading

MrGVSV commented Sep 17, 2024 •

edited

Loading