search

hecrj / coffee

An opinionated 2D game engine for Rust
https://docs.rs/coffee
MIT License
1.08k stars 55 forks source link

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

Closed oguzkocer closed 4 years ago

oguzkocer commented 4 years ago

It'd be good if we could get access to set_cursor_visible and set_cursor_icon of winit::Window. This should be fairly simple to do once #62 is merged in.

@hecrj I'd be happy to work on this if you are OK with this addition. I was thinking we could just add these two methods in graphics::Window, which would just call the winit::Window methods. For the CursorIcon enum, I am guessing we'd want to have an enum in coffee that maps one to one with the winit::window::CursorIcon since it doesn't look like winit is exposed to the consumers of this crate at all. If you have other ideas, I am happy to follow them as well, just let me know if you're interested.

For context, the reason I am interested in these methods is to be able to show a different icon in specific cases. Although set_cursor_icon would be enough to show the standard cursor icons, I believe set_cursor_visible is necessary to completely hide it to show a custom icon. I don't know which one I'll go with yet, but I think someone else might also find use for one or the other. Furthermore, a lot of games do hide the mouse icon completely, so the visibility control would be necessary for that as well.

hecrj commented 4 years ago

I was thinking we could just add these two methods in graphics::Window, which would just call the winit::Window methods.

An interesting alternative, I think, would be to add a new Game::cursor_icon(&self) -> CursorIcon method and let the game loop call this every frame and handle when to change the cursor, centralizing mutations. This would allow us to control what happens when a UserInterface also needs to change the icon (I think it should have priority).

For the CursorIcon enum, I am guessing we'd want to have an enum in coffee that maps one to one with the winit::window::CursorIcon since it doesn't look like winit is exposed to the consumers of this crate at all.

Yes, that sounds great! The enum could have a Hidden variant or similar. There is already a MouseCursor enum in the ui::core module, but it has a slightly different connotation, so I would wait a bit before we try to unify both (maybe after #88 gets merged).

I'd be happy to work on this if you are OK with this addition.

Go ahead! Let me know if you need anything or have any questions!

oguzkocer commented 4 years ago

Thanks a lot for the quick turnaround @hecrj, very much appreciated! 🙇

Game::cursor_icon(&self) -> CursorIcon sounds great to me 👍

The enum could have a Hidden variant or similar. There is already a MouseCursor enum in the ui::core module, but it has a slightly different connotation, so I would wait a bit before we try to unify both (maybe after #88 gets merged).

👍

Go ahead! Let me know if you need anything or have any questions!

I think it might be worth waiting for #62 to be merged before tackling this. I looked into branching from improvement/new-winit-event-loop, but that branch doesn't build for me due to small changes in winit or something, not sure 😬. I also checked if I could just fix the minor build issue, but if I am understanding it correctly you have a patch for winit that you are referring to, so I didn't want to get too deep into it without knowing the context behind it.

On that same subject, excuse me if this issue is the wrong place to ask this, but is there a specific thing holding off the merge of #62? Is there anything I can help with? I am pretty new to Rust, but I can try my best to help get that moving along. Feel free to reply to this in #62 for visibility.

hecrj commented 4 years ago

@oguzkocer You are right, the new winit release has been in alpha for a while and the API is still changing slightly.

On that same subject, excuse me if this issue is the wrong place to ask this, but is there a specific thing holding off the merge of #62? Is there anything I can help with? I am pretty new to Rust, but I can try my best to help get that moving along.

This winit issue is the main blocker: https://github.com/rust-windowing/winit/issues/1082.

Just tonight, I have been able to make macOS work with the revamped redraw request API here: https://github.com/hecrj/winit/commit/1c081d313db30fca3f048f5398b3c28c30412ab1. However, I still have to test it properly.

If you want to help with iOS and Linux Wayland support, I am sure the winit maintainers (and myself) will really appreciate it!

In any case, I have pushed a commit to #62 which should fix the build issues on macOS for the time being and also fixed the conflicts with master.

oguzkocer commented 4 years ago

This winit issue is the main blocker: rust-windowing/winit#1082.

Thanks a lot for the clarification!

If you want to help with iOS and Linux Wayland support, I am sure the winit maintainers (and myself) will really appreciate it!

As much as I'd like to help with that, after checking out the conversation and some of the PRs around it, it looks like it could be a bit more than I could handle at this point. I am sorry about that. Hopefully I'll be a bit more comfortable with Rust and the ecosystem to get involved with more crates in the future.

In any case, I have pushed a commit to #62 which should fix the build issues on macOS for the time being and also fixed the conflicts with master.

Thank you for this as well! I intended to start working on the cursor changes tonight, but ended up debugging why the examples weren't working. I left a comment about it on #62, I hope it's helpful 🤞

hecrj commented 4 years ago

As much as I'd like to help with that, after checking out the conversation and some of the PRs around it, it looks like it could be a bit more than I could handle at this point. I am sorry about that.

There's no need to be sorry! :smile: I was completely clueless just some months ago!

I intended to start working on the cursor changes tonight, but ended up debugging why the examples weren't working.

My bad. I need to start testing everything on multiple platforms... :sweat_smile:

I left a comment about it on #62, I hope it's helpful :crossed_fingers:

Thank you for the catch!

oguzkocer commented 4 years ago

There's no need to be sorry! 😄 I was completely clueless just some months ago!

🙇

My bad. I need to start testing everything on multiple platforms... 😅

No worries, I liked the debugging session. Let me know if you ever need one more person to test on Windows or Mac OS. I am trying my best to spend an hour or two every day with ☕️ so should be able to help out, at least for now.

Thank you for the catch!

Glad it helped!

hecrj commented 4 years ago

This was implemented in #112 :tada: