Skip to content

Loading…

Terrain #510

Merged
merged 308 commits into from

7 participants

@kring
Analytical Graphics, Inc. member

There are three major parts to this:

  • A significant refactoring of the terrain/imagery loading system, making it easier to understand, easier to add terrain providers, and probably less buggy.
  • Lots of changes in ShaderProgram and UniformState and the like to support lighting 2D and Columbus View as if they're 3D. The overall goal is to make 2D and Columbus View look just like 3D, except the Earth is a plane instead of a globe. The water rendering is part of this, sort of.
  • Terrain providers, so we can get real, actual terrain in Cesium.
kring and others added some commits
@kring kring Fix terrain example after widget changes. 43d4595
@kring kring Use child bits in each tile. 7a4be28
@kring kring Use terrain hosted on cesium.cloudapp.net. b0f6603
@kring kring Don't unload tiles with transitioning imagery. 06a2e3f
@kring kring Fix base layer stretching beyond its extent.
Previously, the base layer would not be mapped to tiles that didn't
overlap it at all.
5d56c64
@shunter shunter Merge remote-tracking branch 'origin/master' into terrain 312b0e7
@shunter shunter Update package list and fix widget startup. 6b91652
@shunter shunter Merge branch 'scheduleRenderLoop' into terrain 4a8ac94
@kring kring Merge remote-tracking branch 'origin/ocean' into terrainAndOcean eef53e0
@kring kring Apply water material to terrain tiles. b8f6489
@kring kring Use high-res water mask derived from SRTM data.
This is very hacked up at the moment.
22d5dba
@kring kring Slow down the wave animation. 24f9e1f
@kring kring Use the actual imagery color as the ocean base color. 8197d4c
@kring kring Tweak water fragment shader.
It should now be usable in normal material and as used by the central body
where the water color is supplied as a parameter instead of a uniform.
0d9ee12
@kring kring Merge remote-tracking branch 'origin/master' into terrainAndOcean 2330d10
@kring kring Fix apps that don't use CesiumTerrainProvider. 2612e19
@shunter shunter Merge branch 'master' into terrain 41af797
@kring kring Temporarily use GTOPO30 terrain. 92a5629
@kring kring Merge remote-tracking branch 'origin/master' into terrainAndOcean 12f0633
@kring kring Restore the fancy water.
It went away back the water material's normalMap uniform lost its default.
7e3c189
@kring kring Lighting tweaks. aa655ac
@kring kring Switch to using terrain/water data on cesium.agi.com. 17172c3
@kring kring Merge remote-tracking branch 'origin/master' into terrain 35937d0
@kring kring Merge branch 'terrain' into terrainAndOcean 9f8db91
@pjcozzi pjcozzi Merge branch 'master' into terrainAndOcean e9bfec5
@kring kring Fix test failures and add a new test. 4609b4c
@kring kring Merge branch 'terrain' into terrainAndOcean 45bb4bb
@kring kring Fix Sandcastle examples and test failures. 205475f
@kring kring Merge branch 'lighting' into terrainAndOcean
Conflicts:
	Source/Scene/CentralBody.js
	Source/Shaders/CentralBodyFS.glsl
e26104c
@kring kring Fix water shading. 354c296
@bagnell bagnell Swap column and row parameters in Matrix*.getElementIndex. 011757c
@bagnell bagnell Matrix3.fromQuaternion was returning the transpose of the rotation ma…
…trix.
d237da3
@mramato mramato Update CHANGES f581b9d
@mramato mramato Update CHANGES.md f534ace
@pjcozzi pjcozzi Remove DistanceIntervalMaterial e5ca51f
@pjcozzi pjcozzi Merged lighting into terrainAndOcean c2ae2fd
@kring kring Progress toward using a texture for the water mask. 6085e2c
@kring kring Working texture-based water masking. b37237d
@shunter shunter Fix use of ioQuery. 68ff428
@kring kring Load water mask from terrain tiles. 9417ed9
@kring kring Correctly handle missing water tiles.
Could still use some performance improvement.
135954d
@kring kring Re-use water mask texture instead of re-uploading it. 5b38c60
@kring kring Use smaller water normals texture. 887bfdc
@kring kring Optimizations to water shading. 05e0558
@kring kring Use SRTM terrain and watermask. 3fcc021
@kring kring Tile state improvements. d36074a
@kring kring Remove workaround for bad data.
Even though the data is only partially fixed...
8f8929d
@pjcozzi pjcozzi Merge branch 'lighting' into terrainAndOcean 8669122
@kring kring Add water specular highlights even w/o ocean. 618b806
@kring kring Merge remote-tracking branch 'origin/master' into terrain 60e0e79
@kring kring Merge remote-tracking branch 'origin/lighting' into terrain b5a2078
@kring kring Fix non-terrain examples. c232e19
@kring kring Merge remote-tracking branch 'origin/lighting' into terrainAndOcean a566998
@kring kring Merge remote-tracking branch 'origin/master' into terrainAndOcean 70efd84
@kring kring Merge remote-tracking branch 'origin/master' into terrain aba85dd
@kring kring Merge remote-tracking branch 'origin/terrainAndOcean' into terrain
Conflicts:
	Apps/Sandcastle/gallery/Terrain.html
