Cursor icon support through Game trait #112

oguzkocer · 2019-10-22T03:28:14Z

@hecrj I got stuck on some implementation details & decisions to make for #111, so I thought I'd open a draft PR to make it easier to discuss. The main problem revolves around the following:

When we control the cursor icon through the game trait by updating it every frame, we prevent the system taking over the icon. As an example, if I set the icon to Default and then try to Move the window in Windows 10, the Move icon goes away immediately. That might be what some clients want, but I don't think we should force them into specifying an icon at all times unless they want to.

There are several ways to get around the issue, most of which I think will require us keeping state of the cursor, because as far as I can tell, there is no way to get the current cursor icon from winit.

In Game instead of fn cursor_icon(&self) -> CursorIcon we could have fn cursor_icon(&self, current_icon: CursorIcon) -> CursorIcon which would let the client decide when and how to update the cursor. It'd provide the knowledge of the current cursor_icon which might come in handy in some cases, although I am not sure if it'd be anything common.
We could have an Unchanged variant or something similar in the CursorIcon enum and don't update the icon in this case. I am afraid that this might be a bit more confusing then the first option, but it's hard to say.
In Game instead of fn cursor_icon(&self) -> CursorIcon we could have fn cursor_icon(&self) -> Option<CursorIcon>. None could mean that don't set the cursor_icon manually. It'd result in the last cursor icon to remain as is, but would also let the system take over. Without checking the docs, it might be interpreted as Hidden which would be bad.

I don't even know how this all connects to UserInterface and how the cursor is updated from there. I might be thinking too much over this and just a simple implementation might be fine too, I am not sure. Either way, I'd really appreciate your input on what you think is the best course of action.

Notes about the other parts of this WIP PR:

I don't know if we want to add all the cursor icons from winit or not. I was thinking I'd add only a few of them just to get the implementation out and then separately the different variants could be decided. I am totally fine with implementing the variants you might already have in mind.
Setting cursor icon and visibility separately makes me a little sad 😬
CursorIcon::Hidden being mapped to the Default icon makes me even sadder. If we don't end up keeping track of the state, I think it might be best to use TryFrom trait or have a custom function that returns Option<CursorIcon> and don't update the cursor if an icon variant doesn't make sense. I also played with having an enum like below, but I am not sure if added complexity is worth it.

pub enum CursorIconState {
    Visible(CursorIcon),
    Unchanged,
    Hidden
}

I am leaving the documentation until after the implementation is done to avoid duplicating effort.

@hecrj Sorry for the long PR description. I'd appreciate any feedback you might have. (for the code or the questions) Feel free to ignore the notes for now if you want to focus on the main issue.

hecrj · 2019-10-22T17:18:45Z

For the main issue, I think we could do something similar to what the current UI loop does here (ignoring the CursorReturned / CursorTaken part):

coffee/src/ui.rs

Lines 349 to 358 in 860753b

    
           if new_cursor != self.mouse_cursor { 
        
               if new_cursor == MouseCursor::OutOfBounds { 
        
                   input.update(input::Event::Mouse(mouse::Event::CursorReturned)); 
        
               } else if self.mouse_cursor == MouseCursor::OutOfBounds { 
        
                   input.update(input::Event::Mouse(mouse::Event::CursorTaken)); 
        
               } 
        
               window.update_cursor(new_cursor.into()); 
        
               self.mouse_cursor = new_cursor; 
        
           }

Basically, we store the last cursor_icon and only call Window::update_cursor when it changes. This should stop constantly mutating the cursor.

However, I think this is mainly a winit issue. I do not think user code should be able to override built-in cursor behavior like that, but I am not really familiar with the different windowing APIs.

On a related note, we should probably implement the cursor update logic in the after_draw part of the Loop because the UserInterface loop will need to handle what happens when both the game and UI cursors change. This means we will need to store the current CursorIcon in loop::Default and UserInterface::Loop.

I don't know if we want to add all the cursor icons from winit or not. I was thinking I'd add only a few of them just to get the implementation out and then separately the different variants could be decided.

Yeah, let's just start simple for now. We will need to come up with good names for each of the icons, which won't be easy 😅

Setting cursor icon and visibility separately makes me a little sad 😬
CursorIcon::Hidden being mapped to the Default icon makes me even sadder.

We could implement TryFrom instead of From. It should get rid of the awkward Hidden case and also make the visibility handling more explicit.

I am leaving the documentation until after the implementation is done to avoid duplicating effort.

👍

oguzkocer · 2019-10-22T18:24:29Z

For the main issue, I think we could do something similar to what the current UI loop does here (ignoring the CursorReturned / CursorTaken part):

I actually saw this bit and thought, maybe it's not too bad to keep track of the state of the cursor.

Basically, we store the last cursor_icon and only call Window::update_cursor when it changes. This should stop constantly mutating the cursor.

Although this does address the constant mutation of the cursor, I don't think it addresses the main issue by itself, unless I am missing something. (which is very likely 🙈 ) I assume some games will want to completely control the cursor and don't let the system take over at all. If that assumption is true, we'll still want to set the cursor to whatever is returned from the Game trait even after system changed it. So, we'd still need to have a function signature change or an additional enum variant to handle the other cases.

Having said that, my assumption might be completely off or might be an edge case. So, I'd be totally fine with ignoring all this for now and just implement the general case. I was just worried that by setting the cursor icon through the Game trait, the client might expect it to be set in stone without system interference, at least with the current signature. Hope that makes sense!

However, I think this is mainly a winit issue. I do not think user code should be able to override built-in cursor behavior like that, but I am not really familiar with the different windowing APIs.

Completely agreed! I looked very hard to see if what I am expecting was handled through an enum variant or by an internal logic, but it doesn't seem to be the case. I thought about opening a discussion there, but I am not sure what's the best way to go about it in winit itself either. However I look at it, there is a case involved that makes it more complicated than it needs to be for the general case.

On a related note, we should probably implement the cursor update logic in the after_draw part of the Loop because the UserInterface loop will need to handle what happens when both the game and UI cursors change. This means we will need to store the current CursorIcon in loop::Default and UserInterface::Loop.

Thank you for the note! I must have spent like an hour looking at that code yesterday. I think I am missing something about Rust syntax there to completely understand and follow it. I'll give it another try tonight 🤞

oguzkocer · 2019-10-23T05:52:09Z

@hecrj I've moved the cursor update for the Game to the after_draw and implemented checking whether the cursor needs an update in 9a2fbed.

I've also made UserInterface take game cursor into account in b60a31f, however I opted not to check if the update call is necessary. I tried a bunch of different approaches for this, but I didn't like any of them because it made the code much harder to understand. I feel with these changes, the whole thing is already kind of iffy, so I think it might be best to do that after MouseCursor and CursorIcon merges somehow. For that, I think it might be worth exploring adding a CursorState enum or something similar kind of like below:

pub enum CursorState {
    Hidden,
    OnUIElement(CursorIcon),
    InGame(CursorIcon)
}

or even with more type safety and restrictions:

pub enum CursorState {
    Hidden,
    OnUIElement(UiCursorIcon),
    InGame(GameCursorIcon)
}

This is just a rough idea, I am not proposing these exact states, but hopefully it gets the point across.

In any case, I'd love to hear your thoughts before I continue with this PR. If we don't have a concrete plan for now, I think it might be worth to scope this PR and open an issue for the remaining improvements.

hecrj · 2019-10-24T15:52:14Z

src/ui.rs

+        let new_game_cursor = ui.cursor_icon();
+        if new_game_cursor != self.game_cursor {
+            self.game_cursor = new_game_cursor;
+        }


We may be able to move this inside the if branch below.

Fixed in 13429f2.

hecrj · 2019-10-24T15:54:12Z

src/ui.rs

+            window.update_cursor(self.game_cursor.into());
+        } else {
+            window.set_cursor_visible(true);
+            window.update_cursor(self.ui_cursor.into());


To avoid cursor flickering, it may be a good idea to save the last cursor and visibility in graphics::Window and only actually change state if necessary.

Fixed in 13429f2.

hecrj

I think it's looking quite nice! Thank you! 🎉

Once we unify set_cursor_visible and update_cursor, I think it will be even better.

hecrj · 2019-10-25T03:37:28Z

src/graphics/window.rs

@@ -20,6 +22,8 @@ pub struct Window {
    width: f32,
    height: f32,
    is_fullscreen: bool,
+    cursor_icon: winit::window::CursorIcon,
+    cursor_visible: bool,


We could probably use an Option to encode both things.

Addressed in 66828da.

hecrj · 2019-10-25T03:38:24Z

src/graphics/window.rs

+            self.surface.window().set_cursor_icon(new_cursor);
+            self.cursor_icon = new_cursor;
+        }
+    }


Then, we can make this take an Option<winit::Window::CursorIcon> and remove set_cursor_visible.

Addressed in 66828da.

hecrj · 2019-10-25T03:40:29Z

src/ui.rs

+        if self.mouse_cursor == MouseCursor::OutOfBounds {
+            let game_cursor = ui.cursor_icon();
+            window.set_cursor_visible(game_cursor != CursorIcon::Hidden);
+            window.update_cursor(game_cursor.into());


Finally, if we make CursorIcon implement TryFrom instead of From, then we should be able to call update_cursor easily like this:

window.update_cursor(game_cursor.try_into().ok());

Addressed in 66828da.

hecrj · 2019-10-25T03:42:52Z

src/ui.rs

+            window.update_cursor(game_cursor.into());
+        } else {
+            window.set_cursor_visible(true);
+            window.update_cursor(self.mouse_cursor.into());


Oh, and we will need to add a Some here.

Addressed in 66828da.

…hing

oguzkocer · 2019-10-25T05:01:20Z

@hecrj Thanks for the reviews. I've made your suggested changes and added documentation. I am marking this PR as ready to review. Let me know what you think!

hecrj

I think it looks great! Thank you 🎉

Would you like to update the CHANGELOG too? I enjoy doing that when I finish a feature and I figured you might as well :)

oguzkocer · 2019-10-26T05:15:56Z

Would you like to update the CHANGELOG too? I enjoy doing that when I finish a feature and I figured you might as well :)

@hecrj Thanks for offering! I added a note in 6b7cced, hopefully that works 😅

WIP cursor icon support through Game trait

42aa6a3

oguzkocer added 2 commits October 22, 2019 23:38

Update cursor icon in Game only if it's changed

9a2fbed

Update cursor icon in UserInterface by taking game cursor into account

b60a31f

hecrj reviewed Oct 24, 2019

View reviewed changes

Simplifies cursor update logic across modules

13429f2

hecrj reviewed Oct 25, 2019

View reviewed changes

oguzkocer added 2 commits October 25, 2019 00:47

Implements TryFrom for CursorIcon and uses Option for cursor icon cac…

66828da

…hing

Adds documentation for cursor icon

21ce46f

oguzkocer marked this pull request as ready for review October 25, 2019 05:01

hecrj added the feature New feature or request label Oct 26, 2019

hecrj added this to the 0.4.0 milestone Oct 26, 2019

hecrj approved these changes Oct 26, 2019

View reviewed changes

hecrj changed the title ~~WIP cursor icon support through Game trait~~ Cursor icon support through Game trait Oct 26, 2019

Updates CHANGELOG to include game::cursor_icon addition

6b7cced

hecrj merged commit 5f53955 into hecrj:improvement/new-winit-event-loop Oct 26, 2019

oguzkocer deleted the feature/cursor-icon branch October 27, 2019 18:49

hecrj mentioned this pull request May 10, 2020

Expose control over cursor visibility and icon of winit::Window #111

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cursor icon support through Game trait #112

Cursor icon support through Game trait #112

oguzkocer commented Oct 22, 2019

hecrj commented Oct 22, 2019

oguzkocer commented Oct 22, 2019

oguzkocer commented Oct 23, 2019

hecrj Oct 24, 2019

oguzkocer Oct 25, 2019

hecrj Oct 24, 2019

oguzkocer Oct 25, 2019

hecrj left a comment •

edited

Loading

hecrj Oct 25, 2019

oguzkocer Oct 25, 2019

hecrj Oct 25, 2019

oguzkocer Oct 25, 2019

hecrj Oct 25, 2019

oguzkocer Oct 25, 2019

hecrj Oct 25, 2019

oguzkocer Oct 25, 2019

oguzkocer commented Oct 25, 2019

hecrj left a comment •

edited

Loading

oguzkocer commented Oct 26, 2019

Cursor icon support through Game trait #112

Cursor icon support through Game trait #112

Conversation

oguzkocer commented Oct 22, 2019

hecrj commented Oct 22, 2019

oguzkocer commented Oct 22, 2019

oguzkocer commented Oct 23, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hecrj left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oguzkocer commented Oct 25, 2019

hecrj left a comment • edited Loading

Choose a reason for hiding this comment

oguzkocer commented Oct 26, 2019

hecrj left a comment •

edited

Loading

hecrj left a comment •

edited

Loading