<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/109061/">http://git.reviewboard.kde.org/r/109061/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On February 21st, 2013, 7:01 p.m. UTC, <b>Bernhard Beschow</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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...</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On February 21st, 2013, 7:01 p.m. UTC, <b>Bernhard Beschow</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/109061/diff/1/?file=114638#file114638line33" style="color: black; font-weight: bold; text-decoration: underline;">tests/ProjectionTest.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody style="background-color: #e4d9cb; padding: 4px 8px; text-align: center;">
<tr>
<td colspan="4"><pre style="font-size: 8pt; line-height: 140%; margin: 0; ">namespace Marble</pre></td>
</tr>
</tbody>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">33</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="kt">void</span> <span class="nf">drawLinearRing</span><span class="p">();</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">What do you suggest?</pre>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On February 21st, 2013, 7:01 p.m. UTC, <b>Bernhard Beschow</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<table width="100%" border="0" bgcolor="white" style="border: 1px solid #C0C0C0; border-collapse: collapse; margin: 2px padding: 2px;">
<thead>
<tr>
<th colspan="4" bgcolor="#F0F0F0" style="border-bottom: 1px solid #C0C0C0; font-size: 9pt; padding: 4px 8px; text-align: left;">
<a href="http://git.reviewboard.kde.org/r/109061/diff/1/?file=114638#file114638line353" style="color: black; font-weight: bold; text-decoration: underline;">tests/ProjectionTest.cpp</a>
<span style="font-weight: normal;">
(Diff revision 1)
</span>
</th>
</tr>
</thead>
<tbody>
<tr>
<th bgcolor="#b1ebb0" style="border-right: 1px solid #C0C0C0;" align="right"><font size="2"></font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "></pre></td>
<th bgcolor="#b1ebb0" style="border-left: 1px solid #C0C0C0; border-right: 1px solid #C0C0C0;" align="right"><font size="2">353</font></th>
<td bgcolor="#c5ffc4" width="50%"><pre style="font-size: 8pt; line-height: 140%; margin: 0; "> <span class="n">qDebug</span><span class="p">()</span> <span class="o"><<</span> <span class="s">"polys "</span> <span class="o"><<</span> <span class="n">polys</span><span class="p">.</span><span class="n">size</span><span class="p">();</span></pre></td>
</tr>
</tbody>
</table>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Please no qDebug()s in tests. You can way better move "QCOMPARE( polys.size(), size );" from the bottom of the method here.</pre>
</blockquote>
</blockquote>
<pre style="margin-left: 1em; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
<br />
<p>- Thibaut</p>
<br />
<p>On February 20th, 2013, 8:27 p.m. UTC, Thibaut Gridel wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Marble.</div>
<div>By Thibaut Gridel.</div>
<p style="color: grey;"><i>Updated Feb. 20, 2013, 8:27 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">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.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">ProjectionTest does sanitization checks that screen polygons must satisfy:
closed, contain more than 2 points, no same point twice, repeats</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>src/lib/Projections/AbstractProjection.cpp <span style="color: grey">(6c90e36)</span></li>
<li>src/lib/Projections/AbstractProjection_p.h <span style="color: grey">(1042c7e)</span></li>
<li>src/lib/Projections/CylindricalProjection.cpp <span style="color: grey">(a37060e)</span></li>
<li>src/lib/Projections/CylindricalProjection_p.h <span style="color: grey">(c34c6d0)</span></li>
<li>src/lib/Projections/SphericalProjection.cpp <span style="color: grey">(229103f)</span></li>
<li>src/lib/geodata/data/GeoDataLatLonBox.cpp <span style="color: grey">(d810dab)</span></li>
<li>src/lib/geodata/data/GeoDataLineString.h <span style="color: grey">(ce75340)</span></li>
<li>src/lib/geodata/data/GeoDataLineString.cpp <span style="color: grey">(231a85b)</span></li>
<li>src/lib/geodata/data/GeoDataLineString_p.h <span style="color: grey">(8559e31)</span></li>
<li>src/lib/geodata/data/GeoDataLinearRing.h <span style="color: grey">(9cfce6d)</span></li>
<li>src/lib/geodata/data/GeoDataLinearRing.cpp <span style="color: grey">(573e264)</span></li>
<li>tests/ProjectionTest.cpp <span style="color: grey">(8d4d71b)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/109061/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>