Skip to content

Enable EntityRef::get_by_id and friends to take multiple ids and get multiple pointers back#15593

Merged
alice-i-cecile merged 8 commits intobevyengine:mainfrom
ItsDoot:generic-container
Oct 2, 2024
Merged

Enable EntityRef::get_by_id and friends to take multiple ids and get multiple pointers back#15593
alice-i-cecile merged 8 commits intobevyengine:mainfrom
ItsDoot:generic-container

Conversation

@ItsDoot
Copy link
Copy Markdown
Contributor

@ItsDoot ItsDoot commented Oct 2, 2024

Objective

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

@ItsDoot ItsDoot added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes D-Unsafe Touches with unsafe code in some way S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Oct 2, 2024
Copy link
Copy Markdown
Contributor

@djeedai djeedai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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> {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(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)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might look into doing the same thing for World later today

@MiniaczQ MiniaczQ self-requested a review October 2, 2024 15:35
@ItsDoot ItsDoot marked this pull request as ready for review October 2, 2024 17:58
@ItsDoot
Copy link
Copy Markdown
Contributor Author

ItsDoot commented Oct 2, 2024

Something worth considering: should we return a different error type for the immutable variants, as EntityComponentError::AliasedMutability is never returned for them?

Comment on lines +20 to +22
/// 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),
Copy link
Copy Markdown
Contributor

@MiniaczQ MiniaczQ Oct 2, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think one mutable + immutable refs is also invalid access

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@ItsDoot ItsDoot added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Oct 2, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Oct 2, 2024
@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Oct 2, 2024
Copy link
Copy Markdown
Contributor

@MiniaczQ MiniaczQ left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this &[ComponentId] casting necessary because it can't infer?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, it infers as &[ComponentId; 2] which would use the array impl.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could use &vec![x_id, y_id] it should be closer to a typical use case

Merged via the queue into bevyengine:main with commit 7c6057b Oct 2, 2024
@ItsDoot ItsDoot deleted the generic-container branch October 2, 2024 19:21
robtfm pushed a commit to robtfm/bevy that referenced this pull request Oct 4, 2024
…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`
github-merge-queue bot pushed a commit that referenced this pull request Oct 7, 2024
…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`.
github-merge-queue bot pushed a commit that referenced this pull request Nov 17, 2024
# 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.
mockersf pushed a commit that referenced this pull request Nov 17, 2024
# 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.
ecoskey pushed a commit to ecoskey/bevy that referenced this pull request Dec 2, 2024
# 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.
ecoskey pushed a commit to ecoskey/bevy that referenced this pull request Jan 6, 2025
# 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes D-Unsafe Touches with unsafe code in some way M-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add EntityRef::get_components_dyn() and EntityMut::get_components_dyn() to query multiple components

5 participants