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

cesium_base regression in v1.44 #6411

Closed
keikland opened this issue Apr 2, 2018 · 25 comments
Closed

cesium_base regression in v1.44 #6411

keikland opened this issue Apr 2, 2018 · 25 comments

Comments

@keikland
Copy link

@keikland keikland commented Apr 2, 2018

Hi,
Thanks for the new release. Always appreciated! Unfortunately, my app failed to start and it seems the broken cesium_base directory issue from v1.42, fixed in v1.43, is back again. The load of the terrain json in the Assets directory failed first. The app started when I forced cesium_base to the proper directory.

Best regards,
Kjell

@hpinkos
Copy link
Contributor

@hpinkos hpinkos commented Apr 3, 2018

@keikland this issue, right? #6225
Thanks for reporting the regression

@tfili any idea why this broke again? When we fix it, we should make sure to add a unit test

@tfili
Copy link
Contributor

@tfili tfili commented Apr 3, 2018

@hpinkos There were 2 unit tests added and they are still passing. The only thing that touched this code is your change, #6331. Not sure why that change would break anything, but that would be my guess as to when it broke.

@mreilaender
Copy link

@mreilaender mreilaender commented Apr 10, 2018

I was able to reproduce the problem in an angular app.

Steps to reproduce:

  • ng new cesium#6411
  • yarn add cesium@1.41.0
  • Edit .angular-cli.json and add the following
{
    "assets": [
        { "glob": "**/*", "input": "../node_modules/cesium/Build/Cesium", "output": "./assets/cesium" }
    ],
    "styles": [
        "../node_modules/cesium/Build/Cesium/Widgets/widgets.css"
    ],
    "scripts": [
        "../node_modules/cesium/Build/Cesium/Cesium.js"
    ]
}
  • Edit main.ts and add window['CESIUM_BASE_URL'] = './assets/cesium'; before platformBrowserDynamic().bootstrapModule(AppModule)
  • Init the cesium viewer and place it somewhere
  • Run the App yarn start

Everything works perfectly. Now try upgrading to 1.42.0 by running yarn add cesium@1.42.0. Start the app again and there u go.

Edit: I used angular-cesium but it shouldn't matter. Here is my yarn.lock
Edit: I setup a repository

@emackey
Copy link
Contributor

@emackey emackey commented Apr 10, 2018

Another data point: After debugging a while in gltf-vscode, it looks like CESIUM_BASE_URL is accessed now during Cesium.js initial script parsing. Previously, I could include Cesium.js as a script tag, and then define CESIUM_BASE_URL prior to calling any functions in Cesium.js, and it would work. Now the value is accessed quite early, and I need to declare it before I even import the script.

@mramato
Copy link
Member

@mramato mramato commented Apr 10, 2018

Everything works perfectly. Now try upgrading to 1.42.0 by running yarn add cesium@1.42.0. Start the app again and there u go.

@mreilaender Thanks, 1.42 was known to be broken, it was fixed in 1.43. It's 1.44 that is the concern now since the code has changed again. Can you test with 1.44?

Now the value is accessed quite early, and I need to declare it before I even import the script.

@emackey While it might have worked previously by accident, I think the rule has always been that you must set it before you include the .js file. @shunter might have some thoughts here.

@shunter
Copy link
Contributor

@shunter shunter commented Apr 10, 2018

I don't think we were ever clear about when the global needed to be set. I can believe that it would have worked before because it isn't checked for until buildModuleUrl is called. It looks like #6331 changed things such that buildModuleUrl is called eagerly, while modules are still loading. For example, BingMapsImageryProvider uses it to find the credit logo. It would be better to delay the calls unless there's a strong reason not to. In the Bing Maps case, there's really no reason to call it so early. It could just call it from the constructor.

@emackey
Copy link
Contributor

@emackey emackey commented Apr 10, 2018

Thanks @shunter.

Also note that once buildModuleUrl has been called too eagerly, it caches its result value, so it's broken for the remainder of the life of the page.

