<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/124511/">https://git.reviewboard.kde.org/r/124511/</a>
</td>
</tr>
</table>
<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/124511/diff/4/?file=394335#file394335line37" style="color: black; font-weight: bold; text-decoration: underline;">src/lib/marble/geodata/graphicsitem/GeoLineStringGraphicsItem.cpp</a>
<span style="font-weight: normal;">
(Diff revision 4)
</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">37</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">outline</span><span class="o">-></span><span class="n">setLatLonAltBox</span><span class="p">(</span><span class="k">this</span><span class="o">-></span><span class="n">latLonAltBox</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;">All these properties might change. As a decoration is some sort of a child item that is created and destroyed by the containing GeoGraphicsItem, it should maintain its properties in sync with the containing item Therefore I'd only create the item here and then have GeoGraphicsItem take care of maintaining them (see comment there). If we remove the special handling of the z-value (see comment in GeometryLayer.cpp), this can be done generically in GeoGraphicsItem.</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/124511/diff/4/?file=394336#file394336line42" style="color: black; font-weight: bold; text-decoration: underline;">src/lib/marble/geodata/graphicsitem/GeoPolygonGraphicsItem.h</a>
<span style="font-weight: normal;">
(Diff revision 4)
</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; ">public:</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">42</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="k">static</span> <span class="k">const</span> <span class="kt">float</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;">not used anymore</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/124511/diff/4/?file=394337#file394337line240" style="color: black; font-weight: bold; text-decoration: underline;">src/lib/marble/geodata/graphicsitem/GeoPolygonGraphicsItem.cpp</a>
<span style="font-weight: normal;">
(Diff revision 4)
</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; ">GeoPolygonGraphicsItem::GeoPolygonGraphicsItem( const GeoDataFeature *feature, const GeoDataLinearRing* ring )</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">229</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">foreach</span><span class="p">(</span><span class="n">QPolygonF</span><span class="o">*</span> <span class="n">polygon</span><span class="p">,</span> <span class="n">polygons</span><span class="p">)</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;">We get further performance improvements by special-casing the fake 3D case again here, restoring the old painter translation approach:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%"><span style="color: #000080; font-weight: bold">diff --git a/src/lib/marble/geodata/graphicsitem/GeoPolygonGraphicsItem.cpp b/src/lib/marble/geodata/graphicsitem/GeoPolygonGraphicsItem.cpp</span>
<span style="color: #000080; font-weight: bold">index 6d6e14e..6b47388 100644</span>
<span style="color: #A00000">--- a/src/lib/marble/geodata/graphicsitem/GeoPolygonGraphicsItem.cpp</span>
<span style="color: #00A000">+++ b/src/lib/marble/geodata/graphicsitem/GeoPolygonGraphicsItem.cpp</span>
<span style="color: #800080; font-weight: bold">@@ -227,11 +227,17 @@ void GeoPolygonGraphicsItem::paint( GeoPainter* painter, const ViewportParams* v</span>
viewport->screenCoordinates(*m_ring, polygons);
}
foreach(QPolygonF* polygon, polygons) {
<span style="color: #A00000">- QPolygonF buildingRoof;</span>
<span style="color: #A00000">- foreach(const QPointF &point, *polygon) {</span>
<span style="color: #A00000">- buildingRoof << point + buildingOffset(point, viewport);</span>
<span style="color: #00A000">+ if (drawAccurate3D) {</span>
<span style="color: #00A000">+ QPolygonF buildingRoof;</span>
<span style="color: #00A000">+ foreach(const QPointF &point, *polygon) {</span>
<span style="color: #00A000">+ buildingRoof << point + buildingOffset(point, viewport);</span>
<span style="color: #00A000">+ }</span>
<span style="color: #00A000">+ painter->drawPolygon(buildingRoof);</span>
<span style="color: #00A000">+ } else {</span>
<span style="color: #00A000">+ QPointF const offset = buildingOffset(polygon->boundingRect().center(), viewport);</span>
<span style="color: #00A000">+ painter->translate(offset);</span>
<span style="color: #00A000">+ painter->drawPolygon(*polygon);</span>
}
<span style="color: #A00000">- painter->drawPolygon(buildingRoof);</span>
}
qDeleteAll(polygons);
}
</pre></div>
</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/124511/diff/4/?file=394339#file394339line160" style="color: black; font-weight: bold; text-decoration: underline;">src/lib/marble/graphicsview/GeoGraphicsItem.cpp</a>
<span style="font-weight: normal;">
(Diff revision 4)
</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">160</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">decoration</span><span class="o">-></span><span class="n">p</span><span class="p">()</span><span class="o">-></span><span class="n">m_isDecoration</span> <span class="o">=</span> <span class="nb">true</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;">I'd sync all relevant properties of the passed item here, i.e. call
<code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">decoration->setVisible(visible());</code> and so on. Also these properties must be synced in the decoration when the containing item changes. The very least we need is</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%"><span style="color: #000080; font-weight: bold">diff --git a/src/lib/marble/graphicsview/GeoGraphicsItem.cpp b/src/lib/marble/graphicsview/GeoGraphicsItem.cpp</span>
<span style="color: #000080; font-weight: bold">index 0ff6af1..e2857a3 100644</span>
<span style="color: #A00000">--- a/src/lib/marble/graphicsview/GeoGraphicsItem.cpp</span>
<span style="color: #00A000">+++ b/src/lib/marble/graphicsview/GeoGraphicsItem.cpp</span>
<span style="color: #800080; font-weight: bold">@@ -42,6 +42,9 @@ bool GeoGraphicsItem::visible() const</span>
void GeoGraphicsItem::setVisible( bool visible )
{
setFlag( ItemIsVisible, visible );
<span style="color: #00A000">+ foreach( GeoGraphicsItem* decoration, p()->m_decorations ) {</span>
<span style="color: #00A000">+ decoration->setVisible( visible );</span>
<span style="color: #00A000">+ }</span>
}
GeoGraphicsItem::GeoGraphicsItemFlags GeoGraphicsItem::flags() const
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Without this decorations are painted even at lower zoom levels when the containing item is invisible, consuming render performance and leading to artifacts.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">Other properties like style, highlight etc. must be forwarded similarly I guess -- whereas I'm not sure about ones like the ItemIsMovable flag.</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/124511/diff/4/?file=394341#file394341line277" style="color: black; font-weight: bold; text-decoration: underline;">src/lib/marble/layers/GeometryLayer.cpp</a>
<span style="font-weight: normal;">
(Diff revision 4)
</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; ">bool GeometryLayer::render( GeoPainter *painter, ViewportParams *viewport,</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">271</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">qSort</span><span class="p">(</span><span class="n">items</span><span class="p">.</span><span class="n">begin</span><span class="p">(),</span> <span class="n">items</span><span class="p">.</span><span class="n">end</span><span class="p">(),</span> <span class="p">[](</span><span class="k">const</span> <span class="n">GeoGraphicsItem</span><span class="o">*</span> <span class="n">a</span><span class="p">,</span> <span class="k">const</span> <span class="n">GeoGraphicsItem</span><span class="o">*</span> <span class="n">b</span><span class="p">)</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;">Extending my previous comment wrt using qStableSort instead of qSort: I wonder if we should really sort the list here. Instead we could replace
<code style="text-rendering: inherit;color: #4444cc;padding: 0;white-space: normal;margin: 0;line-height: inherit;">items << items[i]->decorations();</code>
above with something like</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><div class="codehilite" style="background: #f8f8f8"><pre style="line-height: 125%">foreach ( GeoGraphicsItem* decoration, items[i]->decorations() ) {
items.insert( i, decoration );
}
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">and iterate over items in the outer loop in reverse order (since we insert into it during traversal)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">The benefit is that we do not make any changes to the order except for the added decorations. Also we don't need a special zValue in the decorations, and the runtime here is reduced from O(n log n) due to the sorting to O(n).</p></pre>
</div>
</div>
<br />
<p>- Dennis Nienhüser</p>
<br />
<p>On August 13th, 2015, 12:19 a.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 Aug. 13, 2015, 12:19 a.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;">I changed a little bit the decoration creating method, it now uses a QList to store the decorations, so multiple decorations can be added this way. It was an idea for the street labeling, but it makes sense without that too.</p></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;">It works on a freshly pulled version of Marble.</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/GeoPainter.h <span style="color: grey">(7a757b9)</span></li>
<li>src/lib/marble/GeoPainter.cpp <span style="color: grey">(d04138c)</span></li>
<li>src/lib/marble/GeoPainter_p.h <span style="color: grey">(f0c4f9b)</span></li>
<li>src/lib/marble/MarbleGlobal.h <span style="color: grey">(cf2768f)</span></li>
<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/124511/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>