-
Notifications
You must be signed in to change notification settings - Fork 1
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 Human inverse kinematic class #15
Conversation
Hi @davidegorbani if the pr is ready for review I can take it a look |
Yes, I have also started adding the task relative to the SO(3) task. |
ci_env.yml
Outdated
name: biomechanical-analysis-framework | ||
channels: | ||
- conda-forge | ||
- robotology |
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 seems all the packages you use are from the conda-forge channel, we can probably remove this.
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.
Removed!
ci_env.yml
Outdated
- robotology | ||
dependencies: | ||
- cmake | ||
- clangxx>=14.0.0 |
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.
We tipically use the compilers meta package to get the compiler, do you have any need specifically for clang?
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.
No, it was already there so I left it.
.github/workflows/ci.yml
Outdated
# Print the environment variables to simplify development and debugging | ||
# Use conda for main dependencies | ||
- uses: conda-incubator/setup-miniconda@v2 | ||
if: matrix.os == 'ubuntu-latest' | ||
with: | ||
miniforge-variant: Mambaforge | ||
miniforge-version: latest |
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 we can use setup-micromamba
for both, if we need to install anything else I think we can do that via micromamba install
instead of mamba install
.
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 me we can proceed merging. I don't know if also @GiulioRomualdi is planning to review the PR, or if @traversaro has further comments. Otherwise we can proceed merging.
Ok, I will wait before merging. |
public: | ||
HumanIK(){}; // constructor | ||
~HumanIK(){}; // destructor |
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 document the code using doxygen format. Similar to the one in bipedal locomotion framework. This will can be used to automatically generate the documentation
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, I will do it!
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 thinking something more structured. Like https://github.com/ami-iit/bipedal-locomotion-framework/blob/master/src/IK/include/BipedalLocomotion/IK/QPFixedBaseInverseKinematics.h
src/IK/src/InverseKinematics.cpp
Outdated
double HumanIK::getDt() const | ||
{ | ||
return m_dtIntegration.count(); | ||
} |
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.
This will be returned in nanoseconds. Is this wanted?
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.
No, probably it is better to return the dt
in seconds.
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 changed it and now it returns the value in seconds.
src/IK/src/InverseKinematics.cpp
Outdated
bool HumanIK::getJointVelocities(Eigen::Ref<Eigen::VectorXd> jointVelocities) const | ||
{ | ||
jointVelocities = m_jointVelocities; | ||
|
||
return true; | ||
} |
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.
You should check if the size of jointVelocities
is correct
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.
Done it.
Co-authored-by: Giulio Romualdi <giulio.romualdi@gmail.com>
Co-authored-by: Giulio Romualdi <giulio.romualdi@gmail.com>
Thank you @davidegorbani! Feel free to merge it :) |
Thank you! Merging! |
Super! @ami-iit/ifeel @ami-iit/ergocub |
This PR aims to create a first skeleton of the class
HumanIK
that has to compute the inverse kinematics of the human.