Details

Reviewers

Ray Molenkamp (LazyDodo)
Brecht Van Lommel (brecht)
Antonio Vazquez (antoniov)

Maniphest Tasks

T70765: Input Mouse or Tablet events are too sparse when input device moves very fast in Windows

Commits

rBea3e0b3e8cbc: Windows: support high resolution tablet pen events for Wintab
rBd571d615a59a: Windows: support high resolution tablet pen events for Windows Ink

Summary

Edit: This patch has been updated to handle both coalesced WM_POINTER and Wintab input.

This patch applies the following changes to support coalesced WM_POINTER tablet events.

Added GetPointer*InfoHistory functions, and a minor cleanup renaming penInfo to touchInfo.
Removed unused hasButtonMask member from GHOST_PointerInfoWin32.
Optionally initialize Ghost cursor and button events with tablet data, otherwise use GetTabletData to get default or current tablet data.
Added Windows Pointer pen event history so that coalesced pen events are retrieved. Handling for cursor movement, Ghost mouse move events' generation, and nulling of tablet date moved into the pointer input processing function.

This fix only applies to the WM_POINTER API and not Wintab, to fully resolve T70765 requires design decisions not managed in this commit.

At this time it may be impossible to test this on a device that has Wintab drivers because of issues D5641 tries to resolve. D5641 patches an issue where Wintab is always defaulted to if available regardless of user settings, but the fix as written is brittle to regressions and doesn't solve the systemic problem.

Diff Detail

Repository

rB Blender

Branch

wmpointer-coalesced (branched from master)

Build Status

Buildable 7424
Build 7424: arc lint + arc unit

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

This revision now requires changes to proceed.Jan 27 2020, 3:23 PM

Ray Molenkamp (LazyDodo) added inline comments.Jan 30 2020, 3:12 AM

intern/ghost/intern/GHOST_WindowWin32.cpp
1171	not your bug, but int / int = int, so this will be -1 , 0 or 1
1175	same as above.

Modified WM_POINTER to use the individual pointer event's times instead of
Use vector instead of unique_ptr<[]>.
Cleanup WM_POINTER event defines and add ENTER and LEAVE events.
Avoid checking for isInContact for WM_POINTER events and instead always
Fix loss of tilt precision due to int division.

Harbormaster completed remote builds in B6437: Diff 21277.Jan 30 2020, 3:42 AM

Ray Molenkamp (LazyDodo) added inline comments.Jan 30 2020, 4:03 AM

intern/ghost/intern/GHOST_WindowWin32.cpp
1170–1171	penMask
1174–1175	penFlags

Fix tilt mask check against incorrect field.

Harbormaster completed remote builds in B6438: Diff 21278.Jan 30 2020, 4:15 AM

In D6675#157568, @Brecht Van Lommel (brecht) wrote:

At this time it may be impossible to test this on a device that has Wintab drivers because of issues D5641 tries to resolve. D5641 patches an issue where Wintab is always defaulted to if available regardless of user settings, but the fix as written is brittle to regressions and doesn't solve the systemic problem.

Which systemic problem specifically are you referring to?

Switching tablet APIs doesn't teardown/setup wintab appropriately, hence why reordering initialization before tablet APIs are setup patches over the root issue.

intern/ghost/intern/GHOST_SystemWin32.cpp
1116	I'll modify this to only loop in the move case and update the patch. In practice this does not occur because only move events are coalesced, but the code as written doesn't communicate that and is an unnecessary red herring. Edit: just reread what you said and yes, you're understanding before is correct.
1123	I couldn't find an instance of other Ghost events not being generated with Ghost's getMilliseconds() function, and was a tad concerned that editors downstream may assume event's timestamps are monotonic. It's implied that pointer event's provided dwTime and PerformanceCount correlate to GetTickCount and QueryPerformanceCounter used in getMilliseconds() but ambiguity there from differences e.g. in sign for PerformanceCount and QueryPerformanceCounter added uncertainty. I went ahead an implemented this but maybe a good idea to run at least the former concern by Antonio?
1139–1148	This is a hack that I'll rethink. The explanation of why is outlined below in case you have quick thoughts on a better design. In this patch the current tablet data is used to differentiate cursor move events generated by the OS from events generated by a tablet via setCursorPosition. See lines 941-946 of this patch. In this patch the current tablet data returned by GetTabletData set to GHOST_kTabletModeNone is indicative of the cursor event not being from a WM_POINTER tablet event. The unintuitive reason this is only checked in move events is because move events generated after button up would again set the tablet data, leaving mouse events in an an unrecoverable state as tablet data is essentially never null. There's a couple refactors which I can imagine cleaning up this code a bit, but there's a fundamental tension that exists between how Wintab, MouseMove, and WM_POINTER interact that likely necessitates a larger cleanup.
intern/ghost/intern/GHOST_WindowWin32.cpp
1003	Change pushed.
1171	Pushed fix
1175	Ditto above
intern/ghost/intern/GHOST_WindowWin32.h
43	Change pushed.

Remove redundant passing of vector length (vestigial from previously using arrays).

Harbormaster completed remote builds in B6493: Diff 21404.Feb 4 2020, 10:16 PM

I tested this with a Wacom Intuos Pro on Windows 10 today, and this patch seems to break pressure sensitivity. It's working before the patch, but it's always full pressure.

Does it work for you? Which tablet are you using for testing? If you can't find the issue I can spend some time looking into it.

If we can get this working well enough we may just always enable Windows Ink on Windows 8+, and don't try to use Wintab unless it's explicitly set in the preferences. But that can be done once this is working well.

intern/ghost/intern/GHOST_SystemWin32.cpp
1104	Sent to a window when a pointer leaves detection range over the window (hover) or when a pointer moves outside the boundaries of the window. https://docs.microsoft.com/en-us/previous-versions/windows/desktop/inputmsg/wm-pointerleave Should we clear tablet data in response to this? We still want to be able to handle it even if the mouse is not within the window bounds. We may want to handle this though: When a window loses capture of a pointer and it receives the WM_POINTERCAPTURECHANGED notification, it typically will not receive any further notifications. For this reason, it is important that you not make any assumptions based on evenly paired WM_POINTERDOWN/WM_POINTERUP or WM_POINTERENTER/WM_POINTERLEAVE notifications. https://docs.microsoft.com/en-us/previous-versions/windows/desktop/inputmsg/wm-pointercapturechanged

This revision now requires changes to proceed.Feb 11 2020, 2:30 PM

I tested on an XP-Pen artist-pro 12, needed to switch in their driver control panel thingy between the two api's for it to work with windows ink, then fiddle some more in blender to get windows ink working (although do i think that D5641 may fix that issue, have not tested yet)

I had both pressure and tilt data, however currently tilt does not seem to be used and capped to 0..90 degrees rather than -90 .. 90 degrees (given nothing uses it, not a big deal but still...)

In D6675#161223, @Brecht Van Lommel (brecht) wrote:

I tested this with a Wacom Intuos Pro on Windows 10 today, and this patch seems to break pressure sensitivity. It's working before the patch, but it's always full pressure.

You're likely seeing is mouse emulated drawing. Wintab is always initialized if available, see D5641. When a process creates a Wintab context only Wintab events are generated, even if Windows Ink is enabled in the tablet's preferences. If you configure Ghost to use the "Windows Ink" Tablet API subsequent Wintab events will be ignored, but normal mousemove events will still happen, hence no pressure while drawing.

You can check this (outdated) build which fully disabled Wintab to verify my assumptions above:
https://blender.community/c/graphicall/qjbbbc/

Note the following devices were verified to work with pressure when using the Pointer API alone:

Windows Surface (all)
XP-Pen Artist 15.6
Lenovo B590
Wacom Bamboo
Wacom MobileStudio Pro 16
Huion DWH69
Ugee
Huion Kamvas pro 22 2019

... at least a few of these required communicating to the user that they needed to enable Windows Ink in the tablet preferences utility.

In D6675#161223, @Brecht Van Lommel (brecht) wrote:

If we can get this working well enough we may just always enable Windows Ink on Windows 8+, and don't try to use Wintab unless it's explicitly set in the preferences. But that can be done once this is working well.

Related to D5641, Wintab isn't torn down/setup correctly when switching tablet APIs. I was uncertain whether it still makes sense to continue supporting Wintab on Windows 8 and newer but I'll go ahead and write the fix.

intern/ghost/intern/GHOST_SystemWin32.cpp
1139–1148	I updated the diff with something that I think reads more clearly, but is still relying on the window saved tablet data for to avoid generating cursor events while the tablet pointer events are being handled.

Modified WM_POINTER to use the individual pointer event's times instead of
Use vector instead of unique_ptr<[]>.
Cleanup WM_POINTER event defines and add ENTER and LEAVE events.
Avoid checking for isInContact for WM_POINTER events and instead always
Fix loss of tilt precision due to int division.
Fix tilt mask check against incorrect field.
Remove redundant passing of vector length (vestigial from previously using arrays).
Handle handoff between Wintab and Pointer APIs.
Formatting cleanup.

Harbormaster completed remote builds in B6659: Diff 21819.Feb 18 2020, 7:23 AM

Modified WM_POINTER to use the individual pointer event's times instead of
Use vector instead of unique_ptr<[]>.
Cleanup WM_POINTER event defines and add ENTER and LEAVE events.
Avoid checking for isInContact for WM_POINTER events and instead always
Fix loss of tilt precision due to int division.
Fix tilt mask check against incorrect field.
Remove redundant passing of vector length (vestigial from previously using arrays).
Handle handoff between Wintab and Pointer APIs.
Formatting cleanup.
CXO_CSRMESSAGES is not set for the wintab context options, thus
Overlap should also be disabled per Wintab's documentation and examples.
Swapping magic numbers for an enum and cleanup function name.
Cleanup: clarifying variable names.
Add support for coalesced Wintab events while respecting custom button
Restart Wintab to correct system coordinates when display size or layout
Comment cleanup.
Paranoia check to make consistent with Wintab event processing.
Remove unused variable.
Fix errant left clicks due to Wintab driver mouse emulation misbehavior.
Rename variable to clearify usage within Wintab.
Leave the Wintab context open while Blender is running.
Track the number of tablets connected through the Wintab API, and use that
Only process proximity event if it's in range, also remove unnecessary
Make interal window functiona for Wintab mouse translation private.
Documenting Wintab and Win32 Pointer functions, and whitespace formatting.
Whether a tablet is in range is specific to the window, store it there
Create a single buffer to use across mutliple reads from Wintab.
Type not used because there's only one type of cursor event, removing.
The default system context returned by WTInfo(WTI_DEFSYSCTX...) is required
Collapse Wintab enable/disable functions into one, and rename to more
Make sure array accesses are in-bounds when mapping buttons from wintab to
TabletX and TabletY were unused (info(..) only retrieves information).
Only return failure from getting Wintab packets if there was an error in
Only initialize Wintab if all necessary functions were retrieved.
Remove unnecessary assignment, duplicate variable shadowing, and check for
Handle documented idiosyncrasy in Wintab where the queue is deleted prior
Modified Wintab to better support corner cases around window change events

Harbormaster completed remote builds in B7164: Diff 22806.Mar 15 2020, 4:50 AM

Nicholas Rishel (nicholas_rishel) retitled this revision from Partial Fix T70765: Support WM_POINTER coalesced tablet events to Fix T70765: Support WM_POINTER coalesced tablet events.Mar 15 2020, 6:16 AM

Nicholas Rishel (nicholas_rishel) edited the summary of this revision. (Show Details)

@Brecht Van Lommel (brecht) the startup fix from D5641 works after this series of patches, specifically because setTabetAPI was updated to handle switching between Wintab and WM_POINTER events. Would it be more convenient for testing if I rolled that commit on top of this?

@Antonio Vazquez (antoniov) You'll probably want to test this patch.

Nicholas Rishel (nicholas_rishel) retitled this revision from Fix T70765: Support WM_POINTER coalesced tablet events to Fix T70765: Support WM_POINTER and Wintab coalesced tablet events.Mar 15 2020, 7:09 AM

I haven't hardware to test, but I asked to @Daniel Martinez Lara (pepeland) and @Matias Mendiola (mendio) to test the patch.

Code seems fair enough, however, I get no down events until i go in to the prefs menu and fiddle with the Tablet API drop down

Remove Visual Studio warning of uninitialized variables.
Comment cleanup.

Harbormaster completed remote builds in B7333: Diff 23177.Mar 27 2020, 9:29 AM

The code looks fine to me, and I couldn't find bugs testing with a Wacom Intuos Pro.

@Ray Molenkamp (LazyDodo), what does "no down events" mean exactly?

In D6675#173926, @Brecht Van Lommel (brecht) wrote:

The code looks fine to me, and I couldn't find bugs testing with a Wacom Intuos Pro.

@Ray Molenkamp (LazyDodo), what does "no down events" mean exactly?

~~mouse~~ cursor follows the pen, but you can't draw or use any of the UI, but that may have been fixed in the latest diff have not had the time to test yet.

Remove unused GHOST_GetTabletData API function, was not working after this
Be more explicit about which events include tablet data or not
Fix warnings about initialization order
Don't use m_ prefix for local variable

I'm ready to commit this on Monday, assuming issue found by @Ray Molenkamp (LazyDodo) is or will be solved.

Harbormaster completed remote builds in B7351: Diff 23217.Mar 27 2020, 10:49 PM

Brecht Van Lommel (brecht) accepted this revision.Mar 27 2020, 10:50 PM

This revision is now accepted and ready to land.Mar 27 2020, 10:50 PM

Something is still screwy with the drop down, when i save my startup file with either wintab or pointer api window->useTabletAPI will still somehow think it's on automatic. and when it's in that mode, i can only draw when the blender window does not have the focus.

In D6675#174008, @Ray Molenkamp (LazyDodo) wrote:

Something is still screwy with the drop down, when i save my startup file with either wintab or pointer api window->useTabletAPI will still somehow think it's on automatic. and when it's in that mode, i can only draw when the blender window does not have the focus.

That sounds like the issue solved by D5641, but I'm not sure how the behavior you're describing would occur. What you're describing though sounds like Wintab is being enabled when the tablet API is set to to "Windows Ink".

looks like xp-pen's wintab implementation is either broken, or i'm not understanding the wintab docs, windows ink works great though with this tablet, I don't want to hold up this patch any further than i need to.

for this to land D5641 needs to land first otherwise I can not use windows ink by default in my prefs.

beyond that i'm unsure how many other wintab implementations are broken so landing this may cause a tiny bit of a flood on the tracker (we were not using relative buttons previously), is it possible to split it up into 2 patches so we could revert the wintab stuff if it were to cause too much of a disruption?

intern/ghost/intern/GHOST_WindowWin32.cpp
1449	spend some time debugging with @Nicholas Rishel (nicholas_rishel), even when properly configured for relative buttons, xp-pen never seems to fill the hiword for me.

In D6675#174581, @Ray Molenkamp (LazyDodo) wrote:

beyond that i'm unsure how many other wintab implementations are broken so landing this may cause a tiny bit of a flood on the tracker (we were not using relative buttons previously), is it possible to split it up into 2 patches so we could revert the wintab stuff if it were to cause too much of a disruption?

If the Windows Ink commit can land it shouldn't be hard to separate (a lot easier than running both in parallel). Is there an easy way to get Brecht's commits?

I already had the Windows Ink and Wintab changes in separate commits here locally (attached in P1318).

Perhaps we could make Windows Ink the default, and only use Wintab when either manually chosen by the user in the preferences, or when using an older Windows version that does not have the Windows Ink API functions? I'm not sure there are many tablets still that only work with Wintab?

What I didn't understand is if @Ray Molenkamp (LazyDodo)'s tablet stopped working due to coalesced tablet event support, or if it was already broken before those changes? If it did break because of the new changes, we could also leave out that part entirely. It probably wouldn't benefit many users and Wintab could be considered something that we phase and drop entirely when there is no more use for it.

We're checking fields we were not checking before, i left a comment last night in the trouble spot (look for pkt.pkButtons )

Ok, so it's a new issue with this patch. Since Windows Ink does not work on Windows 7 and XP-Pen tablets are relatively common, I don't think the Wintab part of this can be committed as long as we don't have a solution for that.

I think the following needs to be done to get this ready for 2.83:

Improve D5641 so it actually sets the tablet API in time and works for multiple windows.
Change tablet preference Auto behavior to use Windows Ink if the API functions are available.
Commit along with the Windows Ink coalesced tablet event support.

Then if we can find a workaround for the Wintab issue, we can commit that as well.

In D6675#174963, @Brecht Van Lommel (brecht) wrote:

I think the following needs to be done to get this ready for 2.83:

Improve D5641 so it actually sets the tablet API in time and works for multiple windows.

The issues remaining with D5641 should be resolved in this patch unless you want to ensure that the system never has the wrong tablet API set, i.e. that it's constructed with the correct API which is read from preferences ahead of the Ghost System initialization.

This patch allows the tablet API to change during runtime, so when the system is late-updated we get the correct API for the first window having had the incorrect one for milliseconds. Correcting initialization is of dubious value if we intend to slate Wintab for eventual removal as you suggested.

In D6675#174963, @Brecht Van Lommel (brecht) wrote:

Change tablet preference Auto behavior to use Windows Ink if the API functions are available.

It should be noted that's equivalent to defaulting to Windows Ink in Windows 8+, as the API functions are always available on those platforms. You should expect an influx of bugtracker issues because Windows Ink support is not always by default enabled in tablet preferences utilities. When I asked for user feedback on a build with Wintab disabled I had to instruct at a few individuals to enable Windows Ink in their tablet preferences utility so in practice it is an issue.

There may be a way to dynamically test for Windows Ink support but I'll have to get back on that.

Modified WM_POINTER to use the individual pointer event's times instead of
Use vector instead of unique_ptr<[]>.
Cleanup WM_POINTER event defines and add ENTER and LEAVE events.
Avoid checking for isInContact for WM_POINTER events and instead always
Fix loss of tilt precision due to int division.
Fix tilt mask check against incorrect field.
Remove redundant passing of vector length (vestigial from previously using arrays).
Handle handoff between Wintab and Pointer APIs.
Formatting cleanup.
CXO_CSRMESSAGES is not set for the wintab context options, thus
Overlap should also be disabled per Wintab's documentation and examples.
Swapping magic numbers for an enum and cleanup function name.
Cleanup: clarifying variable names.
Add support for coalesced Wintab events while respecting custom button
Restart Wintab to correct system coordinates when display size or layout
Comment cleanup.
Paranoia check to make consistent with Wintab event processing.
Remove unused variable.
Fix errant left clicks due to Wintab driver mouse emulation misbehavior.
Rename variable to clearify usage within Wintab.
Leave the Wintab context open while Blender is running.
Track the number of tablets connected through the Wintab API, and use that
Only process proximity event if it's in range, also remove unnecessary
Make interal window functiona for Wintab mouse translation private.
Documenting Wintab and Win32 Pointer functions, and whitespace formatting.
Whether a tablet is in range is specific to the window, store it there
Create a single buffer to use across mutliple reads from Wintab.
Type not used because there's only one type of cursor event, removing.
The default system context returned by WTInfo(WTI_DEFSYSCTX...) is required
Collapse Wintab enable/disable functions into one, and rename to more
Make sure array accesses are in-bounds when mapping buttons from wintab to
TabletX and TabletY were unused (info(..) only retrieves information).
Only return failure from getting Wintab packets if there was an error in
Only initialize Wintab if all necessary functions were retrieved.
Remove unnecessary assignment, duplicate variable shadowing, and check for
Handle documented idiosyncrasy in Wintab where the queue is deleted prior
Modified Wintab to better support corner cases around window change events
Remove Visual Studio warning of uninitialized variables.
Comment cleanup.
Cleanup: following camelCase naming convention.
Manually track button presses instead of relying on Wintab relative mode
Cleanup: remove GHOST API to query tablet state from Window

Harbormaster completed remote builds in B7410: Diff 23354.Apr 2 2020, 3:58 AM

Cleanup: remove GHOST API to query tablet state from Window

Harbormaster completed remote builds in B7411: Diff 23355.Apr 2 2020, 4:07 AM

Just uploaded a patch that changes button handling to use absolute (currently pressed or not pressed) instead of relative (pressed this packet or released this packet) which I believe should fix XP Pen's tablet handling.

@Brecht Van Lommel (brecht) I pulled your changes into my branch by arc patch and manually cherry picking. When I tried git apply via a .patch file it gave an error without much explanation. If there's a cleaner way to handle this let me know.

Also, after researching it there doesn't appear to be any way to determine if a Windows Ink device is attached until you receive e.g. move events from the device. When a Wintab context is enabled, at least for Wacom it stops sending WM_POINTER* messages. So for automatic tablet API you have to commit to one or the other. Essentially all tablets now support Windows Ink, but there's no way to check whether it's currently enabled without leaving Wintab disabled.

I was about to head out when this came by, i gave it a quick 5 minute shake, given D5641 is applied

Automatic: Uses wintab and works
wintab: Uses wintab and works
windows ink: Uses windows ink and works

Ray Molenkamp (LazyDodo) mentioned this in T53553: Lose wacom presure when use two windows..Apr 2 2020, 11:16 PM

Modified Wintab to remain enabled when the window is not in focus so long

Harbormaster completed remote builds in B7424: Diff 23382.Apr 3 2020, 3:08 AM

The most recent change might resolve what was originally being described in T53553.

For posterity I'm documenting a difference between Wintab and Win32 Pointers that I think is important. If there's a better place to document this let me know.

Wintab treats the tip as a button the same as other buttons the the pen barrel akin to the A key is similar to the Z key. Win32 Pointers treat buttons on the barrel as modifiers to the pen tip similar to how the Shift key modifies the A key.

For Windows Surface devices they go as far as preventing hover right clicking, whereas Wacom will respect the their user preferences as to whether right click starts on hover, but when using Win32 Pointers they don’t issue a left click equivalent as they would for Wintab.

I could recreate the Wintab behavior in Win32 Pointers by interpreting pressure > 0 as left click, but it would definitely be cutting against the grain, and I suspect bug prone.

I think this is ready to commit now, I plan to do that tomorrow.

lgtm as long as D5641 is coming along

Brecht Van Lommel (brecht) mentioned this in D5641: Init the tablet API after reading the user settings.Apr 8 2020, 12:27 PM

Closed by commit rBd571d615a59a: Windows: support high resolution tablet pen events for Windows Ink (authored by Nicholas Rishel (nicholas_rishel), committed by Brecht Van Lommel (brecht)). · Explain WhyApr 8 2020, 12:31 PM

This revision was automatically updated to reflect the committed changes.

Brecht Van Lommel (brecht) mentioned this in rBc43473e88415: Cleanup: remove GHOST API to query tablet state from Window.

Brecht Van Lommel (brecht) added a commit: rBd571d615a59a: Windows: support high resolution tablet pen events for Windows Ink.

Brecht Van Lommel (brecht) mentioned this in rB4e4bf241c8a0: Cleanup: add utility functions for milliseconds conversion.

Brecht Van Lommel (brecht) mentioned this in rB3d8c57f4daec: Cleanup: minor refactoring of pointer event handling.

Brecht Van Lommel (brecht) added a commit: rBea3e0b3e8cbc: Windows: support high resolution tablet pen events for Wintab.

Nicholas Rishel (nicholas_rishel) mentioned this in T70765: Input Mouse or Tablet events are too sparse when input device moves very fast in Windows.Apr 9 2020, 7:16 AM

n1729 (n1729) mentioned this in T75566: Wacom tablet's side switches don't work.Apr 10 2020, 2:46 AM