-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
add support for dynamic vector tiles rendering #3709 #8048
Conversation
Hey @stepankuzmin I haven't looked in detail, but... Does this just replace a source's definition, ie. it's TileJSON? ie. you can't say, change the geometry of this feature or change this feature's attributes? If that's the case, I'd prefer not calling it setData as it's a bit different to a GeoJSON setData. Just thinking out loud, is it better to have a more general setSourceProperty? Similar to setLayoutProperty and setPaintProperty to change any source values. |
Hey @andrewharvey! Yes, |
I've changed source.setSourceProperty('url', 'mapbox://stepankuzmin.cjtd6f5jh2eyk2xqtsyuo1kbz-3w7xz');
source.setSourceProperty('tiles', [
"mapbox://tiles/stepankuzmin.cjtd6f5jh2eyk2xqtsyuo1kbz-3w7xz/{z}/{x}/{y}.vector.pbf",
"mapbox://tiles/stepankuzmin.cjtd6f5jh2eyk2xqtsyuo1kbz-3w7xz/{z}/{x}/{y}.vector.pbf"
]); |
VectorTileSource#setData
for dynamic vector tiles rendering #3709VectorTileSource#setSourceProperty
for dynamic vector tiles rendering #3709
Great feature. Without this for a simple update of tile url will cause map.addSource(<newSourceID>, {
type: 'vector',
url: <newTileURL>
});
// For each layer connected with old source
map.setLayoutProperty(<layerUsedOldSource>, 'source', <newSourceID> });
map.removeSource(<oldSourceID>); |
This will be great for our app, which changes source url parameters based on which filters a user chooses. When replacing a source and clearing the cached tiles, will this prevent the flicker that currently happens when using |
Hi, @aMoniker! Yes, this PR prevents flickering when updating sources. |
src/source/vector_tile_source.js
Outdated
} | ||
|
||
const options = { [name]: value}; | ||
extend(this, pick(options, ['url', 'scheme', 'tileSize'])); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be nicer if we could avoid the duplication of code between here and the constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've moved options setter into _updateOptions
method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@stepankuzmin Thanks for putting together this concept.
The current implementation of setSourceProperty
leaves a bit unclear how
setSourceProperty
can be used to override properties (like min/maxzoom) of a source after it is set using a URL. The call toload
would re-fetch the tilejson and write over those properties.load()
is asynchronous, butthis.options
would already have been updated with new values and referenced bySourceCache#update
before the async load is complete.
This type of change is better suited to be handled via #4875
} | ||
|
||
const sourceCache = this.map.style.sourceCaches[this.id]; | ||
sourceCache.clearTiles(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The load()
method should fire the metadata
and content
events that trigger a SourceCache#reload()
and clears tiles when the new tilejson is available. See:
mapbox-gl-js/src/source/source_cache.js
Lines 69 to 83 in 4eb7d4e
this.on('data', (e) => { | |
// this._sourceLoaded signifies that the TileJSON is loaded if applicable. | |
// if the source type does not come with a TileJSON, the flag signifies the | |
// source data has loaded (i.e geojson has been tiled on the worker and is ready) | |
if (e.dataType === 'source' && e.sourceDataType === 'metadata') this._sourceLoaded = true; | |
// for sources with mutable data, this event fires when the underlying data | |
// to a source is changed. (i.e. GeoJSONSource#setData and ImageSource#serCoordinates) | |
if (this._sourceLoaded && !this._paused && e.dataType === "source" && e.sourceDataType === 'content') { | |
this.reload(); | |
if (this.transform) { | |
this.update(this.transform); | |
} | |
} | |
}); |
Is there a reason this additional call to clearTiles
is needed here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SourceCache#reload()
doesn't remove existing tiles. It causes that after changing the source URL, you still have cached tiles from previous source URL, because they are not in errored
state (see https://github.com/mapbox/mapbox-gl-js/blob/master/src/source/source_cache.js#L225).
So I call clearTiles
to force remove previously cached tiles.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can force errored
state thought, but it looks not so good as clearing tiles with clearTiles
I think about passing an optional callback in setSourceProperty(name: string, value: mixed) {
if (this._tileJSONRequest) {
this._tileJSONRequest.cancel();
}
const sourceCache = this.map.style.sourceCaches[this.id];
sourceCache.clearTiles();
this.load(() => {
const options = { [name]: value};
this._updateOptions(options);
});
} |
This will be an absolute game-changer for us if this gets merged. We have a platform that uses 'parameterized' vector tile URLs to apply time-based filtering etc. To be able to change source URLs without producing the flicker associated with a full map repaint will be amazing. |
@stepankuzmin , thanks a lot for this! I'm closing this for now since there hasn't been any activity for a month or so. Feel free to repoen it once you have more progress on it. |
@arindam1993 I think it's better left open (even as a draft if not completely ready yet) so it can continue to be worked on in a collective way. I'll try to help review if that's what it needs. |
@stepankuzmin thanks for working on this! Let me know if you would like help pushing any of these last things over the finish line I think there are a couple tasks remaining before we can merge this:
namingIt looks like existing methods to update source properties are one method per property. For example, |
Hey @ansis! Thanks for the clarification. I agree with one method per property and I'll get back to this issue soon. |
I've just found this issue as it's likely to be useful for a client I'm working with. The use case there is slightly different: updating server-side data, and wanting to force a tile refresh, without a change in URL. (Although likely to be implemented with a slightly different URL [
It looks like all the interest here is really just about |
Hi everyone! Sorry for the late response. I've renamed |
Are you guys still alive? |
@stepankuzmin that's a great addition I must say. Would be nice to also support raster source with the same functionality. |
VectorTileSource#setSourceProperty
for dynamic vector tiles rendering #3709VectorTileSource#setSourceProperty
for dynamic vector tiles rendering #3709
VectorTileSource#setSourceProperty
for dynamic vector tiles rendering #3709
@ogix Just an aside that this shouldn't be needed for your use case (as I've understood it), GL JS will automatically re-request tiles which have expired based on the maxage cache header you set, check it out with the |
I gave this a test drive, I couldn't find any issues, seems to work as intended. |
Thanks everyone for the help landing this, and apologies for the delay! |
Having a I've found that following the same steps of calling |
Hi @stepankuzmin, I noticed in comment #8048 (comment) you mention this new feature prevents tile flickering. When reloading a tile source using setTiles, the tiles vector layer is still flickering for us. Is this the way we should be using it to avoid flickering? I can't seem to find any other method. Using latest mapbox 2.x. |
Hi @jonseppanen Can you, please, provide some reproducible example for this? |
Hi @stepankuzmin. I'm also seeing flickering like @jonseppanen above. You can see a minimal example here: https://codepen.io/rsummerhill/pen/dyOGXpQ?editors=0010 From looking at the source code I'm guessing that features are removed from the map when |
FWIW, I have found this to be an effective and flicker-free way of refreshing tiles:
|
Is there a solution for the flickering when calling |
Hey everyone! This PR adds the equivalent of GeoJSON Source
setData
to the Vector Sources, so a user can change sources without updating the whole map style.Usage:
What do you think about it?
I'm not sure how to properly reload cached tiles, maybe someone can point me a direction for this?