From 5f0af574101aae30367e2ceb66ada7fdd7e1d1cd Mon Sep 17 00:00:00 2001 From: Rory Stephenson Date: Tue, 18 Jul 2023 17:17:54 +0200 Subject: [PATCH 01/10] Refactor tile eviction and pruning logic. Previously pruning logic was spread around in a way that made it hard to reason about. TileImage contained variables only relevant to pruning which would be partially updated whilst loading tiles and partially updated whilst actually pruning. Considering that loading can happen without pruning this led to unnecessary computation and just made it hard to follow the pruning logic. Pruning and evicting logic is now contained in TileRemovalState which will make testing and making changes far easier. --- .../layer/tile_layer/tile_coordinates.dart | 16 +- lib/src/layer/tile_layer/tile_image.dart | 10 -- .../layer/tile_layer/tile_image_manager.dart | 160 +++++------------- lib/src/layer/tile_layer/tile_layer.dart | 43 +++-- .../layer/tile_layer/tile_removal_state.dart | 128 ++++++++++++++ 5 files changed, 203 insertions(+), 154 deletions(-) create mode 100644 lib/src/layer/tile_layer/tile_removal_state.dart diff --git a/lib/src/layer/tile_layer/tile_coordinates.dart b/lib/src/layer/tile_layer/tile_coordinates.dart index c1138e10b..c6e65baab 100644 --- a/lib/src/layer/tile_layer/tile_coordinates.dart +++ b/lib/src/layer/tile_layer/tile_coordinates.dart @@ -10,14 +10,7 @@ class TileCoordinates extends Point { @override String toString() => 'TileCoordinate($x, $y, $z)'; - @override - bool operator ==(Object other) { - if (other is! TileCoordinates) return false; - - return x == other.x && y == other.y && z == other.z; - } - - // Overriden because Point's distanceTo does not allow comparing with a point + // Overridden because Point's distanceTo does not allow comparing with a point // of a different type. @override double distanceTo(Point other) { @@ -26,6 +19,13 @@ class TileCoordinates extends Point { return sqrt(dx * dx + dy * dy); } + @override + bool operator ==(Object other) { + if (other is! TileCoordinates) return false; + + return x == other.x && y == other.y && z == other.z; + } + @override int get hashCode => Object.hash(x.hashCode, y.hashCode, z.hashCode); } diff --git a/lib/src/layer/tile_layer/tile_image.dart b/lib/src/layer/tile_layer/tile_image.dart index a75fdf68d..5b4dba1dd 100644 --- a/lib/src/layer/tile_layer/tile_image.dart +++ b/lib/src/layer/tile_layer/tile_image.dart @@ -1,7 +1,6 @@ import 'package:flutter/widgets.dart'; import 'package:flutter_map/src/layer/tile_layer/tile_coordinates.dart'; import 'package:flutter_map/src/layer/tile_layer/tile_display.dart'; -import 'package:flutter_map/src/layer/tile_layer/tile_layer.dart'; class TileImage extends ChangeNotifier { bool _disposed = false; @@ -34,15 +33,6 @@ class TileImage extends ChangeNotifier { ImageProvider imageProvider; - /// Current tiles are tiles which are in the current tile zoom AND: - /// * Are visible OR, - /// * Were previously visible and are still within the visible bounds - /// expanded by the [TileLayer.keepBuffer]. - bool current = true; - - /// Used during pruning to determine which tiles should be kept. - bool retain = false; - /// Whether the tile is displayable. This means that either: /// * Loading errored but there is a tile error image. /// * Loading succeeded and the fade animation has finished. diff --git a/lib/src/layer/tile_layer/tile_image_manager.dart b/lib/src/layer/tile_layer/tile_image_manager.dart index 2f1a19ed6..35aa1b9d7 100644 --- a/lib/src/layer/tile_layer/tile_image_manager.dart +++ b/lib/src/layer/tile_layer/tile_image_manager.dart @@ -1,5 +1,3 @@ -import 'dart:math'; - import 'package:collection/collection.dart'; import 'package:flutter_map/src/layer/tile_layer/tile_bounds/tile_bounds.dart'; import 'package:flutter_map/src/layer/tile_layer/tile_bounds/tile_bounds_at_zoom.dart'; @@ -8,6 +6,7 @@ import 'package:flutter_map/src/layer/tile_layer/tile_display.dart'; import 'package:flutter_map/src/layer/tile_layer/tile_image.dart'; import 'package:flutter_map/src/layer/tile_layer/tile_layer.dart'; import 'package:flutter_map/src/layer/tile_layer/tile_range.dart'; +import 'package:flutter_map/src/layer/tile_layer/tile_removal_state.dart'; typedef TileCreator = TileImage Function(TileCoordinates coordinates); @@ -58,11 +57,9 @@ class TileImageManager { return true; } - // For all of the tile coordinates: - // * A TileImage is created if missing (current = true in new TileImages) - // * If it exists current is set to true - // * Of these tiles, those which have not started loading yet are returned. - List setCurrentAndReturnNotLoadedTiles( + /// Creates and returns [TileImage]s which do not already exist with the given + /// [tileCoordinates]. + List createMissingTilesIn( Iterable tileCoordinates, { required TileCreator createTile, }) { @@ -74,7 +71,6 @@ class TileImageManager { () => createTile(coordinates), ); - tile.current = true; if (tile.loadStarted == null) notLoaded.add(tile); } @@ -137,127 +133,49 @@ class TileImageManager { } } - void markAsNoLongerCurrentOutside( - int currentTileZoom, DiscreteTileRange noPruneRange) { - for (final entry in _tiles.entries) { - final tile = entry.value; - final c = tile.coordinates; - - if (tile.current && - (c.z != currentTileZoom || !noPruneRange.contains(Point(c.x, c.y)))) { - tile.current = false; - } - } - } - - // Evicts error tiles depending on the [evictStrategy]. - void evictErrorTiles( - DiscreteTileRange tileRange, - EvictErrorTileStrategy evictStrategy, - ) { - if (evictStrategy == EvictErrorTileStrategy.notVisibleRespectMargin) { - final toRemove = []; - for (final entry in _tiles.entries) { - final tile = entry.value; - - if (tile.loadError && !tile.current) { - toRemove.add(entry.key); - } - } - - for (final key in toRemove) { - _remove(key, evictImageFromCache: (_) => true); - } - } else if (evictStrategy == EvictErrorTileStrategy.notVisible) { - final toRemove = []; - for (final entry in _tiles.entries) { - final tile = entry.value; - final c = tile.coordinates; - - if (tile.loadError && - (!tile.current || !tileRange.contains(Point(c.x, c.y)))) { - toRemove.add(entry.key); - } - } + void evictAndPrune({ + required DiscreteTileRange visibleRange, + required int pruneBuffer, + required EvictErrorTileStrategy evictStrategy, + }) { + final pruningState = TileRemovalState( + tileImages: _tiles.values.toSet(), + visibleRange: visibleRange, + keepRange: visibleRange.expand(pruneBuffer), + evictStrategy: evictStrategy, + ); - for (final key in toRemove) { - _remove(key, evictImageFromCache: (_) => true); - } - } + _evictErrorTiles(pruningState); + _prune(pruningState); } - void prune(EvictErrorTileStrategy evictStrategy) { - for (final tile in _tiles.values) { - tile.retain = tile.current; - } - - for (final tile in _tiles.values) { - if (tile.current && !tile.active) { - final coords = tile.coordinates; - if (!_retainAncestor(coords.x, coords.y, coords.z, coords.z - 5)) { - _retainChildren(coords.x, coords.y, coords.z, coords.z + 2); - } - } - } - - final toRemove = []; - for (final entry in _tiles.entries) { - if (!entry.value.retain) toRemove.add(entry.key); - } - - for (final key in toRemove) { - _removeWithDefaultEviction(key, evictStrategy); + void _evictErrorTiles(TileRemovalState tileRemovalState) { + for (final tileImage in tileRemovalState.errorTilesToEvict()) { + _remove(tileImage.coordinatesKey, evictImageFromCache: (_) => true); } } - // Recurses through the descendants of the Tile at the given coordinates - // setting their [Tile.retain] to true if they are active or loaded. Returns - /// true if any of the descendant tiles were retained. - void _retainChildren(int x, int y, int z, int maxZoom) { - for (var i = 2 * x; i < 2 * x + 2; i++) { - for (var j = 2 * y; j < 2 * y + 2; j++) { - final coords = TileCoordinates(i, j, z + 1); - - final tile = _tiles[coords.key]; - if (tile != null) { - if (tile.active) { - tile.retain = true; - continue; - } else if (tile.loadFinishedAt != null) { - tile.retain = true; - } - } - - if (z + 1 < maxZoom) { - _retainChildren(i, j, z + 1, maxZoom); - } - } - } + void prune({ + required DiscreteTileRange visibleRange, + required int pruneBuffer, + required EvictErrorTileStrategy evictStrategy, + }) { + _prune( + TileRemovalState( + tileImages: _tiles.values.toSet(), + visibleRange: visibleRange, + keepRange: visibleRange.expand(pruneBuffer), + evictStrategy: evictStrategy, + ), + ); } - // Recurses through the ancestors of the Tile at the given coordinates setting - // their [Tile.retain] to true if they are active or loaded. Returns true if - // any of the ancestor tiles were active. - bool _retainAncestor(int x, int y, int z, int minZoom) { - final x2 = (x / 2).floor(); - final y2 = (y / 2).floor(); - final z2 = z - 1; - final coords2 = TileCoordinates(x2, y2, z2); - - final tile = _tiles[coords2.key]; - if (tile != null) { - if (tile.active) { - tile.retain = true; - return true; - } else if (tile.loadFinishedAt != null) { - tile.retain = true; - } - } - - if (z2 > minZoom) { - return _retainAncestor(x2, y2, z2, minZoom); + void _prune(TileRemovalState tileRemovalState) { + for (final tileImage in tileRemovalState.tilesToPrune()) { + _removeWithDefaultEviction( + tileImage.coordinatesKey, + tileRemovalState.evictStrategy, + ); } - - return false; } } diff --git a/lib/src/layer/tile_layer/tile_layer.dart b/lib/src/layer/tile_layer/tile_layer.dart index 0d43ad2b9..d856e0021 100644 --- a/lib/src/layer/tile_layer/tile_layer.dart +++ b/lib/src/layer/tile_layer/tile_layer.dart @@ -545,9 +545,11 @@ class _TileLayerState extends State with TickerProviderStateMixin { } if (event.prune) { - _tileImageManager.evictErrorTiles( - visibleTileRange, widget.evictErrorTileStrategy); - _tileImageManager.prune(widget.evictErrorTileStrategy); + _tileImageManager.evictAndPrune( + visibleRange: visibleTileRange, + pruneBuffer: widget.panBuffer + widget.keepBuffer, + evictStrategy: widget.evictErrorTileStrategy, + ); } } @@ -561,9 +563,11 @@ class _TileLayerState extends State with TickerProviderStateMixin { if (!_outsideZoomLimits(tileZoom)) _loadTiles(visibleTileRange); - _tileImageManager.evictErrorTiles( - visibleTileRange, widget.evictErrorTileStrategy); - _tileImageManager.prune(widget.evictErrorTileStrategy); + _tileImageManager.evictAndPrune( + visibleRange: visibleTileRange, + pruneBuffer: widget.panBuffer + widget.keepBuffer, + evictStrategy: widget.evictErrorTileStrategy, + ); } // For all valid TileCoordinates in the [tileLoadRange], expanded by the @@ -581,16 +585,10 @@ class _TileLayerState extends State with TickerProviderStateMixin { final tileZoom = tileLoadRange.zoom; tileLoadRange = tileLoadRange.expand(widget.panBuffer); - // Mark tiles outside of the tile load range as no longer current. - _tileImageManager.markAsNoLongerCurrentOutside( - tileZoom, - tileLoadRange.expand(widget.keepBuffer), - ); - // Build the queue of tiles to load. Marks all tiles with valid coordinates // in the tileLoadRange as current. final tileBoundsAtZoom = _tileBounds.atZoom(tileZoom); - final tilesToLoad = _tileImageManager.setCurrentAndReturnNotLoadedTiles( + final tilesToLoad = _tileImageManager.createMissingTilesIn( tileBoundsAtZoom.validCoordinatesIn(tileLoadRange), createTile: (coordinates) => _createTileImage(coordinates, tileBoundsAtZoom)); @@ -637,18 +635,33 @@ class _TileLayerState extends State with TickerProviderStateMixin { } widget.tileDisplay.when(instantaneous: (_) { - _tileImageManager.prune(widget.evictErrorTileStrategy); + _pruneWithCurrentCamera(); }, fadeIn: (fadeIn) { // Wait a bit more than tileFadeInDuration to trigger a pruning so that // we don't see tile removal under a fading tile. _pruneLater?.cancel(); _pruneLater = Timer( fadeIn.duration + const Duration(milliseconds: 50), - () => _tileImageManager.prune(widget.evictErrorTileStrategy), + () => _pruneWithCurrentCamera(), ); }); } + void _pruneWithCurrentCamera() { + final camera = MapCamera.of(context); + final visibleTileRange = _tileRangeCalculator.calculate( + camera: camera, + tileZoom: _clampToNativeZoom(camera.zoom), + center: camera.center, + viewingZoom: camera.zoom, + ); + _tileImageManager.prune( + visibleRange: visibleTileRange, + pruneBuffer: widget.panBuffer + widget.keepBuffer, + evictStrategy: widget.evictErrorTileStrategy, + ); + } + bool _outsideZoomLimits(num zoom) => zoom < widget.minZoom || zoom > widget.maxZoom; } diff --git a/lib/src/layer/tile_layer/tile_removal_state.dart b/lib/src/layer/tile_layer/tile_removal_state.dart new file mode 100644 index 000000000..89b960edd --- /dev/null +++ b/lib/src/layer/tile_layer/tile_removal_state.dart @@ -0,0 +1,128 @@ +import 'package:flutter_map/src/layer/tile_layer/tile_coordinates.dart'; +import 'package:flutter_map/src/layer/tile_layer/tile_image.dart'; +import 'package:flutter_map/src/layer/tile_layer/tile_layer.dart'; +import 'package:flutter_map/src/layer/tile_layer/tile_range.dart'; + +class TileRemovalState { + final EvictErrorTileStrategy evictStrategy; + final Map _tileImages; + final DiscreteTileRange _visibleRange; + final DiscreteTileRange _keepRange; + + Set? _visibleTilesMemo; + Set? _keepTilesMemo; + + TileRemovalState({ + required Iterable tileImages, + required DiscreteTileRange visibleRange, + required DiscreteTileRange keepRange, + required this.evictStrategy, + }) : _tileImages = Map.unmodifiable({ + for (var tileImage in tileImages) tileImage.coordinates: tileImage + }), + _visibleRange = visibleRange, + _keepRange = keepRange; + + Set get _visibleTiles => + _visibleTilesMemo ??= Set.unmodifiable(_tileImages.values + .where((tileImage) => _visibleRange.contains(tileImage.coordinates)) + .toSet()); + + Set get _keepTiles => + _keepTilesMemo ??= Set.unmodifiable(_tileImages.values + .where((tileImage) => _keepRange.contains(tileImage.coordinates)) + .toSet()); + + List errorTilesToEvict() { + switch (evictStrategy) { + case EvictErrorTileStrategy.notVisibleRespectMargin: + return _tileImages.values + .where((tileImage) => !_keepTiles.contains(tileImage)) + .toList(); + case EvictErrorTileStrategy.notVisible: + return _tileImages.values + .where((tileImage) => !_visibleTiles.contains(tileImage)) + .toList(); + case EvictErrorTileStrategy.dispose: + case EvictErrorTileStrategy.none: + return const []; + } + } + + List tilesToPrune() { + final retain = Set.from(_keepTiles); + + for (final tile in _tileImages.values) { + if (_keepTiles.contains(tile) && !tile.active) { + final coords = tile.coordinates; + if (!_retainAncestor( + retain, coords.x, coords.y, coords.z, coords.z - 5)) { + _retainChildren(retain, coords.x, coords.y, coords.z, coords.z + 2); + } + } + } + + return _tileImages.values + .where((tileImage) => !retain.contains(tileImage)) + .toList(); + } + + // Recurses through the ancestors of the Tile at the given coordinates adding + // them to [retain] if they are active or loaded. Returns true if any of the + // ancestor tiles were active. + bool _retainAncestor( + Set retain, + int x, + int y, + int z, + int minZoom, + ) { + final x2 = (x / 2).floor(); + final y2 = (y / 2).floor(); + final z2 = z - 1; + final coords2 = TileCoordinates(x2, y2, z2); + + final tile = _tileImages[coords2]; + if (tile != null) { + if (tile.active) { + retain.add(tile); + return true; + } else if (tile.loadFinishedAt != null) { + retain.add(tile); + } + } + + if (z2 > minZoom) { + return _retainAncestor(retain, x2, y2, z2, minZoom); + } + + return false; + } + + // Recurses through the descendants of the Tile at the given coordinates + // adding them to [retain] if they are active or loaded. + void _retainChildren( + Set retain, + int x, + int y, + int z, + int maxZoom, + ) { + for (var i = 2 * x; i < 2 * x + 2; i++) { + for (var j = 2 * y; j < 2 * y + 2; j++) { + final coords = TileCoordinates(i, j, z + 1); + + final tile = _tileImages[coords]; + if (tile != null) { + if (tile.active || tile.loadFinishedAt != null) { + retain.add(tile); + } + } + + if (z + 1 < maxZoom) { + _retainChildren(retain, i, j, z + 1, maxZoom); + } + } + } + } +} From 1633a892776c844989967a136c198c93995c33dd Mon Sep 17 00:00:00 2001 From: Rory Stephenson Date: Tue, 18 Jul 2023 17:44:10 +0200 Subject: [PATCH 02/10] Prevent post-load pruning when loading is triggered by a TileEvent with pruning disabled --- lib/src/layer/tile_layer/tile_layer.dart | 52 ++++++++++++++++-------- 1 file changed, 34 insertions(+), 18 deletions(-) diff --git a/lib/src/layer/tile_layer/tile_layer.dart b/lib/src/layer/tile_layer/tile_layer.dart index d856e0021..965f759d6 100644 --- a/lib/src/layer/tile_layer/tile_layer.dart +++ b/lib/src/layer/tile_layer/tile_layer.dart @@ -462,13 +462,15 @@ class _TileLayerState extends State with TickerProviderStateMixin { // visible until the next build. Therefore, in case this build is executed // before the loading/updating, we must pre-create the missing TileImages // and add them to the widget tree so that when they are loaded they notify - // the Tile and become visible. + // the Tile and become visible. We don't need to prune here as any new tiles + // will be pruned when the map event triggers tile loading. _tileImageManager.createMissingTiles( visibleTileRange, tileBoundsAtZoom, - createTileImage: (coordinate) => _createTileImage( - coordinate, - tileBoundsAtZoom, + createTileImage: (coordinates) => _createTileImage( + coordinates: coordinates, + tileBoundsAtZoom: tileBoundsAtZoom, + pruneAfterLoad: false, ), ); @@ -511,10 +513,11 @@ class _TileLayerState extends State with TickerProviderStateMixin { return color == null ? child : ColoredBox(color: color, child: child); } - TileImage _createTileImage( - TileCoordinates coordinates, - TileBoundsAtZoom tileBoundsAtZoom, - ) { + TileImage _createTileImage({ + required TileCoordinates coordinates, + required TileBoundsAtZoom tileBoundsAtZoom, + required bool pruneAfterLoad, + }) { return TileImage( vsync: this, coordinates: coordinates, @@ -523,7 +526,9 @@ class _TileLayerState extends State with TickerProviderStateMixin { widget, ), onLoadError: _onTileLoadError, - onLoadComplete: _onTileLoadComplete, + onLoadComplete: (coordinates) { + if (pruneAfterLoad) _pruneIfAllTilesLoaded(coordinates); + }, tileDisplay: widget.tileDisplay, errorImage: widget.errorImage, ); @@ -540,8 +545,8 @@ class _TileLayerState extends State with TickerProviderStateMixin { viewingZoom: event.zoom, ); - if (event.load) { - if (!_outsideZoomLimits(tileZoom)) _loadTiles(visibleTileRange); + if (event.load && !_outsideZoomLimits(tileZoom)) { + _loadTiles(visibleTileRange, pruneAfterLoad: event.prune); } if (event.prune) { @@ -561,7 +566,12 @@ class _TileLayerState extends State with TickerProviderStateMixin { tileZoom: tileZoom, ); - if (!_outsideZoomLimits(tileZoom)) _loadTiles(visibleTileRange); + if (!_outsideZoomLimits(tileZoom)) { + _loadTiles( + visibleTileRange, + pruneAfterLoad: true, + ); + } _tileImageManager.evictAndPrune( visibleRange: visibleTileRange, @@ -581,7 +591,10 @@ class _TileLayerState extends State with TickerProviderStateMixin { // Additionally, any current TileImages outside of the [tileLoadRange], // expanded by the [TileLayer.panBuffer] + [TileLayer.keepBuffer], are marked // as not current. - void _loadTiles(DiscreteTileRange tileLoadRange) { + void _loadTiles( + DiscreteTileRange tileLoadRange, { + required bool pruneAfterLoad, + }) { final tileZoom = tileLoadRange.zoom; tileLoadRange = tileLoadRange.expand(widget.panBuffer); @@ -589,9 +602,13 @@ class _TileLayerState extends State with TickerProviderStateMixin { // in the tileLoadRange as current. final tileBoundsAtZoom = _tileBounds.atZoom(tileZoom); final tilesToLoad = _tileImageManager.createMissingTilesIn( - tileBoundsAtZoom.validCoordinatesIn(tileLoadRange), - createTile: (coordinates) => - _createTileImage(coordinates, tileBoundsAtZoom)); + tileBoundsAtZoom.validCoordinatesIn(tileLoadRange), + createTile: (coordinates) => _createTileImage( + coordinates: coordinates, + tileBoundsAtZoom: tileBoundsAtZoom, + pruneAfterLoad: pruneAfterLoad, + ), + ); // Re-order the tiles by their distance to the center of the range. final tileCenter = tileLoadRange.center; @@ -627,8 +644,7 @@ class _TileLayerState extends State with TickerProviderStateMixin { widget.errorTileCallback?.call(tile, error, stackTrace); } - // This is called whether the tile loads successfully or with an error. - void _onTileLoadComplete(TileCoordinates coordinates) { + void _pruneIfAllTilesLoaded(TileCoordinates coordinates) { if (!_tileImageManager.containsTileAt(coordinates) || !_tileImageManager.allLoaded) { return; From 436b7dcdb2218d7958fdff2adc81eabb9a56f3b7 Mon Sep 17 00:00:00 2001 From: Rory Stephenson Date: Sat, 22 Jul 2023 19:35:12 +0200 Subject: [PATCH 03/10] Make it easier to trigger tile loading errors and prevent queued snackbars --- .../lib/pages/tile_loading_error_handle.dart | 84 +++++++++++++++---- 1 file changed, 69 insertions(+), 15 deletions(-) diff --git a/example/lib/pages/tile_loading_error_handle.dart b/example/lib/pages/tile_loading_error_handle.dart index 64de27f77..f958af367 100644 --- a/example/lib/pages/tile_loading_error_handle.dart +++ b/example/lib/pages/tile_loading_error_handle.dart @@ -1,3 +1,6 @@ +import 'dart:ui' as ui; + +import 'package:flutter/foundation.dart'; import 'package:flutter/material.dart'; import 'package:flutter_map/flutter_map.dart'; import 'package:flutter_map_example/widgets/drawer.dart'; @@ -13,10 +16,12 @@ class TileLoadingErrorHandle extends StatefulWidget { } class _TileLoadingErrorHandleState extends State { + static const _showSnackbarDuration = Duration(seconds: 1); + bool _simulateTileLoadErrors = false; + DateTime? _lastShowedTileLoadError; + @override Widget build(BuildContext context) { - var needLoadingError = true; - return Scaffold( appBar: AppBar(title: const Text('Tile Loading Error Handle')), drawer: buildDrawer(context, TileLoadingErrorHandle.route), @@ -24,34 +29,37 @@ class _TileLoadingErrorHandleState extends State { padding: const EdgeInsets.all(8), child: Column( children: [ + SwitchListTile( + title: const Text('Simulate tile loading errors'), + value: _simulateTileLoadErrors, + onChanged: (newValue) => setState(() { + _simulateTileLoadErrors = newValue; + }), + ), const Padding( padding: EdgeInsets.only(top: 8, bottom: 8), - child: Text('Turn on Airplane mode and try to move or zoom map'), + child: Text( + 'Enable tile load error simulation or disable internet and try to move or zoom map.'), ), Flexible( child: Builder(builder: (BuildContext context) { return FlutterMap( - options: MapOptions( - initialCenter: const LatLng(51.5, -0.09), + options: const MapOptions( + initialCenter: LatLng(51.5, -0.09), initialZoom: 5, - onPositionChanged: (MapPosition mapPosition, bool _) { - needLoadingError = true; - }, ), children: [ TileLayer( urlTemplate: 'https://tile.openstreetmap.org/{z}/{x}/{y}.png', - - // For example purposes. It is recommended to use - // TileProvider with a caching and retry strategy, like - // NetworkTileProvider or CachedNetworkTileProvider userAgentPackageName: 'dev.fleaflet.flutter_map.example', + evictErrorTileStrategy: EvictErrorTileStrategy.none, errorTileCallback: (tile, error, stackTrace) { - if (needLoadingError) { + if (_showErrorSnackBar) { + _lastShowedTileLoadError = DateTime.now(); WidgetsBinding.instance.addPostFrameCallback((_) { ScaffoldMessenger.of(context).showSnackBar(SnackBar( - duration: const Duration(seconds: 1), + duration: _showSnackbarDuration, content: Text( error.toString(), style: const TextStyle(color: Colors.black), @@ -59,9 +67,11 @@ class _TileLoadingErrorHandleState extends State { backgroundColor: Colors.deepOrange, )); }); - needLoadingError = false; } }, + tileProvider: _simulateTileLoadErrors + ? _SimulateErrorsTileProvider() + : null, ), ], ); @@ -72,4 +82,48 @@ class _TileLoadingErrorHandleState extends State { ), ); } + + bool get _showErrorSnackBar => + _lastShowedTileLoadError == null || + DateTime.now().difference(_lastShowedTileLoadError!) - + const Duration(milliseconds: 50) > + _showSnackbarDuration; +} + +class _SimulateErrorsTileProvider extends TileProvider { + _SimulateErrorsTileProvider() : super(); + + @override + ImageProvider getImage( + TileCoordinates coordinates, + TileLayer options, + ) => + _SimulateErrorImageProvider(); +} + +class _SimulateErrorImageProvider + extends ImageProvider<_SimulateErrorImageProvider> { + _SimulateErrorImageProvider(); + + @override + ImageStreamCompleter load( + _SimulateErrorImageProvider key, + Future Function( + Uint8List, { + bool allowUpscaling, + int? cacheHeight, + int? cacheWidth, + }) decode, + ) => + _SimulateErrorImageStreamCompleter(); + + @override + Future<_SimulateErrorImageProvider> obtainKey(ImageConfiguration _) => + Future.error('Simulated tile loading error'); +} + +class _SimulateErrorImageStreamCompleter extends ImageStreamCompleter { + _SimulateErrorImageStreamCompleter() { + throw 'Simulated tile loading error'; + } } From e60fa0c4c27f3e2c41d8e57dfc6907095a47c2e3 Mon Sep 17 00:00:00 2001 From: Rory Stephenson Date: Sat, 22 Jul 2023 19:45:00 +0200 Subject: [PATCH 04/10] Rename TileImage active to readyToDisplay for clarity and prevent TileImages with no errorImage from being marked as ready to display. Also fixed handling of errors when tile fading is disabled. --- lib/src/layer/tile_layer/tile_image.dart | 90 +++++++++++-------- .../layer/tile_layer/tile_removal_state.dart | 12 +-- 2 files changed, 57 insertions(+), 45 deletions(-) diff --git a/lib/src/layer/tile_layer/tile_image.dart b/lib/src/layer/tile_layer/tile_image.dart index 5b4dba1dd..520617245 100644 --- a/lib/src/layer/tile_layer/tile_image.dart +++ b/lib/src/layer/tile_layer/tile_image.dart @@ -5,6 +5,12 @@ import 'package:flutter_map/src/layer/tile_layer/tile_display.dart'; class TileImage extends ChangeNotifier { bool _disposed = false; + // Controls fade-in opacity. + AnimationController? _animationController; + + // Whether the tile is displayable. See [readyToDisplay]. + bool _readyToDisplay = false; + /// Used by animationController. Still required if animation is disabled in /// case the tile display is changed at a later point. final TickerProvider vsync; @@ -13,8 +19,6 @@ class TileImage extends ChangeNotifier { /// indicate the position of the tile at that zoom level. final TileCoordinates coordinates; - AnimationController? _animationController; - /// Callback fired when loading finishes with or withut an error. This /// callback is not triggered after this TileImage is disposed. final void Function(TileCoordinates coordinates) onLoadComplete; @@ -26,20 +30,14 @@ class TileImage extends ChangeNotifier { onLoadError; /// Options for how the tile image is displayed. - TileDisplay _display; + TileDisplay _tileDisplay; /// An optional image to show when a loading error occurs. final ImageProvider? errorImage; ImageProvider imageProvider; - /// Whether the tile is displayable. This means that either: - /// * Loading errored but there is a tile error image. - /// * Loading succeeded and the fade animation has finished. - /// * Loading succeeded and there is no fade animation. - bool _active = false; - - // True if an error occurred during loading. + /// True if an error occurred during loading. bool loadError = false; /// When loading started. @@ -60,7 +58,7 @@ class TileImage extends ChangeNotifier { required this.onLoadError, required TileDisplay tileDisplay, required this.errorImage, - }) : _display = tileDisplay, + }) : _tileDisplay = tileDisplay, _animationController = tileDisplay.when( instantaneous: (_) => null, fadeIn: (fadeIn) => AnimationController( @@ -69,8 +67,9 @@ class TileImage extends ChangeNotifier { ), ); - double get opacity => _display.when( - instantaneous: (instantaneous) => _active ? instantaneous.opacity : 0.0, + double get opacity => _tileDisplay.when( + instantaneous: (instantaneous) => + _readyToDisplay ? instantaneous.opacity : 0.0, fadeIn: (fadeIn) => _animationController!.value, )!; @@ -78,7 +77,14 @@ class TileImage extends ChangeNotifier { String get coordinatesKey => coordinates.key; - bool get active => _active; + /// Whether the tile is displayable. This means that either: + /// * Loading errored but an error image is configured. + /// * Loading succeeded and the fade animation has finished. + /// * Loading succeeded and there is no fade animation. + /// + /// Note that [opacity] can be less than 1 when this is true if instantaneous + /// tile display is used with a maximum opacity less than 1. + bool get readyToDisplay => _readyToDisplay; // Used to sort TileImages by their distance from the current zoom. double zIndex(double maxZoom, int currentZoom) => @@ -86,8 +92,8 @@ class TileImage extends ChangeNotifier { // Change the tile display options. set tileDisplay(TileDisplay newTileDisplay) { - final oldTileDisplay = _display; - _display = newTileDisplay; + final oldTileDisplay = _tileDisplay; + _tileDisplay = newTileDisplay; // Handle disabling/enabling of animation controller if necessary oldTileDisplay.when( @@ -98,7 +104,7 @@ class TileImage extends ChangeNotifier { _animationController = AnimationController( duration: fadeIn.duration, vsync: vsync, - value: _active ? 1.0 : 0.0, + value: _readyToDisplay ? 1.0 : 0.0, ); }, ); @@ -146,7 +152,7 @@ class TileImage extends ChangeNotifier { this.imageInfo = imageInfo; if (!_disposed) { - _activate(); + _display(); onLoadComplete(coordinates); } } @@ -155,42 +161,49 @@ class TileImage extends ChangeNotifier { loadError = true; if (!_disposed) { - _activate(); + if (errorImage != null) _display(); onLoadError(this, exception, stackTrace); onLoadComplete(coordinates); } } - void _activate() { + // Initiates fading in and marks this TileImage as readyToDisplay when fading + // finishes. If fading is disabled or a loading error occurred this TileImage + // becomes readyToDisplay immediately. + void _display() { final previouslyLoaded = loadFinishedAt != null; loadFinishedAt = DateTime.now(); - _display.when( + if (loadError) { + assert( + errorImage != null, + 'A TileImage should not be displayed if loading errors and there is no ' + 'errorImage to show.', + ); + _readyToDisplay = true; + if (!_disposed) notifyListeners(); + return; + } + + _tileDisplay.when( instantaneous: (_) { - _active = true; + _readyToDisplay = true; if (!_disposed) notifyListeners(); }, fadeIn: (fadeIn) { - if (loadError && errorImage != null) { - _active = true; - if (!_disposed) notifyListeners(); - return; - } - final fadeStartOpacity = previouslyLoaded ? fadeIn.reloadStartOpacity : fadeIn.startOpacity; if (fadeStartOpacity == 1.0) { - _active = true; + _readyToDisplay = true; if (!_disposed) notifyListeners(); - return; + } else { + _animationController!.reset(); + _animationController!.forward(from: fadeStartOpacity).then((_) { + _readyToDisplay = true; + if (!_disposed) notifyListeners(); + }); } - - _animationController!.reset(); - _animationController!.forward(from: fadeStartOpacity).then((_) { - _active = true; - if (!_disposed) notifyListeners(); - }); }, ); } @@ -213,8 +226,7 @@ class TileImage extends ChangeNotifier { } } - // Mark the image as inactive. - _active = false; + _readyToDisplay = false; _animationController?.stop(canceled: false); _animationController?.value = 0.0; notifyListeners(); @@ -234,6 +246,6 @@ class TileImage extends ChangeNotifier { @override String toString() { - return 'TileImage($coordinates, active: $_active)'; + return 'TileImage($coordinates, readyToDisplay: $_readyToDisplay)'; } } diff --git a/lib/src/layer/tile_layer/tile_removal_state.dart b/lib/src/layer/tile_layer/tile_removal_state.dart index 89b960edd..74b2fd38e 100644 --- a/lib/src/layer/tile_layer/tile_removal_state.dart +++ b/lib/src/layer/tile_layer/tile_removal_state.dart @@ -53,7 +53,7 @@ class TileRemovalState { final retain = Set.from(_keepTiles); for (final tile in _tileImages.values) { - if (_keepTiles.contains(tile) && !tile.active) { + if (_keepTiles.contains(tile) && !tile.readyToDisplay) { final coords = tile.coordinates; if (!_retainAncestor( retain, coords.x, coords.y, coords.z, coords.z - 5)) { @@ -68,8 +68,8 @@ class TileRemovalState { } // Recurses through the ancestors of the Tile at the given coordinates adding - // them to [retain] if they are active or loaded. Returns true if any of the - // ancestor tiles were active. + // them to [retain] if they are ready to display or loaded. Returns true if + // any of the ancestor tiles were ready to display. bool _retainAncestor( Set retain, int x, @@ -84,7 +84,7 @@ class TileRemovalState { final tile = _tileImages[coords2]; if (tile != null) { - if (tile.active) { + if (tile.readyToDisplay) { retain.add(tile); return true; } else if (tile.loadFinishedAt != null) { @@ -100,7 +100,7 @@ class TileRemovalState { } // Recurses through the descendants of the Tile at the given coordinates - // adding them to [retain] if they are active or loaded. + // adding them to [retain] if they are ready to display or loaded. void _retainChildren( Set retain, int x, @@ -114,7 +114,7 @@ class TileRemovalState { final tile = _tileImages[coords]; if (tile != null) { - if (tile.active || tile.loadFinishedAt != null) { + if (tile.readyToDisplay || tile.loadFinishedAt != null) { retain.add(tile); } } From 05a6f5f2c64e14f4fc6f879c001798ed42cf26da Mon Sep 17 00:00:00 2001 From: Rory Stephenson Date: Mon, 24 Jul 2023 11:19:17 +0200 Subject: [PATCH 05/10] Add first tests for TileRemovalState --- .../tile_layer/tile_removal_state_test.dart | 136 ++++++++++++++++++ test/test_utils/test_app.dart | 17 +-- test/test_utils/test_tile_image.dart | 8 ++ test/test_utils/test_tile_provider.dart | 13 ++ 4 files changed, 159 insertions(+), 15 deletions(-) create mode 100644 test/layer/tile_layer/tile_removal_state_test.dart create mode 100644 test/test_utils/test_tile_image.dart create mode 100644 test/test_utils/test_tile_provider.dart diff --git a/test/layer/tile_layer/tile_removal_state_test.dart b/test/layer/tile_layer/tile_removal_state_test.dart new file mode 100644 index 000000000..50f8a47a5 --- /dev/null +++ b/test/layer/tile_layer/tile_removal_state_test.dart @@ -0,0 +1,136 @@ +import 'dart:math'; + +import 'package:flutter/src/scheduler/ticker.dart'; +import 'package:flutter_map/flutter_map.dart'; +import 'package:flutter_map/src/layer/tile_layer/tile_range.dart'; +import 'package:flutter_map/src/layer/tile_layer/tile_removal_state.dart'; +import 'package:flutter_map/src/misc/private/bounds.dart'; +import 'package:test/test.dart'; + +import '../../test_utils/test_tile_image.dart'; + +void main() { + group('tilesToPrune', () { + test('prunes tiles outside of the visible range', () { + final tileImages = [ + MockTileImage( + coordinates: const TileCoordinates(1, 1, 1), + loadFinished: true, + readyToDisplay: true, + ), + MockTileImage( + coordinates: const TileCoordinates(2, 1, 1), + loadFinished: true, + readyToDisplay: true, + ), + ]; + final removalState = TileRemovalState( + tileImages: tileImages, + visibleRange: DiscreteTileRange( + 1, + Bounds(const Point(2, 1), const Point(3, 3)), + ), + keepRange: DiscreteTileRange( + 1, + Bounds(const Point(2, 1), const Point(3, 3)), + ), + evictStrategy: EvictErrorTileStrategy.none, + ); + expect(removalState.tilesToPrune(), [tileImages.first]); + }); + + test('keeps ancestor tile if a tile has not loaded yet', () { + final tileImages = [ + MockTileImage( + coordinates: const TileCoordinates(0, 0, 0), + loadFinished: true, + readyToDisplay: true, + ), + MockTileImage( + coordinates: const TileCoordinates(0, 0, 1), + loadFinished: false, + readyToDisplay: false, + ), + ]; + final removalState = TileRemovalState( + tileImages: tileImages, + visibleRange: DiscreteTileRange( + 1, + Bounds(const Point(0, 0), const Point(0, 0)), + ), + keepRange: DiscreteTileRange( + 1, + Bounds(const Point(0, 0), const Point(0, 0)), + ), + evictStrategy: EvictErrorTileStrategy.none, + ); + expect(removalState.tilesToPrune(), isNot(contains(tileImages.first))); + }); + + test('keeps descendant tile if there is no loaded tile obscuring it', () { + final tileImages = [ + MockTileImage( + coordinates: const TileCoordinates(0, 0, 0), + loadFinished: false, + readyToDisplay: false, + ), + MockTileImage( + coordinates: const TileCoordinates(0, 0, 1), + loadFinished: false, + readyToDisplay: false, + ), + MockTileImage( + coordinates: const TileCoordinates(0, 0, 2), + loadFinished: true, + readyToDisplay: true, + ), + ]; + final removalState = TileRemovalState( + tileImages: tileImages, + visibleRange: DiscreteTileRange( + 1, + Bounds(const Point(0, 0), const Point(0, 0)), + ), + keepRange: DiscreteTileRange( + 1, + Bounds(const Point(0, 0), const Point(0, 0)), + ), + evictStrategy: EvictErrorTileStrategy.none, + ); + expect(removalState.tilesToPrune(), isNot(contains(tileImages.last))); + }); + }); +} + +class MockTileImage extends TileImage { + @override + final bool readyToDisplay; + + MockTileImage({ + required super.coordinates, + required this.readyToDisplay, + required bool loadFinished, + void Function(TileCoordinates coordinates)? onLoadComplete, + void Function(TileImage tile, Object error, StackTrace? stackTrace)? + onLoadError, + TileDisplay? tileDisplay, + super.errorImage, + }) : super( + vsync: const MockTickerProvider(), + imageProvider: testWhiteTileImage, + onLoadComplete: onLoadComplete ?? (_) {}, + onLoadError: onLoadError ?? (_, __, ___) {}, + tileDisplay: const TileDisplay.instantaneous(), + ) { + loadFinishedAt = loadFinished ? DateTime.now() : null; + } +} + +class MockTickerProvider extends TickerProvider { + const MockTickerProvider(); + + @override + Ticker createTicker(TickerCallback onTick) { + return Ticker((elapsed) {}); + } +} diff --git a/test/test_utils/test_app.dart b/test/test_utils/test_app.dart index ca0fc6684..9866d2add 100644 --- a/test/test_utils/test_app.dart +++ b/test/test_utils/test_app.dart @@ -1,18 +1,16 @@ -import 'dart:convert'; - import 'package:flutter/material.dart'; import 'package:flutter_map/src/layer/circle_layer.dart'; import 'package:flutter_map/src/layer/marker_layer.dart'; import 'package:flutter_map/src/layer/polygon_layer.dart'; import 'package:flutter_map/src/layer/polyline_layer.dart'; -import 'package:flutter_map/src/layer/tile_layer/tile_coordinates.dart'; import 'package:flutter_map/src/layer/tile_layer/tile_layer.dart'; -import 'package:flutter_map/src/layer/tile_layer/tile_provider/base_tile_provider.dart'; import 'package:flutter_map/src/map/map_controller.dart'; import 'package:flutter_map/src/map/options.dart'; import 'package:flutter_map/src/map/widget.dart'; import 'package:latlong2/latlong.dart'; +import 'test_tile_provider.dart'; + class TestApp extends StatelessWidget { const TestApp({ super.key, @@ -61,14 +59,3 @@ class TestApp extends StatelessWidget { ); } } - -class TestTileProvider extends TileProvider { - // Base 64 encoded 256x256 white tile. - static const _whiteTile = - 'iVBORw0KGgoAAAANSUhEUgAAAQAAAAEAAQMAAABmvDolAAAAAXNSR0IB2cksfwAAAAlwSFlzAAALEwAACxMBAJqcGAAAAANQTFRF////p8QbyAAAAB9JREFUeJztwQENAAAAwqD3T20ON6AAAAAAAAAAAL4NIQAAAfFnIe4AAAAASUVORK5CYII='; - - @override - ImageProvider getImage( - TileCoordinates coordinates, TileLayer options) => - MemoryImage(base64Decode(_whiteTile)); -} diff --git a/test/test_utils/test_tile_image.dart b/test/test_utils/test_tile_image.dart new file mode 100644 index 000000000..086bee067 --- /dev/null +++ b/test/test_utils/test_tile_image.dart @@ -0,0 +1,8 @@ +import 'dart:convert'; + +import 'package:flutter/painting.dart'; + +// Base 64 encoded 256x256 white tile. +const _whiteTile = + 'iVBORw0KGgoAAAANSUhEUgAAAQAAAAEAAQMAAABmvDolAAAAAXNSR0IB2cksfwAAAAlwSFlzAAALEwAACxMBAJqcGAAAAANQTFRF////p8QbyAAAAB9JREFUeJztwQENAAAAwqD3T20ON6AAAAAAAAAAAL4NIQAAAfFnIe4AAAAASUVORK5CYII='; +final testWhiteTileImage = MemoryImage(base64Decode(_whiteTile)); diff --git a/test/test_utils/test_tile_provider.dart b/test/test_utils/test_tile_provider.dart new file mode 100644 index 000000000..d51f3d285 --- /dev/null +++ b/test/test_utils/test_tile_provider.dart @@ -0,0 +1,13 @@ +import 'package:flutter/rendering.dart'; +import 'package:flutter_map/src/layer/tile_layer/tile_coordinates.dart'; +import 'package:flutter_map/src/layer/tile_layer/tile_layer.dart'; +import 'package:flutter_map/src/layer/tile_layer/tile_provider/base_tile_provider.dart'; + +import 'test_tile_image.dart'; + +class TestTileProvider extends TileProvider { + @override + ImageProvider getImage( + TileCoordinates coordinates, TileLayer options) => + testWhiteTileImage; +} From 0464742aff46b38a9b8c92a415804d4aa494d155 Mon Sep 17 00:00:00 2001 From: Rory Stephenson Date: Mon, 24 Jul 2023 14:07:07 +0200 Subject: [PATCH 06/10] Refactor TileRemovalState to TileImageView --- .../layer/tile_layer/tile_image_manager.dart | 49 +++++++++----- ...emoval_state.dart => tile_image_view.dart} | 66 +++++++------------ ...te_test.dart => tile_image_view_test.dart} | 61 ++++++++++++----- 3 files changed, 98 insertions(+), 78 deletions(-) rename lib/src/layer/tile_layer/{tile_removal_state.dart => tile_image_view.dart} (56%) rename test/layer/tile_layer/{tile_removal_state_test.dart => tile_image_view_test.dart} (70%) diff --git a/lib/src/layer/tile_layer/tile_image_manager.dart b/lib/src/layer/tile_layer/tile_image_manager.dart index 35aa1b9d7..b0ef5191d 100644 --- a/lib/src/layer/tile_layer/tile_image_manager.dart +++ b/lib/src/layer/tile_layer/tile_image_manager.dart @@ -4,9 +4,9 @@ import 'package:flutter_map/src/layer/tile_layer/tile_bounds/tile_bounds_at_zoom import 'package:flutter_map/src/layer/tile_layer/tile_coordinates.dart'; import 'package:flutter_map/src/layer/tile_layer/tile_display.dart'; import 'package:flutter_map/src/layer/tile_layer/tile_image.dart'; +import 'package:flutter_map/src/layer/tile_layer/tile_image_view.dart'; import 'package:flutter_map/src/layer/tile_layer/tile_layer.dart'; import 'package:flutter_map/src/layer/tile_layer/tile_range.dart'; -import 'package:flutter_map/src/layer/tile_layer/tile_removal_state.dart'; typedef TileCreator = TileImage Function(TileCoordinates coordinates); @@ -138,20 +138,33 @@ class TileImageManager { required int pruneBuffer, required EvictErrorTileStrategy evictStrategy, }) { - final pruningState = TileRemovalState( - tileImages: _tiles.values.toSet(), + final pruningState = TileImageView( + tileImages: _tiles, visibleRange: visibleRange, keepRange: visibleRange.expand(pruneBuffer), - evictStrategy: evictStrategy, ); - _evictErrorTiles(pruningState); - _prune(pruningState); + _evictErrorTiles(pruningState, evictStrategy); + _prune(pruningState, evictStrategy); } - void _evictErrorTiles(TileRemovalState tileRemovalState) { - for (final tileImage in tileRemovalState.errorTilesToEvict()) { - _remove(tileImage.coordinatesKey, evictImageFromCache: (_) => true); + void _evictErrorTiles( + TileImageView tileRemovalState, + EvictErrorTileStrategy evictStrategy, + ) { + switch (evictStrategy) { + case EvictErrorTileStrategy.notVisibleRespectMargin: + for (final tileImage + in tileRemovalState.errorTilesOutsideOfKeepMargin()) { + _remove(tileImage.coordinatesKey, evictImageFromCache: (_) => true); + } + case EvictErrorTileStrategy.notVisible: + for (final tileImage in tileRemovalState.errorTilesNotVisible()) { + _remove(tileImage.coordinatesKey, evictImageFromCache: (_) => true); + } + case EvictErrorTileStrategy.dispose: + case EvictErrorTileStrategy.none: + return; } } @@ -161,21 +174,21 @@ class TileImageManager { required EvictErrorTileStrategy evictStrategy, }) { _prune( - TileRemovalState( - tileImages: _tiles.values.toSet(), + TileImageView( + tileImages: _tiles, visibleRange: visibleRange, keepRange: visibleRange.expand(pruneBuffer), - evictStrategy: evictStrategy, ), + evictStrategy, ); } - void _prune(TileRemovalState tileRemovalState) { - for (final tileImage in tileRemovalState.tilesToPrune()) { - _removeWithDefaultEviction( - tileImage.coordinatesKey, - tileRemovalState.evictStrategy, - ); + void _prune( + TileImageView tileRemovalState, + EvictErrorTileStrategy evictStrategy, + ) { + for (final tileImage in tileRemovalState.staleTiles()) { + _removeWithDefaultEviction(tileImage.coordinatesKey, evictStrategy); } } } diff --git a/lib/src/layer/tile_layer/tile_removal_state.dart b/lib/src/layer/tile_layer/tile_image_view.dart similarity index 56% rename from lib/src/layer/tile_layer/tile_removal_state.dart rename to lib/src/layer/tile_layer/tile_image_view.dart index 74b2fd38e..7aca827e2 100644 --- a/lib/src/layer/tile_layer/tile_removal_state.dart +++ b/lib/src/layer/tile_layer/tile_image_view.dart @@ -1,59 +1,39 @@ +import 'dart:collection'; + import 'package:flutter_map/src/layer/tile_layer/tile_coordinates.dart'; import 'package:flutter_map/src/layer/tile_layer/tile_image.dart'; -import 'package:flutter_map/src/layer/tile_layer/tile_layer.dart'; import 'package:flutter_map/src/layer/tile_layer/tile_range.dart'; -class TileRemovalState { - final EvictErrorTileStrategy evictStrategy; - final Map _tileImages; +class TileImageView { + final Map _tileImages; final DiscreteTileRange _visibleRange; final DiscreteTileRange _keepRange; - Set? _visibleTilesMemo; - Set? _keepTilesMemo; - - TileRemovalState({ - required Iterable tileImages, + TileImageView({ + required Map tileImages, required DiscreteTileRange visibleRange, required DiscreteTileRange keepRange, - required this.evictStrategy, - }) : _tileImages = Map.unmodifiable({ - for (var tileImage in tileImages) tileImage.coordinates: tileImage - }), + }) : _tileImages = UnmodifiableMapView(tileImages), _visibleRange = visibleRange, _keepRange = keepRange; - Set get _visibleTiles => - _visibleTilesMemo ??= Set.unmodifiable(_tileImages.values - .where((tileImage) => _visibleRange.contains(tileImage.coordinates)) - .toSet()); - - Set get _keepTiles => - _keepTilesMemo ??= Set.unmodifiable(_tileImages.values - .where((tileImage) => _keepRange.contains(tileImage.coordinates)) - .toSet()); + List errorTilesOutsideOfKeepMargin() => _tileImages.values + .where((tileImage) => + tileImage.loadError && !_keepRange.contains(tileImage.coordinates)) + .toList(); - List errorTilesToEvict() { - switch (evictStrategy) { - case EvictErrorTileStrategy.notVisibleRespectMargin: - return _tileImages.values - .where((tileImage) => !_keepTiles.contains(tileImage)) - .toList(); - case EvictErrorTileStrategy.notVisible: - return _tileImages.values - .where((tileImage) => !_visibleTiles.contains(tileImage)) - .toList(); - case EvictErrorTileStrategy.dispose: - case EvictErrorTileStrategy.none: - return const []; - } - } + List errorTilesNotVisible() => _tileImages.values + .where((tileImage) => + tileImage.loadError && !_visibleRange.contains(tileImage.coordinates)) + .toList(); - List tilesToPrune() { - final retain = Set.from(_keepTiles); + List staleTiles() { + final tilesInKeepRange = _tileImages.values + .where((tileImage) => _keepRange.contains(tileImage.coordinates)); + final retain = Set.from(tilesInKeepRange); - for (final tile in _tileImages.values) { - if (_keepTiles.contains(tile) && !tile.readyToDisplay) { + for (final tile in tilesInKeepRange) { + if (!tile.readyToDisplay) { final coords = tile.coordinates; if (!_retainAncestor( retain, coords.x, coords.y, coords.z, coords.z - 5)) { @@ -82,7 +62,7 @@ class TileRemovalState { final z2 = z - 1; final coords2 = TileCoordinates(x2, y2, z2); - final tile = _tileImages[coords2]; + final tile = _tileImages[coords2.key]; if (tile != null) { if (tile.readyToDisplay) { retain.add(tile); @@ -112,7 +92,7 @@ class TileRemovalState { for (var j = 2 * y; j < 2 * y + 2; j++) { final coords = TileCoordinates(i, j, z + 1); - final tile = _tileImages[coords]; + final tile = _tileImages[coords.key]; if (tile != null) { if (tile.readyToDisplay || tile.loadFinishedAt != null) { retain.add(tile); diff --git a/test/layer/tile_layer/tile_removal_state_test.dart b/test/layer/tile_layer/tile_image_view_test.dart similarity index 70% rename from test/layer/tile_layer/tile_removal_state_test.dart rename to test/layer/tile_layer/tile_image_view_test.dart index 50f8a47a5..e0552a782 100644 --- a/test/layer/tile_layer/tile_removal_state_test.dart +++ b/test/layer/tile_layer/tile_image_view_test.dart @@ -1,18 +1,38 @@ +import 'dart:collection'; import 'dart:math'; import 'package:flutter/src/scheduler/ticker.dart'; import 'package:flutter_map/flutter_map.dart'; +import 'package:flutter_map/src/layer/tile_layer/tile_image_view.dart'; import 'package:flutter_map/src/layer/tile_layer/tile_range.dart'; -import 'package:flutter_map/src/layer/tile_layer/tile_removal_state.dart'; import 'package:flutter_map/src/misc/private/bounds.dart'; import 'package:test/test.dart'; import '../../test_utils/test_tile_image.dart'; void main() { - group('tilesToPrune', () { + Map tileImagesMappingFrom(List tileImages) => + // Unmodifiable ensures we don't modify the tileImage collection in + // TileImageView + UnmodifiableMapView({ + for (final tileImage in tileImages) tileImage.coordinates.key: tileImage + }); + + Matcher containsTileImage( + Map tileImages, + TileCoordinates coordinates, + ) => + contains(tileImages[coordinates.key]!); + + Matcher doesNotContainTileImage( + Map tileImages, + TileCoordinates coordinates, + ) => + isNot(containsTileImage(tileImages, coordinates)); + + group('staleTiles', () { test('prunes tiles outside of the visible range', () { - final tileImages = [ + final tileImages = tileImagesMappingFrom([ MockTileImage( coordinates: const TileCoordinates(1, 1, 1), loadFinished: true, @@ -23,8 +43,9 @@ void main() { loadFinished: true, readyToDisplay: true, ), - ]; - final removalState = TileRemovalState( + ]); + + final removalState = TileImageView( tileImages: tileImages, visibleRange: DiscreteTileRange( 1, @@ -34,13 +55,15 @@ void main() { 1, Bounds(const Point(2, 1), const Point(3, 3)), ), - evictStrategy: EvictErrorTileStrategy.none, ); - expect(removalState.tilesToPrune(), [tileImages.first]); + expect( + removalState.staleTiles(), + containsTileImage(tileImages, const TileCoordinates(1, 1, 1)), + ); }); test('keeps ancestor tile if a tile has not loaded yet', () { - final tileImages = [ + final tileImages = tileImagesMappingFrom([ MockTileImage( coordinates: const TileCoordinates(0, 0, 0), loadFinished: true, @@ -51,8 +74,8 @@ void main() { loadFinished: false, readyToDisplay: false, ), - ]; - final removalState = TileRemovalState( + ]); + final removalState = TileImageView( tileImages: tileImages, visibleRange: DiscreteTileRange( 1, @@ -62,13 +85,15 @@ void main() { 1, Bounds(const Point(0, 0), const Point(0, 0)), ), - evictStrategy: EvictErrorTileStrategy.none, ); - expect(removalState.tilesToPrune(), isNot(contains(tileImages.first))); + expect( + removalState.staleTiles(), + doesNotContainTileImage(tileImages, const TileCoordinates(0, 0, 0)), + ); }); test('keeps descendant tile if there is no loaded tile obscuring it', () { - final tileImages = [ + final tileImages = tileImagesMappingFrom([ MockTileImage( coordinates: const TileCoordinates(0, 0, 0), loadFinished: false, @@ -84,8 +109,8 @@ void main() { loadFinished: true, readyToDisplay: true, ), - ]; - final removalState = TileRemovalState( + ]); + final removalState = TileImageView( tileImages: tileImages, visibleRange: DiscreteTileRange( 1, @@ -95,9 +120,11 @@ void main() { 1, Bounds(const Point(0, 0), const Point(0, 0)), ), - evictStrategy: EvictErrorTileStrategy.none, ); - expect(removalState.tilesToPrune(), isNot(contains(tileImages.last))); + expect( + removalState.staleTiles(), + doesNotContainTileImage(tileImages, const TileCoordinates(0, 0, 2)), + ); }); }); } From d17673550ded72cfd81d9bf0caeb75186f9470aa Mon Sep 17 00:00:00 2001 From: Rory Stephenson Date: Mon, 24 Jul 2023 14:57:45 +0200 Subject: [PATCH 07/10] Add tests for error image methods in TileImageView --- .../tile_layer/tile_image_view_test.dart | 189 +++++++++++------- 1 file changed, 117 insertions(+), 72 deletions(-) diff --git a/test/layer/tile_layer/tile_image_view_test.dart b/test/layer/tile_layer/tile_image_view_test.dart index e0552a782..43e72b855 100644 --- a/test/layer/tile_layer/tile_image_view_test.dart +++ b/test/layer/tile_layer/tile_image_view_test.dart @@ -1,4 +1,3 @@ -import 'dart:collection'; import 'dart:math'; import 'package:flutter/src/scheduler/ticker.dart'; @@ -11,12 +10,9 @@ import 'package:test/test.dart'; import '../../test_utils/test_tile_image.dart'; void main() { - Map tileImagesMappingFrom(List tileImages) => - // Unmodifiable ensures we don't modify the tileImage collection in - // TileImageView - UnmodifiableMapView({ + Map tileImagesMappingFrom(List tileImages) => { for (final tileImage in tileImages) tileImage.coordinates.key: tileImage - }); + }; Matcher containsTileImage( Map tileImages, @@ -30,31 +26,29 @@ void main() { ) => isNot(containsTileImage(tileImages, coordinates)); + DiscreteTileRange discreteTileRange( + int x1, + int y1, + int x2, + int y2, { + required int zoom, + }) => + DiscreteTileRange( + zoom, + Bounds(Point(x1, y1), Point(x2, y2)), + ); + group('staleTiles', () { - test('prunes tiles outside of the visible range', () { + test('tiles outside of the keep range are stale', () { final tileImages = tileImagesMappingFrom([ - MockTileImage( - coordinates: const TileCoordinates(1, 1, 1), - loadFinished: true, - readyToDisplay: true, - ), - MockTileImage( - coordinates: const TileCoordinates(2, 1, 1), - loadFinished: true, - readyToDisplay: true, - ), + MockTileImage(1, 1, 1), + MockTileImage(2, 1, 1), ]); final removalState = TileImageView( tileImages: tileImages, - visibleRange: DiscreteTileRange( - 1, - Bounds(const Point(2, 1), const Point(3, 3)), - ), - keepRange: DiscreteTileRange( - 1, - Bounds(const Point(2, 1), const Point(3, 3)), - ), + visibleRange: discreteTileRange(2, 1, 3, 3, zoom: 1), + keepRange: discreteTileRange(2, 1, 3, 3, zoom: 1), ); expect( removalState.staleTiles(), @@ -62,29 +56,15 @@ void main() { ); }); - test('keeps ancestor tile if a tile has not loaded yet', () { + test('ancestor tile is not stale if a tile has not loaded yet', () { final tileImages = tileImagesMappingFrom([ - MockTileImage( - coordinates: const TileCoordinates(0, 0, 0), - loadFinished: true, - readyToDisplay: true, - ), - MockTileImage( - coordinates: const TileCoordinates(0, 0, 1), - loadFinished: false, - readyToDisplay: false, - ), + MockTileImage(0, 0, 0), + MockTileImage(0, 0, 1, loadFinished: false, readyToDisplay: false), ]); final removalState = TileImageView( tileImages: tileImages, - visibleRange: DiscreteTileRange( - 1, - Bounds(const Point(0, 0), const Point(0, 0)), - ), - keepRange: DiscreteTileRange( - 1, - Bounds(const Point(0, 0), const Point(0, 0)), - ), + visibleRange: discreteTileRange(0, 0, 0, 0, zoom: 1), + keepRange: discreteTileRange(0, 0, 0, 0, zoom: 1), ); expect( removalState.staleTiles(), @@ -92,40 +72,100 @@ void main() { ); }); - test('keeps descendant tile if there is no loaded tile obscuring it', () { + test('descendant tile is not stale if there is no loaded tile obscuring it', + () { final tileImages = tileImagesMappingFrom([ - MockTileImage( - coordinates: const TileCoordinates(0, 0, 0), - loadFinished: false, - readyToDisplay: false, - ), - MockTileImage( - coordinates: const TileCoordinates(0, 0, 1), - loadFinished: false, - readyToDisplay: false, - ), - MockTileImage( - coordinates: const TileCoordinates(0, 0, 2), - loadFinished: true, - readyToDisplay: true, - ), + MockTileImage(0, 0, 0, loadFinished: false, readyToDisplay: false), + MockTileImage(0, 0, 1, loadFinished: false, readyToDisplay: false), + MockTileImage(0, 0, 2), ]); final removalState = TileImageView( tileImages: tileImages, - visibleRange: DiscreteTileRange( - 1, - Bounds(const Point(0, 0), const Point(0, 0)), - ), - keepRange: DiscreteTileRange( - 1, - Bounds(const Point(0, 0), const Point(0, 0)), - ), + visibleRange: discreteTileRange(0, 0, 0, 0, zoom: 1), + keepRange: discreteTileRange(0, 0, 0, 0, zoom: 1), ); expect( removalState.staleTiles(), doesNotContainTileImage(tileImages, const TileCoordinates(0, 0, 2)), ); }); + + test( + 'returned elements can be removed from the source collection in a for loop', + () { + final tileImages = tileImagesMappingFrom([ + MockTileImage(1, 1, 1), + ]); + + final removalState = TileImageView( + tileImages: tileImages, + visibleRange: discreteTileRange(2, 1, 3, 3, zoom: 1), + keepRange: discreteTileRange(2, 1, 3, 3, zoom: 1), + ); + expect( + removalState.staleTiles(), + containsTileImage(tileImages, const TileCoordinates(1, 1, 1)), + ); + // If an iterator over the original collection is returned then when + // looping over that iterator and removing from the original collection + // a concurrent modification exception is thrown. This ensures that the + // returned collection is not an iterable over the original collection. + for (final staleTile in removalState.staleTiles()) { + tileImages.remove(staleTile.coordinatesKey)!; + } + }); + }); + + test('errorTilesOutsideOfKeepMargin', () { + final tileImages = tileImagesMappingFrom([ + MockTileImage(1, 1, 1, loadError: true), + MockTileImage(2, 1, 1), + MockTileImage(1, 2, 1), + MockTileImage(2, 2, 1, loadError: true), + ]); + final tileImageView = TileImageView( + tileImages: tileImages, + visibleRange: discreteTileRange(1, 2, 1, 2, zoom: 1), + keepRange: discreteTileRange(1, 2, 2, 2, zoom: 1), + ); + expect( + tileImageView.errorTilesOutsideOfKeepMargin().map((e) => e.coordinates), + [const TileCoordinates(1, 1, 1)], + ); + + // If an iterator over the original collection is returned then when + // looping over that iterator and removing from the original collection + // a concurrent modification exception is thrown. This ensures that the + // returned collection is not an iterable over the original collection. + for (final tileImage in tileImageView.errorTilesOutsideOfKeepMargin()) { + tileImages.remove(tileImage.coordinatesKey)!; + } + }); + + test('errorTilesNotVisible', () { + final tileImages = tileImagesMappingFrom([ + MockTileImage(1, 1, 1, loadError: true), + MockTileImage(2, 1, 1), + MockTileImage(1, 2, 1), + MockTileImage(2, 2, 1, loadError: true), + ]); + final tileImageView = TileImageView( + tileImages: tileImages, + visibleRange: discreteTileRange(1, 2, 1, 2, zoom: 1), + keepRange: discreteTileRange(1, 2, 2, 2, zoom: 1), + ); + expect( + tileImageView.errorTilesNotVisible().map((e) => e.coordinates), + [const TileCoordinates(1, 1, 1), const TileCoordinates(2, 2, 1)], + ); + + // If an iterator over the original collection is returned then when + // looping over that iterator and removing from the original collection + // a concurrent modification exception is thrown. This ensures that the + // returned collection is not an iterable over the original collection. + for (final tileImage in tileImageView.errorTilesOutsideOfKeepMargin()) { + tileImages.remove(tileImage.coordinatesKey)!; + } }); } @@ -133,16 +173,20 @@ class MockTileImage extends TileImage { @override final bool readyToDisplay; - MockTileImage({ - required super.coordinates, - required this.readyToDisplay, - required bool loadFinished, + MockTileImage( + int x, + int y, + int zoom, { + this.readyToDisplay = true, + bool loadFinished = true, + bool loadError = false, void Function(TileCoordinates coordinates)? onLoadComplete, void Function(TileImage tile, Object error, StackTrace? stackTrace)? onLoadError, TileDisplay? tileDisplay, super.errorImage, }) : super( + coordinates: TileCoordinates(x, y, zoom), vsync: const MockTickerProvider(), imageProvider: testWhiteTileImage, onLoadComplete: onLoadComplete ?? (_) {}, @@ -150,6 +194,7 @@ class MockTileImage extends TileImage { tileDisplay: const TileDisplay.instantaneous(), ) { loadFinishedAt = loadFinished ? DateTime.now() : null; + this.loadError = loadError; } } From 6631602fd4100d42977d734ef85506395e84ecc3 Mon Sep 17 00:00:00 2001 From: Rory Stephenson Date: Mon, 24 Jul 2023 16:20:40 +0200 Subject: [PATCH 08/10] Update documentation and prevent under-pruning of tiles caused by adding the panBuffer and keepBuffer rather than choosing the higher of the two --- lib/src/layer/tile_layer/tile_layer.dart | 13 +++++++------ lib/src/layer/tile_layer/tile_layer_options.dart | 8 +++++--- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/lib/src/layer/tile_layer/tile_layer.dart b/lib/src/layer/tile_layer/tile_layer.dart index 965f759d6..892a7e98a 100644 --- a/lib/src/layer/tile_layer/tile_layer.dart +++ b/lib/src/layer/tile_layer/tile_layer.dart @@ -145,10 +145,11 @@ class TileLayer extends StatefulWidget { /// unloading them. final int keepBuffer; - /// When panning the map, extend the tilerange by this many tiles in each - /// direction. - /// Will cause extra tile loads, and impact performance. - /// Be careful increasing this beyond 0 or 1. + /// When loading tiles only visible tiles are loaded by default. This option + /// increases the loaded tiles by the given number on both axis which can help + /// prevent the user from seeing loading tiles whilst panning. Setting the + /// pan buffer too high can impact performance, typically this is set to zero + /// or one. final int panBuffer; /// Tile image to show in place of the tile that failed to load. @@ -575,7 +576,7 @@ class _TileLayerState extends State with TickerProviderStateMixin { _tileImageManager.evictAndPrune( visibleRange: visibleTileRange, - pruneBuffer: widget.panBuffer + widget.keepBuffer, + pruneBuffer: math.max(widget.panBuffer, widget.keepBuffer), evictStrategy: widget.evictErrorTileStrategy, ); } @@ -673,7 +674,7 @@ class _TileLayerState extends State with TickerProviderStateMixin { ); _tileImageManager.prune( visibleRange: visibleTileRange, - pruneBuffer: widget.panBuffer + widget.keepBuffer, + pruneBuffer: math.max(widget.panBuffer, widget.keepBuffer), evictStrategy: widget.evictErrorTileStrategy, ); } diff --git a/lib/src/layer/tile_layer/tile_layer_options.dart b/lib/src/layer/tile_layer/tile_layer_options.dart index 1e65364e2..fc18e2dcb 100644 --- a/lib/src/layer/tile_layer/tile_layer_options.dart +++ b/lib/src/layer/tile_layer/tile_layer_options.dart @@ -4,11 +4,13 @@ typedef TemplateFunction = String Function( String str, Map data); enum EvictErrorTileStrategy { - // never evict error Tiles + /// Never evict error Tiles. none, - // evict error Tiles during _pruneTiles / _abortLoading calls + + /// Evict error Tiles during pruning. dispose, - // evict error Tiles which are not visible anymore but respect margin (see keepBuffer option) + + /// Evict error Tiles which are not visible anymore but respect margin (see keepBuffer option) // (Tile's zoom level not equals current _tileZoom or Tile is out of viewport) notVisibleRespectMargin, // evict error Tiles which are not visible anymore From d9d20a1f3c1452498e56864859389f77f06dd6a0 Mon Sep 17 00:00:00 2001 From: Rory Stephenson Date: Mon, 24 Jul 2023 18:04:51 +0200 Subject: [PATCH 09/10] Documentation and method name improvements --- example/lib/pages/tile_loading_error_handle.dart | 6 +++--- lib/src/layer/tile_layer/tile_image_manager.dart | 9 ++++++--- lib/src/layer/tile_layer/tile_layer_options.dart | 16 ++++++++++------ 3 files changed, 19 insertions(+), 12 deletions(-) diff --git a/example/lib/pages/tile_loading_error_handle.dart b/example/lib/pages/tile_loading_error_handle.dart index f958af367..577b12704 100644 --- a/example/lib/pages/tile_loading_error_handle.dart +++ b/example/lib/pages/tile_loading_error_handle.dart @@ -16,7 +16,7 @@ class TileLoadingErrorHandle extends StatefulWidget { } class _TileLoadingErrorHandleState extends State { - static const _showSnackbarDuration = Duration(seconds: 1); + static const _showSnackBarDuration = Duration(seconds: 1); bool _simulateTileLoadErrors = false; DateTime? _lastShowedTileLoadError; @@ -59,7 +59,7 @@ class _TileLoadingErrorHandleState extends State { _lastShowedTileLoadError = DateTime.now(); WidgetsBinding.instance.addPostFrameCallback((_) { ScaffoldMessenger.of(context).showSnackBar(SnackBar( - duration: _showSnackbarDuration, + duration: _showSnackBarDuration, content: Text( error.toString(), style: const TextStyle(color: Colors.black), @@ -87,7 +87,7 @@ class _TileLoadingErrorHandleState extends State { _lastShowedTileLoadError == null || DateTime.now().difference(_lastShowedTileLoadError!) - const Duration(milliseconds: 50) > - _showSnackbarDuration; + _showSnackBarDuration; } class _SimulateErrorsTileProvider extends TileProvider { diff --git a/lib/src/layer/tile_layer/tile_image_manager.dart b/lib/src/layer/tile_layer/tile_image_manager.dart index b0ef5191d..8c87f5170 100644 --- a/lib/src/layer/tile_layer/tile_image_manager.dart +++ b/lib/src/layer/tile_layer/tile_image_manager.dart @@ -96,7 +96,10 @@ class TileImageManager { } } - void _removeWithDefaultEviction(String key, EvictErrorTileStrategy strategy) { + void _removeWithEvictionStrategy( + String key, + EvictErrorTileStrategy strategy, + ) { _remove( key, evictImageFromCache: (tileImage) => @@ -108,7 +111,7 @@ class TileImageManager { final toRemove = Map.from(_tiles); for (final key in toRemove.keys) { - _removeWithDefaultEviction(key, evictStrategy); + _removeWithEvictionStrategy(key, evictStrategy); } } @@ -188,7 +191,7 @@ class TileImageManager { EvictErrorTileStrategy evictStrategy, ) { for (final tileImage in tileRemovalState.staleTiles()) { - _removeWithDefaultEviction(tileImage.coordinatesKey, evictStrategy); + _removeWithEvictionStrategy(tileImage.coordinatesKey, evictStrategy); } } } diff --git a/lib/src/layer/tile_layer/tile_layer_options.dart b/lib/src/layer/tile_layer/tile_layer_options.dart index fc18e2dcb..f04aec09b 100644 --- a/lib/src/layer/tile_layer/tile_layer_options.dart +++ b/lib/src/layer/tile_layer/tile_layer_options.dart @@ -4,17 +4,21 @@ typedef TemplateFunction = String Function( String str, Map data); enum EvictErrorTileStrategy { - /// Never evict error Tiles. + /// Never evict images for tiles which failed to load. none, - /// Evict error Tiles during pruning. + /// Evict images for tiles which failed to load when they are pruned. dispose, - /// Evict error Tiles which are not visible anymore but respect margin (see keepBuffer option) - // (Tile's zoom level not equals current _tileZoom or Tile is out of viewport) + /// Evict images for tiles which failed to load and: + /// - do not belong to the current zoom level AND/OR + /// - are not visible, respecting the pruning buffer (the maximum of the + /// [keepBuffer] and [panBuffer]. notVisibleRespectMargin, - // evict error Tiles which are not visible anymore - // (Tile's zoom level not equals current _tileZoom or Tile is out of viewport) + + /// Evict images for tiles which failed to load and: + /// - do not belong to the current zoom level AND/OR + /// - are not visible notVisible, } From 28c2513d0db0b4aede30444f22301c7fe387a17f Mon Sep 17 00:00:00 2001 From: Luka S Date: Mon, 24 Jul 2023 17:55:58 +0100 Subject: [PATCH 10/10] Updated `TileCoordinates` equality method --- lib/src/layer/tile_layer/tile_coordinates.dart | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/src/layer/tile_layer/tile_coordinates.dart b/lib/src/layer/tile_layer/tile_coordinates.dart index c6e65baab..724a08794 100644 --- a/lib/src/layer/tile_layer/tile_coordinates.dart +++ b/lib/src/layer/tile_layer/tile_coordinates.dart @@ -21,9 +21,11 @@ class TileCoordinates extends Point { @override bool operator ==(Object other) { - if (other is! TileCoordinates) return false; - - return x == other.x && y == other.y && z == other.z; + if (identical(this, other)) return true; + return other is TileCoordinates && + other.x == x && + other.y == y && + other.z == z; } @override