Skip to content

Conversation

@jcxue
Copy link

@jcxue jcxue commented Apr 30, 2018

add a function to convert quaternion to Euler angles.

Copy link
Member

@pwaller pwaller left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your contribution.

This seems OK to me - but I don't have time to go through and verify that the code is correct in all the cases.

The best I can think of is that you should add a round-trip test for every RotationOrder. If you do that, I can't see a reason not to merge it.

a_x := float32(math.Pi / 16.0)
a_y := float32(math.Pi / 8.0)
a_z := float32(math.Pi / 4.0)
a1, a2, a3 := QuatToAngles(AnglesToQuat(a_x, a_y, a_z, XYZ), XYZ)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could we add a test which checks all of the possible RotationOrder?

@@ -1,4 +1,4 @@
// This file is generated from mgl32/matstack\matstack.go; DO NOT EDIT
// This file is generated from mgl32/matstack/matstack.go; DO NOT EDIT
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please can you revert this change. codegen.go obviously needs a bugfix.

@pwaller
Copy link
Member

pwaller commented Apr 11, 2019

I tried extending the test in the obvious way (testing XYX, etc) but I can't make sense of the results and I've run out of time for now. In that case X and Z end up flipped on the output and I'm not sure whether this makes sense or not.

This might be an interesting function to test by round-tripping lots of random numbers through it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants