Cursor icon support through Game trait

@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.

For the main issue, I think we could do something similar to what the current UI loop does here (ignoring the CursorReturned / CursorTaken part): https://github.com/hecrj/coffee/blob/860753b2d44e15ddfa3f8887aa3b9229ddeb4e7c/src/ui.rs#L349-L358

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 :sweat_smile:

Setting cursor icon and visibility separately makes me a little sad :grimacing: 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.

:+1:

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 🤞

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

I've also made UserInterface take game cursor into account in b60a31f8172ad02b2d981371f35e6f96f4841575, 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 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!

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 6b7ccedbe98776465cb053ebcb740585d74128ce, hopefully that works 😅

hecrj / coffee

Cursor icon support through Game trait #112