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

WMTS image provider with subdomains drops query #9606

Merged
merged 12 commits into from
Jun 15, 2021
Merged

Conversation

srothst1
Copy link
Contributor

@srothst1 srothst1 commented Jun 9, 2021

Fixes issue #9598

Changed the behavior of the requestImage function when useKpv is true in the file WebMapTileServiceImageryProvider.js. Updated the constructor in WebMapTileServiceImageryProvider.js to indicate that it is safe to use KPV when {s} is present yet no other keywords are present.

Issue derived from this forum post.

@ebogo1
@lilleyse

@cesium-concierge
Copy link

Thanks for the pull request @srothst1!

  • ✔️ 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.

Source/Scene/WebMapTileServiceImageryProvider.js Outdated Show resolved Hide resolved
Source/Scene/WebMapTileServiceImageryProvider.js Outdated Show resolved Hide resolved
Source/Scene/WebMapTileServiceImageryProvider.js Outdated Show resolved Hide resolved
Source/Scene/WebMapTileServiceImageryProvider.js Outdated Show resolved Hide resolved
@ebogo1
Copy link
Contributor

ebogo1 commented Jun 9, 2021

Also, CHANGES.md should be updated in the "Fixes" category.

@ebogo1
Copy link
Contributor

ebogo1 commented Jun 9, 2021

Ah - another thing. It looks like your changes introduced some problems with eslint so the build is failing (there might be some failing specs too). To test for this before pushing/committing, you can run npm run eslint and npm run test and see if any errors come up.

For the latter, it can be helpful to use syntax like fit instead of it to run just a specific test, or fdescribe instead of describe to run just a specific block of tests.

VSCode has a handy eslint extension too.

…pec.js, removed unnecessary comments from WebMapTileServiceImageryProvider.js
CHANGES.md Outdated Show resolved Hide resolved
@ebogo1
Copy link
Contributor

ebogo1 commented Jun 11, 2021

@srothst1 I pushed a couple small changes:

  1. In the constructor moved templateValues into the else block because it wasn't needed anywhere else
  2. I defined templateValues outside the if-else blocks in requestImage() to avoid making a different variable depending on whether useKvp was true. This is mostly an aesthetic preference but it makes the diff smaller.

Once CI passes and @tfili gives a final look this should be good to go 👍

@srothst1
Copy link
Contributor Author

@ebogo1 Thank you for helping me clean up the code. It looks like the checks have passed!

@tfili
Copy link
Contributor

tfili commented Jun 15, 2021

@srothst1 I pushed a small tweak. This code worked by accident

expect(uri.scheme + "://" + uri.authority).toEqual("http://wmtsa.invalid" || "http://wmtsb.invalid" || "http://wmtsc.invalid");

Using the || operator will return the first term that isn't "falsy" (eg. null, undefined, false, 0, NaN, ""), so this will always test for "http://wmtsa.invalid" since that is a "truthy" value.

It picks the subdomain by adding, row, column and level and modulusing it by the number of domains. In this case it was 5 + 12 + 1, so 18 % 3 = 0, so it selected subdomains[0] or a. I modified the second request to select http://wmtsb.invalid just to make sure.

@srothst1
Copy link
Contributor Author

@tfili great! are we in a good spot to merge this pull request?

@tfili
Copy link
Contributor

tfili commented Jun 15, 2021

Thanks @srothst1 . Looks good.

@tfili tfili merged commit a64fd5e into master Jun 15, 2021
@tfili tfili deleted the WMTS-provider-drops-param branch June 15, 2021 14:46
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.

4 participants