[Kde-games-devel] Review Request: New classes KGameRenderer and KGameRenderedItem

[Stefan Majewsky]

> On 2010-06-14 17:36:46, Parker Coates wrote:
> > trunk/KDE/kdegames/kdiamond/src/staging.h, lines 161-162
> > <http://reviewboard.kde.org/r/4283/diff/3/?file=28570#file28570line161>
> >
> >     I think it would probably make sense to have a setSpriteKey(QString) method.

Can be added, yes. At the moment, the API is pretty much limited to what was needed ATM. It will generally be expanded wherever appropriate.


> On 2010-06-14 17:36:46, Parker Coates wrote:
> > trunk/KDE/kdegames/kdiamond/src/staging.cpp, lines 32-36
> > <http://reviewboard.kde.org/r/4283/diff/3/?file=28571#file28571line32>
> >
> >     Hardcoding the cache name based on the app name could be a problem for games like KGoldrunner that use multiple SVGs. I'm not really sure what the best way of handling that would be.

One could allow multiple KGameRenderer instances by distinguishing caches with an additional QString argument.


> On 2010-06-14 17:36:46, Parker Coates wrote:
> > trunk/KDE/kdegames/kdiamond/src/staging.cpp, lines 101-103
> > <http://reviewboard.kde.org/r/4283/diff/3/?file=28571#file28571line101>
> >
> >     This is just a matter of personal style, but I'm curious of why you don't just use a foreach loop.

When I was working on a cellular automaton engine (a private pet project), I could increase the iteration performance ten-fold by replacing foreach by const_iterator loops (from 10fps to 100fps). That was some kind of epiphany for me, so now I avoid foreach when the loop is likely going over very much elements or called very often.


> On 2010-06-14 17:36:46, Parker Coates wrote:
> > trunk/KDE/kdegames/kdiamond/src/staging.cpp, lines 335-338
> > <http://reviewboard.kde.org/r/4283/diff/3/?file=28571#file28571line335>
> >
> >     Since m_item is never used, I'm assuming this code should be removed?

That was a test to replace "class KGameRenderedItem : public QObject, public QGraphicsPixmapItem" by "class KGameRenderedItem : public QGraphicsObject" with an internal QGraphicsPixmapItem child.


> On 2010-06-14 17:36:46, Parker Coates wrote:
> > trunk/KDE/kdegames/kdiamond/src/staging_p.h, lines 44-46
> > <http://reviewboard.kde.org/r/4283/diff/3/?file=28572#file28572line44>
> >
> >     From the API docs it seems that KImageCache already contains an in-memory, process-local cache. Why add another layer on top of it?

Seems like I overlooked this. I will look into using this instead.


- Stefan


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/4283/#review6114
-----------------------------------------------------------


On 2010-06-13 20:54:58, Stefan Majewsky wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/4283/
> -----------------------------------------------------------
> 
> (Updated 2010-06-13 20:54:58)
> 
> 
> Review request for KDE Games.
> 
> 
> Summary
> -------
> 
> KGameRenderer is a small SVG->PNG rendering framework for use in KDE games. For the story behind it, see http://kde.markmail.org/message/plfkwfeoni6nvpk2
> 
> This diff reflects the current state of my local work branch: KGameRenderer is in a single source file inside the KDiamond source tree. I am in the process of porting KDiamond to KGameRenderer alongside writing KGameRenderer for testing reasons.
> 
> The relevant code is in the files "staging(-p)?.(h|cpp)". Eventually, KGameRenderer shall be moved inside libkdegames. I will take care of porting all applications in the kdegames module to KGameRenderer where it is fruitful.
> 
> 
> Diffs
> -----
> 
>   trunk/KDE/kdegames/kdiamond/src/staging.h PRE-CREATION 
>   trunk/KDE/kdegames/kdiamond/src/staging.cpp PRE-CREATION 
>   trunk/KDE/kdegames/kdiamond/src/staging_p.h PRE-CREATION 
> 
> Diff: http://reviewboard.kde.org/r/4283/diff
> 
> 
> Testing
> -------
> 
> KDiamond still works, and even feels a bit faster although I have yet to fully take advantage of KGameRenderer's facilities.
> 
> 
> Thanks,
> 
> Stefan
> 
>