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

[Merged by Bors] - Support system.in_schedule() and system.on_startup() #7790

Closed
wants to merge 41 commits into from

Conversation

JoJoJet
Copy link
Member

@JoJoJet JoJoJet commented Feb 23, 2023

Objective

Support the following syntax for adding systems:

App::new()
    .add_system(setup.on_startup())
    .add_systems((
        show_menu.in_schedule(OnEnter(GameState::Paused)),
        menu_ssytem.in_set(OnUpdate(GameState::Paused)),
        hide_menu.in_schedule(OnExit(GameState::Paused)),
    ))

Solution

Add the traits IntoSystemAppConfig{s}, which provide the extension methods necessary for configuring which schedule a system belongs to. These extension methods return IntoSystemAppConfig{s}, which App::add_system{s} uses to choose which schedule to add systems to.


Changelog

  • Added the extension methods in_schedule(label) and on_startup() for configuring the schedule a system belongs to.

Future Work

  • Replace all uses of add_startup_system in the engine.
  • Deprecate this method

@JoJoJet
Copy link
Member Author

JoJoJet commented Feb 23, 2023

Alright, I've removed the state extension methods for now. We'll let the design for them cook a little more.

I also replaced every usage of add_system{s}_to_schedule with .in_schedule (thank the gods for regex).

I think we should also deprecate add_startup_system{s}, but I imagine that will be more controversial so I'll leave it for a follow-up.

@JoJoJet JoJoJet changed the title Support system.in_schedule() and system.on_enter(state) Support system.in_schedule() and system.on_startup() Feb 23, 2023
@alice-i-cecile alice-i-cecile removed the X-Controversial There is active debate or serious implications around merging this PR label Feb 23, 2023
@alice-i-cecile
Copy link
Member

I think we should also deprecate add_startup_system{s}, but I imagine that will be more controversial so I'll leave it for a follow-up.

Agreed on both counts. With the state API cut, I've removed the controversial label and feel good about getting this in.

@JoJoJet JoJoJet added A-App Bevy apps and plugins and removed A-ECS Entities, components, systems, and events labels Feb 23, 2023
@github-actions
Copy link
Contributor

Example breakout failed to run, please try running it locally and check the result.

@JoJoJet
Copy link
Member Author

JoJoJet commented Feb 23, 2023

It seems to work fine on my machine. What does it mean when CI/run-examples passes but the bot says it fails?

@JoJoJet
Copy link
Member Author

JoJoJet commented Feb 23, 2023

I think the bot is just slow. breakout failed earlier, but it got resolved by the time the bot commented.

@JoJoJet
Copy link
Member Author

JoJoJet commented Feb 24, 2023

I realized that we probably don't want to expose SystemConfigs as IntoIterator, so I refactored the PR to not require that. I accidentally made the code a bit clearer as a result, too.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Feb 24, 2023
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

bors r+

bors bot pushed a commit that referenced this pull request Feb 24, 2023
# Objective

Support the following syntax for adding systems:

```rust
App::new()
    .add_system(setup.on_startup())
    .add_systems((
        show_menu.in_schedule(OnEnter(GameState::Paused)),
        menu_ssytem.in_set(OnUpdate(GameState::Paused)),
        hide_menu.in_schedule(OnExit(GameState::Paused)),
    ))
```

## Solution

Add the traits `IntoSystemAppConfig{s}`, which provide the extension methods necessary for configuring which schedule a system belongs to. These extension methods return `IntoSystemAppConfig{s}`, which `App::add_system{s}` uses to choose which schedule to add systems to.

---

## Changelog

+ Added the extension methods `in_schedule(label)` and  `on_startup()` for configuring the schedule a system belongs to.

## Future Work

* Replace all uses of `add_startup_system` in the engine.
* Deprecate this method
@bors bors bot changed the title Support system.in_schedule() and system.on_startup() [Merged by Bors] - Support system.in_schedule() and system.on_startup() Feb 24, 2023
@bors bors bot closed this Feb 24, 2023
@JoJoJet JoJoJet deleted the in_schedule branch February 24, 2023 19:22
MonaMayrhofer added a commit to MonaMayrhofer/iyes_loopless that referenced this pull request Mar 9, 2023
MonaMayrhofer added a commit to MonaMayrhofer/iyes_loopless that referenced this pull request Mar 9, 2023
Shfty pushed a commit to shfty-rust/bevy that referenced this pull request Mar 19, 2023
…7790)

# Objective

Support the following syntax for adding systems:

```rust
App::new()
    .add_system(setup.on_startup())
    .add_systems((
        show_menu.in_schedule(OnEnter(GameState::Paused)),
        menu_ssytem.in_set(OnUpdate(GameState::Paused)),
        hide_menu.in_schedule(OnExit(GameState::Paused)),
    ))
```

## Solution

Add the traits `IntoSystemAppConfig{s}`, which provide the extension methods necessary for configuring which schedule a system belongs to. These extension methods return `IntoSystemAppConfig{s}`, which `App::add_system{s}` uses to choose which schedule to add systems to.

---

## Changelog

+ Added the extension methods `in_schedule(label)` and  `on_startup()` for configuring the schedule a system belongs to.

## Future Work

* Replace all uses of `add_startup_system` in the engine.
* Deprecate this method
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-App Bevy apps and plugins C-Enhancement A new feature C-Usability A simple quality-of-life change that makes Bevy easier to use S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants