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

[MERGED] sokol_app.h: support for settings x11 cursor #678

Closed
wants to merge 1 commit into from

Conversation

tomc1998
Copy link
Contributor

Add a new api call, sapp_set_cursor

Add some standard cursors, same as GLFW, e.g. sapp_set_cursor(&SAPP_IBEAM_CURSOR)

Only implemented on x11, tested on my own (zig) project locally

@floooh
Copy link
Owner

floooh commented Jun 23, 2022

Thanks for the PR! Definitely looks useful, especially for the Dear ImGui integration, but I'll need to figure out how to implement this best for Windows and macOS (looking at GLFW can hopefully help there too).

I'll continue working on the sokol-nim bindings for the next few days, and after that I'll try to look into the PR.

@floooh
Copy link
Owner

floooh commented Jun 23, 2022

...there's a couple of CI compilation issues on Windows and Mac, not sure if they can be taken care of without Windows/Mac access (if not, I can look into those):

D:\a\sokol\sokol\sokol_app.h(2544,1): error C2016: C requires that a struct or union have at least one member [D:\a\sokol\sokol\tests\build\win_gl_debug\compile\sokol-compiletest-c.vcxproj]
D:\a\sokol\sokol\sokol_app.h(11523,58): warning C4100: 'cursor': unreferenced formal parameter [D:\a\sokol\sokol\tests\build\win_gl_debug\compile\sokol-compiletest-cxx.vcxproj]
/Users/runner/work/sokol/sokol/tests/compile/../../sokol_app.h:11523:58: error: unused parameter 'cursor' [-Werror,-Wunused-parameter]
SOKOL_API_IMPL void sapp_set_cursor(const sapp_cursor_t *cursor) {

@floooh
Copy link
Owner

floooh commented Jul 4, 2022

FIY I'll start working on this PR now in a branch created from the PR.

One thing I noticed and which I'm going to change is that I'll turn the sapp_cursor_t struct into a simple sapp_cursor enum.

Next I'll look around how to do the Windows and macOS implementation.

Finally I'll change the imgui-samples to use the cursor images.

@floooh
Copy link
Owner

floooh commented Jul 4, 2022

...I'll probably limit the selection of cursor shapes to what GLFW defines:

https://www.glfw.org/docs/3.3/group__shapes.html

...I assume that this is the common subset across X11, macOS and Linux.

@tomc1998
Copy link
Contributor Author

tomc1998 commented Jul 4, 2022

@floooh sapp_cursor_t contains the actual platform-independent cursor data/handle, that needs to be a struct not an enum.

_sapp_x11_create_standard_cursor is the equivalent to GLFW's glfwCreateStandardCursor, but I chose not to expose that to the user & instead just create all the cursors at startup & put them in global vars. Maybe this doesn't suit sokol, in which case remove the globals & expose an sapp_create_standard_cursor function like glfw, which switches on an enum value

The 10 'standard' cursors I initialize at startup are taken from the defines at glfw3.h:1135, possibly this is more up to date than the docs? limiting sokol's support to the same as GLFW's official docs is probably reasonable though. You can see these are used in glfw/tests/cursor.c

@floooh
Copy link
Owner

floooh commented Jul 4, 2022

...ah ok, I've been looking at GLFW 3.3, not 3.4. Ok, I'll use the GLFW 3.4 cursor image types.

I'll still change the public cursor type to an enum, wrapping the platform specific cursor handle in a public type isn't really needed (note that in GLFW, the cursor 'handles' are also just defines: https://github.com/glfw/glfw/blob/da6713cd096a40a4512f468b34c189017e73f987/include/GLFW/glfw3.h#L1161)

I'm still ending up rewriting most of the PR unfortunately, because of the complex relationship between sapp_show/hide_mouse() and the currently set cursor type (on X11, hiding and showing the cursor would always end up calling XUndefineCursor() which I think would switch back to the system default cursor.

The functions _sapp_x11_show_mouse() and _sapp_x11_set_cursor() will basically be merged into a single _sapp_x11_update_cursor() function.

@floooh
Copy link
Owner

floooh commented Jul 4, 2022

You can track my changes here:

master...tomc1998-master

I have also added the mouse cursor update to sokol_imgui.h, on Linux this is working fine. Over the next few days I'll try to look into Windows, macOS and HTML5 (I think updating the cursor should also be possible on the web).

Oh, and I also updated the public names to MOUSECURSOR or mouse_cursor, because the other mouse-related functions are using 'mouse' (e.g. sapp_show_mouse() - sapp_show_mouse_cursor() would now probably make more sense, but I don't want to break compatibility, might make sense for another update which would break compatibility for other reasons though).

This is branched from your PR, so after merging your original contribution should still be properly attributed in the history :)

@tomc1998
Copy link
Contributor Author

tomc1998 commented Jul 4, 2022

Ah go ahead, i don't understand how sokol does other cursor stuff so my code is likely wrong 👍

note that in GLFW, the cursor 'handles' are also just defines

That's what i was trying to explain, these aren't handles! they're just parameters that you pass into glfwCreateStandardCursor to get a handle, which contains the platform specific stuff. sapp_cursor_t is the equivalent of GLFWcursor. That's the difference with my impl, I already call 'create_standard_cursor' for all the standard cursors at startup & then expose them as global vars - glfw doesn't do this, and lets you call 'create_standard_cursor' in your own time.

@floooh
Copy link
Owner

floooh commented Jul 4, 2022

Ah ok, I prefer the enum approach though, less private information leaking into the user side of the API. In a later version we could add another function to associate a custom cursor image with a cursor image, a bit similar to the sapp_set_icon() function (something like

sapp_set_mouse_cursor_image(sapp_mouse_cursor cursor, const sapp_mouse_cursor_desc* desc);

@floooh
Copy link
Owner

floooh commented Jul 4, 2022

PS: another nice effect of this PR is that we can hopefully remove the somewhat weird "user cursor" feature (sapp_desc.user_cursor and SAPP_EVENTTYPE_UPDATE_CURSOR), I never particularly liked this, but it was a necessary workaround for switching the cursor image on Windows.

@floooh
Copy link
Owner

floooh commented Jul 5, 2022

Ok, macOS implemented: 8c1bc36

I guess the reason why GLFW didn't have the 'diagonal' cursor types was because macOS doesn't expose those in the public API (but everybody is using the private methods anyway)

@floooh
Copy link
Owner

floooh commented Jul 6, 2022

Windows implemented: 4549723

@floooh
Copy link
Owner

floooh commented Jul 6, 2022

Reminders:

  • HTML5
  • minimal UWP hack (just fix the compilation issue) UWP (full support)
  • can remove SAPP_EVENT_UPDATE_CURSOR???
  • update header documentation
  • update changelog (post merge)
  • sokol_imgui.h: enable cursor type switching via simgui_desc

@floooh
Copy link
Owner

floooh commented Jul 8, 2022

HTML 5: 56d0910

@floooh
Copy link
Owner

floooh commented Jul 9, 2022

UWP: dfc01d4

@floooh
Copy link
Owner

floooh commented Jul 10, 2022

Ok, my branch which I created from your PR has been merged, but this time Github didn't detect the association with the original PR and didn't mark the PR as merged. Your commits are still recorded in the history though.

So I gonna need to "Close" this PR, even though it has been actually merged.

@floooh floooh closed this Jul 10, 2022
@floooh floooh changed the title sokol_app.h: support for settings x11 cursor [MERGED] sokol_app.h: support for settings x11 cursor Jul 10, 2022
@floooh
Copy link
Owner

floooh commented Jul 10, 2022

...I'll update the changelog next and see how the new enum looks in the Zig and Nim bindings, I priobably need to manually tweak the name mapping.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants