Add UrlTemplateImageryProvider #2795

kring · 2015-06-09T12:43:31Z

This is a continuation of the work started by @kaktus40.

I tweaked it to make it more similar to Leaflet's TileLayer, and to match up with Cesium coding standards better.

…rMoreVersatile

…rovider

kaktus40 · 2015-06-09T17:48:23Z

Hello,
I have a javascript newbie question: with you implementation is it safe to use, at the same time, two instances of UrlTemplateImageryProvider (see the "private global members" projectedScratchComputed and degreesScratchComputed) ?
There is no risk of competition when the application will request a list of tiles from the two providers?

pjcozzi · 2015-06-09T21:30:13Z

Source/Scene/UrlTemplateImageryProvider.js

+        this._tileWidth = defaultValue(options.tileWidth, 256);
+        this._tileHeight = defaultValue(options.tileHeight, 256);
+        this._minimumLevel = defaultValue(options.minimumLevel, 0);
+        this._maximumLevel = defaultValue(options.maximumLevel, 18);


It's fine if this is outside the scope of this PR, but are we sure we want to keep this default as 18? We have ran into issues where users thought Cesium couldn't handle their high-res imagery because of this. Are we paying a big cost by increasing it?

Assuming we just can't make it some high number by default, why not just force the user to specify it? 18 is just a guess on our part and in general guesses are bad defaults.

I think it would be fine to increase it to, say 25. The only cost is errors in the console when you zoom past the level that actually exists. But that happens anytime the extent is not global or the quadtree has different depths in different branches, too, so I'm not really worried about it.

25 or whatever you think is reasonable is OK with me. Can you make the update throughout in this PR?

pjcozzi · 2015-06-09T21:32:55Z

Source/Scene/UrlTemplateImageryProvider.js

+        return url;
+    }
+
+    defineProperties(UrlTemplateImageryProvider.prototype, {


Don't we usually include a @default with properties?

pjcozzi · 2015-06-09T21:40:42Z

@kring are you happy with the test coverage (sorry, I'm on Mac right now).

pjcozzi · 2015-06-09T21:41:38Z

Do we want to rewrite (now or later) any of our imagery providers to use this under the hood?

pjcozzi · 2015-06-09T21:42:11Z

Source/Scene/UrlTemplateImageryProvider.js

+     * @param {Number} [options.tileWidth=256] Pixel width of image tiles.
+     * @param {Number} [options.tileHeight=256] Pixel height of image tiles.
+     *
+     * @see ArcGisMapServerImageryProvider


Should we update the doc for these to point here?

pjcozzi · 2015-06-09T22:00:48Z

Can we add this to the Sandcastle example to make the feature easier to find for users browsing the examples?

kring · 2015-06-10T02:31:06Z

I have a javascript newbie question: with you implementation is it safe to use, at the same time, two instances of UrlTemplateImageryProvider (see the "private global members" projectedScratchComputed and degreesScratchComputed) ?
There is no risk of competition when the application will request a list of tiles from the two providers?

No, because JavaScript has no shared-memory threading. You never need to worry about two threads accessing those variables at the same time. You do need to worry about re-entrancy (i.e. you get partway through a process that uses some memory, and then cycle back to start a new operation that uses the same memory), but that can't happen here.

We use this trick all over the place in Cesium to reduce the need to construct new objects, which improves performance.

kring · 2015-06-10T02:35:54Z

@kring are you happy with the test coverage (sorry, I'm on Mac right now).

No idea, I developed this on Linux. I think I hit the major paths, but I'm sure I missed a bunch of DeveloperErrors and such.

I'll try to check it out tonight on a Windows machine.

kring · 2015-06-10T02:51:24Z

Do we want to rewrite (now or later) any of our imagery providers to use this under the hood?

I'm actually tempted to deprecate some of our imagery providers, such as OpenStreetMapImageryProvider and TileMapServiceImageryProvider. Let me think about it more.

mramato · 2015-06-10T03:05:54Z

I'm actually tempted to deprecate some of our imagery providers, such as OpenStreetMapImageryProvider and TileMapServiceImageryProvider. Let me think about it more.

I totally support this idea, assuming it's not a step back in usability.

kring · 2015-06-10T03:08:48Z

I totally support this idea, assuming it's not a step back in usability.

Well it would be, a bit. You'd have to know how to construct a tile URL with appropriate placeholders. Currently you just need to know whether it's a TMS-y or OSM-y tile service. And in the TMS case it will query tilemapresource.xml to figure out some things for you.

But I think all this might be outside the scope of Cesium. Libraries / plugins separate from Cesium could provide these capabilities and much more (like @kaktus40's OGC querying stuff). Unfortunately it's currently much too hard to use plugins with Cesium, so that's still a big step backward in usability.

pjcozzi · 2015-06-10T11:41:08Z

Wouldn't a good first step be to replace TMS/OSM/etc implementations to delegate to this new type? It reduces the amount of code we have and delays the plugin discussion until we are in a better position to support it.

pjcozzi · 2015-06-10T11:41:59Z

No idea, I developed this on Linux. I think I hit the major paths, but I'm sure I missed a bunch of DeveloperErrors and such.

91% - some low-hanging fruit left.

kring · 2015-06-10T12:24:47Z

Ok everything here is addressed, except:

I haven't updated the Sandcastle example. A little thought needs to go into this because that example is meant to mirror the imagery layers tutorial. I guess I should just add a new layer to that example (rather than switching an existing one to use this provider), but I'm not sure yet which one.
I haven't changed the other imagery providers to be implemented in terms of this one. I agree that's a reasonable thing to do.

I'll get to these things as soon as I can, hopefully tomorrow night. This is in a pretty good state for merging, anyway, if you want to do so in the meantime.

kaktus40 · 2015-06-15T10:18:45Z

Hello, I made url member settable in order to have a workaround for using dimensions (time, elevation or others) by changing the url on the fly. With the tweaks, it's not possible. Except for time and elevation, the others dimension names don't follow a nomenclature and also I'd like to make a suggestion: is it possible to have some keywords defining during instantiation of the class and use this user keywords later?
An example: the WMServer has tiles with wavelength bands dimension (number in Angstrom)

<Dimension name="wavelength" units="Angstrom" unitSymbol="Ao">3000,4000,5000,6000<Dimension>

For requesting the tiles in a wavelength of 4000 Angstrom, I should add to the url "&DIM_WAVELENGTH=4000".
My suggestion:

var wms = new Cesium.UrlTemplateImageryProvider({
url : 'https://programs.communications.gov.au/geoserver/ows?tiled=true&' +
'transparent=true&format=image%2Fpng&exceptions=application%2Fvnd.ogc.se_xml&' +
'styles=&service=WMS&version=1.1.1&request=GetMap&' +
'layers=public%3AMyBroadband_Availability&srs=EPSG%3A3857&' +
'bbox={westProjected}%2C{southProjected}%2C{eastProjected}%2C{northProjected}&' +
'width=256&height=256&DIM_WAVELENGTH={wavelength}&TIME={time}',
rectangle : Cesium.Rectangle.fromDegrees(96.799393, -43.598214999057824, 153.63925700000001, -9.2159219997013),
// wms will forge url with two dimensions : time with the value '2015-06-03' and wavelength with the value 3000.
user_keywords: {wavelength:3000,time:'2015-06-03'}
});
//request some tiles with time dimension equals to '2015-06-03' and wavelength band equals to 3000 Angstrom
wms.requestImage(1,2,3);
wms.requestImage(3,2,3);
...
//Changes the time
wms.changeValueUserKeywords({time:'2015-05-10'});
wms.requestImage(1,2,3);
//Changes the wavelength
wms.changeValueUserKeywords({wavelength:4000});
wms.requestImage(3,2,3);
//Changes two dimensions
wms.changeValueUserKeywords(wavelength:5000,time:'2015-04-15');
wms.requestImage(3,2,3);

kring · 2015-06-15T13:11:14Z

I made url member settable in order to have a workaround for using dimensions (time, elevation or others) by changing the url on the fly.

@kaktus40 I don't think changing the URL on the fly will actually work anyway, even in your original implementation. Did you try it? Changing the URL after tiles have already been downloaded will not cause those tiles to be re-downloaded. So changing the URL will just make your map inconsistent (some tiles with the old parameter, some with the new). A much better solution is to create a new imagery provider with the new parameter, and then you can use the alpha=0 trick to cause that new imagery provider to be loaded before it is needed.

Regarding user-specified parameter placeholders: I thought about adding this, but couldn't think of a good use-case, and it would add a lot of complexity. Basically, I don't think a mutable url property is useful, and in the absence of a mutable url property I don't think user-specified parameter placeholders are useful either.

pjcozzi · 2015-06-15T14:16:22Z

@kring is this ready? Please merge in master. We can submit an issue for this:

I haven't changed the other imagery providers to be implemented in terms of this one. I agree that's a reasonable thing to do.

kaktus40 · 2015-06-15T14:42:57Z

I was aware of the tiles inconsistency if the value of a user keyword was changed. The use case for me was to refresh the earth on the fly (leaded by user in another method) after defining a value for a dimension. In this use case, the user accepts the latency of refreshing the world.

kaktus40 · 2015-06-15T20:43:45Z

To illustrate the relativity of inconsistency, I added some pictures provided by bingImageryProvider. It seems the tiles were taken from different times and the result isn't shocking. When I zoom, the inconsistency disappears.

kring · 2015-06-16T00:54:22Z

I was aware of the tiles inconsistency if the value of a user keyword was changed.

It would be inappropriate for Cesium to let users create this kind of non-deterministic inconsistency in their application. At some point, we'll probably teach Cesium how to use dynamic imagery, and at that point we can upgrade UrlTemplateImageryProvider to allow user-specified parameters. Until then, if you really need this (IMO very undesirable) behavior of showing half the globe with one parameter and half with the other, you can hack something up to do it. Create a new UrlTemplateImageryProvider with the new URL and then monkey patch the _urlParts property from the new provider into the old one, maybe. But Cesium doesn't need to make that sort of thing easy.

mramato · 2015-06-16T00:58:45Z

I agree with @kring here and this behavior is consistent with the rest of the imagery providers.

…rovider Conflicts: CHANGES.md

kring · 2015-06-16T11:28:26Z

@kring is this ready? Please merge in master.

Yep, ready.

I started implementing the other imagery providers in terms of this one in the useUrlTemplateImageryProvider branch, and it's a thing of beauty. WebMapServiceImageryProvider becomes trivial, because I added template-based pickFeatures logic to UrlTemplateImageryProvider, too. It's going to take me awhile to convert everything over, though, so I support merging this as-is in the meantime.

pjcozzi · 2015-06-16T20:50:49Z

Sounds awesome, I submitted #2814.

pjcozzi · 2015-06-16T21:05:08Z

All looks good, thanks again @kaktus40 and @kring. This is a great feature!

…yProvider Add UrlTemplateImageryProvider

kaktus40 and others added 9 commits May 14, 2015 07:51

adding a schema

4952b66

replace schema by pattern

49f2dcf

Merge remote-tracking branch 'upstream/master' into TMSImageryProvide…

b177ff3

…rMoreVersatile

create a new ImageryPovider working on a template

3e45e48

adding myself in CONTRIBUTORS.md

895dc2b

Merge remote-tracking branch 'upstream/master' into TMSImageryProvide…

bc6deb6

…rMoreVersatile

UrlTemplateImageryProvider improvements and specs.

1541ac7

Merge remote-tracking branch 'origin/master' into UrlTemplateImageryP…

08f1b06

…rovider

Update CHANGES.md.

d3dbf89

Whitespace

c70843e

pjcozzi reviewed Jun 9, 2015
View reviewed changes

Whitespace

5987e69

pjcozzi reviewed Jun 9, 2015
View reviewed changes

pjcozzi mentioned this pull request Jun 9, 2015

Add UrlTemplateImageryProvider #2709

Closed

pjcozzi reviewed Jun 9, 2015
View reviewed changes

Changes from pull request feedback.

01a7e70

Improve test coverage.

79a891d

Merge remote-tracking branch 'origin/master' into UrlTemplateImageryP…

e05a986

…rovider Conflicts: CHANGES.md

Encode substituted URL parameters.

91e9583

pjcozzi added a commit that referenced this pull request Jun 16, 2015

Merge pull request #2795 from AnalyticalGraphicsInc/UrlTemplateImager…

c8bd90e

…yProvider Add UrlTemplateImageryProvider

pjcozzi merged commit c8bd90e into master Jun 16, 2015

pjcozzi deleted the UrlTemplateImageryProvider branch June 16, 2015 21:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add UrlTemplateImageryProvider #2795

Add UrlTemplateImageryProvider #2795

kring commented Jun 9, 2015

kaktus40 commented Jun 9, 2015

pjcozzi Jun 9, 2015

mramato Jun 10, 2015

kring Jun 10, 2015

pjcozzi Jun 10, 2015

pjcozzi Jun 9, 2015

pjcozzi commented Jun 9, 2015

pjcozzi commented Jun 9, 2015

pjcozzi Jun 9, 2015

pjcozzi commented Jun 9, 2015

kring commented Jun 10, 2015

kring commented Jun 10, 2015

kring commented Jun 10, 2015

mramato commented Jun 10, 2015

kring commented Jun 10, 2015

pjcozzi commented Jun 10, 2015

pjcozzi commented Jun 10, 2015

kring commented Jun 10, 2015

kaktus40 commented Jun 15, 2015

kring commented Jun 15, 2015

pjcozzi commented Jun 15, 2015

kaktus40 commented Jun 15, 2015

kaktus40 commented Jun 15, 2015

kring commented Jun 16, 2015

mramato commented Jun 16, 2015

kring commented Jun 16, 2015

pjcozzi commented Jun 16, 2015

pjcozzi commented Jun 16, 2015

Add UrlTemplateImageryProvider #2795

Add UrlTemplateImageryProvider #2795

Conversation

kring commented Jun 9, 2015

kaktus40 commented Jun 9, 2015

pjcozzi Jun 9, 2015

Choose a reason for hiding this comment

mramato Jun 10, 2015

Choose a reason for hiding this comment

kring Jun 10, 2015

Choose a reason for hiding this comment

pjcozzi Jun 10, 2015

Choose a reason for hiding this comment

pjcozzi Jun 9, 2015

Choose a reason for hiding this comment

pjcozzi commented Jun 9, 2015

pjcozzi commented Jun 9, 2015

pjcozzi Jun 9, 2015

Choose a reason for hiding this comment

pjcozzi commented Jun 9, 2015

kring commented Jun 10, 2015

kring commented Jun 10, 2015

kring commented Jun 10, 2015

mramato commented Jun 10, 2015

kring commented Jun 10, 2015

pjcozzi commented Jun 10, 2015

pjcozzi commented Jun 10, 2015

kring commented Jun 10, 2015

kaktus40 commented Jun 15, 2015

kring commented Jun 15, 2015

pjcozzi commented Jun 15, 2015

kaktus40 commented Jun 15, 2015

kaktus40 commented Jun 15, 2015

kring commented Jun 16, 2015

mramato commented Jun 16, 2015

kring commented Jun 16, 2015

pjcozzi commented Jun 16, 2015

pjcozzi commented Jun 16, 2015