Bug 452662 - Two-finger pan/rotate gesture glitch with changing touch points
Summary: Two-finger pan/rotate gesture glitch with changing touch points
Status: RESOLVED FIXED
Alias: None
Product: krita
Classification: Applications
Component: Shortcuts and Canvas Input Settings (other bugs)
Version First Reported In: git master (please specify the git hash!)
Platform: Microsoft Windows Microsoft Windows
: NOR normal
Target Milestone: ---
Assignee: sh_zam
URL:
Keywords: regression
Depends on:
Blocks:
 
Reported: 2022-04-15 16:10 UTC by Alvin Wong
Modified: 2022-08-11 08:09 UTC (History)
1 user (show)

See Also:
Latest Commit:
Version Fixed/Implemented In:
Sentry Crash Report:


Attachments
tablet log, the gesture has been performed multiple times (21.85 KB, text/plain)
2022-04-15 16:10 UTC, Alvin Wong
Details
tablet log with touch point state (31.30 KB, text/plain)
2022-07-20 11:35 UTC, Alvin Wong
Details

Note You need to log in before you can comment on or make changes to this bug.
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