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

[Dennis Nienhüser]

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


what's needed to fix the poles? special-case it and don't optimize around there as a dirty hack maybe?


src/lib/marble/ViewportParams.cpp (line 378)
<https://git.reviewboard.kde.org/r/123239/#comment56982>

    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.



src/lib/marble/geodata/data/GeoDataLineString.cpp (line 134)
<https://git.reviewboard.kde.org/r/123239/#comment56984>

    curly brackets please



src/lib/marble/geodata/data/GeoDataLineString.cpp (line 136)
<https://git.reviewboard.kde.org/r/123239/#comment56987>

    This is like a cache with 1 entry? Does using a QCache make sense?



src/lib/marble/geodata/data/GeoDataLineString.cpp (line 138)
<https://git.reviewboard.kde.org/r/123239/#comment56981>

    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)



src/lib/marble/geodata/data/GeoDataLineString.cpp (line 161)
<https://git.reviewboard.kde.org/r/123239/#comment56983>

    Let's have a QVector<double> m_resolution member and place the values there, then replace the implementation with sth like
    
        return m_resolution.value(level, m_resolution[17]);



src/lib/marble/geodata/data/GeoDataLineString.cpp (line 225)
<https://git.reviewboard.kde.org/r/123239/#comment56985>

    curly brackets please and move to the top of the method



src/lib/marble/geodata/data/GeoDataLineString.cpp (line 255)
<https://git.reviewboard.kde.org/r/123239/#comment56986>

    curly brackets please



src/lib/marble/geodata/data/GeoDataLineString_p.h (line 104)
<https://git.reviewboard.kde.org/r/123239/#comment56979>

    newline missing



src/lib/marble/projections/AbstractProjection.cpp (line 51)
<https://git.reviewboard.kde.org/r/123239/#comment56980>

    why not share with geodatalinestringprivate somehow?


- Dennis Nienhüser


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/664bfea6/attachment.html>