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

Fast and correct one-shot systems with a system registry resource #7999

Closed
wants to merge 69 commits into from

Conversation

Pascualex
Copy link
Contributor

@Pascualex Pascualex commented Mar 9, 2023

Objective

  • Working with exclusive world access is not always easy: in many cases, a standard system or three is more ergonomic to write, and more modularly maintainable.
  • For small, one-off tasks (commonly handled with scripting), running an event-reader system incurs a small but flat overhead cost and muddies the schedule.
  • Certain forms of logic (e.g. turn-based games) want very fine-grained linear and/or branching control over logic.
  • SystemState is not automatically cached, and so performance can suffer and change detection breaks.
  • Fixes One-shot systems via Commands for scripting-like logic #2192.
  • Partial workaround for API for dynamic management of systems (adding and removing at runtime) #279.

Solution

  • Adds a SystemRegistry resource to the World, which stores initialized systems keyed by their SystemSet.
  • Allows users to call world.run_system(my_system) and commands.run_system(my_system), without re-initializing or losing state (essential for change detection).
  • Add a Callback type to enable convenient use of dynamic one shot systems and reduce the mental overhead of working with Box<dyn SystemSet>.
  • Allow users to run systems based on their SystemSet, enabling more complex user-made abstractions.

Status

  • Only allow the registry to run single systems, instead of whole sets.

Future work

  • Parameterized one-shot systems would improve reusability and bring them closer to events and commands. The API could be something like run_system_with_input(my_system, my_input) and use the In SystemParam.
  • We should evaluate unification of commands and one-shot systems since they are two different ways to run logic on demand over a World.

Prior attempts

This PR continues the work done in #4090.

@Pascualex
Copy link
Contributor Author

@alice-i-cecile I don't like/understand indexing systems by their sets. I like the idea of moving systems out of schedules and into a centralized system registry, but IMO it should only be possible to run them by type.

The use-case of running multiple systems on demand seems better handled by schedules than by this "run in linear registration order".

I know that system labels and sets have been merged into a single concept. So I'm not sure what could be an alternative to sets but with a 1-1 relationship with systems.

@alice-i-cecile
Copy link
Member

IMO it should only be possible to run them by type.

Agreed here: we should only accept a impl Into<SystemTypeSet> and enforce uniqueness. I don't think there's a reasonable use case for "one shot systems but you have multiple distinct copies of the systems and want to keep their internal state distinct". In that case, the user should newtype them.

The use-case of running multiple systems on demand seems better handled by schedules than by this "run in linear registration order".

Strongly agreed.

@alice-i-cecile alice-i-cecile added this to the 0.11 milestone Mar 9, 2023
@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events X-Controversial There is active debate or serious implications around merging this PR labels Mar 9, 2023
@Pascualex
Copy link
Contributor Author

I'd like to unify the following commands:

pub struct RunSystemCommand<...> {
    _phantom_marker: PhantomData<M>,
    system: S,
}

// formerly RunSystemsBySetCommand
pub struct RunCallback {
    pub callback: Callback,
}

RunCallback stores the SystemTypeSet and requires preregistration, while RunSystemCommand stores the system itself and handles system registration. I don't see a strong reason to employ different strategies here. We could have Callback implement IntoSystem or just have run_callback call run_system if they employed the same strategy.

I personally favor preregistration for explicitness on the fact that this is being stored and cached.

use bevy_ecs::system::{Callback, SystemRegistryError};

impl App {
/// Register a system with any number of [`SystemLabel`]s.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

SystemLabel should be removed from the docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the docs are not up to date, I want to stabilize some things first :)

@Pascualex
Copy link
Contributor Author

@alice-i-cecile I have another question. #4090 stablishes as future work:

Use this API to store all systems, regardless of whether or not they belong to a schedule, and solve #2777.

Does this still make sense? How would this work with multiple instances of a system inside a world? Should they share change detection or each have their own?

@alice-i-cecile
Copy link
Member

I don't think that proposal makes sense any more :) Like you said, it doesn't play nice with multiple copies of systems, and we already have schedules for that.

@Pascualex
Copy link
Contributor Author

Pascualex commented Mar 10, 2023

Yes, sadly I started to feel the same way after thinking about some of the details involved.

Even if we renounce to this idea of a global system registry where all systems are stored, we may still want a solution for running systems on demand instead of polling. Kind of like schedules but for single systems.

To me it makes sense to call this single-system schedule-like things "Commands", that's what my proposal in #7707 was about.

I know that you said that these systems usually don't require exclusive access, but the fact that they are not scheduled is what removes the possibility of parallelization (unless we can think of some cool solution). Just like this system registry solution requires using commands or direct access to the world to run the systems, even for non-exclusive systems.

The current commands for entity and component management could be renamed to something like exclusive/direct/world commands. That way we'd have exclusive commands (which wouldn't have the overhead of systems) and system commands (which would have the ergonomics of systems).

Edit: Conceptually there would be two ways of running systems of demand, each for a different use-case and with different capabilities:

  • Schedules: allow to run a group of systems on demand handling collisions, ordering and optional parallelization.
  • Commands: allow to run a single system (or direct function call) on demand, with the possibility of passing some arguments.

@alice-i-cecile
Copy link
Member

We really can't reuse the name Commands: that's already taken for Commands.

I agree that we should be able to run systems without pre-registration. I'm currently conflicted between:

  1. Require a distinct method like run_uncached
  2. Just cache it the first time the system is run.

The first is more explicit, the second is substantially more ergonomic.

@Pascualex
Copy link
Contributor Author

Sorry for not providing more updates. I was diving deep in Bevy internals but I started on a new position shortly after. I'll try to continue contributing in the future, but right now I don't have the time.

@alice-i-cecile alice-i-cecile added the S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! label Apr 14, 2023
@alice-i-cecile alice-i-cecile removed this from the 0.11 milestone Jun 19, 2023
@Trashtalk217 Trashtalk217 mentioned this pull request Jun 26, 2023
github-merge-queue bot pushed a commit that referenced this pull request Sep 19, 2023
I'm adopting this ~~child~~ PR.

# Objective

- Working with exclusive world access is not always easy: in many cases,
a standard system or three is more ergonomic to write, and more
modularly maintainable.
- For small, one-off tasks (commonly handled with scripting), running an
event-reader system incurs a small but flat overhead cost and muddies
the schedule.
- Certain forms of logic (e.g. turn-based games) want very fine-grained
linear and/or branching control over logic.
- SystemState is not automatically cached, and so performance can suffer
and change detection breaks.
- Fixes #2192.
- Partial workaround for #279.

## Solution

- Adds a SystemRegistry resource to the World, which stores initialized
systems keyed by their SystemSet.
- Allows users to call world.run_system(my_system) and
commands.run_system(my_system), without re-initializing or losing state
(essential for change detection).
- Add a Callback type to enable convenient use of dynamic one shot
systems and reduce the mental overhead of working with Box<dyn
SystemSet>.
- Allow users to run systems based on their SystemSet, enabling more
complex user-made abstractions.

## Future work

- Parameterized one-shot systems would improve reusability and bring
them closer to events and commands. The API could be something like
run_system_with_input(my_system, my_input) and use the In SystemParam.
- We should evaluate the unification of commands and one-shot systems
since they are two different ways to run logic on demand over a World.

### Prior attempts

- #2234
- #2417
- #4090
- #7999

This PR continues the work done in
#7999.

---------

Co-authored-by: Alice Cecile <[email protected]>
Co-authored-by: Federico Rinaldi <[email protected]>
Co-authored-by: MinerSebas <[email protected]>
Co-authored-by: Aevyrie <[email protected]>
Co-authored-by: Alejandro Pascual Pozo <[email protected]>
Co-authored-by: Rob Parrett <[email protected]>
Co-authored-by: François <[email protected]>
Co-authored-by: Dmytro Banin <[email protected]>
Co-authored-by: James Liu <[email protected]>
@alice-i-cecile
Copy link
Member

Merged elsewhere!

rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
I'm adopting this ~~child~~ PR.

# Objective

- Working with exclusive world access is not always easy: in many cases,
a standard system or three is more ergonomic to write, and more
modularly maintainable.
- For small, one-off tasks (commonly handled with scripting), running an
event-reader system incurs a small but flat overhead cost and muddies
the schedule.
- Certain forms of logic (e.g. turn-based games) want very fine-grained
linear and/or branching control over logic.
- SystemState is not automatically cached, and so performance can suffer
and change detection breaks.
- Fixes bevyengine#2192.
- Partial workaround for bevyengine#279.

## Solution

- Adds a SystemRegistry resource to the World, which stores initialized
systems keyed by their SystemSet.
- Allows users to call world.run_system(my_system) and
commands.run_system(my_system), without re-initializing or losing state
(essential for change detection).
- Add a Callback type to enable convenient use of dynamic one shot
systems and reduce the mental overhead of working with Box<dyn
SystemSet>.
- Allow users to run systems based on their SystemSet, enabling more
complex user-made abstractions.

## Future work

- Parameterized one-shot systems would improve reusability and bring
them closer to events and commands. The API could be something like
run_system_with_input(my_system, my_input) and use the In SystemParam.
- We should evaluate the unification of commands and one-shot systems
since they are two different ways to run logic on demand over a World.

### Prior attempts

- bevyengine#2234
- bevyengine#2417
- bevyengine#4090
- bevyengine#7999

This PR continues the work done in
bevyengine#7999.

---------

Co-authored-by: Alice Cecile <[email protected]>
Co-authored-by: Federico Rinaldi <[email protected]>
Co-authored-by: MinerSebas <[email protected]>
Co-authored-by: Aevyrie <[email protected]>
Co-authored-by: Alejandro Pascual Pozo <[email protected]>
Co-authored-by: Rob Parrett <[email protected]>
Co-authored-by: François <[email protected]>
Co-authored-by: Dmytro Banin <[email protected]>
Co-authored-by: James Liu <[email protected]>
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-Feature A new feature, making something new possible S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! 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
3 participants