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

Adaptive bounds calculation #12286

Merged
merged 10 commits into from
Oct 17, 2022
Merged

Adaptive bounds calculation #12286

merged 10 commits into from
Oct 17, 2022

Conversation

stepankuzmin
Copy link
Contributor

@stepankuzmin stepankuzmin commented Oct 7, 2022

This PR implements the adaptive bounds calculation proposed here #12199

Alternatively, we could do adaptive bounds calculation similar to the one happening in tile_transform.js for alternative projections — start with 4 corners, and delve recursively into midpoints while they stray too much from Euclidean midpoints. This might still produce a noticeable error, but will at least be a significant improvement to the current results. We should also be careful of the case where midpoint is in the middle but the line gets curvy on either side of the midpoint.

To calculate the globe bounds, it recursively iterates other midpoints of the canvas and raycasts the screen coordinates to the globe, expanding the bounds. It also expands the bounds if poles are visible.

Before

Screen Shot 2022-10-13 at 15 57 55-before

After

Screen Shot 2022-10-13 at 15 58 15-after

Before

Screen Shot 2022-10-17 at 14 12 10

After

Screen Shot 2022-10-17 at 14 12 25

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
  • 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>Improve the globe bounds calculation</changelog>

@stepankuzmin stepankuzmin added the skip changelog Used for PRs that do not need a changelog entry label Oct 7, 2022
@stepankuzmin stepankuzmin added bug 🐞 and removed skip changelog Used for PRs that do not need a changelog entry labels Oct 13, 2022
@stepankuzmin stepankuzmin marked this pull request as ready for review October 13, 2022 13:12
@stepankuzmin stepankuzmin requested a review from a team as a code owner October 13, 2022 13:12
Copy link
Contributor

@SnailBones SnailBones left a comment

Choose a reason for hiding this comment

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

LGTM, nice work @stepankuzmin! Added one nit about testing coverage.

test/unit/ui/map.test.js Show resolved Hide resolved
Copy link
Member

@mourner mourner left a comment

Choose a reason for hiding this comment

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

Nice work!

src/geo/transform.js Outdated Show resolved Hide resolved
src/geo/transform.js Show resolved Hide resolved
stepankuzmin and others added 2 commits October 14, 2022 11:07
Co-authored-by: Volodymyr Agafonkin <agafonkin@gmail.com>
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.

3 participants