<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://svn.reviewboard.kde.org/r/5081/">http://svn.reviewboard.kde.org/r/5081/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On September 5th, 2010, 7:38 p.m., <b>Stefan Majewsky</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="/r/5081/diff/2/?file=34517#file34517line20" style="color: black; font-weight: bold; text-decoration: underline;">/trunk/KDE/kdegames/kblocks/KBlocksSvgItem.cpp</a>
<span style="font-weight: normal;">
(Diff revision 2)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">20</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">setRenderSize</span><span class="p">(</span><span class="n">renderer</span><span class="o">-></span><span class="n">boundsOnSprite</span><span class="p">(</span><span class="n">spriteKey</span><span class="p">).</span><span class="n">size</span><span class="p">().</span><span class="n">toSize</span><span class="p">());</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
<p>On September 9th, 2010, 6:33 a.m., <b>Brian Croom</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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?</pre>
</blockquote>
<p>On September 9th, 2010, 1:17 p.m., <b>Parker Coates</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.
</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
<br />
<p>- Brian</p>
<br />
<p>On August 25th, 2010, 3:42 p.m., Brian Croom wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://svn.reviewboard.kde.orgrb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDE Games, Mauricio Piacentini and Stefan Majewsky.</div>
<div>By Brian Croom.</div>
<p style="color: grey;"><i>Updated 2010-08-25 15:42:18</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">This straightforward patch makes the KBlocks rendering code use KGameRenderer instead of making its own KGameTheme and QSvgRenderer instances</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I have tested playing games and switching themes, and encountered no problems.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>/trunk/KDE/kdegames/kblocks/KBlocksGraphics.h <span style="color: grey">(1167058)</span></li>
<li>/trunk/KDE/kdegames/kblocks/KBlocksGraphics.cpp <span style="color: grey">(1167058)</span></li>
<li>/trunk/KDE/kdegames/kblocks/KBlocksItemGroup.cpp <span style="color: grey">(1167058)</span></li>
<li>/trunk/KDE/kdegames/kblocks/KBlocksScene.cpp <span style="color: grey">(1167058)</span></li>
<li>/trunk/KDE/kdegames/kblocks/KBlocksSvgItem.h <span style="color: grey">(1167058)</span></li>
<li>/trunk/KDE/kdegames/kblocks/KBlocksSvgItem.cpp <span style="color: grey">(1167058)</span></li>
<li>/trunk/KDE/kdegames/kblocks/main.cpp <span style="color: grey">(1167058)</span></li>
</ul>
<p><a href="http://svn.reviewboard.kde.org/r/5081/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>