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

One-shot systems through Commands and Schedules #7707

Closed
Pascualex opened this issue Feb 16, 2023 · 4 comments
Closed

One-shot systems through Commands and Schedules #7707

Pascualex opened this issue Feb 16, 2023 · 4 comments
Labels
A-ECS Entities, components, systems, and events C-Enhancement A new feature D-Complex Quite challenging from either a design or technical perspective. Ask for help! X-Controversial There is active debate or serious implications around merging this PR
Milestone

Comments

@Pascualex
Copy link
Contributor

Pascualex commented Feb 16, 2023

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

I've found other attempts at solving this problem, it's well explained in #4090. Basically I want to be able to use the command pattern with SystemParams.

Commands should also keep their ability to trigger behavior associated with concrete data.

// behavior not associated with concrete command data
commands.add(write_hello_world); // function command
commands.add(WriteHelloWorld); // struct command

// behavior associated with concrete command data
commands.add(WriteNumber(42)); // static function command doesn't work

And IMO the command pattern should also support adding commands to the command queue within commands. This is one of their main advantages compared to events. I believe this is currently completely supported so we just need to keep the behavior as is.

What solution would you like?

In an ideal world you would be able to simply add your SystemParams to the write function.

struct WriteNumber(i32);
impl Command for WriteNumber {
    fn write(self, mut numbers: ResMut<Numbers>) {
        numbers.push(self.0);
    }
}

This is obviously not possible because it doesn't comply with the write signature defined in the Command trait. I'm not sure if it would be possible to do it with some tricks and magic and then register the command so that its SystemState gets cached.

Anyhow, the next best solution I can think off is to allow systems to take a command as an input.

app.register_command(write_number);
struct WriteNumber(i32);
fn write_number(command: In<WriteNumber>, mut numbers: ResMut<Numbers>) {
    numbers.push(command.0);
}

The closest thing I've been able to do to implement this solution is through schedules and resources.

First, the code that supports this hacky solution.

struct SystemCommand<T: 'static + Send + Sync>(T);

impl<T: 'static + Send + Sync> Command for SystemCommand<T> {
    fn write(self, world: &mut World) {
        world.insert_resource(CommandIn(self.0));
        world.run_schedule(CommandScheduleLabel::<T>::default());
        world.remove_resource::<CommandIn<T>>();
    }
}

#[derive(Resource)]
struct CommandIn<T: 'static + Send + Sync>(T);

#[derive(ScheduleLabel)]
struct CommandScheduleLabel<T: 'static + Send + Sync> {
    phantom_data: PhantomData<T>,
}

// Default, Clone, PartialEq, Eq, Hash and Debug implementations for CommandScheduleLabel

trait RegisterCommand {
    fn register_command<T: 'static + Send + Sync, P>(
        &mut self,
        system: impl IntoSystemConfig<P>,
    ) -> &mut Self;
}

impl RegisterCommand for App {
    fn register_command<T: 'static + Send + Sync, P>(
        &mut self,
        system: impl IntoSystemConfig<P>,
    ) -> &mut Self {
        self.init_schedule(CommandScheduleLabel::<T>::default())
            .add_system_to_schedule(CommandScheduleLabel::<T>::default(), system)
    }
}

And a simple example using this solution.

fn main() {
    App::new()
        .init_resource::<Numbers>()
        // ideally: register_command(count_to)
        .register_command::<CountTo, _>(count_to)
        .register_command::<WriteNumber, _>(write_number)
        .add_startup_systems((add_commands, apply_system_buffers, read_numbers).chain())
        .run();
}

#[derive(Resource, Default, Deref, DerefMut)]
struct Numbers(Vec<i32>);

struct CountTo(i32);

struct WriteNumber(i32);

fn add_commands(mut commands: Commands) {
    // ideally: commands.add(CountTo(3));
    commands.add(SystemCommand(CountTo(3)));
    commands.add(SystemCommand(WriteNumber(100)));
    commands.add(SystemCommand(CountTo(2)));
}

// ideally: command: In<CountTo>
fn count_to(command: Res<CommandIn<CountTo>>, mut commands: Commands) {
    let CountTo(number) = command.0;
    for i in 1..=number {
        commands.add(SystemCommand(WriteNumber(i)));
    }
}

fn write_number(command: Res<CommandIn<WriteNumber>>, mut numbers: ResMut<Numbers>) {
    let WriteNumber(number) = command.0;
    numbers.push(number);
}

fn read_numbers(numbers: Res<Numbers>) {
    for number in &**numbers {
        println!("{number}");
    }
}

The output of the program is consistent with the behavior commands should have when inserting other commands.

1 2 3 100 1 2

As you can probably tell, I'm not super familiar with the Bevy internals and my solution is only interacting with its surface. I would love to read your suggestions on how to improve this and your overall feedback on this type of solution.

The most obvious concerns right now are getting the "ideal" syntax working and analyzing the performance impact of running all these different schedules. To avoid regressions we could have ExclusiveCommands to reproduce the current behavior of taking the whole world with no need for an schedule.

What alternative(s) have you considered?

There are many other alternatives, but none of them fit these particular requirements.

I apologize if I've misrepresented any of these solutions out of ignorance.

Additional context

If you want to experiment with this I've set up a small repo you can fork:
https://github.com/pascualex/bevy_system_commands

@Pascualex Pascualex added C-Enhancement A new feature S-Needs-Triage This issue needs to be labelled labels Feb 16, 2023
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events D-Complex Quite challenging from either a design or technical perspective. Ask for help! X-Controversial There is active debate or serious implications around merging this PR and removed S-Needs-Triage This issue needs to be labelled labels Feb 16, 2023
@alice-i-cecile alice-i-cecile added this to the 0.11 milestone Feb 16, 2023
@nicopap
Copy link
Contributor

nicopap commented Feb 17, 2023

Saying "I want a way to add commands to the command queue within commands" would help understand better what you are requesting 😉

Hmm, maybe I'm mistaken. You might be asking for a way to use commands to manipulate the schedule graph, adding and removing systems. I would advise against it, or designing your game around a similar concept. Here are the cons:

  • Decoupling control flow from code, results in difficult to understand code. If your system is added in a command in system2, and spawns system3, you'll have a bad time hunting through your code to figure out why a certain code path was triggered. I think limiting the chain of system trigger to a single step is a worthwhile limitation.
  • Rust is not friendly to callback-based APIs. Using the ECS as a workaround for ownership management sure works, but you lose type safety (for example, you'll have to depend on the system execution order to make a resource available before another system runs, etc.)
  • This would require re-computing your schedules every time it is update, a fairly costly operation.
  • This will reduce system parallelization

You can already use SystemParam in contexts where you "only" have access to a &mut World, use the SystemState API. I don't think being able to directly use SystemParam for the write method add much value, since you already can use SystemState. I understand it might be interesting to avoid the responsibility of handling a &mut World so here is a proposition:

struct SystemCommandCache<T: SystemParam>(T::State);
trait SystemCommand {
  type Param: SystemParam;
  fn sys_write(params: SystemParamItem<Self::Param>);
}
impl<T: SystemCommand> Command for T {
  fn write(self, world: &mut World) {
    if !world.contains_resource::<SystemCommandCache<T::Param>>() {
     // default system state value
    }
    world.scope(|mut cache: Mut<SystemCommandCache<T::Param>>, world| {
      let mut system_state: SystemState<T::Param> = SystemState::new(world);
      let mut command_write_params = system_state.get_mut(world);
      self.sys_write(command_write_params);
      // TODO: caching as described in the SystemState docs
    });
  }
}

This definitively doesn't compile, but I'm fairly certain with some work you can get something like that compiling.

Now there is still the issue of applying commands afterward. I'm not sure when commands added through your system here will be ran. And if there is a way to immediately trigger a command flush at the exit of a command flush.

I doubt it's a good idea to manipulate the command queue within a command, Trivially, one can deduce it is going to be very challenging to implement, especially with regard to exclusive mutable access. Looping the command application seems a bit more complex than it is worth.

I've a few prototype with an unreasonable amount of trait shenanigans, one involves literally storing systems in Components (yes it's possible 😄) so maybe you'll find some solace in crates I might publish in the future.

@Pascualex
Copy link
Contributor Author

Pascualex commented Feb 17, 2023

Saying "I want a way to add commands to the command queue within commands" would help understand better what you are requesting 😉

I've updated the original post, thank you.

Your criticism is fair, let me address it to better explain my use case.

Hmm, maybe I'm mistaken. You might be asking for a way to use commands to manipulate the schedule graph, adding and removing systems. I would advise against it, or designing your game around a similar concept. Here are the cons:

  • Decoupling control flow from code, results in difficult to understand code. If your system is added in a command in system2, and spawns system3, you'll have a bad time hunting through your code to figure out why a certain code path was triggered. I think limiting the chain of system trigger to a single step is a worthwhile limitation.

I know Bevy offers great tools to create game simulations. Working with schedules, system ordering and timesteps is great and the most ergonomic implementation I've found for simulations of this kind.

But I'm building an editor and not a game. This should be a fully supported use case because:

  • Many games include complex editors with advanced functionality such as command undo/redo.
  • Bevy wants to build their own editor using the engine.

I'd have to think more about it, but the command pattern might also be a great way to implement turn based games.

  • Rust is not friendly to callback-based APIs. Using the ECS as a workaround for ownership management sure works, but you lose type safety (for example, you'll have to depend on the system execution order to make a resource available before another system runs, etc.)
  • This would require re-computing your schedules every time it is update, a fairly costly operation.
  • This will reduce system parallelization

Some use cases are inherently very difficult to parallelize and are willing to take the performance hit for other kind of guarantees. I want to skip the schedules completely and just apply changes to the world in a sequential manner, without having a gigantic system that needs to query all kinds of stuff.

Commands are the perfect fit, but currently less ergonomic than systems. So this proposal is just trying to keep the exact same functionality that commands have right now, but making them more ergonomic.

As you can see in the "alternatives" section, I'm not the only one who wants this (or something very similar) and many others have proposed solutions to this problem.

You can already use SystemParam in contexts where you "only" have access to a &mut World, use the SystemState API. I don't think being able to directly use SystemParam for the write method add much value, since you already can use SystemState. I understand it might be interesting to avoid the responsibility of handling a &mut World so here is a proposition:

struct SystemCommandCache<T: SystemParam>(T::State);
trait SystemCommand {
  type Param: SystemParam;
  fn sys_write(params: SystemParamItem<Self::Param>);
}
impl<T: SystemCommand> Command for T {
  fn write(self, world: &mut World) {
    if !world.contains_resource::<SystemCommandCache<T::Param>>() {
     // default system state value
    }
    world.scope(|mut cache: Mut<SystemCommandCache<T::Param>>, world| {
      let mut system_state: SystemState<T::Param> = SystemState::new(world);
      let mut command_write_params = system_state.get_mut(world);
      self.sys_write(command_write_params);
      // TODO: caching as described in the SystemState docs
    });
  }
}

This definitively doesn't compile, but I'm fairly certain with some work you can get something like that compiling.

I know the SystemState API can help here and I appreciate your example (in fact I'd also thought of something along those lines but hadn't actually coded it). But surely you agree that those solutions aren't quite as ergonomic as systems, where your arguments are provided automatically without any explicit reference to this API.

Now there is still the issue of applying commands afterward. I'm not sure when commands added through your system here will be ran. And if there is a way to immediately trigger a command flush at the exit of a command flush.

I doubt it's a good idea to manipulate the command queue within a command, Trivially, one can deduce it is going to be very challenging to implement, especially with regard to exclusive mutable access. Looping the command application seems a bit more complex than it is worth.

I was pleasantly surprised to find out that the command stack works exactly as I would expect it to work in this regard. You can see it in my example:

1. I add commands: [CountTo(3), WriteNumber(100), CountTo(2)]
Stack: [head: CountTo(3), WriteNumber(100), CountTo(2)]
2. CountTo(3) is processed and inserts [WriteNumber(1), WriteNumber(2), WriteNumber(3)]
Stack: [head: WriteNumber(1), WriteNumber(2), WriteNumber(3), WriteNumber(100), CountTo(2)]
3. WriteNumber(1, 2, 3, 100) are processed
Stack: [head: CountTo(2)]
4. CountTo(2) is processed and inserts [WriteNumber(1), WriteNumber(2)]
Stack: [head: WriteNumber(1), WriteNumber(2)]
5. WriteNumber(1, 2) are processed

There is no need for extra flushes. If any of these commands inserted a Despawn command, it would also get processed at the right moment.

I've a few prototype with an unreasonable amount of trait shenanigans, one involves literally storing systems in Components (yes it's possible 😄) so maybe you'll find some solace in crates I might publish in the future.

