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

fix for kinematic constraints parsing #2267

Merged

Conversation

jrgnicho
Copy link
Contributor

Description

Minor change to properly parse the constraints parameter loaded from a yaml file. As described in the kinematic_constraints::constructConstraints function the yaml file needs to have the following structure:

 name: constraint_name
 constraint_ids: [constraint_1, constraint_2]
 constraints:
   constraint_1:
     type: orientation
     frame_id: world
     link_name: tool0
     orientation: [0, 0, 0]  # [r, p, y]
     tolerances: [0.01, 0.01, 3.15]
     weight: 1.0
   constraint_2:
     type: position
     frame_id: base_link
     link_name: tool0
     target_offset: [0.1, 0.1, 0.1]  # [x, y, z]
     region:
       x: [0.1, 0.4]  # [min, max]
       y: [0.2, 0.3]
       z: [0.1, 0.6]
     weight: 1.0

Prior to this change this yaml structure fails to load.

Checklist

  • [ x] Required by CI: Code is auto formatted using clang-format
  • [ N/A] Extend the tutorials / documentation reference
  • [ N/A] Document API changes relevant to the user in the MIGRATION.md notes
  • [ N/A] Create tests, which fail without this PR reference
  • [ N/A] Include a screenshot if changing a GUI
  • [ x] While waiting for someone to review your request, please help review another open pull request to support the maintainers

@mergify
Copy link

mergify bot commented Jul 15, 2023

Please target the main branch for development, we will backport the changes to humble for you if approved and if they don't break API.

@jrgnicho jrgnicho requested review from sea-bass and removed request for henningkayser July 15, 2023 01:25
@codecov
Copy link

codecov bot commented Jul 15, 2023

Codecov Report

Patch coverage: 59.29% and project coverage change: +0.46 🎉

Comparison is base (c9a9e40) 50.50% compared to head (725b93d) 50.95%.

❗ Current head 725b93d differs from pull request most recent head 607a44d. Consider uploading reports for the commit 607a44d to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2267      +/-   ##
==========================================
+ Coverage   50.50%   50.95%   +0.46%     
==========================================
  Files         386      382       -4     
  Lines       31732    31891     +159     
==========================================
+ Hits        16022    16248     +226     
+ Misses      15710    15643      -67     
Impacted Files Coverage Δ
...n/allvalid/collision_detector_allocator_allvalid.h 100.00% <ø> (ø)
.../collision_detection/test_collision_common_panda.h 98.73% <ø> (ø)
...it/collision_detection/test_collision_common_pr2.h 100.00% <ø> (ø)
..._core/collision_detection/src/collision_common.cpp 50.00% <ø> (+10.00%) ⬆️
...t_core/collision_detection/src/collision_tools.cpp 0.00% <ø> (ø)
...ction_bullet/collision_detector_allocator_bullet.h 100.00% <ø> (ø)
...src/bullet_integration/bullet_cast_bvh_manager.cpp 52.86% <ø> (ø)
...bullet_integration/bullet_discrete_bvh_manager.cpp 50.00% <ø> (ø)
...ion_bullet/src/bullet_integration/bullet_utils.cpp 64.77% <ø> (+0.12%) ⬆️
.../src/bullet_integration/contact_checker_common.cpp 84.73% <ø> (ø)
... and 99 more

... and 104 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@sea-bass sea-bass left a comment

Choose a reason for hiding this comment

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

I've traced down all the dots and parameter structure and this LGTM.

However, two things:

  1. Could you rebase and retarget this to the main branch so the fix goes there, and then I can mark this PR for humble backporting?
  2. I'm actually not a maintainer, so we'll still need other approvals :)

Copy link
Contributor

@sjahr sjahr left a comment

Choose a reason for hiding this comment

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

Looks good, thanks for the fix! If you re-target it at main, we can easily backport it by just adding and additional tag

@jrgnicho jrgnicho changed the base branch from humble to main July 17, 2023 15:37
@mergify
Copy link

mergify bot commented Jul 17, 2023

This pull request is in conflict. Could you fix it @jrgnicho?

@jrgnicho
Copy link
Contributor Author

Looks good, thanks for the fix! If you re-target it at main, we can easily backport it by just adding and additional tag

OK, I can do that. For future PR's targeting a branch other than main how should I proceed?

@jrgnicho jrgnicho force-pushed the fix/kinematic-constraints-parameter-parsing branch from 725b93d to 607a44d Compare July 17, 2023 15:41
@jrgnicho
Copy link
Contributor Author

Just targeted the change to the main branch

@jrgnicho jrgnicho added the backport-humble Mergify label that triggers a PR backport to Humble label Jul 17, 2023
@sea-bass
Copy link
Contributor

OK, I can do that. For future PR's targeting a branch other than main how should I proceed?

I think what you just did is perfect: Put up PR to main, add the backport-humble label, and that should do it :)

The one exception would be if the PR in question is a bug fix that doesn't make sense on main (it's been fixed/refactored, etc.) in which case targeting the non-main branch directly is OK.

@sjahr sjahr enabled auto-merge (squash) July 17, 2023 16:08
@sjahr sjahr merged commit b0f0f68 into moveit:main Jul 17, 2023
6 of 7 checks passed
mergify bot pushed a commit that referenced this pull request Jul 17, 2023
henningkayser pushed a commit that referenced this pull request Aug 1, 2023
(cherry picked from commit b0f0f68)

Co-authored-by: Jorge Nicho <jrgnichodevel@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport-humble Mergify label that triggers a PR backport to Humble
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants