Bug 452662

Summary: Two-finger pan/rotate gesture glitch with changing touch points
Product: [Applications] krita Reporter: Alvin Wong <alvin>
Component: Shortcuts and Canvas Input SettingsAssignee: sh_zam <shzam>
Status: RESOLVED FIXED    
Severity: normal CC: halla
Priority: NOR Keywords: regression
Version First Reported In: git master (please specify the git hash!)   
Target Milestone: ---   
Platform: Microsoft Windows   
OS: Microsoft Windows   
Latest Commit: Version Fixed/Implemented In:
Sentry Crash Report:
Attachments: tablet log, the gesture has been performed multiple times
tablet log with touch point state

Description Alvin Wong 2022-04-15 16:10:39 UTC
Created attachment 148175 [details]
tablet log, the gesture has been performed multiple times

Hold one finger down completely still, then flick (press, move, release) using another finger repeatedly. Sometimes you can see the canvas jump as if you have never released the second finger. Sometimes the canvas rotates 180 degrees.

IRC conversation:

<sh_zam> windragon: the problem seems here: 
<sh_zam> [2528] krita.tabletlog: "[       ] TouchUpdate      TouchScreen id: 100663306 hires:      930,   557.5 prs: 0.5 rot: 0; id: 100663297 hires:      294,     288 prs: 0.5 rot: 0; "
<sh_zam> [2528] krita.tabletlog: "[       ] TouchUpdate      TouchScreen id: 100663297 hires:      294,     288 prs: 0.5 rot: 0; id: 100663307 hires:      948,   342.5 prs: 0.5 rot: 0; "
<sh_zam> the touchpoints are flipped by the driver apparently. 
<windragon> sh_zam: the id values are mapped correctly though
<sh_zam> this doesn't seem like a regression caused by the gesture handling code. Because we don't do anything which will require us to use IDs. Where as for rotation code because angle is computed with the finger placement, it seems that this *is* the source. 
<sh_zam> windragon: yes, but we calculate delta between fingers and assume them to be in order always: https://invent.kde.org/szaman/krita/blob/6c46fcbc1324b50756ff2e36b748c49c301ebec1/libs/ui/input/kis_rotate_canvas_action.cpp#L155-156
<windragon> which is an incorrect assumption
<sh_zam> windragon: yes 
<windragon> sh_zam: but more importantly, there is no sign in the latter half of the tablet log that it even acknowledged that the second finger has ever been lifted from the touchscreen
<windragon> the touch point id changed, but the gesture handling probably didn't pick that up
Comment 1 sh_zam 2022-07-14 08:34:11 UTC
Hi Alvin!

Can you please apply this patch and paste the tablet logger output of this buggy action?

From 8febc9de49046fb53e19cd676234ecfd6ecbc040 Mon Sep 17 00:00:00 2001
From: Sharaf Zaman <shzam@sdf.org>
Date: Thu, 14 Jul 2022 06:31:53 +0000
Subject: [PATCH] Add touch point state information to tablet debugger

---
 libs/ui/input/kis_tablet_debugger.cpp | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/libs/ui/input/kis_tablet_debugger.cpp b/libs/ui/input/kis_tablet_debugger.cpp
index ec91a4302e..30aab3252f 100644
--- a/libs/ui/input/kis_tablet_debugger.cpp
+++ b/libs/ui/input/kis_tablet_debugger.cpp
@@ -183,7 +183,8 @@ QString KisTabletDebugger::eventToString(const QTouchEvent &ev, const QString &p
         s << "id: " << touchpoint.id() << " ";
         s << "hires: " << qSetFieldWidth(8) << touchpoint.screenPos().x() << qSetFieldWidth(0) << "," << qSetFieldWidth(8) << touchpoint.screenPos().y() << qSetFieldWidth(0) << " ";
         s << "prs: " << touchpoint.pressure() << " ";
-        s << "rot: "<< touchpoint.rotation() << "; ";
+        s << "rot: "<< touchpoint.rotation() << " ";
+        s << "state: 0x" << hex << touchpoint.state() << "; ";
     }
 
     return string;
-- 
2.37.0
Comment 2 Alvin Wong 2022-07-20 11:35:46 UTC
Created attachment 150768 [details]
tablet log with touch point state

Sorry for the delay. Here is the log with the state flag included. In this log I repeated the flick 9 times, on the 8th time the canvas flipped 180 degrees.
Comment 3 Bug Janitor Service 2022-07-26 10:25:08 UTC
A possibly relevant merge request was started @ https://invent.kde.org/graphics/krita/-/merge_requests/1533
Comment 4 sh_zam 2022-08-10 16:03:59 UTC
Git commit 2b53e47537961f1c004f871810ef239575a2b205 by Sharaf Zaman.
Committed on 10/08/2022 at 14:58.
Pushed by szaman into branch 'master'.

Fix two finger gesture glitch causing canvas to jump/rotate

Typically the state is reset as a symptom of continuous touch events
even after the finger is lifted. What I mean:

1. User lifts a finger (in two finger touch). We get a touch event with
two touchpoints and it is forwarded to this action.

2. Due to slight jitter another touch event but with single touch point
is sent by the OS and this is what resets the state of an action which
relies on state of previous events.

But on Windows or in the case of this bug, (2) is never sent, this makes
the action think that the touchpoint was never released, therefore the
state is never reset.

M  +14   -1    libs/ui/input/kis_shortcut_matcher.cpp

https://invent.kde.org/graphics/krita/commit/2b53e47537961f1c004f871810ef239575a2b205
Comment 5 sh_zam 2022-08-11 08:09:26 UTC
Git commit a72f69165e56bb345cfa034145fdc4a2215f9516 by Sharaf Zaman.
Committed on 11/08/2022 at 08:05.
Pushed by szaman into branch 'krita/5.1'.

Fix two finger gesture glitch causing canvas to jump/rotate

Typically the state is reset as a symptom of continuous touch events
even after the finger is lifted. What I mean:

1. User lifts a finger (in two finger touch). We get a touch event with
two touchpoints and it is forwarded to this action.

2. Due to slight jitter another touch event but with single touch point
is sent by the OS and this is what resets the state of an action which
relies on state of previous events.

But on Windows or in the case of this bug, (2) is never sent, this makes
the action think that the touchpoint was never released, therefore the
state is never reset.
(cherry picked from commit 2b53e47537961f1c004f871810ef239575a2b205)

M  +14   -1    libs/ui/input/kis_shortcut_matcher.cpp

https://invent.kde.org/graphics/krita/commit/a72f69165e56bb345cfa034145fdc4a2215f9516