Refactor tile removal

[rorystephenson]

Tile pruning/removal is complicated and hard to follow because the relevant code is spread around various classes and not executed in a linear fashion. When loading tiles their 'current' value is updated and then later when pruning it is used to determine whether a tile should be kept or not. This PR attempts to simplify error tile removal and pruning by making it a linear operation and avoiding changing the state of TileImages during this operation.

One big benefit of this change is testing tile pruning/evicting becomes much easier. This is important because there are tricky scenarios that we need to support like keeping tiles visible whilst zooming out until a tile at a lower level is fully loaded. In fact when making these changes I realised that right now we have a bug in this. Try using the web demo, disabling internet and zooming out. The tiles from the previous higher zoom disappear.

[rorystephenson]

@ibrierley I figure it makes more sense to continue discussing your comment on improving pruning here.

I don't completely understand the difference in what you have proposed, but I'm definitely keen to explore any options for improving this logic.

[JaffaKetchup]

@rorystephenson I think Ian's on holiday for now.

I will say that maybe it would be a good idea to mix a partial resolution for #1430 into this. Essentially, just a way for the image provider and/or tile provider to detect that the tile has been pruned: this will allow for future usage to cancel the ongoing HTTP request.

[rorystephenson]

@JaffaKetchup if I understand right that issue is about having a mechanism for cancelling in-progress tile requests?

I think that's definitely a worthwhile feature. It sounds like some people who want it should actually use the tile transformer to prevent loading lots of tiles during movement but that doesn't cover all use cases.

I wonder if it belongs in a separate PR to make the change history clearer.

[JaffaKetchup]

I think that's definitely a worthwhile feature. It sounds like some people who want it should actually use the tile transformer to prevent loading lots of tiles during movement but that doesn't cover all use cases.

The issue with that is it would delay loading the tiles that are actually wanted.

I wonder if it belongs in a separate PR to make the change history clearer.

Possibly. The actual HTTP cancel mechanism is not yet implemented in Dart (dart:http), but it may be soon. I was just suggesting implementing the rough framework (as they both cover pruning): I made a rough implementation with a shared Completer<void> that was triggered by the pruning logic and listened to by the image provider.

[rorystephenson]

I think that's definitely a worthwhile feature. It sounds like some people who want it should actually use the tile transformer to prevent loading lots of tiles during movement but that doesn't cover all use cases.

The issue with that is it would delay loading the tiles that are actually wanted.

I am specifically referring to animating from one position/zoom to another and using the tile update transformer like it is used in the animated map controller example and flutter_map_animations. In that case a load is triggered at the destination position and pruning is paused until we reach the destination. This doesn't cover cases like a fling animation or people who wouldn't want to use the tile transformer for whatever reason so we still need to be able to cancel pending loads.

I wonder if it belongs in a separate PR to make the change history clearer.

Possibly. The actual HTTP cancel mechanism is not yet implemented in Dart (dart:http), but it may be soon. I was just suggesting implementing the rough framework (as they both cover pruning): I made a rough implementation with a shared Completer<void> that was triggered by the pruning logic and listened to by the image provider.

I will happily look at this in a follow-up PR. I'm hoping to constrain this PR to refactoring and bug fixing as much as possible.

---

On another note it's not clear to me exactly why we need to have a separate error tile eviction handler. I wonder if that functionality has been broken over time or possibly I just misunderstand it. It seems like it was added to resolve a bug where a failed tile load is cached and thus the tile always fails to load. I'm curious why there are different options for how this is done (visible/keep margin and dispose/none).

Here's what I think is missing for this PR to be done:

• Understand if/why tile eviction needs customisable behaviour (EvictErrorTileStragegy). Add tests to cover important behaviour there.
• Consider whether we can improve handling of visible error tiles. One scenario I have run in to which is a bit weird is when successfully loading tiles, then disabling internet and zooming out, enabling internet and moving around you end up with a kind of border. We should have some kind of mechanism manually or automatically reloading visible error tiles to resolve this. The issue is demonstrated here:
  

