Upend resource loading approach #6035

hpinkos · 2017-12-06T22:16:26Z

Not ready to merge, looking for early feedback on this approach.

An alternate solution for #6019 (based on #6019 (comment))

Instead of adding yet another argument to loadWithXhr, this creates a Resource class that handles all the arguments used by loadWithXhr and methods for getting a derived resource with a modified url from the parent resource.

Resource will also help us replace all the joinUrls and crazy url string concatenation we do all over the code base. Instead of building a url with a query string by appending them together, it has separate url, queryParameter and hash properties so we can make changes to them separately. The getter for url builds the final url from all of these pieces.

@mramato @shunter what do you think of this approach vs #6019? Is this what you had in mind?

The changes I made thus far were fairly painless. I left the more complicated ones for the end:

Other TODOs:

shunter

I think this is a good start and fits what I had imagined. I see there is still a lot of cleanup to go through. Hopefully in the end a lot of this code will seem simpler than it previously did.

shunter · 2017-12-06T22:33:58Z

Source/Core/Resource.js

+                    uri.query = undefined;
+                }
+
+                if (defined(uri.fragment)) {


Fragment is an interesting case, since it's not actually sent to the server, so technically it's can't affect a remote resource in any way. I suppose it makes sense since this encapsulates an URI, though. I would be curious if any of the existing functionality actually makes use of fragments (I suspect not).

shunter · 2017-12-06T22:35:04Z

Source/Core/Resource.js

+
+        this._url = '';
+        this._queryParameters = defaultValue(options.queryParameters, {});
+        this._hash = defaultValue(options.hash, '');


Probably prefer the term "fragment" to "hash".

shunter · 2017-12-06T22:37:21Z

Source/Core/Resource.js

+        this.url = options.url;
+
+        this.headers = options.headers;
+        this.request = options.request;


I suspect we can continue further and inline the contents of Request into Resource as well.

shunter · 2017-12-06T22:41:56Z

Source/Core/CesiumTerrainProvider.js

@@ -629,7 +635,13 @@ define([
            extensionList.push('watermask');
        }

-        var promise = loadArrayBuffer(url, getRequestHeader(extensionList), request);
+        var resource = this._resource.getDerivedResource({
+            url: urlTemplates[(x + tmsY + level) % urlTemplates.length].replace('{z}', level).replace('{x}', x).replace('{y}', tmsY),


URL template interpolation might be another good candidate to move into Resource, since we do this in a number of places. Basically it could accept a templated URL and a dictionary of key-value parameters.

a-stacey · 2017-12-14T06:15:39Z

I know that this is early stage code looking for feedback, but I was wondering whether any thought had gone into what options in retryOnError(options) was going to look like https://github.com/AnalyticalGraphicsInc/cesium/pull/6035/files#diff-2fc7c9967319d4e36d2cfaa797fd2326R164?

I have recently been working on implementing refreshing tokens for ArcGisMapServerImageryProvider and being able to detect and update the token specifically when the token expires is quite essential (to make sure that the token refresh code can specifically renew the token only when this is known or likely to be the underlying cause):
https://github.com/TerriaJS/cesium/pull/25/files#diff-0aee433aff2911bddecaa6f3d1515d8dR585
https://github.com/TerriaJS/cesium/pull/25/files#diff-0aee433aff2911bddecaa6f3d1515d8dR651

It seems that the structure in this branch/PR allows much more scope for options to include the status code when present then the structure on #6019 and in particular the issue described by @mramato comment on the status code response #6019 (comment).

samherrmann · 2017-12-14T14:17:35Z

Would this solution also support the loading of Billboard images?

mramato · 2017-12-14T14:25:45Z

@a-stacey The goal is to definitely provide the status code where possible. (any time the browser returns it)

@samherrmann Yes, the goal of this approach is that anywhere Cesium currently takes a string url/uri will now be able to take a Resource object as well.

Glad to see we already have two people interested in this PR, I think it will fix a lot of long-standing issues with how Cesium handles external requests.

shunter · 2017-12-14T14:48:13Z

Copying my comment which I accidentally left on the old PR:

Also consider the differences in how images are requested by imagery providers - loadImage won't be able to handle request headers, for example. Automatic transparent fallback to loadImageViaBlob can be done if the request needs it (for performance reasons, it should prefer direct loadImage if it will work for a given resource).

See discussion thread: https://groups.google.com/d/topic/cesium-dev/3NxyEFmMzDM/discussion

… all working with resources.

…eoJson/CZML datasources.

mramato · 2018-01-17T16:14:16Z

Source/Core/loadJsonp.js

+            callbackParameterName = options.callbackParameterName;
+        }
+        if (defined(request)) {
+            //TODO deprecate


mramato · 2018-01-17T16:14:57Z

Source/Core/loadText.js

-        });
+    function loadText(urlOrResource, headers, request) {
+        if (typeof urlOrResource === 'string') {
+            urlOrResource = new Resource({


Missing deprecation if headers or request is provided?

mramato · 2018-01-17T16:28:18Z

Source/Scene/DiscardMissingTileImagePolicy.js

@@ -87,7 +89,7 @@ define([
            that._isReady = true;
        }

-        when(loadImageViaBlob(options.missingImageUrl), success, failure);
+        when(loadImageViaBlob(new Resource({url: options.missingImageUrl})), success, failure);


This file can be reverted.

mramato · 2018-01-17T16:30:28Z

Source/Scene/ImageryProvider.js

+            });
+        }
+        if (defined(request)) {
+            //TODO deprecation warning


mramato · 2018-01-17T16:38:37Z

Phew, I started to run out of steam at the end there, but I think that's everything I have for my first pass. Awesome work @tfili (and thank you @hpinkos for getting the ball rolling).

My main concern is the whole race condition/modification of resource issue I brought up in #6035 (comment)

I would really appreciate another full set of eyeballs on this (particular your @shunter) so let me know what bribes you are currently accepting 😄

Added workaround for broken tilesets.

tfili · 2018-01-18T22:55:54Z

Ok, I think this is ready for another look.

mramato · 2018-01-18T23:04:02Z

Thanks @tfili, I'll do another pass ASAP.

mramato · 2018-01-19T00:36:26Z

@tfili is that last checkbox done, or is that still TODO?

mramato · 2018-01-19T00:51:12Z

@tfili I'm getting 161 failures running the tests locally, do they pass for you? (I'm on Linux, I'll check the Windows side and see if it happens there too). I'm surprised travis is passing.

mramato · 2018-01-19T01:16:10Z

@tfili never mind, false alarm. Looks like a GPU driver update was causing the issues.

mramato · 2018-01-19T01:32:54Z

Source/Core/Request.js

+     *
+     * @returns {Request} The modified result parameter or a new Resource instance if one was not provided.
+     */
+    Request.prototype.clone = function(result) {


Looks like you have a comment in Resource.clone about purposefully not cloning the request, so is this still used or should this file be reverted?

mramato · 2018-01-19T01:34:40Z

I think that's the only other comments I have. Does anyone else want to look at this? At the very least it would be good if someone hammered on it some more in case there was any breaking I missed. (but it will also have 2 weeks in master to settle down before release, so we'll be fine either way).

tfili · 2018-01-19T02:28:00Z

@mramato I reverted Request.js and yes, the checkbox was already done. I think this is good unless someone else has something.

pjcozzi · 2018-01-19T13:18:48Z

I don't have bandwidth to review, but impressive stats @tfili @hpinkos!

mramato · 2018-01-19T15:15:30Z

Thanks again @hpinkos and @tfili! Plenty of time until the next release in case we missed anything.

cesium-concierge · 2018-01-19T15:15:36Z

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

https://groups.google.com/d/topic/cesium-dev/3NxyEFmMzDM/discussion

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

The issue at #6035 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.

I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

shunter · 2018-02-21T22:48:59Z

Source/Scene/BingMapsImageryProvider.js

+        });
+
+        urlResource.addQueryParameters({
+            key: this._key


Adding the key here results in it being sent with every tile request, which is not correct. The key is only supposed to be sent with the metadata request, as it was in the previous code. I'll open a PR.

hpinkos added 3 commits December 6, 2017 12:44

load functions

973cfe3

core

9ccb83c

renderer, most of scene

acedbdb

shunter reviewed Dec 6, 2017

View reviewed changes

hpinkos added 2 commits December 7, 2017 16:12

Merge branch 'master' into upend-resource-loading

388869e

templating [ci skip]

6188fd7

hpinkos and others added 8 commits December 18, 2017 11:24

Merge branch 'master' into upend-resource-loading

b0f4281

remaining imagery providers [ci skip]

18f9d51

Got ArcGisMapServerImageryProvider, BingMapsImageryProvider and Model…

5d92dd9

… all working with resources.

Fixed a bunch more resource code.

2b7fafc

Fixed UrlTemplateImageryProvider

9e66863

Got almost all imagery working and fixed Core/load*.js functions.

9fb5bfd

Fixed Terrain providers.

dc77bbd

Finished Model, ModelInstanceCollection, SingleTileImageryProvider, G…

c9d1f6d

…eoJson/CZML datasources.

hpinkos mentioned this pull request Jan 11, 2018

RequestOptions for handling refresh tokens and http request failures #6019

Closed

Tom Fili added 11 commits January 11, 2018 12:20

Most of KML is done.

b68a1e8

Trying to fix sourceUri's for Datasources.

56ce327

Cleanup of resource and finished KML.

811982f

Got tests passing.

6eaee54

Cleanup and eslint fixes.

5255195

Got all tests passing. Added some doc.

e4bf6dd

Added retry logic.

ef42d79

Fixed tests, KML and WMTS.

9849fc8

Merged in master.

194d522

Fixed tests.

c507b39

Added error to retry logic and fixed entity models.

bdd5fe6

mramato reviewed Jan 17, 2018

View reviewed changes

Source/Scene/ImageryProvider.js Outdated

});

}

if (defined(request)) {

//TODO deprecation warning

Copy link

Contributor

mramato Jan 17, 2018

Choose a reason for hiding this comment

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

TODO

Tom Fili and others added 7 commits January 17, 2018 14:28

Added workaround for broken tilesets.

fda9e3c

Merge pull request #6125 from AnalyticalGraphicsInc/root-tiles-hack

ca33f8c

Added workaround for broken tilesets.

Fixes from PR comments.

998f293

More cleanup from PR comments.

abd04ab

Removed isDirectory property.

b4edc87

Fixed CesiumTerrainProvider parentUrl resolving.

9a528e9

Update CHANGES.md

a9e489e

mramato reviewed Jan 19, 2018

View reviewed changes

Tom Fili added 2 commits January 18, 2018 21:16

Reverted request changes that aren't needed.

303eef6

Reverted request changes that aren't needed.

366db6d

mramato merged commit 9dc0836 into master Jan 19, 2018

mramato deleted the upend-resource-loading branch January 19, 2018 15:15

shunter reviewed Feb 21, 2018

View reviewed changes

shunter mentioned this pull request Feb 21, 2018

Don't send key with every Bing tile request. #6250

Merged

shunter mentioned this pull request Mar 21, 2019

Fix Resource template replacement. #7668

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upend resource loading approach #6035

Upend resource loading approach #6035

hpinkos commented Dec 6, 2017 •

edited by tfili

Loading

shunter left a comment

shunter Dec 6, 2017

shunter Dec 6, 2017

shunter Dec 6, 2017

shunter Dec 6, 2017

a-stacey commented Dec 14, 2017

samherrmann commented Dec 14, 2017

mramato commented Dec 14, 2017

shunter commented Dec 14, 2017

mramato Jan 17, 2018

mramato Jan 17, 2018

mramato Jan 17, 2018

mramato Jan 17, 2018

mramato commented Jan 17, 2018

tfili commented Jan 18, 2018

mramato commented Jan 18, 2018

mramato commented Jan 19, 2018

mramato commented Jan 19, 2018

mramato commented Jan 19, 2018

mramato Jan 19, 2018

mramato commented Jan 19, 2018

tfili commented Jan 19, 2018

pjcozzi commented Jan 19, 2018

mramato commented Jan 19, 2018

cesium-concierge commented Jan 19, 2018

shunter Feb 21, 2018

Upend resource loading approach #6035

Upend resource loading approach #6035

Conversation

hpinkos commented Dec 6, 2017 • edited by tfili Loading

shunter left a 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

a-stacey commented Dec 14, 2017

samherrmann commented Dec 14, 2017

mramato commented Dec 14, 2017

shunter commented Dec 14, 2017

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

mramato commented Jan 17, 2018

tfili commented Jan 18, 2018

mramato commented Jan 18, 2018

mramato commented Jan 19, 2018

mramato commented Jan 19, 2018

mramato commented Jan 19, 2018

Choose a reason for hiding this comment

mramato commented Jan 19, 2018

tfili commented Jan 19, 2018

pjcozzi commented Jan 19, 2018

mramato commented Jan 19, 2018

cesium-concierge commented Jan 19, 2018

Choose a reason for hiding this comment

hpinkos commented Dec 6, 2017 •

edited by tfili

Loading