Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Fixed fabs() use in quaternion interpolation #1479
Fixed fabs() use in quaternion interpolation #1479
Changes from all commits
7fc2104
ced91bc
513868d
f2ef841
b27bd2d
ea133d0
a5b9d3c
d3db1e6
70fa710
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
There is currently some ongoing refactoring around RNG, see #1267. @sjahr any thoughts if we should merge this and refactor later?
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.
It's not half-theta at all, that really is the angle between the two quaternions. The halving only shows up when you when you convert between Quaternions and axis-angle stuff to my knowledge
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.
100% agree that just calling into Eigen would be way easier (and probably better/safer) than DIY-ing the math ourselves; I just didn't feel confident suggesting a larger rewrite than was necessary to fix the bug.
Do note that Eigen's Quaternion parameters are in a different order than MoveIt's.
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.
I think we can merge this now b/c it's an obvious improvement. Thanks! especially for the new test
I'm still not sure I agree on the factor of 2 though. We can make an issue and figure it out later.
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.
Oh I think I misread your website. Lemme take a look at what they're doing, Quaternions are tricky
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.
Ok, found a source here that features the *2 as well https://nl.mathworks.com/matlabcentral/answers/415936-angle-between-2-quaternions
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.
Here's a fantastic introduction to quaternions that only slightly breaks my brain: https://eater.net/quaternions
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.
https://math.stackexchange.com/questions/4053346/the-angle-between-two-quaternion-vectors-is-equal-to-half-the-angle-between-thei
Oh my god the angle is half the angle... I think this is a definitions issue. Do we want the angle between rotations, or do we want the angle between the quaternions?
On the referenced Wikipedia article: "Compute Ω as the angle subtended by the arc, so that cos Ω = p0 ∙ p1, the n-dimensional dot product of the unit vectors from the origin to the ends."
So (if I read this correctly) it's the angle between the vectors (quaternions are basically 4D vectors), not the angle between the rotations that the quaternions represent. Therefore, it's theta and not half-theta.
This also reinforces my belief that we really should just let Eigen handle this...
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.
In light of that explanation, this code here (https://github.com/ros-planning/moveit2/blob/main/moveit_core/robot_model/src/floating_joint_model.cpp#L122) is also wrong (the method name explicitly mentions "rotation" and not "quaternion angle"), and is missing the factor 2.
(Doesn't change anything for our case since the slerp operation explicitly requires the angle between quaternions, not rotations)