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

Add poles support in getBounds on the Globe #12315

Merged
merged 13 commits into from
Oct 24, 2022
Merged

Conversation

stepankuzmin
Copy link
Contributor

@stepankuzmin stepankuzmin commented Oct 18, 2022

This PR adds the pole support in getBounds on the Globe. It adds a new util function polesInViewport, that detects if the poles are visible in the current viewport by intersecting the location with the viewport in screen coordinates and checking if they are behind the globe surface.

Previously the poles support was removed in #12318

Before

Screen Shot 2022-10-21 at 18 13 10

After

Screen Shot 2022-10-21 at 18 12 31

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 poles support in getBounds when projection is set to globe</changelog>

@stepankuzmin stepankuzmin changed the title Fix poles support in getBounds on the Globe Add poles support in getBounds on the Globe Oct 20, 2022
@stepankuzmin stepankuzmin marked this pull request as ready for review October 21, 2022 15:14
@stepankuzmin stepankuzmin requested a review from a team as a code owner October 21, 2022 15:14
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.

Nice work Stepan! Just a few nits.

test/unit/ui/map.test.js Outdated Show resolved Hide resolved
src/geo/transform.js Outdated Show resolved Hide resolved
src/geo/transform.js Outdated Show resolved Hide resolved
src/geo/projection/globe_util.js Outdated Show resolved Hide resolved
src/geo/transform.js Outdated Show resolved Hide resolved
@stepankuzmin stepankuzmin merged commit 8502e7d into main Oct 24, 2022
@stepankuzmin stepankuzmin deleted the fix-globe-getBounds-poles branch October 24, 2022 12:26
@karimnaaji
Copy link
Contributor

@SnailBones @stepankuzmin Should we cherry-pick that fix in the beta so it's available in final?

@SnailBones
Copy link
Contributor

SnailBones commented Oct 24, 2022

@karimnaaji If we're doing a second beta anyway then absolutely! If we're releasing metrics in 2.12 soon, I think this could probably wait until then. I doubt it's worth the effort to go through an extra release just for this improvement

Though I'm open to different opinions if this is more critical than I think. @stepankuzmin

EDT: Misunderstood @karimnaaji`s proposal, I think this is low-risk enough that we could release it directly into the final without another beta. Though I'd be curious to your thoughts @stepankuzmin

@stepankuzmin
Copy link
Contributor Author

stepankuzmin commented Oct 25, 2022

This should be a low risk to push it into the final release, but this might affect the existing users with the globe enabled. So we might play safe and ship it into beta to see if there is any feedback on the new behavior. Anywise, I'm okay to push it to the final release @karimnaaji @SnailBones

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