Keep camera outside terrain

[chrneumann]

Fix for #1542. Currently proof of concept, comments appreciated.

It uses the transformCameraUpdate functionality to validate the next camera position, possibly adjusting it. By using transformCameraUpdate, the changes can be kept small. The assigned function checks if the next camera position will be inside terrain. If that would be the case, the camera is moved up (keeping its center), so that it is above the terrain. The pitch is adjusted to keep the map's center. IMHO this leads to a good user experience. Tried other options (stopping the camera or just adjusting height), but this seems best.

Proof of concept: Needs some more testing. transformCameraUpdate type signature is changed to pass the whole transform. Might not be needed, did not look at this yet. If needed, an internal wrapper for transformCameraUpdate might be the solution.

Includes a temporary workaround for #4233.

Launch Checklist

• [x] 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.
• [x] 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.
• [ ] Post benchmark scores.
• [ ] Add an entry to CHANGELOG.md under the ## main section.

[HarelM]

I think the approach presented here is a very elegant one. Thanks! This still needs some love and care, but otherwise a very good idea to solve this issue. Assuming we solve the questions raised in #4243

[chrneumann]

Waiting for #4299 (fixes camera jumps) to be approved before polishing this PR.

[HarelM]

Generally, the linked PR looks like the right solution, so you can assume it will be merged, the only comments I have there are related to tests...

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 94.33962% with 3 lines in your changes missing coverage. Please review.

Project coverage is 87.55%. Comparing base (d9e75d2) to head (2b8f28b). Report is 202 commits behind head on main.

Files with missing lines	Patch %	Lines
src/ui/camera.ts	94.33%	0 Missing and 3 partials :warning:

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4300      +/-   ##
==========================================
- Coverage   87.77%   87.55%   -0.22%     
==========================================
  Files         246      246              
  Lines       33421    33446      +25     
  Branches     2206     2236      +30     
==========================================
- Hits        29334    29283      -51     
- Misses       3080     3134      +54     
- Partials     1007     1029      +22     

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

[chrneumann]

Found more problems (fixed in https://github.com/maplibre/maplibre-gl-js/pull/4300/commits/f6bdf15fe07a039135755f38a231d54c40ced9c7) with the direct manipulation of the transform, which leads to jumps and other problems. One should keep in mind that there might be other places where the transform is manipulated directly instead of using the _getTransformForUpdate / _applyUpdatedTransform combo.

Moved the logic to the camera.

[HarelM]

This looks good! There are failing render tests though, please see if there's a need to update the expected image of the fog tests due to this fix.

[chrneumann]

This looks good! There are failing render tests though, please see if there's a need to update the expected image of the fog tests due to this fix.

Yes, the pitch of the camera changed because the camera sits on the terrain surface instead of having the minimum distance to the surface. Changed the render images.

[chrneumann]

Build size is bigger, I think that's fine?

[HarelM]

Yes, don't worry about it.

[HarelM]

the pitch of the camera changed because the camera sits on the terrain surface instead of having the minimum distance to the surface. Changed the render images.

Really? This seems strange... is the pitch this high? Are you sure the threshold is not too tight or something?

[chrneumann]

Really? This seems strange... is the pitch this high? Are you sure the threshold is not too tight or something?

I removed the padding altogether. Makes code simpler and I don't think its needed at all for UX.

[HarelM]

Can you please add a changelog entry?

[HarelM]

Thanks!!