beginning of matrix class

beginning of matrix class


BUG=
TEST=

Committed: https://code.google.com/p/dart/source/detail?r=2360

[mattsh, 2011-12-08 04:19:03]

This doesn't have all the math operations we need yet, but has the basic
structure we need I think.

[sra1, 2011-12-08 06:07:07]

It seems that your matrix is motivated by geometric transformations (as opposed
to, e.g. finite element methods).

How is it related to WebKitCSSMatrix and SVGMatrix?

Should there be some common interfaces between your matrix and the DOM matrix
types?

[mattsh, 2011-12-08 06:21:48]

On 2011/12/08 06:07:07, sra1 wrote:
> It seems that your matrix is motivated by geometric transformations (as
opposed
> to, e.g. finite element methods).
> 
> How is it related to WebKitCSSMatrix and SVGMatrix?
> 
> Should there be some common interfaces between your matrix and the DOM matrix
> types?
Yes, this matrix is for representing 3d transforms.
There are operations we want to have on this class that WebKitCSSMatrix doesn't
have (for example, a method to create matrix for a perspective projection.
We may want to wrap WebKitCSSMatrix and use it as an optimization where
available, so in that case we'd store the elements in the WebKitCSSMatrix object
instead of the dart List object, but that shouldn't affect the interface to this
class.  I'd be curious to know what speedup factor that gives us.

[mattsh, 2011-12-08 19:40:35]

Actually, I think it's better to port the goog.vec.mat4 class here to dart
http://code.google.com/p/closure-library/source/browse/trunk/closure/goog/vec...

so I think I will close this CL.

[mattsh, 2011-12-12 18:18:01]

OK, I integrated code from goog/vec/mat4.js into this CL, and rewrote to use
operators.  (My feeling is we can add in-place multiply in the future easily,
but I wanted to start with the cleaner API that takes advantage of Dart language
features.)

[Jacob, 2011-12-12 18:38:15]

Looks good

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/float32.dart
File utils/matrix/float32.dart (right):

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/float32.dart#n...
utils/matrix/float32.dart:7: * the browser.
note that this is really a Float64Array

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/float32.dart#n...
utils/matrix/float32.dart:9: 
extra line break

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/matrix4.dart
File utils/matrix/matrix4.dart (right):

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/matrix4.dart#n...
utils/matrix/matrix4.dart:33: Vector3(double x, double y, double z) : x = x, y =
y, z = z {}
replace " {}" with ";"

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/matrix4.dart#n...
utils/matrix/matrix4.dart:35: double magnitude() {
switch short one liners to use => syntax

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/matrix4.dart#n...
utils/matrix/matrix4.dart:93: static int rc(int row, int col) => row + col * 4;
add a leading _?

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/matrix4.dart#n...
utils/matrix/matrix4.dart:156: Matrix4() : buf = new Float32Array(16) {}
move constructors above members and replace " {}" with ";" here and elsewhere

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/matrix4.dart#n...
utils/matrix/matrix4.dart:162: static Matrix4 identity() {
change statics for identity, rotation and transpose to factory constructors
e.g. new Matrix4.identity()

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/matrix4.dart#n...
utils/matrix/matrix4.dart:270: static Matrix4 lookAt(Vector3 eye, Vector3
center, Vector3 up) {
make factory constructor

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/matrix4.dart#n...
utils/matrix/matrix4.dart:305: static Matrix4 perspective(double fovyDegrees,
double aspectRatio,
factory constructor

[mattsh, 2011-12-12 19:14:41]

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/float32.dart
File utils/matrix/float32.dart (right):

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/float32.dart#n...
utils/matrix/float32.dart:7: * the browser.
On 2011/12/12 18:38:17, Jacob wrote:
> note that this is really a Float64Array

Done.

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/float32.dart#n...
utils/matrix/float32.dart:9: 
On 2011/12/12 18:38:17, Jacob wrote:
> extra line break

Done.

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/matrix4.dart
File utils/matrix/matrix4.dart (right):

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/matrix4.dart#n...
utils/matrix/matrix4.dart:33: Vector3(double x, double y, double z) : x = x, y =
y, z = z {}
On 2011/12/12 18:38:17, Jacob wrote:
> replace " {}" with ";"

Done.

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/matrix4.dart#n...
utils/matrix/matrix4.dart:35: double magnitude() {
On 2011/12/12 18:38:17, Jacob wrote:
> switch short one liners to use => syntax

Prefer non-arrow syntax here, since it is consistent with other methods.

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/matrix4.dart#n...
utils/matrix/matrix4.dart:93: static int rc(int row, int col) => row + col * 4;
On 2011/12/12 18:38:17, Jacob wrote:
> add a leading _? 

Prefer to keep it public.

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/matrix4.dart#n...
utils/matrix/matrix4.dart:156: Matrix4() : buf = new Float32Array(16) {}
On 2011/12/12 18:38:17, Jacob wrote:
> move constructors above members and replace " {}" with ";" here and elsewhere

Done.

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/matrix4.dart#n...
utils/matrix/matrix4.dart:162: static Matrix4 identity() {
On 2011/12/12 18:38:17, Jacob wrote:
> change statics for identity, rotation and transpose to factory constructors
> e.g. new Matrix4.identity()
Actually, prefer these to be static methods, not factory constructors, because
it makes the call-site cleaner.

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/matrix4.dart#n...
utils/matrix/matrix4.dart:270: static Matrix4 lookAt(Vector3 eye, Vector3
center, Vector3 up) {
On 2011/12/12 18:38:17, Jacob wrote:
> make factory constructor

see above

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/matrix4.dart#n...
utils/matrix/matrix4.dart:305: static Matrix4 perspective(double fovyDegrees,
double aspectRatio,
On 2011/12/12 18:38:17, Jacob wrote:
> factory constructor

see above

[Jacob, 2011-12-12 20:56:51]

lgtm

[Bob Nystrom, 2011-12-12 21:46:09]

Some random comments but otherwise LGTM

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/float32.dart
File utils/matrix/float32.dart (right):

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/float32.dart#n...
utils/matrix/float32.dart:24: buf[i] = (value != null) ? value.toDouble() : 0.0;
Why are you checking for null here?

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/matrix4.dart
File utils/matrix/matrix4.dart (right):

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/matrix4.dart#n...
utils/matrix/matrix4.dart:32: // bug
http://code.google.com/p/dart/issues/detail?id=777
According to the bug, this is fixed now.

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/matrix4.dart#n...
utils/matrix/matrix4.dart:33: Vector3(double x, double y, double z) : x = x, y =
y, z = z {}
Why not just use this.x, this.y, this.z?

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/matrix4.dart#n...
utils/matrix/matrix4.dart:35: double magnitude() {
On 2011/12/12 19:14:45, mattsh wrote:
> On 2011/12/12 18:38:17, Jacob wrote:
> > switch short one liners to use => syntax
> 
> Prefer non-arrow syntax here, since it is consistent with other methods.

Personally, I'd prefer arrow here too (and elsewhere). Strict consistency with
other methods would probably rule out all arrows for methods since you'll likely
have at least one that can't use it.

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/matrix4.dart#n...
utils/matrix/matrix4.dart:35: double magnitude() {
Should this be a getter? Also, I'd add magnitudeSquared.

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/matrix4.dart#n...
utils/matrix/matrix4.dart:48: return new Vector3(-x,-y,-z);
Space after comma.

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/matrix4.dart#n...
utils/matrix/matrix4.dart:52: return new Vector3(this.x - other.x, this.y -
other.y, this.z - other.z);
Get rid of "this.".

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/matrix4.dart#n...
utils/matrix/matrix4.dart:56: double xResult = y * other.z - z * other.y;
"final" instead of "double".

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/matrix4.dart#n...
utils/matrix/matrix4.dart:72: *   m00 m01 m02 m03
Indent these two more and dartdoc will recognize it as preformatted.

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/matrix4.dart#n...
utils/matrix/matrix4.dart:77: * These are stored in a 16 element Float32Array,
in column major
Put Float32Array in square brackets to cross-link.

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/matrix4.dart#n...
utils/matrix/matrix4.dart:87: final Float32Array buf;
"buffer" or "elements"?

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/matrix4.dart#n...
utils/matrix/matrix4.dart:93: static int rc(int row, int col) => row + col * 4;
On 2011/12/12 19:14:45, mattsh wrote:
> On 2011/12/12 18:38:17, Jacob wrote:
> > add a leading _? 
> 
> Prefer to keep it public.

If you want this to be public, I would give it a meaningful name.

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/matrix4.dart#n...
utils/matrix/matrix4.dart:338: // compute determinant
"Compute determinant."

[mattsh, 2011-12-12 22:48:26]

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/float32.dart
File utils/matrix/float32.dart (right):

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/float32.dart#n...
utils/matrix/float32.dart:24: buf[i] = (value != null) ? value.toDouble() : 0.0;
On 2011/12/12 21:46:09, Bob Nystrom wrote:
> Why are you checking for null here?
Might not be necessary, but it's emulating the Float32Array behavior where
null's automatically convert to zero.  Probably I should add a comment.

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/matrix4.dart
File utils/matrix/matrix4.dart (right):

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/matrix4.dart#n...
utils/matrix/matrix4.dart:33: Vector3(double x, double y, double z) : x = x, y =
y, z = z {}
On 2011/12/12 21:46:09, Bob Nystrom wrote:
> Why not just use this.x, this.y, this.z?

Currently the this.x style syntax does not lift the type into the constructor
signature.  I added a TODO and reference to bug
http://code.google.com/p/dart/issues/detail?id=464

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/matrix4.dart#n...
utils/matrix/matrix4.dart:35: double magnitude() {
On 2011/12/12 21:46:09, Bob Nystrom wrote:
> On 2011/12/12 19:14:45, mattsh wrote:
> > On 2011/12/12 18:38:17, Jacob wrote:
> > > switch short one liners to use => syntax
> > 
> > Prefer non-arrow syntax here, since it is consistent with other methods.
> 
> Personally, I'd prefer arrow here too (and elsewhere). Strict consistency with
> other methods would probably rule out all arrows for methods since you'll
likely
> have at least one that can't use it.

OK, switch to arrow syntax.

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/matrix4.dart#n...
utils/matrix/matrix4.dart:35: double magnitude() {
On 2011/12/12 21:46:09, Bob Nystrom wrote:
> Should this be a getter? Also, I'd add magnitudeSquared.

I think it's better to keep it a method, to make it more obvious there's a
calculation involved that might not be inlined.

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/matrix4.dart#n...
utils/matrix/matrix4.dart:48: return new Vector3(-x,-y,-z);
On 2011/12/12 21:46:09, Bob Nystrom wrote:
> Space after comma.

Done.

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/matrix4.dart#n...
utils/matrix/matrix4.dart:52: return new Vector3(this.x - other.x, this.y -
other.y, this.z - other.z);
On 2011/12/12 21:46:09, Bob Nystrom wrote:
> Get rid of "this.".

Done.

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/matrix4.dart#n...
utils/matrix/matrix4.dart:56: double xResult = y * other.z - z * other.y;
On 2011/12/12 21:46:09, Bob Nystrom wrote:
> "final" instead of "double".
I think I prefer writing the type here.

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/matrix4.dart#n...
utils/matrix/matrix4.dart:72: *   m00 m01 m02 m03
On 2011/12/12 21:46:09, Bob Nystrom wrote:
> Indent these two more and dartdoc will recognize it as preformatted.

Done.

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/matrix4.dart#n...
utils/matrix/matrix4.dart:77: * These are stored in a 16 element Float32Array,
in column major
On 2011/12/12 21:46:09, Bob Nystrom wrote:
> Put Float32Array in square brackets to cross-link.

Done.

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/matrix4.dart#n...
utils/matrix/matrix4.dart:87: final Float32Array buf;
On 2011/12/12 21:46:09, Bob Nystrom wrote:
> "buffer" or "elements"?
Tried elements, but actually it just doesn't look at nice considering how
frequently this is used.  Seems too long.

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/matrix4.dart#n...
utils/matrix/matrix4.dart:93: static int rc(int row, int col) => row + col * 4;
On 2011/12/12 21:46:09, Bob Nystrom wrote:
> On 2011/12/12 19:14:45, mattsh wrote:
> > On 2011/12/12 18:38:17, Jacob wrote:
> > > add a leading _? 
> > 
> > Prefer to keep it public.
> 
> If you want this to be public, I would give it a meaningful name.
> 

Done.

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/matrix4.dart#n...
utils/matrix/matrix4.dart:338: // compute determinant
On 2011/12/12 21:46:09, Bob Nystrom wrote:
> "Compute determinant."

Done.

[Jacob, 2011-12-12 22:59:13]

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/matrix4.dart
File utils/matrix/matrix4.dart (right):

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/matrix4.dart#n...
utils/matrix/matrix4.dart:27: final double x;
One more high level comment.  Is there a good reason why these should be double
instead of num?

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/matrix4.dart#n...
utils/matrix/matrix4.dart:33: Vector3(double x, double y, double z) : x = x, y =
y, z = z {}
is there a good reason why these are double instead of num?

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/matrix4.dart#n...
utils/matrix/matrix4.dart:63: return "Vector3($x,$y,$z)";
one more style nit. why not "($x, $y, $z)" instead of "Vector3($x,$y,$z)"

I think 
"Location is $myVector, velocity is $myVelocityVector"
prints a more human readable result if
it prints
"Location is (44,2,1), velocity is (1,2,3)"
instead of
"Location is Vector3(44,2,1), velocity is Vector(1,2,3)"

If you want the type name you should be calling some not yet available
introspection method rather than toString.

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/matrix4.dart#n...
utils/matrix/matrix4.dart:162: static Matrix4 identity() {
On 2011/12/12 19:14:45, mattsh wrote:
> On 2011/12/12 18:38:17, Jacob wrote:
> > change statics for identity, rotation and transpose to factory constructors
> > e.g. new Matrix4.identity()
> Actually, prefer these to be static methods, not factory constructors, because
> it makes the call-site cleaner.

One advantage of making the factory constructors is that you could later only
expose the interface  while with a static method you are stuck (i believe)

[mattsh, 2011-12-12 23:06:35]

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/matrix4.dart
File utils/matrix/matrix4.dart (right):

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/matrix4.dart#n...
utils/matrix/matrix4.dart:27: final double x;
On 2011/12/12 22:59:14, Jacob wrote:
> One more high level comment.  Is there a good reason why these should be
double
> instead of num?

I think for consistency and interoperability with Matrix4 they need to be
double.  (Otherwise all users of this class would need to carefully insert
toDouble() whenever they accessed one of the values.)

http://codereview.chromium.org/8872007/diff/15001/utils/matrix/matrix4.dart#n...
utils/matrix/matrix4.dart:33: Vector3(double x, double y, double z) : x = x, y =
y, z = z {}
On 2011/12/12 22:59:14, Jacob wrote:
> is there a good reason why these are double instead of num?

See above