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

setFeatureState Throws Error #8634

Closed
econ24 opened this issue Aug 14, 2019 · 6 comments · Fixed by #9305
Closed

setFeatureState Throws Error #8634

econ24 opened this issue Aug 14, 2019 · 6 comments · Fixed by #9305
Assignees
Labels

Comments

@econ24
Copy link

econ24 commented Aug 14, 2019

mapbox-gl-js version: v1.2.1

browser: Chrome Version 70.0.3538.77 (Official Build) (64-bit)

Steps to Trigger Behavior

  1. Add vector tile source S
  2. Add 2 layers, layer A and layer B, both using the same source-layer from vector tile source S
  3. use setFeatureState on layer A
  4. remove layer B
  5. use setFeatureState on layer A

Link to Demonstration

https://jsbin.com/yuruvibiqa/edit?html,js,output

I removed my mapboxgl.accessToken.
The error occurs after the setTimeout function removes a layer.

Expected Behavior

setFeatureState works as intended.

Actual Behavior

setFeatureState throws the following error:

Uncaught TypeError: Cannot read property 'queryRadius' of undefined
at Ou.setFeatureState (tile.js:464)
at Vu.coalesceChanges (source_state.js:154)
at i.prepare (source_cache.js:170)
at ao.render (painter.js:340)
at r._render (map.js:1753)
at map.js:1837

@asheemmamoowala
Copy link
Contributor

@econ24 Thank you for reporting this issue. I am not able to reproduce the bug in the jsfiddle. It looks like the style used in the demo is private and cannot be accessed by other access tokens.

Could you provide a demo with a public style or one that uses a mapbox style or other simplified style where this issue can be reproduced.

@econ24
Copy link
Author

econ24 commented Aug 14, 2019

I updated the fiddle with a public style. I don't believe the mapbox account I am using is an enterprise account so the tileset should be public.

@verbeeckjan
Copy link

I experience this bug too. It is very annoying when switching between layers by adding and removing them.

@cammanderson
Copy link
Contributor

We are experiencing the same issue. Appears to be happening through switching layers and use of feature state.

@lpmcampos
Copy link

I'm facing this issue as well, under the same conditions of @econ24.
Mapbox GL version I'm using is v0.54.0 but I've tried also with v1.6.0 which presents the same behavour...

image

@asheemmamoowala
Copy link
Contributor

Did a little investigation to understand what is causing this issue.

When a layer is removed, it is marked for removal and then processed on the next render by updating the grouping of layers in the works by these two chunks

if (this._changed) {
const updatedIds = Object.keys(this._updatedLayers);
const removedIds = Object.keys(this._removedLayers);
if (updatedIds.length || removedIds.length) {
this._updateWorkerLayers(updatedIds, removedIds);
}

updateLayers(mapId: string, params: {layers: Array<LayerSpecification>, removedIds: Array<string>}, callback: WorkerTileCallback) {
this.getLayerIndex(mapId).update(params.layers, params.removedIds);
callback();
}

The update layer index is not re-applied to the tiles though, so existing buckets on the main thread continue to hang on to removed layer ids and are being processed unneccesarily at:

for (const id in this.buckets) {
const bucket = this.buckets[id];
// Buckets are grouped by common source-layer
const sourceLayerId = bucket.layers[0]['sourceLayer'] || '_geojsonTileLayer';

The shortcut to fix this would be to add a null guard on painter.style.getLayer(id) before further processing for that layer id. But this does not solve the root cause, which is that tiles and buckets on the main thread have not been notified of removed layers. This may also be true of updated layers, but I have not confirmed that as of yet.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants