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

[Parker Coates]

> 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.
> 
> Stefan Majewsky wrote:
>     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.)

What are the remaining use cases for allowing direct access to the SVG renderer? If boundsOnElement is added to the KGameRenderer, what other things are people going to want to do? The QSvgRenderer API really isn't that big.

By the way, KSvgRenderer is now deprecated. You should be using QSvgRenderer. (KSvgRenderer only ever added one feature on top of QSR; support for loading compressed SVGZ files and this has long since been added to QSvgRenderer.)

As for cache access, really all that anyone would ever want for storing custom data are the find() and insert() methods. As for multithreading and caching, do keep in mind that QPixmaps *cannot* be used outside the main thread, so it's unlikely that you'll ever be accessing the cache from more than one thread.


> 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.
> 
> Stefan Majewsky wrote:
>     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?

Hmm. After a bit of research, it seems sprite has many definitions, so I don't think either of us is actually wrong. :) It was mostly with respect to static background/foreground elements that the term sprite felt out of place to me, but by some definitions these can be considered sprites and I can't think of any better term. (I was going to suggest "element", I hadn't considered that with multiple frames there isn't a one to one mapping.)


> 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.
> 
> Stefan Majewsky wrote:
>     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.

That seems reasonable enough.


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.
> 
> Stefan Majewsky wrote:
>     A cached interface to QSvgRenderer::boundsOnElement is on the todolist now.

Excellent.


- Parker


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


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
> 
>