> 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 > >
_______________________________________________ Marble-devel mailing list Marble-devel@kde.org https://mail.kde.org/mailman/listinfo/marble-devel
