Page MenuHome

Wintab rewrite - high frequency events
ClosedPublic

Authored by Nicholas Rishel (nicholas_rishel) on May 26 2020, 7:57 AM.

Details

Summary

Previously Wintab was handled by saving the most recent tablet pressure and tilt information and deferred appending tablet infromation to Windows mouse events. This caused synchronization issues evident at the beginning and ending of strokes where pressure and tilt were either ahead or behind in time from mouse button up or down events. This also dicarded swaths of data which resulted in blockly grease pencil lines most apparent when a context switch resulted in several coalesced mouse events.

This patch changes the behavior of Wintab to instead rely entirely on Wintab information for pressure, tilt, position, and button input.

Wintab has several design decisions and conventions which complicate relying soley on it's input while retaining current workflows reliant on non-API behavior. For example, many device optionally allow the user to map barrel buttons to non-mouse actions. These mappings may or may not modify the intended behavior when touching the stylus down, such as scroll vs alt mappings. This behavior is not exposed in the Wintab API, but Wintab will continue updating button state sans this necessary context.

To work around the problem, this refactor synchronizations tablet input to Windows mouse down and up events, this captures events which should result in pen input while allowing events such as pen scrolling. Until a Windows mouse down event fires Wintab input is left unprocessed; when a Windows up event occurs Wintab is processed until a corresponding button up event is found.

Wintab allows for either button state or changes to be reported, but not both. An earlier refactor tried to use button changes to let state to be managed by Wintab. This was replaced when it was found that button change events were unreliable at corner cases such as switching windows. It was also found that with Wacom drivers Wintab peek functions would modify events in the queue causing errant and loss of button events.

This patch opts to read all Wintab events into a queue as they arrive, removing events as they become stale. This was chosen over using Wintab peek functions due to the aforementioned issue. As a bonus this seems to work better as it prevents the queue in Wintab from filling, thus neither a flood of events need to be handled when Wintab processing begins and a Wintab implementation need not be trusted to overwrite old events in it's queue.

Diff Detail

Repository
rB Blender

Event Timeline

This revision is a continuation of D6675 which had it's changes to Wintab rolled back.

The blocking issues noted in the last revision have either been reported resolved in patched builds (T75566, T75723) or have not replied to follow up questions but describe a problem likely a duplicate of confirmed resolved reports (T75583, T75566 similar and confirmed resolved).

T75623 will likely return if this lands, but the reporter has not provided necessary feedback at this time and is beyond the 1 week reply window.

I have a lot of added logging information specific to Wintab that should probably be included in this patch. Right now it's a bunch of printfs but I assume there's a better way to add it as a compile option?

I started a thread on BA to get feedback to rule out low hanging bugs if any remain. I'm opening the issue now to get the ball rolling but we should probably wait at least a week for feedback in that thread, given that these changes significantly change how Wintab is handled and that the API is fragile and often buggy.

Nicholas Rishel (nicholas_rishel) retitled this revision from Return of the Wintab refactor. to Wintab refactor - high frequency events.Jun 10 2020, 12:24 AM
  • Finish the comment's thought explaining pessimistic button ups events for
  • Add Wintab debug logging as a CMake option WITH_WINTAB_DEBUG.

Will the debugging info be something we'd need on a regular basis from end users? if so it may be better to add a startup parameter to blender, if it's more 'it's handy to have now to diagnose things, but once the dust settles i don't see anyone using it 6 weeks from now` it may be better to keep it a build option and remove it before landing.

As for this diff, It seems to work great with my xp pen, regardless of mode (ink vs wintab) but i'm by no means a tablet user, (ie open greasepencil mode, draw some circles, yup that works!)

So it's probably best to have the people that reported issues previously that made us revert the last attempt have a go at this (did we keep a list of tickets there?)

I put up a test build based on todays master with debug logging enabled over here : https://blender.community/c/graphicall/knbbbc/

Will the debugging info be something we'd need on a regular basis from end users? if so it may be better to add a startup parameter to blender, if it's more 'it's handy to have now to diagnose things, but once the dust settles i don't see anyone using it 6 weeks from now` it may be better to keep it a build option and remove it before landing.

