Fix call to ApproximateTerrainHeights.initialize from GroundPolylinePrimitive #6715
Conversation
|
@hpinkos, thanks for the pull request! Maintainers, we have a signed CLA from @hpinkos, so you can review this at any time.
I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome. 🌍 🌎 🌏 |
|
No need to update CHANGES.md because this bug was never released |
| @@ -382,7 +382,7 @@ define([ | |||
| return initPromise; | |||
| } | |||
|
|
|||
| GroundPolylinePrimitive._initPromise = ApproximateTerrainHeights.initialize('../Assets/approximateTerrainHeights.json') | |||
| GroundPolylinePrimitive._initPromise = ApproximateTerrainHeights.initialize() | |||
There was a problem hiding this comment.
@hpinkos this seems to cause the JSON load to fail for local hosted and unproxied (?) apps. I'm getting a runtime error for the deployed Sandcastle demos:
http://cesium-dev.s3-website-us-east-1.amazonaws.com/cesium/fix-approx-heights/Apps/Sandcastle/index.html?src=Polyline.html
Is there a good way to test this with a proxy?
There was a problem hiding this comment.
ApproximateTerrainHeights.initialize needs to use buildModuleUrl like everything else, never load a Cesium required asset without it.
There was a problem hiding this comment.
I mean, this is what GroundPrimitive does already so this should be the right thing to do. I don't know where that runtime error is coming from. I'll see if I can figure it out.
There was a problem hiding this comment.
@mramato ApproximateTerrainHeights.initialize does use buildModuleUrl
There was a problem hiding this comment.
Ah okay, I this is happening because ApproximateTerrainHeights is being used by GroundPolylinePrimitive in a webworker
There was a problem hiding this comment.
sorry about this >__<
There was a problem hiding this comment.
Probably no one has ever tried to use buildModuleUrl from a worker, which is pretty unusual. I suspect an easier path would be to load the terrain height data in the main thread and send the data across, rather than trying to re-load it independently in the worker.
There was a problem hiding this comment.
Alternatively you could compute the full absolute url and send that across to the worker.
@shunter this seems sensible, but before diving in, the terrain heights are a giant (~500 kb) JSON. Should we keep that around on the main thread as a string to avoid having to re-stringify? Or should we just let that happen and not worry about the additional cost on spinning up workers? Alternatively I guess there's the rabbit hole of making the terrain heights JSON packable to a typed array... and accessing that typed array instead of traversing the JSON... but maybe that's out of scope for right now? |
|
I'm going to get started with just letting the stringification happen for now, since that seems simplest. compute is cheap (ish) [EDIT] yeah, some simple |
|
Well it looks like the entire JSON array is kept permanently in memory anyway as ApproximateTerrainHeights._terrainHeights so you could just send and restore that. |
|
Cool, @shunter looks like this should work. @bagnell #6615 added a bunch of async stuff to our workers so geometry could be created asynchronously on the worker itself. I'm going to go ahead and revert that, we can revisit it once geometry creation actually needs to be async like for querying a server for terrain heights or something. |
|
Yep, thanks for taking this one over @likangning93! |
Fixes #6711
@likangning93 can you review and merge?