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

urdf: fix test and clean up internals #1126

Merged
merged 3 commits into from
Sep 21, 2022
Merged

Conversation

scpeters
Copy link
Member

@scpeters scpeters commented Sep 2, 2022

🦟 Bug fix

Fixes incorrect kinematics in a URDF test file and updates test accordingly. Also clean up some internal parser functions.

Summary

The urdf_gazebo_extensions.urdf file had incorrect kinematics with link3 being the child of two different joints. This adds a link4 and replaces joint13 with joint14 connecting link1 to link4. This also fixes some expectations about the implicit_spring_damper tags.

I also cleaned up some of the internals of parser_urdf in 90595c0:

  • remove unused variables
  • remove unused function parameters
  • make input function parameters const

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • Consider updating Python bindings (if the library has them)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining Signed-off-by messages.

The urdf_gazebo_extensions.urdf file had incorrect kinematics
with link3 being the child of two different joints.
This adds a link4 and converts joint13 to joint14.
This also fixes some expectations about the
implicit_spring_damper tags.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters scpeters requested a review from azeey as a code owner September 2, 2022 23:59
@scpeters scpeters added the bug Something isn't working label Sep 2, 2022
@github-actions github-actions bot added Gazebo 1️1️ Dependency of Gazebo classic version 11 🏰 citadel Ignition Citadel labels Sep 3, 2022
@osrf-triage osrf-triage added this to Inbox in Core development Sep 3, 2022
@codecov
Copy link

codecov bot commented Sep 3, 2022

Codecov Report

Merging #1126 (b361b5b) into sdf9 (3b82d6a) will not change coverage.
The diff coverage is n/a.

❗ Current head b361b5b differs from pull request most recent head 90595c0. Consider uploading reports for the commit 90595c0 to get more accurate results

@@           Coverage Diff           @@
##             sdf9    #1126   +/-   ##
=======================================
  Coverage   87.81%   87.81%           
=======================================
  Files          64       64           
  Lines       10099    10099           
=======================================
  Hits         8868     8868           
  Misses       1231     1231           

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Remove some unused parameters, and make some const.

Signed-off-by: Steve Peters <scpeters@openrobotics.org>
@scpeters scpeters changed the title urdf extension test: fix URDF kinematics urdf: fix test and clean up internals Sep 14, 2022
@scpeters scpeters moved this from Inbox to In review in Core development Sep 19, 2022
Copy link
Contributor

@j-rivero j-rivero left a comment

Choose a reason for hiding this comment

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

LGTM. One silly question probably: why was necessary to rename the name joint13 to joint14 in the test/integration/urdf_gazebo_extensions.urdf file?

@scpeters
Copy link
Member Author

LGTM. One silly question probably: why was necessary to rename the name joint13 to joint14 in the test/integration/urdf_gazebo_extensions.urdf file?

the naming scheme in this file uses single digits for links and pairs of digits for the joints corresponding to the parent and child link. So the change of the joint name from joint13 to joint14 isn't significant in itself, but the implied change of the child link from link3 to link4 is significant for the kinematics, since link3 was the child of both joint23 and joint13, which is not allowed in URDF. After this change, link3 has only one parent joint.

@scpeters scpeters merged commit b7f0cba into sdf9 Sep 21, 2022
Core development automation moved this from In review to Done Sep 21, 2022
@scpeters scpeters deleted the scpeters/fix_urdf_gz_ext_test branch September 21, 2022 21:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working 🏰 citadel Ignition Citadel Gazebo 1️1️ Dependency of Gazebo classic version 11
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

2 participants