Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

map.queryTerrainElevation not returning expected values #3736

Closed
mthaddon opened this issue Feb 24, 2024 · 7 comments · Fixed by #3854
Closed

map.queryTerrainElevation not returning expected values #3736

mthaddon opened this issue Feb 24, 2024 · 7 comments · Fixed by #3854
Labels
bug Something isn't working PR is more than welcomed Extra attention is needed terrain

Comments

@mthaddon
Copy link
Contributor

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
Copy link
Collaborator

HarelM commented Feb 24, 2024

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
Copy link
Contributor Author

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
Copy link
Collaborator

HarelM commented Feb 26, 2024

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 HarelM added bug Something isn't working terrain PR is more than welcomed Extra attention is needed labels Feb 26, 2024
@HarelM
Copy link
Collaborator

HarelM commented Feb 26, 2024

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

return elevation - this.transform.elevation;

@sbachinin
Copy link
Collaborator

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
Copy link
Collaborator

HarelM commented Mar 13, 2024

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
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working PR is more than welcomed Extra attention is needed terrain
Projects
None yet
3 participants