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

[Wayland] Add mouse grab/lock functionality. #15103

Merged
merged 13 commits into from
Mar 20, 2023
Merged

[Wayland] Add mouse grab/lock functionality. #15103

merged 13 commits into from
Mar 20, 2023

Conversation

vanfanel
Copy link
Contributor

@vanfanel vanfanel commented Mar 18, 2023

Description

This implements mouse grab/lock in Wayland, needed for some cores like Dosbox-core to work correctly with mouse-driven games.

This was done with massive help from @ColinKinloch

Related Issues

This long-standing issue is now fixed:
realnc/dosbox-core#48
Mouse was misbehaving in Wayland because of the missing feature this PR adds: mouse grab/lock.

@i30817
Copy link
Contributor

i30817 commented Mar 18, 2023

This PR makes my touch pad think it's always dragging in the last direction in scummvm even if i'm not touching it anymore. Driver is sdl2 or udev or x or linuxraw .

It happens already in the main menu (and the games too). I think you have a bit of more work to do.

Edit: although i was using a debug core, to try to debug this same bug with the author, so i'll redownload it and try again.

edit: still happens.

@vanfanel
Copy link
Contributor Author

Ok, don't merge it yet. There's something still going on...

@vanfanel vanfanel changed the title [Wayland] Add mouse grab/lock functionality. [WIP] [Wayland] Add mouse grab/lock functionality. Mar 18, 2023
@ColinKinloch
Copy link
Contributor

Relative motion is working. But the cursor lock isn't. I'll have a play around and see if I can get it working.

Copy link
Contributor

@ColinKinloch ColinKinloch left a comment

Choose a reason for hiding this comment

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

This will allow the user to unlock the cursor.

input/drivers/wayland_input.c Outdated Show resolved Hide resolved
input/common/wayland_common.h Show resolved Hide resolved
@ColinKinloch
Copy link
Contributor

Okay, I've figured it out, the main issue is that the void *data field of input_wl_grab_mouse is the retroarch wayland input object not the graphics option, so all the wayland objects passed to the locking function are NULL.

This should make things work better:

static void input_wl_grab_mouse(void *data, bool state)
{
   input_ctx_wayland_data_t *wl = (input_ctx_wayland_data_t*)data;

@ColinKinloch
Copy link
Contributor

Also a locked pointer has a listener which can notify retroarch when the pointer has succesfully locked / unlocked.
I think the escape key forces unlock which the unlocked event will notify.

vanfanel and others added 2 commits March 18, 2023 20:40
Co-authored-by: Colin Kinloch <colin@kinlo.ch>
Co-authored-by: Colin Kinloch <colin@kinlo.ch>
@vanfanel
Copy link
Contributor Author

vanfanel commented Mar 18, 2023

@ColinKinloch This change:


static void input_wl_grab_mouse(void *data, bool state)
{
   input_ctx_wayland_data_t *wl = (input_ctx_wayland_data_t*)data;


leads to some compilation errors:


input/drivers/wayland_input.c: In function 'input_wl_grab_mouse':
input/drivers/wayland_input.c:410:10: error: 'input_ctx_wayland_data_t' {aka 'struct input_ctx_wayland_data'} has no member named 'pointer_constraints'
  410 |    if (wl->pointer_constraints) {
      |          ^~
input/drivers/wayland_input.c:413:12: error: 'input_ctx_wayland_data_t' {aka 'struct input_ctx_wayland_data'} has no member named 'locked_pointer'
  413 |          wl->locked_pointer = zwp_pointer_constraints_v1_lock_pointer(wl->pointer_constraints,
      |            ^~
input/drivers/wayland_input.c:413:73: error: 'input_ctx_wayland_data_t' {aka 'struct input_ctx_wayland_data'} has no member named 'pointer_constraints'
  413 |          wl->locked_pointer = zwp_pointer_constraints_v1_lock_pointer(wl->pointer_constraints,
      |                                                                         ^~
input/drivers/wayland_input.c:414:15: error: 'input_ctx_wayland_data_t' {aka 'struct input_ctx_wayland_data'} has no member named 'surface'
  414 |             wl->surface, wl->wl_pointer, NULL, ZWP_POINTER_CONSTRAINTS_V1_LIFETIME_PERSISTENT);
      |               ^~
input/drivers/wayland_input.c:414:28: error: 'input_ctx_wayland_data_t' {aka 'struct input_ctx_wayland_data'} has no member named 'wl_pointer'
  414 |             wl->surface, wl->wl_pointer, NULL, ZWP_POINTER_CONSTRAINTS_V1_LIFETIME_PERSISTENT);
      |                            ^~
input/drivers/wayland_input.c:418:42: error: 'input_ctx_wayland_data_t' {aka 'struct input_ctx_wayland_data'} has no member named 'locked_pointer'
  418 |          zwp_locked_pointer_v1_destroy(wl->locked_pointer);
      |                                          ^~
input/drivers/wayland_input.c:419:12: error: 'input_ctx_wayland_data_t' {aka 'struct input_ctx_wayland_data'} has no member named 'locked_pointer'
  419 |          wl->locked_pointer = NULL;
      |            ^~

@ColinKinloch
Copy link
Contributor

Yup, the input member of the gfx object is a pointer to the input object and vice verca.

@ColinKinloch
Copy link
Contributor

ColinKinloch commented Mar 18, 2023

I should spend some time making the wayland driver more readable. Not naming two separate objects wl adding member names to the listeners etc.

@vanfanel
Copy link
Contributor Author

vanfanel commented Mar 18, 2023

Yup, the input member of the gfx object is a pointer to the input object and vice verca.

@ColinKinloch I did the needed changes in this commit:

vanfanel@6feaa60

However, I am still seeing the same pointer drifting / auto-moving.

@ColinKinloch
Copy link
Contributor

Okay, so it's successfully locking the pointer and delta_x is being set correctly.
On my end when the pointer is locked it doesn't move.
So in input_wl_poll you want to set wl->mouse.x += wl->mouse.delta_x but only if the pointer is locked.

@vanfanel
Copy link
Contributor Author

Okay, so it's successfully locking the pointer and delta_x is being set correctly. On my end when the pointer is locked it doesn't move. So in input_wl_poll you want to set wl->mouse.x += wl->mouse.delta_x but only if the pointer is locked.

@ColinKinloch And how do I know if the pointer is locked at that point?

@ColinKinloch
Copy link
Contributor

ColinKinloch commented Mar 18, 2023

You should add a listener with locked and unlocked function using zwp_locked_pointer_v1_add_listener to the locked pointer. On unlocked you should set wl->locked_pointer = NULL.
Then you can test whether the pointer is locked by the existence of the locked_pointer property.

@vanfanel
Copy link
Contributor Author

@ColinKinloch Ok, did what you said in commit 74913ea

But the cursor is still drifting / auto-moving.

@ColinKinloch
Copy link
Contributor

Oh, I see, move the lines from input_wl_poll to handle_relative_motion

@vanfanel
Copy link
Contributor Author

vanfanel commented Mar 19, 2023

@ColinKinloch Done so in commit f29a351

But the pointer is still auto-moving / drifting.

@ColinKinloch
Copy link
Contributor

Hmm, could you take a recording?

@vanfanel
Copy link
Contributor Author

vanfanel commented Mar 19, 2023

Hmm, could you take a recording?

@ColinKinloch Of course. Here it is:

ishar-retroarch.mp4

As you can see, it auto-moves after I manually move it: as if the pointer had inertia.
It doesn't start auto-moving until I move it.
Maybe something related to acceleration or speed?

@i30817
Copy link
Contributor

i30817 commented Mar 19, 2023

In the touchpad here, if i press exactly in the center i can 'stop' the cursor after it starts moving even i wasn't pressing it anymore if i first counteract the movement.

It's really is some kind of inertia.

@ColinKinloch
Copy link
Contributor

Ah, I was wrong about the unlocked. Since the lifetime is set to persistent the locked_pointer object is still valid, so we don't want to set locked_pointer to NULL.

Copy link
Contributor

@ColinKinloch ColinKinloch left a comment

Choose a reason for hiding this comment

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

This change will fix a crash I'm having

input/drivers/wayland_input.c Outdated Show resolved Hide resolved
input/common/wayland_common.c Outdated Show resolved Hide resolved
input/common/wayland_common.c Outdated Show resolved Hide resolved
@ColinKinloch
Copy link
Contributor

Since relative_motion only fires when the pointer is moving you need to reset delta_x to zero.
So I suppose do it here:

case RETRO_DEVICE_ID_MOUSE_X:
return screen ? wl->mouse.x : wl->mouse.delta_x;
case RETRO_DEVICE_ID_MOUSE_Y:
return screen ? wl->mouse.y : wl->mouse.delta_y;

something like

int x = screen ? wl->mouse.x : wl->mouse.delta_x;
wl->mouse.delta_x = 0;
return x;

@vanfanel
Copy link
Contributor Author

@ColinKinloch Ok! Reset these deltas in commit e7ac2af, and everything seems to work well no, no more strange "inertia"!

@i30817 Can you try in your touchpad configuration, please, and tell us if there's something else you notice? :)

@i30817
Copy link
Contributor

i30817 commented Mar 19, 2023

Everything working. Even myst 3, and versailles 1865 rotoscaped landscapes are working (these games have 360º viewports that rotate around with the mouse).

@ColinKinloch
Copy link
Contributor

From limited testing I see one more thing.
Currently in the RetroArch menu, when the pointer is grabbed, I can lose my cursor off the side of the screen.
The x11 driver has this code to clamp the cursor position:

/* Clamp X */
if (x11->mouse_x < 0)
x11->mouse_x = 0;
if (x11->mouse_x >= win_attr.width)
x11->mouse_x = (win_attr.width - 1);
/* Clamp Y */
if (x11->mouse_y < 0)
x11->mouse_y = 0;
if (x11->mouse_y >= win_attr.height)
x11->mouse_y = (win_attr.height - 1);

@ColinKinloch
Copy link
Contributor

Are there any FPSs with mouse look on RetroArch?

@vanfanel
Copy link
Contributor Author

Are there any FPSs with mouse look on RetroArch?

@ColinKinloch I thought TyrQuake would support mouselook, but it doesn't look like it, I have just tried and mouse doesn't seem to work with it.

What would be the best place to do the coordinates clamping as in X11?

@ColinKinloch
Copy link
Contributor

I've figured out the FPS thing. You need to set "Port 1 Controls" to mouse and keyboard, works great :)

@ColinKinloch
Copy link
Contributor

In the x11 the clamping is done in the poll function when the input isn't grabbed, so I suppose it should be done in input_wl_poll.

@vanfanel
Copy link
Contributor Author

I've figured out the FPS thing. You need to set "Port 1 Controls" to mouse and keyboard, works great :)

Did you do it with TyrQuake? Or did you use another core?

@ColinKinloch
Copy link
Contributor

ColinKinloch commented Mar 19, 2023

TyrQuake, PrBoom, vitaQuakeII.
Anything with content available via the content downloader :)

@ColinKinloch
Copy link
Contributor

You should take a look at video_driver_translate_coord_viewport_wrap and video_driver_translate_coord_viewport used in the winraw and x11 drivers. It seems to clamp the values.

@vanfanel
Copy link
Contributor Author

@ColinKinloch I added the clamping as x11 does in commit fb47a49

How does it work for you?

input/drivers/wayland_input.c Outdated Show resolved Hide resolved
Co-authored-by: Colin Kinloch <colin@kinlo.ch>
Copy link
Contributor

@ColinKinloch ColinKinloch left a comment

Choose a reason for hiding this comment

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

This fixes HiDPI displays getting clamped to a smaller area.

input/drivers/wayland_input.c Outdated Show resolved Hide resolved
Co-authored-by: Colin Kinloch <colin@kinlo.ch>
@vanfanel vanfanel changed the title [WIP] [Wayland] Add mouse grab/lock functionality. [Wayland] Add mouse grab/lock functionality. Mar 20, 2023
@LibretroAdmin LibretroAdmin merged commit aaa53da into libretro:master Mar 20, 2023
@r3claimer
Copy link

@ColinKinloch
Hello, this commit appears to have broken wayland display out for me. RA still runs fine and I can hear audio but the screen is black and I get the follow errors in the log. Launching the UI by itself also results in a black screen. I am one of the developers for Jelos, a custom linux distro. I get the following error in the logs:

[INFO] [Video]: Found display server: "null".
error marshalling arguments for lock_pointer (signature noo?ou): null value passed for arg 2
Error marshalling request: Invalid argument

Rolling back to prior commit fixes the issue: 8d1e575

@ColinKinloch
Copy link
Contributor

Which compositor are you using? KDE, Gnome?

@ColinKinloch
Copy link
Contributor

The log is saying that wl->wl_pointer is NULL in:

gfx->locked_pointer = zwp_pointer_constraints_v1_lock_pointer(gfx->pointer_constraints,
gfx->surface, gfx->wl_pointer, NULL, ZWP_POINTER_CONSTRAINTS_V1_LIFETIME_PERSISTENT);

@ColinKinloch
Copy link
Contributor

@brooksytech what compositor does Jelos use?

@r3claimer
Copy link

@brooksytech what compositor does Jelos use?

Weston in Kiosk shell mode.

@r3claimer
Copy link

And looks like its been fixed already with the new commits you made. Thank You!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants