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.

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

Allow disjoint mutable world access via EntityMut #9419

Merged
merged 43 commits into from Aug 29, 2023

Conversation

JoJoJet
Copy link
Member

@JoJoJet JoJoJet commented Aug 11, 2023

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.

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.

@alice-i-cecile alice-i-cecile added C-Enhancement A new feature A-ECS Entities, components, systems, and events D-Complex Quite challenging from either a design or technical perspective. Ask for help! labels Aug 11, 2023
@JoJoJet JoJoJet added the A-Reflection Runtime information about types label Aug 11, 2023
@iiYese
Copy link
Contributor

iiYese commented Aug 12, 2023

I mostly left it because EntityRef::world is mildly useful sometimes

I would like identified cases of when. The API duplication is getting ridiculous now

  • &mut World
  • Commands
  • EntityCommands
  • EntityRef
  • EntityMut
  • EntityBorrow
  • EntityBorrowMut

Should try not to bloat this further until refactors on the horizon are possible. If no good argument can be made for or against keeping EntityRef then a push for its removal should be made to find one.

@JoJoJet
Copy link
Member Author

JoJoJet commented Aug 12, 2023

I mostly left it because EntityRef::world is mildly useful sometimes

I would like identified cases of when.

I like to think of EntityRef as a performance-optimized (Entity, &World) tuple, which can be useful any time you're working with direct world access (I have used it for this before). If we removed EntityRef::world, then a bunch of users would have to change some method signatures from fn(EntityRef) to fn(EntityRef, &World) -- not the biggest loss but still annoying.

That said, I'm open to doing it if we decide it's a good idea. I think the path with the least API breakages would be to remove EntityRef, and rename EntityBorrow -> EntityRef. This would open the question of what to call EntityBorrowMut, which would seem out of place without a corresponding EntityBorrow.

@JoJoJet
Copy link
Member Author

JoJoJet commented Aug 12, 2023

I've added some unit tests that are known to fail due to limitations in our current access model. Since the issue is with overly restrictive accesses, it will not cause undefined behavior, so I think it is okay to fix this issue later. Also, note that this issue already existed on the main branch with EntityRef queries.

@JoJoJet
Copy link
Member Author

JoJoJet commented Aug 13, 2023

I have merged EntityBorrow into EntityRef, and removed EntityRef::world.

@JoJoJet
Copy link
Member Author

JoJoJet commented Aug 13, 2023

I have also renamed EntityMut to EntityWorldMut, and renamed EntityBorrowMut to EntityMut. Since I've made so many changes, I've updated the PR description to give an overview of the current state of this PR.

@JoJoJet JoJoJet changed the title Add APIs for disjoint access to world entities Allow disjoint mutable world access via EntityMut Aug 13, 2023
crates/bevy_ecs/src/world/entity_ref.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/world/entity_ref.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@maniwani maniwani left a comment

Choose a reason for hiding this comment

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

This looks good to me. I mostly wanted to make sure the access is sound.

I think renaming the existing types is a little strange, but ig it's better for queries to get the types with shorter names. It's fine if there's some type duplication between "queried" and "direct-world" access though. We may find a need for an EntityWorldRef later.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Aug 14, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Aug 28, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 28, 2023
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Aug 28, 2023

I think there are subtle merge conflicts here @JoJoJet; can you rebase and verify things work locally?

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Aug 28, 2023
Merged via the queue into bevyengine:main with commit bc8bf34 Aug 29, 2023
20 checks passed
@JoJoJet JoJoJet deleted the entity-borrow branch August 29, 2023 00:02
@cart cart mentioned this pull request Oct 13, 2023
43 tasks
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
# 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 <alice.i.cecile@gmail.com>
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 A-Reflection Runtime information about types C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide C-Enhancement A new feature D-Complex Quite challenging from either a design or technical perspective. Ask for help! P-Unsound A bug that results in undefined compiler behavior 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
4 participants