-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Restore separate methods for World::get_many_entities
#18234
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?
Conversation
Add `get_many_entities` and `iter_entities` methods to handle lookups of multiple entities, instead.
crates/bevy_ecs/src/world/mod.rs
Outdated
| /// This is useful in contexts where you only have read-only access to the [`World`]. | ||
| #[inline] | ||
| pub fn iter_entities(&self) -> impl Iterator<Item = EntityRef<'_>> + '_ { | ||
| pub fn iter_all_entities(&self) -> impl Iterator<Item = EntityRef<'_>> + '_ { |
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 can also be impl EntitySetIterator!
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.
Yup, but that should be a separate PR (especially if I wind up reverting the name change and not even touching this line).
crates/bevy_ecs/src/world/mod.rs
Outdated
|
|
||
| /// Returns a mutable iterator over all entities in the `World`. | ||
| pub fn iter_entities_mut(&mut self) -> impl Iterator<Item = EntityMut<'_>> + '_ { | ||
| pub fn iter_all_entities_mut(&mut self) -> impl Iterator<Item = EntityMut<'_>> + '_ { |
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.
Same here, impl EntitySetIterator.
|
Thanks for the PR! Why make For consistency, we also want both When adding |
Honestly, I've been looking at these methods for so long that the words have lost all meaning :). It does seem weird that adding So, your proposal relative to this is renaming: The result is that we're adding all 8 combinations of Is that right? Sure, I can do that. |
Yeah! As an aside, for |
crates/bevy_ecs/src/world/mod.rs
Outdated
| /// // Trying to access the same entity multiple times will fail. | ||
| /// assert!(world.get_many_entities_mut([id1, id1]).is_err()); | ||
| /// ``` | ||
| pub fn get_many_entities_mut<T: TrustedEntityBorrow, const N: usize>( |
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.
it looks like I left a few as
T: TrustedEntityBorrow. Should I switch those back toEntity, too?Did you? I don't see them!
Here's one! I'll switch get_many_entities_mut to [Entity; N] and get_many_entities_unique_mut to UniqueEntityArray<N>.
Restrict arrays and `UniqueEntityArray`s from `T: TrustedEntityBorrow` to `Entity`.
This reverts commit dccd52e.
ItsDoot
left a comment
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.
Two things:
- Seems to be missing
DeferredWorld::iter_many_entitiesas a counterpart toWorld::iter_many_entities. - We'll also need these functions on
EntityFetcherwhich was added since.
Overall I'm alright with this change, the unique entity set stuff certainly helps compared to the original situation.
| /// - Only the first entity found to be missing will be returned. | ||
| /// - Returns [`EntityMutableFetchError::AliasedMutability`] if the same entity is requested multiple times. | ||
| /// [`EntityDoesNotExistError`] if any entity does not exist in the world. | ||
| /// |
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.
| /// | |
| /// | |
| /// # Examples | |
| /// |
| /// Retrieves an [`EntityMut`] that exposes read and write operations for the given `entity`. | ||
| /// This will panic if the `entity` does not exist. Use [`DeferredWorld::get_entity_mut`] if you want | ||
| /// to check for entity existence instead of implicitly panic-ing. | ||
| /// |
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.
| /// | |
| /// | |
| /// # Examples | |
| /// |
| pub fn get_many_entities_mut<const N: usize>( | ||
| &mut self, | ||
| entities: [Entity; N], | ||
| ) -> Result<[EntityMut<'_>; N], EntityMutableFetchError> { |
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 wonder if at some point we should provide a variant that returns [Result<EntityMut<'_>, EntityMutableFetchError>; N]. 🤔 Unfortunately std doesn't provide a transpose function for results <-> arrays, so we'd have to provide that function ourselves or make the user perform it themselves.
| /// Retrieves an [`EntityRef`] that exposes read-only operations for the given `entity`. | ||
| /// This will panic if the `entity` does not exist. Use [`World::get_entity`] if you want | ||
| /// to check for entity existence instead of implicitly panic-ing. | ||
| /// |
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.
| /// | |
| /// | |
| /// # Examples | |
| /// |
| /// Retrieves an [`EntityWorldMut`] that exposes read and write operations for the given `entity`. | ||
| /// This will panic if the `entity` does not exist. Use [`World::get_entity_mut`] if you want | ||
| /// to check for entity existence instead of implicitly panic-ing. | ||
| /// |
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.
| /// | |
| /// | |
| /// # Examples | |
| /// |
| /// - Only the first entity found to be missing will be returned. | ||
| /// - Returns [`EntityMutableFetchError::AliasedMutability`] if the same entity is requested multiple times. | ||
| /// [`EntityDoesNotExistError`] if any entity does not exist in the world. | ||
| pub fn get_many_unique_entities<const N: usize>( |
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.
Example?
I think the
Yeah, I got stalled on this PR once that got merged because I didn't really want three copies, and it seemed like there ought to be a way to combine |
|
I think this is the right direction here:
|
Objective
Reduce the complexity of the
World::get_entityAPI by removing theF: WorldEntityFetchtype parameter and restoring separateWorld::get_many_entitiesmethods.It would be good for consistency to have
Query::getandWorld::get_entitywork the same way, but per discussion on #18036 (comment),WorldEntityFetchwas too much complexity forQuery::get. Instead, let's removeWorldEntityFetchand makeWorld::get_entitywork likeQuery::get.This undoes #15614 (although I did not attempt to implement it as a revert). A few things have changed since then that makes the duplication less of a problem than it was when
WorldEntityFetchwas introduced:One change is that Bevy has improved handling for
Results, so we no longer expect to offer panicking versions of all APIs, and we do not need to restore the panickingmany_entitiesandmany_entities_mut.Another change is the introduction of
EntitySetand related traits in #16547. This lets us generalize over types that hold sets of uniqueEntitys, so we do not need a separate method for every such type.Solution
Remove
F: WorldEntityFetchparameter fromWorld::get_entity()and related methods, and have it only acceptEntity.Introduce new methods to handle the various "many" cases:
get_many_entitiestakes an array and returns an array ofEntityRef. These are read-only, so duplicates are okay.iter_entitiestakes an iterator and returns an iterator ofEntityRef. These are read-only, so duplicates are okay.get_many_entities_muttakes an array and returns an array ofEntityMut. It performs the O(N^2) duplicate check. This is acceptable because we expect N to be small for arrays.get_many_entities_unique_muttakes aUniqueEntityArrayand returns an array ofEntityMut.UniqueEntityArrayensures there are no duplicates.iter_entities_muttakes anEntitySetand returns an iterator ofEntityMut.EntitySetensures there are no duplicates.Also rename the existing
iter_entities(_mut)methods toiter_all_entities(_mut), to free up the name.Migration Guide
World::entity(),World::entity_mut(),World::get_entity(), andWorld::get_entity_mut()now only take a singleEntityinstead of being generic and accepting collections. If you were passing a collection, you will need to call a different method.If you were using a panicking variant, first replace it with
get_entity(e).unwrap()orget_entity_mut(e).unwrap().Then, replace:
World::get_entity::<[Entity; N]>->World::get_many_entities()World::get_entity_mut::<[Entity; N]>->World::get_many_entities_mut()World::get_entity::<&[Entity]>->World::iter_entities().collect::<Vec<_>>()World::get_entity_mut::<&[Entity]>-> This has no direct equivalent, as it performed an O(N^2) check that can be expensive for large slices. First convert the collection to anEntitySet, such asEntityHashSet, then callWorld::iter_entities(e).World::get_entity::<&EntityHashSet>->World::iter_entities(e).map(|e| (e.id(), e)).collect::<EntityHashMap<_>>()World::get_entity_mut::<&EntityHashSet>->World::iter_entities_mut(e).map(|e| (e.id(), e)).collect::<EntityHashMap<_>>()In addition,
World::iter_entities(_mut)has been renamediter_all_entities(_mut)to make the name available for these new methods. If you were callingiter_entitiesoriter_entities_mut, the functionality is still available under the new name.