Most Wintab bugtracker reports go into a limbo state because developers can not reproduce without hardware. Logging is the only way to effectively triage to at least. I felt a compile option was a happy medium, that it occupies a similar niche of rare but irreplaceable as debugging Ghost events which is why I used Ghost's debug compile switch as a template.

I strongly think it should stay because it's a foothold to begin debugging Wintab reports which otherwise become permanent "known issues". My gut reaction is that it's best left as a compile option, could you expand on why a runtime option might be better?

So it's probably best to have the people that reported issues previously that made us revert the last attempt have a go at this (did we keep a list of tickets there?)

Afaik I followed up on every Wintab report (even preexisting bugs) in the bugtracker. See my second comment in this issue for a quick recap.

I strongly think it should stay because it's a foothold to begin debugging Wintab reports which otherwise become permanent "known issues". My gut reaction is that it's best left as a compile option, could you expand on why a runtime option might be better?

Target audience! People that would report issues with their tablet are likely going to be the artist type, and those are generally not the "i can build blender from source" kind of people.

  • Finish the comment's thought explaining pessimistic button ups events for
  • Add Wintab debug logging as a CMake option WITH_WINTAB_DEBUG.
  • Remove Wintab logging.
  • Update tooltip, Tablet API can now be changed at runtime.

Rebase on master.

Nicholas Rishel (nicholas_rishel) retitled this revision from Wintab refactor - high frequency events to Wintab rewrite - high frequency events.Oct 22 2020, 12:30 AM
Ray molenkamp (LazyDodo) requested changes to this revision.Oct 22 2020, 2:36 PM

Taking another pass at this, there is quite a mix of comment styles, sometimes even in the same function, best to follow the style guide on this.

@Brecht Van Lommel (brecht) do you want to take a last pass at this before landing?

This revision now requires changes to proceed.Oct 22 2020, 2:36 PM
  • Cleanup: Use C style comments, punctuation, and capitalization.
  • Cleanup: rename functions for consistency with other process*Events functions.
  • Cleanup: comment style.
  • Commented assumption was incorrect, using tickCountToMillis causes underflow when compared to current time.
intern/ghost/intern/GHOST_WindowWin32.cpp
1397–1428 ↗(On Diff #30292)

This is a hack. Maybe it's unavoidable or the solution is too complex, but there should be a better comment explaining why there is not better solution or what the better solution would be.

From the comment I understand that the reason for dropping events is to avoid incorrect events from before the window lost and regained focus, but the condition for outdated events is based on time elapsed.

Really what should be happening is that events are queued in proper order. If an event occured before the window lost focus, it should be queued before the window lost focus event.

If these kinds of events are not coming in through the same event stream and there is no way to order them (based on timestamps or something), then this may be needed, but it's really fragile.

intern/ghost/intern/GHOST_WindowWin32.cpp
1397–1428 ↗(On Diff #30292)

How is the following?

Wintab (per documentation but may vary with implementation) does not update while it's event buffer is filled. This becomes an issue because we need some synchronization point between Wintab events and Win32 events, so we can't drain and process the queue immediately. We need to associate Wintab mouse events to Win32 mouse events because Wintab buttons very modal (a left click is not always a left click) and there's no way to reconstruct their mode from the Wintab API alone. There is no guaranteed ordering between Wintab and Win32 mouse events, so we synchronize on mouse button events. Win32 Mouse button down begins Wintab cursor tracking, all Wintab (mouse associated) buttons up ends it.
  • Commit history cleanup, build fix.
  • More history cleanup, build fix.
  • Add explanation for dequing Wintab PACKETs during WT_PACKET event into a local queue to consume during WM_* mouse events.

I have not reviewed the code in detail, but if you handle bug reports this should be fine.

This revision was not accepted when it landed; it landed in state Needs Review.Sat, Oct 31, 12:36 AM
This revision was automatically updated to reflect the committed changes.