mapbox / mapbox-gl-native

Interactive, thoroughly customizable maps in native Android, iOS, macOS, Node.js, and Qt applications, powered by vector tiles and OpenGL
https://mapbox.com/mobile
Other
4.36k stars 1.33k forks source link

Support query elevation data from raster DEM source. #16443

Closed ystsoi closed 4 years ago

ystsoi commented 4 years ago

I have enhanced queryRenderedFeatures to return elevation data from raster DEM source for point query.

Initially, I want to add a method to return elevation data. But after I studied queryRenderedFeatures, I thought it may not be a bad idea to just enhance it, as currently it does nothing for raster DEM source.

The implementation will return "ele" and "zoom" for a point query. "ele" is the elevation in meters. It will return the interpolated value without any rounding, and let developers decide how many decimal places to keep themselves. "zoom" is the zoom level of the tile from which the elevation is obtained. The implementation tries to return one result for a query. But when the device is offline, it seems that multiple results may be returned with different zoom levels. Developers can use the one with the highest zoom level.

This PR will affect existing Mapbox projects which do point query without any layer IDs and filters. Probably, the logic in those projects are general enough to handle this extra result.

See whether this PR is acceptable. If not, see whether it is possible to provide some other ways to retrieve the elevation data.

astojilj commented 4 years ago

@ystsoi Thank you for the PR and for documenting the approach clearly with the code. The topic is very interesting. Before we check the code in details, it is important to clarify the use case and the caveats.

  1. How frequently (how many times per render frame) would you need to access this? It is understandable if you cannot share all the details but some additional details about use case would help understanding if this is the best performing approach.
  2. How stable (zoom level invariant) the value should be? Depending on map zoom, DEM tiles of different zoom levels get loaded and different value gets returned.
  3. Does your style already use hillshade or you're adding hillshade layer mostly in order to access elevation?
  4. Raster DEM tile source is a specialization of raster tile source: do you see a need for generic raster pixel value querying, also for e.g. satellite/raster?

Considering this, queryRenderedFeatures looks like the proper way to return pixel value of rendered hillshade. For other needs, especially if there is no need for hillshade layer, https://docs.mapbox.com/help/tutorials/find-elevations-with-tilequery-api/ should work better.

ystsoi commented 4 years ago

@astojilj Thank you for your reply. As I cannot go too further before the feature can be integrated into Mapbox, so I cannot provide much concrete information.

  1. For the use case, the feature can be used for getting the elevation of a POI. I thought that the feature can also be used for generating an elevation profile for the visible part of a route, but after some tests, it seems that queryRenderedFeatures should be run in UI thread, so a complex route can block the UI. Oversight the threading issue!

  2. Right, will return different values for different zoom levels. As the precision of the elevation data is not very high by nature, I suppose that some rounding may alleviate the issue.

  3. Probably, hillshade will be used in those apps interested in elevation info. Just a choice between Terrain v2 or the RGB version. Recently, when I worked on hillshading in Mapbox, I noticed an issue in the RGB version, and I noticed that there may be way to improve both the effect and the efficiency of using the RGB version. Maybe, I will share more about this later on a separate issue.

  4. Currently, not work on satellite/raster, so no idea on these. But if the SDK can return the underlying pixel data in batch efficiently for an area rather than for just a point, encapsulated with some calculation logic, maybe, the elevation profile problem can be solved.

Tilequery API seems not providing elevation in high precision. Also, if elevation can be obtained from the map directly, then it is possible for the users to get elevation in offline state.

Should need further work.

ystsoi commented 4 years ago

https://github.com/mapbox/mapbox-gl-native/issues/16459 is the issue I have mentioned in the previous comment.

By limiting the max zoom level of the source, the hillshading effect could actually be better. This could also improve the stability of the returned elevation, decrease the download size for display or offline use.

astojilj commented 4 years ago
  1. For the use case, the feature can be used for getting the elevation of a POI. I thought that the feature can also be used for generating an elevation profile for the visible part of a route, but after some tests, it seems that queryRenderedFeatures should be run in UI thread, so a complex route can block the UI. Oversight the threading issue!

If this and other constraints are clear to developer using queryRenderedFeatures then this doesn't look like a problem.

However, it seems more constraining that for elevation profile it is required to render it and ensure that all DEM tiles get loaded and rendered so that queryRenderedFeatures could access the data. Would that still be acceptable for the implementation you're planning?

Did you reconsider if contour elevation data from Terrain V2 would be useful? https://docs.mapbox.com/vector-tiles/reference/mapbox-terrain-v2/#elevation It comes with very low precision on low zoom levels.

ystsoi commented 4 years ago

Actually, I thought about just returning all dem data for all visible tiles, and then do calculation on the app side. But as busy on some other tasks, so don't have time to work on this yet.

Maybe, this PR is still useful for elevation profile. Why not just simplify complex routes first before generating an elevation profile? It is still very good if a rough elevation profile can be provided to users in offline state. Just letting users know the various constraints, including the visibility, should be OK.

See whether this PR is good to commit. Maybe, can use this function first, and improve later if needed. Thanks.

stale[bot] commented 4 years ago

This pull request has been automatically detected as stale because it has not had recent activity and will be archived. Thank you for your contributions.