Enable EntityRef::get_by_id and friends to take multiple ids and get multiple pointers back#15593
Conversation
djeedai
left a comment
There was a problem hiding this comment.
A few remarks for discussion, but otherwise looks all good to me. Looking forward to get that API, and the current implementation is quite neat!
| // SAFETY: | ||
| // - No aliased mutability is caused because `HashSet` guarantees unique elements. | ||
| // - No mutable references are returned by `fetch_ref`. | ||
| unsafe impl DynamicComponentFetch for &'_ HashSet<ComponentId> { |
There was a problem hiding this comment.
Note for discussion: I found myself on the World API, which is similar, wanting an overload which takes as input a HashMap<ComponentId, T> for any T. The hashmap keys have the same unicity guarantee as a HashSet but there's no way to obtain the latter from the former without allocating a copy of all keys. And I think it's not uncommon for the caller to have some data associated with each component retrieved, to process them uniformly. Thoughts?
There was a problem hiding this comment.
(the core issue here is that Rust is missing some "set" slice so unlike normal slices which can abstract any array-like container, we have to use concrete types likeHashMap and HashSet instead, which means any missing one in the API requires converting to the other available one)
There was a problem hiding this comment.
I have been working on an EntitySet trait, which would abstract over the property of uniqueness like this. It'll be for Entity only, but ComponentId will eventually just be Entity IIRC.
There was a problem hiding this comment.
Might look into doing the same thing for World later today
|
Something worth considering: should we return a different error type for the immutable variants, as |
| /// The component with the given [`ComponentId`] was requested mutably more than once. | ||
| #[error("The component with ID {0:?} was requested mutably more than once.")] | ||
| AliasedMutability(ComponentId), |
There was a problem hiding this comment.
I think one mutable + immutable refs is also invalid access
There was a problem hiding this comment.
True, but with the current API it's not possible to request both mutable and immutable references at the same time. You can only receive either all mutable references, or all immutable references.
MiniaczQ
left a comment
There was a problem hiding this comment.
Looks good, just one question
| /// Unlike [`EntityRef::get`], this returns a raw pointer to the component, | ||
| /// which is only valid while the `'w` borrow of the lifetime is active. | ||
| /// // Then, get the components by ID. You'll receive a vec of ptrs. | ||
| /// let ptrs = world.entity(entity).get_by_id(&[x_id, y_id] as &[ComponentId]); |
There was a problem hiding this comment.
Is this &[ComponentId] casting necessary because it can't infer?
There was a problem hiding this comment.
Correct, it infers as &[ComponentId; 2] which would use the array impl.
There was a problem hiding this comment.
could use &vec![x_id, y_id] it should be closer to a typical use case
…t multiple pointers back (bevyengine#15593) # Objective - Closes bevyengine#15577 ## Solution The following functions can now also take multiple component IDs and return multiple pointers back: - `EntityRef::get_by_id` - `EntityMut::get_by_id` - `EntityMut::into_borrow_by_id` - `EntityMut::get_mut_by_id` - `EntityMut::into_mut_by_id` - `EntityWorldMut::get_by_id` - `EntityWorldMut::into_borrow_by_id` - `EntityWorldMut::get_mut_by_id` - `EntityWorldMut::into_mut_by_id` If you pass in X, you receive Y: - give a single `ComponentId`, receive a single `Ptr`/`MutUntyped` - give a `[ComponentId; N]` (array), receive a `[Ptr; N]`/`[MutUntyped; N]` - give a `&[ComponentId; N]` (array), receive a `[Ptr; N]`/`[MutUntyped; N]` - give a `&[ComponentId]` (slice), receive a `Vec<Ptr>`/`Vec<MutUntyped>` - give a `&HashSet<ComponentId>`, receive a `HashMap<ComponentId, Ptr>`/`HashMap<ComponentId, MutUntyped>` ## Testing - Added 4 new tests. --- ## Migration Guide - The following functions now return an `Result<_, EntityComponentError>` instead of a `Option<_>`: `EntityRef::get_by_id`, `EntityMut::get_by_id`, `EntityMut::into_borrow_by_id`, `EntityMut::get_mut_by_id`, `EntityMut::into_mut_by_id`, `EntityWorldMut::get_by_id`, `EntityWorldMut::into_borrow_by_id`, `EntityWorldMut::get_mut_by_id`, `EntityWorldMut::into_mut_by_id`
…nd get multiple references back (#15614) # Objective Following the pattern established in #15593, we can reduce the API surface of `World` by providing a single function to grab both a singular entity reference, or multiple entity references. ## Solution The following functions can now also take multiple entity IDs and will return multiple entity references back: - `World::entity` - `World::get_entity` - `World::entity_mut` - `World::get_entity_mut` - `DeferredWorld::entity_mut` - `DeferredWorld::get_entity_mut` If you pass in X, you receive Y: - give a single `Entity`, receive a single `EntityRef`/`EntityWorldMut` (matches current behavior) - give a `[Entity; N]`/`&[Entity; N]` (array), receive an equally-sized `[EntityRef; N]`/`[EntityMut; N]` - give a `&[Entity]` (slice), receive a `Vec<EntityRef>`/`Vec<EntityMut>` - give a `&EntityHashSet`, receive a `EntityHashMap<EntityRef>`/`EntityHashMap<EntityMut>` Note that `EntityWorldMut` is only returned in the single-entity case, because having multiple at the same time would lead to UB. Also, `DeferredWorld` receives an `EntityMut` in the single-entity case because it does not allow structural access. ## Testing - Added doc-tests on `World::entity`, `World::entity_mut`, and `DeferredWorld::entity_mut` - Added tests for aliased mutability and entity existence --- ## Showcase <details> <summary>Click to view showcase</summary> The APIs for fetching `EntityRef`s and `EntityMut`s from the `World` have been unified. ```rust // This code will be referred to by subsequent code blocks. let world = World::new(); let e1 = world.spawn_empty().id(); let e2 = world.spawn_empty().id(); let e3 = world.spawn_empty().id(); ``` Querying for a single entity remains mostly the same: ```rust // 0.14 let eref: EntityRef = world.entity(e1); let emut: EntityWorldMut = world.entity_mut(e1); let eref: Option<EntityRef> = world.get_entity(e1); let emut: Option<EntityWorldMut> = world.get_entity_mut(e1); // 0.15 let eref: EntityRef = world.entity(e1); let emut: EntityWorldMut = world.entity_mut(e1); let eref: Result<EntityRef, Entity> = world.get_entity(e1); let emut: Result<EntityWorldMut, Entity> = world.get_entity_mut(e1); ``` Querying for multiple entities with an array has changed: ```rust // 0.14 let erefs: [EntityRef; 2] = world.many_entities([e1, e2]); let emuts: [EntityMut; 2] = world.many_entities_mut([e1, e2]); let erefs: Result<[EntityRef; 2], Entity> = world.get_many_entities([e1, e2]); let emuts: Result<[EntityMut; 2], QueryEntityError> = world.get_many_entities_mut([e1, e2]); // 0.15 let erefs: [EntityRef; 2] = world.entity([e1, e2]); let emuts: [EntityMut; 2] = world.entity_mut([e1, e2]); let erefs: Result<[EntityRef; 2], Entity> = world.get_entity([e1, e2]); let emuts: Result<[EntityMut; 2], EntityFetchError> = world.get_entity_mut([e1, e2]); ``` Querying for multiple entities with a slice has changed: ```rust let ids = vec![e1, e2, e3]); // 0.14 let erefs: Result<Vec<EntityRef>, Entity> = world.get_many_entities_dynamic(&ids[..]); let emuts: Result<Vec<EntityMut>, QueryEntityError> = world.get_many_entities_dynamic_mut(&ids[..]); // 0.15 let erefs: Result<Vec<EntityRef>, Entity> = world.get_entity(&ids[..]); let emuts: Result<Vec<EntityMut>, EntityFetchError> = world.get_entity_mut(&ids[..]); let erefs: Vec<EntityRef> = world.entity(&ids[..]); // Newly possible! let emuts: Vec<EntityMut> = world.entity_mut(&ids[..]); // Newly possible! ``` Querying for multiple entities with an `EntityHashSet` has changed: ```rust let set = EntityHashSet::from_iter([e1, e2, e3]); // 0.14 let emuts: Result<Vec<EntityMut>, QueryEntityError> = world.get_many_entities_from_set_mut(&set); // 0.15 let emuts: Result<EntityHashMap<EntityMut>, EntityFetchError> = world.get_entity_mut(&set); let erefs: Result<EntityHashMap<EntityRef>, EntityFetchError> = world.get_entity(&set); // Newly possible! let emuts: EntityHashMap<EntityMut> = world.entity_mut(&set); // Newly possible! let erefs: EntityHashMap<EntityRef> = world.entity(&set); // Newly possible! ``` </details> ## Migration Guide - `World::get_entity` now returns `Result<_, Entity>` instead of `Option<_>`. - Use `world.get_entity(..).ok()` to return to the previous behavior. - `World::get_entity_mut` and `DeferredWorld::get_entity_mut` now return `Result<_, EntityFetchError>` instead of `Option<_>`. - Use `world.get_entity_mut(..).ok()` to return to the previous behavior. - Type inference for `World::entity`, `World::entity_mut`, `World::get_entity`, `World::get_entity_mut`, `DeferredWorld::entity_mut`, and `DeferredWorld::get_entity_mut` has changed, and might now require the input argument's type to be explicitly written when inside closures. - The following functions have been deprecated, and should be replaced as such: - `World::many_entities` -> `World::entity::<[Entity; N]>` - `World::many_entities_mut` -> `World::entity_mut::<[Entity; N]>` - `World::get_many_entities` -> `World::get_entity::<[Entity; N]>` - `World::get_many_entities_dynamic` -> `World::get_entity::<&[Entity]>` - `World::get_many_entities_mut` -> `World::get_entity_mut::<[Entity; N]>` - The equivalent return type has changed from `Result<_, QueryEntityError>` to `Result<_, EntityFetchError>` - `World::get_many_entities_dynamic_mut` -> `World::get_entity_mut::<&[Entity]>1 - The equivalent return type has changed from `Result<_, QueryEntityError>` to `Result<_, EntityFetchError>` - `World::get_many_entities_from_set_mut` -> `World::get_entity_mut::<&EntityHashSet>` - The equivalent return type has changed from `Result<Vec<EntityMut>, QueryEntityError>` to `Result<EntityHashMap<EntityMut>, EntityFetchError>`. If necessary, you can still convert the `EntityHashMap` into a `Vec`.
# Objective Seemed to have missed the export of `DynamicComponentFetch` from #15593. `TryFromFilteredError` which is returned by `impl TryFrom<FiliteredEntityMut/Ref> for EntityRef/Mut` also seemed to have been missing. ## Solution Export both of them.
# Objective Seemed to have missed the export of `DynamicComponentFetch` from #15593. `TryFromFilteredError` which is returned by `impl TryFrom<FiliteredEntityMut/Ref> for EntityRef/Mut` also seemed to have been missing. ## Solution Export both of them.
# Objective Seemed to have missed the export of `DynamicComponentFetch` from bevyengine#15593. `TryFromFilteredError` which is returned by `impl TryFrom<FiliteredEntityMut/Ref> for EntityRef/Mut` also seemed to have been missing. ## Solution Export both of them.
# Objective Seemed to have missed the export of `DynamicComponentFetch` from bevyengine#15593. `TryFromFilteredError` which is returned by `impl TryFrom<FiliteredEntityMut/Ref> for EntityRef/Mut` also seemed to have been missing. ## Solution Export both of them.
Objective
EntityRef::get_components_dyn()andEntityMut::get_components_dyn()to query multiple components #15577Solution
The following functions can now also take multiple component IDs and return multiple pointers back:
EntityRef::get_by_idEntityMut::get_by_idEntityMut::into_borrow_by_idEntityMut::get_mut_by_idEntityMut::into_mut_by_idEntityWorldMut::get_by_idEntityWorldMut::into_borrow_by_idEntityWorldMut::get_mut_by_idEntityWorldMut::into_mut_by_idIf you pass in X, you receive Y:
ComponentId, receive a singlePtr/MutUntyped[ComponentId; N](array), receive a[Ptr; N]/[MutUntyped; N]&[ComponentId; N](array), receive a[Ptr; N]/[MutUntyped; N]&[ComponentId](slice), receive aVec<Ptr>/Vec<MutUntyped>&HashSet<ComponentId>, receive aHashMap<ComponentId, Ptr>/HashMap<ComponentId, MutUntyped>Testing
Migration Guide
Result<_, EntityComponentError>instead of aOption<_>:EntityRef::get_by_id,EntityMut::get_by_id,EntityMut::into_borrow_by_id,EntityMut::get_mut_by_id,EntityMut::into_mut_by_id,EntityWorldMut::get_by_id,EntityWorldMut::into_borrow_by_id,EntityWorldMut::get_mut_by_id,EntityWorldMut::into_mut_by_id