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

Fix pole geometry elevation sampling #12133

Merged
merged 5 commits into from Aug 10, 2022
Merged

Conversation

mpulkki-mapbox
Copy link
Contributor

@mpulkki-mapbox mpulkki-mapbox commented Aug 4, 2022

image

Pole geometry in globe view is rendered by extrapolating vertex, elevation and color data from the bordering tiles. A triangle fan expanding towards either of the geographic poles is created for each tile without top or bottom neighbour y == 0 || y == tiles - 1.

A bug was found where elevation of the pole geometry seems to be off when an exaggerated terrain is used with the Terrain-DEM v1 source. After some investigation I found out that the metric elevation applied to pole vertices is in incorrect coordinate space. Terrain-RGB v1 source works fine as the elevation is always (observed to be) 0 for bordering tiles which is not the case with DEM v1.

This PR fixes the following issues:

  1. Local coordinate spaces are unified by defining pole geometry vertices in the default ECEF coordinates. This simplifies the design as a single globe matrix can be shared and a specialized matrix is no longer required for rendering pole triangles.
  2. Conversion from meters to local units is fixed.
  3. Elevation vector for a tile is computed by interpolating normal vectors of tile corner points. This obviously doesn't work with pole vertices as the result is undefined for coordinates outside tile boundaries. Elevation vectors are computed directly from the vertex position.

Two new render tests are added to validate the fix.

Partly fixes #12037.

Launch Checklist

  • briefly describe the changes in this PR
  • include before/after visuals or gifs if this PR includes visual changes
  • write tests for all new functionality
  • document any changes to public APIs
  • post benchmark scores
  • manually test the debug page
  • tagged @mapbox/map-design-team @mapbox/static-apis if this PR includes style spec API or visual changes
  • tagged @mapbox/gl-native if this PR includes shader changes or needs a native port
  • 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>Fix elevation of pole geometry when exaggerated terrain is used</changelog>

@mpulkki-mapbox mpulkki-mapbox marked this pull request as ready for review August 4, 2022 10:46
@mpulkki-mapbox mpulkki-mapbox changed the title Fix pole geometry elevation sampling with exaggerated terrain Fix pole geometry elevation sampling Aug 4, 2022
@mpulkki-mapbox mpulkki-mapbox requested review from karimnaaji and akoylasar and removed request for karimnaaji August 4, 2022 11:39
const point = tr.point;
const ws = tr.worldSize;
const s = tr.worldSize / (tr.tileSize * numTiles);
const poleMatrix = calculateGlobeMatrix(tr);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we could use tr.globeMatrix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch!

@astojilj
Copy link
Contributor

astojilj commented Aug 9, 2022

Partly fixes #12037.

What's the remaining issue required to fix #12037?

@mpulkki-mapbox
Copy link
Contributor Author

mpulkki-mapbox commented Aug 9, 2022

Partly fixes #12037.

What's the remaining issue required to fix #12037?

There are two sources for holes on the globe view. This pull request fixes poles but does not implement skirts that would be required for stitching seams between adjacent tiles

@mpulkki-mapbox mpulkki-mapbox merged commit 5b9609f into main Aug 10, 2022
@mpulkki-mapbox mpulkki-mapbox deleted the mpulkki-fix-pole-elevation branch August 10, 2022 06:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

North pole cap and Greenland have holes with exaggerated extrusions
3 participants