I'll keep an eye out, trait shenanigans can be very tricky :)

@Pascualex
Copy link
Contributor Author

Pascualex commented Feb 17, 2023

Inspired by the @nicopap comment and @alice-i-cecile work in #4090 I've been able to improve the API. But this is still a hacky implementation because I don't fully understand Bevy internals.

The new code supporting the implementation no longer uses schedules (which renders my title outdated 😩).

// I need to use the new type pattern to add the new functionality
struct SystemCommand<T: 'static + Send + Sync>(T);
impl<T: 'static + Send + Sync> Command for SystemCommand<T> {
    fn write(self, world: &mut World) {
        world.resource_scope(|world, mut cache: Mut<SystemCommandCache<T>>| {
            cache.0.run(self.0, world);
            cache.0.apply_buffers(world);
        });
    }
}

// this is the main reason to call this solution hacky
// SystemTypeIdLabel and a centralized system registry are likely a better solution
// (see #4090 for more details)
#[derive(Resource)]
struct SystemCommandCache<T: 'static + Send + Sync>(Box<dyn System<In = T, Out = ()>>);

// implemented as a trait only for convenience
pub trait RegisterSystemCommand {
    fn register_system_command<T: 'static + Send + Sync, P>(
        &mut self,
        system: impl IntoSystem<T, (), P>,
    ) -> &mut Self;
}
impl RegisterSystemCommand for App {
    fn register_system_command<T: 'static + Send + Sync, P>(
        &mut self,
        system: impl IntoSystem<T, (), P>,
    ) -> &mut Self {
        let mut system = IntoSystem::into_system(system);
        system.initialize(&mut self.world);
        self.insert_resource(SystemCommandCache(Box::new(system)))
    }
}

// implemented as a trait only for convenience
pub trait AddSystemCommand {
    fn add_system_command<T: 'static + Send + Sync>(&mut self, command: T);
}
impl<'w, 's> AddSystemCommand for Commands<'w, 's> {
    fn add_system_command<T: 'static + Send + Sync>(&mut self, command: T) {
        self.add(SystemCommand(command));
    }
}

And the same example as before with the new improved API.

fn main() {
    App::new()
        .init_resource::<Numbers>()
        .register_system_command(count_to)
        .register_system_command(write_number)
        .add_startup_systems((add_commands, apply_system_buffers, read_numbers).chain())
        .run();
}

#[derive(Resource, Default, Deref, DerefMut)]
struct Numbers(Vec<i32>);

fn add_commands(mut commands: Commands) {
    commands.add_system_command(CountTo(3));
    commands.add_system_command(WriteNumber(100));
    commands.add_system_command(CountTo(2));
}

struct CountTo(i32);
fn count_to(In(CountTo(number)): In<CountTo>, mut commands: Commands) {
    for i in 1..=number {
        commands.add_system_command(WriteNumber(i));
    }
}

struct WriteNumber(i32);
fn write_number(In(WriteNumber(number)): In<WriteNumber>, mut numbers: ResMut<Numbers>) {
    numbers.push(number);
}

fn read_numbers(numbers: Res<Numbers>) {
    for number in &**numbers {
        println!("{number}");
    }
}

@Pascualex
Copy link
Contributor Author

I'm going to close this for now in favor of #7999. See run_system_with_input in the "Future work" section for a similar API to the one proposed here.

@Pascualex Pascualex closed this as not planned Won't fix, can't repro, duplicate, stale Mar 13, 2023
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-Enhancement A new feature D-Complex Quite challenging from either a design or technical perspective. Ask for help! X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

No branches or pull requests

3 participants