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 off-by-one in compressing/decompressing texture coordinates #4355

Merged
merged 8 commits into from
Sep 28, 2016
1 change: 1 addition & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ Change Log
* Fixed billboard rotation when sized in meters. [#3979](https://github.com/AnalyticalGraphicsInc/cesium/issues/3979)
* Fixed timeline touch events. [#4305](https://github.com/AnalyticalGraphicsInc/cesium/pull/4305)
* Fixed a bug that could lead to incorrect terrain heights when using `HeightmapTerrainData` with an encoding in which actual heights were equal to the minimum representable height.
* Fixed a bug in `AttributeCompression.compressTextureCoordinates` and `decompressTextureCoordinates` that could cause a small inaccuracy in the encoded texture coordinates.
* Added `DebugCameraPrimitive` to visualize the view frustum of a camera.

### 1.25 - 2016-09-01
Expand Down
22 changes: 12 additions & 10 deletions Source/Core/AttributeCompression.js
Original file line number Diff line number Diff line change
Expand Up @@ -75,9 +75,9 @@ define([
* @param {Cartesian3} vector The normalized vector to be compressed into 2 byte 'oct' encoding.
* @param {Cartesian2} result The 2 byte oct-encoded unit length vector.
* @returns {Cartesian2} The 2 byte oct-encoded unit length vector.
*
*
* @exception {DeveloperError} vector must be normalized.
*
*
* @see AttributeCompression.octEncodeInRange
* @see AttributeCompression.octDecode
*/
Expand Down Expand Up @@ -124,14 +124,14 @@ define([

/**
* Decodes a unit-length vector in 2 byte 'oct' encoding to a normalized 3-component vector.
*
*
* @param {Number} x The x component of the oct-encoded unit length vector.
* @param {Number} y The y component of the oct-encoded unit length vector.
* @param {Cartesian3} result The decoded and normalized vector.
* @returns {Cartesian3} The decoded and normalized vector.
*
*
* @exception {DeveloperError} x and y must be an unsigned normalized integer between 0 and 255.
*
*
* @see AttributeCompression.octDecodeInRange
*/
AttributeCompression.octDecode = function(x, y, result) {
Expand Down Expand Up @@ -268,7 +268,7 @@ define([
/**
* Pack texture coordinates into a single float. The texture coordinates will only preserve 12 bits of precision.
*
* @param {Cartesian2} textureCoordinates The texture coordinates to compress
* @param {Cartesian2} textureCoordinates The texture coordinates to compress. Both coordinates must be in the range 0.0-1.0.
* @returns {Number} The packed texture coordinates.
*
*/
Expand All @@ -279,8 +279,9 @@ define([
}
//>>includeEnd('debug');

var x = textureCoordinates.x === 1.0 ? 4095.0 : (textureCoordinates.x * 4096.0) | 0;
var y = textureCoordinates.y === 1.0 ? 4095.0 : (textureCoordinates.y * 4096.0) | 0;
// Move x and y to the range 0-4095;
var x = (textureCoordinates.x * 4095.0) | 0;
var y = (textureCoordinates.y * 4095.0) | 0;
return 4096.0 * x + y;
};

Expand All @@ -303,8 +304,9 @@ define([
//>>includeEnd('debug');

var temp = compressed / 4096.0;
result.x = Math.floor(temp) / 4096.0;
result.y = temp - Math.floor(temp);
var xZeroTo4095 = Math.floor(temp);
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe var xZeroTo4095 = compressed >> 12 etc?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah that's a good point, we could do this whole thing as integers and only make it a float at the end.

Copy link
Contributor

Choose a reason for hiding this comment

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

The reason I did it this way was to mirror what was done in the shader which doesn't have any bitwise operations, but its up to you.

result.x = xZeroTo4095 / 4095.0;
result.y = (compressed - xZeroTo4095 * 4096) / 4095;
return result;
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@
vec2 czm_decompressTextureCoordinates(float encoded)
{
float temp = encoded / 4096.0;
float stx = floor(temp) / 4096.0;
float sty = temp - floor(temp);
float xZeroTo4095 = floor(temp);
float stx = xZeroTo4095 / 4095.0;
float sty = (encoded - xZeroTo4095 * 4096.0) / 4095.0;
return vec2(stx, sty);
}
33 changes: 29 additions & 4 deletions Specs/Core/AttributeCompressionSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -490,24 +490,49 @@ defineSuite([

it('compresses texture coordinates', function() {
var coords = new Cartesian2(0.5, 0.5);
expect(AttributeCompression.decompressTextureCoordinates(AttributeCompression.compressTextureCoordinates(coords), new Cartesian2())).toEqual(coords);
expect(AttributeCompression.decompressTextureCoordinates(AttributeCompression.compressTextureCoordinates(coords), new Cartesian2())).toEqualEpsilon(coords, 1.0 / 4096.0);
});

it('compress texture coordinates throws without texture coordinates', function() {
expect(function() {
AttributeCompression.compressTextureCoordinates(undefined);
}).toThrowDeveloperError();
});

it('decompress texture coordinates throws without encoded texture coordinates', function() {
expect(function() {
AttributeCompression.decompressTextureCoordinates(undefined, new Cartesian2());
}).toThrowDeveloperError();
});

it('decompress texture coordinates throws without result', function() {
expect(function() {
AttributeCompression.decompressTextureCoordinates(0.0, undefined);
}).toThrowDeveloperError();
});

it('compresses/decompresses 1.0', function() {
var coords = new Cartesian2(1.0, 1.0);
expect(AttributeCompression.decompressTextureCoordinates(AttributeCompression.compressTextureCoordinates(coords), new Cartesian2())).toEqual(coords);
});

it('compresses/decompresses 0.0', function() {
var coords = new Cartesian2(1.0, 1.0);
expect(AttributeCompression.decompressTextureCoordinates(AttributeCompression.compressTextureCoordinates(coords), new Cartesian2())).toEqual(coords);
});

it('compresses/decompresses 0.5 / 1.0', function() {
var coords = new Cartesian2(0.5, 1.0);
expect(AttributeCompression.decompressTextureCoordinates(AttributeCompression.compressTextureCoordinates(coords), new Cartesian2())).toEqualEpsilon(coords, 1.0 / 4095.0);
});

it('compresses/decompresses 1.0 / 0.5', function() {
var coords = new Cartesian2(1.0, 0.5);
expect(AttributeCompression.decompressTextureCoordinates(AttributeCompression.compressTextureCoordinates(coords), new Cartesian2())).toEqualEpsilon(coords, 1.0 / 4095.0);
});

it('compresses/decompresses values very close but not equal to 1.0', function() {
var coords = new Cartesian2(0.99999999999999, 0.99999999999999);
expect(AttributeCompression.decompressTextureCoordinates(AttributeCompression.compressTextureCoordinates(coords), new Cartesian2())).toEqualEpsilon(coords, 1.0 / 4095.0);
});
});
8 changes: 4 additions & 4 deletions Specs/Core/TerrainEncodingSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ defineSuite([
expect(encoding.getStride()).toEqual(3);
expect(buffer.length).toEqual(encoding.getStride());

expect(encoding.decodeTextureCoordinates(buffer, 0)).toEqualEpsilon(texCoords, CesiumMath.EPSILON14);
expect(encoding.decodeTextureCoordinates(buffer, 0)).toEqualEpsilon(texCoords, 1.0 / 4095.0);
});

it('encodes textureCoordinates with quantization and normals', function() {
Expand All @@ -198,7 +198,7 @@ defineSuite([
expect(encoding.getStride()).toEqual(4);
expect(buffer.length).toEqual(encoding.getStride());

expect(encoding.decodeTextureCoordinates(buffer, 0)).toEqualEpsilon(texCoords, CesiumMath.EPSILON14);
expect(encoding.decodeTextureCoordinates(buffer, 0)).toEqualEpsilon(texCoords, 1.0 / 4095.0);
});

it('encodes height with quantization and without normals', function() {
Expand All @@ -214,7 +214,7 @@ defineSuite([
expect(encoding.getStride()).toEqual(3);
expect(buffer.length).toEqual(encoding.getStride());

expect(encoding.decodeHeight(buffer, 0)).toEqualEpsilon(height, CesiumMath.EPSILON14);
expect(encoding.decodeHeight(buffer, 0)).toEqualEpsilon(height, 200.0 / 4095.0);
});

it('encodes height with quantization and normals', function() {
Expand All @@ -230,7 +230,7 @@ defineSuite([
expect(encoding.getStride()).toEqual(4);
expect(buffer.length).toEqual(encoding.getStride());

expect(encoding.decodeHeight(buffer, 0)).toEqualEpsilon(height, CesiumMath.EPSILON14);
expect(encoding.decodeHeight(buffer, 0)).toEqualEpsilon(height, 200.0 / 4095.0);
});

it('gets oct-encoded normal', function() {
Expand Down
2 changes: 1 addition & 1 deletion Specs/Renderer/BuiltinFunctionsSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -219,7 +219,7 @@ defineSuite([
it('has czm_decompressTextureCoordinates', function() {
var fs =
'void main() { ' +
' gl_FragColor = vec4(czm_decompressTextureCoordinates(8390656.0) == vec2(0.5, 0.5)); ' +
' gl_FragColor = vec4(czm_decompressTextureCoordinates(8386559.0) == vec2(0.4998779, 0.4998779)); ' +
'}';
context.verifyDrawForSpecs(fs);
});
Expand Down