`SystemRunner` param - run systems inside other systems

[vil-mo]

What problem does this solve or what need does it fill?

Currently ways of running systems from other systems are very limited. This proposal aims to provide a way to easily run system as a SystemParam

What solution would you like?

Add a SystemParam - SystemRunner<T: System + FromWorld>

#[derive(Default)]
struct MySystem;
impl System for MySystem {
    In = In<i32>;
    Out = String;
    //...
}

fn running_my_system(mut runner: SystemRunner<MySystem>) {
    let output = runner.run(50);
    info!("Output of MySystem: {output}");
}

Unergonomic System implementation

To use this param, user would have to explicitly specify the type of the system they will use. Implementing System trait on it's own is hard - unsafe code is not something users want to deal with. FunctionSystems are the main way people are used to working with the bevy systems, but they don't implement FromWorld, and even if it did, you would have to either specify the marker or box every system.

So, it would be nice to have a macro that labels a function system with a specified type name.

#[system(MySystem)]
pub fn my_system(query: Query<&mut MyComponent>, resourse: Res<MyRes>) -> i32 {
    //...
}

expands to

pub struct MySystem(SystemState<(Query<'static, 'static, &'static mut MyComponent>, Res<'static, MyRes>)>);

impl FromWorld for MySystem {
    fn from_world(world: &mut World) -> Self {
        MySystem(SystemState::new(world))
    }
}

impl System for MySystem {
    //... Basically the same as FunctionSystem implementation
}

Needed controversial change

There is a conflict between System::update_archetype_component_access and SystemParam::new_archetype. You can't call update_archetype_component_access from new_archetype. Without that you wouldn't be able to call update_archetype_component_access on an inner system and simultaneously have access rights to run the system.

We can add a new_archetype method to System trait or update_archetype_component_access to SystemParam trait. We can pass UnsafeWorldCell to the new_archetype.

The best option in my opinion - add a new_archetype method to the System trait. There will be system-specific code for updating access. update_archetype_component_access will have a default implementation common for all systems. It will be basically the same as the current FunctionSystem's implementation. Systems would also need a method that exposes system's archetype generation for update_archetype_component_access to have default implementation. Method that exposes archetype generation should be fallible so that ZST systems can say that they don't need to update archetype access.

[andriyDev]

So first, to list out the ways you can run a system today:

1. Make your system take &mut World and call run_system_cached.
2. Run a system as a command using commands.register_system and comments.run_system
3. Don't make the function a system at all - just make it take borrows to the thing you need, and make your "outer" system take all the required function params as system params. You can even use QueryLens to make systems more broadly applicable.

It would be helpful to understand why these are not desirable.

---

As for the implementation ideas, can we do SystemRunner<my_system>? Function items are unique types, so I would expect this to work but maybe there's some hitch I'm missing.

[benfrankel]

There's a commands.run_system_cached as well.

[vil-mo]

@andriyDev 1 and 2 - Those methods require exclusive world access. Sometimes it can matter. 3 - It's not as ergonomic as having a system runner that abstracts you out of writing all those system params.

---

No, function items are not rust types

[vil-mo]

By the way, I've just managed to put the thought into text and #[system] macro is actually a good way to make run_system_cached. If we do this

run_system_cached<T: System + FromWorld>(&mut World);

And remove the ZST requirement - It would actually be more ergonomic version of run_system

registered_system_cached looks like this

