Terrain sample #559

Merged
merged 22 commits into from Mar 21, 2013

Conversation

Projects
None yet
3 participants
Contributor

chris-cooper commented Mar 11, 2013

This is an initial pass at a terrain query contribution for Kevin to look at. You should be able to just use the button in the Terrain Sandcastle to test. Note that visually it looks better in Columbus view as I think there might be a z clip render issue in 3d view.

Hopefully the comments in the code will indicate what I'm trying to do, though I'm sure there's better ways of actually doing those things.

Member

kring commented Mar 11, 2013

Chris, thanks for putting this together! Would it make sense to work together to polish this into something we can merge into master? Maybe you could grant me write access to this repo, or create a separate one that we can both write to?

Contributor

chris-cooper commented Mar 12, 2013

Hi Kevin,

You should now have write access to that repo.

Cheers,
Chris

On 12 March 2013 01:24, Kevin Ring notifications@github.com wrote:

Chris, thanks for putting this together! Would it make sense to work
together to polish this into something we can merge into master? Maybe you
could grant me write access to this repo, or create a separate one that we
can both write to?


Reply to this email directly or view it on GitHubhttps://github.com/AnalyticalGraphicsInc/cesium/pull/559#issuecomment-14715664
.

Member

kring commented Mar 12, 2013

We disable depth testing against terrain by default (I'm actually surprised it's not off in Columbus View as well), which is why you could see the billboards through the mountains. With depth testing enabled it's looking good.

kring added some commits Mar 12, 2013

@kring kring Fix JSHint warnings, and other minor cleanup. ff3667a
@kring kring Refactor terrainSample function.
Mostly I moved some of the interpolation functionality into
`HeightmapTerrainData`, and used some fancy features of the promise
library to avoid manually tracking tile completion.
a4ed38d
Member

kring commented Mar 12, 2013

@chris-cooper, take a look at my latest changes. I've simplified it a bit by eliminating the need for TileTerrain, moving the interpolation functionality into HeightmapTerrainData, and using some fancy features of the promise library.

As I see it, there are a few remaining issues before this is "production ready":

  • Terrain data is downloaded completely independently for rendering versus height queries.
  • There's no caching of tiles for height queries. Each query downloads tiles (or more likely pulls them from the browser cache).
  • It depends on a small change I made to CesiumTerrainProvider to prevent it from deferring requests, because in this scenario that behavior just greatly complicates our query implementation. This change needs to be propagated to the other terrain providers, or alternatively the TerrainProvider interface needs to be refactored slightly so that someone other the terrain provider is responsible for deferring requests when it is appropriate to do so.
Contributor

chris-cooper commented Mar 12, 2013

Ah nice. It's looking much better already. Great to be rid of those timeouts, and I guess I didn't look hard enough for the interpolation code ;-) . For my initial use caching of tiles wasn't important, but I'm sure this interface could still apply well to some interactive use cases if caching was in place. Are the remaining issues things you'll be able to look at in due course? Let me know if you want me to do anything.

Member

kring commented Mar 18, 2013

@bagnell, @mramato, @shunter, or whoever else: I think this is ready for master. We didn't do any of the caching stuff I mentioned above, but the performance is reasonably good with just the browser cache. There are lots of improvements we can make in the future, but it's useful as-is.

Member

pjcozzi commented Mar 18, 2013

I'd like to look at this but it won't be until later this week.

Member

kring commented Mar 20, 2013

In the documentation is it worth indicating that the height value is as provided by the terrain provider and is generally an elevation above the ellipsoid, not above mean sea level. e.g. It won't necessarily be zero if you sample in the ocean.

Good idea, @chris-cooper. Done.

@pjcozzi pjcozzi and 1 other commented on an outdated diff Mar 20, 2013

Source/Scene/sampleTerrain.js
+ * or another error occurs, the height is set to undefined. As is typical of the
+ * {@link Cartographic} type, the supplied height is a height above the reference ellipsoid
+ * (such as {@link Ellipsoid.WGS84}) rather than an altitude above mean sea level. In other
+ * words, it will not necessarily be 0.0 if sampled in the ocean.
+ *
+ * @exports sampleTerrain
+ *
+ * @param {TerrainProvider} terrainProvider The terrain provider from which to query heights.
+ * @param {Number} level The terrain level-of-detail from which to query terrain heights.
+ * @param {Cartographic[]} positions The positions to update with terrain heights.
+ *
+ * @returns {Promise} A promise that, when resolved, indicates that the query has completed.
+ *
+ * @example
+ * // Query the terrain height of two Cartographic positions
+ * var terrainProvider = new CesiumTerrainProvider({
@pjcozzi

pjcozzi Mar 20, 2013

Member

Why not pass positions to the promise so the original input doesn't need to be closed over in the promise?

@kring

kring Mar 21, 2013

Member

That's reasonable, though I think it's important that the example make clear that the original list of positions is modified.

@pjcozzi pjcozzi commented on the diff Mar 20, 2013

Apps/Sandcastle/gallery/Terrain.html
@@ -133,6 +134,77 @@
}
}
}).placeAt('zoomButtons');
+
+ var terrainSamplePositions;
@pjcozzi

pjcozzi Mar 20, 2013

Member

I think I can write this example more concisely by avoiding the removeAll calls. I'll clean it up on a branch in the main repo.

@pjcozzi pjcozzi commented on an outdated diff Mar 20, 2013

Source/Scene/sampleTerrain.js
+ throw new DeveloperError('terrainProvider is required.');
+ }
+ if (typeof level === 'undefined') {
+ throw new DeveloperError('level is required.');
+ }
+ if (typeof positions === 'undefined') {
+ throw new DeveloperError('positions is required.');
+ }
+
+ var tilingScheme = terrainProvider.getTilingScheme();
+
+ var i;
+
+ // Sort points into a set of tiles
+ var tileRequests = []; // Result will be an Array as it's easier to work with
+ {
@pjcozzi

pjcozzi Mar 20, 2013

Member

What are these brackets for? JavaScript doesn't have block-level scope. One day we will have it with let replacing var.

@pjcozzi pjcozzi commented on an outdated diff Mar 20, 2013

Source/Scene/sampleTerrain.js
+/*global define*/
+define([
+ '../Core/defaultValue',
+ '../Core/DeveloperError',
+ './TerrainState',
+ '../ThirdParty/when'
+ ], function(
+ defaultValue,
+ DeveloperError,
+ TerrainState,
+ when) {
+ "use strict";
+
+ /**
+ * Initiates a terrain height query for an array of {@link Cartographic} positions by
+ * requesting tiles from a terrain provider, sampling, and interpolating. The query
@pjcozzi

pjcozzi Mar 20, 2013

Member

Is it reasonable to document what interpolation is used? Later, I can see users wanting to be able to specify, but in the meantime, we should probably tell them how we do it. In my experience, it is always a hot topic.

Member

pjcozzi commented Mar 20, 2013

Tests pass. Example runs well. Just those few questions.

Member

kring commented Mar 21, 2013

This is ready for another look.

Member

pjcozzi commented Mar 21, 2013

Looks good. Tests and example are still OK. Merging.

Thanks @chris-cooper

@pjcozzi pjcozzi added a commit that referenced this pull request Mar 21, 2013

@pjcozzi pjcozzi Merge pull request #559 from NICTA/terrainSample
Terrain sample
f481633

@pjcozzi pjcozzi merged commit f481633 into AnalyticalGraphicsInc:master Mar 21, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment