Skip to content
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

Fix stale tiles being shown when calling VectorTileSource#setTiles while the map is moving #913

Conversation

vanilla-lake
Copy link
Contributor

Fix stale tiles being shown when calling VectorTileSource#setTiles while the map is moving

Before this PR, when calling VectorTileSource#setTiles, the source cache was cleared before the TileJSON was reloaded asynchronously. If the map was moving while VectorTileSource#setTiles was called, it was possible for the cache to be repopulated with the old tiles if VectorTileSource#loadTile happened to be called before the TileJSON had been reloaded. This was due to the VectorTileSource#tiles property, which is used by VectorTileSource#loadTile, only being updated asynchronously after the TileJSON had reloaded.

Launch Checklist

  • Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
  • Briefly describe the changes in this PR.
  • Write tests for all new functionality.
  • Manually test the debug page.
  • Add an entry inside this element for inclusion in the maplibre-gl-js changelog: <changelog></changelog>.

@github-actions
Copy link
Contributor

github-actions bot commented Jan 28, 2022

Bundle size report:

Size Change: +5 B
Total Size Before: 194 kB
Total Size After: 194 kB

Output file Before After Change
maplibre-gl.js 184 kB 184 kB +5 B
maplibre-gl.css 9.5 kB 9.5 kB 0 B
ℹ️ View Details
Source file Before After Change
rollup/build/tsc/src/source/vector_tile_source.js 1.11 kB 1.11 kB -1 B

@HarelM
Copy link
Collaborator

HarelM commented Jan 28, 2022

Besides the comment above I think this looks good.
I think that someone else should go over it besides me though... @birkskyum @wipfli

Copy link
Member

@birkskyum birkskyum left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! With the latest commit it lgtm.

@birkskyum birkskyum merged commit 321c416 into maplibre:main Jan 28, 2022
@vanilla-lake vanilla-lake deleted the Fix-stale-tiles-being-shown-when-calling-VectorTileSource#setTiles-while-the-map-is-moving branch January 28, 2022 15:14
@vanilla-lake
Copy link
Contributor Author

No problem! 😄

@HarelM
Copy link
Collaborator

HarelM commented Feb 10, 2022

@vanilla-lake I'm running the benchmark and i see the following error in the console:
image
I have a feeling this is related to this change.
Can you please check if this is caused by this PR by any chance?
To see this do: npm run start-bench and open the browser at http://localhost:9966/bench/versions and wait for a while.

@wipfli
Copy link
Member

wipfli commented Feb 10, 2022

I think I saw a similar error message with the headless version npm run benchmarks. Also there, you have to wait a while until this happens.

@birkskyum
Copy link
Member

birkskyum commented Feb 10, 2022

I see this sourceCaches error as well on the commit before this (c8e8345), and I see it immediately when going to the workertransfer test

@vanilla-lake
Copy link
Contributor Author

The issue is indeed related to the changes in this PR. Sorry about that... I created PR #984 which should hopefully resolve the issue.

@khmm12
Copy link
Contributor

khmm12 commented Mar 15, 2022

@vanilla-lake the issue is also reproducible in "real world" if you destroy the map instance during JSON request.

UPDATE: How deep does the rabbit hole go? (c)

#1099

@vanilla-lake
Copy link
Contributor Author

vanilla-lake commented Mar 15, 2022

Thanks for the fix @khmm12! Your fix seems correct, since calling SourceCache.onRemove() will call VectorTileSource.onRemove() which will itself cancel _tileJSONRequest.

This error could happen anytime there is a vector tile source and Map.style becomes null or undefined. As far as I can see, the only ways this can happen is if Map.setStyle() is called with a null or undefined argument. In this case, Style._remove() should be called on the existing style from Map._updateStyle(), now cancelling any pending TileJSON requests thanks to your fix.

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.

5 participants