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

[jensmh at gmx.de]

> On 2010-02-18 00:18:47, Torsten Rahn wrote:
> > Looks awesome. Please consider my comments :)

Thanks for the quick review.

I'm not sure about the TileId change ([] -> at()), so I have left it this way for now as it's not a big issue (we can discuss later I think) and fixed the other issues.


- jmho


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


On 2010-02-15 18:36:48, jmho wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> http://reviewboard.kde.org/r/2953/
> -----------------------------------------------------------
> 
> (Updated 2010-02-15 18:36:48)
> 
> 
> 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
> 
>