Changes Window usage to Arc<Window>

[matt328]

I wrapped the usage of Window with an Arc. When using vulkano, it's Surface struct also wants to own the Window. Issue #9.

[msparkles]

How do you obtain an Arc of the window?

[parasyte]

How do you obtain an Arc of the window?

Arc::new(window)

😆

But honestly, I don't think this is the right approach. I have experimented with allowing game_loop to accept any type instead of requiring winit::Window. E.g.:

Click to expand patch...

diff --git a/examples/using_a_window.rs b/examples/using_a_window.rs
index 8e7cd8a..cfa8451 100644
--- a/examples/using_a_window.rs
+++ b/examples/using_a_window.rs
@@ -18,7 +18,7 @@ fn main() {
     }, |g| {
         g.game.your_render_function(&g.window);
     }, |g, event| {
-        if !g.game.your_window_handler(event) { g.exit(); }
+        if !g.game.your_window_handler(&g.window, event) { g.exit(); }
     });
 }

@@ -41,8 +41,11 @@ impl Game {
     }

     // A very simple handler that returns false when CloseRequested is detected.
-    pub fn your_window_handler(&self, event: Event<()>) -> bool {
+    pub fn your_window_handler(&self, window: &Window, event: &Event<()>) -> bool {
         match event {
+            Event::MainEventsCleared => {
+                window.request_redraw();
+            }
             Event::WindowEvent { event, .. } => match event {
                 WindowEvent::CloseRequested => {
                     return false;
diff --git a/src/helper.rs b/src/helper.rs
index 41b92a9..440db83 100644
--- a/src/helper.rs
+++ b/src/helper.rs
@@ -55,16 +55,16 @@ mod helper {
     use super::*;
     use winit::event::Event;
     use winit::event_loop::{ControlFlow, EventLoop};
-    use winit::window::Window;

     pub use winit;

-    pub fn game_loop<G, U, R, H, T>(event_loop: EventLoop<T>, window: Window, game: G, updates_per_second: u32, max_frame_time: f64, mut update: U, mut render: R, mut handler: H) -> !
+    pub fn game_loop<G, U, R, H, T, W>(event_loop: EventLoop<T>, window: W, game: G, updates_per_second: u32, max_frame_time: f64, mut update: U, mut render: R, mut handler: H) -> !
         where G: 'static,
-              U: FnMut(&mut GameLoop<G, Time, Window>) + 'static,
-              R: FnMut(&mut GameLoop<G, Time, Window>) + 'static,
-              H: FnMut(&mut GameLoop<G, Time, Window>, &Event<'_, T>) + 'static,
+              U: FnMut(&mut GameLoop<G, Time, W>) + 'static,
+              R: FnMut(&mut GameLoop<G, Time, W>) + 'static,
+              H: FnMut(&mut GameLoop<G, Time, W>, &Event<'_, T>) + 'static,
               T: 'static,
+              W: 'static,
     {
         let mut game_loop = GameLoop::new(game, updates_per_second, max_frame_time, window);

@@ -80,9 +80,6 @@ mod helper {
                         *control_flow = ControlFlow::Exit;
                     }
                 },
-                Event::MainEventsCleared => {
-                    game_loop.window.request_redraw();
-                },
                 _ => {},
             }
         })

This has an issue that because the generic window is type-erased, game_loop can no longer call the request_redraw() method for you; this must be done in the handler closure.

A second issue is that this remains strongly typed with other Winit types! In other words, this patch alone does not address #12.

In any case, it does address #9 without requiring Arc. Using it goes something like this, assuming surface is a vulkano Surface that owns a Window:

Click to expand example code...

fn main() {
    // ...

    let game = Game::default();

    game_loop(event_loop, surface, game, 240, 0.1, |g| {
        g.game.your_update_function();
    }, |g| {
        g.game.your_render_function(g.window.window());
    }, |g, event| {
        if !g.game.your_window_handler(g.window.window(), event) { g.exit(); }
    });
}

#[derive(Default)]
struct Game;

impl Game {
    pub fn your_update_function(&mut self) {
        todo!();
    }

    pub fn your_render_function(&self, window: &Window) {
        todo!();
    }

    pub fn your_window_handler(&self, window: &Window, event: &Event<()>) -> bool {
        match event {
            Event::MainEventsCleared => {
                window.request_redraw();
            }
            Event::WindowEvent { event, .. } => match event {
                WindowEvent::CloseRequested => {
                    return false;
                },
                _ => {},
            },
            _ => {},
        }

        true
    }
}

An even better solution (IMHO) would be providing a trait to abstract over the window details that game-loop needs. The trait could then be implemented for anything that provides access to a window, be it winit, tau, vulkano, sdl2, etc.

[msparkles]

Arc::new(window)

Sorry in advance that I have stopped using this crate, and therefore cannot reproduce the problem I had when I tried using it RN. But:

pub fn new(data: T) -> Arc<T>

...

it's Surface struct also wants to own the Window.

So this still doesn't seem to solve the problem? Unless I'm missing somewhere that vulkano can simply take an Arc of the WIndow.

[parasyte]

Unless I'm missing somewhere that vulkano can simply take an Arc of the WIndow.

Vulkano type-erases the window that you give it, so yes that’s exactly how you would use this PR.

Note that I am not the PR author and this has been open for a long time. I’m just here to offer an alternative for OP and your question caught my eye in the process!

[tuzz]

Hey, sorry, I'll try to review this soon. I think switching to Arc might be sensible. I also like @parasyte's idea of introducing a trait so that different windowing implementations could be made compatible with game_loop.

[matt328]

I authored the PR, and like I said, I'm still pretty green when it comes to rust, but having forked the repo and made this change locally, it seems to work fine, and just figured I'd toss up a PR. I also now realize I could have just chosen not to use the helper method included and supplied my own, but I'm glad it's sparked some discussion!

[tuzz]

I think this is a nice improvement in that it allows you to keep ownership of the window - but is there a way to use an Into trait so that you can supply either a Window or an Arc<Window>. I want to keep the interface as simple as possible and if someone doesn't need to keep hold of Window or doesn't understand what Arc is, I'd rather avoid complicating things.

[parasyte]

but is there a way to use an Into trait so that you can supply either a Window or an Arc<Window>.

It isn't possible to impl Into<Arc<Window>>, exactly, because that would violate the orphan rule (Into and Window are both defined in external crates). Instead, you can use your own trait on any type that you want to support (just implement it on both Window and Arc<Window>).

This is more or less what I was talking about in my first comment. Here's an example with a marker trait (defines no methods) but it could just as easily require a request_redraw method to address the other issue I described. Playground

[tuzz]

I've decided to go ahead and change the windowing interface to Arc<Window> as discussed above. I don't think there are any major downsides so I've released this breaking change in version 1.0.0. Thanks everyone and sorry it's taken me so long to get around to this change!

I've also updated tao to version 0.21.x. I wasn't able to update to 0.22.x because the example uses the menu and menu was moved out of tao in that version. If anyone would like to have a go at updating to 0.22.x that would be appreciated. Thanks agian.