<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 />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On August 9th, 2015, 8:49 nachm. UTC, <b>Dennis Nienhüser</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">In https://git.reviewboard.kde.org/r/124154/ I suggested to change the algorithm to compute the offsets to avoid that they get too large for small buildings. Looking at the result of that I realized that my suggestion was a bad one: We now have offsets that vary quite a lot and give the impression of different building heights that altogether look a bit strange. So I sat down to come up with a better suggestion, and while playing with it ran into other issues with the current approach. Here's a screenshot that shows what I mean (left image part):</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><img alt="3D effect issues" src="http://nienhueser.de/marble/marble-3d-building-comparison.png" style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" />
- A is the result of my bad suggestion: The offset now depends on the area of the polygon, which makes no real sense
- B is a rendering placement error. The decoration resembles the walls, which are based on the floor, while the polygon itself resembles the roof of the building. To achieve the 3D-effect, the roof needs to be moved, not the walls/floor.
- C is the result of the "cheap" rendering where we do not really calculate the walls, but just a shifted polygon. This is the hardest one to fix.</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">To fix A, we can use a more realistic offset calculation. A good approach is to calculate the height (in pixels) of the virtual camera observing the scene, relate it to the (assumed) height of the building (again in pixels) and calculate the wall size from that. Fixing B is simpler, we just need to shift the painter of the "real" polygon instead of the decoration polygon. Fixing C is the hardest part and requires us to calculate all visible building walls as polygons and paint them. Here is a patch that does all that:</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 0e0f881..e4321a7 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">@@ -41,6 +41,10 @@ const float GeoPolygonGraphicsItem::s_maximumOffset = 8;</span>
void GeoPolygonGraphicsItem::createDecorations()
{
<span style="color: #00A000">+ if (isDecoration()) {</span>
<span style="color: #00A000">+ return;</span>
<span style="color: #00A000">+ }</span>
<span style="color: #00A000">+</span>
GeoPolygonGraphicsItem *fake3D;
switch ( feature()->visualCategory() ) {
<span style="color: #800080; font-weight: bold">@@ -104,6 +108,28 @@ void GeoPolygonGraphicsItem::createDecorations()</span>
this->addDecoration(fake3D);
}
<span style="color: #00A000">+QPointF GeoPolygonGraphicsItem::buildingOffset(const QPointF &point, const ViewportParams *viewport) const</span>
<span style="color: #00A000">+{</span>
<span style="color: #00A000">+ qreal const cameraFactor = 0.5 * tan(0.5 * 110 * DEG2RAD);</span>
<span style="color: #00A000">+ qreal const buildingHeightMeter = 8.0;</span>
<span style="color: #00A000">+ qreal const buildingFactor = buildingHeightMeter / EARTH_RADIUS;</span>
<span style="color: #00A000">+</span>
<span style="color: #00A000">+ qreal const cameraHeightPixel = viewport->width() * cameraFactor;</span>
<span style="color: #00A000">+ qreal const buildingHeightPixel = viewport->radius() * buildingFactor;</span>
<span style="color: #00A000">+</span>
<span style="color: #00A000">+ qreal const offsetX = point.x() - viewport->width() / 2.0;</span>
<span style="color: #00A000">+ qreal const alpha1 = atan2(offsetX, cameraHeightPixel);</span>
<span style="color: #00A000">+ qreal const alpha2 = atan2(offsetX, cameraHeightPixel-buildingHeightPixel);</span>
<span style="color: #00A000">+ qreal const shiftX = 2 * (cameraHeightPixel-buildingHeightPixel) * sin(0.5*(alpha2-alpha1));</span>
<span style="color: #00A000">+</span>
<span style="color: #00A000">+ qreal const offsetY = point.y() - viewport->height() / 2.0;</span>
<span style="color: #00A000">+ qreal const beta1 = atan2(offsetY, cameraHeightPixel);</span>
<span style="color: #00A000">+ qreal const beta2 = atan2(offsetY, cameraHeightPixel-buildingHeightPixel);</span>
<span style="color: #00A000">+ qreal const shiftY = 2 * (cameraHeightPixel-buildingHeightPixel) * sin(0.5*(beta2-beta1));</span>
<span style="color: #00A000">+</span>
<span style="color: #00A000">+ return QPointF(shiftX, shiftY);</span>
<span style="color: #00A000">+}</span>
<span style="color: #00A000">+</span>
const GeoDataLatLonAltBox& GeoPolygonGraphicsItem::latLonAltBox() const
{
<span style="color: #800080; font-weight: bold">@@ -118,9 +144,9 @@ const GeoDataLatLonAltBox& GeoPolygonGraphicsItem::latLonAltBox() const</span>
void GeoPolygonGraphicsItem::paint( GeoPainter* painter, const ViewportParams* viewport )
{
<span style="color: #A00000">- Q_UNUSED( viewport );</span>
<span style="color: #A00000">-</span>
painter->save();
<span style="color: #00A000">+ bool const isBuildingFrame = isDecoration();</span>
<span style="color: #00A000">+ bool const isBuildingRoof = !decorations().isEmpty();</span>
QPen currentPen = painter->pen();
<span style="color: #800080; font-weight: bold">@@ -128,7 +154,7 @@ void GeoPolygonGraphicsItem::paint( GeoPainter* painter, const ViewportParams* v</span>
painter->setPen( QPen() );
}
else {
<span style="color: #A00000">- if ( !style()->polyStyle().outline() || isDecoration() ) {</span>
<span style="color: #00A000">+ if ( !style()->polyStyle().outline() || isBuildingFrame ) {</span>
currentPen.setColor( Qt::transparent );
}
else {
<span style="color: #800080; font-weight: bold">@@ -143,13 +169,6 @@ void GeoPolygonGraphicsItem::paint( GeoPainter* painter, const ViewportParams* v</span>
if ( currentPen.style() != style()->lineStyle().penStyle() )
currentPen.setStyle( style()->lineStyle().penStyle() );
<span style="color: #A00000">-</span>
<span style="color: #A00000">- if ( painter->mapQuality() != Marble::HighQuality</span>
<span style="color: #A00000">- && painter->mapQuality() != Marble::PrintQuality ) {</span>
<span style="color: #A00000">- QColor penColor = currentPen.color();</span>
<span style="color: #A00000">- penColor.setAlpha( 255 );</span>
<span style="color: #A00000">- currentPen.setColor( penColor );</span>
<span style="color: #A00000">- }</span>
}
if ( painter->pen() != currentPen )
<span style="color: #800080; font-weight: bold">@@ -160,7 +179,7 @@ void GeoPolygonGraphicsItem::paint( GeoPainter* painter, const ViewportParams* v</span>
painter->setBrush( QColor( Qt::transparent ) );
}
else {
<span style="color: #A00000">- if ( isDecoration() ) {</span>
<span style="color: #00A000">+ if ( isBuildingFrame ) {</span>
painter->setBrush( style()->polyStyle().paintedColor().darker(150) );
} else if ( painter->brush().color() != style()->polyStyle().paintedColor() ) {
painter->setBrush( style()->polyStyle().paintedColor() );
<span style="color: #800080; font-weight: bold">@@ -168,36 +187,58 @@ void GeoPolygonGraphicsItem::paint( GeoPainter* painter, const ViewportParams* v</span>
}
}
<span style="color: #A00000">- if ( isDecoration() ) {</span>
<span style="color: #A00000">- qreal polygonCenterX, polygonCenterY, cameraX, cameraY,</span>
<span style="color: #A00000">- polygonLeft, polygonTop;</span>
<span style="color: #A00000">- viewport->screenCoordinates( latLonAltBox().center(), polygonCenterX, polygonCenterY );</span>
<span style="color: #A00000">- viewport->screenCoordinates( viewport->focusPoint(), cameraX, cameraY );</span>
<span style="color: #A00000">- viewport->screenCoordinates( latLonAltBox().west(), latLonAltBox().north(),</span>
<span style="color: #A00000">- polygonLeft, polygonTop);</span>
<span style="color: #A00000">-</span>
<span style="color: #A00000">- qreal polygonWidth = (polygonCenterX - polygonLeft) * 2;</span>
<span style="color: #A00000">- qreal polygonHeight = (polygonCenterY - polygonTop) * 2;</span>
<span style="color: #A00000">-</span>
<span style="color: #A00000">- QVector2D polygonCenter(polygonCenterX, polygonCenterY);</span>
<span style="color: #A00000">- QVector2D cameraPosition(cameraX, cameraY);</span>
<span style="color: #A00000">- QVector2D translate = cameraPosition - polygonCenter;</span>
<span style="color: #A00000">-</span>
<span style="color: #A00000">- qreal offset = qSqrt( (qPow(polygonWidth, 2) + qPow(polygonHeight, 2)) /</span>
<span style="color: #A00000">- (qPow(viewport->width(), 2) + qPow(viewport->height(), 2)) ) *</span>
<span style="color: #A00000">- qPow( s_maximumOffset, 2 );</span>
<span style="color: #A00000">- if (offset > s_maximumOffset) offset = s_maximumOffset;</span>
<span style="color: #A00000">-</span>
<span style="color: #A00000">- translate.setX( ((translate.x() / (viewport->width() / 2))) * offset);</span>
<span style="color: #A00000">- translate.setY( ((translate.y() / (viewport->height() / 2))) * offset);</span>
<span style="color: #A00000">-</span>
<span style="color: #A00000">- painter->translate( translate.toPointF() );</span>
<span style="color: #00A000">+ if ( isBuildingFrame ) {</span>
<span style="color: #00A000">+ QVector<QPolygonF*> polygons;</span>
<span style="color: #00A000">+ if (m_polygon) {</span>
<span style="color: #00A000">+ viewport->screenCoordinates(m_polygon->outerBoundary(), polygons);</span>
<span style="color: #00A000">+ } else if (m_ring) {</span>
<span style="color: #00A000">+ viewport->screenCoordinates(*m_ring, polygons);</span>
<span style="color: #00A000">+ }</span>
<span style="color: #00A000">+ foreach(QPolygonF* polygon, polygons) {</span>
<span style="color: #00A000">+ if (polygon->isEmpty()) {</span>
<span style="color: #00A000">+ continue;</span>
<span style="color: #00A000">+ }</span>
<span style="color: #00A000">+ int const size = polygon->size();</span>
<span style="color: #00A000">+ for (int i=1; i<size; ++i) {</span>
<span style="color: #00A000">+ QPointF const & a = (*polygon)[i-1];</span>
<span style="color: #00A000">+ QPointF const & b = (*polygon)[i];</span>
<span style="color: #00A000">+ QPointF const shiftA = buildingOffset(a, viewport);</span>
<span style="color: #00A000">+ if (polygon->contains(shiftA)) {</span>
<span style="color: #00A000">+ continue;</span>
<span style="color: #00A000">+ }</span>
<span style="color: #00A000">+ QPointF const shiftB = buildingOffset(b, viewport);</span>
<span style="color: #00A000">+ if (polygon->contains(shiftB)) {</span>
<span style="color: #00A000">+ continue;</span>
<span style="color: #00A000">+ }</span>
<span style="color: #00A000">+ painter->drawPolygon(*polygon);</span>
<span style="color: #00A000">+ QPolygonF buildingSide = QPolygonF() << a << a+shiftA << b+shiftB << b;</span>
<span style="color: #00A000">+ painter->drawPolygon(buildingSide);</span>
<span style="color: #00A000">+ }</span>
<span style="color: #00A000">+ }</span>
<span style="color: #00A000">+ qDeleteAll(polygons);</span>
}
<span style="color: #A00000">-</span>
<span style="color: #A00000">- if ( m_polygon ) {</span>
<span style="color: #A00000">- painter->drawPolygon( *m_polygon );</span>
<span style="color: #A00000">- } else if ( m_ring ) {</span>
<span style="color: #A00000">- painter->drawPolygon( *m_ring );</span>
<span style="color: #00A000">+ else if (isBuildingRoof) {</span>
<span style="color: #00A000">+ QVector<QPolygonF*> polygons;</span>
<span style="color: #00A000">+ if (m_polygon) {</span>
<span style="color: #00A000">+ viewport->screenCoordinates(m_polygon->outerBoundary(), polygons);</span>
<span style="color: #00A000">+ } else if (m_ring) {</span>
<span style="color: #00A000">+ viewport->screenCoordinates(*m_ring, polygons);</span>
<span style="color: #00A000">+ }</span>
<span style="color: #00A000">+ foreach(QPolygonF* polygon, polygons) {</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">+ }</span>
<span style="color: #00A000">+ qDeleteAll(polygons);</span>
<span style="color: #00A000">+ }</span>
<span style="color: #00A000">+ else {</span>
<span style="color: #00A000">+ if ( m_polygon ) {</span>
<span style="color: #00A000">+ painter->drawPolygon( *m_polygon );</span>
<span style="color: #00A000">+ } else if ( m_ring ) {</span>
<span style="color: #00A000">+ painter->drawPolygon( *m_ring );</span>
<span style="color: #00A000">+ }</span>
}
painter->restore();
<span style="color: #000080; font-weight: bold">diff --git a/src/lib/marble/geodata/graphicsitem/GeoPolygonGraphicsItem.h b/src/lib/marble/geodata/graphicsitem/GeoPolygonGraphicsItem.h</span>
<span style="color: #000080; font-weight: bold">index 7c4bc9a..90f3bb9 100644</span>
<span style="color: #A00000">--- a/src/lib/marble/geodata/graphicsitem/GeoPolygonGraphicsItem.h</span>
<span style="color: #00A000">+++ b/src/lib/marble/geodata/graphicsitem/GeoPolygonGraphicsItem.h</span>
<span style="color: #800080; font-weight: bold">@@ -30,8 +30,9 @@ public:</span>
virtual void paint( GeoPainter* painter, const ViewportParams *viewport );
<span style="color: #A00000">-protected:</span>
<span style="color: #00A000">+private:</span>
virtual void createDecorations();
<span style="color: #00A000">+ QPointF buildingOffset(const QPointF &point, const ViewportParams *viewport) const;</span>
const GeoDataPolygon *const m_polygon;
const GeoDataLinearRing *const m_ring;
</pre></div>
</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">It works very well, even performance-wise. This is because there is little overhead in geo->screen position calculation. Nevertheless because of C several more polygons are being painted (all visible walls), so performance is worse than before. It does run quite well on my Nexus 4 though even with high quality painting enabled during animations, here is an awesome screencast showing it: http://nienhueser.de/marble/marble-android-3d-buildings.mp4 and a screenshot:</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;"><img alt="3D buildings" src="http://nienhueser.de/marble/marble-android-3d-buildings.png" style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: normal;" /></p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">For a final implementation we might look into the following approach:
- At higher zoom, disable all decorations
- At medium zoom, use the fake 3D approach
- At high zoom, use the real 3D approach from here</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I'd prefer that to taking the map quality into account: Map quality is never going to be changed by the vast majority of all users. I'd also like to enable the decorations during animations (e.g. while panning the map).</p></pre>
</blockquote>
<p>On August 10th, 2015, 2:26 vorm. UTC, <b>Dávid Kolozsvári</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">This is very cool, especially the video!
Shifting the "base"(real) polygon was a great idea, it works much better this way. Though the math in GeoPolygonGraphicsItem::buildingOffset() is a "little" more advanced than my approach, but I understand it at least, and I even learned some math this way :)</p>
<p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">I will update this patch with this solution, and with your suggestion of taking into account the zoom and map quality.</p></pre>
</blockquote>
<p>On August 10th, 2015, 8:12 vorm. UTC, <b>Torsten Rahn</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;"><p style="padding: 0;text-rendering: inherit;margin: 0;line-height: inherit;white-space: inherit;">In the screenshots it looks like some inner rings are missing in the polygon ... is this a fault of the patch or is it due to the data?</p></pre>
</blockquote>
</blockquote>
<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;">Does the tan() calculation for determining the cameraFactor get optimized away? I also wonder whether it's possible to develop sin(atan(x)-atan(y)) into a power series to improve performance. Oh and I'd really like to get rid of EARTH_RADIUS whereever possible.</p></pre>
<br />
<p>- Torsten</p>
<br />
<p>On August 5th, 2015, 7:31 nachm. 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. 5, 2015, 7:31 nachm.</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>