mapbox-gl@1.0.0 + improvements by etpinard · Pull Request #3987 · plotly/plotly.js

etpinard · 2019-06-26T17:34:44Z

This PR will serve as a base for all v1.49.0 mapbox-related improvements.

In brief, this PR:

bumps mapbox-gl to v1.0.0 in 2d4fe9b (which went smooth apart from 5b43b8c) 🎉
commit 41d167f allows users to use third-party layout.mapbox.style (e.g. from Open-Street Map, see mock in 23ffe4f). This resolves[Feature Request] use own style.json instead of mapbox access token #3592

cc @plotly/plotly_js

... navigation menu is now not included by default

- update no-access-token error msg - fixup click mocking action - fix relayouting cnt - add TODOs about new (and annoying) mapbox-gl warn msg - add info atlas-server logic and test

- that is if all mapbox subplots on graph do not use Mapbox styles, DO NOT error when mapbox access token isn't set - moreover, add helpful log when multiple distinct tokens are set in same layout

... to avoid console warning on subplot instantiation

antoinerg · 2019-06-26T18:34:26Z

- staticPlot: gd._context.staticPlot
- });
-
+ mapbox = new Mapbox(gd, id);


Nice clean up here 👍

antoinerg · 2019-06-26T18:43:13Z

I am a bit surprised that the markers in the legend are now smaller in test/image/baselines/mapbox_0.png. Can you tell me what changes introduced in this PR are causing this?

etpinard · 2019-06-26T18:52:43Z

Can you tell me what changes introduced in this PR are causing this?

I forgot to update the mapbox baselines in #3732 or #2840

antoinerg · 2019-06-26T18:55:55Z

Thanks @etpinard for the very nice PR! I also noticed that mapbox_fill now renders properly the fillcolor 👍

💃

- valid choropleth datum must have a valid location AND `z` value - set calcdata[i][j].index during calc so that choroplethmapbox can use it in its convert.js

... and update choropleth baselines that didn't show the "correct" default "black"

for geo AND mapbox base plot modules ... that way we do this only once when geo and mapbox modules are registered

- reusing choropleth calc, hover, select, eventData and feature2polygon - use `PlotlyGeoAssets` to stash fetched geojson url data - use two layers, one fill (to filled choropleth polygon), one line to draw `marker.line` styles - add mockAxis on mapbox subplot instances, used to format choroplethmapbox 'z' on hover

... similar to how we do things on geo subplots

- use _rehover wrapper similar to cartesian subplots and call it on moveend - split fx init code into prototype.initFx

- where by default chroroplethmapbox traces are placed above the top most "water" layer - N.B. we need to remove/add again layers when *below* is updated

Introducing densitymapbox traces

Add support for raster and image mapbox layers

Introducing choroplethmapbox traces

etpinard · 2019-07-03T14:05:24Z

@plotly/plotly_js I'm looking for a final review.

This PR now includes:

mapbox-gl@v1.1.0
support for custom non-mapbox base styles
new open-street-map base style
new trace type choroplethmabpox
new trace type densitymapbox
support for raster and image layout mapbox layers
choroplethmarker.line.color dflt fix

archmoj · 2019-07-03T15:04:14Z

So @nicolaskruchten , the only parameters we need to agree on are:
tileSize - which is the minimum visual size to display tiles for this layer. The default is 512, we could lower it to 256 (like mapbox does in their docs). But all in all this should not be a big deal.
minzoom / maxzoom - which I thought of setting for a max range of 0 to 22

According to https://blog.mapbox.com/512-map-tiles-cb5bfd6e72ba, it looks they suggested using 512 resolution.
Also that may help reduce the number of requests to the server in addition to less i/o process for caching.

etpinard · 2019-07-03T15:07:36Z

Also that may help reduce the number of requests to the server in addition to less i/o process for caching.

How? I'm curious.

archmoj · 2019-07-03T15:26:04Z

Also that may help reduce the number of requests to the server in addition to less i/o process for caching.
How? I'm curious.

I have rather limited experience on this.
Anyway regarding i/o caching more tiles means more files to seek for.
I think one advantage of tiles of 256 x 256 was that the integer index could simply fit in an unsigned short integers (0-65535). That may be one reason it is used in bucket rendering engines. On the other hand requesting for a tile of 512 x 512 may reduce the number of requests by four; plus they may have done other optimisations e.g. related to clipping text printed close the tile boundaries.

etpinard · 2019-07-03T18:19:06Z

@archmoj so, I'm assuming you'd vote for having tileSize:512? I don't have a strong opinion. I can't notice a performance different between the two (on my laptop using the plotly wifi)

archmoj · 2019-07-03T18:24:32Z

@archmoj so, I'm assuming you'd vote for having tileSize:512? I don't have a strong opinion. I can't notice a performance different between the two (on my laptop using the plotly wifi)

Yes in the case of mapbox.

... as showSkipped is now turned on by default on CI.

etpinard · 2019-07-03T18:38:45Z

Upon further inspection, tileSize:256 (top subplot) looks a lot better then tileSize:512 (bottom subplot):

What do you think @archmoj ?

archmoj · 2019-07-03T19:06:19Z

Upon further inspection, tileSize:256 (top subplot) looks a lot better then tileSize:512 (bottom subplot):
What do you think @archmoj ?

@etpinard
Just to double check:
Did you clean the cache?

archmoj · 2019-07-03T19:27:10Z

Confirmed. Let's use 256.

