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

scale geometric error with tileset #7411

Merged
merged 10 commits into from
Sep 19, 2019
2 changes: 1 addition & 1 deletion Source/Core/GeometryPipeline.js
Original file line number Diff line number Diff line change
Expand Up @@ -808,7 +808,7 @@ define([

Matrix4.inverse(modelMatrix, inverseTranspose);
Matrix4.transpose(inverseTranspose, inverseTranspose);
Matrix4.getRotation(inverseTranspose, normalMatrix);
Matrix4.getMatrix3(inverseTranspose, normalMatrix);

transformVector(normalMatrix, attributes.normal);
transformVector(normalMatrix, attributes.tangent);
Expand Down
21 changes: 21 additions & 0 deletions Source/Core/Matrix3.js
Original file line number Diff line number Diff line change
Expand Up @@ -1023,6 +1023,27 @@ define([
return result;
};

var UNIT = new Cartesian3(1, 1, 1);

/**
* Extracts the rotation assuming the matrix is an affine transformation.
*
* @param {Matrix3} matrix The matrix.
* @param {Matrix3} result The object onto which to store the result.
* @returns {Cartesian3} The modified result parameter
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
* @returns {Cartesian3} The modified result parameter
* @returns {Matrix3} The modified result parameter

*/
Matrix3.getRotation = function(matrix, result) {
lilleyse marked this conversation as resolved.
Show resolved Hide resolved
//>>includeStart('debug', pragmas.debug);
Check.typeOf.object('matrix', matrix);
Check.typeOf.object('result', result);
//>>includeEnd('debug');

var inverseScale = Cartesian3.divideComponents(UNIT, Matrix3.getScale(matrix, scratchScale), scratchScale);
result = Matrix3.multiplyByScale(matrix, inverseScale, result);

return result;
};

function computeFrobeniusNorm(matrix) {
var norm = 0.0;
for (var i = 0; i < 9; ++i) {
Expand Down
15 changes: 11 additions & 4 deletions Source/Core/Matrix4.js
Original file line number Diff line number Diff line change
Expand Up @@ -2149,6 +2149,13 @@ define([
return result;
};

/**
* @deprecated moved to Matrix4.getMatrix3
*/
Matrix4.getRotation = function(matrix, result) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's fine to keep the original description and then insert the @deprecated line. Also add a deprecationWarning to the function.

    /**
     * Gets the upper left 3x3 rotation matrix of the provided matrix, assuming the matrix is a affine transformation matrix.
     *
     * @param {Matrix4} matrix The matrix to use.
     * @param {Matrix3} result The object onto which to store the result.
     * @returns {Matrix3} The modified result parameter.
     *
     * @deprecated This function has been deprecated. Use {@link Matrix4#getMatrix3} instead.
     *
     * @example
     * // returns a Matrix3 instance from a Matrix4 instance
     *
     * // m = [10.0, 14.0, 18.0, 22.0]
     * //     [11.0, 15.0, 19.0, 23.0]
     * //     [12.0, 16.0, 20.0, 24.0]
     * //     [13.0, 17.0, 21.0, 25.0]
     *
     * var b = new Cesium.Matrix3();
     * Cesium.Matrix4.getRotation(m,b);
     *
     * // b = [10.0, 14.0, 18.0]
     * //     [11.0, 15.0, 19.0]
     * //     [12.0, 16.0, 20.0]
     */
    Matrix4.getRotation = function(matrix, result) {
        deprecationWarning('Matrix4.getRotation', 'Matrix4.getRotation is deprecated and will be removed in Cesium 1.65. Use Matrix4.getMatrix3 instead.');
        return Matrix4.getMatrix3(matrix, result);
    };

Copy link
Contributor

Choose a reason for hiding this comment

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

Add the deprecation notice to CHANGES.md:

##### Deprecated :hourglass_flowing_sand:
* The function `Matrix4.getRotation` has been deprecated and renamed to `Matrix4.getMatrix3`. `Matrix4.getRotation` will be removed in version 1.65. [#7411](https://github.com/AnalyticalGraphicsInc/cesium/pull/7411)

Copy link
Contributor

Choose a reason for hiding this comment

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

There are still a lot of occurrences of Matrix4.getRotation in the code that should be changed to Matrix4.getMatrix3 (one in Frustum.html, the rest in specs).

return Matrix4.getMatrix3(matrix, result);
};

/**
* Gets the upper left 3x3 rotation matrix of the provided matrix, assuming the matrix is a affine transformation matrix.
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated typo I noticed.

Suggested change
* Gets the upper left 3x3 rotation matrix of the provided matrix, assuming the matrix is a affine transformation matrix.
* Gets the upper left 3x3 rotation matrix of the provided matrix, assuming the matrix is an affine transformation matrix.

*
Expand All @@ -2165,13 +2172,13 @@ define([
* // [13.0, 17.0, 21.0, 25.0]
*
* var b = new Cesium.Matrix3();
* Cesium.Matrix4.getRotation(m,b);
* Cesium.Matrix4.getMatrix3(m,b);
*
* // b = [10.0, 14.0, 18.0]
* // [11.0, 15.0, 19.0]
* // [12.0, 16.0, 20.0]
*/
Matrix4.getRotation = function(matrix, result) {
Matrix4.getMatrix3 = function(matrix, result) {
//>>includeStart('debug', pragmas.debug);
Check.typeOf.object('matrix', matrix);
Check.typeOf.object('result', result);
Expand Down Expand Up @@ -2286,7 +2293,7 @@ define([
if (Math.abs(det) < CesiumMath.EPSILON21) {
// Special case for a zero scale matrix that can occur, for example,
// when a model's node has a [0, 0, 0] scale.
if (Matrix3.equalsEpsilon(Matrix4.getRotation(matrix, scratchInverseRotation), scratchMatrix3Zero, CesiumMath.EPSILON7) &&
if (Matrix3.equalsEpsilon(Matrix4.getMatrix3(matrix, scratchInverseRotation), scratchMatrix3Zero, CesiumMath.EPSILON7) &&
Cartesian4.equals(Matrix4.getRow(matrix, 3, scratchBottomRow), scratchExpectedBottomRow)) {

result[0] = 0.0;
Expand Down Expand Up @@ -2353,7 +2360,7 @@ define([
//>>includeEnd('debug');

//This function is an optimized version of the below 4 lines.
//var rT = Matrix3.transpose(Matrix4.getRotation(matrix));
//var rT = Matrix3.transpose(Matrix4.getMatrix3(matrix));
//var rTN = Matrix3.negate(rT);
//var rTT = Matrix3.multiplyByVector(rTN, Matrix4.getTranslation(matrix));
//return Matrix4.fromRotationTranslation(rT, rTT, result);
Expand Down
6 changes: 3 additions & 3 deletions Source/Core/Transforms.js
Original file line number Diff line number Diff line change
Expand Up @@ -376,7 +376,7 @@ define([
//>>includeEnd('debug');

var transform = Transforms.headingPitchRollToFixedFrame(origin, headingPitchRoll, ellipsoid, fixedFrameTransform, scratchENUMatrix4);
var rotation = Matrix4.getRotation(transform, scratchHPRMatrix3);
var rotation = Matrix4.getMatrix3(transform, scratchHPRMatrix3);
return Quaternion.fromRotationMatrix(rotation, result);
};

Expand Down Expand Up @@ -421,7 +421,7 @@ define([
transformCopy = Matrix4.setTranslation(transformCopy, Cartesian3.ZERO, transformCopy);

toFixedFrame = Matrix4.multiply(toFixedFrame, transformCopy, toFixedFrame);
var quaternionRotation = Quaternion.fromRotationMatrix(Matrix4.getRotation(toFixedFrame, hprRotationScratch), hprQuaternionScratch);
var quaternionRotation = Quaternion.fromRotationMatrix(Matrix4.getMatrix3(toFixedFrame, hprRotationScratch), hprQuaternionScratch);
quaternionRotation = Quaternion.normalize(quaternionRotation, quaternionRotation);

return HeadingPitchRoll.fromQuaternion(quaternionRotation, result);
Expand Down Expand Up @@ -878,7 +878,7 @@ define([
// Assuming the instance are positioned in WGS84, invert the WGS84 transform to get the local transform and then convert to 2D
var fromENU = Transforms.eastNorthUpToFixedFrame(rtcCenter, ellipsoid, scratchFromENU);
var toENU = Matrix4.inverseTransformation(fromENU, scratchToENU);
var rotation = Matrix4.getRotation(matrix, scratchRotation);
var rotation = Matrix4.getMatrix3(matrix, scratchRotation);
var local = Matrix4.multiplyByMatrix3(toENU, rotation, result);
Matrix4.multiply(swizzleMatrix, local, result); // Swap x, y, z for 2D
Matrix4.setTranslation(result, projectedPosition, result); // Use the projected center
Expand Down
24 changes: 13 additions & 11 deletions Source/Renderer/UniformState.js
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,7 @@ define([
if (this._inverseTransposeModelDirty) {
this._inverseTransposeModelDirty = false;

Matrix4.getRotation(this.inverseModel, m);
Matrix4.getMatrix3(this.inverseModel, m);
Matrix3.transpose(m, m);
}

Expand Down Expand Up @@ -967,7 +967,7 @@ define([

function setView(uniformState, matrix) {
Matrix4.clone(matrix, uniformState._view);
Matrix4.getRotation(matrix, uniformState._viewRotation);
Matrix4.getMatrix3(matrix, uniformState._viewRotation);

uniformState._view3DDirty = true;
uniformState._inverseView3DDirty = true;
Expand All @@ -989,7 +989,7 @@ define([

function setInverseView(uniformState, matrix) {
Matrix4.clone(matrix, uniformState._inverseView);
Matrix4.getRotation(matrix, uniformState._inverseViewRotation);
Matrix4.getMatrix3(matrix, uniformState._inverseViewRotation);
}

function setProjection(uniformState, matrix) {
Expand Down Expand Up @@ -1294,7 +1294,8 @@ define([
uniformState._normalDirty = false;

var m = uniformState._normal;
Matrix4.getRotation(uniformState.inverseModelView, m);
Matrix4.getMatrix3(uniformState.inverseModelView, m);
Matrix3.getRotation(m, m);
lilleyse marked this conversation as resolved.
Show resolved Hide resolved
Matrix3.transpose(m, m);
}
}
Expand All @@ -1304,24 +1305,25 @@ define([
uniformState._normal3DDirty = false;

var m = uniformState._normal3D;
Matrix4.getRotation(uniformState.inverseModelView3D, m);
Matrix4.getMatrix3(uniformState.inverseModelView3D, m);
Matrix3.getRotation(m, m);
Matrix3.transpose(m, m);
}
}

function cleanInverseNormal(uniformState) {
if (uniformState._inverseNormalDirty) {
uniformState._inverseNormalDirty = false;

Matrix4.getRotation(uniformState.inverseModelView, uniformState._inverseNormal);
Matrix4.getMatrix3(uniformState.inverseModelView, uniformState._inverseNormal);
Matrix3.getRotation(uniformState._inverseNormal, uniformState._inverseNormal);
}
}

function cleanInverseNormal3D(uniformState) {
if (uniformState._inverseNormal3DDirty) {
uniformState._inverseNormal3DDirty = false;

Matrix4.getRotation(uniformState.inverseModelView3D, uniformState._inverseNormal3D);
Matrix4.getMatrix3(uniformState.inverseModelView3D, uniformState._inverseNormal3D);
Matrix3.getRotation(uniformState._inverseNormal3D, uniformState._inverseNormal3D);
}
}

Expand Down Expand Up @@ -1424,15 +1426,15 @@ define([
} else {
view2Dto3D(that._cameraPosition, that._cameraDirection, that._cameraRight, that._cameraUp, that._frustum2DWidth, that._mode, that._mapProjection, that._view3D);
}
Matrix4.getRotation(that._view3D, that._viewRotation3D);
Matrix4.getMatrix3(that._view3D, that._viewRotation3D);
that._view3DDirty = false;
}
}

function updateInverseView3D(that){
if (that._inverseView3DDirty) {
Matrix4.inverseTransformation(that.view3D, that._inverseView3D);
Matrix4.getRotation(that._inverseView3D, that._inverseViewRotation3D);
Matrix4.getMatrix3(that._inverseView3D, that._inverseViewRotation3D);
that._inverseView3DDirty = false;
}
}
Expand Down
19 changes: 15 additions & 4 deletions Source/Scene/Cesium3DTile.js
Original file line number Diff line number Diff line change
Expand Up @@ -140,12 +140,15 @@ define([
* @readonly
*/
this.geometricError = header.geometricError;
this._geometricError = header.geometricError;
lilleyse marked this conversation as resolved.
Show resolved Hide resolved

if (!defined(this.geometricError)) {
this.geometricError = defined(parent) ? parent.geometricError : tileset._geometricError;
if (!defined(this._geometricError)) {
this._geometricError = defined(parent) ? parent.geometricError : tileset._geometricError;
Cesium3DTile._deprecationWarning('geometricErrorUndefined', 'Required property geometricError is undefined for this tile. Using parent\'s geometric error instead.');
}

this.updateGeometricErrorScale();

var refine;
if (defined(header.refine)) {
if (header.refine === 'replace' || header.refine === 'add') {
Expand Down Expand Up @@ -990,7 +993,7 @@ define([

// Find the transformed center and halfAxes
center = Matrix4.multiplyByPoint(transform, center, center);
var rotationScale = Matrix4.getRotation(transform, scratchMatrix);
var rotationScale = Matrix4.getMatrix3(transform, scratchMatrix);
halfAxes = Matrix3.multiply(rotationScale, halfAxes, halfAxes);

if (defined(result)) {
Expand All @@ -1014,7 +1017,7 @@ define([
// This is why the transform is calculated as the difference between the initial transform and the current transform.
transform = Matrix4.multiplyTransformation(transform, Matrix4.inverseTransformation(initialTransform, scratchTransform), scratchTransform);
center = Matrix4.multiplyByPoint(transform, center, center);
var rotationScale = Matrix4.getRotation(transform, scratchMatrix);
var rotationScale = Matrix4.getMatrix3(transform, scratchMatrix);
halfAxes = Matrix3.multiply(rotationScale, halfAxes, halfAxes);

if (defined(result) && (result instanceof TileOrientedBoundingBox)) {
Expand Down Expand Up @@ -1114,12 +1117,20 @@ define([
this._viewerRequestVolume = this.createBoundingVolume(header.viewerRequestVolume, this.computedTransform, this._viewerRequestVolume);
}

this.updateGeometricErrorScale();

// Destroy the debug bounding volumes. They will be generated fresh.
this._debugBoundingVolume = this._debugBoundingVolume && this._debugBoundingVolume.destroy();
this._debugContentBoundingVolume = this._debugContentBoundingVolume && this._debugContentBoundingVolume.destroy();
this._debugViewerRequestVolume = this._debugViewerRequestVolume && this._debugViewerRequestVolume.destroy();
};

Cesium3DTile.prototype.updateGeometricErrorScale = function() {
var scale = Matrix4.getScale(this.computedTransform, scratchScale);
var uniformScale = Cartesian3.maximumComponent(scale);
this.geometricError = this._geometricError * uniformScale;
};

function applyDebugSettings(tile, tileset, frameState) {
if (!frameState.passes.render) {
return;
Expand Down
2 changes: 1 addition & 1 deletion Source/Scene/Instanced3DModel3DTileContent.js
Original file line number Diff line number Diff line change
Expand Up @@ -378,7 +378,7 @@ define([
hasCustomOrientation = true;
} else if (eastNorthUp) {
Transforms.eastNorthUpToFixedFrame(instancePosition, Ellipsoid.WGS84, instanceTransform);
Matrix4.getRotation(instanceTransform, instanceRotation);
Matrix4.getMatrix3(instanceTransform, instanceRotation);
} else {
Matrix3.clone(Matrix3.IDENTITY, instanceRotation);
}
Expand Down
4 changes: 2 additions & 2 deletions Source/Scene/Model.js
Original file line number Diff line number Diff line change
Expand Up @@ -2892,7 +2892,7 @@ define([
var mInverseTranspose = new Matrix3();
return function() {
Matrix4.inverse(runtimeNode.computedMatrix, mInverse);
Matrix4.getRotation(mInverse, mInverseTranspose);
Matrix4.getMatrix3(mInverse, mInverseTranspose);
return Matrix3.transpose(mInverseTranspose, mInverseTranspose);
};
},
Expand All @@ -2903,7 +2903,7 @@ define([
return function() {
Matrix4.multiplyTransformation(uniformState.view, runtimeNode.computedMatrix, mv);
Matrix4.inverse(mv, mvInverse);
Matrix4.getRotation(mvInverse, mvInverseTranspose);
Matrix4.getMatrix3(mvInverse, mvInverseTranspose);
return Matrix3.transpose(mvInverseTranspose, mvInverseTranspose);
};
},
Expand Down
35 changes: 35 additions & 0 deletions Specs/Core/Matrix3Spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -639,6 +639,41 @@ defineSuite([
expect(result).toEqual(expected);
});

it('getRotation returns matrix without scale', function() {
var matrix = new Matrix3(1.0, 2.0, 3.0, 4.0, 5.0, 6.0, 7.0, 8.0, 9.0);
var result = new Matrix3();
var expected = Matrix3.fromArray([
0.12309149097933272, 0.4923659639173309, 0.8616404368553291,
0.20739033894608505, 0.5184758473652127, 0.8295613557843402,
0.26726124191242440, 0.5345224838248488, 0.8017837257372732
]);
var scale = new Cartesian3();
var expectedScale = new Cartesian3(1.0, 1.0, 1.0);
result = Matrix3.getRotation(matrix, result);
console.log(result);
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove console.log(result)

var resultScale = Matrix3.getScale(result, scale);
expect(resultScale).toEqualEpsilon(expectedScale, CesiumMath.EPSILON14);
lilleyse marked this conversation as resolved.
Show resolved Hide resolved
expect(result).toEqualEpsilon(expected, CesiumMath.EPSILON14);
});

it('getRotation does not modify rotation matrix', function() {
var tmp = new Matrix3();
var result = new Matrix3();
var rotation = Matrix3.clone(Matrix3.IDENTITY, new Matrix3());
Matrix3.multiply(rotation, Matrix3.fromRotationX(1.0, tmp), rotation);
Matrix3.multiply(rotation, Matrix3.fromRotationY(2.0, tmp), rotation);
Matrix3.multiply(rotation, Matrix3.fromRotationZ(3.0, tmp), rotation);
result = Matrix3.getRotation(rotation, result);
expect(rotation).toEqualEpsilon(result, CesiumMath.EPSILON14);
expect(rotation).not.toBe(result);
});

it('getRotation throws without a matrix', function() {
expect(function() {
return Matrix3.getRotation();
}).toThrowDeveloperError();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Also add a separate check for getRotation throws without a result


it('transpose works with a result parameter that is an input result parameter', function() {
lilleyse marked this conversation as resolved.
Show resolved Hide resolved
var matrix = new Matrix3(1.0, 2.0, 3.0, 4.0, 5.0, 6.0, 7.0, 8.0, 9.0);
var expected = new Matrix3(1.0, 4.0, 7.0, 2.0, 5.0, 8.0, 3.0, 6.0, 9.0);
Expand Down