Fix #109968: Mouse warp relocates the cursor incorrectly on WIN32 #111020

Guillermo Venegas · 2023-08-10T19:51:05+02:00

Guillermo Venegas commented

2023-08-10 19:51:05 +02:00

Changes the behavior of the mouse warp when the user is performing a grab action.
Previously if the mouse where outside the area of grab the mouse will
be relocated within this bounds, but this relies that the mouse never leaves the app,
in rares cases (if the user moves the mouse extremely faster) the mouse can
leave the window and the blender can no longer warp the mouse.

Clipping the mouse to the target bounds allow the mouse to move
freely within this area, this also ensures the mouse will not be moved
beyond the bounds. To allow the movement if the cursor hits an edge,
the mouse will placed at the opposite edge to be able to continue this movement.

All movement is accumulated, and is applied to the initial position of the mouse
when the grab action were started.

Note:
ClipCursor(...) clips the cursor at the specified rectangle, the mouse cannot leave this rectangle
until the clipping is overridden, if Blender freezes while clipping, the user might feel the mouse
is trapped in this rectangle, but alt + tab or ctrl + alt + del should free the mouse.

For example: if the user sets a high subdivision level with a drag action, Blender will likely freeze with
the mouse clipped, alt + tab would free the mouse, but anytime soon the Blender unfreezes
and becomes active, and since the drag action is not stopped, is likely that the Blender freezes again
as soon the mouse enter the window (the drag action is not cancelled if Blender loses focus).

Changes the behavior of the mouse warp when the user is performing a grab action. Previously if the mouse where outside the area of grab the mouse will be relocated within this bounds, but this relies that the mouse never leaves the app, in rares cases (if the user moves the mouse extremely faster) the mouse can leave the window and the blender can no longer warp the mouse. Clipping the mouse to the target bounds allow the mouse to move freely within this area, this also ensures the mouse will not be moved beyond the bounds. To allow the movement if the cursor hits an edge, the mouse will placed at the opposite edge to be able to continue this movement. All movement is accumulated, and is applied to the initial position of the mouse when the grab action were started. Note: `ClipCursor(...)` clips the cursor at the specified rectangle, the mouse cannot leave this rectangle until the clipping is overridden, if Blender freezes while clipping, the user might feel the mouse is trapped in this rectangle, but `alt + tab` or `ctrl + alt + del` should free the mouse. For example: if the user sets a high subdivision level with a drag action, Blender will likely freeze with the mouse clipped, `alt + tab` would free the mouse, but anytime soon the Blender unfreezes and becomes active, and since the drag action is not stopped, is likely that the Blender freezes again as soon the mouse enter the window (the drag action is not cancelled if Blender loses focus).

Guillermo Venegas added 3 commits 2023-08-10 19:51:16 +02:00

ccd133d1c1 fix

3a4669ec8b make format

23f50b2098 typos

Guillermo Venegas added 3 commits 2023-08-12 04:15:14 +02:00

ff4f5122c6 Merge remote-tracking branch 'origin/main' into fix-mouse-warp

524d2c2ccb Merge remote-tracking branch 'origin/main' into fix-mouse-warp

6f518a408c changes

Harley Acheson commented

2023-08-13 19:14:45 +02:00

Hey, this seems to work and does not have the "mouse escaping" issue described here: #102346

It makes sense to use the Windows API call ClipCursor for this. I do struggle to understand some of the change though, which is probably just me, mostly about the naming and use of wait_to_position. Is there any way you can take a look at this with the idea of "what if someone dumb has to understand this?" Maybe variable name changes, comments, different ordering, or maybe a separation of the getCursorGrabModeIsWarp path (early exit if not)?

Hey, this seems to work and does not have the "mouse escaping" issue described here: #102346 It makes sense to use the Windows API call ClipCursor for this. I do struggle to understand some of the change though, _which is probably just me_, mostly about the naming and use of `wait_to_position`. Is there any way you can take a look at this with the idea of "what if someone dumb has to understand this?" Maybe variable name changes, comments, different ordering, or maybe a separation of the getCursorGrabModeIsWarp path (early exit if not)?

Harley Acheson referenced this pull request

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

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

Guillermo Venegas added 2 commits 2023-08-14 00:36:09 +02:00

