Projections public API #11002

ryanhamley · 2021-09-10T01:08:19Z

Launch Checklist

briefly describe the changes in this PR
- implements a public API for projections with style spec changes and get/setProjections methods
write tests for all new functionality
document any changes to public APIs
post benchmark scores
manually test the debug page
tagged @mapbox/map-design-team @mapbox/static-apis if this PR includes style spec API or visual changes
- @mapbox/map-design-team for a review of the api
apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog></changelog>

TODO:

On initial change, the new projected tiles retain a "shadow" of the previous tiles for some reason

Add render and unit tests
Add some documentation to the public API
Fix Flow

src/style-spec/validate/validate_projection.js

src/geo/transform.js

src/style/style.js

karimnaaji · 2021-09-16T16:16:30Z

cc @kiryldz

tristen · 2021-10-11T18:40:28Z

src/style-spec/reference/v8.json

+        }
+      }
+    },
+    "center": {


@ryanhamley is there some sort of requires array (i.e.

mapbox-gl-js/src/style-spec/reference/v8.json

Lines 956 to 960 in 7743fbf

"requires": [

{

"line-join": "miter"

}

],

) that we could add for both center and parallels to mark albers or lambert as required?

Technically, center and parallels are never required. If they aren't supplied by the user, the defaults will be used.

I understand that. It's that center and parallels require name to equal specific values for custom values to work. Specifying this in the spec helps a tool like Studio understand that.

Ah I misunderstood your question. Yes, this makes sense and I will update the spec.

On that topic, since center and parallels need a conical projection and are specific to it (Albers/Lambert), have we considered reflecting that in the name structure of it? e.g. conical-center or conic-center, depending on what's most appropriate.

It makes it more clear that it's unused for non-conical projections, and allows other projection specific spec to be introduced.

center is currently used for conic projections but could be eventually supported by non-conic projections.

parallels would only be for conic projections I think. Just parallels seems clear enough to me but I'm open to conic-parallels if others think it's clearer. d3's parameter is named just parallels. proj strings use lat_1 and lat_2.

center is currently used for conic projections but could be eventually supported by non-conic projections.

Ok makes sense to leave center as is then.

tristen · 2021-10-11T19:13:44Z

src/style-spec/reference/v8.json

@@ -3862,6 +3871,52 @@
      }
    }
  },
