From 79021c78c629d97ef1207378a7ecf93bf4f0e6ce Mon Sep 17 00:00:00 2001 From: Nicola Papale Date: Sat, 6 Jan 2024 20:40:13 +0100 Subject: [PATCH] Fix perf degradation on web builds (#11227) # Objective - Since #10702, the way bevy updates the window leads to major slowdowns as seen in - #11122 - #11220 - Slow is bad, furthermore, _very_ slow is _very_ bad. We should fix this issue. ## Solution - Move the app update code into the `Event::WindowEvent { event: WindowEvent::RedrawRequested }` branch of the event loop. - Run `window.request_redraw()` When `runner_state.redraw_requested` - Instead of swapping `ControlFlow` between `Poll` and `Wait`, we always keep it at `Wait`, and use `window.request_redraw()` to schedule an immediate call to the event loop. - `runner_state.redraw_requested` is set to `true` when `UpdateMode::Continuous` and when a `RequestRedraw` event is received. - Extract the redraw code into a separate function, because otherwise I'd go crazy with the indentation level. - Fix #11122. ## Testing I tested the WASM builds as follow: ```sh cargo run -p build-wasm-example -- --api webgl2 bevymark python -m http.server --directory examples/wasm/ 8080 # Open browser at http://localhost:8080 ``` On main, even spawning a couple sprites is super choppy. Even if it says "300 FPS". While on this branch, it is smooth as butter. I also found that it fixes all choppiness on window resize (tested on Linux/X11). This was another issue from #10702 IIRC. So here is what I tested: - On `wasm`: `many_foxes` and `bevymark`, with `argh::from_env()` commented out, otherwise we get a cryptic error. - Both with `PresentMode::AutoVsync` and `PresentMode::AutoNoVsync` - On main, it is consistently choppy. - With this PR, the visible frame rate is consistent with the diagnostic numbers - On native (linux/x11) I ran similar tests, making sure that `AutoVsync` limits to monitor framerate, and `AutoNoVsync` doesn't. ## Future work Code could be improved, I wanted a quick solution easy to review, but we really need to make the code more accessible. - #9768 - ~~**`WinitSettings::desktop_app()` is completely borked.**~~ actually broken on main as well ### Review guide Consider enable the non-whitespace diff to see the _real_ change set. --- crates/bevy_render/src/renderer/mod.rs | 4 + crates/bevy_winit/src/lib.rs | 248 ++++++++++++++----------- crates/bevy_winit/src/winit_config.rs | 4 +- 3 files changed, 150 insertions(+), 106 deletions(-) diff --git a/crates/bevy_render/src/renderer/mod.rs b/crates/bevy_render/src/renderer/mod.rs index 7a606e3027e00..9e09dfba9ce52 100644 --- a/crates/bevy_render/src/renderer/mod.rs +++ b/crates/bevy_render/src/renderer/mod.rs @@ -71,6 +71,10 @@ pub fn render_system(world: &mut World) { for window in windows.values_mut() { if let Some(wrapped_texture) = window.swap_chain_texture.take() { if let Some(surface_texture) = wrapped_texture.try_unwrap() { + // TODO(clean): winit docs recommends calling pre_present_notify before this. + // though `present()` doesn't present the frame, it schedules it to be presented + // by wgpu. + // https://docs.rs/winit/0.29.9/wasm32-unknown-unknown/winit/window/struct.Window.html#method.pre_present_notify surface_texture.present(); } } diff --git a/crates/bevy_winit/src/lib.rs b/crates/bevy_winit/src/lib.rs index 5e347e656d2d2..4f3a34ab71841 100644 --- a/crates/bevy_winit/src/lib.rs +++ b/crates/bevy_winit/src/lib.rs @@ -360,6 +360,7 @@ pub fn winit_runner(mut app: App) { } } } + runner_state.redraw_requested = false; match event { Event::NewEvents(start_cause) => match start_cause { @@ -412,7 +413,7 @@ pub fn winit_runner(mut app: App) { return; }; - let Ok((mut window, mut cache)) = windows.get_mut(window_entity) else { + let Ok((mut window, _)) = windows.get_mut(window_entity) else { warn!( "Window {:?} is missing `Window` component, skipping event {:?}", window_entity, event @@ -655,17 +656,33 @@ pub fn winit_runner(mut app: App) { window: window_entity, }); } + WindowEvent::RedrawRequested => { + runner_state.redraw_requested = false; + run_app_update_if_should( + &mut runner_state, + &mut app, + &mut focused_windows_state, + event_loop, + &mut create_window_system_state, + &mut app_exit_event_reader, + &mut redraw_event_reader, + ); + } _ => {} } - if window.is_changed() { - cache.window = window.clone(); + let mut windows = app.world.query::<(&mut Window, &mut CachedWindow)>(); + if let Ok((window, mut cache)) = windows.get_mut(&mut app.world, window_entity) { + if window.is_changed() { + cache.window = window.clone(); + } } } Event::DeviceEvent { event: DeviceEvent::MouseMotion { delta: (x, y) }, .. } => { + runner_state.redraw_requested = true; let (mut event_writers, ..) = event_writer_system_state.get_mut(&mut app.world); event_writers.mouse_motion.send(MouseMotion { delta: Vec2::new(x as f32, y as f32), @@ -726,119 +743,142 @@ pub fn winit_runner(mut app: App) { app.world.entity_mut(entity).insert(wrapper); } - event_loop.set_control_flow(ControlFlow::Poll); + event_loop.set_control_flow(ControlFlow::Wait); } } - Event::AboutToWait => { - if runner_state.active.should_run() { - if runner_state.active == ActiveState::WillSuspend { - runner_state.active = ActiveState::Suspended; - #[cfg(target_os = "android")] - { - // Remove the `RawHandleWrapper` from the primary window. - // This will trigger the surface destruction. - let mut query = - app.world.query_filtered::>(); - let entity = query.single(&app.world); - app.world.entity_mut(entity).remove::(); - event_loop.set_control_flow(ControlFlow::Wait); - } - } - let (config, windows) = focused_windows_state.get(&app.world); - let focused = windows.iter().any(|window| window.focused); - let should_update = match config.update_mode(focused) { - UpdateMode::Continuous | UpdateMode::Reactive { .. } => { - // `Reactive`: In order for `event_handler` to have been called, either - // we received a window or raw input event, the `wait` elapsed, or a - // redraw was requested (by the app or the OS). There are no other - // conditions, so we can just return `true` here. - true - } - UpdateMode::ReactiveLowPower { .. } => { - runner_state.wait_elapsed - || runner_state.redraw_requested - || runner_state.window_event_received - } - }; - - if app.plugins_state() == PluginsState::Cleaned && should_update { - // reset these on each update - runner_state.wait_elapsed = false; - runner_state.window_event_received = false; - runner_state.redraw_requested = false; - runner_state.last_update = Instant::now(); - - app.update(); + _ => (), + } + if runner_state.redraw_requested { + let (_, winit_windows, _, _) = event_writer_system_state.get_mut(&mut app.world); + for window in winit_windows.windows.values() { + window.request_redraw(); + } + } + }; - // decide when to run the next update - let (config, windows) = focused_windows_state.get(&app.world); - let focused = windows.iter().any(|window| window.focused); - match config.update_mode(focused) { - UpdateMode::Continuous => { - event_loop.set_control_flow(ControlFlow::Poll); - } - UpdateMode::Reactive { wait } - | UpdateMode::ReactiveLowPower { wait } => { - if let Some(next) = runner_state.last_update.checked_add(*wait) { - runner_state.scheduled_update = Some(next); - event_loop.set_control_flow(ControlFlow::WaitUntil(next)); - } else { - runner_state.scheduled_update = None; - event_loop.set_control_flow(ControlFlow::Wait); - } - } - } + trace!("starting winit event loop"); + // TODO(clean): the winit docs mention using `spawn` instead of `run` on WASM. + if let Err(err) = event_loop.run(event_handler) { + error!("winit event loop returned an error: {err}"); + } +} - if let Some(app_redraw_events) = - app.world.get_resource::>() - { - if redraw_event_reader.read(app_redraw_events).last().is_some() { - runner_state.redraw_requested = true; - event_loop.set_control_flow(ControlFlow::Poll); - } - } +fn run_app_update_if_should( + runner_state: &mut WinitAppRunnerState, + app: &mut App, + focused_windows_state: &mut SystemState<(Res, Query<&Window>)>, + event_loop: &EventLoopWindowTarget<()>, + create_window_system_state: &mut SystemState<( + Commands, + Query<(Entity, &mut Window), Added>, + EventWriter, + NonSendMut, + NonSendMut, + ResMut, + ResMut, + )>, + app_exit_event_reader: &mut ManualEventReader, + redraw_event_reader: &mut ManualEventReader, +) { + if !runner_state.active.should_run() { + return; + } + if runner_state.active == ActiveState::WillSuspend { + runner_state.active = ActiveState::Suspended; + #[cfg(target_os = "android")] + { + // Remove the `RawHandleWrapper` from the primary window. + // This will trigger the surface destruction. + let mut query = app.world.query_filtered::>(); + let entity = query.single(&app.world); + app.world.entity_mut(entity).remove::(); + event_loop.set_control_flow(ControlFlow::Wait); + } + } + let (config, windows) = focused_windows_state.get(&app.world); + let focused = windows.iter().any(|window| window.focused); + let should_update = match config.update_mode(focused) { + // `Reactive`: In order for `event_handler` to have been called, either + // we received a window or raw input event, the `wait` elapsed, or a + // redraw was requested (by the app or the OS). There are no other + // conditions, so we can just return `true` here. + UpdateMode::Continuous | UpdateMode::Reactive { .. } => true, + // TODO(bug): This is currently always true since we only run this function + // if we received a `RequestRedraw` event. + UpdateMode::ReactiveLowPower { .. } => { + runner_state.wait_elapsed + || runner_state.redraw_requested + || runner_state.window_event_received + } + }; - if let Some(app_exit_events) = app.world.get_resource::>() { - if app_exit_event_reader.read(app_exit_events).last().is_some() { - event_loop.exit(); - } - } - } + if app.plugins_state() == PluginsState::Cleaned && should_update { + // reset these on each update + runner_state.wait_elapsed = false; + runner_state.last_update = Instant::now(); - // create any new windows - // (even if app did not update, some may have been created by plugin setup) - let ( - commands, - mut windows, - event_writer, - winit_windows, - adapters, - handlers, - accessibility_requested, - ) = create_window_system_state.get_mut(&mut app.world); - - create_windows( - event_loop, - commands, - windows.iter_mut(), - event_writer, - winit_windows, - adapters, - handlers, - accessibility_requested, - ); + app.update(); - create_window_system_state.apply(&mut app.world); + // decide when to run the next update + let (config, windows) = focused_windows_state.get(&app.world); + let focused = windows.iter().any(|window| window.focused); + match config.update_mode(focused) { + UpdateMode::Continuous => { + runner_state.redraw_requested = true; + } + UpdateMode::Reactive { wait } | UpdateMode::ReactiveLowPower { wait } => { + // TODO(bug): this is unexpected behavior. + // When Reactive, user expects bevy to actually wait that amount of time, + // and not potentially infinitely depending on plateform specifics (which this does) + // Need to verify the plateform specifics (whether this can occur in + // rare-but-possible cases) and replace this with a panic or a log warn! + if let Some(next) = runner_state.last_update.checked_add(*wait) { + runner_state.scheduled_update = Some(next); + event_loop.set_control_flow(ControlFlow::WaitUntil(next)); + } else { + runner_state.scheduled_update = None; + event_loop.set_control_flow(ControlFlow::Wait); } } - _ => (), } - }; - trace!("starting winit event loop"); - if let Err(err) = event_loop.run(event_handler) { - error!("winit event loop returned an error: {err}"); + if let Some(app_redraw_events) = app.world.get_resource::>() { + if redraw_event_reader.read(app_redraw_events).last().is_some() { + runner_state.redraw_requested = true; + } + } + + if let Some(app_exit_events) = app.world.get_resource::>() { + if app_exit_event_reader.read(app_exit_events).last().is_some() { + event_loop.exit(); + } + } } + + // create any new windows + // (even if app did not update, some may have been created by plugin setup) + let ( + commands, + mut windows, + event_writer, + winit_windows, + adapters, + handlers, + accessibility_requested, + ) = create_window_system_state.get_mut(&mut app.world); + + create_windows( + event_loop, + commands, + windows.iter_mut(), + event_writer, + winit_windows, + adapters, + handlers, + accessibility_requested, + ); + + create_window_system_state.apply(&mut app.world); } fn react_to_resize( diff --git a/crates/bevy_winit/src/winit_config.rs b/crates/bevy_winit/src/winit_config.rs index b91e25d340afa..ecece207d6b3d 100644 --- a/crates/bevy_winit/src/winit_config.rs +++ b/crates/bevy_winit/src/winit_config.rs @@ -76,7 +76,7 @@ pub enum UpdateMode { /// - new [window](`winit::event::WindowEvent`) or [raw input](`winit::event::DeviceEvent`) /// events have appeared Reactive { - /// The minimum time from the start of one update to the next. + /// The approximate time from the start of one update to the next. /// /// **Note:** This has no upper limit. /// The [`App`](bevy_app::App) will wait indefinitely if you set this to [`Duration::MAX`]. @@ -93,7 +93,7 @@ pub enum UpdateMode { /// Use this mode if, for example, you only want your app to update when the mouse cursor is /// moving over a window, not just moving in general. This can greatly reduce power consumption. ReactiveLowPower { - /// The minimum time from the start of one update to the next. + /// The approximate time from the start of one update to the next. /// /// **Note:** This has no upper limit. /// The [`App`](bevy_app::App) will wait indefinitely if you set this to [`Duration::MAX`].