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

Allow local-running Cesium to load its resources #5892

Merged
merged 4 commits into from
Oct 20, 2017
Merged

Conversation

hpinkos
Copy link
Contributor

@hpinkos hpinkos commented Oct 9, 2017

Replaces #5831

Fixes #5830

@cesium-concierge
Copy link

@hpinkos, thanks for the pull request! Maintainers, we have a signed CLA from @hpinkos, so you can review this at any time.

I noticed that CHANGES.md has not been updated. If this change updates the public API in any way, fixes a bug, or makes any non-trivial update, please add a bullet point to CHANGES.md and comment on this pull request so we know it was updated. For more info, see the Pull Request Guidelines.

I am a bot who helps you make Cesium awesome! Thanks again.

@hpinkos hpinkos requested a review from mramato October 9, 2017 16:09
CHANGES.md Outdated

* Breaking Changes
*
* Added the ability to load Cesium's assets from the local file system [#5830](https://github.com/AnalyticalGraphicsInc/cesium/issues/5830)
Copy link
Contributor

Choose a reason for hiding this comment

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

This description should probably be more specific as Cesium still can't access the local file system in typical scenarios because of browser security. Does this new feature support only Chromium with security disabled? Or does it impact other browsers as well? It's unclear to me is xhr.status === 0 is common or not.

Copy link
Contributor

Choose a reason for hiding this comment

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

To answer my own question, status 0 for file protocol is very common and even jQuery deals with it: https://github.com/jquery/jquery/blob/master/src/ajax/xhr.js#L17

@mramato
Copy link
Contributor

mramato commented Oct 20, 2017

I tweaked the commands, but that's it. Thanks.

@mramato mramato merged commit 7eb80b0 into master Oct 20, 2017
@mramato mramato deleted the local-resources branch October 20, 2017 21:07
@thw0rted
Copy link
Contributor

thw0rted commented Feb 8, 2018

I can see this change live in the source (I'm building from NPM via Webpack) but I still have the problem it purports to fix. Should I pursue here or open a new ticket?

Chromium latest, Ubuntu 16.04, running with disable-web-security and allow-file-access-from-files. I set up an imagery provider using Natural Earth tiles (createTileMapServiceImageryProvider) and it works fine over HTTP(S), but fails locally. I stepped through the debugger, and the failure handler in that function (metadataFailure) does trigger, but the argument (error) is an XHR with status = 0 and a properly parsed and loaded response. I could load nodes out of the XML by playing with it in the dev tools console. Long story short, it appears that loadXML is firing its otherwise handler even though the browser isn't blocking the request.

(15 minutes pass...) Aaaand I got it. The above patch catches requests to an absolute file URI, only. My problem is I'm making a relative request -- asking for Assets/Textures/NaturalEarthII/tilemapresource.xml does not normalize to the absolute path before reaching loadWithXHR, which means the new test (url.indexOf('file://')) fails, which means it's not treated as a file URL and the status = 0 exemption is ignored.

I still (!!!) haven't sorted out my CLA issues with management so I will humbly suggest merely changing L178 (in this patch, which is still the latest commit to loadWithXhr.js from

localFile = url.indexOf('file://') === 0;

to

localFile = (url.indexOf('file://') === 0) || window.location.origin === 'file://';

The catch of course is that this is "browser-dependent", per the docs on Location.origin, but the patch is specifically for a Chrome quirk in the first place (right?), so I think it all works out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants