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

Web map tile service imagery provider #3270

Closed
wants to merge 5 commits into from
Closed

Web map tile service imagery provider #3270

wants to merge 5 commits into from

Conversation

adamdavidcole
Copy link

Created function createWebTileServiceImageryProvider which utilizes the generic UrlTemplateImageryProvider
@pjcozzi @TiffanyLu

@adamdavidcole
Copy link
Author

I get strange behavior when I use this imagery provider. For instance in http://localhost:8080/Apps/Sandcastle/index.html?src=Imagery%20Layers%20Manipulation.html&label=Beginner changing the dropdown to USGS and unchecking all the options except tile coordinates and then zooming past 6 levels causes problems.
However, I get the same results with the current implementation of Web map tile service imagery provider (which can be seen by following the same steps here: http://cesiumjs.org/Cesium/Apps/Sandcastle/index.html?src=Imagery%20Layers%20Manipulation.html&label=Beginner)

Let me know if you have an idea why this is the case.

Still to do in this pull request is to finish the deprecation process for Web map tile service imagery provider.

Thanks!
Adam

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 1, 2015

Let me know if you have an idea why this is the case.

This could just be an issue with the data the service is returning, not the request Cesium is making. I am not 100% sure though. Can you please submit a separate issue about it?

@@ -1,13 +0,0 @@
<?xml version="1.0" encoding="UTF-8"?>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to remove this?

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 1, 2015

Still to do in this pull request is to finish the deprecation process for Web map tile service imagery provider.

Put a task list in the top comment of this pull request to manage what is left like in #3225 for example.

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 1, 2015

Part of #2814.

@kring in #2814, you said implementing WebMapTileServiceImageryProvider with UrlTemplateImageryProvider may be complicated. This looks straightforward. Are we missing something?

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 1, 2015

Did you run the tests? There are several failures:

image

Never open a pull request with failing tests unless part of the pull request comment is asking for help on why they are failing.

@adamdavidcole
Copy link
Author

In my recent update, I fixed the way the urlTemplate is constructed and completed the deprecation process for WebMapTileServiceImageryProvider. I listed it for deprecation in 1.19, does that sound right? When tested by hand, the map imagery shows up as expected (aka. the same way it shows up on cesiumjs.org currently).

After these changes I'm still failing 5 tests in the spec, mostly the result of two separate problems. The first is that two tests expect to access the format by using 'provider.format' (seen here: https://github.com/tiffanylu/cesium/blob/webMapTileServiceImageryProvider/Specs/Scene/createWebMapTileServiceImageryProviderSpec.js#L223 and https://github.com/tiffanylu/cesium/blob/webMapTileServiceImageryProvider/Specs/Scene/createWebMapTileServiceImageryProviderSpec.js#L254). I believe that this issue should be resolved by removing those two line from the spec code.

Another two tests fail because WebMapTileServiceImageryProvider provides an option for passing tileMatrixLabels which map labels to strings. As a result tests like this fail:
Expected '1' to equal 'second'." As of now I do not think there is any way to set up labels in UrlTemplateImageryProvider. One option here is to remove providing labels as an option for createWebTileServiceImageryProvider.

Finally, the last test which fails is this: Expected function 'getTileCredits' to exist on WebMapTileServiceImageryProvider because it should implement interface ImageryProvider. I can't figure out why this fails because UrlTemplateImageryProvider does define the function getTileCredits: https://github.com/tiffanylu/cesium/blob/webMapTileServiceImageryProvider/Source/Scene/UrlTemplateImageryProvider.js#L447.

Please let me know what you think I should do to solve these testing issues.

Thanks!

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 15, 2015

@adamdavidcole can you merge in master. There is probably a trivial conflict in CHANGES.md.

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 15, 2015

I listed it for deprecation in 1.19, does that sound right?

Yes

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 15, 2015

The first is that two tests expect to access the format by using 'provider.format' (seen here: https://github.com/tiffanylu/cesium/blob/webMapTileServiceImageryProvider/Specs/Scene/createWebMapTileServiceImageryProviderSpec.js#L223 and https://github.com/tiffanylu/cesium/blob/webMapTileServiceImageryProvider/Specs/Scene/createWebMapTileServiceImageryProviderSpec.js#L254). I believe that this issue should be resolved by removing those two line from the spec code.

This is fine since it is a breaking change; createWebMapTileServiceImageryProvider returns a UrlTemplateImageProvider, which does not have this property.

However, please make sure that the KVP requests that use the format still work, e.g., https://github.com/tiffanylu/cesium/blob/webMapTileServiceImageryProvider/Source/Scene/WebMapTileServiceImageryProvider.js#L201

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 15, 2015

Another two tests fail because WebMapTileServiceImageryProvider provides an option for passing tileMatrixLabels which map labels to strings. As a result tests like this fail:
Expected '1' to equal 'second'." As of now I do not think there is any way to set up labels in UrlTemplateImageryProvider. One option here is to remove providing labels as an option for createWebTileServiceImageryProvider.

I think you need to add support to UrlTemplateImageryProvider for this in one form or another, e.g., see https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/Scene/WebMapTileServiceImageryProvider.js#L167

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 15, 2015

Finally, the last test which fails is this: Expected function 'getTileCredits' to exist on WebMapTileServiceImageryProvider because it should implement interface ImageryProvider. I can't figure out why this fails because UrlTemplateImageryProvider does define the function getTileCredits: https://github.com/tiffanylu/cesium/blob/webMapTileServiceImageryProvider/Source/Scene/UrlTemplateImageryProvider.js#L447.

Replace this test with a test like this: https://github.com/tiffanylu/cesium/blob/webMapTileServiceImageryProvider/Specs/Scene/createOpenStreetMapImageryProviderSpec.js#L34

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 3, 2016

@adamdavidcole thanks for your initial work on this; we're going to hold off for now. Hope you are having a great summer and thanks again for all your past contributions to Cesium!

@pjcozzi pjcozzi closed this Jun 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants