dmah42 / dart-box2d

Box2D for Dart
http://dart-lang.github.io/dart-box2d/
34 stars 5 forks source link

use vector_math library instead of custom vector implementation. #17

Closed laszlokorte closed 11 years ago

laszlokorte commented 11 years ago

As there is already a vector_math library it would be neat to reuse it instead of reimplementing the vector and matrix classes.

I added vector_math as dependency and removed the old Vector2, Matrix22 and Matrix33 classes as well as the MathBox.radToDegree and degreeToRad methods (they are also available in vector_math).

Some methods on Vector2 and Matrix22/33 are not included in the vector_math lib. I have changed them to be global functions prefixed with the classname of their previous classes:

void Vector2_crossVectorAndNumToOut(Vector2 a, double s, Vector2 out) - already in vector_math master as Vector2#scaleOrthogonalInto(double scale, Vector2 out) void Vector2_minToOut(Vector2 a, Vector2 b, Vector2 out) void Vector2_maxToOut(Vector2 a, Vector2 b, Vector2 out) void Matrix2_solveToOut(Matrix2 a, Vector2 b, Vector2 out) void Matrix3_solve22ToOut(Matrix3 a, Vector2 b, Vector2 out) void Matrix3_solve33ToOut(Matrix3 a, Vector3 b, Vector3 out)

laszlokorte commented 11 years ago

The vector_math library has no Aabb2 class, only Aabb3 and even this is missing most of the methods box2d uses (contains(otherBox) and center for example)

I am not sure about the performance impact of the extra dimension in Aabb3.

johnmccutchan commented 11 years ago

I would accept a pull request with an Aabb2 class for vector_math.

dmah42 commented 11 years ago

Firstly, there's conflicts here.

Secondly, this has been tried before (see issue #13) with stability issues in the time of impact solver. Do all demos run correctly in dartium and JS? Can you post before and after benchmark output?

laszlokorte commented 11 years ago

The conflict is just because of the num->double change. I will rebase. All demos are working correctly. I will post the benchmarks.

laszlokorte commented 11 years ago

Benchmark: https://gist.github.com/laszlokorte/5950805

With vector_math it seems to be slower. I am not sure if it is due to a difference inside the vector/matrix classes or if I have just exchanged some fast operations by slower ones by not using the corrects methods.

johnmccutchan commented 11 years ago

Some of the tests appear to be faster with vector_math. All told it's a mixed bag. I'm really curious specifically where vector_math is slower, should be easy to fix.

laszlokorte commented 11 years ago

Oh yeah I did not notice. I have absolutely no idea how to profile dart code.

dmah42 commented 11 years ago

https://code.google.com/p/dart/wiki/Profiling

I'd be interested to see what comes out of this, specifically the difference between vector_math and non-vector_math rather than just general slow code paths.

Eyeballing the benchmarks, I think that the performance is better with fewer solve loops and worse with more. I'm not sure what to make of that.

On Mon, Jul 8, 2013 at 11:16 AM, Laszlo Korte notifications@github.comwrote:

Oh yeah I did not notice. I have absolutely no idea how to profile dart code.

— Reply to this email directly or view it on GitHubhttps://github.com/dart-lang/dart-box2d/pull/17#issuecomment-20625080 .

johnmccutchan commented 11 years ago

Given the time delta's we are talking about the profiler probably won't help.

Laszlo, did any of your changes introduce memory allocations?

laszlokorte commented 11 years ago

I think I found the difference. I did not tested it yet because I have other stuff todo but I just reread the changeset and the (only) thing I noticed is the change from matrix.col1 to matrix.getColumn(0) matrix.col2 to matrix.getColumn(1)

col1 was a direct property access before because the old matrix2 stored it's values in in two vector2s now its a copy from the matrix' storage into a new vector.

I will have a closer look at it later.

laszlokorte commented 11 years ago

Better now: https://gist.github.com/anonymous/5956141

laszlokorte commented 11 years ago

I noticed there is a Transform class with a rotation Matrix2 and a translation Vector2. Should we replace it with just a Matrix3? The code for Transform.mulTransToOut() is already in vector_math Matrix3.

dmah42 commented 11 years ago

I'm going to pull this locally to test it before merging, but it looks good to me. When it lands, we can talk about cleaning up the code (replacing Transform, AABB, etc).

johnmccutchan commented 11 years ago

sgtm.

laszlokorte commented 11 years ago

ok

johnmccutchan commented 11 years ago

Lazlo, I've created three issues in vector_math to address the final bits.