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

Jump to bottom

[Merged by Bors] - Fix Window feedback loop between the OS and Bevy #7517

Closed
wants to merge 2 commits into from
Closed

[Merged by Bors] - Fix Window feedback loop between the OS and Bevy #7517

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0ba7104
ignore changes from winit
tim-blackbird Jan 27, 2023
df992cc
Remove `ModifiesWindows`
tim-blackbird Feb 5, 2023
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: 1 addition & 3 deletions crates/bevy_pbr/src/lib.rs
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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
View file
Open in desktop
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();
}
Comment on lines +584 to +586
Copy link
Contributor Author

@tim-blackbird tim-blackbird Feb 6, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

This is the key change.
We update the cached window after receiving new info from winit to avoid seeing it as a change we need to send back to winit again.

}
event::Event::DeviceEvent {
event: DeviceEvent::MouseMotion { delta: (x, y) },
Expand Down
Loading