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

Plaftorm: Add cursor management support #383

Closed
wants to merge 1 commit into from

Conversation

@Melix19
Copy link
Contributor

Melix19 commented Oct 17, 2019

Platform support:

  • AndroidApplication It's rarely used
  • GlfwApplication
  • Sdl2Application
  • GlxApplication
  • EmscriptenApplication It'll be made by mosra
  • XEglApplication

The main usage for this (that is the main reason on why I implemented this) is for the ImGuiIntegration, because right now when hovering the edges of the windows/other draggable elements the cursor doesn't change due to the lack of this feature.

I'm not entirely sure about how good it's implemented, e.g. the way I constructed the array using the last enum element to get the size, and the whole switch-case that I needed to make (yeah, basically everything).

Also, I was thinking about adding the cursor creation in the Application constructor as well, using the Configuration class, but even there I was unsure if it was a good idea or not.

@Melix19 Melix19 force-pushed the Melix19:add_cursor branch from ae06ab5 to 2f46543 Oct 17, 2019
@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Oct 17, 2019

Codecov Report

Merging #383 into master will increase coverage by 3.86%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #383      +/-   ##
=========================================
+ Coverage   72.24%   76.1%   +3.86%     
=========================================
  Files         350     345       -5     
  Lines       18634   16919    -1715     
=========================================
- Hits        13463   12877     -586     
+ Misses       5171    4042    -1129
Impacted Files Coverage Δ
src/Magnum/Audio/Extensions.h 50% <0%> (-50%) ⬇️
src/Magnum/SceneGraph/AbstractFeature.hpp 60% <0%> (-11.43%) ⬇️
src/Magnum/Shaders/Flat.h 71.42% <0%> (-10.39%) ⬇️
src/Magnum/Audio/Listener.h 12.5% <0%> (-9.73%) ⬇️
src/Magnum/Shaders/DistanceFieldVector.h 80% <0%> (-6.67%) ⬇️
src/Magnum/SceneGraph/Camera.h 58.33% <0%> (-5.96%) ⬇️
src/Magnum/SceneGraph/AbstractFeature.h 44.44% <0%> (-5.56%) ⬇️
src/Magnum/Audio/Playable.h 75% <0%> (-5%) ⬇️
src/Magnum/SceneGraph/AbstractGroupedFeature.h 75% <0%> (-5%) ⬇️
src/Magnum/Shaders/Vector.h 80% <0%> (-4.62%) ⬇️
... and 199 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 52ff540...d2a5f81. Read the comment docs.

@Melix19 Melix19 force-pushed the Melix19:add_cursor branch 2 times, most recently from cbefac5 to 5321ba6 Oct 17, 2019
@mosra mosra added this to the 2019.0c milestone Oct 18, 2019
@mosra mosra added this to TODO in Platforms via automation Oct 18, 2019
@Melix19 Melix19 force-pushed the Melix19:add_cursor branch from 5321ba6 to 753e2f4 Oct 18, 2019
Copy link
Owner

mosra left a comment

Thank you, this is great!

For other platforms:

  • Glx / XEglApplication -- don't care, those lack a lot of features anyway and are there just for barebones app setup on embedded systems (like Raspberry Pi and such) ... and then everybody would install GLFW/SDL anyway

  • AndroidApplication -- this would be useful only for Androids with a mouse connected. I think that's so rare there isn't a reason to bother either :)

  • Emscripten -- looking at their APIs, there doesn't seem to be any way to do this. A random idea -- this could be done similarly to how setContainerCssClass() works, adding a CSS class to the <canvas> element, and then expanding the WebApplication.css for example like this:

    #canvas.cursor-crosshair { cursor: crosshair; }
    #canvas.cursor-ibeam { cursor: text; }

    This of course depends on if you have Emscripten set up or not (I don't remember if you do, sorry). If you don't, i can add this myself post-merge :)

Last thing: I'd like to put a stable release out after the weekend. Is it okay if this waits post-release (to have enough time to iron out issues etc.)? Or do you need it in the release?

src/Magnum/Platform/GlfwApplication.h Outdated Show resolved Hide resolved
src/Magnum/Platform/GlfwApplication.h Outdated Show resolved Hide resolved
src/Magnum/Platform/GlfwApplication.h Outdated Show resolved Hide resolved
@Melix19

