scale geometric error with tileset #7411

Vineg · 2018-12-13T16:29:26Z

#5330

removed geometricError recalculation on each "get"

cesium-concierge · 2018-12-13T16:29:28Z

Thanks for the pull request @Vineg!

✔️ Signed CLA found.
❔ CHANGES.md was not updated.
- If this change updates the public API in any way, please add a bullet point to CHANGES.md.

Reviewers, don't forget to make sure that:

Cesium Viewer works.
Works in 2D/CV.
Works (or fails gracefully) in IE11.

I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

hpinkos · 2018-12-13T16:32:11Z

Thanks @Vineg! @lilleyse will review this soon

Vineg · 2018-12-28T10:41:53Z

@lilleyse this is ready for review

Source/Core/Matrix3.js

hpinkos · 2018-12-28T14:53:40Z

Can you add some unit tests for the new matrix function?

hpinkos · 2018-12-28T14:54:39Z

Those are all my comments. @lilleyse will do the final review. Thanks @Vineg!

Vineg · 2018-12-28T16:49:47Z

@hpinkos @lilleyse done

cesium-concierge · 2019-01-28T03:00:15Z

Thanks again for your contribution @Vineg!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

hpinkos · 2019-01-28T15:23:10Z

@lilleyse can you review this?

lilleyse · 2019-01-28T15:31:56Z

Yes, sorry for the delay. I plan to review this soon.

lilleyse

Thank @Vineg, just some small comments from me.

I opened CesiumGS/3d-tiles#367 to keep track of this change in the spec.

Source/Core/Matrix3.js

Source/Renderer/UniformState.js

Source/Scene/Cesium3DTile.js

Specs/Core/Matrix3Spec.js

hpinkos · 2019-02-21T16:18:08Z

@Vineg @geoscan-builder bump =) let us know when these comments are addressed and this is ready for another look. Thanks!

Vineg · 2019-02-21T16:26:38Z

Yeah, I hope to get back to this soon

cesium-concierge · 2019-03-24T02:00:11Z

Thanks again for your contribution @Vineg!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

cesium-concierge · 2019-04-23T02:00:21Z

Thanks again for your contribution @Vineg!

No one has commented on this pull request in 30 days. Maintainers, can you review, merge or close to keep things tidy?

I'm going to re-bump this in 30 days. If you'd like me to stop, just comment with @cesium-concierge stop. If you want me to start again, just delete the comment.

Vineg · 2019-04-23T09:10:46Z

@cesium-concierge stop

Vineg · 2019-05-13T18:07:32Z

@lilleyse applied proposed changes and moved getRotation to getMatrix3 as discussed in #7398

hpinkos · 2019-09-18T13:51:43Z

@lilleyse what's the status of this PR?

lilleyse · 2019-09-18T15:30:03Z

@hpinkos Waiting on my review which I'll finish up now.

lilleyse

Thansk @Vineg, this is really close.

lilleyse · 2019-09-18T15:34:55Z

Source/Core/Matrix4.js