2c98549
@kring kring Make water normal textures seamless.
This should eliminate (or at least substantially reduce) artifacts in the
water.
71a38d7
@kring kring Fix ocean on Android devices. cf691c2
@kring kring Don't over-allocate memory for vertices. 8479405
@kring kring Load terrain from cesium.agi.com. 28efcbc
@kring kring Remove old varying from CB vertex shader. 162ea31
@kring kring WIP improvements to Columbus View / 2D lighting. 1ce0c1c
@kring kring Closer to correct 2D/CV lighting. d6c86f2
@kring kring Merge remote-tracking branch 'origin/camera' into lighting2d fcca8e5
@kring kring Adjust terrain example after camera merge. 80cb21e
@kring kring Fix 2D lighting. 47237dd
@kring kring Fix lighting discontinuity when the camera leaves reality. abdf1b4
@kring kring Eliminate allocations in view2DTo3D. 6e081ce
@kring kring Merge remote-tracking branch 'origin/master' into lighting2d 81e9cd2
@kring kring Merge remote-tracking branch 'origin/master' into terrain 31956fe
@kring kring Merge remote-tracking branch 'origin/terrain' into lighting2d 29b0381
@kring kring Cleanup, mostly of documentation. 043e631
@kring kring Add tests for new automatic uniforms. b540e63
@mramato mramato Merge branch 'master' into lighting2d a0a5624
@kring kring Fix exception at Terrain example startup. c090b1c
@kring kring Lighting tweaks.
Fix specular lighting bug that caused multiple highlights per light source
in 2D and Columbus View.  Also made the pseudo-moon a bit dimmer and
optimize the ocean shader slightly.
881ad1d
@kring kring Much improved lighting in both 3D and 2D. 9d42179
@kring kring Lighting tweaks. 89c0f5d
@kring kring Lighting cleanup. 15d213b
@kring kring Merge remote-tracking branch 'origin/master' into terrain 31a606c
@kring kring Merge remote-tracking branch 'origin/terrain' into lighting2d 50bf74c
@kring kring Build ocean rendering into CentralBody.
Previously it was hacked in via the material system.
0904777
@kring kring Make no-wave shading match with-wave shading. d7e06c8
@kring kring Fix non-terrain examples. 15ea3af
@kring kring Merge remote-tracking branch 'origin/master' into terrain b02b935
@kring kring Fix water in 2D. 5ff6291
@kring kring Changes from review.
This includes an attempt at fixing the polygon lighting that didn't work.
I'm pretty sure it's more correct, though.
ea578d5
@kring kring Merge pull request #367 from AnalyticalGraphicsInc/lighting2d
Make 2D and Columbus View lighting match 3D
28e8d1c
@kring kring Pseudo-moon highlight and texture coordinates at the poles.
Manipulate the texture coordinates so the water looks halfway decent at
the poles.
e88ff8b
@emackey emackey Merge remote-tracking branch 'origin/master' into terrain d7aafcd
@emackey emackey Tweak Terrain demo in Sandcastle. e101262
@emackey emackey More Terrain Sandcastle demo tweaks. 09580e0
@kring kring Fix tile bounding sphere debugging. fd54bf4
@kring kring Use the new, smaller terrain source. 7011677
@kring kring Imagery refinement prior to terrain downloaded.
Not entirely working yet.
5a6ae53
@kring kring Fix remaining problems with terrain refinement. ecfa351
@vicb vicb Build createVerticesFromHeightmap 8cdff05
@kring kring Render terrain in Columbus View. 50b47b2
@mramato mramato Merge remote-tracking branch 'origin/master' into terrain ac3923a
@kring kring Merge pull request #381 from vicb/terrain
Build createVerticesFromHeightmap
5945bce
@pjcozzi pjcozzi commented on an outdated diff
Source/Scene/CentralBodySurface.js
((21 lines not shown))
}
tile = tile.loadNext;
} while (Date.now() < endTime && typeof tile !== 'undefined');
}
+ var cartesian3Scratch = new Cartesian3(0.0, 0.0, 0.0);
@pjcozzi Analytical Graphics, Inc. member
pjcozzi added a note

Same comment about Cartesian3.ZERO.clone() throughout. I don't care one way or the other, but Cesium being consistent throughout would be good.

@kring Analytical Graphics, Inc. member
kring added a note

The most common thing I've seen elsewhere in Cesium is new Cartesian3() (193 occurrences). I've been doing new Cartesian3(0.0, 0.0, 0.0) (37 occurrences) because it's slightly more performant and arguably clearer. I'm fine with switching to new Cartesian3() though. I'm not a fan of Cartesian3.ZERO.clone() (9 occurrences).

@kring Analytical Graphics, Inc. member
kring added a note

