bevy icon indicating copy to clipboard operation
bevy copied to clipboard

Unify ExclusiveSystemParams with normal SystemParams

Open ItsDoot opened this issue 1 year ago • 3 comments

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

  • Removed ExclusiveSystemParam, ExclusiveFunctionSystem, and ExclusiveSystemParamFunction.
  • Changed apply_deferred into a custom System implementation. As a byproduct, this reduces the memory usage of each apply_deferred instance significantly (from minimum 320 bytes to just 4 bytes).
    • This change is necessary because of the differing implementations of System for FunctionSystem and ExclusiveFunctionSystem.
  • &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 avatar Dec 03 '24 08:12 ItsDoot

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.

chescock avatar Dec 03 '24 13:12 chescock

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.

hymm avatar Dec 03 '24 16:12 hymm

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.

chescock avatar Dec 04 '24 01:12 chescock