I am not 100% this should be addressed in this PR either.

[JaffaKetchup]

whatever reason so we still need to be able to cancel pending loads.

Yeah, this was more intended for slow networks. There's a limit on the number of IO HTTP threads, so on a slow network, a standard user pinch zoom gesture would need to wait for all the tiles to fully load before loading the final (useful) ones.

---

I wasn't aware of the different pruning options for failed tiles, but it is indeed intended to handle outdated cached tiles. Perhaps no customizability is needed, I can't think of why it would be. Error tiles should work the same/similar way as other tiles, but they should be more aggressively reloaded.

Consider whether we can improve handling of visible error tiles. One scenario I have run in to which is a bit weird is when successfully loading tiles, then disabling internet and zooming out, enabling internet and moving around you end up with a kind of border. We should have some kind of mechanism manually or automatically reloading visible error tiles to resolve this.

Agreed, I have noticed this before.

Maybe we group this in with another PR with the cancel loads feature.

[rorystephenson]

whatever reason so we still need to be able to cancel pending loads.

Yeah, this was more intended for slow networks. There's a limit on the number of IO HTTP threads, so on a slow network, a standard user pinch zoom gesture would need to wait for all the tiles to fully load before loading the final (useful) ones.

Nice that's a good use-case to be aware of.

I wasn't aware of the different pruning options for failed tiles, but it is indeed intended to handle outdated cached tiles. Perhaps no customizability is needed, I can't think of why it would be. Error tiles should work the same/similar way as other tiles, but they should be more aggressively reloaded.

This was initially added in #576 although I'm not even sure if it's necessary anymore. It was specifically added to resolve a bug where tiles that previously loaded with an error would have their (non-existent?) image cached and when re-loaded would not show any image. I have tested with the default eviction and tile provider (no eviction, NetworkTileProvider) as follows and the bug was not reproduced (the new tiles loaded):

1. Load some tiles.
2. Turn off internet and zoom in (tiles will not load, old zoom tiles remain visible).
3. Zoom back out to original zoom.
4. Turn internet back on.
5. Zoom in.

So I wonder if, when using a different TileProvider, this is still relevant or not?

Consider whether we can improve handling of visible error tiles. One scenario I have run in to which is a bit weird is when successfully loading tiles, then disabling internet and zooming out, enabling internet and moving around you end up with a kind of border. We should have some kind of mechanism manually or automatically reloading visible error tiles to resolve this.

Agreed, I have noticed this before.

Maybe we group this in with another PR with the cancel loads feature.

Yeah I think that sounds like a good idea.

[JaffaKetchup]

I have tested with the default eviction and tile provider (no eviction, NetworkTileProvider) as follows and the bug was not reproduced (the new tiles loaded):

Sure, if you manually reload the tiles, they will reload. However, as your screen recording above shows, they will not reload automatically. Perhaps we should try to re-request them on any interaction, even if we would not usually refresh/prune/add/remove them. Or maybe we need a timer of some sort.

[rorystephenson]

I have tested with the default eviction and tile provider (no eviction, NetworkTileProvider) as follows and the bug was not reproduced (the new tiles loaded):

Sure, if you manually reload the tiles, they will reload. However, as your screen recording above shows, they will not reload automatically. Perhaps we should try to re-request them on any interaction, even if we would not usually refresh/prune/add/remove them. Or maybe we need a timer of some sort.

