Page MenuHome

GHOST: Add support for precision touchpad gestures on Windows 10
Needs ReviewPublic

Authored by pembem22 on May 8 2020, 9:51 AM.
Tags
None
Tokens
"Love" token, awarded by abdo25."Love" token, awarded by ejnaren."Like" token, awarded by gilberto_rodrigues."Burninate" token, awarded by wouterstomp."Like" token, awarded by Cyclicz."Like" token, awarded by amonpaike."Like" token, awarded by jenkm.

Details

Summary

Hello, this patch adds support for precision touchpad gestures on Windows 10 using Direct Manipulation API. Gestures work exactly like on macOS, with full support for pan/pinch and inertia. This works by creating a viewport with a fake scrollable which is reset after every gesture and converts any changes to the content's transform into GHOST trackpad events (as explained here). The code is taken from the Chromium project, so I'm not sure where to mention this.

Tested on Windows 10 and Windows 7.

Fixes T70754: Windows 10 Precision Touchpad multi-touch inputs not detected by Blender, T69264: Enable Touchpad Multitouch Gestures on Windows.

Demo:

Diff Detail

Repository
rB Blender

Event Timeline

pembem22 requested review of this revision.May 8 2020, 9:51 AM
pembem22 created this revision.
pembem22 planned changes to this revision.May 8 2020, 2:30 PM
pembem22 updated this revision to Diff 24606.May 11 2020, 11:57 AM
pembem22 edited the summary of this revision. (Show Details)

Added check for Windows version.

Instead of depreciating Windows 7, the solution is normally to dynamically load relevant functions and redefine relevant structs and defines. Could you elaborate on why this doesn't work if it's a problem?

Ray molenkamp (LazyDodo) requested changes to this revision.May 26 2020, 3:41 PM

The reason api level is set to 0x601 is that when you set it to a higher version, it allows you to use functions that do not exist in win7 causing startup issues on those platforms (ie export abc not found in somesystemlibrary.dll) the fact that you tested this on win7 and seemingly have no issues is great, but all other devs now need to triple check msdn when they use a function it's not one that'll cause issues rather than just getting a build error.

Given we're not quite ready yet to lift the minimum requirements _WIN32_WINNT needs to stay at 0x601 and you'll have to work around this in some other way.

This revision now requires changes to proceed.May 26 2020, 3:41 PM

adding the user interface guys, since they should have a look at this as well from the UI perspective.

pembem22 updated this revision to Diff 25113.May 26 2020, 4:24 PM
pembem22 edited the summary of this revision. (Show Details)

Don't change Windows API version.

Thanks for the explanation. I redefined all required structs for DM API.

Brecht Van Lommel (brecht) requested changes to this revision.May 26 2020, 4:59 PM

Could all this gesture code be moved into new GHOST_TrackpadWin32.h/cpp files?

Most of the code could be moved into a GHOST_TrackpadWin32 class, which would be either a new class or just GHOST_DirectManipulationViewportEventHandler renamed and extended.

intern/ghost/intern/GHOST_SystemWin32.cpp
508–511

What is this updating exactly?

It's not clear to me that it's correct to update things after multiple events have been handled, rather than after every event or specific types of events..

1799

Missing break;.

intern/ghost/intern/GHOST_WindowWin32.cpp
454

What is this magic 125 number? Can it be defined just once instead of repeated?

523

Is the concept of a gesture state needed, does that state need to be stored at all?

I would expect it to send both events if both zoom and pan changes.

intern/ghost/intern/GHOST_WindowWin32.h
659

cursor_x, cursor_y don't need to be class member, they can be local variables.

1005–1008

Any reason not to use std::unique_ptr?

This revision now requires changes to proceed.May 26 2020, 4:59 PM
intern/ghost/intern/GHOST_WindowWin32.h
1005–1008

They are COM objects, which need lifetime management though IUnknown, ComPtr will do this for you while unique_ptr won't, previously there was/is not a whole lot of COM usage in our codebase and we have used the raw interface pointers previously in the directX bindings for openXR.

I don't think we have any guidance here

