[Marble-devel] Review Request 124074: Implemented outlines for GeoLineStringGraphicsItems.

[Torsten Rahn]

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


Great patch - but still needs quite some refinement imho ... :-)


src/lib/marble/geodata/graphicsitem/GeoLineStringGraphicsItem.cpp (line 39)
<https://git.reviewboard.kde.org/r/124074/#comment55767>

    why 0.01 and not 0.001? What are possible problems with regard to messing with the zValue directly (which is also accessible to the API user) as opposed to possibly introducing an "internalZValue" property that still maintains the same actual zValue for both the outline as well as the actual line from the outside?
    
    If we keep it like this then we should at least make 0.01 a variable.



src/lib/marble/layers/GeometryLayer.cpp (line 247)
<https://git.reviewboard.kde.org/r/124074/#comment55770>

    I don't get the intention behind this. In the Texture case setNeedsUpdate basically means that the previously rendered texture part of the viewport (which is cached as a pixmap) can not be used anymore and hence the whole viewport texture needs to be recalculated (instead of just painting the cached pixmap of the texture).
    This doesn't seem to be the case here. Introducing such a method might make sense if we cache "artificial items" or cache "(p)resorted" data. But this is not the case here - here it just seems to call repaintNeeded(). repaintNeeded() on the other side is just about a "weaker" repaint that still keeps cached data intact. So introducing this method like this doesn't look right to me.



src/lib/marble/layers/GeometryLayer.cpp (line 273)
<https://git.reviewboard.kde.org/r/124074/#comment55769>

    This happens on every single redraw. I'm quite sure this does have a performance impact. Any numbers for the expected performance impact e.g. on somewhat older hardware (think android ...)? What about caching these?



src/lib/marble/layers/GeometryLayer.cpp (line 283)
<https://git.reviewboard.kde.org/r/124074/#comment55768>

    This gets sorted on every single repaint. How long does this take for a bigger OSM file on slow hardware? Would it make sense to cache this somewhere?



src/lib/marble/layers/GeometryLayer.cpp (line 301)
<https://git.reviewboard.kde.org/r/124074/#comment55771>

    Please use qDeleteAll() ...


- Torsten Rahn


On Juni 12, 2015, 1:17 vorm., Dávid Kolozsvári wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124074/
> -----------------------------------------------------------
> 
> (Updated Juni 12, 2015, 1:17 vorm.)
> 
> 
> Review request for Marble.
> 
> 
> Repository: marble
> 
> 
> Description
> -------
> 
> This patch adds support for outlines on `GeoLineStringGraphicsItems`, which represent the roads, streets, paths and ways on the OSM vector tiles. The main idea is to draw behind every `GeoLineStringGraphicsItem` the same line(implemented a `GeoLineStringGraphicsItem::copyAsOutline()` function for this purpose) with the pen color(outline color) and the original line on top of that with the brush color(fill color) and a little thinner.
> Painting these outlines have a small impact on the performance: cca. 10 ms(on my laptop) on a large map, which has 2000+ lines. Because of that, this feature can be disabled if map quality is set to `Low` or `Outline`.
> 
> On the other hand, I've added some new features to `GeoDataVisualCategory`, like `HighwayCycleway` and `HighwayFootway`, which are now rendered correctly on OSM vector tiles.
> 
> 
> Diffs
> -----
> 
>   src/lib/marble/MarbleMap.cpp bd4049f 
>   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/layers/GeometryLayer.h 35b7490 
>   src/lib/marble/layers/GeometryLayer.cpp 9eb3f50 
> 
> Diff: https://git.reviewboard.kde.org/r/124074/diff/
> 
> 
> Testing
> -------
> 
> The performance impact is acceptable, the result is ok and works for me.
> 
> 
> Thanks,
> 
> Dávid Kolozsvári
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/marble-devel/attachments/20150612/66ef6f4c/attachment-0001.html>