Add support for `cameraForBounds` on globe projection #12138

karimnaaji · 2022-08-05T00:41:07Z

Fixes https://github.com/mapbox/gl-js-team/issues/479

This PR adds support for cameraForBounds on globe and updates the method used by cameraForBounds to account fully for 3d perspective. A demo of the prototype to help visualize the new method is available here.

Currently, cameraForBounds only takes into account the map bearing in 2d dimension. In order to use it on globe as well as extend the method to account for pitch appropriately, the camera frustum adjustment has to be done in using the 3d bounding box of the coordinates we are fitting.

The new method reconstructs a 3d bounding box from the perspective of the camera, as can be seen in the following video:

frustum-fit.mov

It's important to note that left and right planes aren't perfectly tight with the initial bounding box; this is an overall preferable behavior as we ensure that that the final camera placement is looking at the centroid of both the reconstructed aabb in camera space and the original bounds.

Launch Checklist

briefly describe the changes in this PR
include before/after visuals or gifs if this PR includes visual changes
write tests for all new functionality
document any changes to public APIs
manually test the debug page
tagged @mapbox/gl-native if this PR includes shader changes or needs a native port
apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog>Add support for cameraForBounds with globe view</changelog>

mpulkki-mapbox · 2022-08-09T11:55:26Z

These animations are awesome and really help to understand the idea behind the code! I have few questions as this is a really fascinating topic!

I see that you're finding the best fit for the camera frustum using an AABB in camera's coordinate space that's been built by encapsulating the AABB transformed from the ecef space. Why do you think it's necessary step to construct an intermediate AABB in ecef space first? Right now result of the algorithm seems to vary as a function of longitude and latitude as the bounding box is aligned in ecef space and not in camera space (your interactive demo is really useful for spotting things like these!)

Taking this route you could first try to transform corners of the lat&lng bounds into world space (aka pixel space) using the globe matrix and then building the AABB. This could help with the aforementioned issue as the up vector in world space is always pointing to same direction because camera actually moves on a flat plane in mercator coordinates. In other words intermediate AABBs built this way should always be aligned with the tangent plane at the centroid location (I think).

A slightly modified approach I've been thinking about is to simplify the problem into fitting points inside a frustum. It should be a fairly simple problem as camera centroid, bearing & pitch are fixed and only distance to the ground is changing. Given a set of points (corners of the lat&lng region in this case) and camera frustum planes you could find the closest intersection point between any pair of frustum planes and points and use that distance to move the camera forward.

karimnaaji · 2022-08-23T17:02:20Z

These animations are awesome and really help to understand the idea behind the code! I have few questions as this is a really fascinating topic!

Thanks for giving a look!

I see that you're finding the best fit for the camera frustum using an AABB in camera's coordinate space that's been built by encapsulating the AABB transformed from the ecef space. Why do you think it's necessary step to construct an intermediate AABB in ecef space first? Right now result of the algorithm seems to vary as a function of longitude and latitude as the bounding box is aligned in ecef space and not in camera space (your interactive demo is really useful for spotting things like these!)

Taking this route you could first try to transform corners of the lat&lng bounds into world space (aka pixel space) using the globe matrix and then building the AABB. This could help with the aforementioned issue as the up vector in world space is always pointing to same direction because camera actually moves on a flat plane in mercator coordinates. In other words intermediate AABBs built this way should always be aligned with the tangent plane at the centroid location (I think).

Yes that's a good point, it's worth investigating whether we could skip an extra space transformation in order to reduce the longitude/latitude dependency by doing it directly in world space as you suggest, I'll investigate that as it might might also result in a better fit overall.

A slightly modified approach I've been thinking about is to simplify the problem into fitting points inside a frustum. It should be a fairly simple problem as camera centroid, bearing & pitch are fixed and only distance to the ground is changing. Given a set of points (corners of the lat&lng region in this case) and camera frustum planes you could find the closest intersection point between any pair of frustum planes and points and use that distance to move the camera forward.

I investigated this, but unless I missed something, finding the closest intersection of the frustum planes aligned with a set of points may not always result in the camera being aligned or looking at the centroid, which I think is an important property of cameraForBounds placement API. Although it would result in tighter placement, keeping the centroid of the bounds being fit aligned with the center (as implemented in this PR) is a tradeoff.

- Account for aspect ratio - Adjust for aspect ratio contribution - Support for pitch - Support on mercator - Add debug page Cleanup Cleanup Update _cameraForBoxAndBearing with new technique Update _cameraForBoxAndBearing with new technique for globe Account for transition Account for option max zoom Cleanups Fix case when there is no sphere intersection

