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.

Sign up for GitHub

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

Jump to bottom

fix fill extrusion querying for colinear points #9454

Merged
merged 1 commit into from
Mar 24, 2020
Merged

fix fill extrusion querying for colinear points #9454

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
46 changes: 26 additions & 20 deletions src/style/style_layer/fill_extrusion_style_layer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,34 +81,40 @@ export function getIntersectionDistance(projectedQueryGeometry: Array<Point>, pr
// Check whether points are coincident and use other points if they are.
let i = 0;
const a = projectedFace[i++];
let b, c;
let b;
while (!b || a.equals(b)) {
b = projectedFace[i++];
if (!b) return Infinity;
}
while (!c || a.equals(c) || b.equals(c)) {
c = projectedFace[i++];
if (!c) return Infinity;
}

const p = projectedQueryGeometry[0];
// Loop until point `c` is not colinear with points `a` and `b`.
for (; i < projectedFace.length; i++) {
const c = projectedFace[i];

const p = projectedQueryGeometry[0];

const ab = b.sub(a);
const ac = c.sub(a);
const ap = p.sub(a);

const ab = b.sub(a);
const ac = c.sub(a);
const ap = p.sub(a);
const dotABAB = dot(ab, ab);
const dotABAC = dot(ab, ac);
const dotACAC = dot(ac, ac);
const dotAPAB = dot(ap, ab);
const dotAPAC = dot(ap, ac);
const denom = dotABAB * dotACAC - dotABAC * dotABAC;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can this be replaced by ab.cross(ac)? you could also use that to check for co-linerarity, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The objects are the Point class which doesn't have .cross() (I think that is in one of the gl-matrix classes). If it was already there this would be probably cleaner but I'm not sure it's worth adding at this point

you could also use that to check for co-linerarity, right?

At first I checked for collinearity directly but because of precision stuff we'd need to pick a threshold for what we count as colinear. At first I hit a case where my colinear check said they were not colinear enough but the whole method still failed to produce a valid distance. I think checking the output should be more reliable because it avoids that gap

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough, makes sense to me!


const dotABAB = dot(ab, ab);
const dotABAC = dot(ab, ac);
const dotACAC = dot(ac, ac);
const dotAPAB = dot(ap, ab);
const dotAPAC = dot(ap, ac);
const denom = dotABAB * dotACAC - dotABAC * dotABAC;
const v = (dotACAC * dotAPAB - dotABAC * dotAPAC) / denom;
const w = (dotABAB * dotAPAC - dotABAC * dotAPAB) / denom;
const u = 1 - v - w;
const v = (dotACAC * dotAPAB - dotABAC * dotAPAC) / denom;
const w = (dotABAB * dotAPAC - dotABAC * dotAPAB) / denom;
const u = 1 - v - w;

// Use the barycentric weighting along with the original triangle z coordinates to get the point of intersection.
const distance = a.z * u + b.z * v + c.z * w;

if (isFinite(distance)) return distance;
}

// Use the barycentric weighting along with the original triangle z coordinates to get the point of intersection.
return a.z * u + b.z * v + c.z * w;
return Infinity;

} else {
// The counts as closest is less clear when the query is a box. This
Expand Down
37 changes: 37 additions & 0 deletions test/integration/query-tests/regressions/mapbox-gl-js#8999-2/expected.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
[
{
"geometry": {
"type": "Polygon",
"coordinates": [
[
[
-19.9951171875,
-40.01078714046552
],
[
-19.9951171875,
40.01078714046551
],
[
-19.9951171875,
59.99898612060443
],
[
19.9951171875,
20.014645445341372
],
[
-19.9951171875,
-40.01078714046552
]
]
]
},
"type": "Feature",
"properties": {
"layer": "extrusion_colinear_points"
},
"source": "extrusion_colinear_points",
"state": {}
}
]
70 changes: 70 additions & 0 deletions test/integration/query-tests/regressions/mapbox-gl-js#8999-2/style.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,70 @@
{
"version": 8,
"metadata": {
"test": {
"debug": true,
"width": 200,
"height": 200,
"queryGeometry": [
100,
40
]
}
},
"pitch": 0,
"center": [
0,
0
],
"zoom": 0,
"sources": {
"extrusion_colinear_points": {
"type": "geojson",
"tolerance": 0,
"data": {
"type": "Feature",
"properties": {
"layer": "extrusion_colinear_points"
},
"geometry": {
"type": "Polygon",
"coordinates": [
[
[
-20,
-40
],
[
-20,
40
],
[
-20,
60
],
[
20,
20
],
[
-20,
-40
]
]
]
}
}
}
},
"layers": [
{
"id": "extrusion_colinear_points",
"type": "fill-extrusion",
"source": "extrusion_colinear_points",
"paint": {
"fill-extrusion-color": "#ecc",
"fill-extrusion-height": 0
}
}
]
}