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

can change window settings at runtime #644

Merged
merged 10 commits into from
Oct 15, 2020

Conversation

mockersf
Copy link
Member

@mockersf mockersf commented Oct 7, 2020

Added events to change a few settings for the window. This will keep the windows descriptor and the window from winit in sync.

It could be implemented as an external plugin but it would mean reimplementing some of the code from bevy_winit, like for fullscreen videomode

@karroffel karroffel added C-Feature A new feature, making something new possible A-Windowing Platform-agnostic interface layer to run your app in labels Oct 7, 2020
@cart
Copy link
Member

cart commented Oct 7, 2020

Yeah events seem like a reasonable approach. I see three options for "backend agnostic window property setters":

  1. Use events like this. Update both the backend properties (winit) and bevy_window::Window properties when the event is processed.
  2. expose getters and setters in bevy_window::Window. Setters push changes to an internal "window command queue", which backends then clear each frame. This gives users a friendlier "property based" api / immediately updates bevy_window::Window properties (which seems ideal because they are the source of truth).
  3. create a WindowBackend trait that each backend implements. bevy_window::Windows would then store this internally and call it when a property needs to change. This has a number of implications: (1) dynamic dispatch / boxing, which is a bit uglier (2) well-defined window interface (3) window properties that can be applied "immediately" are applied immediately. bevy_render uses this approach for its RenderResourceContext.

I think my preference is (2) or (3)

@mockersf
Copy link
Member Author

mockersf commented Oct 7, 2020

I gave a try at (2) by adding setters. not sure about getters though? I don't really like having getters that just give the value of the field, but I guess they are needed if I make all the fields private... wdyt?

@cart
Copy link
Member

cart commented Oct 7, 2020

Yeah the purpose of getters in this case is to allow us to make the fields private, which prevents users from setting them directly. This protects them from setting fields directly and nothing happening / state getting out of sync. I think we need to do this to protect users from shooting themselves in the foot.

@mockersf
Copy link
Member Author

mockersf commented Oct 8, 2020

added the getters

I left the field canvas as pub as I'm not sure how it should work if changing the canvas?

also the command_queue is public but doc hidden, to be usable from the backend

crates/bevy_window/src/window.rs Outdated Show resolved Hide resolved
examples/window/change_window_settings.rs Outdated Show resolved Hide resolved
examples/window/change_window_settings.rs Outdated Show resolved Hide resolved
examples/ecs/parallel_query.rs Outdated Show resolved Hide resolved
crates/bevy_winit/src/lib.rs Outdated Show resolved Hide resolved
crates/bevy_window/src/window.rs Show resolved Hide resolved
crates/bevy_window/src/window.rs Show resolved Hide resolved
@cart
Copy link
Member

cart commented Oct 8, 2020

Im fine with leaving the canvas field as-is for now

@mockersf
Copy link
Member Author

mockersf commented Oct 9, 2020

thank you for your review, I applied most of it

@memoryruins
Copy link
Contributor

Thanks for looking into this! While trying the branch on Windows 10 with cargo run --example window_settings, it immediately hanged after the window opened. All other examples continued to run normally as before.

Is anyone else able to reproduce this behavior on Windows?

Looking at the logs (after adding pretty_env_logger to the example), once change_window is called the window stops responding and the trace begins to only emit gilrs' ticks.

 TRACE bevy_ecs::schedule::parallel_executor > run stage "update"
 TRACE bevy_ecs::schedule::parallel_executor > running systems 0..3
 TRACE bevy_ecs::schedule::parallel_executor > prepare 0 &bevy_window::system::exit_on_window_close_system with 0 dependents and 0 dependencies
 TRACE bevy_ecs::schedule::parallel_executor > prepare 1 &bevy_winit::change_window with 1 dependents and 0 dependencies
 TRACE bevy_ecs::schedule::parallel_executor > run &bevy_window::system::exit_on_window_close_system
 TRACE bevy_ecs::schedule::parallel_executor >   * system (1) triggers events: (2): 1
 TRACE bevy_ecs::schedule::parallel_executor > prepare 2 &window_settings::change_title with 0 dependents and 1 dependencies
 TRACE bevy_ecs::schedule::parallel_executor > run &bevy_winit::change_window
 TRACE gilrs::ff::server                     > (Ticks(17)) Setting ff state of Device { inner: FfDevice { inner: Device { id: 0 } }, position: [0.0, 0.0, 0.0], gain: 1.0 } to Magnitude { strong: 0, weak: 0 }
 TRACE gilrs::ff::server                     > (Ticks(18)) Setting ff state of Device { inner: FfDevice { inner: Device { id: 0 } }, position: [0.0, 0.0, 0.0], gain: 1.0 } to Magnitude { strong: 0, weak: 0 }
 TRACE gilrs::ff::server                     > (Ticks(19)) Setting ff state of Device { inner: FfDevice { inner: Device { id: 0 } }, position: [0.0, 0.0, 0.0], gain: 1.0 } to Magnitude { strong: 0, weak: 0 }
...

bevy_window

@mockersf
Copy link
Member Author

mockersf commented Oct 9, 2020

Is anyone else able to reproduce this behavior on Windows?

seems like it's not possible on Windows to change the title of a window from another thread...

