Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update imagery providers to use UrlTemplateImageryProvider #2814

Closed
pjcozzi opened this issue Jun 16, 2015 · 10 comments
Closed

Update imagery providers to use UrlTemplateImageryProvider #2814

pjcozzi opened this issue Jun 16, 2015 · 10 comments
Labels
cleanup good first issue An opportunity for first time contributors

Comments

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 16, 2015

No description provided.

@pjcozzi
Copy link
Contributor Author

pjcozzi commented Sep 9, 2015

@kring could you make a list of imagery providers that could be implemented using UrlTemplateImageryProvider in order of difficulty? We may have some help for this.

@kring
Copy link
Member

kring commented Sep 13, 2015

It should be possible to implement all of them in terms of UrlTemplateImageryProvider.

These are already done:

  • WebMapServiceImageryProvider
  • MapboxImageryProvider

These should be really easy:

  • TileMapServiceImageryProvider
  • OpenStreetMapImageryProvider

These may be more complicated:

  • GoogleEarthImageryProvider
  • ArcGisMapServerImageryProvider
  • WebMapTileServiceImageryProvider

These don't make a lot of sense to change:

  • GridImageryProvider
  • SingleTileImageryProvider
  • TileCoordinatesImageryProvider

We'll need to add a {quadKey} or something similar to UrlTemplateImageryProvider for BingMapsImageryProvider, #1378.

@pjcozzi pjcozzi added the good first issue An opportunity for first time contributors label Sep 13, 2015
@pjcozzi
Copy link
Contributor Author

pjcozzi commented Sep 13, 2015

Thanks @kring.

@TiffanyLu @adamdavidcole this could be a nice project after the KML examples depending on your interests. It would only be a few weeks, not the rest of the semester.

@sen-lu
Copy link
Contributor

sen-lu commented Sep 29, 2015

@pjcozzi when you say KML examples, did you mean CZML or did you want us to do additional work with KML support?

@pjcozzi
Copy link
Contributor Author

pjcozzi commented Sep 29, 2015

Just CZML. That is a typo.

@pjcozzi
Copy link
Contributor Author

pjcozzi commented Jan 2, 2017

@kring do you think this is still worth someone pursuing for any of the remaining types?

@kring
Copy link
Member

kring commented Jan 3, 2017

@kring do you think this is still worth someone pursuing for any of the remaining types?

Nah, not much to be gained IMO. I'll close this.

@kring kring closed this as completed Jan 3, 2017
@mramato
Copy link
Contributor

mramato commented Jan 3, 2017

It is kind of weird and inconsistent that we use functions for some types classes for others. I know perfect is the enemy of the good and all that, but it makes our API harder to use.

@hpinkos
Copy link
Contributor

hpinkos commented Jan 3, 2017

Seconded. Consistency is key for API usability. Maybe we could rework createOpenStreetMapImageryProvider and createTileMapServiceImageryProvider into classes that inherit from UrlTemplateImageryProvider instead.

@hpinkos hpinkos reopened this Jan 3, 2017
@kring
Copy link
Member

kring commented Jan 5, 2017

Sure, I can see that the inconsistency makes the API harder to use.

So, @mramato and @hpinkos, are you suggesting that we move to an all-function API? So users never do new WebMapServiceImageryProvider(...), they instead do createWebMapServiceImageryProvider(...)? I'm not sure that's an improvement. In fact it's inconsistent with how most everything else works in Cesium. But if we do want that, it's just a matter of making function wrappers for the imagery types and marking the classes private. It's completely unrelated to this issue, which is talking about how the imagery providers are implemented. For example, WebMapServiceImageryProvider is already implemented in terms of UrlTemplateImageryProvider, yet it is constructed with new rather than by calling a function.

So maybe we should be all classes? It's worth revisiting why we have functions - rather than classes - for OSM and TMS in the first place. There were basically two arguments for it:

  1. Those classes were never very useful in the first place. Leaflet doesn't have them, for example. Interfacing with TMS and OSM with just a URL template is pretty straightforward. If we thought about it more carefully from the beginning, we might not have even provided specific OSM and TMS support.
  2. Writing those providers as classes requires a bunch of trivial boilerplate. We implement a bunch of functions that do nothing other than call the same function on UrlTemplateImageryProvider. By making them functions instead, we make Cesium smaller.

So if we accept point 1, we might also say that the usability point is pretty much moot because most people will use UrlTemplateImageryProvider directly anyway.

Point 2 could be addressed with @hpinkos's suggestion: inherit from UrlTemplateImageryProvider rather than aggregating it. AFAIK this would be the first use of inheritance anywhere in Cesium, though.

TL;DR: this issue should be closed. If there's something specific you want to see happen with the TMS and OSM functions (or all the other imagery providers maybe?) then write an issue to say that. This one is only talking about changing the way the imagery providers are implemented, which is totally orthogonal to the public API.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup good first issue An opportunity for first time contributors
Projects
None yet
Development

No branches or pull requests

5 participants