pub fn register_system_cached<I, O, M, S>(&mut self, system: S) -> SystemId<I, O>
where
    I: SystemInput + 'static,
    O: 'static,
    S: IntoSystem<I, O, M> + 'static,
{
    const {
        assert!(
            size_of::<S>() == 0,
            "Non-ZST systems (e.g. capturing closures, function pointers) cannot be cached.",
        );
    }
    // ...

But systems are cached by <S as IntoSystem>::System type So it's a footgun. You can do this.

struct First;
impl IntoSystem for First {
    type System = MySystem;
    fn into_system(self) -> Self::System {
        MySystem::do_one_thing();
    }
}

struct Second;
impl IntoSystem for Second {
    type System = MySystem;
    fn into_system(self) -> Self::System {
        MySystem::do_other_thing();
    }
}

And if the result of the First is cached, then the result of the Second would be discarded and instead the First would run

So to solve this, it should either be changed so that systems are cached by the S type, but then you should preserve the ZST-ness of the input, and it just sounds wrong Or the above

[chescock]

Ooh, this would be really cool!

This might enable us to define pipe systems using ordinary function systems:

fn pipe<S1: System, S2: System>(input: S1::Input, runners: ParamSet<(SystemRunner<S1>, SystemRunner<S2>)>) -> S2::Out {
    let value = runners.p0().run(input);
    runners.p1().run(value)
}

[chescock]

The #[system(MySystem)] part seems like it will be controversial, too. You might be able to avoid it for the initial version if you make the systems generic and use a SystemParamBuilder. The downside is that you need &mut World to construct it, but that could be streamlined later.

// Define the builder
struct SystemRunnerParamBuilder<S: System>(S);

impl<S: System> SystemRunnerParamBuilder<S> {
    fn new<M>(s: impl IntoSystem<S::In, S::Out, M, System = S>) -> Self {
        Self(IntoSystem::into_system(s))
    }
}

unsafe impl<S: System> SystemParamBuilder<SystemRunner<S>> for SystemRunnerParamBuilder<S> {
    fn build(self, world: &mut World, meta: &mut SystemMeta) -> <SystemRunner<S> as SystemParam>::State {
        // Register the access and return the system as the runner's state
    }
}

// Use it to build a system
fn my_system(i: In<i32>) -> String {
    i.to_string()
}

fn running_my_system<S: System<In = In<i32>, Out = String>>(runner: SystemRunner<S>) {
    let output = runner.run(50);
    info!("Output of MySystem: {output}");
}

let mut world = World::new();
// `S` and `M` get inferred here from `my_system`
let system = (SystemRunnerParamBuilder::new(my_system),)
    .build_state(&mut world)
    .build_system(running_my_system);

[benfrankel]

And if the result of the First is cached, then the result of the Second would be discarded and instead the First would run

This is fine FWIW. MySystem is a ZST, so MySystem::do_one_thing() and MySystem::do_other_thing() are necessarily identical and do the same thing, so First and Second are equivalent and interchangeable as far as run_system_cached is concerned.

[vil-mo]

@benfrankel MySystem can be not ZST. Look at the implementation of the function - it checks only the type that implements IntoSystem, but not the actual system

It actually would be bad if it could only cache ZST's. What's the point of caching ZST? It would be the same as run_system_once

[benfrankel]

@benfrankel MySystem can be not ZST. Look at the implementation of the function - it checks only the type that implements IntoSystem, but not the actual system

Hm I remember that the ZST check is done on S because S::System will be something like a FunctionSystem which includes the system state and will never be a ZST, but I don't remember why it's cached by S::System instead of S. Maybe I forgot to change that to use S as well.

EDIT: I changed the ZST check from S::System to S in a small commit to fix CI, so I probably just forgot to change it for CachedSystemId<S::System> as well: https://github.com/bevyengine/bevy/pull/14920/commits/0c9535a48998240d98464840a5e9361f3063e9e1. Made a PR to fix this: https://github.com/bevyengine/bevy/pull/16694.

It actually would be bad if it could cache only ZST's. What's the point of caching ZST? It would be the same as run_system_once

I'm not 100% sure what you mean by this, you mean if the system had no system state there would be no reason to cache it?

[vil-mo]

you mean if the system had no system state there would be no reason to cache it

yes, that's what I meant

[alice-i-cecile]

Why couldn't we impl SystemParam for SystemData<MySystem>? That seems like the most direct approach and should be pretty easy, although I still don't really understand why this is valuable in the first place.

[chescock]

Why couldn't we impl SystemParam for SystemData<MySystem>?

What's SystemData? If you mean SystemState<P>, that doesn't have an actual function to run, just the state required to construct a SystemParam from the world.

[alice-i-cecile]

Sorry, I should have explained further.

struct SystemData<S: IntoSystem>{
    state: SystemState<S>,
   function: S
}

// Inherits the required metadata from S, which is constructed as the union of the system param inside that system
impl SystemParam for SystemData<S>{ todo!() }

[alice-i-cecile]

I think that one-shot systems with commands do a pretty solid job here: I don't think we should be adding more complexity / competing approaches just to enable more powerful generic programming, given that this should all be doable externally.