-
Notifications
You must be signed in to change notification settings - Fork 285
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
Fix incorrect rotational motion of BallJoint and FreeJoint #518
Conversation
[BallJoint] generalized velocity: relative angular velocity from parent to child generalized acceleration: relative angular acceleration from parent to child [FreeJoint] generalized velocity: relative spatial velocity from parent to child generalized acceleration: relative spatial acceleration from parent to child
… method for BodyNode velocities.
@@ -508,25 +508,9 @@ void FreeJoint::setAngularAcceleration( | |||
|
|||
//============================================================================== | |||
Eigen::Matrix6d FreeJoint::getLocalJacobianStatic( | |||
const Eigen::Vector6d& _positions) const | |||
const Eigen::Vector6d& /*positions*/) const |
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 if positions
isn't going to be used at all here, then we should either
- Remove the
positions
argument entirely or - Save the current positions in a temporary
Eigen::Vector6d
set the current positions topositions
and then reset the positions to their original values at the end. If the incomingpositions
vector matches the current positions, then this shouldn't be very much overhead, because the bookkeeping should all shortcircuit.
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 wait, I take that back. Does this function imply that the Jacobian is now completely agnostic to the current position values?
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.
Yes. Previously a Jacobian of FreeJoint was dependent on joint positions like other joints but now it's only dependent on the transform from child BodyNode to the joint. Since this function is a virtual function declared in Joint so it still has positions argument.
…ctors ...since it will be updated when they get called by the kinematics auto-updating mechanism
@mxgrey I also removed the Jacobian computation of BallJoint/FreeJoint in the construction time since it will be updated whenever it gets called through the auto-updating kinematics mechanism. I would like to merge this soon so it would be good if you make comments you might have on this PR. |
It looks good to me now 👍 At some point, I think we should consider organizing and standardizing the unit tests a bit more rigorously, but that's not an immediate concern. |
Yeah, we can first consider to categorize the tests into something like file-wise tests, subject-wise tests, and regression tests. |
Fix incorrect rotational motion of BallJoint and FreeJoint
Does this help with #424? Or is that already fixed in dart5? |
Yes, I believe this helps with #424, but for sure I need to double check by rerunning the test with dart 5.1. |
This PR fixes the incorrect rotational motion of BallJoint and FreeJoint near +/- 180 degrees.