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 SDL_MOUSE_RELATIVE_CURSOR_VISIBLE #7947

Merged
merged 5 commits into from
Jun 16, 2024
Merged

Add SDL_MOUSE_RELATIVE_CURSOR_VISIBLE #7947

merged 5 commits into from
Jun 16, 2024

Conversation

expikr
Copy link
Contributor

@expikr expikr commented Jul 7, 2023

This hint lets you keep the hardware cursor visible while RelativeMode is active.

Tested working flawlessly on Love2D 11.4 cherry-picked to SDL2.

This also solves #7943 based on my testing, though I have no idea why.

Sample Love2D program:

visible.zip

yaw,pitch = 0,0
sensitivity = 0.0625

function love.load()
    local ffi = require('ffi')
    local sdl = ffi.load('SDL2.dll')
    ffi.cdef[[
        typedef enum {SDL_HINT_DEFAULT , SDL_HINT_NORMAL , SDL_HINT_OVERRIDE } SDL_HintPriority ;
        bool SDL_SetHintWithPriority(const char *name,  const char *value, SDL_HintPriority priority);
    ]]
    sdl.SDL_SetHintWithPriority('SDL_MOUSE_RELATIVE_CURSOR_VISIBLE','1',ffi.C.SDL_HINT_OVERRIDE)
    love.mouse.setCursor(love.mouse.getSystemCursor('crosshair'))
    love.mouse.setRelativeMode(true)
end

function love.draw()
    local w,h = love.graphics.getDimensions()
    love.graphics.setLineStyle('rough')
    love.graphics.setColor(1,0,0)
    love.graphics.line(0,h/2-0.5,w,h/2-0.5)
    love.graphics.line(w/2-0.5,0,w/2-0.5,h)
    love.graphics.setColor(0,1,1)
    love.graphics.line(0,h/2+0.5,w,h/2+0.5)
    love.graphics.line(w/2+0.5,0,w/2+0.5,h)
    love.graphics.setColor(1,1,1)
    love.graphics.print('yaw: ' .. yaw .. '\npitch: ' .. pitch)
end

function love.mousemoved( x, y, dx, dy, istouch )
    if not love.mouse.getRelativeMode() then return end
    yaw   = ( (yaw + dx*sensitivity + 180) % (360) ) - 180
    pitch = math.max( -90, math.min(pitch + dy*sensitivity, 90) )
end

function love.keypressed(key)
    if key=='escape' then 
       love.mouse.setRelativeMode(not love.mouse.getRelativeMode()) 
       local w,h = love.graphics.getDimensions()
       love.mouse.setPosition(w/2,h/2)
    end
end

compiled dll


This pull request also effectively solves #7840 when used in combination with setting SDL_MOUSE_RELATIVE_MODE_CENTER to false.

Basically, this allows me to just perma-enable relativemode so that I always receive WM_INPUT, then just do my own ClipCursor WinAPI manipulations. The once-every-three-second ClipCursor call by SDL can be counteracted by my own monitoring, though perhaps it might also make sense to create a flag that changes the frequency of the correction, both for mitigating @Susko3 's issue by increasing the frequency, or for solving my use case by setting frequency to zero.

It also allows for the map-panning mechanic that @slouken described, but now it is actually possible to achieve it with a hardware cursor rather than being limited to janky osu-esq software cursors.

closes #7250
closes #7840
closes #7936
closes #7943

@slime73
Copy link
Contributor

slime73 commented Jul 7, 2023

I don't know about other platforms but this won't work as expected on macOS, for at least two reasons:

  • There's an internal call to hide the cursor inside the macOS SetRelativeMouseMode implementation:

    /* The hide/unhide calls are redundant most of the time, but they fix
    * https://bugzilla.libsdl.org/show_bug.cgi?id=2550
    */
    if (enabled) {
    [NSCursor hide];
    } else {
    [NSCursor unhide];
    }

  • The system API to put the mouse into relative mode (CGAssociateMouseAndCursorPosition) stops the hardware cursor from moving entirely, while relative mode is on. In order to work around that, the macOS implementation of relative mode would probably need a completely different approach, if there even is one.

@slime73
Copy link
Contributor

slime73 commented Jul 7, 2023

I might have seen this discussed before but I can't find it now - what are the motivations for having the hardware cursor visible while relative mode is active? They seem like opposing concepts to me.

@expikr
Copy link
Contributor Author

expikr commented Jul 7, 2023

The motivation is to keep the hardware cursor visible at a static position, to provide continuity in visual motion such as camera/map panning.

