Finish Matrix4 implementation. #129

mramato · 2012-07-24T21:55:58Z

proper error checking.
static versions of all prototype functions.
Matrices are now indexed like Arrays and you can perform all matrix operations using array types.
functions that provide an optional result parameter for object re-use.
100% unit test code coverage
Complete documentation.
Optimized the hell out of internal Matrix4 computations. Code using Matrix4 still need to be optimized to re-use objects, but Matrix4 itself is much faster now. Informal tests showed just the Skeleton going from 200fps to 215fps in Chrome and Firefox went from 17ms per frame to 16ms per frame.
Updated CHANGES.

These changes are related to issues #71, #84, #86 (those issues will remain open until we address them for every Core type).

There are still some regressions that need to be fixed but it now matches Matrix2 and Matrix3 API-wise.

Replace Jobs.downloadImage and getImageFromUrl with promise-based loadImage. Change jsonp to work in a promise-based way. Fix some other random documentation problems I noticed along the way.

1. All toEqual calls in specs were actually doing toString result comparisons, which was cuastom some tests to pass even though they failed. I added a toEqual override in addDefaultMatchers and fixed offending tests. 2. Fix inverseTransformation and add more tests to Matrix4.

1. Coverage up to 100% 2. Fix missing semicolons in loadImage.js 3. There's still a bug in 2D/Columbus mode that I need to track down.

Conflicts: Source/Core/loadImage.js

Also add missing error check in Cartesian3.

1. Flesh out documentation. 2. Rename some Matrix4.fromX functions to Matrix4.computeX so they make sense. 3. Optimize internal Matrix4 computations to avoid needless array indexing and checks.

pjcozzi · 2012-07-25T14:06:39Z

Specs/Core/Matrix4Spec.js

