[Kde-games-devel] Review Request: Port KBlocks to KGameRenderer

[Brian Croom]

> On 2010-09-05 19:38:53, Stefan Majewsky wrote:
> > /trunk/KDE/kdegames/kblocks/KBlocksSvgItem.cpp, line 20
> > <http://svn.reviewboard.kde.org/r/5081/diff/2/?file=34517#file34517line20>
> >
> >     This is not correct. You're setting the render size in SVG coordinates, not in view coordinates. You'll need to adjust the render sizes to the sizes in view coordinates in resize events.
> >     
> >     Alternatively, you can use the primaryView function in KGameRenderedObjectItem (KGameRenderer::setDefaultPrimaryView would be your friend then, but this works only if you instantiate the view before any items).
> >     
> >     It's admittedly not easy to choose between these possibilities (number one involves writing a ton of boring boilerplate, while number two is deprecated because of its potentially bad performance and its surely bad API design). The real solution would be to use Tagaro::Board, which is designed for exactly this usecase, but Tagaro is far away from being ready for primetime.
> 
> Brian Croom wrote:
>     I don't understand why this is necessarily incorrect. For one thing, I believe this is equivalent to how the current version works, with QGraphicsSvgItem's whose bounding rects and transformation matrices are never changed from their defaults. Secondly, it does work properly this way with both of the currently available themes. Is your concern that a new theme might structure the SVG differently so this breaks?
>     
>     The key to this working is that the scene is never resized in KBlocks. When the view gets resize events it just modifies its view matrix using fitInView while the scene remains untouched.
>     
>     Since doing it this way works and isn't any more broken than the current code, can we just do it this way at least until the Tagaro classes are a viable solution?
> 
> Parker Coates wrote:
>     The real question is whether "views().first().mapFromScene( item->sceneBoundingRect() ).size().toSize() == item->pixmap()->size()". If it does not, then that means some pixmap transformations are happening which are both expensive and ugly.
>

The answer to that would be no, except in the case where the view size matches the scene size. That seems to be a decision made explicitly at some point. The code contains e.g. handles for resize events for modifying scene contents, but they are all commented out in favor of modifying the view transformation and keeping the scene size static.


- Brian


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


On 2010-08-25 15:42:18, Brian Croom wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/5081/
> -----------------------------------------------------------
> 
> (Updated 2010-08-25 15:42:18)
> 
> 
> Review request for KDE Games, Mauricio Piacentini and Stefan Majewsky.
> 
> 
> Summary
> -------
> 
> This straightforward patch makes the KBlocks rendering code use KGameRenderer instead of making its own KGameTheme and QSvgRenderer instances
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdegames/kblocks/KBlocksGraphics.h 1167058 
>   /trunk/KDE/kdegames/kblocks/KBlocksGraphics.cpp 1167058 
>   /trunk/KDE/kdegames/kblocks/KBlocksItemGroup.cpp 1167058 
>   /trunk/KDE/kdegames/kblocks/KBlocksScene.cpp 1167058 
>   /trunk/KDE/kdegames/kblocks/KBlocksSvgItem.h 1167058 
>   /trunk/KDE/kdegames/kblocks/KBlocksSvgItem.cpp 1167058 
>   /trunk/KDE/kdegames/kblocks/main.cpp 1167058 
> 
> Diff: http://svn.reviewboard.kde.org/r/5081/diff
> 
> 
> Testing
> -------
> 
> I have tested playing games and switching themes, and encountered no problems.
> 
> 
> Thanks,
> 
> Brian
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/kde-games-devel/attachments/20100910/4057208a/attachment.htm