Core: Explicitly throttle mousemove events #110148

Joseph Eagar · 2023-07-15T22:48:14+02:00

Joseph Eagar commented

2023-07-15 22:48:14 +02:00

This PR changes the behavior of INBETWEEN_MOUSEMOVE to operate
as a simple limit on the number of MOUSEMOVE events per second.

The current behavior of INBETWEEN_MOUSEMOVE is indeterminate.
Apparently the idea was to drop MOUSEMOVE events for some
tools during heavy CPU load; this was implemented by
detecting runs of MOUSEMOVE events inside the
event queue and changing their type to INBETWEEN_MOUSEMOVE
(while leaving the event at the head of the run as MOUSEMOVE).

Basically the size of the event queue was used to throttle
MOUSEMOVE events, presumably under the assumption that
the size of the queue is proportional to CPU and GPU load.
There is however another determinant of the current event
queue size: input device drivers. Some tablet drivers will
push events in such a way that INBETWEEN_MOUSEMOVE
events are generated without any significant CPU load.

This caused #93796. While the fix for that bug was to not
ignore INBETWEEN_MOUSEMOVE events, other cases are not so clear.
For example anchored strokes in sculpt mode, along with the grab,
rotate, thumb, snake hook, elastic deform, boundary and pose
brushes are deliberately set to ignore INBETWEEN_MOUSEMOVE
events, and I think this makes sense. But this means the interactivity
of these tools will depend in part on the specific device and driver
setup of individual users, which is not good.

This PR changes the behavior of INBETWEEN_MOUSEMOVE to operate as a simple limit on the number of MOUSEMOVE events per second. The current behavior of INBETWEEN_MOUSEMOVE is indeterminate. Apparently the idea was to drop MOUSEMOVE events for some tools during heavy CPU load; this was implemented by detecting runs of MOUSEMOVE events inside the event queue and changing their type to INBETWEEN_MOUSEMOVE (while leaving the event at the head of the run as MOUSEMOVE). Basically the size of the event queue was used to throttle MOUSEMOVE events, presumably under the assumption that the size of the queue is proportional to CPU and GPU load. There is however another determinant of the current event queue size: input device drivers. Some tablet drivers will push events in such a way that INBETWEEN_MOUSEMOVE events are generated without any significant CPU load. This caused #93796. While the fix for that bug was to not ignore INBETWEEN_MOUSEMOVE events, other cases are not so clear. For example anchored strokes in sculpt mode, along with the grab, rotate, thumb, snake hook, elastic deform, boundary and pose brushes are deliberately set to ignore INBETWEEN_MOUSEMOVE events, and I think this makes sense. But this means the interactivity of these tools will depend in part on the specific device and driver setup of individual users, which is not good.

Joseph Eagar added 1 commit 2023-07-15 22:48:24 +02:00

4a6de9c13e Core: Explicitly throttle mousemove events

The behavior of INBETWEEN_MOUSEMOVE was indeterminate.
Apparently the idea was to drop mousemove events for some
tools during heavy CPU load; this was implemented by
changing the type of runs of MOUSEMOVE events inside the
event queue to INBETWEEN_MOUSEMOVE, with only the event
at the head of the queue MOUSEMOVE.

Basically the size of the event queue was used to throttle
MOUSEMOVE events, presumably under the assumption that
the size of the queue is proportional to CPU and GPU load.
This turned out to be incorrect.

This PR removes the old behavior and uses a simple time
limit instead for throttling.  There is a maximum number
of MOUSEMOVE events per second (MOUSEMOVES_PER_SECOND)
and events that fail this limit are retyped to
INBETWEEN_MOUSEMOVE.

Joseph Eagar requested review from Campbell Barton 2023-07-15 22:48:58 +02:00

Ray molenkamp requested review from Nicholas Rishel 2023-07-15 22:53:52 +02:00

Brecht Van Lommel commented

2023-07-18 14:41:38 +02:00

