[Marble-devel] Review Request: Separate GeoGraphicsItem from MarbleGraphicsItem and turn the latter into 2D objects

Fri Aug 3 10:18:36 UTC 2012

> On Aug. 2, 2012, 4:52 p.m., Torsten Rahn wrote:
> > src/lib/graphicsview/MarbleGraphicsItem.h, line 134
> > <http://git.reviewboard.kde.org/r/105828/diff/1/?file=75795#file75795line134>
> >
> >     You might want to add this information here: 
> >     
> >     paint() has different semantics for ScreenGraphicsItems/AbstractDataPluginItems and GeoGraphicsItems
> >     The GeoPainter used in "real" GeoGraphicsItems repeats items in x-direction if neccessary.
> >     For ScreenGraphicsItems/AbstractDataPluginItems, this is done in paintEvent() presumably due to caching.

This information is still correct.

> On Aug. 2, 2012, 4:52 p.m., Torsten Rahn wrote:
> > src/lib/graphicsview/GeoGraphicsItem.h, line 127
> > <http://git.reviewboard.kde.org/r/105828/diff/1/?file=75788#file75788line127>
> >
> >     This comment should probably outline the difference in behavior
> >     
> >     paint() has different semantics for ScreenGraphicsItems/AbstractDataPluginItems and GeoGraphicsItems
> >     The GeoPainter used in "real" GeoGraphicsItems repeats items in x-direction if neccessary.
> >     For ScreenGraphicsItems/AbstractDataPluginItems, this is done in paintEvent() presumably due to caching.

I updated the documentation to:

> Paints the item using the given GeoPainter.
> 
> Note that depending on the projection and zoom level, the item may be visible more than once. GeoPainter will therefore automatically "repeat" primitives which have a geo position (GeoDataCoordinates).

- Bernhard

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

