[PATCH v2 0/1] MR7352: winewayland: Request warp to win32 position on pointer unlock.

[Attila Fidan (＠atticf)]

If a surface is being clipped and hides the cursor, drawing its own one,
winewayland constrains using a pointer lock and enables Wayland relative
motion. If the application decides to stop drawing its own cursor and
make the cursor visible, winewayland will disable relative motion and
pointer lock, and enable pointer confinement. The user will perceive a
pointer jump from the win32/application drawn cursor to where the
Wayland pointer is after being unlocked and an absolute motion event is
received, because they were desynchronized due to the Wayland one being
locked in place.

The pointer constraints protocol says this:
> If the client is drawing its own cursor, it should update the position
> hint to the position of its own cursor. A compositor may use this
> information to warp the pointer upon unlock in order to avoid pointer
> jumps.

So, right before unlocking, make a request for the compositor to warp
the pointer to the win32 position on pointer unlock.

--
  v2: winewayland: Request warp to win32 position on pointer unlock.
 
https://gitlab.winehq.org/wine/wine/-/merge_requests/7352

[PATCH v2 1/1] winewayland: Request warp to win32 position on pointer unlock.

[Attila Fidan]

From: Attila Fidan <dev(a)print0.net>

If a surface is being clipped and hides the cursor, drawing its own one,
winewayland constrains using a pointer lock and enables Wayland relative
motion. If the application decides to stop drawing its own cursor and
make the cursor visible, winewayland will disable relative motion and
pointer lock, and enable pointer confinement. The user will perceive a
pointer jump from the win32/application drawn cursor to where the
Wayland pointer is after being unlocked and an absolute motion event is
received, because they were desynchronized due to the Wayland one being
locked in place.

The pointer constraints protocol says this:
> If the client is drawing its own cursor, it should update the position
> hint to the position of its own cursor. A compositor may use this
> information to warp the pointer upon unlock in order to avoid pointer
> jumps.

So, right before unlocking, make a request for the compositor to warp
the pointer to the win32 position on pointer unlock.
---
 dlls/winewayland.drv/wayland_pointer.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/dlls/winewayland.drv/wayland_pointer.c b/dlls/winewayland.drv/wayland_pointer.c
index c20ba170285..f8c2cebc881 100644
--- a/dlls/winewayland.drv/wayland_pointer.c
+++ b/dlls/winewayland.drv/wayland_pointer.c
@@ -772,6 +772,32 @@ static void wayland_pointer_update_constraint(struct wl_surface *wl_surface,
 
     if (!needs_lock && pointer->zwp_locked_pointer_v1)
     {
+        POINT cursor_pos;
+        struct wayland_win_data *data;
+        struct wayland_surface *surface;
+        int warp_x, warp_y;
+
+        NtUserGetCursorPos(&cursor_pos);
+        if (!(data = wayland_win_data_get(wl_surface_get_user_data(wl_surface))))
+            goto unlock;
+        if (!(surface = data->wayland_surface))
+        {
+            wayland_win_data_release(data);
+            goto unlock;
+        }
+        wayland_surface_coords_from_window(surface,
+                cursor_pos.x - surface->window.rect.left,
+                cursor_pos.y - surface->window.rect.top,
+                &warp_x, &warp_y);
+        wayland_win_data_release(data);
+        zwp_locked_pointer_v1_set_cursor_position_hint(
+                pointer->zwp_locked_pointer_v1,
+                wl_fixed_from_int(warp_x),
+                wl_fixed_from_int(warp_y));
+        wl_surface_commit(wl_surface);
+        TRACE("Attempted warp to %d,%d, surface-local %d,%d.\n",
+                (int)cursor_pos.x, (int)cursor_pos.y, warp_x, warp_y);
+unlock:
         TRACE("Unlocking from hwnd=%p\n", pointer->constraint_hwnd);
         zwp_locked_pointer_v1_destroy(pointer->zwp_locked_pointer_v1);
         pointer->zwp_locked_pointer_v1 = NULL;
-- 
GitLab

https://gitlab.winehq.org/wine/wine/-/merge_requests/7352

[Alexandros Frantzis (＠afrantzis)]

Alexandros Frantzis (@afrantzis) commented about dlls/winewayland.drv/wayland_pointer.c:
> if (!needs_lock && pointer->zwp_locked_pointer_v1)
>      {
> +        POINT cursor_pos;
> +        struct wayland_win_data *data;
> +        struct wayland_surface *surface;
> +        int warp_x, warp_y;
> +
> +        NtUserGetCursorPos(&cursor_pos);
> +        if (!(data = wayland_win_data_get(wl_surface_get_user_data(wl_surface))))

We would like to avoid acquiring the `win_data_mutex` while holding the `wayland_pointer.mutex` (which is always held while calling `wayland_pointer_update_constraint`), to avoid potential deadlocks (even if it's not a problem now, it could become one in the future). Perhaps calculate the cursor position externally and pass it in, like we do for the confine rect in `WAYLAND_ClipCursor`?

Passing in the cursor position in `wayland_pointer_clear_constraint` would be more involved (since the function itself is called under pointer mutex lock), but I think we can ignore that case anyway since we call that function only on surface destruction.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/7352#note_95001

[Alexandros Frantzis (＠afrantzis)]

Alexandros Frantzis (@afrantzis) commented about dlls/winewayland.drv/wayland_pointer.c:
> +            goto unlock;
> +        if (!(surface = data->wayland_surface))
> +        {
> +            wayland_win_data_release(data);
> +            goto unlock;
> +        }
> +        wayland_surface_coords_from_window(surface,
> +                cursor_pos.x - surface->window.rect.left,
> +                cursor_pos.y - surface->window.rect.top,
> +                &warp_x, &warp_y);
> +        wayland_win_data_release(data);
> +        zwp_locked_pointer_v1_set_cursor_position_hint(
> +                pointer->zwp_locked_pointer_v1,
> +                wl_fixed_from_int(warp_x),
> +                wl_fixed_from_int(warp_y));
> +        wl_surface_commit(wl_surface);
> Complex surface state changes can be performed by arbitrary threads (notably GL/Vulkan threads), and we want to ensure that such changes are only committed "atomically" when complete. To ensure this we perform such changes and commits only while holding the win_data for the corresponding window. In this case it's good enough to just guard the commit itself, since the cursor position hint itself doesn't interact with other state. Going back to the previous point, we would prefer not to acquire the win_data lock while holding the pointer lock, so this needs to be done outside the `wayland_pointer_update_constraint` function.

Actually, I noticed that we also do a `set_region` (which is also double-buffered state) in this function without any corresponding commit. I guess this tends to work because we eventually perform some commit for other reasons (e.g., redraws), but in any case it's better to have an explicit commit.

Again, we can ignore the case of `wayland_pointer_clear_constraint` since this is only called on surface destruction.

-- 
https://gitlab.winehq.org/wine/wine/-/merge_requests/7352#note_95002