The reason behind INBETWEEN_MOUSEMOVE is that for most operators these do not matter. For example for UI button highlighting or transform tools, doing work for such events would only waste CPU cycles.

This pull request makes those operators do unnecessary work again, which I don't think is the right solution. This is especially a problem when e.g. transforming a heavy mesh that is already slow, this would make it slower still. Sculpt/paint on the other hand get some type of rate limiting from stroke spacing and so are not affected the same way.

I think the solution for this should be in the sculpt/paint operators that do handle INBETWEEN_MOUSEMOVE. From what I remember these don't redraw on such inbetween events, but they could be made to redraw every N ms.

The ideal timing value is not entirely clear, if redrawing is slow compared to the sculpt/paint operation, doing too many redraws can make things even slower.

The reason behind `INBETWEEN_MOUSEMOVE` is that for most operators these do not matter. For example for UI button highlighting or transform tools, doing work for such events would only waste CPU cycles. This pull request makes those operators do unnecessary work again, which I don't think is the right solution. This is especially a problem when e.g. transforming a heavy mesh that is already slow, this would make it slower still. Sculpt/paint on the other hand get some type of rate limiting from stroke spacing and so are not affected the same way. I think the solution for this should be in the sculpt/paint operators that do handle `INBETWEEN_MOUSEMOVE`. From what I remember these don't redraw on such inbetween events, but they could be made to redraw every N ms. The ideal timing value is not entirely clear, if redrawing is slow compared to the sculpt/paint operation, doing too many redraws can make things even slower.

Nicholas Rishel commented

2023-07-24 00:47:00 +02:00

I don't believe we want to pursue this approach. This will just introduce a new bug for users asking why they have periodic straight line jumps in otherwise smooth brush strokes.

In general I suspect our approach of queuing separate events is killing performance, as each and every INBETWEEN_MOUSEMOVE needs to push/pop the full operator call stack. A more mechanically sympathetic approach would be to expose an interface to drain the INBETWEEN_MOUSEMOVE queue and handle them all at once in a single operator call, i.e the way most OS handle mouse events. Bonus points for extending GHOST and WM to queue runs of mouse movements in a single function call.

I don't believe we want to pursue this approach. This will just introduce a new bug for users asking why they have periodic straight line jumps in otherwise smooth brush strokes. In general I suspect our approach of queuing separate events is killing performance, as each and every `INBETWEEN_MOUSEMOVE` needs to push/pop the full operator call stack. A more mechanically sympathetic approach would be to expose an interface to drain the `INBETWEEN_MOUSEMOVE` queue and handle them all at once in a single operator call, i.e the way most OS handle mouse events. Bonus points for extending `GHOST` and `WM` to queue runs of mouse movements in a single function call.

👍 1

Nicholas Rishel requested changes 2023-07-24 00:59:51 +02:00

Nicholas Rishel left a comment

Rejected for systemic concerns noted in PR comments, requesting changes to put a pin in that.

Campbell Barton requested changes 2024-01-09 03:10:02 +01:00

Campbell Barton left a comment

Rejecting as the current distinction between MOUSEMOVE & INBETWEEN_MOUSEMOVE works well enough and I'm not convinced removing cursor events is a good approach.

In general I'd rather give operators receive all the events and the operator code can choose to skip some.

Removing INBETWEEN_MOUSEMOVE will impact add-ons too, so this kind of a change could back-fire in ways we don't expect.

Rejecting as the current distinction between MOUSEMOVE & INBETWEEN_MOUSEMOVE works well enough and I'm not convinced removing cursor events is a good approach. In general I'd rather give operators receive all the events and the operator code can choose to skip some. Removing `INBETWEEN_MOUSEMOVE` will impact add-ons too, so this kind of a change could back-fire in ways we don't expect.

Brecht Van Lommel commented

2024-01-09 11:05:47 +01:00

Let's close this then.

Brecht Van Lommel closed this pull request

2024-01-09 11:05:49 +01: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

Core: Explicitly throttle mousemove events #110148

Pull request closed