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

Per world access lists and utils #17198

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

ElliottjPierce
Copy link

Objective

Fixes #16933

Note

This PR proposes a direct solution to #16933. The issue is technically still in design phase, and while there may be better solutions, I wanted to get the conversation going. This is the least complex solution I could think of.

Since this PR is sort of a base-line implementation to create some discussion, I haven't done mush polish. If the community thinks this sort of implementation is worth pursuing, I would want to put more time into polishing it up with more documentation, tests, and maybe an example.

Solution

I made four main changes to facilitate this. These could be split into smaller PRs later.

First, I added universal access lists. See crates/bevy_ecs/src/query/access.rs UniversalAccess. This stores multiple Accesss, one per relevant world. This lets access be tracked over multiple worlds. I used a SmallVec instead of a HashMap for pairing worlds to accesses because I suspect it will perform better, but this could always be changed.

Second, I converted SystemMeta and FilteredAccessSet to use UniversalAccess. I also updated the relevant references. This formed the backbone for the feature since it allows systems to be parallelized across multiple world accesses.

Third, I created an abstraction in crates/bevy_ecs/src/world/links.rs that makes it easy to nest or reference worlds in each other. This is what makes the feature useful.

Fourth, I added SystemParam::update_meta. I needed a way to notify nested parameters of archetype changes. This needed to be within the scope of System::update_archetype_component_access, otherwise SystemMeta may be changed in a way that conflicts with the scheduler or the nested param my not have been fully updated. Using this new interface, I was able to fully connect nested params.

As a note, the Link abstraction uses some sneaky privacy rules to safely access a resource in a way that is traditionally unsafe. Although I don't see any issues with it, I encourage it to be very carefully reviewed in particular.

Testing

I have done minimal testing myself. If the community likes this sort of implementation, I would want more tests for sure. If anyone has any requests for more tests, I'd be happy to add them.

The current CI passes for me. I'm on a M2 Mac.

As far as I can tell, though not polished, the current state of the PR is safe, so please try to break it.


Showcase

The following is an excerpt of the test I wrote for a proof on concept. See crates/bevy_ecs/src/world/links.rs for full context and refer to #16933 for the long-term vision.

let mut main = World::new();
main.insert_resource(MyRes);
main.link(NestedWorld(World::new()));
main.peek_link::<NestedWorld>()
    .unwrap()
    .deref_mut()
    .0
    .insert_resource(MyRes);

fn my_system(
    main_res: ResMut<MyRes>,
    main_query: Query<&mut MyComponent>,
    nested_res: Linked<NestedWorld, ResMut<MyRes>>,
    nested_query: Linked<NestedWorld, Query<&'static mut MyComponent>>,
) {
    // would do stuff here
}

main.run_system_once(my_system).unwrap();

let mut schedule = Schedule::default();
schedule.add_systems(my_system);
schedule.run(&mut main);

This kind of interface could make sub app extraction much much easier to work with (and likely faster from parallelization). In the long run, it may also enable multiple sub apps' extractions to be run in parallel. This would be huge for performance of projects with multiple big sub apps, like those using avian physics.

Migration Guide

Most notably, SystemMeta's access data types have changed. If the community is in favor of this kind of implementation, I can work on a more complete migration guide.

Final Note:

This is my first time contributing to bevy or open source in general, so I am extremely open to feedback. I have done my best to follow the contributing guidelines, but let me know if anything needs to be done differently. I'm very excited about this feature, so any feedback or requests are very welcome.

Design Decision: When tracking system exclusivity (ie: &mut World, etc), should we handle exclusive systems as exclusive to only their referenced world or as exclusive to their home world?  I chose exclusive to their home world, as it was easier to implement

To clarify, if there is a resource in the main world that holds another world, and a system requires exclusive access to the nested world, it currently gets exclusive access to the main world (the system's home world) too. To change this, we would need to return a list from `System::is_exclusive` instead of just a bool. This may be worth looking at in future PRs.

In the meantime, to fulfill the simple solution, I refactored conflicting system record into their own enum instead of having a four-element tuple with special meanings.

I did my best with the conflict reporting messages. It looks like there is some confusion in the pre-existing comments about how to handle conflicts from conflicting non-exclusive systems that have no conflicting components. The comment now on line 1422 describes one way this can happen.
This was very smooth and user-friendly. This is an indication to me that this may be the kind of implementation we should go for, though this implementation is still very much a prototype so far.
A note on Cargo.toml: cargo fmt changed the indent of the file. I'm assuming, this is because rustfmt.toml changed the spacing it preferred. I had to add the const_new feature to small_vec to maintain the const new api of UniversalAccess.
I didn't see a way around adding `SystemParam::update_meta`. It is not ideal, but it makes sense to have it, and we certainly needed it here.

I removed a todo comment looking for an early out for updating archetypes. This is because such an out would be impractical now that linked worlds would need to be checked.

I made the `Link` resource private to its file to enable some scary unsafe access. The comments have more details, but the idea is that this ensures it is only accessed from the file, and we can more easily enforce manual synchronization from there.
see updated docs for Link for more. I was previously prototyping a way to allow nested exclusive access, before realizing it was not necessary anyway since `&mut World` can't go in `Linked` anyway.
Copy link
Contributor

github-actions bot commented Jan 6, 2025

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events X-Controversial There is active debate or serious implications around merging this PR S-Needs-Review Needs reviewer attention (from anyone!) to move forward A-App Bevy apps and plugins C-Feature A new feature, making something new possible M-Needs-Release-Note Work that should be called out in the blog due to impact labels Jan 7, 2025
@ElliottjPierce
Copy link
Author

ElliottjPierce commented Jan 7, 2025

@PPakalns I would appreciate your input here too since you expressed interest in #16933. I want to walk away from this PR (merged or not) with a solid plan for implementing the feature.

@BenjaminBrienen BenjaminBrienen added the D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes label Jan 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-App Bevy apps and plugins A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes M-Needs-Release-Note Work that should be called out in the blog due to impact S-Needs-Review Needs reviewer attention (from anyone!) to move forward X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Systems with parallelizable mutable access to multiple worlds.
3 participants