Adding support for WMS-T Time parameter to WebMapServiceImageryProvider #6348

tamarmot · 2018-03-19T17:40:52Z

Implementing a fix for #2581 as I also needed this functionality to work with lunaserv.

Please review specifically line 211 in WebMapServiceImageryProvider.js
Also I wasn't sure if I had to do something else to support the pickFeatures in this case.

Happy for any feedback, thanks!
Tamar

…rovider

hpinkos · 2018-03-19T18:12:54Z

Wow, thanks @tamarmot! This has been on our to-do list for a while.

@tfili can you review?

tfili · 2018-03-19T18:26:27Z

Source/Scene/WebMapServiceImageryProvider.js

@@ -13,7 +13,8 @@ define([
        '../Core/WebMercatorTilingScheme',
        '../ThirdParty/Uri',
        './GetFeatureInfoFormat',
-        './UrlTemplateImageryProvider'
+        './UrlTemplateImageryProvider',
+        '../Scene/TimeDynamicImagery'


TimeDynamicImagery.js is in the same directory. No need to the ../Scene

thanks, will make this change

tfili · 2018-03-19T18:37:03Z

Source/Scene/WebMapServiceImageryProvider.js

+            });
+        }
+
+        if (defined(options.clock)) {


Why is the time parameter a special case? Can't the dynamic properties remain generic like WMTS?

The user can pass any parameters they want, but in this case we are building a time parameter out of the time intervals / clock information.

I have made changes, but I still get the current time from the clock and create a time parameter if it does not exist in the parameters from the interval.

We shouldn't be hard coding that time parameter. When I hooked it up into WMTS, there were many different ways server used this parameter. For instance some of them expected TIME as the parameter. Others had a parameter epoch, and time was the offset from this time or some expected time to be a range in the format start/end. In these cases, this code wouldn't work. We don't do this in WMTS and I don't think we should do it here. This needs to be generic and not specific to your use case.

tfili · 2018-03-19T18:42:22Z

Source/Scene/WebMapServiceImageryProvider.js

+
+        if (defined(dynamicIntervalData)) {
+            if (!isNaN(dynamicIntervalData)){
+                parameters['time'] = interval.start.toString();


Not exactly sure what is going on here? If dynamicIntervalData is a number, we just use the start time? Why is that?

Ah, that may have been specific to my use case. Let me look further. Good catch, thanks.

tamarmot · 2018-03-19T23:50:22Z

I have made some updates based on your feedback, and expanded the test suite. Can you take a look? I think it makes more sense now. Thanks.

tfili · 2018-03-20T18:54:00Z

Source/Scene/WebMapServiceImageryProvider.js

+        if (defined(dynamicIntervalData)) {
+            if (dynamicIntervalData instanceof Object){
+                try {
+                    Object.keys(dynamicIntervalData).forEach(function (key, index) {


This is just copying dynamicIntervalData. We can just pass that directly into setQueryParameters.

tfili · 2018-03-20T18:55:56Z

Source/Scene/WebMapServiceImageryProvider.js

+                    });
+                } catch (err){
+                    // Warn the user, this may not be a problem
+                    console.warn('Data from interval has problems.', err);


In Cesium we don't output errors to the console if its a error by the developer. We occasionally use it if there is an error from a remote server. You can search for DeveloperError to see how we use it and also remove it from the minified build. Although I anticipate that this try/catch will go away.

tfili · 2018-03-20T18:56:15Z

Source/Scene/WebMapServiceImageryProvider.js

+                }
+            }
+        }
+        if (!('time' in keys)){


This should be removed like above.

tamarmot · 2018-03-20T19:17:47Z

Thanks for the continued feedback, @tfili, I'll review and update.

GatorScott · 2018-03-20T19:39:02Z

Will you be adding support for other dimensions as was done for WMTS, including the predefined "elevation"? I use a WMS for weather forecast data that has additional dimensions "run" and "forecast".

tamarmot · 2018-03-20T19:57:28Z

Hi @GatorScott you can add any parameters you want via the data in your TimeInterval.
https://cesiumjs.org/Cesium/Build/Documentation/TimeInterval.html?classFilter=inter

tamarmot · 2018-03-20T19:58:59Z

Ok @tfili your suggestions are implemented; please especially review the code I changed in the constructor, the pushing of parameters here might be redundant as the timeDynamicImagery may automagically handle this on first load? Not sure.

tfili · 2018-03-22T18:56:19Z

@tamarmot I believe @GatorScott is referring to non-time dynamic parameters. See the dimensions option in the WebMapTileServiceImageryProvider's constructor. I agree for consistency that should be added.
Scratch that. It looks like WMS already has parameters that do this.

tfili · 2018-03-22T19:00:38Z

Source/Scene/WebMapServiceImageryProvider.js

-        if (defined(options.clock)) {
-            if (defined(options.parameters)) {
-                options.parameters['time'] = options.clock.currentTime.toString();
+            if (defined(options.clock)){


This doesn't need to be done in the constructor, you are always setting them in requestImage

tfili · 2018-03-22T19:09:43Z

Source/Scene/WebMapServiceImageryProvider.js

-        var resource = imageryProvider._tileProvider._resource; // We actually want to set the time parameter within the tile provider.
-        var parameters = {};
-        var keys = [];
+        var resource = imageryProvider._tileProvider._resource; // We actually want to set the query parameters within the tile provider.


Since we can have multiple images in flight, including some for a future time with different dynamic data, you want something like this, that will clone the resource but merge in the dynamic query parameters. Also, you don't need to check if dynamicIntervalData is an object.

You want to do something like

var resource = imageryProvider._tileProvider._resource; if (defined(dynamicIntervalData)) { resource = resource.getDerivedResource({ queryParameters: objectToLowercase(dynamicIntervalData) }); }

The reason why I am updating the parameters on the resource in the tile provider is because it is the one that is using them. If I make a copy of the resource as you suggest, then I have no way of setting that as the resource in the tile provider (in this case the UrlTemplateImageryProvider). It doesn't feel right to update the parameters on the resource in the tile provider, but the API for requestImage does not allow for passing the parameters any other way. Thoughts?

tamarmot · 2018-03-23T01:36:02Z

I've pushed the code cleanup; I am still setting the query parameters on the resource within the tile provider. ..

tamarmot · 2018-03-30T00:26:23Z

Hi @tfili any chance you might review this soon? Thanks a ton.

tfili · 2018-03-30T01:13:53Z

@tamarmot, sorry for the delay. It’s been on my list all week. I’ve been swamped. I’ll hopefully get to it tomorrow or early next week.

tfili · 2018-03-30T18:24:00Z

Source/Scene/WebMapServiceImageryProvider.js

@@ -27,7 +28,8 @@ define([
        WebMercatorTilingScheme,
        Uri,
        GetFeatureInfoFormat,
-        UrlTemplateImageryProvider) {
+        TimeDynamicImagery,
+        UrlTemplateImageryProvider        ) {


Fix white space

tfili · 2018-03-30T18:24:40Z

Source/Scene/WebMapServiceImageryProvider.js

@@ -67,6 +69,9 @@ define([
     * @param {String|String[]} [options.subdomains='abc'] The subdomains to use for the <code>{s}</code> placeholder in the URL template.
     *                          If this parameter is a single string, each character in the string is a subdomain.  If it is
     *                          an array, each element in the array is a subdomain.
+     * @param {Clock} [options.clock] A Clock instance that is used when determining the value for the time dimension. Required when options.times is specified.
+     * @param {TimeIntervalCollection} [options.times] TimeIntervalCollection with its data property being an object containing time dynamic dimension and their values.
+     *


Remove extra blank line.

tfili · 2018-03-30T18:30:53Z

Source/Scene/WebMapServiceImageryProvider.js

+        }
+
+        if (defined(options.proxy)) {
+            deprecationWarning('WebMapServiceImageryProvider.proxy', 'The options.proxy parameter has been deprecated. Specify options.url as a Resource instance and set the proxy property there.');


This deprecation was removed in the master for our release next week. deprecationWarning isn't included in this file anymore.

tamarmot · 2018-04-03T21:47:51Z

done, thanks!

…

On Tue, Apr 3, 2018 at 7:32 AM, Tom Fili ***@***.***> wrote: Looking at issue, I was able to create this example that seems to work correctly. var viewer = new Cesium.Viewer('cesiumContainer'); function dataCallback(interval, index) { var time; if (index === 0) { // leading time = Cesium.JulianDate.toIso8601(interval.stop); } else { time = Cesium.JulianDate.toIso8601(interval.start); } return { Time: time }; } var times = Cesium.TimeIntervalCollection.fromIso8601({ iso8601: '2014-06-30/2017-06-16/P1D', leadingInterval: true, trailingInterval: true, isStopIncluded: false, // We want stop time to be part of the trailing interval dataCallback: dataCallback }); var imageryLayers = viewer.imageryLayers;var provider = new Cesium.WebMapServiceImageryProvider({ url: "http://mesonet.agron.iastate.edu/cgi-bin/wms/nexrad/n0q-t.cgi?", layers: "nexrad-n0q-wmst", parameters: { format: 'image/png', transparent: 'true' }, times: times, clock: viewer.clock });imageryLayers.addImageryProvider(provider); provider.readyPromise .then(function() { var start = Cesium.JulianDate.fromIso8601('2014-06-30'); var end = Cesium.JulianDate.fromIso8601('2017-06-16'); viewer.timeline.zoomTo(start, end); var clock = viewer.clock; clock.startTime = start; clock.endTime = end; clock.currentTime = start; clock.clockRange = Cesium.ClockRange.LOOP_STOP; clock.multiplier = 86400; }); @tamarmot <https://github.com/tamarmot> Can you merge in master to resolve the conflict. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6348 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFgK8Ha7kHaFN13Yf8K23gZ-VyVHk6eGks5tk4f2gaJpZM4Swmjr> .

hpinkos · 2018-06-19T20:19:13Z

@tfili bump

Are we waiting on anything else?

tfili · 2018-06-20T17:25:19Z

Source/Scene/WebMapServiceImageryProvider.js

+        var currentInterval;
+
+        if (!defined(timeDynamicImagery)){
+            return this._tileProvider.requestImage(x, y, level, request);


This should be requestImage(this, x, y, level, request), just like below but without the interval.

tfili · 2018-06-20T17:25:54Z

CHANGES.md

@@ -57,8 +60,14 @@ Change Log
 * Fixed improper zoom during model load failure. [#6305](https://github.com/AnalyticalGraphicsInc/cesium/pull/6305)
 * Fixed rendering vector tiles when using `invertClassification`. [#6349](https://github.com/AnalyticalGraphicsInc/cesium/pull/6349)
 * Fixed occlusion when `globe.show` is `false`. [#6374](https://github.com/AnalyticalGraphicsInc/cesium/pull/6374)
+<<<<<<< HEAD


This merge has a conflict.

tfili · 2018-06-20T17:27:08Z

Yeah, I dropped the ball on merging this. Besides the bad merge in CHANGES and the one small tweak, I'm fine with merging this.

tamarmot · 2018-06-20T18:45:26Z

Thanks all. I am on vacation now back in July...

…

On Jun 20, 2018, at 7:27 PM, Tom Fili ***@***.***> wrote: Yeah, I dropped the ball on merging this. Besides the bad merge in CHANGES and the one small tweak, I'm fine with merging this. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

hpinkos · 2018-07-30T21:45:18Z

@tamarmot hop you had a good vacation! Let us know when this is ready for another look =)

gadew64 · 2018-08-21T22:54:39Z

Hi. I've been using the updated code successfully - it works great thank you thank you. However, I don't think the GetFeatureInfo request has been updated - I don't see the time param in querystrings.

gadew64 · 2018-08-22T20:42:24Z

I think I fixed this - used same approach as @tamarmot in WebMapServiceImageryProvider.js, changed/added following:

 // called if there is dynamicIntervalData.  adds that data to the pickFeaturesResource 
// queryParameters collection in order to select data by proper Time.
// added 22AUG2018
function pickFeatures(imageryProvider, x, y, level, longitude, latitude, interval) {
    var dynamicIntervalData = defined(interval) ? interval.data : undefined;
    var tileProvider = imageryProvider._tileProvider;

    if (defined(dynamicIntervalData)) {
        // We set the query parameters within the tile provider, because it is managing the query.
        tileProvider._pickFeaturesResource.setQueryParameters(dynamicIntervalData);
    }
    return tileProvider.pickFeatures(x, y, level, longitude, latitude);
}

/**
 * Asynchronously determines what features, if any, are located at a given longitude and latitude within
 * a tile.  This function should not be called before {@link ImageryProvider#ready} returns true.
 *
 * @param {Number} x The tile X coordinate.
 * @param {Number} y The tile Y coordinate.
 * @param {Number} level The tile level.
 * @param {Number} longitude The longitude at which to pick features.
 * @param {Number} latitude  The latitude at which to pick features.
 * @return {Promise.<ImageryLayerFeatureInfo[]>|undefined} A promise for the picked features that will resolve when the asynchronous
 *                   picking completes.  The resolved value is an array of {@link ImageryLayerFeatureInfo}
 *                   instances.  The array may be empty if no features are found at the given location.
 *
 * @exception {DeveloperError} <code>pickFeatures</code> must not be called before the imagery provider is ready.
 */
WebMapServiceImageryProvider.prototype.pickFeatures = function(x, y, level, longitude, latitude) {
    var result;
    var timeDynamicImagery = this._timeDynamicImagery;
    var currentInterval;

    if (!defined(timeDynamicImagery)){
        return this._tileProvider.pickFeatures(x, y, level, longitude, latitude);
    }

    currentInterval = timeDynamicImagery.currentInterval;
    result = pickFeatures(this, x, y, level, longitude, latitude, currentInterval);
    return result;
    
    // replaced single line below with code that checked for and used currentInterval 
    // to select proper feature info by Time.
    // updated 22AUG2018
    //return this._tileProvider.pickFeatures(x, y, level, longitude, latitude);
};

Look Ok?

hpinkos · 2018-08-23T12:56:46Z

@gadew64 yes, that looks fine to me. Are you interested in opening a pull request to finish this up?

tamarmot · 2018-08-23T13:27:46Z

I'd be delighted if someone else could finish up the integration, clearly I no longer have time :)

…

On Thu, Aug 23, 2018 at 8:57 AM Hannah ***@***.***> wrote: @gadew64 <https://github.com/gadew64> yes, that looks fine to me. Are you interested in opening a pull request to finish this up? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6348 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AFgK8ChieSejZULj4g16RbzLeyiYcbOdks5uTqaxgaJpZM4Swmjr> .

tfili · 2018-08-30T15:57:15Z

@tamarmot I've finished up the last changes for you. Thanks again and sorry for it taking so long.

@gadew64 I put in the code for time dynamic pickFeature. Stepping through the code, it seems to work as intended but I don't have a server to test with. Could you give me one to use or can you test this branch as-is to make sure it works for you?

heinerlamprecht · 2018-10-04T07:20:34Z

Any idea if and when this PR might be merged? This feature is very important for us. Thus I would highly appreciate the merge.

tfili · 2018-10-04T12:49:18Z

@heinerlamprecht Do you have a server that has time-dynamic feature picking to test it? Otherwise I can probably just merge it.

heinerlamprecht · 2018-10-04T13:14:09Z

@tfili No I haven't.

I need this feature, because our WMS provides frequently updated data, and the viewport needs to refresh the display accordingly. At the moment, the map is only updated if the user pans or zooms enough.

Feature picking is currently not implemented (and for this specific service not needed at all).

kring · 2018-10-04T13:25:32Z

In case it helps, here's a WMS server with a time dimension and time-dynamic GetFeatureInfo:
https://nrt-au.dea.ga.gov.au/?service=WMS&version=1.3.0&request=GetCapabilities

And here it is working in TerriaJS (but using our own WMS-T support):
https://nationalmap.gov.au/#share=s-oiUiGZAypzVsY4nxA1P2Q7vK0SA

tfili · 2018-10-04T19:03:24Z

Thanks again @tamarmot

thw0rted · 2018-11-16T11:18:44Z

Source/Scene/WebMapServiceImageryProvider.js

+         */
+        clock : {
+            get : function() {
+                return this._timeDynamicImagery.clock;


I know this is already merged, but there's a bug here. If times isn't passed in the constructor, this._timeDynamicImagery is undefined so this (and all other such references in the getters/setters below) will throw. Once a WMSImageryProvider is initialized as "non-dynamic" it's impossible to make it dynamic, so any references to dynamic properties should return undefined.

thw0rted · 2018-11-21T12:13:24Z

Hi @kring @tfili @tamarmot , I may have found a deeper issue here, with the design of TimeDynamicImagery / TimeIntervalCollection. I hope I'm just using it wrong. I can open a separate issue if need be, but the short version is:

I found this sample data in a related ticket, I think. I'm working on a simple parser for the Capabilities document, so that I can advertise layers to the user and automatically configure the supported time parameters. To that end, I'm looking at the <Dimension name="time"> tag.

On this server, the value of that tag is 2011-02-16/2018-12-31/PT5M. OK, let's set up a new WebMapServiceImageryProvider with times = TimeIntervalCollection.fromISO8601({iso8601: str}). This causes parseDuration to create 828000 JulianDates, one for every 5 minutes over 7 years or so. Of course, the browser seems to hang for a long time while this is happening. But to my understanding, this is the only way of specifying that, when the Clock is set to a given time, we need to select the 5-minute interval surrounding that time and start requesting new imagery at that time (or, arguably, for that 5-minute interval).

Is there another way to handle this? The 8601 Interval spec allows a single, simple string to specify an effectively unlimited number of intervals (0001-01-01/9999-12-31/PT1M), so instantiating each interval described, all at once, seems inefficient. Is there a callback-based way of specifying intervals, so that we just describe the mechanism that maps an instant to an interval once? Are changes needed to the code to support this?

hpinkos · 2018-11-21T14:24:34Z

@thw0rted can you please open a new issue? It'll be easier to discuss everything there. A lot of the team is off for Thanksgiving this week, but we'll try to get back to you next week. Thanks!

thw0rted · 2018-11-21T14:46:50Z

OK, no problem. I'll leave a trackback so it shows up here. Also, as an experiment, I'm designing a class that implements the TimeIntervalCollection interface but works the way I described (keeping only a start, stop, and period) to see if it works for my current needs. If it does, maybe I can clean it up to share.

tamarmot added 4 commits March 16, 2018 17:57

WebMapServiceImageryProvider now supports clock and times

9bd6f5c

code cleanup and testing for adding time to web map service imagery p…

9773587

…rovider

merged with upstream

eed558c

corrected tests

cc7d7c5

tfili requested changes Mar 19, 2018

View reviewed changes

clean up the handling of data from intervals

e3e47ba

tamarmot added 2 commits March 19, 2018 16:51

merged changes.md

7b9c87f

merged CHANGES.md

78381d8

tfili reviewed Mar 20, 2018

View reviewed changes

Source/Scene/WebMapServiceImageryProvider.js Outdated

}

}

}

if (!('time' in keys)){

Copy link

Contributor

tfili Mar 20, 2018

Choose a reason for hiding this comment

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

This should be removed like above.

refactored WebMapServiceImageryProvider to be agnostic about parameters

24b88cb

tfili reviewed Mar 22, 2018

View reviewed changes

tamarmot added 2 commits March 22, 2018 18:33

code cleanup

19b0d31

Merge remote-tracking branch 'upstream/master'

b8648f1

merged with upstream

d7e2863

tfili reviewed Mar 30, 2018

View reviewed changes

merged changes

08b64b6

tfili reviewed Jun 20, 2018

View reviewed changes

Tom Fili added 2 commits August 30, 2018 11:10

Merge remote-tracking branch 'agi/master'

42ba843

Updated CHANGES, added in time dynamic pickFeature and general cleanup.

9501620

Tom Fili added 3 commits October 4, 2018 09:37

Merged agi master

2e0327c

Fixed CHANGES typo

d4ca1aa

Fixed CHANGES typo

34a99a1

tfili merged commit 2bf42bc into CesiumGS:master Oct 4, 2018

OmarShehata mentioned this pull request Nov 2, 2018

WMS-T (time) support in WebMapServiceImageryProvider #2581

Closed

thw0rted reviewed Nov 16, 2018

View reviewed changes

thw0rted mentioned this pull request Nov 23, 2018

TimeIntervalCollection is a poor fit for very granular time ranges #7282

Open

Adding support for WMS-T Time parameter to WebMapServiceImageryProvider #6348

Adding support for WMS-T Time parameter to WebMapServiceImageryProvider #6348

Conversation

tamarmot commented Mar 19, 2018

hpinkos commented Mar 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tamarmot commented Mar 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tamarmot commented Mar 20, 2018

GatorScott commented Mar 20, 2018

tamarmot commented Mar 20, 2018

tamarmot commented Mar 20, 2018

tfili commented Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

tfili Mar 22, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tamarmot commented Mar 23, 2018

tamarmot commented Mar 30, 2018

tfili commented Mar 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tamarmot commented Apr 3, 2018 via email

hpinkos commented Jun 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tfili commented Jun 20, 2018

tamarmot commented Jun 20, 2018 via email

hpinkos commented Jul 30, 2018

gadew64 commented Aug 21, 2018

gadew64 commented Aug 22, 2018

hpinkos commented Aug 23, 2018

tamarmot commented Aug 23, 2018 via email

tfili commented Aug 30, 2018

heinerlamprecht commented Oct 4, 2018

tfili commented Oct 4, 2018

heinerlamprecht commented Oct 4, 2018

kring commented Oct 4, 2018 • edited Loading

tfili commented Oct 4, 2018

Choose a reason for hiding this comment

thw0rted commented Nov 21, 2018 • edited Loading

hpinkos commented Nov 21, 2018

thw0rted commented Nov 21, 2018

tfili commented Mar 22, 2018 •

edited

Loading

tfili Mar 22, 2018 •

edited

Loading

kring commented Oct 4, 2018 •

edited

Loading

thw0rted commented Nov 21, 2018 •

edited

Loading