Create new imagery provider for MapBox styles #7698

bampakoa · 2019-04-01T16:01:54Z

This work intends to incorporate the new Mapbox styles API into Cesium.

Fixes #4015

cesium-concierge · 2019-04-01T16:01:57Z

Thanks for the pull request @bampakoa!

✔️ Signed CLA found.
❔ CHANGES.md was not updated.
- If this change updates the public API in any way, please add a bullet point to CHANGES.md.
❔ Unit tests were not updated.
- Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

Cesium Viewer works.
Works in 2D/CV.
Works (or fails gracefully) in IE11.

hpinkos · 2019-04-01T16:04:59Z

Great, thanks @bampakoa! We'll take a look at this after the release goes out

bampakoa · 2019-04-01T16:09:45Z

@hpinkos this is a draft PR yet. I plan to push some more code along with tests and README changes this week and then I can notify you for reviewing 😉, if that is ok for you. But you can always have a look in the meantime 😃

hpinkos · 2019-04-01T16:10:36Z

Oh yes! I missed that. Yeah, let me know when its ready =)

bampakoa · 2019-04-04T16:11:57Z

@hpinkos this is ready for review now. You can check it directly in the Hello World app by selecting the Mapbox layers from the picker (I guess I should revert it back l8r to the old Mapbox 😃 ). I have not updated the CHANGES.md file yet since there is not any relevant entry for the next release 😟

@cesium-concierge I have updated the tests 😉

mrlubos · 2019-04-08T09:03:41Z

Thanks for making this pull request @bampakoa ! ❤️

cesium-concierge · 2019-05-09T02:00:14Z

Thanks again for your contribution @bampakoa!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

hpinkos · 2019-05-09T13:26:31Z

@OmarShehata can you review?

OmarShehata · 2019-05-20T23:55:11Z

Thanks for putting this together @bampakoa ! It seems to work pretty well. I do have some notes/questions:

Can you merge in master and push?
Why is it called MapboxStyleProvider instead of MapboxStyleImageryProvider? It is an imagery provider, so it should be consistent with the other imagery providers listed here https://cesiumjs.org/Cesium/Build/Documentation/ImageryProvider.html?classFilter=ImageryProvid
It should probably be added to that list too I think ^
Since the old Mapbox API is deprecated, is there a reason to keep around the old MapboxImageryProvider? Can this new class just replace the old one?
- I think this would be a breaking change in that the user needs to change the parameters they pass to the constructor, to choose which style they want, right?

mrlubos · 2019-05-21T07:03:23Z

@OmarShehata Hey, to the last point, the old Mapbox API still works, so I wouldn't break it!

bampakoa · 2019-05-21T07:39:56Z

@OmarShehata thanks very much for your feedback 😃

Can you merge in master and push?

Do you mean to rebase to the master?

Why is it called MapboxStyleProvider instead of MapboxStyleImageryProvider? It is an imagery provider, so it should be consistent with the other imagery providers listed here https://cesiumjs.org/Cesium/Build/Documentation/ImageryProvider.html?classFilter=ImageryProvid

You are right, I will update the PR

It should probably be added to that list too I think ^

Sounds good to me. I will update the PR

Since the old Mapbox API is deprecated, is there a reason to keep around the old MapboxImageryProvider? Can this new class just replace the old one?

I think that we shouldn't because as @mrlubos points out the old API has not been deprecated yet. See this comment for more info.

OmarShehata · 2019-05-21T16:14:17Z

Do you mean to rebase to the master?

A rebase isn't necessary, you just need to pull the latest master from this repository into your branch here. See https://help.github.com/en/articles/merging-an-upstream-repository-into-your-fork

I think that we shouldn't because as @mrlubos points out the old API has not been deprecated yet. See this comment for more info.

Thanks for the explanation - I'm still a little bit confused about the difference between these two routes that both get raster tiles from Mapbox:

Is the idea that using the style API (the second one) you have more fine grained control over customizing the look of the map? If so, do you have an example of something you can do with this that you couldn't do before?

I'm worried it's still a little big confusing having 2 Mapbox imagery providers that seem like they do the same thing, but just pass different parameters/use a different route? Would it make sense to have one MapboxImageryProvider but have a constructor option to use the style API?

bampakoa · 2019-05-21T18:55:03Z

Is the idea that using the style API (the second one) you have more fine grained control over customizing the look of the map? If so, do you have an example of something you can do with this that you couldn't do before?

@OmarShehata Yes, that is right. You can create whatever style you want in Mapbox Studio (such as this one which is GoT based) and then render it inside the client. Here is a brief overview of the differences between them and here is an article that explains why Mapbox is going to deprecate the old map styles

Would it make sense to have one MapboxImageryProvider but have a constructor option to use the style API?

I think that it would be complicated to maintain such a code as the two implementations have different required properties in order to create an object for each (mapId vs styleId). If we have two different constructors we are giving our users the opportunity to use whichever method they have created their maps and are comfortable with.

FYI, I have merged with master but I do not continue to the renaming issue until it will be clear if it makes sense to proceed or not 😃

OmarShehata · 2019-05-22T14:08:34Z

I see what you mean now (that GoT inspired map is awesome!) Thanks for the explanation. I think that's a good reason to have two separate classes if they are different APIs as you describe.

I noticed that they never actually use the word "deprecate" in that blog post. It seems like it's still a supported way of getting map tiles. I would say go ahead and name the new provider MapboxStyleImageryProvider.

And then @tfili can you do a final review and merge after that?

bampakoa · 2019-05-22T14:12:31Z

@OmarShehata what about the base layer picker control? Should I leave it as is, to use the initial API of Mapbox?

OmarShehata · 2019-05-22T14:27:06Z

Since they're functionally equivalent, I don't think it really matters, but perhaps switching to the newer API is a good idea, so if we did want to add any custom styles to the base layer picker in the future it would be easier to do that.

tfili · 2019-05-22T16:06:54Z

Source/Scene/MapboxStyleProvider.js

+ * @constructor
+ *
+ * @param {Object} [options] Object with the following properties:
+ * @param {String} [options.url='https://api.mapbox.com/styles/v1/'] The Mapbox server url.


This should be a String or a Resource like all our terrain/imagery providers.

@tfili I kept it as String in order to be the same as the current MapboxImageryProvider. Shall I change it?

Yeah, all our providers take Resource objects now. The MapBox Imagery provider was an oversight.

@tfili I updated the type of the parameter. I think the code already takes resource into account 😉

- Rename from MapboxStyleProvider - Modify CHANGES.md - Add reference to ImageryProvider class

bampakoa · 2019-05-22T18:35:33Z

@OmarShehata @tfili this is ready

cesium-concierge · 2019-06-29T02:00:15Z

Thanks again for your contribution @bampakoa!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

OmarShehata · 2019-06-29T14:07:57Z

@tfili this one would be nice to get in for release since it's already ready!

tfili · 2019-06-29T16:21:52Z

Thanks @bampakoa. Sorry it took so long. Everything seems to work great.

bampakoa · 2019-06-29T20:30:47Z

@tfili @OmarShehata my change has been mistaken added to the 1.58 version release notes in CHANGES.md. Could you please correct it or do you want me to open a PR for this?

OmarShehata · 2019-06-29T21:49:43Z

Oh, good catch @bampakoa ! I got it here #7973

mramato · 2019-07-01T01:57:36Z

I still don't understand why we need a 2 Mapbox providers in master? Is there a good reason to use one over the other?

OmarShehata · 2019-07-01T02:29:10Z

@mramato Mapbox has two different APIs for creating and serving imagery. See #7698 (comment)

From what I understand, the newer one allows you to do customize more, but the old workflow isnt deprecated and removing it in Cesiumjs would be a breaking change.

mrlubos · 2019-07-01T02:30:49Z

@OmarShehata Yesss. Do you know if this new imagery will support vector tiles? (as opposed to the raster tiles served by the previous version)

mramato · 2019-07-01T13:37:10Z

@OmarShehata I understand that, so is there a reason there wasn't jut an in-place update or are the two incompatible? If the latter, why wasn't the old one deprecated so that we can remove it 3 months from now? Does the old version provide user-accessible functionality that the new one does not?

mrlubos · 2019-07-01T13:44:12Z

@mramato These are questions for Mapbox, not for Cesium. Mapbox released a new API with breaking changes.

OmarShehata · 2019-07-01T13:58:43Z

@mramato

why wasn't the old one deprecated so that we can remove it 3 months from now

I think the initial confusion here was that the Mapbox static tiles API was deprecated in favor of the styles API. It looks like it is supported and will be supported for the foreseeable future. This is just supporting an additional workflow with different options. For example the raster (old) API lets you specify the image format: https://docs.mapbox.com/api/maps/#raster-tiles. The styles (new) API https://docs.mapbox.com/api/maps/#static-tiles doesn't have this option. I think deprecating and removing it in the CesiumJS side would be removing a feature that's still actively used by the community.

@mrlubos

Do you know if this new imagery will support vector tiles? (as opposed to the raster tiles served by the previous version)

@bampakoa can correct me if I'm wrong here, but it looks like the vector tiles are rasterized on the server, and that's what CesiumJS renders. We do have a feature request to support vector tiles directly in Cesium here: #2132

mrlubos · 2019-07-01T14:17:54Z

@OmarShehata I understand Mapbox supports vector tiles directly - https://docs.mapbox.com/api/maps/#retrieve-vector-tiles

I tried to use this endpoint in Cesium but to no avail.

OmarShehata · 2019-07-01T14:22:59Z

I understand Mapbox supports vector tiles directly

Right, it's CesiumJS that doesn't support rendering these vector tiles.

mrlubos · 2019-07-01T14:54:38Z

Is this an issue for a separate pull request @OmarShehata @hpinkos? Anything on the roadmap?

OmarShehata · 2019-07-01T15:20:18Z

@mrlubos yes this would be a separate feature from this pull request. You can see the full discussion here: #2132

bampakoa · 2019-07-01T19:02:19Z

@OmarShehata you are right. Here is an excerpt from the article https://blog.mapbox.com/use-your-mapbox-studio-styles-everywhere-e29ce5954e47

Mapbox now supports older browsers like Internet Explorer 9 and the styles you create in Studio are perfectly backwards-compatible. To make this possible, we render vector tiles into raster images with Mapbox GL Native in the cloud and produce tiles compatible with traditional mapping libraries like Mapbox.js, Leaflet, OpenLayers, and even embeddable in emails!

@mrlubos

bampakoa added 3 commits April 1, 2019 18:54

Create new imagery provider for MapBox styles

2b33321

Add scaleFactor to provider options

3767828

Add unit test

d42a3aa

bampakoa marked this pull request as ready for review April 4, 2019 16:09

Merge remote-tracking branch 'upstream/master' into mapbox-styles

2c19420

tfili reviewed May 22, 2019

View reviewed changes

MapboxStyleImageryProvider

c9ce7cd

- Rename from MapboxStyleProvider - Modify CHANGES.md - Add reference to ImageryProvider class

Update url parameter type

beb1490

tfili merged commit 27ef481 into CesiumGS:master Jun 29, 2019

bampakoa deleted the mapbox-styles branch June 29, 2019 16:32

OmarShehata mentioned this pull request Jun 29, 2019

Missing info in CHANGES #7973

Merged

OmarShehata mentioned this pull request Jul 17, 2019

new Mapbox Style API #8008

Closed

OmarShehata mentioned this pull request Aug 4, 2020

Improve MapboxImageryProvider documentation #9081

Open

Create new imagery provider for MapBox styles #7698

Create new imagery provider for MapBox styles #7698

Conversation

bampakoa commented Apr 1, 2019

cesium-concierge commented Apr 1, 2019

hpinkos commented Apr 1, 2019

bampakoa commented Apr 1, 2019 • edited Loading

hpinkos commented Apr 1, 2019

bampakoa commented Apr 4, 2019

mrlubos commented Apr 8, 2019

cesium-concierge commented May 9, 2019

hpinkos commented May 9, 2019

OmarShehata commented May 20, 2019

mrlubos commented May 21, 2019

bampakoa commented May 21, 2019 • edited Loading

OmarShehata commented May 21, 2019

bampakoa commented May 21, 2019 • edited Loading

OmarShehata commented May 22, 2019

bampakoa commented May 22, 2019

OmarShehata commented May 22, 2019

tfili May 22, 2019

Choose a reason for hiding this comment

bampakoa May 22, 2019 • edited Loading

Choose a reason for hiding this comment

tfili May 29, 2019

Choose a reason for hiding this comment

bampakoa May 30, 2019

Choose a reason for hiding this comment

bampakoa commented May 22, 2019

cesium-concierge commented Jun 29, 2019

OmarShehata commented Jun 29, 2019

tfili commented Jun 29, 2019

bampakoa commented Jun 29, 2019 • edited Loading

OmarShehata commented Jun 29, 2019

mramato commented Jul 1, 2019

OmarShehata commented Jul 1, 2019

mrlubos commented Jul 1, 2019

mramato commented Jul 1, 2019

mrlubos commented Jul 1, 2019

OmarShehata commented Jul 1, 2019

mrlubos commented Jul 1, 2019 • edited Loading

OmarShehata commented Jul 1, 2019

mrlubos commented Jul 1, 2019

OmarShehata commented Jul 1, 2019

bampakoa commented Jul 1, 2019

bampakoa commented Apr 1, 2019 •

edited

Loading

bampakoa commented May 21, 2019 •

edited

Loading

bampakoa commented May 21, 2019 •

edited

Loading

bampakoa May 22, 2019 •

edited

Loading

bampakoa commented Jun 29, 2019 •

edited

Loading

mrlubos commented Jul 1, 2019 •

edited

Loading