Skip to content

Added CachedTileProvider #1094

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed

Conversation

AbhishekDoshi26
Copy link

Issue Solved: Using NetworkTileProvider, the experience isn't that much smoother. I have provided the definition for CachedTileProvider which uses cached_network_image to cache the tile image so that users can get a smoother experience.

@JaffaKetchup
Copy link
Member

JaffaKetchup commented Nov 30, 2021

Hi there,
There has been a long discussion about caching previously.

What it boiled down to:

  • the need for caching is common
  • an integration like this PR would work but is limited and relies on more external libraries/dependencies (which is harder to maintain effectively)
  • an integration like this may not be reliable enough (due to OS caching behaviour) - this is really important for health and safety reasons, and should not be taken lightly (consider a lost hiker in cold weather without Internet access)
  • any more complex integration will be needed to be done in a plugin

I am an owner of the current plugin for caching ('flutter_map_tile_caching'), and it solves all these issues whilst providing many more advanced features.

This PR may still be useful, but please consider using the more advanced and reliable plugin (as mentioned above).

Thanks for the contribution :)

@github-actions
Copy link

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the Stale label Dec 31, 2021
@JaffaKetchup
Copy link
Member

JaffaKetchup commented Feb 4, 2022

Other maintainers, @kengu @ibrierley etc.

Would like a second opinion on this, as I feel as I am biased in this situation, as I own a package that does this? Close or merge?
Thanks.

@ibrierley
Copy link
Contributor

I think it may need further explanation for me, as I'm a bit out of touch with the providers and caching. My experience typically has been everything currently caches ok, but naturally my experience may be different.

Couple of questions for whoever...

Is there any reason for this to be needed in flutter_map itself (as opposed to the ability to be added as a plugin and added to the readme with a note on the differences)? I'm typically for keeping new things as plugins unless there's a compelling reason for it.

What specific benefits does it give over the existing Providers, I'm not clear about if it would be smoother and if so why ?

What currently provides caching with flutter_map (without a plugin), is it purely the o.s caching images at a low level without a plugin? My crude understanding is NetworkImageWithRetry provides some caching, but am a bit unclear if that does something on top of the o.s caching iyswim.

Apologies if some of this is treading over old ground :D.

@github-actions github-actions bot removed the Stale label Feb 5, 2022
@JaffaKetchup
Copy link
Member

What specific benefits does it give over the existing Providers, I'm not clear about if it would be smoother and if so why ?

This may provide faster tile loading times, especially on slow tile servers. It would also work offline. But it won't necessarily make things 'smoother'.

I think "caching" can be meant in two ways: short-term or long-term. I think short-term is currently handled OK (see below); but there is no long-term support built into 'flutter_map', which is what this PR and the existing plugin is trying to fix.


What currently provides caching with flutter_map (without a plugin), is it purely the o.s caching images at a low level without a plugin? My crude understanding is NetworkImageWithRetry provides some caching, but am a bit unclear if that does something on top of the o.s caching iyswim.

It's a bit unclear how caching currently works. I think Flutter does perform some caching automatically when NetworkImage is used, but I think it only lasts until app restart.

  • The NonCachingNetworkTileProvider actually uses NetworkImage, so it does actually cache somewhat, making the name misleading.
  • However, the NetworkTileProvider uses a custom ImageProvider, which I don't think provides any caching at all.

Really, the names need fixing, but that's a big breaking change (unless type definitions can be used somehow to make transitions easier). Maybe a future PR.


There are two main reasons that I think this tile provider is not suitable for merging yet:

This tile provider would provide long-term caching, through 'flutter_cache_manager'. But the concerning part for me is this statement on that library's:

By default the cached files are stored in the temporary directory of the app. This means the OS can delete the files any time.

This is a safety concern for me. I think about some user relying on an app with this tile provider, being told that the tiles will be cached safely. Let's say that user is a hiker, or something like that: when they find themselves on top of a mountain in cold weather with no Internet, they'll get their phone out for the map. And then they'll be stuck because the OS has decided to free up some space in the background.

This PR also doesn't provide an easy way to manage those tiles: the only way to remove the cached tiles would be to clear the whole app cache.


Therefore, I would be in favour of closing this PR. More functionality is needed, which would probably make this better for a plugin instead of a PR, which already exists.

@AbhishekDoshi26
Copy link
Author

AbhishekDoshi26 commented Feb 5, 2022

I agree. This PR's main cause is to provide caching. It actually does make it smoother because the tiles don't need to load every time. So let's suppose if the user has already visited a tile, it will be cached and hence when the user again goes to that particular tile, the user will get a smoother experience.

The caching works in the same way as flutter_cache_manager or even native cache manager. The tile image is converted to small chunks which are then saved to OS's cache manager. OS only clears this cache (especially iOS) when the app is performing or occupying a large number of resources.

Now, why would we like to clear the cache? One scenario can be that the tile design has changed and you want the user to get the latest tiles. In this case, your URL of tiles will also change and hence the provider will load from the new URL and ultimately caching that and overwriting the previous one.

However, I am totally ok to close the PR if everyone thinks it won't be much useful.

@ibrierley
Copy link
Contributor

Well, personally I think I'd prefer it as a plugin, and then extend flutter_maps readme to be a bit clearer about some of the caching options and why one would choose one over the other (noting the differences between normal caching and offline caching etc).

@JaffaKetchup
Copy link
Member

JaffaKetchup commented Feb 6, 2022

@kengu Any ideas?

user will get a smoother experience.

Less grey tiles: yes. Better frame rate: not necessarily, but potentially (AFAIK from my experience with my plugin, which may be wrong).

The caching works in the same way as flutter_cache_manager or even native cache manager. The tile image is converted to small chunks which are then saved to OS's cache manager. OS only clears this cache (especially iOS) when the app is performing or occupying a large number of resources.

This is still too much of a risk for me, but maybe OK for others. I know iOS is quite heavy with things like this, along with some Android vendors like Samsung.

Now, why would we like to clear the cache?

You mentioned if the user has changed the tile layer, which I totally accept. But after a while, the storage will build up, and without an easy way for the user to clear the cache (without going through Settings), the user may become annoyed at the app.

@JaffaKetchup
Copy link
Member

Ok, for now, I'm going to close this PR. Potentially in the future it can be reconsidered, but for now, this is non-essential (there are many essential PRs to merge right now) and kind-of already exists.
Still, thank you for your contribution, and feel free to submit more in the future should you feel the need to. Thanks!

@JosefWN
Copy link
Contributor

JosefWN commented Jul 12, 2022

I know I'm a bit late to this discussion, but perhaps one does not exclude the other? I've had one app where I've really needed the functionality provided by flutter_map_tile_caching, and I've also had other apps where I've preferred to keep things lightweight. In my current app I'm actually using the tile provider submitted in this PR, simply because it's not critical if the cache is cleared.

Including it would be a convenience, and a doc comment could warn that the cache could be cleared when the OS sees fit? Then again it's not many lines of code to copy into your project either...

@ibrierley
Copy link
Contributor

I'm probably a little out of touch on caching....

  1. One thing I've not quite understood with tile caching...does Flutter by default cache images ? (I find this difficult to check, always seems to be a bit of unnecessary confusion on this one...)

  2. The image cache solutions I think typically, save to the cache folder (which the system can release any time..fine for many cases)...why don't we do one which gives the option to save to the AppsDocumentsDirectory, which aren't deleted by the O.S (maybe with a max size/retention period setting)?

@JosefWN
Copy link
Contributor

JosefWN commented Jul 12, 2022

I'm pretty out of the loop too to be honest.

I'm assuming that Flutter does not cache images on disk at least, they seem to recommend using cached_network_image for that: https://docs.flutter.dev/cookbook/images/cached-images

The ability to store the images in the apps directory seems to be discussed for the flutter_cache_manager which cached_network_image depends on (although the post is a bit old): Baseflow/flutter_cache_manager#238 (comment)

@AbhishekDoshi26
Copy link
Author

I still feel having a light weight cache tile provider would be a better option

@JaffaKetchup
Copy link
Member

Flutter usually caches images until an app restart in its image providers. The custom image providers used in this repo do the same, but less reliably.

The cached_network_image package caches on disk, until the app cache is cleared by the user or OS.

In terms of a simple tile provider using the above package, I'd be opposed to it:

  • You need the framework to manage that store (eg. clear)
  • You need the framework to link tiles with file names - which is harder than it sounds due to all the different platform implementations
  • You have to make it 100% clear to developers and users that it is not be reliable - could be a significant risk of someone is relying on a tile set in a dangerous situation, that's been cleared by accident
  • You have to manage people's expectations - eg. Can I see how much space the cache is taking up? No.
  • It's easy to create a custom tile provider that does this yourself
  • I have a package that does this 🤣 In all seriousness, there would still be a place for my package, so this isn't a worry.

Those are the main reasons it was denied originally.

@JaffaKetchup
Copy link
Member

JaffaKetchup commented Jul 12, 2022

On the other hand, I would be up for making the documentation clear on how to make your own tile provider for caching using this package, including clear notices for the things I commented before, and alternatives.

@JosefWN
Copy link
Contributor

JosefWN commented Jul 12, 2022

I don't feel strongly about it, like I said earlier, I think the two complement one another. If offline functionality is important then your solution is a no-brainer, but for someone like me who has continuous connectivity it carries features I don't need (and with them possibly increased overhead, and a greater risk of bugs). I understand that cached_network_image might not be perfect either, but it's relatively simple, has 3.5k likes on pub.dev and 2.1k stars on GitHub (hopefully some developer momentum), which is what appeals to me.

@ibrierley
Copy link
Contributor

Maybe this should be part of a separate discussion...

I think what's unclear currently is what any solution has over another...(ignoring file providers for the moment). It feels like there are 3+ core components of which may be desirable (any more?).

Temporary cache, not guaranteed (I feel like FM should include this, even if its via OS).
Permanent "cache" (i.e where the OS can't auto clean up, so maybe that's not even cache at that point :D) (fine not to include this, but there should be info on it).
Retries (feels like this is desirable as a core FM component, if not there should be info on it).
(I care less about placeholders, but others probably do, so could be worth a mention)

I.e default flutter_map I think uses network_image_provider...does this provider caching (even if via system) and does it retry
Same for plugins like flutter_map_tile_caching, cached_tile_provider and cached_network_image. Any others of note ?

(possibly also how to write your own as info, as typically in the past I have just done this anyway when I couldn't use FMs, but I'm out of date on that now).

@JaffaKetchup
Copy link
Member

Maybe this should be part of a separate discussion...

Probably a good idea. Open it here on GitHub, or use Discord?

Temporary cache, not guaranteed (I feel like FM should include this, even if its via OS).

If, by temporary, you mean: "until the app is closed", then we already provide this.
If you mean: "until the OS decides to clear the cache", we do not provide this. This is mostly what this PR seems to be about, but note that it can be customized to be a more "permanent" cache.

Permanent "cache" (i.e where the OS can't auto clean up, so maybe that's not even cache at that point :D) (fine not to include this, but there should be info on it)

I would agree with this.

Retries (feels like this is desirable as a core FM component, if not there should be info on it).

We kind of support this, through RetryClient where specified. See https://github.com/fleaflet/flutter_map/blob/f55e15602b59890c623604944fe727a4bc42894f/lib/src/layer/tile_layer/tile_provider/network_image_provider.dart for the current implementation.

I.e default flutter_map I think uses network_image_provider...does this provider caching (even if via system) and does it retry

Since v2, we use NetworkNoRetryTileProvider by default, which uses https://github.com/fleaflet/flutter_map/blob/f55e15602b59890c623604944fe727a4bc42894f/lib/src/layer/tile_layer/tile_provider/network_no_retry_image_provider.dart. NetworkTileProvider does support RetryClient.
In terms of caching, this only performs caching "until the app is closed".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants