Allow creating multiple windows

[mohe2015]

Hello,

this is an idea on how to allow creating multiple windows. If you like this approach there would definitely be release notes missing as this is a breaking change. Also I would love some help on the multiwindow example as that approach seemed to only work on the web for me and not on desktop (Linux, Wayland).

Kind regards Moritz

[asny]

Hi 👋

Thanks for your PR 🙏 I like the overall idea of supporting multiple windows I just think it's a bit unfortunate that the setup becomes more complex for the common case (one window). However, I definitely see why you want to separate the event loop from the window, that makes a lot of sense. I'm not really sure what the good solution is, I'll have to think about it 🤔 Any input is more than welcome. Also, FYI, this is a bit related to #342. If that was done, maybe it's possible to create the windows with winit and then just use the context and event loop functionality from three-d?

I'm not sure how much I can help with fixing multiple windows on native. Seems to me like it's pretty simple to setup in winit and that you have done it correctly. Did the winit example work for you? Your multiwindow example is sort of working for me. There's two windows, but the second window was blinking and not updating, but that's probably a small issue. I'm on mac, so could be something with linux (wouldn't be the first time) if it's working less well for you.

[mohe2015]

Thanks for your PR pray I like the overall idea of supporting multiple windows I just think it's a bit unfortunate that the setup becomes more complex for the common case (one window). However, I definitely see why you want to separate the event loop from the window, that makes a lot of sense. I'm not really sure what the good solution is, I'll have to think about it thinking Any input is more than welcome.

I already thought about this a bit and this PR is what I came up with. I know it's not great but I didn't come up with a better solution.

Also, FYI, this is a bit related to #342. If that was done, maybe it's possible to create the windows with winit and then just use the context and event loop functionality from three-d?

Creating windows seems to require a reference to the EventLoop https://docs.rs/winit/latest/winit/window/struct.Window.html#method.new so I think we need to handle that explicitly. Then I think you can use https://docs.rs/three-d/0.15.0/three_d/window/struct.Window.html#method.from_winit_window where I removed the EventLoop parameter in this PR.

I'm not sure how much I can help with fixing multiple windows on native. Seems to me like it's pretty simple to setup in winit and that you have done it correctly. Did the winit example work for you?

Yes it did.

Your multiwindow example is sort of working for me. There's two windows, but the second window was blinking and not updating, but that's probably a small issue. I'm on mac, so could be something with linux (wouldn't be the first time) if it's working less well for you.

Maybe the not updating is the same problem on Linux just showing differently. Maybe you have an idea where in the code it is broken? I suspect the self.gl.swap_buffers().unwrap(); because that's where it panics for me.

RUST_BACKTRACE=! cargo run --example multiwindow
    Finished dev [unoptimized + debuginfo] target(s) in 0.18s
     Running `target/debug/examples/multiwindow`
thread 'main' panicked at 'called `Result::unwrap()` on an `Err` value: GlutinError(Error { raw_code: Some(12301), raw_os_message: None, kind: BadSurface })', /home/moritz/Documents/three-d/src/window/winit_window.rs:326:52
stack backtrace:
   0: rust_begin_unwind
             at /rustc/8460ca823e8367a30dda430efda790588b8c84d3/library/std/src/panicking.rs:575:5
   1: core::panicking::panic_fmt
             at /rustc/8460ca823e8367a30dda430efda790588b8c84d3/library/core/src/panicking.rs:64:14
   2: core::result::unwrap_failed
             at /rustc/8460ca823e8367a30dda430efda790588b8c84d3/library/core/src/result.rs:1790:5
   3: core::result::Result<T,E>::unwrap
             at /rustc/8460ca823e8367a30dda430efda790588b8c84d3/library/core/src/result.rs:1112:23
   4: three_d::window::winit_window::Window::get_render_loop::{{closure}}
             at ./src/window/winit_window.rs:326:29
   5: multiwindow::main::{{closure}}
             at ./examples/multiwindow/src/main.rs:11:9
   6: winit::platform_impl::platform::sticky_exit_callback
             at /home/moritz/.cargo/registry/src/github.com-1ecc6299db9ec823/winit-0.28.3/src/platform_impl/linux/mod.rs:884:9
   7: winit::platform_impl::platform::wayland::event_loop::EventLoop<T>::run_return
             at /home/moritz/.cargo/registry/src/github.com-1ecc6299db9ec823/winit-0.28.3/src/platform_impl/linux/wayland/event_loop/mod.rs:549:21
   8: winit::platform_impl::platform::wayland::event_loop::EventLoop<T>::run
             at /home/moritz/.cargo/registry/src/github.com-1ecc6299db9ec823/winit-0.28.3/src/platform_impl/linux/wayland/event_loop/mod.rs:223:25
   9: winit::platform_impl::platform::EventLoop<T>::run
             at /home/moritz/.cargo/registry/src/github.com-1ecc6299db9ec823/winit-0.28.3/src/platform_impl/linux/mod.rs:792:56
  10: winit::event_loop::EventLoop<T>::run
             at /home/moritz/.cargo/registry/src/github.com-1ecc6299db9ec823/winit-0.28.3/src/event_loop.rs:305:9
  11: multiwindow::main
             at ./examples/multiwindow/src/main.rs:10:5
  12: core::ops::function::FnOnce::call_once
             at /rustc/8460ca823e8367a30dda430efda790588b8c84d3/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

