Fix inflated map.getBounds due to horizonShift and edge of map #10909
Conversation
SnailBones
added
bug 🐞
skip changelog
| @@ -231,6 +231,66 @@ test('transform', (t) => { | |||
| t.end(); | |||
| }); | |||
|
|
|||
| t.test('getBounds (#10261)', (t) => { | |||
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| @@ -231,6 +231,66 @@ test('transform', (t) => { | |||
| t.end(); | |||
| }); | |||
|
|
|||
| t.test('getBounds (#10261)', (t) => { | |||
There was a problem hiding this comment.
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.
src/geo/transform.js
Outdated
| 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)); |
There was a problem hiding this comment.
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).
| 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; |
|
@SnailBones This looks good and works well from my testing, my last concern before approving would be on the default value added to the |
Co-authored-by: Karim Naaji <karim.naaji@gmail.com>
karimnaaji
removed
the
skip changelog
Launch Checklist
Closes #10261
Also fixed an undocumented issue: with 3d terrain the getBounds function ignores padding.
Updates flow version to support
Array.flatMap()<changelog>map.getBounds() correctly matches horizon and North/South edges of map and respects padding with 3D terrain enabled.</changelog>Tests: