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

[Stefan Majewsky]

> On 2010-06-13 19:30:21, Parker Coates wrote:
> > trunk/KDE/kdegames/kdiamond/src/staging.h, line 64
> > <http://reviewboard.kde.org/r/4283/diff/2/?file=28399#file28399line64>
> >
> >     My initial thought was "Do we actually need the animation stuff here, or would that be better off moved to a separate subclass, for those games that actually use frame based animations.", but maybe that's overkill. I guess my biggest question is if there is any runtime cost from the inclusion of the frame stuff for applications that don't use them.

There's no measureable runtime cost. If you do not use animated frames, the spritePixmap() method will check "frame == -1" and see that the sprite key does not have to be changed to obtain the SVG element key.


> On 2010-06-13 19:30:21, Parker Coates wrote:
> > trunk/KDE/kdegames/kdiamond/src/staging.h, lines 87-97
> > <http://reviewboard.kde.org/r/4283/diff/2/?file=28399#file28399line87>
> >
> >     Are these methods really necessary if we already have a getter for the KGameTheme? Personally, I could see "convenience" getters like these leading to confusion. I think it'd be better if developer thought of the KGameTheme as the canonical source for all the information contained there.

Good point. I'll remove them.


> On 2010-06-13 19:30:21, Parker Coates wrote:
> > trunk/KDE/kdegames/kdiamond/src/staging.h, lines 99-106
> > <http://reviewboard.kde.org/r/4283/diff/2/?file=28399#file28399line99>
> >
> >     If you're worried about the user deleting the pointer, couldn't you get away with returning references instead? (Also, if the KGameTheme is returned const, the user wouldn't be able to call load on it anyway.)
> >     
> >     Also, presumably users might want access to the cache as well. I know for sure that KPat will need to store custom data in the cache between runs.

I have already changed the second method to return a const pointer. If one wants render(), one can use the spritePixmap() method.

About cache usage: Good point, but I'm unsure about the interface. If the API allows direct access to the cache, we might get into problems once I start to move rendering and cache retrieval to worker threads. (From what I can see from the KSharedDataCache code, it's not thread-safe by itself.)


> On 2010-06-13 19:30:21, Parker Coates wrote:
> > trunk/KDE/kdegames/kdiamond/src/staging.h, lines 120-129
> > <http://reviewboard.kde.org/r/4283/diff/2/?file=28399#file28399line120>
> >
> >     I'm not sure about the use of the word "sprite" here. Presumably these methods will be used for plenty of things that aren't technically sprites.

Hm, then probably my idea of the meaning of the word "sprite" is wrong. I wanted to avoid the word "element" because the elements of KGameRenderer do not exactly correspond to SVG elements if they have multiple frames. Can you come up with a better term?


On 2010-06-13 19:30:21, Stefan Majewsky wrote:
> > The one major thing I see missing from this API that I really would like to see for my games is a means of getting boundsOnElement where the result is store in the cache. In KPat I need to read the aspect ratio of certain elements, so to avoid having to load the SVG every time on start up, I store a dummy pixmap in the KPixmapCache with the appropriate dimensions and then read that back again at start up. It's a hack, but it works well enough.
> > 
> > With the planned port, I was obviously going to take advantage of the genericness of KSharedDataCache and store the QRectF directly, but if this could be implemented directly in KGameRenderer, that would be ideal. I think this would also be of use for games like KBlocks that load their layout directly from the SVG.

A cached interface to QSvgRenderer::boundsOnElement is on the todolist now.


- Stefan


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


On 2010-06-12 12:37:07, Stefan Majewsky wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/4283/
> -----------------------------------------------------------
> 
> (Updated 2010-06-12 12:37:07)
> 
> 
> 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/CMakeLists.txt 1135863 
>   trunk/KDE/kdegames/kdiamond/src/animator.cpp 1135863 
>   trunk/KDE/kdegames/kdiamond/src/board.cpp 1135863 
>   trunk/KDE/kdegames/kdiamond/src/diamond.h 1135863 
>   trunk/KDE/kdegames/kdiamond/src/diamond.cpp 1135863 
>   trunk/KDE/kdegames/kdiamond/src/mainwindow.cpp 1135863 
>   trunk/KDE/kdegames/kdiamond/src/renderer.h 1135863 
>   trunk/KDE/kdegames/kdiamond/src/renderer.cpp 1135863 
>   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
> 
>