New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BulletIntegration: fix Matrix3x3 conversion along with unit tests #34

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
2 participants
@xqms
Copy link
Contributor

xqms commented Jan 8, 2019

Bullet matrices are row-major, Magnum matrices are column-major.
The Matrix3x3 unit test did not account for this and tested the wrong behavior.
The test for Matrix4x4 seems fine, haven't looked in detail, though.

This probably hasn't been noticed before because BulletIntegration::MotionState uses the Matrix3x3 conversion only in the Magnum -> Bullet direction, and this is typically only done once during btRigidBody initialization. The bullet example in magnum-examples uses identity rotation at initialization...

BulletIntegration: fix Matrix3x3 conversion along with unit tests
Bullet matrices are row-major, Magnum matrices are column-major.
The unit test did not account for this and tested the wrong behavior.
The test for Matrix4x4 seems fine, haven't looked in detail, though.

@mosra mosra added this to TODO in Physics via automation Jan 9, 2019

@mosra mosra self-assigned this Jan 9, 2019

@mosra mosra added this to the 2019.01 milestone Jan 9, 2019

@mosra

This comment has been minimized.

Copy link
Owner

mosra commented Jan 9, 2019

Hi, thanks a lot for reporting and fixing this!

Interesting (and frightening) that this hasn't been noticed yet, huh. Looking into Bullet sources at how the Matrix4 conversion is done, yeah this was definitely wrong. @Squareys were you aware of this?

Merged your commit in 67059e5 (without the debug prints) and then, in 13356bf, to be really sure, updated the 3x3 test to now also convert the matrix to a quaternion and check that it indeed produces a correct value.

@mosra mosra closed this Jan 9, 2019

Physics automation moved this from TODO to Done Jan 9, 2019

@xqms

This comment has been minimized.

Copy link
Contributor

xqms commented Jan 9, 2019

Ah, sorry, the debug stuff should not have been in there, that slipped through. Thanks for checking and merging :-)

@xqms xqms deleted the xqms:bullet-fix-matrix3x3 branch Jan 9, 2019

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