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

[Torsten Rahn]

> On Juli 19, 2015, 6:01 vorm., Dennis Nienhüser wrote:
> > what's needed to fix the poles? special-case it and don't optimize around there as a dirty hack maybe?

I need to check. Can't be rocket science though. :)


> On Juli 19, 2015, 6:01 vorm., 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.

Yes it's used only in one place. The semantic is backwards.


> On Juli 19, 2015, 6:01 vorm., Dennis Nienhüser wrote:
> > src/lib/marble/geodata/data/GeoDataLineString.cpp, line 136
> > <https://git.reviewboard.kde.org/r/123239/diff/1/?file=359920#file359920line136>
> >
> >     This is like a cache with 1 entry? Does using a QCache make sense?

no it's just storing a single previous value for comparison.


> On Juli 19, 2015, 6:01 vorm., 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)

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.


> On Juli 19, 2015, 6:01 vorm., Dennis Nienhüser wrote:
> > src/lib/marble/geodata/data/GeoDataLineString.cpp, line 161
> > <https://git.reviewboard.kde.org/r/123239/diff/1/?file=359920#file359920line161>
> >
> >     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]);

That's indeed a better solution - I will change it accordingly.


> On Juli 19, 2015, 6:01 vorm., 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?

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.


- Torsten


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


On April 3, 2015, 1:34 nachm., 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 nachm.)
> 
> 
> 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/50663cc9/attachment-0001.html>