-
Notifications
You must be signed in to change notification settings - Fork 38
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
Distance and gravity tasks #717
Conversation
c3db224
to
7c88593
Compare
fdc8f56
to
a6078ce
Compare
CI seemed happy, I cleaned the history |
I realized there might have been a mistake in the The tests in the current form do not fail in either case, so I tried to switch back the tasks to constraint in a3baa19, but it still fails on CI (while on my PC it was working better). With 954b608 I moved them back to cost. |
I dug further into this and realized that the gravity error was always zero independently from how big the modification to the initial joint configuration was. Then, I realized two things:
Now, when changing the sign in the gravity task, the test case fails as expected! Curious to see the output of CI |
CI is happy 🎉 I am going to clean again the history |
e3cb598
to
700bf61
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.
LGTM!
@GiulioRomualdi friendly ping. This code has been tested also by @EhsanRanjbari in his application |
I will check the PR within today. I'm sorry for the late response |
Rebasing on top of master |
m_baseFrameName removed resize world_T_framePosition pow(world_T_framePosition(2),2) added made m_computedDistance public member Fixed use of relative distance task DIstanceTask added made m_computedDistance public member gitignore updated DistanceTask PR review: Naming fixed, m_frameName and m_frameEEName removed, unused headers removed.
Using fixed dimension vector for frame position in Distance task The input distance is normalized Applied clang-format to DistanceTask
Added DistanceTask to QPInverseKinematicsTest
GravityTask_Preliminary_Implementation GravityTask_Preliminary_ImplementationRev2 Fixed setEstimateGravityDir method matrices size fixed m_robotVelocityVariable.name fixed trailing whitespace fixed m_angularJacobian fixed, setEstimateGravityDir parameter fixed body to world rotation added feedforward gyro added
Fixed sign in GravityTask Added gravity task documentation. Applied clang-format to GravityTask
Added GravityTask to QPInverseKinematics Fix failure of QPInveseKinematicsTest by moving Gravity task to low priority Moving also distance task to low priority in QPInverseKinematics Fixed a sign mistake in GravityTask. This might have been the reason why the IK was failing before Added explanation on how the gravity task is obtained directly in the code.
It was necessary to refactor a bit the structure of the code, porting in each test case the addition of the tasks we are interested in. Added a small rotation error in the base. This is necessary since the random models generated by iDynTree seem always planar.
1d17868
to
9e5b03e
Compare
9e5b03e
to
972ab04
Compare
For some reason, the CI stays stuck in the dependencies phase on Ubuntu. |
Here the output:
|
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.
Some small comments
src/IK/src/DistanceTask.cpp
Outdated
|
||
toEigen(this->subA(m_robotVelocityVariable)) | ||
= (m_framePosition.transpose() * m_jacobian.topRows<3>()) | ||
/ (std::max(0.001, m_computedDistance)); |
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.
Can you remove 0.001 from the code? (I don't know if it makes sense to have it as parameter)
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.
It helps for the stability of the algorithm. I have moved it to an optional input parameter in c47e541
bool GravityTask::setDesiredGravityDirectionInTargetFrame( | ||
const Eigen::Ref<const Eigen::Vector3d> desiredGravityDirection) | ||
{ | ||
m_desiredZDirectionBody = desiredGravityDirection; |
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.
what if the norm is almost zero?
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.
The normalize
method from Eigen should already consider this case: https://github.com/libigl/eigen/blob/1f05f51517ec4fd91eed711e0f89e97a7c028c0e/Eigen/src/Core/Dot.h#L142-L147
Basically, if the norm is too small, it is left unchanged.
Regarding the CI problem, it's not related to this PR still is really annoying 😭 |
Co-authored-by: Giulio Romualdi <giulio.romualdi@gmail.com>
469f4b8
to
88eb49a
Compare
Thank you @S-Dafarra and @EhsanRanjbari for the contribution. Merging! |
I suspect that the CI problem has something to do with I say this because when I was modifying some |
Implements the
DistanceTask
andGravityTask
initially implemented by @EhsanRanjbari