Page MenuHome

Fix T83930 and Fix T84659: Walk navigation tablet bugs.
AbandonedPublic

Authored by Nicholas Rishel (nicholas_rishel) on Jun 19 2021, 7:19 AM.

Details

Summary

The change modifies Ghost to send a dummy cursor event when a tablet stylus leave tracking range, as a signal to operators that tablet tracking has ended. The walk operator is modified to use this signal to reset the offset origin.

The walk operator has also been modified to not send a mouse centering event during initialization to simplify logic in it's modal callback.

Fixes T83930, allowing walk navigation to continue without jumping back
after repositioning pen.

Fixes T84659, allow walk navigation to start (for Windows Ink) from
keyboard shortcut when pen is in use.

Windows test build
Mac x86 test build
Mac ARM test build
Linux test build

Diff Detail

Repository
rB Blender
Branch
fix-tablet-walk (branched from master)
Build Status
Buildable 15313
Build 15313: arc lint + arc unit

Event Timeline

Nicholas Rishel (nicholas_rishel) created this revision.

Try to trigger phab update.

If there's a way to hide the parented revision's commits let me know, I can't fathom why they're showing up here. Only commit 76efd2ac2277 is relevant for this review, the rest was handled in D11508.

Credit where credit is due, the essence of this fix was first proposed by @Vincent Blankfield (vvv) in D9899.

The idea is that after a tablet leaves the tracking area, a "noop" cursor move without tablet data is sent to signal absolute position tracking has ended. This is a bit of a hack of the current behavior, but it's pragmatic and low risk.

A more robust solution might instead instead add "cursor wrap" support for tablets, allowing mouse and tablets to accumulate position interchangeably while a modal operator is running. This would not fix the present issue of walk navigation not working, as the walk operator re-implements cursor grabbing manually instead of relying on Ghost's handling.

This could use testers on mac/linux to ensure no walk navigation regressions with tablet input.

Rebase after Wintab merge.

@Dalai Felinto (dfelinto) Could you have someone in BF/Studio check that this doesn't cause regressions in Linux?

This revision is now accepted and ready to land.Jun 29 2021, 1:46 AM

I tried the test build out on Linux and it doesn't fix T83930 for me. I'm using a Wacom Cintiq but I don't think that would cause any different behaviour in Blender from a regular pen tablet.

Checked this patch on Windows with an old Wacom Bamboo and Windows Ink enabled. It kind of fixes the pen repositioning. But there is a more important issue - sometimes the tablet walk navigation goes into an uncontrolled spin. Not sure about exact steps to reproduce, but it happens more often then not. It apparently doesn't occur when the pen has been tapped prior to entering the walk navigation. It's hard to tell whether the previous issues are really fixed with this unstable behaviour.

The spin appears to be caused by multiple 'warp' events, caused by the setCursorPosition() call in processCursorEvent(). And that is caused by the tablet input being processed like non tablet input. The cause for that is marked by "TODO supply tablet data" in processWintabEvent() fallback case. I may be wrong about some things here. If so, sorry about that. I haven't got much time investigating this, and I don't have any understanding of this event system.

I tried simply supplying a dummy tablet data, but in my testing it wasn't quite enough. The repositioning still failed sometimes. For example, when the pointer went outside the window, or when switching back and forth between mouse and tablet.

Anyway, I've hacked together a patch that seems to fix the known issues for me. It isn't supposed to be committed like that, and it isn't properly tested. It's just a quick and dirty hack to illustrate the problems, with quick and dirty comments here and there.

The issues it addresses:

  1. Can't start walk navigation with a pen in range.
  2. Uncontrollable spin when using tablet for walk navigation.
  3. Pen repositioning rotates the view.

The patch is based on this branch (46221219d880c8148f783788d88b712ec3d19ae0 of fix-tablet-walk).

  • Cleanup: Swap defines for static const in walk and fly mode.
  • Defer mouse centering until mouse have moved.
  • Add signal for end of tablet input to x11 and cocoa.
This revision is now accepted and ready to land.Jul 9 2021, 7:29 PM
Nicholas Rishel (nicholas_rishel) added inline comments.
source/blender/editors/space_view3d/view3d_navigate_walk.c
957–958 ↗(On Diff #39383)

@Campbell Barton (campbellbarton) Thoughts on removing separated walk scaling for tablet input? This revision should allow full rotation by moving the stylus outside of the tracking range, similar to picking up and repositioning a mouse.

Anyone I can ping for a tablet UX perspective of whether this is preferable to leave in place?

Got a little bit lost in branches, but tested the linked build (f07f069ce268 from wintab_fallback_walknav). It's much more stable than the previous version, but it still has some issues with the pen repositioning. For example:

  • Make Blender window not fullscreen.
  • Tap the pen in the 3D View.
  • Enter the walk navigation (Shift+~).
  • Get the pen in range without touching the tablet surface at top, bottom, left and right.
  • So far everything should work fine.
  • Now get the pen in range outside of the Blender window (touch or no touch doesn't matter).
  • Get the pen in range without touching the tablet surface at top, bottom, left and right, inside of the Blender window.
  • Now the view occasionally jumps around when the pen is repositioned.

Not sure about the reason for this, but it doesn't happen with my silly hack diff.

  • Another tablet leave condition.
  • Return in walk when switching devices to delay warping to center.

@Vincent Blankfield (vvv) I think this recent change merges what worked in your patch with my changes. What was happening was for a brief moment when the cursor re-entered the window it was recognized as a mouse before Wintab starts up (the issue wasn't present in WinInk). Essentially, by returning early in the walk operator when switching between tablet and mouse, you avoid warping the cursor for a bit. This seems to be good enough to prevent the race condition.

You can recreate the issue in the current patch by leaving the window with tablet, and returning with mouse, while the walk operator is active. I can't think of a fix for this beyond adding the idea of semi-device aware relative movement to Ghost's cursor events and adding them to the walk operator. Adding that would carry additional debugging cost/risk. At the moment I'd prefer to avoid that since afaik it would only be relevant for the walk operator in this corner case. That said, such a change could be useful as a base for building pen/mouse switching and pen repositioning while the cursor is grabbed.

Tested f07f069ce268 of wintab_fallback_walknav with the changes from Diff 39388 manually applied (the "return" and the "before focus lost" case). If I understand correctly, this patch is the latest state of the development. But I'm not sure what this patch is based on. My git says it doesn't apply neither to master, nor to wintab_fallback_walknav or fix-tablet-walk. I'm not very proficient in this stuff, so I'm probably doing something wrong here.

Anyway, f07f069ce268 + Diff 39388 changes doesn't fix the problem for me. Here's a video of me testing it:

At first everything works fine (after the pen has been tapped inside the Blender window). But after the pen enters in range over the terminal window, the walk navigation breaks. The view starts jumping around randomly on pen repositioning.

In the recorded test I've added some debug output and the visualization of walk->center and walk->prev_mval. Also the cursor isn't hidden. Here's the patch with all the debug changes based on f07f069ce268:

As for my initial silly demo patch that passes my testing, it ignores more than one event when changing from absolute to relative and back. First it returns on the actual change event, the second event will be with is_cursor_first so in case of a mouse move, it will be ignored, and the cursor will be recentered instead. I've simplified that patch a bit. Indeed messing with walk->center_mval was unnecessary. Still, this patch isn't thought through, I just made the old logic work for me without any understanding of it. Here's the simplified version, still based on 46221219d880:

P.S. Sorry for making so much noise here. If this is a problem for anyone, say /stop and I will stop complaining. Until then I will be an annoying unsatisfied user.

@Vincent Blankfield (vvv) the easiest way to get the update is to either download the raw diff (from this page) and apply it to a new branch from master (locally) or install archanist and run arc patch D11651. Note that you might have to resolve conflicts (I'll try to keep this patch updated so there aren't any). You can find notes on archanist here.

Don't stop, the detailed reports are useful. I'll check if I can recreate.

Edit: ack, thought the fix worked because Tablet API had been set to WinInk.

After further work on this I found things were getting increasingly hacky as I introduced more edge case fixes. I've decided on another broader approach which differs significantly from this one, so closing this issue in favor of a new revision. I'll tag this revision once the alternative is uploaded.

BTW, I have tried that Win test build up there and it works fine for me.