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 to map jumping when bounds cross longitude 180 #10903

Merged
merged 8 commits into from Aug 13, 2021
Merged

Conversation

SnailBones
Copy link
Contributor

@SnailBones SnailBones commented Jul 30, 2021

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:

  • The bounds span 360 degrees AND The map is moved quickly against the edge so that the center passes beyond the edge. The user can effectively "force" the map to the other side. (Previously this wouldn't have happened, but the only working case was bounds from -180 to 180.)
  • Bounds spanning over 360 degrees are treated at higher zoom levels as effectively no bounds. (I think this isn't really a valid use case, and I'm not seeing any Github issues about it. If I'm wrong, I think it's still an improvement over previous/current behavior which is sudden snapping to an unrelated point on the map.)

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.

  • Fix crossing the antimeridian from west to east
  • Fix crossing the antimeridian from east to west
  • write tests for all new functionality
  • Check edge cases
  • Add support for entering bounds without unwrapping longitudes (As described in How to constrain the map to a region spanning the 180/-180 longitude? #6985)
  • post benchmark scores
  • manually test the debug page
  • tagged @mapbox/gl-native if this PR includes shader changes or needs a native port
  • add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog>'maxBounds' property across the 180th meridian work correctly.</changelog>

src/geo/transform.js Outdated Show resolved Hide resolved
@SnailBones SnailBones marked this pull request as ready for review August 1, 2021 21:36
Copy link
Contributor

@rreusser rreusser left a 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.

@@ -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
Copy link
Contributor

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:

// 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;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great idea!

Copy link
Contributor Author

@SnailBones SnailBones Aug 9, 2021

Choose a reason for hiding this comment

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

How's this?

Suggested change
// 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);
Copy link
Contributor

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?

Copy link
Contributor Author

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.

src/geo/transform.js Outdated Show resolved Hide resolved
Copy link
Contributor

@rreusser rreusser left a 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.

debug/antimeridian.html Outdated Show resolved Hide resolved
debug/antimeridian.html Outdated Show resolved Hide resolved
debug/antimeridian.html Outdated Show resolved Hide resolved
src/geo/transform.js Show resolved Hide resolved
src/geo/transform.js Show resolved Hide resolved
src/geo/transform.js Show resolved Hide resolved
Co-authored-by: Ricky Reusser <rreusser@users.noreply.github.com>
Copy link
Contributor

@rreusser rreusser left a 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.

@SnailBones SnailBones merged commit 19400a0 into main Aug 13, 2021
@SnailBones SnailBones deleted the aidan/long-180-jump branch August 13, 2021 01:01
@SnailBones SnailBones added this to the v2.4.2 milestone Sep 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
2 participants