(⏸︎) Replace TileLayer.errorImage with TileLayer.tilePlaceholder

[rorystephenson]

When a tile fails to load and there are no existing loaded tiles which overlay with it from another zoom level we are left with a blank portion on the map. When there are no loaded tiles visible, for example because of a network problem, there is no visual feedback when zooming/panning/rotating because the map is just a single solid color. This prompted me to add a placeholder image.

We already have an error image implementation but the error image will not show up until loading fails which, in the case of a poor network connection, can take quite a while.

The placeholder implementation gives visual feedback both whilst loading and when loading fails.

Note that the current implementation just fills the viewport with the placeholder tile behind all other tiles. It would be more efficient to avoid adding placeholder tiles which are completely obscured by other visual tiles but my attempts to do this were unsuccessful due to the complex nature of tile pruning.

[rorystephenson]

@JaffaKetchup / @TesteurManiak / @ibrierley etc this idea came to me so I whipped together a quick implementation.

WDYT?

A preview:

[ibrierley]

Just relating to your last comment, I can't recall exactly the logic (and we may already do something similar, or it may have been removed...) but would it make sense for each new tile to have your placeholder which is switched to the live tile when loaded, but until the live tile is loaded its sorted first in the list of tiles (that way any unloaded tile would automatically be at the back of the display and any previous zoom tiles overlayed in front)

Not sure if that's as clear as mud!

[rorystephenson]

Just relating to your last comment, I can't recall exactly the logic (and we may already do something similar, or it may have been removed...) but would it make sense for each new tile to have your placeholder which is switched to the live tile when loaded, but until the live tile is loaded its sorted first in the list of tiles (that way any unloaded tile would automatically be at the back of the display and any previous zoom tiles overlayed in front)

Not sure if that's as clear as mud!

I didn't try that and it may be possible but I suspect when zooming out that will cause the grid to be overlayed over higher-zoom tiles until the new tiles load. I'll have to give it a try to be sure. I've always found the pruning logic tough to reason about, that's probably an area that could really benefit from some dedicated tests to make sure we retain tiles appropriately when zooming.

[JaffaKetchup]

Unless I've missed something, it looks like this was already possible through tileBuilder? Have a look at the tile builder example page, it shows how to check when the tile is loading.

It also looks like the ability to display an error image has been removed? If so, I think the separate error image could be useful.

Maybe what this points to is a requirement for the reworking and simplification of the tileBuilder/placeholder/error system into one unified tileBuilder.
At the moment, we pass through the entire TileImage - although some information may be useful, is it really necessary to pass it all through? It's quite a core part of FM, and usually stuff like that shouldn't be directly exposed, especially as it could have functions called on it or states changed externally.
Perhaps we should pass through these things:

• context
• Widget 'tileImage' as currently is, or null if not yet loaded
• enum 'status'
  • Not yet requested
  • Requested, awaiting response
  • Failed
  • Available/loaded
• Duration 'loadingDuration', or null if not yet loaded
• Any other useful statistics

We then provide a default implementation which just redirects the first 3 statuses to the grid you created, and otherwise just renders the image.

[rorystephenson]

I think you're right that this can probably be replicated with tileBuilder although I think it would require adding a Stack for each tile because a tile can be loaded but not yet fully visible (whilst it is fading in). I think that's not ideal for performance reasons and in the end it would be complicated for a user to define a tile builder which works correctly for this and I think we can do it more efficiently in the library.

I've never actually used tileBuilder in any of my projects, I'm curious if anyone else here has? I always shied away from it for performance concerns, I'd rather avoid adding a bunch of extra widgets especially given that the tile layer can be moved fast resulting in a lot of widget creation/destroying.

In any case I definitely agree that this area can be improved and I'm not at all tied to the solution that this PR presents. I'd love to have a better idea of if/how people are using tileBuilder and errorImage. In particular I wonder if anyone is using errorImage because it can take a while to appear (if a network request is hanging). My reasoning for replacing it with placeholderImage is that it still gives you the feedback that a tile has not loaded whilst also giving you visual zoom/pan/rotate feedback whilst it is loading.

P.S.

Tile pruning is still one part of the tile layer which I find quite hard to reason about. The pruning logic depends on various TileImage variables (current/retain/active) and there are complicated requirements around keeping tiles when zooming out until they are covered up by a fully loaded tile in a lower zoom etc. It's a tough problem in general, when I looked at Google Maps for some inspiration I found bugs in their placeholder image which caused it to flicker or remain visible on top of tiles.

[mohammedX6]

I was facing the same problem, my workaround is to put lat_lon_grid_plugin behind the tile layer, it's work perfectly

[rorystephenson]

@mohammedX6 that's definitely a good option!

In this case I would like to make it as efficient as possible by only adding placeholders behind tiles which are not completely visible. I will revisit this PR after #1596.

[mohammedX6]

That's nice idea

[rorystephenson]

I've updated this to incorporate changes from #1596 and added a simple algorithm which reduces the number of placeholders created.

[JaffaKetchup]

