-
Notifications
You must be signed in to change notification settings - Fork 49
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
Relative jacobian fix #109
Relative jacobian fix #109
Conversation
…an ends at root it is possible to construct relative Jacobians without any duplicated joints (e.g both chains end at root). Trying to remove joints in this case is undefined behavior
If both bodies belong to the same chain then the parent joint of the reference body was always included, possibly increasing the number of dofs
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.
Other than the point about the potentially duplicated constructor this looks ok
Thanks for rebasing this on the latest master.
@@ -20,8 +20,25 @@ namespace rbd | |||
Jacobian::Jacobian() {} | |||
|
|||
Jacobian::Jacobian(const MultiBody & mb, const std::string & bodyName, const Eigen::Vector3d & point) | |||
: Jacobian(mb, bodyName, mb.body(0).name(), point) |
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 this should still work, the first while
loop in the constructor will break on index = 0
so the index != refBodyIndex_
branch is not taken and we end up with the same code
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 ended up with the two constructors because I couldn't find an easy way to make the base case (no ref body) works after the second fix as it would always discard the model's root joint. I could use something like an empty string or a special name for the ref body to know that I need to go all the way to the root joint but I thought that in the end it was simpler and less error prone like that.
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.
Ah I see, I walked through the code while looking at the current version but with the modified version it will indeed skip the root body.
I could use something like an empty string or a special name for the ref body to know that I need to go all the way to the root joint but I thought that in the end it was simpler and less error prone like that.
I think we can go with if(refBodyIndex_ != 0 && index == refBodyIndex_)
as a condition to break?
- Fix compatibility with Cython >= 3.0.0 - Fix packaging issue in Jammy - Fix yaml-cpp target name change in >= 0.8.0 - Fix incorrect joint path computation for relative jacobians (#109)
Supersedes #78
See both commit messages for the two individual fixes description