TileLayer Reimplementation (& v4 Release Preparation)

[rorystephenson]

This PR started with wanting to fix the broken updateInterval option before I got sucked in to the TileLayer rabbit hole. I hope I've significantly cleaned up the code without breaking too much. The old code was recalculating map boundaries and loading tiles on every build and in general I found it very hard to follow. Tile loading/pruning is done out of the build method now and it should be much more efficient. I've done my best to comment the code but please mention anywhere where that's missing! I am sure there are some mistakes since the changes are extensive. I'm also very happy for any suggestions for improvement.

Breaking changes

• Opacity is no longer an option, you can just wrap the layer with it if needed.
• Tile renamed to TileImage
• Coords renamed to TileCoordinate and the individual coordinates are not ints.
• minNativeZoom/maxNativeZoom now int? Instead of double? although they already were interpreted as whole numbers
• updateInterval removed since it was doing nothing. The same behaviour is now possible using the tileUpdateTransformer option.
• tileFadeInDuration/tileFadeInStart/tileFadeInStartWhenOverride now moved to a single TileFadeIn option.
• ErrorTileCallback signature now has Object instead of dynamic for the error, it also now contains a nullable StackTrace

Features

• TileUpdateTransformer allows filtering/modifying tile updates triggered by movement. See AnimatedMapController example where this is used to preload tiles at the target location and disable loading/pruning whilst moving which causes a LOT of tile downloads for big distances.
• First tile layer tests added (not covering everything but its a start).
• Updates to individual tiles are now done only to that tile rather than calling setState(){}. This is possible because TileImage (previously Tile) is now a ChangeNotifier and Tile (previously AnimatedTile) listens for changes and updates as appropriate. In fact there are no calls to setState((){}) in TileLayer now.

[JaffaKetchup]

Hey @rorystephenson,
A special thanks for taking this on :)
We'll get round to looking at it soon I'm sure!

[rorystephenson]

Hey @rorystephenson, A special thanks for taking this on :) We'll get round to looking at it soon I'm sure!

It has been a BIG mission! Its been close to a year since I last dived in and whilst last time I fixed lag issues and tidied things up a fair bit I'm quite stoked with the improvements In this PR. I am in the process of writing up the changes so it will be easier to get a hang of the diff once I've done that.

[rorystephenson]

Tile was renamed to TileImage and Coord to TileCoordinate.

The rest are just re-ordered because my IDE does it automatically and the order was broken. Given that only a small amount of exports were out of order I think it makes sense to re-order them.

[JaffaKetchup]

It has been a BIG mission! Its been close to a year since I last dived in and whilst last time I fixed lag issues and tidied things up a fair bit I'm quite stoked with the improvements In this PR. I am in the process of writing up the changes so it will be easier to get a hang of the diff once I've done that.

I can imagine, we've been putting it off for ages just because none of us have the time to do things like this unfortunately.
Looking forward to reviewing and getting this released! I'll add a special thanks to you on the Credits page of the docs for the next version.

[ibrierley]

Yes, thanks for looking at this. Apologies, I'm away for over 2 weeks, so won't get chance short term to do a review (happy if anyone else wants to confirm all ok tho). Just wondering about the removal of Level stuff. It's a while since I looked at that, but I was thinking some of it was being uses as a form of caching to avoid recalculations, but I may be wrong. Is that the case, just wanted to make sure there wouldn't be any performance issues from the changes (altho tbh I doubt it would be that much anyway to calculate each zoom level update). I may be on the wrong track though, just can't really check from here, so apologies if so!

[TesteurManiak]

Wow this is some huge work you've done here thank you! I'll try to review the code as much as I can in the following days and test it on the different examples. Maybe after this PR is merged we should publish a "dev" version of the package on pub.dev to gather some more feedbacks 🤔

[JaffaKetchup]

Yep, a dev version might be a good idea, as there will also be other merged changes.
We can notify Discord members to update if they want to help test.

[rorystephenson]