Should probably rename this to be clearer what it does (I know it's internal only, but still should have a more self-explanatory name). Perhaps use @visibleForTesting?

[rorystephenson]

This is not related to testing, it's an optional filter. I followed the naming convention that I've often seen in dart e.g: https://api.flutter.dev/flutter/dart-core/Iterable/where.html

[JaffaKetchup]

Ah, that makes more sense. I'm aware that test is usually the terminology used, but usually the method name itself adds to the context. Maybe it should be filter or filterTest?

[JaffaKetchup]

Personally, I still think errorImage should be kept. I haven't used it myself, but I can think that some people would want a different error image (let's just say https://http.cat for example), instead of a placeholder/loading image (the square grid, for example).

Additionally, would it perhaps make sense to allow placeholderImage (and errorImage) to take any Widgets?

Does this PR make the grid placeholder the default, or is there no placeholder by default? I think the grid being the default makes sense.

---

PS (unrelated). I'm not sure whether #1563 & #1602 might be related to a bug in the pruning logic? Or whether the bug is somewhere else.

[rorystephenson]

Personally, I still think errorImage should be kept. I haven't used it myself, but I can think that some people would want a different error image (let's just say https://http.cat for example), instead of a placeholder/loading image (the square grid, for example).

My reasons for removing errorImage are as follows:

1. It leads to clunky UI. When zooming out, if a tile load fails, you may have already loaded tiles from a higher zoom level that cover the same area which the errorImage will obscure.
2. The placeholderImage handles the most important behaviours (in my opinion) of the errorImage, it gives a visual feedback when tiles are missing for a part of the map (from all zooms) as well as giving visual feedback when moving/zooming the map.
3. It's not clear if anyone actually uses it. Like many TileLayer features it was added in one huge PR. That PR was a big improvement for TileLayer but it also added many features which were not requested by users and thus may not even be used.
4. It complicates pruning logic. If tile loading errors and there is an errorImage then, for pruning purposes, we treat the tile the same as a successfully loaded tile (we prune tiles that it obscures). If there is no errorImage then the tile is treated as if it has not loaded.

For these reasons my inclination would be to remove errorImage, most of all because I suspect that due to points 1 and 3 it is not used and, if it is used, the useful behaviours are more elegantly handled by placeholderImage.

It's totally possible that the clunky UI doesn't bother some users and that they will be disappointed if this features is removed, it's really hard to know 🤷 . Given the points I've raised above what is your feeling?

Additionally, would it perhaps make sense to allow placeholderImage (and errorImage) to take any Widgets?

I've made this change and simplified the creation of a placeholder grid tile by moving to CustomPaint since we don't need to generate an ImageProvider anymore. The only downside is that it prevents us from implementing a more efficient way of rendering the placeholders (e.g. rendering them all using a single painter instead of multiple positioned widgets containing painters). That's probably OK given that there are a small amount of them and the performance difference is likely to be insignificant. I've just added a commit which makes this change.

Does this PR make the grid placeholder the default, or is there no placeholder by default? I think the grid being the default makes sense.

To avoid a breaking change it was disabled by default but I agree that it is a sensible default and I've now made it enabled by default, it can be set to null to disable it.

PS (unrelated). I'm not sure whether #1563 & #1602 might be related to a bug in the pruning logic? Or whether the bug is somewhere else.

I haven't tested locally but my first guess after a brief glance would be that a map event is not being triggered by the movement and therefore loading/pruning does not get triggered.

[JaffaKetchup]

It complicates pruning logic. If tile loading errors and there is an errorImage then, for pruning purposes, we treat the tile the same as a successfully loaded tile (we prune tiles that it obscures). If there is no errorImage then the tile is treated as if it has not loaded.

Would there be a disadvantage to treating it like a non-loaded image?

Given the points I've raised above what is your feeling?

I'm still in the same mind, but maybe @ibrierley, @TesteurManiak, and @mootw would like to add their opinions :D.

---

I haven't tested locally but my first guess after a brief glance would be that a map event is not being triggered by the movement and therefore loading/pruning does not get triggered.

I would agree with this, but I can't understand why the normal way of calling camera.move (which I think they both do, and I know #1602 definitely does) wouldn't cause loading/pruning.

[TesteurManiak]

Tried to catch up on the discussion, I'd tend to agree with @rorystephenson on removing the errorImage (or at least deprecate it). I cannot confirm that it is commonly used (Can at least say that I've never used it), and while we might get a more complex API to obtain a similar visual result I think the potential gain in performance is worth it.

I'm always in favor of deprecation, if possible, before removing a property so you can inform the user of its issues and share some code sample to facilitate the transition. In this case, having a "before/after" in the deprecation message and a code sample directly in the code comments would help existing users to adopt the new way of displaying placeholders.

[rorystephenson]

Just an update. I've recently started a new job and I don't have a lot of extra time to dedicate to this right now.

[JaffaKetchup]

@rorystephenson No problem, hope you're enjoying your new job.

[JaffaKetchup]

This is now being moved into Draft status, to unblock the v6 release.