-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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_ConfineCursor #4892
Add SDL_ConfineCursor #4892
Conversation
Change looks good to me, thanks! |
I think it's reasonable to internally add a window relative rect to the implementation and then expose it via a second API, and document the interaction between them. |
I spent some time thinking about this and I wonder if this is the best way to go... Brandon, feel free to correct: ConfineCursor shouldn't actually grab the cursor at all, it should only define the rectangle to which the mouse is restricted to once the mouse is focused or captured. So, you could call this...
... but the window may not be focused at this time, it just knows where to fence the cursor once focus is gained. This way we can have the implementation be entirely separate from the rest of the API, and if the application actually wants to grab they can use the existing API immediately afterward, so weird collisions won't occur. This also makes our life a lot easier in retaining Epic's behavior in UE; we can still support their case where passing |
This sounds good to me! |
Yeah there should not be a need to grab the cursor if we are confining it to a region. Yeah there are issues with this overall in our stuff mainly when attaching a debugger :(. Unlike grabs which you can forcefully break with xdotool. Thanks again for upstreaming this! |
2660152
to
faea3c3
Compare
Something urgent has come up at home so it may be a couple days before I can do this properly, but because the 2.0.18 deadline is in checks notes a week, I did the following:
Not all of the UE changes are strictly ConfineCursor but it definitely takes up the majority of the changes. Grepping X11_ConfineCursor is the easiest way to start. |
faea3c3
to
d08fa2b
Compare
This can wait until you can get the implementation in and tested, that's fine. Thanks! |
Threw together the X portion of this to hopefully make this easier for someone else to work on... it's basically a cleanup of Epic's changes, I'm pretty sure this is everything related to ConfineCursor but a second pair of eyes will help a lot here. TODO:
Nice-to-Have TODO:
|
ef4aa7b
to
0e821e3
Compare
Crisis was averted, so I was able to write the test as well... now you can test with:
For whatever reason the confinement geometry doesn't seem to do what I expect it to, but I don't know the expected behavior here. It definitely confines the cursor at least so it could just be me. Once this has been tested we're good to merge. The expected behavior (AFAIK) is that the example above should confine your cursor to the center of the window in a 720p-sized box. |
e3180cc
to
c42c16a
Compare
Generally its used as a replacement to warping the mouse to the center of the screen. So it should be used along with relative mouse dx/dy, as well as hiding the cursor (doesnt have to be). That is at least our current use case for it in UE. We were running into issues with wrap + hard enough movement being able to click out side of the windows region. Though a mouse grab could have solved that as well, AFAIK warping/mouse grabs are never going to be supported for Wayland (been a while since Ive checked). Another issue is the Win32 API supports or deals with sub region window confinement, ie. not the full Window size, which is something UE does in a few places for its GUI so this gave us support for that as well. |
FYI, SDL's relative mode has been greatly improved in 2.0.18 so you shouldn't need a separate API for this use case (it even works well under RDP!) Confining to an area of the GUI is definitely still useful and a use case for this patch. |
I can also confirm that while warping isn't supported in Wayland, both relative motion and confinement are, so the full behavior should be supported as long as it doesn't depend on the global desktop coordinates (which it looks like the X implementation does, which may explain why my test parameters weren't doing what I thought they would). |
I can look at getting this code in and bumping us to 2.0.18 in 6 days, 14 hours, 33 minutes. Or so. Thanks again for the help, this is fantastic! |
Ah good to know! I have been meaning to track down what looks like to be huge DX/DY generated when using RDP (tiger VNC) on Linux, sooo possibly those fixes address that :). Thanks! |
Aha, so now I know where I was at fault... ConfineCursor is, from what I can tell, a very direct port of Win32 ClipCursor. To cite source (while trying to meet the source disclosure maximum...), the Windows cursor Lock is one line, a call to
Wayland can do window-relative confinement but not quite screen-relative, but at the same time the engine does the right thing and uses this as extra bulletproofing for relative mouse support, making this mostly an X11/Win32 feature (similar to GetWindowBordersSize, actually) while Wayland won't care thanks to the relative pointer protocol doing the right thing from the very start. EDIT: AKA:
Should probably read what I copypaste! |
I think for the purposes of the new API it makes sense for the function to take a window and have the coordinates be window relative. Also whenever that window takes focus, the confinement should take effect. Does this break any use case for UE? |
Yeah Window relative should be fine, we just have to calculate that rather then just absolute cords, something we would have to fix for Wayland anyway :). And the current code in UE on FocusOut we bring barriers down and FocusIn re-confine that region (only if it has input focus), though only X11 video driver area. So yeah happy to adjust our stuff for a better API! |
Rebased and adjusted to make this window-relative. It actually seems to have simplified the implementation a bit while also making it more robust: We no longer even try to use move/resize to update the confinement rectangle (it had some really bizarre behavior before and now it's perfectly consistent!) and the actual implementation is basically the same minus an added query for window bounds. At this point I'm really happy with the state of this patch, so if there aren't any objections this is now A-OK to merge. I'll figure out Wayland later. |
cbad8db
to
e45be45
Compare
This looks good, let's put it in right after we release 2.0.18 |
Let us know if this is urgent and needs to get in for 2.0.18! |
This is pretty urgent since it's a bugfix for ABI compatibility with Unreal Engine 4 & 5 - if we don't have this entry point, attempts to use SDL_DYNAMIC_API will crash the program. |
Ah, got it. Does the entry point in UE 4 and 5 have the same signature? (i.e. take a window and a rect?) |
It has almost the same signature - it currently returns an SDL_bool while we return an int, but thankfully this isn't life-or-death:
|
Actually I stand corrected - in looking over the Git history for the various releases, I forgot that one big problem we had with dynapi was that ConfineCursor wasn't ever actually exposed to the table. So in reality this just affects UE5 which is in early access, and the entry point hasn't actually been properly tagged yet, so we can make the change when updating to 2.0.18 and no breakages will occur. |
This API and implementation comes from the Unreal Engine branch of SDL, which originally called this "SDL_ConfineCursor". Some minor cleanup and changes for consistency with the rest of SDL_video, but there are two major changes: 1. The coordinate system has been changed so that `rect` is _window_ relative and not _screen_ relative, making it easier to implement without having global access to the display. 2. The UE version unset all rects when passing `NULL` as a parameter for `window`, this has been removed as it was an unused feature anyhow. Currently this is only implemented for X, but can be supported on Wayland and Windows at minimum too.
e45be45
to
4a138c9
Compare
Yay it's in! Some last minute notes:
But yeah, this should synchronize the ABIs once 2.0.18 is out and pulled in! |
Awesome! Thanks again for getting this upstreamed, and we'll update our fork as soon as 2.0.18 gets released :) |
Oh, yeah, this rocks... thanks Ethan and Sam! |
You're welcome! The Windows implementation is in: 7d21322 |
and now the macOS implementation: 4db546b |
Wayland implementation (meant to push this to my fork, good thing it was a good branch!) ae67c7d |
The X11 version seems to depend on |
Yes, that seems like a good idea. |
Pushed fa3330b for it (please make sure nothing is broken.) |
A build break issue was reported at 4b42c05#commitcomment-61427659 Proposed the attached patch. OK to apply if @MAN-AT-ARMS reports success? |
He did report success. OK to apply? |
Yup |
Patch applied as 03019c9 |
This adds a new function similar to an extension found in the Unreal Engine version of SDL. The API is changed from UE only in the return value; UE returns an SDL_bool while this returns an int to be consistent with the rest of SDL_video.
Notably absent is a real implementation, for two reasons:
From what I can tell, this API pre-dates much of the grab/focus functionality that exists in modern SDL - it's basically SetWindowMouseGrab but with an added SDL_Rect parameter. This leads me to wonder... would it make more sense to rework this into
SDL_SetWindowMouseGrabRegion
, and expand the existing sysvideo API to include a rect? As the Wayland driver points out, the only real difference even in the backend is the added rectangle, and from what I can tell the X implementation just adds pointer barriers via XFixes, so it seems like implementing exactly the Epic extension would just duplicate a lot of existing functionality and introduce weird collisions with window state and such. The only unique behavior is that passing NULL for the SDL_Window* parameter removes all confinement rects; I don't know how important this feature is but I suppose it wouldn't be hard to call SetWindowMouseGrab(SDL_FALSE) for all your windows...CC @mikesart @BrandonSchaefer, in case I've missed any critical details.
Part of #4822, fixes #4850