fix querying features that extend across tile boundaries by ansis · Pull Request #6036 · mapbox/mapbox-gl-js

ansis · 2018-01-22T22:53:29Z

Some rendered features (circles, symbols, lines) can extend outside of the boundaries of the tile they are stored in. When querying we have to check all tiles within additionalRadius of the query.

Benchmarks

https://bl.ocks.org/anonymous/raw/9762d28049090d90438b81a109918626/
https://bl.ocks.org/anonymous/raw/d69b9f2d20bb2720849c029571929611/

QueryPoint is measurably slower. In order to fix the bug we have to do more work so it makes sense and isn't huge change.

Alternative

I tried a different approach (delaying the short-circuiting until later) that is slightly nicer but a bit slower: 06d2dd3

Launch Checklist

briefly describe the changes in this PR
write tests for all new functionality
[N/A] document any changes to public APIs
post benchmark scores
[N/A] manually test the debug page

Next steps

porting to -native after review

cc @lilykaiser

ChrisLoer

Overall approach looks good to me.

I'm not sure if the "undefined additionalRadius" case will actually happen, but if it does maybe just initializing additionalRadius to 0 in the Tile constructor would take care of that.

Am I right that this still doesn't fix the tile boundary issue for symbols, since SymbolStyleLayer#queryRadius always returns 0? It wouldn't be too hard to collect a max tile unit query radius for symbols at layout time, but I don't know how to deal with pitch-scaling. Maybe adjust the query radius based on the largest possible scaling factor (i.e. the pitch scaling applied along the bottom of the tile)?

For that matter, doesn't this also break in pitched maps for circles with circle-pitch-scale: map? circle-pitch-alignment should be fine because we'll err on the conservative side.

ChrisLoer · 2018-01-23T01:32:32Z

            const tile = this._tiles[ids[i]];
            const tileID = tile.tileID;
+            const scale = Math.pow(2, this.transform.zoom - tile.tileID.overscaledZ);
+            const additionalRadius = tile.additionalRadius * EXTENT / tile.tileSize / scale;


Can't getIds() return tiles that haven't loaded yet, in which case tile.additionalRadius will be undefined, and this expression will result in NaN?

Yeah - I think it makes sense to initialize Tile#additionalRadius to 0 in the Tile constructor. (And if we weren't going to do that, we'd probably want to type it ?number -- this is a common issue in our flow typing)

ChrisLoer · 2018-01-23T01:35:13Z

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


Since we're storing the additionalRadius value here, can't we re-use the value in Tile#queryRenderedFeature instead of re-calculating? It's probably not that expensive to calculate, but it still seems like a good code simplification.

anandthakker · 2018-01-26T18:32:14Z

    refreshedUponExpiration: boolean;
    reloadCallback: any;
    resourceTiming: ?Array<PerformanceResourceTiming>;
+    additionalRadius: number;


Maybe rename to something like renderedFeatureBuffer

"buffer" is like "layer" or "source" -- it means too many things in our code base! Maybe "padding" or "query padding"?

Ugh you're so right

anandthakker · 2018-01-26T18:37:05Z

            const tile = this._tiles[ids[i]];
            const tileID = tile.tileID;
+            const scale = Math.pow(2, this.transform.zoom - tile.tileID.overscaledZ);
+            const additionalRadius = tile.additionalRadius * EXTENT / tile.tileSize / scale;


Yeah - I think it makes sense to initialize Tile#additionalRadius to 0 in the Tile constructor. (And if we weren't going to do that, we'd probably want to type it ?number -- this is a common issue in our flow typing)

fix #5756 Some rendered features (circles, symbols, lines) can extend outside of the boundaries of the tile they are stored in. When querying we have to check all tiles within `additionalRadius` of the query.

`"circle-pitch-scaling": "viewport"` adjsuts circle-radius based on the distance from the center of the viewport. This incorporates this scaling into the calculations used by feature querying.

ansis · 2018-03-06T23:08:56Z

I made the suggested changes in ea8a2ee and dbe3c23 and then expanded the scope of the pr by implementing support for circle-pitch-alignment and circle-pitch-scale for querying (
3346dc6, dbe3c23). Would it be better to split off the last two commits into a separate pr?

ChrisLoer · 2018-03-06T23:57:41Z

        this._alignedPosMatrixCache = {};
    }
