Improve tile management

[maRci002]

Hello, I made some changes mainly in tile_layer.dart in order to keep the old tiles on the screen when zooming if new ones not avaible. These changes based on current Leaflet's tile fetching /retaining strategy see TileLayer.js / GridLayer.js.

I also added some missing features like minZoom / minNativeZoom / maxNativeZoom / zoomReverse / zoomOffset / errorImage and fixed / tracked down some other problems which I will mention in closes section.

Normal use case (fast internet connection)

Simulating slow internet connection

Add support for Error tiles

Add support for minNativeZoom / maxNativeZoom (this demo shows locked minNativeZoom: 16 while real zoom is about 12/10 <- it will be a real trial for mobile phones bandwidth / render time)

Support for remove tiles if we are out of zoom limits (we don't call any setState if we are out of bounds, however we pay attention for zoom lvl changes) -- this can happen if forinstance MapOptions has maxZoom: 18, but TileLayerOptions has maxZoom: 16; works with minZoom too, and fixed #59 overscale problems (helps if MapOptions / TileLayerOptions has same maxZoom, however without this fix overscale could go beyond limits which could have caused remove all tiles).

closes #59
closes #79
closes #125
closes #227 (we don't need remove maxZoom because we may have multiple layers and if current tilelayer is more than maxZoom then _removeAllTiles called also applies to minZoom; we stop fetching new tiles when not inside min /maxZoom moreover we skip setState calls for performance reasons)
closes #339 (performance -- maybe this PR resolves this because build() method was designed to run as fast as possible (_update is throttled and _pruneTiles / _setView called only when necessary))
closes #386
closes #429
closes #444
closes #457
closes #463
closes #492 (performance -- maybe this PR resolves this because build() method was designed to run as fast as possible (_update is throttled and _pruneTiles / _setView called only when necessary))
closes #517
closes #523
closes #536
closes #538

PR will be closed also:
closes #525
closes #570

PR also gives opportunity to finish these issues easily (we can add some callbacks to TileLayerOptions - when a Tile is downloaded / become active (when loaded + and fade ends) / Tile loads with an error / or when all tiles loaded (_noTilesToLoad internal function can fire callback when the grid layer loaded all visible tiles)):
see #345
see #377

[edihasaj]

Good job man, this is very close to what I was thinking to achieve.
Good job.

[maRci002]

ImageProvider placeholderImage; become deprecated, anyway we can still use it at lowest retained / active level (lowest avaible level based on lowest zIndex).

If someone wants to observe tile fetching / pruning / aborting then revert remove diagnostic prints commit.

Maybe fade in can be improved (Tile has an animationController), but user is able to increase default 100ms.

[maRci002]

If maintainer plans to merge this PR then pls wait a bit because I am going to add some documentation and making some performance refactors (ie. making local function call from setState instead of creating closure-s).

[porfirioribeiro]

Good job! I hope this gets merged some time soon!
This tiles bug was stressing a little bit

[obiwanzenobi]

Nice! It looks very much like leaflet now.

I was working on same problem and one of my consideration was what to do with panning - should the tiles be retained?
Preview without:

Preview with retain on panning:

[aytunch]

@maRci002 Thanks for this PR, I hope it gets merged very soon
@obiwanzenobi Thanks for the illustration, I definitely think the tiles should retain. Especially when we have markers on the map, not retaining and seeing a marker on empty container looks very bad imho

[pento]

Not much difference that I could observe. It possibly takes slightly less time for the old tiles to disappear, but there's still a crossover where both the old and new tiles are visible at the same time.

[johnpryan]

Woohoo! I'm taking a look now, but this is looking great so far!

[johnpryan]

I think this looks great, if anyone sees their issue closed and think it was a mistake, please leave a comment. That said, I went through the issues and it looks like this closes them all, with the exception of maybe #492 and #339.

@maRci002 thanks a TON for this contribution!

[johnpryan]

@maRci002 do you always put null on the left hand side of an equality check? :P

[maRci002]

Sometimes I like using yoda style 😄

It has it own advantage; forinstance in JS this is a valid expression statement (but user wanted to check if i equals to 7, like i == 7 or i === 7)

var i = 0;
if (i=7){
  // this block will be executed
}
console.log(i); // prints 7

Or in java:

String i = null;
if (i.equals("CONSTANT")) {} // NullPointerException

It has it's own benefits but not in Dart 😅

[maRci002]

@johnpryan you are welcome.

[maRci002]

@pento so you are using multiple TileLayerOptions, it can be a problem if base layer loads faster than the transparent layer or vice versa because they are fully independent (even in Leaflet) however this is not your case.

I think this will help your problem just change this:

    tile.loaded = DateTime.now();
    if (options.tileFadeInDuration == null ||
        (tile.loadError && null == options.errorImage)) {
      tile.active = true;
    } else {
      tile.startFadeInAnimation(options.tileFadeInDuration, this);
    }

    setState(() {});

    if (_noTilesToLoad()) {
      // Wait a bit more than tileFadeInDuration (the duration of the tile fade-in)
      // to trigger a pruning.
      Future.delayed(
        options.tileFadeInDuration != null
            ? options.tileFadeInDuration + const Duration(milliseconds: 50)
            : const Duration(milliseconds: 50),
        () {
          setState(_pruneTiles);
        },
      );
    }

To this:

    setState(() {
      tile.loaded = DateTime.now();
      if (options.tileFadeInDuration == null ||
          (tile.loadError && null == options.errorImage)) {
        tile.active = true;
      } else {
        tile.startFadeInAnimation(options.tileFadeInDuration, this);
      }

      if (_noTilesToLoad()) {
        // Call _pruneTiles immediately after tileFadeInDuration (the duration of the tile fade-in)
        if (options.tileFadeInDuration != null) {
          Future.delayed(
            options.tileFadeInDuration,
            () {
              setState(_pruneTiles);
            },
          );
        } else {
          _pruneTiles();
        }
      }
    });

Or maybe to this:

    setState(() {
      tile.loaded = DateTime.now();
      if (options.tileFadeInDuration == null ||
          (tile.loadError && null == options.errorImage)) {
        tile.active = true;
      } else {
        tile.startFadeInAnimation(options.tileFadeInDuration, this);
      }

      if (_noTilesToLoad()) {
        // call _pruneTiles immediately, not waiting for fade in
        _pruneTiles();
      }
    });

[maRci002]

@obiwanzenobi tiles which are on the same zoom level are retained based on margin.

    var margin = options.keepBuffer;
    var noPruneRange = Bounds(
      tileRange.bottomLeft - CustomPoint(margin, -margin),
      tileRange.topRight + CustomPoint(margin, -margin),
    );

[edihasaj]

Nice! It looks very much like leaflet now.

I was working on same problem and one of my consideration was what to do with panning - should the tiles be retained?
Preview without:

Preview with retain on panning:

I think this is very important, hence should be merged. Can you pull a new request with this?

[kengu]

@maRci002 - I think this constructor body introduced error reported in #608. The reason why PR #609 works is that it replaces the state in the widget-tree, forcing a fresh _TileLayerState to be constructed.

In it self, the code is not wrong, the problem occurs in _addTile, where assignment to array _tiles[tileCoordsToKey] is done after the body is executed.

_tiles[tileCoordsToKey] = Tile(
      coords: coords,
      coordsKey: tileCoordsToKey,
      tilePos: _getTilePos(coords),
      current: true,
      level: _levels[coords.z],
      imageProvider: options.tileProvider.getImage(_wrapCoords(coords), options),
      tileReady: _tileReady,
    );

The callback method tileReady is called indirectly from the method _tileOnLoad when the image has been loaded. If the tile is already cached, the method will return immediately, invoking tileReady before the assignment to _tiles[tileCoordsToKey] is completed. In the actual callback method _tileReady, the assignment is evaluated.

var key = _tileCoordsToKey(coords);
    tile = _tiles[key];
    if (null == tile) {
      return;
    }

and as you can see, the method will return before it is rendered to the canvas.

Working solution

• Removing the code from constructor body
• using the new method in loadTileImage in PR https://github.com/johnpryan/flutter_map/pull/584/files#diff-d0fc1fe821e34169c8068a76a40415f3R1007 to load image expliclity in _addTile

I've done this change in https://github.com/DISCOOS/flutter_map/blob/track_change_and_evict_errors/lib/src/layer/tile_layer.dart#L887 and it solves the problem:

void _addTile(Coords<double> coords) {
    var tileCoordsToKey = _tileCoordsToKey(coords);
    _tiles[tileCoordsToKey] = Tile(
      coords: coords,
      coordsKey: tileCoordsToKey,
      tilePos: _getTilePos(coords),
      current: true,
      level: _levels[coords.z],
      imageProvider: options.tileProvider.getImage(_wrapCoords(coords), options),
      tileReady: _tileReady,
    );
    _tiles[tileCoordsToKey].loadTileImage();
  }

I could make an PR with the change to #584 if you're interested?

[maRci002]

I'll checkout your suggestions soon. However #609 works because if drawer detects if user wants to navigate to the same menu then it just closes drawer instead of reopen same menu. You can test it if you pan away then if you try to reopen same menu then map current location stays the same as before.

[kengu]

Yes, of course. My bad.

[velval]

Hi @maRci002 thanks for this. I am using flutter_map 0.10.1+1 and I can see the map is retaining the previous tiles when zooming if the network connection is slow. However, I am testing the case where I manually set the device to flight mode and switch off all internet connections. In this case when trying to zoom the map shows a grey tile instead of keep displaying the previous zoom tiles.
Was wondering if there is an option on the TileLayerOptions that we need to switch or if this is just the expected behaviour. Thanks in advance.

[maRci002]

@velval Leaflet does the same thing so this is the intended behaviour if a Tile loads with error then we show empty Tile on screen because it's loaded so we can throw away old level Tiles.

https://leafletjs.com/ on PC I turned off my internet and gray Tiles appear when zooming in or out. You can use errorImage: which will show your placeholder Image if a tile loads with error.
However there is a little difference if you turn back your internet on Leaflet then it won't reload your Tiles (we do the same thing) however when you zoom out and back to error Tiles then they will be refetched. With PR #577 this behavior can be achived, note: I'm going to add network change detection to it and maybe a version where a little reload Icon will be on error Tiles and if user taps it then Tile will be reloaded (manual version).

And 582 will allow to do anything with Tiles including error Tiles.

[rorystephenson]

@maRci002 I'm working on refactoring the tile layer code again and I've noticed that the z coordinate of tiles (the zoom level of tiles) is always a whole number but the minNativeZoom and maxNativeZoom are doubles. I wondered if there is a use case for this? If it is possible to change them to ints instead that would allow me to simplify some of the tile related code and cache some values for various zoom levels.

[maRci002]

@maRci002 I'm working on refactoring the tile layer code again and I've noticed that the z coordinate of tiles (the zoom level of tiles) is always a whole number but the minNativeZoom and maxNativeZoom are doubles. I wondered if there is a use case for this?

If I read the doc it suggest me this should be integer.

  /// Maximum zoom number the tile source has available. If it is specified, the
  /// tiles on all zoom levels higher than maxNativeZoom will be loaded from
  /// maxNativeZoom level and auto-scaled.
  final double? maxNativeZoom;

I am not fully aware of the project at the moment but here you should test what happens for instance when _tileZoom is 19 and zoom is 20.1 (zoom is calculated by _clampZoom which uses both minNativeZoom and maxNativeZoom). I think it will call _setView which will reset _tileZoom

flutter_map/lib/src/layer/tile_layer/tile_layer.dart

Lines 616 to 621 in 3e8368d

	// _update just loads more tiles. If the tile zoom level differs too much
	// from the map's, let _setView reset levels and prune old tiles.
	if ((zoom - _tileZoom!).abs() > 1) {
	_setView(map, center, zoom);
	return;
	}

[rorystephenson]

Thanks for the response @maRci002! I think your right that in practice it is used like an integer anyway 👍 .

[rorystephenson]

@maRci002 I just noticed that the tile provider rounds the tile numbers anyway so it looks like an int is fine 🎉

flutter_map/lib/src/layer/tile_layer/tile_provider/base_tile_provider.dart

Lines 29 to 31 in 3e8368d

	'x': coords.x.round().toString(),
	'y': coords.y.round().toString(),
	'z': z.round().toString(),

[maRci002]

@rorystephenson I think _tileZoom should be an int? _clampZoom should accept int value and return int value and maxNativeZoom / minNativeZoom should be also int? values.

maxZoom / minZoom seems fine as double values for instance if maxZoom is 3.2 then 3.200001 will remove the tiles (by setting: tileZoom = null):

flutter_map/lib/src/layer/tile_layer/tile_layer.dart

Lines 518 to 522 in 3e8368d

	void _setView(FlutterMapState map, LatLng center, double zoom) {
	double? tileZoom = _clampZoom(zoom.roundToDouble());
	if ((tileZoom > widget.maxZoom) || (tileZoom < widget.minZoom)) {
	tileZoom = null;
	}