@TesteurManiak I've just encountered a race condition scenario between the TileLayer build and the map events triggering tile updating. It's possible for the TileLayer to build before a MapEvent causes new tiles to be added to the tile manager. In this scenario tiles may be missing until another build occurs. Surprisingly I've only seen this in a very particular scenario but I need to think about how it should be handled properly and whether this means I need to change how tile management works. So for no I'd suggest holding off on reviewing.

[mootw]

No worries, let us know when you're ready again or if you need any assistance. I am happy to help troubleshoot if needed!

[rorystephenson]

Thanks @TesteurManiak & @mootw .

In this PR I have moved tile loading/pruning out of the TileLayer build(), it is now triggered by listening to the stream of MapEvents. There are two reasons for doing this:

1. Make the build faster.
2. Allow tile loading/pruning to be debounced/filtered/modified.

When I started testing the app in profile mode a race condition occurred due to a quirk in FlutterMapState initialization where the FlutterMap's size is first null, then CustomPoint(0,0) and finally the actual size (a quirk caused by how LayoutBuilder works). This was causing only one tile to be loaded on the first load of the map. As I mentioned this is a race condition and it doesn't always happen, sometimes LayoutBuilder sets the actual size on the first go. I have just pushed a commit with some debugPrints to clarify what is happening when the race condition does/doesn't occur.

Race condition NOT occuring

I/flutter (20296): FlutterMapState LayoutBuilder set size: (395.42857142857144, 739.4285714285714)
I/flutter (20296): Size is non-zero, triggering tile update/prune.
I/flutter (20296): Updating tiles, map size: CustomPoint (395.42857142857144, 739.4285714285714)
I/flutter (20296): TileLayer build (map size CustomPoint (395.42857142857144, 739.4285714285714))
I/flutter (20296): FlutterMapState post frame callback (calling onMapReady)

Race condition occurring

I/flutter (20426): FlutterMapState LayoutBuilder set size: (0.0, 0.0)
I/flutter (20426): Size is zero, scheduling tile update/prune in next frame. (TileLayer didChangeDependencies)
I/flutter (20426): TileLayer build (map size CustomPoint (0.0, 0.0))
I/flutter (20426): FlutterMapState post frame callback (calling onMapReady)
I/flutter (20426): TileLayer Post-frame callback update, size is: CustomPoint (0.0, 0.0)
I/flutter (20426): Updating tiles, map size: CustomPoint (0.0, 0.0)
I/flutter (20426): FlutterMapState LayoutBuilder set size: (395.42857142857144, 739.4285714285714)
I/flutter (20426): TileLayer build (map size CustomPoint (395.42857142857144, 739.4285714285714))

So this actually highlights an issue in FlutterMapState where onMapReady is called before the map has set its size. This is worth looking in to separately and I can add a workaround for TileLayer but it got me thinking: can this happen when the map moves?

When the map is moved it triggers both a rebuild of the TileLayer and a MapEvent which triggers tile update/prune. If the build happens before the update/prune then the TileLayer build may be missing tiles and they won't be shown until another build is triggered. We could add some setState() calls but that feels like a bandage and will cause extra builds which I want to avoid.

If it is possible for map movement to trigger the build before the emitted MapEvent triggers tile update/prune it wouldn't be very noticeable with normal movement since the build/tile update gets triggered many times a second but when the map is moved with MapController the tiles may not load.

I have just run some quick tests locally by modifying the MapController example page to zoom in a bit further so that the three locations do not share any tiles and added some debugPrints to see when the TileLayer build/tile update get fired with respect to each other when MapController triggers movement. I tapped t between London/Paris/Dublin around 50 times and every time the tiles loaded and the tile update/prune was triggered before the build but I don't know enough about flutter/dart to say for sure that this will always occur or if it's possible that the build happens first and the tiles do not appear.

If we can be sure that the update/prune will always happen before the build then I will just add a small workaround for the issue mentioned above and all is well. If not then I need to reconsider the design (I have an idea but it will require digging in to pruning logic to be sure it works).

EDIT

Actually if we want to be able to support debouncing of map tile updates (which I think is definitely a good idea) then we're guaranteed to have the build run before the tile update/prune in some cases and a change in the design is necessary to avoid missing tiles.

Generally speaking the issue is that if a load happens after a build for a given map position the newly loaded tiles should appear on the map (ideally without triggering a rebuild of the whole TileLayer).

[JaffaKetchup]

(Little bit lost, I must admit, but the idea of being able to control tile loading (debounce etc) seems like a nice feature, although I can't really think of many use cases.)

EDIT: Re-read a little later, and makes more sense now.

[rorystephenson]

(Little bit lost, I must admit, but the idea of being able to control tile loading (debounce etc) seems like a nice feature, although I can't really think of many use cases.)

EDIT: Re-read a little later, and makes more sense now.

So this should replace the broken updateInterval feature as you can debounce updates and it also allows for smarter tile loading/pruning based on the event that triggered movement. See the AnimatedMapController example for a demonstration of that.

---

@TesteurManiak / @JaffaKetchup / @mootw I have resolved the aforementioned issue by pre-creating (but not loading) missing TileImages in the build method. I am sure there are things to fix and improve but for now I think an initial review is possible and I've updated the PR description to outline the changes. Please let me know if you run in to issues or have suggestions for improvements!

[JaffaKetchup]

@rorystephenson Great :) I'll try to look over this this weekend.

[JaffaKetchup]

I've spotted one bug that wasn't there previously in the example app. Toggling the dark mode in the tile builder page does nothing now.

Additionally, the non-exposing of TileCoordinate/Coords is also a bug - plugins rely on it quite heavily in some cases.
I would also prefer to keep Coords named Coords instead of TileCoordinate, just for compatibility reasons and keeping things short. Open to other ideas on that!

---

I've also spotted an issue in the current 'master' and this branch, to do with the InteractiveFlags.
If dragging and flinging is enabled, then flinging is disabled mid-fling, the map gets a setState() called during build error.
If flinging is then re-enabled, the map splits into two, separately controllable halves.
Moving to another page crashes the entire app frame.

[rorystephenson]

I've spotted one bug that wasn't there previously in the example app. Toggling the dark mode in the tile builder page does nothing now.

Additionally, the non-exposing of TileCoordinate/Coords is also a bug - plugins rely on it quite heavily in some cases. I would also prefer to keep Coords named Coords instead of TileCoordinate, just for compatibility reasons and keeping things short. Open to other ideas on that!

I've also spotted an issue in the current 'master' and this branch, to do with the InteractiveFlags. If dragging and flinging is enabled, then flinging is disabled mid-fling, the map gets a setState() called during build error. If flinging is then re-enabled, the map splits into two, separately controllable halves. Moving to another page crashes the entire app frame.

Nice spotting I'll have a look at the dark mode bug and try add a test for it as well.

---

Maybe it's worth tackling the other issue in a separate pr if it's present in master as well in case it takes a while to merge this pr.

[rorystephenson]

The dark mode issue was caused by me removing the tileContainerBuilder functionality (I'd forgotten to remove the option but it was doing nothing). The same behaviour can be achieved by wrapping the TileLayer with the desired widget and I have changed the example to do so. If there is a use case where the wrapping widget needs access to the list of tiles, which TileContainerBuilder provided, then removing it is a mistake and I will reinstate it. Are you aware of any?

I did the same thing when I removed the opacity option. I was trying to simplify the TileLayer without removing functionality as there are a lot of options however if you prefer that we keep some of these options or deprecate them before removing them I'm fine with that.

---

As for TileCoordinate I have re-exported it 👍. I changed it from simply Coords because at first it wasn't clear to me that it was only used by the TileLayer and I was hesitant to modify it. It is slightly specific to TileLayer in that the distanceTo implementation ignores the z coordinate. That said I'm fine with changing it back if you think it's not worthwhile.

The conflict should be resolved now.

[JaffaKetchup]

Just done a little bit of testing on the example app (Windows). We're doing excellently on UI/Build Frame time, but Raster Time is often quite high:

I guess it's high because we've got loads of images, but I can't understand this:

The entire screen, except the map widget, seems to repaint every time the map repaints - but takes longer despite having barely anything within it! RepaintBoundarys make no difference.

[rorystephenson]

(Also still need to resolve #1475 (comment)) I'll do some profiling tonight just to see if there's any obvious & quick improvements that can be made.

True I forgot about that 👍 .

@rorystephenson are you on a high performance network? When I tested on web I barely noticed a difference if any at all. However, I was able to notice (not profile as of yet), a noticeable amount of jank when zooming on my phone using a test app which loads a significant number of layers and tiles (mid hundreds at once while zooming). compared to wherever I was testing from last. Unfortunately I did not write down the HEAD of that. This was also how I am able to consistently get the crashing issue. I discussed earlier. I apologize that I have not had the time to look at either of these yet.

I'm on decent but not extremely fast wifi. I don't see much of a difference on my computer when testing the web version but there is quite a noticeable difference when I test the web version with my phone (my phone is a few years old, maybe with a newer phone it is less noticeable).

I forgot about the concurrency issue as well, that's definitely worth resolving if we can reproduce it. Does your test app change the tile image url? I ask because reloadImages is only called when the tile url changes.

[rorystephenson]

Here's an update on the concurrent modification error that @mootw spotted:

I just had a quick dive in to see if I could understand how the concurrent modification can occur and I think I've found it. The reloadImages method calls load() on the TileImage which in turn listens for the image loading. This looks like the callback will be called asynchronously but as the documentation mentions it can actually be called synchronously and when this happens onLoadComplete is called which can trigger a prune that causes tiles to be removed whilst we are still iterating them in reloadImages.

A similar issue could also theoretically occur if a synchronous error is thrown when when loading the tile image since it also calls onLoadComplete.

I can see a few different ways to solve this:

1. Change how reloadImages works. First make a copy of the list of tiles to reload (List.from(_tiles.values)), and then iterate it calling load().
2. Add a newTile parameter to the onLoadComplete() (_activate already has logic to determine if a TileImage is new. Then if onLoadComplete() is called with newTile: false we can avoid pruning since we wouldn't expect pruning to be necessary when reloading an existing tile, only when adding a new TileImage as that implies that the visual bounds have changed. On the other hand this may cause problems if all tiles are reloaded whilst the map is moving since we don't trigger the prune in the onLoadComplete callback when there are remaining tiles to load and the last tile may be a reloaded tile.
3. Delay the calls to onLoadComplete (Future.delayed(Duration.zero, () => onLoadComplete(coordinates))). This will work but it also feels very hacky and we'd have to check that it doesn't introduce any slow-down.

Any better ideas?

I am fairly certain that this issue can only happen when calling reloadTiles since it calls load() whilst iterating the TileImage collection. The only other place load() is called is when adding new tiles but the collection being iterated is a List which is a filtered/copied list of TileImages.

[JaffaKetchup]

Nice work! (It's 11pm here for me now, so I'll be back tomorrow and can check it out then.)

[rorystephenson]

I've gone ahead and added a commit with my preferred fix for the concurrent modification issue (it now iterates a copy of the tile collection instead of iterating the tile collection directly).

I've also revisited the v1.1.1 example on mobile web and the performance differences are not consistent for me. If someone is experiencing noticeable and consistent performance differences between v1.1.1 and this version I'm interested in having a look. If so please specify the following:

1. Device used.
2. Target platform (if web please mention whether you overriding the web renderer with canvaskit or html).

---

So I think we're mostly just waiting on the addition of a MapEvent for map size change.

[JaffaKetchup]

Thanks @rorystephenson, sorry it dropped off my radar. Your implementation looks good.

Are you going to be able to add that last point? Happy to try to add it if you can't - although this area (sizing) appears to be particularly vulnerable to bugs.

Still trying to figure out the long double raster times - if you have any insight into that, that would be helpful. But I don't think that's much different to any previous version, it's just something that's never been paid much attention.

Significant changes since last review

[rorystephenson]

Are you going to be able to add that last point? Happy to try to add it if you can't - although this area (sizing) appears to be particularly vulnerable to bugs.

I'm happy to although my time available is quite unpredictable at the moment so let's see if I manager to get this done. I just started by looking at why sometimes on startup the size is initially zero and it seems to be due to how flutter works, more info on this issue: flutter/flutter#125796

Still trying to figure out the long double raster times - if you have any insight into that, that would be helpful. But I don't think that's much different to any previous version, it's just something that's never been paid much attention.

Will try to have a look at this.

[rorystephenson]

@JaffaKetchup I've added a couple of commits with the implementation of the map size change event. I made some other changes while I was there as I noticed some things which could be simplified/improved. See the commit messages for more info.

[JaffaKetchup]

Thanks @rorystephenson, I'll try to review in the coming day(s). I'll also update the docs with those last few breaking changes.

I've added a special credit for you to the v4 docs.

[mootw]

Tested on complex app on android and web. The crash has been fixed, and performance seems acceptable (though I do note some potential optimizations and issues we have discussed above). I did not profile it against previous versions yet though. Interestingly loading tiles on the web seems like the image decode is faster (for cached tiles) though i did not measure it. I think the opacity comment for TileDisplay.instantaneous should be more prominent in some way (maybe as part of the migration docs?) to encourage using an opacity widget over the tile layer to prevent overlapping tiles with weird behavior when the opacity is not 1.

In general though, I like most of these changes and they should be incredibly helpful for the future of flutter_map. Thank you for your contributions! 🎉

(Though before we merge we should get another review or two because I probably missed something and there are a lot of changes)

[JaffaKetchup]

LGTM! Would like an additional review, but not strictly necessary, cc. @ibrierley @TesteurManiak.

[TesteurManiak]

The code looks good to me 👍 (Not a big fan of the use of the ! operator, I prefer to use a transitive variable to benefit from the type shadowing but this can be part of a future refactor)

[ibrierley]

I'm fine with all of this! I can't see the performance issues now that I was seeing before, so happy for this to go ahead. Thanks for all of the work.

[JaffaKetchup]

🎉 Thanks for the contribution.
Please join the Discord server to keep informed about updates.

[josxha]

Hi @rorystephenson, I currently working on some code improvements in #1594 and wanted to ask if there is a reason to have the logic splitted into four loops here?

[rorystephenson]

Hi @josxha, it's great that you're digging in looking for a way to improve the code here.

It looks like this code originates from this commit.

I don't think we can reduce the for loops in the current form (more on that at the end). Tiles have an existing retain value (from a previous prune run) which might be set to true or false. Imagine if a tile, tile X, previously had a retain value of true but it is no longer current. In the current implementation we first set tile X's retain to the value of current, false, in the first for loop. Then we iterate through the tiles and, if we need to retain tile X because it is the ancestor/child of a current tile that is not loaded yet we will set its retain to true.

If we combine those two for loops we might first encounter a non-loaded child of tile X when iterating and set its retain value to true before iterating over tile X and setting its retain back to false because it is not current.

We cannot combine the 2nd/3rd for loops because we need to iterate all tiles before we know that we do not need to retain a child (as mentioned before, it may be the child/parent of a non-loaded current tile).

Finally we can't combine the 3rd and 4th for loops because this would cause a concurrent modification exception. Calling _removeWithDefaultEviction removes an element from _tiles so we must first build a new list of removals from _tiles before iterating over the new list and performing the removals.

---

That said the fact that we have four loops to perform removals is a code smell and you're right to point it out. I have been experimenting with refactoring tile pruning to make the logic easier to follow and you have made me realise that I can probably remove an iteration in my new implementation. I need to finish my work on that before opening a PR though.

[ibrierley]

Just r.e the removals. I never really liked the whole prune methodology, it felt kind of wrong, and I did have a dabble at a change, but it wasn't quite as smooth.

The basic premise was to not think about pruning tiles, but starting from scratch, "what tiles do we need to display immediately", if a tile is outstanding, see if there's a parent (or child) tile zoom that covers it (similar in concept to the retainParent/Child iirc), and use that.

It's a subtle difference mentally, if you wanted a peak
(basically line 244-384). As mentioned, it didn't work as well so I wouldn't just copy it, but it may just give a slightly different angle on it..I wonder if my problem was that it wouldn't necessarily try and use a tile from the last frame, it would pick any as a backup). Don't want to distract the work you're doing tho, so don't get bogged down on that this. Maybe it should be a separate change.

[josxha]

Thank you very much for this great explaination @rorystephenson!