Toggle to allow Camera Below the Surface

[Samarth1696]

Solution to #5004.

before terrainCollision: true:

terrainCollision-on.mp4

after terrainCollision: false:

terrainCollision-off.mp4

Just added small changes.

Launch Checklist

• 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.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 81.81818% with 2 lines in your changes missing coverage. Please review.

Project coverage is 88.61%. Comparing base (8555091) to head (26d0aea).

Files with missing lines	Patch %	Lines
src/ui/camera.ts	81.81%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5028      +/-   ##
==========================================
- Coverage   89.25%   88.61%   -0.64%     
==========================================
  Files         269      269              
  Lines       38286    38297      +11     
  Branches     2347     2423      +76     
==========================================
- Hits        34172    33937     -235     
- Misses       3117     3322     +205     
- Partials      997     1038      +41     

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

[HarelM]

I think I would recommend something more descriptive, terrainCollision sounds like a callback and not a boolean value...

Also, is there another way to achieve this without adding this to the public API?

[Samarth1696]

I think I would recommend something more descriptive, terrainCollision sounds like a callback and not a boolean value...

I also thought of that. Maybe straight away call it as goUnderTheTerrain?

Also, is there another way to achieve this without adding this to the public API?

Ok, noted. Users will just have to use the setter to enable it.

[HarelM]

I thought about something like allowTerrainCollision or allowCameraBelowSurface, IDK... you can post on slack to get better ideas for names I guess...

Ok, noted. Users will just have to use the setter to enable it.

If we have a setter I think would should allow initializing the map with it, but I'm asking if there's even something else that requires even less public API changes?

[Samarth1696]

Based on this conversation I think allowCameraBelowSurface will look more sensible: #4851 (review)

If we have a setter I think would should allow initializing the map with it, but I'm asking if there's even something else that requires even less public API changes?

I have reverted changes in Map. Let me know if this is what you wanted.

[HarelM]

Map inherits from camera, so ant public method in camera is also public for map. I'm trying to see if there's a way to avoid exposing this somehow...?

[Samarth1696]

Maybe set the variable as private? I can't think of any other idea than this..

[Samarth1696]

Just letting you know I will be working on hacks to add it somehow on the plugin side. Also I was wrong, even with _elevateCameraIfInsideTerrain being returned early the camera was elevating up above the terrain surface. I will find a way to stop that also if I can.. idk..

Sorry for any false arguments that may have been irritating for you.. I am still learning you know... :/

[HarelM]

No problem.
We have discussed this issue yesterday in the monthly meeting.
A proposal that was discussed was to fire an event when this occurs to let outside code decide what to do and how to react to this situation.
I think this will allow better ergonomics around this and won't change the public API dramatically.