[Marble-devel] Review Request: Refactoring of tile loading code (all in one)
jensmh at gmx.de
jensmh at gmx.de
Thu Feb 18 15:22:34 CET 2010
> On 2010-02-18 00:18:05, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/TileId.cpp, line 39
> > <http://reviewboard.kde.org/r/2953/diff/1/?file=19512#file19512line39>
> >
> > .at(0).toUInt();
The Qt documentation says they are the same, but I can change it of course.
> On 2010-02-18 00:18:05, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/TileId.cpp, line 40
> > <http://reviewboard.kde.org/r/2953/diff/1/?file=19512#file19512line40>
> >
> > .at(1).toUInt();
Same here, and I guess you meant toInt() here?
> On 2010-02-18 00:18:05, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/TileId.cpp, line 42
> > <http://reviewboard.kde.org/r/2953/diff/1/?file=19512#file19512line42>
> >
> > .at(3).toUInt();
same as above
> On 2010-02-18 00:18:05, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/TileId.cpp, line 41
> > <http://reviewboard.kde.org/r/2953/diff/1/?file=19512#file19512line41>
> >
> > .at(2).toUInt();
same as above
> On 2010-02-18 00:18:05, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/AbstractScanlineTextureMapper.cpp, line 629
> > <http://reviewboard.kde.org/r/2953/diff/1/?file=19490#file19490line629>
> >
> > Shouldn't we also have an
> >
> > Q_ASSERT( m_tileSize.isEmpty() )
> >
> > here? This case would be fatal too and would lead to division by zero elsewhere ....
Indeed, assuming you meant !m_tileSize.isEmpty() ;)
> On 2010-02-18 00:18:05, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/MarbleModel.cpp, line 948
> > <http://reviewboard.kde.org/r/2953/diff/1/?file=19501#file19501line948>
> >
> > There should be a comment I guess that this method will only temporarily "pollute" the MarbleModel class ....
I agree that this should be reworked, also the implementation is a little bit hacky as it just uses the groundDataset.
> On 2010-02-18 00:18:05, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/TextureTile.h, line 38
> > <http://reviewboard.kde.org/r/2953/diff/1/?file=19508#file19508line38>
> >
> > I don't like the "SimpleTile vs. Tile" terminology since something can be simple in so many ways: SimpleTileLoader sounds e.g. more like a more convenient tile loader to me. So maybe we should make that clearer by naming it "Tile vs. CompositTile" or something like that.
As we discussed the class names are now:
SimpleTile -> TextureTile (original TextureTile -> StackedTile)
SimpleTileLoader -> TileLoader (original TileLoader -> StackedTileLoader)
> On 2010-02-18 00:18:05, Torsten Rahn wrote:
> > /trunk/KDE/kdeedu/marble/src/lib/MergedLayerDecorator.cpp, line 223
> > <http://reviewboard.kde.org/r/2953/diff/1/?file=19505#file19505line223>
> >
> > Arg, this was surely detaching before ... right?
Yes, it did a copy of the QImage before.
- jmho
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviewboard.kde.org/r/2953/#review4194
-----------------------------------------------------------
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
>
>
More information about the Marble-devel
mailing list