archmoj · 2019-07-04T18:01:57Z

This is likely the first time ever that multiple new trace types were added in one PR!
Brilliant work @etpinard
🥇
💃

etpinard added 10 commits June 26, 2019 11:59

bump mapbox-gl to 1.0.0 🎉
2d4fe9b

🔪 clean navigation block
95240e2
... navigation menu is now not included by default

add missing .catch(failTest)
e0c684f

adapt tests for mapbox-gl@v1
889abb6
- update no-access-token error msg - fixup click mocking action - fix relayouting cnt - add TODOs about new (and annoying) mapbox-gl warn msg - add info atlas-server logic and test

adapt find-mapbox-access-token for maps that don't use Mapbox styles
41d167f
- that is if all mapbox subplots on graph do not use Mapbox styles, DO NOT error when mapbox access token isn't set - moreover, add helpful log when multiple distinct tokens are set in same layout

mv DOM-related Lib function to src/lib/dom.js
1ee02da

add mapbox-gl CSS when registering mapbox basePlotModule
5b43b8c
... to avoid console warning on subplot instantiation

modernize createMapbox API
a2d2522

update mapbox baseline (using mapbox-gl@v1)
1987b22

add mapbox using OSM map raster style mock
23ffe4f

etpinard added feature something new status: reviewable labels Jun 26, 2019

etpinard added this to the v1.49.0 milestone Jun 26, 2019

antoinerg reviewed Jun 26, 2019
View reviewed changes

etpinard added 13 commits June 26, 2019 16:18

improve Choropleth.calc
43c568f
- valid choropleth datum must have a valid location AND `z` value - set calcdata[i][j].index during calc so that choroplethmapbox can use it in its convert.js

add dflt to choropleth marker.line.color
91e0319
... and update choropleth baselines that didn't show the "correct" default "black"

add choroplethmapbox attributes
8fc1dbe

add PlotlyGeoAssets to window during Plotly.register
70878b7
for geo AND mapbox base plot modules ... that way we do this only once when geo and mapbox modules are registered

fetch GeoJSON URLs found in calcdata before updateData()
012a749

sort all choroplethmapbox traces below scattermapbox traces
f7f3190
... similar to how we do things on geo subplots

add GeoJSON feature 'properties' to eventData
4e88075

improve mapbox hover after drag
a166761
- use _rehover wrapper similar to cartesian subplots and call it on moveend - split fx init code into prototype.initFx

first cut choroplethmapbox tests
9a602ba

allow numbers as *locations* items when using custom *geojson*
a457722

fixup choroplethmapbox hover content post-#3968
1359307

implement *below* in choroplethmapbox traces
4d7e0aa
- where by default chroroplethmapbox traces are placed above the top most "water" layer - N.B. we need to remove/add again layers when *below* is updated

etpinardand others added 7 commits June 28, 2019 16:39

Merge pull request#3993from plotly/densitymapbox-pr
ca48461
Introducing densitymapbox traces

raster support
a765249

fix syntax test
7dcc506

do not decode raster source URL
0c99017

add support for sourcetype: 'image' layers
153c6b4

add some layer sourcetype -> type logic
54efdad

add raster/raster and image/raster layers
0b17b62

etpinard mentioned this pull request Jul 2, 2019
Add support for raster and image mapbox layers #4006
Merged

etpinard added 4 commits July 2, 2019 12:05

bump mapbox-gl to 1.1.0
f5bc2e4

Merge pull request#4006from plotly/mapbox-raster-layer
b69cf4b
Add support for raster and image mapbox layers

Merge branch 'mapbox-v1' into choroplethmapbox-pr
6d7929c

Merge pull request#3988from plotly/choroplethmapbox-pr
eb04b4c
Introducing choroplethmapbox traces

etpinard mentioned this pull request Jul 3, 2019
ENH: Add support for heatmap MapBox layers #3110
Closed

etpinard added 3 commits July 3, 2019 14:26

Merge branch 'master' into mapbox-v1
a646575

fixup package-lock
8c48aa2

rm unnecessary --showSkipped CLI arg
dfa4ec7
... as showSkipped is now turned on by default on CI.

This comment has been minimized.
Sign in to view

etpinard merged commit c1ef691 into masterJul 4, 2019

etpinard deleted the mapbox-v1 branch July 4, 2019 18:04

Mahdis-z mentioned this pull request Jul 31, 2019
1.49 Mapbox features plotly/documentation#1412
Closed

Uh oh!

Uh oh!

Conversation

etpinard commented Jun 26, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

antoinergJun 26, 2019

Choose a reason for hiding this comment

Uh oh!

antoinerg commented Jun 26, 2019

Uh oh!

etpinard commented Jun 26, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

antoinerg commented Jun 26, 2019

Uh oh!

etpinard commented Jul 3, 2019

Uh oh!

archmoj commented Jul 3, 2019

Uh oh!

etpinard commented Jul 3, 2019

Uh oh!

archmoj commented Jul 3, 2019

Uh oh!

etpinard commented Jul 3, 2019

Uh oh!

archmoj commented Jul 3, 2019

Uh oh!

etpinard commented Jul 3, 2019• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

archmoj commented Jul 3, 2019

Uh oh!

This comment has been minimized.

archmoj commented Jul 3, 2019

Uh oh!

archmoj commented Jul 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

etpinard commented Jun 26, 2019•
edited
Loading

etpinard commented Jun 26, 2019•
edited
Loading

etpinard commented Jul 3, 2019•
edited
Loading