[asny]

Sorry for the late reply, I had a busy week.

I already thought about this a bit and this PR is what I came up with. I know it's not great but I didn't come up with a better solution.

I think it's a good solution, don't get me wrong. It's very similar to the solution to support custom events (#320) which I also think was a good solution. So my concern is that the default window becomes more complex to set up in the general case (#320 had the same issue) and, maybe more importantly, three-d needs to support these window features which is not really the purpose of the crate. Also, if winit makes some changes, I'll have to make changes as well. I would rather spend my time on rendering features.

Creating windows seems to require a reference to the EventLoop https://docs.rs/winit/latest/winit/window/struct.Window.html#method.new so I think we need to handle that explicitly. Then I think you can use https://docs.rs/three-d/0.15.0/three_d/window/struct.Window.html#method.from_winit_window where I removed the EventLoop parameter in this PR.

I tried the approach I had in mind in #345 since I anyway wanted to do that refactor (it's almost ready to be merged). So the idea is to neither take ownership of the window nor the event loop. So the only functionality from three-d relating to the window setup, is creating a context from the winit window and generating FrameInput from the winit WindowEvents. I made an example here which is actually quite simple. I also tried making an example with multiple windows here which is working similar to your example (the windows are created, but the rendering is not working correctly). What do you think? Similarly, I also removed the custom event handling since that could be handled completely outside of three-d.

Maybe the not updating is the same problem on Linux just showing differently. Maybe you have an idea where in the code it is broken? I suspect the self.gl.swap_buffers().unwrap(); because that's where it panics for me.

Yes, I believe it's the same problem, that just causes a crash on linux and weird rendering on mac. So I think it's not related to multiple windows, since the windows are working just fine, it's related to the OpenGL context (created using glutin). Because I only used one context, I just made that the current context using make_current, but that's not working when having multiple contexts. We need to switch which context is the current one. Your welcome to look into it, otherwise, I will when I have a bit of time.

[mohe2015]

Sorry for the late reply, I had a busy week.

Definitely a quick reply especially for an open source project and additionally considering you put some thought into code and also wrote a nice alternative implementation.

What do you think? Similarly, I also removed the custom event handling since that could be handled completely outside of three-d.

345 and the multiwindow example are looking great. I would really favor that implementation instead of mine. Decoupling winit from three-d is nice.

Your welcome to look into it, otherwise, I will when I have a bit of time.

I have a patch that fixes that issue in a hopefully sensible way:

diff --git a/examples/multiwindow/src/main.rs b/examples/multiwindow/src/main.rs
index 671262ed..802f7068 100644
--- a/examples/multiwindow/src/main.rs
+++ b/examples/multiwindow/src/main.rs
@@ -29,15 +29,20 @@ pub fn main() {
     };

     let mut windows = HashMap::new();
