run a system from a command

[mockersf]

Fixes #2192

Can run a system from a command.

This adds a command:

commands.run_system(sync_system.system());

[TheRawMeatball]

Why the extra refcell and mutex?

[Ratysz]

This looks like it's one-shot, I think it's safe to take ownership when executing.

What about .config(), how does that behave?

[NathanSWard]

So a few things with this.

1. I don't think we necessarily want to have this as pub since it's more of an implementation detail.

2. as a small optimization, we could optionally declare the struct as such:

struct RunSystem {
    system: UnsafeCell<Box<dyn System<In = (), Out = ()>>>,
}

unsafe impl Sync for RunSystem {}

IIUC, we can safely use UnsafeCell and unsafe impl Sync since we know no data races are possible as Commands have exclusive mutable access to the World.

Note: I'm not necessarily advocating for the unsafe path, however, it is worth considering since then we don't have to worry about the overhead that Mutex and RefCell introduce.

[NathanSWard]

This is assuming you even need the UnsafeCell in the first place...?

[mockersf]

I don't think we necessarily want to have this as pub since it's more of an implementation detail.

All commands are pub, so I would prefer to continue that way

as a small optimization, we could optionally declare the struct as such

I would prefer to avoid unsafe unless it's a clear gain in perf? But I don't think perf are a big issue for one off systems

[NathanSWard]

Cool, the pub thing makes sense!

Though now that we removed the RefCell, I'd still prefer the unsafe impl Sync for RunSystem and remove the Mutex, however I don't feel super strongly about this.

[NathanSWard]

Looking at this makes it seem like a copy paste error in my head haha 😅
Could we change this to be like for _ in 0..5 or something similar?

[mockersf]

It is a copy paste! from

bevy/examples/ecs/system_param.rs

Lines 33 to 35 in 653c103

	commands.spawn().insert(Player);
	commands.spawn().insert(Player);
	commands.spawn().insert(Player);

But I removed one just to be different

[NathanSWard]

I still prefer the for - in syntax, but I guess I'm ok with how it is.

[mockersf]

Why the extra refcell and mutex?

systems need to be mut to be initialised and executed, and we don't have a mut command.
I googled interior mutability and https://doc.rust-lang.org/book/ch15-05-interior-mutability.html was the first it so didn't think much about it. The RefCell doesn't look needed indeed 👍

[mockersf]

What about .config(), how does that behave?

it works fine, added in the example 👍

[TheRawMeatball]

we don't have a mut command

Can you explain? Why wouldn't this work?

impl Command for RunSystem {
    fn write(mut self: Box<Self>, world: &mut World) {
        self.system.initialize(world);
        self.system.run((), world);
    }
}

[TheRawMeatball]

Also, should we maybe just implement command for FnOnce(&mut World) instead of dealing with wrapping stuff in systems?

[Ratysz]

Why not both?

[mockersf]

Can you explain? Why wouldn't this work?

Because that's not the signature of the trait function but it seems that's not a problem... so 👍

[Ratysz]

self is just a binding, you can declare it mutable on your own. It's the same as doing let mut this = self; as the first thing in the function's scope.

[alice-i-cecile]

That's an extremely simple implementation, wow.

I can expand more on how you might use this in the Bevy book (probably an advanced example) down the line.

[NathanSWard]

This is a bit of bikeshedding, but I feel the name run_system doesn't really express the intent of only running the system once. I could see someone easily getting confused thinking this adds the system to the overall app and expects is to execute each frame.
Perhaps we could use run_once or one_shot, run_one_shot or something to that extent.

[alice-i-cecile]

one_shot feels most natural to me, but I think it's not going to be clear to ESL users or those outside of gaming culture.

run_once is probably best.

[mockersf]

I think it should still have system in the name:

• makes it clearer and parallel with add_system
• the command struct name should mirror the method on commands, and I think a RunOnce is very not clear from the name

run_system_once? run_one_shot_system?

[MinerSebas]

What happens if the provided system calls commands itself?
Do they get ignored and if not when are they executed? Directly after the system finishes? After all regular commands were applied?

[mockersf]

What happens if the provided system calls commands itself?

They (now) get executed directly after the system finishes (~90% sure?)

[Ratysz]

Does this work with omitting .system()?

[mockersf]

of course it does 🥷

[wilk10]

Is it ok to ask for an update on this? (no presh though, of course).
i would totally love to be able to use this.
also, i think i prefer run_system_once as well

[alice-i-cecile]

