-
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
Add a new Criteria constructor for creating sequence #1273
Conversation
dart/dynamics/Linkage.hpp
Outdated
Criteria( | ||
BodyNode* start, | ||
BodyNode* target, | ||
bool includeBoth = false); |
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 realize that this is just copying from Chain.hpp
and that I'm probably the one that came up with the includeBoth
concept, but I'm not actually sure that I understand the motive. Why should the options be:
- Include both the start and the target, or
- Leave out whichever one is upstream
I don't really see what the significance of the upstream link would be that we would want to leave it out. The only thing I can think of is maybe we want to "start" from a body node that's at a fork and then "target" a body node that's at the end of a branch. I suppose that would be helpful for constructing chains within multi-arm skeletons.
I'm fine with leaving it as it is and merging it, but I thought I'd go ahead and voice my own confusion at an API that I probably made myself. If anyone else finds it confusing, we should probably consider tweaking it somehow.
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 realize that this is just copying from
Chain.hpp
For the record, it is identical except that mChain
option is false in this constructor. That's why I didn't make Chain use this constructor (maybe I should let Chain just use this constructor and change the option to true?).
Regarding includeBoth
, I assumed that this option is about whether it includes the parent joint of the upstream body or not because the joint is technically not in between start
and target
bodies. In this case, we could change the option name and the document more clear.
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 good call! You're exactly right: it's about whether to include the parent joints of both the start and the target. A more descriptive name would definitely be helpful. Maybe includeUpstreamParentJoint
?
Problem
No convenient way to create a sequence Linkage.
Solution
Add an utility constructor to
Linkage::Criteria
to create a sequence Linkage:Issues
Before creating a pull request
clang-format
Before merging a pull request
CHANGELOG.md