+    });
+
+    it('createPerspectiveFieldOfView works without a result parameter', function() {
+        var expected = new Matrix4(1, 0,                   0,                  0,


Spacing is a bit off here and below if you care.

pjcozzi · 2012-07-25T14:25:16Z

I can't figure out to expand the diff for changes to Matrix4.js, but here are a couple comments:

Matrix4.getTranslation needs doc, right?
We don't have to do it as part of this pull request, but I'd like us to soon look at Matrix4.toArray performance for setting uniforms since allocating there was a problem in the past. This can be part of the larger investigation into allocations.

mramato · 2012-07-25T14:39:34Z

Good catch on getTranslation. Looks like there are a couple other methods that can use some doc cleanup as well. I'll make those changes ASAP.

As for toArray, I definitely plan on looking into it. To ease your conscience in the mean time, I'll add that I've eliminated so many unnecessary temporary Matrix instances, that things are still faster even with the toArray call. I want to look into using cached TypeArrays in places like this as well to see how it affects performance and memory pressure. I definitely plan on doing this all in a separate effort once all of the Core cleanup I'm working on it finished. I'm optimistic that there is a ton of room for performance improvement in our code base.

pjcozzi · 2012-07-25T15:02:49Z

To ease your conscience in the mean time, I'll add that I've eliminated so many unnecessary temporary Matrix instances, that things are still faster even with the toArray call.

This is great and those optimizations are long overdue. However, there are several types of matrices like the projection matrix, east-north-up-to-fixed, and matrices in a transform hierarchy for a model, that will not change frame-to-frame, which we didn't help in this case. Anyway, I know you're all over it...

mramato · 2012-07-25T15:05:48Z

Okay, documentation changes are in. I found a few prototype functions that were missing doc and there were stray param tags missing in some places. Everything has correct doc now.

pjcozzi · 2012-07-25T15:11:42Z

@shunter review and merge this when you can, please. It's OK with me.

shunter · 2012-07-25T17:12:24Z

CHANGES.md

   * All functions starting with `multiplyWith` now start with `multiplyBy` to be consistent with functions starting with `divideBy`. 
+   * The `multiplyWithMatrix` function on each `Matrix` type was renamed to `multiply`.
+   * All three Matrix classes have been largely re-written for consistency and performance.  The `values` property has been eliminated and Matrices are no longer immutable.  Code that previously looked like `matrix = matrix.setColumn0Row0(12);` now looks like `matrix[Matrix2.COLUMN0ROW0] = 12;`.  Code that previously looked like `matrix.setColumn3(cartesian3);` now looked like `matrix.setColum(3, cartesian3, matrix)`. 


typo at the end: setColum

Thanks, fixed.

shunter · 2012-07-25T17:29:30Z

Specs/addDefaultMatchers.js


-        return a === b;
+        if (a !== null && b !== null && typeof a !== 'undefined' && typeof b !== 'undefined') {
+            return a.toString() === b.toString();


Is this a good idea? I wouldn't expect a.toEqual(b) to potentially toString them and compare. Any object without a toString override will compare equal to any other object.

Me and @shunter discussed the reason this code was changed and he ended up making further changes (see 41c8e7f) which improves Jasmine testing overall and ensure our equals methods are always called when available.

… toEqual.

mramato · 2012-07-25T18:25:06Z

okay @shunter I think we're good to go.

Finish Matrix4 implementation.

updated STK Cesium examples to use fromDegrees

anne-gropler · 2016-05-18T09:37:27Z

Hello,
I think the implementation of Matrix4.fromCamera is deprecated, since the camera has no .eye and no .target property, but .direction and .up and others.

pjcozzi · 2016-05-18T11:50:05Z

Thanks for the report, @anne-gropler. I submitted #3927.

mramato and others added 16 commits July 23, 2012 09:30

Initial Matrix4 refactoring.

6724408

There are still some regressions that need to be fixed but it now matches Matrix2 and Matrix3 API-wise.

Ongoing Matrix4 work.

bc09efc

Merge remote-tracking branch 'origin/master' into matrix4

fe3bb5c

Add when promise library and switch some APIs to use them.

4b6aff8

Replace Jobs.downloadImage and getImageFromUrl with promise-based loadImage. Change jsonp to work in a promise-based way. Fix some other random documentation problems I noticed along the way.

CHANGES.md tweak.

75beff0

Ongoing Matrix4 work.

5bff297

1. Coverage up to 100% 2. Fix missing semicolons in loadImage.js 3. There's still a bug in 2D/Columbus mode that I need to track down.

Minor optimization.

41483ab

Merge remote-tracking branch 'origin/master' into matrix4

230e872

Conflicts: Source/Core/loadImage.js

Merge branch 'master' into matrix4

395a566

Fix and optimize Matrix4.inverseTransformation

d3eff78

Also add missing error check in Cartesian3.

One line fix for issue #127 which I introduced with c1e68f1

d4ad9f2

Remove test for an internal function.

f646be5

Final Matrix4 changes

3b0d1ec

1. Flesh out documentation. 2. Rename some Matrix4.fromX functions to Matrix4.computeX so they make sense. 3. Optimize internal Matrix4 computations to avoid needless array indexing and checks.

Update CHANGES.

e845ca4

Add missing test to bring Matrix4 coverage up to 100%

0928cc9

ghost assigned shunter Jul 24, 2012

mramato added 2 commits July 24, 2012 17:58

Merge remote-tracking branch 'origin/master' into matrix4

049ad93

Optimize Matrix4.multiply my extracting locals.

b735bf0

pjcozzi reviewed Jul 25, 2012
View reviewed changes

Add missing Matrix4 documentation.

a921dd0

shunter reviewed Jul 25, 2012
View reviewed changes

Fix typo.

2abce61

shunter reviewed Jul 25, 2012
View reviewed changes

shunter and others added 2 commits July 25, 2012 14:11

Use a custom Jasmine equality tester instead of replacing the default…

41c8e7f

… toEqual.

Hopefully final Matrix4 changes after review.

8308afc

shunter added a commit that referenced this pull request Jul 25, 2012

Merge pull request #129 from AnalyticalGraphicsInc/matrix4

4c48259

Finish Matrix4 implementation.

shunter merged commit 4c48259 into master Jul 25, 2012

pjcozzi added a commit that referenced this pull request Oct 22, 2015

Merge pull request #129 from AnalyticalGraphicsInc/fromDegrees

07e10c6

updated STK Cesium examples to use fromDegrees

pjcozzi mentioned this pull request May 18, 2016

Matrix4.fromCamera should use position/direction... #3927

Closed

AnalyticalGraphics unassigned shunter Feb 18, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Finish Matrix4 implementation. #129

Finish Matrix4 implementation. #129

mramato commented Jul 24, 2012

pjcozzi Jul 25, 2012

pjcozzi commented Jul 25, 2012

mramato commented Jul 25, 2012

pjcozzi commented Jul 25, 2012

mramato commented Jul 25, 2012

pjcozzi commented Jul 25, 2012

shunter Jul 25, 2012

mramato Jul 25, 2012

shunter Jul 25, 2012

mramato Jul 25, 2012

mramato commented Jul 25, 2012

anne-gropler commented May 18, 2016

pjcozzi commented May 18, 2016

Finish Matrix4 implementation. #129

Finish Matrix4 implementation. #129

Conversation

mramato commented Jul 24, 2012

pjcozzi Jul 25, 2012

Choose a reason for hiding this comment

pjcozzi commented Jul 25, 2012

mramato commented Jul 25, 2012

pjcozzi commented Jul 25, 2012

mramato commented Jul 25, 2012

pjcozzi commented Jul 25, 2012

shunter Jul 25, 2012

Choose a reason for hiding this comment

mramato Jul 25, 2012

Choose a reason for hiding this comment

shunter Jul 25, 2012

Choose a reason for hiding this comment

mramato Jul 25, 2012

Choose a reason for hiding this comment

mramato commented Jul 25, 2012

anne-gropler commented May 18, 2016

pjcozzi commented May 18, 2016