[Marble-devel] Re: Review Request: Use ViewportParams rather than AbstractProjection for projection calculations regarding the visible area

[Torsten Rahn]

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://svn.reviewboard.kde.org/r/5539/#review8039
-----------------------------------------------------------


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? 


- Torsten


On 2010-10-06 15:29:28, Bernhard Beschow wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://svn.reviewboard.kde.org/r/5539/
> -----------------------------------------------------------
> 
> (Updated 2010-10-06 15:29:28)
> 
> 
> Review request for marble.
> 
> 
> Summary
> -------
> 
> 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).
> 
> 
> Diffs
> -----
> 
>   /trunk/KDE/kdeedu/marble/src/lib/GeoPainter.cpp 1183221 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleMap.cpp 1183221 
>   /trunk/KDE/kdeedu/marble/src/lib/MarblePhysics.cpp 1183221 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleWidget.cpp 1183221 
>   /trunk/KDE/kdeedu/marble/src/lib/MarbleWidgetInputHandler.cpp 1183221 
>   /trunk/KDE/kdeedu/marble/src/lib/MeasureTool.cpp 1183221 
>   /trunk/KDE/kdeedu/marble/src/lib/PlacemarkLayout.cpp 1183221 
>   /trunk/KDE/kdeedu/marble/src/lib/PlacemarkPainter.cpp 1183221 
>   /trunk/KDE/kdeedu/marble/src/lib/Projections/MercatorProjection.cpp 1183221 
>   /trunk/KDE/kdeedu/marble/src/lib/VectorMap.cpp 1183221 
>   /trunk/KDE/kdeedu/marble/src/lib/ViewParams.h 1183221 
>   /trunk/KDE/kdeedu/marble/src/lib/ViewParams.cpp 1183221 
>   /trunk/KDE/kdeedu/marble/src/lib/ViewportParams.h 1183221 
>   /trunk/KDE/kdeedu/marble/src/lib/ViewportParams.cpp 1183221 
>   /trunk/KDE/kdeedu/marble/src/lib/graphicsview/MarbleGraphicsItem.cpp 1183221 
>   /trunk/KDE/kdeedu/marble/src/lib/graphicsview/MarbleGraphicsItem_p.h 1183221 
>   /trunk/KDE/kdeedu/marble/src/lib/graphicsview/ScreenGraphicsItem_p.h 1183221 
>   /trunk/KDE/kdeedu/marble/src/plasmoid/worldclock.cpp 1183221 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/crosshairs/CrosshairsPlugin.cpp 1183221 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/mapscale/MapScaleFloatItem.cpp 1183221 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/overviewmap/OverviewMap.cpp 1183221 
>   /trunk/KDE/kdeedu/marble/src/plugins/render/positionmarker/PositionMarker.cpp 1183221 
> 
> Diff: http://svn.reviewboard.kde.org/r/5539/diff
> 
> 
> Testing
> -------
> 
> Works for me (KDE version of Marble).
> 
> 
> Thanks,
> 
> Bernhard
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://mail.kde.org/pipermail/marble-devel/attachments/20101008/64e5defe/attachment.htm