I switched to new Cartesian3() everywhere.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pjcozzi pjcozzi commented on an outdated diff
Source/Scene/CentralBodySurface.js
((261 lines not shown))
+ // Parent tile data is not yet received or upsampled, so assume (for now) that this
+ // child tile is not available.
+ return false;
+ }
+
+ return parent.terrainData.isChildAvailable(parent.x, parent.y, tile.x, tile.y);
+ }
+
+ function createWaterMaskTexture(surface, context, waterMask) {
+ var result;
+
+ var waterMaskSize = Math.sqrt(waterMask.length);
+ if (waterMaskSize === 1 && (waterMask[0] === 0 || waterMask[0] === 255)) {
+ // Tile is entirely land or entirely water.
+ if (typeof surface._allWaterTexture === 'undefined') {
+ surface._allWaterTexture = context.createTexture2D({
@pjcozzi Analytical Graphics, Inc. member
pjcozzi added a note

Not sure if it is worth it, but Context.getDefaultTexture() could also work for you here.

@pjcozzi Analytical Graphics, Inc. member
pjcozzi added a note

P.S. looking forward to optimized shaders for land-only, etc. :+1:

@kring Analytical Graphics, Inc. member
kring added a note

Not sure if it is worth it, but Context.getDefaultTexture() could also work for you here.

Water textures are reference counted, so unless you want the default texture to be reference counted as well, I should probably leave it as-is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pjcozzi pjcozzi commented on an outdated diff
Source/Scene/HeightmapTerrainData.js
((16 lines not shown))
+ defaultValue,
+ BoundingSphere,
+ Cartesian3,
+ DeveloperError,
+ HeightmapTessellator,
+ CesiumMath,
+ Occluder,
+ TaskProcessor,
+ GeographicTilingScheme,
+ TerrainMesh,
+ TerrainProvider,
+ when) {
+ "use strict";
+
+ /**
+ * Terrain data for a single {@link Tile} where the terrain data is represented as a heightmap. A heightmap
@pjcozzi Analytical Graphics, Inc. member
pjcozzi added a note

Could use an @example.

@pjcozzi Analytical Graphics, Inc. member
pjcozzi added a note

Bad doc link: Tile. Doc for Tile is not generated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pjcozzi pjcozzi commented on an outdated diff
Source/Scene/HeightmapTerrainData.js
((22 lines not shown))
+ Occluder,
+ TaskProcessor,
+ GeographicTilingScheme,
+ TerrainMesh,
+ TerrainProvider,
+ when) {
+ "use strict";
+
+ /**
+ * Terrain data for a single {@link Tile} where the terrain data is represented as a heightmap. A heightmap
+ * is a rectangular array of heights in row-major order from south to north and west to east.
+ *
+ * @alias HeightmapTerrainData
+ * @constructor
+ *
+ * @param {TypedArray} buffer The buffer containing height data.
@pjcozzi Analytical Graphics, Inc. member
pjcozzi added a note

Better to use an object literal for arguments like HeightmapTessellator?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pjcozzi pjcozzi commented on an outdated diff
Source/Scene/HeightmapTerrainData.js
((64 lines not shown))
+ * @param {Number} [structure.elementMultiplier=256.0] The multiplier used to compute the height value when the
+ * stride property is greater than 1. For example, if the stride is 4 and the strideMultiplier
+ * is 256, the height is computed as follows:
+ * `height = buffer[index] + buffer[index + 1] * 256 + buffer[index + 2] * 256 * 256 + buffer[index + 3] * 256 * 256 * 256`
+ * This is assuming that the isBigEndian property is false. If it is true, the order of the
+ * elements is reversed.
+ * @param {Boolean} [structure.isBigEndian=false] Indicates endianness of the elements in the buffer when the
+ * stride property is greater than 1. If this property is false, the first element is the
+ * low-order element. If it is true, the first element is the high-order element.
+ * @param {Boolean} [createdByUpsampling=false] True if this instance was created by upsampling another instance;
+ * otherwise, false.
+ *
+ * @see TerrainData
+ */
+ var HeightmapTerrainData = function HeightmapTerrainData(buffer, width, height, childTileMask, structure, createdByUpsampling, waterMask) {
+ this._buffer = buffer;
@pjcozzi Analytical Graphics, Inc. member
pjcozzi added a note

Throw DeveloperError for undefined buffer, width, or height?

@pjcozzi Analytical Graphics, Inc. member
pjcozzi added a note

Similar comment throughout this file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pjcozzi pjcozzi commented on an outdated diff
Source/Scene/TerrainMesh.js
((38 lines not shown))
+ * U and V are the texture coordinates.
+ * @type {Float32Array}
+ */
+ this.vertices = vertices;
+
+ /**
+ * The indices describing how the vertices are connected to form triangles.
+ * @type {Uint16Array}
+ */
+ this.indices = indices;
+
+ /**
+ * The lowest height in the tile, in meters above the ellipsoid.
+ * @type {Number}
+ */
+ this.minHeight = minHeight;
@pjcozzi Analytical Graphics, Inc. member
pjcozzi added a note

Again, probably spell out min and max.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pjcozzi pjcozzi commented on an outdated diff
Source/Shaders/CentralBodyFS.glsl
@@ -13,9 +15,19 @@ uniform float u_dayTextureOneOverGamma[TEXTURE_UNITS];
uniform vec4 u_dayTextureTexCoordsExtent[TEXTURE_UNITS];
#endif
+#ifdef SHOW_REFLECTIVE_OCEAN
+uniform sampler2D u_waterMask;
+uniform vec4 u_waterMaskTranslationAndScale;
+uniform float u_zoomedOutOceanSpecularIntensity;
+
+#ifdef SHOW_OCEAN_WAVES
@pjcozzi Analytical Graphics, Inc. member
pjcozzi added a note

Do we have to nest this #ifdef inside #ifdef SHOW_REFLECTIVE_OCEAN. Not sure if it is cleaner one way or the other.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pjcozzi pjcozzi commented on an outdated diff
Source/Shaders/CentralBodyFS.glsl
((30 lines not shown))
}
+
+#ifdef SHOW_REFLECTIVE_OCEAN
+
+#ifdef SHOW_OCEAN_WAVES
+vec4 getNoise(vec2 uv, float time)
@pjcozzi Analytical Graphics, Inc. member
pjcozzi added a note

Is this duplicate from the water material? Should this be a czm_ function?

@kring Analytical Graphics, Inc. member
kring added a note

Almost. The getNoise in the water shader takes an angleInRadians that the ocean doesn't (currently?) use. If I make this a czm_ function, do you have a suggestion for the name? Is czm_getNoise too general?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pjcozzi pjcozzi commented on an outdated diff
Source/Renderer/ShaderProgram.js
@@ -275,6 +275,44 @@ define([
},
/**
+ * An automatic GLSL uniform representing a 4x4 view transformation matrix that
+ * transforms 3D world coordinates to eye coordinates. In 3D mode, this is identical to
+ * {@link ShaderProgram#czm_view}, but in 2D and Columbus View it represents the view matrix
@pjcozzi Analytical Graphics, Inc. member
pjcozzi added a note

Bad doc link: ShaderProgram#czm_view.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pjcozzi pjcozzi commented on an outdated diff
Source/Renderer/ShaderProgram.js
@@ -312,6 +350,44 @@ define([
},
/**
+ * An automatic GLSL uniform representing a 3x3 view rotation matrix that
+ * transforms vectors in 3D world coordinates to eye coordinates. In 3D mode, this is identical to
+ * {@link ShaderProgram#czm_viewRotation}, but in 2D and Columbus View it represents the view matrix
@pjcozzi Analytical Graphics, Inc. member
pjcozzi added a note

Bad doc link: ShaderProgram#czm_viewRotation.

@pjcozzi Analytical Graphics, Inc. member
pjcozzi added a note

Same comment throughout for anything that begins with ShaderProgram#czm_.

@kring Analytical Graphics, Inc. member
kring added a note

Bad doc link: ShaderProgram#czm_viewRotation.

Is there any way to create a link like this that actually works? CC @shunter and @kristiancalhoun

@pjcozzi Analytical Graphics, Inc. member
pjcozzi added a note

Links to GLSL uniforms work with @see. Do they not work with @link?

@kring Analytical Graphics, Inc. member
kring added a note

Ah, the trick is to leave off the ShaderProgram# before the uniform name, then @link works.

Next question: what about static, non-GLSL functions. Like HeightmapTessellator#computeVertices below? Do we have any way to link to those?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pjcozzi pjcozzi commented on an outdated diff
Source/Scene/HeightmapTerrainData.js
((19 lines not shown))
+ DeveloperError,
+ HeightmapTessellator,
+ CesiumMath,
+ Occluder,
+ TaskProcessor,
+ GeographicTilingScheme,
+ TerrainMesh,
+ TerrainProvider,
+ when) {
+ "use strict";
+
+ /**
+ * Terrain data for a single {@link Tile} where the terrain data is represented as a heightmap. A heightmap
+ * is a rectangular array of heights in row-major order from south to north and west to east.
+ *
+ * @alias HeightmapTerrainData
@pjcozzi Analytical Graphics, Inc. member
pjcozzi added a note

The generated help page for this is way too wide, but that is a separate issue, and also affects other help pages.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pjcozzi pjcozzi commented on an outdated diff
Source/Core/HeightmapTessellator.js
((24 lines not shown))
+ "use strict";
+
+ /**
+ * Contains functions to create a mesh from a heightmap image.
+ *
+ * @exports HeightmapTessellator
+ *
+ * @see ExtentTessellator
+ * @see CubeMapEllipsoidTessellator
+ * @see BoxTessellator
+ * @see PlaneTessellator
+ */
+ var HeightmapTessellator = {};
+
+ /**
+ * The default structure of a heightmap, as given to {@link HeightmapTessellator#computeVertices}.
@pjcozzi Analytical Graphics, Inc. member
pjcozzi added a note

Bad doc link: HeightmapTessellator#computeVertices.

@pjcozzi Analytical Graphics, Inc. member
pjcozzi added a note

Yes, use HeightmapTessellator.computeVertices instead of HeightmapTessellator#computeVertices.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pjcozzi pjcozzi commented on the diff
Source/Scene/ArcGisImageServerTerrainProvider.js
((21 lines not shown))
+ writeTextToCanvas,
+ DeveloperError,
+ CesiumMath,
+ Ellipsoid,
+ Event,
+ TerrainProvider,
+ GeographicTilingScheme,
+ HeightmapTerrainData,
+ when) {
+ "use strict";
+
+ /**
+ * A {@link TerrainProvider} that produces terrain geometry by tessellating height maps
+ * retrieved from an ArcGIS ImageServer.
+ *
+ * @alias ArcGisImageServerTerrainProvider
@pjcozzi Analytical Graphics, Inc. member
pjcozzi added a note

Help for this is not generated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pjcozzi pjcozzi commented on the diff
Source/Scene/CesiumTerrainProvider.js
((12 lines not shown))
+ '../ThirdParty/when'
+ ], function(
+ defaultValue,
+ loadArrayBuffer,
+ throttleRequestByServer,
+ writeTextToCanvas,
+ DeveloperError,
+ Event,
+ GeographicTilingScheme,
+ HeightmapTerrainData,
+ TerrainProvider,
+ when) {
+ "use strict";
+
+ /**
+ * A {@link TerrainProvider} that produces geometry by tessellating height maps
@pjcozzi Analytical Graphics, Inc. member
pjcozzi added a note

This generated help doesn't show up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pjcozzi pjcozzi commented on the diff
Source/Scene/EllipsoidTerrainProvider.js
@@ -41,7 +21,6 @@ define([
*
* @alias EllipsoidTerrainProvider
@pjcozzi Analytical Graphics, Inc. member
pjcozzi added a note

This generated help doesn't show up.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pjcozzi pjcozzi commented on an outdated diff
Source/Scene/HeightmapTerrainData.js
((30 lines not shown))
+ /**
+ * Terrain data for a single {@link Tile} where the terrain data is represented as a heightmap. A heightmap
+ * is a rectangular array of heights in row-major order from south to north and west to east.
+ *
+ * @alias HeightmapTerrainData
+ * @constructor
+ *
+ * @param {TypedArray} buffer The buffer containing height data.
+ * @param {Number} width The width (longitude direction) of the heightmap, in samples.
+ * @param {Number} height The height (latitude direction) of the heightmap, in samples.
+ * @param {Number} [childTileMask=15] A bit mask indicating which of this tile's four children exist.
+ * If a child's bit is set, geometry will be requested for that tile as well when it
+ * is needed. If the bit is cleared, the child tile is not requested and geometry is
+ * instead upsampled from the parent. The bit values are as follows:
+ * <table>
+ * <th><td>Bit Position</td><td>Bit Value</td><td>Child Tile</td></th>
@pjcozzi Analytical Graphics, Inc. member
pjcozzi added a note

Strangely, this row has an extra left-most column than the others. Perhaps replace th with tr.

@kring Analytical Graphics, Inc. member
kring added a note

Turns out <th> is the header version of <td>, not <tr>. HTML is hard. Fixed now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pjcozzi pjcozzi commented on an outdated diff
Source/Scene/HeightmapTerrainData.js
((185 lines not shown))
+ if ((this._width % 2) === 1 && (this._height % 2) === 1) {
+ // We have an odd number of posts greater than 2 in each direction,
+ // so we can upsample by simply dropping half of the posts in each direction.
+ result = upsampleBySubsetting(this, tilingScheme, thisX, thisY, thisLevel, descendantX, descendantY, descendantLevel);
+ } else {
+ // The number of posts in at least one direction is even, so we must upsample
+ // by interpolating heights.
+ result = upsampleByInterpolating(this, tilingScheme, thisX, thisY, thisLevel, descendantX, descendantY, descendantLevel);
+ }
+
+ return result;
+ };
+
+ /**
+ * Determines if a given child tile is available, based on the
+ * {@link HeightmapTerrainData#childTileMask}. The given child tile coordinates are assumed
@pjcozzi Analytical Graphics, Inc. member
pjcozzi added a note

Bad doc link: HeightmapTerrainData#childTileMask. Might want to check all the @link tags for ones that use # for non-prototype members. I'll mark the ones I notice.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pjcozzi
Analytical Graphics, Inc. member

Is backface culling on in Columbus view? When I go under the ground, I can see the backfaces.

@pjcozzi
Analytical Graphics, Inc. member

FYI. Backface culling in 3D works.

@pjcozzi
Analytical Graphics, Inc. member

This doesn't need to be part of this pull request, but refinement doesn't appear to work the same in 3D and Columbus view. For example, in the terrain Sandcastle example, zoom to Mount Everest in 3D. Now do the same in Columbus view. You'll get a much lower resolution version, even if you zoom very close to it.

@pjcozzi
Analytical Graphics, Inc. member

I get this artifact in Firefox. Not sure that we can do anything about it:

image

@pjcozzi pjcozzi commented on an outdated diff
Apps/Sandcastle/gallery/Terrain.html
((51 lines not shown))
+
+ function createMenuItem(scene, centralBody, providerInfo) {
+ return new MenuItem({
+ label : providerInfo.name,
+ onClick : function() {
+ cb.setTerrainProvider(providerInfo.provider);
+ createTerrainMenu(scene, centralBody, terrainProviders);
+ }
+ });
+ }
+
+ function createTerrainMenu(scene, centralBody, terrainProviders) {
+ domConstruct.place(
+ '<table id="terrainToolbar">' +
+ ' <tr><td align="right">Terrain Provider:</td><td id="terrainMenu"></td></tr>' +
+ ' <tr><td align="right">Zoom to:</td><td id="zoomButtons"></td></tr>' +
@pjcozzi Analytical Graphics, Inc. member
pjcozzi added a note

@bagnell if you zoom to any of these places in the terrain Sandcastle example when in 2D, you get:

Uncaught DeveloperError: lookAt is not supported while morphing.
Error
    at new DeveloperError (http://localhost:8080/Source/Core/DeveloperError.js:44:22)
    at CameraController.lookAt (http://localhost:8080/Source/Scene/CameraController.js:772:19)
    at Button.onClick (http://localhost:8080/Apps/Sandcastle/templates/bucket.html:106:46)
    at declare._onClick (http://localhost:8080/ThirdParty/dojo-release-1.8.3-src/dijit/form/_ButtonMixin.js:43:29)
    at inherited (http://localhost:8080/ThirdParty/dojo-release-1.8.3-src/dojo/_base/declare.js:189:30)
    at declare._onClick (http://localhost:8080/ThirdParty/dojo-release-1.8.3-src/dijit/form/Button.js:64:17)
    at HTMLSpanElement.<anonymous> (http://localhost:8080/ThirdParty/dojo-release-1.8.3-src/dojo/_base/lang.js:373:45)
    at HTMLSpanElement.handles (http://localhost:8080/ThirdParty/dojo-release-1.8.3-src/dijit/a11yclick.js:84:15) (on line 772 of http://localhost:8080/Source/Scene/CameraController.js)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pjcozzi
Analytical Graphics, Inc. member

@kring this is pretty solid. I'd still like to:

  • Review the tests when they are ready.
  • Read the rest of the generated doc when it is ready.

Also, the roadmap needs some updates. Does the server page need updates too?

What is the plan for exaggerating height? Can a user do this dynamically? If not, it doesn't have to be part of this pull request, but will be important for some folks.

Later, we also need to precompile the 2D/Columbus-view shaders since the initial transition is basically skipped while the shaders compile. CC #410/#451

@pjcozzi pjcozzi commented on the diff
Source/Scene/ArcGisImageServerTerrainProvider.js
((19 lines not shown))
+ getImagePixels,
+ throttleRequestByServer,
+ writeTextToCanvas,
+ DeveloperError,
+ CesiumMath,
+ Ellipsoid,
+ Event,
+ TerrainProvider,
+ GeographicTilingScheme,
+ HeightmapTerrainData,
+ when) {
+ "use strict";
+
+ /**
+ * A {@link TerrainProvider} that produces terrain geometry by tessellating height maps
+ * retrieved from an ArcGIS ImageServer.
@pjcozzi Analytical Graphics, Inc. member
pjcozzi added a note

"Image Server?"

@pjcozzi Analytical Graphics, Inc. member
pjcozzi added a note

I mean "Image Server", not "ImageServer."

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pjcozzi pjcozzi commented on an outdated diff
Source/Scene/ArcGisImageServerTerrainProvider.js
((78 lines not shown))
+ isBigEndian : true
+ };
+
+ this._errorEvent = new Event();
+
+ if (typeof description.credit !== 'undefined') {
+ // Create the copyright message.
+ this._logo = writeTextToCanvas(description.credit, {
+ font : '12px sans-serif'
+ });
+ }
+ }
+
+ /**
+ * Requests the geometry for a given tile. This function should not be called before
+ * {@link ArcGisImageServerTerrainProvider#isReady} returns true. The result must include terrain data and
@pjcozzi Analytical Graphics, Inc. member
pjcozzi added a note

Not sure that we want to mention water here since this terrain provider doesn't have a water mask.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pjcozzi pjcozzi commented on the diff
Source/Scene/ArcGisImageServerTerrainProvider.js
((30 lines not shown))
+ "use strict";
+
+ /**
+ * A {@link TerrainProvider} that produces terrain geometry by tessellating height maps
+ * retrieved from an ArcGIS ImageServer.
+ *
+ * @alias ArcGisImageServerTerrainProvider
+ * @constructor
+ *
+ * @param {String} description.url The URL of the ArcGIS ImageServer service.
+ * @param {String} [description.token] The authorization token to use to connect to the service.
+ * @param {Object} [description.proxy] A proxy to use for requests. This object is expected to have a getURL function which returns the proxied URL, if needed.
+ * @param {TilingScheme} [description.tilingScheme] The tiling scheme specifying how the terrain
+ * is broken into tiles. If this parameter is not provided, a {@link GeographicTilingScheme}
+ * is used.
+ * @param {Ellipsoid} [description.ellipsoid] The ellipsoid. If the tilingScheme is specified,
@pjcozzi Analytical Graphics, Inc. member
pjcozzi added a note

I'm on the fence, but is it really worth taking both a TilingScheme and Ellipsoid? It's a bit awkward for the user, but I guess it makes it easier for them if they just want to provide an ellipsoid or if the provider wants to change the default tiling scheme later.

@pjcozzi Analytical Graphics, Inc. member
pjcozzi added a note

Same comment for the other providers too, of course.

@kring Analytical Graphics, Inc. member
kring added a note

Unless you're strongly opposed, I'd like to keep it.

@pjcozzi Analytical Graphics, Inc. member
pjcozzi added a note

OK with me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pjcozzi pjcozzi commented on an outdated diff
Source/Scene/CentralBody.js
((7 lines not shown))
+ * @memberof CentralBody
+ *
+ * @returns {TerrainProvider} The terrain provider.
+ */
+ CentralBody.prototype.getTerrainProvider = function() {
+ return this._surface.getTerrainProvider();
+ };
+
+ /**
+ * Sets the terrain provider providing surface geometry for this central body.
+ *
+ * @memberof CentralBody
+ *
+ * @param {TerrainProvider} terrainProvider The new terrain provider.
+ */
+ CentralBody.prototype.setTerrainProvider = function(terrainProvider) {
@pjcozzi Analytical Graphics, Inc. member
pjcozzi added a note

Is it possible/reasonable to expose terrainProvider as a property and shadow it to know when it changes like the normal map URL?

@kring Analytical Graphics, Inc. member
kring added a note

I think this is a pattern that we will come to regret in Cesium, but it's certainly more consistent with what we're doing elsewhere, so I made the change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mramato mramato commented on the diff
Source/Scene/CentralBodySurface.js
@@ -130,6 +154,33 @@ define([
debugCreateRenderCommandsForTileBoundingSphere(this, context, frameState, centralBodyUniformMap, shaderSet, renderState, colorCommandList);
};
+ CentralBodySurface.prototype.getTerrainProvider = function() {
+ return this._terrainProvider;
+ }
@mramato Analytical Graphics, Inc. member
mramato added a note

Missing a ;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mramato mramato commented on an outdated diff
Apps/Sandcastle/gallery/Terrain.html
((139 lines not shown))
+ }).placeAt('zoomButtons');
+ }
+
+ // Parse URL configuration options into endUserOptions
+ var endUserOptions = {};
+ if (window.location.search) {
+ endUserOptions = ioQuery.queryToObject(window.location.search.substring(1));
+ }
+
+ // Initialize a viewer capable of drag-and-drop
+ // and user customizations.
+ var widget = new CesiumViewerWidget({
+ endUserOptions : endUserOptions,
+ enableDragDrop : true
+ });
+ widget.placeAt('cesiumContainer');
@mramato Analytical Graphics, Inc. member
mramato added a note

You need to add widget.fullscreenElement = document.body; here so that everything shows up in full screen mode and not just the viewer widget.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pjcozzi pjcozzi commented on an outdated diff
Source/Shaders/CentralBodyFS.glsl
((18 lines not shown))
+ mat3 enuToEye = czm_eastNorthUpToEyeCoordinates(v_positionMC, normalEC);
+
+ vec2 ellipsoidTextureCoordinates = czm_ellipsoidWgs84TextureCoordinates(normalMC);
+ vec2 ellipsoidFlippedTextureCoordinates = czm_ellipsoidWgs84TextureCoordinates(normalMC.zyx);
+
+ vec2 textureCoordinates = mix(ellipsoidTextureCoordinates, ellipsoidFlippedTextureCoordinates, u_morphTime * smoothstep(0.9, 0.95, normalMC.z));
+
+ color = computeWaterColor(v_positionEC, textureCoordinates, enuToEye, startDayColor, mask);
+ }
+#endif
+
+ gl_FragColor = color;
+}
+
+#ifdef SHOW_OCEAN_WAVES
+vec4 getNoise(vec2 uv, float time)
@pjcozzi Analytical Graphics, Inc. member
pjcozzi added a note

RE: czm_getNoise (github appears to have lost your comment).

Maybe call it czm_getWaterNoise and mark it @private. This avoids the code duplication, but doesn't commit to it publicly. The full shader system will, of course, make this cleaner...one day.

@kring Analytical Graphics, Inc. member
kring added a note

Maybe call it czm_getWaterNoise and mark it @private. This avoids the code duplication, but doesn't commit to it publicly. The full shader system will, of course, make this cleaner...one day.

Done. By the way, the comment is still there, it's just attached to an oudated diff, just like this one will be.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@shunter shunter commented on an outdated diff
Source/Scene/CentralBodySurface.js
((169 lines not shown))
+ };
+ }
+
+ function propagateNewUpsampledDataToChildren(tile) {
+ // Now that there's new data for this tile:
+ // - child tiles that were previously upsampled need to be re-upsampled based on the new data.
+
+ // Generally this is only necessary when a child tile is upsampled, and then one
+ // of its ancestors receives new (better) data and we want to re-upsample from the
+ // new data.
+
+ if (typeof tile.children !== 'undefined') {
+ for (var childIndex = 0; childIndex < 4; ++childIndex) {
+ var childTile = tile.children[childIndex];
+ if (childTile.state !== TileState.START) {
+ if (childTile.terrainData !== 'undefined' && !childTile.terrainData.wasCreatedByUpsampling()) {
@shunter Analytical Graphics, Inc. member
shunter added a note

Missing typeof on the undefined check. I guess this is not tested?

@kring Analytical Graphics, Inc. member
kring added a note

Good catch. The effect of this is extremely subtle. It's certainly not tested currently, and the effect wasn't even apparent in normal use. I'll try to come up with a way to test this sort of thing but I'm not overly optimistic.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
kring and others added some commits
@kring kring Fix links to uniforms in ShaderProgram.
Also fix problem preventing doc from being generated for
ArcGisImageServerTerrainProvider.
e32afd9
@shunter shunter Remove incorrect undefined check. The shaders in the storage object c…
…an never be undefined anyway.
23112e6
@kring kring Fix several doc problems. db07974
@shunter shunter commented on an outdated diff
Source/Scene/CentralBodySurface.js
((202 lines not shown))
+ childTile.state = TileState.LOADING;
+ }
+ }
+ }
+ }
+
+ function propagateNewLoadedDataToChildren(tile) {
+ // Now that there's new data for this tile:
+ // - child tiles that were previously upsampled need to be re-upsampled based on the new data.
+ // - child tiles that were previously deemed unavailable may now be available.
+
+ if (typeof tile.children !== 'undefined') {
+ for (var childIndex = 0; childIndex < 4; ++childIndex) {
+ var childTile = tile.children[childIndex];
+ if (childTile.state !== TileState.START) {
+ if (childTile.terrainData !== 'undefined' && !childTile.terrainData.wasCreatedByUpsampling()) {
@shunter Analytical Graphics, Inc. member
shunter added a note

Missing typeof on the undefined check.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@kring
Analytical Graphics, Inc. member

What is the plan for exaggerating height? Can a user do this dynamically? If not, it doesn't have to be part of this pull request, but will be important for some folks.

Heights are baked into the vertex array, so dynamically changing the exaggeration would require us to either regenerate the vertices or bust out some other tricks. Definitely a separate pull request, I'd say.

@shunter shunter commented on an outdated diff
Source/Scene/TerrainProvider.js
((24 lines not shown))
offsetInBytes : 0,
strideInBytes : stride
- }, {
+ }];
+
+ attributes.push({
@shunter Analytical Graphics, Inc. member
shunter added a note

I think the previous version, declaring all elements in the array literal instead of using push for the second element, will be slightly faster and is just as readable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pjcozzi
Analytical Graphics, Inc. member

Heights are baked into the vertex array, so dynamically changing the exaggeration would require us to either regenerate the vertices or bust out some other tricks. Definitely a separate pull request, I'd say.

OK with me. I wonder if we can do something simple like extrude along the geodetic surface normal in the vertex shader. No need to figure it out now. Let's just keep the roadmap updated.

@pjcozzi
Analytical Graphics, Inc. member

I think this is a pattern that we will come to regret in Cesium, but it's certainly more consistent with what we're doing elsewhere, so I made the change

Efficient getters/setters will help. Actually for this property, we could probably just use them. Doesn't matter to me though.

@shunter shunter commented on an outdated diff
Source/Scene/CentralBodySurface.js
((63 lines not shown))
+ tile.northeastCornerCartesian.negate(cartesian3Scratch).cross(Cartesian3.UNIT_Z, cartesian3Scratch).normalize(tile.eastNormal);
+ ellipsoid.geodeticSurfaceNormal(southeastCornerCartesian, cartesian3Scratch).cross(tile.southwestCornerCartesian.subtract(southeastCornerCartesian, cartesian3Scratch2), cartesian3Scratch).normalize(tile.southNormal);
+ ellipsoid.geodeticSurfaceNormal(northwestCornerCartesian, cartesian3Scratch).cross(tile.northeastCornerCartesian.subtract(northwestCornerCartesian, cartesian3Scratch2), cartesian3Scratch).normalize(tile.northNormal);
+ }
+
+ function processTerrainStateMachine(surface, context, terrainProvider, tile) {
+ var loaded = tile.loadedTerrain;
+ var upsampled = tile.upsampledTerrain;
+ var suspendUpsampling = false;
+
+ if (typeof loaded !== 'undefined') {
+ loaded.processLoadStateMachine(context, terrainProvider, tile.x, tile.y, tile.level);
+
+ // Publish the terrain data on the tile as soon as it is available.
+ // We'll potentially need it to upsample child tiles.
+ if (loaded.state.value >= TerrainState.RECEIVED.value) {
@shunter Analytical Graphics, Inc. member
shunter added a note

This is pretty sneaky, but I think conceptually Cesium enumerations are already strongly tied to their numeric values, so I think it's OK. You can omit the .value and it will still work (enumerations coerce to numbers correctly), and maybe that would be better.

@shunter Analytical Graphics, Inc. member
shunter added a note

On the other hand, using .value is probably the fastest version, if performance is most important here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@kring
Analytical Graphics, Inc. member

Ok, this is ready for another (final?) look.

@mramato
Analytical Graphics, Inc. member

Okay, I'll do a quick review since I haven't really looked at this yet.

@mramato
Analytical Graphics, Inc. member

The Sandcastle example still throws an exception when you hit one of the zoom buttons in 2D.

@mramato mramato commented on the diff
Source/Core/Ellipsoid.js
@@ -71,6 +71,35 @@ define([
};
/**
+ * Duplicates an Ellipsoid instance.
+ *
+ * @memberof Ellipsoid
+ *
+ * @param {Ellipsoid} ellipsoid The ellipsoid to duplicate.
+ * @param {Ellipsoid} [result] The object onto which to store the result, or undefined if a new
+ * instance should be created.
+ * @returns {Ellipsoid} The cloned Ellipsoid.
+ */
+ Ellipsoid.clone = function(ellipsoid, result) {
@mramato Analytical Graphics, Inc. member
mramato added a note

Does it make sense for this to be a "static" function? Everything else in this file is on the prototype. At the very least we should provide a prototype version (and my vote would be to get rid of the static version). Also, we usually treat Ellipsoid as an immutable type (obviously it can't really be one in javascript), does providing a result parameter here break that paradigm? Don't we have a bunch of baked in assumptions that Ellipsoids don't change?

@kring Analytical Graphics, Inc. member
kring added a note

I'm using this to reconstitute an ellipsoid after passing it to a web worker. All the functions get stripped away and only the fields are left. By taking this neutered Ellipsoid and passing it through Ellipsoid.clone, I get back an actual Ellipsoid that is equivalent to the original. A prototype method won't work for that.

@mramato Analytical Graphics, Inc. member
mramato added a note

Good enough for me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@kring
Analytical Graphics, Inc. member

The Sandcastle example still throws an exception when you hit one of the zoom buttons in 2D.

Fixed. It's ridiculous the hoops we have to jump through to create a similar view in all 3 modes, of course.

@mramato mramato commented on the diff
Source/Scene/CentralBody.js
((16 lines not shown))
+ * testing primitives against terrain is that slight numerical noise or terrain level-of-detail
+ * switched can sometimes make a primitive that should be on the surface disappear underneath it.
+ *
+ * @type Boolean
+ */
+ this.depthTestAgainstTerrain = false;
+
+ /**
+ * The size of the terrain tile cache, expressed as a number of tiles. Any additional
+ * tiles beyond this number will be freed, as long as they aren't needed for rendering
+ * this frame. A larger number will consume more memory but will show detail faster
+ * when, for example, zooming out and then back in.
+ *
+ * @type Number
+ */
+ this.tileCacheSize = 100;
@mramato Analytical Graphics, Inc. member
mramato added a note

I'm more curious than anything else, did we do any testing to pick this number?

@kring Analytical Graphics, Inc. member
kring added a note

Cozzi asked the same thing. The answer is no, I totally made it up. Most of the time we "need" more than 100 tiles for rendering anyway, so the value of 100 basically means unload everything you don't need right now. A higher number would keep tiles in memory when you, say, zoom out and then back in, which would be nice. On the other than, having memory usage drop when you zoom out is a nice feature, too. I don't know the right answer.

@mramato Analytical Graphics, Inc. member
mramato added a note

Then the question is simple, do we care more about visual quality or memory? I think quality comes first so I would opt for a higher number by default. If people raise concerns over memory usage, we can always change the default.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mramato
Analytical Graphics, Inc. member

I completely agree about the camera. We need to make sure that at the "highest" programmatic level, all functionality is exposed in world-space and the difference between 2D/3D/Columbus are completely hidden.

@mramato
Analytical Graphics, Inc. member

I still have a race condition of some sort where if I start with an empty cache, switch to 2D, and zoom all the way in, I get major culling issues and half the screen just goes black. However, I just tried master and it appears to happen there too, so it shouldn't hold this up.

@mramato mramato commented on an outdated diff
Source/Scene/CesiumTerrainProvider.js
((14 lines not shown))
+ defaultValue,
+ loadArrayBuffer,
+ throttleRequestByServer,
+ writeTextToCanvas,
+ DeveloperError,
+ Event,
+ GeographicTilingScheme,
+ HeightmapTerrainData,
+ TerrainProvider,
+ when) {
+ "use strict";
+
+ /**
+ * A {@link TerrainProvider} that produces geometry by tessellating height maps
+ * retrieved from a Cesium terrain server. The format of the terrain tiles is described on the
+ * {@link https://github.com/AnalyticalGraphicsInc/cesium/wiki/Cesium-Terrain-Server Cesium wiki}.
@mramato Analytical Graphics, Inc. member
mramato added a note

This link is formatted incorrectly and leads to creating new wiki page.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mramato mramato commented on the diff
Source/Scene/CesiumTerrainProvider.js
((105 lines not shown))
+ var heightBuffer = new Uint16Array(buffer, 0, that._heightmapWidth * that._heightmapWidth);
+ return new HeightmapTerrainData({
+ buffer : heightBuffer,
+ childTileMask : new Uint8Array(buffer, heightBuffer.byteLength, 1)[0],
+ waterMask : new Uint8Array(buffer, heightBuffer.byteLength + 1, buffer.byteLength - heightBuffer.byteLength - 1),
+ width : that._heightmapWidth,
+ height : that._heightmapWidth,
+ structure : that._terrainDataStructure
+ });
+ });
+ };
+
+ /**
+ * Gets an event that is raised when the terrain provider encounters an asynchronous error. By subscribing
+ * to the event, you will be notified of the error and can potentially recover from it. Event listeners
+ * are passed an instance of {@link TileProviderError}.
@mramato Analytical Graphics, Inc. member
mramato added a note

This link is good but is broken, seems like TileProviderError doesn't get doc generated

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@mramato
Analytical Graphics, Inc. member

I didn't review it as closely as @pjcozzi did, but I looked over the code and did our "release" testing so confirm everything works. Other than the 2 doc issues, this is fine with me.

@mramato
Analytical Graphics, Inc. member

Also; I tried mobile

Tegra 3: water looks pretty bad, and it's slow; but everything seems to work. I don't think we made anything on mobile worse (which is the low bar I was setting when I tried it out).

Adreno 225: Looks freakin' awesome, everything works.

@pjcozzi
Analytical Graphics, Inc. member

@kring tests look good. Looks like you used the WebGL category accordingly. As @mramato said, they pass in Chrome and Firefox.

Coverage is reasonable enough. I'd probably at least add tests for the DeveloperError in TilingScheme.createRectangleOfLevelZeroTiles. All the imagery/terrain providers don't cover their "must not be called before the imagery provider is ready." I'm not sure how hard that is to test. We can say that it's outside the scope of this pull request though.

Let me know when the doc fixes are in, and we'll merge this. In the meantime, I'll get the tutorial ready to go - just need to update the date.

@kring
Analytical Graphics, Inc. member

I'd probably at least add tests for the DeveloperError in TilingScheme.createRectangleOfLevelZeroTiles.

Ha, that's some serious bike shedding. I'll add it tomorrow if this isn't already merged by then.

@pjcozzi
Analytical Graphics, Inc. member

Ha, that's some serious bike shedding. I'll add it tomorrow if this isn't already merged by then.

Sorry, there's actually 3 developer errors there. We should really cover all developer errors. I'm not sure that I ever opened a pull request that did not cover reasonable ones to test like these.

@kring
Analytical Graphics, Inc. member

Sorry, there's actually 3 developer errors there. We should really cover all developer errors. I'm not sure that I ever opened a pull request that did not cover reasonable ones to test like these.

Ok, this is taken care of. Just for the record, this code was added with the imagery_layers merge, and wasn't touched here.

Also, we clearly disagree on the importance of these DeveloperErrors. I was just making the case to Scott the other day that, in most cases, adding these undefined checks is a waste of time (developer time and a tiny bit of run time), and adding tests for them is even more so a waste of developer time.

Meanwhile, this pull request includes some very "tricky" tests of the tile load pipeline. Feedback on those would be valuable, as would another pair of eyes looking at what important aspects I may have missed (even though the coverage is good). Looking at coverage numbers of trivial code without looking at the hard stuff is classic bike shedding. I understand why it happens, and it's likely I'd do the same thing while reviewing someone else's pull request. So I probably sounded like an ass above, but we shouldn't kid ourselves.

@pjcozzi
Analytical Graphics, Inc. member

Looks good. Merging. Email the dev list once we get the tutorial up.

As for getting rid of developer error, I'm all ears. I talked about doing this in limited cases - for optimized release builds - a really long time ago. If you think it's pressing, bring it up on the dev list.

@pjcozzi pjcozzi merged commit 1c45e05 into master
@pjcozzi pjcozzi deleted the terrain branch
@pjcozzi pjcozzi referenced this pull request
Merged

Terrain Exaggeration #3200

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.