[Kde-games-devel] Review Request 117785: fix performance issues

[Thomas Lübking]

> On May 1, 2014, 4:36 p.m., Roney Gomes wrote:
> > I ask you favor, stop doing unnecessary changes to the project's coding style. Although I also don't like it, those changes make your patch more difficult to read.
> > 
> > Consider carefully the issues I pointed bellow. If you have any questions or objections, feel free to expose them.
> > 
> > PS: for some reason I do not understand, the following glitches appeared when I tested your patch. See the screenshot: http://imagebin.org/308512

Don't "worry" - I do neither intend nor suggest to push this patch en block - not even caching sprites and revmoving eventloop recursion belong into one commit.

But please understand that, while working on it, i will boldly continue to simply turn everything i want to understand into readable code and I do not intend to revert those changes until once, after things got finally sorted out, i'll pick the relevant bits into the present code.


I would however suggest to pipe the entire code through astyle and perform one "clenup code" commit, but i don't have to maintain this ;-P


> See the screenshot: http://imagebin.org/308512
"Interesting"
I was about to say "not here", but it seems to depend on the tile size.
My guess is that this is related to the mask detection of the (particular? only tried on egypt theme for the screenshot) ball image. The mask will be too small and the painter clipping in the paint event thus too strict, leaving artifacts between two frames.
My guess is that this unrelated to the actual changes but was simply covered by the continuous full scene repaints.

Either using QGraphicsPixmapItem::HeuristicMaskShape or by applying the brick/grid structure updates on the games background brush/renderBackground() (what would likely also be the best performance choice, but requires re-setting of the brush on level changes) might get rid of that.


> On May 1, 2014, 4:36 p.m., Roney Gomes wrote:
> > wall.cpp, line 160
> > <https://git.reviewboard.kde.org/r/117785/diff/2/?file=269058#file269058line160>
> >
> >     Why these variables need to be static? If they have to be preserved for future calls of update() then store them as private attributes of KBounceWall. Let's try to keep our code as object-oriented as possible.
> >     
> >     Notice that the same argument applies to gs_cachedSize.

You actually what to keep them as static member, i'd say (since they're not required for each wall object - they're always the same), but yes, using a local static is more the interim solution to check for performance gains.


- Thomas


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/117785/#review57064
-----------------------------------------------------------


On April 28, 2014, 7:35 p.m., Thomas Lübking wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/117785/
> -----------------------------------------------------------
> 
> (Updated April 28, 2014, 7:35 p.m.)
> 
> 
> Review request for KDE Games, Ian Wadham and Roney Gomes.
> 
> 
> Repository: kbounce
> 
> 
> Description
> -------
> 
> Roney, I hope your the correct addressee - Ian referecend some "Roney" in a private mail.
> The patch looks bigger than it is, i had to partially fix coding style in order to read through it, sorry.
> 
> The MAJOR issue with the code is that it causes recursions on the eventloop by altering graphicsitems from the paint() call ("setPixmap()"), so i scrathced that.
> It seems a cause was that a full repaint was forced by setting some invisible fullsize pixmap item, while the only requirement for this was actually when finishing a wall - so i replaced that with an explicit full update call on occasion.
> 
> Also all non-raster graphicssystems HATE QPixmap::fill(Qt::transparent) causing a 24bit -> 32bit change and it's esp. bad on the nvidia driver.
> So i reduced that (and allocation) by re-using the old (ARGB) pixmap on a 64x64 alignment.
> Ian, you might esp. check that impact on OSX.
> 
> Last I cached the sprites, seemed reducing cpu on permanent paint (but i didn't really measure)
> 
> I'll also mark & comment the actual changes in the diff.
> 
> 
> Diffs
> -----
> 
>   board.h 75f66d4 
>   board.cpp 46b923b 
>   gameobject.h 9fb5788 
>   gamewidget.cpp 23cb6ba 
>   wall.h c56efa1 
>   wall.cpp df487a0 
> 
> Diff: https://git.reviewboard.kde.org/r/117785/diff/
> 
> 
> Testing
> -------
> 
> 100% cpu -> 1-2% cpu and more fluid =)
> No visible regression spotted.
> 
> 
> Thanks,
> 
> Thomas Lübking
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-games-devel/attachments/20140501/1fbb663c/attachment-0001.html>