Skip to content

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::world_access_level return value.

Solution

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

  • 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-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide M-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.

@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 2 times, most recently 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.
ecoskey pushed a commit to ecoskey/bevy that referenced this pull request Jan 6, 2025
# Objective

- Required by bevyengine#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.
@ItsDoot ItsDoot force-pushed the unify-exclusive-systems branch from 0fe16c6 to ee98027 Compare January 30, 2025 03:13
@ItsDoot ItsDoot force-pushed the unify-exclusive-systems branch from f06d99c to ed7762e Compare February 4, 2025 02:19
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-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide M-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.

6 participants