This comment has been minimized.

Copy link
Contributor Author

Melix19 commented Oct 18, 2019

Thanks for taking time to review this :D

This of course depends on if you have Emscripten set up or not

Unfortunately I don't have Emscripten set up and I don't have really a clear idea on how to do it.

Is it okay if this waits post-release?

Yeah, I'll be fine. I can always use the --Head, so no problem there.

@mosra

This comment has been minimized.

Copy link
Owner

mosra commented Oct 18, 2019

Okay, then I can do the Emscripten part myself, no worries :)

@Melix19 Melix19 force-pushed the Melix19:add_cursor branch 3 times, most recently from b61970f to a4451d3 Oct 18, 2019
@Melix19

This comment has been minimized.

Copy link
Contributor Author

Melix19 commented Oct 18, 2019

Well, I think that I made everything... almost.

I discovered that imgui also wants to show/hide the cursor as he needs to, so maybe I can add a showCursor() and hideCursor() as well

@mosra

This comment has been minimized.

Copy link
Owner

mosra commented Oct 18, 2019

Is that orthogonal to the cursor type? Since it could be done via setCursor(Cursor::None) and then show again as setCursor(Cursor::WhateverWasThereBefore).

Btw., I spotted some cursor-related APIs in GlfwApplication, isn't this hiding/showing there already?

@Melix19

This comment has been minimized.

Copy link
Contributor Author

Melix19 commented Oct 18, 2019

Is that orthogonal to the cursor type?

Yeah, I can make it relative to the type adding a Cursor::Hidden (and a Cursor::Disabled for glfw that has this extra option that can hide the cursor and enable unlimited mouse movement).

I spotted some cursor-related APIs in GlfwApplication, isn't this hiding/showing there already?

Yep, there is already a cursorMode() inside of GlfwApplication::Configuration, but you can only change the mode on creation and not in real time.

@mosra

This comment has been minimized.

Copy link
Owner

mosra commented Oct 18, 2019

Hmm. SDL has that as setMouseLocked(), fortunately that's not just on creation. (Having only a possibility to do that once in Configuration for GLFW seems kinda useless, heh.)