@mramato
Copy link
Member

@mramato mramato commented Apr 10, 2018

The main problem is that it's really hard to track down these "eager" calls so that they can be caught by unit tests. Any ideas? I suppose we could have a special test that loads Cesium.js and sees if buildModuleUrl has been called at all, but not sure how to fit that into our current framework.

@shunter
Copy link
Contributor

@shunter shunter commented Apr 10, 2018

I started fixing this and it seems like the first and only time anyone tried to call buildModuleUrl early in 5 years was introduced in the above PR, so perhaps it's not that common.

@shunter
Copy link
Contributor

@shunter shunter commented Apr 10, 2018

cc #6439

@hpinkos
Copy link
Contributor

@hpinkos hpinkos commented Apr 10, 2018

@keikland we just merged in #6439, can you please check out master and see if that fixes your problem? You can see our Build Guide for instructions on building the code base.

@keikland
Copy link
Author

@keikland keikland commented Apr 10, 2018

Hi Hannah,
I am fairly decent in some other languages, but scrape by in Javascript with fairly simple coding, and still have not gotten anywhere near set up or comfortable with git/node/npm.. system-level development and building. Even with the build guide I quickly got concerned about the time it could take. If you have the chance to do a build and send it to me I can revert quickly. Otherwise I could possibly take a stab at the end of the week.
Kjell

@hpinkos
Copy link
Contributor

@hpinkos hpinkos commented Apr 10, 2018

@keikland No worries! Actually, I forgot that we make a zip file with the build as part of our continuous integration so you should be able to download it to test here: http://cesium-dev.s3-website-us-east-1.amazonaws.com/cesium/master/Cesium-1.44.0-master26369.zip

@keikland
Copy link
Author

@keikland keikland commented Apr 10, 2018

@hpinkos
Copy link
Contributor

@hpinkos hpinkos commented Apr 11, 2018

Okay, thanks @keikland, we'll keep looking into it

@emackey
Copy link
Contributor

@emackey emackey commented Apr 11, 2018

@keikland Are you sure you were using the new version? It's not available as an npm package yet, so you'd have to manually edit your node_modules folder to get it in there.

I actually pulled down @mreilaender's sample repo because I was lacking a live sample of angular-cesium. It took some fiddling to get the un-released build of Cesium into node_modules, but that was once done the demo worked fine.

@mramato
Copy link
Member

@mramato mramato commented Apr 11, 2018

@emackey there should be no fiddling required, all you need to do is change package.json to point to the ci produced npm package: for example, master is currently at http://cesium-dev.s3-website-us-east-1.amazonaws.com/cesium/master/cesium-1.44.0-master26380.tgz You can get the latest links by click on the green checkbox on the GitHub branches page.

@emackey
Copy link
Contributor

@emackey emackey commented Apr 11, 2018

@mramato Well that's easier, thanks.

...
    "@angular/router": "^5.2.0",
    "angular-cesium": "^0.0.48",
    "cesium": "http://cesium-dev.s3-website-us-east-1.amazonaws.com/cesium/master/cesium-1.44.0-master26380.tgz",
    "core-js": "^2.4.1",
...
npm install
npm run start

Works with zero fiddling.

@keikland
Copy link
Author

@keikland keikland commented Apr 11, 2018

Got a link to a zip to the master from @hpinkos, and it had what I expected to find. The only hesitation about my test - after midnight - was that it is possible i forgot to make sure the cache was cleared. Can try again in a couple of hours.

@keikland
Copy link
Author

@keikland keikland commented Apr 11, 2018

