fix fill-extrusion querying #7499

ansis · 2018-10-29T16:30:11Z

This adds building querying by projecting the geometries and querying the projected geometries. Queries are sped up with support for non-rectangular grid index queries.

Results are sorted by distance by depth so that the first result is always the closest no matter the rotation or layer order. In the debug example /debug/extrusion-query.html red is the closest, green is the second closest and blue is the furthest.

New query tests cover querying and sorting.

Launch Checklist

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

ChrisLoer

I only got as far today as figuring out where the changes were and playing with the demo. Haven't started wrapping my head around the math yet, but it looks like everything would break if it weren't right so it must be fine. 😉

So far it looks awesome!

src/data/feature_index.js

test/integration/query-tests/fill-extrusion/base-in/style.json

ChrisLoer

OK, I think I finally understand how this works! Most of my comments are naming/comment related.

src/source/source_cache.js

src/geo/transform.js

ChrisLoer · 2018-11-30T23:36:27Z

src/data/feature_index.js

+        const matching3D = this.grid3D.query(
+                cameraMinX - queryPadding, cameraMinY - queryPadding, cameraMaxX + queryPadding, cameraMaxY + queryPadding,
+                (bx1, by1, bx2, by2) => {
+                    return polygonIntersectsBox(args.cameraQueryGeometry[0], bx1 - queryPadding, by1 - queryPadding, bx2 + queryPadding, by2 + queryPadding);


This args.cameraQueryGeometry[0] looks like it's not accounting for query geometries with multiple rings, but we don't support that in the first place, do we? Can we just change this code to work with a plain Array<Point>?

This switches both queryGeometry and cameraQueryGeometry to 780ad42

ChrisLoer · 2018-11-30T23:55:53Z

src/geo/transform.js

@@ -592,6 +592,38 @@ class Transform {
        const topPoint = vec4.transformMat4(p, p, this.pixelMatrix);
        return topPoint[3] / this.cameraToCenterDistance;
    }
+
+    getCameraPoint() {


I struggled figuring out just from the name and the math, what "camera point" meant. I think we should comment (and maybe rename?). Here's the intuition I worked out for how to think of it:

// For a given query point, this represents an upper bound on how far // away the base of something rendered at that point could be. // With pitch=0, this is essentially dividing the screen into quadrants // around the center point of the screen, since perspective just pushes // the top of extrusions "away" from the center. // As pitch increases, the potential base for a building shifts "down" // towards the bottom of the screen, and potentially below the bottom of // the screen

Although I think that's probably not the most accurate description. Am I right that this calculation is basically just super conservative -- always include half the screen horizontally, and for the y-coordinate shift use Math.tan(pitch) * this.cameraToCenterDistance as a rough approximation of how far down the "tallest building we could render" would go?

Done in 0b38e35

src/geo/transform.js

ChrisLoer · 2018-11-30T23:58:51Z

src/geo/transform.js

+        return this.centerPoint.add(new Point(0, latOffset));
+    }
+
+    getCameraQueryGeometry(queryGeometry: Array<Point>): Array<Point> {


Notes I made for myself trying to figure out how this worked:

// Given a query geometry in screen coordinates, return a geometry that includes // all of the original query as well as all possible areas of the screen where the // *base* of a visible extrusion could be. // - For point queries, the line from the query point to the "camera point" // - For other geometries, the envelope of the query geometry and the "camera point"

I think this should be commented to explain what it means... I'm still kind of working that out.

improved in 92d59b7, thanks for the wording!

src/source/query_features.js

ChrisLoer · 2018-12-01T01:12:33Z

src/style/style_layer/fill_extrusion_style_layer.js

+        const ac = c.sub(a);
+        const ap = p.sub(a);
+
+        const dotABAB = dot(ab, ab);


This looked like magic to me at first, but I guess it's not actually that complicated:

Find the barycentric coordinates of the projected point within the first triangle of the face, using only the xy plane. It doesn't matter if the point is outside the first triangle because all the triangles in the face are in the same plane.

Use the barycentric weighting along with the original triangle z coordinates to get the point of intersection.

The "barycentric" comment above was enough to point me in the right direction, but some elaboration along these lines would make it easier to read.

Documented in 5662072

Perfect！Is there a IOS & Android version?

@BrightBeacon this will be ported to mapbox-gl-native but there is no ETA

ansis · 2019-01-08T20:30:58Z

@ChrisLoer thanks for the thorough review! Can you take a look at my changes and do a second review?

ChrisLoer

Looks good to me!

Although do we want to publish an updated version of GridIndex before we merge this?

ansis force-pushed the fill-extrusion-query branch from 7fc0bf8 to 9686bf4 Compare November 28, 2018 17:58

ansis requested a review from ChrisLoer November 28, 2018 23:20

ChrisLoer reviewed Nov 29, 2018

View reviewed changes

src/data/feature_index.js Outdated Show resolved Hide resolved

src/data/feature_index.js Show resolved Hide resolved

test/integration/query-tests/fill-extrusion/base-in/style.json Show resolved Hide resolved

ChrisLoer reviewed Dec 1, 2018

View reviewed changes

ChrisLoer approved these changes Jan 8, 2019

View reviewed changes

ansis added 18 commits January 8, 2019 20:38

fix querying fill-extrusion layers

ec22b79

fix tests

8c18d39

fix query tests

2255e87

fix tests

64983c3

fixup

ed7e2c4

factor out code

9bee8dc

only use expanded query if a 3d layer is included

aa722e7

use sourceCache's transform

235b609

switch query geometry from multipolygon to polygon

162baa8

add explanations

296ee0f

fiuxp above

422d9e5

improve description using chris' words

94e709f

fix pixelPosMatrix naming

40da47b

add chris' comments to help clarify

85d5646

add fill extrusion box query test

dad2c62

fixup

4601282

sort 3d features as well

08148de

update grid-index dep

a6f34f2

ansis force-pushed the fill-extrusion-query branch from 36db53a to a6f34f2 Compare January 9, 2019 01:38

ansis merged commit 2119eaa into master Jan 9, 2019

ansis deleted the fill-extrusion-query branch January 9, 2019 01:46

ryanhamley mentioned this pull request Jan 14, 2019

tooltip hover extrusions #7772

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix fill-extrusion querying #7499

fix fill-extrusion querying #7499

ansis commented Oct 29, 2018 •

edited

Loading

ChrisLoer left a comment

ChrisLoer left a comment

ChrisLoer Nov 30, 2018

ansis Jan 8, 2019

ChrisLoer Nov 30, 2018

ansis Jan 8, 2019

ChrisLoer Nov 30, 2018

ansis Jan 8, 2019

ChrisLoer Dec 1, 2018

ansis Jan 8, 2019

BrightBeacon Jan 9, 2019

ansis Jan 9, 2019

ansis commented Jan 8, 2019

ChrisLoer left a comment

fix fill-extrusion querying #7499

fix fill-extrusion querying #7499

Conversation

ansis commented Oct 29, 2018 • edited Loading

Launch Checklist

ChrisLoer left a comment

Choose a reason for hiding this comment

ChrisLoer 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

Choose a reason for hiding this comment

ansis commented Jan 8, 2019

ChrisLoer left a comment

Choose a reason for hiding this comment

ansis commented Oct 29, 2018 •

edited

Loading