On Aug. 2, 2012, 4:23 p.m., Bernhard Beschow wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://git.reviewboard.kde.org/r/105828/
> -----------------------------------------------------------
> 
> (Updated Aug. 2, 2012, 4:23 p.m.)
> 
> 
> Review request for Marble.
> 
> 
> Description
> -------
> 
> This review request includes two patches:
> 
> 
> 1. API change: don't have GeoGraphicsItem inherit MarbleGraphicsItem; introduce GeoLabelGraphicsItem as a placeholder
> 
> GeoGraphicsItems and MarbleGraphicsItems shouldn't be mixed for the following reasons:
> 
> * paintEvent() is NOT called for real GeoGraphicsItems
> Instead, paint() is called directly. As a result, "real" GeoGraphicsItems don't make use of caching.
> 
> * paint() has different semantics for ScreenGraphicsItems/AbstractDataPluginItems and GeoGraphicsItems
> The GeoPainter used in "real" GeoGraphicsItems repeats items in x-direction if neccessary.
> For ScreenGraphicsItems/AbstractDataPluginItems, this is done in paintEvent() presumably due to caching.
> 
> * ScreenGraphicsItems/AbstractDataPluginItems are 2D objects, whereas GeoGraphicsItems are 3D objects
> ScreenGraphicsItems/AbstractDataPluginItems represent two-dimensional labels whose size() is independent of the zoom level and they always face the screen (a.k.a. billboards).
> GeoGraphicsItems don't make use of size(). Instead, their size and shape changes depending on the projection.
> 
> Since AbstractDataPluginItem has coordinates(), it was derived from GeoGraphicsItem.
> In order to keep a hight level of source-compatibilty, this patch introduces the placeholder class GeoLabelGraphicsItem.
> It implements the methods GeoGraphicsItem used to deliver to the AbstractDataPluginItems.
> 
> 
> 2. API change: reflect in the API that MarbleGraphicsItems are now 2D objects
> 
> All MarbleGraphicsItems now take a QPainter rather than a GeoPainter, which is sufficient for 2D items.
> In addition, the ViewportParams, renderPosition and GeoSceneLayer parameters have been removed from all paint() methods.
> 
> * using plain 2D API rather than GeoPainter avoids possible side effects of GeoPainter such as repeating in x-direction, which could disturb caching
> * plain 2D items also shouldn't depend on the current viewport, especially on the projection
>   Reacting to viewport changes is currently only possible by subclassing AbstractFloatItem and by reimplementing changeViewport(), where this method is called always before paint().
> * the renderPosition and GeoSceneLayer parameters were never used and were probably overdesigned
> 
> 
> Diffs
> -----
> 
>   .reviewboardrc 285adfe926c9b544d711997e10624059252f3961 
>   src/lib/AbstractDataPluginItem.h dbaeeb3b71f4f6bd649d84bdd71130d041c9104b 
>   src/lib/AbstractDataPluginItem.cpp 511a11b4e23f8cf256ac849b1a5f888c6688bfe1 
>   src/lib/AbstractFloatItem.cpp e54b72f05210b5ded2fda0eb33c82c4b9fef328e 
>   src/lib/graphicsview/CMakeLists.txt 8d51bb841b573f5c49646965c389b593f2cf98e4 
>   src/lib/graphicsview/FrameGraphicsItem.h 891512bd955ff218d17ce004e0abd4e841975c2f 
>   src/lib/graphicsview/FrameGraphicsItem.cpp f91281f219d6b5141f82802eafb4fa9a2f5d563e 
>   src/lib/graphicsview/GeoGraphicsItem.h 6d888c75816c4abb46771cbccb11e805d4e7b3d4 
>   src/lib/graphicsview/GeoGraphicsItem.cpp 399a10a69baaac820b858f0d09931fedb548a54f 
>   src/lib/graphicsview/GeoGraphicsItem_p.h 6e9657fdaa9fbfbd08a537b3c073093ae796c47a 
>   src/lib/graphicsview/GeoLabelGraphicsItem.h PRE-CREATION 
>   src/lib/graphicsview/GeoLabelGraphicsItem.cpp PRE-CREATION 
>   src/lib/graphicsview/LabelGraphicsItem.h 187cb265549167c5c1ab02df5839f9fd4b4aa38a 
>   src/lib/graphicsview/LabelGraphicsItem.cpp ca0943a6bb1e4f7ceba161dc37d9e6ee6201de5b 
>   src/lib/graphicsview/MarbleGraphicsItem.h 83ca8cd5de8bf155c0eedf08b7a7ba8c306742b4 
>   src/lib/graphicsview/MarbleGraphicsItem.cpp f136b7ed04305bfcfdd0953122165f84012c8a17 
>   src/lib/graphicsview/MarbleGraphicsItem_p.h 2b0f50e1746ab2d852de545ee5bcdd53b1778d16 
>   src/lib/graphicsview/ScreenGraphicsItem_p.h 1f84c0f013dfc90d8941d0e06890977f3df2fcce 
>   src/lib/graphicsview/WidgetGraphicsItem.h d98b6320b7541077de5821e2f9f3bb02260f49c7 
>   src/lib/graphicsview/WidgetGraphicsItem.cpp 0ea63460520ae590c5760fe62059e03bd3cd02af 
>   src/lib/graphicsview/WidgetGraphicsItem_p.h 81eb026118a60bbea370310e8142ab00f0527b44 
>   src/plugins/render/compass/CompassFloatItem.h afdad8e276b5721db598c51443b737465293cbd9 
>   src/plugins/render/compass/CompassFloatItem.cpp d1b4b149ff4ba184710b4345205b950eb02a770c 
>   src/plugins/render/earthquake/EarthquakeItem.h 3a23e7ab1b104ea5aaf430066ccd748a19a15ac2 
>   src/plugins/render/earthquake/EarthquakeItem.cpp 305063de4f6a808cdcd88866f12429a802ae0c04 
>   src/plugins/render/elevationprofilefloatitem/ElevationProfileFloatItem.h 788faaab46577b8b8816ab5312bf4498fdb4b0e3 
>   src/plugins/render/elevationprofilefloatitem/ElevationProfileFloatItem.cpp eb13aee3ff654fb961a34ce79294fca43b78e330 
>   src/plugins/render/elevationprofilemarker/ElevationProfileMarker.h 59f8139764a03facf6003d252463311fdc969f26 
>   src/plugins/render/mapscale/MapScaleFloatItem.h b68a46b0386809166ffcc03b8ae0159908f07067 
>   src/plugins/render/mapscale/MapScaleFloatItem.cpp 1c17ed05ec7c25c37ea2d29dc2b75e9b4d0b94b2 
>   src/plugins/render/opendesktop/OpenDesktopItem.h 78078d91299fa899450cea25f006eac6ed7aec8a 
>   src/plugins/render/opendesktop/OpenDesktopItem.cpp b7930ee3c1173bdb3331f8244ea3efaa4a068fb3 
>   src/plugins/render/overviewmap/OverviewMap.h 9db547d10714a692172d4b2f1e292e9ca7bce5a1 
>   src/plugins/render/overviewmap/OverviewMap.cpp bc9aea455b65381d9aef19aec70d4212186180cd 
>   src/plugins/render/postalcode/PostalCodeItem.h 999f1d23e1bb3c21cf12f533f8879995fedfdf78 
>   src/plugins/render/postalcode/PostalCodeItem.cpp 13702d7f3860b8244c5b3402a5c08264f475fb27 
>   src/plugins/render/progress/ProgressFloatItem.h 326ae7853eccb84472b87e9db48ad8d8f21b2d89 
>   src/plugins/render/progress/ProgressFloatItem.cpp 251d2402ccc35c4e27974a31dd93fd4d1db1f4fb 
>   src/plugins/render/wikipedia/WikipediaItem.h 38f00a892f214819ee6760f7ab7d793785f265bd 
>   src/plugins/render/wikipedia/WikipediaItem.cpp 65bdac91792b3021c8a315dac470c24fdd6730ca 
> 
> Diff: http://git.reviewboard.kde.org/r/105828/diff/
> 
> 
> Testing
> -------
> 
> Online plugins and float items still seem to be rendered properly.
> 
> Question: Should GeoLabelGraphicsItem be renamed, e.g. to BillboardGraphicsItem?
> 
> 
> Thanks,
> 
> Bernhard Beschow
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/marble-devel/attachments/20120803/3b713dbf/attachment.html>