Add SDL_ConfineCursor

[flibitijibibo]

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:

1. The X implementation, as you might expect, does more than add a single function and I haven't had time to try and bring the changes to upstream. I threw together a starting point for Wayland because I knew it would be mostly straightforward. However...
2. It's not actually clear if this exact API makes sense anymore as of 2.0.16

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

[BrandonSchaefer]

Change looks good to me, thanks!

[slouken]

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.

[flibitijibibo]

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

extern SDL_Window* window;
SDL_Rect confinement;
SDL_SetWindowSize(window, 1920, 1080);
confinement.x = (1920 / 2) - (1280 / 2);
confinement.y = (1080 / 2) - (720 / 2);
confinement.w = 1280;
confinement.h = 720;
SDL_ConfineCursor(window, &confinement);

... 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 NULL for the window just removes all the confinement rectangles and we don't have to worry about what the window focus behavior is supposed to be.

[slouken]

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

This sounds good to me!

[BrandonSchaefer]

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!

[flibitijibibo]

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:

• Pushed an updated commit to include the clarifications made above, but also intentionally has no implementations, so it can be safely merged before 2.0.18 in case we miss the deadline.
• Pushed a branch that contains the UE changes to 2.0.16, so everyone can see this if they want to beat me to this: flibitijibibo@c8bc08a

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.

[slouken]

This can wait until you can get the implementation in and tested, that's fine.

Thanks!

[flibitijibibo]

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:

• Test X11 (probably need to add to a new key to the SDL test framework)
• Make the appropriate changes to configure.ac Done

Nice-to-Have TODO:

• Wayland support (just store a wl_region and maybe call Wayland_Input_confine_cursor again)

[flibitijibibo]

Crisis was averted, so I was able to write the test as well... now you can test with:

cmake .. -DSDL_SHARED=OFF -DSDL_TEST=ON
make
./test/testgl2 --geometry 1440x900 --confine-cursor 80,90,1280,720

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.

[BrandonSchaefer]

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.

[slouken]

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

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.

[flibitijibibo]

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

[mikesart]

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!

[BrandonSchaefer]

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

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.

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!

[flibitijibibo]

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 ::ClipCursor. On Linux, however...

	// Lock/Unlock the cursor
	if ( Bounds == nullptr )
	{
		SDL_ConfineCursor(CurrentFocusWindow->GetHWnd(), nullptr);
	}
	else
	{
		CursorClipRect = GetConfinementRect(Bounds, CurrentFocusWindow);

		// We dont want to set a negative bounding region. If Top, Left, Bottom, Right are all 0
		if (CursorClipRect.x >= 0 && CursorClipRect.y >= 0 && CursorClipRect.w > 0 && CursorClipRect.h > 0)
		{
			SDL_ConfineCursor(CurrentFocusWindow->GetHWnd(), &CursorClipRect);
		}
	}

GetConfinementRect is basically a routine that applies the results of SDL_GetWindowBordersSize to Bounds. So contrary to what I thought above, this should basically be treated as ClipCursor, AKA the dimensions are screen relative, not window relative, even though the boundaries are attached to the window.

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:

  * \param rect A rectangle area in screen space coordinates.

Should probably read what I copypaste!

[slouken]

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?

[BrandonSchaefer]

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!

[flibitijibibo]

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.

[slouken]

This looks good, let's put it in right after we release 2.0.18

[slouken]

Let us know if this is urgent and needs to get in for 2.0.18!

[flibitijibibo]

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.

[slouken]

Ah, got it. Does the entry point in UE 4 and 5 have the same signature? (i.e. take a window and a rect?)
I'm fine with adding a UE specific entrypoint that provides compatibility but isn't advertised in the headers.

[flibitijibibo]

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:

• UE doesn't actually use the return value at all, so we can rely on the fact that sizeof(SDL_bool) lines up and get away with the behavioral difference.
• Changing this and backporting it to all supported UE versions while not actually changing behavior shouldn't be difficult. (Brandon, I'll write that patch up this week if nobody beats me to it.)

[flibitijibibo]

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.

[flibitijibibo]

Yay it's in! Some last minute notes:

• We renamed this to SDL_SetWindowMouseRect, to align with the other SDL_Window functions
• We removed the null window parameter functionality, since it wasn't in use and isn't something we support for the other functions. This is easy to replicate on the app side if needed.

But yeah, this should synchronize the ABIs once 2.0.18 is out and pulled in!

[BrandonSchaefer]

Awesome! Thanks again for getting this upstreamed, and we'll update our fork as soon as 2.0.18 gets released :)

[mikesart]

Oh, yeah, this rocks... thanks Ethan and Sam!

[slouken]

You're welcome!

The Windows implementation is in: 7d21322

[slouken]

and now the macOS implementation: 4db546b

[flibitijibibo]

Wayland implementation (meant to push this to my fork, good thing it was a good branch!) ae67c7d

[sezero]

The X11 version seems to depend on X11/extensions/XInput2.h i.e. libXi-devel because of BarrierEventID type definition. Should we adjust the --enable-video-x11-xfixes option to depend on that?

[slouken]

The X11 version seems to depend on X11/extensions/XInput2.h i.e. libXi-devel because of BarrierEventID type definition. Should we adjust the --enable-video-x11-xfixes option to depend on that?

Yes, that seems like a good idea.

[sezero]

Pushed fa3330b for it (please make sure nothing is broken.)

[sezero]

A build break issue was reported at 4b42c05#commitcomment-61427659

Proposed the attached patch. OK to apply if @MAN-AT-ARMS reports success?
patch.txt

[sezero]

A build break issue was reported at 4b42c05#commitcomment-61427659

Proposed the attached patch. OK to apply if @MAN-AT-ARMS reports success? patch.txt

He did report success. OK to apply?

[slouken]

A build break issue was reported at 4b42c05#commitcomment-61427659
Proposed the attached patch. OK to apply if @MAN-AT-ARMS reports success? patch.txt

He did report success. OK to apply?

Yup

[sezero]

Patch applied as 03019c9