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

Fix sampleTerrain when using ArcGIS Terrain #9286

Merged
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
17 commits
Select commit Hold shift + click to select a range
c2d0e53
* added the intellj shelf to the git ignore list
DanielLeone Dec 15, 2020
93605ad
* hijacked the Terrain.html Sandcastle for testing
DanielLeone Dec 15, 2020
96b24e2
* added a switch inside sample terrain which may or may not call crea…
DanielLeone Dec 18, 2020
2a25258
* maybe fixed the tests
DanielLeone Dec 18, 2020
1b6f86b
* HeightmapTerrainData.interpolateHeight() will now return undefined …
DanielLeone Jan 6, 2021
3097f21
refactored so now sampleTerrain will infinitely call createMesh until…
DanielLeone Jan 7, 2021
3193628
fixed some comments
DanielLeone Jan 7, 2021
3be316d
whoopsie! only create the mesh if we actually need it - thanks unit t…
DanielLeone Jan 7, 2021
f9d5a09
Merge remote-tracking branch 'upstream/terrainDataThrottleControl' in…
DanielLeone Jan 12, 2021
95490b9
* refactored sampleTerrain to use the non-throttle create mesh call
DanielLeone Jan 12, 2021
1c51236
Merge remote-tracking branch 'upstream/terrainDataThrottleControl' in…
DanielLeone Jan 14, 2021
7bffb5a
* fixed a lot of comments and es6 code
DanielLeone Jan 14, 2021
a1a9d54
change to using defined() in sampleTerrainSpec.js
DanielLeone Jan 14, 2021
67001cd
Merge remote-tracking branch 'upstream/master' into fix-arcgis-sample…
DanielLeone Feb 2, 2021
ccf37c1
Merge branch 'terrainDataThrottleControl' into fix-arcgis-sample-terr…
IanLilleyT Feb 28, 2021
3bb6330
fixed changes.md and added arcgis to terrain sandcastle
IanLilleyT Feb 28, 2021
c813c23
Merge branch 'master' into fix-arcgis-sample-terrain-height
IanLilleyT Feb 28, 2021
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -32,3 +32,4 @@ yarn.lock
# WebStorm user-specific
.idea/workspace.xml
.idea/tasks.xml
.idea/shelf
4 changes: 4 additions & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@

- Added `Cartesian2.cross`. [#9305](https://github.com/CesiumGS/cesium/pull/9305)

##### Fixes :wrench:

- Fixed `sampleTerrain` and `sampleTerrainMostDetailed` not working for `ArcGISTiledElevationTerrainProvider`. [#9286](https://github.com/CesiumGS/cesium/pull/9286)

### 1.77 - 2021-01-04

##### Additions :tada:
Expand Down
10 changes: 9 additions & 1 deletion Source/Core/ArcGISTiledElevationTerrainProvider.js
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,14 @@ Object.defineProperties(ArcGISTiledElevationTerrainProvider.prototype, {
*/
availability: {
get: function () {
return undefined;
//>>includeStart('debug', pragmas.debug)
if (!this._ready) {
throw new DeveloperError(
"availability must not be called before the terrain provider is ready."
);
}
//>>includeEnd('debug');
return this._tilesAvailable;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

},
},
});
Expand Down Expand Up @@ -650,6 +657,7 @@ function requestAvailability(that, level, x, y) {

// Mark whole area as having availability loaded
that._tilesAvailablityLoaded.addAvailableTileRange(
level,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

😮

Copy link
Contributor

Choose a reason for hiding this comment

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

this makes me question _tilesAvailablityLoaded even more...

xOffset,
yOffset,
xOffset + dim,
Expand Down
12 changes: 11 additions & 1 deletion Source/Core/HeightmapTerrainData.js
Original file line number Diff line number Diff line change
Expand Up @@ -434,8 +434,18 @@ HeightmapTerrainData.prototype.interpolateHeight = function (
var heightOffset = structure.heightOffset;
var heightScale = structure.heightScale;

var isMeshCreated = defined(this._mesh);
var isLERCEncoding = this._encoding === HeightmapEncoding.LERC;
var isInterpolationImpossible = !isMeshCreated && isLERCEncoding;
if (isInterpolationImpossible) {
// We can't interpolate using the buffer because it's LERC encoded
// so please call createMesh() first and interpolate using the mesh;
// as mesh creation will decode the LERC buffer
return undefined;
}

var heightSample;
if (defined(this._mesh)) {
if (isMeshCreated) {
var buffer = this._mesh.vertices;
var encoding = this._mesh.encoding;
var exaggeration = this._mesh.exaggeration;
Expand Down
71 changes: 67 additions & 4 deletions Source/Core/sampleTerrain.js
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,33 @@ function doSampling(terrainProvider, level, positions) {
});
}

/**
* Calls {@link TerrainData#interpolateHeight} on a given {@link TerrainData} for a given {@link Cartographic} and
* will assign the height property if the return value is not undefined.
*
* If the return value is false; it's suggesting that you should call {@link TerrainData#createMesh} first.
* @param {Cartographic} position The position to interpolate for and assign the height value to
* @param {TerrainData} terrainData
* @param {Rectangle} rectangle
* @returns {Boolean} If the height was actually interpolated and assigned
* @private
*/
function interpolateAndAssignHeight(position, terrainData, rectangle) {
var height = terrainData.interpolateHeight(
rectangle,
position.longitude,
position.latitude
);
if (height === undefined) {
// if height comes back as undefined, it may implicitly mean the terrain data
// requires us to call TerrainData.createMesh() first (ArcGIS requires this in particular)
// so we'll return false and do that next!
return false;
}
position.height = height;
return true;
}

function createInterpolateFunction(tileRequest) {
var tilePositions = tileRequest.positions;
var rectangle = tileRequest.tilingScheme.tileXYToRectangle(
Expand All @@ -107,14 +134,50 @@ function createInterpolateFunction(tileRequest) {
tileRequest.level
);
return function (terrainData) {
var isMeshRequired = false;
for (var i = 0; i < tilePositions.length; ++i) {
var position = tilePositions[i];
position.height = terrainData.interpolateHeight(
rectangle,
position.longitude,
position.latitude
var isHeightAssigned = interpolateAndAssignHeight(
position,
terrainData,
rectangle
);
// we've found a position which returned undefined - hinting to us
// that we probably need to create a mesh for this terrain data.
// so break out of this loop and create the mesh - then we'll interpolate all the heights again
if (!isHeightAssigned) {
isMeshRequired = true;
break;
}
}

if (!isMeshRequired) {
// all position heights were interpolated - we don't need the mesh
return when.resolve();
}

// create the mesh - and interpolate all the positions again
return terrainData
.createMesh({
tilingScheme: tileRequest.tilingScheme,
x: tileRequest.x,
y: tileRequest.y,
level: tileRequest.level,
// interpolateHeight will divide away the exaggeration - so passing in 1 is fine; it doesn't really matter
exaggeration: 1,
// don't throttle this mesh creation because we've asked to sample these points;
// so sample them! We don't care how many tiles that is!
throttle: false,
})
.then(function () {
// mesh has been created - so go through every position (maybe again)
// and re-interpolate the heights - presumably using the mesh this time
for (var i = 0; i < tilePositions.length; ++i) {
var position = tilePositions[i];
// if it doesn't work this time - that's fine, we tried.
interpolateAndAssignHeight(position, terrainData, rectangle);
}
});
};
}

Expand Down
191 changes: 191 additions & 0 deletions Specs/Core/sampleTerrainSpec.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
import { ArcGISTiledElevationTerrainProvider } from "../../Source/Cesium.js";
import { Cartographic } from "../../Source/Cesium.js";
import { CesiumTerrainProvider } from "../../Source/Cesium.js";
import { createWorldTerrain } from "../../Source/Cesium.js";
import { RequestScheduler } from "../../Source/Cesium.js";
import { Resource } from "../../Source/Cesium.js";
import { sampleTerrain } from "../../Source/Cesium.js";

describe("Core/sampleTerrain", function () {
Expand Down Expand Up @@ -99,4 +102,192 @@ describe("Core/sampleTerrain", function () {
expect(positions[0].height).toBeDefined();
});
});

describe("with terrain providers", function () {
beforeEach(function () {
RequestScheduler.clearForSpecs();
});

afterEach(function () {
Resource._Implementations.loadWithXhr =
Resource._DefaultImplementations.loadWithXhr;
});

function spyOnTerrainDataCreateMesh(terrainProvider) {
// do some sneaky spying so we can check how many times createMesh is called
var originalRequestTileGeometry = terrainProvider.requestTileGeometry;
spyOn(terrainProvider, "requestTileGeometry").and.callFake(function (
x,
y,
level,
request
) {
// Call the original function!
return originalRequestTileGeometry
.call(terrainProvider, x, y, level, request)
.then(function (tile) {
spyOn(tile, "createMesh").and.callThrough();
// return the original tile - after we've spied on the create mesh method
return tile;
});
});
}

function expectTileAndMeshCounts(
terrainProvider,
numberOfTilesRequested,
wasFirstTileMeshCreated
) {
// all came from a single tile
expect(terrainProvider.requestTileGeometry.calls.count()).toEqual(
numberOfTilesRequested
);

// get the return terrain data for our spies
return (
terrainProvider.requestTileGeometry.calls
.first()
// return value was the promise of the TerrainData
.returnValue.then(function (terrainData) {
// make sure the mesh was only created once!
expect(terrainData.createMesh.calls.count()).toEqual(
wasFirstTileMeshCreated ? 1 : 0
);
})
);
}

function endsWith(value, suffix) {
return value.indexOf(suffix, value.length - suffix.length) >= 0;
}

function patchXHRLoad(proxySpec) {
Resource._Implementations.loadWithXhr = function (
url,
responseType,
method,
data,
headers,
deferred,
overrideMimeType
) {
// find a key (source path) path in the spec which matches (ends with) the requested url
var availablePaths = Object.keys(proxySpec);
var proxiedUrl = null;
Copy link
Contributor

@IanLilleyT IanLilleyT Jan 14, 2021

Choose a reason for hiding this comment

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

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.


for (var i = 0; i < availablePaths.length; i++) {
var srcPath = availablePaths[i];
if (endsWith(url, srcPath)) {
proxiedUrl = proxySpec[availablePaths[i]];
break;
}
}

// it's a whitelist - meaning you have to proxy every request explicitly
if (!proxiedUrl) {
throw new Error(
"Unexpected XHR load to url: " +
url +
"; spec includes: " +
availablePaths.join(", ")
);
}

// make a real request to the proxied path for the matching source path
return Resource._DefaultImplementations.loadWithXhr(
proxiedUrl,
responseType,
method,
data,
headers,
deferred,
overrideMimeType
);
};
}

it("should work for Cesium World Terrain", function () {
patchXHRLoad({
"/layer.json": "Data/CesiumTerrainTileJson/9_759_335/layer.json",
"/9/759/335.terrain?v=1.2.0":
"Data/CesiumTerrainTileJson/9_759_335/9_759_335.terrain",
});
var terrainProvider = new CesiumTerrainProvider({
url: "made/up/url",
});

spyOnTerrainDataCreateMesh(terrainProvider);

var positionA = Cartographic.fromDegrees(
86.93666235421982,
27.97989963555095
);
var positionB = Cartographic.fromDegrees(
86.9366623542198,
27.9798996355509
);
var positionC = Cartographic.fromDegrees(
86.936662354213,
27.979899635557
);

var level = 9;

return sampleTerrain(terrainProvider, level, [
positionA,
positionB,
positionC,
]).then(function () {
expect(positionA.height).toBeCloseTo(7780, 0);
expect(positionB.height).toBeCloseTo(7780, 0);
expect(positionC.height).toBeCloseTo(7780, 0);
// 1 tile was requested (all positions were close enough on the same tile)
// and the mesh was not created because we're using CWT - which doesn't need the mesh for interpolation
return expectTileAndMeshCounts(terrainProvider, 1, false);
});
});

it("should work for ArcGIS terrain", function () {
patchXHRLoad({
"/?f=pjson": "Data/ArcGIS/9_214_379/root.json",
"/tilemap/10/384/640/128/128":
"Data/ArcGIS/9_214_379/tilemap_10_384_640_128_128.json",
"/tile/9/214/379": "Data/ArcGIS/9_214_379/tile_9_214_379.tile",
});

var terrainProvider = new ArcGISTiledElevationTerrainProvider({
url: "made/up/url",
});

spyOnTerrainDataCreateMesh(terrainProvider);

var positionA = Cartographic.fromDegrees(
86.93666235421982,
27.97989963555095
);
var positionB = Cartographic.fromDegrees(
86.9366623542198,
27.9798996355509
);
var positionC = Cartographic.fromDegrees(
86.936662354213,
27.979899635557
);

var level = 9;
return sampleTerrain(terrainProvider, level, [
positionA,
positionB,
positionC,
]).then(function () {
// 3 very similar positions
expect(positionA.height).toBeCloseTo(7681, 0);
expect(positionB.height).toBeCloseTo(7681, 0);
expect(positionC.height).toBeCloseTo(7681, 0);
// 1 tile was requested (all positions were close enough on the same tile)
// and the mesh was created once because we're using an ArcGIS tile
return expectTileAndMeshCounts(terrainProvider, 1, true);
});
});
});
});
Loading