-
Notifications
You must be signed in to change notification settings - Fork 767
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
Normalize rotations after composition #555
Conversation
@Alexma3312 FYI 🙂 |
Amazing find :) Two questions:
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very cool! But please search for normalize through the GTSAM code base as I think we might already have some of this in place. I can be mistaken.
And: we really cannot add this in the low-level operations. We can discuss in person.
gtsam/geometry/Rot3Q.cpp
Outdated
/* ************************************************************************* */ | ||
Rot3 Rot3::operator*(const Rot3& R2) const { | ||
return Rot3(quaternion_ * R2.quaternion_); | ||
return normalize(Rot3(quaternion_ * R2.quaternion_)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's unacceptable I'm afraid: people will have to do this explicitly themselves, it can't be part of the low-level geometry.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated.
I did a search and the only There is a |
OK, please add comments in both places to compare and contrast them. Maybe move declaration and definitions of the new method to be adjacent To ClosestTo. |
This PR fixes #372
I added
normalize
function to orthogonalize the rotation after composition for the matrix representation and I used thenormalized
method for the quaternion representation.I added a test case based on #372 and while it initially failed, it now succeeds, validating this approach.
The idea is from here.