6da452712d Merge remote-tracking branch 'origin/main' into fix-mouse-warp

e3ebe38e4e use the same terminology before pr

Guillermo Venegas added 1 commit 2023-08-14 00:40:04 +02:00

31a465ef2b use the const specifier

Guillermo Venegas commented

2023-08-14 00:52:07 +02:00

If the mouse hits an edge the mouse will be teleported at the opposite edge
Sometimes SetMousePosition don't work immediately, is used because that,
to wait until the mouse event comes with updated position

I updated the pr, to use terminology already used

If the mouse hits an edge the mouse will be teleported at the opposite edge Sometimes `SetMousePosition` don't work immediately, is used because that, to wait until the mouse event comes with updated position I updated the pr, to use terminology already used

Campbell Barton requested changes 2023-08-14 01:27:38 +02:00

Campbell Barton left a comment

Only some minor notes as I can't test this easily.

intern/ghost/intern/GHOST_SystemWin32.cc Outdated

						
				@ -1054,6 +1054,59 @@ void GHOST_SystemWin32::processPointerEvent(

				    }

				  }

				}

				BOOL ClipCursorAtPoint(const int32_t x, const int32_t y)

Campbell Barton commented

2023-08-14 01:24:52 +02:00

Should this be static ? (same for ClipCursorAtBounds).

Also, I'd rather use snake case for local functions, although the conventions here are mixed (even in this patch).

Should this be `static` ? (same for `ClipCursorAtBounds`). Also, I'd rather use snake case for local functions, although the conventions here are mixed (even in this patch).

guishe marked this conversation as resolved