intern/ghost/intern/GHOST_WindowWin32.h
1005–1008

Ok, it's fine to use it then.

Julian Eisel (Severin) added inline comments.
intern/ghost/intern/GHOST_WindowWin32.h
1005–1008

I wouldn't mind, even prefer using ComPtr XR or other Win32 code (XR code did use it at first). In general I prefer RAII memory management.

Probably the main reason why I removed them was the fact that we didn't use them elsewhere, which made me insecure about using them. It seemed like on purpose.

intern/ghost/intern/GHOST_WindowWin32.h
1005–1008

if we're ok with using smart pointers, we should probably also update the dx bindings, i think i made @Julian Eisel (Severin) take them out previously, the explanation above was to explain why unique_ptr would not work, it's not an endorsement or rejection of the use of ComPtr

intern/ghost/intern/GHOST_WindowWin32.h
43

Please move this after pktdef.h with a separating newline, wintab.h through pktdef.h are a logical block of related includes/defines.

pembem22 added inline comments.May 27 2020, 10:10 AM
intern/ghost/intern/GHOST_SystemWin32.cpp
508–511

This updates viewport through IDirectManipulationUpdateManager because it's configured in DIRECTMANIPULATION_VIEWPORT_OPTIONS_MANUALUPDATE mode. After that OnContentUpdated is called and trackpad events are added to the queue if needed. I'm not sure where this code should be.

I also tried other update modes. It works without manual updating, but there is no inertia for some reason.

intern/ghost/intern/GHOST_WindowWin32.cpp
523

Viewport doesn't change scale until DM recognizes pinch gesture. When it does, pan updates are incorrect (fast diagonal movement).

intern/ghost/intern/GHOST_WindowWin32.h
659

cursor_x and cursor_y are updated only when the gesture starts and are used until the next gesture. Otherwise inertia animation will update UI under the cursor, which is incorrect if cursor changed position.

pembem22 updated this revision to Diff 25381.EditedJun 2 2020, 11:47 AM
  • Move DM code to new files.
  • Fix viewport not resetting when user moves cursor by increasing viewport size.
  • Fix stuck pinch gesture by increasing epsilon.
  • Adress review comments.
pembem22 marked 5 inline comments as done.Jun 2 2020, 11:49 AM

Unfortunately Chrome's use of DirectManipulation (by extension this adaptation) is a hack to get around Precision Touchpad's lack of Pointer API support. DirectManipulation's intended use is to tie user input into DirectComposition, so compositor scaling and translating. This is why multitouch touchpad interactions like rotation are notably absent.

The InteractionContext API in Ninput.dll is what we want but can't have. InteractionContext takes Pointer data and interprets it as e.g. translate, rotate, and scale; but doesn't work here because Precision Touchpads only emit DM_POINTERHITTEST events. That said even if the Pointer API was supported by precision touchpads, it's not clear the InteractionContext API would work for them given it doesn't support "mouse in pointer" Pointer data.

This is probably the best we're getting until a hypothetical introduction of EnableTouchpadInPointer() equivalent to EnableMouseInPointer(). It appears that the plan was to use the Pointer API for Precision Touchpads given the PT_TOUCHPAD type and that the undocumented use of wparam in DM_POINTERHITTEST to get the pointer ID, but this area hasn't seen any changes in years and I see no indications of any imminent changes either.

intern/ghost/intern/GHOST_SystemWin32.cpp
508

Only the active window should be updated.

508–511

The update manager's intended use was to be passed to DirectComposition which drives the updates. This is as good a place as any to emulate it's behavior, afaik there's no way to get better granularity because the only relevant event from Precision Touchpad devices is DM_POINTERHITTEST which only triggers when the event starts.

intern/ghost/intern/GHOST_WindowWin32.cpp
523

@Brecht Van Lommel (brecht) the translation components of the transformation matrix become nonsensical when scaling occurs. DM's GetCenterPoint had the same problem. This issue is documented in Chrome's source.

pembem22 updated this revision to Diff 25709.Jun 9 2020, 8:30 PM

Update only the active window.

pembem22 marked an inline comment as done.Jun 9 2020, 8:31 PM