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 abrupt jump with globe view + maxBounds after transition to mercator #12124

Merged
merged 2 commits into from
Aug 10, 2022

Conversation

mourner
Copy link
Member

@mourner mourner commented Aug 1, 2022

Quick follow-up to #12114. Make sure the constraining logic is the same throughout the zoom range for the globe view so that there are no abrupt jumps when zooming in. We don't have to be very precise here because it doesn't make sense to use maxBounds on higher zoom levels coupled with the globe view anyway (the advantage of the latter disappears).

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • manually test the debug page
  • apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'

@mourner mourner added the skip changelog Used for PRs that do not need a changelog entry label Aug 1, 2022
@mourner mourner requested a review from karimnaaji August 1, 2022 15:24
Copy link
Contributor

@karimnaaji karimnaaji left a comment

Choose a reason for hiding this comment

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

One thing to note is that we trade the jump for slightly different behaviors when switching between mercator/globe:

  • Here are the constraints with globe (correctly constraining map center), the bounds are represented by the markers:
globe.mov
  • And here with a switch back to mercator, constraining center and then viewport after a switch of projection:
globe-mercator-transition.mov

Overall, it seems preferable to introduce this fix to prevent abrupt jumps during the transition; projection switches are typically less frequent compared to crossing the transition, but this could still be confusing from a user perspective.

I'm in favor of merging but could we have a small note about the differences in the documentation? e.g. when using an explicit globe or alternate projection, we constrain only map center vs viewport otherwise, I don't believe we have a mention of that yet and seem to only refer to pan/zoom as constraints https://docs.mapbox.com/mapbox-gl-js/api/map/#map#setmaxbounds

Pan and zoom operations are constrained within these bounds. If a pan or zoom is performed that would display regions outside these bounds, the map will instead display a position and zoom level as close as possible to the operation's request while still remaining within the bounds.

Otherwise, I wonder if Mercator would benefit from adopting the same logic as well by constraining the map center instead. Even though that would be a breaking change, the current viewport constraints only really work in 2d top-down views. When starting to pitch, we can easily cross these bounds on the viewport and the constraints aren't honored. I'm not sure if that was a deliberate decision or if the addition of pitch and bearing didn't fully consider this API when first introduced.

@mourner
Copy link
Member Author

mourner commented Aug 8, 2022

Added a small note to setMaxBounds docs.

I'm not sure if that was a deliberate decision or if the addition of pitch and bearing didn't fully consider this API when first introduced.

I think we never reconsidered maxBounds behavior after pitch was introduced. I think the intent of the original behavior was to avoid empty stripes on top/bottom when zooming out an ordinary map, so that it always covers the view. Not sure whether we should change the default behavior now — either has tradeoffs, but we could explore this in a separate issue.

@mourner mourner merged commit 79af3b6 into main Aug 10, 2022
@mourner mourner deleted the globe-constrain-center-mercator branch August 10, 2022 10:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip changelog Used for PRs that do not need a changelog entry
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants