-
-
Notifications
You must be signed in to change notification settings - Fork 5.8k
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
Tile pruning too agressive? #4039
Comments
Yeah, at some point I removed it and then didn't readd it back. Need to look whether it's worth doing at all. |
@mourner, not sure what "it" refers to in your sentence ;) |
I'm unable to see how the bisected commit affects tile pruning :-S Maybe it's time to see if #3551 is not a bad idea after all? |
@yohanboniface I referred to retaining tiles on the current zoom when panning. I removed it when reworking the logic meaning to add it back some time later, but never did. I'm not sure we should though — or maybe we should but in a limited form (e.g. retain tiles only within X pixels from the current screen). Retaining already loaded tiles is different from #3551 though (which is loading yet unseen tiles). |
Can be checked visually on #4206. I recommend keeping at least one tile in memory on each sides. What is the downside of keeping more tiles? Higher memory usage? |
@hyperknot Bloated DOM. |
I think this can be fixed after rc1. |
One scenario where this will matter is smooth zooming with zoomSnap = 0, zoomAnimation = false and trackpad scrolling. On current master, this triggers thousands of tile loads even with a simple zoom-out, zoom-in (might need trackpad to reproduce smoothness): |
The amount of tiles retained on each side should be configurable. |
On mobile devices this effect is very annoying even in wifi env. The tiles load to slow if you just scroll around the map. It takes several seconds to load a new tile, displaying white background. |
Basically reuses some of the ideas from #3551 and the square-by-default tileSize option.
Basically reuses some of the ideas from #3551 and the square-by-default tileSize option.
Hello, Just wanted to add my "+1" to this issue with my use case. Behavious was acceptable in 0.7.7 but really laggy in 1.0.x. because of what @yohanboniface explains. I am not sure what the best solution is here, having a configurable tile buffer kept in memory (@hyperknot) could work in some situations (panning) but zooming in and out would still suffer from the same issue. I think having some kind of tile queue kept in memory with a Time-To-Live or a "last seen" parameter would be a better solution. That queue would have a configurable max length (0 by default if you want to keep the existing behaviour) but like @pke says, it is annoying even in wifi envrionnements, so the default value should be more than 0 in my opinion. Fabien |
Hello, I had a go at this issue. At first I tried having an array as a property of the GridLayer for unused tiles like in leaflet 0.7.7 but I realized that the "this._tiles" object already had a similar purpose and the change could just be in the value of the "retain" property on tiles. I added a simple check based on diff between now and the "loaded" timing. If inferior to a value configurable in the options (no value by default = recycling disabled), the tile is retained Here is the PR if you want to give it a try: Add the option to your tileLayer, such as tileRecycleDelay: 15, which means that tiles loaded in the last 15 seconds are retained. What do you think? |
thanks @fnicollet for the effort! While its a good solution I think it does not help to adhere to the expectations user have from panning and zooming a map. When the user pans a map back and forth, even after 15secs she expects the the tiles that have been loaded in the surrounding to be still there. Also this patch of yours does not solve the problem of loading tiles that are only some pixels off the current view borders. They experience is then to see a white background when you try to pan around the borders. |
I think pruning based on tile age is useless. I would maybe have one configurable parameter for the number of tile rows loaded outside the visible area. And another configurable parameter for the number of loaded tiles kept in a FIFO buffer cache. |
Yes, the patch doesn't solve the problem of loading tiles that are just off the border but @IvanSanchez "bufferSize" does solve this (different) problem, see his commit just before my message You can't really compare vector maps and image tiles. The data size is different and the other issue with image tiles is the "bloated" DOM, which can make the map slow if too many DOM objects are on the map, wether or not they are currently displayed. |
I wasn't happy enough with the available solutions, then I spent a bit of time coding algorithms. So now we have:
IMHO the FIFO solution would be the best, but can use some better calculations (keep some number of off-screen pixels, account for map size and tile size, redo on a |
I'm waiting for a fifth option to decide. |
Until a new version in verse from @IvanSanchez I'm on the FIFO too: it gives better results when a user only goes in one direction and back, or when zooming. |
Amazing work @IvanSanchez :) I would go for both the buffer and the FIFO MRU solution. |
Really nice work @IvanSanchez. I think the solution is a combination of #4649 and #4650.
|
@yohanboniface This might take a while. |
<3 <3 <3 Github is missing a feature to merge a comment into the code. |
Now we have a Leaflet T-Shirt: |
Yes, #4650 looks very simple and perfect. I'd just call it noPruneRange since it's actually the non-pruning tiles. |
@hyperknot Renamed. |
Seems like everyone thinks that #4650 is the way to go. @hyperknot promised to whip up a few unit tests, so the plan is to wait a couple of days for that to happen. |
Thanks! I'd like to test this in my mapbox app. However mapbox still uses leaflet 0.7. How can I force it to use the newest version? fwiw: I am using webpack to bundle my app. |
@pke you won't :-) Mapbox is using MapboxGL JS now. But you can use Mapbox maps in Leaflet rc1 very simply! |
@hyperknot mapbox-js is not MapboxGL. Its still supported isn't it? How would I load mapbox maps in leaflet? |
Is it somehow possible to delay or cancel the "tileunload" event? Then everybody would be able to implement his own unloading strategy. |
@AndreasSchmid1 sorry, no, the pruning algorithm isn't customizable like that for now. |
1.0 master:
0.7:
Git bisect points to a4e8f4e
@mourner @IvanSanchez any clue, or should I investigate? :)
The text was updated successfully, but these errors were encountered: