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

Main/Render schedules be run at different real timing. #8976

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
4 changes: 2 additions & 2 deletions benches/benches/bevy_ecs/scheduling/schedule.rs
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ pub fn build_schedule(criterion: &mut Criterion) {
for _ in 0..graph_size {
app.add_systems(Update, empty_system);
}
app.update();
app.world.run_schedule(Update);
});
});

Expand All @@ -111,7 +111,7 @@ pub fn build_schedule(criterion: &mut Criterion) {
// This is necessary since dependency resolution does not occur until the game runs.
// FIXME: Running the game clutters up the benchmarks, so ideally we'd be
// able to benchmark the dependency resolution directly.
app.update();
app.world.run_schedule(Update);
});
});
}
Expand Down
70 changes: 33 additions & 37 deletions crates/bevy_app/src/app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -64,13 +64,12 @@ pub struct App {
/// the application's event loop and advancing the [`Schedule`].
/// Typically, it is not configured manually, but set by one of Bevy's built-in plugins.
/// See `bevy::winit::WinitPlugin` and [`ScheduleRunnerPlugin`](crate::schedule_runner::ScheduleRunnerPlugin).
pub runner: Box<dyn Fn(App) + Send>, // Send bound is required to make App Send
/// The schedule that systems are added to by default.
///
/// The schedule that runs the main loop of schedule execution.
/// # Note
///
/// This is initially set to [`Main`].
pub main_schedule_label: BoxedScheduleLabel,
/// Inside the runner function, `World::clear_trackers()` must be called periodically.
/// If that isn't called on a world, it may lead to memory leaks in `RemovedComponents`.
pub runner: Box<dyn FnOnce(App) + Send + Sync>,
sub_apps: HashMap<AppLabelId, SubApp>,
plugin_registry: Vec<Box<dyn Plugin>>,
plugin_name_added: HashSet<String>,
Expand Down Expand Up @@ -135,28 +134,34 @@ pub struct SubApp {
/// The [`SubApp`]'s instance of [`App`]
pub app: App,

/// The schedule to run by default.
B-head marked this conversation as resolved.
Show resolved Hide resolved
///
/// This is initially set to [`Main`].
pub main_schedule_label: BoxedScheduleLabel,

/// A function that allows access to both the main [`App`] [`World`] and the [`SubApp`]. This is
/// useful for moving data between the sub app and the main app.
extract: Box<dyn Fn(&mut World, &mut App) + Send>,
extract: Box<dyn Fn(&mut World, &mut App) + Send + Sync>,
}

impl SubApp {
/// Creates a new [`SubApp`].
///
/// The provided function `extract` is normally called by the [`update`](App::update) method.
/// The provided function `extract` is normally called by the [`run_schedule`](World::run_schedule) method.
/// After extract is called, the [`Schedule`] of the sub app is run. The [`World`]
/// parameter represents the main app world, while the [`App`] parameter is just a mutable
/// reference to the `SubApp` itself.
pub fn new(app: App, extract: impl Fn(&mut World, &mut App) + Send + 'static) -> Self {
pub fn new(app: App, extract: impl Fn(&mut World, &mut App) + Send + Sync + 'static) -> Self {
Self {
app,
main_schedule_label: Box::new(Main),
extract: Box::new(extract),
}
}

/// Runs the [`SubApp`]'s default schedule.
pub fn run(&mut self) {
self.app.world.run_schedule(&*self.app.main_schedule_label);
self.app.world.run_schedule(&*self.main_schedule_label);
self.app.world.clear_trackers();
}

Expand Down Expand Up @@ -219,38 +224,19 @@ impl App {
sub_apps: HashMap::default(),
plugin_registry: Vec::default(),
plugin_name_added: Default::default(),
main_schedule_label: Box::new(Main),
building_plugin_depth: 0,
}
}

/// Advances the execution of the [`Schedule`] by one cycle.
///
/// This method also updates sub apps.
/// Update sub apps.
/// See [`insert_sub_app`](Self::insert_sub_app) for more details.
///
/// The schedule run by this method is determined by the [`main_schedule_label`](App) field.
/// By default this is [`Main`].
///
/// # Panics
///
/// The active schedule of the app must be set before this method is called.
pub fn update(&mut self) {
#[cfg(feature = "trace")]
let _bevy_update_span = info_span!("update").entered();
{
#[cfg(feature = "trace")]
let _bevy_main_update_span = info_span!("main app").entered();
self.world.run_schedule(&*self.main_schedule_label);
}
pub fn update_sub_apps(&mut self) {
for (_label, sub_app) in self.sub_apps.iter_mut() {
#[cfg(feature = "trace")]
let _sub_app_span = info_span!("sub app", name = ?_label).entered();
sub_app.extract(&mut self.world);
sub_app.run();
}

self.world.clear_trackers();
}

/// Starts the application by calling the app's [runner function](Self::set_runner).
Expand Down Expand Up @@ -293,7 +279,7 @@ impl App {
}

/// Check that [`Plugin::ready`] of all plugins returns true. This is usually called by the
/// event loop, but can be useful for situations where you want to use [`App::update`]
/// event loop, but can be useful for situations where you want to no use [`App::run`]
pub fn ready(&self) -> bool {
for plugin in &self.plugin_registry {
if !plugin.ready(self) {
Expand All @@ -304,8 +290,8 @@ impl App {
}

/// Run [`Plugin::finish`] for each plugin. This is usually called by the event loop once all
/// plugins are [`App::ready`], but can be useful for situations where you want to use
/// [`App::update`].
/// plugins are [`App::ready`], but can be useful for situations where you want to no use
/// [`App::run`].
pub fn finish(&mut self) {
// temporarily remove the plugin registry to run each plugin's setup function on app.
let plugin_registry = std::mem::take(&mut self.plugin_registry);
Expand All @@ -316,7 +302,7 @@ impl App {
}

/// Run [`Plugin::cleanup`] for each plugin. This is usually called by the event loop after
/// [`App::finish`], but can be useful for situations where you want to use [`App::update`].
/// [`App::finish`], but can be useful for situations where you want to no use [`App::run`].
pub fn cleanup(&mut self) {
// temporarily remove the plugin registry to run each plugin's setup function on app.
let plugin_registry = std::mem::take(&mut self.plugin_registry);
Expand Down Expand Up @@ -636,6 +622,11 @@ impl App {
/// The runner function is usually not set manually, but by Bevy integrated plugins
/// (e.g. `WinitPlugin`).
///
/// # Note
///
/// Inside the runner function, `World::clear_trackers()` must be called periodically.
/// If that isn't called on a world, it may lead to memory leaks in `RemovedComponents`.
///
/// # Examples
///
/// ```
Expand All @@ -644,14 +635,14 @@ impl App {
/// fn my_runner(mut app: App) {
/// loop {
/// println!("In main loop");
/// app.update();
/// app.world.run_schedule(Main);
/// }
/// }
///
/// App::new()
/// .set_runner(my_runner);
/// ```
pub fn set_runner(&mut self, run_fn: impl Fn(App) + 'static + Send) -> &mut Self {
pub fn set_runner(&mut self, run_fn: impl FnOnce(App) + 'static + Send + Sync) -> &mut Self {
self.runner = Box::new(run_fn);
self
}
Expand Down Expand Up @@ -970,7 +961,12 @@ fn run_once(mut app: App) {
app.finish();
app.cleanup();

app.update();
{
#[cfg(feature = "trace")]
let _main_schedule_span = info_span!("main schedule", name = ?Main).entered();
app.world.run_schedule(Main);
}
app.update_sub_apps();
}

/// An event that indicates the [`App`] should exit. This will fully exit the app process at the
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_app/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ pub mod prelude {
pub use crate::{
app::App,
main_schedule::{
First, FixedUpdate, Last, Main, PostStartup, PostUpdate, PreStartup, PreUpdate,
First, FixedUpdate, Last, Main, PostStartup, PostUpdate, PreStartup, PreUpdate, Render,
Startup, StateTransition, Update,
},
DynamicPlugin, Plugin, PluginGroup,
Expand Down
6 changes: 5 additions & 1 deletion crates/bevy_app/src/main_schedule.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ use bevy_ecs::{
world::{Mut, World},
};

/// The schedule that contains the app logic that is evaluated each tick of [`App::update()`].
/// The schedule that contains the app logic that is evaluated each tick of event loop.
///
/// By default, it will run the following schedules in the given order:
///
Expand Down Expand Up @@ -95,6 +95,10 @@ pub struct PostUpdate;
#[derive(ScheduleLabel, Clone, Debug, PartialEq, Eq, Hash)]
pub struct Last;

/// The main render schedule.
#[derive(ScheduleLabel, Debug, Hash, PartialEq, Eq, Clone)]
pub struct Render;

/// Defines the schedules to be run for the [`Main`] schedule, including
/// their order.
#[derive(Resource, Debug)]
Expand Down
46 changes: 42 additions & 4 deletions crates/bevy_app/src/schedule_runner.rs
Original file line number Diff line number Diff line change
@@ -1,10 +1,17 @@
use crate::{
app::{App, AppExit},
plugin::Plugin,
Main,
};
use bevy_ecs::{
event::{Events, ManualEventReader},
schedule::BoxedScheduleLabel,
};
use bevy_ecs::event::{Events, ManualEventReader};
use bevy_utils::{Duration, Instant};

#[cfg(feature = "trace")]
use bevy_utils::tracing::info_span;

#[cfg(target_arch = "wasm32")]
use std::{cell::RefCell, rc::Rc};
#[cfg(target_arch = "wasm32")]
Expand Down Expand Up @@ -43,17 +50,21 @@ impl Default for RunMode {
/// typically, the `winit` event loop
/// (see [`WinitPlugin`](https://docs.rs/bevy/latest/bevy/winit/struct.WinitPlugin.html))
/// executes the schedule making [`ScheduleRunnerPlugin`] unnecessary.
#[derive(Default)]
pub struct ScheduleRunnerPlugin {
/// Determines whether the [`Schedule`](bevy_ecs::schedule::Schedule) is run once or repeatedly.
pub run_mode: RunMode,
/// The schedule to run by default.
///
/// This is initially set to [`Main`].
pub main_schedule_label: BoxedScheduleLabel,
}

impl ScheduleRunnerPlugin {
/// See [`RunMode::Once`].
pub fn run_once() -> Self {
ScheduleRunnerPlugin {
run_mode: RunMode::Once,
main_schedule_label: Box::new(Main),
}
}

Expand All @@ -63,18 +74,38 @@ impl ScheduleRunnerPlugin {
run_mode: RunMode::Loop {
wait: Some(wait_duration),
},
main_schedule_label: Box::new(Main),
}
}
}

impl Default for ScheduleRunnerPlugin {
fn default() -> Self {
ScheduleRunnerPlugin {
run_mode: RunMode::Loop { wait: None },
main_schedule_label: Box::new(Main),
}
}
}

impl Plugin for ScheduleRunnerPlugin {
fn build(&self, app: &mut App) {
let run_mode = self.run_mode;
let main_schedule_label = self.main_schedule_label.clone();
app.set_runner(move |mut app: App| {
// Prevent panic when schedules do not exist
app.init_schedule(main_schedule_label.clone());

let mut app_exit_event_reader = ManualEventReader::<AppExit>::default();
match run_mode {
RunMode::Once => {
app.update();
{
#[cfg(feature = "trace")]
let _main_schedule_span =
info_span!("main schedule", name = ?main_schedule_label).entered();
app.world.run_schedule(main_schedule_label);
}
app.update_sub_apps();
}
RunMode::Loop { wait } => {
let mut tick = move |app: &mut App,
Expand All @@ -91,7 +122,14 @@ impl Plugin for ScheduleRunnerPlugin {
}
}

app.update();
{
#[cfg(feature = "trace")]
let _main_schedule_span =
info_span!("main schedule", name = ?main_schedule_label).entered();
app.world.run_schedule(&main_schedule_label);
}
app.update_sub_apps();
app.world.clear_trackers();

if let Some(app_exit_events) =
app.world.get_resource_mut::<Events<AppExit>>()
Expand Down
10 changes: 5 additions & 5 deletions crates/bevy_asset/src/asset_server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -645,7 +645,7 @@ pub fn free_unused_assets_system(asset_server: Res<AssetServer>) {
mod test {
use super::*;
use crate::{loader::LoadedAsset, update_asset_storage_system};
use bevy_app::{App, Update};
use bevy_app::{App, Main, Update};
use bevy_ecs::prelude::*;
use bevy_reflect::{TypePath, TypeUuid};
use bevy_utils::BoxedFuture;
Expand Down Expand Up @@ -896,19 +896,19 @@ mod test {
// asset is loading
assert_eq!(LoadState::Loading, get_load_state(&handle, &app.world));

app.update();
app.world.run_schedule(Main);
Copy link
Member

Choose a reason for hiding this comment

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

I think this is worse: we're leaking internal details and making a very common operation more verbose.

Can we keep App::update around and just call this on the main schedule label?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did not do any test code refactoring, so it is verbose. 😅

Generally, &mut World is passed in the exclusive system, so it is basically never used in that form. Probably in the form of runner functions and test code only.

And this PR will no longer be used in the engine code. So I think the maintenance will be forgotten and App::update will stop working somewhere.

Copy link
Contributor

Choose a reason for hiding this comment

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

Following up on @alice-i-cecile 's question, can we keep App::update()?

It is used in tests and examples, and replacing it with the four-line stanza makes it more difficult to write tests that need to run schedules. Making testing more difficult should be avoided. I say this after adding easily a dozen app.update() calls in the stepping PR for an example demonstrating stepping.

I think the maintenance will be forgotten and App::update will stop working somewhere.

The same argument applies if we replace the dozens of the existing App::update() calls with four other lines. The difference is that we'll only need to fix App::update() when it breaks.

I think we do need for some function to exist on mut App that is the equivalent of "run everything".

// asset should exist and be loaded at this point
assert_eq!(LoadState::Loaded, get_load_state(&handle, &app.world));
assert!(get_asset(&handle, &app.world).is_some());

// after dropping the handle, next call to `tick` will prepare the assets for removal.
drop(handle);
app.update();
app.world.run_schedule(Main);
assert_eq!(LoadState::Loaded, get_load_state(&weak_handle, &app.world));
assert!(get_asset(&weak_handle, &app.world).is_some());

// second call to tick will actually remove the asset.
app.update();
app.world.run_schedule(Main);
assert_eq!(
LoadState::Unloaded,
get_load_state(&weak_handle, &app.world)
Expand All @@ -918,7 +918,7 @@ mod test {
// finally, reload the asset
let handle = load_asset(path.clone(), &app.world).typed();
assert_eq!(LoadState::Loading, get_load_state(&handle, &app.world));
app.update();
app.world.run_schedule(Main);
assert_eq!(LoadState::Loaded, get_load_state(&handle, &app.world));
assert!(get_asset(&handle, &app.world).is_some());
}
Expand Down
4 changes: 2 additions & 2 deletions crates/bevy_asset/src/debug_asset_server.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
//!
//! Internal assets (e.g. shaders) are bundled directly into an application and can't be hot
//! reloaded using the conventional API.
use bevy_app::{App, Plugin, Update};
use bevy_app::{App, Main, Plugin, Update};
use bevy_ecs::{prelude::*, system::SystemState};
use bevy_tasks::{IoTaskPool, TaskPoolBuilder};
use bevy_utils::{Duration, HashMap};
Expand Down Expand Up @@ -81,7 +81,7 @@ impl Plugin for DebugAssetServerPlugin {
}

fn run_debug_asset_app(mut debug_asset_app: NonSendMut<DebugAssetApp>) {
debug_asset_app.0.update();
debug_asset_app.0.world.run_schedule(Main);
}

pub(crate) fn sync_debug_assets<T: Asset + Clone>(
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_core/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ mod tests {
TypeRegistrationPlugin::default(),
FrameCountPlugin::default(),
));
app.update();
app.world.run_schedule(Main);

let frame_count = app.world.resource::<FrameCount>();
assert_eq!(1, frame_count.0);
Expand Down
Loading