Page MenuHome

Fix T70765: Support WM_POINTER and Wintab coalesced tablet events
AcceptedPublic

Authored by Nicholas Rishel (nicholas_rishel) on Jan 26 2020, 10:21 PM.

Details

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 6438
Build 6438: arc lint + arc unit

Event Timeline

Nicholas Rishel (nicholas_rishel) retitled this revision from Added GetPointer*InfoHistory functions, and a minor cleanup renaming penInfo to touchInfo. to Partial Fix T70765: Support WM_POINTER coalesced tablet events.Jan 26 2020, 10:32 PM
Nicholas Rishel (nicholas_rishel) edited the summary of this revision. (Show Details)

I'm unable to judge the interplay between this and rB2e9d5ba2115ceabdc0e8cb2eaa148c81d7b04667 brecht be a better judge here.

intern/ghost/intern/GHOST_SystemWin32.cpp
969

wouldn't we just end up with the latest data here? assuming there was a sequence of contact down, move, up, pause, down, move events in the queue?

972

The events come with a time stamp field, can we correlate it to the timestamps ghost uses? if there is a large queue of events they will all carry more or less the same timestamp due to small loop the ghost events are generated in and additional precision we had in the source data has gone lost.

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?

intern/ghost/intern/GHOST_SystemWin32.cpp
965

This seems like it could send multiple button down/up events even when there is only one?

I'd expect it to generate multiple cursor move events and one down/up event?

988–997

I think the tablet data in the event and the tablet data in the window should match. It's not clear to me why for one it takes into account isInContact and the other not?

intern/ghost/intern/GHOST_WindowWin32.cpp
1008

Use std::vector, it's simpler than std::unique_ptr here.

intern/ghost/intern/GHOST_WindowWin32.h
42

Don't use std::unique_ptr. It may be a good idea to start using it at some point, but when we do it will be for the whole file/module. Otherwise these multiple types of memory management are confusing.

Brecht Van Lommel (brecht) requested changes to this revision.Jan 27 2020, 3:23 PM
This revision now requires changes to proceed.Jan 27 2020, 3:23 PM
intern/ghost/intern/GHOST_WindowWin32.cpp
1077

not your bug, but int / int = int, so this will be -1 , 0 or 1

1081

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.
intern/ghost/intern/GHOST_WindowWin32.cpp
1076–1077

penMask

1080–1081

penFlags

Nicholas Rishel (nicholas_rishel) marked 3 inline comments as done.
  • Fix tilt mask check against incorrect field.

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
965

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.

972

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?

988–997

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
1008

Change pushed.

1077

Pushed fix

1081

Ditto above

intern/ghost/intern/GHOST_WindowWin32.h
42

Change pushed.

Nicholas Rishel (nicholas_rishel) marked an inline comment as done.
  • Remove redundant passing of vector length (vestigial from previously using arrays).
Brecht Van Lommel (brecht) requested changes to this revision.Feb 11 2020, 2:30 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
956

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...)

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.

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
988–997

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.
  • 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
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.Sun, Mar 15, 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.Sun, Mar 15, 7:09 AM

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.

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?

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.

This revision is now accepted and ready to land.Fri, Mar 27, 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.

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".