+
+    getPitchScaleFactor() {


Maybe we could change the name to something like maxPitchScaleFactor along with a comment something along the lines of "returns the pitch scaling factor for the very top of the viewport"?

ChrisLoer · 2018-03-07T21:52:06Z

 type QueryParameters = {
    scale: number,
    bearing: number,
+    cameraToCenterDistance: number,


Why do we split cameraToCenterDistance out as its own parameter in all these calls? It seems like if we're passing in the transform, we can just get it from the transform when we need it?

ChrisLoer · 2018-03-07T22:24:50Z

-        const radius = this.paint.get('circle-radius').evaluate(feature) * pixelsToTileUnits;
-        const stroke = this.paint.get('circle-stroke-width').evaluate(feature) * pixelsToTileUnits;
-        return multiPolygonIntersectsBufferedMultiPoint(translatedPolygon, geometry, radius + stroke);
+        const radius = this.paint.get('circle-radius').evaluate(feature);


This all looks right to me but it took me a while to reason through it. Would this sort of explanatory comment help?

// For pitch-alignment: map, compare feature geometry to query geometry in the plane of the tile // Otherwise, compare geometry in the plane of the viewport // A circle with fixed scaling relative to the viewport gets larger in tile space as it moves into the distance // A circle with fixed scaling relative to the map gets smaller in viewport space as it moves into the distance

ChrisLoer

Yeah this looks good -- just really tricky to reason about!

I have just two requests for name change/added comments and it looks like it might be possible to get rid of passing the cameraToCenterDistance param, but overall it looks ready to me.

I filed #6298 for the remaining work to do on SymbolStyleLayer#queryRadius

`"circle-alignment-scaling": "map"` draws circles on the map plane instead of the viewport plane. This fixes querying for those circles.

ansis · 2018-03-12T16:08:37Z

comment added
getPitchFactor renamed to maxPitchScaleFactor
redundant cameraToCenterDistance parameter removed

ChrisLoer · 2018-03-12T17:37:32Z

🎉

ansis requested review from ChrisLoer and anandthakker January 22, 2018 22:53

ChrisLoer suggested changes Jan 23, 2018

View reviewed changes

anandthakker reviewed Jan 26, 2018

View reviewed changes

jfirebaugh added this to the 0.45.0 milestone Feb 28, 2018

ansis force-pushed the fix-circle-query-tile-boundary branch from 6383b0d to aab251b Compare March 6, 2018 21:04

ansis added 3 commits March 6, 2018 18:05

fix querying features that extend across tile boundaries

ea8a2ee

fix #5756 Some rendered features (circles, symbols, lines) can extend outside of the boundaries of the tile they are stored in. When querying we have to check all tiles within `additionalRadius` of the query.

rename additionalRadius to queryPadding

b4d0f13

fix querying circles with -pitch-scaling=viewport

3346dc6

`"circle-pitch-scaling": "viewport"` adjsuts circle-radius based on the distance from the center of the viewport. This incorporates this scaling into the calculations used by feature querying.

ansis force-pushed the fix-circle-query-tile-boundary branch from aab251b to dbe3c23 Compare March 6, 2018 23:06

ChrisLoer reviewed Mar 6, 2018

View reviewed changes

ChrisLoer reviewed Mar 7, 2018

View reviewed changes

ChrisLoer mentioned this pull request Mar 7, 2018

Implement SymbolStyleLayer#queryRadius() (queryRenderedFeatures fix) #6298

Closed

ChrisLoer approved these changes Mar 7, 2018

View reviewed changes

fix querying circles with -pitch-alignment=map

f3f80f3

`"circle-alignment-scaling": "map"` draws circles on the map plane instead of the viewport plane. This fixes querying for those circles.

ansis force-pushed the fix-circle-query-tile-boundary branch from dbe3c23 to f3f80f3 Compare March 12, 2018 15:59

ansis merged commit b98a9f7 into master Mar 12, 2018

ansis deleted the fix-circle-query-tile-boundary branch March 12, 2018 16:08

jfirebaugh mentioned this pull request Mar 14, 2018

queryRenderedFeatures failing for circle layers on a tile boundary mapbox/mapbox-gl-native#10615

Closed

This was referenced Mar 30, 2018

only index features strictly within tile boundaries #6429

Merged

port circle querying fixes mapbox/mapbox-gl-native#11571

Merged

hyperknot mentioned this pull request May 6, 2018

queryRenderedFeatures doesn't detect circles correctly #6622

Closed

Conversation

ansis commented Jan 22, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Benchmarks

Alternative

Launch Checklist

Next steps

Uh oh!

ChrisLoer left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ansis commented Mar 6, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChrisLoer left a comment

Choose a reason for hiding this comment

Uh oh!

ansis commented Mar 12, 2018

Uh oh!

ChrisLoer commented Mar 12, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ansis commented Jan 22, 2018 •

edited

Loading