API for traversing Relationships and RelationshipTargets in dynamic contexts
#21601
Conversation
alice-i-cecile
added
C-Feature
| Some(unsafe { | ||
| #bevy_ecs_path::relationship::ComponentRelationshipAccessor::<Self>::relationship( | ||
| core::mem::offset_of!(Self, #relationship_member) | ||
| ) | ||
| }) |
There was a problem hiding this comment.
I never really reviewed derive macro source code because it scares me, but where does one write safety comments of this if not here? Is the CI not caring about safety comments in macros?
| pub unsafe fn relationship(entity_field_offset: usize) -> Self | ||
| where | ||
| C: Relationship, | ||
| { | ||
| Self { | ||
| accessor: RelationshipAccessor::Relationship { | ||
| entity_field_offset, | ||
| linked_spawn: C::RelationshipTarget::LINKED_SPAWN, | ||
| }, | ||
| phantom: Default::default(), | ||
| } | ||
| } |
There was a problem hiding this comment.
It is a bit weird to see one information (LINKED_SPAWN) be taken from C, but another (entity_field_offset) is not. I guess it is not worth the hassle to bring another offset method to RelationShip though but it would make this method safe in case users want to directly use this. That would also not require the user to have access to the field for offset_of!.
If this had no parameter, one could use this for a generic API that needs this RelationshipAccessor but has no Components access.
There was a problem hiding this comment.
Getting offset is inherently unsafe as there is no way to know that it is actually the correct offset, we just have to trust that it is.
There was a problem hiding this comment.
If what you mean is that it can store the get function of Relationship similar to RelationshipTarget::iter, that can be done, but would introduce unnecessary overhead when we can get the value without having to call a function.
There was a problem hiding this comment.
I was thinking of something like
pub trait Relationship {
const ENTITY_FIELD_OFFSET: usize = core::mem::offset_of!(Self, #relationship_member);... that this method here picks up like LINKED_SPAWN.
There was a problem hiding this comment.
That would work, yes, although in both cases this requires the derive macro to set it, at which point there is little difference in how it actually gets to RelationshipAccessor. In fact, the entire ComponentRelationshipAccessor is a bit redundant - it exists mostly to avoid having to mark Component trait as unsafe.
| `ComponentDescriptor` now stores additional data for working with relationships in dynamic contexts. | ||
| This resulted in changes to `ComponentDescriptor::new_with_layout`: | ||
|
|
||
| - Now requires additional parameter `relationship_accessor`, which should be set to `None` for all existing components. |
There was a problem hiding this comment.
"for all existing components" seems to not fit really? The user calls ComponentDescriptor::new_with_layout for components that do not yet exist to pass the descriptor to World::register_component_with_descriptor. This should instead point out that new relationship components in dynamic contexts are not supported and thus None should be passed in here.
| - Now requires additional parameter `relationship_accessor`, which should be set to `None` for all existing components. | |
| - Now requires additional parameter `relationship_accessor`, which should be set to `None` as relationships need further registrations that is only covered by the derive macro. |
There was a problem hiding this comment.
What I meant to say here is that all existing code can just put None here and it would be correct, relationship_accessor is only relevant for new code that would want to register relationships.
There was a problem hiding this comment.
Ah yes from that perspective it makes sense. Maybe phrase it like "all existing code"?
3 participants
Objective
Currently there is no way to traverse relationships in type-erased contexts or to define dynamic relationship components, which is a hole in the current relationships api.
Solution
Introduce
RelationshipAccessorto describe a way to getEntityvalues from any registered relationships in dynamic contexts and store it onComponentDescriptor. This allows to traverse relationships without knowing their type, which is useful for working with entity hierarchies using non-default components.Testing
Added a simple test/example of how to use this api to traverse hierarchies in a type-erased context.