Bug 344968

Summary: [Brush-Presets-Docker] flickers and slow down all Krita canvas performances in certain conditions
Product: [Applications] krita Reporter: David REVOY <info>
Component: DockersAssignee: Stefano Bonicatti <smjert>
Status: RESOLVED FIXED    
Severity: normal CC: freebox64, halla, kde, mr.glaceon, smjert
Priority: NOR    
Version: 2.9   
Target Milestone: ---   
Platform: Compiled Sources   
OS: Linux   
Latest Commit: Version Fixed In:
Sentry Crash Report:
Attachments: This patch sets fixed scrollbars to test

Description David REVOY 2015-03-09 09:23:41 UTC
Hi, I found a bug, but very hard to explain. 
I made a 2min videos to explain it : https://share.kde.org/public.php?service=files&t=8995ecbafe76bfc38b68d5f2a5d336ed

it involves this elements :
- resizing UI, Brush preset docker with a Tag list of preset
- Decreasing a lot Krita performances of painting on canvas.
- Random change of the brush outline decoration.

It's also 100% reproducible.
Comment 1 Halla Rempt 2015-03-09 09:30:25 UTC
Hm... It must have something to do with the brush docker relayout constantly. I haven't been able to reproduce the flicker yet, but that might just be because my desktop is too fast to show it. As to whether it's fixable -- it might even be a Qt issue. Or something weird in our layout code in the resource chooser.
Comment 2 mvowada 2015-03-10 11:54:29 UTC
I can confirm both: the flickering of the UI, and the slowdowns of the brushes. It seems that the issue doesn't show up with the Favorite presets tag. (Ubuntu 14.04 Unity - Krita 2.9.1).
Comment 3 mvowada 2015-03-10 11:58:12 UTC
...well, actually it shows up also using the "Favorite Presets" tag if many brush presets are present.
Comment 4 Moritz Molch 2015-03-12 13:14:15 UTC
Created attachment 91542 [details]
This patch sets fixed scrollbars to test

I spent some hours yesterday evening to try to tackle this issue down. I'm not entirely sure whether it is the cause or simply a symptom but it might be that the problem lies in updateView() in KoResourceItemView. It calculates the width of the columns from the viewports width and then sets them. However, when the scrollbars are set to ScrollBarAsNeeded, they don't know what to do and there seems to be some looping going on because when the scrollbars are showing themselves they cause a resize that in turn calls updateView() again etc.
This patch hides the horizontal scrollbar and always shows the vertical one when using fixed columns for testing purposes. Unfortunately I wasn't able to reproduce the slowdown issue but it might be well worth it to test the patch if you are affected.
To make a proper fix would require some time, but it would be good to know to anyone who wants to fix it whether the patch goes into the right direction.
Comment 5 Halla Rempt 2015-03-12 13:16:14 UTC
Yes, that sounds very likely.
Comment 6 David REVOY 2015-03-12 14:22:49 UTC
Hi Mmolch, 
I tested the patch, and here is the report :
1. I could still resize the brush preset docker to make it flickers.
2. But the performances/big slowdown is now not as dramatic as in calligra/2.9 :  It's slow but the stroke end and no missing end-tail, or brush decoration outline changing to another brush. So, I notice an improvement.
3. A side effect:  Krita main windows could not be resized in my WM ( on XFCE right now ), just maximised or the windowed floating mode was locked in a mini position. 
I reverted the patch after test.
Comment 7 Halla Rempt 2015-06-06 07:41:49 UTC
*** Bug 348778 has been marked as a duplicate of this bug. ***
Comment 8 Stefano Bonicatti 2015-06-08 08:56:43 UTC
I've compiled Krita and calligra with -O0 and -g, then i've put a kRealBacktrace inside KoResourceItemView::updateView and that helped a lot pinpointing the issue.
What i saw is that alternately there was a call path coming from KoResourceItemChooser::baseLengthChanged and one coming from KoResourceItemView::resizeEvent.
A third one, but that was not repeating so much times indefinitely, but basicly only when actually resizing by hand, comes from  KoResourceItemChooser::setSynced, called by KisPresetChooser::resizeEvent.
Now KoResourceItemChooser::baseLengthChanged is called by KoResourceItemChooser::updateView which is connected to the signal sigSizeChanged which is emitted by KoResourceItemView::resizeEvent, as you can see here then we have already 2 calls of updateView, happening together, that comes from the resizeEvent (one through a signal, one through a direct call).
Now given that the profiler shows a high amount of time spent in viewportEvent (because it end up calling the scaling of the images of the brushes) and that updateView (of the Ko[...]View) when calling setColumnWidth or setRowWidth end up generating a new viewportEvent (the stack traces always show that
Comment 9 Stefano Bonicatti 2015-06-08 10:49:42 UTC
Err while assigning to myself this issue i didn't saw i had a comment half wrote from yesterday.
Disregard that comment... a better explanation can be given by simply checking, in updateView, the rowHeight and columnWidth, current and desired, and when the scrollbar appears.
mmolch basically said it, as soon as scrollbar appears, updateView code tries to fit the tiles horizontally, by changing the columnWidth and the rowHeight (because tiles are squares), but this tiles height reduction is enough to not need the scrollbars anymore, so they hide and cause a viewport expansion in width.. which causes an expansion of tiles (width and height), and again the tiles height is too much to fit in the viewport, so the scrollbar appears again and the loop continues.

Now i see only two ways of fixing this:
1) mmolch way, always hide the horizontal bar (this isn't needed anyway because it should always fit horizontally) and show the vertical bar so that no width resize happens due to scrollbar appearing.
2) Always leave some margin on the right side to account for scrollbars... even if this is a bit ugly (tiles not perfectly fitting) and maybe difficult to pre-calculate the expected scrollbar size.

In the end the cleanest way for me is 1, though i don't get why David still gets flicker, can you make a short video with that issue and the patch applied + the main window resizing issue you have?
Because when i tested i had no more slowdowns and i was able to resize the main windows just fine.
Comment 10 David REVOY 2015-06-08 11:19:47 UTC
>> In the end the cleanest way for me is 1
Ok , I suggest to push the patch in 2.9 and master to ease further test. ( It can't hurt afaik )  It will be easier for me to see how the docker behaves on a week of painting and this way I can keep track of other commit too. As soon as I can reproduce, I'll screencapture.
Comment 11 Stefano Bonicatti 2015-06-08 14:51:29 UTC
The problem is that the patch is in Ko.. class, that means that comes from calligra and has to be reviewed first.
So i can open the review and prepare a branch with it but "nothing else".
Comment 12 Stefano Bonicatti 2015-06-08 17:37:29 UTC
https://git.reviewboard.kde.org/r/124042/ here is the review and krita-presetdockerflickering-bonicatti is the branch to test the patch.
Comment 13 mvowada 2015-06-09 14:30:16 UTC
Hello,
and thank you for the patch. It works for me on Ubuntu 14.04 - Unity.

As an aside, I think that it could possibly solve a little issue currently affecting the Palette docker with a large number of swatches.

Steps:
1. Ctrl+mouse-wheel over the swatches to enlarge their sizes and so adjusting the total width of the rows to fit to the opening (without triggering the horizontal scrollbar).
2. Click-and-drag the top or the bottom edge of the Palette docker, to reduce the height of the docker so to trigger the vertical scrollbars.

Result:
The swatches are not resized to fit to the new width of the opening and the horizontal scrollbar appears.

Hope you don't mind for this digression. Thanks
Comment 14 Stefano Bonicatti 2015-06-20 08:57:02 UTC
Git commit 2c592c5872a9e35e0b4c17a1c3a97da02a183c89 by Stefano Bonicatti.
Committed on 20/06/2015 at 08:53.
Pushed by stefanobonicatti into branch 'calligra/2.9'.

Fix preset docker flickering and slowdown

When vertically resizing the preset docker and the vertical
scrollbar appears, the preset tiles are resized to fit the
viewport with their width, but this also reduces their height
not needing the scrollbar anymore.
The scrollbar disappearing lets the viewport expand and so the tiles,
in height, making the scrollbar needed again and this loops forever
generating a considerable slowdown.
The fix consists in always showing the vertical scrollbar so that no
resizing happens due to them.

To avoid unnecessary work and event spam we check in
KoResourceItemChooser::setSynced function if the value passed
corresponds to its current status already.
REVIEW: 124042

Thanks to mmolch for the initial patch.

M  +3    -0    libs/widgets/KoResourceItemChooser.cpp
M  +15   -1    libs/widgets/KoResourceItemView.cpp

http://commits.kde.org/calligra/2c592c5872a9e35e0b4c17a1c3a97da02a183c89
Comment 15 David REVOY 2015-06-23 19:22:05 UTC
Hey, I updated my install of calligra/2.9,  and I noticed I can't reproduce this bug anymore. Thank you!
Comment 16 David REVOY 2015-07-15 16:20:52 UTC
A bit of bug triage here on my threads , this one have to be marked as 'RESO' too  :3