Details
Diff Detail
- Repository
- rB Blender
Event Timeline
This is ready for review.
@Brecht Van Lommel (brecht) I chose against deprecating Automatic/switching to Windows Ink by default as the remaining Wintab issues were coordinate mapping and mouse mode support. This rewrite opts for behavior similar to Qt and GTK which handles mapping tablet to Win32 screen coordinates manually. One difference is that on button press, the input between Win32 and Wintab is compared as a sync point to determine if Wintab coordinates can be trusted. I believe this synchronization method is simple enough to reason about to be worth the benefits. The former should fix several tablets that were incorrectly scaled when Wintab itself handled the scaling, and the latter retains mouse mode support with pressure.
| intern/ghost/intern/GHOST_SystemWin32.cpp | ||
|---|---|---|
| 1010 | I would like to leave this unpopulated for a short time to get feedback from nightly users when coordinate mapping has failed. | |
I don't currently have a tablet hooked up, and don't know these APIs, so don't take the following too strongly as they could be dumb.
I'm unsure if you have deliberately removed WM_POINTERENTER. Even if not processed I'd rather see it listed, even if just passed to processPointerEvent() where it is ignored.
Not your fault, but part of what makes this harder to follow is that enum GHOST_TTabletAPI contains GHOST_kTabletAutomatic, GHOST_kTabletNative, GHOST_kTabletWintab. Yet there are many comments that talk of "Windows Ink API". I realize this is GHOST_kTabletNative, but could we rename this enum? If not I'd love to see comments that make this clearer when mentioned.
Similarly I find useTabletAPI() potentially confusing since it takes an API, returns a bool, and so with the verb "use" seems like it would SET the Tablet API, rather than confirm a particular one is being used. Would rather that be "usingTabletAPI()" or isUsing or similar.
devicesPresent() could read to be either returning a number of devices or whether there are any. Since it is the latter returning a bool it would be clearer to return "m_numDevices > 0" rather than just m_numDevices. Pedantic, I know.
I find mapWintabToSysCoordinates() confusing as it is only ever called with the same pair of variables used for both in and out, so not clear why the in is needed. And they mix LONG and int types. Probably something I am missing there.
m_mousePresent does not sound like it would keep track of whether the mouse is "over or captured by the window" but whether one exists.
Of course there are lots of variables already in there that also bug me. Like m_inLiveResize could just be m_isResizing
Will try to peer through more later.
Could easily be a switch break case like other ignored Win32 events. I just removed it because I had added it a while back and it has become irrelevant after refactoring. There are several other pointer callbacks also not present in the callback switch so this one didn't seem special.
Not your fault, but part of what makes this harder to follow is that enum GHOST_TTabletAPI contains GHOST_kTabletAutomatic, GHOST_kTabletNative, GHOST_kTabletWintab. Yet there are many comments that talk of "Windows Ink API". I realize this is GHOST_kTabletNative, but could we rename this enum? If not I'd love to see comments that make this clearer when mentioned.
Making things worse is that we don't even use the Windows Ink API, we use Pointer events. But Pointer events piggyback on Windows Ink hence why the latter must be enabled in tablet preferences. I wasn't sure how to square this circle.
I find mapWintabToSysCoordinates() confusing as it is only ever called with the same pair of variables used for both in and out, so not clear why the in is needed.
This is an attempt to be a resilient to refactoring, when in and out are the same (in the call) you know you're changing the same value intentionally. An easy refactor mistake if in and out are the same (in the function declaration) is to double remap coordinates because you forgot to copy your argument when you just needed to test the coordinates. I'm open to other ideas here though, maybe taking a "point" struct, returning a point struct would be more intuitive? In a nicer language this would have been a tuple in, return tuple out.
And they mix LONG and int types. Probably something I am missing there.
In and extent from Wintab are int and LONG. No idea why, should probably just drop the weirdness and int (int32_t? Ghost_Int32?) everything.
m_mousePresent does not sound like it would keep track of whether the mouse is "over or captured by the window" but whether one exists.
m_trackingMouse/mouseTracked better? I chose m_mousePresent because mouse tracking is just informing us when the mouse leaves (out of the client area and not captured).
For anything I haven't replied to assume I nodded in agreement.
I can't really spot errors in the code, but it's hard to guess how it works in practice. Only minor comments.
Have users tested a build with this patch? Any known issues, or does it only improve things as far as we know?
| intern/ghost/intern/GHOST_SystemWin32.cpp | ||
|---|---|---|
| 877–878 | I guess this event is often redundant, and already handed through a mouse move event? Do we know it gets filtered out in the window manager, or does it lead to double event handling? | |
| 1557 | Don't do assignments in conditionals like this. It's not generally something we do in Blender code, better to follow the (implicit) convention. Also don't use auto unless it's a specifically long type name like an STL iterator. | |
| intern/ghost/intern/GHOST_Wintab.cpp | ||
| 486 | Remove debug print? | |
@Brecht Van Lommel (brecht) I'd appreciate any feedback on how to make the handling more clear, or what's unclear that I might revisit. A bit of this is the nature of the problem (synchronizing two event streams, working around common Wintab bugs) but I suspect there's room to clarify things.
I solicited feedback on Blender Artists and in T88852 from people who reported issues with the last attempt, thus far no issues that weren't fixed with a driver update.
I'll anticipate a regression in Walk navigation for tablets that I'll need to be address, relevant issues during last attempt were T84659 and/or T83930.
| intern/ghost/intern/GHOST_SystemWin32.cpp | ||
|---|---|---|
| 877–878 | This would not generally be redundant because tablet events (Wintab and WinPointer iirc) are asynchronously ordered from mouse events. If we're receiving a button event while a tablet is present, either Wintab handling failed to steal the event from Windows event queue (common case) or the mouse event was not generated from the stylus (e.g. a tablet pad's express buttons). In general operators should be tolerant of spurious mouse events because 1. variable pressure without motion requires this and 2. Windows intentionally generates spurious mouse movements which are currently not filtered out. | |
| intern/ghost/intern/GHOST_WindowWin32.h | ||
| 262–290 | Todo | |
| intern/ghost/intern/GHOST_SystemWin32.cpp | ||
|---|---|---|
| 1557 | Would auto be appropriate when assigning from constructors and/or casts? For instance auto info = (GHOST_WIN32_WTInfo)::GetProcAddress(handle.get(), "WTInfoA");
if (!info) {
return nullptr;
}
auto open = (GHOST_WIN32_WTOpen)::GetProcAddress(handle.get(), "WTOpenA");
if (!open) {
return nullptr;
}and auto pointerPenInfo = std::vector<POINTER_PEN_INFO>(outCount); | |
- Rename to clarify API used.
- Renaming and debug cleanup.
- Remove autos, remove assignment as conditions.
- Added comment.
@Harley Acheson (harley) I thought some more on mapWintabToSysCoordinates(), I didn't mention that the reason I wanted refactorability here was because we may want to have some special checks, e.g. if mapping from the data provided by Wintab fails, try mapping to the entire virtual screen instead. I expect at least one more pass after getting broader user feedback from nightly, either checking other mappings will be necessary and I would harden the function by differentiating raw and mapped tablet coordinates with types, or it's unnecessary and it will be rewritten so that GHOST_Wintab only returns mapped points.
| intern/ghost/intern/GHOST_SystemWin32.cpp | ||
|---|---|---|
| 932 | There's quite a few comparisons in the [constant] [comparison] [variable] form, while it is sometimes advocated as "the safe way, just in case you make a typo, and ignore all warnings the compiler emits about that" it also makes the code harder to read than necessary. I don't think we have any guidance in the code-style guide, given my personal preferences commonly being at odds with our code-style I'm going to defer the final decision here to @Brecht Van Lommel (brecht) Whatever way is chosen though, its application at this point is inconsistent [constant] [comparison] [variable] and [variable] [comparison] [constant] both appear in newly added code. | |
| intern/ghost/intern/GHOST_Wintab.cpp | ||
|---|---|---|
| 447 | There's a lot going on here, and this comment is only moderately more readable than the code down below Given there seems to be a fair bit of repetition as well is there any chance this could be decomposed further in perhaps some functions with a more descriptive names? | |
| intern/ghost/intern/GHOST_Wintab.cpp | ||
|---|---|---|
| 448 | if i'm reading the code right, is this what is happening here? double x_normalized = ((double)x_in) - tab.org[0]) / tab.ext[0]; double y_normalized = ((double)y_in) - tab.org[1]) / tab.ext[1]; x_out = (int) ((x_normalized * sys.ext[0]) + sys.org[0]); y_out = (int) ((y_normalized * sys.ext[1]) + sys.org[1]); | |
| intern/ghost/intern/GHOST_Wintab.cpp | ||
|---|---|---|
| 448 | Essentially yes, the else case being similar except it moves the origin to origin+extent and maps in reverse over the same range. Reversing over the range initially tripped me up so I had some resistance to straying much from the documentation. I opt to compute in integers given that's how the documentation presents it, see comments under this section, and that I couldn't convince myself it would not reduce rounding error. If you wanted to break it into it's parts while staying in integers it would go:
| |
- Allow system fallback for cursor movement from pointer events instead of manual handling.
- Comment cleanup.
- Pull assignment out of if statement.
- Remove auto.
- Cleanup of Wintab event processing function.
- Missed brackets.
I can't really think of good suggestion for how to improve this implementation. If it worked fine in testing so far, we just have to try committing again I think.
| intern/ghost/intern/GHOST_SystemWin32.cpp | ||
|---|---|---|
| 877–878 | Ah, I missed that this code only runs for tablets. It seems reasonable then. I guess at worst on the WM side it will generate an extra inbetween mousemove event, which is not a problem. | |
| 932 | [variable] [comparison] [constant] is most common in the code and I would consider that the implicit convention that is suggested to be followed. | |