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

Use UnsafeWorldCell to increase code quality for SystemParam #8174

Merged

Conversation

JoJoJet
Copy link
Member

@JoJoJet JoJoJet commented Mar 23, 2023

Objective

The type &World is currently in an awkward place, since it has two meanings:

  1. Read-only access to the entire world.
  2. Interior mutable access to the world; immutable and/or mutable access to certain portions of world data.

This makes &World difficult to reason about, and surprising to see in function signatures if one does not know about the interior mutable property.

The type UnsafeWorldCell was added in #6404, which is meant to alleviate this confusion by adding a dedicated type for interior mutable world access. However, much of the engine still treats &World as an interior mutable-ish type. One of those places is SystemParam.

Solution

Modify SystemParam::get_param to accept UnsafeWorldCell instead of &World. Simplify the safety invariants, since the UnsafeWorldCell type encapsulates the concept of constrained world access.


Changelog

SystemParam::get_param now accepts an UnsafeWorldCell instead of &World. This type provides a high-level API for unsafe interior mutable world access.

Migration Guide

For manual implementers of SystemParam: the function get_item now takes UnsafeWorldCell instead of &World. To access world data, use:

  • .get_entity(), which returns an UnsafeEntityCell which can be used to access component data.
  • get_resource() and its variants, to access resource data.

@JoJoJet JoJoJet added A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change labels Mar 23, 2023
@alice-i-cecile alice-i-cecile added this to the 0.11 milestone Mar 23, 2023
@alice-i-cecile alice-i-cecile added M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide and removed M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Mar 23, 2023
@alice-i-cecile
Copy link
Member

Currently at least this appears to be completely non-breaking.

@JoJoJet
Copy link
Member Author

JoJoJet commented Mar 23, 2023

Any manual implementations of SystemParam will be broken.

@alice-i-cecile alice-i-cecile added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Mar 23, 2023
@james7132 james7132 self-requested a review March 23, 2023 09:43
@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 Apr 1, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Apr 1, 2023
Merged via the queue into bevyengine:main with commit 3442a13 Apr 1, 2023
@JoJoJet JoJoJet deleted the system-param-unsafe-world-cell branch April 2, 2023 12:06
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-Code-Quality A section of code that is hard to understand or change M-Needs-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.

3 participants