-
Notifications
You must be signed in to change notification settings - Fork 39
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
[TimeVaryingDCMPlanner] restructure of setDCMReference() and setContactPhaseList() #220
Conversation
…after TimeVaryingDCMPlanner::setContactPhaseList()
716a8bf
to
e10b5e6
Compare
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.
Ok for me! But I would wait for @S-Dafarra approval as well.
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.
Just a simple suggestion. So basically, you can set the initial dcm condition without changing the foot list, right?
@@ -491,6 +494,8 @@ struct TimeVaryingDCMPlanner::Impl | |||
bool setupOptimizationProblem(const ContactPhaseList& contactPhaseList, | |||
const DCMPlannerState& initialState) | |||
{ | |||
constexpr auto errorPrefix = "[TimeVaryingDCMPlanner::Impl::setupOptimizationProblem] "; |
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.
Maybe we can add some tags to distinguish between errors and info messages.
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 would do it in a separate PR for all the error/warning/info in the repo. Indeed now all the errors/warnings/info are printed with cerr
and there is not a common function to call. #137 (comment)
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.
For instance, we can use something like this: https://github.com/robotology/idyntree/blob/2e5cf7180a517bf91777d48761c4c12c94e7e404/src/core/include/iDynTree/Core/Utils.h#L66-L93
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 was not referring to cerr
but rather to the message itself. It is not clear whether the message is an error or not. I was suggesting adding something like [ERROR]
since it is going to be much easier to find them in the middle of tons of other messages.
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.
On that one I agree. Done in 7999c2f
This possibility was already implemented in the first version. With this PR the user can set the reference dcm (used for regularization) independently from the foot list |
Comment #220 (comment) updated. @S-Dafarra |
#208 implements
setDCMReference()
however the user had to call the function aftersetContactPhaseList()
. Thanks to this PR,setDCMReference()
does not depends onsetContactPhaseList()
and the user can call it whenever their want.cc @paolo-viceconte