> 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
