Pitch > 90 degrees #4851

NathanMOlson · 2024-10-17T17:47:41Z

Allow pitch angle > 90 degrees. #4717

Changes:

Add centerClampedToGround configuration variable. The default is true, which maintains previous behavior. To use pitch angles higher than 90 degrees, this must be set to true, and the elevation must be provided. The elevation must be floating in space above the ground to keep the camera from going below ground.
Add elevation to JumpToOptions. This is needed to set camera pitch > 90 degrees, as the center point must be above the camera, which must be above the terrain.
Extend maxPitch to 180.
Change recalculateZoom() to keep the camera position constant. This means that it now adjusts the center point, in addition to zoom and elevation. This is important to keep the camera from jumping when terrain is updated behind the scenes. This is also the biggest change and the one with the most potential for unwanted ripples.

demo: https://nathanmolson.github.io/camera-centric

TODO

Figure out whether elevation needs to be added to style spec. Design Proposal: Add Elevation maplibre-style-spec#851
Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
Briefly describe the changes in this PR.
Link to related issues.
Include before/after visuals or gifs if this PR includes visual changes.
Write tests for all new functionality.
Document any changes to public APIs.
Add an entry to CHANGELOG.md under the ## main section.

Additional details about the center point elevation:
The Transform variables center, elevation, zoom, pitch, bearing, and fov control the location of the camera indirectly.

elevation sets the height of the "center point" above sea level. In the typical use case (centerClampedToGround = true), the library modifies elevation in an attempt to keep the center point always on the terrain (or 0 MSL if no terrain is enabled).

zoom sets the distance from the center point to the camera (in conjunction with fovInRadians, which is currently hardcoded).

Together, zoom, elevation, and pitch set the altitude of the camera.

To allow pitch > 90, the "center point" must be placed off of the ground. This will allow the camera to stay above the ground when it pitches above 90. This requires setting centerClampedToGround = false

The same math applies whether the center point is on terrain or not, and whether the camera is above or below the ground:

In most cases, having the camera underground is undesirable.

To help users position the camera, a new function calculateCameraOptionsFromCameraLngLatAltRotation() has been added to Camera.

codecov-commenter · 2024-10-17T17:50:35Z

Codecov Report

Attention: Patch coverage is 92.45283% with 12 lines in your changes missing coverage. Please review.

Project coverage is 87.95%. Comparing base (0ffa890) to head (a5cf6fa).

Files with missing lines	Patch %	Lines
src/geo/projection/globe_transform.ts	0.00%	5 Missing ⚠️
src/ui/camera.ts	86.48%	3 Missing and 2 partials ⚠️
src/geo/projection/mercator_transform.ts	98.64%	0 Missing and 1 partial ⚠️
src/ui/handler_manager.ts	83.33%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4851      +/-   ##
==========================================
+ Coverage   87.77%   87.95%   +0.18%     
==========================================
  Files         265      265              
  Lines       37970    38084     +114     
  Branches     2446     2415      -31     
==========================================
+ Hits        33328    33498     +170     
+ Misses       3568     3513      -55     
+ Partials     1074     1073       -1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

HarelM · 2024-10-17T19:28:21Z

Is it possible to move some parts of this to a plugin? Or maybe place that in an example instead?
I would like to keep changes to the public API as minimal as possible for what I believe is a niche requirement, if possible.

NathanMOlson · 2024-10-17T21:05:09Z

Is it possible to move some parts of this to a plugin? Or maybe place that in an example instead?
I would like to keep changes to the public API as minimal as possible for what I believe is a niche requirement, if possible.

Certainly the jumpToLLA() function and it's supporting changes can be pulled out to a plugin.

I think the other changes (change in recalculateZoom(), increase of maxPitch to 180, and addition of elevation to JumpToOptions) need to stay. The only change affecting the public API would be the addition of elevation to JumpToOptions.

HarelM · 2024-10-18T04:03:04Z

How does elevation and zoom correlate? Can one set an elevation value that will change the zoom value?
It might be better to start with a design before jumping to implementation to see that there is an alignment on what should be the solution...

NathanMOlson · 2024-10-18T15:36:17Z

How does elevation and zoom correlate? Can one set an elevation value that will change the zoom value?

elevation sets the height of the "center point" above sea level. When 3D terrain is used, this is the height of the terrain at the center point, otherwise it is zero. To allow pitch > 90, I intend to position the "center point" floating in space above the terrain. If the center point remains tied to terrain, then pitch > 90 will cause the camera to move under the terrain.

zoom sets the distance from the center point to the camera (in conjunction with fovInRadians, which is currently hardcoded).

Together, zoom, elevation, and pitch set the altitude of the camera.

To allow pitch > 90, I want to move the "center point" off of the ground. This will allow the camera to stay above the ground when it pitches above 90.

It might be better to start with a design before jumping to implementation to see that there is an alignment on what should be the solution...

Where/how should I present the design?

NathanMOlson · 2024-10-18T15:49:07Z

I've reduced the scope of this PR to only changes that allow pitch greater than 90 degrees.

HarelM · 2024-10-18T18:20:03Z

The design can be presented either as an initial comment of an issue, and initial comment in a PR or a discussion.
I would advise to present it before diving into code changes as it makes changes harder when you "fall in love" with a solution :-)

In any case, the diagram above makes things a lot more clearer.
The first question that comes to mind though is how would this support pitch of 180 degrees? Elevation would be set to infinity?

test/integration/render/run_render_tests.ts

src/ui/map.ts

src/geo/projection/globe_transform.ts

src/geo/projection/mercator_transform.ts

NathanMOlson · 2024-10-18T18:35:29Z

The first question that comes to mind though is how would this support pitch of 180 degrees? Elevation would be set to infinity?

