WIP: Fix #109968: Win32 Mouse Warp Bounds #110937

Harley Acheson · 2023-08-08T21:54:53+02:00

Harley Acheson commented

2023-08-08 21:54:53 +02:00

Use Cursor Grab bounds, rather than a portion of the window bounds, for
GHOST_kGrabHide

I was asked to look at this by @ideasman42 but this will probably require some consultation with him because this area of code has some odd history.

The minimum changes I can make to fix the reported behavior is just this:

diff --git a/intern/ghost/intern/GHOST_SystemWin32.cc b/intern/ghost/intern/GHOST_SystemWin32.cc
index 6eaf9cf9d03..3d21c92b242 100644
--- a/intern/ghost/intern/GHOST_SystemWin32.cc
+++ b/intern/ghost/intern/GHOST_SystemWin32.cc
@@ -1084,28 +1084,28 @@ GHOST_EventCursor *GHOST_SystemWin32::processCursorEvent(GHOST_WindowWin32 *wind
       GHOST_TAxisFlag bounds_axis = GHOST_kAxisNone;
 
       if (window->getCursorGrabMode() == GHOST_kGrabHide) {
         window->getClientBounds(bounds);
 
         /* WARNING(@ideasman42): The current warping logic fails to warp on every event,
          * so the box needs to small enough not to let the cursor escape the window but large
          * enough that the cursor isn't being warped every time.
          * If this was not the case it would be less trouble to simply warp the cursor to the
          * center of the screen on every motion, see: D16558 (alternative fix for #102346). */
-        const int32_t subregion_div = 4; /* One quarter of the region. */
-        const int32_t size[2] = {bounds.getWidth(), bounds.getHeight()};
-        const int32_t center[2] = {(bounds.m_l + bounds.m_r) / 2, (bounds.m_t + bounds.m_b) / 2};
+        //const int32_t subregion_div = 4; /* One quarter of the region. */
+        //const int32_t size[2] = {bounds.getWidth(), bounds.getHeight()};
+        //const int32_t center[2] = {(bounds.m_l + bounds.m_r) / 2, (bounds.m_t + bounds.m_b) / 2};
         /* Shrink the box to prevent the cursor escaping. */
-        bounds.m_l = center[0] - (size[0] / (subregion_div * 2));
-        bounds.m_r = center[0] + (size[0] / (subregion_div * 2));
-        bounds.m_t = center[1] - (size[1] / (subregion_div * 2));
-        bounds.m_b = center[1] + (size[1] / (subregion_div * 2));
+        //bounds.m_l = center[0] - (size[0] / (subregion_div * 2));
+        //bounds.m_r = center[0] + (size[0] / (subregion_div * 2));
+        //bounds.m_t = center[1] - (size[1] / (subregion_div * 2));
+        //bounds.m_b = center[1] + (size[1] / (subregion_div * 2));
         bounds_margin = 0;
         bounds_axis = GHOST_TAxisFlag(GHOST_kAxisX | GHOST_kAxisY);
       }
       else {
         /* Fallback to window bounds. */
         if (window->getCursorGrabBounds(bounds) == GHOST_kFailure) {
           window->getClientBounds(bounds);
         }
         bounds_margin = 2;
         bounds_axis = window->getCursorGrabAxis();

I can't find any reason why we want to wrap on a bounds smaller than the window (or grabBounds if set) when in GrabHide. So this PR just removes this in a nicer way than above.

Use Cursor Grab bounds, rather than a portion of the window bounds, for GHOST_kGrabHide --- I was asked to look at this by @ideasman42 but this will probably require some consultation with him because this area of code has some odd history. The minimum changes I can make to fix the reported behavior is just this: ``` diff --git a/intern/ghost/intern/GHOST_SystemWin32.cc b/intern/ghost/intern/GHOST_SystemWin32.cc index 6eaf9cf9d03..3d21c92b242 100644 --- a/intern/ghost/intern/GHOST_SystemWin32.cc +++ b/intern/ghost/intern/GHOST_SystemWin32.cc @@ -1084,28 +1084,28 @@ GHOST_EventCursor *GHOST_SystemWin32::processCursorEvent(GHOST_WindowWin32 *wind GHOST_TAxisFlag bounds_axis = GHOST_kAxisNone; if (window->getCursorGrabMode() == GHOST_kGrabHide) { window->getClientBounds(bounds); /* WARNING(@ideasman42): The current warping logic fails to warp on every event, * so the box needs to small enough not to let the cursor escape the window but large * enough that the cursor isn't being warped every time. * If this was not the case it would be less trouble to simply warp the cursor to the * center of the screen on every motion, see: D16558 (alternative fix for #102346). */ - const int32_t subregion_div = 4; /* One quarter of the region. */ - const int32_t size[2] = {bounds.getWidth(), bounds.getHeight()}; - const int32_t center[2] = {(bounds.m_l + bounds.m_r) / 2, (bounds.m_t + bounds.m_b) / 2}; + //const int32_t subregion_div = 4; /* One quarter of the region. */ + //const int32_t size[2] = {bounds.getWidth(), bounds.getHeight()}; + //const int32_t center[2] = {(bounds.m_l + bounds.m_r) / 2, (bounds.m_t + bounds.m_b) / 2}; /* Shrink the box to prevent the cursor escaping. */ - bounds.m_l = center[0] - (size[0] / (subregion_div * 2)); - bounds.m_r = center[0] + (size[0] / (subregion_div * 2)); - bounds.m_t = center[1] - (size[1] / (subregion_div * 2)); - bounds.m_b = center[1] + (size[1] / (subregion_div * 2)); + //bounds.m_l = center[0] - (size[0] / (subregion_div * 2)); + //bounds.m_r = center[0] + (size[0] / (subregion_div * 2)); + //bounds.m_t = center[1] - (size[1] / (subregion_div * 2)); + //bounds.m_b = center[1] + (size[1] / (subregion_div * 2)); bounds_margin = 0; bounds_axis = GHOST_TAxisFlag(GHOST_kAxisX | GHOST_kAxisY); } else { /* Fallback to window bounds. */ if (window->getCursorGrabBounds(bounds) == GHOST_kFailure) { window->getClientBounds(bounds); } bounds_margin = 2; bounds_axis = window->getCursorGrabAxis(); ``` I can't find any reason why we want to wrap on a bounds smaller than the window (or grabBounds if set) when in GrabHide. So this PR just removes this in a nicer way than above.

Harley Acheson added 1 commit 2023-08-08 21:55:07 +02:00

af08b0e0af Fix #109968 : Win32 Mouse Warp Bounds

Use Cursor Grab bounds, rather than a portion of the window bounds, for
GHOST_kGrabHide

Harley Acheson commented

2023-08-08 21:56:24 +02:00

@PratikPB2123 - can you test?

Harley Acheson requested review from Campbell Barton 2023-08-08 21:56:43 +02:00

Campbell Barton commented

2023-08-09 02:08:26 +02:00

First of all, it would be good to find the root cause of #109968, if the cursor is hidden it should not matter where it's wrapped internally.

The intended behavior is:

The cursor is hidden.
It wraps around the center of the window to prevent "escape" (#102346)
The operation finishes and the cursor is made visible and it's location restored.

If these steps are not working properly, that seems like a bug in hiding/restoring the cursor, not in it's wrapping.

It would be good to attempt to resolve this problem at the level of hiding/restoring instead of changing internal wrapping logic.
If that's not possible a detailed explanation should be included for why a hidden cursor's location matters on WIN32 that is different from other platforms.

From looking at the video it seems the cursor is being made visible when it shouldn't be (although the screen-recording software could be adding in a software cursor). That doesn't account for the cursor being warped to a different location once color picking is complete.

The change to GHOST_Window.cc looks like it will impact other platforms, it may be OK but I couldn't say for sure without checking. For an isolated fix, I'd rather remove this change, perform the spesific check in GHOST/Win32 and consider a change that impacts all platforms separately (testing X11/Wayland/macOS to see it's not causing problems).

First of all, it would be good to find the root cause of #109968, if the cursor is hidden it should not matter where it's wrapped internally. The intended behavior is: - The cursor is hidden. - It wraps around the center of the window to prevent "escape" (#102346) - The operation finishes and the cursor is made visible and it's location restored. If these steps are not working properly, that seems like a bug in hiding/restoring the cursor, not in it's wrapping. It would be good to attempt to resolve this problem at the level of hiding/restoring instead of changing internal wrapping logic. If that's not possible a detailed explanation should be included for why a hidden cursor's location matters on WIN32 that is different from other platforms. From looking at the video it seems the cursor is being made visible when it shouldn't be _(although the screen-recording software could be adding in a software cursor)_. That doesn't account for the cursor being warped to a different location once color picking is complete. The change to `GHOST_Window.cc` looks like it will impact other platforms, it may be OK but I couldn't say for sure without checking. For an isolated fix, I'd rather remove this change, perform the spesific check in GHOST/Win32 and consider a change that impacts all platforms separately (testing X11/Wayland/macOS to see it's not causing problems).

Pratik Borhade commented

2023-08-09 12:51:18 +02:00

@Harley , this restores 3.3 behavior (last working version) i.e. both the issues mentioned in the #109968 do not appear anymore with PR applied

The cursor is hidden.

In some cases, cursor is visible (even in 3.3) during click-drag on redo panel properties, I'm not sure why.

From looking at the video it seems the cursor is being made visible when it shouldn't be

Actually, reporter is just clicking at random points (and not click-dragging 😅 )

@Harley , this restores 3.3 behavior (last working version) i.e. both the issues mentioned in the #109968 do not appear anymore with PR applied - - - > The cursor is hidden. In some cases, cursor is visible (even in 3.3) during click-drag on [redo panel properties](https://projects.blender.org/blender/blender/issues/109968#issuecomment-992576), I'm not sure why. > From looking at the video it seems the cursor is being made visible when it shouldn't be Actually, reporter is just clicking at random points (and not click-dragging 😅 )

Harley Acheson commented

2023-08-10 00:09:40 +02:00

It wraps around the center of the window to prevent "escape" (#102346)

With this patch applied I can still get the mouse to escaped as described in #102346. Frustratingly this happens even if I use a Windows API call (ClipCursor) to keep it within bounds.

From looking at the video it seems the cursor is being made visible when it shouldn't be (although the screen-recording software could be adding in a software cursor). That doesn't account for the cursor being warped to a different location once color picking is complete.

It isn't that the cursor is moved to a different location at the end of color picking. It is more that it unexpectedly moves there while doing multiple short and fast little drags within the area. That unexpected move is outside of the "mouse close" bounds and that ends the operation.

So far not having a lot of luck here.

> - It wraps around the center of the window to prevent "escape" (#102346) With this patch applied I can still get the mouse to escaped as described in #102346. Frustratingly this happens even if I use a Windows API call (ClipCursor) to keep it within bounds. > From looking at the video it seems the cursor is being made visible when it shouldn't be _(although the screen-recording software could be adding in a software cursor)_. That doesn't account for the cursor being warped to a different location once color picking is complete. It isn't that the cursor is moved to a different location at the end of color picking. It is more that it unexpectedly moves there while doing multiple short and fast little drags within the area. That unexpected move is outside of the "mouse close" bounds and that ends the operation. So far not having a lot of luck here.

Harley Acheson changed title from ~~Fix #109968: Win32 Mouse Warp Bounds~~ to WIP: Fix #109968: Win32 Mouse Warp Bounds

2023-08-10 00:10:00 +02:00

Harley Acheson commented

2023-08-13 19:17:41 +02:00

Alternate fix #111020 by @guishe does not have the bad "mouse escape" behavior from #102346.

Alternate fix #111020 by @guishe does **not** have the bad "mouse escape" behavior from #102346.

Harley Acheson commented

2023-10-09 21:13:47 +02:00

Closing in favor of #113066

Harley Acheson closed this pull request

2023-10-09 21:13:48 +02:00

Pull request closed

Please reopen this pull request to perform a merge.

Sign in to join this conversation.

No reviewers

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

WIP: Fix #109968: Win32 Mouse Warp Bounds #110937

Pull request closed