Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

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

Closed
wants to merge 41 commits into from

Conversation

joseph-gio
Copy link
Member

@joseph-gio joseph-gio 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

@joseph-gio
Copy link
Member Author

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.

@joseph-gio joseph-gio 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.

@joseph-gio joseph-gio 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.

@joseph-gio
Copy link
Member Author

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

@joseph-gio
Copy link
Member Author

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

@joseph-gio
Copy link
Member Author

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
@joseph-gio joseph-gio 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
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
A-App Bevy apps and plugins C-Feature A new feature, making something new possible C-Usability A targeted 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