[Kde-games-devel] Review Request: Add a colorOfSprite method to KGameRenderer.

[Stefan Majewsky]

> On 2010-08-21 13:27:06, Stefan Majewsky wrote:
> > The only technical difference between renderer->colorFromSprite(key) and renderer->spritePixmap(key, QSize(3, 3)).toImage().pixel(1, 1) is that the former one is a bit more performant because you save two pixmap conversions, but given the pixmap size and the number of times you'll need to call this method, I consider these negligible.
> > 
> > Therefore, I see no immediate reason to add this to KGameRenderer itself.

Seems like I missed the following sentence at first:

"If we don't add this method to KGR, how can we change KGR to make it possible to implement it in a derived class."

So you basically want a clean, protected API for access to KGR's cache and renderers from a subclass. I will be helpful in designing and implementing such an API if you show me a realistic usecase. KGR's API is designed in a way that tries to make no assumptions about the implementation. API that provides access to the cache and the renderer breaks this design principle, and I will only support this when it is clear that the added value outweighs the potential BC problems. (I'm sorry if I'm sounding harsh, I just want to make my point clear.)


- Stefan


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


On 2010-08-20 13:52:48, Parker Coates wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/5062/
> -----------------------------------------------------------
> 
> (Updated 2010-08-20 13:52:48)
> 
> 
> Review request for KDE Games.
> 
> 
> Summary
> -------
> 
> In both KPat and Killbots, text colours are stored in the SVG as a simple filled rectangle. This system came about during discussions with some of our artists who really didn't like the idea of having to manually type RGB triplets into .desktop files and preferred being able to preview colour combinations live in Inkscape.
> 
> In porting these games to KGameRenderer, I ran into several problems implementing this colour extracting code. Issues included:
>  - not having external access to the/a QSvgRenderer
>  - not having external access to the KImageCache caching the theme details
>  - not having a reliable way to clear any process local cache immediately before or after a theme change.
> 
> Rather than fight for the inclusion of additional (protected?) API, I decided to just implement a colorOfSprite() method directly in KGameRenderer to see what that would look like. The code is almost an exact copy of the boundsOfElement() implementation, so it turned out to be chiefly an exercise in copy/paste/find/replace.
> 
> Taking the selfish view, including this method in KGR would make life easy for me, but I understand if there's objection from others. I'm mostly posting this to start a discussion about it. If we don't add this method to KGR, how can we change KGR to make it possible to implement it in a derived class.
> 
> PS: Ideally, QSvgRenderer would provide brushFromElement() and penFromElement() methods to directly extract these details from the SVG primitives without the hackish step of rendering anything.
> 
> 
> Diffs
> -----
> 
>   trunk/KDE/kdegames/libkdegames/kgamerenderer.h 1164843 
>   trunk/KDE/kdegames/libkdegames/kgamerenderer.cpp 1164843 
>   trunk/KDE/kdegames/libkdegames/kgamerenderer_p.h 1164843 
> 
> Diff: http://reviewboard.kde.org/r/5062/diff
> 
> 
> Testing
> -------
> 
> Tested in KPat and Killbots. Seems to work as expected.
> 
> I also double checked that the word is consistently spelt "color" and not "colour". My fingers type EN_US only begrudgingly. ;)
> 
> 
> Thanks,
> 
> Parker
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kde-games-devel/attachments/20100822/b465b7eb/attachment-0001.htm