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

Dávid Kolozsvári freedawson at gmail.com
Sat Jun 13 03:14:24 UTC 2015 


> On June 12, 2015, 2:02 a.m., Torsten Rahn wrote:
> > src/lib/marble/layers/GeometryLayer.cpp, line 279
> > <https://git.reviewboard.kde.org/r/124074/diff/1/?file=379731#file379731line279>
> >
> >     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?

I tried another solution for this: generating these outlines in the constructor of a `GeoLineStringGraphicsItem` and then create a `getOutline()` method. In the end the performance impact was almost the same, because:
*iterating through the `items` list,
*typecasting(`dynamic_cast`),
*adding the new outline to the `items` list,
*adding it to the `lineStringOutlines` list
has a bigger impact on the performance than this simple "shallow copy" of a `GeoLineStringGraphicsItem`. Storing the outline for every `GeoLineStringGraphicsItem` would make sense, but I think this solution is nicer, because I don't like the idea of doing "unneccesarry" things in a constructor, and this seems more "virtual" and transparent for me.

As for the numbers, I got 1 ms for a small and 2 ms for a large map, so I think it's acceptable. The sorting has a greater impact on the performance, but I think it is acceptable too if we look at the results :)
The major impact on the performance comes from rendering: 20-25 ms for a small and 80-110 for a large map.


> On June 12, 2015, 2:02 a.m., Torsten Rahn wrote:
> > src/lib/marble/layers/GeometryLayer.cpp, line 289
> > <https://git.reviewboard.kde.org/r/124074/diff/1/?file=379731#file379731line289>
> >
> >     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?

I think something like this should have been already present because every `GeoGraphicsItem` has a z-value and this should be taken into account when drawing them.
As for the performance: for a small map, it takes only 1-2 ms to sort the list*, for a big map it takes 5-7 ms, while the painting takes 20-25 ms for a small and 80-110 ms for a big map. The performance impact is about 5-10% and I think this is a must in order to properly draw everything depending on their z-value.
Caching it doesn't make sense for me, because at every repaint the `items` list is changed, so it needs to be sorted again.

*I think that my laptop has a pretty slow hardware, but even though, the relative performance impact should remain the same for slower and faster machines too (about 5-10%).


> On June 12, 2015, 2:02 a.m., Torsten Rahn wrote:
> > src/lib/marble/layers/GeometryLayer.cpp, line 253
> > <https://git.reviewboard.kde.org/r/124074/diff/1/?file=379731#file379731line253>
> >
> >     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.

I just wanted to be consistent to the `TextureLayer`, but you are right, there is no sense in this method. I'll remove it in the new revision.


> On June 12, 2015, 2:02 a.m., Torsten Rahn wrote:
> > src/lib/marble/geodata/graphicsitem/GeoLineStringGraphicsItem.cpp, line 39
> > <https://git.reviewboard.kde.org/r/124074/diff/1/?file=379729#file379729line39>
> >
> >     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.

I think I've deleted a `0`, I originally wanted `0.001`. Creating a variable would make sense, so I'll upload a revision including it.

As I saw, these already serve as an "internal z-value", because they are not altered by the node's z-value: they are initialized in `GeometryLayerPrivate::initializeDefaultValues()` and then aren't modified. In the end it would make sense to take into account this internal z-value, then to have a proper "real life" z-value and the draw order of the items would be estabilished by these two values(the "real life" value would have the higher priority). But I don't want to include it in this patch :)


> On June 12, 2015, 2:02 a.m., Torsten Rahn wrote:
> > src/lib/marble/layers/GeometryLayer.cpp, line 307
> > <https://git.reviewboard.kde.org/r/124074/diff/1/?file=379731#file379731line307>
> >
> >     Please use qDeleteAll() ...

Ok.


- Dávid


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


On June 12, 2015, 1:17 a.m., Dávid Kolozsvári wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124074/
> -----------------------------------------------------------
> 
> (Updated June 12, 2015, 1:17 a.m.)
> 
> 
> 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/20150613/4f7d76e9/attachment-0001.html>

More information about the Marble-devel mailing list