Fixup

Apply center offset

mourner

I didn't get through all the math logic in detail, but basically I think this needs a few more passes of simplification — the code is a bit too verbose / repetitive in places, and adds quite a bit of bundle size (+1.3KB gzipped!) for the feature. Pointed out some low hanging fruits but there should be more opportunities to cut down / simplify.

src/util/primitives.js

src/ui/camera.js

src/util/primitives.js

src/geo/transform.js

src/ui/camera.js

mpulkki-mapbox

Looks and works great! I have few minor questions in the comments

src/util/primitives.js

mpulkki-mapbox · 2022-09-02T07:30:14Z

src/geo/transform.js

+        let zoom = getZoom(this.zoom);
+        let diff = Math.abs(zoom - getZoom(zoom));
+        let lastdiff;
+        while (lastdiff !== diff) {


Is this guaranteed to converge in all cases? Could there be a risk of an infinite loop due to floating point comparison?

It should converge in a few iterations, but as a safeguard it may be wise to add a max iteration count or an epsilon comparison, as it's difficult to confirm that floating point comparison will not break this. This method will not be necessary in gl-native, as adaptive scaling to get constant globe size isn't applied yet.

src/ui/camera.js

Co-authored-by: Volodymyr Agafonkin <agafonkin@gmail.com>

karimnaaji · 2022-09-02T17:24:43Z

Thanks for the review and helpful simplification suggestions, I've addressed most of them and will add more tests for the corner case of requesting entries such as [-180, 0], [180, 0]. Regarding the concern of size:

and adds quite a bit of bundle size (+1.3KB gzipped!)

it will level-out with #12211 (once fitScreenCoordinates and fitBounds all go through the method introduced here, allowing to delete some code and benefit from globe support), I've split the work in two separate PRs to make it easier to review.

SnailBones

I'm seeing some strange behavior with bounds surrounding a pole. The map usually zooms much further out than I would expect. Sometimes, when starting at high zoom and pitch, zoom doesn't change at all.

pole.bounds.mov

I'm also noticing that the globe will spin around rather than crossing the antimeridian, this seems to be an existing issue with map.easeTo().

bounds.spin.mov

mourner

A few more nits but overall 👍

src/geo/projection/globe_util.js

src/ui/camera.js

Co-authored-by: Volodymyr Agafonkin <agafonkin@gmail.com>

karimnaaji · 2022-09-06T20:33:03Z

I'm seeing some strange behavior with bounds surrounding a pole. The map usually zooms much further out than I would expect. Sometimes, when starting at high zoom and pitch, zoom doesn't change at all.

That's a side from the method we adopted in #11951. Minimizing scaling towards the pole has the effect of not resolving the real zoom for a given altitude. Since we re-adjust the scaling to be constant instead of implicitly zooming in but aren't adjusting zoom at the same time, we may resolve a much higher zoom than if we were to zoom towards the pole, compared to before #11353.

The current approach towards resolving altitude -> true z in this PR is an approximation: https://github.com/mapbox/mapbox-gl-js/pull/12138/files#diff-a2b3330768267b8c7996b8bf292fcd864e8b768f538436813b0d7e19f7202176R1988, which may introduce a fairly localized issue around the poles/extreme latitudes as you noticed. I think we can improve and put more effort into deriving true zoom if that becomes more of an issue in practice.

A suggested fix towards the original issue of #11353 would be to adjust the zoom dynamically, so that such calculation are always correct, but this had other side effects since styling can depend on zoom. It's not a trivial problem and these are some of the trade-offs that we have to account for.

I'm also noticing that the globe will spin around rather than crossing the antimeridian, this seems to be an existing issue with map.easeTo().

This isn't an issue directly related to this PR, and part of easeTo.

karimnaaji force-pushed the karim/camera-for-bounds branch 4 times, most recently from bdf4e15 to 50b5c03 Compare August 25, 2022 20:51

karimnaaji marked this pull request as ready for review August 26, 2022 21:15

karimnaaji requested review from mpulkki-mapbox and im029 August 26, 2022 21:15

karimnaaji force-pushed the karim/camera-for-bounds branch 3 times, most recently from 7349c6f to 0b71553 Compare August 31, 2022 00:23

karimnaaji mentioned this pull request Aug 31, 2022

Fix aabb.fromPoints API #12207

Merged

6 tasks

karimnaaji force-pushed the karim/camera-for-bounds branch 2 times, most recently from 6eb2518 to 1dc1903 Compare August 31, 2022 23:56

karimnaaji added 12 commits September 1, 2022 10:51

Simplifications

53a88f7

Simplifications

907940c

Use tighter AABB constructed from camera orientation

c549e48

Simplifications from latest change

8f9314f

Address FIXME wrt fixed pixel space conversion

9beae6d

Fixup

Remove unused

941dd0f

Flow and lint

a550d34

Reintroduce padding options

3ed79e4

Apply center offset

Fix unit tests

f1ebb1e

Adding tests

7ee9aa6

Add more unit test

825bd2b

karimnaaji force-pushed the karim/camera-for-bounds branch 2 times, most recently from 6ee28b1 to 825bd2b Compare September 1, 2022 18:09

karimnaaji mentioned this pull request Sep 1, 2022

Add support for fitBounds and fitScreenCoordinates on globe #12211

Merged

7 tasks

karimnaaji force-pushed the karim/camera-for-bounds branch from fbb80cd to 825bd2b Compare September 1, 2022 18:36

mourner requested changes Sep 2, 2022

View reviewed changes

mpulkki-mapbox reviewed Sep 2, 2022

View reviewed changes

karimnaaji and others added 5 commits September 2, 2022 09:09

Address review comments

1b93f17

Address review comments

ca8a035

Update src/ui/camera.js

945efe8

Co-authored-by: Volodymyr Agafonkin <agafonkin@gmail.com>

Address review comments

a7b789a

Address review comments

762159c

SnailBones reviewed Sep 2, 2022

View reviewed changes

mourner approved these changes Sep 5, 2022

View reviewed changes

src/geo/projection/globe_util.js Outdated Show resolved Hide resolved

src/ui/camera.js Outdated Show resolved Hide resolved

src/ui/camera.js Show resolved Hide resolved

src/ui/camera.js Outdated Show resolved Hide resolved

karimnaaji and others added 2 commits September 6, 2022 09:10

Update src/geo/projection/globe_util.js

6e2ed3f

Co-authored-by: Volodymyr Agafonkin <agafonkin@gmail.com>

Update src/ui/camera.js

85351bd

Co-authored-by: Volodymyr Agafonkin <agafonkin@gmail.com>

karimnaaji force-pushed the karim/camera-for-bounds branch from ed8f240 to 85351bd Compare September 6, 2022 16:38

karimnaaji added 3 commits September 6, 2022 09:43

Apply review comment

5fc1a6b

Address review comment

1bc7c9b

Fix flow

9a817f8

karimnaaji merged commit ca2f99f into main Sep 6, 2022

karimnaaji deleted the karim/camera-for-bounds branch September 6, 2022 20:33

This was referenced Mar 29, 2023

v2.11 introduces a bug that causes misalignment of object and selection #12632

Closed

Revert FoV getter in Transform #12633

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for `cameraForBounds` on globe projection #12138

Add support for `cameraForBounds` on globe projection #12138

karimnaaji commented Aug 5, 2022 •

edited

Loading

mpulkki-mapbox commented Aug 9, 2022 •

edited

Loading

karimnaaji commented Aug 23, 2022

mourner left a comment

mpulkki-mapbox left a comment

mpulkki-mapbox Sep 2, 2022

karimnaaji Sep 2, 2022 •

edited

Loading

karimnaaji commented Sep 2, 2022 •

edited

Loading

SnailBones left a comment

mourner left a comment

karimnaaji commented Sep 6, 2022

Add support for cameraForBounds on globe projection #12138

Add support for cameraForBounds on globe projection #12138

Conversation

karimnaaji commented Aug 5, 2022 • edited Loading

Launch Checklist

mpulkki-mapbox commented Aug 9, 2022 • edited Loading

karimnaaji commented Aug 23, 2022

mourner left a comment

Choose a reason for hiding this comment

mpulkki-mapbox left a comment

Choose a reason for hiding this comment

mpulkki-mapbox Sep 2, 2022

Choose a reason for hiding this comment

karimnaaji Sep 2, 2022 • edited Loading

Choose a reason for hiding this comment

karimnaaji commented Sep 2, 2022 • edited Loading

SnailBones left a comment

Choose a reason for hiding this comment

mourner left a comment

Choose a reason for hiding this comment

karimnaaji commented Sep 6, 2022

Add support for `cameraForBounds` on globe projection #12138

Add support for `cameraForBounds` on globe projection #12138

karimnaaji commented Aug 5, 2022 •

edited

Loading

mpulkki-mapbox commented Aug 9, 2022 •

edited

Loading

karimnaaji Sep 2, 2022 •

edited

Loading

karimnaaji commented Sep 2, 2022 •

edited

Loading