> 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
> 
>

_______________________________________________
Marble-devel mailing list
[email protected]
https://mail.kde.org/mailman/listinfo/marble-devel

Reply via email to