Conversation
Better consistency with the rest of bevy_ecs's API.
Probably out of scope for this PR
alice-i-cecile
added
A-ECS
|
I'm considering documenting more of the reflect machinery in this PR, but I'm unsure if that will make it harder or easier to review. |
alice-i-cecile
added
A-Reflection
alice-i-cecile
added
M-Migration-Guide
| }; | ||
| use bevy_reflect::{impl_reflect_value, FromType, Reflect, ReflectDeserialize}; | ||
|
|
||
| /// A runtime type-reflectable component |
There was a problem hiding this comment.
Not entirely correct. ReflectComponent is a type-erased collection of operations on a given component type. By itself, it doesn't contain any reference to a given component. It's meant to be used as metadata that is fetched Res<TypeRegistryArc>.
It can be used to get a dyn Reflect reference given a World reference and Entity, but by itself it does not hold nor refer to component data at all.
This isn't too intuitive at all, so an explicit example is probably best here.
There was a problem hiding this comment.
Ah, that's a very useful explanation! I was muddling through based on my first pass examining the raw code.
Co-authored-by: James Liu <contact@jamessliu.com>
Co-authored-by: James Liu <contact@jamessliu.com>
oddfacade
left a comment
oddfacade
left a comment
There was a problem hiding this comment.
hope this isn't too pedantic. left some comments based on the things i struggled to understand while trying to figure out how this thing works.
| /// Inserts the non-erased value of `component` (with type `C`) into the `entity` | ||
| /// | ||
| /// # Panics | ||
| /// `component` must have the same type `C` as the type used to create this struct |
There was a problem hiding this comment.
afaik component only needs to be applicable to C, but need not actually be of type C. for instance, on this line, component may be a DynamicStruct.
| /// a [`ReflectComponent`] is a type-erased version of a component's data, | ||
| /// can be transformed into ['dyn Reflect'](Reflect) trait objects, | ||
| /// which can be worked with generically | ||
| /// and loaded from disk in a type-safe fashion. |
There was a problem hiding this comment.
Perhaps it would be more accurate to say something like "a ReflectComponent exposes type-erased methods for interacting with components of a certain type." IMO what you have here makes it sound like the ReflectComponent actually stores the component instance's data or something, when in reality it's essentially a virtual method table associated with the component's type. It doesn't so much "transform into" a dyn reflect as fetch the component from the world and then cast it to a dyn reflect.
| /// Once a [`ReflectComponent`] object has been created, you can use that concrete struct | ||
| /// to use the methods on this type, which always implicitly affect only the component type originally used to create this struct. |
There was a problem hiding this comment.
This could really use an example. The interaction between ReflectComponent and any given &dyn Reflect is profoundly unintuitive.
Also, nit: "always implicitly affects only..." to me sounds like it's suggesting you can use these methods without worrying about types, because they handle the checking for you. In actual fact the situation is a bit more dire; some of these methods panic if the reference you give them is of the wrong type.
| impl ReflectComponent { | ||
| pub fn add_component(&self, world: &mut World, entity: Entity, component: &dyn Reflect) { | ||
| (self.add_component)(world, entity, component); | ||
| /// Inserts the non-erased value of `component` (with type `C`) into the `entity` |
There was a problem hiding this comment.
what happens if the entity already has a component of type C?
|
Those are excellent comments; thank you @oddfacade. |
|
Closing; I'm not confident in my understanding here. |
3 participants
Objective
ReflectComponentis undocumented and very complex to use.Solution
Migration Guide
ReflectComponent::add_componenthas been renamed toReflectComponent::insert_componentfor consistency with other insertion APIs.