+  "projection": {
+    "name": {
+      "type": "string",


Any reason why this isn't an enum?

I think we want the API to be as flexible as possible. We may make it possible in future versions of this API for developers to supply their own projections, in which case an enum wouldn't work.

Making this enum allows Studio to pull a list of known values from the spec rather than asking the user to get the string right or hard coding something in. This is also how current properties that require specific values are shaped.

tristen · 2021-10-11T19:15:34Z

src/style-spec/reference/v8.json

+  "projection": {
+    "name": {
+      "type": "string",
+      "doc": "The name of the projection to be used for rendering the map. Available projections are Albers Alaska ('alaska'), Albers USA ('albers'), Equal Earth ('equal-earth'), Equirectangular/WGS84 ('equirectangular'), Globe ('globe'), Lambert ('lambert'), Mercator ('mercator'), Natural Earth ('natural-earth'), and Winkel Tripel ('winkel').",


It would be nice to remove the "Available projections" documented here into a values object for type: enum.

Also the names here changed since the initial PR state.

test/unit/ui/map.test.js

mourner

This looks good overall! The only doubt I have is around getProjectionOptions and related logic like handling string vs object — it seems like it's spread or repeated around across many places (setProjection in three different classes, both inside the method and also outside before calling it, etc.), so the flow gets confusing. It's hard to pinpoint exactly, but ideally each part of that logic would only happen at one place.

mourner · 2021-10-13T16:33:23Z

debug/projections.html

@@ -43,6 +43,7 @@
        <fieldset>
            <label>Projection:</label>
            <select id="projName">
+                <option value="albers" selected>Albers USA</option>
                <option value="alaska">Albers Alaska</option>
                <option value="albers" selected>Albers USA</option>


Nit: duplicate albers

src/style-spec/diff.js

mourner · 2021-10-13T16:37:14Z

src/style-spec/reference/v8.json

+  "projection": {
+    "name": {
+      "type": "string",
+      "doc": "The name of the projection to be used for rendering the map. Available projections are Albers Alaska ('alaska'), Albers USA ('albers'), Equal Earth ('equal-earth'), Equirectangular/WGS84 ('equirectangular'), Globe ('globe'), Lambert ('lambert'), Mercator ('mercator'), Natural Earth ('natural-earth'), and Winkel Tripel ('winkel').",


Also the names here changed since the initial PR state.

mourner · 2021-10-13T16:37:41Z

src/style-spec/reference/v8.json

+      "required": true,
+      "sdk-support": {
+        "basic functionality": {
+          "js": "2.5.0"


Suggested change

"js": "2.5.0"

"js": "2.6.0"

src/geo/transform.js

karimnaaji · 2021-10-13T17:32:29Z

src/style-spec/reference/v8.json

@@ -94,6 +94,15 @@
        "delay": 0
      }
    },
+    "projection": {
+      "type": "projection",
+      "doc": "The projection the map should be rendered in. Possible projections are Albers USA, Albers Alaska, Equal Earth, Equirectangular (WGS84), Globe, Lambert, Mercator, Natural Earth, and Winkel Tripel.",


Should Globe be removed in this first phase? I think it's also referenced further down.

Yep, my incoming commit that updates the style spec also removes references to globe. I was guessing at what would be included when I first wrote this out.

karimnaaji · 2021-10-13T17:33:18Z

src/style-spec/reference/v8.json

+        "center": [-154, 50],
+        "parallels": [55, 65]


We don't have a spec for globe view yet, but it seems like it could be a fit under the projection namespace. I've got a few questions:

~~Are these properties (center/parallel) shared between all current projections?~~ Addressed by other comment

When we introduce globe and if we find that it's a good fit under the projection namespace, could we extend this spec so that these two properties are optional?

Is there room to eventually have globe or projection specific properties under the projection namespace? (e.g. globe-*, for any specific visual styling or tuning of the globe)

Is there room to eventually have globe or projection specific properties under the projection namespace? (e.g. globe-*, for any specific visual styling or tuning of the globe)

Yes, I think so

When we introduce globe and if we find that it's a good fit under the projection namespace, could we extend this spec so that these two properties are optional?

They should be optional now

src/ui/map.js

src/style-spec/reference/v8.json

karimnaaji · 2021-10-13T18:05:16Z

src/style-spec/reference/v8.json

+        }
+      }
+    },
+    "center": {


On that topic, since center and parallels need a conical projection and are specific to it (Albers/Lambert), have we considered reflecting that in the name structure of it? e.g. conical-center or conic-center, depending on what's most appropriate.

It makes it more clear that it's unused for non-conical projections, and allows other projection specific spec to be introduced.

src/ui/map.js

karimnaaji · 2021-10-13T20:02:38Z

src/ui/map.js

 * @example
 * var map = new mapboxgl.Map({
 *   container: 'map',
 *   center: [-122.420679, 37.772537],
 *   zoom: 13,
 *   style: style_object,
 *   hash: true,
- *   projection: 'winkel',
+ *   projection: {


Is it strictly necessary to have projection as part of the map constructor? Removing it could simplify the logic wrt _projectionSetAtRuntime.

For consistency I'd expect projection/terrain/fog to have very similar behaviors/usage as they are all root properties. What's surprising may be that we have a projection in the map constructor but no terrain or fog in it, so we wouldn't have completeness here.

Is there any issue with setting the map projection after the style has loaded?

After looking at all the parameters, it seems like this would be the first style specification introduced as option to the map constructor.

The way it's implemented right now a projection provided by the style is used differently than terrain provided by the style. It interprets the projection in the style more as a default than as a fixed value. If a projection has already been set at runtime, it will ignore the projection in the style. This is similar to how zoom/center/bearing can be set in a style. They are only used if nothing else is set. zoom/center/etc can be set in the map constructor.

Conceptually, projections are kind of a map setting whose default can be specified in a style.

Fog and terrain are completely a part of the style. Projections and camera settings are part of how the style is used.

This all is somewhat vague and a bit unsure. We should discuss this more before releasing.

karimnaaji

LGTM, we can continue some review details on the larger PR once this one is merged.

It gets treated the same as if it were never set. Projections from a stylesheet will override the default.

* rough projection support * projections stencil clipping and refactor (#410) * Enable stencil clipping for line and fill layers * Use buffers from tile * Refactor tile bounds buffers * Rename things to not exclusively be RasterBounds * Create projections directory * More refactoring * Combine matrix calculations * Cleanup * Refactor projections to new folder * Begin debug work * Tile boundaries are working * Refactor indexbuffer and segmentvector to per painter * merge projectx and projecty functions * nits Co-authored-by: Ansis Brammanis <ansis@mapbox.com> * Projections fix location issues (#414) * Add debug page * Add projection option * Wire up projection code with worker * Rename projections folder to projection * Refactor and update free camera * Add Projection type and fix center calculations * Fix bug with transform._center * Make Winkel projection noop for now * Update demo HTML and CSS * temp remove undistortion Co-authored-by: Ansis Brammanis <ansis@mapbox.com> * [projections] Adaptive geometry resampling for alternative projections (#10753) * implement adaptive resampling of reprojected geometry * address feedback * Refactor projections code to get all tests passing (#10732) * [projections] Simplify and optimize tile transform code (#10780) * simplify projections tile transform * skip resampling for mercator * [projections] Fix performance regression in draw_background (#10747) * separate tiles for background layers * additional lint & flow fixes * try fixing tests * try fixing render tests Co-authored-by: Ansis Brammanis <ansis@mapbox.com> * Pin chrome to version 91 (#10887) (#10896) * Pin to chrome version 91 * Pin chrome version for test-browser Co-authored-by: Arindam Bose <arindam.bose@mapbox.com> * Refactor raw projections, handle projection options (#10913) * refactor raw projections, handle projection options * add unprojection to winkel tripel * fix flow * Pin chrome to version 91 (#10887) * Pin to chrome version 91 * Pin chrome version for test-browser * fix lint * remove to superfluous sin calls Co-authored-by: Arindam Bose <arindam.bose@mapbox.com> * Fix bearing for non-mercator projections (#10781) * Use adaptive resampling with MARTINI & Earcut for non-Mercator tiles (#10980) * use adaptive resampling and earcut for non-Mercator tile bounds * fix unit test * use adaptive MARTINI mesh for non-Mercator raster tiles * Clamp unproject to valid geo range in alternate projections (#10992) * clamp unproject to mercator bounds in all projections * fix marker test * avoid wrapping center for non-Mercator projections * extend alt projections clamping to full lat range * correct zoom, bearing and shear for projections (#10976) * fix zoom, bearing and skew for projections * refactor adjustments * lint * add comments * Fix circle and heatmap on alternate projections (#11074) * fix circle & heatmap on alternate projections (blunder) * fix unit test * fix pitch, line-width and other properties for projections (#11080) and: - fix fill-extrusions - remove global projection variable to allow multiple maps on one page - avoid recalculating tileTransform * Add Equal Earth, Natural Earth and Lambert Conformal Conic projections (#11091) * Fix constraining logic for alternate projections (#11092) * adaptive bbox for projections, refactor resampling * better precision for adaptive bounds * remove leftover * fix zoom/shear adjustments near poles * optimize tile transform * fix lint * attempt to fix tests * simplify, clarify and consolidate constraining logic * minor renames in transform * safer clamping for zoom adjustments * Projections public API (#11002) Co-authored-by: Ansis Brammanis <ansis@mapbox.com> * fix conflicts * fix seams around alternate-projected tiles (#11119) * fix unit tests * remove alaska * Basic support for custom maxBounds in alternate projections (#11121) * rudimentary support for custom maxBounds in alternate projections * fix flow * fix image and video sources in alternate projections (#11123) * clean up debug pages * remove uncessary deg <--> rad conversions * fix filename casing * fix queryRenderedFeatures for alternate projections (#11125) * Projections fixups (#11127) * disable terrain and fog for alternate projections (#11126) * Lazily instantiate projected tile debug buffers and release projected buffers when tiles are unloaded (#11128) * enable lod tile loading for projections (#11129) * enable lod tile loading for projections to significantly reduce the number of tiles at low zoom levels * use Math.hypot(...) Co-authored-by: Vladimir Agafonkin <agafonkin@gmail.com> * add comments Co-authored-by: Vladimir Agafonkin <agafonkin@gmail.com> * allow map.setProjection(null) * add limitations * avoid recreating tile buffer Co-authored-by: Karim Naaji <karim.naaji@gmail.com> * fix assertion error * fix requires * center projections vertically Center projections vertically in 0 to 1 range. This shouldn't matter but there is some constraining behavior that is currently affected by this. * Fix tile buffer destroyed but not reset (#11134) * mention settin bounds in projection docs Co-authored-by: Ryan Hamley <ryan.hamley@mapbox.com> Co-authored-by: Vladimir Agafonkin <agafonkin@gmail.com> Co-authored-by: Arindam Bose <arindam.bose@mapbox.com> Co-authored-by: Karim Naaji <karim.naaji@gmail.com>

ryanhamley added the feature 🍏 label Sep 10, 2021

ryanhamley changed the base branch from main to projections September 10, 2021 01:08

ryanhamley force-pushed the projections-api branch from 82fee12 to 9f4a3c4 Compare September 11, 2021 01:18

ryanhamley marked this pull request as ready for review September 11, 2021 01:22

ryanhamley self-assigned this Sep 11, 2021

ryanhamley requested review from mourner and ansis September 11, 2021 01:22

mourner reviewed Sep 14, 2021

View reviewed changes

src/style-spec/validate/validate_projection.js Outdated Show resolved Hide resolved

src/geo/transform.js Outdated Show resolved Hide resolved

src/style/style.js Outdated Show resolved Hide resolved

ryanhamley force-pushed the projections-api branch from 9f4a3c4 to be4cd82 Compare September 23, 2021 17:41

tristen reviewed Oct 11, 2021

View reviewed changes

ryanhamley force-pushed the projections-api branch from 7743fbf to 2e6ac95 Compare October 12, 2021 02:43

ryanhamley commented Oct 12, 2021

View reviewed changes

test/unit/ui/map.test.js Outdated Show resolved Hide resolved

mourner reviewed Oct 13, 2021

View reviewed changes

src/geo/transform.js Show resolved Hide resolved

karimnaaji reviewed Oct 13, 2021

View reviewed changes

src/ui/map.js Outdated Show resolved Hide resolved

karimnaaji reviewed Oct 13, 2021

View reviewed changes

src/ui/map.js Outdated Show resolved Hide resolved

karimnaaji reviewed Oct 13, 2021

View reviewed changes

Update style-spec

3e38618

karimnaaji approved these changes Oct 13, 2021

View reviewed changes

ryanhamley added 6 commits October 13, 2021 16:29

Update runtime option

6a4a454

Add get/setProjection api

3000389

Add docs

c89d55f

Add unit tests

d28680a

Add a couple more unit tests

95022df

Add validation

590b0d1

ryanhamley and others added 11 commits October 13, 2021 16:29

Update handling stylesheet projections

739bba8

Add clearBackground and projection diffing

df0256f

Committing what I have

57eb018

fix double rotation

5ec63b9

Rebase base branch and fixup code

e9b60ab

Update debug page

d17c13e

remove unused

d289d82

try to refactor projection not being set at runtime

2f8cc6f

setProjection() resets it completely

7d8f863

It gets treated the same as if it were never set. Projections from a stylesheet will override the default.

Update style spec and docs

2eba9f7

fix flow

164cbf9

ryanhamley force-pushed the projections-api branch from 9f142eb to 164cbf9 Compare October 13, 2021 23:41

fixup

17d4dce

ansis approved these changes Oct 14, 2021

View reviewed changes

ryanhamley merged commit 0cc5e30 into projections Oct 14, 2021

ryanhamley deleted the projections-api branch October 14, 2021 00:16

kiryldz mentioned this pull request Oct 14, 2021

Add experimental Globe projection example mapbox/mapbox-maps-ios#733

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Projections public API #11002

Projections public API #11002

ryanhamley commented Sep 10, 2021 •

edited

Loading

karimnaaji commented Sep 16, 2021

tristen Oct 11, 2021

ryanhamley Oct 12, 2021

tristen Oct 12, 2021

ryanhamley Oct 13, 2021

karimnaaji Oct 13, 2021

ansis Oct 13, 2021

karimnaaji Oct 13, 2021

tristen Oct 11, 2021

ryanhamley Oct 12, 2021

tristen Oct 12, 2021

tristen Oct 11, 2021

mourner Oct 13, 2021

mourner left a comment

mourner Oct 13, 2021

mourner Oct 13, 2021

mourner Oct 13, 2021

karimnaaji Oct 13, 2021

ryanhamley Oct 13, 2021

karimnaaji Oct 13, 2021 •

edited

Loading

ansis Oct 13, 2021

karimnaaji Oct 13, 2021

karimnaaji Oct 13, 2021 •

edited

Loading

karimnaaji Oct 13, 2021

ansis Oct 13, 2021

ansis Oct 13, 2021

karimnaaji left a comment

Projections public API #11002

Projections public API #11002

Conversation

ryanhamley commented Sep 10, 2021 • edited Loading

Launch Checklist

karimnaaji commented Sep 16, 2021

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

karimnaaji Oct 13, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karimnaaji Oct 13, 2021 • 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

karimnaaji left a comment

Choose a reason for hiding this comment

ryanhamley commented Sep 10, 2021 •

edited

Loading

karimnaaji Oct 13, 2021 •

edited

Loading

karimnaaji Oct 13, 2021 •

edited

Loading