If you want to implement this too, what about the following, to have it consistent for both:

  • have Cursor::Hidden that hides the cursor (and, I don't know, the GL app would draw its own -- laggy one -- for example)
  • have Cursor::HiddenLocked (Disabled doesn't feel like a descriptive name to me) that on GLFW sets this different type, and on SDL hides the cursor calls SDL_SetRelativeMouseMode()

And with this, I think it'd be good to have a cursor() getter as well -- so the app can query the state again (and in case of SDL, it would call SDL_GetRelativeMouseMode() to differentiate between Hidden and HiddenLocked).

Then the setMouseLocked() / isMouseLocked() and GLFW's Configuration::CursorMode could get deprecated in favor of this unified API. (I can do all that deprecation labelling and documenting.)

@Melix19

This comment has been minimized.

Copy link
Contributor Author

Melix19 commented Oct 18, 2019

SDL_SetRelativeMouseMode() is not exactly identical to glfw's Disabled state: glfw locks the cursor on it's position and make it invisible (even moving the mouse, the cursor stays hidden and at the same position); SDL_SetRelativeMouseMode() makes the cursor position only be relative to the window, so it can be moved and it will interact with everything that's in the window.
I know that it's not a major difference but we should't consider this unified if this behavior is slightly different.

The closest we can get is using SDL_SetWindowGrab, it locks the cursor on it's current position as glfw's Disabled state does (or in the top left of the windows if the cursor was outside of the window when the function is called, instead of the center of the screen that glfw uses as default, but I think that this is an insignificant difference).

@Melix19

This comment has been minimized.

Copy link
Contributor Author

Melix19 commented Oct 18, 2019

Oh, ok, nevermind, actually the glfw's Disabled state is the same as the SDL_SetRelativeMouseMode, but if it's called multiple time (I was testing it in an ImGui context) the cursor gets like resetted to the last position, but with SDL_SetRelativeMouseMode actually not and it works well

@mosra

This comment has been minimized.

Copy link
Owner

mosra commented Oct 18, 2019

If you look at setMouseLocked(), it calls both SDL_SetWindowGrab() and SDL_SetRelativeMouseMode() ... but it's been a long time since I did those and I don't really remember what's the desired behavior and if/how GLFW matches that. Also can't really test / verify on anything locally, lately it's been all headless rendering and algorithms, no games at all :)

We could also postpone this part to later, if you don't feel like digging deep into this stuff.

@Melix19

This comment has been minimized.

Copy link
Contributor Author

Melix19 commented Oct 18, 2019

I don't really remember what's the desired behavior and if/how GLFW matches that

After having documented better I found the real differences:

  • GLFW_CURSOR_DISABLED hides the cursor and make it to behave like a camera control, so it has unlimited space to be moved (not expected from Cursor::HiddenLocked).
  • GLFW_CURSOR_HIDDEN just makes the cursor hidden when it's hovering the window, it does not affect the default cursor behavior (so it's good for Cursor::Hidden).
  • SDL_SetRelativeMouseMode() makes the cursor hidden but it locks the cursor to the window borders, so the cursor can't go over them (expected from Cursor::HiddenLocked).

So the problem here is Glfw, in their documentation I haven't found a real alternative to SDL_SetRelativeMouseMode, so the only thing that we could do is to fake it if we really want to.

We could also postpone this part to later, if you don't feel like digging deep into this stuff.

Maybe for now I could add a Cursor::HiddenLocked for sdl using the SDL_SetRelativeMouseMode() and a Cursor::HiddenUnlimited (?) using the GLFW_CURSOR_DISABLED?

@mosra

This comment has been minimized.

Copy link
Owner

mosra commented Oct 18, 2019

Isn't then GLFW_CURSOR_DISABLED the same as SDL_SetRelativeMouseMode() together with SDL_SetWindowGrab()? That's what I'd expect from HiddenLocked -- cursor hidden, unlimited movement, never leaving the window (i.e., a FPS camera). I can't see a case where I would want a (hidden) cursor that gets stuck on window edges instead of letting me move freely.

@Melix19

This comment has been minimized.

Copy link
Contributor Author

Melix19 commented Oct 18, 2019

That's what I'd expect from HiddenLocked -- cursor hidden, unlimited movement, never leaving the window

That's what I'd expect either from HiddenLocked, and it's what setMouseLocked() currently does so it's correct. GLFW_CURSOR_DISABLED has unlimited movement as well, but they do this as the absolute position of the cursor is unlimited and not limited to the window.

E.g. with setMouseLocked() using the MouseMoveEvent::position(), you'll get values within the window size, but with GLFW_CURSOR_DISABLED you'll get an absolute position of like if the cursor is moving outside of the window and even outside the screen. This is what I meant.

@mosra

This comment has been minimized.

Copy link
Owner

mosra commented Oct 19, 2019

SDL_SetRelativeMouseMode() doc says that

Only relative motion events will be delivered, the mouse position will not change.

Which, as far as I understand, means MouseMoveEvent::position() is always the same (probably keeping the last coordinate from the time when relative mode was requested), and thus ~useless -- one should query relativePosition() instead.

Then, as of 89d4a75, GlfwApplication supports relativePosition() as well, which is currently calculated on the application side as a delta to previous move event position. Thus relativePosition() should behave the same for both (while position() not). I think that's consistent enough (as long as the behavior is documented, suggesting users to query relativePosition()) -- and if that's not enough for very specific use cases, one can always call the underlying GLFW/SDL APIs directly.

@Melix19 Melix19 force-pushed the Melix19:add_cursor branch from a4451d3 to 78138e7 Oct 19, 2019
@Melix19

This comment has been minimized.

Copy link
Contributor Author

Melix19 commented Oct 19, 2019

Ok then, now it should be good to go!

Which, as far as I understand, means MouseMoveEvent::position() is always the same (probably keeping the last coordinate from the time when relative mode was requested)

Well, you're right, reading the description I understand that too, but actually, from my test, the absolute position actually changes, but remains within the size of the window (at least on macOS)
so maybe we should document that better than Sdl does :D

Copy link
Owner

mosra left a comment

Last three minor things and then I think it's good to go as well 🎉 Thank you!

src/Magnum/Platform/Sdl2Application.cpp Outdated Show resolved Hide resolved
src/Magnum/Platform/Sdl2Application.cpp Outdated Show resolved Hide resolved
src/Magnum/Platform/GlfwApplication.h Outdated Show resolved Hide resolved
@Melix19 Melix19 force-pushed the Melix19:add_cursor branch from 78138e7 to b816dfe Oct 19, 2019
@Melix19

This comment has been minimized.

Copy link
Contributor Author

Melix19 commented Oct 19, 2019

Last three minor things and then I think it's good to go as well

Done

@mosra

This comment has been minimized.

Copy link
Owner

mosra commented Oct 19, 2019

Thanks a lot! Do you have the ImGuiIntegration counterpart as well? :) So I have something to test with when I get to implementing the Emscripten side.

