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

implement TileMapServiceImageryProvider in terms of UrlTemplateImageryProvider #3150

Closed
wants to merge 40 commits into from

Conversation

Projects
None yet
5 participants
@tiffanylu
Copy link
Contributor

commented Oct 30, 2015

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

This comment has been minimized.

Copy link
Member

commented Oct 30, 2015

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

This comment has been minimized.

Copy link
Member

commented Oct 30, 2015

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

@pjcozzi

This comment has been minimized.

Copy link
Member

commented Oct 30, 2015

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

image

@pjcozzi

This comment has been minimized.

Copy link
Member

commented Oct 30, 2015

@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

This comment has been minimized.

Copy link
Member

commented Nov 2, 2015

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.

@@ -315,13 +315,7 @@ define([
*/
tileWidth : {
get : function() {
//>>includeStart('debug', pragmas.debug);
if (!this._ready) {

This comment has been minimized.

Copy link
@kring

kring Nov 2, 2015

Member

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

@@ -95,28 +95,12 @@ define([
}
//>>includeEnd('debug');

var url = options.url;
var url = appendForwardSlash(options.url);

this._url = url;
this._ready = false;

This comment has been minimized.

Copy link
@kring

kring Nov 2, 2015

Member

Remove this.

@kring

This comment has been minimized.

Copy link
Member

commented Nov 2, 2015

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?

@tiffanylu

This comment has been minimized.

Copy link
Contributor Author

commented Nov 2, 2015

@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

This comment has been minimized.

Copy link
Member

commented Nov 2, 2015

@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 comment has been minimized.

Copy link
Member

commented Nov 3, 2015

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

@kring

This comment has been minimized.

Copy link
Member

commented Nov 25, 2015

@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.

@tiffanylu

This comment has been minimized.

Copy link
Contributor Author

commented Nov 25, 2015

@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

This comment has been minimized.

Copy link
Member

commented Dec 1, 2015

@tiffanylu any update here?

@tiffanylu

This comment has been minimized.

Copy link
Contributor Author

commented Dec 2, 2015

@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

This comment has been minimized.

Copy link
Member

commented Dec 9, 2015

@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

This comment has been minimized.

Copy link
Member

commented Dec 9, 2015

@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));
}
CHANGES.md Outdated
@@ -7,6 +7,8 @@ Change Log
* Deprecated `HeightmapTessellator`. It will be removed in 1.17.
* Deprecated `TerrainMesh`. It will be removed in 1.17.
* Deprecated `OpenStreetMapImageryProvider`. It will be removed in 1.18. Use `createOpenStreetMapImageryProvider` instead.
* Deprecated `TileMapServiceImageryProvider`. It will be removed in 1.18. Use `createTileMapServiceImageryProvider` instead.

This comment has been minimized.

Copy link
@mramato

mramato Dec 9, 2015

Member

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

@mramato

This comment has been minimized.

Copy link
Member

commented Dec 14, 2015

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

@tiffanylu

This comment has been minimized.

Copy link
Contributor Author

commented Dec 16, 2015

@mramato yes it is!

@mramato

This comment has been minimized.

Copy link
Member

commented Dec 16, 2015

@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.

* Reduced the amount of both GPU and CPU memory used by terrain. The CPU memory was reduced by up to 40%.
* `CorridorGeometry` and `PolylineVolumeGeometry` render short segments [#3293](https://github.com/AnalyticalGraphicsInc/cesium/issues/3293)
* `Rectangle.fromCartographicArray` finds the smallest rectangle regardess of whether or not it crosses the international date line. [#3227](https://github.com/AnalyticalGraphicsInc/cesium/issues/3227)
* Bug fix for `CorridorGeometry` with nearly colinear points [#3320](https://github.com/AnalyticalGraphicsInc/cesium/issues/3320)


This comment has been minimized.

Copy link
@mramato

mramato Dec 16, 2015

Member

Please remove this whitespace.

@@ -14,11 +14,15 @@ Change Log
* Removed `HeightmapTessellator` from the public API. It is now private and subject to change without notice.
* Removed `TerrainMesh` from the public API. It is now private and subject to change without notice.
* Removed `jsonp`. Use `loadJsonp` instead.
* Deprecated
* Deprecated `TileMapServiceImageryProvider`. It will be removed in 1.18. Use `createTileMapServiceImageryProvider` instead.
* Refactored `UrlTemplateImageryProvider.reinitialize` to accept a promise to `options`.

This comment has been minimized.

Copy link
@mramato

mramato Dec 16, 2015

Member

Let's change this to:

`UrlTemplateImageryProvider` and `UrlTemplateImageryProvider.reinitialize` now accept a promise to an `options` object in addition to taking the object directly.
@mramato

This comment has been minimized.

Copy link
Member

commented Dec 16, 2015

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.

@tiffanylu

This comment has been minimized.

Copy link
Contributor Author

commented Dec 16, 2015

@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

This comment has been minimized.

Copy link
Member

commented Dec 18, 2015

@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

This comment has been minimized.

Copy link
Contributor

commented Jan 25, 2016

Closing this because I finished making the changes in #3460

@hpinkos hpinkos closed this Jan 25, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.