add `MercatorCoordinate` type #7488

ansis · 2018-10-26T00:44:09Z

This adds a MercatorCoordinate to expose a way to convert from from LngLat to the mercator coordinates used by custom layers.

fixes #7485

class MercatorCoordinate {
    x: number;
    y: number;
    z: number;

    constructor(x: number, y: number, z?: number) { ... }
    static fromLngLat(lngLatLike: LngLatLike, altitude?: number) { ... }
    toLngLat(){ ... }
    toAltitude() { ... }
}

Launch Checklist

briefly describe the changes in this PR
write tests for all new functionality
document any changes to public APIs
manually test the debug page

TODO

switch Transform to use MercatorCoordinate for projection
clarify units and difference from standard web mercator
fix tests on last commit

andrewharvey · 2018-10-26T03:35:15Z

src/geo/mercator_coordinate.js

+ * A `MercatorCoordinate` object represents a 3 dimensional position in projected web mercator coordinates.
+ *
+ * The "world size" used by `MercatorCoordinate` is 1, meaning `MercatorCoordinate(0, 0, 0)` is the north-west
+ * corner of the mercator world and `MercatorCoordinate(1, 1, 0)` is the south-east corner of the mercator world.


This documentation provides great clarity, I really appreciate it.

A lot of geospatial people might be confused by this definition though since the web mercator projection is in meters and has a projected bounds of

-20026376.39 -20048966.10 20026376.39 20048966.10

Since MercatorCoordinate here is a a unit vector, I'm wondering if we need to be more explicit that this is not ESPG:3857 web mercator 🤔 ?

@andrewharvey thats for flagging this! I'll have to think more closely whether we should match that or if we should just clarify the documentation.

I like the simplicity of a unit vector as the underlying representation compared to EPSG:3857, however I very much see it as an internal implementation detail.

I would think that most applications of custom layers are working with data in WGS84 LL so with these helper methods the actual implementation doesn't matter outside of GL JS internals.

I clarified this a bit in 83cda37. Do you think this is clear enough?

Also, I think the projected bounds on that website are wrong. I think it should be 20037508.

I like the simplicity of a unit vector as the underlying representation compared to EPSG:3857, however I very much see it as an internal implementation detail.

Yep, it makes conversion from vector tiles to these units and back simpler. It also allows vector tile positions to be stored without loss of precision. I think it makes sense to keep it as is.

Do you think renaming it would help? UnitMercatorCoordinate to emphasize the unitness of it? or TiledMercatorCoordinate to nod in the direction of the tiled coordinate spaces this borrows from? I think I'm leaning towards just MercatorCoordinate

I like the new documentation in 83cda37 👍

-1 to TiledMercatorCoordinate, that makes me think xyz tiles which vary by zoom level, but in fact this MercatorCoordinate doesn't vary my zoom level and isn't broken into tiles.

I like UnitMercatorCoordinate since it's more explicit that these are unit coordinates (0 to 1),

ansis · 2018-10-26T04:19:33Z

The main thing that would be useful to have reviewed so far are the first couple commits.

The last commit (8856fbf) is a big one and it replaces the previous internal Coordinate type with MercatorCoordinate. This isn't related to the main goal of exposing a coordinate type. When I started using MercatorCoordinate in Transform I realized that it would work better than Coordinate in most places we are using it. The commit is still a bit broken. I can split this into a different pr if that helps.

mourner

This looks great! I also really appreciate the clarity the PR brings because I was always confused by that Coordinate class in transform before.

mourner · 2018-10-26T09:15:49Z

src/geo/mercator_coordinate.js

+    constructor(x: number, y: number, z?: number) {
+        this.x = +x;
+        this.y = +y;
+        this.z = z === undefined ? 0 : +z;


Nit: we can use ES default parameters for things like this (below too for altitude).

mourner · 2018-10-26T09:18:43Z

src/geo/transform.js

+        const lat = clamp(lnglat.lat, -this.maxValidLatitude, this.maxValidLatitude);
+        const clamped = new LngLat(lnglat.lng, lat);
+        const coord = MercatorCoordinate.fromLngLat(clamped);
+        return new Point(coord.x * this.worldSize, coord.y * this.worldSize);


My only concern with this change is that we significantly increase the number of allocations in potentially hot code, like here — instead of a single Point allocation, we do 3 — Point, MercatorCoordinate, Point again.

Do you think this is significant enough to warrant splitting out the projection into lngToMercatorX(...) and latToMercatorY(...) for internal use?

Perhaps not, but I tend to be on the safe side :) Your call.

Does 0d4c2ca look good?

mourner · 2018-10-26T09:20:06Z

src/geo/transform.js

-            minX = this.lngX(lngRange[0]);
-            maxX = this.lngX(lngRange[1]);
+            minX = this.project(new LngLat(lngRange[0], 0)).x;
+            maxX = this.project(new LngLat(lngRange[1], 0)).x;


To add to my point above, here, instead of zero allocations, we do 4 * (1 + 3) = 16, along with double-projection.

asheemmamoowala · 2018-10-29T22:43:29Z

src/geo/mercator_coordinate.js

+ * `MercatorCoordinate(1, 1, 0)` is the south-east corner. If you are familiar with
+ * [vector tiles](https://github.com/mapbox/vector-tile-spec) it may be helpful to think
+ * of the coordinate space as the `0/0/0` tile with an extent of `1`.
+ *


Does there need to be a caveat about precision here? I.e. When using a mercator co-ordinate to place something on a custom layer, it may not match the exact location at higher zooms.

Those issues are more on the rendering side of things than the MercatorCoordinate side. We should probably add caveats and work arounds in the docs for custom layers. #7268 kind of covers this

ansis · 2018-10-30T18:27:22Z

src/geo/transform.js

-            const ubr = this.pointCoordinate(new Point(0, this.height), 0);
-            const w0 = Math.floor(Math.min(utl.column, utr.column, ubl.column, ubr.column));
-            const w1 = Math.floor(Math.max(utl.column, utr.column, ubl.column, ubr.column));
+            const utl = this.pointCoordinate(new Point(0, 0));


This existed before this pr so I think it's ok to leave for now. I think the point creations should be fine. This code only runs once per source per frame

add MercatorCoordinate type, fix #7485

33d6089

ansis requested review from mourner and asheemmamoowala October 26, 2018 00:44

ansis added 2 commits October 25, 2018 20:44

fixup

2672d01

use MercatorCoordinate for projection in Transform

050b1e5

andrewharvey reviewed Oct 26, 2018

View reviewed changes

replace Coordinate with MercatorCoordinate

8856fbf

andrewharvey mentioned this pull request Oct 26, 2018

expose mercator projection for custom layers #7485

Closed

clarify differences with epsg3857

83cda37

mourner reviewed Oct 26, 2018

View reviewed changes

ansis added 4 commits October 26, 2018 13:13

use es default values

ec9ef36

fix tests

cd3e462

seprate projection from type for internal use

0d4c2ca

fixup

f6cd838

mourner approved these changes Oct 29, 2018

View reviewed changes

fixup

b381e56

ansis merged commit cedefd6 into master Oct 29, 2018

ansis mentioned this pull request Oct 29, 2018

how to calculate the value of tanslate based on longitude and latitude. #7481

Closed

asheemmamoowala reviewed Oct 29, 2018

View reviewed changes

mourner deleted the mercator-coordinate branch August 1, 2019 12:32

vulkd mentioned this pull request Dec 12, 2019

Use same algorithm in latY #5930

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add `MercatorCoordinate` type #7488

add `MercatorCoordinate` type #7488

ansis commented Oct 26, 2018 •

edited

Loading

andrewharvey Oct 26, 2018 •

edited

Loading

ansis Oct 26, 2018

andrewharvey Oct 26, 2018

ansis Oct 26, 2018

andrewharvey Oct 26, 2018

ansis commented Oct 26, 2018

mourner left a comment

mourner Oct 26, 2018

mourner Oct 26, 2018

ansis Oct 26, 2018

mourner Oct 27, 2018

ansis Oct 29, 2018

mourner Oct 26, 2018

asheemmamoowala Oct 29, 2018

ansis Oct 30, 2018

This comment was marked as resolved.

ansis Oct 30, 2018

add MercatorCoordinate type #7488

add MercatorCoordinate type #7488

Conversation

ansis commented Oct 26, 2018 • edited Loading

Launch Checklist

TODO

andrewharvey Oct 26, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ansis commented Oct 26, 2018

mourner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

add `MercatorCoordinate` type #7488

add `MercatorCoordinate` type #7488

ansis commented Oct 26, 2018 •

edited

Loading

andrewharvey Oct 26, 2018 •

edited

Loading