maplibre / maplibre-gl-js

MapLibre GL JS - Interactive vector tile maps in the browser
https://maplibre.org/maplibre-gl-js/docs/
Other
6.61k stars 712 forks source link

map.queryTerrainElevation not returning expected values #3736

Closed mthaddon closed 1 week ago

mthaddon commented 8 months ago

map.queryTerrainElevation returns unexpected values. I've created a demo page that loads elevation tiles and centers on Everest. It then displays the value of map.queryTerrainElevation whenever the map idle event is triggered.

maplibre-gl-js version: 4.0.2

browser: Chrome Version 121.0.6167.85

Steps to Trigger Behavior

  1. Load https://design-deploy.eu/maptest.html
  2. Move around the map and observe the value in the top left corner

Link to Demonstration

https://design-deploy.eu/maptest.html

Expected Behavior

The page runs:

  map.on('idle', () => {
    document.getElementById("elevation").innerHTML = map.queryTerrainElevation([86.922623, 27.986065]); 
  });

I'd expect the values written to be close to the height of Everest.

Actual Behavior

Values fluctuate quite widely, also including minus values.

HarelM commented 8 months ago

This is dependant on the loaded tiles, in different zoom levels you'd have different terrain tiles, which might change the elevation value. I don't know if this is what's causing this issue, but it's worth taking into consideration.

mthaddon commented 8 months ago

Thanks for the response. I understand you might get slightly different elevation values based on zoom level, but is it expected that if we're querying the exact same location every time (Everest, which is 8,849m high) we'd get values that fluctuate widely (143, 646, 12 in a quick test I just did moving around a little on the demo page)?

HarelM commented 8 months ago

Yeah, I see your point. The demo page is querying the same location and if I move the map a bit I get very different results. @mthaddon can you see if the same happens with terrain rgb tiles instead of terrarium?

CC: @prozessor13

HarelM commented 8 months ago

CC: @manhcuongincusar1 I see that the substitute of elevation and transform.elevation is what's causing these value changes. I hope @manhcuongincusar1 can recall why this substitute was needed and if there's a bug in the current code. As a workaround, I think you can currently use map.terrain.getElevationForLngLatZoom(...) to overcome this issue. Nonetheless, this is a bug or unexpected results... https://github.com/maplibre/maplibre-gl-js/blob/db07ac44c8f27291d0409b2af11a80b0f82a6e19/src/ui/camera.ts#L1493

sbachinin commented 8 months ago

This is not a bug but rather a problem of naming and documentation. This method (contrary to tsdoc) doesn't return meters from sea level. It returns a special offset that is used for proper placement of custom 3d objects. It's very well explained here:

// `queryTerrainElevation` gives us the elevation of a point on the terrain
// **relative to the elevation of `center`**,
// where `center` is the point on the terrain that the middle of the camera points at.
// If we didn't account for that offset, and the scene lay on a point on the terrain that is
// below `center`, then the scene would appear to float in the air.

queryTerrainElevation was introduced to solve this issue. And it still solves it well. But again, it's not about meters from sea level. The comments and name are incorrect.

What to do

Plan A

Change the misleading comments, explain what queryTerrainElevation really does. Plus: no breaking change Minus: name queryTerrainElevation is still misleading + users don't have a proper method to get elevation above sea level. (They can call getElevationForLngLatZoom but it's quite verbose).

Plan B (better but with breaking changes).

1) queryTerrainElevation should return meters above sea level, as the name implies: return this.terrain.getElevationForLngLatZoom(LngLat.convert(lngLatLike), this.transform.tileZoom);
This call gives satisfactory results, though there are caveats.
Results begin to deviate when the point is far outside the viewport and/or on low zoom levels.
The further you zoom/pan from this lngLat, the stranger the results. Especially when zoom is less than 5. At zoom 0 you get elevation 0. IDK if it's critical.
It works ok if coordinate is in view and zoom is somewhere > 10. I suppose this covers most reasonable use cases.

2) Existing queryTerrainElevation code should be called something like getElevationOffset or getAltitudeOffset.
But a much better solution will be not to expose this stuff at all. (At the moment IDK if it's doable). Because it's not a feature but only a necessary evil. It's quite low-level and ideally a user shouldn't know anything about this offset.

Also we need to fix "3d models with terrain" page. Currently I see no custom objects there.

HarelM commented 8 months ago

I would advise to change the docs as an initial solution. Create an issue for expected breaking changes for version 5 and track this change there. Also as part of the documentation update, add note about this being subject to change in next release. We have recently published version 4, we should wait at least half a year to publish a new breaking change version I believe.

mthaddon commented 7 months ago

Thanks for the explanations. I've updated the test page so it's now displaying the following:

var msg = "queryTerrainElevation: " + map.queryTerrainElevation([86.922623, 27.986065]) + " Elevation: " + map.terrain.getElevationForLngLatZoom(maplibregl.LngLat.convert([86.922623, 27.986065]), map.transform.tileZoom) + ", Zoom: " + map.getZoom();

getElevationForLngLatZoom seems to be what I'd expect.