Fix sampleTerrain when using ArcGIS Terrain #9286

DanielLeone · 2020-12-15T08:21:51Z

Hi all,

I've got a WIP fix an issue and I'm keen for some feedback/suggestions.
This PR fixes #8007

There's 2 bugs as mentioned in the issue.

The availability property was not exposed from the ArcGISTiledElevationTerrainProvider.
- I've fixed that in the same way the @OmarShehata suggested - buy exposing _tilesAvailable
calling interpolateHeight on a HeightmapTerrainData instance from ArcGISTiledElevationTerrainProvider will return undefined.
- The reason this is happening is because:
  1. the data returned from an ArcGIS server is encoded in LERC format
  2. by default the interpolateHeight method of HeightmapTerrainData will try to use that array buffer (returned from the server) directly like a 2D height map array lookup thing.
  3. this just does not work for the buffer since it's encoded in a different format
- However we can avoid this code path by just calling createMesh() before calling interpolateHeight
  - We can see the first line of createVerticiesFromHeightmap (which is what createMesh calls) is where the LERC decoding happens.
  - This then forces us to use the mesh for interpolateHeight rather than the LERC buffer. This code path works as expected.

I've opted for this solution (just calling createMesh on every tile) inside sampleTerrain as it seemed to be the lowest friction change to me; but I don't think it's an optimal solution.

Another maybe better idea:

Make the ArcGIS Terrain Provider return a different HeightmapTerrainData class which already has it's mesh created beforehand; and then to meet the TerrainData interface createMesh would just be a no-op; and upsample seems to use the mesh anyway to build a new height map buffer. So MeshedHeightmapTerrainData.upsample -> HeightmapTerrainData or something.

Current issue with this solution:

all the sampleTerrain unit tests are failing and I'm not sure why. It appears when using Cesium World Terrain (as in the unit tests); interpolateHeight via the mesh code path isn't working for some reason 😢 . Any ideas on this one? I couldn't find any unit tests covering QuantizedMeshTerrainData.interpolateHeight when using the mesh code; nor do I see where it's ever really run in production (until now in this PR)... any help here is appreciated.

A less-than-ideal solution to this new issue would be

Only call createMesh for ArcGIS tiles.

Other changes:

Fixed the doc for TerrainData.createMesh (it was missing the terrain exaggeration param)
Added the IntelliJ shelf to the git ignore list
Updated the Terrain sandcastle so you can left and right click for sampleTerrain and sampleTerrainMostDetailed respectively and a few other changes - this is just temporary while testing though.

Any help is much appreciated; thanks!

* fixed the doc for `TerrainData.createMesh` (missing param) * exposed `_tileAvailability` on ArcGISTiledElevationTerrainProvider as the `availability`; this fixes sampleTerrainMostDetailed which requires that property. * made `sampleTerrain` call `createMesh` on every tile requested; this fixes ArcGIS terrain which currently requires the mesh to be built before interpolating height (because the request buffer is still encoded in LERC and the decoding happens during mesh building)

* increased the active task limit for now as it was getting in the way

cesium-concierge · 2020-12-15T08:21:54Z

Thank you so much for the pull request @DanielLeone! I noticed this is your first pull request and I wanted to say welcome to the Cesium community!

The Pull Request Guidelines is a handy reference for making sure your PR gets accepted quickly, so make sure to skim that.

❌ Missing CONTRIBUTORS.md entry.
- Please add yourself to the contributors file!
✔️ Signed CLA found.
❔ CHANGES.md was not updated.
- If this change updates the public API in any way, please add a bullet point to CHANGES.md.
❔ Unit tests were not updated.
- Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

Cesium Viewer works.
Works in 2D/CV.
Works (or fails gracefully) in IE11.

DanielLeone · 2020-12-15T08:32:40Z

Source/Core/ArcGISTiledElevationTerrainProvider.js

@@ -650,6 +657,7 @@ function requestAvailability(that, level, x, y) {

    // Mark whole area as having availability loaded
    that._tilesAvailablityLoaded.addAvailableTileRange(
+      level,


I just saw this - it looked like a missing parameter - I've got no tests or further context on this one! 😛 Will revert later.

this makes me question _tilesAvailablityLoaded even more...

DanielLeone · 2020-12-15T08:34:29Z

Source/Core/sampleTerrain.js

+      // Make sure to generate our mesh for each tile first
+      //  because some tiles actually require the mesh to interpolate heights
+      //  (eg: ArcGISTiledElevationTerrainProvider)
+      .then(createMeshCreatorFunction(tileRequest))


This is essentially the fix I've added. Call createMesh on each tile as it comes back. However; as mentioned, this causes CWT terrain interpolateHeight to return undefined - I haven't figured out why yet :)

DanielLeone · 2020-12-15T08:36:11Z

Source/Core/HeightmapTerrainData.js

@@ -182,7 +182,7 @@ Object.defineProperties(HeightmapTerrainData.prototype, {
  },
 });

-var taskProcessor = new TaskProcessor("createVerticesFromHeightmap");
+var taskProcessor = new TaskProcessor("createVerticesFromHeightmap", 5000);


Ahhh this previous limit of 5 was interfering with tests or the sand castle or something... I can't really remember 😛 I guess because every tile now has to go across the worker (during createMesh), there's a lot more traffic, and therefore chances for that limit of 5 to be reached pretty quick. Will revert / figure it out later.

DanielLeone · 2020-12-15T08:38:50Z

Source/Core/ArcGISTiledElevationTerrainProvider.js

+        );
+      }
+      //>>includeEnd('debug');
+      return this._tilesAvailable;


This was the other fix mentioned... I'm not really across how the TileAvailability class works and if this _tilesAvailable property is setup correctly. I did notice there's actually two properties _tilesAvailablityLoaded and _tilesAvailable in the ArcGIS Terrain Provider; not sure which one is correct in this case.

Either way, I think this makes sampleTerrainMostDetailed work as expected 👍

yeah, this seems like the right thing to return. I'm still wrapping my head around _tilesAvailablityLoaded. I think it only says whether availability has been loaded for tiles, not whether a tile is actually available. Not sure why _tilesAvailable can't do take care of this, but there's probably a good reason.

DanielLeone · 2020-12-15T08:46:36Z

@IanLilleyT You might be interested in this one as #8481 is next on the chopping block 😄

…teMesh * added some unit tests to sample terrain to test both code paths (ArcGIS and CWT) * added a bunch of test-data files so it doesn't have to hit a real terrain sever

* maybe fixed eslint

IanLilleyT · 2020-12-18T16:34:55Z

@DanielLeone

I looked over the code and the PR write-up helped get me up to speed quickly. I came to a pretty similar conclusion to you, and here's my summary (just to make sure I have all the details right): interpolateHeights is supposed to work for a HeightmapTerrainData even if its triangle mesh hasn't been created yet, and it does this by accessing heights directly from the buffer that came from the server. But in this case the buffer from ArcGisTiledElevationTerrainProvider is LERC-encoded so it doesn't work until it has been decoded. And the current workaround is to call the createMesh function from inside sampleTerrain (which triggers the LERC-decode) in order to turn the heightmap into a mesh so it can be interpolated. I agree it's not the optimal solution because it adds some messiness to sampleTerrain.

Your idea of creating a new MeshedHeightmapTerrainData that forces the heightmap to be a mesh from the start feels like the right direction. But I think instead of creating a new TerrainData type, it could make sense to call the LERC-decode from inside ArcGisTiledElevationTerrainProvider's requestTileGeometry and then create a regular HeightmapTerrainData from the decoded buffer. The decode call would probably need to go in a worker otherwise the main thread will be stalled with all these LERC decodes.

