[Marble-devel] Review Request 124511: Merged the decoration(124154) and the label placement(124498) review requests.
Dennis Nienhüser
dennis at nienhueser.de
Mon Aug 10 11:18:23 UTC 2015
> On Aug. 9, 2015, 8:49 nachm., Dennis Nienhüser wrote:
> > 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):
> >
> > 
> > - 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.
> >
> > 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:
> >
> > ```
> > diff --git a/src/lib/marble/geodata/graphicsitem/GeoPolygonGraphicsItem.cpp b/src/lib/marble/geodata/graphicsitem/GeoPolygonGraphicsItem.cpp
> > index 0e0f881..e4321a7 100644
> > --- a/src/lib/marble/geodata/graphicsitem/GeoPolygonGraphicsItem.cpp
> > +++ b/src/lib/marble/geodata/graphicsitem/GeoPolygonGraphicsItem.cpp
> > @@ -41,6 +41,10 @@ const float GeoPolygonGraphicsItem::s_maximumOffset = 8;
> >
> > void GeoPolygonGraphicsItem::createDecorations()
> > {
> > + if (isDecoration()) {
> > + return;
> > + }
> > +
> > GeoPolygonGraphicsItem *fake3D;
> >
> > switch ( feature()->visualCategory() ) {
> > @@ -104,6 +108,28 @@ void GeoPolygonGraphicsItem::createDecorations()
> > this->addDecoration(fake3D);
> > }
> >
> > +QPointF GeoPolygonGraphicsItem::buildingOffset(const QPointF &point, const ViewportParams *viewport) const
> > +{
> > + qreal const cameraFactor = 0.5 * tan(0.5 * 110 * DEG2RAD);
> > + qreal const buildingHeightMeter = 8.0;
> > + qreal const buildingFactor = buildingHeightMeter / EARTH_RADIUS;
> > +
> > + qreal const cameraHeightPixel = viewport->width() * cameraFactor;
> > + qreal const buildingHeightPixel = viewport->radius() * buildingFactor;
> > +
> > + qreal const offsetX = point.x() - viewport->width() / 2.0;
> > + qreal const alpha1 = atan2(offsetX, cameraHeightPixel);
> > + qreal const alpha2 = atan2(offsetX, cameraHeightPixel-buildingHeightPixel);
> > + qreal const shiftX = 2 * (cameraHeightPixel-buildingHeightPixel) * sin(0.5*(alpha2-alpha1));
> > +
> > + qreal const offsetY = point.y() - viewport->height() / 2.0;
> > + qreal const beta1 = atan2(offsetY, cameraHeightPixel);
> > + qreal const beta2 = atan2(offsetY, cameraHeightPixel-buildingHeightPixel);
> > + qreal const shiftY = 2 * (cameraHeightPixel-buildingHeightPixel) * sin(0.5*(beta2-beta1));
> > +
> > + return QPointF(shiftX, shiftY);
> > +}
> > +
> >
> > const GeoDataLatLonAltBox& GeoPolygonGraphicsItem::latLonAltBox() const
> > {
> > @@ -118,9 +144,9 @@ const GeoDataLatLonAltBox& GeoPolygonGraphicsItem::latLonAltBox() const
> >
> > void GeoPolygonGraphicsItem::paint( GeoPainter* painter, const ViewportParams* viewport )
> > {
> > - Q_UNUSED( viewport );
> > -
> > painter->save();
> > + bool const isBuildingFrame = isDecoration();
> > + bool const isBuildingRoof = !decorations().isEmpty();
> >
> > QPen currentPen = painter->pen();
> >
> > @@ -128,7 +154,7 @@ void GeoPolygonGraphicsItem::paint( GeoPainter* painter, const ViewportParams* v
> > painter->setPen( QPen() );
> > }
> > else {
> > - if ( !style()->polyStyle().outline() || isDecoration() ) {
> > + if ( !style()->polyStyle().outline() || isBuildingFrame ) {
> > currentPen.setColor( Qt::transparent );
> > }
> > else {
> > @@ -143,13 +169,6 @@ void GeoPolygonGraphicsItem::paint( GeoPainter* painter, const ViewportParams* v
> >
> > if ( currentPen.style() != style()->lineStyle().penStyle() )
> > currentPen.setStyle( style()->lineStyle().penStyle() );
> > -
> > - if ( painter->mapQuality() != Marble::HighQuality
> > - && painter->mapQuality() != Marble::PrintQuality ) {
> > - QColor penColor = currentPen.color();
> > - penColor.setAlpha( 255 );
> > - currentPen.setColor( penColor );
> > - }
> > }
> >
> > if ( painter->pen() != currentPen )
> > @@ -160,7 +179,7 @@ void GeoPolygonGraphicsItem::paint( GeoPainter* painter, const ViewportParams* v
> > painter->setBrush( QColor( Qt::transparent ) );
> > }
> > else {
> > - if ( isDecoration() ) {
> > + if ( isBuildingFrame ) {
> > painter->setBrush( style()->polyStyle().paintedColor().darker(150) );
> > } else if ( painter->brush().color() != style()->polyStyle().paintedColor() ) {
> > painter->setBrush( style()->polyStyle().paintedColor() );
> > @@ -168,36 +187,58 @@ void GeoPolygonGraphicsItem::paint( GeoPainter* painter, const ViewportParams* v
> > }
> > }
> >
> > - if ( isDecoration() ) {
> > - qreal polygonCenterX, polygonCenterY, cameraX, cameraY,
> > - polygonLeft, polygonTop;
> > - viewport->screenCoordinates( latLonAltBox().center(), polygonCenterX, polygonCenterY );
> > - viewport->screenCoordinates( viewport->focusPoint(), cameraX, cameraY );
> > - viewport->screenCoordinates( latLonAltBox().west(), latLonAltBox().north(),
> > - polygonLeft, polygonTop);
> > -
> > - qreal polygonWidth = (polygonCenterX - polygonLeft) * 2;
> > - qreal polygonHeight = (polygonCenterY - polygonTop) * 2;
> > -
> > - QVector2D polygonCenter(polygonCenterX, polygonCenterY);
> > - QVector2D cameraPosition(cameraX, cameraY);
> > - QVector2D translate = cameraPosition - polygonCenter;
> > -
> > - qreal offset = qSqrt( (qPow(polygonWidth, 2) + qPow(polygonHeight, 2)) /
> > - (qPow(viewport->width(), 2) + qPow(viewport->height(), 2)) ) *
> > - qPow( s_maximumOffset, 2 );
> > - if (offset > s_maximumOffset) offset = s_maximumOffset;
> > -
> > - translate.setX( ((translate.x() / (viewport->width() / 2))) * offset);
> > - translate.setY( ((translate.y() / (viewport->height() / 2))) * offset);
> > -
> > - painter->translate( translate.toPointF() );
> > + if ( isBuildingFrame ) {
> > + QVector<QPolygonF*> polygons;
> > + if (m_polygon) {
> > + viewport->screenCoordinates(m_polygon->outerBoundary(), polygons);
> > + } else if (m_ring) {
> > + viewport->screenCoordinates(*m_ring, polygons);
> > + }
> > + foreach(QPolygonF* polygon, polygons) {
> > + if (polygon->isEmpty()) {
> > + continue;
> > + }
> > + int const size = polygon->size();
> > + for (int i=1; i<size; ++i) {
> > + QPointF const & a = (*polygon)[i-1];
> > + QPointF const & b = (*polygon)[i];
> > + QPointF const shiftA = buildingOffset(a, viewport);
> > + if (polygon->contains(shiftA)) {
> > + continue;
> > + }
> > + QPointF const shiftB = buildingOffset(b, viewport);
> > + if (polygon->contains(shiftB)) {
> > + continue;
> > + }
> > + painter->drawPolygon(*polygon);
> > + QPolygonF buildingSide = QPolygonF() << a << a+shiftA << b+shiftB << b;
> > + painter->drawPolygon(buildingSide);
> > + }
> > + }
> > + qDeleteAll(polygons);
> > }
> > -
> > - if ( m_polygon ) {
> > - painter->drawPolygon( *m_polygon );
> > - } else if ( m_ring ) {
> > - painter->drawPolygon( *m_ring );
> > + else if (isBuildingRoof) {
> > + QVector<QPolygonF*> polygons;
> > + if (m_polygon) {
> > + viewport->screenCoordinates(m_polygon->outerBoundary(), polygons);
> > + } else if (m_ring) {
> > + viewport->screenCoordinates(*m_ring, polygons);
> > + }
> > + foreach(QPolygonF* polygon, polygons) {
> > + QPolygonF buildingRoof;
> > + foreach(const QPointF &point, *polygon) {
> > + buildingRoof << point + buildingOffset(point, viewport);
> > + }
> > + painter->drawPolygon(buildingRoof);
> > + }
> > + qDeleteAll(polygons);
> > + }
> > + else {
> > + if ( m_polygon ) {
> > + painter->drawPolygon( *m_polygon );
> > + } else if ( m_ring ) {
> > + painter->drawPolygon( *m_ring );
> > + }
> > }
> >
> > painter->restore();
> > diff --git a/src/lib/marble/geodata/graphicsitem/GeoPolygonGraphicsItem.h b/src/lib/marble/geodata/graphicsitem/GeoPolygonGraphicsItem.h
> > index 7c4bc9a..90f3bb9 100644
> > --- a/src/lib/marble/geodata/graphicsitem/GeoPolygonGraphicsItem.h
> > +++ b/src/lib/marble/geodata/graphicsitem/GeoPolygonGraphicsItem.h
> > @@ -30,8 +30,9 @@ public:
> >
> > virtual void paint( GeoPainter* painter, const ViewportParams *viewport );
> >
> > -protected:
> > +private:
> > virtual void createDecorations();
> > + QPointF buildingOffset(const QPointF &point, const ViewportParams *viewport) const;
> >
> > const GeoDataPolygon *const m_polygon;
> > const GeoDataLinearRing *const m_ring;
> > ```
> >
> > 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:
> >
> > 
> >
> > 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
> >
> > 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).
>
> Dávid Kolozsvári wrote:
> 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 :)
>
> I will update this patch with this solution, and with your suggestion of taking into account the zoom and map quality.
>
> Torsten Rahn wrote:
> 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?
>
> Torsten Rahn wrote:
> 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.
>
> Dennis Nienhüser wrote:
> The compiler should take care of tan() when optimizations are enabled. Alternatively I could make the variable static to enforce a one-time evaluation only, but I'm pretty sure the compiler handles it fine on its own.
>
> In order to get rid of EARTH_RADIUS I'd need access to the planet, which viewportparams does not yet offer. Similarly I'd rather share '110' (camera viewing angle) with the same value used in MarbleAbstractPresenter. Do you think it makes sense to make both available in ViewportParams? Or by some other means?
Re inner rings missing: Which ones are you missing? I can't spot any in this part of the map, see http://www.openstreetmap.org/relation/62518#map=19/48.99820/8.40971
- Dennis
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124511/#review83626
-----------------------------------------------------------
On Aug. 5, 2015, 7:31 nachm., Dávid Kolozsvári wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124511/
> -----------------------------------------------------------
>
> (Updated Aug. 5, 2015, 7:31 nachm.)
>
>
> Review request for Marble.
>
>
> Repository: marble
>
>
> Description
> -------
>
> 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.
>
>
> Diffs
> -----
>
> src/lib/marble/GeoPainter.h 7a757b9
> src/lib/marble/GeoPainter.cpp d04138c
> src/lib/marble/GeoPainter_p.h f0c4f9b
> src/lib/marble/MarbleGlobal.h cf2768f
> src/lib/marble/geodata/data/GeoDataFeature.h ea23cd8
> src/lib/marble/geodata/data/GeoDataFeature.cpp 6f330fb
> src/lib/marble/geodata/data/GeoDataFeature_p.h 496c356
> src/lib/marble/geodata/graphicsitem/GeoLineStringGraphicsItem.h 4842809
> src/lib/marble/geodata/graphicsitem/GeoLineStringGraphicsItem.cpp 4320c07
> src/lib/marble/geodata/graphicsitem/GeoPolygonGraphicsItem.h f469dfb
> src/lib/marble/geodata/graphicsitem/GeoPolygonGraphicsItem.cpp 81cfe9a
> src/lib/marble/graphicsview/GeoGraphicsItem.h 4ca4727
> src/lib/marble/graphicsview/GeoGraphicsItem.cpp b8fa693
> src/lib/marble/graphicsview/GeoGraphicsItem_p.h 01becfc
> src/lib/marble/layers/GeometryLayer.cpp 9eb3f50
>
> Diff: https://git.reviewboard.kde.org/r/124511/diff/
>
>
> Testing
> -------
>
> It works on a freshly pulled version of Marble.
>
>
> Thanks,
>
> Dávid Kolozsvári
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/marble-devel/attachments/20150810/26512d3b/attachment-0001.html>
More information about the Marble-devel
mailing list