[Marble-devel] Review Request 124511: Merged the decoration(124154) and the label placement(124498) review requests.

[Dennis Nienhüser]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124511/#review83780
-----------------------------------------------------------



src/lib/marble/geodata/graphicsitem/GeoLineStringGraphicsItem.cpp (line 37)
<https://git.reviewboard.kde.org/r/124511/#comment57999>

    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.



src/lib/marble/geodata/graphicsitem/GeoPolygonGraphicsItem.h (line 42)
<https://git.reviewboard.kde.org/r/124511/#comment57998>

    not used anymore



src/lib/marble/geodata/graphicsitem/GeoPolygonGraphicsItem.cpp (line 229)
<https://git.reviewboard.kde.org/r/124511/#comment58001>

    We get further performance improvements by special-casing the fake 3D case again here, restoring the old painter translation approach:
    
    ```
    diff --git a/src/lib/marble/geodata/graphicsitem/GeoPolygonGraphicsItem.cpp b/src/lib/marble/geodata/graphicsitem/GeoPolygonGraphicsItem.cpp
    index 6d6e14e..6b47388 100644
    --- a/src/lib/marble/geodata/graphicsitem/GeoPolygonGraphicsItem.cpp
    +++ b/src/lib/marble/geodata/graphicsitem/GeoPolygonGraphicsItem.cpp
    @@ -227,11 +227,17 @@ void GeoPolygonGraphicsItem::paint( GeoPainter* painter, const ViewportParams* v
                 viewport->screenCoordinates(*m_ring, polygons);
             }
             foreach(QPolygonF* polygon, polygons) {
    -            QPolygonF buildingRoof;
    -            foreach(const QPointF &point, *polygon) {
    -                buildingRoof << point + buildingOffset(point, viewport);
    +            if (drawAccurate3D) {
    +                QPolygonF buildingRoof;
    +                foreach(const QPointF &point, *polygon) {
    +                    buildingRoof << point + buildingOffset(point, viewport);
    +                }
    +                painter->drawPolygon(buildingRoof);
    +            } else {
    +                QPointF const offset = buildingOffset(polygon->boundingRect().center(), viewport);
    +                painter->translate(offset);
    +                painter->drawPolygon(*polygon);
                 }
    -            painter->drawPolygon(buildingRoof);
             }
             qDeleteAll(polygons);
         }
    ```



src/lib/marble/graphicsview/GeoGraphicsItem.cpp (line 160)
<https://git.reviewboard.kde.org/r/124511/#comment58000>

    I'd sync all relevant properties of the passed item here, i.e. call
    `decoration->setVisible(visible());` and so on. Also these properties must be synced in the decoration when the containing item changes. The very least we need is
    ```
    diff --git a/src/lib/marble/graphicsview/GeoGraphicsItem.cpp b/src/lib/marble/graphicsview/GeoGraphicsItem.cpp
    index 0ff6af1..e2857a3 100644
    --- a/src/lib/marble/graphicsview/GeoGraphicsItem.cpp
    +++ b/src/lib/marble/graphicsview/GeoGraphicsItem.cpp
    @@ -42,6 +42,9 @@ bool GeoGraphicsItem::visible() const
     void GeoGraphicsItem::setVisible( bool visible )
     {
         setFlag( ItemIsVisible, visible );
    +    foreach( GeoGraphicsItem* decoration, p()->m_decorations ) {
    +        decoration->setVisible( visible );
    +    }
     }
     
     GeoGraphicsItem::GeoGraphicsItemFlags GeoGraphicsItem::flags() const
    ```
    
    Without this decorations are painted even at lower zoom levels when the containing item is invisible, consuming render performance and leading to artifacts.
    
    Other properties like style, highlight etc. must be forwarded similarly I guess -- whereas I'm not sure about ones like the ItemIsMovable flag.



src/lib/marble/layers/GeometryLayer.cpp (line 271)
<https://git.reviewboard.kde.org/r/124511/#comment57997>

    Extending my previous comment wrt using qStableSort instead of qSort: I wonder if we should really sort the list here. Instead we could replace
    `items << items[i]->decorations();`
    above with something like
    ```
    foreach ( GeoGraphicsItem* decoration, items[i]->decorations() ) {
      items.insert( i, decoration );
    }
    ```
    and iterate over items in the outer loop in reverse order (since we insert into it during traversal)
    
    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).


- Dennis Nienhüser


On Aug. 13, 2015, 12:19 a.m., Dávid Kolozsvári wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124511/
> -----------------------------------------------------------
> 
> (Updated Aug. 13, 2015, 12:19 a.m.)
> 
> 
> 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/20150813/c8217fb5/attachment-0001.html>