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

Add the possibility to use BLF IK in walking module #118

Merged
merged 27 commits into from
Sep 21, 2022

Conversation

GiulioRomualdi
Copy link
Member

@GiulioRomualdi GiulioRomualdi commented Sep 7, 2022

@GiulioRomualdi GiulioRomualdi force-pushed the feature/blf_ik branch 2 times, most recently from cae1e49 to 08d8772 Compare September 10, 2022 15:29
@GiulioRomualdi
Copy link
Member Author

@S-Dafarra I think you can review the PR

Copy link
Collaborator

@S-Dafarra S-Dafarra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mostly minor comments. Let me know if something would require too much of rework

Comment on lines 12 to 13
[stance]
name "stance"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't these two a bit redundant?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, this is the way in which the MultiStateWeightProvider works. The provider wants a vector named stances that contains the name of the groups associated to each state. Then for each state, we specify a name and the weight itself.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the name of the group be different from the name parameter? Do you envision some case where this could be useful?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the name of the group be different from the name parameter?

Yes

Do you envision some case where this could be useful?

For the time being, I don't see the reason why they should be different.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2cf39b7 and 85c278f changes the names of the groups using uppercase. This should reduce the misunderstanding

Comment on lines 44 to 47
m_usejointRetargeting = false;
int usejointRetargeting = 0;
ptr->getParameter("use_joint_retargeting", usejointRetargeting);
m_usejointRetargeting = usejointRetargeting != 0;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this casting needed here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use_joint_retargeting is actually a number stored in the main configuration file.

ptr->getParameter("use_joint_retargeting", m_usejointRetargeting);

will fail since m_usejointRetargeting is a bool and the check inside blf fails. We should change the code in blf

Copy link
Collaborator

@S-Dafarra S-Dafarra Sep 21, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But why this works instead?

m_useRootLinkForHeight = false;
ptr->getParameter("use_root_link_for_height", m_useRootLinkForHeight);

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because the parameter use_root_link_for_height is a boolean. By the way, this ami-iit/bipedal-locomotion-framework#566 should remove this limitation of blf

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed here: 97b6c70

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.

3 participants