Fix/remaining issues camera inside terrain #4551

chrneumann · 2024-08-13T10:37:32Z

Launch Checklist

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

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

This reverts commit 644ae1f.

This reverts commit b104163.

codecov-commenter · 2024-08-13T10:44:18Z

Codecov Report

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

Project coverage is 90.70%. Comparing base (d3f2bca) to head (dfeb449).
Report is 4 commits behind head on main.

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

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4551      +/-   ##
==========================================
+ Coverage   90.48%   90.70%   +0.22%     
==========================================
  Files         265      265              
  Lines       38113    38116       +3     
  Branches     3166     3150      -16     
==========================================
+ Hits        34488    34575      +87     
+ Misses       2650     2601      -49     
+ Partials      975      940      -35

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

src/geo/transform.ts

HarelM · 2024-08-24T13:14:45Z

@chrneumann where are we with this?

chrneumann · 2024-08-24T13:39:44Z

@chrneumann where are we with this?

Waiting for your reply, see the open code review.

HarelM · 2024-08-24T13:54:10Z

Hu?
Can you link to it?

chrneumann · 2024-08-28T13:08:43Z

Hu? Can you link to it?

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

HarelM · 2024-08-28T13:31:33Z

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

chrneumann · 2024-08-28T13:49:08Z

Are you sure you have submitted your response?

Ok, sorry, I did not submit the review.

HarelM · 2024-09-14T18:10:40Z

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

…camera-inside-terrain

chrneumann · 2024-09-19T15:33:33Z

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 · 2024-09-19T16:16:18Z

Should this PR still be in draft mode?

HarelM · 2024-09-19T18:04:23Z

Can you please add a changelog entry?

…camera-inside-terrain

chrneumann · 2024-09-30T09:42:00Z

Done. New transform stuff also merged.

src/geo/transform.ts

HarelM · 2024-09-30T20:40:03Z

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 · 2024-09-30T21:29:08Z

Yes, the try/catch is not covered by a test. We talked about that before: #4551 (comment) 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 · 2024-10-01T08:08:14Z

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

chrneumann · 2024-10-01T08:38:12Z

No, did you see the discussion at #4551 (comment) ?

HarelM · 2024-10-01T09:35:52Z

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.

…camera-inside-terrain

chrneumann · 2024-10-30T10:21:23Z

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 · 2024-10-30T10:44:54Z

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

…camera-inside-terrain

HarelM · 2024-10-31T13:24:50Z

@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 · 2024-10-31T14:09:15Z

@chrneumann is this ready to me merged?

Yes, it's ready.

NathanMOlson · 2024-10-31T15:44:12Z

src/ui/camera.test.ts