So @wilk10 @zoeycode, IMO we're mostly waiting on consensus about "is this a feature we want to support, and for what use cases". This feature has some potential for footguns:

• the delayed effect of commands may be hard to reason about
• commands are relatively slow and executed in a single-threaded way right now
• this could result in bizarre spaghetti code without developer discipline
• we don't have great tools to control or debug the execution order of commands yet

That said, this is trivial to implement as an end user who's familiar with the ECS internals: there's no special engine access needed. So perhaps we should take the time to do it right.

What use cases would you like to use this for? That would be very useful input for us to both shape the feature and the supporting docs.

[zyllian]

My primary use case is UI code. Right now, one-offs are handled through events, but it requires explicit ordering to ensure changes go through on the same frame, but this gets more and more difficult the more levels you go through (for instance a popup which only appears under certain circumstances, otherwise it advances immediately).

More generally, I think being able to run a system once would be useful as a means of replacing systems which listen every frame for events that only rarely occur.

Also, I'm not sure how feasible this is, but it'd be nice to be able to pass arguments to one-off systems as well.

[alice-i-cecile]

My primary use case is UI code. Right now, one-offs are handled through events, but it requires explicit ordering to ensure changes go through on the same frame, but this gets more and more difficult the more levels you go through (for instance a popup which only appears under certain circumstances, otherwise it advances immediately).

Makes sense. bevyengine/rfcs#25 discusses a variant on this sort of pattern in the same UI context.

More generally, I think being able to run a system once would be useful as a means of replacing systems which listen every frame for events that only rarely occur.

Yep, see #2192, which inspired this issue :)

Also, I'm not sure how feasible this is, but it'd be nice to be able to pass arguments to one-off systems as well.

You could maybe use system chaining for this. Local resources + .config might work better though?

[mockersf]

Also, I'm not sure how feasible this is, but it'd be nice to be able to pass arguments to one-off systems as well.

You can with a Local<T> system parameter and calling .config() on your system, check the example in the PR:
https://github.com/bevyengine/bevy/pull/2234/files#diff-293c10ec0e0905212ce097b6badf947c206c62c6924c21f77086795ad824a203R22

[wilk10]

@alice-i-cecile Thanks for looking into this! My main interest is to allow something like:

use bevy::prelude::*;
use std::collections::VecDeque;

fn main() {
    App::build()
        .add_plugins(MinimalPlugins)
        .add_startup_system(spawn.system())
        .add_system(run.system())
        .run();
}

#[derive(PartialEq, Eq)]
enum MyData {
    A(u32),
    B,
}

struct DataQueue{
    queue: VecDeque<MyData>,
}

fn spawn(mut commands: Commands) {
    let mut queue = VecDeque::new();
    queue.extend(vec![MyData::A(42), MyData::B, MyData::A(17)]);
    commands.insert_resource(DataQueue{queue});
}

fn run(mut commands: Commands, data: Res<DataQueue>) {
    if let Some(my_data) = data.queue.front() {
        match my_data {
            MyData::A(_) => commands.run_system(system_a),
            MyData::B => commands.run_system(system_b),
        }
    }
}

fn system_a(mut data: ResMut<DataQueue>) {
    if let Some(my_data) = data.queue.pop_front() {
        if let MyData::A(inner) = my_data {
            println!("A: {}", inner);
        }
    }
}

fn system_b(mut data: ResMut<DataQueue>) {
    if let Some(my_data) = data.queue.pop_front() {
        if let MyData::B = my_data {
            println!("B");
        }
    }
}

AKA: arbitrarily running systems once in a dynamic order, accessing resources (or components) previously inserted.

I have a bunch of Actions to perform broken down into a variable number of SubActions (eg: Action is "leave room", SubActions: "walk to door", "open door", "leave"), and this pattern is really convenient to easily compose sequences of SubActions each executed by a specific system that cannot be generalised.

It's turn-based, so performance is less of an issue.

[cart]

I'm closing this for now because I don't think "constructing systems on demand and running them serially" is a pattern we should encourage generally (see the comments in #2192).

Happy to re-open if more arguments in favor of this direction are made, but I think we should be moving toward one (or both) of the following:

• Pre-adding labeled "on demand" systems to a schedule, which only run on a command.
• Adding the ability to, at runtime, queue up arbitrary systems to run within the schedule.

[alice-i-cecile]

I added a tool to ergonomically run one-off systems in #3839 for the purpose of assertions. They're explicitly only useful for testing though, which means that the arguments around code reuse and performance are not relevant.