Details

Reviewers

Nicholas Rishel (nicholas_rishel)
Ray Molenkamp (LazyDodo)
Brecht Van Lommel (brecht)

Commits

rBe58b18888c0e: GHOST: Add support for precision touchpad gestures on Windows

Summary

Hello, this patch adds support for precision touchpad gestures on Windows 8.1 and newer 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.

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

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

@Nicholas Rishel (nicholas_rishel) could you please review the patch?

Harley Acheson (harley) awarded a token.Jan 9 2022, 2:14 AM

@Andrii Symkin (pembem22) Excited to get this into Blender. :)

There's one significant change I'd like to see, I don't think the trackpad should be holding onto a pointer to the window. I haven't fully considered what changes would be necessary, but at a high level I suspect that would mean returning event data to the GHOST_SystemWin32 to generate events instead of generating them directly in the trackpad.

intern/ghost/intern/GHOST_TrackpadWin32.cpp
23–80	I'd pull out the "could fail" dmanip setup code into a factory function and make this constructor private. This cleans up following code that could assume everything is initialized if the function can be called.
90	Example of a check that could be made obsolete with the above factory function suggestion.
137–141	I'd remove this, no need to repeat what should be in Window's documentation.
187–195	This state machine, and why it's necessary, should be commented. Your explanation in the prior inline comment should work here.
intern/ghost/intern/GHOST_TrackpadWin32.h
49–56	We don't use SAL annotations so `_In_` should be removed.
63–64	You can use `GHOST_SystemWin32 system = (GHOST_SystemWin32 )GHOST_System::getSystem();` in local variables to avoid saving the system here. I think m_window should also be removed, but I haven't fully thought through that yet. The latter will likely require some control flow changes.
79–81	Ditto above, plus you can get the `hwnd` from a `GHOST_WindowWin32` instances with `getHWND`.
82–86	@Ray Molenkamp (LazyDodo) @Harley Acheson (harley) I remember there was some talk about using `ComPtr` vs `unique_ptr` in these kind of cases. Do you recall what the decision was?

Merge branch 'master' into precision-touchpad
Remove SAL annotations
Don't store System and HWND in classes
Remove redundant comment
Document gesture state machine
Don't check for NULL when deleting
Use factory method to create DirectManipulationHelper
Declare variables where used, use static_cast, const
Silence unused variable warnings

Harbormaster completed remote builds in B19964: Diff 47024.Jan 13 2022, 3:01 PM

There's one significant change I'd like to see, I don't think the trackpad should be holding onto a pointer to the window. I haven't fully considered what changes would be necessary, but at a high level I suspect that would mean returning event data to the GHOST_SystemWin32 to generate events instead of generating them directly in the trackpad.

I'm not sure how this can be implemented. Right now, every DirectManipulationHelper uses its window pointer in the constructor and destructor and has its gesture state. Gesture state probably can be moved to GHOST_SystemWin32 because it should be impossible to perform multiple gestures simultaneously, but that doesn't seem like a good solution.

intern/ghost/intern/GHOST_TrackpadWin32.h
82–86	There's a discussion about this in inline comments.

@Nicholas Rishel (nicholas_rishel) - I remember there was some talk about using ComPtr vs unique_ptr in these kind of cases. Do you recall what the decision was?

I vaguely remember a conversation between @Ray Molenkamp (LazyDodo) and @Julian Eisel (Severin) regarding those but can't recall details.

In D7660#366788, @Harley Acheson (harley) wrote:

@Nicholas Rishel (nicholas_rishel) - I remember there was some talk about using ComPtr vs unique_ptr in these kind of cases. Do you recall what the decision was?

I vaguely remember a conversation between @Ray Molenkamp (LazyDodo) and @Julian Eisel (Severin) regarding those but can't recall details.

That conversation was actually in this ticket guys..... :)

I'm definitely in favor of using RAII types like ComPtr<> or unique_ptr<>. Was a bit unsure when I first considered using it in Ghost, but that was before we basically went: let's have "modern" C++ as much as possible.

Alright, after a bit of thought I'd approve this patch with the following changes. There might be a minor cleanup round afterward but I think all big questions are addressed after this.

Lot of inline comments following. At a high level the significant requests are as follows:

Remove dependencies for GHOST_System and GHOST_Window by...
Passing DPI and HWND to the create function.
Adding a GetTouchpadInfo function that returns TOUCHPAD_INFO {dx, dy, dscale}
Generating Touchpad events in GHOST_SystemWin32 with info from GetTouchpadInfo.
Adding a SetDpi function which is updated when WM_DPICHANGED event occurs.

Sorry that the HWND request is the opposite of what was last requested, I was hoping it might be entirely removable at the time but have since got a better handle on why that won't be possible.

intern/ghost/intern/GHOST_SystemWin32.cpp
430–432	I found that the direct manipulation handlers are called during `DispatchMessageW` above, so probably best to move this block before the `PeekMessageW` loop. `GetTouchpadInfo` discussed in another comment should occur after `PeekMessageW`. Here is where you'd call `system->pushEvent(new GHOST_EventTrackpad(...)`.
432	Check for null.
intern/ghost/intern/GHOST_TrackpadWin32.cpp
19–21	So after a bit more research into this, I think it's impossible to move the `HWND` from this class while keeping RAII cleanup, but we can remove `GHOST_WindowWin32` and `GHOST_SystemWin32`. I think there are a lot of maintainability benefits of removing the latter two as it makes pointer ownership much cleaner, and between the two `HWND` is the less risky thing to use after it's resource has been freed. Sorry for asking for the removal of passing in a `HWND` just to ask you to add it back in to instead remove the `GHOST_` dependencies, the former was a lower hanging fruit to understand at the time. Note removing the `GHOST_System` dependency will also remove the need for `GHOST_EventTrackpad` as the event generation would be moved out too.
30	`DM_ASSERT` is a tad misleading, assert is expected to be a noop in release. `DM_CHECK_RESULT_AND_EXIT_EARLY` was suggested as better but not ideal alternative when I asked other maintainers. I tend to prefer not using macros, but in this case I think it's the right call given the constraints.
111	Since this loop is called every frame, I think we should track the viewport's `DIRECTMANIPULATION_STATUS` and only call `UPDATE` when the status is `DIRECTMANIPULATION_RUNNING` or `DIRECTMANIPULATION_INERTIA`.
179	Instead of polling this every frame, pass in the dpi during initialization and update it when `WM_DPICHANGED` occurs. This gets us halfway to removing the `GHOST_Window` dependency.
218	Nitpick: phrasing "next updates" is a bit awkward, I'd use "following updates".
220–251	Instead of pushing event here, save the accumulated state. Add a function `GetTouchpadInfo` to the helper which grabs the accumulated state, returns e.g. a `TOUCHPAD_INFO` struct. When that state is pulled, it should be zeroed sans the fractional part. This allows us to remove `GHOST_System` and the remainder of `GHOST_Window` from the touchpad handler.

Store DPI value in GHOST_DirectManipulationViewportEventHandler
Rename macro
Update DM only when active
Move event creation out of Window class

Harbormaster completed remote builds in B20449: Diff 48149.Feb 9 2022, 6:52 PM

The last pass restructure looks good. The remainder is mostly memory safety.

I think this will be ready to push to the 3.2 branch afterward. Heads up that I might make minor changes if I notice anything before pushing.

intern/ghost/intern/GHOST_SystemWin32.cpp
430	The active window can change while events are handled (and iirc can be destructed to) so this pointer may be incorrect or invalid by this point. We need to get it again. My preferred fix is to pull out the following logic into a `system->processTrackpad` function and get the active window in the function.
intern/ghost/intern/GHOST_TrackpadWin32.cpp
130–139	In `GHOST_DirectManipulationHelper::create` the current pattern is to create the Helper, and the populate it. Because of this it may be in a partially initialized state during destruction, and these cleanup calls would be null pointer dereferences. My preferred fix here would be to not allow Helper to ever be in a partially initialized state. What this would mean is that in `create` instead of passing references to the helper's members, you would pass local variables to initialize with and initialize helper with those. Just as an example (removing error checking for clarity). // Before: GHOST_DirectManipulationHelper GHOST_DirectManipulationHelper::create(HWND hWnd, uint16_t dpi) { GHOST_DirectManipulationHelper helper = new GHOST_DirectManipulationHelper(hWnd); HRESULT hr = ::CoCreateInstance(CLSID_DirectManipulationManager, nullptr, CLSCTX_INPROC_SERVER, IID_PPV_ARGS(&helper->m_directManipulationManager)); hr = helper->m_directManipulationManager->GetUpdateManager( IID_PPV_ARGS(&helper->m_directManipulationUpdateManager)); ... return helper; } // After: GHOST_DirectManipulationHelper *GHOST_DirectManipulationHelper::create(HWND hWnd, uint16_t dpi) { Microsoft::WRL::ComPtr<IDirectManipulationManager> directManipulationManager; HRESULT hr = ::CoCreateInstance(CLSID_DirectManipulationManager, nullptr, CLSCTX_INPROC_SERVER, IID_PPV_ARGS(&directManipulationManager)); Microsoft::WRL::ComPtr<IDirectManipulationUpdateManager> directManipulationUpdateManager; hr = directManipulationManager->GetUpdateManager( IID_PPV_ARGS(&directManipulationUpdateManager)); ... return new GHOST_DirectManipulationHelper(hWnd, directManipulationManager, directManipulationUpdateManager, ...); }
intern/ghost/intern/GHOST_WindowWin32.h
409–412	This needs to be initialized to `nullptr` (see the intializer list in `GHOST_WindowWin32`'s constructor).

I'll resign from reviewing this, unless anyone wants me to look at something specific in this commit.

Merge branch 'master' into precision-touchpad
Update license headers
Move trackpad event creation code to separate function
Initialize m_directManipulationHelper to null in GHOST_WindowWin32's constructor
Use different creation pattern for GHOST_DirectManipulationHelper to avoid partially initialized state

Harbormaster completed remote builds in B20557: Diff 48362.Feb 15 2022, 4:26 PM

Andrii Symkin (pembem22) marked 3 inline comments as done.Feb 15 2022, 4:27 PM

Chris Cook (CCook) awarded a token.Feb 17 2022, 10:27 AM

@Andrii Symkin (pembem22) heads up that this is still on my radar. Only reason it hasn't landed is because I've run into some issues with my local builds, and I saw some weird behavior just before the build issues. This is my top priority once I sort that out; I don't anticipate asking for any further revision.

Nicholas Rishel (nicholas_rishel) added a project: BF Blender (3.2).Mar 2 2022, 6:51 AM

Merge branch 'master' into precision-touchpad
Fix initialization order warning

Harbormaster completed remote builds in B20930: Diff 49062.Mar 8 2022, 9:33 PM

Nicholas Rishel (nicholas_rishel) accepted this revision.Mar 9 2022, 1:33 AM

Ray Molenkamp (LazyDodo) resigned from this revision.Mar 9 2022, 1:39 AM

This revision is now accepted and ready to land.Mar 9 2022, 1:39 AM

@Nicholas Rishel (nicholas_rishel) can you please commit the patch? I don't have the commit rights.

In D7660#382582, @Andrii Symkin (pembem22) wrote:

@Nicholas Rishel (nicholas_rishel) can you please commit the patch? I don't have the commit rights.

There were some inconsistencies between MacOS and Windows touchpad that need to be sussed out before pushing to master; notably in rotation chirality and pan direction (holding shift while two finger panning). I noticed these issues just before the final commit.

I was already discussing the issue with some developers with MacBooks handy, but if you have a MacBook and would prefer to investigate please do so.

The plan is still to land this in 3.2. In case you're concerned there's no reason to think we'll miss that deadline.

In D7660#382852, @Nicholas Rishel (nicholas_rishel) wrote:

I was already discussing the issue with some developers with MacBooks handy, but if you have a MacBook and would prefer to investigate please do so.

Yes, I can test it on a MacBook as well. I just did that and looks like on Windows orbiting direction is incorrect. Maybe it's related to the fact that the "Scroll direction" option is supported on macOS. Panning gestures are affected by it, while orbiting ones do not, which means there's additional logic that's not in GHOST.

I was also curious if holding shift might change what events occur on MacOS. Does two finger panning always generate a NSEventTypeScrollWheel event even when shift is held? If so is it possible the event's deltaX, deltaY, or isDirectionInvertedFromDevice are negated (I think the last one is what you were referring to being supported on MacOS)?

Could you also check if ctrl and two finger panning does anything on MacOS?

Fix inverted rotation direction in 3D viewport

Harbormaster completed remote builds in B21072: Diff 49350.Mar 16 2022, 9:24 PM

Holding control or command while panning on macOS results in zooming, the same is also true on Windows. I can't test which events are generated on macOS, but I don't think that pressing any buttons on a keyboard would affect a touchpad in any way.

Simply changing the isDirectionInverted parameter from false to true seems to fix the issue, as it only affects rotation in 3D viewport.

Simply changing the isDirectionInverted parameter from false to true seems to fix the issue, as it only affects rotation in 3D viewport.

It affects not only the rotation in 3D viewport but also many other functions. Most likely you need to use false for isDirectionInverted, as it really is in your case. Otherwise you will get an wrong direction when using WM_event_absolute_delta_x / WM_event_absolute_delta_y or vice versa when these functions are not used. The idea is that the upward movement of the fingers should increase the value (for example, the number of loop cuts or when using Ctrl + MOUSEPAN over the button).

I assume that the issue may be the wrong code in viewrotate_invoke, see my fixes for viewrotate_invoke in D8521. And take a look at all these T82006 fixes too, read the descriptions/comments for a better insight into how it is meant to work. Unfortunately I can't test this patch on Windows.

@Yevgeny Makarov (jenkm) just tested on Windows and trackpad 2 finger pan delta is negative when pushing up and to the left. Up is opposite of what you described, what's negative horizontally on MacOS? With isDirectionInverted set to true things appeared to work correctly on Windows, including 2 finger pan up to increase edge loop cut count.

This isDirectionInverted is when the content follows the finger (natural scroll direction), otherwise it's like scrolling using a scrollbar i.e. the content moves in the opposite direction to the cursor/scrollbar direction.

This is an option in OS preferences: "Natural scroll direction" (macOS) and "Reverse scrolling direction" (Windows). So you need to read this OS preference and not just set true or false.

Further, it seems to be on Mac and Windows, the meaning of this preference is opposite:
https://bootcamp.uxdesign.cc/natural-scrolling-mac-vs-reverse-scrolling-windows-e48656275081
https://jessequinnlee.com/2015/07/25/natural-scrolling-vs-reverse-scrolling/

In general, the bottom line is that panning the 3D view, scrolling panels, etc. follows the system preferences (the direction is the same as in other programs).
But, for example, the 3D view rotation always follows the fingers, and for changing the value - moving up/right is always increasing the value (regardless of the system preferences).

So what it looks like we need to inspect the registry key HKCU\SOFTWARE\Microsoft\Windows\CurrentVersion\PrecisionTouchPad\ScrollDirection for the scroll direction while setting up GHOST_DMHelper so we can populate isDirectionInverted appropriately, then register for changes to said registry key.

@Andrii Symkin (pembem22) want to take this on too?

Yeah, I'll do it. Where would you suggest putting registry-related code in? GHOST_DirectManipulationHelper seems to be the right place.

GHOST_DirectManipulationHelper would be my first guess too. I'm not sure how the async modifier for RegNotifyChangeKeyValue works, so that modifier might change what works best for the code. I think you'll better understand that once you dive into the question. :)

Quick aside, do you have an account on blender.chat?

Load scroll direction value from registry when initializing touchpad
Merge branch 'master' into precision-touchpad

This revision is now accepted and ready to land.Apr 5 2022, 11:19 AM

Harbormaster completed remote builds in B21464: Diff 50106.Apr 5 2022, 11:19 AM

In D7660#385490, @Nicholas Rishel (nicholas_rishel) wrote:

GHOST_DirectManipulationHelper would be my first guess too. I'm not sure how the async modifier for RegNotifyChangeKeyValue works, so that modifier might change what works best for the code. I think you'll better understand that once you dive into the question. :)

I looks like another thread needs to be created, because both non-async RegNotifyChangeKeyValue and async RegNotifyChangeKeyValue + WaitForSingleObject are blocking, unless there's a way to wait for an hEvent without blocking a thread.

In D7660#385490, @Nicholas Rishel (nicholas_rishel) wrote:

Quick aside, do you have an account on blender.chat?

I do, same username as here.

Just checking the docs on WaitForSingleObject, it looks like it's non-blocking if the dwMilliseconds argument is zero. If you go this route I'd suggest checking for changes when the gesture begins; we know it won't change mid-gesture and doesn't need to be checked until then.

I poked a bit at this and found we could throw a callback in a thread pool with some combination of the REG_NOTIFY_THREAD_AGNOSTIC flag for RegNotifyChangeKeyValue and the WT_EXECUTEINPERSISTENTTHREAD flag for RegisterWaitForSingleObject. In my opinion that's a lot of noise when the above or a manual separate thread should work well enough.

Richard Allinson (richardallinso) removed a subscriber: Richard Allinson (richardallinso).Apr 6 2022, 12:26 AM

Listen for scroll direction changes in registry

This revision is now accepted and ready to land.Apr 6 2022, 9:27 PM

Harbormaster completed remote builds in B21491: Diff 50173.Apr 6 2022, 9:27 PM

Andrii Symkin (pembem22) requested review of this revision.Apr 6 2022, 9:28 PM

Looks good!

We just missed the window for new features in Blender 3.2. I asked around whether we should consider this a bug fix or new feature; we decided new feature because we've run into issues in the past for late changes to Ghost.

I'll be merging this as soon as I can - should be April 27th. Thank you for your patience and persistence in seeing this patch through. :)

This revision is now accepted and ready to land.Apr 7 2022, 4:28 AM