Guard against negative height samples in HeightmapTerrainData

[chris-cooper]

We are getting some strange artefacts with heightmap terrain that were introduced somewhere between Cesium versions 1.15 and 1.18.

Tiles outside the main dataset area are getting pushed up. On further investigation some of the remapped heightSamples from interpolateMeshHeight() are below zero, and setHeight() then encodes them as a value close to 65535 instead of zero. I notice in this commit an | 0 was added...

heights[index + i] = (height / divisor) | 0

Maybe @bagnell can provide some insight as to what that was guarding against as it may no longer be needed.

Note that this should still allow elevations below WGS84 zero. It's just the encoding provided by setHeight() doesn't seem to be built to handle the negative numbers being generated by interpolateMeshHeight().

[kring]

@chris-cooper I could be missing something, but I don't see how this change makes sense.

First, your claim that this should still allow heights below WGS84 zero. Are you sure? It looks like to me like all heights created by upsampling will be clamped at zero. That Math.max is applied prior to encoding. In fact, you can see this in your test, right? You start with a heightmap with some heights below zero, upsample it, and now your heights are clamped at zero.

Now it's true that HeightmapTerrainData heights are stored encoded. The encoding is defined by the structure parameter to the constructor. If you're using the standard CesiumTerrainProvider it is the number of 1/5 meter units above -1000 meters. You can use a different encoding that is more suitable to your data, if desired, but you'll have to change CesiumTerrainProvider or write your own terrain provider.

Because the encoded height buffer is a Uint16Array (again, in CesiumTerrainProvider, other terrain providers can do something different), if the encoded value ends up being negative, you'll end up with a large positive number instead. That sounds like what you're seeing.

It should only happen if your heights are outside the valid range of your structure though (i.e. below -1000 meters with the default structure).

So how can that happen? Well, sometime around when you started seeing the problem, Cesium started encoding terrain tile meshes in a clever way to save memory. It also started upsampling from meshes instead of upsampling from the original height data. The heights are encoded with AttributeCompression.compressTextureCoordinates, which IMO is pretty suspect. In particular that check for x === 1.0 that then clamps the value to 4095 is totally dodgy. It means that there are values slightly less than 1.0 that yield an x or y value in that function higher than 4095, allowing the encoding y value to potentially spill over into the x component when decoded. 😱 I don't know for sure that that is the cause of the problem you're seeing, but I bet it is.

So I'd suggest:

• At a minimum: move the Math.max(height, 0) inside setHeight. So: heights[index + i] = Math.max(0, height);
• Better: investigate if AttributeCompression.compressTextureCoordinates and decompressTextureCoordinates are broken and in the likely case that they are, fix them.

[kring]

By the way the | 0 you noticed was added simply ensures the values are integers by stripping off everything after the decimal point (i.e. rounding toward zero). I don't think it should cause any problems.

[chris-cooper]

Thanks for having a look @kring. I've moved the clamping as you've suggested. I tried changing compressTextureCoordinates and decompressTextureCoordinates but that didn't fix the problem.

[kring]

@chris-cooper I've just committed what I think is the right fix to texture coordinate compression to the compressedTextureCoordinates branch. Can you try it out? If that still doesn't fix your problem... is there any way I can take a look with your tileset? The change in this PR is ok with me now, but I still wonder if there's a deeper problem.

[kring]

Chris and I spent awhile looking at his problem in more detail. Basically, here's what was happening:

• Chris is using a custom terrain provider that is very similar to CesiumTerrainProvider with heightmaps except that it uses a different structure. Unlike the default heightmap structure, which supports a minimum height of -1000 meters, Chris's structure uses the terrain's actual minimum height as the minimum value. In other words, a value of 0 in the height buffer represents the actual minimum height in the terrain, and a value smaller than the minimum cannot be represented at all.
• The multiple iterations of upsampling and encoding/decoded of heights (using a different encoding scheme for storing in the mesh versus storing in the height buffer) meant that a tiny bit of floating point error was introduced in an upsampled mesh.
• The tiny error resulted in a height very slightly smaller than the minimum value (like in the 15th significant digit or so). When this value very slightly less than 0 was written to a Uint16Array, it became a large positive number instead.
• As a result, vertices that were meant to be at the minimum height instead ended up near the maximum height.

So the solution here of clamping the heightmap-encoded height value to 0 just before writing it into the heightmap buffer is reasonable. The only potential problem is some other terrain provider (not CesiumTerrainProvider or any other built-in one, and not Chris's) uses a type that can support negative numbers (e.g. Float32Array instead of Uint16Array), and a corresponding structure that intentionally allows the values to be negative, this change will incorrectly clamp them.

I'm going to hold off on merging this a bit longer in order to think about how much of a problem this actually is, and what we might do about it.

[kring]

@chris-cooper can you try out this branch?
https://github.com/AnalyticalGraphicsInc/cesium/compare/PropellerAero-heightmap-fix?expand=1

You'll need to add the new fields, lowestEncodedHeight and highestEncodedHeight to the structure created in your terrain provider. You'll want to use the same values as CesiumTerrainProvider is using.

[chris-cooper]

Thanks for your time @kring. Sorry it took longer than expected to investigate. Yes your fix work and is safer than the one I proposed. I've updated the spec. I notice the ternary operators will fall back to the old behaviour if lowestEncodedHeight or highestEncodedHeight is undefined which allows the other specs to pass.

I think compressTextureCoordinates should still be fixed like you proposed, but probably in a separate PR.

[pjcozzi]

Should this include an update to CHANGES.md? If so, please update in master.

[pjcozzi]

Nevermind, I see #4354 (comment)