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

Fix broken URL in WMTS + time Sandcastle #8352

Merged
merged 3 commits into from
Feb 3, 2020
Merged

Conversation

OmarShehata
Copy link
Contributor

@OmarShehata OmarShehata commented Nov 3, 2019

This fixes #8310. The URL in the current sandcastle seems to no longer be valid. I found this one with some trial and error. I added a comment about where the data comes from so it'd be easy to replace with other datasets.

I also made a few other quality improvements:

  • Slowed down the clock multiplier by half. Before it was constantly requesting new tiles so you couldn't actually see one complete layer before it started requesting new tiles again.
  • Made the layer semi-transparent so you can see the dynamic clouds overlayed on the underlying geography.
  • Made the thumbnail a zoomed in view that shows the clouds on top of the globe, which communicates a little better what this example is about.

I also wanted to ask how hard it would be to make the layer queue up all the new tiles and not display them until the whole layer is ready. This is because it's still kind of hard to see changes when they come piecemeal like this. So if you're trying to follow a specific hurricane for example it makes it hard to visually parse what changed from frame to frame. This originally came up on the forum:

The below code should update the tiles every 10-minutes on the ticker. (I've staged tiles at 16:50Z, 17:00Z, 17:10Z) I'd like it to appear to the user like they are viewing an animated GIF.

forum thread

Deployed Sandcastle: http://cesium-dev.s3-website-us-east-1.amazonaws.com/cesium/fix-wmts-sandcastle/Apps/Sandcastle/index.html?src=Web%20Map%20Tile%20Service%20with%20Time.html

@cesium-concierge
Copy link

Thanks for the pull request @OmarShehata!

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

@mramato
Copy link
Contributor

mramato commented Nov 7, 2019

I also wanted to ask how hard it would be to make the layer not queue up all the new tiles and not display them until the whole layer is ready.

I'm a little confused by the double negative here. Do you want them to change as they become available or change all at once? I believe the current behavior is supposed to be that they change all at once, but @tfili implemented this feature and can confirm.

@GatorScott
Copy link

Looking at this, I'd suggest reducing the multiplier to 7200x which provides enough time for the server to deliver all of the images and for the user to observe them. They do need to change all at once to properly represent the time dimension's properties (e.g. daily).

@cesium-concierge
Copy link

Thanks again for your contribution @OmarShehata!

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
Copy link
Contributor Author

I'm a little confused by the double negative here. Do you want them to change as they become available or change all at once?

Sorry - I meant the latter. They should change all at once (like this Planet Satellite Imagery blog I just stumbled on).

It looks like it does change all at once only when all the tiles for that layer have been loaded (so going back and forth between two iterations works as expected). Fili did mention it will try to preload tiles in the upcoming point in time but I guess I was moving the clock too fast for that to complete.

I changed the multiplier to 7200 as suggested above so this is ready.

@agrondalski
Copy link

Should there be a separate issue created to make sure they all change at once? It does not seem to be working that way currently.

@hpinkos
Copy link
Contributor

hpinkos commented Jan 29, 2020

@OmarShehata is this ready? @mramato did you want to take another look at this?

@OmarShehata
Copy link
Contributor Author

@hpinkos @mramato yes this is ready to merge. I opened a separate issue for whether we want to consider improving how we handle time dynamic imagery here: #8581

@lilleyse
Copy link
Contributor

lilleyse commented Feb 3, 2020

The imagery dataset is a different one than I remembered but at least the Sandcastle shows something now. 👍

@lilleyse lilleyse merged commit bc8b44e into master Feb 3, 2020
@cesium-concierge
Copy link

Congratulations on closing the issue! I found these Cesium forum links in the comments above:

https://groups.google.com/forum/#!topic/cesium-dev/1Gdl4FhaaDA

If this issue affects any of these threads, please post a comment like the following:

The issue at #8352 has just been closed and may resolve your issue. Look for the change in the next stable release of Cesium or get it now in the master branch on GitHub https://github.com/AnalyticalGraphicsInc/cesium.

@lilleyse lilleyse deleted the fix-wmts-sandcastle branch February 3, 2020 17:30
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.

WMTS + Time Sandcastle is broken
7 participants