Fix/remaining issues camera inside terrain

[chrneumann]

Launch Checklist

Add buffer so that camera doesn't sit directly on terrain. Prevent crashes when zoom is wrongly calculated.

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

[codecov-commenter]

Codecov Report

:x: Patch coverage is 50.00000% with 3 lines in your changes missing coverage. Please review. :warning: Please upload report for BASE (main@2112f2d). Learn more about missing BASE report. :warning: Report is 784 commits behind head on main.

Files with missing lines	Patch %	Lines
src/ui/camera.ts	50.00%	3 Missing :warning:

Additional details and impacted files

@@           Coverage Diff           @@
##             main    #4551   +/-   ##
=======================================
  Coverage        ?   90.70%           
=======================================
  Files           ?      265           
  Lines           ?    38116           
  Branches        ?     3150           
=======================================
  Hits            ?    34575           
  Misses          ?     2601           
  Partials        ?      940           

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

:rocket: New features to boost your workflow:

• :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[HarelM]

@chrneumann where are we with this?

[chrneumann]

@chrneumann where are we with this?

Waiting for your reply, see the open code review.

[HarelM]

Hu? Can you link to it?

[chrneumann]

Hu? Can you link to it?

https://github.com/maplibre/maplibre-gl-js/pull/4551/files/fe6191c166ac876ad07af08b277ec3e3d4538a9b#r1715277344

[HarelM]

Are you sure you have submitted your response? This is what I see on my side:

[chrneumann]

Are you sure you have submitted your response?

Ok, sorry, I did not submit the review.

[HarelM]

Ok, looks good, can you check why the test is failing?

[chrneumann]

Ok, looks good, can you check why the test is failing?

Fixed that, a render test failed because of the readded buffer between terrain surface and camera.

[HarelM]

Should this PR still be in draft mode?

[HarelM]

Can you please add a changelog entry?

[chrneumann]

Done. New transform stuff also merged.

[HarelM]

Looking at the fact that you added code without a failing test can indicate that the added code in the transform class is not covered by tests?

[chrneumann]

Yes, the try/catch is not covered by a test. We talked about that before: https://github.com/maplibre/maplibre-gl-js/pull/4551#discussion_r1743515541 Best would be to have more insights from someone who knows more about the recalculation code. Not sure, if you got a reply? But yeah, I could also try to crash it and find out which values lead to the crash and add a test with these values. But knowing why it crashes in the first place would be better.

[HarelM]

Did you push the commit? I still see that the code coverage is not getting to this catch block...

[chrneumann]

No, did you see the discussion at https://github.com/maplibre/maplibre-gl-js/pull/4551#discussion_r1782227028 ?

[HarelM]

Yes, regardless of if the solution is an "if" or a try-catch, there's a need to add a test the reproduces this issue (I think a unit test is the right kind of test here, but I don't mind either way). Once you have a scenario that can be reproduced we can better talk about the solution, I believe.

[chrneumann]

I can't reproduce the crash any more in the current main branch. Still kept the added surface padding (camera stays not directly on the ground), as it prevents some graphic glitches.

But there seems to be a regression with the transformCameraUpdate. Possibly because the camera transform is updated in the new changes without considering this option. Needs a closer look.

[chrneumann]

But there seems to be a regression with the transformCameraUpdate. Possibly because the camera transform is updated in the new changes without considering this option. Needs a closer look.

Started with 5.0.0-pre.4

[HarelM]

@NathanMOlson is this change problematic for what you are working on? I think this is a good addition to the camera elevation logic, but it might complicate things for your use case. @chrneumann is this ready to me merged?

[chrneumann]

@chrneumann is this ready to me merged?

Yes, it's ready.

[HarelM]

Thanks @NathanMOlson! These are interesting questions and observations.

[Samarth1696]

Tbh I would like to disable this feature for the use case for aircraft. Currently motion control works with jumpTo() in every frame. From what I have seen from my work of motion control that the camera goes under the terrain regardless of elevateCameraIfInsideTerrain is being called, this is because of jumpTo() I think. This feature would be correct for the use case for normal users but things might go wrong for aircraft motion control use case.

We can have a on and off switch for this feature where the user can choose between going under the terrain and not going under it. I know for the default UX no one wants to go under the Terrain but we have to find a solution for the aircraft use case.