Add queryTerrainElevation exaggeration option #10602

arindam1993 · 2021-04-21T02:18:04Z

Adds 3 API methods and and a new type LngLatElevation = { location: LngLatLike, elevation: number | nulll }

project3d(LngLatElevation) : Point : Similar to map.project but instead of assuming altitude implicitly as the terrain altitude, it lets the user specify an absolute altitude with respect to mean sea level
unproject3d(point: Point, options: {exaggerated: boolean}): LngLatElevation : null: Similar to map.unproject() but also returns elevation, and accounts for horizon visibility without clamping the point down to the horizon.
queryTerrainElevation(lngLat: LngLatLike, options: {exaggerated: boolean}): number | null: Returns the elevation at specfied lngLat, returns null if it hasnt been loaded yet.

Launch Checklist

briefly describe the changes in this PR
write tests for all new functionality
document any changes to public APIs
manually test the debug page
apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog>Add map.queryElevation(point) and map.isUnderneath(point)</changelog>

astojilj · 2021-04-21T05:16:24Z

src/ui/map.js

+     *   var elevation = map.queryTerrainElevation(e.point);
+     * });
+     */
+    queryTerrainElevation(point: PointLike): number | null {


Why not returning also LngLat of the raycast point, I mean in addition to elevation?

We already have unproject for that. Initially I was actually thinking of adding altitude or z to LngLat itself and not add new methods, but that made things a bit tricky.

For all out other functions that take a LngLat (project, map.getCenter()/setCenter(), easeTo() etc) we have already assumed that LngLat is at the ground surface and we automatically interpret its altitude by looking at the height underneath the terrain for that supplied LngLat. If we add z encoded into the LngLat itself we then need another mechanism to disambiguate between LngLat.z and the terrain z at that location.

So I felt it was simpler to keep the assumption that a LngLat is always on the ground/terrain surface like we have currently and instead add a new method to extract the z.

This got addressed later - original implementation here was using transform.elevation.pointCoordinate and not clamping points above ground (clamping to ground done in unproject using transform.pointCoordinate3D).

While unproject is a best effort to seamlessly integrate skybox and terrain to existing application code, there is no raycast API exposed that wouldn't clamp.

karimnaaji · 2021-04-21T16:28:27Z

src/ui/map.js

+     *   var onMap = map.isUnderneath(e.point);
+     * });
+     */
+    isUnderneath(point: PointLike): boolean {


Naming for this one is tricky. I didn't immediately understand the meaning of that function (I had to read the docs to understand exactly what it does), maybe there is a better name for this function?

The internal isPointOnMap is pretty explicit but directly using that would add a redundant map in the name (e.g. map.isPointOnMap(point)).

A few more ideas that I thought about but didn't seem immediately better: map.isColliding(point), map.isBelow(point), map.isIntersecting(point).

Yea I tried to be a bit cheeky with the name because the redundant use of both Point and Map in internal code seemed fine, but was a bit weird when writing out the example.

karimnaaji · 2021-04-21T16:28:49Z

src/ui/map.js

+     * });
+     */
+    isUnderneath(point: PointLike): boolean {
+        return this.transform.isPointOnMap(Point.convert(point));


Concerning the behavior of this API:

Is there any overlap between this and the internal: transform.isPointAboveHorizon()? Basically, is !transform.isPointAboveHorizon(point) equivalent to isPointOnMap(point)?

As it seems like this API does not account for extrusion above the map plane, should we give a mention of that in the docs?

I added a comment for 1.
https://github.com/mapbox/mapbox-gl-js/pull/10602/files#r617818148

Good point about extrusions. Would it be clearer if we renamed the function to isOverGround ? or something similar or map.isGroundUnderneath(point)?

How about simply map.isOver(Point)?

oh yea, how about isOverlapping ? because over has weird height connotations (floating over)

Ok how about even simpler map.overlaps(point)?

I like overlaps because it's really succinct and clear, but using a verb doesn't seem common in our APIs (haven't seen any), it seems like all checks are using the is* approach, so maybe what you suggested (isOverlapping?).

Overlap: To lie or extend over and cover part of.

Overlaps what?

Similar to #10602 (comment), Can this be folded into unproject with an option or as an overload? If it's too soon to do that, maybe we don't add this method yet.

On terrain, if there is a use case to get point on a map, only if raycast hits the map, this is not optimal: it mandates two DEM tree raycast calls for each point: unproject & overlap.

How about adding method raycast - that would not clamp like unproject and provide elevation for the raycast point and null, if map is not hit?
I understand that raycast already sounds like a convenience wrap to queryRenderedFeatures. I'm not worried about that though - in a way, overlaps here could be seen as implementing (map.queryRenderedFeatures(e.point, {layers:['sky']}).length == 0).

karimnaaji · 2021-04-21T16:34:50Z

src/ui/map.js

+
+    /**
+     * Returns `true` when the pixel underneath `point` is a part of the Map
+     * and `false` when the pixel underneath is part of the surrounding whitespace or a `sky` layer.


It might be difficult to disambiguate that clearly in the documentation, but this notion of whitespace might be hard to visualize with fog.

arindam1993 · 2021-04-21T19:25:19Z

src/geo/transform.js

@@ -961,6 +961,8 @@ class Transform {

    /**
     * Returns true if a screenspace Point p, is above the horizon.
+     * This approximates the map as an infinite plane and does not account for z0-z3
+     * wherein the map is small quad with whitespace above the north pole and below the south pole.


@karimnaaji I added the explanation on why transform.IsPointAboveHorizon is not equivalent to isPointOnMap here.

This function is only used for queryRenderedFeatures where its used to early reject a few specific features from intersection evaluation. And this "infinite-plane" like approximation is necessary in that case because map aligned symbols and circles can extend beyond the north pole.

arindam1993

First pass at addressing comments

karimnaaji

Capturing some conversation we had about this and ideas/concerns that were brought up:

(1) About adding a specific API with Point (e.g. queryElevation(Point) in addition to queryElevation(LngLat): My suggestion is to not add that as it's just a helper for queryElevation(unproject(Point)) and is redundant and not as slow as we might think based on profiling. If we also consider what are the most common cases for these functions it seems appropriate: elevation profile from current view + less frequent per point queries.

There were concerns about recommending queryElevation(unproject(Point)) approach as the way forward to do a query on a Point.

That is very slow though, unproject has to raytrace through the dem tree, it actually gets the elevation out, then throws it away and then we linear search for a tile to get the elevation at a latlng

Profiling for the following diff:

diff --git a/debug/3d-playground.html b/debug/3d-playground.html
index 4615a0011..c28a8cc98 100644
--- a/debug/3d-playground.html
+++ b/debug/3d-playground.html
@@ -244,6 +244,10 @@ map.on('style.load', function() {
         }
     }];
+    map.on("mousemove", (e) => {
+        map.queryTerrainElevationAt(map.unproject(e.point));
+    });
+
     setInterval(() => {
         if (demo3d.animateCamera) {
             animationIndex %= animations.length - 1;
(END)

showed about ~140us per query from my testing, which is reasonable for something that may happen at worst once a frame on mouse move. I couldn't think of a strong case for multiple Points per frame queries.

(2) I recommend leaving queryElevation(LngLat) as is, since it allows for further geometry extensions without breaking changes (LngLat | [LngLat, ...] | [[LngLat, ...],[LngLat, ...] | ...). We would currently only provide it for one LngLat, and extend for geometries if needed.

(3) It was proposed to use a generic approach API such as:

queryRenderedSurface(point: PointLike, options: { fog?})  {
  return {
    onSurface: boolean, //is this point on the ground
    terrainHeight?: //includes exaggeration
    elevation?: Number, //excludes exaggeration
    fogOpacity?: // only if speicfied in options,
}

Though, for granularity and simplicity while also reducing cognitive load and have focused purpose functions I wouldn't suggest doing that (worth noting that we're also directly matching native SDKs).

So essentially this PR would only be adding isUnderneath, although I'm still unsure about the best naming there.

arindam1993 · 2021-04-23T00:02:25Z

Yea you're right its really not worth it for the performance difference.
The worst case I was able to profile was a 10x slowdown, but from 0.01ms to 0.1ms, Not worth it at those scales.

karimnaaji · 2021-04-23T00:15:11Z

debug/threejs-antenna.html

@@ -109,7 +109,7 @@
                .makeTranslation(
                    modelTransform.translateX,
                    modelTransform.translateY,
-                    map.queryTerrainElevationAt(modelOrigin) * modelTransform.scale
+                    map.queryTerrainElevationAtLocation(modelOrigin) * modelTransform.scale


I think that's a leftover from the changes: queryTerrainElevation

karimnaaji · 2021-04-23T00:15:37Z

src/ui/map.js

     */
-    queryTerrainElevationAt(lnglat: LngLatLike): number | null {
+    queryTerrainElevation(lnglat: LngLatLike): number | null {


karimnaaji · 2021-04-23T00:17:17Z

src/ui/map.js

     * @returns {number | null} The elevation in meters, accounting for `terrain.exaggeration`.
     * @example
     * var coordinate = [-122.420679, 37.772537];
-     * var elevation = map.queryTerrainElevationAt(coordinate);
+     * var elevation = map.queryTerrainElevationAtLocation(coordinate);


-> queryTerrainElevation

asheemmamoowala · 2021-04-22T00:31:42Z

src/ui/map.js

@@ -2171,22 +2171,58 @@ class Map extends Camera {
    }

    /**
-     * Queries the currently loaded data for elevation at a geographical location. This accounts for the value of `exaggeration` set on `terrain`.
+     * Queries the currently loaded data for elevation at a geographical location. The elevation is returned in `meters` and accounts for the value of `exaggeration` set on `terrain`.
     * Returns `null` if `terrain` is disabled or if terrain data for the location hasn't been loaded yet.
     *
     * In order to guarantee that the terrain data is loaded ensure that the geographical location is visible and wait for the `idle` event to occur.


In order to guarantee that the terrain data is loaded ensure that the geographical location is visible and wait for the idle event to occur.

This documentation makes it unclear if and when it is OK to call this method. The debug pages modified in this PR call this method from the Custom Layer render callback, which contradicts this statement.
The qRF docs use:

Only features that are currently rendered are included.

Similar language would clearly communicating when it is safe to call this method and what one can expect in the response. i.e returned output should match whats' rendered at that time.

Interesting, to me this is way clearer. "When a features is rendered" requires implicit knowledge about events, that everything is only guaranteed to finish rendering on `idle, but this tells you directly in the comment.

asheemmamoowala · 2021-04-22T00:33:08Z

src/ui/map.js

+    queryTerrainElevation(point: PointLike): number | null {
+        if (!this.transform.elevation) return null;
+
+        const result = this.transform.elevation.pointCoordinate(Point.convert(point));


The elevation.pointCoordinate method mentions that calling it can cause a stall. How would that affect custom layer rendering and would it be able to read the data for the current frame being rendered?

mapbox-gl-js/src/terrain/elevation.js

Lines 143 to 144 in 798d073

* and sampling depth from the custom depth buffer. This function (currently) introduces

* a potential stall (few frames) due to it reading pixel information from the gpu.

That comment is outdated bcoz we don't use gpu readback anymore, it uses the DEMTree to raycast on CPU.

karimnaaji

Offline discussion: removed project3d/unproject3d to put it in another PR to get changes related to exaggeration option in queryTerrainElevation merged.

Change looks good to me!

Arindam Bose added 3 commits April 20, 2021 18:58

Add isUnderneath

b21b3d1

Add unit tests

c28b373

Fix lint error

0816ff4

arindam1993 added the feature 🍏 label Apr 21, 2021

arindam1993 requested a review from karimnaaji April 21, 2021 02:18

astojilj reviewed Apr 21, 2021

View reviewed changes

karimnaaji reviewed Apr 21, 2021

View reviewed changes

arindam1993 commented Apr 21, 2021

View reviewed changes

ryanhamley added this to the v2.3 milestone Apr 21, 2021

karimnaaji reviewed Apr 22, 2021

View reviewed changes

Arindam Bose added 2 commits April 22, 2021 16:43

Remove querying of terrain elevation on the screen

5b23cc9

Fix lint error

ea01502

Arindam Bose added 2 commits April 22, 2021 17:07

rename to overlaps

861e6d9

Rename method in debug page

db68c4a

karimnaaji reviewed Apr 23, 2021

View reviewed changes

asheemmamoowala reviewed Apr 23, 2021

View reviewed changes

Arindam Bose added 2 commits April 26, 2021 15:33

Fix some nits

035d46c

unproject3d/project3d and new LngLatElevation type

89bc7a7

arindam1993 changed the title ~~Add map.queryElevation(point) and map.isUnderneath(point)~~ unproject3d/project3d and queryTerrainElevation May 3, 2021

Arindam Bose added 4 commits May 3, 2021 13:53

Remove unused unit test

c310da4

Fix lint and test errors

43c87aa

Update docs to account for TerrainQueryOptions

b0ee201

Remove projec3d/unproject3d

2cfbfbc

karimnaaji approved these changes May 5, 2021

View reviewed changes

karimnaaji changed the title ~~unproject3d/project3d and queryTerrainElevation~~ Add queryTerrainElevation exaggeration option May 5, 2021

karimnaaji merged commit 954c2a0 into main May 5, 2021

karimnaaji deleted the get-underneath-api branch May 5, 2021 00:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add queryTerrainElevation exaggeration option #10602

Add queryTerrainElevation exaggeration option #10602

arindam1993 commented Apr 21, 2021 •

edited

Loading

astojilj Apr 21, 2021 •

edited

Loading

arindam1993 Apr 21, 2021 •

edited

Loading

astojilj Apr 23, 2021

karimnaaji Apr 21, 2021

arindam1993 Apr 21, 2021

karimnaaji Apr 21, 2021

arindam1993 Apr 21, 2021 •

edited

Loading

karimnaaji Apr 22, 2021

arindam1993 Apr 23, 2021

arindam1993 Apr 23, 2021

karimnaaji Apr 23, 2021 •

edited

Loading

asheemmamoowala Apr 23, 2021

astojilj Apr 23, 2021 •

edited

Loading

karimnaaji Apr 21, 2021

arindam1993 Apr 21, 2021

arindam1993 left a comment

karimnaaji left a comment •

edited

Loading

arindam1993 commented Apr 23, 2021

karimnaaji Apr 23, 2021

karimnaaji Apr 23, 2021

karimnaaji Apr 23, 2021

asheemmamoowala Apr 22, 2021

arindam1993 Apr 23, 2021

asheemmamoowala Apr 22, 2021

arindam1993 Apr 23, 2021

karimnaaji left a comment

	* and sampling depth from the custom depth buffer. This function (currently) introduces
	* a potential stall (few frames) due to it reading pixel information from the gpu.

Add queryTerrainElevation exaggeration option #10602

Add queryTerrainElevation exaggeration option #10602

Conversation

arindam1993 commented Apr 21, 2021 • edited Loading

Launch Checklist

astojilj Apr 21, 2021 • edited Loading

Choose a reason for hiding this comment

arindam1993 Apr 21, 2021 • 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

arindam1993 Apr 21, 2021 • 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

karimnaaji Apr 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

astojilj Apr 23, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arindam1993 left a comment

Choose a reason for hiding this comment

karimnaaji left a comment • edited Loading

Choose a reason for hiding this comment

arindam1993 commented Apr 23, 2021

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

karimnaaji left a comment

Choose a reason for hiding this comment

arindam1993 commented Apr 21, 2021 •

edited

Loading

astojilj Apr 21, 2021 •

edited

Loading

arindam1993 Apr 21, 2021 •

edited

Loading

arindam1993 Apr 21, 2021 •

edited

Loading

karimnaaji Apr 23, 2021 •

edited

Loading

astojilj Apr 23, 2021 •

edited

Loading

karimnaaji left a comment •

edited

Loading