Skip to content
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

Merged
merged 2 commits into from
Nov 13, 2023

Conversation

BenjaminNavarro
Copy link
Contributor

Supersedes #78

See both commit messages for the two individual fixes description

…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
Copy link
Member

@gergondet gergondet left a 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)
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

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?

@gergondet gergondet merged commit 237b74d into jrl-umi3218:master Nov 13, 2023
20 checks passed
gergondet added a commit that referenced this pull request Nov 13, 2023
- 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants