queryRenderedFeatures doesnt take into account feature states #6569

asheemmamoowala · 2018-04-26T21:17:08Z

Addresses #6536 .

When updating a feature's paint properties using Map.setFeatureState, each tile should refresh its queryPadding. Additionally, feature state needs to be present on the feature object when evaluating paint properties for the intersection test.

asheemmamoowala · 2018-04-26T21:39:38Z

src/source/source_cache.js

@@ -168,7 +168,7 @@ class SourceCache extends Evented {
            this._source.prepare();
        }

-        this._state.coalesceChanges(this._tiles);
+        this._state.coalesceChanges(this._tiles, this.map ? this.map.painter : null);


@anandthakker, in #6236(comment), we discussed (and agreed upon) :

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)?

What do you think of moving back the tile-specific code from this._state.coalesceChanges into SourceCache, to reduce the painter spaghetti created here.
Not a burning issue though.

I'm thinking we should keep this as-is, because I expect this bit of 🍝 to be temporary: via #4875 and #4012, I'm hoping we'll end up in a place where each tile can easily hold on to a view of the style layers that were used to lay it out, and that will let us eliminate the need to thread painter through here.

asheemmamoowala · 2018-04-26T21:41:24Z

src/source/query_features.js

@@ -24,6 +24,7 @@ export function queryRenderedFeatures(sourceCache: SourceCache,
            wrappedTileID: tileIn.tileID.wrapped().key,
            queryResults: tileIn.tile.queryRenderedFeatures(
                styleLayers,
+                sourceCache.getFeatureState.bind(sourceCache),


SourceFeatureState maintains different states for each sourceLayer. Instead of passing state for the entire source, pass a function instead.

What's the advantage of passing this callback rather than a reference to the source's SourceFeatureState object?

SourceCache#getFeatureState has logic for handling GeoJSON sources where there is no sourceLayer in the query results. After considering that options, I decided that it would be better to pass the callback instead of duplicating that logic.

Hmm -- there are several other places in the code that already have awareness of _geojsonTileLayer... as such, I'd be inclined to say we should just dupe the logic, or else move it into SourceFeatureState.

@anandthakker What is the advantage of duplicating the logic over passing the bound function? This PR has already reduced the spred of _geojsonTileLayer by consolidating it in SourceCache.

It just seems clearer to me: I find a callback as a method of passing data to be harder to read and understand as compared to passing an object. Would moving the _geojsonTileLayer logic into SourceFeatureState give us the best of both worlds?

Would moving the _geojsonTileLayer logic into SourceFeatureState give us the best of both worlds?

I don't think so. I could change the name of the callback from getState to queryState, but all of these changes seem minor and not that significant advantages to whats already here.

I agree with @anandthakker that moving the logic into SourceFeatureState or duping the logic would be a bit clearer than passing the callback here

anandthakker · 2018-05-01T18:19:34Z

src/source/query_features.js

@@ -24,6 +24,7 @@ export function queryRenderedFeatures(sourceCache: SourceCache,
            wrappedTileID: tileIn.tileID.wrapped().key,
            queryResults: tileIn.tile.queryRenderedFeatures(
                styleLayers,
+                sourceCache.getFeatureState.bind(sourceCache),


What's the advantage of passing this callback rather than a reference to the source's SourceFeatureState object?

anandthakker · 2018-05-01T18:21:31Z

src/source/tile.js

@@ -418,7 +419,7 @@ class Tile {
        }
    }

-    setFeatureState(states: LayerFeatureStates) {
+    setFeatureState(states: LayerFeatureStates, painter: any) {


Could the second argument be the Style itself rather than painter, since it's only the style that we actually need?

I used painter to maintain similarity with loadVectorData, but it doesn't have to be that way - for both these methods. The only advantage I see of passing painter as the argument is that it communicates that the render style is being used. This becomes important when considering the Immutable Styl/Render-classes split.

Oh, yeah that's odd that loadVectorData takes a painter rather than a style object. I lean slightly towards passing Style here, but since this will get refactored in the nearish future with the immutable style state changes, it's not a big deal either way.

The only advantage I see of passing painter as the argument is that it communicates that the render style is being used. This becomes important when considering the Immutable Styl/Render-classes split

I think the correct style to use here will be the one that the tile will already hold a reference to -- namely, the one that was used at layout time, since that's the one that will be consistent with the data in the buckets.

since this will get refactored in the nearish future with the immutable style state changes, it's not a big deal either way.

I'd prefer to keep it as-is in that case.

I think the correct style to use here will be the one that the tile will already hold a reference to

👍

… tests

ansis

Looks good to me. I agree with @anandthakker about that one callback tweak, but other than that it looks good

ansis · 2018-05-03T20:38:42Z

src/source/query_features.js

@@ -24,6 +24,7 @@ export function queryRenderedFeatures(sourceCache: SourceCache,
            wrappedTileID: tileIn.tileID.wrapped().key,
            queryResults: tileIn.tile.queryRenderedFeatures(
                styleLayers,
+                sourceCache.getFeatureState.bind(sourceCache),


I agree with @anandthakker that moving the logic into SourceFeatureState or duping the logic would be a bit clearer than passing the callback here

asheemmamoowala requested review from ansis and anandthakker April 26, 2018 21:17

asheemmamoowala force-pushed the fix-query-padding branch from 85683b3 to a180b4f Compare April 26, 2018 21:26

asheemmamoowala commented Apr 26, 2018

View reviewed changes

asheemmamoowala force-pushed the fix-query-padding branch from a180b4f to facfb57 Compare April 27, 2018 00:04

anandthakker reviewed May 1, 2018

View reviewed changes

Asheem Mamoowala added 3 commits May 1, 2018 14:24

Add test for #6536

b446114

Update tile queryPadding with every feature state update

d15de74

Include feature state in expression evaluation for query intersection…

3b3e273

… tests

asheemmamoowala force-pushed the fix-query-padding branch from facfb57 to 3b3e273 Compare May 1, 2018 23:01

anandthakker approved these changes May 3, 2018

View reviewed changes

ansis approved these changes May 3, 2018

View reviewed changes

Use SourceFeatureState Object instead of bound feature state method.

b35126f

asheemmamoowala merged commit c9c4f61 into master May 3, 2018

asheemmamoowala deleted the fix-query-padding branch May 3, 2018 23:11

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

queryRenderedFeatures doesnt take into account feature states #6569

queryRenderedFeatures doesnt take into account feature states #6569

asheemmamoowala commented Apr 26, 2018

asheemmamoowala Apr 26, 2018

anandthakker Apr 27, 2018

asheemmamoowala Apr 26, 2018 •

edited

Loading

anandthakker May 1, 2018

asheemmamoowala May 1, 2018

anandthakker May 1, 2018

asheemmamoowala May 1, 2018

anandthakker May 2, 2018

asheemmamoowala May 2, 2018

ansis May 3, 2018

anandthakker May 1, 2018

anandthakker May 1, 2018

asheemmamoowala May 1, 2018

anandthakker May 1, 2018

asheemmamoowala May 1, 2018

ansis left a comment

ansis May 3, 2018

queryRenderedFeatures doesnt take into account feature states #6569

queryRenderedFeatures doesnt take into account feature states #6569

Conversation

asheemmamoowala commented Apr 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asheemmamoowala Apr 26, 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

ansis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

asheemmamoowala Apr 26, 2018 •

edited

Loading