implement TileMapServiceImageryProvider in terms of UrlTemplateImageryProvider

[sen-lu]

continuation of #3119
part of #2814

@pjcozzi @kring

Note that these changes cause some tests to fail, most due to floating point comparisons and some due to credit being undefined before the promise resolves. These failures are addressed in #3151

[pjcozzi]

I thought the epsilon test failures were already there on your MacBook Air? If not, we did not need to split up the pull request. No concern either way.

[pjcozzi]

I merged master (and the test fixes) into this branch.

[pjcozzi]

I get one test failure in this branch (and not in master):

[pjcozzi]

@kring can you look at this? I'm still not sure about this approach; it seems like instead of throwing a DeveloperError when a property isn't ready, now we just return the default initialized value. Perhaps we need to set ready on the UrlTemplateImageryProvider and do the checks there?

The current doc for UrlTemplateImageryProvider is confusing since it says not to call a property until ready is true, but ready is always true.

        /**
         * Gets the width of each tile, in pixels. This function should
         * not be called before {@link UrlTemplateImageryProvider#ready} returns true.
         * @memberof UrlTemplateImageryProvider.prototype
         * @type {Number}
         * @readonly
         * @default 256
         */
        tileWidth : {
            get : function() {
                return this._tileWidth;
            }
        },

[kring]

The current doc for UrlTemplateImageryProvider is confusing since it says not to call a property until ready is true, but ready is always true.

You should not call those properties until ready is true. That's part of the contract of the ImageryProvider interface. It's an implementation detail of UrlTemplateImageryProvider that ready is always true.

[kring]

You still need these checks. But instead of !this._ready use !defined(this._imageryProvider).

[kring]

Hmm ok this is actually pretty complicated. We can't create the UrlTemplateImageryProvider until we know all the parameters to create it with, yet we'd really like to use some bits of it earlier. Specifically, we need to expose its errorEvent immediately after the constructor returns so that users can subscribe to it and be notified when something (a tile failure or a metadata failure) goes wrong. Ideally, we'd also be able to directly access its properties, like tileWidth, and have it throw an exception if it's not ready yet.

I think the only sensible way to do that is to change how UrlTemplateImageryProvider works. Here's what I have in mind, though I haven't really tested it:
https://github.com/AnalyticalGraphicsInc/cesium/compare/deferredUrlTemplate

With changes like that, we can construct an instance of UrlTemplateImageryProvider in the constructor for TileMapServiceImageryProvider:

this._imageryProvider = new UrlTemplateImageryProvider({
    deferReadiness: true
});

And then delegate most of our properties directly to that instance right away. After we load the TMS metadata, we call reinitialize on the UrlTemplateImageryProvider.

Does that all make sense?

[sen-lu]

@kring Yes, that makes perfect sense! This is actually almost exactly what @pjcozzi and I were talking about a few days ago, and something I was planning on experimenting with on my own.

What can @adamdavidcole and I do to help? Do you want us to pause work on implementing the rest of the imagery providers until your updates are merged, or would you like us to help with testing, etc.?

[kring]

@TiffanyLu I'm happy for you to merge my deferredUrlTemplate branch into your branch and run with it from there. I created it just to demonstrate the concept. If you could flesh it out with tests and such, that would be great!

[pjcozzi]

This all sounds great! @kring thanks for looking into this.

[kring]

@mramato your second suggestion is a cool idea (the first would be terrible IMO, sorry!). One downside is that the current reinitialize thing lets people do crazy things like change their URL parameters on the fly. At least one user on the forum wanted this, and thanked me when I implemented it. See the bottom of here:
https://github.com/AnalyticalGraphicsInc/cesium/compare/deferredUrlTemplate

So we'll lose that with the promise-based approach.

@TiffanyLu to answer part of your question that I think Matt missed:
Instead of raising errorEvent directly inside the load callback, you should cause the promise to be rejected (throwing an exception will do the trick). Then, inside UrlTemplateImageryProvider, you should add a .otherwise that handles the rejection by raising the errorEvent.

[sen-lu]

@kring How can I structure the .otherwise inside UrlTemplateImageryProvider such that I can pass in the information necessary to raise the errorEvent specific to each imagery provider? Or should I wrap the errorEvent handling in createTileMapServiceImageryProvider within a call to deferred.otherwise as well? Also, following style guidelines, is there a certain exception I should be throwing?

[pjcozzi]

@TiffanyLu any update here?

[sen-lu]

@mramato @pjcozzi refactored UrlTemplateImageryProvider to accept a promise to options. A few remaining issues:

• @kring still unsure how to do errorEvent handling (see above comment)
• two tests are failing:

  • Scene/UrlTemplateImageryProvider requires the url to be specified

    expect(function() { return new UrlTemplateImageryProvider({}); }).toThrowDeveloperError() fails, even though when I step through the creation of the UrlTemplateImageryProvider it breaks into the error handling block

  • Scene/UrlTemplateImageryProvider resolves readyPromise
    upon calling provider.readyPromise.then(function(result) {...}), result is undefined

[mramato]

@TiffanyLu to fix the first test error, put the following code at the very top of the function.

//>>includeStart('debug', pragmas.debug);
if (!defined(options)) {
    throw new DeveloperError('options is required.');
}
if (!when.isPromise(options) && !defined(options.url)) {
    throw new DeveloperError('options is required.');
}
//>>includeEnd('debug');

The problem is that we used to fail immediately if options or options.url was undefined. Without the above code, the failure doesn't happen until the promise is rejected/resolved. Now we fail immediately if the value isn't a promise.

For the result undefined problem, you need to add a return true; statement to the end of the callback (after the line that._pickFeaturesUrlParts = ...). Since you are not currently returning a value, the result of the promise is undefined.

This still leaves one more failure, in createTileMapServiceImageryProvider. Line 167 should be deferred.reject not imageryProvider.readyPromise.reject.

After that, everything should pass.

[mramato]

@TiffanyLu To address your question regarding the errorEvent, I think what @kring was saying was to add an .otherwise to the readyPromise to raise the event inside UrlTemplateIImageryProvider

Specifically in the constructor's promise chain:

this._readyPromise = when(options).then(function(properties) {
    ...initialization code
}).otherwise(function(e){
    that._errorEvent.raiseEvent(e);
});;

However, since TileProviderError.handleError is handling raising the event and part of my comment to change 167 to use deferred.reject instead, I think this largely resolves the issue. One last thing you'll want to change is wrap the deferred.reject(new RuntimeError(message)); call in an if block in case there is a retry:

if(!metadataError.retry) {
    deferred.reject(new RuntimeError(message));
}

[mramato]

After merging in master, these changes need to go into the 1.17 section.

[mramato]

@TiffanyLu, I just noticed your recent commits. Is this ready?

[sen-lu]

@mramato yes it is!

[mramato]

@TiffanyLu I'm a little confused. It looks like reinitialize has been removed from UrlTemplateImageryProvider? I thought we wanted to be sure to have that.

Also, there are a few jsHint errors, please fix them.

[mramato]

Please remove this whitespace.

[mramato]

There is some trivial missing coverage in the unit tests for the new code. If you're on Windows you can run coverage but if you're on a Mac or Linux I'm okay with just just addressing this in a future PR, since Windows is the only way to run coverage right now.

[sen-lu]

@mramato I thought earlier discussion proposed that we replace the reinitialize function with accepting a promise to options. Was that not the case?

Regarding test coverage, I'm on a Mac.

[mramato]

@kring mentioned that it was a feature a lot of people desired, so I assumed that meant we were keeping it. It should be trivial to put back, since it's basically where all of the constructor work is done.

[hpinkos]

Closing this because I finished making the changes in #3460