I definitely agree with this although, despite what the name implies, this is not what the EvictErrorTileStrategy option was initially intended to do. It was introduced to fix a bug where previously errored tiles couldn't be loaded anymore because there was a (presumably blank) entry in the ImageProvider's image cache for the error tile. The fix was to call evict() on the imageProvider of the tile which failed to load which would remove the ImageProvider's cached image entry for the error tile. It seems that this has since been fixed by other changes (there have been a lot of changes to tile providers since this option was introduced) or it is only necessary when using certain tile providers since it doesn't seem to be necessary when using NetworkTileProvider. For some reason a secondary behaviour was added in the original PR: when using EvictErrorTileStragegy.notVisible or EvictErrorTileStragegy.notVisibleRespectMargin it is possible that some error tiles will be pruned when they wouldn't usually be. That PR's name was changed to "WIP: Evict error tiles" and was then merged so I suspect is was merged in an unfinished state.

Here's what the different EvictErrorTileStrategy values do:

• none: Never evict the image cache entry for an error tile even when pruning that tile. I don't know why you wouldn't want to evict the cache entry of a tile which failed to load? In the testing mentioned previously we don't seem to cache the errored tile images when NetworkTileProvider is used so either it only affects some tile providers or this has been made redundant by changes to tile providers since this option was introduced (e.g. we used to add an entry to the image cache even when loading errored but now we don't). I suspect this option was only introduced to maintain existing behaviour which seems unnecessary as the existing behaviour was clearly broken?
• dispose: Evict the entry from the image cache when an error tile is pruned.
• notVisibleRespectMargin: Remove the tile from the tile manager and evict the entry from the image cache when an error tile is not from the current zoom or is outside of the keep bounds (visible tile bounds expanded by the max of keepBuffer and panBuffer). This is different from dispose because normal pruning keeps an error tile if it is the ancestor/descendant of a visible tile which has not finished loading yet whereas this option will remove the tile.
• notVisible: Like notVisibleRespectMargin except it uses the visible bounds instead of keep bounds.

Note that with the last two options they affect more than just image cache eviction for error tiles. They can actually cause removal (pruning) of error tiles when they wouldn't usually be pruned and thus those error tiles will be reloaded on the next movement/tile event. These options will not prune tiles in the visible or keep bounds and reloading of those tiles won't happen until the next tile load is triggered by further movement. These options seem far too limited to me and EvictErrorTileStrategy seems to be a leaky abstraction in the sense that it's controlling more than just error tile image eviction (eviction != pruning).

We need to figure out if evicting entries from the image cache is necessary for error tiles. This depends on whether tile providers behave differently (is it possible for a error tile image to be cached with some providers?) and whether an entry is added to the image cache for error tiles at all. Depending on those two factors we should either always or never evict error tile image cache entries.

Apart from that we should aim to provide better behaviour when tiles fail to load, some scenarios (let me know if you can think of others):

• Internet dropped out which caused some loading failures. When internet comes back it would be great if app developers had the ability to detect the internet returning (connectivity package supports this) and could trigger error tile reloads via MapController.
• The internet is slow which cause some tiles to time-out and fail to load. It would be great if an option existed to automatically retry (with exponential backoff). This is a complicated issue because we need to avoid retrying many failed loads at once which is just going to cause even more load failures on a bad connection.

The desired behaviour also depends on the TileProvider used. It's unlikely that a user will want to retry a tile load for tiles that are provided by AssetTileProvider.

Since the original error image eviction behaviour was added a lot has changed and I think, now that tiles are represented by TileImage which is a ChangeNotifier, it may be simpler and more efficient to trigger a reload of tiles that failed to load instead of pruning them and then adding them back again in the next tile load.

Given the complexity of all of this it may make more sense to first finish this PR which will simplify further pruning/evicting changes before attempting to tackle error tile reloads in a separate PR.

[JaffaKetchup]

Ok, lets leave all of that for another PR.
I guess we need to pass a lot more control through to the TileProvider, to allow it to decide how it works (as you mentioned with the differences between assets and networks).
We should probably resolve both of the methods you mentioned with a backoff timer. We don't want the user to have to monitor the network, and we also can't rely on a plugin to do that for ourselves.

[JaffaKetchup]

LGTM apart from this comment.