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] - Change definition of ScheduleRunnerPlugin #2606

Closed
wants to merge 1 commit into from
Closed

[Merged by Bors] - Change definition of ScheduleRunnerPlugin #2606

wants to merge 1 commit into from

Conversation

tsoutsman
Copy link
Contributor

Objective

  • Allow ScheduleRunnerPlugin to be instantiated without curly braces. Other plugins in the library already use the semicolon syntax.
  • Currently, you have to do the following:
App::build()
    .add_plugin(bevy::core::CorePlugin)
    .add_plugin(bevy::app::ScheduleRunnerPlugin {})
  • With the proposed change you can do this:
App::build()
    .add_plugin(bevy::core::CorePlugin)
    .add_plugin(bevy::app::ScheduleRunnerPlugin)

Solution

  • Change the ScheduleRunnerPlugin definition to use a semicolon instead of curly braces.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Aug 6, 2021
@NiklasEi NiklasEi added C-Usability A targeted quality-of-life change that makes Bevy easier to use and removed S-Needs-Triage This issue needs to be labelled labels Aug 6, 2021
Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

This change makes sense, but we do also need to update the docs for this plugin.

I've added a starting point for you. If you don't want to take over this documentation change, I can move it into another PR.

Comment on lines 48 to 49
/// Configures an App to run its [Schedule](bevy_ecs::schedule::Schedule) according to a given
/// [RunMode]
Copy link
Member

Choose a reason for hiding this comment

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

We'd be remiss not to update this documentation here, since the plugin no longer owns the RunMode. This should be something like

Suggested change
/// Configures an App to run its [Schedule](bevy_ecs::schedule::Schedule) according to a given
/// [RunMode]
/// Configures an App to run its [`Schedule`](bevy_ecs::schedule::Schedule) according to the [`ScheduleRunnerSettings`] resource.
/// By default, this loops infinitely with no delay (i.e. runs the simulation* as fast as possible)
/// ```rust
/// # use bevy_app::{App, EventWriter, AppExit};
/// let mut counter: u32 = 0;
/// App::new().add_resource(ScheduleRunnerSettings::default())
/// .add_plugin(ScheduleRunnerPlugin).add_system(
/// |exit: EventWriter<AppExit>| if counter >= 10 {exit.send(AppExit)}else{ counter+=1}
);
/// assert_eq!(counter, 10);
/// ```
/// Please note that this plugin does not use windowing**, for that use the winit plugin.
///
/// If no runner is added, then the app will simulate once then exit.

* If someone has a suggestion for a word other than simulation, I'm keen to hear it - I feel like it's not quite right here.
** Better phrasing here would be good - I'm trying to clarify that this will overwrite the winit plugin (if added later).

Copy link
Contributor Author

@tsoutsman tsoutsman Aug 7, 2021

Choose a reason for hiding this comment

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

Sorry, this review has been pending for a few days. I didn't realise that I had to submit the review for the comments to be published.

Here's my attempt at documenting ScheduleRunnerPlugin. I'm new to both Rust and Bevy so there are quite a few issues with my documentation.

Suggested change
/// Configures an App to run its [Schedule](bevy_ecs::schedule::Schedule) according to a given
/// [RunMode]
/// Configures an App to run its [`Schedule`](bevy_ecs::schedule::Schedule) according to the [`ScheduleRunnerSettings`] resource.
/// The default [`ScheduleRunnerSettings`] will lead to the app running infinitely in a loop as fast as possible.
/// ```rust
/// # use bevy_app::{App, AppExit, EventWriter, ScheduleRunnerPlugin, ScheduleRunnerSettings};
/// let mut counter: u32 = 0;
/// App::new()
/// .insert_resource(ScheduleRunnerSettings::default())
/// .add_plugin(ScheduleRunnerPlugin)
/// .add_system(|mut exit: EventWriter<AppExit>| {
/// if counter >= 10 {
/// exit.send(AppExit)
/// } else {
/// counter += 1
/// }
/// });
/// assert_eq!(counter, 10);
/// ```
/// `ScheduleRunnerSettings::default()` is equivalent to `ScheduleRunnerSettings::run_loop(std::time::Duration::ZERO)`.*
///
/// The scheduler can also be configured to run in a loop, with a minimum time between each iteration.**
/// The following code will take ten seconds*** to run, as each system loop will take one second.
/// ```rust
/// # use bevy_app::{App, AppExit, EventWriter, ScheduleRunnerPlugin, ScheduleRunnerSettings};
/// let mut counter: u32 = 0;
/// App::new()
/// .insert_resource(ScheduleRunnerSettings::run_loop(std::time::Duration::from_secs(1)))
/// .add_plugin(ScheduleRunnerPlugin)
/// .add_system(|exit: EventWriter<AppExit>| {
/// if counter >= 10 {
/// exit.send(AppExit)
/// } else {
/// counter += 1
/// }
/// });
/// assert_eq!(counter, 10);
/// ```
///
/// The [`Schedule`](bevy_ecs::schedule::Schedule) can also be configured to run the game loop just once:
/// ```rust
/// # use bevy_app::{App, AppExit, EventWriter, ScheduleRunnerPlugin, ScheduleRunnerSettings};
/// let mut counter: u32 = 0;
/// App::new()
/// .insert_resource(ScheduleRunnerSettings::run_once())
/// .add_plugin(ScheduleRunnerPlugin)
/// .add_system(|| counter += 1);
/// assert_eq!(counter, 1);
/// ```
/// However, this is the same behaviour as an app with no scheduler.
///
/// Please note that this plugin does not add windowing, for that use the [`WinitPlugin`](../bevy_winit/struct.WinitPlugin.html).

* Technically this isn't true as the RunMode wait time is different, but practically it's the same. I think the statement might help explain what default does but it might also just add to the confusion.
** I'm not sure if this is actually correct. Does Bevy run all the systems and then wait the specified time, or run all the systems and then check if the time from the previous loop is greater than the specified time?
*** I think it'll actually take 9 seconds because it would exit without waiting on the tenth iteration but that might make it more confusing and isn't very important.

A (big?) issue is that none of these tests will actually pass as the borrow checker complains. I'm not sure if there's an easy way to fix this - my little knowledge of Rust and Bevy suggests there isn't. Also, there's a lot of code repetition, and the second example doesn't do a good job of showcasing run_loop.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Also more generally should this documentation be placed in ScheduleRunnerPlugin or ScheduleRunnerSettings?

Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

I'm happy to merge this as-is, as we can always take the docs into another PR if the author would prefer that.

@DJMcNab DJMcNab added A-App Bevy apps and plugins S-Needs-Migration-Guide labels Aug 6, 2021
@cart
Copy link
Member

cart commented Aug 10, 2021

Yup I agree that the docs should be updated, but lets just get this in now. We can do a follow up pr for adding the missing docs.

@cart
Copy link
Member

cart commented Aug 10, 2021

bors r+

bors bot pushed a commit that referenced this pull request Aug 10, 2021
# Objective

- Allow `ScheduleRunnerPlugin` to be instantiated without curly braces. Other plugins in the library already use the semicolon syntax.
- Currently, you have to do the following:
```rust
App::build()
    .add_plugin(bevy::core::CorePlugin)
    .add_plugin(bevy::app::ScheduleRunnerPlugin {})
```
- With the proposed change you can do this:
```rust
App::build()
    .add_plugin(bevy::core::CorePlugin)
    .add_plugin(bevy::app::ScheduleRunnerPlugin)
```

## Solution

- Change the `ScheduleRunnerPlugin` definition to use a semicolon instead of curly braces.
@bors bors bot changed the title Change definition of ScheduleRunnerPlugin [Merged by Bors] - Change definition of ScheduleRunnerPlugin Aug 10, 2021
@bors bors bot closed this Aug 10, 2021
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-Usability A targeted quality-of-life change that makes Bevy easier to use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants