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

Make the root_link the father of the base_link instead of the other way around #142

Merged
merged 3 commits into from
Sep 29, 2020

Conversation

traversaro
Copy link
Member

This permits to avoid the Gazebo bug described in #140 .
Furthermore, this will make sure that even after fixed joint lumping the link in Classic Gazebo will be called "root_link".

For an extensive rationale, see #140 (comment) .

This PR requires #141 to ensure that even if merge the changes will not reach directly the master branch.

…ay around

This permits to avoid the Gazebo bug described in #140 . 
Furthermore, this will make sure that even after fixed joint lumping the link in Classic Gazebo will be called "root_link".
@traversaro
Copy link
Member Author

@Nicogene probably it could make sense to to the same change in the iCub 3 model.

…k_fixed_joint

This is for consistency with the rest of "fake links/additional frames", whose joints
have always the name <additional_frame_name>_fixed_joint .
@Nicogene
Copy link
Member

Do you mean 9e94025 ?

Or also the changes in the yaml.in?

@traversaro
Copy link
Member Author

I mean just the change in the yaml.in, the commit you link is part of another PR (#141).

Comment on lines +947 to +951
<joint name="base_link_fixed_joint" type="fixed">
<origin xyz="0 0 0" rpy="0 -0 0" />
<axis xyz="0 0 0" />
<parent link="base_link" />
<child link="root_link" />
<parent link="root_link" />
<child link="base_link" />
Copy link
Collaborator

@prashanthr05 prashanthr05 Sep 29, 2020

Choose a reason for hiding this comment

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

Probably we should document this decision/change somewhere more visible than in the issues?

Copy link
Member Author

Choose a reason for hiding this comment

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

I plan to add a CHANGELOG.md in icub-models as done for all the other repos.

Copy link
Collaborator

@prashanthr05 prashanthr05 left a comment

Choose a reason for hiding this comment

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

Ok for me! Maybe we can wait for another reviewer's (mainly @Nicogene being one of the active contributors for this repo) approval before proceeding for the merge.

Copy link
Member

@Nicogene Nicogene left a comment

Choose a reason for hiding this comment

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

Ok for me too, I tried it successfully with the icub 3 upper body model 👍

@traversaro traversaro merged commit 0ee6723 into master Sep 29, 2020
@prashanthr05
Copy link
Collaborator

@traversaro shall we then delete the branch?

@traversaro traversaro deleted the remove-base-link-madness branch September 29, 2020 21:22
@traversaro
Copy link
Member Author

@traversaro shall we then delete the branch?

Done!

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