+    Matrix4.getRotation = function(matrix, result) {
+        return Matrix4.getMatrix3(matrix, result);
+    };
+
    /**
     * Gets the upper left 3x3 rotation matrix of the provided matrix, assuming the matrix is a affine transformation matrix.


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.

lilleyse · 2019-09-18T15:35:55Z

Source/Core/Matrix3.js

+     *
+     * @param {Matrix3} matrix The matrix.
+     * @param {Matrix3} result The object onto which to store the result.
+     * @returns {Cartesian3} The modified result parameter


Suggested change

* @returns {Cartesian3} The modified result parameter

* @returns {Matrix3} The modified result parameter

lilleyse · 2019-09-18T15:41:15Z

Source/Core/Matrix4.js

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


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); };

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)

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).

lilleyse · 2019-09-18T15:54:10Z

Specs/Core/Matrix3Spec.js

+        var scale = new Cartesian3();
+        var expectedScale = new Cartesian3(1.0, 1.0, 1.0);
+        result = Matrix3.getRotation(matrix, result);
+        console.log(result);


Remove console.log(result)

lilleyse · 2019-09-18T16:14:47Z

Specs/Core/Matrix3Spec.js

+        expect(function() {
+            return Matrix3.getRotation();
+        }).toThrowDeveloperError();
+    });


Also add a separate check for getRotation throws without a result

lilleyse · 2019-09-18T16:16:41Z

Note to self after merging: this PR fixes both #5330 and #7398

And update the spec: CesiumGS/3d-tiles#367

hpinkos · 2019-09-19T14:15:00Z

I'm going to merge this into a branch and take over the rest of the clean up. Thanks for all the work you did for this @Vineg!

geoscan-builder added 4 commits December 7, 2018 19:40

scale geometric error alongside tileset

3632059

refs CesiumGS#5152 option to switch normal shading

4a8c92d

refs CesiumGS#7398 moved getRotation method to Matrix3,

ca38e70

removed geometricError recalculation on each "get"

refs CesiumGS#5152 revert commit

fd5f6a6

hpinkos reviewed Dec 28, 2018

View reviewed changes

Source/Core/Matrix3.js Outdated Show resolved Hide resolved

geoscan-builder added 2 commits December 28, 2018 19:19

Merge remote-tracking branch 'cesium/master' into geometric-error-scale

899f799

added unit tests for getRotation function, little cleanup

905d783

lilleyse mentioned this pull request Jan 31, 2019

Allow tileset transform to scale geometric error CesiumGS/3d-tiles#367

Closed

lilleyse reviewed Jan 31, 2019

View reviewed changes

geoscan-builder added 4 commits May 13, 2019 20:01

added rotation matrix specs

ebb058f

Merge branch 'cesium-master' into geometric-error-scale

ed7bd07

refs CesiumGS#7398: moved Matrix4.getRotation to Matrix4.getMatrix3

698cc16

added missing semicolon

36e569a

lilleyse reviewed Sep 18, 2019

View reviewed changes

hpinkos changed the base branch from master to scale-geometric-error September 19, 2019 14:14

hpinkos merged commit 2ea3899 into CesiumGS:scale-geometric-error Sep 19, 2019

hpinkos mentioned this pull request Sep 19, 2019

Scale geometric error with tileset #8182

Merged

lilleyse mentioned this pull request Mar 4, 2020

Refine: Replace #8660

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

scale geometric error with tileset #7411

scale geometric error with tileset #7411

Vineg commented Dec 13, 2018

cesium-concierge commented Dec 13, 2018

hpinkos commented Dec 13, 2018

Vineg commented Dec 28, 2018

hpinkos commented Dec 28, 2018

hpinkos commented Dec 28, 2018

Vineg commented Dec 28, 2018

cesium-concierge commented Jan 28, 2019

hpinkos commented Jan 28, 2019

lilleyse commented Jan 28, 2019

lilleyse left a comment •

edited

Loading

hpinkos commented Feb 21, 2019

Vineg commented Feb 21, 2019

cesium-concierge commented Mar 24, 2019

cesium-concierge commented Apr 23, 2019

Vineg commented Apr 23, 2019

Vineg commented May 13, 2019

hpinkos commented Sep 18, 2019

lilleyse commented Sep 18, 2019

lilleyse left a comment

lilleyse Sep 18, 2019

lilleyse Sep 18, 2019

lilleyse Sep 18, 2019

lilleyse Sep 18, 2019

lilleyse Sep 18, 2019

lilleyse Sep 18, 2019

lilleyse Sep 18, 2019

lilleyse commented Sep 18, 2019 •

edited

Loading

hpinkos commented Sep 19, 2019

	* 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.

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

scale geometric error with tileset #7411

scale geometric error with tileset #7411

Conversation

Vineg commented Dec 13, 2018

cesium-concierge commented Dec 13, 2018

hpinkos commented Dec 13, 2018

Vineg commented Dec 28, 2018

hpinkos commented Dec 28, 2018

hpinkos commented Dec 28, 2018

Vineg commented Dec 28, 2018

cesium-concierge commented Jan 28, 2019

hpinkos commented Jan 28, 2019

lilleyse commented Jan 28, 2019

lilleyse left a comment • edited Loading

Choose a reason for hiding this comment

hpinkos commented Feb 21, 2019

Vineg commented Feb 21, 2019

cesium-concierge commented Mar 24, 2019

cesium-concierge commented Apr 23, 2019

Vineg commented Apr 23, 2019

Vineg commented May 13, 2019

hpinkos commented Sep 18, 2019

lilleyse commented Sep 18, 2019

lilleyse left a comment

Choose a reason for hiding this comment

lilleyse Sep 18, 2019

Choose a reason for hiding this comment

lilleyse Sep 18, 2019

Choose a reason for hiding this comment

lilleyse Sep 18, 2019

Choose a reason for hiding this comment

lilleyse Sep 18, 2019

Choose a reason for hiding this comment

lilleyse Sep 18, 2019

Choose a reason for hiding this comment

lilleyse Sep 18, 2019

Choose a reason for hiding this comment

lilleyse Sep 18, 2019

Choose a reason for hiding this comment

lilleyse commented Sep 18, 2019 • edited Loading

hpinkos commented Sep 19, 2019

lilleyse left a comment •

edited

Loading

lilleyse commented Sep 18, 2019 •

edited

Loading