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

Migrate the rest of the engine to UnsafeWorldCell #8833

Merged
merged 23 commits into from
Jun 15, 2023

Conversation

JoJoJet
Copy link
Member

@JoJoJet JoJoJet commented Jun 12, 2023

Objective

Follow-up to #6404 and #8292.

Mutating the world through a shared reference is surprising, and it makes the meaning of &World unclear: sometimes it gives read-only access to the entire world, and sometimes it gives interior mutable access to only part of it.

This is an up-to-date version of #6972.

Solution

Use UnsafeWorldCell for all interior mutability. Now, &World always gives you read-only access to the entire world.


Changelog

TODO - do we still care about changelogs?

Migration Guide

Mutating any world data using &World is now considered unsound -- the type UnsafeWorldCell must be used to achieve interior mutability. The following methods now accept UnsafeWorldCell instead of &World:

  • QueryState: get_unchecked, iter_unchecked, iter_combinations_unchecked, for_each_unchecked, get_single_unchecked, get_single_unchecked_manual.
  • SystemState: get_unchecked_manual
let mut world = World::new();
let mut query = world.query::<&mut T>();

// Before:
let t1 = query.get_unchecked(&world, entity_1);
let t2 = query.get_unchecked(&world, entity_2);

// After:
let world_cell = world.as_unsafe_world_cell();
let t1 = query.get_unchecked(world_cell, entity_1);
let t2 = query.get_unchecked(world_cell, entity_2);

The methods QueryState::validate_world and SystemState::matches_world now take a WorldId instead of &World:

// Before:
query_state.validate_world(&world);

// After:
query_state.validate_world(world.id());

The methods QueryState::update_archetypes and SystemState::update_archetypes now take UnsafeWorldCell instead of &World:

// Before:
query_state.update_archetypes(&world);

// After:
query_state.update_archetypes(world.as_unsafe_world_cell_readonly());

@JoJoJet JoJoJet added A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide labels Jun 12, 2023
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Each of these commits makes sense to me. Pretty mechanical stuff.

Very happy to see validate_world get a more sensible API, and even happier to finally purge our code base of this pattern.

Copy link
Contributor

@jakobhellermann jakobhellermann left a comment

Choose a reason for hiding this comment

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

The changes look good to me.

I do wonder how we should balance the tradeoff between not having similar copies of methods for &World and UnsafeWorldCell vs not exposing people to implementation details.
For example in the render code, self.cameras.update_archetypes(world.as_unsafe_world_cell()) is unfortunate because it is actually used in a few places outside of just bevy_ecs internals.

Maybe it would be worth it to have update_archetypes(&World) and update_archetypes_unsafe_world_cell(UnsafeWorldCell<'_>) here?

Anyways, thanks for actually finishing the migration, I hope we can get this into 0.11.

@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 Jun 13, 2023
@JoJoJet
Copy link
Member Author

JoJoJet commented Jun 13, 2023

Maybe it would be worth it to have update_archetypes(&World) and update_archetypes_unsafe_world_cell(UnsafeWorldCell\u003C'_>) here?

IMO the ideal way of doing this would be to have World deref to UnsafeWorldCell, that way &World could implicitly coerce to &UnsafeWorldCell -- although we'd have to change the definition of UWC which may not be worth the churn at this point.

You're right though, just adding update_archetypes_unsafe_world_cell is probably the simplest way of improving this.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 15, 2023
Merged via the queue into bevyengine:main with commit db8d365 Jun 15, 2023
20 checks passed
@JoJoJet JoJoJet deleted the unsafe-world-cell-port branch June 15, 2023 02:12
github-merge-queue bot pushed a commit that referenced this pull request Oct 2, 2023
# Objective

We've done a lot of work to remove the pattern of a `&World` with
interior mutability (#6404, #8833). However, this pattern still persists
within `bevy_ecs` via the `unsafe_world` method.

## Solution

* Make `unsafe_world` private. Adjust any callsites to use
`UnsafeWorldCell` for interior mutability.
* Add `UnsafeWorldCell::removed_components`, since it is always safe to
access the removed components collection through `UnsafeWorldCell`.

## Future Work

Remove/hide `UnsafeWorldCell::world_metadata`, once we have provided
safe ways of accessing all world metadata.

---

## Changelog

+ Added `UnsafeWorldCell::removed_components`, which provides read-only
access to a world's collection of removed components.
ameknite pushed a commit to ameknite/bevy that referenced this pull request Oct 3, 2023
# Objective

We've done a lot of work to remove the pattern of a `&World` with
interior mutability (bevyengine#6404, bevyengine#8833). However, this pattern still persists
within `bevy_ecs` via the `unsafe_world` method.

## Solution

* Make `unsafe_world` private. Adjust any callsites to use
`UnsafeWorldCell` for interior mutability.
* Add `UnsafeWorldCell::removed_components`, since it is always safe to
access the removed components collection through `UnsafeWorldCell`.

## Future Work

Remove/hide `UnsafeWorldCell::world_metadata`, once we have provided
safe ways of accessing all world metadata.

---

## Changelog

+ Added `UnsafeWorldCell::removed_components`, which provides read-only
access to a world's collection of removed components.
ameknite pushed a commit to ameknite/bevy that referenced this pull request Oct 3, 2023
# Objective

We've done a lot of work to remove the pattern of a `&World` with
interior mutability (bevyengine#6404, bevyengine#8833). However, this pattern still persists
within `bevy_ecs` via the `unsafe_world` method.

## Solution

* Make `unsafe_world` private. Adjust any callsites to use
`UnsafeWorldCell` for interior mutability.
* Add `UnsafeWorldCell::removed_components`, since it is always safe to
access the removed components collection through `UnsafeWorldCell`.

## Future Work

Remove/hide `UnsafeWorldCell::world_metadata`, once we have provided
safe ways of accessing all world metadata.

---

## Changelog

+ Added `UnsafeWorldCell::removed_components`, which provides read-only
access to a world's collection of removed components.
ameknite pushed a commit to ameknite/bevy that referenced this pull request Oct 3, 2023
# Objective

We've done a lot of work to remove the pattern of a `&World` with
interior mutability (bevyengine#6404, bevyengine#8833). However, this pattern still persists
within `bevy_ecs` via the `unsafe_world` method.

## Solution

* Make `unsafe_world` private. Adjust any callsites to use
`UnsafeWorldCell` for interior mutability.
* Add `UnsafeWorldCell::removed_components`, since it is always safe to
access the removed components collection through `UnsafeWorldCell`.

## Future Work

Remove/hide `UnsafeWorldCell::world_metadata`, once we have provided
safe ways of accessing all world metadata.

---

## Changelog

+ Added `UnsafeWorldCell::removed_components`, which provides read-only
access to a world's collection of removed components.
regnarock pushed a commit to regnarock/bevy that referenced this pull request Oct 13, 2023
# Objective

We've done a lot of work to remove the pattern of a `&World` with
interior mutability (bevyengine#6404, bevyengine#8833). However, this pattern still persists
within `bevy_ecs` via the `unsafe_world` method.

## Solution

* Make `unsafe_world` private. Adjust any callsites to use
`UnsafeWorldCell` for interior mutability.
* Add `UnsafeWorldCell::removed_components`, since it is always safe to
access the removed components collection through `UnsafeWorldCell`.

## Future Work

Remove/hide `UnsafeWorldCell::world_metadata`, once we have provided
safe ways of accessing all world metadata.

---

## Changelog

+ Added `UnsafeWorldCell::removed_components`, which provides read-only
access to a world's collection of removed components.
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
# Objective

We've done a lot of work to remove the pattern of a `&World` with
interior mutability (bevyengine#6404, bevyengine#8833). However, this pattern still persists
within `bevy_ecs` via the `unsafe_world` method.

## Solution

* Make `unsafe_world` private. Adjust any callsites to use
`UnsafeWorldCell` for interior mutability.
* Add `UnsafeWorldCell::removed_components`, since it is always safe to
access the removed components collection through `UnsafeWorldCell`.

## Future Work

Remove/hide `UnsafeWorldCell::world_metadata`, once we have provided
safe ways of accessing all world metadata.

---

## Changelog

+ Added `UnsafeWorldCell::removed_components`, which provides read-only
access to a world's collection of removed components.
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-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide C-Code-Quality A section of code that is hard to understand or change 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.

None yet

3 participants