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
Closed

Plaftorm: Add cursor management support #383

wants to merge 1 commit into from

Conversation

melix99
Copy link
Contributor

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

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

@mosra mosra added this to the 2019.0c milestone Oct 18, 2019
Copy link
Owner

@mosra mosra left a comment

Choose a reason for hiding this comment

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

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
@melix99
Copy link
Contributor Author

melix99 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
Copy link
Owner

mosra commented Oct 18, 2019

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

@melix99
Copy link
Contributor Author

melix99 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
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?

@melix99
Copy link
Contributor Author

melix99 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
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.)

@melix99
Copy link
Contributor Author

melix99 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).

@melix99
Copy link
Contributor Author

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

@melix99
Copy link
Contributor Author

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

@melix99
Copy link
Contributor Author

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

@melix99
Copy link
Contributor Author

melix99 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 mosra left a comment

Choose a reason for hiding this comment

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

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
@melix99
Copy link
Contributor Author

melix99 commented Oct 19, 2019

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

Done

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

@melix99
Copy link
Contributor Author

melix99 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 mosra left a comment

Choose a reason for hiding this comment

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

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
@melix99
Copy link
Contributor Author

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

Copy link
Owner

@mosra mosra left a comment

Choose a reason for hiding this comment

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

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

@mosra
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
@melix99 melix99 deleted the add_cursor branch November 6, 2019 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

3 participants