[Marble-devel] MarbleGraphicsItem

[Torsten Rahn]

Hi Bastian,

Am Montag, 11. Mai 2009 20:14:18 schrieb Bastian Holst:
> I'm currently working on the AbstractDataPlugins for Marble. As the items
> of these Plugins have similar features as (future) GeoDataPlacemark and
> GeoData..., Torsten proposed to create an abstract class
> MarbleGraphicsItem, being the parent of both, the GeoDataClasses, painting
> things on the screen, and AbstractDataPluginItems.
> I attached a first header for these items and would like to know if you see
> any problems with this approach. 

Nice start. I see a few issues that should be easy to fix:

First off I now realize that there are a few cases (e.g. the float items) where 
the geographical aspect doesn't make any sense (having a coordinate() method 
for a float item doesn't make sense e.g.).

So there are (imho) two solutions:

1.) We introduce another abstraction layer.

MarbleGraphicsItem (includes all the basic 2D stuff, like a virtual pos() 
method, boundingRect(), etc.) 
<- GeoGraphicsItem (includes stuff like coordinates() and latLonAltBox() )

So GeoGraphicsItem would inherit from MarbleGraphicsItem.
(and MarbleGraphicsItem might at some point be replaced by QGraphicsItem as 
soon as that is an option -- maybe for Qt5 )

2.) We just make MarbleGraphicsItem a geo-agnostic graphics item that can 
handle projections.
The geographical aspect would be added by the GeoDataGeometry, GeoDataFeature 
and the AbstractDataPluginItem. 

Personally I currently favour 1).

>class MarbleGraphicsItem {
> public:
>    MarbleGraphicsItem(); // Should the MarbleGraphicsItem take a parent
> (type MarbleGraphicsItem) ~MarbleGraphicsItem();

According to Patrick it would just optionally take a parent ID (as geodata 
objects can be copied).

>    virtual bool paint( GeoPainter *painter, ViewportParams *viewport,
>                        const QString& renderPos, GeoSceneLayer * layer = 0
> ) = 0;

Looks fine to me for a start :-)

>    /**
>     * @brief Get the id of the item.
>     */
>    int id() const; // is already in GeoDataObject, should GeoDataObject be
> a parent of MarbleGraphicsItem?

Good question. We need Patricks input for this. 
If this is desirable then it should only be added to GeoGraphicsItem. 

>    GeoDataCoordinates coordinate() const;
>    void setCoordinate( const GeoDataPoint &point );

All this should be moved to GeoGraphicsItem (and we should add latLonAltBox() 
). 

>    QString target();
>    void setTarget( QString target );

Hm, I wonder whether this should be here: You can display an item on a planet 
but I wonder whether doing that or not shouldn't just be handled by the plugin 
that instantiates the items: After all we don't have a "mapTheme()" method 
either (and of course I don't sanely argue for having it ;-)
Of course you could see "Target" as an extension of specifying the location. 
Hm ...

Personally I think that we shouldn't add this to the API (at least not for 
now). And that the target evaluation should be done by the instance which 
instantiates and renders

>    bool contains( const QPoint& point ) const;
>    virtual QSize size() = 0;

Looks good. For the contains method we certainly need a  

virtual QRectF boundingRect() const

method.

>    /**
>     * This has to be called on every paint event to update the position on
> the widget */
>    void updatePaintPosition( ViewportParams *viewport );

Hm, what would this do specifically?

Currently we want to avoid calculating the position on the screen as much as 
possible (as it can be very expensive depending on the projection you choose) 
-- at least if you do it for lots of items.

Avoiding calculating the position when you decide whether an item should be 
painted works like this:

1.) Check whether the current view-LatLonAltBox covers the latLonAltBox of the 
item. (most likely it doesn't --> return)

2.) Check whether the level of detail (or other criteria) allows the item to 
be painted.

3.) If so only then calculate the position (involves possibly Matrix-
multiplication and trigonometric methods, so it's expensive for all non-flat 
projections)

That's why I'd like to keep a possible "pos" method virtual and make it only 
being evaluated if it really needs to be called ...

>    // I'm not sure if the following should be part of MarbleGraphicsItem of
> AbstractDataPluginItem /**
>     * Returning the action associated to the item
>     */
>    QAction action();

I don't think that this should be part of a generic GeoGraphicsItem in general 
(too much fat). You might want to extend this to become a QList<QAction*> 
after all ..

Regards,

Torsten