Imagine you're moving your cursor towards the topright of the screen, as you get close to the edge you hold down mouse-3 to trigger map panning. The cursor stays on the same pixel location of the screen while the map camera scrolls. Visually, this is identical to your eyes "chasing" the cursor as it moves across the screen in non-relative mode.

For now, I am satisfied with just having it visibly static at the exact center of the screen. The ability to change the relative-mode-center can be a different flag in the future, or worked around with direct WinAPI calls for now. But This pull request is necessary for any of the workarounds to work properly, because otherwise SDL emits erroneous cursor changes that cannot be cancelled unless you swap out the entire Windows Message loop with your own, at which point you might as well just hand-roll the entire framerwork yourself.

The system API to put the mouse into relative mode (CGAssociateMouseAndCursorPosition) stops the hardware cursor from moving entirely, while relative mode is on. In order to work around that, the macOS implementation of relative mode would probably need a completely different approach, if there even is one.

Yes, that is the intended behavior. The cursor is supposed to remain stationary but visible.

@slime73
Copy link
Contributor

slime73 commented Jul 7, 2023

Would it make sense to have an API to retrieve a bitmap of hardware cursors instead so you could draw it at an arbitrary position while relative mode is active? I don't know if that's actually feasible on every OS, but it would avoid the issues I mentioned.

@expikr
Copy link
Contributor Author

expikr commented Jul 7, 2023

i can already retrieve the in-memory bitmap of the hardware cursor on Windows using GetCursorInfo -> CopyIcon, but that's not exactly what I'm looking for. I'm looking for specifically the exact state that the current hardware cursor is in, because API calls to change the hardware cursor is instantaneous rather than frame-present timing dependent.

@slouken
Copy link
Collaborator

slouken commented Jul 7, 2023

Have you tried this change while connected to your computer via Remote Desktop?

@expikr
Copy link
Contributor Author

expikr commented Jul 7, 2023

Have you tried this change while connected to your computer via Remote Desktop?

Just tested, works flawlessly on remote desktop, and no visible jitter either which is great.

@expikr
Copy link
Contributor Author

expikr commented Jul 14, 2023

Am I overlooking something that is preventing this pull request from being reviewed? I imagined that adding a hint would be a relatively straightforward change, unless I'm missing something.

@slouken
Copy link
Collaborator

slouken commented Jul 14, 2023

No, just time. I need to sit down and think through the implications of what you're asking to add. It's also not supported across platforms, so there's that to consider.

@expikr
Copy link
Contributor Author

expikr commented Jul 14, 2023

I see. Would it reduce the amount to consider if I limit the scope to SDL_MOUSE_RELATIVE_CURSOR_VISIBLE_ON_WINDOWS instead? The intent was that the warp jitter being made visible by this flag is desired, since this is meant also as a debug canary, but if you feel that cursor jitter should never be visible no matter what, then reducing the scope to only rawinput-enabled platforms is also fine by me.

The core issue really boils down to the fact that SDL monopolizes access to instantaneously responding to WM_SETCURSOR, so even if the developer wishes to make the cursor visible via his workaround, there is no escape hatch to hook into the messageloop callback.

@slouken
Copy link
Collaborator

slouken commented Jul 14, 2023

Go ahead and leave it as-is for now. If it's access to the message loop you're looking for, you can call SDL_SetWindowsMessageHook()

@Susko3
Copy link
Contributor

Susko3 commented Jul 20, 2023

Are you building a cross-platform app? If not it seems you're better off just using win32 instead of SDL, as you seem fairly comfortable with it. Or use many of the Windows-only app technologies.

But if you are building for cross-platform, I hope you realise having hints like this doesn't solve anything.


