forestia/bevy
2
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix Window feedback loop between the OS and Bevy (#7517)

Browse Source 
# Objective

Fix #7377
Fix #7513

## Solution

Record the changes made to the Bevy `Window` from `winit` as 'canon' to avoid Bevy sending those changes back to `winit` again, causing a feedback loop.

## Changelog

* Removed `ModifiesWindows` system label.
  Neither `despawn_window` nor `changed_window` actually modify the `Window` component so all the `.after(ModifiesWindows)` shouldn't be necessary.
* Moved `changed_window` and `despawn_window` systems to `CoreStage::Last` to avoid systems making changes to the `Window` between `changed_window` and the end of the frame as they would be ignored.

## Migration Guide
The `ModifiesWindows` system label was removed.


Co-authored-by: devil-ira <justthecooldude@gmail.com>
This commit is contained in:
ira 2023-02-07 14:18:13 +00:00
parent 6314f50e7b
commit f69f1329e0
7 changed files with 49 additions and 83 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

4
crates/bevy_pbr/src/lib.rs
View File

@ -11,7 +11,6 @@ mod render;
pub use alpha::*; pub use alpha::*;
use bevy_transform::TransformSystem; use bevy_transform::TransformSystem;
use bevy_window::ModifiesWindows;
pub use bundle::*; pub use bundle::*;
pub use fog::*; pub use fog::*;
pub use light::*; pub use light::*;
@ -199,8 +198,7 @@ impl Plugin for PbrPlugin {
.in_set(SimulationLightSystems::AssignLightsToClusters) .in_set(SimulationLightSystems::AssignLightsToClusters)
.after(TransformSystem::TransformPropagate) .after(TransformSystem::TransformPropagate)
.after(VisibilitySystems::CheckVisibility) .after(VisibilitySystems::CheckVisibility)
.after(CameraUpdateSystem) .after(CameraUpdateSystem),
.after(ModifiesWindows),
) )
.add_system( .add_system(
update_directional_light_cascades update_directional_light_cascades

2
crates/bevy_render/src/camera/projection.rs
View File

@ -7,7 +7,6 @@ use bevy_reflect::{
std_traits::ReflectDefault, FromReflect, GetTypeRegistration, Reflect, ReflectDeserialize, std_traits::ReflectDefault, FromReflect, GetTypeRegistration, Reflect, ReflectDeserialize,
ReflectSerialize, ReflectSerialize,
}; };
use bevy_window::ModifiesWindows;
use serde::{Deserialize, Serialize}; use serde::{Deserialize, Serialize};
/// Adds [`Camera`](crate::camera::Camera) driver systems for a given projection type. /// Adds [`Camera`](crate::camera::Camera) driver systems for a given projection type.
@ -43,7 +42,6 @@ impl<T: CameraProjection + Component + GetTypeRegistration> Plugin for CameraPro
.add_system( .add_system(
crate::camera::camera_system::<T> crate::camera::camera_system::<T>
.in_set(CameraUpdateSystem) .in_set(CameraUpdateSystem)
.after(ModifiesWindows)
// We assume that each camera will only have one projection, // We assume that each camera will only have one projection,
// so we can ignore ambiguities with all other monomorphizations. // so we can ignore ambiguities with all other monomorphizations.
// FIXME: Add an archetype invariant for this https://github.com/bevyengine/bevy/issues/1481. // FIXME: Add an archetype invariant for this https://github.com/bevyengine/bevy/issues/1481.

2
crates/bevy_text/src/lib.rs
View File

@ -28,7 +28,6 @@ use bevy_asset::AddAsset;
use bevy_ecs::prelude::*; use bevy_ecs::prelude::*;
use bevy_render::{camera::CameraUpdateSystem, ExtractSchedule, RenderApp}; use bevy_render::{camera::CameraUpdateSystem, ExtractSchedule, RenderApp};
use bevy_sprite::SpriteSystem; use bevy_sprite::SpriteSystem;
use bevy_window::ModifiesWindows;
use std::num::NonZeroUsize; use std::num::NonZeroUsize;
#[derive(Default)] #[derive(Default)]
@ -83,7 +82,6 @@ impl Plugin for TextPlugin {
.add_system( .add_system(
update_text2d_layout update_text2d_layout
.in_base_set(CoreSet::PostUpdate) .in_base_set(CoreSet::PostUpdate)
.after(ModifiesWindows)
// Potential conflict: `Assets<Image>` // Potential conflict: `Assets<Image>`
// In practice, they run independently since `bevy_render::camera_update_system` // In practice, they run independently since `bevy_render::camera_update_system`
// will only ever observe its own render target, and `update_text2d_layout` // will only ever observe its own render target, and `update_text2d_layout`

5
crates/bevy_ui/src/lib.rs
View File

@ -34,7 +34,6 @@ use bevy_app::prelude::*;
use bevy_ecs::prelude::*; use bevy_ecs::prelude::*;
use bevy_input::InputSystem; use bevy_input::InputSystem;
use bevy_transform::TransformSystem; use bevy_transform::TransformSystem;
use bevy_window::ModifiesWindows;
use stack::ui_stack_system; use stack::ui_stack_system;
pub use stack::UiStack; pub use stack::UiStack;
use update::update_clipping_system; use update::update_clipping_system;
@ -110,7 +109,6 @@ impl Plugin for UiPlugin {
widget::text_system widget::text_system
.in_base_set(CoreSet::PostUpdate) .in_base_set(CoreSet::PostUpdate)
.before(UiSystem::Flex) .before(UiSystem::Flex)
.after(ModifiesWindows)
// Potential conflict: `Assets<Image>` // Potential conflict: `Assets<Image>`
// In practice, they run independently since `bevy_render::camera_update_system` // In practice, they run independently since `bevy_render::camera_update_system`
// will only ever observe its own render target, and `widget::text_system` // will only ever observe its own render target, and `widget::text_system`
@ -135,8 +133,7 @@ impl Plugin for UiPlugin {
.add_system( .add_system(
flex_node_system flex_node_system
.in_set(UiSystem::Flex) .in_set(UiSystem::Flex)
.before(TransformSystem::TransformPropagate) .before(TransformSystem::TransformPropagate),
.after(ModifiesWindows),
) )
.add_system(ui_stack_system.in_set(UiSystem::Stack)) .add_system(ui_stack_system.in_set(UiSystem::Stack))
.add_system( .add_system(

3
crates/bevy_window/src/lib.rs
View File

@ -137,9 +137,6 @@ impl Plugin for WindowPlugin {
} }
} }
#[derive(Debug, Hash, PartialEq, Eq, Clone, SystemSet)]
pub struct ModifiesWindows;
/// Defines the specific conditions the application should exit on /// Defines the specific conditions the application should exit on
#[derive(Clone)] #[derive(Clone)]
pub enum ExitCondition { pub enum ExitCondition {

32
crates/bevy_winit/src/lib.rs
View File

@ -6,7 +6,7 @@ mod winit_config;
mod winit_windows; mod winit_windows;
use bevy_ecs::system::{SystemParam, SystemState}; use bevy_ecs::system::{SystemParam, SystemState};
use system::{changed_window, create_window, despawn_window}; use system::{changed_window, create_window, despawn_window, CachedWindow};
pub use winit_config::*; pub use winit_config::*;
pub use winit_windows::*; pub use winit_windows::*;
@ -26,7 +26,7 @@ use bevy_utils::{
}; };
use bevy_window::{ use bevy_window::{
exit_on_all_closed, CursorEntered, CursorLeft, CursorMoved, FileDragAndDrop, Ime, exit_on_all_closed, CursorEntered, CursorLeft, CursorMoved, FileDragAndDrop, Ime,
ModifiesWindows, ReceivedCharacter, RequestRedraw, Window, WindowBackendScaleFactorChanged, ReceivedCharacter, RequestRedraw, Window, WindowBackendScaleFactorChanged,
WindowCloseRequested, WindowCreated, WindowFocused, WindowMoved, WindowResized, WindowCloseRequested, WindowCreated, WindowFocused, WindowMoved, WindowResized,
WindowScaleFactorChanged, WindowScaleFactorChanged,
}; };
@ -39,7 +39,6 @@ use winit::{
event_loop::{ControlFlow, EventLoop, EventLoopBuilder, EventLoopWindowTarget}, event_loop::{ControlFlow, EventLoop, EventLoopBuilder, EventLoopWindowTarget},
}; };
use crate::system::WinitWindowInfo;
#[cfg(target_arch = "wasm32")] #[cfg(target_arch = "wasm32")]
use crate::web_resize::{CanvasParentResizeEventChannel, CanvasParentResizePlugin}; use crate::web_resize::{CanvasParentResizeEventChannel, CanvasParentResizePlugin};
@ -70,7 +69,6 @@ impl Plugin for WinitPlugin {
app.init_non_send_resource::<WinitWindows>() app.init_non_send_resource::<WinitWindows>()
.init_resource::<WinitSettings>() .init_resource::<WinitSettings>()
.set_runner(winit_runner) .set_runner(winit_runner)
.configure_set(ModifiesWindows.in_base_set(CoreSet::PostUpdate))
// exit_on_all_closed only uses the query to determine if the query is empty, // exit_on_all_closed only uses the query to determine if the query is empty,
// and so doesn't care about ordering relative to changed_window // and so doesn't care about ordering relative to changed_window
.add_systems( .add_systems(
@ -79,7 +77,7 @@ impl Plugin for WinitPlugin {
// Update the state of the window before attempting to despawn to ensure consistent event ordering // Update the state of the window before attempting to despawn to ensure consistent event ordering
despawn_window.after(changed_window), despawn_window.after(changed_window),
) )
.in_set(ModifiesWindows), .in_base_set(CoreSet::Last),
); );
#[cfg(target_arch = "wasm32")] #[cfg(target_arch = "wasm32")]
@ -349,7 +347,7 @@ pub fn winit_runner(mut app: App) {
// Fetch and prepare details from the world // Fetch and prepare details from the world
let mut system_state: SystemState<( let mut system_state: SystemState<(
NonSend<WinitWindows>, NonSend<WinitWindows>,
Query<(&mut Window, &mut WinitWindowInfo)>, Query<(&mut Window, &mut CachedWindow)>,
WindowEvents, WindowEvents,
InputEvents, InputEvents,
CursorEvents, CursorEvents,
@ -376,7 +374,7 @@ pub fn winit_runner(mut app: App) {
return; return;
}; };
let (mut window, mut info) = let (mut window, mut cache) =
if let Ok((window, info)) = window_query.get_mut(window_entity) { if let Ok((window, info)) = window_query.get_mut(window_entity) {
(window, info) (window, info)
} else { } else {
@ -394,7 +392,6 @@ pub fn winit_runner(mut app: App) {
window window
.resolution .resolution
.set_physical_resolution(size.width, size.height); .set_physical_resolution(size.width, size.height);
info.last_winit_size = size;
window_events.window_resized.send(WindowResized { window_events.window_resized.send(WindowResized {
window: window_entity, window: window_entity,
@ -421,11 +418,7 @@ pub fn winit_runner(mut app: App) {
window.resolution.physical_height() as f64 - position.y, window.resolution.physical_height() as f64 - position.y,
); );
// bypassing change detection to not trigger feedback loop with system `changed_window` window.set_physical_cursor_position(Some(physical_position));
// this system change the cursor position in winit
window
.bypass_change_detection()
.set_physical_cursor_position(Some(physical_position));
cursor_events.cursor_moved.send(CursorMoved { cursor_events.cursor_moved.send(CursorMoved {
window: window_entity, window: window_entity,
@ -439,14 +432,7 @@ pub fn winit_runner(mut app: App) {
}); });
} }
WindowEvent::CursorLeft { .. } => { WindowEvent::CursorLeft { .. } => {
// Component window.set_physical_cursor_position(None);
if let Ok((mut window, _)) = window_query.get_mut(window_entity) {
// bypassing change detection to not trigger feedback loop with system `changed_window`
// this system change the cursor position in winit
window
.bypass_change_detection()
.set_physical_cursor_position(None);
}
cursor_events.cursor_left.send(CursorLeft { cursor_events.cursor_left.send(CursorLeft {
window: window_entity, window: window_entity,
@ -594,6 +580,10 @@ pub fn winit_runner(mut app: App) {
}, },
_ => {} _ => {}
} }
if window.is_changed() {
cache.window = window.clone();
}
} }
event::Event::DeviceEvent { event::Event::DeviceEvent {
event: DeviceEvent::MouseMotion { delta: (x, y) }, event: DeviceEvent::MouseMotion { delta: (x, y) },

82
crates/bevy_winit/src/system.rs
View File

@ -39,20 +39,19 @@ pub(crate) fn create_window<'a>(
mut winit_windows: NonSendMut<WinitWindows>, mut winit_windows: NonSendMut<WinitWindows>,
#[cfg(target_arch = "wasm32")] event_channel: ResMut<CanvasParentResizeEventChannel>, #[cfg(target_arch = "wasm32")] event_channel: ResMut<CanvasParentResizeEventChannel>,
) { ) {
for (entity, mut component) in created_windows { for (entity, mut window) in created_windows {
if winit_windows.get_window(entity).is_some() { if winit_windows.get_window(entity).is_some() {
continue; continue;
} }
info!( info!(
"Creating new window {:?} ({:?})", "Creating new window {:?} ({:?})",
component.title.as_str(), window.title.as_str(),
entity entity
); );
let winit_window = winit_windows.create_window(event_loop, entity, &component); let winit_window = winit_windows.create_window(event_loop, entity, &window);
let current_size = winit_window.inner_size(); window
component
.resolution .resolution
.set_scale_factor(winit_window.scale_factor()); .set_scale_factor(winit_window.scale_factor());
commands commands
@ -61,18 +60,14 @@ pub(crate) fn create_window<'a>(
window_handle: winit_window.raw_window_handle(), window_handle: winit_window.raw_window_handle(),
display_handle: winit_window.raw_display_handle(), display_handle: winit_window.raw_display_handle(),
}) })
.insert(WinitWindowInfo { .insert(CachedWindow {
previous: component.clone(), window: window.clone(),
last_winit_size: PhysicalSize {
width: current_size.width,
height: current_size.height,
},
}); });
#[cfg(target_arch = "wasm32")] #[cfg(target_arch = "wasm32")]
{ {
if component.fit_canvas_to_parent { if window.fit_canvas_to_parent {
let selector = if let Some(selector) = &component.canvas { let selector = if let Some(selector) = &window.canvas {
selector selector
} else { } else {
WINIT_CANVAS_SELECTOR WINIT_CANVAS_SELECTOR
@ -106,11 +101,10 @@ pub(crate) fn despawn_window(
} }
} }
/// Previous state of the window so we can check sub-portions of what actually was changed. /// The cached state of the window so we can check which properties were changed from within the app.
#[derive(Debug, Clone, Component)] #[derive(Debug, Clone, Component)]
pub struct WinitWindowInfo { pub struct CachedWindow {
pub previous: Window, pub window: Window,
pub last_winit_size: PhysicalSize<u32>,
} }
// Detect changes to the window and update the winit window accordingly. // Detect changes to the window and update the winit window accordingly.
@ -121,18 +115,16 @@ pub struct WinitWindowInfo {
// - [`Window::canvas`] currently cannot be updated after startup, not entirely sure if it would work well with the // - [`Window::canvas`] currently cannot be updated after startup, not entirely sure if it would work well with the
// event channel stuff. // event channel stuff.
pub(crate) fn changed_window( pub(crate) fn changed_window(
mut changed_windows: Query<(Entity, &mut Window, &mut WinitWindowInfo), Changed<Window>>, mut changed_windows: Query<(Entity, &mut Window, &mut CachedWindow), Changed<Window>>,
winit_windows: NonSendMut<WinitWindows>, winit_windows: NonSendMut<WinitWindows>,
) { ) {
for (entity, mut window, mut info) in &mut changed_windows { for (entity, mut window, mut cache) in &mut changed_windows {
let previous = &info.previous;
if let Some(winit_window) = winit_windows.get_window(entity) { if let Some(winit_window) = winit_windows.get_window(entity) {
if window.title != previous.title { if window.title != cache.window.title {
winit_window.set_title(window.title.as_str()); winit_window.set_title(window.title.as_str());
} }
if window.mode != previous.mode { if window.mode != cache.window.mode {
let new_mode = match window.mode { let new_mode = match window.mode {
bevy_window::WindowMode::BorderlessFullscreen => { bevy_window::WindowMode::BorderlessFullscreen => {
Some(winit::window::Fullscreen::Borderless(None)) Some(winit::window::Fullscreen::Borderless(None))
@ -156,19 +148,15 @@ pub(crate) fn changed_window(
winit_window.set_fullscreen(new_mode); winit_window.set_fullscreen(new_mode);
} }
} }
if window.resolution != previous.resolution { if window.resolution != cache.window.resolution {
let physical_size = PhysicalSize::new( let physical_size = PhysicalSize::new(
window.resolution.physical_width(), window.resolution.physical_width(),
window.resolution.physical_height(), window.resolution.physical_height(),
); );
// Prevents "window.resolution values set from a winit resize event" from
// being set here, creating feedback loops.
if physical_size != info.last_winit_size {
winit_window.set_inner_size(physical_size); winit_window.set_inner_size(physical_size);
} }
}
if window.physical_cursor_position() != previous.physical_cursor_position() { if window.physical_cursor_position() != cache.window.physical_cursor_position() {
if let Some(physical_position) = window.physical_cursor_position() { if let Some(physical_position) = window.physical_cursor_position() {
let inner_size = winit_window.inner_size(); let inner_size = winit_window.inner_size();
@ -184,21 +172,21 @@ pub(crate) fn changed_window(
} }
} }
if window.cursor.icon != previous.cursor.icon { if window.cursor.icon != cache.window.cursor.icon {
winit_window.set_cursor_icon(converters::convert_cursor_icon(window.cursor.icon)); winit_window.set_cursor_icon(converters::convert_cursor_icon(window.cursor.icon));
} }
if window.cursor.grab_mode != previous.cursor.grab_mode { if window.cursor.grab_mode != cache.window.cursor.grab_mode {
crate::winit_windows::attempt_grab(winit_window, window.cursor.grab_mode); crate::winit_windows::attempt_grab(winit_window, window.cursor.grab_mode);
} }
if window.cursor.visible != previous.cursor.visible { if window.cursor.visible != cache.window.cursor.visible {
winit_window.set_cursor_visible(window.cursor.visible); winit_window.set_cursor_visible(window.cursor.visible);
} }
if window.cursor.hit_test != previous.cursor.hit_test { if window.cursor.hit_test != cache.window.cursor.hit_test {
if let Err(err) = winit_window.set_cursor_hittest(window.cursor.hit_test) { if let Err(err) = winit_window.set_cursor_hittest(window.cursor.hit_test) {
window.cursor.hit_test = previous.cursor.hit_test; window.cursor.hit_test = cache.window.cursor.hit_test;
warn!( warn!(
"Could not set cursor hit test for window {:?}: {:?}", "Could not set cursor hit test for window {:?}: {:?}",
window.title, err window.title, err
@ -206,19 +194,19 @@ pub(crate) fn changed_window(
} }
} }
if window.decorations != previous.decorations if window.decorations != cache.window.decorations
&& window.decorations != winit_window.is_decorated() && window.decorations != winit_window.is_decorated()
{ {
winit_window.set_decorations(window.decorations); winit_window.set_decorations(window.decorations);
} }
if window.resizable != previous.resizable if window.resizable != cache.window.resizable
&& window.resizable != winit_window.is_resizable() && window.resizable != winit_window.is_resizable()
{ {
winit_window.set_resizable(window.resizable); winit_window.set_resizable(window.resizable);
} }
if window.resize_constraints != previous.resize_constraints { if window.resize_constraints != cache.window.resize_constraints {
let constraints = window.resize_constraints.check_constraints(); let constraints = window.resize_constraints.check_constraints();
let min_inner_size = LogicalSize { let min_inner_size = LogicalSize {
width: constraints.min_width, width: constraints.min_width,
@ -235,7 +223,7 @@ pub(crate) fn changed_window(
} }
} }
if window.position != previous.position { if window.position != cache.window.position {
if let Some(position) = crate::winit_window_position( if let Some(position) = crate::winit_window_position(
&window.position, &window.position,
&window.resolution, &window.resolution,
@ -262,42 +250,42 @@ pub(crate) fn changed_window(
winit_window.set_minimized(minimized); winit_window.set_minimized(minimized);
} }
if window.focused != previous.focused && window.focused { if window.focused != cache.window.focused && window.focused {
winit_window.focus_window(); winit_window.focus_window();
} }
if window.window_level != previous.window_level { if window.window_level != cache.window.window_level {
winit_window.set_window_level(convert_window_level(window.window_level)); winit_window.set_window_level(convert_window_level(window.window_level));
} }
// Currently unsupported changes // Currently unsupported changes
if window.transparent != previous.transparent { if window.transparent != cache.window.transparent {
window.transparent = previous.transparent; window.transparent = cache.window.transparent;
warn!( warn!(
"Winit does not currently support updating transparency after window creation." "Winit does not currently support updating transparency after window creation."
); );
} }
#[cfg(target_arch = "wasm32")] #[cfg(target_arch = "wasm32")]
if window.canvas != previous.canvas { if window.canvas != cache.window.canvas {
window.canvas = previous.canvas.clone(); window.canvas = cache.window.canvas.clone();
warn!( warn!(
"Bevy currently doesn't support modifying the window canvas after initialization." "Bevy currently doesn't support modifying the window canvas after initialization."
); );
} }
if window.ime_enabled != previous.ime_enabled { if window.ime_enabled != cache.window.ime_enabled {
winit_window.set_ime_allowed(window.ime_enabled); winit_window.set_ime_allowed(window.ime_enabled);
} }
if window.ime_position != previous.ime_position { if window.ime_position != cache.window.ime_position {
winit_window.set_ime_position(LogicalPosition::new( winit_window.set_ime_position(LogicalPosition::new(
window.ime_position.x, window.ime_position.x,
window.ime_position.y, window.ime_position.y,
)); ));
} }
info.previous = window.clone(); cache.window = window.clone();
} }
} }
} }