[Marble-devel] Review Request: Refactoring of tile loading code (all in one)

[jensmh at gmx.de]

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

Review request for marble.


Summary
-------

This patchset focuses on refactoring of the tile loading code. This touches the
classes TileLoader (as expected), but also TextureTile, AbstractTile,
MergedLayerDecorator, AbstractScanlineTextureMapper and MarbleModel.

The most important (or visible) change perhaps is that the handling of the
clouds layer now is done in a generic way and was removed from
MergedLayerDecorator.

There are two new classes, SimpleTextureTile and SimpleTileLoader. They were
introduced in the hope to decrease complexity. SimpleTextureTile represents one
single tile in contrast to TextureTile which represents a composed tile.
Similarily the SimpleTileLoader deals with loading/updating simple (not
composed) tiles and the TileLoader deals with composed tiles, using the
SimpleTileLoader and concentrating on the composing part.

>From TextureTile the code for loading images (or replacement images) from the
file system was moved to SimpleTileLoader.

In AbstractTile, m_created removed, the value is derived now from the
SimpleTextureTiles inside TextureTile. setState was removed, the value is now
derived from the states of the SimpleTextureTiles.

Remaining issues:
- where should "clouds visible" property go?
  MarbleModel comes to mind as this is one level above MergeLayerDecorator
- instead of TileLoader::loadTile having the forMergedLayerDecorator parameter
  one could think of using SimpleTileLoader inside MergedLayerDecorator
- extend DGML texture element or below with "mergeRule" possible values: Copy, Multiply

Besides, patch also contains a number of smaller changes which were needed in
preparation, but might in some cases also make sense on their own.

- TileId: add map theme id, needed do identify tiles across different map themes.
  This change breaks the cloudes layer again, this time it is because clouds
  rendering relies on TileLoader::updateTile triggering MarbleModel::paintTile,
  which is only done if the tile is found to be currently displayed, which is
  true for the underlying tile of the bluemarble theme, but not for the
  cloud tile. Now that TileId contains the map theme id, the cloud tile
  is no longer recognized as the bluemarble tile.
- MapThemeManager: add method mapThemes() which returns all map themes.
- TileLoader: add MapThemeManager parameter to constructor.
- TileLoader: add hash with texture layers.
- TileLoader::loadTile: switch over to new internal hash of texture layers.
- TileLoader: remove method layer() which returns the current texture layer.  
  The current texture layer should not be part of TileLoader's state. So
  as a step towards the removal of this, remove method layer() which returns
  it and switch users of TileLoader::layer() over to alternatives.
- TileLoader: remove methods globalWidth() and globalHeight().
  As these methods depend on thew current texture layer and
  AbstractScanlineTextureMapper was the only user of these methods,
  move them there.
- TileLoader: switch updateTile methods from using the current texture layer
  over to the hash of texture layers and use the given TileId to select.
- TileLoader: remove unused signal paintTile.
- TileLoader: remove methods tileWidth() and tileHeight().
  The result of these methods depend on a given texture layer. As TileLoader's
  state should not contain a texture layer, move this to
  AbstractScanlineTextureMapper. A different possibility would have been to
  add a GeoSceneTexture parameter, but since these methods are not part of
  the tile loading core business, but more convenience methods for
  the texture mappers, I like it more this way.
- TileLoader: make maximumTileLevel() static again and add texture layer parameter.
  This way the texture layer can be removed from TileLoader's state.
  The parameter of TileLoaderHelp::themeStr() had to be constified for this to compile.
- TileLoader: remove setLayer method and corresponding data member.
- TileLoader::initTextureLayers: find really all texture layers.
  A map theme may have (and in fact some do) several texture layers, so
  we have to iterate through all layers to find them.
- MergedLayerDecorator: use TileLoader instead of maunally instanciating a
  TextureTile and then calling textureTile::loadDataset.
  In order to prevent infinite recursion, there is a (hard coded) check in
  TileLoader::mergeDecorations to prevent this (half heartedly).
  Loading/displaying of clouds layer if cloud tiles are not available locally
  remains broken.
- Fix constness of getters which return pointers to members.
  Const getters should return pointers to const member objects and
  non-const getters should return pointers to non-const member objects.
- Fix compilation after constness fixes in geodata/scene.
- TileLoader: remove old updateTile slot, in which the qimage data was read
  from the filesystem again instead of using the in-memory copy.
- TileLoaderHelper: we can pass a pointer to a const GeoSceneTexture here.
- GeoSceneTexture: add method hasMaximumTileLevel().


Diffs
-----

  /trunk/KDE/kdeedu/marble/src/lib/AbstractScanlineTextureMapper.h 1090608 
  /trunk/KDE/kdeedu/marble/src/lib/AbstractScanlineTextureMapper.cpp 1090608 
  /trunk/KDE/kdeedu/marble/src/lib/AbstractTile.h 1090608 
  /trunk/KDE/kdeedu/marble/src/lib/AbstractTile.cpp 1090608 
  /trunk/KDE/kdeedu/marble/src/lib/AbstractTile_p.h 1090608 
  /trunk/KDE/kdeedu/marble/src/lib/CMakeLists.txt 1090608 
  /trunk/KDE/kdeedu/marble/src/lib/EquirectScanlineTextureMapper.h 1090608 
  /trunk/KDE/kdeedu/marble/src/lib/EquirectScanlineTextureMapper.cpp 1090608 
  /trunk/KDE/kdeedu/marble/src/lib/MapThemeManager.h 1090608 
  /trunk/KDE/kdeedu/marble/src/lib/MapThemeManager.cpp 1090608 
  /trunk/KDE/kdeedu/marble/src/lib/MarbleMap.cpp 1090608 
  /trunk/KDE/kdeedu/marble/src/lib/MarbleModel.h 1090608 
  /trunk/KDE/kdeedu/marble/src/lib/MarbleModel.cpp 1090608 
  /trunk/KDE/kdeedu/marble/src/lib/MercatorScanlineTextureMapper.h 1090608 
  /trunk/KDE/kdeedu/marble/src/lib/MercatorScanlineTextureMapper.cpp 1090608 
  /trunk/KDE/kdeedu/marble/src/lib/MergedLayerDecorator.h 1090608 
  /trunk/KDE/kdeedu/marble/src/lib/MergedLayerDecorator.cpp 1090608 
  /trunk/KDE/kdeedu/marble/src/lib/SphericalScanlineTextureMapper.h 1090608 
  /trunk/KDE/kdeedu/marble/src/lib/SphericalScanlineTextureMapper.cpp 1090608 
  /trunk/KDE/kdeedu/marble/src/lib/TextureTile.h 1090608 
  /trunk/KDE/kdeedu/marble/src/lib/TextureTile.cpp 1090608 
  /trunk/KDE/kdeedu/marble/src/lib/TextureTile_p.h 1090608 
  /trunk/KDE/kdeedu/marble/src/lib/TileId.h 1090608 
  /trunk/KDE/kdeedu/marble/src/lib/TileId.cpp 1090608 
  /trunk/KDE/kdeedu/marble/src/lib/TileLoader.h 1090608 
  /trunk/KDE/kdeedu/marble/src/lib/TileLoader.cpp 1090608 
  /trunk/KDE/kdeedu/marble/src/lib/TileLoaderHelper.h 1090608 
  /trunk/KDE/kdeedu/marble/src/lib/TileLoaderHelper.cpp 1090608 
  /trunk/KDE/kdeedu/marble/src/lib/geodata/scene/GeoSceneMap.h 1090608 
  /trunk/KDE/kdeedu/marble/src/lib/geodata/scene/GeoSceneMap.cpp 1090608 
  /trunk/KDE/kdeedu/marble/src/lib/geodata/scene/GeoSceneTexture.h 1090608 

Diff: http://reviewboard.kde.org/r/2953/diff


Testing
-------


Thanks,

jmho