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.

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

Fix inflated map.getBounds due to horizonShift and edge of map #10909

Merged
merged 14 commits into from
Aug 27, 2021
Merged
2 changes: 1 addition & 1 deletion .flowconfig
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,7 @@
.*/_site/.*

[version]
0.100.0
0.103.0

[options]
server.max_workers=4
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@
"eslint-plugin-html": "^6.1.1",
"eslint-plugin-import": "^2.22.1",
"eslint-plugin-jsdoc": "^32.3.4",
"flow-bin": "^0.100.0",
"flow-bin": "0.103.0",
SnailBones marked this conversation as resolved.
Show resolved Hide resolved
"gl": "^4.9.0",
"glob": "^7.1.6",
"is-builtin-module": "^3.0.0",
Expand Down
105 changes: 75 additions & 30 deletions src/geo/transform.js
Original file line number Diff line number Diff line change
Expand Up @@ -687,7 +687,7 @@ class Transform {

// Do a depth-first traversal to find visible tiles and proper levels of detail
const stack = [];
const result = [];
let result = [];
const maxZoom = z;
const overscaledZ = options.reparseOverscaled ? actualZ : z;

Expand Down Expand Up @@ -820,7 +820,8 @@ class Transform {

if (this.fogCullDistSq) {
const fogCullDistSq = this.fogCullDistSq;
result.splice(0, result.length, ...result.filter(entry => {
const horizonLineFromTop = this.horizonLineFromTop();
SnailBones marked this conversation as resolved.
Show resolved Hide resolved
result = result.filter(entry => {
const min = [0, 0, 0, 1];
const max = [EXTENT, EXTENT, 0, 1];

Expand All @@ -834,8 +835,14 @@ class Transform {
if (sqDist === 0) { return true; }

let overHorizonLine = false;
const horizonLineFromTop = this.horizonLineFromTop();
if (sqDist > fogCullDistSq && horizonLineFromTop !== 0) {

// Terrain loads at one zoom level lower than the raster data,
// so the following checks whether the terrain sits above the horizon and ensures that
// when mountains stick out above the fog (due to horizon-blend),
// we haven’t accidentally culled some of the raster tiles we need to draw on them.
// If we don’t do this, the terrain is default black color and may flash in and out as we move toward it.

if (this.elevation && sqDist > fogCullDistSq && horizonLineFromTop !== 0) {
SnailBones marked this conversation as resolved.
Show resolved Hide resolved
const projMatrix = this.calculateProjMatrix(entry.tileID.toUnwrapped());

let minmax;
Expand All @@ -858,13 +865,13 @@ class Transform {
// NDC to Screen
const screenCoordY = (1 - worldFar[1]) * this.height * 0.5;

// Prevent cutting tiles crossing over the horizon lines to
// Prevent cutting tiles crossing over the horizon line to
// prevent pop-in and out within the fog culling range
overHorizonLine = screenCoordY < horizonLineFromTop;
}

return sqDist < fogCullDistSq || overHorizonLine;
}));
});
}

const cover = result.sort((a, b) => a.distanceSq - b.distanceSq).map(a => a.tileID);
Expand Down Expand Up @@ -1071,11 +1078,10 @@ class Transform {
* @param {Point} p top left origin screen point, in pixels.
* @private
*/
pointCoordinate(p: Point): MercatorCoordinate {
pointCoordinate(p: Point, z?: number = this._centerAltitude): MercatorCoordinate {
SnailBones marked this conversation as resolved.
Show resolved Hide resolved
const horizonOffset = this.horizonLineFromTop(false);
const clamped = new Point(p.x, Math.max(horizonOffset, p.y));

return this.rayIntersectionCoordinate(this.pointRayIntersection(clamped));
return this.rayIntersectionCoordinate(this.pointRayIntersection(clamped, z));
SnailBones marked this conversation as resolved.
Show resolved Hide resolved
}

/**
Expand Down Expand Up @@ -1152,11 +1158,26 @@ class Transform {
*/
getBounds(): LngLatBounds {
if (this._terrainEnabled()) return this._getBounds3D();

let tl = this.pointCoordinate(new Point(this._edgeInsets.left, this._edgeInsets.top));
let tr = this.pointCoordinate(new Point(this.width - this._edgeInsets.right, this._edgeInsets.top));
const br = this.pointCoordinate(new Point(this.width - this._edgeInsets.right, this.height - this._edgeInsets.bottom));
const bl = this.pointCoordinate(new Point(this._edgeInsets.left, this.height - this._edgeInsets.bottom));

// Snap points if off the edge of the map.
const slope = (p1, p2) => (p2.y - p1.y) / (p2.x - p1.x);

if (tl.y > 1 && tr.y >= 0) tl = new MercatorCoordinate((1 - bl.y) / slope(bl, tl) + bl.x, 1);
else if (tl.y < 0 && tr.y <= 1) tl = new MercatorCoordinate(-bl.y / slope(bl, tl) + bl.x, 0);

if (tr.y > 1 && tl.y >= 0) tr = new MercatorCoordinate((1 - br.y) / slope(br, tr) + br.x, 1);
else if (tr.y < 0 && tl.y <= 1) tr = new MercatorCoordinate(-br.y / slope(br, tr) + br.x, 0);

return new LngLatBounds()
.extend(this.pointLocation(new Point(this._edgeInsets.left, this._edgeInsets.top)))
.extend(this.pointLocation(new Point(this.width - this._edgeInsets.right, this._edgeInsets.top)))
.extend(this.pointLocation(new Point(this.width - this._edgeInsets.right, this.height - this._edgeInsets.bottom)))
.extend(this.pointLocation(new Point(this._edgeInsets.left, this.height - this._edgeInsets.bottom)));
.extend(this.coordinateLocation(tl))
.extend(this.coordinateLocation(tr))
.extend(this.coordinateLocation(bl))
.extend(this.coordinateLocation(br));
}

_getBounds3D(): LngLatBounds {
Expand All @@ -1172,29 +1193,46 @@ class Transform {
}, {min: Number.MAX_VALUE, max: 0});
minmax.min *= elevation.exaggeration();
minmax.max *= elevation.exaggeration();
const top = this.horizonLineFromTop();
return [
new Point(0, top),
new Point(this.width, top),
new Point(this.width, this.height),
new Point(0, this.height)
].reduce((acc, p) => {
return acc
.extend(this.coordinateLocation(this.rayIntersectionCoordinate(this.pointRayIntersection(p, minmax.min))))
.extend(this.coordinateLocation(this.rayIntersectionCoordinate(this.pointRayIntersection(p, minmax.max))));
}, new LngLatBounds());
// const top = this.horizonLineFromTop();
SnailBones marked this conversation as resolved.
Show resolved Hide resolved

const topLeft = new Point(this._edgeInsets.left, this._edgeInsets.top);
const topRight = new Point(this.width - this._edgeInsets.right, this._edgeInsets.top);
const bottomRight = new Point(this.width - this._edgeInsets.right, this.height - this._edgeInsets.bottom);
const bottomLeft = new Point(this._edgeInsets.left, this.height - this._edgeInsets.bottom);

// Consider far points at the maximum possible elevation
// and near points at the minimum to ensure full coverage.
let tl = this.pointCoordinate(topLeft, minmax.min);
let tr = this.pointCoordinate(topRight, minmax.min);
const br = this.pointCoordinate(bottomRight, minmax.max);
const bl = this.pointCoordinate(bottomLeft, minmax.max);

// Snap points if off the edge of the map.
const slope = (p1, p2) => (p2.y - p1.y) / (p2.x - p1.x);

if (tl.y > 1 && tr.y >= 0) tl = new MercatorCoordinate((1 - bl.y) / slope(bl, tl) + bl.x, 1);
else if (tl.y < 0 && tr.y <= 1) tl = new MercatorCoordinate(-bl.y / slope(bl, tl) + bl.x, 0);

if (tr.y > 1 && tl.y >= 0) tr = new MercatorCoordinate((1 - br.y) / slope(br, tr) + br.x, 1);
else if (tr.y < 0 && tl.y <= 1) tr = new MercatorCoordinate(-br.y / slope(br, tr) + br.x, 0);

return new LngLatBounds()
.extend(this.coordinateLocation(tl))
.extend(this.coordinateLocation(tr))
.extend(this.coordinateLocation(bl))
.extend(this.coordinateLocation(br));
Copy link
Contributor

Choose a reason for hiding this comment

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

Any chance part of this code could be shared between _getBounds3D and getBounds? There seem to be some potential for compressing that a bit (The only difference I noticed is on the z value of pointCoordinate).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea!


}

/**
* Returns position of horizon line from the top of the map in pixels. If horizon is not visible, returns 0.
* Returns position of horizon line from the top of the map in pixels.
* If horizon is not visible, returns 0 by default or a negative value if called with clampToTop = false.
* @private
*/
horizonLineFromTop(clampToTop: boolean = true): number {
// h is height of space above map center to horizon.
const h = this.height / 2 / Math.tan(this._fov / 2) / Math.tan(Math.max(this._pitch, 0.1)) + this.centerOffset.y;
// incorporate 3% of the area above center to account for reduced precision.
const horizonEpsilon = 0.03;
SnailBones marked this conversation as resolved.
Show resolved Hide resolved
const offset = this.height / 2 - h * (1 - horizonEpsilon);
const offset = this.height / 2 - h * (1 - this._horizonShift);
return clampToTop ? Math.max(0, offset) : offset;
}

Expand Down Expand Up @@ -1717,12 +1755,16 @@ class Transform {
return !!this._elevation;
}

isHorizonVisibleForPoints(p0: Point, p1: Point): boolean {
// Check if any of the four corners are off the edge of the rendered map
isCornerOffEdge(p0: Point, p1: Point): boolean {
const minX = Math.min(p0.x, p1.x);
const maxX = Math.max(p0.x, p1.x);
const minY = Math.min(p0.y, p1.y);
const maxY = Math.max(p0.y, p1.y);

const horizon = this.horizonLineFromTop(false);
if (minY < horizon) return true;
Copy link
Contributor

Choose a reason for hiding this comment

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

👍 Nice early exit.


const min = new Point(minX, minY);
const max = new Point(maxX, maxY);

Expand All @@ -1739,9 +1781,11 @@ class Transform {

for (const corner of corners) {
const rayIntersection = this.pointRayIntersection(corner);
// Point is above the horizon
if (rayIntersection.t < 0) {
return true;
}
// Point is off the bondaries of the map
const coordinate = this.rayIntersectionCoordinate(rayIntersection);
if (coordinate.x < minWX || coordinate.y < minWY ||
coordinate.x > maxWX || coordinate.y > maxWY) {
Expand All @@ -1753,6 +1797,7 @@ class Transform {
}

// Checks the four corners of the frustum to see if they lie in the map's quad.
//
isHorizonVisible(): boolean {
// we consider the horizon as visible if the angle between
// a the top plane of the frustum and the map plane is smaller than this threshold.
Expand All @@ -1761,7 +1806,7 @@ class Transform {
return true;
}

return this.isHorizonVisibleForPoints(new Point(0, 0), new Point(this.width, this.height));
return this.isCornerOffEdge(new Point(0, 0), new Point(this.width, this.height));
}

/**
Expand Down
2 changes: 1 addition & 1 deletion src/ui/camera.js
Original file line number Diff line number Diff line change
Expand Up @@ -848,7 +848,7 @@ class Camera extends Evented {
const raycast = this._raycastElevationBox(point0, point1);

if (!raycast) {
if (this.transform.isHorizonVisibleForPoints(point0, point1)) {
if (this.transform.isCornerOffEdge(point0, point1)) {
return this;
}

Expand Down
72 changes: 66 additions & 6 deletions test/unit/geo/transform.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -337,6 +337,66 @@ test('transform', (t) => {
t.end();
});

t.test('getBounds (#10261)', (t) => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It would be nice to have a test for _getBounds3D with transform isolated, but it requires elevation.visibleDemTiles. Perhaps we could create a mock for that?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that should work by mocking it in such a way that you get a valid minmax value, as it's the only dependency on elevation in this method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm leaning toward pushing these tests to the future given the tests with map seem to cover the function pretty well, and this seems less urgent than the other tasks on my plate. But if you think this is important I'd be happy to look into it.

const transform = new Transform();
transform.resize(500, 500);
transform.zoom = 2;
transform.pitch = 80;

t.test('Looking at North Pole', (t) => {
transform.center = {lng: 0, lat: 90};
t.deepEqual(transform.center, {lng: 0, lat: 79.3677012485858});
const bounds = transform.getBounds();

// Bounds stops at the edge of the map
t.same(bounds.getNorth().toFixed(6), transform.maxValidLatitude);
// Top corners of bounds line up with side of view
t.same(transform.locationPoint(bounds.getNorthWest()).x.toFixed(10), 0);
t.same(transform.locationPoint(bounds.getNorthEast()).x.toFixed(10), transform.width);
// Bottom of bounds lines up with bottom of view
t.same(transform.locationPoint(bounds.getSouthEast()).y.toFixed(10), transform.height);
t.same(transform.locationPoint(bounds.getSouthWest()).y.toFixed(10), transform.height);

t.same(toFixed(bounds.toArray()), toFixed([[ -56.6312307639145, 62.350646608460806 ], [ 56.63123076391412, 85.0511287798 ]]));

t.end();
});
t.test('Looking at South Pole', (t) => {
transform.bearing = 180;
transform.center = {lng: 0, lat: -90};

t.deepEqual(transform.center, {lng: 0, lat: -79.3677012485858});
const bounds = transform.getBounds();

// Bounds stops at the edge of the map
t.same(bounds.getSouth().toFixed(6), -transform.maxValidLatitude);
// Top corners of bounds line up with side of view
t.same(transform.locationPoint(bounds.getSouthEast()).x.toFixed(10), 0);
t.same(transform.locationPoint(bounds.getSouthWest()).x.toFixed(10), transform.width);
// Bottom of bounds lines up with bottom of view
t.same(transform.locationPoint(bounds.getNorthEast()).y.toFixed(10), transform.height);
t.same(transform.locationPoint(bounds.getNorthWest()).y.toFixed(10), transform.height);

t.same(toFixed(bounds.toArray()), toFixed([[ -56.6312307639145, -85.0511287798], [ 56.63123076391412, -62.350646608460806]]));

t.end();
});
t.end();

function toFixed(bounds) {
const n = 10;
return [
[normalizeFixed(bounds[0][0], n), normalizeFixed(bounds[0][1], n)],
[normalizeFixed(bounds[1][0], n), normalizeFixed(bounds[1][1], n)]
];
}

function normalizeFixed(num, n) {
// workaround for "-0.0000000000" ≠ "0.0000000000"
return parseFloat(num.toFixed(n)).toFixed(n);
}
});

test('coveringTiles', (t) => {
const options = {
minzoom: 1,
Expand Down Expand Up @@ -1046,7 +1106,7 @@ test('transform', (t) => {

t.test('isHorizonVisible', (t) => {

t.test('isHorizonVisibleForPoints', (t) => {
t.test('isCornerOffEdge', (t) => {
const transform = new Transform();
transform.maxPitch = 85;
transform.resize(800, 800);
Expand All @@ -1058,21 +1118,21 @@ test('transform', (t) => {
t.true(transform.isHorizonVisible());

p0 = new Point(0, 0); p1 = new Point(10, 10);
t.true(transform.isHorizonVisibleForPoints(p0, p1));
t.true(transform.isCornerOffEdge(p0, p1));

p0 = new Point(0, 250); p1 = new Point(10, 350);
t.true(transform.isHorizonVisibleForPoints(p0, p1));
t.true(transform.isCornerOffEdge(p0, p1));

p0 = new Point(0, transform.horizonLineFromTop() - 10);
p1 = new Point(10, transform.horizonLineFromTop() + 10);
t.true(transform.isHorizonVisibleForPoints(p0, p1));
t.true(transform.isCornerOffEdge(p0, p1));

p0 = new Point(0, 700); p1 = new Point(10, 710);
t.false(transform.isHorizonVisibleForPoints(p0, p1));
t.false(transform.isCornerOffEdge(p0, p1));

p0 = new Point(0, transform.horizonLineFromTop());
p1 = new Point(10, transform.horizonLineFromTop() + 10);
t.false(transform.isHorizonVisibleForPoints(p0, p1));
t.false(transform.isCornerOffEdge(p0, p1));

t.end();
});
Expand Down