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

Unify ExclusiveSystemParams with normal SystemParams #16622

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

ItsDoot
Copy link
Contributor

@ItsDoot ItsDoot commented Dec 3, 2024

Objective

Remove ExclusiveSystemParam and ExclusiveFunctionSystem in favor of just SystemParams, and define a system's exclusivity based on its parameters' SystemParam::is_exclusive return value.

Solution

Note: This PR relies on #16642 to be merged first.

  • Removed ExclusiveSystemParam, ExclusiveFunctionSystem, and ExclusiveSystemParamFunction.
  • &mut World, &mut QueryState<D, F>, and &mut SystemState<P> now implement SystemParam.
    • &mut World registers exclusive access, so it cannot be used alongside any parameter that accesses the world
    • &mut QueryState and &mut SystemState don't hold access to the world, so can be used alongside it
  • As SystemParams:
    • &mut QueryState now will automatically update its archetypes via SystemParam::new_archetype
    • &mut SystemState will now have its deferred mutations automatically applied via SystemParam::apply
  • DeferredWorld's init_state was updated to align with &World and &mut World

TODO

  • Observers cannot be allowed to use &mut World, but this PR in its current state allows it.
    • We should allow them to use DeferredWorld, however. We should consider splitting "exclusivity" into "structural exclusivity" and "deferred exclusivity".
  • Decide if &mut QueryState and &mut SystemState shouldn't do their things above automatically.
  • Consider adding new lints to bevy_lint to tell users not to use two &mut Worlds in a system or &mut World + Query/Res/etc

Testing

Most pre-existing tests are passing currently, will need to add some new ones before merging.


Showcase

Systems with &mut World no longer require it to be the first parameter (or second if using system input). It can now be placed anywhere in the function arguments:

// Before:
fn do_stuff(world: &mut World, local: Local<usize>, query: &mut QueryState<&Foo>) {}
// or
fn do_stuff_with(In(v): In<i32>, world: &mut World, local: Local<usize>, query: &mut QueryState<&Foo>) {}

// Now also possible:
fn do_stuff(local: Local<usize>, world: &mut World, query: &mut QueryState<&Foo>) {}
// or
fn do_stuff(local: Local<usize>, query: &mut QueryState<&Foo>, world: &mut World) {}
// or
fn do_stuff_with(In(v): In<i32>, local: Local<usize>, query: &mut QueryState<&Foo>, world: &mut World) {}
// or
fn do_stuff_with(In(v): In<i32>, local: Local<usize>, world: &mut World, query: &mut QueryState<&Foo>) {}

Migration Guide

TODO

@ItsDoot ItsDoot added 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 M-Needs-Release-Note Work that should be called out in the blog due to impact D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Dec 3, 2024
@ItsDoot ItsDoot added this to the 0.16 milestone Dec 3, 2024
@chescock
Copy link
Contributor

chescock commented Dec 3, 2024

How are you handling parameters like &Archetypes and Query<Entity> that have no Access but still rely on the structure of the world not changing? That is, a system like

fn do_something(world: &mut World, archetypes: &Archetypes, query: Query<Entity>) {}

would be UB, since the &mut World could change archetypes or add entities. But those params currently register no access and don't look different from Local, so I think your change might allow them.

I think that's why #4166 had enum WorldAccessLevel.

@hymm hymm added the X-Controversial There is active debate or serious implications around merging this PR label Dec 3, 2024
@hymm
Copy link
Contributor

hymm commented Dec 3, 2024

Added the controversial tag since there was a old pr with this approach, but we merged the current approach instead.

You should add tests for the current SystemParams that conflict with &mut World, since that's now a safety concern. Maybe 2 tests each, one with &mut World first and one with &mut World second.

crates/bevy_ecs/src/schedule/executor/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/schedule/executor/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/schedule/executor/mod.rs Outdated Show resolved Hide resolved
system_meta.component_access_set.write_all();
let mut access = Access::default();
access.read_all();
if !system_meta
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the existing code for DeferredWorld not check for conflicts? This change should probably pulled out if so as that's a safety concern.

crates/bevy_ecs/src/system/system_param.rs Show resolved Hide resolved

let mut filtered_access = FilteredAccess::default();

filtered_access.read_all();
Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like we shouldn't set any filtered_access as &mut World doesn't filter.

Copy link
Contributor

Choose a reason for hiding this comment

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

I feel like we shouldn't set any filtered_access as &mut World doesn't filter.

component_access_set is the one that actually gets checked for conflicts by things like Res and Query, so you do want to populate it. FilteredAccess::default() is unfiltered, so this is declaring read access with no filter. It could also be written FilteredAccess::matches_everything(), which may be more clear.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah right, I misremembered what the filtered_access was for. This is fine then.

@ItsDoot ItsDoot marked this pull request as draft December 3, 2024 17:59
@chescock
Copy link
Contributor

chescock commented Dec 4, 2024

A fun thing this would enable is putting &mut World inside a ParamSet so you can switch between full access and typed parameters. That's not any more powerful than putting the other params in a SystemState, but it might be more ergonomic in some cases.

Another thing this would enable is adding SystemParamBuilder<SystemState<P>> and SystemParamBuilder<QueryState<D, F>> implementations in the future, which would make it easier to use dynamic queries in exclusive systems.

@ItsDoot ItsDoot force-pushed the unify-exclusive-systems branch from 761edc6 to 9a77c73 Compare December 4, 2024 04:30
@ItsDoot ItsDoot force-pushed the unify-exclusive-systems branch from 9a77c73 to 150266d Compare December 4, 2024 04:32
github-merge-queue bot pushed a commit that referenced this pull request Dec 5, 2024
# Objective

- Required by #16622 due to differing implementations of `System` by
`FunctionSystem` and `ExclusiveFunctionSystem`.
- Optimize the memory usage of instances of `apply_deferred` in system
schedules.

## Solution

By changing `apply_deferred` from being an ordinary system that ends up
as an `ExclusiveFunctionSystem`, and instead into a ZST struct that
implements `System` manually, we save ~320 bytes per instance of
`apply_deferred` in any schedule.

## Testing

- All current tests pass.

---

## Migration Guide

- If you were previously calling the special `apply_deferred` system via
`apply_deferred(world)`, don't.
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 D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide M-Needs-Release-Note Work that should be called out in the blog due to impact S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged 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.

4 participants