crertel / graphmath

An Elixir library for performing 2D and 3D mathematics.
The Unlicense
79 stars 14 forks source link

Minor correction to Vec2.rotate/2 #9

Closed matthewphilyaw closed 8 years ago

matthewphilyaw commented 8 years ago

Used https://en.wikipedia.org/wiki/Rotation_matrix as my reference and believe that the equation for CCW rotation is (x_ct - y_st, x_st + y_ct) and corrected accordingly.

I noticed the issue when rotating (0, 1) by PI the previous implementation would produce (0, 1) as the new vec, which is obviously not correct.

I added six test cases for the y axis to confirm the following

rotate({0,1}, :math.pi) = {0,-1} rotate({0,-1}, :math.pi) = {0,1} rotate({0,1}, :math.pi/2) = {-1,0} rotate({0,-1}, :math.pi/2) = {1,0} rotate({0,1}, -:math.pi/2) = {1,0} rotate({0,-1}, -:math.pi/2) = {-1,0}

crertel commented 8 years ago

Just saw this--am very sorry for the delay!

I'll be bringing this in and fixing the bug. :)

Would you like to be acknowledged in the contributors list?

matthewphilyaw commented 8 years ago

No worries and sure just happy to help. :)

I'd review my math just to make sure but I believe those changes are correct.

crertel commented 8 years ago

Great!

Would you mind updating your master from mine so that the CI server isn't busted?

I recently (read: tonight) fixed the config, but that's why your PR is currently failing.

matthewphilyaw commented 8 years ago

@crertel I rebased over master just a heads up if you have the branch locally.

I noticed the CI issue, meant to mention that as well just slipped my mind.

crertel commented 8 years ago

Fantastic!

Thank you--the math looks correct, and the tests pass.

I'm kinda kicking myself for not testing all quadrants on my first pass, but this makes life a lot easier. I'll be cutting a new release soonest. Thanks again!

matthewphilyaw commented 8 years ago

Awesome, thanks!

Glad to help.