I'm tempted to remove custom encodings from HeightmapTerrainData and createVerticesFromHeightmap entirely, but there could be custom TerrainProviders out there that use them, so it would be a breaking change. But the fact that TerrainData functions are supposed to work pre and post createMesh makes me think there is a problem with the HeightmapTerrainData API allowing these encodings.

IanLilleyT · 2021-01-01T23:01:16Z

I was thinking about this a little more and started to change my mind from my previous comment. If a TerrainData is supposed to be a black box that converts arbitrary terrain formats to a TerrainMesh, then starting out in LERC format and decoding inside HeightmapTerrainData makes sense. So my earlier idea of moving the LERC decode into ArcGISTiledElevationTerrainProvider is probably not the right move.

So I'm liking the original approach of calling createMesh from sampleTerrain. But instead of having a special check for ArcGISTiledElevationTerrainProvider in sampleTerrain, what if we make interpolateHeight return undefined when the TerrainData is not ready to be interpolated. Then sampleTerrain would call createMesh followed by another interpolateHeight. And the nice part is it's not much code change from the original PR.

…is the mesh is not created and the encoding is LERC. * sampleTerrain will now call createMesh if interpolateHeight first returns undefined.

DanielLeone · 2021-01-06T13:06:38Z

@IanLilleyT Sounds good to me. Code has been updated! 👍 Let me know and I'll revert some of those other temporary changes.

… it returns a promise; working around the fact you can only schedule 5 meshes to be created at once.

DanielLeone · 2021-01-07T00:45:12Z

Okay, one more change. I reverted the 5000 limit on the task processor - so we're back to 5 as in master.
This broke sampling if more than 5 meshes needed to be sampled.
Fixed by putting in a tasty recursive promise ⚡

…ests

IanLilleyT · 2021-01-08T14:20:13Z

Nice

My two issues right now are:

I don't feel so good about the sleep. It's not really something the rest of the codebase does. We might be able to add a new option to createMesh like noThrottle to avoid needing an infinite loop in sampleTerrain. I hope to work on a PR for something like that today.
The new test data is too big. Is it possible to make it a lot smaller?

Anyway, feel free to start removing any of the temp code. Once my PR is merged we can do code review. Thanks!

DanielLeone · 2021-01-11T01:48:07Z

Okay no worries. Please let me know when the noThrottle option is in and I'll update all these things together. 👍

IanLilleyT · 2021-01-11T21:29:15Z

@DanielLeone, I opened #9313. Once you have a chance to merge with that branch please set this PR's base branch to terrainDataThrottleControl.

Let me know if you have any questions!

…to fix-arcgis-sample-terrain-height # Conflicts: # Source/Core/TerrainData.js

* shuffled and reduced the size of the test data * bulked out the test assertions a little

DanielLeone · 2021-01-12T13:12:33Z

@IanLilleyT Done!

I got it down to 180KB of test data; I hope that's okay :)
90% of that size is the terrain tiles themselves - I've put in 1 CWT tile and 1 ArcGIS terrain tile.
I'd like to use these same tiles as part of the tests for my next PR.
Let me know if there's anything else :) 🌮

IanLilleyT

@DanielLeone Overall this looks really good. In the end not a lot of code changed which I love to see! I tried it out with the Terrain.html you had in a previous commit and everything is working as expected. There's several little comments, mostly for the specs which need to be ES5 instead of ES6.

Also, please add an entry to the Fixes section for the Feburary release in CHANGES.md (the Fixes section doesn't exist yet but it will be underneath the Additions section. I made a mistake with the order in terrainDataThrottleControl branch so you'll have to merge first).

Source/Core/sampleTerrain.js

Specs/Core/sampleTerrainSpec.js

Specs/Data/Terrain/assets_cesium_com/layer.json

…to fix-arcgis-sample-terrain-height

* moved the test data files around * added a line to CHANGES.md

DanielLeone · 2021-01-14T14:54:21Z

@IanLilleyT thanks for all the suggestions 👍 I think I got them all 🌮

IanLilleyT · 2021-01-14T15:11:28Z

Specs/Core/sampleTerrainSpec.js

-          sourceUrl.pathname + sourceUrl.search + sourceUrl.hash;
+        // find a key (source path) path in the spec which matches (ends with) the requested url
+        var availablePaths = Object.keys(proxySpec);
+        var proxiedUrl = null;


Do var proxiedUrl; and down below if (!defined(proxiedUrl)) { (will need to include defined.js). That's the more standard way of handling null/undefined in the repo.

IanLilleyT · 2021-01-14T15:14:06Z

@DanielLeone I posted one last code request

If all goes well #9313 and this will be in the February release!

DanielLeone · 2021-01-14T23:27:09Z

@IanLilleyT Thanks for writing the throttling code and the quick PR reviews! 👍 🌮

IanLilleyT · 2021-02-01T23:56:54Z

Unfortunately didn't get #9313 reviewed in time for the February release, so it will be out in the March release.

…-terrain-height

…ain-height

IanLilleyT · 2021-02-28T20:30:41Z

Merged at last! Thanks @DanielLeone!

IanLilleyT · 2021-02-28T20:33:32Z

Now time to peek at #9389

DanielLeone added 2 commits December 15, 2020 11:24

* hijacked the Terrain.html Sandcastle for testing

93605ad

* increased the active task limit for now as it was getting in the way

DanielLeone commented Dec 15, 2020

View reviewed changes

DanielLeone added 2 commits December 18, 2020 11:37

* added a switch inside sample terrain which may or may not call crea…

96b24e2

…teMesh * added some unit tests to sample terrain to test both code paths (ArcGIS and CWT) * added a bunch of test-data files so it doesn't have to hit a real terrain sever

* maybe fixed the tests

2a25258

* maybe fixed eslint

* HeightmapTerrainData.interpolateHeight() will now return undefined …

1b6f86b

…is the mesh is not created and the encoding is LERC. * sampleTerrain will now call createMesh if interpolateHeight first returns undefined.

DanielLeone added 2 commits January 7, 2021 08:37

refactored so now sampleTerrain will infinitely call createMesh until…

3097f21

… it returns a promise; working around the fact you can only schedule 5 meshes to be created at once.

fixed some comments

3193628

whoopsie! only create the mesh if we actually need it - thanks unit t…

3be316d

…ests

IanLilleyT mentioned this pull request Jan 11, 2021

Added throttle option to TerrainData.createMesh #9313

Merged

DanielLeone added 2 commits January 12, 2021 12:07

Merge remote-tracking branch 'upstream/terrainDataThrottleControl' in…

f9d5a09

…to fix-arcgis-sample-terrain-height # Conflicts: # Source/Core/TerrainData.js

* refactored sampleTerrain to use the non-throttle create mesh call

95490b9

* shuffled and reduced the size of the test data * bulked out the test assertions a little

DanielLeone changed the base branch from master to terrainDataThrottleControl January 12, 2021 11:34

IanLilleyT suggested changes Jan 12, 2021

View reviewed changes

DanielLeone added 2 commits January 14, 2021 12:47

Merge remote-tracking branch 'upstream/terrainDataThrottleControl' in…

1c51236

…to fix-arcgis-sample-terrain-height

* fixed a lot of comments and es6 code

7bffb5a

* moved the test data files around * added a line to CHANGES.md

DanielLeone requested a review from IanLilleyT January 14, 2021 14:54

IanLilleyT suggested changes Jan 14, 2021

View reviewed changes

change to using defined() in sampleTerrainSpec.js

a1a9d54

Merge remote-tracking branch 'upstream/master' into fix-arcgis-sample…

67001cd

…-terrain-height

DanielLeone mentioned this pull request Feb 23, 2021

[WIP] Faster picking for ArcGIS Terrain / Heightmaps #9389

Closed

IanLilleyT added 2 commits February 28, 2021 13:52

Merge branch 'terrainDataThrottleControl' into fix-arcgis-sample-terr…

ccf37c1

…ain-height

fixed changes.md and added arcgis to terrain sandcastle

3bb6330

lilleyse closed this Feb 28, 2021

lilleyse deleted the branch CesiumGS:master February 28, 2021 19:55

Merge branch 'master' into fix-arcgis-sample-terrain-height

c813c23

IanLilleyT reopened this Feb 28, 2021

IanLilleyT changed the base branch from terrainDataThrottleControl to master February 28, 2021 19:59

IanLilleyT merged commit 368d9a4 into CesiumGS:master Feb 28, 2021

zouyaoji mentioned this pull request Mar 10, 2021

vc-navigation崩溃错误 zouyaoji/vue-cesium#71

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix sampleTerrain when using ArcGIS Terrain #9286

Fix sampleTerrain when using ArcGIS Terrain #9286

DanielLeone commented Dec 15, 2020

cesium-concierge commented Dec 15, 2020

DanielLeone Dec 15, 2020

IanLilleyT Dec 18, 2020

IanLilleyT Dec 18, 2020

DanielLeone Dec 15, 2020

DanielLeone Dec 15, 2020

DanielLeone Dec 15, 2020

IanLilleyT Dec 18, 2020

DanielLeone commented Dec 15, 2020

IanLilleyT commented Dec 18, 2020 •

edited

Loading

IanLilleyT commented Jan 1, 2021

DanielLeone commented Jan 6, 2021

DanielLeone commented Jan 7, 2021

IanLilleyT commented Jan 8, 2021

DanielLeone commented Jan 11, 2021

IanLilleyT commented Jan 11, 2021

DanielLeone commented Jan 12, 2021

IanLilleyT left a comment •

edited

Loading

DanielLeone commented Jan 14, 2021

IanLilleyT Jan 14, 2021 •

edited

Loading

IanLilleyT commented Jan 14, 2021

DanielLeone commented Jan 14, 2021

IanLilleyT commented Feb 1, 2021

IanLilleyT commented Feb 28, 2021

IanLilleyT commented Feb 28, 2021

Fix sampleTerrain when using ArcGIS Terrain #9286

Fix sampleTerrain when using ArcGIS Terrain #9286

Conversation

DanielLeone commented Dec 15, 2020

cesium-concierge commented Dec 15, 2020

DanielLeone Dec 15, 2020

Choose a reason for hiding this comment

IanLilleyT Dec 18, 2020

Choose a reason for hiding this comment

IanLilleyT Dec 18, 2020

Choose a reason for hiding this comment

DanielLeone Dec 15, 2020

Choose a reason for hiding this comment

DanielLeone Dec 15, 2020

Choose a reason for hiding this comment

DanielLeone Dec 15, 2020

Choose a reason for hiding this comment

IanLilleyT Dec 18, 2020

Choose a reason for hiding this comment

DanielLeone commented Dec 15, 2020

IanLilleyT commented Dec 18, 2020 • edited Loading

IanLilleyT commented Jan 1, 2021

DanielLeone commented Jan 6, 2021

DanielLeone commented Jan 7, 2021

IanLilleyT commented Jan 8, 2021

DanielLeone commented Jan 11, 2021

IanLilleyT commented Jan 11, 2021

DanielLeone commented Jan 12, 2021

IanLilleyT left a comment • edited Loading

Choose a reason for hiding this comment

DanielLeone commented Jan 14, 2021

IanLilleyT Jan 14, 2021 • edited Loading

Choose a reason for hiding this comment

IanLilleyT commented Jan 14, 2021

DanielLeone commented Jan 14, 2021

IanLilleyT commented Feb 1, 2021

IanLilleyT commented Feb 28, 2021

IanLilleyT commented Feb 28, 2021

IanLilleyT commented Dec 18, 2020 •

edited

Loading

IanLilleyT left a comment •

edited

Loading

IanLilleyT Jan 14, 2021 •

edited

Loading