I added a workaround just for Windows and title.
As I don't have a Windows available, I tested the fix on a VM with very low resources, it seems to work but everything is very slow in it... @memoryruins could you check on your system please? I also tested resolution and window mode changes on Windows and they work fine, but not resizable and decorations as my system kept crashing too much... could you also test those two?

@memoryruins
Copy link
Contributor

Changing resizable, decorations, title, resolution, and WindowModes all work like a charm for me; great work!

@memoryruins
Copy link
Contributor

seems like it's not possible on Windows to change the title of a window from another thread...

I wonder if it would be reasonable to forward all window change events, not only title changes, to be handled the main thread. Then we would remove the special casing too. I've heard MacOS has some requirements about the main thread too, though I'm unable to test this.

@memoryruins
Copy link
Contributor

memoryruins commented Oct 9, 2020

The simplest way to force everything onto the main thread would be changing the change_window system into a thread_local_system; this should also avoid the need for the extra event readers. I tried the following changes in and everything continued to work for me:

diff --git a/crates/bevy_winit/src/lib.rs b/crates/bevy_winit/src/lib.rs
index 5b3d08fc..69d383f4 100644
--- a/crates/bevy_winit/src/lib.rs
+++ b/crates/bevy_winit/src/lib.rs
@@ -9,7 +9,7 @@ pub use winit_config::*;
 pub use winit_windows::*;
 
 use bevy_app::{prelude::*, AppExit};
-use bevy_ecs::{IntoQuerySystem, Res, ResMut, Resources};
+use bevy_ecs::{IntoThreadLocalSystem, Resources, World};
 use bevy_math::Vec2;
 use bevy_window::{
     CreateWindow, CursorMoved, Window, WindowCloseRequested, WindowCreated, WindowResized, Windows,
@@ -30,18 +30,13 @@ impl Plugin for WinitPlugin {
             // .add_event::<winit::event::WindowEvent>()
             .init_resource::<WinitWindows>()
             .set_runner(winit_runner)
-            .add_system(change_window.system());
-
-        #[cfg(target_os = "windows")]
-        app.add_event::<WindowRename>();
+            .add_system(change_window.thread_local_system());
     }
 }
 
-fn change_window(
-    winit_windows: Res<WinitWindows>,
-    mut windows: ResMut<Windows>,
-    #[cfg(target_os = "windows")] mut rename_events: ResMut<Events<WindowRename>>,
-) {
+fn change_window(_: &mut World, resources: &mut Resources) {
+    let winit_windows = resources.get::<WinitWindows>().unwrap();
+    let mut windows = resources.get_mut::<Windows>().unwrap();
     for bevy_window in windows.iter_mut() {
         let id = bevy_window.id();
         for command in bevy_window.drain_commands() {
@@ -69,13 +64,8 @@ fn change_window(
                     }
                 }
                 bevy_window::WindowCommand::SetTitle { title } => {
-                    #[cfg(not(target_os = "windows"))]
-                    {
-                        let window = winit_windows.get_window(id).unwrap();
-                        window.set_title(&title);
-                    }
-                    #[cfg(target_os = "windows")]
-                    rename_events.send(WindowRename { id, title });
+                    let window = winit_windows.get_window(id).unwrap();
+                    window.set_title(&title);
                 }
                 bevy_window::WindowCommand::SetResolution { width, height } => {
                     let window = winit_windows.get_window(id).unwrap();
@@ -95,12 +85,6 @@ fn change_window(
     }
 }
 
-#[cfg(target_os = "windows")]
-struct WindowRename {
-    id: bevy_window::WindowId,
-    title: String,
-}
-
 fn run<F>(event_loop: EventLoop<()>, event_handler: F) -> !
 where
     F: 'static + FnMut(Event<'_, ()>, &EventLoopWindowTarget<()>, &mut ControlFlow),
@@ -149,8 +133,6 @@ pub fn winit_runner(mut app: App) {
     let mut create_window_event_reader = EventReader::<CreateWindow>::default();
     let mut app_exit_event_reader = EventReader::<AppExit>::default();
 
-    #[cfg(target_os = "windows")]
-    let mut window_rename_event_reader = EventReader::<WindowRename>::default();
 
     handle_create_window_events(
         &mut app.resources,
@@ -180,15 +162,6 @@ pub fn winit_runner(mut app: App) {
             }
         }
 
-        #[cfg(target_os = "windows")]
-        if let Some(window_rename_events) = app.resources.get_mut::<Events<WindowRename>>() {
-            if let Some(event) = window_rename_event_reader.latest(&window_rename_events) {
-                let winit_windows = app.resources.get_mut::<WinitWindows>().unwrap();
-                let window = winit_windows.get_window(event.id).unwrap();
-                window.set_title(&event.title);
-            }
-        }
-
         match event {
             event::Event::WindowEvent {
                 event: WindowEvent::Resized(size),

@cart
Copy link
Member

cart commented Oct 9, 2020

I think thats a good call. Better to bias toward safety here.

@mockersf
Copy link
Member Author

mockersf commented Oct 9, 2020

also works on my Mac with a thread_local_system 👍

@cart cart merged commit 76cc258 into bevyengine:master Oct 15, 2020
@mockersf mockersf deleted the change_window_settings branch October 18, 2020 12:30
joshuajbouw pushed a commit to joshuajbouw/bevy that referenced this pull request Oct 24, 2020
can change window settings at runtime
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Windowing Platform-agnostic interface layer to run your app in C-Feature A new feature, making something new possible
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants