Camera can move inside 3D terrain

[mattgrid]

This is a follow-on ticket from #1241. It's possible to move the camera to within terrain when it's extruded from the 2D map surface.

@zdila said:

May be a different issue, but in my case not only closest tiles but all. Open https://labs.maptiler.com/samples/maplibre/terrain/#style=satellite&lat=48.57344071&lng=20.46078220&zoom=17.16&bearing=-127.95&pitch=76.00&3d=true then zoom a little bit and tiles starts to disappear. If you refresh the page, nothing is shown until you zoom out. MapLibre 2.3.0.

@prozessor13 replied:

Walking with the camera into terrain is still an open issue :/

Expected behaviour

When the camera "bumps up" against 3D terrain, it should not be possible to move the camera to within the 3D surface.

Actual behaviour

When you move the camera towards 3D terrain, nothing stops you when you "hit" the surface. You can keep moving through the surface.

[prozessor13]

I think i will not have time to look onto this in near future. Also I think before start implementing this, all possible scenarios should be played through. What should happen if, for example:

• map pitch changed with mouse-interaction, and camera collide with terrain.
  • may changing pitch should stop?
  • or should camera go up?
• map bearing changed with mouse-interaction, and camera collide with terrain.
  • I think camera should go up?
  • should it also rotate downwards do look onto the same center?
• flyTo/easeTo collide with terrain
• ...

[767160]

My company would be happy to hire someone to fix this issue

[wipfli]

@767160 thanks for sharing. You can ping me on slack and I can maybe put you in touch with people who are interested... https://osmus-slack.herokuapp.com/

[767160]

@wipfli Sorry, I do not use slack. Anyone interested, please send me an email at [email protected]

[HarelM]

Assigned L bounty.
Link to parent Bounty: maplibre/maplibre#189
If you would like to work on this and you think the bounty is not enough please ping me or contact @767160

[prozessor13]

I already have a basic working prototype for this, may in near future i find the time to create a PR.

[SnailBones]

I'd be interested in working on this if you're open to passing the baton @prozessor13.

[HarelM]

I've increased the bounty as I think L bounty isn't enough.

[prozessor13]

This is the basic working code, based on old versions of transform.ts and painter.ts, but i think the patch should work in the current classes as well.

Regards. Max.

diff --git a/src/geo/transform.ts b/src/geo/transform.ts
index 466fafaf7..bedc18210 100644
--- a/src/geo/transform.ts
+++ b/src/geo/transform.ts
@@ -520,6 +520,14 @@ class Transform {
         return {lngLat, altitude: altitude + this.elevation};
     }
 
+    // check that camera is always over terrain
+    checkTerrainCollision(terrain: Terrain) {
+        const camera = this.getCameraPosition();
+        const altitude = this.getElevation(camera.lngLat, terrain) + 100;
+        if (camera.altitude < altitude)
+            this.pitch = Math.acos((altitude - this._elevation) * this._pixelPerMeter / this.cameraToCenterDistance) / Math.PI * 180;
+    }
+
     /**
      * This method works in combination with freezeElevation activated.
      * freezeElevtion is enabled during map-panning because during this the camera should sit in constant height.
diff --git a/src/render/painter.ts b/src/render/painter.ts
index a707f3b21..61cd282b6 100644
--- a/src/render/painter.ts
+++ b/src/render/painter.ts
@@ -415,10 +415,16 @@ class Painter {
             // this is disabled, because render-to-texture is rendering all layers from bottom to top.
             this.opaquePassCutoff = 0;
 
+            // check if projMatrix has changed
+            if (!mat4.equals(this.terrainFacilitator.matrix, this.transform.projMatrix)) {
+               this.terrainFacilitator.dirty = true;
+               mat4.copy(this.terrainFacilitator.matrix, this.transform.projMatrix);
+               this.transform.checkTerrainCollision(this.style.map.terrain);
+            }
+
             // update coords/depth-framebuffer on camera movement, or tile reloading
             const newTiles = this.style.map.terrain.sourceCache.tilesAfterTime(this.terrainFacilitator.renderTime);
-            if (this.terrainFacilitator.dirty || !mat4.equals(this.terrainFacilitator.matrix, this.transform.projMatrix) || newTiles.length) {
-                mat4.copy(this.terrainFacilitator.matrix, this.transform.projMatrix);
+            if (this.terrainFacilitator.dirty || newTiles.length) {
                 this.terrainFacilitator.renderTime = Date.now();
                 this.terrainFacilitator.dirty = false;
                 drawDepth(this, this.style.map.terrain);

[typebrook]

This approach works great when changing bearing!

But there are some issues:

1. When I pan map toward a slope, map rendering stop and I got the following errors:

Uncaught Error: failed to invert matrix         transform.ts:937:22    
    _calcMatrices transform.ts:937
    set pitch transform.ts:186
    checkTerrainCollision transform.ts:527
    render painter.ts:404
    ...

2. When camera is facing the bottom of a slope, I try to increase the pitch with Ctrl and mouse. Then the map shaking very quickly since pitch is changing by checkTerrainCollision.

[prozessor13]

For 1) please try:

diff --git a/src/geo/transform.ts b/src/geo/transform.ts
index bedc18210..4132d5687 100644
--- a/src/geo/transform.ts
+++ b/src/geo/transform.ts
@@ -523,9 +523,11 @@ class Transform {
     // check that camera is always over terrain
     checkTerrainCollision(terrain: Terrain) {
         const camera = this.getCameraPosition();
-        const altitude = this.getElevation(camera.lngLat, terrain) + 100;
-        if (camera.altitude < altitude)
-            this.pitch = Math.acos((altitude - this._elevation) * this._pixelPerMeter / this.cameraToCenterDistance) / Math.PI * 180;
+        const altitude = this.getElevation(camera.lngLat, terrain) + 10;
+        if (camera.altitude < altitude) {
+            const pitch = Math.acos((altitude - this._elevation) * this._pixelPerMeter / this.cameraToCenterDistance) / Math.PI * 180;
+            if (!isNaN(pitch)) this.pitch = pitch;
+        }
     }
 
     /**

[typebrook]

Yes, this make sense.

Would you like to change constant 100 by zoom level when assign altitude?

[prozessor13]

Yes, may 100 is to much! Feel free to make a proposal/change. This is only a quick and not very well tested solution!
Next, i will not create a PR and tests and all that stuff, may @SnailBones or you is willed?

[typebrook]

may @SnailBones or you is willed?

Sure, I am willing to work on this. Since I am new to frontend. This issue seems the perfect one to help me realize map rendering workflow.

But @SnailBones has already leave the comment before,let @HarelM decide the assignee.

@prozessor13
BTW, is it possible to check terrain collision in Transform._constrain()?

I notice this naming may related to constrain camera when terrain collision happens. But it seems Terrain is necessary to decide camera altitude.

[prozessor13]

may @SnailBones or you is willed?

Sure, I am willing to work on this. Since I am new to frontend. This issue seems the perfect one to help me realize map rendering workflow.

But @SnailBones has already leave the comment before,let @HarelM decide the assignee.

@prozessor13 BTW, is it possible to check terrain collision in Transform._constrain()?

I notice this naming may related to constrain camera when terrain collision happens. But it seems Terrain is necessary to decide camera altitude.

Yes, this is the dilemma, that the Transform instance does nothing know about terrain. To be honest, my implementation was really a quick one, i did not go into detail. So it is really only a proposal! Feel free to change the code!

[HarelM]

Well @SnailBones was the first one to request, so it's up to him to decide if he would like to push this forward or leave it to @typebrook.

[SnailBones]

Well @SnailBones was the first one to request, so it's up to him to decide if he would like to push this forward or leave it to @typebrook.

All yours @typebrook! Feel free to tag me for review.

[typebrook]

Thank you @SnailBones, I'll make a PR with tests in these days.

[typebrook]

@SnailBones @prozessor13 @HarelM

Sorry for the late reply. A new draft PR is made.

Since I am still not confident about realizing map rendering flow, if you guys find my approach is too naive, feel free to correct me or change the assignee.

[chrneumann]

@HarelM could you please assign this to me?

[HarelM]

Sure!
Have you registered with the developer pool?

[chrneumann]

Yes, I've talked to Oliver.
Thanks!

[HarelM]

I'm reopening this bug as it looks like the fix did not fully solve it.
I have panned and moved the map in the doc site and it seems like I can still reach a state where the camera is inside the terrain - I did not do anything special, just used zoom and pan.

[chrneumann]

It think it just looks as it is inside the terrain, because I removed the buffer in the last steps. The camera sits right on top of the terrain, which I think leads to this visual glitches. I would suggest to readd the buffers, that looks better.

The crash should be the one in #4235 and less related to this issue.

[chrneumann]

Another example. The camera is not inside the terrain, but directly on the surface. This can be prevented by adding the buffer.

[HarelM]

So let try and add the buffer back and see if we don't get these issues anymore.

[chrneumann]

I think you can't prevent visual glitches completely, but adding the buffer would prevent the more common ones. For example, if a terrain on the left side of your view is really close and steep you might have to prevent the camera sitting next to it. But what if on the right side, there is also a steep hill very close? Should the camera be prevented to be in such a valley? But it might be ok if the field of view is narrow (the camera "fits" into the valley). But maybe the user resizes the canvas (and such the camera doesn't fit any more). There are so many factors to consider. I think that would make the solution way to complicated. So yeah, the buffer should prevent most, but will not prevent all visual glitches if you don't want to restrict camera movement too much.

As said, the crashes are something different, they must not occur. I would really suggest to handle them in a less catastrophic way. They should not lead the map to stop rendering. As the transformCamera implementiation depends on other code not directly manipulating the map's transform, there still might be places where this does happen and possibly will happen again with new code. I think it will take some time to harden this, and until then, problems with this should not lead to crashes. Maybe some form of linting that detects direct manipulation of the transform would be nice. Or really setting the transform private and only allow manipulation through the methods.

[HarelM]

We are doing a big refactoring of the trasform as part of the globe branch, so I hope it will be better afterwards.
Let's add the buffet and make sure that most common cases are handled and that it's harder to create a "bad" scene.

[chrneumann]

Done in #4551
Looks visually fine and the crashes are logged to console instead of thrown. Could not find any other issues.