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

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

Closed
wants to merge 1 commit into from
+1 −1
Closed

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion crates/bevy_app/src/schedule_runner.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ impl ScheduleRunnerSettings {
/// Configures an App to run its [Schedule](bevy_ecs::schedule::Schedule) according to a given
/// [RunMode]
Comment on lines 48 to 49
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 •
edited
Loading

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?

#[derive(Default)]
pub struct ScheduleRunnerPlugin {}
pub struct ScheduleRunnerPlugin;

impl Plugin for ScheduleRunnerPlugin {
fn build(&self, app: &mut App) {
Expand Down