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 1 commit
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
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
15 changes: 6 additions & 9 deletions Source/Core/sampleTerrain.js
Original file line number Diff line number Diff line change
Expand Up @@ -100,15 +100,14 @@ function doSampling(terrainProvider, level, positions) {
}

/**
* Calls [interpolateHeight]{@link TerrainData.prototype.interpolateHeight} on a given {@link TerrainData} for a given {@link Cartographic} and
* 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 [createMesh]{@link TerrainData.prototype.createMesh} first.
* 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
* @return {boolean} If the height was actually interpolated and assigned
* @returns {Boolean} If the height was actually interpolated and assigned
* @private
*/
function interpolateAndAssignHeight(position, terrainData, rectangle) {
Expand All @@ -127,9 +126,6 @@ function interpolateAndAssignHeight(position, terrainData, rectangle) {
return true;
}

// I'm guessing we always want no terrain exaggeration when calling sample terrain directly
var defaultTerrainExaggeration = 1;

function createInterpolateFunction(tileRequest) {
var tilePositions = tileRequest.positions;
var rectangle = tileRequest.tilingScheme.tileXYToRectangle(
Expand Down Expand Up @@ -167,9 +163,10 @@ function createInterpolateFunction(tileRequest) {
x: tileRequest.x,
y: tileRequest.y,
level: tileRequest.level,
exaggeration: defaultTerrainExaggeration,
// 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! I don't care how many tiles that is!
// so sample them! We don't care how many tiles that is!
throttle: false,
})
.then(function () {
Expand Down
91 changes: 48 additions & 43 deletions Specs/Core/sampleTerrainSpec.js
Original file line number Diff line number Diff line change
@@ -1,13 +1,9 @@
import {
Cartographic,
RequestScheduler,
Resource,
} from "../../Source/Cesium.js";
import {
CesiumTerrainProvider,
ArcGISTiledElevationTerrainProvider,
} from "../../Source/Cesium.js";
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 @@ -107,7 +103,7 @@ describe("Core/sampleTerrain", function () {
});
});

describe("with terrain providers", () => {
describe("with terrain providers", function () {
beforeEach(function () {
RequestScheduler.clearForSpecs();
});
Expand All @@ -118,15 +114,15 @@ describe("Core/sampleTerrain", function () {
});

function spyOnTerrainDataCreateMesh(terrainProvider) {
// do some sneaky spying so we can check out many times createMesh is called
// 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
) {
// all the original functioN!
// Call the original function!
return originalRequestTileGeometry
.call(terrainProvider, x, y, level, request)
.then(function (tile) {
Expand Down Expand Up @@ -161,6 +157,10 @@ describe("Core/sampleTerrain", function () {
);
}

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

function patchXHRLoad(proxySpec) {
Resource._Implementations.loadWithXhr = function (
url,
Expand All @@ -171,43 +171,49 @@ describe("Core/sampleTerrain", function () {
deferred,
overrideMimeType
) {
var sourceUrl = new URL(url, "https://google.com");
var sourceUrlPath =
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;
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;
}
}

if (!Object.keys(proxySpec).includes(sourceUrlPath)) {
var msg =
// it's a whitelist - meaning you have to proxy every request explicitly
if (!proxiedUrl) {
throw new Error(
"Unexpected XHR load to url: " +
sourceUrlPath +
" (from url: " +
url +
"); spec includes: " +
Object.keys(proxySpec).join(", ");
console.error(msg);
throw new Error(msg);
url +
"; spec includes: " +
availablePaths.join(", ")
);
}
var targetUrl = proxySpec[sourceUrlPath];

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

it("should work for Cesium World Terrain", () => {
it("should work for Cesium World Terrain", function () {
patchXHRLoad({
"/fake/url/layer.json": "Data/Terrain/assets_cesium_com/layer.json",
"/fake/url/9/759/335.terrain?v=1.2.0":
"Data/Terrain/assets_cesium_com/9_759_335.terrain",
"/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: "/fake/url",
requestVertexNormals: false,
requestWaterMask: false,
url: "made/up/url",
});

spyOnTerrainDataCreateMesh(terrainProvider);
Expand All @@ -231,7 +237,7 @@ describe("Core/sampleTerrain", function () {
positionA,
positionB,
positionC,
]).then(() => {
]).then(function () {
expect(positionA.height).toBeCloseTo(7780, 0);
expect(positionB.height).toBeCloseTo(7780, 0);
expect(positionC.height).toBeCloseTo(7780, 0);
Expand All @@ -241,17 +247,16 @@ describe("Core/sampleTerrain", function () {
});
});

it("should work for ArcGIS terrain", () => {
it("should work for ArcGIS terrain", function () {
patchXHRLoad({
"/fake/url/?f=pjson": "Data/Terrain/elevation3d_arcgis_com/root.json",
"/fake/url/tilemap/10/384/640/128/128":
"Data/Terrain/elevation3d_arcgis_com/tilemap_10_384_640_128_128.json",
"/fake/url/tile/9/214/379":
"Data/Terrain/elevation3d_arcgis_com/tile_9_214_379.tile",
"/?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: "/fake/url",
url: "made/up/url",
});

spyOnTerrainDataCreateMesh(terrainProvider);
Expand All @@ -274,7 +279,7 @@ describe("Core/sampleTerrain", function () {
positionA,
positionB,
positionC,
]).then(() => {
]).then(function () {
// 3 very similar positions
expect(positionA.height).toBeCloseTo(7681, 0);
expect(positionB.height).toBeCloseTo(7681, 0);
Expand Down