Skip to content

Commit

Permalink
Fix Window feedback loop between the OS and Bevy (#7517)
Browse files Browse the repository at this point in the history
# 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>
  • Loading branch information
tim-blackbird and tim-blackbird committed Feb 7, 2023
1 parent 6314f50 commit f69f132
Show file tree
Hide file tree
Showing 7 changed files with 49 additions and 83 deletions.
4 changes: 1 addition & 3 deletions crates/bevy_pbr/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@ mod render;

pub use alpha::*;
use bevy_transform::TransformSystem;
use bevy_window::ModifiesWindows;
pub use bundle::*;
pub use fog::*;
pub use light::*;
Expand Down Expand Up @@ -199,8 +198,7 @@ impl Plugin for PbrPlugin {
.in_set(SimulationLightSystems::AssignLightsToClusters)
.after(TransformSystem::TransformPropagate)
.after(VisibilitySystems::CheckVisibility)
.after(CameraUpdateSystem)
.after(ModifiesWindows),
.after(CameraUpdateSystem),
)
.add_system(
update_directional_light_cascades
Expand Down
2 changes: 0 additions & 2 deletions crates/bevy_render/src/camera/projection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ use bevy_reflect::{
std_traits::ReflectDefault, FromReflect, GetTypeRegistration, Reflect, ReflectDeserialize,
ReflectSerialize,
};
use bevy_window::ModifiesWindows;
use serde::{Deserialize, Serialize};

/// Adds [`Camera`](crate::camera::Camera) driver systems for a given projection type.
Expand Down Expand Up @@ -43,7 +42,6 @@ impl<T: CameraProjection + Component + GetTypeRegistration> Plugin for CameraPro
.add_system(
crate::camera::camera_system::<T>
.in_set(CameraUpdateSystem)
.after(ModifiesWindows)
// We assume that each camera will only have one projection,
// so we can ignore ambiguities with all other monomorphizations.
// FIXME: Add an archetype invariant for this https://github.com/bevyengine/bevy/issues/1481.
Expand Down
2 changes: 0 additions & 2 deletions crates/bevy_text/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ use bevy_asset::AddAsset;
use bevy_ecs::prelude::*;
use bevy_render::{camera::CameraUpdateSystem, ExtractSchedule, RenderApp};
use bevy_sprite::SpriteSystem;
use bevy_window::ModifiesWindows;
use std::num::NonZeroUsize;

#[derive(Default)]
Expand Down Expand Up @@ -83,7 +82,6 @@ impl Plugin for TextPlugin {
.add_system(
update_text2d_layout
.in_base_set(CoreSet::PostUpdate)
.after(ModifiesWindows)
// Potential conflict: `Assets<Image>`
// In practice, they run independently since `bevy_render::camera_update_system`
// will only ever observe its own render target, and `update_text2d_layout`
Expand Down
5 changes: 1 addition & 4 deletions crates/bevy_ui/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@ use bevy_app::prelude::*;
use bevy_ecs::prelude::*;
use bevy_input::InputSystem;
use bevy_transform::TransformSystem;
use bevy_window::ModifiesWindows;
use stack::ui_stack_system;
pub use stack::UiStack;
use update::update_clipping_system;
Expand Down Expand Up @@ -110,7 +109,6 @@ impl Plugin for UiPlugin {
widget::text_system
.in_base_set(CoreSet::PostUpdate)
.before(UiSystem::Flex)
.after(ModifiesWindows)
// Potential conflict: `Assets<Image>`
// In practice, they run independently since `bevy_render::camera_update_system`
// will only ever observe its own render target, and `widget::text_system`
Expand All @@ -135,8 +133,7 @@ impl Plugin for UiPlugin {
.add_system(
flex_node_system
.in_set(UiSystem::Flex)
.before(TransformSystem::TransformPropagate)
.after(ModifiesWindows),
.before(TransformSystem::TransformPropagate),
)
.add_system(ui_stack_system.in_set(UiSystem::Stack))
.add_system(
Expand Down
3 changes: 0 additions & 3 deletions crates/bevy_window/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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
#[derive(Clone)]
pub enum ExitCondition {
Expand Down
32 changes: 11 additions & 21 deletions crates/bevy_winit/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ mod winit_config;
mod winit_windows;

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_windows::*;
Expand All @@ -26,7 +26,7 @@ use bevy_utils::{
};
use bevy_window::{
exit_on_all_closed, CursorEntered, CursorLeft, CursorMoved, FileDragAndDrop, Ime,
ModifiesWindows, ReceivedCharacter, RequestRedraw, Window, WindowBackendScaleFactorChanged,
ReceivedCharacter, RequestRedraw, Window, WindowBackendScaleFactorChanged,
WindowCloseRequested, WindowCreated, WindowFocused, WindowMoved, WindowResized,
WindowScaleFactorChanged,
};
Expand All @@ -39,7 +39,6 @@ use winit::{
event_loop::{ControlFlow, EventLoop, EventLoopBuilder, EventLoopWindowTarget},
};

use crate::system::WinitWindowInfo;
#[cfg(target_arch = "wasm32")]
use crate::web_resize::{CanvasParentResizeEventChannel, CanvasParentResizePlugin};

Expand Down Expand Up @@ -70,7 +69,6 @@ impl Plugin for WinitPlugin {
app.init_non_send_resource::<WinitWindows>()
.init_resource::<WinitSettings>()
.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,
// and so doesn't care about ordering relative to changed_window
.add_systems(
Expand All @@ -79,7 +77,7 @@ impl Plugin for WinitPlugin {
// Update the state of the window before attempting to despawn to ensure consistent event ordering
despawn_window.after(changed_window),
)
.in_set(ModifiesWindows),
.in_base_set(CoreSet::Last),
);

#[cfg(target_arch = "wasm32")]
Expand Down Expand Up @@ -349,7 +347,7 @@ pub fn winit_runner(mut app: App) {
// Fetch and prepare details from the world
let mut system_state: SystemState<(
NonSend<WinitWindows>,
Query<(&mut Window, &mut WinitWindowInfo)>,
Query<(&mut Window, &mut CachedWindow)>,
WindowEvents,
InputEvents,
CursorEvents,
Expand All @@ -376,7 +374,7 @@ pub fn winit_runner(mut app: App) {
return;
};

let (mut window, mut info) =
let (mut window, mut cache) =
if let Ok((window, info)) = window_query.get_mut(window_entity) {
(window, info)
} else {
Expand All @@ -394,7 +392,6 @@ pub fn winit_runner(mut app: App) {
window
.resolution
.set_physical_resolution(size.width, size.height);
info.last_winit_size = size;

window_events.window_resized.send(WindowResized {
window: window_entity,
Expand All @@ -421,11 +418,7 @@ pub fn winit_runner(mut app: App) {
window.resolution.physical_height() as f64 - position.y,
);

// 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(Some(physical_position));
window.set_physical_cursor_position(Some(physical_position));

cursor_events.cursor_moved.send(CursorMoved {
window: window_entity,
Expand All @@ -439,14 +432,7 @@ pub fn winit_runner(mut app: App) {
});
}
WindowEvent::CursorLeft { .. } => {
// Component
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);
}
window.set_physical_cursor_position(None);

cursor_events.cursor_left.send(CursorLeft {
window: window_entity,
Expand Down Expand Up @@ -594,6 +580,10 @@ pub fn winit_runner(mut app: App) {
},
_ => {}
}

if window.is_changed() {
cache.window = window.clone();
}
}
event::Event::DeviceEvent {
event: DeviceEvent::MouseMotion { delta: (x, y) },
Expand Down
Loading

0 comments on commit f69f132

Please sign in to comment.