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

[Brian Croom]

> On 2010-09-05 19:38:53, Stefan Majewsky wrote:
> > I finally test-compiled. Everything looks fine, except for a major problem which hinders me from committing the patch:

Hey Stefan, thanks for the review.

Unrelated to this patch, I realized that the "Pause" action is enabled when KBlocks is started even though there is no game started. Trying to pause the non-existent game causes a crash. Do you want to take care of fixing that yourself or should I open a new review request with a patch for it?


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

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?


- 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/20100909/a3168cdd/attachment-0001.htm