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

Add an API for accessing world storages from UnsafeWorldCell #8280

Closed
wants to merge 14 commits into from

Conversation

JoJoJet
Copy link
Member

@JoJoJet JoJoJet commented Apr 1, 2023

Objective

Re-do of #8175.

UnsafeWorldCell is a type that provides interior mutable access to a world. Currently, the only way to access world data is through high-level methods. In order to make UnsafeWorldCell a full replacement for the interior-mutable-ish version of &World, we need a way of accessing the world's internal data storages.

Solution

Provide an API for accessing a world's internal data stores via UnsafeWorldCell. World accesses are gated behind unsafe, with the unsafe code deferred as late as possible to encourage clean code.

/// # Safety
/// `world` must have access to the `Score` resource.
unsafe fn get_score_unchecked(world: UnsafeWorldCell) -> Ptr {
    let score_id = world.components().resource_id::<Score>().unwrap();
    // SAFETY: Caller ensures we have access to `Score`.
    world.storages().resources.get(score_id).unwrap().get_data().unwrap()
                               ^^^^^^^^^^^^^ this is the unsafe part
}

Changelog

  • Added the UnsafeStorages API, for interior mutable access to a World's internal data stores via UnsafeWorldCell.

Future Work

Use this API in the engine. In particular, it will be useful when porting Query and WorldQuery over to using UnsafeWorldCell.

@JoJoJet JoJoJet added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events labels Apr 1, 2023
/// Accessing world data that do not have access to, or mutably accessing
/// data that you only have read-access to, is considered undefined behavior.
pub struct UnsafeStorages<'a> {
pub sparse_sets: UnsafeSparseSets<'a>,
Copy link
Member

Choose a reason for hiding this comment

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

Missing docs on these fields.

@@ -41,3 +43,71 @@ pub struct Storages {
/// Backing storage for `!Send` resources.
pub non_send_resources: Resources<false>,
}

/// Provides interior-mutable access to a world's internal data storages.
Copy link
Member

Choose a reason for hiding this comment

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

We should link to the critical concepts here. "interior mutability" and "undefined behavior" would be very nice to link out to for newcomers to Rust who run into this in the docs.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree. I imagine the rustonomicon will be a good starting point, but let me know if there are other resources you think would be worth including here.

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.

Overarching docs feedback:

  1. We should link to [Ticks] where referenced in the docs: this is very jargon.
  2. We need to explain why the Option that we return from most of this APIs could be None.

@jakobhellermann
Copy link
Contributor

Do we have a usecase for non-Unsafe SparseSets/Resources/...? Why not move the unsafety as far down as possible, to Column::get_* and ComponentSparseSet::get_*?

@JoJoJet
Copy link
Member Author

JoJoJet commented Apr 1, 2023

I was planning on removing/hiding those APIs in a follow-up, if it seems like a good idea after migrating the engine. For now, I want this PR to be non-breaking.

@jakobhellermann
Copy link
Contributor

I was planning on removing/hiding those APIs in a follow-up, if it seems like a good idea after migrating the engine. For now, I want this PR to be non-breaking.

Hm, I think if you plan to do it anyways, doing it now would unnecessary work for you, and we wouldn't need to review twice. Once for this PR, then again for moving the unsafe to get_*, and after that this code would be removed anyways.

@JoJoJet
Copy link
Member Author

JoJoJet commented May 10, 2023

I've been thinking about this off and on, and I think that a good way of handling this API would be:

  1. Add an UnsafeStorages API, like the one in this PR.
  2. Deprecate, and eventually make private, World::storages.

This would make UnsafeWorldCell and UnsafeStorages the only way to access the internal storages of a world. With Storages made private, we can freely simplify it and remove several of the repetitive API methods.

Thoughts?

@JoJoJet JoJoJet marked this pull request as draft May 10, 2023 21:47
@JoJoJet
Copy link
Member Author

JoJoJet commented May 10, 2023

After even more thought, I don't think this PR is the right direction.

@JoJoJet JoJoJet closed this May 10, 2023
@JoJoJet JoJoJet deleted the unsafe-storages-api branch September 20, 2023 05:24
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-Feature A new feature, making something new possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants