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

Update approximateTerrainHeights.json #6557

Closed
mramato opened this issue May 3, 2018 · 8 comments
Closed

Update approximateTerrainHeights.json #6557

mramato opened this issue May 3, 2018 · 8 comments

Comments

@mramato
Copy link
Contributor

mramato commented May 3, 2018

approximateTerrainHeights.json was generated from the STK World Terrain as of #3903, with all of the improvements made to Cesium World Terrain, we should regenerate this file.

Long term plan is still for this data to be accurately provided by each terrain provider, but approximateTerrainHeights.json will always be a good fallback for servers that don't have the information.

@thw0rted
Copy link
Contributor

While you're looking at this: is there an error handler that's supposed to fire if approximateTerrainHeights fails to load? I had a configuration error that was causing the request to bomb and I burned a little too much time trying to figure out why one system wouldn't show any entities when the exact same code worked on another system (spoiler: without approximate terrain heights, the entities never finish getting their primitives). It would be really nice to get at least an error on the console to the effect that this super important part of Viewer initialization did not (and will not) happen. (I can file a separate issue, if there's not already an appropriate event.)

@hpinkos
Copy link
Contributor

hpinkos commented May 29, 2018

@thw0rted thanks for pointing that out, it doesn't look like we have an otherwise for loading the approximateTerrainHeights.json file https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/Core/ApproximateTerrainHeights.js#L58-L60

@mramato
Copy link
Contributor Author

mramato commented Sep 14, 2018

It was pointed out on the forum that we could significantly decrease the size of this file by rounding the numbers, which should have negligible impact because it's an approximation anyway.

@tfili given the large terrain updates lately, we should probably do this soon, do you remember where the script to generate this file is?

@mramato
Copy link
Contributor Author

mramato commented Sep 14, 2018

hpinkos pushed a commit that referenced this issue Sep 18, 2019
Optimize approximateTerrainHeights file for quicker page loads (Solves #6557)
@hpinkos
Copy link
Contributor

hpinkos commented Sep 18, 2019

Rounding the values was added in #7959 but we still want to regenerate the values at some point so I'm leaving this issue open

@thw0rted
Copy link
Contributor

thw0rted commented Nov 3, 2020

Hi @hpinkos , it looks like the error-handling issue was never resolved. Is this still on the radar at all?

Actually, I was investigating some other issues I have (for boring, personal reasons) with the viewer dynamically loading content after startup, and it occurred to me that I can't think of a circumstance where you'd use Cesium without any GroundPrimitive, which depends on ApproximateTerrainHeights, which loads the JSON file. So if every consumer needs the data, why not build it into the bundle using require instead of using an XHR? That'd be one less thing that can go wrong at runtime.

@thw0rted
Copy link
Contributor

thw0rted commented Nov 3, 2020

This was actually easier than what I suggested before, because if you have a JSON file you can turn it into a valid ES6 module by just adding export default at the beginning. That let me throw together this build real quick. It builds and runs without apparent issue, with two caveats:

  • The tests for StaticGroundGeometryColorBatch and StaticGeometryPerMaterialBatch fail but only if run in the whole suite -- running just those two specs works fine. This suggests that some earlier test is manipulating the ApproximateTerrainHeights module (it's the only one I changed), but I haven't been able to figure out how
    • ETA: I realized that I had been grepping through Source/ without looking at Specs/, and it turns out that tons of tests poke around in the guts of ApproximateTerrainHeights. Even ignoring my suggestion from this fork, maybe those interactions should be replaced with a mock?
  • It looks like the non-ES6 version of the createGroundPolylineGeometry worker inlines all its dependencies (recursively), including ApproximateTerrainHeights, which means that it gets its own (bundled) copy of the data. In a non-ES6 environment, I don't have a good solution for this. (I had made a snide remark here but it looks like Firefox still hasn't gotten onboard with ES6 modules in a web worker, so, oops?)

@ggetz
Copy link
Contributor

ggetz commented Jan 5, 2024

Given we'd like to move in the direction of removing the dependency on ApproximateTerrainHeights, I'm going to close this issue.

@ggetz ggetz closed this as completed Jan 5, 2024
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

4 participants