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

Return above-sea-level elevation from queryTerrainElevation + Pass lowered mercator matrix to custom layer #3854

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

sbachinin
Copy link
Collaborator

This fixes #3736 without introducing breaking changes.
I think it would also be "the proper way" in terms of positioning custom layer objects.

Actually it creates changes to public API.
But I'll argue that they are non-breaking in a sense that all existing user-written code will work as before.

  • Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
  • Briefly describe the changes in this PR.
  • Link to related issues.
  • 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.
  • Add an entry to CHANGELOG.md under the ## main section.

Solution is to

  1. return above-sea-level elevation from queryTerrainElevation (instead of returning this strange offset between the elevation of a given point and elevation of centerPoint).
  2. pass a "lowered" mercator matrix (instead of just mercator matrix) to render() method of a custom layer. "Lowered" means that mercator matrix is translated by the same Z distance as proj matrix. And this allows to use intuitive above-sea-level elevation to position custom layer objects.

These 2 changes together ensure that:

  • user who expects queryTerrainElevation to return the actual above-sea-level elevation will get what he needs.
  • user who uses queryTerrainElevation to translate his custom layer objects will get the same results without changing the existing code.

Technically this PR changes the public API (queryTerrainElevation returns a different number + different matrix passed to render) but I believe that these 2 changes compensate each other, with all existing code working as before.

That is, there is no change in how these values can be used.

So far the only sensible way to use them was to translate a given matrix by "x" (whatever is returned from queryTerrainElevation). From the user's POV, this "x" in itself was a meaningless number (some cryptic offset). And the Z component of the matrix (based on the elevation of the centerPoint) is meaningless too.

So it seems very unlikely that anyone uses them for anything other than translation of Z by x.
And this translation doesn't change, only now these values make more sense, "x" being just above-sea-level elevation. And we no longer need to explain this weird offset .

If this approach looks ok, I'll try add some tests about custom layer rendering.

@codecov-commenter
Copy link

codecov-commenter commented Mar 18, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.53%. Comparing base (44b582e) to head (8c02251).
Report is 4 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3854      +/-   ##
==========================================
- Coverage   86.83%   86.53%   -0.30%     
==========================================
  Files         241      241              
  Lines       32341    32342       +1     
  Branches     1962     2172     +210     
==========================================
- Hits        28082    27986      -96     
- Misses       3340     3408      +68     
- Partials      919      948      +29     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@HarelM
Copy link
Member

HarelM commented Mar 19, 2024

I would prefer to merge this when we work on 5.x just to be on the safe side.
I agree that the public API didn't change in terms of breaking the code, but the behavior changed and we can't know for sure how people are using this API call...

@olsen232
Copy link
Contributor

FYI this fixes #3797
Or to put this another way #3736 and #3797 are duplicates or at least are two sides of the same coin

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

map.queryTerrainElevation not returning expected values
4 participants