-    for _ in 0..2 {
+    for i in 0..2 {
         let window = Window::new(&event_loop).unwrap();
-        let context =
-            WindowedContext::from_winit_window(&window, three_d::SurfaceSettings::default())
-                .unwrap();
+        let context = WindowedContext::from_winit_window(
+            &window,
+            three_d::SurfaceSettings {
+                vsync: false, // Wayland hangs in swap_buffers when one window is minimized or occluded
+                ..three_d::SurfaceSettings::default()
+            },
+        )
+        .unwrap();

         let camera = Camera::new_perspective(
             Viewport::new_at_origo(1, 1),
-            vec3(0.0, 0.0, 2.0),
+            vec3(0.0, 0.0, 2.0 + i as f32 * 4.0),
             vec3(0.0, 0.0, 0.0),
             vec3(0.0, 1.0, 0.0),
             degrees(45.0),
@@ -66,8 +71,12 @@ pub fn main() {
                 window.request_redraw();
             }
         }
-        winit::event::Event::RedrawRequested(_) => {
-            for (window, context, frame_input_generator, scene) in windows.values_mut() {
+        winit::event::Event::RedrawRequested(window_id) => {
+            if let Some((window, context, frame_input_generator, scene)) =
+                windows.get_mut(window_id)
+            {
+                context.make_current().unwrap();
+
                 let frame_input = frame_input_generator.generate(context);

                 scene.camera.set_viewport(frame_input.viewport);
diff --git a/src/window/winit_window/windowed_context.rs b/src/window/winit_window/windowed_context.rs
index dd8198ae..d32cc874 100644
--- a/src/window/winit_window/windowed_context.rs
+++ b/src/window/winit_window/windowed_context.rs
@@ -85,7 +85,7 @@ mod inner {

 #[cfg(not(target_arch = "wasm32"))]
 mod inner {
-    use glutin::surface::*;
+    use glutin::{prelude::PossiblyCurrentContextGlSurfaceAccessor, surface::*};

     use super::*;
     ///
@@ -208,6 +208,11 @@ mod inner {
             self.surface.resize(&self.glutin_context, width, height);
         }

+        /// Make this context current. Needed when using multiple windows (contexts)
+        pub fn make_current(&self) -> Result<(), WindowError> {
+            Ok(self.glutin_context.make_current(&self.surface)?)
+        }
+
         /// Swap buffers - should always be called after rendering.
         pub fn swap_buffers(&self) -> Result<(), WindowError> {
             Ok(self.surface.swap_buffers(&self.glutin_context)?)

The additional issue with e.g. wayland is annoying. https://github.com/libsdl-org/SDL/issues/4335 SDL also hat an issue with that in the past. For now I just disabled VSync for the example but this is not a good solution especially considering that this probably now renders as many frames as possible and not e.g. 60 FPS or so.

[mohe2015]

Closing the windows could probably be handled in the example before merging it. Currently it just panics which is not too bad but could probably be easily fixed.

[asny]

Definitely a quick reply especially for an open source project and additionally considering you put some thought into code and also wrote a nice alternative implementation.

True, people shouldn't expect this fast implementation 😆 But I try to at least reply within a couple of days 🙂

345 and the multiwindow example are looking great. I would really favor that implementation instead of mine. Decoupling winit from three-d is nice.

Great 👍 I fixed the last couple of issues and then it's ready to merged from my point of view.

I have a patch that fixes that issue in a hopefully sensible way:

Super nice and simple fix 💪 I see no problem with adding the multi window example to three-d so I've added your changes to #345.

The additional issue with e.g. wayland is annoying. libsdl-org/SDL#4335 SDL also hat an issue with that in the past. For now I just disabled VSync for the example but this is not a good solution especially considering that this probably now renders as many frames as possible and not e.g. 60 FPS or so.

Yeah, that is annoying, but I guess it's up to users to work around that. Could we only do it if using wayland? Or maybe add a link to some elaborate explanation?

Closing the windows could probably be handled in the example before merging it. Currently it just panics which is not too bad but could probably be easily fixed.

I think I fixed it in a4ceb00, at least it's working for me, is it also working for your?

[asny]

Ok, I would prefer one more thing before merge; that the multiwindow example worked on web. Should hopefully be a small task, probably just making multiple canvases, if you have time?

Also, I need to fix CI. The previous setup for automatic runtime testing of examples didn't work for the multiwindow and winit_window examples. I've tried to fix it but didn't really succeed yet, so I disabled it temporarily for #345.

[asny]

Merged the other improvements, so the multi window example is now in #352

[asny]

352 has been merged, so closing this PR.

[mohe2015]

Thank you very much for implementing this!

[asny]

No problem and thank you for the suggestion and your help 🙂