Clamp latitude value to valid Web Mercator range when projecting to y #6918

ryanhamley · 2018-07-05T17:49:39Z

Launch Checklist

briefly describe the changes in this PR
- clamps latitude values to valid Web Mercator range
- handles large, wrapped longitude values by reducing them to a +-180 range and avoiding world spin animation
write tests for all new functionality
manually test the debug page

hyperknot · 2018-07-05T18:12:14Z

That should take care of latitude (as long as clamp works correctly with Infnity as well).

Longitude on the other hand is tricky:

It should support wrapped bounds
Yet, if a too big lon value is given, it starts a crazy world rotating animation.
Like:

fitBounds([0,-40,10000,40])

Actually, fitBounds([0,-40,10000,40]) is wrong for an other reason: it zooms out the map, thus breaking the lat constraints. I believe it should do a mod 360 for exampe.

My idea is that for longitude, there should be some kind of reasonable clamping so that Infinity works, as well as a mod 360 operation.

asheemmamoowala · 2018-07-05T19:03:58Z

src/geo/transform.js

@@ -295,6 +295,12 @@ class Transform {
     * @returns {number} pixel coordinate
     */
    latY(lat: number) {
+        const validLatitude = 85.05113;


Declare as static constant outside this method, maybe in util.js or on LngLat

Actually, there is this.latRange = [-85.05113, 85.05113]; in the same file in an other place as well, as well as a couple of -85.0511 values in style-spec/reference/v8.json.

Good catch. I'll refactor these references to a single place.

You probably cannot refactor the json one, but maybe it's a good idea to have the same precision level in all 3 places (that is, probably stick with 85.0511)

I don't think it should be declared in LngLat because the way I understand it is that the LngLat is based around EPSG84 coordinates which are valid up to +-90° so the issue is more in projecting from those coordinates to valid Web Mercator ones (EDIT: well in projecting from y to to Web Mercator lat anyway). I'm leaning towards using the latRange property in Transform.

Using latRange would prevent general transform/projection calls from using otherwise valid calls with LatLng outside the bounds specified in Map#setMaxBound. Im not sure that would be clear/expected.

Ahhhhh, yeah I was misunderstanding what latRange was. Thanks for the clarification.

mourner · 2018-07-05T21:14:57Z

I recently tried to tackle the same longitude issue described by @hyperknot in Supercluster — perhaps you could reuse some of that logic: https://github.com/mapbox/supercluster/blob/master/index.js#L72-L84

hyperknot · 2018-07-06T00:09:05Z

Ah, I looked into this, the silly negative modulo of JS really makes it complicated. Picking @mourner's solution verbatim is probably the best idea.

(BTW, why the 180 special case? This way 180 maps to 180 and 540 maps to -180. Was the original aim to make min between -180 .. 179.99 and max between -179.99 .. 180?)

hyperknot · 2018-07-06T14:35:33Z

src/geo/transform.js

@@ -295,6 +297,9 @@ class Transform {
     * @returns {number} pixel coordinate
     */
    latY(lat: number) {
+        if (lat > this.maxValidLatitude || lat < this.maxValidLatitude * -1) {


Is there any point in using an if here? Why not just call clamp directly?

That's probably fine. Originally, I had a console warning inside the if statement but I think we've decided to favor documentation over a warning so this can be removed.

hyperknot · 2018-07-26T21:43:34Z

Is there anything blocking this? I thought copying @mourner's snippet from supercluster would solve longitude. We just need to fix the 540 bug there (mapbox/supercluster#96).

ryanhamley · 2018-07-26T21:56:07Z

No, I just have been pushing to get some other stuff finished and this was put on the back burner. I can finish up this PR for just the latitude fix if you're interested in putting together a PR for the longitude fix separately @hyperknot. Otherwise, it will probably be next week before I finish this.

hyperknot · 2018-07-26T21:58:19Z

I'm happy to do a PR for lon fix from the supercluster snippet.

ryanhamley · 2018-08-02T00:05:25Z

@mourner Can you take a look at my latest implementation, particularly the change to transform#lngX? I copied over your math from the Supercluster code you linked to earlier. Is that all that's needed to properly handle large longitude values? It looks as if it's working correctly just manually debugging but I'm not sure if there's something I'm not thinking of.

hyperknot · 2018-08-02T00:07:54Z

@ryanhamley I think @mourner's code is having two cases to make sure that -180 gets mapped to -180 when it's the min side and and +180 to +180 when it's the max side.

mourner

I'm now hesitating about the longitude wrapping — it can bring some unintended consequences. Maybe let's leave that for a different PR?

mourner · 2018-08-03T13:32:21Z

src/geo/transform.js


        this._renderWorldCopies = renderWorldCopies === undefined ? true : renderWorldCopies;
        this._minZoom = minZoom || 0;
        this._maxZoom = maxZoom || 22;

-        this.setMaxBounds();
+        this.latRange = [this.maxValidLatitude * -1, this.maxValidLatitude];


Let's move this back to setMaxBounds to make sure the behavior is consistent (and setMaxBounds(null) resets the map to the state it was by default).

Also nit: -this.maxValidLatitude is more readable than this.maxValidLatitude * -1.

ryanhamley · 2018-08-03T18:26:17Z

I'm now hesitating about the longitude wrapping — it can bring some unintended consequences. Maybe let's leave that for a different PR?

@mourner Agreed. I'm reverting that change and will just close out the original latitude clamping issue with this PR. I think there's potential to combine longitude wrapping with #6985 in a single PR.

ryanhamley · 2018-08-03T19:22:32Z

@mourner I think this PR is good to go from my standpoint if you can look it over and give it a 👍 if it looks good.

mourner · 2018-08-03T20:46:11Z

test/unit/ui/camera.test.js

@@ -503,7 +503,7 @@ test('camera', (t) => {
            const camera = createCamera({ zoom: 0 });
            camera.rotateTo(90, { around: [5, 0], duration: 0 });
            t.equal(camera.getBearing(), 90);
-            t.deepEqual(fixedLngLat(camera.getCenter()), fixedLngLat({ lng: 4.999999999999972, lat: 0.000014144426558004852 }));
+            t.deepEqual(fixedLngLat(camera.getCenter()), fixedLngLat({ lng: 4.999999999999972, lat: 0.000002552471840999715 }));


Quick question — why is value here different? Latitude is close to 0 so isn't supposed to be affected by clamping here...

This is caused by a slight change in the maxValidLatitude value from 85.05113 to 85.051129. Should I revert to the older value?

… coordinate

ryanhamley added the under development 🚧 label Jul 5, 2018

asheemmamoowala reviewed Jul 5, 2018

View reviewed changes

ryanhamley changed the title ~~Clamp latitude value to valid Web Mercator range when projection to y…~~ Clamp latitude value to valid Web Mercator range when projecting to y Jul 5, 2018

hyperknot reviewed Jul 6, 2018

View reviewed changes

ryanhamley mentioned this pull request Jul 18, 2018

How to constrain the map to a region spanning the 180/-180 longitude? #6985

Closed

ryanhamley force-pushed the 6906-clamp-latitude-projections branch 2 times, most recently from e477c1c to eb58382 Compare August 1, 2018 20:26

mourner reviewed Aug 3, 2018

View reviewed changes

mourner approved these changes Aug 7, 2018

View reviewed changes

ryanhamley removed the under development 🚧 label Aug 7, 2018

Clamp latitude value to valid Web Mercator range when projection to y…

cfd24b6

… coordinate

ryanhamley force-pushed the 6906-clamp-latitude-projections branch from fb1de16 to cfd24b6 Compare August 7, 2018 17:32

ryanhamley merged commit cfd24b6 into master Aug 7, 2018

ryanhamley deleted the 6906-clamp-latitude-projections branch August 7, 2018 17:45

ryanhamley mentioned this pull request Sep 21, 2018

Add API for allowing the map to zoom out to z0 #7326

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clamp latitude value to valid Web Mercator range when projecting to y #6918

Clamp latitude value to valid Web Mercator range when projecting to y #6918

ryanhamley commented Jul 5, 2018 •

edited

Loading

hyperknot commented Jul 5, 2018

asheemmamoowala Jul 5, 2018

hyperknot Jul 5, 2018 •

edited

Loading

ryanhamley Jul 5, 2018

hyperknot Jul 5, 2018 •

edited

Loading

ryanhamley Jul 5, 2018 •

edited

Loading

asheemmamoowala Jul 5, 2018

ryanhamley Jul 5, 2018

mourner commented Jul 5, 2018

hyperknot commented Jul 6, 2018

hyperknot Jul 6, 2018

ryanhamley Jul 6, 2018

hyperknot commented Jul 26, 2018 •

edited

Loading

ryanhamley commented Jul 26, 2018

hyperknot commented Jul 26, 2018 •

edited

Loading

ryanhamley commented Aug 2, 2018

hyperknot commented Aug 2, 2018

mourner left a comment

mourner Aug 3, 2018

ryanhamley commented Aug 3, 2018

ryanhamley commented Aug 3, 2018

mourner Aug 3, 2018

ryanhamley Aug 3, 2018

Clamp latitude value to valid Web Mercator range when projecting to y #6918

Clamp latitude value to valid Web Mercator range when projecting to y #6918

Conversation

ryanhamley commented Jul 5, 2018 • edited Loading

Launch Checklist

hyperknot commented Jul 5, 2018

Choose a reason for hiding this comment

hyperknot Jul 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hyperknot Jul 5, 2018 • edited Loading

Choose a reason for hiding this comment

ryanhamley Jul 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mourner commented Jul 5, 2018

hyperknot commented Jul 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hyperknot commented Jul 26, 2018 • edited Loading

ryanhamley commented Jul 26, 2018

hyperknot commented Jul 26, 2018 • edited Loading

ryanhamley commented Aug 2, 2018

hyperknot commented Aug 2, 2018

mourner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanhamley commented Aug 3, 2018

ryanhamley commented Aug 3, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ryanhamley commented Jul 5, 2018 •

edited

Loading

hyperknot Jul 5, 2018 •

edited

Loading

hyperknot Jul 5, 2018 •

edited

Loading

ryanhamley Jul 5, 2018 •

edited

Loading

hyperknot commented Jul 26, 2018 •

edited

Loading

hyperknot commented Jul 26, 2018 •

edited

Loading