Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Iterate ALL Entities in world (get EntityRef/EntityMut) #5504

Closed
Moulberry opened this issue Jul 31, 2022 · 5 comments · Fixed by #9419
Closed

Iterate ALL Entities in world (get EntityRef/EntityMut) #5504

Moulberry opened this issue Jul 31, 2022 · 5 comments · Fixed by #9419
Labels
A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible

Comments

@Moulberry
Copy link
Contributor

What problem does this solve or what need does it fill?

There are cases where direct access to EntityRef/EntityMut is needed in an iterating context

What solution would you like?

An archetype-based solution, ideally we could have a &EntityRef or &mut EntityMut in queries, which would automatically imply exclusive world access thinking. (#5496 (comment))

What alternative(s) have you considered?

Iterating Entity (the ID) and calling World::entity. This solution works, but it is unintuitive and suffers from worse performance than a built-in solution for iterating EntityRef/EntityMut

Additional context

Original discussion: #5496
@Moulberry Moulberry added C-Feature A new feature, making something new possible S-Needs-Triage This issue needs to be labelled labels Jul 31, 2022
@nicopap nicopap added A-ECS Entities, components, systems, and events and removed S-Needs-Triage This issue needs to be labelled labels Jul 31, 2022
@alice-i-cecile
Copy link
Member

Blocked on #4166. This work will require us to fully unify exclusive and ordinary systems.

@james7132 james7132 mentioned this issue Dec 4, 2022
Closed
@james7132
Copy link
Member

#6843 should add support to get an iterator of EntityRef over all entities given a World, however getting an Iterator of EntityMut is going to be highly unsound without LendingIterator to ensure only one EntityMut is valid at any given time.

bors bot pushed a commit that referenced this issue Dec 5, 2022
@james7132
Allow iterating over with EntityRef over the entire World (#6843)
e8c0df9 
# Objective
Partially addresses #5504. Allow users to get an `Iterator<Item = EntityRef<'a>>` over all entities in the `World`.

## Solution
Change `World::iter_entities` to return an iterator of `EntityRef` instead of `Entity`.

Not sure how to tackle making an `Iterator<Item = EntityMut<'_>>` without being horribly unsound. Might need to wait for `LendingIterator` to stabilize so we can ensure only one of them is valid at a given time.

---

## Changelog
Changed: `World::iter_entities` now returns an iterator of `EntityRef` instead of `Entity`.
ickshonpe pushed a commit to ickshonpe/bevy that referenced this issue Dec 6, 2022
@james7132 @ickshonpe
Allow iterating over with EntityRef over the entire World (bevyengine…
262e4a0 
…#6843)

# Objective
Partially addresses bevyengine#5504. Allow users to get an `Iterator<Item = EntityRef<'a>>` over all entities in the `World`.

## Solution
Change `World::iter_entities` to return an iterator of `EntityRef` instead of `Entity`.

Not sure how to tackle making an `Iterator<Item = EntityMut<'_>>` without being horribly unsound. Might need to wait for `LendingIterator` to stabilize so we can ensure only one of them is valid at a given time.

---

## Changelog
Changed: `World::iter_entities` now returns an iterator of `EntityRef` instead of `Entity`.
@james7132 james7132 mentioned this issue Dec 15, 2022
Merged
@jakobhellermann
Copy link
Contributor

however getting an Iterator of EntityMut is going to be highly unsound without LendingIterator to ensure only one EntityMut is valid at any given time.

This is just because we currently store a &mut World in EntityMut, right? In theory nothing prevents us from having two EntityMuts that refer to different entities that are alive at the same time?

@jakobhellermann
Copy link
Contributor

In theory nothing prevents us from having two EntityMuts that refer to different entities that are alive at the same time?

Correction: only if we remove EntityRef::world and EntityMut::into_world_mut/world_scope/world_mut/world.

@james7132
Copy link
Member

This is just because we currently store a &mut World in EntityMut, right? In theory nothing prevents us from having two EntityMuts that refer to different entities that are alive at the same time?

Due to the use of swap_remove and entity locations being updated, structural mutations (adding/removing components) to entities via EntityMut can and will invalidate the locations in other EntityRefs/EntityMuts and the metadata used to make the original iterator.

We'd likely need a mutable version of EntityRef that disallows structural mutations.

alradish pushed a commit to alradish/bevy that referenced this issue Jan 22, 2023
@james7132 @alradish
Allow iterating over with EntityRef over the entire World (bevyengine…
9236ad7 
…#6843)

# Objective
Partially addresses bevyengine#5504. Allow users to get an `Iterator<Item = EntityRef<'a>>` over all entities in the `World`.

## Solution
Change `World::iter_entities` to return an iterator of `EntityRef` instead of `Entity`.

Not sure how to tackle making an `Iterator<Item = EntityMut<'_>>` without being horribly unsound. Might need to wait for `LendingIterator` to stabilize so we can ensure only one of them is valid at a given time.

---

## Changelog
Changed: `World::iter_entities` now returns an iterator of `EntityRef` instead of `Entity`.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this issue Feb 1, 2023
@james7132 @ItsDoot
Allow iterating over with EntityRef over the entire World (bevyengine…
aa2a590 
…#6843)

# Objective
Partially addresses bevyengine#5504. Allow users to get an `Iterator<Item = EntityRef<'a>>` over all entities in the `World`.

## Solution
Change `World::iter_entities` to return an iterator of `EntityRef` instead of `Entity`.

Not sure how to tackle making an `Iterator<Item = EntityMut<'_>>` without being horribly unsound. Might need to wait for `LendingIterator` to stabilize so we can ensure only one of them is valid at a given time.

---

## Changelog
Changed: `World::iter_entities` now returns an iterator of `EntityRef` instead of `Entity`.
github-merge-queue bot pushed a commit that referenced this issue Jun 22, 2023
@james7132 @Carter0
@jakobhellermann @cart
Implement WorldQuery for EntityRef (#6960)
70f91b2 
# Objective
Partially address #5504. Fix #4278. Provide "whole entity" access in
queries. This can be useful when you don't know at compile time what
you're accessing (i.e. reflection via `ReflectComponent`).

## Solution
Implement `WorldQuery` for `EntityRef`. 

- This provides read-only access to the entire entity, and supports
anything that `EntityRef` can normally do.
- It matches all archetypes and tables and will densely iterate when
possible.
- It marks all of the ArchetypeComponentIds of a matched archetype as
read.
- Adding it to a query will cause it to panic if used in conjunction
with any other mutable access.
 - Expanded the docs on Query to advertise this feature.
 - Added tests to ensure the panics were working as intended.
 - Added `EntityRef` to the ECS prelude.

To make this safe, `EntityRef::world` was removed as it gave potential
`UnsafeCell`-like access to other parts of the `World` including aliased
mutable access to the components it would otherwise read safely.

## Performance
Not great beyond the additional parallelization opportunity over
exclusive systems. The `EntityRef` is fetched from `Entities` like any
other call to `World::entity`, which can be very random access heavy.
This could be simplified if `ArchetypeRow` is available in
`WorldQuery::fetch`'s arguments, but that's likely not something we
should optimize for.

## Future work
An equivalent API where it gives mutable access to all components on a
entity can be done with a scoped version of `EntityMut` where it does
not provide `&mut World` access nor allow for structural changes to the
entity is feasible as well. This could be done as a safe alternative to
exclusive system when structural mutation isn't required or the target
set of entities is scoped.

---

## Changelog
Added: `Access::has_any_write`
Added: `EntityRef` now implements `WorldQuery`. Allows read-only access
to the entire entity, incompatible with any other mutable access, can be
mixed with `With`/`Without` filters for more targeted use.
Added: `EntityRef` to `bevy::ecs::prelude`.
Removed: `EntityRef::world`

## Migration Guide
TODO

---------

Co-authored-by: Carter Weinberg <[email protected]>
Co-authored-by: Jakob Hellermann <[email protected]>
Co-authored-by: Carter Anderson <[email protected]>
@JoJoJet JoJoJet mentioned this issue Aug 11, 2023
Merged
github-merge-queue bot pushed a commit that referenced this issue Aug 29, 2023
@JoJoJet @alice-i-cecile
Allow disjoint mutable world access via EntityMut (#9419)
bc8bf34 
# Objective

Fix #4278
Fix #5504
Fix #9422

Provide safe ways to borrow an entire entity, while allowing disjoint
mutable access. `EntityRef` and `EntityMut` are not suitable for this,
since they provide access to the entire world -- they are just helper
types for working with `&World`/`&mut World`.

This has potential uses for reflection and serialization

## Solution

Remove `EntityRef::world`, which allows it to soundly be used within
queries.

`EntityMut` no longer supports structural world mutations, which allows
multiple instances of it to exist for different entities at once.
Structural world mutations are performed using the new type
`EntityWorldMut`.

```rust
fn disjoint_system(
     q2: Query<&mut A>,
     q1: Query<EntityMut, Without<A>>,
) { ... }

let [entity1, entity2] = world.many_entities_mut([id1, id2]);
*entity1.get_mut::<T>().unwrap() = *entity2.get().unwrap();

for entity in world.iter_entities_mut() {
    ...
}
```

---

## Changelog

- Removed `EntityRef::world`, to fix a soundness issue with queries.
+ Removed the ability to structurally mutate the world using
`EntityMut`, which allows it to be used in queries.
+ Added `EntityWorldMut`, which is used to perform structural mutations
that are no longer allowed using `EntityMut`.

## Migration Guide

**Note for maintainers: ensure that the guide for #9604 is updated
accordingly.**

Removed the method `EntityRef::world`, to fix a soundness issue with
queries. If you need access to `&World` while using an `EntityRef`,
consider passing the world as a separate parameter.

`EntityMut` can no longer perform 'structural' world mutations, such as
adding or removing components, or despawning the entity. Additionally,
`EntityMut::world`, `EntityMut::world_mut` , and
`EntityMut::world_scope` have been removed.
Instead, use the newly-added type `EntityWorldMut`, which is a helper
type for working with `&mut World`.

---------

Co-authored-by: Alice Cecile <[email protected]>
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this issue Jan 9, 2024
@JoJoJet @alice-i-cecile
Allow disjoint mutable world access via EntityMut (bevyengine#9419)
77d0070 
# Objective

Fix bevyengine#4278
Fix bevyengine#5504
Fix bevyengine#9422

Provide safe ways to borrow an entire entity, while allowing disjoint
mutable access. `EntityRef` and `EntityMut` are not suitable for this,
since they provide access to the entire world -- they are just helper
types for working with `&World`/`&mut World`.

This has potential uses for reflection and serialization

## Solution

Remove `EntityRef::world`, which allows it to soundly be used within
queries.

`EntityMut` no longer supports structural world mutations, which allows
multiple instances of it to exist for different entities at once.
Structural world mutations are performed using the new type
`EntityWorldMut`.

```rust
fn disjoint_system(
     q2: Query<&mut A>,
     q1: Query<EntityMut, Without<A>>,
) { ... }

let [entity1, entity2] = world.many_entities_mut([id1, id2]);
*entity1.get_mut::<T>().unwrap() = *entity2.get().unwrap();

for entity in world.iter_entities_mut() {
    ...
}
```

---

## Changelog

- Removed `EntityRef::world`, to fix a soundness issue with queries.
+ Removed the ability to structurally mutate the world using
`EntityMut`, which allows it to be used in queries.
+ Added `EntityWorldMut`, which is used to perform structural mutations
that are no longer allowed using `EntityMut`.

## Migration Guide

**Note for maintainers: ensure that the guide for bevyengine#9604 is updated
accordingly.**

Removed the method `EntityRef::world`, to fix a soundness issue with
queries. If you need access to `&World` while using an `EntityRef`,
consider passing the world as a separate parameter.

`EntityMut` can no longer perform 'structural' world mutations, such as
adding or removing components, or despawning the entity. Additionally,
`EntityMut::world`, `EntityMut::world_mut` , and
`EntityMut::world_scope` have been removed.
Instead, use the newly-added type `EntityWorldMut`, which is a helper
type for working with `&mut World`.

---------

Co-authored-by: Alice Cecile <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Allow disjoint mutable world access via EntityMut JoJoJet/bevy
5 participants
@james7132 @alice-i-cecile @Moulberry @jakobhellermann @nicopap