<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/104990/">http://git.reviewboard.kde.org/r/104990/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On May 22nd, 2012, 10:50 a.m., <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;">I wonder if it is a good idea to "fix" wrong usage of our API because "fixing" introduces the potential of surprises. I'd expect the API to return garbage if I pass garbage (garbage-in-garbage-out principle) rather than having it perform arbitrary changes in the background. After all, fixing usually depends on the context and in addition, there are more efficient methods to ensure intended behavior of corner cases such as unit tests.
Moreover, I don't like the flags approach which adds even more complexity to our API. In particular, it'll be hard to predict the behavior of instances of GeoDataCoordinates ("does the instance normalize or not?"). I think that a isNormalized() method, perhaps in conjunction with Q_ASSERT(), could catch wrong usage of our API and will not add additional complexity.</pre>
</blockquote>
<p>On May 22nd, 2012, 11:41 a.m., <b>Torsten Rahn</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;">We think about adding this patch since there have been multiple people who had passed longitudes from 0 to 360 degrees to Marble (and therefore bumped into misrenderings). Actually there is nothing bad about it: On planets like Venus the [0;360] interval is even common standard (http://www2.jpl.nasa.gov/magellan/guide3.html). Also it's quite natural to expect that a method that takes angles as a parameter performs well even if for multiples of PI/180° (sine, cosine, tan ... which also happily take values like 3 * M_PI).
So this kind of input is far from "garbage". It seems to be common practice for some, so we shouldn't harass people and annoy them with unnecessary asserts if they just want to display their data.
So personally I think that for all cases where the intention is clear (e.g. 54°N, 200°E) we should do the conversion automatically. We should not unnecessarily harass people to change their habits there. For cases where the input seems to be ambigious or unusual (e.g. 120°N, 20°E) a Q_ASSERT() might be a reasonable practice.
I also don't think that flags are bad as long as they have a merit and as long as they are not necessary for the usual default use cases.</pre>
</blockquote>
<p>On May 22nd, 2012, 12:28 p.m., <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;">I totally agree that our API should allow for passing multiples of PI/180°. So where is the point of automatically normalizing the values behind the scene? Doesn't that introduce unexpected behavior? And why is it not sufficient to use Q_ASSERT( foo.isNormalized() ) in unusual cases?</pre>
</blockquote>
<p>On May 22nd, 2012, 12:43 p.m., <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;">BTW, my concern is that more bugs like 285570 will occur, as we try to be "smart" and "fix" things behind the scene.* Since I strongly prefer to work with predictable API, I'm against this patch.
* Even if no values get "fixed" in this case, the bug still demonstrates what I think will be the result of the patch.</pre>
</blockquote>
<p>On May 22nd, 2012, 1:28 p.m., <b>Torsten Rahn</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;">The point of normalization behind the scenes is that it's needed for proper rendering in order to do stuff like:
- proper bounding box calculations and comparisons
- proper deadline handling
- proper coordinate comparisons
Currently we do the required normalization at runtime in the implementation. This has two disadvantages:
- If we forget to call the normalization in the implementation this creates bugs and the user is not be able to determine the cause.
- It decreases run-time speed.
The only disadvantage of normalization behind the scenes would be that if the user passes e.g. 30°N and 200°E and he queries the coordinates again he gets 30°N and 160°W instead. But this isn't much different from other APIs where there is a similar situation. And I also don't think that this is a common use case. I'm quite sure in fact that the assumption that 200° can safely get passed to a GeoDataCoordinate is hugely more frequent than the few use cases where the API user just uses GeoDataCoordinate objects as containers that are supposed to preserve the numerical values and where the lack of this feature will lead to fatal bugs. I think the latter is the exotic exception and for that the user can still use the flag if he really wants that behavior.
The only other solution to preserve the values would be to keep a cached copy of the original or normalized values. But that would increase resource consumption.
Regarding the fear of bugs: Personally I think that the switch of the default behavior will get us rid of quite a few existing bugs inside Marble and also will create less bugs on the side of Marble Library users. I think this outweighs the bugs caused by this switch.
</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;">Which bugs would this patch fix in particular? Without these bug reports I don't feel well informed enough to discuss the problem being solved. In addition, having unit tests showing the core of the problem (which could also help us avoiding these bugs in the future) would add some clarity as well.
Right now, it seems to me that the approach taken in the patch will just shift the problem of proper bbox, dateline and coordinate handling elsewhere since programmers still have to remember to set or unset the flag. In addition, though, it makes our API more flaky and thus harder to predict. In particular, I expect that e.g. bbox and coordinate comparisons (and calculations) become very hard to comprehend, because four cases have to be dealt with: two instances with or without the flag set, or mixed. What will happen in these cases? Won't that complexity introduce new traps for new (and old) Marble developers?
Moreover, due to the added complexity, the proposed solution seems a bit overengineered to me and I fear it will ultimatively lead to unmaintainable code. In fact, there are already a few overengineered corners in Marble which make improvements quite hard to achieve (e.g. OpenGL). I've tried hard in the past to resolve at least some of them, so I'd be very disappointed seeing yet another overengineered piece of code being introduced.
In summary, I think that the problem statement should be further clarified and a solution should be developed together which we all want to live with.</pre>
<br />
<p>- Bernhard</p>
<br />
<p>On May 19th, 2012, 9:15 a.m., Dennis Nienhüser wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for Marble.</div>
<div>By Dennis Nienhüser.</div>
<p style="color: grey;"><i>Updated May 19, 2012, 9:15 a.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;">We don't officially support non-normalized coordinates (lon outside -180...180 degree, lat outside -90..90 degree). To avoid potential pitfalls when developers using Marble as a library ignore that, it might make sense to normalize coordinates behind the scenes. The patch adds that for GeoDataCoordinates, which should cover most cases. It's controlled by a flag (using QFlags for future extensibility) which defaults to normalization.</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;">The major concern should be performance.
Loading a long route (2500 km, 35.000 coordinates), the performance was pretty much identical (~200 ms) for several runs with and without normalization
The startup time for parsing the various files at startup increases slighty from around 1200 ms on my system to around 1300 ms. We might reduce this difference when we remove now unnecessary calls to normalizeLonLat e.g. in GeoDataLatLonBox::fromString. It could even lead to performance improvements later on.
A callgrind log measuring the start, initialization and shutdown of Marble shows that 0.07% of the time (110.000 calls) was spent in normalizeLonLat(). From that 50.000 calls could be spared when removing the normalization in GeoDataLatLonBox::fromString. In summary, the runtime overhead seems to be small.
</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/geodata/data/GeoDataCoordinates.h <span style="color: grey">(8929430)</span></li>
<li>src/lib/geodata/data/GeoDataCoordinates.cpp <span style="color: grey">(c1563a6)</span></li>
<li>src/lib/geodata/data/GeoDataCoordinates_p.h <span style="color: grey">(14c5164)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/104990/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>