intern/ghost/intern/GHOST_SystemWin32.cc Outdated

						
				@ -1057,0 +1098,4 @@

				  const BOOL result = ClipCursor(&rect);

				  if (warp_x) {

				    /* x axis clipping is `[left,ritgh)`*/

Campbell Barton commented

2023-08-14 01:25:16 +02:00

picky, spelling, bracket type mismatch.

Guillermo Venegas commented

2023-08-14 04:12:30 +02:00

I was using math notation but better:
The x-axis clipping is defined by 'left <= x < right'.

I was using math notation but better: `The x-axis clipping is defined by 'left <= x < right'.`

guishe marked this conversation as resolved

intern/ghost/intern/GHOST_SystemWin32.cc Outdated

						
				@ -1112,0 +1141,4 @@

				  static bool is_warping = false;

				  if (bounds_axis != GHOST_kAxisNone) {

				    if (is_warping) {

				      /* The cursor should be clipped at `{[x_prev, x_prev+1), [y_prev, y_prev+1)}`.

Campbell Barton commented

2023-08-14 01:26:25 +02:00

How certain is it that the cursor will be clipped? What happens if it's not?

picky, bracket type mismatch.

How certain is it that the cursor will be clipped? What happens if it's not? picky, bracket type mismatch.

Guillermo Venegas commented

2023-08-14 04:54:52 +02:00

is_warping=true; is set after clip_cursor_at_point(x_new, y_new);

with just one ClipCursor(...) the cursor will be clipped unless ClipCursor(null) is called or is override in other place o by other program (this last would be unwanted behavior), but in all cases this will be a race for clip the cursor (it would be something unexpected and unwanted to happen), this should be considerate?

ClipCursor(...) could fail, but I couldn't say how likely it is, would it be a concern?

clip_cursor_at_point(x_new, y_new); it can also be called here if necessary

`is_warping=true;` is set after `clip_cursor_at_point(x_new, y_new);` with just one `ClipCursor(...)` the cursor will be clipped unless `ClipCursor(null)` is called or is override in other place o by other program (this last would be unwanted behavior), but in all cases this will be a race for clip the cursor (it would be something unexpected and unwanted to happen), this should be considerate? ClipCursor(...) could fail, but I couldn't say how likely it is, would it be a concern? `clip_cursor_at_point(x_new, y_new);` it can also be called here if necessary

Guillermo Venegas commented

2023-08-14 19:20:30 +02:00

Added clip_cursor_at_point(x_prev, y_prev); here in case the program looses focus

Added `clip_cursor_at_point(x_prev, y_prev);` here in case the program looses focus

Campbell Barton commented

2023-08-15 02:14:49 +02:00

ClipCursor(...) could fail, but I couldn't say how likely it is, would it be a concern?

This is mostly a case of communicating to whoever needs to read or debug the code in the future, if it's not a concern, simply say so. For e.g.

/* The request to `ClipCursor` may be rejected by the WIN32 API, this should not be considered an error,
 * however in the case this happens the cursor may "escape" the window and GHOST is unable to prevent that.
 * In practice this does not seem to be a problem. */

Note that I accidentally edited your comment but since restored the original text.

> ClipCursor(...) could fail, but I couldn't say how likely it is, would it be a concern? This is mostly a case of communicating to whoever needs to read or debug the code in the future, if it's not a concern, simply say so. For e.g. ``` /* The request to `ClipCursor` may be rejected by the WIN32 API, this should not be considered an error, * however in the case this happens the cursor may "escape" the window and GHOST is unable to prevent that. * In practice this does not seem to be a problem. */ ``` ---- Note that I accidentally edited your comment but since restored the original text.

Guillermo Venegas added 1 commit 2023-08-14 04:10:08 +02:00

b0d8a8f29e style, static, typos and fixes

Guillermo Venegas commented

2023-08-14 05:11:20 +02:00

something to consider, operations like walk or fly that are not canceled when changing the active program with alt+tab generates unwanted behavior

something to consider, operations like `walk or fly` that are not canceled when changing the active program with `alt+tab` generates unwanted behavior <video src="/attachments/dd1c134a-3875-4f74-a918-965a94ec5231" title="2023-08-13 21-06-50.mp4" controls></video> <video src="/attachments/d53d2786-a127-4014-b229-e43b531958fd" title="2023-08-13 21-06-50.mp4" controls></video>

2023-08-13 21-06-50.mp4

5.8 MiB

2023-08-13 21-17-38.mp4

2.2 MiB

Guillermo Venegas added 2 commits 2023-08-14 19:17:31 +02:00

570e4da2f6 dont clip if blender not active

25a20d85b6 Merge remote-tracking branch 'origin/main' into fix-mouse-warp

Guillermo Venegas commented

2023-08-14 19:27:53 +02:00

In case that blender loses focus and if the mouse is moved over blender it will only update the mouse position as it is, and it will no longer clip the mouse, only when the focus is restored all the movement and the clipping will be restored.

In case that blender loses focus and if the mouse is moved over blender it will only update the mouse position as it is, and it will no longer clip the mouse, only when the focus is restored all the movement and the clipping will be restored. <video src="/attachments/c1868d08-e5a1-43c5-8cec-168601e7c800" title="2023-08-14 11-15-54.mp4" controls></video>

2023-08-14 11-15-54.mp4

3.1 MiB

Guillermo Venegas requested review from Campbell Barton 2023-08-14 19:28:42 +02:00

Campbell Barton requested changes 2023-08-15 02:08:35 +02:00

intern/ghost/intern/GHOST_SystemWin32.cc Outdated

						
				@ -1055,6 +1055,60 @@ void GHOST_SystemWin32::processPointerEvent(

				  }

				}

				static BOOL clip_cursor_at_point(const int32_t x, const int32_t y)

Campbell Barton commented

2023-08-15 02:03:36 +02:00

A short doc-string noting why it's needed to clip at a point would be good.

intern/ghost/intern/GHOST_SystemWin32.cc Outdated

						
				@ -1058,0 +1088,4 @@

				                                  int32_t &x,

				                                  int32_t &y)

				{

				  const bool warp_x = bounds_axis & GHOST_kAxisX;

Campbell Barton commented

2023-08-15 02:07:04 +02:00

Suggest assert that GHOST_ASSERT(warp_x || warp_y, "Expected warp on at least one axis");.

Suggest assert that `GHOST_ASSERT(warp_x || warp_y, "Expected warp on at least one axis");`.

intern/ghost/intern/GHOST_SystemWin32.cc Outdated

						
				@ -1058,0 +1091,4 @@

				  const bool warp_x = bounds_axis & GHOST_kAxisX;

				  const bool warp_y = bounds_axis & GHOST_kAxisY;

				  RECT rect;

Campbell Barton commented

2023-08-15 02:06:04 +02:00

The type of RECT members is LONG so LONG_MIN / LONG_MAX could be used? (realize in practice they're the same, more a matter of general correctness).

The type of `RECT` members is `LONG` so `LONG_MIN` / `LONG_MAX` could be used? (realize in practice they're the same, more a matter of general correctness).

Campbell Barton commented

2023-08-15 02:30:18 +02:00

Only review basics here, I'd prefer if @Harley go over the details.
In my experience changes to this area often cause problems which are difficult to predict, so it would be good to double check various cases to ensure this isn't a change that needs to be reverted later.

Some requests:

Document the expected outcome if Blender hangs while the cursor is grabbed & clipped.

This used to be supported on X11 but was eventually disabled as the clipping was so effective it meant users needed to reboot their system if the cursor was constrained to a window and blender hung (or hit a break-point during debugging).
Test this change different kinds of grabbing & previous bug reports.
- Painting, ref: #102792.
- Editing number buttons, ref: #103253
- Walk/fly mode, ref: #102346.
- Check a fast increase of subdivision-surface-levels doesn't lock up the system entirely.

Only review basics here, I'd prefer if @Harley go over the details. In my experience changes to this area often cause problems which are difficult to predict, so it would be good to double check various cases to ensure this isn't a change that needs to be reverted later. Some requests: - Document the expected outcome if Blender hangs while the cursor is grabbed & clipped. This used to be supported on X11 but was eventually disabled as the clipping was so effective it meant users needed to reboot their system if the cursor was constrained to a window and blender hung (or hit a break-point during debugging). - Test this change different kinds of grabbing & previous bug reports. - Painting, ref: #102792. - Editing number buttons, ref: #103253 - Walk/fly mode, ref: #102346. - Check a fast increase of subdivision-surface-levels doesn't lock up the system entirely.

Campbell Barton changed title from ~~Alternative Fix #109968~~ to Fix #109968: Mouse warp relocates the cursor incorrectly on WIN32

2023-08-15 03:29:18 +02:00

Guillermo Venegas commented

2023-08-15 20:58:58 +02:00

Notes:

I force the mouse to show its actual location
For Painting, ref: #102792. mouse can move up/down and the brush preview sculpt circle can show this movement

Tests:

x	Main	Patch
Painting, ref: #102792.	mouse stays at grab location after release (last 2s)
Number, ref: #103253	mouse stays at grab location after release (last 2s) (tested only quick changes)	Infinite drag works as expected
Walk/fly, ref: #102346.	Mouse leaves window
Subdiv	Freezes on subdiv level edit	Freezes on subdiv level edit and mouse cant leave the window until `alt+tab`

Notes: 1. I force the mouse to show its actual location 2. For **Painting, ref: #102792.** mouse can move up/down and the brush preview sculpt circle can show this movement Tests: |x| Main | Patch | | ----------- | ----------- | ----------- | |Painting, ref: #102792.| mouse stays at grab location after release (last 2s)<video src="/attachments/93005871-db20-41db-9bbe-a290f988192e" title="2023-08-15 12-52-47.mp4" controls></video>|<video src="/attachments/db04a294-af03-4ed6-abee-236628d3ba57" title="2023-08-15 12-55-31.mp4" controls></video>| |Number, ref: #103253|mouse stays at grab location after release (last 2s) (tested only quick changes)<video src="/attachments/722aefec-4a80-4ddd-8f48-d94d1b29217e" title="2023-08-15 10-33-17.mp4" controls></video>|Infinite drag works as expected<video src="/attachments/f82a3725-aca1-4364-b285-d3d9af2c6cda" title="2023-08-15 10-35-49.mp4" controls></video>| |Walk/fly, ref: #102346.|Mouse leaves window<video src="/attachments/34a67970-6609-49e2-9825-1fc43feba0db" title="2023-08-15 12-51-37.mp4" controls></video>|<video src="/attachments/3c4644d9-58ef-4c7c-baed-606414761b00" title="2023-08-15 12-44-46.mp4" controls></video>| |Subdiv|Freezes on subdiv level edit<video src="/attachments/e5e699c9-11ef-4d5b-970f-3ee3f362a618" title="2023-08-15 11-52-32.mp4" controls></video>|Freezes on subdiv level edit and mouse cant leave the window until `alt+tab`<video src="/attachments/e75e715a-d80b-470b-842e-b8c5f763de56" title="2023-08-15 12-13-44.mp4" controls></video>|

2023-08-15 12-51-37.mp4

3.0 MiB

2023-08-15 10-30-22.mp4

2.2 MiB

2023-08-15 12-31-07.mp4

3.3 MiB

2023-08-15 10-33-17.mp4

975 KiB

2023-08-15 11-52-32.mp4

802 KiB

2023-08-15 10-32-59.mp4

632 KiB

2023-08-15 12-13-44.mp4

1.7 MiB

2023-08-15 12-44-46.mp4

5.4 MiB

2023-08-15 10-35-49.mp4

1.4 MiB

2023-08-15 12-55-31.mp4

2.6 MiB

2023-08-15 12-52-47.mp4

2.2 MiB

Guillermo Venegas added 2 commits 2023-08-16 01:40:50 +02:00

069fd102ab Merge remote-tracking branch 'origin/main' into fix-mouse-warp

03859b4eb9 requested changes

Guillermo Venegas added 1 commit 2023-08-16 01:41:12 +02:00

60b286d916 Merge remote-tracking branch 'origin/main' into fix-mouse-warp

Guillermo Venegas added 1 commit 2023-08-16 01:45:53 +02:00

095a96df79 missing changes

Harley Acheson commented

2023-08-16 02:41:23 +02:00

@guishe

Don't think I am ignoring you. I will start looking at this in depth again very soon - just a busy day today.

In a nutshell, fixing this is important and will get attention. Using ClipCursor() sure looks like the right (and maybe only) way to do this properly. However, failures when using this API call can be quite catastrophic. If any code path fails to unset with ClipCursor(nullptr) the user could be left in a situation where they can't save or even use other applications until they figure out how to task Blender away.

The Windows API does have a call to enquire on whether there is currently clipping, GetClipCursor, but it is a pain to use. It doesn't return null or false if no clipping, but instead the extent of the desktop.

I'm thinking about adding some wrapper functions for ClipCursor to help ensure we never get out of sync. For example add an is_cursor_clipped to system, move your clip_cursor_at_point and clip_cursor_at_bounds there and another to reset. Does this sound helpful? Is this something you could do instead? Are there any other ways you can think to ensure we don't leave the user clipped somehow?

@guishe Don't think I am ignoring you. I will start looking at this in depth again very soon - just a busy day today. In a nutshell, fixing this is important and will get attention. Using ClipCursor() sure looks like the right (and maybe only) way to do this properly. However, failures when using this API call can be quite catastrophic. If any code path fails to unset with ClipCursor(nullptr) the user could be left in a situation where they can't save or even use other applications until they figure out how to task Blender away. The Windows API does have a call to enquire on whether there is currently clipping, GetClipCursor, but it is a pain to use. It doesn't return null or false if no clipping, but instead the extent of the desktop. I'm thinking about adding some wrapper functions for ClipCursor to help ensure we never get out of sync. For example add an `is_cursor_clipped` to system, move your clip_cursor_at_point and clip_cursor_at_bounds there and another to reset. Does this sound helpful? Is this something you could do instead? Are there any other ways you can think to ensure we don't leave the user clipped somehow?

Guillermo Venegas commented

2023-08-16 03:57:11 +02:00

@Harley I like the idea of the wrapper but don't know if we can really enforce that blender own the clip zone, at least on windows
If the clip rectangle changes must be like other program is racing to acquire too the clip zone or the api fails
Maybe instead of trying to make it work, cancel the drag action, because its the most likely that something out of our control is happening, and add a trace that something happened

@Harley I like the idea of the wrapper but don't know if we can really enforce that blender own the clip zone, at least on windows If the clip rectangle changes must be like other program is racing to acquire too the clip zone or the api fails Maybe instead of trying to make it work, cancel the drag action, because its the most likely that something out of our control is happening, and add a trace that something happened

Pratik Borhade referenced this pull request

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

Mouse cursor jumps to the center of 3D View (Redo Panel: Add Object Operators) #111327

Guillermo Venegas added 3 commits 2023-08-24 23:22:54 +02:00

555700a54b Merge remote-tracking branch 'origin/main' into fix-mouse-warp

2ec2ca9f52 Merge remote-tracking branch 'origin/main' into fix-mouse-warp

ce58ecf52a typos

This pull request has changes conflicting with the target branch.