Open street map imagery provider

[adamdavidcole]

Updated OpenStreetMapImageryProvider to return a configured UrlTemplateImageryProvider to reduce code redundancy.

[adamdavidcole]

Note: Two tests now fail -- one which seems to be a rounding error: Scene/OpenStreetMapImageryProvider rectangle passed to constructor does not affect tile numbering
Expected ({ west: 0.09999999999999964, south: 0.2, east: 0.3000000000000007, north: 0.4 }) to equal ({ west: 0.1, south: 0.2, east: 0.3, north: 0.4 }).

^Should numbers off by such a small amount be equal?

Also failing: Scene/OpenStreetMapImageryProvider requestImage returns a promise for an image and loads it for cross-origin use
Expected 'made/up/osm/server/{z}/{x}/{y}.png' to equal 'made/up/osm/server/'.
--I think this is a byproduct of the test expecting an OpenStreetMapImageryProvider implementation of requestImage when it is now getting the UrlTemplateImageryProvider version of the function.

Finally: to pass the following test:
"Scene/OpenStreetMapImageryProvider conforms to ImageryProvider interface
Expected function 'getTileCredits' to exist on OpenStreetMapImageryProvider because it should implement interface ImageryProvider."
--I had to keep the code for getTileCredits, requestImage, and pickFeatures to conform to the ImageryProvider interface even though they are never called and I don't think they are necessary.

Please let me know how I should handle these errors either by changing my code or the test code.

Thanks!

[pjcozzi]

Note: Two tests now fail -- one which seems to be a rounding error:

Test with an epsilon, for example, see #3151.

[pjcozzi]

Finally: to pass the following test:
"Scene/OpenStreetMapImageryProvider conforms to ImageryProvider interface
Expected function 'getTileCredits' to exist on OpenStreetMapImageryProvider because it should implement interface ImageryProvider."
--I had to keep the code for getTileCredits, requestImage, and pickFeatures to conform to the ImageryProvider interface even though they are never called and I don't think they are necessary.

We could either remove this test make a version of toConformToInterface that works with instances instead of the class. @kring what do you think?

[pjcozzi]

Also failing: Scene/OpenStreetMapImageryProvider requestImage returns a promise for an image and loads it for cross-origin use
Expected 'made/up/osm/server/{z}/{x}/{y}.png' to equal 'made/up/osm/server/'.
--I think this is a byproduct of the test expecting an OpenStreetMapImageryProvider implementation of requestImage when it is now getting the UrlTemplateImageryProvider version of the function.

This is a breaking change since now url doesn't return the url the user provided, and instead returns the url passed to UrlTemplateImageryProvider. Perhaps we should override the url property (assuming that is possible) before returning it from the OpenStreetMapImageryProvider constructor function.

@kring do you have another idea?

[kring]

I wonder if we can just return new UrlTemplateImageryProvider(...) like we do here.

That pattern doesn't actually work. You can chain constructors in JavaScript, but have to do it with function.call, not a call to new.

e.g. in HeadingPitchRange, instead of:

return new CoreHeadingPitchRange(heading, pitch, range);

You need:

CoreHeadingPitchRange.call(this, heading, pitch, range);

Although you can probably change the entire implementation of HeadingPitchRange in Scene to:

return CoreHeadingPitchRange;

This is a breaking change since now url doesn't return the url the user provided, and instead returns the url passed to UrlTemplateImageryProvider.

Right, that's the other problem with this approach. I don't think overriding the url property is going to go well. Basically you're doing classical inheritance here without an actual is-a relationship. Don't do it. You need to be wrapping, not inheriting, even though wrapping is more code.

[kring]

To slim down Cesium longer term, I'd definitely support deprecating OpenStreetMapImageryProvider entirely and instead introducing a simple function that creates a correctly-configured UrlTemplateImageryProvider from the parameters currently given to OpenStreetMapImageryProvider. By making it a function instead of a class, we avoid the inheritance problems, and by giving it a new name we avoid the backward compatibility problems.

[pjcozzi]

I'd definitely support deprecating OpenStreetMapImageryProvider entirely and instead introducing a simple function that creates a correctly-configured UrlTemplateImageryProvider from the parameters currently given to OpenStreetMapImageryProvide

I was thinking the same thing. Any reason not to do that now?

[kring]

I was thinking the same thing. Any reason not to do that now?

No reason I can think of!

[pjcozzi]

@adamdavidcole let's go with @kring's suggestion:

I'd definitely support deprecating OpenStreetMapImageryProvider entirely and instead introducing a simple function that creates a correctly-configured UrlTemplateImageryProvider from the parameters currently given to OpenStreetMapImageryProvide

See our Deprecation Guide.

[adamdavidcole]

I updated the pull request by reverting and deprecating the original OpenStreetMapImageryProvider, however I wasn't sure how to decide which future version the deprecated API should be removed.

I created a function that creates a UrlTemplateImageryProvider in the OpenStreetMapImageryProviderGenerator file -- but I feel like a better filename and/or instantiation method should be used. I also created the Spec for it which passes all tests except for this test which still fails:
"Scene/OpenStreetMapImageryProvider conforms to ImageryProvider interface
Expected function 'getTileCredits' to exist on OpenStreetMapImageryProviderGenerator because it should implement interface ImageryProvider."

However, I feel like it should pass because UrlTemplateImageryProvider has a getTileCredits function (https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/Scene/UrlTemplateImageryProvider.js#L447).

[pjcozzi]

@adamdavidcole can you please merge master (from the Cesium repo) into this branch. There will be some, hopefully minor, merge conflicts.

[pjcozzi]

@kring do you want to review this?

[adamdavidcole]

I updated the pull request based on our conversation last week, specifically by making the new file 'getOpenStreetMapImageryProvider' a function with a sensible name, replacing all instances of OpenStreetMapImageryProvider with the new function in both the code and comments/code examples, and updating the CHANGES.md file.

I wasn't sure if I should wait for this pull request to get merged before creating an issue to deprecate OpenStreetMapImageryProvider in Cesium 1.18.

Also,
"Scene/OpenStreetMapImageryProvider conforms to ImageryProvider interface
Expected function 'getTileCredits' to exist on OpenStreetMapImageryProviderGenerator because it should implement interface ImageryProvider

still fails for some reason I can't figure out because UrlTemplateImageryProvider has a getTileCredits function defined: https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/Scene/UrlTemplateImageryProvider.js#L447

[pjcozzi]

I wasn't sure if I should wait for this pull request to get merged before creating an issue to deprecate OpenStreetMapImageryProvider in Cesium 1.18.

It's fine to submit this issue now.

[pjcozzi]

This looks good to me. I made some tweaks, most notably in dc5e473 and 229cc57.

Does anyone else want to review this?

@adamdavidcole what are you working on next?

[pjcozzi]

Part of #2814.

[pjcozzi]

@adamdavidcole this looks good, but it has one test failure when all the tests are ran:

Note that this test doesn't fail when just the OSM tests are ran:

http://localhost:8080/Specs/SpecRunner.html?spec=Scene%2FgetOpenStreetMapImageryProvider

I think this test use to pass. Perhaps a recent change in master that was merged in broke it?

[kring]

This looks good to me as well. One minor nitpick: maybe createOpenStreetMapImageryProvider instead of getOpenStreetMapImageryProvider?

[adamdavidcole]

@pjcozzi -- thanks for those documentation and spec tweeks, they definitely make a lot of sense. Also, I'm not getting that test failure when I run all tests. I'll try to see if I can recreate those results somehow.

Hi @kring, -- thanks that name change suggestion makes sense to me. I updated the pull request to reflect that.

Thanks to both of you for reviewing the request!

[pjcozzi]

I restarted my browser and cleared the cache, and I don't get the test failure either. Awesome!

Thanks again @adamdavidcole!

[pjcozzi]

I also submitted #3222 for the deprecation.

[mramato]

Specs/.jshintrc should not have been submitted as part of this PR and needs to be deleted in master.

[mramato]

I took care of it.

[pjcozzi]

Good eye, thanks @mramato.