No need to set elevation to infinity. It just needs to be set high enough that the camera is above the terrain.

src/geo/projection/mercator_utils.ts

src/geo/transform_interface.ts

src/shaders/terrain.fragment.glsl

src/ui/map.ts

HarelM · 2024-10-18T20:00:16Z

Is it possible to clean the git history as well? Sorry for not picking on that, but it makes review harder because I need to expand every time I look at this PR as git thinks there's a lot of commits...

HarelM · 2024-10-18T20:04:55Z

Can you elaborate a bit of the center change that is added.
I know that we already suffer from the zoom change at the end of a panning when terrain is on, can you explain what is expected to change here?
A video or drawing works be great.
Thanks!!

test/integration/render/tests/terrain/fog/expected.png

test/integration/render/tests/sky/basic/expected.png

package.json

src/geo/projection/mercator_utils.ts

src/geo/transform_helper.ts

src/geo/transform_interface.ts

src/geo/projection/mercator_transform.test.ts

src/ui/camera.ts

test/examples/center-point.html

HarelM · 2024-10-22T05:56:54Z

src/ui/handler_manager.ts

-        const preZoomAroundLoc = tr.screenPointToLocation(panDelta ? around.sub(panDelta) : around);
+        // If we are rotating about the center point, avoid numerical issues near the horizon by using the transform's
+        // center directly, instead of computing it from the screen point
+        const preZoomAroundLoc = around.distSqr(tr.centerPoint) < 1.0e-2 ?


This needs to be covered by a test I believe. Same goes for the other place that has a similar logic.

src/ui/camera.ts

HarelM · 2024-10-22T06:12:36Z

src/geo/projection/mercator_transform.ts

+        // elevation to Mercator Z using the scale factor at the center point (not the camera point). Since the center point is
+        // initially unknown, we compute it using the scale factor at the camera point. This gives us a better estimate of the
+        // center point scale factor, which we use to recompute the center point. We repeat until the error is very small.
+        // This typically takes about 5 iterations.


Is there a way to calculate this without an iterative approach? This seems like a heavy calculation that is being made often...

If I'm not missing something, there's 1 cos, 1 atan, 1 exp, and ~20 arithmetic operations per loop. That doesn't seem too heavy to me? I can't think of a way to do it without iteration, but if it's too heavy we could reduce the accuracy threshold, which is currently set at 10^-12 m. This would result in fewer iterations being performed.

src/geo/projection/mercator_transform.ts

developer-guides/center-point.md

…tionsFromCameraLngLatAltRotation()

NathanMOlson marked this pull request as draft October 17, 2024 17:47

NathanMOlson mentioned this pull request Oct 17, 2024

Add API support for adjusting camera roll angle, extend pitch angle range #4717

Open

HarelM requested a review from ibesora October 18, 2024 04:03

NathanMOlson changed the title ~~Camera-centric~~ Pitch > 90 degrees Oct 18, 2024

HarelM reviewed Oct 18, 2024

View reviewed changes

test/integration/render/run_render_tests.ts Outdated Show resolved Hide resolved

HarelM reviewed Oct 18, 2024

View reviewed changes

src/ui/map.ts Outdated Show resolved Hide resolved

HarelM reviewed Oct 18, 2024

View reviewed changes

src/ui/map.ts Outdated Show resolved Hide resolved

HarelM reviewed Oct 18, 2024

View reviewed changes

src/geo/projection/globe_transform.ts Outdated Show resolved Hide resolved

HarelM reviewed Oct 18, 2024

View reviewed changes

src/geo/projection/mercator_transform.ts Outdated Show resolved Hide resolved

HarelM reviewed Oct 18, 2024

View reviewed changes

src/geo/projection/mercator_utils.ts Outdated Show resolved Hide resolved

HarelM reviewed Oct 18, 2024

View reviewed changes

src/geo/transform_interface.ts Outdated Show resolved Hide resolved

HarelM reviewed Oct 18, 2024

View reviewed changes

src/shaders/terrain.fragment.glsl Outdated Show resolved Hide resolved

HarelM reviewed Oct 18, 2024

View reviewed changes

src/ui/map.ts Outdated Show resolved Hide resolved

NathanMOlson force-pushed the camera-centric branch from ecb12b8 to a53c094 Compare October 19, 2024 15:37

NathanMOlson commented Oct 19, 2024

View reviewed changes

test/integration/render/tests/terrain/fog/expected.png Outdated Show resolved Hide resolved

NathanMOlson commented Oct 19, 2024

View reviewed changes

test/integration/render/tests/sky/basic/expected.png Outdated Show resolved Hide resolved

HarelM reviewed Oct 20, 2024

View reviewed changes

package.json Outdated Show resolved Hide resolved

HarelM reviewed Oct 20, 2024

View reviewed changes

src/geo/projection/mercator_utils.ts Outdated Show resolved Hide resolved

HarelM reviewed Oct 20, 2024

View reviewed changes

src/geo/transform_helper.ts Outdated Show resolved Hide resolved

HarelM reviewed Oct 20, 2024

View reviewed changes

src/geo/transform_interface.ts Outdated Show resolved Hide resolved

NathanMOlson added 5 commits October 21, 2024 20:15

fix build

7ab857a

update high pitch + terrain render test to remove setPitch() call

ba54703

use transform's elevation in setLocationAtPoint()

95ba6d8

fix jumps when using the mouse to rotate past 90 degrees pitch

4edc371

further safety against numerical issues near the horizon

338c78b

NathanMOlson force-pushed the camera-centric branch from f4a6497 to 338c78b Compare October 22, 2024 03:16

fix spelling

e71e8a8

NathanMOlson marked this pull request as ready for review October 22, 2024 03:37