-
Notifications
You must be signed in to change notification settings - Fork 287
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
Dirtying Jacobian update flags whenever the parent transform is changed -- Release 5.0 patch #500
Conversation
@@ -1339,6 +1339,14 @@ void BodyNode::notifyTransformUpdate() | |||
{ | |||
notifyVelocityUpdate(); // Global Velocity depends on the Global Transform | |||
|
|||
// Jacobian calculations are dependent on the parent's world transform, but | |||
// not on the world transform of their own BodyNode, so they must be dirtied | |||
// regardless of whether the world transform of this BodyNode is already dirty |
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 agree with that the transform of a parent BodyNode can be changed without its child BodyNodes being informed that their Jacobians are now dirty. Suppose we have a serial chain and the position of i-th joint changed then only the child BodyNodes Jacobian will be dirty when all the Jacobians of descendant BodyNodes should be dirty as well. In that sense, it seems good solution to set the flags at here instead of Joint::notifyPositionUpdate().
One concern is that we might want to update this comment a bit. If I'm correct, the Jacobian of a BodyNode is actually dependent on the parent's Jacobian and the relative transform (or the joint positions of the parent Joint) of the BodyNode (to transform the coordinates of the parent's Jacobian from the parent's to the BodyNode's).
Also, it would be good to clarify more why and when these flags must be dirtied regardless of whether the relative transform of this BodyNode is already dirty. Basically, we need this for the case, when BodyNode::notifyTransformUpdate() is called, Jacobian is updated while the relative transform of this BodyNode is not updated yet. However, this case wouldn't happen if the BodyNode has a parent BodyNode. The relative transform will be consequently updated when the Jacobian is updated to be used in transforming the coordinates of the parent's Jacobian. You can check this in BodyNode::updateBodyJacobian(). That being said, we place blew code here (before the relative transform's dirtiness check) for the case this BodyNode doesn't have parent BodyNode. Maybe this might be too much detailed note but it would prevent us from spending time to interpret again the intention of this code in the future.
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.
Suppose we have a serial chain and the position of i-th joint changed then only the child BodyNodes Jacobian will be dirty when all the Jacobians of descendant BodyNodes should be dirty as well. In that sense, it seems good solution to set the flags at here instead of Joint::notifyPositionUpdate().
This is a good point. It may be the case that the regression test is only passing because of the fact that we request the transform of the final BodyNode before dirtying the Jacobian. I'm going to adjust the regression test to account for this, and I bet it's going to start failing.
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'm now thinking we might need two update tracks:
notifyJacobianUpdate()
would only deal with updating the Jacobians, and it would cut off at whichever BodyNode already knows its Jacobian is dirty (if a parent BodyNode's Jacobian is dirty, then all its children's Jacobians must be dirty as well). notifyTransformUpdate()
will remain the same, but the very first thing it will do is call its own notifyJacobianUpdate()
.
However, notifyJacobianUpdate()
will need to be a member of the JacobianNode
class, not the BodyNode class. That way all JacobianNode
types (such as EndEffector
) will make use of it.
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.
This sounds good to me! These updates might be able to be applied to osgDart
branch or master once the branch is merged since JacobianNode
is introduced by osgDart
branch.
In addition, we might need notifyJacobianDerivUpdate()
, and it should be called by BodyNode::notifyVelocityUpdate()
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've made the changes that have been discussed, and I've reworked the unittest so that it fails without these latest changes.
I created PR #503 based on the above discussion. |
More efficient Jacobian update checking
Dirtying Jacobian update flags whenever the parent transform is changed -- Release 5.0 patch
This is a bug fix and regression test for release-5.0 which fixes issue #499.
Thanks to Nathan Ratliff for bringing this issue to our attention!