@Melix19

This comment has been minimized.

Copy link
Contributor Author

Melix19 commented Oct 19, 2019

Do you have the ImGuiIntegration counterpart as well?

I made a really bad version inside my application just for testing (so not actually in the ImGuiIntegration). But I have some concerns on doing it inside ImGuiIntegration, like, how could I manage to access the Application::Cursor without having to include the Sdl2Application? Because I was thinking to make another CursorMap to map the Application::Cursor to the imgui's cursor flags (taking note that imgui uses some cursors that sdl has but glfw doesn't have).

Or maybe I'm thinking it in a wrong way?

@mosra mosra changed the title [WIP] Plaftorm: Add cursor management support Plaftorm: Add cursor management support Oct 19, 2019
Copy link
Owner

mosra left a comment

Heh. So I thought I'd look into how the ImGuiIntegration part could be implemented ... and ended up doing the whole thing: mosra/magnum-integration#56 😆

What's important, while integrating your code I thought it would make sense to move the cursor-related stuff into the "Mouse handling" sections and then discovered there's GlfwApplication::warpCursor(const Vector2i&), calling glfwWarpCursor(). Since you did all this locking/warping/window grabbing investigation, can you tell how does this API fit into the rest? Should we leave it there? Call implicitly to be consistent with what SDL's API do? Expose some equivalent on SDL side?

Sorry that new problems keep piling up 😅

src/Magnum/Platform/Sdl2Application.h Outdated Show resolved Hide resolved
src/Magnum/Platform/GlfwApplication.h Outdated Show resolved Hide resolved
@Melix19

This comment has been minimized.

Copy link
Contributor Author

Melix19 commented Oct 19, 2019

I thought it would make sense to move the cursor-related stuff into the "Mouse handling" sections

Yep, it makes more sense

can you tell how does this API fit into the rest? Expose some equivalent on SDL side?

I think that it's good, even because there is SDL_WarpMouseInWindow for SDL, so I could just add a warpCursor() in there as well and improving the unified API.

@mosra

This comment has been minimized.

Copy link
Owner

mosra commented Oct 19, 2019

Ohh! So warp is "mouse goes out on one side but comes back on the other", right, not related to mouse locking or cursor hiding or that. Yup, agree on adding the equivalent on SDL side.

@Melix19 Melix19 force-pushed the Melix19:add_cursor branch from b816dfe to 8411045 Oct 19, 2019
@mosra
mosra approved these changes Oct 19, 2019
Copy link
Owner

mosra left a comment

Thank you! If I get some extra free time to finish the Emscripten part, this could go in to this release already.

@Melix19 Melix19 force-pushed the Melix19:add_cursor branch from 8411045 to b2942eb Oct 19, 2019
@Melix19 Melix19 force-pushed the Melix19:add_cursor branch from b2942eb to d2a5f81 Oct 19, 2019
@mosra

This comment has been minimized.

Copy link
Owner

mosra commented Nov 6, 2019

Merged in 40bfa7a, sorry for the delay caused by all the release management stuff.

In addition I implemented Emscripten support in 04827d3 and c091ed0 (wow, CSS has 36 cursor types), plus deprecated the old, inconsistent and useless APIs in favor of your new ones in 8d9d247 and c4e3e3c.

Next I'm going to merge the ImGui side and upload an updated example. Thank you for this!

@mosra mosra closed this Nov 6, 2019
Platforms automation moved this from TODO to Done Nov 6, 2019
@Melix19 Melix19 deleted the Melix19:add_cursor branch Nov 6, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Platforms
  
Done
3 participants
You can’t perform that action at this time.