-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Fix to map jumping when bounds cross longitude 180 #10903
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Still need to work through the behavior more carefully. Submitting partial comments from a not-yet-finished review.
src/geo/transform.js
Outdated
@@ -887,17 +887,20 @@ class Transform { | |||
zoomScale(zoom: number) { return Math.pow(2, zoom); } | |||
scaleZoom(scale: number) { return Math.log(scale) / Math.LN2; } | |||
|
|||
// World coordinates from LngLat |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just an idea: It doesn't transfer directly, but when I added comments to explain numerical units, I added ranges since that's the easiest way for me to immediately understand the meaning:
mapbox-gl-js/src/geo/transform.js
Lines 82 to 88 in e0d6a56
// Transform from screen coordinates to GL NDC, [0, w] x [h, 0] --> [-1, 1] x [-1, 1] | |
// Roughly speaking, applies pixelsToGLUnits scaling with a translation | |
glCoordMatrix: Float32Array; | |
// Inverse of glCoordMatrix, from NDC to screen coordinates, [-1, 1] x [-1, 1] --> [0, w] x [h, 0] | |
labelPlaneMatrix: Float32Array; | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great idea!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How's this?
// World coordinates from LngLat | |
// Transform from LngLat to world coordinates [-180, 180] x [90, -90] --> [0, this.worldSize] x [0, this.worldSize] |
transform.lngRange = [160, 190]; | ||
transform.latRange = [-55, -23]; | ||
|
||
transform.center = new LngLat(-170, -40); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where does -170 come from? I'd expect (160 + 190) / 2
--> 175. -170 is equivalent to 190, so is it using the east bound as the center?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this test sets the center to the east bound in order to test that the map position snaps to the correct side. The exact position is arbitrary, it could be any position out of bounds on the east side of the map.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks really good so far and seems to work well when I test it out! Nice work! 👏 👏 I had just a couple nitpicks as well as a question about what looks to me like possible use of uninitialized variables.
Co-authored-by: Ricky Reusser <rreusser@users.noreply.github.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification. I'm content here! I wonder if it really needs the long-term debug page, but no particular issue with it either.
Launch Checklist
Closes #10447.
Closes #6985.
These two issues arise from the same cause: when maxBounds crossed the 180th Meridian, the view would be adjusted incorrectly, snapping to the other edge of the map and preventing the user from crossing the Meridian in either direction.
This PR provides a fix by wrapping and normalizing the bounds and position before applying the update.
To allow maps across the 180th meridian to work with correct (wrapped) longitude, I added a check in setMaxBounds to unwrap them.
Two minor issues unsolved by this fix:
These both happen because the map center is now "wrapped" before checking maxBounds. This prevents issues with the 180th meridian, but it also means that the map position has no reference for where it is on the map separate from its world position.
@mapbox/gl-native
if this PR includes shader changes or needs a native portmapbox-gl-js
changelog:<changelog>'maxBounds' property across the 180th meridian work correctly.</changelog>