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 via Commands for scripting-like logic #2192

Closed
alice-i-cecile opened this issue May 17, 2021 · 17 comments · Fixed by #8963
Closed

One-shot systems via Commands for scripting-like logic #2192

alice-i-cecile opened this issue May 17, 2021 · 17 comments · Fixed by #8963
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

@alice-i-cecile
Copy link
Member

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

Some operations are only done very rarely: scripting-like game logic that modifies levels, unlocks achievements or handles a boss's special attack come to mind.

These systems clutter the schedule, waste resources as they constantly spin, and are harder to extend.

What solution would you like?

There are two parts to my proposed solution:

  1. Add Commands::run_system(system: impl Into<SystemDescriptor>). This runs the specified system on the World the next time commands are processed.
  2. Create a pattern where we store references to system functions on components, and dynamically run those systems when the appropriate trigger is met, allowing for scripting-like flexibility with low-overhead and seamless ergonomic Bevy syntax.

What alternative(s) have you considered?

Writing custom commands types each time will be onerous and have much increased boilerplate, and because they take exclusive access to World, force the users to learn a new syntax and reduce our ability to parallelize commands of this sort in the future.

This same functionality could also be framed as "dynamically inserting systems", but that may complicate the API somewhat when you often don't end up caring a lot about overhead.

Other approaches to reactivity may be able to fill this niche in some form instead.

Additional context

Making sure the ordering on this is correct enough is going to be one of the larger challenges. May be better to wait until after #1375. Chaining also needs some careful thought.

Brought up in response to the following user request:

Is it possible / practical / good practice to pass a function around inside an event? My thought process is an NPC could have an interaction with a very specific procedure following it like opening a secret door across the map or unlocking a new dialogue option with another NPC so much so that having a system that contains every one of these niche interactions inside a match would be rather hideous.

Depending on the efficiency and ergonomics of this, could also be used to support push data-flow for UI (see #254).

Related to #1273.

@alice-i-cecile alice-i-cecile added C-Enhancement A new feature A-ECS Entities, components, systems, and events S-Needs-RFC This issue or PR is particularly complex, and needs an approved RFC before it can be merged labels May 17, 2021
@Ratysz
Copy link
Contributor

Ratysz commented May 17, 2021

Create a pattern where we store references to system functions on components [...]

I'm not sure what this entails on Bevy side. A mention in the book(s) and an example?

This same functionality could also be framed as "dynamically inserting systems", but that may complicate the API somewhat when you often don't end up caring a lot about overhead.

I'm not sure what you mean by that. Inserting systems dynamically is not an API problem: it can't be done without forcing the systems executor to recalculate its cached data, which is necessarily slow.

Making sure the ordering on this is correct enough is going to be one of the larger challenges. May be better to wait until after #1375.

Commands' order is entirely dependant on the order of the systems that issue them. This reads like proposing we add another layer of ordering on top of that, which is bound to get ugly. Stageless won't change anything here.

Chaining also needs some careful thought.

Why? Chaining two systems is essentially making them into one system, no special considerations needed.


I don't think there are any obstacles for a built-in arbitrary closure command (if we don't have one already). Dressing said closure up as a system shouldn't be a challenge, at first glance, but: systems are stateful - they have their own local variables. How should that be handled here?

This might be once again us running into the trap of trying to jam everything and anything into our system abstraction.

I think it's best to instead consider the accessor API for this. It's semantically more correct: a thing that's not a normally scheduled system but needs potentially-shared access to world data.

@cart
Copy link
Member

cart commented May 18, 2021

I think pre-registering a system with the executor, retrieving a handle from that, then triggering a run of the system via commands (or some other abstraction) is probably the way to go if we want to support this.

It definitely has synergies with "reactivity", which entails running a pre-defined system on a trigger. The only difference is "manual trigger" vs "data access trigger".

The executor is already "greedy" (it just loops over systems that need to run and sees if they can run), so this feels like a natural extension of that. Reactions and manual system triggers should both probably run before the source system's dependencies run, so I think its probably just a matter of only flipping the "finished" state for the source system once all triggered systems have finished.

@alice-i-cecile
Copy link
Member Author

I think pre-registering a system with the executor, retrieving a handle from that, then triggering a run of the system via commands (or some other abstraction) is probably the way to go if we want to support this.

I'm on board with this strategy: should be quite clean and support the scripting-like use cases very nicely. As long as we can store the handles on components we should be good.

@alice-i-cecile
Copy link
Member Author

I'm not sure what this entails on Bevy side. A mention in the book(s) and an example?

Yes, likely as part of an example game of some sort plus a page in the book. This sort of design pattern is really important to communicate IMO, but very challenging to understand without context.

@alice-i-cecile
Copy link
Member Author

One other useful bit of functionality that would be helpful: the ability to pass in new data to the system via the Input type on system chaining. This should probably be split from the default no-input form for ergonomic reasons.

@Ratysz
Copy link
Contributor

Ratysz commented May 19, 2021

That's not what chaining is for. We don't have a centralized way to pipe data directly between systems (like in e.g. run criteria), and I don't think we need one - events and other kinds of channels should work just fine.

@mockersf
Copy link
Member

mockersf commented May 19, 2021

there's also the config method on FunctionSystem that can be used to setup locals

@alice-i-cecile
Copy link
Member Author

For posterity, my current thinking is that this pattern is less than optimal. It creates a lot of spaghetti in terms of dependencies and ordering, and has a serious but largely hidden performance impact. It's also very painful to refactor away from.

Instead, I think there are two, better alternatives:

@cart
Copy link
Member

cart commented Dec 20, 2021

To add to this: I think we should have a way to queue runs of systems (in the same stage as the queuing system or some future stage) via their labels. This is slightly different than disabling and enabling at runtime, because queuing twice should result in two runs of the system.

@hlb8122
Copy link
Contributor

hlb8122 commented Jan 23, 2022

Here's an approach to a one-shot style scheme.

https://gist.github.com/hlb8122/33ef96079aa3ba9d5cbd3611f3e4e27c

@alice-i-cecile
Copy link
Member Author

Reopening this: we need something along these lines still, and this is a nice discussion of what will and will not work.

@alice-i-cecile
Copy link
Member Author

I was discussing a workaround with a user today.

  1. Store a HashMap<MyLabel, BoxedSystem> in a resource.
  2. Add systems to this resource, initializing them when they're added.
  3. Use World::resource_scope to run these in an exclusive system.

This is (somewhat) ergonomic, and avoids the reinitialization pain. #2777 would make this pattern even cleaner.

@alice-i-cecile alice-i-cecile added 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-RFC This issue or PR is particularly complex, and needs an approved RFC before it can be merged labels Feb 16, 2023
@alice-i-cecile alice-i-cecile added this to the 0.11 milestone Feb 16, 2023
@pascualex
Copy link
Contributor

@alice-i-cecile, I've been thinking about infrequent events and ECS for a while now and every time I dig deeper I find you've already been there (both here and on Reddit 😅). Given that you've changed your stance on the topic over the months I'd like to know what is your current thinking. Do you still find this a non-optimal pattern? Do you have thoughts on the direction to follow after closing #4090?

I believe this pattern fills some legitimate needs not well supported by classic ECS, but I find it hard to implement without creating confusion around when it should be used or avoided. Even after making a proposal to have ergonomic system params in commands, I still fear an easy-to-use API that completely defeats the parallelism of ECS.

With an ergonomic command API I'm tempted to use them for a bunch of things. Some could be handled with events, but the constant polling for infrequent actions and the explosion of EventWriters that a system might need for very divergent logic just don't feel right. Others could be handled with simple function calls, but in complex systems that forces me to add a lot of boilerplate to fetch the params on the outer system and to pass them to the inner functions.

But I know that by relying too heavily on them I'm bypassing schedules and all the good parallelization stuff. I've been thinking of ways of mitigating this command drawback but it clearly is a very complex topic and so far I've only reached dead ends.

@alice-i-cecile
Copy link
Member Author

I think that this is an important pattern: polling isn't a good fit for everything.

I think that redoing #4090 on the new scheduler should be straightforward and valuable. I also think we can make them very fast. I just closed out that PR as the scheduling internals had / were about to change too dramatically. If you'd like, I'd be happy to mentor you on redoing the PR.

@pascualex
Copy link
Contributor

That sounds great, I'll start tomorrow with the redo. I'll start with the same approach as #4090, but I'd like to discuss #7707 (mainly my last comment) before merging. Using commands instead of labels opens the possibility of passing arguments to the one-shot systems and they require almost none additional boilerplate.

@alice-i-cecile
Copy link
Member Author

I'd be okay to have them take basically an impl System object, and then cache if and only if we can :) I don't think that should use commands though: there's no need for &mut World in most cases.

@pascualex
Copy link
Contributor

I'll first redo #4090 to fully understand your proposal and then reevaluate my reasoning for #7707.

@alice-i-cecile alice-i-cecile modified the milestones: 0.11, 0.12 Jun 19, 2023
github-merge-queue bot pushed a commit that referenced this issue 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 <alice.i.cecile@gmail.com>
Co-authored-by: Federico Rinaldi <gisquerin@gmail.com>
Co-authored-by: MinerSebas <66798382+MinerSebas@users.noreply.github.com>
Co-authored-by: Aevyrie <aevyrie@gmail.com>
Co-authored-by: Alejandro Pascual Pozo <alejandro.pascual.pozo@gmail.com>
Co-authored-by: Rob Parrett <robparrett@gmail.com>
Co-authored-by: François <mockersf@gmail.com>
Co-authored-by: Dmytro Banin <banind@cs.washington.edu>
Co-authored-by: James Liu <contact@jamessliu.com>
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this issue 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 <alice.i.cecile@gmail.com>
Co-authored-by: Federico Rinaldi <gisquerin@gmail.com>
Co-authored-by: MinerSebas <66798382+MinerSebas@users.noreply.github.com>
Co-authored-by: Aevyrie <aevyrie@gmail.com>
Co-authored-by: Alejandro Pascual Pozo <alejandro.pascual.pozo@gmail.com>
Co-authored-by: Rob Parrett <robparrett@gmail.com>
Co-authored-by: François <mockersf@gmail.com>
Co-authored-by: Dmytro Banin <banind@cs.washington.edu>
Co-authored-by: James Liu <contact@jamessliu.com>
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
6 participants