If this were added, I would expect this to leverage the already available public API, something like making SDL_ShowCursor actually work with relative mode (haven't checked if it does). Or having a hint "implicitly hide the cursor in relative mode".

With this PR, calling the following APIs basically makes a contradiction:

SDL_SetHint(SDL_HINT_MOUSE_RELATIVE_CURSOR_VISIBLE, "1");
SDL_SetRelativeMouseMode(SDL_TRUE);
SDL_ShowCursor(SDL_DISABLE);

The cursor is in relative mode, so it should be visible, but according to ShowCursor, it should not be visible.

@expikr
Copy link
Contributor Author

expikr commented Jul 20, 2023

With this PR, calling the following APIs basically makes a contradiction:

The cursor is in relative mode, so it should be visible, but according to ShowCursor, it should not be visible.

Are you sure about that?

image

Before:

    if (cursor && mouse->cursor_shown && !mouse->relative_mode) {

After:

    if (cursor && mouse->cursor_shown && !(mouse->relative_mode && !mouse->relative_mode_cursor_visible)) {

It seems to me that this simply reverses the existing ad-hoc condition that made ShowCursor not work for relative mode.

You can test this in Love2D:

Example Love2D script
yaw,pitch = 0,0
sensitivity = 0.0625

function love.load()
    local ffi = require('ffi')
    sdl = ffi.load('SDL2.dll')
    ffi.cdef[[
        typedef enum {SDL_HINT_DEFAULT , SDL_HINT_NORMAL , SDL_HINT_OVERRIDE } SDL_HintPriority ;
        bool SDL_SetHintWithPriority(const char *name,  const char *value, SDL_HintPriority priority);
        int SDL_ShowCursor(int toggle);
    ]]
    sdl.SDL_SetHintWithPriority('SDL_MOUSE_RELATIVE_CURSOR_VISIBLE','1',ffi.C.SDL_HINT_OVERRIDE)
    love.mouse.setCursor(love.mouse.getSystemCursor('crosshair'))
    love.mouse.setRelativeMode(true)
end

function love.draw()
    local w,h = love.graphics.getDimensions()
    love.graphics.setLineStyle('rough')
    love.graphics.setColor(1,0,0)
    love.graphics.line(0,h/2-0.5,w,h/2-0.5)
    love.graphics.line(w/2-0.5,0,w/2-0.5,h)
    love.graphics.setColor(0,1,1)
    love.graphics.line(0,h/2+0.5,w,h/2+0.5)
    love.graphics.line(w/2+0.5,0,w/2+0.5,h)
    love.graphics.setColor(1,1,1)
    love.graphics.print('yaw: ' .. yaw .. '\npitch: ' .. pitch)
end

function love.mousemoved( x, y, dx, dy, istouch )
    if not love.mouse.getRelativeMode() then return end
    yaw   = ( (yaw + dx*sensitivity + 180) % (360) ) - 180
    pitch = math.max( -90, math.min(pitch + dy*sensitivity, 90) )
end

function love.keypressed(key)
    if key=='escape' then 
       love.mouse.setRelativeMode(not love.mouse.getRelativeMode()) 
       local w,h = love.graphics.getDimensions()
       love.mouse.setPosition(w/2,h/2)
    end
    if key=='h' then
       if 1==sdl.SDL_ShowCursor(-1) then
          sdl.SDL_ShowCursor(0)
       else
          sdl.SDL_ShowCursor(1)
       end
    end
end

visible-toggle.zip

But if you are building for cross-platform, I hope you realise having hints like this doesn't solve anything.

Can you elaborate whether that is you personally not seeing any use for it in your specific use case, or if you are privy to some usage information/statistics/sentiments/feedbacks that you could illuminate this discussion with? If it is the former, can you elaborate on your rationale for forming that opinion?

Are you building a cross-platform app? If not it seems you're better off just using win32 instead of SDL, as you seem fairly comfortable with it. Or use many of the Windows-only app technologies.

I have means for making Windows-specific apps, but this pull request is about specifically giving SDL an escape hatch to not step over the toes with its inadvertently opinionated design choice. In short, this is about SDL rather than about my app.

@Susko3
Copy link
Contributor

Susko3 commented Aug 19, 2023

It seems you have missed the point I was trying to make with the code snippet. I wasn't talking about specifics of the internal implementation of this feature. I was trying to illustrate that the API proposal, in its current form, is badly designed.

Are you having trouble seeing the design flaw? I can try to explain it to you using mathematical logic if you wish.

@expikr
Copy link
Contributor Author

expikr commented Aug 24, 2023

@slouken I'm just curious if you could share, in broad strokes, what are some of the kind of considerations that first comes to your mind when mulling over a pull request as this? Being a non-default behavior and also being an obscurely optional flag, I'm having a hard time imagining what kind of unforeseen side-effects would be caused by its addition, so it would be nice to have some preliminary perspective to chew on while waiting.

@slouken
Copy link
Collaborator

slouken commented Aug 24, 2023

In general I look at whether this introduces code complexity that will add cost to maintenance, whether it makes sense to apply more broadly than just one platform and use case, and whether it's possible for the effect to be implemented outside of SDL if it's a narrow use case.

Here, I'll be looking at all of your feedback and the other PR you have pending, so it's a large enough task that I have to set aside some time to review it, and I haven't had a chance yet.

@slouken
Copy link
Collaborator

slouken commented Dec 29, 2023

Hello! I have not forgotten about your PRs, I just have other things to work on for the SDL3 release. I plan to revisit these before the initial release.

@slouken slouken modified the milestones: 3.2.0, 2.30.0 Dec 29, 2023
@Susko3
Copy link
Contributor

Susko3 commented Dec 30, 2023

I still think that this hint, if implemented, should be changed to SDL_HINT_MOUSE_RELATIVE_IMPLICITLY_HIDE (defaults to true, which would replicate current behaviour on main).

The meaning of the hint should be: "Enabling relative mouse mode will implicitly hide the cursor" -- i.e. calling SDL_SetRelativeMouseMode(SDL_TRUE) will also call SDL_CursorVisible(SDL_FALSE) (of course, disabling relative mode should restore cursor visibility to how it was before).

Having a hint named SDL_MOUSE_RELATIVE_CURSOR_VISIBLE doesn't make much sense given that SDL_CursorVisible() exists.


And I guess you should also add a big fat warning:

Warning

The hardware/OS cursor position when relative mouse mode is enabled is implementation-defined and might not match the mouse coordinates from SDL events, SDL_GetMouseState(), etc. And might not respect application-requested position such as those requested by SDL_WarpMouseInWindow().

@slouken
Copy link
Collaborator

slouken commented Apr 15, 2024

You should be able to do: git rebase -i origin and use 'f' to flatten this to a single commit.

@expikr
Copy link
Contributor Author

expikr commented Apr 16, 2024

I don't know what I'm supposed to do once it took me inside vim.

What is preventing the use of the Squash-and-Merge option?

@slouken
Copy link
Collaborator

slouken commented Apr 16, 2024

Type ZZ in vim, after you finish your edits.

@expikr
Copy link
Contributor Author

expikr commented Apr 16, 2024

fixup 7d6e5931c
#
# Commands:
# p, pick <commit> = use commit
# r, reword <commit> = use commit, but edit the commit message
# e, edit <commit> = use commit, but stop for amending
# s, squash <commit> = use commit, but meld into previous commit
# f, fixup [-C | -c] <commit> = like "squash" but keep only the previous
#                    commit's log message, unless -C is used, in which case
#                    keep only this commit's message; -c is same as -C but
#                    opens the editor
# x, exec <command> = run command (the rest of the line) using shell
# b, break = stop here (continue rebase later with 'git rebase --continue')
# d, drop <commit> = remove commit
# l, label <label> = label current HEAD with a name
# t, reset <label> = reset HEAD to a label
# m, merge [-C <commit> | -c <commit>] <label> [# <oneline>]
#         create a merge commit using the original merge commit's
#         message (or the oneline, if no original merge commit was
#         specified); use -c <commit> to reword the commit message
# u, update-ref <ref> = track a placeholder for the <ref> to be updated
#                       to this position in the new commits. The <ref> is
#                       updated at the end of the rebase
#
# These lines can be re-ordered; they are executed from top to bottom.
#
# If you remove a line here THAT COMMIT WILL BE LOST.
#
# You are editing the todo file of an ongoing interactive rebase.
# To continue rebase after editing, run:
#     git rebase --continue
$ git rebase --continue
error: commit 7d6e5931ce2e1c8f42a3cc5cd06dfd149b9ebb93 is a merge but no -m option was given.
hint: Could not execute the todo command
hint:
hint:     fixup 7d6e5931ce2e1c8f42a3cc5cd06dfd149b9ebb93
hint:
hint: It has been rescheduled; To edit the command before continuing, please
hint: edit the todo list first:
hint:
hint:     git rebase --edit-todo
hint:     git rebase --continue

@slouken
Copy link
Collaborator

slouken commented Apr 16, 2024

Oh, I thought you were on a branch. Go ahead and edit todo and cancel the fixup. You can create a new branch, or just leave it and I can sort it out later.

@slouken slouken merged commit ee559d5 into libsdl-org:main Jun 16, 2024
39 checks passed
@slouken
Copy link
Collaborator

slouken commented Jun 16, 2024

I went ahead and merged this. It's not useful on its own, but it serves as a good starting point for your other discussion.

@expikr
Copy link
Contributor Author

expikr commented Jun 20, 2024

Thanks!

Is there any chance it can get cherry-picked a future SDL2 release, either in its current form or maybe after further refinement?

The main usecase I have for this is as a drop-in DLL replacement for Love2D 11.x (since SDL3 is slated for 12.0)

slouken pushed a commit that referenced this pull request Jun 20, 2024
slouken pushed a commit that referenced this pull request Jun 20, 2024
(cherry picked from commit ee559d5)
(cherry picked from commit 194d72b)
@slouken
Copy link
Collaborator

slouken commented Jun 20, 2024

Yep, I cherry-picked this out for the next SDL2 release.

@expikr
Copy link
Contributor Author

expikr commented Jun 29, 2024

The combination of RELATIVE_CURSOR_VISIBLE and RELATIVE_CURSOR_CENTER have now made this possible:

2024-06-26.21-53-54.mp4

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment