<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="12" style="border: 1px #c9c399 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/124154/">https://git.reviewboard.kde.org/r/124154/</a>
</td>
</tr>
</table>
<br />
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Looks lovely. I also tested it on my system and changed the animation rendering quality to high to see the view-dependent shadows change while dragging. Awesome :)</p></pre>
<br />
<div>
<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="https://git.reviewboard.kde.org/r/124154/diff/1/?file=381164#file381164line85" style="color: black; font-weight: bold; text-decoration: underline;">src/lib/marble/geodata/data/GeoDataFeature.h</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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; ">class GEODATA_EXPORT GeoDataFeature : public GeoDataObject</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#e9eaa8" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2">85</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">SmallCountyCapital</span><span class="p">,</span><span class="ew"> </span></pre></td>
<th bgcolor="#e9eaa8" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">85</font></th>
<td bgcolor="#fdfebc" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">SmallCountyCapital</span><span class="p">,</span></pre></td>
</tr>
</tbody>
</table>
<div style="margin-left: 2em;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">If it doesn't take too much time, please split off the changes in this patch which clean up whitespace and commit them directly to master. That way the real changes are easier to spot (here and in the future when looking at the git history).</p></pre>
</div>
</div>
<br />
<div>
<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="https://git.reviewboard.kde.org/r/124154/diff/1/?file=381170#file381170line181" style="color: black; font-weight: bold; text-decoration: underline;">src/lib/marble/geodata/graphicsitem/GeoPolygonGraphicsItem.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</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; ">const GeoDataLatLonAltBox& GeoPolygonGraphicsItem::latLonAltBox() const</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">179</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">translate</span><span class="p">.</span><span class="n">setX</span><span class="p">(</span> <span class="p">(</span><span class="n">translate</span><span class="p">.</span><span class="n">x</span><span class="p">()</span> <span class="o">/</span> <span class="p">(</span><span class="n">viewport</span><span class="o">-></span><span class="n">width</span><span class="p">()</span> <span class="o">/</span> <span class="mi">2</span><span class="p">))</span> <span class="o">*</span> <span class="n">s_maximumOffset</span> <span class="p">);</span></pre></td>
</tr>
</tbody>
</table>
<div style="margin-left: 2em;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Looks great most times. For large viewports and small buildings the offset seems a bit high to me though, maybe tweak it a bit for that case?</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">One way could be to take the screen size of the polygon into account.</p></pre>
</div>
</div>
<br />
<div>
<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="https://git.reviewboard.kde.org/r/124154/diff/1/?file=381172#file381172line162" style="color: black; font-weight: bold; text-decoration: underline;">src/lib/marble/graphicsview/GeoGraphicsItem.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<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">162</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">return</span> <span class="n">p</span><span class="p">()</span><span class="o">-></span><span class="n">m_isDecoration</span><span class="p">;</span></pre></td>
</tr>
</tbody>
</table>
<div style="margin-left: 2em;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Is there a situation where m_isDecoration is not the same as (m_cachedDecoration != nullptr)? Otherwise I'd favor returning that here and spare the m_isDecoration member (to avoid redundancy).</p></pre>
</div>
</div>
<br />
<p>- Dennis Nienhüser</p>
<br />
<p>On June 23rd, 2015, 12:01 p.m. UTC, Dávid Kolozsvári wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="12" style="border: 1px #888a85 solid; border-radius: 6px; -moz-border-radius: 6px; -webkit-border-radius: 6px;">
<tr>
<td>
<div>Review request for Marble.</div>
<div>By Dávid Kolozsvári.</div>
<p style="color: grey;"><i>Updated June 23, 2015, 12:01 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
marble
</div>
<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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This patch introduces a new virtual method <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">GeoGraphicsItem::createDecoration()</code> to enable the easy creation of decoration for every class thet is inherited from <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">GeoGraphicsItem</code>.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">There are two new decorations implemented in this patch:</p>
<ul style="padding: 0;text-rendering: inherit;margin: 0 0 0 1em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Outlines for <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">GeoLineStringGraphicsItem</code>, for better street rendering in OSM vector tiles;</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Fake3D effect for <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">GeoPolygonGraphicsItem</code> if they represent buildings in OSM vector tiles.</li>
</ul>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">For performance reasons these decorations are only painted when the map quality is set to noraml or higher.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Other changes:</p>
<ul style="padding: 0;text-rendering: inherit;margin: 0 0 0 1em;line-height: inherit;white-space: normal;">
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Changed some of the color styles of OSM related items, e.g. streets, to match the default color scheme of <a href="http://www.openstreetmap.org/" style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">openstreetmap maps</a>;</li>
<li style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">Implemented two new OSM feature: <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">HighwayFootway</code> and <code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">HighwayCycleway</code>.</li>
</ul></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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">In the end, I've decided to cache the decorations for every item, and thus the performance is much better than in the <a href="https://git.reviewboard.kde.org/r/124074/" style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;">previous review request</a>.
This patch works for me and the results are better than I expected.</p></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>src/lib/marble/geodata/data/GeoDataFeature.h <span style="color: grey">(ea23cd8)</span></li>
<li>src/lib/marble/geodata/data/GeoDataFeature.cpp <span style="color: grey">(6f330fb)</span></li>
<li>src/lib/marble/geodata/data/GeoDataFeature_p.h <span style="color: grey">(496c356)</span></li>
<li>src/lib/marble/geodata/graphicsitem/GeoLineStringGraphicsItem.h <span style="color: grey">(4842809)</span></li>
<li>src/lib/marble/geodata/graphicsitem/GeoLineStringGraphicsItem.cpp <span style="color: grey">(4320c07)</span></li>
<li>src/lib/marble/geodata/graphicsitem/GeoPolygonGraphicsItem.h <span style="color: grey">(f469dfb)</span></li>
<li>src/lib/marble/geodata/graphicsitem/GeoPolygonGraphicsItem.cpp <span style="color: grey">(81cfe9a)</span></li>
<li>src/lib/marble/graphicsview/GeoGraphicsItem.h <span style="color: grey">(4ca4727)</span></li>
<li>src/lib/marble/graphicsview/GeoGraphicsItem.cpp <span style="color: grey">(b8fa693)</span></li>
<li>src/lib/marble/graphicsview/GeoGraphicsItem_p.h <span style="color: grey">(01becfc)</span></li>
<li>src/lib/marble/layers/GeometryLayer.cpp <span style="color: grey">(9eb3f50)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/124154/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>