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 as Commands -- allow commands to use SystemParams #7252

Closed
wants to merge 17 commits into from

Conversation

JoJoJet
Copy link
Member

@JoJoJet JoJoJet commented Jan 17, 2023

Objective

Fixes #2192.

Currently, anonymous one-shot commands can be defined as a closure accepting &mut World. Working with exclusive world access is quite cumbersome, and is usually only necessary if a command makes changes to a world's archetypes. In cases where a command simply needs to access and mutate world data, it is much more ergonomic to do so via SystemParams such as ResMut<> or Query.

Solution

  • Add the trait CommandSystemParam, which is a subset of SystemParam that excludes types that don't make sense for one-shot systems, such as Local<> or Commands.
  • Any systems consisting of CommandSystemParams may be used as a command.

Example

// One-shot systems are run by calling `Commands::add`.
commands.add(|main_character: Res<MainCharacter>,
              mut characters: Query<&mut Character>| {
    let main_character = characters.get_mut(main_character.id()).unwrap();
    main_character.do_thing();
});

TODO: Docs. I'd like to see the response to this PR before putting in the effort to write full documentation.

TODO: Look into storing "command systems" in a registry resource, to avoid duplicate initialization code. This will require benchmarking.

Changelog

TODO

Migration Guide

TODO

@JoJoJet JoJoJet added C-Enhancement A new feature A-ECS Entities, components, systems, and events X-Controversial There is active debate or serious implications around merging this PR labels Jan 17, 2023
@JoJoJet
Copy link
Member Author

JoJoJet commented Jan 17, 2023

@alice-i-cecile I don't need a full review or approval yet, but I'd like to get your opinion on this PR before I move forward with it.

@alice-i-cecile
Copy link
Member

I agree that we want a commands-powered API for one-shot systems :) Concerns with this approach though:

  1. This doesn't cache state, which means a) serious efficiency cost b) no change detection. This was a dealbreaker for Cart in my early experiments, which is what led to the design in Fast and correct one-shot systems with a system registry resource #4090.
  2. I think you really want a dedicated collection for systems that are supposed to run at the next command flush, in order to unlock future parallelism.
  3. I don't like the addition of yet another trait with CommandSystemParam. With 1 resolved, I don't think it's needed.

Excited to see work towards one-shot systems in one form or another, but I'm not sold on this approach.

@JoJoJet
Copy link
Member Author

JoJoJet commented Jan 17, 2023

  1. I think that the efficiency cost is overstated -- this approach has no overhead compared to using SystemState manually inside of a |&mut World| command. Also, I don't think change detection even makes sense within a one-shot system, so this isn't a loss IMO.
  2. This is probably a cleaner solution.

@JoJoJet JoJoJet closed this Jan 17, 2023
@alice-i-cecile
Copy link
Member

I think that the efficiency cost is overstated

Generally agreed, but not my requirement :p

Also, I don't think change detection even makes sense within a one-shot system, so this isn't a loss IMO.

Once you start looking at using one shot systems for UI callbacks this makes a lot of sense to me :)

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 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.

One-shot systems via Commands for scripting-like logic
2 participants