<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://svn.reviewboard.kde.org/r/5539/">http://svn.reviewboard.kde.org/r/5539/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On October 8th, 2010, 2:23 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;">Ok, I had a look at this patch yesterday already. The motivation is nice since we also need some similar action for taking camera focus into account.
Now the disadvantage of this patch is that almost all the projection methods need to get moved into the Viewport class. This kind of replicates lots of methods and introduces the awareness of the existance of GeoCoordinates into the ViewPorts class. So in a way this floods this class a bit with lots of stuff that had been encapsulated away before.
Still this step makes sense to some degree: The only other option would be to "muddy" the projection classes with stuff like focus/elevation calculation. Putting an additional screenpixel conversion class (which would handle focus, elevation etc.) between Viewport and Abstractprojection might be another option but of course that could lead to other disadvantages. Any comments from your side regarding these thoughts?
</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;">> Any comments from your side regarding these thoughts?
Regarding the flooding, I think that the conversion (a.k.a. projection) methods actually make sense inside the ViewportParams class: Since we probably don't want to have an altitude concept in the projection classes, it should be implemented inside ViewportParams. That way, code that builds on ViewportParams will transparently use height information. So the conversion methods in ViewportParams will always calculate the correct (lon, lat) pair for each pixel and vice versa, no matter whehter an altitude model is active or not. Of course, we could also introduce a third class, but I'd like to try the pragramtic approach first.
That said, I think that the projection classes (rather than ViewportParams) should be freed from the altitude concept, which is already present in the GeoDataCoordinates class. That way, the projection classes would only implement the bare mathematical conversion formulas which ideally shouldn't be polluted with other concepts. Unfortunately that is not possible in a naive way since the spherical projection depends on the rotation. So we may either pass the rotation as an additional argument to the conversion methods in the projection classes or pragmatically stay with the current virtual methods which take the viewport into account.
As a benefit of freeing the projection classes from the altitude concept, they can be reused with less potential side effects elswhere. For instance, they could be applied to textures.
</pre>
<br />
<p>- Bernhard</p>
<br />
<p>On October 6th, 2010, 3:29 p.m., Bernhard Beschow wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://svn.reviewboard.kde.orgrb/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 Bernhard Beschow.</div>
<p style="color: grey;"><i>Updated 2010-10-06 15:29:28</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;">Using ViewportParams rather than AbstractProjection simpliefies statements like viewport->currentProjection()->screenCoordinates(...,viewport,...) to viewport->screenCoordinates(...).
That way, in the future, ViewportParams could be used for projection calculations that need to take the current viewport (and possibly an elevation model) into account, while AbstractProjections represent the pure mathematical concept (i.e. no elevation model).</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;">Works for me (KDE version of Marble).</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>/trunk/KDE/kdeedu/marble/src/lib/GeoPainter.cpp <span style="color: grey">(1183221)</span></li>
<li>/trunk/KDE/kdeedu/marble/src/lib/MarbleMap.cpp <span style="color: grey">(1183221)</span></li>
<li>/trunk/KDE/kdeedu/marble/src/lib/MarblePhysics.cpp <span style="color: grey">(1183221)</span></li>
<li>/trunk/KDE/kdeedu/marble/src/lib/MarbleWidget.cpp <span style="color: grey">(1183221)</span></li>
<li>/trunk/KDE/kdeedu/marble/src/lib/MarbleWidgetInputHandler.cpp <span style="color: grey">(1183221)</span></li>
<li>/trunk/KDE/kdeedu/marble/src/lib/MeasureTool.cpp <span style="color: grey">(1183221)</span></li>
<li>/trunk/KDE/kdeedu/marble/src/lib/PlacemarkLayout.cpp <span style="color: grey">(1183221)</span></li>
<li>/trunk/KDE/kdeedu/marble/src/lib/PlacemarkPainter.cpp <span style="color: grey">(1183221)</span></li>
<li>/trunk/KDE/kdeedu/marble/src/lib/Projections/MercatorProjection.cpp <span style="color: grey">(1183221)</span></li>
<li>/trunk/KDE/kdeedu/marble/src/lib/VectorMap.cpp <span style="color: grey">(1183221)</span></li>
<li>/trunk/KDE/kdeedu/marble/src/lib/ViewParams.h <span style="color: grey">(1183221)</span></li>
<li>/trunk/KDE/kdeedu/marble/src/lib/ViewParams.cpp <span style="color: grey">(1183221)</span></li>
<li>/trunk/KDE/kdeedu/marble/src/lib/ViewportParams.h <span style="color: grey">(1183221)</span></li>
<li>/trunk/KDE/kdeedu/marble/src/lib/ViewportParams.cpp <span style="color: grey">(1183221)</span></li>
<li>/trunk/KDE/kdeedu/marble/src/lib/graphicsview/MarbleGraphicsItem.cpp <span style="color: grey">(1183221)</span></li>
<li>/trunk/KDE/kdeedu/marble/src/lib/graphicsview/MarbleGraphicsItem_p.h <span style="color: grey">(1183221)</span></li>
<li>/trunk/KDE/kdeedu/marble/src/lib/graphicsview/ScreenGraphicsItem_p.h <span style="color: grey">(1183221)</span></li>
<li>/trunk/KDE/kdeedu/marble/src/plasmoid/worldclock.cpp <span style="color: grey">(1183221)</span></li>
<li>/trunk/KDE/kdeedu/marble/src/plugins/render/crosshairs/CrosshairsPlugin.cpp <span style="color: grey">(1183221)</span></li>
<li>/trunk/KDE/kdeedu/marble/src/plugins/render/mapscale/MapScaleFloatItem.cpp <span style="color: grey">(1183221)</span></li>
<li>/trunk/KDE/kdeedu/marble/src/plugins/render/overviewmap/OverviewMap.cpp <span style="color: grey">(1183221)</span></li>
<li>/trunk/KDE/kdeedu/marble/src/plugins/render/positionmarker/PositionMarker.cpp <span style="color: grey">(1183221)</span></li>
</ul>
<p><a href="http://svn.reviewboard.kde.org/r/5539/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>