[Marble-devel] Review Request 123239: Optimize linestring rendering via automatically assigned detail level values.

[Dennis Nienhüser]

> On July 19, 2015, 6:01 a.m., Dennis Nienhüser wrote:
> > src/lib/marble/ViewportParams.cpp, line 378
> > <https://git.reviewboard.kde.org/r/123239/diff/1/?file=359917#file359917line378>
> >
> >     did you catch all usage of this function? It's a public method, I wonder if that subtly introduces bugs in 3rd party stuff if we don't rename it at the same time.
> 
> Torsten Rahn wrote:
>     Yes it's used only in one place. The semantic is backwards.

Ah, now I see the catch, we have three overloads of resolve(). Go ahead, makes sense.


> On July 19, 2015, 6:01 a.m., Dennis Nienhüser wrote:
> > src/lib/marble/geodata/data/GeoDataLineString.cpp, line 138
> > <https://git.reviewboard.kde.org/r/123239/diff/1/?file=359920#file359920line138>
> >
> >     What's the reason not to use a function here? Is it supposed to be faster, more readable, or to allow fine-tuning of values?
> >     
> >     What about using a QMap<int,double> instead and calling lowerBound() to return the level? That is as readable and fine-tunable and faster wrt. computational complexity (for large input sizes at least which arguably is not the case here)
> 
> Torsten Rahn wrote:
>     The reason not to use a function here is that this makes it easier to tweak and fine-tune single values based on trial and error testing on the screen. Using a function this would become harder to understand and would be less transparent.
>     Yes, using QMap could be a possible alternative - and in this case it would certainly be a bit slower for most cases - no idea whether it would be more readable.

Ok, fine.


> On July 19, 2015, 6:01 a.m., Dennis Nienhüser wrote:
> > src/lib/marble/projections/AbstractProjection.cpp, line 51
> > <https://git.reviewboard.kde.org/r/123239/diff/1/?file=359922#file359922line51>
> >
> >     why not share with geodatalinestringprivate somehow?
> 
> Torsten Rahn wrote:
>     I thought about it. But I'd like to keep this stuff still in private classes. Also putting it into GeoDataLineString would require to make this method static. So the current solution seems to be more appropriate.

What about having a comment on top of each pointing to the other? I'd like to avoid them going out of sync.


- Dennis


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


On April 3, 2015, 1:34 p.m., Torsten Rahn wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/123239/
> -----------------------------------------------------------
> 
> (Updated April 3, 2015, 1:34 p.m.)
> 
> 
> Review request for Marble, Bernhard Beschow, Dennis Nienhüser, Marius Stanciu, and Thibaut Gridel.
> 
> 
> Repository: marble
> 
> 
> Description
> -------
> 
> Allow for automatically assigning detail values to the nodes inside a linestring.
> This mechanism is used for PN2 parsing in this patch.
> Taking the detail level into account this approach improves geometry layer performance by 20% on my i7 based machine.
> This patch also fixes the "backwards" semantical meaning of ViewportParams::resolve()
> In the next step this optimization could also be applied during KML and OSM parsing.
> 
> 
> Diffs
> -----
> 
>   src/lib/marble/ViewportParams.cpp bca8ea9 
>   src/lib/marble/geodata/data/GeoDataCoordinates.h 610b423 
>   src/lib/marble/geodata/data/GeoDataLineString.h 4e4e3da 
>   src/lib/marble/geodata/data/GeoDataLineString.cpp 8864518 
>   src/lib/marble/geodata/data/GeoDataLineString_p.h 2c7b9ec 
>   src/lib/marble/projections/AbstractProjection.cpp 591adc0 
>   src/lib/marble/projections/AbstractProjection_p.h 7bbfcb5 
>   src/lib/marble/projections/AzimuthalProjection.cpp d6e7376 
>   src/lib/marble/projections/CylindricalProjection.cpp 42345ab 
>   src/plugins/runner/pn2/Pn2Runner.cpp fd363c8 
> 
> Diff: https://git.reviewboard.kde.org/r/123239/diff/
> 
> 
> Testing
> -------
> 
> Tested manually using the Atlas map, Plain map and Political map without apparent regressions. "make test" runs fine.
> Checked the geometry layer values by starting Marble with --runtimeTrace. On average the msec values are about 20% lower on my machine (and I hope that improvement is even better on slower CPUs).
> Ideas for unit tests regarding this patch are welcome. ;-)
> 
> 
> Thanks,
> 
> Torsten Rahn
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/marble-devel/attachments/20150719/bb736232/attachment.html>