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

[Thomas Lübking]

> On April 27, 2014, 2:38 p.m., Roney Gomes wrote:
> > Despite the issues highlighted below, your patch looks OK to me. Let's wait for Ian's next comments and see how things behave on OSX.
> 
> Thomas Lübking wrote:
>     There's a pending issue about caching the sprites - the cache must be cleared on theme changes
>     (just in case: Ian somehow dragged me into this - i had not installed any games nor played kbounce before ;-)
> 
> Ian Wadham wrote:
>     Hey! Hey! Not guilty ... :-) I was just using KBounce as the simplest test-bed for methods to get around Qt Mac's reluctance to use raster graphics and you were helping me get to grips with that problem, Thomas. And I am very grateful for that help. It will fix a whole slew of graphics problems in KDE apps on Apple OS X at one go and will improve the look of KDE apps in general on that platform.
>     
>     For the record, Roney was my GSoC student in 2012 and KBounce was the first game we ported to newer graphics and sound libraries. Roney is now the maintainer of KBounce. We both got into a mess with porting KBounce, largely because the old game was using some methods that happened to have the same names as QGV methods, but different intent of course. That produced some unbelievable graphics glitches. Time was running on and I suggested we leave the QPainter usage as it was, rather than finish the job by operating the pixmap items through QGraphicsScene. But how did redraw() become paint(...) and introduce the unintended recursion?
> 
> Thomas Lübking wrote:
>     Not complaining either :)
>     I just wanted to point out that I've limited experience on the game (well, "had" ;-)
>     
>     > But how did redraw() become paint(...) and introduce the unintended recursion?
>     I don't know, but ::setPixmap() alters the scene and that causes an update in the recursion, so it must not be called (unconditionally) from the paint cycle.
>     (It updates on the next eventcycle so that you can "foo->setPixmap(bar); bar->setPixmap(foo);" and only get one update of course.)
> 
> Ian Wadham wrote:
>     > There's a pending issue about caching the sprites - the cache must be cleared on theme changes
>     
>     Not sure what you meant by this, Thomas, but AFAIK the caches in all KDE Games accumulate themed pixmaps and different sized pixmaps of the same theme until a file-size limit is reached. This makes theme changes and resizes get faster as time goes on. We used to hand-code all that, but now libkdegames KGameRenderer does it.

I placed a local static cache Pixmap to avoid creating pixmaps - i however also looked up the sprite code and the size parameter seems completely ignored (for now) so it basically just prevents the (at 60Hz and for w*h tiles, though ...) dictionary lookup and the additional CPU drop probably simply stems from there.


- Thomas


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


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/20140429/bd73d154/attachment-0001.html>