Pitch > 90 degrees

[NathanMOlson]

Allow pitch angle > 90 degrees. https://github.com/maplibre/maplibre-gl-js/issues/4717

Changes:

~~1. Add public API jumpToLLA(), which specifies the camera position_ instead of the center point as in jumpTo(). This is basically a thin wrapper around jumpTo() which calculates the JumpToOptions based on the incoming JumpToLLAOptions.~~ 2. 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. 3. Extend maxPitch to 180. 4. 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. ~~5. Replaced calls to this.transform.setElevation() with this.transform.recalculateZoom() when terrain changes. This is important to prevent camera jumps, but is causing some failing render tests. There is still some work to be done here.~~

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

TODO ~~- [ ] Fix issues with render tests caused by recalculateZoom()~~

• [x] Add render tests

• [ ] Figure out whether elevation needs to be added to style spec. https://github.com/maplibre/maplibre-style-spec/issues/851

• [X] Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!

• [X] Briefly describe the changes in this PR.

• [X] Link to related issues.

• [X] Include before/after visuals or gifs if this PR includes visual changes.

• [X] Write tests for all new functionality.

• [X] Document any changes to public APIs.

• [x] Add an entry to CHANGELOG.md under the ## main section.

[codecov-commenter]

Codecov Report

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

Project coverage is 90.37%. Comparing base (d42e0da) to head (53e5f0e). Report is 3 commits behind head on main.

Files with missing lines	Patch %	Lines
src/geo/projection/globe_transform.ts	0.00%	8 Missing :warning:
src/ui/camera.ts	93.02%	0 Missing and 3 partials :warning:
src/ui/handler_manager.ts	83.33%	0 Missing and 1 partial :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4851      +/-   ##
==========================================
+ Coverage   89.94%   90.37%   +0.42%     
==========================================
  Files         265      265              
  Lines       37977    38095     +118     
  Branches     2529     3191     +662     
==========================================
+ Hits        34159    34427     +268     
+ Misses       2951     2696     -255     
- Partials      867      972     +105     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[HarelM]

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]

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]

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]

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]

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

[HarelM]

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?

[NathanMOlson]

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.

[HarelM]

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]

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!!

[HarelM]

I see that this is still in Draft, please ping me when it's ready for review, although I already added some comments, sorry about that...

[NathanMOlson]

I see that this is still in Draft, please ping me when it's ready for review, although I already added some comments, sorry about that...

No problem, I appreciate the feedback and the earlier the better.

[HarelM]

Ok, I think I went over most of the changes in this PR. It would be great to move the drawings to the initial comment in this PR so that future readers will have a better picture of why this change was introduced and what is the design behind it. There's also the question if center can be a 3 numbers array instead of two in terms of public API instead of adding an elevation field. I don't have strong feelings either way, I'll post it on slack and see if I can get some feedback.

[HarelM]

@kubapelc @ibesora would you like to go over this PR as well? I think it's currently in a good state to be merged. It is only an opt-in behavior basically that currently can't really be used by current handlers, which I'm not sure I know how I feel about it... @NathanMOlson does this PR requires the following PR?

• #4779

[NathanMOlson]

@NathanMOlson does this PR requires the following PR?

No, those changes are independent.

[NathanMOlson]

One thing that worries me is that using your example I'm able to go below earth. Shouldn't that be impossible?

I don't think it should be fundamentally impossible.

You can make it impossible by setting maxPitch to 90, or by setting the right combination of maxPitch, minZoom, and elevation (for example, setting minZoom to 14.75 in the example, where maxPitch=105 and elevation=541).

[NathanMOlson]

One thing that worries me is that using your example I'm able to go below earth. Shouldn't that be impossible?

I don't think it should be fundamentally impossible.

Maybe a good compromise between "going underground is impossible" and "no fundamental limits on going underground" is "mouse/touch navigation never puts the camera underground".

Here's a proof of concept that ignores mouse/touch commands that result in the camera being underground. It works in the center-point demo (which doesn't have terrain).

Changes to mercator_camera_helper.ts:

     handleMapControlsRollPitchBearingZoom(deltas: MapControlsDeltas, tr: ITransform): void {
         if (deltas.bearingDelta) tr.setBearing(tr.bearing + deltas.bearingDelta);
-        if (deltas.pitchDelta) tr.setPitch(tr.pitch + deltas.pitchDelta);
+        if (deltas.pitchDelta) {
+            const originalPitch = tr.pitch;
+            tr.setPitch(tr.pitch + deltas.pitchDelta);
+            if (tr.getCameraAltitude() < 0) {
+                tr.setPitch(originalPitch);
+            }
+        }
         if (deltas.rollDelta) tr.setRoll(tr.roll + deltas.rollDelta);
-        if (deltas.zoomDelta) tr.setZoom(tr.zoom + deltas.zoomDelta);
+        if (deltas.zoomDelta) {
+            const originalZoom = tr.zoom;
+            tr.setZoom(tr.zoom + deltas.zoomDelta);
+            if (tr.getCameraAltitude() < 0) {
+                tr.setZoom(originalZoom);
+            }
+        }
     }

To fully work, this would need to support terrain, and would probably have some glitches due to the fact that terrain is constantly changing as the resolution of queried tiles changes. In the terrain case, it might be better to either accept/reject the entire deltas struct instead of the individual components.

[HarelM]

For terrain the following method is being used to make sure the camera is not inside the terrain: https://github.com/maplibre/maplibre-gl-js/blob/491ac04f27c6bba1bf4f3e4b15cea0ddffcff16d/src/ui/camera.ts#L1100-L1113 Which is being used by a global transform update for every movement of the camera: https://github.com/maplibre/maplibre-gl-js/blob/491ac04f27c6bba1bf4f3e4b15cea0ddffcff16d/src/ui/camera.ts#L1122

I'm guessing this should be similar, or the elevateCameraIfInsideTheTerrain can be updated to take into account when the pitch is causing the camera to maybe go underground...

[NathanMOlson]

We could easily update _elevateCameraIfInsideTheTerrain() to handle the no-terrain case.

There are two things I don't like about applying the limit there.

1. It applies to all transform updates, making it impossible to place the camera underground in cases where that is the desired behavior.
2. At that point we have lost the reason why the camera went underground. We just have a transform that puts the camera underground, and we always "fix" it the same way: by raising the camera altitude. This feels strange when the camera entered the ground due to zooming out, but rather than simply limiting the zoom, the correction primarily uses pitch. I would prefer if I try to zoom out to place the camera underground, my zoom change is rejected instead of causing a pitch change.

[HarelM]

For me it feels like solving the same issue in different places, so I prefer to have a single point that handles this, it doesn't have to be the current solution.

[NathanMOlson]

I've moved the "underground" check in the no-terrain case to _elevateCameraIfInsideTerrain().