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.

Sign up for GitHub

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

Jump to bottom

Remove OnUpdate system set #8239

Closed
alice-i-cecile opened this issue Mar 28, 2023 · 0 comments · Fixed by #8260
Closed

Remove OnUpdate system set #8239

alice-i-cecile opened this issue Mar 28, 2023 · 0 comments · Fixed by #8260
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use X-Controversial There is active debate or serious implications around merging this PR
Milestone
0.11

Comments

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Mar 28, 2023 •
edited
Loading

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

This system set is redundant and footgunny.

Problems:

  1. Obscures the mental model.
  2. Redundant to run_if(in_state)) run conditions.
  3. Only works in the main schedule: see multiple states and scheduling #8234.

What solution would you like?

Remove it completely.

Beginners can simply use run conditions on each systems.

More advanced users who care about the overhead of duplicate checks of the state can make their own run conditions for this, just like everything else.

What alternative(s) have you considered?

  1. We could panic when users try and apply an unconfigured system set to a system. This breaks plugin crates, who often want to expose public but unconfigured labels to make a nice API.
  2. We could automatically configure this set with the in_state condition somehow in every schedule. This is a hacky special-case.
  3. We could add an extension method on our system types to have nicer syntax than run_if(in_state(AppState::Playing)). This is a lot of complex maintenance burden and obscures the mental model again.

Additional context

The need for this system set was dramatically reduced by #8079, which eliminated base sets and made schedules more explicit.
@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use X-Controversial There is active debate or serious implications around merging this PR labels Mar 28, 2023
@alice-i-cecile alice-i-cecile added this to the 0.11 milestone Mar 28, 2023
@lewiszlw lewiszlw mentioned this issue Mar 30, 2023
Merged
@cart cart closed this as completed in 5c7abb0 Apr 4, 2023
kezuo pushed a commit to kezuo/bevy that referenced this issue Apr 22, 2023
@lewiszlw @alice-i-cecile
Remove OnUpdate system set (#8260)
4441265 
# Objective

- Fixes bevyengine/bevy#8239.

## Solution

- Replace `OnUpdate` with `run_if(in_state(xxx))`.

---

## Migration Guide

- Replace `OnUpdate` with `run_if(in_state(xxx))`.

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
@D0ubleD0uble D0ubleD0uble mentioned this issue Oct 14, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Milestone
0.11
Development

Successfully merging a pull request may close this issue.

Remove OnUpdate system set lewiszlw/bevy
1 participant
@alice-i-cecile