-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Add helper methods to dynamically access relationships #21639
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
base: main
Are you sure you want to change the base?
Add helper methods to dynamically access relationships #21639
Conversation
Change-Id: I0fc5f429189a9a55c233ac353cdb0767ed1502e6
|
Is there a reason why these functions return |
You're right, |
| ) -> Option<impl Iterator<Item = Entity> + use<'w>> { | ||
| self.access | ||
| .has_component_read(relationship_target_id) | ||
| // SAFETY: We have read access | ||
| .then(|| unsafe { | ||
| self.entity | ||
| .get_relationship_targets_by_id(relationship_target_id) | ||
| }) | ||
| .flatten() |
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.
Why can't this be self.as_readonly().get_relationship_targets_by_id(relationship_target_id), like get_relationship_by_id is? ... Oh, I see, that would borrow from a temporary. Wait, then how do we do that elsewhere? ... Hmm, I think some of the lifetimes are wrong here.
The read-only methods on mutable EntityRefs need to return things that borrow from '_ instead of 'w, since otherwise you could do
let borrows_r1 = filtered_entity.get_relationship_targets_by_id(r1);
let borrows_r1_mutably = filtered_entity.get_mut_by_id(r1);
// Both variables are alive here, because the lifetime in `borrows_r1`
// wasn't tied to the borrow of `filtered_entity`!(If we want methods that return 'w, they need to consume self, like into_mut() does. We don't need those here, though, since they're uncommon and users can build them out of entity_ref.into_readonly().get_relationship_targets_by_id().)
So, I think this should be:
| ) -> Option<impl Iterator<Item = Entity> + use<'w>> { | |
| self.access | |
| .has_component_read(relationship_target_id) | |
| // SAFETY: We have read access | |
| .then(|| unsafe { | |
| self.entity | |
| .get_relationship_targets_by_id(relationship_target_id) | |
| }) | |
| .flatten() | |
| ) -> Option<impl Iterator<Item = Entity> + use<'_>> { | |
| self.as_readonly().get_relationship_targets_by_id(relationship_target_id) |
| pub fn get_relationship_targets_by_id( | ||
| &self, | ||
| relationship_target_id: ComponentId, | ||
| ) -> Option<impl Iterator<Item = Entity> + use<'_>> { |
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.
In contrast, EntityRef::get_relationship_targets_by_id can use 'w, because EntityRef knows nothing else can have mutable access for the rest of the 'w lifetime. So this can be:
| ) -> Option<impl Iterator<Item = Entity> + use<'_>> { | |
| ) -> Option<impl Iterator<Item = Entity> + use<'w>> { |
| // SAFETY: We have read-only access to all components of this entity. | ||
| unsafe { self.cell.get_relationship_by_id(relationship_id) } |
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.
And I'd recommend writing these in terms of as_readonly(), like we do with EntityMut::get(), since that means we can write them with safe code and not have to check as many lifetimes manually:
| // SAFETY: We have read-only access to all components of this entity. | |
| unsafe { self.cell.get_relationship_by_id(relationship_id) } | |
| self.as_readonly().get_relationship_by_id(relationship_id) |
| // SAFETY: We have read-only access to all components of this entity. | ||
| unsafe { | ||
| self.cell | ||
| .get_relationship_targets_by_id(relationship_target_id) | ||
| } |
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.
| // SAFETY: We have read-only access to all components of this entity. | |
| unsafe { | |
| self.cell | |
| .get_relationship_targets_by_id(relationship_target_id) | |
| } | |
| self.as_readonly().get_relationship_targets_by_id(relationship_target_id) |
Objective
#21601 introduced helpers to dynamically access relationship values
The
RelationshipAccessorcan be hard to use manually as it requires manipulating pointers; we can provide helper functions inUnsafeEntityCelland friends to access those.