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

[Bernhard Beschow]

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


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.


tests/ProjectionTest.cpp
<http://git.reviewboard.kde.org/r/109061/#comment20882>

    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.



tests/ProjectionTest.cpp
<http://git.reviewboard.kde.org/r/109061/#comment20883>

    Please no qDebug()s in tests. You can way better move "QCOMPARE( polys.size(), size );" from the bottom of the method here.



tests/ProjectionTest.cpp
<http://git.reviewboard.kde.org/r/109061/#comment20884>

    Move in front of first loop.


- Bernhard Beschow


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/20130221/b6261958/attachment.html>