@@ -2028,6 +2028,9 @@ describe('#flyTo', () => {
        };
        camera.transform = {
            elevation: 0,
+            recalculateZoom: () => true,


this should be recalculateZoomAndCenter() now

NathanMOlson · 2024-10-31T16:01:39Z

src/ui/camera.ts

        if (!this.terrain && tr.elevation >= 0 && tr.pitch <= 90) {
            return {};
        }
+        const surfacePadding = Math.min(500, 20 * (25 - tr.zoom));


Why do we need this surface padding?

At a typical zoom of 15, this sets the minimum altitude at 200m. This amount of surface padding is problematic for ground-level use cases (like an aircraft on the runway, or touring a city at eye-level.)

It can help prevent graphic glitches when the camera is directly moving on the surface. Do these use cases have terrain enabled? If so, one could make the padding optional.

The "aircraft on runway" (#4717) use case (#4717) will definitely have terrain enabled.

NathanMOlson · 2024-10-31T16:04:53Z

src/ui/camera.ts

        if (!this.terrain && tr.elevation >= 0 && tr.pitch <= 90) {
            return {};
        }
+        const surfacePadding = Math.min(500, 20 * (25 - tr.zoom));


The linear scaling with zoom level doesn't make sense to me. I think it would be more appropriate to have something like 500 / (2**tr.zoom).

I don't have strong arguments for the current calculation, but why do you think non-linear is better?

Because the distance from camera to center point (in meters) is inversely proportional to 2**tr.zoom

If you increase zoom by 1, you are cutting the distance from camera to center point in half. So it seems like you should also cut the padding in half.

NathanMOlson · 2024-10-31T16:07:42Z

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

The "expected" image shows this bug: #4859

NathanMOlson · 2024-10-31T16:09:18Z

test/integration/render/tests/high-pitch/terrain-pitch95/expected.png

I don't think any of the "expected" images for these render tests should change. The fact that they are changing means that the pitch angle is being adjusted, and I don't think it should be in any of these cases.

It's shifted because the padding is added which results in a changed angle (the center stays the same).

More comments

HarelM · 2024-10-31T19:22:33Z

Thanks @NathanMOlson! These are interesting questions and observations.

Samarth1696 · 2024-11-01T04:48:02Z

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.

chrneumann added 4 commits August 13, 2024 11:50

Revert "Remove padding from camera elevation if inside terrain."

6ce770b

This reverts commit 644ae1f.

Reapply "Fix render tests for terrain."

2efeea5

This reverts commit b104163.

Don't directly update transform in _finalizeElevation.

d569899

Prevent throwing in case of wrong zoom calculation.

fe6191c

chrneumann mentioned this pull request Aug 13, 2024

Camera can move inside 3D terrain #1542

Open

HarelM reviewed Aug 13, 2024

View reviewed changes

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

HarelM added 2 commits August 28, 2024 17:30

Merge branch 'main' into fix/remaining-issues-camera-inside-terrain

db287d1

Merge branch 'main' into fix/remaining-issues-camera-inside-terrain

b5efbf6

HarelM added the waiting for code review fixes label Aug 28, 2024

HarelM and others added 2 commits September 4, 2024 13:44

Merge branch 'main' into fix/remaining-issues-camera-inside-terrain

9950b94

Rework catching of errors on zoom recalculation.

3484b91

HarelM and others added 4 commits September 14, 2024 21:10

Merge branch 'main' into fix/remaining-issues-camera-inside-terrain

1d83c6e

Merge remote-tracking branch 'origin/main' into fix/remaining-issues-…

c2825a0

…camera-inside-terrain

Fix camera unit test.

103794d

Fix render test.

1ee53ee

Merge branch 'main' into fix/remaining-issues-camera-inside-terrain

cba97c1

chrneumann marked this pull request as ready for review September 19, 2024 16:29

chrneumann added 2 commits September 30, 2024 11:25

Merge remote-tracking branch 'origin/main' into fix/remaining-issues-…

a5dc76d

…camera-inside-terrain

Add changelog entry.

cd88615

HarelM reviewed Sep 30, 2024

View reviewed changes

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

Remove old transform, apply changes to mercator transform.

9940ea5

Merge remote-tracking branch 'origin/main' into fix/remaining-issues-…

0f0d4de

…camera-inside-terrain

chrneumann and others added 3 commits October 30, 2024 12:03

Fix build and render tests.

a177ba4

Merge remote-tracking branch 'origin/main' into fix/remaining-issues-…

e412a62

…camera-inside-terrain

Merge branch 'main' into fix/remaining-issues-camera-inside-terrain

dfeb449

HarelM previously approved these changes Oct 31, 2024

View reviewed changes

NathanMOlson reviewed Oct 31, 2024

View reviewed changes

birkskyum mentioned this pull request Nov 6, 2024

Toggle for Terrain collision detection, to disable it #5004

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix/remaining issues camera inside terrain #4551

Fix/remaining issues camera inside terrain #4551

chrneumann commented Aug 13, 2024 •

edited

Loading

codecov-commenter commented Aug 13, 2024 •

edited

Loading

HarelM commented Aug 24, 2024

chrneumann commented Aug 24, 2024

HarelM commented Aug 24, 2024

chrneumann commented Aug 28, 2024 •

edited

Loading

HarelM commented Aug 28, 2024

chrneumann commented Aug 28, 2024

HarelM commented Sep 14, 2024

chrneumann commented Sep 19, 2024

HarelM commented Sep 19, 2024

HarelM commented Sep 19, 2024

chrneumann commented Sep 30, 2024

HarelM commented Sep 30, 2024

chrneumann commented Sep 30, 2024

HarelM commented Oct 1, 2024

chrneumann commented Oct 1, 2024

HarelM commented Oct 1, 2024

chrneumann commented Oct 30, 2024

chrneumann commented Oct 30, 2024

HarelM commented Oct 31, 2024

chrneumann commented Oct 31, 2024

NathanMOlson Oct 31, 2024

NathanMOlson Oct 31, 2024 •

edited

Loading

chrneumann Oct 31, 2024

NathanMOlson Oct 31, 2024

NathanMOlson Oct 31, 2024

chrneumann Oct 31, 2024

NathanMOlson Oct 31, 2024

NathanMOlson Oct 31, 2024

NathanMOlson Oct 31, 2024

chrneumann Oct 31, 2024

HarelM commented Oct 31, 2024

Samarth1696 commented Nov 1, 2024

Fix/remaining issues camera inside terrain #4551

Are you sure you want to change the base?

Fix/remaining issues camera inside terrain #4551

Conversation

chrneumann commented Aug 13, 2024 • edited Loading

Launch Checklist

codecov-commenter commented Aug 13, 2024 • edited Loading

Codecov Report

HarelM commented Aug 24, 2024

chrneumann commented Aug 24, 2024

HarelM commented Aug 24, 2024

chrneumann commented Aug 28, 2024 • edited Loading

HarelM commented Aug 28, 2024

chrneumann commented Aug 28, 2024

HarelM commented Sep 14, 2024

chrneumann commented Sep 19, 2024

HarelM commented Sep 19, 2024

HarelM commented Sep 19, 2024

chrneumann commented Sep 30, 2024

HarelM commented Sep 30, 2024

chrneumann commented Sep 30, 2024

HarelM commented Oct 1, 2024

chrneumann commented Oct 1, 2024

HarelM commented Oct 1, 2024

chrneumann commented Oct 30, 2024

chrneumann commented Oct 30, 2024

HarelM commented Oct 31, 2024

chrneumann commented Oct 31, 2024

Choose a reason for hiding this comment

NathanMOlson Oct 31, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HarelM commented Oct 31, 2024

Samarth1696 commented Nov 1, 2024

chrneumann commented Aug 13, 2024 •

edited

Loading

codecov-commenter commented Aug 13, 2024 •

edited

Loading

chrneumann commented Aug 28, 2024 •

edited

Loading

NathanMOlson Oct 31, 2024 •

edited

Loading