Arbitrary states for feature interactivity #6263

asheemmamoowala · 2018-03-02T02:02:07Z

Based on @ansis' work in data-update-proof-of-concept

Implements map.setFeatureState(), a new method to set key: value pairs on specific features in a source. The source must provide unique feature id's per source-layer for this to work correctly.

Also implemented ['state', '<stateName>'] expression syntax. Similar to get, this allows accessing a feature's state instead of source data property.

todo

Apply state to all symbol instances
~~Improve ExpressionCreate benchmark~~
Implement feature id checking per source-layer
Only evaluate state-dependent layers
- Conditionally enable dynamicDraw on vertex and index buffers
Don't reparse expressions (Make Expressions fully deserializable #6255)

Launch Checklist

write tests for all new functionality
document any changes to public APIs
post benchmark scores
create debug page

cc @ansis @mollymerp

asheemmamoowala · 2018-03-09T00:40:32Z

src/data/bucket.js

@@ -89,6 +93,7 @@ module.exports = {
            // look up StyleLayer objects from layer ids (since we don't
            // want to waste time serializing/copying them from the worker)
            (bucket: any).layers = layers;
+            (bucket: any).stateDependentLayers = layers.filter((l) => l.isStateDependent());


Pre-compute the layers with state-dependent expressions during deserialize. These are needed to re-compute the expressions in ProgramConfiguration until expression evaluators can be re-created when deserialized (#6255)
After that change, layer ids will be sufficient

mollymerp

@asheemmamoowala this is looking good! I think the biggest question left for me is whether this is the best way to store and mutate the individual features' states.

I wonder if we could use the _idMap on each bucket as the canonical state store. Keeping state/state changes separate from the feature ids and indices seems like it may add in some unnecessary complexity.

mollymerp · 2018-03-26T21:19:11Z

src/data/program_configuration.js

+    updatePaintArrays(featureStates: FeatureStates, vtLayer: VectorTileLayer, layers: $ReadOnlyArray<TypedStyleLayer>): boolean {
+        let changed: boolean = false;
+        layers.forEach((layer) => {
+            changed =  changed || this.programConfigurations[layer.id].updatePaintArrays(featureStates, vtLayer, layer);


I think in general we avoid using forEach – looks like this would be better suited for a for (let i = 0; i <layers.length;... loop with a break statement if changed is ever true.

👍 or even for(const layer of layers) {

(I think we can't break early from the loop, though, since we always need to call updatePaintArrays on each layer's ProgramConfiguration)

mollymerp · 2018-03-26T22:23:07Z

src/data/bucket/circle_bucket.js

                options.featureIndex.insert(feature, geometry, index, sourceLayerIndex, this.index);
            }
        }
    }

+    update(states: FeatureStates, vtLayer: VectorTileLayer) {
+        if (!this.stateDependentLayers.length) return;
+        this.changed = this.programConfigurations.updatePaintArrays(states, vtLayer, this.stateDependentLayers);


can we just use uploaded for this state as well instead of adding changed state? in other words, a dirty = true return value from updatePaintArrays would set uploaded to false

It looks like uploaded and changed may be distinct in order to avoid re-uploading the layout and index buffers when only the paint buffers need to change.

Alternative simplification: could we move the changed tracking into ProgramConfigurationSet?

@anandthakker - If we follow that idea, the correct place to track changes would be in the individual ProgramConfiguration's. That would require that Bucket.uploadPending() check the state of each program.

It's not clear to me that the result is simpler.

If we follow that idea, the correct place to track changes would be in the individual ProgramConfiguration's

We can put a dirty property on ProgramConfigurationSet, which is set during updatePaintArrays, checked in upload() to decide whether to actually do work (and then cleared). I suppose it might not be simpler per se, but it has the advantage of localizing and centralizing this state tracking in program_configuration.js, rather than having it replicated in each bucket.

mollymerp · 2018-03-26T22:38:55Z

src/source/source_cache.js

@@ -52,6 +54,7 @@ class SourceCache extends Evented {
    transform: Transform;
    _isIdRenderable: (id: number) => boolean;
    used: boolean;
+    _state: SourceFeatureState


are there any alternatives to storing state for all features on all loaded tiles in the SourceCache? seems like this is a muddling of concerns 💭

I think it does make sense to store a SourceFeatureState here on the SourceCache, since it belongs to a source and should be discarded precisely when a source is discarded.

But I agree that the state-aware logic in below feels a bit out of place here. What if we:

move the responsibility for calling Tile#updateFeatureState during prepare() directly into SourceFeatureState#coalesceChanges() (i.e., take a set of tiles as a parameter to coalesceChanges)?

Add a SourceFeatureState#initializeTileState(Tile) method and replace the other two cases of tile.updateFeatureState(this._state.state); with this._state.initializeTileState(tile)?

Since SourceCache is responsible for creating sources and managing their tiles, it is the appropriate place to manage the feature state of the source as well

I disagree. SourceCache is already very difficult to reason about, in part because it's doing too much. (See also #2291.) Ideally, it would not manipulate the "content" of its Tiles at all (except perhaps setting tile.state, since that's directly related to its job of caching tiles).

mollymerp · 2018-03-26T23:05:14Z

src/style/style.js

@@ -780,6 +780,23 @@ class Style extends Evented {
        return this.getLayer(layer).getPaintProperty(name);
    }

+    setFeatureState(source: string, feature: string, key: string, value: any, sourceLayer?: string) {
+        if (this.sourceCaches[source] === undefined) {
+            throw new Error('There is no source with this ID');


for a more helpful error message, maybe print the passed source id?

mollymerp · 2018-03-26T23:09:27Z

src/style/style.js

+
+    getFeatureState(source: string, feature: string, key?: string, sourceLayer?: string) {
+        if (this.sourceCaches[source] === undefined) {
+            throw new Error('There is no source with this ID');


same comment about error as above

anandthakker · 2018-03-27T17:27:30Z

src/data/program_configuration.js

+            });
+        }
+
+        this._bufferPos = length;


🤔 should this be += instead of =?

At first, I thought that too. The length input parameter is confusing - its the end index of the paint array after adding new elements. Its usage in the binder popluatePaintArray makes it more clear.

Oh, yeah that's confusing! Maybe we should rename the length parameter to targetLength or something?

anandthakker · 2018-03-27T17:28:32Z

src/data/program_configuration.js

    constructor() {
        this.binders = {};
        this.cacheKey = '';

        this._buffers = [];
+        this._idMap = {};
+        this._bufferPos = 0;


Nit: name this a little more descriptively: maybe _populatedVertexCount? _paintBufferLength? 🤷‍♂️

anandthakker · 2018-03-27T17:29:35Z

src/data/program_configuration.js

+        for (const id in featureStates) {
+            const posArray = this._idMap[id];
+
+            if (posArray) {


Nit: if (!posArray) continue;

anandthakker · 2018-03-27T17:36:05Z

src/data/program_configuration.js

+
+            if (posArray) {
+                for (let i = 0; i < posArray.length; i++) {
+                    const pos = posArray[i];


Nit: for (const pos of posArray) {

anandthakker · 2018-03-27T17:39:41Z

src/data/program_configuration.js

+    updatePaintArrays(featureStates: FeatureStates, vtLayer: VectorTileLayer, layers: $ReadOnlyArray<TypedStyleLayer>): boolean {
+        let changed: boolean = false;
+        layers.forEach((layer) => {
+            changed =  changed || this.programConfigurations[layer.id].updatePaintArrays(featureStates, vtLayer, layer);


👍 or even for(const layer of layers) {

anandthakker · 2018-03-27T18:52:25Z

src/source/source_cache.js

@@ -52,6 +54,7 @@ class SourceCache extends Evented {
    transform: Transform;
    _isIdRenderable: (id: number) => boolean;
    used: boolean;
+    _state: SourceFeatureState


I think it does make sense to store a SourceFeatureState here on the SourceCache, since it belongs to a source and should be discarded precisely when a source is discarded.

But I agree that the state-aware logic in below feels a bit out of place here. What if we:

move the responsibility for calling Tile#updateFeatureState during prepare() directly into SourceFeatureState#coalesceChanges() (i.e., take a set of tiles as a parameter to coalesceChanges)?

Add a SourceFeatureState#initializeTileState(Tile) method and replace the other two cases of tile.updateFeatureState(this._state.state); with this._state.initializeTileState(tile)?

anandthakker · 2018-03-27T18:54:54Z

src/style-spec/expression/definitions/index.js

+                (ctx, [key]) => get(key.evaluate(ctx), ctx.state())
+            ]
+        ]
+    },


Since there's only one implementation, use:

ValueType, [StringType], (ctx, [key]) => get(key.evaluate(ctx), ctx.state())

anandthakker · 2018-03-27T18:56:01Z

src/style-spec/expression/is_constant.js

@@ -8,6 +8,8 @@ function isFeatureConstant(e: Expression) {
    if (e instanceof CompoundExpression) {
        if (e.name === 'get' && e.args.length === 1) {
            return false;
+        } else if (e.name === 'state' && e.args.length === 1) {


Just e.name === 'state', no need to check argument count

anandthakker · 2018-03-27T18:56:18Z

src/style-spec/expression/is_constant.js

@@ -28,6 +30,19 @@ function isFeatureConstant(e: Expression) {
    return result;
 }

+function isStateConstant(e: Expression) {
+    if (e instanceof CompoundExpression) {
+        if (e.name === 'state' && e.args.length === 1) {


No need to check e.args.length

anandthakker · 2018-03-27T18:57:44Z

src/ui/map.js

+     */
+    setFeatureState(source: string, feature: string, key: string, value: any, sourceLayer?: string) {
+        this.style.setFeatureState(source, feature, key, value, sourceLayer);
+        this._rerender();


Use this._update()

asheemmamoowala · 2018-03-27T23:40:00Z

src/data/bucket/circle_bucket.js

                options.featureIndex.insert(feature, geometry, index, sourceLayerIndex, this.index);
            }
        }
    }

+    update(states: FeatureStates, vtLayer: VectorTileLayer) {
+        if (!this.stateDependentLayers.length) return;
+        this.changed = this.programConfigurations.updatePaintArrays(states, vtLayer, this.stateDependentLayers);


@anandthakker - If we follow that idea, the correct place to track changes would be in the individual ProgramConfiguration's. That would require that Bucket.uploadPending() check the state of each program.

It's not clear to me that the result is simpler.

asheemmamoowala · 2018-03-27T23:51:34Z

src/data/program_configuration.js

+                        const binder: Binder<any> = this.binders[property];
+                        if (binder instanceof ConstantBinder) continue;
+                        if ((binder: any).expression.isStateDependent === true) {
+                            //AHM: Remove after https://github.com/mapbox/mapbox-gl-js/issues/6255


Is it OK to leave this comment in here? Or should it be separated into a ticket?

asheemmamoowala · 2018-03-28T00:14:09Z

src/data/program_configuration.js

+            });
+        }
+
+        this._bufferPos = length;


At first, I thought that too. The length input parameter is confusing - its the end index of the paint array after adding new elements. Its usage in the binder popluatePaintArray makes it more clear.

src/source/source_cache.js

@@ -52,6 +54,7 @@ class SourceCache extends Evented {
    transform: Transform;
    _isIdRenderable: (id: number) => boolean;
    used: boolean;
+    _state: SourceFeatureState


lucaswoj · 2018-04-03T23:57:14Z

src/ui/map.js

+     * @param {string | number | boolean} value The value to be set.
+     * @param {string} sourceLayer (optional) The source-layer identifier for Vector Tile sources.
+     */
+    setFeatureState(source: string, feature: string, key: string, value: any, sourceLayer?: string) {


Calls to setFeatureState with a sourceLayer parameter may be more clear if we pass the sourceLayer as an object rather than an optional parameter: source: string | { id: string; layerId: string; }. What do you think?

This seems like a good improvement! I'd also like to make the API error out if a source layer is not provided for Vector sources. It would mean checking the type of the source id, but it would prevent erroneous use of the API.

lucaswoj · 2018-04-04T00:00:02Z

src/source/source_state.js

+const util = require('../util/util');
+const Tile = require('./tile');
+
+export type FeatureStates = {[feature_id: string]: {[key: string]: string | number | boolean }};


Do we intend to only allow string | number | boolean in feature states? Aren't GeoJSON properties allowed to include null, arrays, and objects now?

I think everything but object is OK to use. Nested objects are not supported in expressions, so it wouldn't be usable for runtime styling, and possibly cause confusion.

@anandthakker Are nested objects supported in GeoJSON feature properties? I suspect the least confusing thing will be parity between GeoJSON feature properties and feature state.

Yep, they're supported. E.g., the following works:

map.on('load', () => { map.addLayer({ id: 'test', source: { type: 'geojson', data: { type: 'Feature', properties: { x: { y: 'hello' } }, geometry: { type: 'Point', coordinates: [0, 0] } } }, type: 'symbol', layout: { 'text-field': ["get", "y", ["get", "x"]] } }) })

Although note that in queryRenderedFeatures, array or object property values currently get stringified.

Thanks for the correction @anandthakker! In that case, feature states should accept all JSON types.
For queryRenderedFeatures, this means that arrays and objects would be stringified in properties but not in state.

Yeah, that's fine -- qRF shouldn't stringify them, it's just a limitation of a current implementation detail: #2434

lucaswoj · 2018-04-04T00:01:25Z

src/data/bucket/symbol_bucket.js

@@ -414,16 +418,28 @@ class SymbolBucket implements Bucket {
        }
    }

+    update(states: FeatureStates, vtLayer: VectorTileLayer) {


I would assume that a method called Bucket#update did something more general than this (like update all the data in the bucket). Would a name like Bucket#updateFeatureState be more clear?

Bucket#update updates the dynamic buffers in the bucket with the passed in states. The layout and index buffers are static, and so cannot be updated. The more specific name is more descriptive, but it doesn't distinguish itself from any other type of update.

lucaswoj · 2018-04-04T00:01:54Z

src/data/bucket/symbol_bucket.js

+            this.opacityVertexBuffer = context.createVertexBuffer(this.opacityVertexArray, shaderOpacityAttributes, true);
+            // This is a performance hack so that we can write to opacityVertexArray with uint32s
+            // even though the shaders read uint8s
+            this.opacityVertexBuffer.itemSize = 1;


Can you explain how this works in more detail? Is this prone to break in the future?

Maybe @ChrisLoer can help with its futureproof-ness. I havent changed this code, except move it so it runs only once.

Yeah, this is a relatively brittle performance hack. We might be able to make it more future proof by pushing the hack into StructArray so the external interface was always clearly "this is an array of uint8s" even if uint32s were used internally. But it's a weird special case and it's not clear to me how we'd define that for StructArray.

Background: we do lots of opacity vertex updates for symbols: for each symbol, we have one packed opacity value that we need to update on all four vertices for every glyph. Profiling in Chrome, the actual cost of building the array showed up as a significant part of rendering time. Pushing n/4 uint32s turned out to be significantly faster than pushing n uint8s (see #5150 (comment), there's an additional wrinkle that the same change included packing "target opacity" into the same value). At some point it will probably make sense to re-test this optimization, since testing this kind of thing is so squirrelly and changes in the javascript engine could render it useless.

We might be able to make it more future proof by pushing the hack into StructArray

I think that's a good approach 👍

ansis

Overall this looks great! I commented on a couple things that raised some questions for me and a couple things that looked like possible bugs. The changes I think might be necessary are:

a fix for the this.needsUpload/|| short-circuiting if that is not intentional
queryPadding possibly being invalid if the statistics.max value changed

ansis · 2018-04-06T20:36:36Z

src/data/program_configuration.js

+                paintArray.emplace(i, value);
+            }
+
+            this.statistics.max = Math.max(this.statistics.max, value);


This doesn't handle the case where the maximum value is replaced with a smaller value and the max should decrease. I think this is fine though, because having a bigger max won't cause bugs the way we use it and maintaining the real max value would be more complicated and expensive. This should be fine and safe the way it is, right?

Keeping track of the max value correctly may not be worth it. When a property value is reduced, the update logic would need to filter the previously populated values to check if there is a new lower max.

Leaving this as is won't return incorrect results, but it will increase the number of features that are matched in the initial query (with queryPadding) and then have to be filtered out.

If there are use cases that significantly affect the performance of queryRenderedFeatures, this can be revisited.

ansis · 2018-04-06T20:41:44Z

src/ui/map.js

+     * 
+     * @returns {any} The value of the specified specified state or all states.
+     */
+    getFeatureState(source: string | { sourceId: string; sourceLayer: string; }, feature: string, key?: string): any {


The optional key param changes the return type. Should returning the full state object be split into a separate method? If it's useful to return the entire state as an object, should it be possible to set state with an object instead of individual calls for each key?

The optional key parameter is only for internal use and can be removed from the external interface.
I'm not entirely sure that getting and setting the entire state as an object is a good idea. Setting state as a single object can create confusion - Does it extend the current object or replace it? Updating a single value would require fetching the entire object first and setting it again.

ansis · 2018-04-06T20:44:33Z

src/data/program_configuration.js

+
+    updatePaintArrays(featureStates: FeatureStates, vtLayer: VectorTileLayer, layers: $ReadOnlyArray<TypedStyleLayer>) {
+        for (const layer of layers) {
+            this.needsUpload =  this.needsUpload || this.programConfigurations[layer.id].updatePaintArrays(featureStates, vtLayer, layer);


If this.needsUpload is true before is gets called then the || will short-circuit and this.programConfigurations[layer.id].updatePaintArrays(...) will not get called. It should always get called, right?

This is a great catch! I didn't run into the short-circuiting, but it is possible.

ansis · 2018-04-06T20:46:38Z

src/source/source_state.js

+            const layerStates = {};
+            for (const id in this.stateChanges[sourceLayer]) {
+                this.state[sourceLayer][id] = extend(
+                                                {},


Is it necessary to create and extend a new object? or could it just extend the existing this.state[sourceLayer][id]?

ansis · 2018-04-06T20:51:58Z

src/data/program_configuration.js

+                paintArray.emplace(i, value);
+            }
+
+            this.statistics.max = Math.max(this.statistics.max, value);


Also, the max changing means the cached queryPadding value might be invalid:

mapbox-gl-js/src/source/tile.js

Lines 170 to 174 in bd5a653

this.queryPadding = 0;

for (const id in this.buckets) {

const bucket = this.buckets[id];

this.queryPadding = Math.max(this.queryPadding, painter.style.getLayer(bucket.layerIds[0]).queryRadius(bucket));

}

I think we need some way of either invalidating it, or if it is cheap enough, recalculating it every time we need to use it instead of caching it.

lucaswoj · 2018-04-18T19:52:55Z

I haven't been this excited about a new feature in a loooong time!

ansis · 2018-04-18T20:09:11Z

This looks good to me. The only remaining questions I have are details about the external interface. The current interface seems fine but I think it's worth looking at this extra carefully since it will be public.

The current public signature is:

setFeatureStateValue(
    source: string | { sourceId: string; sourceLayer: string; },
    feature: string,
    key: string,
    value: any)

setFeatureStateValue seems like a good name, but would setFeatureState be just as clear?

What if the method signature was:

setFeatureStateValue(
    feature: {source: string; sourceLayer: string; id: string},
    key: string,
    value: any)

The advantages I'm seeing:

(very minor) the first argument is always an object
if we set source and sourceLayer on geojson features returned by queryRenderedFeatures and on the features event handlers are called with we could pass those features directly to this method:
setFeatureStateValue(feature, 'key', 'value') instead of
setFeatureStateValue({ source: 'id', sourceLayer: 'layerId' }, feature.id, 'key', 'value').

@lucaswoj @ChrisLoer @anandthakker @mollymerp @jfirebaugh did any of you want to review this any further?

jfirebaugh · 2018-04-18T20:21:35Z

I agree with @ansis's suggestion for public API. In addition, I think the expression operator should be spelled "feature-state" to match the setter and leave room for the possibility of future "global" state.

anandthakker · 2018-04-18T20:24:20Z

Should there be a method that allows updating multiple state values at once? E.g., updateFeatureState(feature, { key1: 'value1', key2: 'value2' })

setFeatureStateValue(feature, 'key', 'value') instead of setFeatureStateValue({ source: 'id', sourceLayer: 'layerId' }, feature.id, 'key', 'value').

This is appealing. Is there any chance it would come across as though this method actually makes changes to feature itself?

asheemmamoowala · 2018-04-18T20:38:38Z

queryPadding possibly being invalid if the statistics.max value changed

@ansis I opened #6536 to make queryPadding computation on-demand.

asheemmamoowala · 2018-04-18T21:49:52Z

Should there be a method that allows updating multiple state values at once? E.g., updateFeatureState(feature, { key1: 'value1', key2: 'value2' })

@anandthakker Were you thinking of this as a replacement for setFeatureStateValue, or in addition to it?

asheemmamoowala · 2018-04-18T21:58:11Z

if we set source and sourceLayer on geojson features returned by queryRenderedFeatures and on the features event handlers

I'm concerned that this would make the API less convenient to use when not directly piping the result from an event handler or call to Map#queryRenderedFeatures(). It also makes it hard to hold on to just the 'identifier' portion of a feature without the additional layer and properties objects.

A globalid object on geojson features could be added instead, that corresponds to the {source: string; sourceLayer: string; id: string} object.

map.on('mousemove', 'places', function(e) {
  const feature = e.features[0];
  map.setFeatureStateValue(feature.globalid, 'hover', true);
  currentId = feature.globalid;

anandthakker · 2018-04-18T23:20:03Z

Were you thinking of this as a replacement for setFeatureStateValue, or in addition to it?

I was thinking of it as an alternative, mainly because it seems to me like it covers the setFeatureStateValue case nearly as ergonomically: updateFeatureState(feature, {key1: 'value1'}) (or updateFeatureState(feature, {[key]: value}) if key and value are variables) seems pretty close to setFeatureStateValue(feature, key, value)

asheemmamoowala · 2018-04-18T23:50:33Z

@anandthakker updateFeatureState breaks from the convention of get/set methods used on Map. I do like the idea of a single set/update method for multiple key/value pairs though.

Map#setFeatureState(feature, { key: value, ... }, update /* = true */) would follow the existing convention and avoid confusion over replace vs. merge of the state parameter

anandthakker · 2018-04-19T00:19:14Z

I'm okay with breaking with that convention in this case: 1) there are ideas about changing it anyway, and 2) I think it's different in a meaningful way: update is doing more than just setting a single property.

…iple properties.

lucaswoj · 2018-04-19T16:14:10Z

The original API design, setFeatureState(feature, key, value), is much more consistent with the rest of our APIs, such as setPaintProperty and ["feature-state"]. If we are going to adopt a new naming paradigm (i.e. update vs set), let's do it separately and everywhere.

I don't have a strong opinion about whether or not we also allow setFeatureState(feature, values).

#2741 is about internal architecture not naming conventions.

anandthakker · 2018-04-19T16:32:27Z

I see this not as a new naming convention, but instead a different behavior for which there isn't a precedent in our existing APIs. There's a broader convention that setXxxx() methods are merely setters, which would lead me to think that setFeatureState(feature, {p1: v1}) would replace the given feature's state with {p1: v1}, rather than merging {p1: v1} into the existing state.

jfirebaugh · 2018-04-19T17:06:37Z

React's setState does a merge. I think the name we had was fine.

anandthakker · 2018-04-19T18:00:02Z

Another inconsistency: our own setStyle() does not do a merge. See also #4006

…ation and tests.

asheemmamoowala · 2018-04-20T04:52:57Z

Benchmark results

thunderkid · 2018-05-25T00:31:10Z

This looks like nice technology, but I'm trying to understand exactly when it should be used. It's generally been encouraged to have a separate layer for features that change rapidly (eg a layer that contains only a copy of the hovered item but with some different styling.) That approach is pretty performant, and as long as the hovered item obscures the original it works well. So when should the separate hover-layer approach be used and when should one use a single layer with setFeatureState? Are there any general guidelines?

lucaswoj · 2018-05-25T17:14:52Z

So when should the separate hover-layer approach be used and when should one use a single layer with setFeatureState? Are there any general guidelines?

setFeatureState was designed specifically with the hover use case in mind. I would not recommend using the "hover layer approach" now that setFeatureState has been implemented.

The only case in which you may want to use a second layer is if the geometry of a feature needs to change rapidly (i.e. for a drawing plugin) as setFeatureState cannot change the feature's geometry.

shayke · 2018-06-23T15:09:40Z

I'm not sure if there is a bug in this feature or I'm using it wrong
but please take a look at those (hopefully) equivalent examples:

Using setFeatureState - http://jsfiddle.net/01b97w0L/672/
Using extra layer - http://jsfiddle.net/01b97w0L/673/

The idea is to select all "related" features to the selected one and color them as red (regardless of zoom level).
If you click on the left most line and zoom out

setFeatureState - Doesn't color the zoomed out features
"extra layer" - properly colors every feature on the map (regardless of zoom level)

EDIT: So it seems as setting the same feature id to those features work, is that how it's suppose to work? Since the documentation says the feature id should be unique but it doesn't say anything across zoom levels.

ansis · 2018-06-26T14:46:43Z

@shayke your version using setFeatureState isn't working because querySourceFeatures only returns features in the current viewport. This might be something we should fix for geojson sources where we do have all the features. In the mean time, you could filter the source geojson data yourself to get all the ids of features with the correct county

Amit-Gore · 2018-06-27T22:46:29Z

@lucaswoj Does setFeatureState always require feature.id? Can it work on feature.properties.name?
Any good working example will be appreciated!

asheemmamoowala · 2018-06-27T22:53:01Z

@Amit-Gore Currently setFeatureState does require the top-level feature.id attribute. See #6019 for a discussion on future improvements to this.

asheemmamoowala added the under development label Mar 2, 2018

asheemmamoowala commented Mar 9, 2018

View reviewed changes

asheemmamoowala force-pushed the update-paint-arrays branch from f10a553 to d1cd559 Compare March 9, 2018 19:56

mollymerp reviewed Mar 26, 2018

View reviewed changes

anandthakker reviewed Mar 27, 2018

View reviewed changes

asheemmamoowala commented Mar 28, 2018

View reviewed changes

asheemmamoowala changed the title ~~[wip] Arbitrary states for feature interactivity~~ Arbitrary states for feature interactivity Apr 3, 2018

lucaswoj reviewed Apr 4, 2018

View reviewed changes

asheemmamoowala force-pushed the update-paint-arrays branch from e65553b to bd5a653 Compare April 6, 2018 00:25

asheemmamoowala mentioned this pull request Apr 6, 2018

Decouple layout and populating paint attribute buffers #4576

Open

ansis suggested changes Apr 6, 2018

View reviewed changes

asheemmamoowala force-pushed the update-paint-arrays branch 2 times, most recently from 8e79e95 to 25d23f9 Compare April 17, 2018 21:20

ansis approved these changes Apr 18, 2018

View reviewed changes

asheemmamoowala force-pushed the update-paint-arrays branch from 25d23f9 to 6a88bce Compare April 18, 2018 20:26

asheemmamoowala mentioned this pull request Apr 18, 2018

Map.queryRenderedFeatures uses stale tile padding #6536

Open

Asheem Mamoowala added 4 commits April 18, 2018 18:47

Track feature id to feature index in paint buffers.

1fd379e

Fix minor typo

aafefb5

Implement state as properties

d49d7f6

Update API args order and support retrieval of all states

7cdd0dc

Asheem Mamoowala added 2 commits April 18, 2018 18:52

Separately signal upload and update for SybmolBuffer

f3174e8

Rename setFatureStateValue to updateState. Allow setting/getting mult…

adcc01e

…iple properties.

asheemmamoowala force-pushed the update-paint-arrays branch from 4b0aedb to adcc01e Compare April 19, 2018 01:52

Asheem Mamoowala added 2 commits April 19, 2018 12:11

Fix query integration tests.

2721a29

Rename feature state setter to Map#setFeatureState. Update document…

ad5e67e

…ation and tests.

ChrisLoer and others added 2 commits April 19, 2018 22:19

Update highlightpoints.html to use setFeatureState

9a2f755

Add Feature states benchmark

2bde933

asheemmamoowala removed the under development 🚧 label Apr 24, 2018

asheemmamoowala merged commit 0050d40 into master Apr 26, 2018

asheemmamoowala deleted the update-paint-arrays branch April 26, 2018 17:08

This was referenced Apr 26, 2018

RFC: Updating the appearance of individual features #6020

Closed

queryRenderedFeatures doesnt take into account feature states #6569

Merged

jfirebaugh mentioned this pull request Apr 27, 2018

map.addLayer / map.removeLayer / map.setFilter seem slow on Firefox #6561

Closed

mollymerp mentioned this pull request May 2, 2018

Data-driven styling support for *-pattern properties #6289

Merged

11 tasks

This was referenced May 5, 2018

QueryRenderedFeatures should include source and sourceLayer mapbox/mapbox-gl-native#11846

Closed

RFC: Feature identity for interactivity #6019

Closed

tusbar mentioned this pull request Jun 19, 2018

Use feature states for map highlights etalab/geo.data.gouv.fr#647

Merged

asheemmamoowala mentioned this pull request Jul 17, 2018

Port feature state APIs to Native mapbox/mapbox-gl-native#12413

Closed

brunoabinader mentioned this pull request Aug 13, 2018

Arbitrary states for feature interactivity mapbox/mapbox-gl-native#12613

Closed

	this.queryPadding = 0;
	for (const id in this.buckets) {
	const bucket = this.buckets[id];
	this.queryPadding = Math.max(this.queryPadding, painter.style.getLayer(bucket.layerIds[0]).queryRadius(bucket));
	}

Arbitrary states for feature interactivity #6263

Arbitrary states for feature interactivity #6263

Conversation

asheemmamoowala commented Mar 2, 2018 • edited Loading

todo

Launch Checklist

Choose a reason for hiding this comment

mollymerp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

asheemmamoowala Apr 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ansis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asheemmamoowala Apr 13, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lucaswoj commented Apr 18, 2018

ansis commented Apr 18, 2018 • edited Loading

jfirebaugh commented Apr 18, 2018

anandthakker commented Apr 18, 2018

asheemmamoowala commented Apr 18, 2018

asheemmamoowala commented Apr 18, 2018

asheemmamoowala commented Apr 18, 2018 • edited Loading

anandthakker commented Apr 18, 2018

asheemmamoowala commented Apr 18, 2018 • edited Loading

anandthakker commented Apr 19, 2018

lucaswoj commented Apr 19, 2018 • edited Loading

anandthakker commented Apr 19, 2018 • edited Loading

jfirebaugh commented Apr 19, 2018

anandthakker commented Apr 19, 2018

asheemmamoowala commented Apr 20, 2018

asheemmamoowala commented Mar 2, 2018 •

edited

Loading

asheemmamoowala Apr 5, 2018 •

edited

Loading

asheemmamoowala Apr 13, 2018 •

edited

Loading

ansis commented Apr 18, 2018 •

edited

Loading

asheemmamoowala commented Apr 18, 2018 •

edited

Loading

asheemmamoowala commented Apr 18, 2018 •

edited

Loading

lucaswoj commented Apr 19, 2018 •

edited

Loading

anandthakker commented Apr 19, 2018 •

edited

Loading

shayke commented Jun 23, 2018 •

edited

Loading

Amit-Gore commented Jun 27, 2018 •

edited

Loading