Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add cursor support #252

Merged
merged 1 commit into from
Jan 17, 2025
Merged

Add cursor support #252

merged 1 commit into from
Jan 17, 2025

Conversation

Nemrav
Copy link
Contributor

@Nemrav Nemrav commented Aug 20, 2024

Draft, loads cursors, animated and not animated, handles switching between cursors.
Currently contains code just for testing/demoing: right click the mouse to cycle through loaded cursors. The watch is set as an i-beam cursor so that you can try hovering over a text field (ex. in the new-game menu) to observe how the Cursor Manager handles auto-switching between animated cursors.

Loading of the cursors and generating new resolutions handled by the c++ side CursorSingleton. Managing what frame to display, and when to switch cursors handled by gdscript side CursorManager. This architecture is because we want a c++ dataloader, but we also need to listen to the scene tree to know when its safe to call a hardware cursor change.

Possible improvements:

  • Doesn't support the full image spec (unhandled things like 16bit colour images that vic2 doesn't use)
  • Might want to change how I handle collecting both .cur and .CUR files in the loading function (perhaps a new, "ignore-case" parameter for .lookup_files_in_dir_recursive?
  • CursorManager creates a new compat_Cursor inner class whenever we switch cursors, which re-fetches the cursor stored in CursorSingleton. We can store a list of all compat_cursors to avoid this.

@Nemrav Nemrav added the enhancement New feature or request label Aug 20, 2024
Copy link
Contributor

@Hop311 Hop311 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some random comments from looking through the code (not form proper testing yet)

@Nemrav Nemrav marked this pull request as ready for review November 23, 2024 20:53
@Nemrav
Copy link
Contributor Author

Nemrav commented Nov 23, 2024

Main concern for the review is the initial loading screen:

  • is it fine that the loading screen initially uses the normal arrow cursor until the player's mouse moves to cause an update to the "busy" (loading) cursor. I think this might just be down to godot.

@Nemrav Nemrav changed the title Cursors Add cursor support Nov 23, 2024
@Nemrav Nemrav force-pushed the cursors branch 2 times, most recently from f702d13 to 502df1c Compare December 11, 2024 21:52
Copy link
Contributor

@Hop311 Hop311 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work 🎉
Could you address the comments I've made + rebase onto the current master branch? I'll continue reviewing tomorrow after work.
Also I noticed that in the province search panel, the cursor goes to the normal I shape when hovering over the text edit box, while in Vic2 it stays as the pointing gauntlet.

@Nemrav Nemrav force-pushed the cursors branch 2 times, most recently from e4fc499 to b3110e2 Compare December 14, 2024 19:57
@Nemrav Nemrav force-pushed the cursors branch 3 times, most recently from 3de13ca to 6cacebb Compare December 16, 2024 00:38
Copy link
Contributor

@Hop311 Hop311 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left a bunch of comments, the most significant being about handling pixels without creating loads of intermediate PackedByteArrays.
Lemme know what you come up with, tbh we're nearing the point where I should just approve this (maybe after Spartan taking a quick look too) and leave further optimisations for the future, as this is working fine but just has so much code that it's easy to keep finding things to tweak

@Nemrav Nemrav force-pushed the cursors branch 5 times, most recently from da53da1 to fd98127 Compare December 18, 2024 15:22
Copy link
Contributor

@Hop311 Hop311 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work, I've left some simple GDScript comments, once they're addressed I'm happy to get this merged :)

@Nemrav Nemrav force-pushed the cursors branch 3 times, most recently from d42d2c8 to 674aa8f Compare December 19, 2024 02:35
Hop311
Hop311 previously approved these changes Dec 19, 2024
Copy link
Contributor

@Hop311 Hop311 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another absolutely phenomenal PR, well done Nemrav 🎉 Approved, just with a couple final non-blocking comments:

  • I don't see the spinning compass mouse on the loading screen, but that's probably because everything gets loaded in a single call to the SIM repo/dataloader, so we'll need to switch to a staged approach in a later PR to have access to custom cusors before the game finishes loading
  • Brick mentioned some remaining Mac issues, up to you and him how to deal with those

@Nemrav
Copy link
Contributor Author

Nemrav commented Dec 19, 2024

* I don't see the spinning compass mouse on the loading screen, but that's probably because everything gets loaded in a single call to the SIM repo/dataloader, so we'll need to switch to a staged approach in a later PR to have access to custom cusors before the game finishes loading

This seems to be a quirk of godot. To get it to appear, you need to initial logo animation away to get the proper loading screen which has the cursor mode marker as "busy" (this is expected). Then, you need to move the mouse so godot registers that it needs to change the cursor to match what the control nodes specify (not what we want). We could get around this at a later date by manually changing the Arrow_cursor to "busy" when the stop watch is needed as a work-around.

@Nemrav Nemrav force-pushed the cursors branch 2 times, most recently from 7f3771e to d619d92 Compare January 14, 2025 17:10
Also set the normal, busy, and ibeam cursors.
Copy link
Contributor

@Hop311 Hop311 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM on Windows, and we can fix any Mac issues in a future PR. It can be hard to catch the busy icon when the game is loading, often I only see the normal one but I think that's mostly down to timing issues between the game window and the dataloader.

@Nemrav Nemrav merged commit bc8a7de into OpenVicProject:master Jan 17, 2025
9 checks passed
@Nemrav Nemrav deleted the cursors branch January 17, 2025 21:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants