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

[Parker Coates]

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


Sorry, I haven't really had time to do a full review, but I have gone through the header and come up with some questions. Since I promised to do a review if you put it up on ReviewBoard, I thought I might as well publish what I have instead of waiting until I can do a full review. Also, at the rate you're progressing, I wouldn't be surprised much of the implementation shown here has already changed.


trunk/KDE/kdegames/kdiamond/src/staging.h
<http://reviewboard.kde.org/r/4283/#comment5693>

    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.



trunk/KDE/kdegames/kdiamond/src/staging.h
<http://reviewboard.kde.org/r/4283/#comment5694>

    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.



trunk/KDE/kdegames/kdiamond/src/staging.h
<http://reviewboard.kde.org/r/4283/#comment5696>

    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.



trunk/KDE/kdegames/kdiamond/src/staging.h
<http://reviewboard.kde.org/r/4283/#comment5695>

    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.


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.

- Parker


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