Rewrite KisInputManager
AbandonedPublic

Authored by abrahams on Aug 25 2015, 6:30 PM.

Details

Reviewers
dkazakov
rempt
Maniphest Tasks
T530: Fix tablet jitter
Summary

This drastically changes the way KisInputManager works. Instead of relying on MouseMove events to carry our information, we transmit information from every event. I moved KisManager::Private into its own kis_input_manager_p.h / .cpp since it was cluttering the main logic.

This patch disables the various macros like start_ignore_cursor_events() and so on. I'm sure it breaks touch support and other corners of the event handling loop but I don't have a suitable device to test this on. In Windows with our custom event handler it works very nicely on my Intuos.

In Linux, using the default Qt handler, this works just fine. The problem is that we still receive fake MouseMove events, which mess up the pressure response. (We get big max-pressure blotches in the middle of the line.) I tried to ensure we accept every single tablet event we come across, but in fact it seems like we receive MouseMove events carrying different position information from the tablet events, so the events themselves were not even triggered by event rejection! My log looks like:

"      MouseMove        btn: 0 btns: 1 pos: 1314,1177 gpos: 4223,1404 "
"      TabletMove       btn: 0 btns: 1 pos: 1314,1178 gpos: 4223,1405 hires:  4223.25, 1404.95 prs: 0.242676 Stylus Pen id: 794728369274 xTilt: 16 yTilt: 1 rot: 0 z: 0 tp: 0 "
"      MouseMove        btn: 0 btns: 1 pos: 1317,1177 gpos: 4226,1404 "
"      TabletMove       btn: 0 btns: 1 pos: 1318,1177 gpos: 4227,1404 hires:  4226.52, 1404.03 prs: 0.242676 Stylus Pen id: 794728369274 xTilt: 16 yTilt: 1 rot: 0 z: 0 tp: 0 "
"      MouseMove        btn: 0 btns: 1 pos: 1320,1178 gpos: 4229,1405 "
"      TabletMove       btn: 0 btns: 1 pos: 1321,1178 gpos: 4230,1405 hires:  4229.61, 1405.11 prs: 0.242676 Stylus Pen id: 794728369274 xTilt: 16 yTilt: 1 rot: 0 z: 0 tp: 0 "

On the other hand,

Diff Detail

Repository
R8 Calligra
Branch
wreck-KisInputManager
Lint
No Linters Available
Unit
No Unit Test Coverage
abrahams updated this revision to Diff 605.Aug 25 2015, 6:30 PM
abrahams retitled this revision from to Rewrite KisInputManager.
abrahams updated this object.
abrahams edited the test plan for this revision. (Show Details)
abrahams added reviewers: dkazakov, rempt.
abrahams set the repository for this revision to R8 Calligra.
abrahams added a project: Krita.
abrahams updated this object.Aug 25 2015, 6:48 PM
abrahams updated this revision to Diff 607.Aug 25 2015, 8:03 PM
abrahams removed R8 Calligra as the repository for this revision.
abrahams updated this revision to Diff 608.Aug 25 2015, 8:19 PM
abrahams updated this object.
abrahams updated this object.
abrahams updated this revision to Diff 610.Aug 25 2015, 11:33 PM

I just implemented a very simple "event eater" to toss away every mouse event that immediately follows a tablet event. Now it seems to work in Linux. (Lines are still a little jaggy without smoothing, but I think that's because the linux driver does less smoothing than the Windows one by default...)

Qt 5.3 is supposed to provide a feature to help us avoid this. The function QMouseEvent::source() is supposed to return a nonzero value for fake mousemove events. However it only ever returns a zero value. The documentation punts on responsibility, saying Note: Many platforms provide no such information. On such platforms Qt::MouseEventNotSynthesized is returned always. Does that mean it's not worth pursuing a bug report?

abrahams updated this revision to Diff 611.Aug 26 2015, 1:56 AM
abrahams updated this object.

Actually enable the event eater (=

rempt edited edge metadata.Aug 26 2015, 7:01 AM

Erm... I'm not sure the diff is what it should be? It touches all kinds of stuff, icons, other parts of calligra?

abrahams updated this revision to Diff 613.Aug 26 2015, 12:37 PM
abrahams edited edge metadata.

Darn it. I went to test it in Windows and found a small bug, but when I updated the diffI accidentally pushed my entire collection of WIP patches. This should fix it.

dkazakov edited edge metadata.Aug 28 2015, 9:02 AM

Hi, Michael!

Actually, the approach of "eat one mouse event after a tablet event" may not work. At least on Windows synthesized events come in asynchronous way, that is absolutely independently from the tablet events flow. Therefore we used proximity+enter/leave+focus-in/out events to handle the state of the stylus, whether it is present or not.

abrahams added a comment.EditedAug 28 2015, 1:18 PM
In D270#4915, @dkazakov wrote:

Hi, Michael!

Actually, the approach of "eat one mouse event after a tablet event" may not work. At least on Windows synthesized events come in asynchronous way, that is absolutely independently from the tablet events flow. Therefore we used proximity+enter/leave+focus-in/out events to handle the state of the stylus, whether it is present or not.

Is that still true in Qt 5? I have only tested on my desktop but I am getting one tablet alternating with one corresponding mouse event through my whole log. Or do you mean system-originated synthetic events?

Another option is to alter the MouseEventEater to encapsulate the earlier stylus logic, at least on Windows. (And we can consider all of that to be a temporary workaround while we push to get QMouseEvent::source() implemented.)

Another potential is to go back to the custom Windows event handler including the other patch I have up, which should (I hope!) prevent Qt from generating any events based on fake stylus events.

Actually, the approach of "eat one mouse event after a tablet event" may not work. At least on Windows synthesized events come in asynchronous way, that is absolutely independently from the tablet events flow. Therefore we used proximity+enter/leave+focus-in/out events to handle the state of the stylus, whether it is present or not.

Is that still true in Qt 5? I have only tested on my desktop but I am getting one tablet alternating with one corresponding mouse event through my whole log. Or do you mean system-originated synthetic events?

Most of the drivers generate synthetic events for the mouse cursor alongside and asynchronous to the tablet ones. To the flow of these events Qt adds some converted events. I didn't check how Qt5 handles that though.

Another option is to alter the MouseEventEater to encapsulate the earlier stylus logic, at least on Windows. (And we can consider all of that to be a temporary workaround while we push to get QMouseEvent::source() implemented.)

I guess it would be the safest approach. Is it difficult to do?

abrahams updated this revision to Diff 678.Aug 31 2015, 6:56 PM
abrahams edited edge metadata.

This version alters the event suppression to match with the original logic. It also fixes a problem with the shortcut matcher.

Another option is to alter the MouseEventEater to encapsulate the earlier stylus logic, at least on Windows. (And we can consider all of that to be a temporary workaround while we push to get QMouseEvent::source() implemented.)

I guess it would be the safest approach. Is it difficult to do?

It doesn't seem to introduce any new problems.

One loose end I can think of is compressed move events. Since we don't send KisTabletEvents anymore, I was unable to salvage much of the original handler. I don't know how to produce these events on my machine so I have no test case.

One loose end I can think of is compressed move events. Since we don't send KisTabletEvents anymore, I was unable to salvage much of the original handler. I don't know how to produce these events on my machine so I have no test case.

You mean moveEventCompresor is not used anymore? Or something different?

To reproduce the bug caused by uncompressed tablet events you should use Shift+Gesture for resizing big auto brushes, like 1000px, or do color picking on highly zoomed-out canvas. If the system goes to a shutter, then the proplem is present.

abrahams updated this revision to Diff 690.Sep 2 2015, 1:00 AM

Fix some issues with zoom, resize and pick color

abrahams added a comment.EditedSep 2 2015, 1:07 AM

Hmm... picking a color on a zoomed out canvas works just fine. my system slows down quite a lot when resizing a brush to be very large with shift-click, whether or not this patch is in use.

I was going to mention there's a strange problem with hovering my pen over the tablet and then zooming in with the mouse wheel... but it seems like a Qt bug! (That is, it's messed up whether I use this patch or not, and the tablet event logger shows wacky looking output.)

In D270#5419, @abrahams wrote:

Hmm... picking a color on a zoomed out canvas works just fine. my system slows down quite a lot when resizing a brush to be very large with shift-click, whether or not this patch is in use.

That sounds like the events don't get compressed, which is a bug.

Could you tell me:

  1. You use Qt's tablet support on both Linux and Windows, right?
  2. Is there tablet event compression still active? The slowdown on Shift+Drag usually means it doesn't work.

I cannot test the patch atm, since my Qt5 build refuses to work :(
It would be nice if we could ask someone else to test it on Windows and Linux as well.

rempt added a comment.Sep 4 2015, 9:05 AM

With this patch (but not the tablet patches, not sure which one to apply), trying to use an intuos3 on Linux gives a stream of

krita(3383)/(default) unknown: Unexpected tool event has come to continuePrimaryAction while being mode 0 !

while only the initial dab gets painted. I would like a single, combined, authoritative patch to be applied and tested :-)

abrahams updated this revision to Diff 698.Sep 4 2015, 12:18 PM

Small fix for debugging

Dmitry - thanks for the information. I'm not sure whether event compression is enabled. With this patch I comment out the KisTabletSupport* classes so I think I should be using the default Qt tablet event handler. I'm going to try out a Windows build since I haven't done one in a while, perhaps that may shed more light on the issue.

What is going wrong with your Qt5 build? I only have experience with build errors on Windows but I did see a big spectrum there.

Boudwijn - this patch is supposed to fix tablet support through a refactoring. This patch is working on without warnings my computer (un?)fortunately so I'll have to take a more focused look to figure out the problem. I'm going to look into Dmitry's comments about compressed tablet events, and look at Windows as well. Could you share a tablet events log with the "unexpected tool events" messages in context?