Zverik / every_door

A dedicated app for collecting thousands of POI for OpenStreetMap
https://every-door.app
ISC License
410 stars 35 forks source link

Upgrade flutter_map to v6 #640

Closed Zverik closed 7 months ago

Zverik commented 1 year ago

Every plugin would need to be rewritten, but I guess worth it.

JaffaKetchup commented 1 year ago

@Zverik FM v6 is on its way very soon!

Zverik commented 1 year ago

Haha thanks for the heads-up Luka :)

JaffaKetchup commented 12 months ago

@Zverik v6 has been released!

Zverik commented 10 months ago

Transferred almost everything. One thing left: when switching tile layers, it doesn't download tiles until the map gets any event (e.g. dragging).

JaffaKetchup commented 10 months ago

Hi @Zverik, Try using the reset parameter, and trigger a new event to its stream whenever you change the layer.

Zverik commented 10 months ago

@JaffaKetchup looks like it — but I'm not changing TileLayer properties, but changing the layer. This function (updated to return TileLayer for v6) is called on every build (which I guess might be not optimal), so when you click a button for changing layer, the function returns another TileLayer, and it's immediately substituted.

In flutter_map v2, it triggered downloading tiles. In v6, the layer changes, which is visible when tiles are in the cache. But if it's a new area, tiles are empty because downloading is not triggered.

Is this something for reset, or a regression in flutter_map?

JaffaKetchup commented 10 months ago

@Zverik Hmm, that does sound like it could be a regression, there have been changes made since v2 that could possibly cause that. reset could still fix this case, since it forces a refresh/load of all tiles, but that would be a workaround instead the correct usage of reset. You would need to create a new one-shot Stream for every TileLayer as well.

In regards to returning a new TileLayer every build, that is less efficient than just changing properties, as it needs to delete old resources and allocate new ones. I'd try to transition to just using one if possible. Additionally, creating a new one every build circumvents some performance improvements and shortcuts that we take (because it does internally 'change' every build, but doesn't tear it all down and build it all up every time). Also, all of that URL manipulation is probably best done inside a TileProvider using the new generateReplacementMap method, and possibly populateTemplatePlaceholders (check the in-code docs for more info). You can see how generateReplacementMap is used efficiently with Bing Maps with our new tutorial - although you seem to already be handling using the older getTileUrl (and additionalOptions is also very similar, just one level up and without access to the 3 arguments of the tile generation logic): https://docs.fleaflet.dev/tile-servers/using-bing-maps.

JaffaKetchup commented 10 months ago

I was also wondering whether you'd be up for having Every Door showcased on our docs site for free: https://docs.fleaflet.dev/showcase#oss-and-or-non-profit-projects? No worries at all if not.

Zverik commented 10 months ago

For the showcase — why not, let's do that :) Just filled out the form.

JaffaKetchup commented 10 months ago

@Zverik Thanks :) It's all live now, let me know if you'd like to change anything.

Zverik commented 10 months ago

Riiight, I got it: before we had a single TileLayer with changing TileLayerOptions, now I rebuild a tile layer every time. No wonder it flickers.

But I change many options of it, not just a tile provider, but also zoom levels, attribution, tile size etc etc. I guess I'll try caching tile layer widgets first.

JaffaKetchup commented 10 months ago

You should just be able to hook whatever state into the TileLayer params, and it'll rebuild efficiently whatever is necessary (fingers crossed :D).

Zverik commented 7 months ago

So I think I've finished the migration, but tiles did not get faster from that.

What I did was, create a TileLayerOptions class that holds all the options (to simplify widget creation), and then use it for creating TileLayers everywhere. Looks more clumsy but okay.

I also did make a single reset stream that I reference from everywhere. Looks like it works.

Still, it flickers like hell while downloading tiles or switching layers. Better when tiles are cached. I don't think anything has changed from me not building TileLayer objects on each repaint, but passing arguments to TileLayer objects in build() methods. Which is expected I think: it still rebuilds the objects, since widgets are supposed to be destroyed and rebuilt dozens times per second.

You can check out how it looks with this apk.

JaffaKetchup commented 7 months ago

It looks much better to me, I can't seem to reproduce any flickering:

https://github.com/Zverik/every_door/assets/58115698/e9e3f599-dc6a-4f79-9c56-cde7edc11c5a

Which is expected I think: it still rebuilds the objects, since widgets are supposed to be destroyed and rebuilt dozens times per second.

Yes, the TileLayer still gets rebuilt, but now it maintains some state between rebuilds, because it's not a completely new instance everytime. I think that's how we've set it up anyway.

Zverik commented 7 months ago

Indeed, there is no flickering when moving the map around — just when downloadting tiles for the first time, which is especially apparent when switching layers with the bottom-right button. More prominent on full-screen map modes, like 3 and 4.

I don't like how it looks compared to v2, but it also doesn't break any important functions.

Sorry I'm this picky, I know you develop this extra useful library and only get more issues and demands :) Thank you Luka for all your work!

JaffaKetchup commented 7 months ago

Sorry I'm this picky, I know you develop this extra useful library and only get more issues and demands :) Thank you Luka for all your work!

No, no, not at all :)

I think the flickering is likely to be caused somewhere in your app, but our tile management algorithm also does need some work. Can you record a video of what you mean?

Zverik commented 7 months ago

Yes, look here:

https://github.com/Zverik/every_door/assets/766031/02c5a441-3e08-4898-be00-0711b43227c7

It feels like downloading tiles became slower. And when starting, there's this strange moment when all tiles are downloaded, but then cleared and displayed again.

JaffaKetchup commented 7 months ago

I see. I wonder if the app didn't install correctly because mine also looked a bit different when opening. I can only think that the 'flickering' could be to do with the reset stream being used incorrectly? Maybe it isn't needed, or maybe it's just being used wrong? Its always hard to benchmark tile loading speeds, sometimes it's the tile server limiting it, or we could have an issue. Have you tried the cancellable tile provider: https://docs.fleaflet.dev/layers/tile-layer/tile-providers#cancellablenetworktileprovider - it could potentially help?