[Marble-devel] Review Request 109061: LineString: move Tessellation out of projection code and fix cylindrical issues

[Thibaut Gridel]

> On Feb. 21, 2013, 7:01 p.m., Bernhard Beschow wrote:
> > Are you sure that the toTesellated() methods should be projection-independent? Given that the globe projection requires (for performance reasons) a low resolution towards the poles while Mercator requires (for visual reasons) the opposite there, I wonder how passing just a radius to toTesellated() takes this into account.
> > 
> > As for the tests, I wonder if the expected result could be added for each test row to avoid the loops. The issue with comparing stuff in a loop is that one has to add debug statements to find out what goes wrong. And the issue with debug statements is that they don't "shut up" if nothing fails.

I think it's a matter of what geodetic path needs to be followed (in any projection) rather than how screen coordinates density should be represented.
I was not aware of the rationale you described, i think however that a manhattanLength of the screen coordinates for the 2 points (original code) doesn't respect your spec for cylindrical projections either: an orthodromic segment with 2 points of same latitude between same 2 meridians has a longer screen coordinates size in mercator projection, as you go towards the poles.

I'd be happy to see a benchmark of curve fidelity vs speed to know which is best.

Regarding tests, the idea is to spot when one breaks something (darn useful in this multiplicity of combinations) and let give traces later if needed. Tessellated cases easily add dozens of points, so passing all values in the input is not a sane option imho.
Let's add 3-4 more test shapes and see where this goes...


> On Feb. 21, 2013, 7:01 p.m., Bernhard Beschow wrote:
> > tests/ProjectionTest.cpp, line 33
> > <http://git.reviewboard.kde.org/r/109061/diff/1/?file=114638#file114638line33>
> >
> >     To improve self-documentation of the code, I'd name the method differently because nothing gets drawn and because the method name should probably indicate which method gets tested.

What do you suggest?


> On Feb. 21, 2013, 7:01 p.m., Bernhard Beschow wrote:
> > tests/ProjectionTest.cpp, line 353
> > <http://git.reviewboard.kde.org/r/109061/diff/1/?file=114638#file114638line353>
> >
> >     Please no qDebug()s in tests. You can way better move "QCOMPARE( polys.size(), size );" from the bottom of the method here.

My wrong, no qDebug indeed.
I prefer the size test "after" the loop because you have a chance to test more in case of one failure based on size.


- Thibaut


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/109061/#review27860
-----------------------------------------------------------


On Feb. 20, 2013, 8:27 p.m., Thibaut Gridel wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/109061/
> -----------------------------------------------------------
> 
> (Updated Feb. 20, 2013, 8:27 p.m.)
> 
> 
> Review request for Marble.
> 
> 
> Description
> -------
> 
> Mostly code refactor and some fixes.
> The goal is to separate concerns between projection specific issues and geodetic concerns.
> Then corner cases for crossing the Idl, circling around a pole, moving past the horizon are easier to handle.
> 
> 
> Diffs
> -----
> 
>   src/lib/Projections/AbstractProjection.cpp 6c90e36 
>   src/lib/Projections/AbstractProjection_p.h 1042c7e 
>   src/lib/Projections/CylindricalProjection.cpp a37060e 
>   src/lib/Projections/CylindricalProjection_p.h c34c6d0 
>   src/lib/Projections/SphericalProjection.cpp 229103f 
>   src/lib/geodata/data/GeoDataLatLonBox.cpp d810dab 
>   src/lib/geodata/data/GeoDataLineString.h ce75340 
>   src/lib/geodata/data/GeoDataLineString.cpp 231a85b 
>   src/lib/geodata/data/GeoDataLineString_p.h 8559e31 
>   src/lib/geodata/data/GeoDataLinearRing.h 9cfce6d 
>   src/lib/geodata/data/GeoDataLinearRing.cpp 573e264 
>   tests/ProjectionTest.cpp 8d4d71b 
> 
> Diff: http://git.reviewboard.kde.org/r/109061/diff/
> 
> 
> Testing
> -------
> 
> ProjectionTest does sanitization checks that screen polygons must satisfy:
> closed, contain more than 2 points, no same point twice, repeats
> 
> 
> Thanks,
> 
> Thibaut Gridel
> 
>

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