Page MenuHome

Partial Fix T70765: Support WM_POINTER coalesced tablet events
Needs ReviewPublic

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

Details

Summary

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 6659
Build 6659: 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.Sun, Jan 26, 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
968

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?

971

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
964

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?

987–996

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
963

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.Mon, Jan 27, 3:23 PM
This revision now requires changes to proceed.Mon, Jan 27, 3:23 PM
intern/ghost/intern/GHOST_WindowWin32.cpp
1105

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

1109

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
1104–1105

penMask

1108–1109

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
964

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.

971

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?

987–996

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
963

Change pushed.

1105

Pushed fix

1109

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.Tue, Feb 11, 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
955

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.Tue, Feb 11, 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
987–996

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.