The link I got from @hpinkos was Cesium-1.44.0-master26369.zip, but I see @emackey uses 26380. Could be the version difference is important, and I just downloaded this. (http://cesium-dev.s3-website-us-east-1.amazonaws.com/cesium/master/Cesium-1.44.0-master26380.zip). Is this the one I should test?

@hpinkos
Copy link
Contributor

@hpinkos hpinkos commented Apr 11, 2018

@keikland 26369 includes the change to the baseUrl that was merged. 26380 is the commit after that. So either should be fine.

@keikland
Copy link
Author

@keikland keikland commented Apr 11, 2018

@hpinkos , just tried 26380 and it loads fine without loading errors when cesium_base_url is not set explicitly Suspect it was a cache issue yesterday. Probably corroborates results by @emackey.

@hpinkos
Copy link
Contributor

@hpinkos hpinkos commented Apr 12, 2018

Okay thanks @keikland! I'm going to close this. Let me know if this problem pops up again when you upgrade to the 1.45 release available on May 1st

@Koesters
Copy link

@Koesters Koesters commented May 9, 2018

I tried to install Cesium 1.45 under Angular 6.

I had everything working under 1.44 with

{
                "glob": "**/*",
                "input": "node_modules/cesium/Build/Cesium",
                "output": "/"
              },
              {
                "glob": "**/*",
                "input": "node_modules/cesium/Source",
                "output": "/assets/cesium"
              },

now in angular 6 it's in angular.json

Deleting

              {
                "glob": "**/*",
                "input": "node_modules/cesium/Source",
                "output": "/assets/cesium"
              },

Leads to no Assets and widgets been found.

Adding again:

              {
                "glob": "**/*",
                "input": "node_modules/cesium/Source",
                "output": "/assets/cesium"
              },

under 1.45 leads then to

GET http://localhost:4200/assets/cesium/Workers/Core/AttributeCompression.js 404 (Not Found)
cesiumWorkerBootstrapper.js:188 Uncaught Error: importScripts failed for Workers/createVerticesFromQuantizedTerrainMesh at http://localhost:4200/assets/cesium/Workers/createVerticesFromQuantizedTerrainMesh.js
http://requirejs.org/docs/errors.html#importscripts
at makeError (cesiumWorkerBootstrapper.js:188)
at Function.req.load (cesiumWorkerBootstrapper.js:1967)
at Object.load (cesiumWorkerBootstrapper.js:1690)
at Module.load (cesiumWorkerBootstrapper.js:852)
at Module.fetch (cesiumWorkerBootstrapper.js:842)
at Module.check (cesiumWorkerBootstrapper.js:874)
at Module.enable (cesiumWorkerBootstrapper.js:1188)
at Object.enable (cesiumWorkerBootstrapper.js:1561)
at Module. (cesiumWorkerBootstrapper.js:1173)
at cesiumWorkerBootstrapper.js:153
makeError @ cesiumWorkerBootstrapper.js:188
req.load @ cesiumWorkerBootstrapper.js:1967
load @ cesiumWorkerBootstrapper.js:1690
load @ cesiumWorkerBootstrapper.js:852
fetch @ cesiumWorkerBootstrapper.js:842
check @ cesiumWorkerBootstrapper.js:874
enable @ cesiumWorkerBootstrapper.js:1188
enable @ cesiumWorkerBootstrapper.js:1561
(anonymous) @ cesiumWorkerBootstrapper.js:1173
(anonymous) @ cesiumWorkerBootstrapper.js:153
each @ cesiumWorkerBootstrapper.js:78
enable @ cesiumWorkerBootstrapper.js:1125
init @ cesiumWorkerBootstrapper.js:806
(anonymous) @ cesiumWorkerBootstrapper.js:1464
setTimeout @ cesiumWorkerBootstrapper.js:18
req.nextTick @ cesiumWorkerBootstrapper.js:1806
localRequire @ cesiumWorkerBootstrapper.js:1453
requirejs @ cesiumWorkerBootstrapper.js:1788
self.onmessage @ cesiumWorkerBootstrapper.js:8

rolling back to 1.44 and restarting ng serve fixes the issue for 1.44 not for 1.45.

@bampakoa
Copy link
Contributor

@bampakoa bampakoa commented May 23, 2018

@Koesters that issue fits better to the Cesium-Angular example repository 😉 . Nevertheless, we have upgraded the repository to Angular 6, so you can check it out.

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

No branches or pull requests

9 participants