Skip to content

Conversation

@stephanie-eng
Copy link
Contributor

@stephanie-eng stephanie-eng commented Jun 7, 2022

Description

Adds support for using mixed constraint types (position and orientation) with OMPL's constrained planner.

Tutorial here: moveit/moveit2_tutorials#421

Checklist

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

@codecov
Copy link

codecov bot commented Jun 7, 2022

Codecov Report

Merging #1319 (9b8790b) into main (212af0f) will decrease coverage by 0.01%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #1319      +/-   ##
==========================================
- Coverage   61.57%   61.56%   -0.00%     
==========================================
  Files         274      274              
  Lines       24965    24966       +1     
==========================================
  Hits        15369    15369              
- Misses       9596     9597       +1     
Impacted Files Coverage Δ
...de/moveit/ompl_interface/detail/ompl_constraints.h 0.00% <ø> (ø)
...mpl/ompl_interface/src/detail/ompl_constraints.cpp 0.00% <0.00%> (ø)
...pl/ompl_interface/src/planning_context_manager.cpp 54.37% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 212af0f...9b8790b. Read the comment docs.

@stephanie-eng stephanie-eng force-pushed the seng/multiple-constraints branch from df69ac1 to 68da677 Compare June 7, 2022 14:03
@stephanie-eng stephanie-eng requested a review from AndyZe June 7, 2022 14:04
@stephanie-eng stephanie-eng marked this pull request as ready for review June 7, 2022 14:04
Copy link
Member

@AndyZe AndyZe left a comment

Choose a reason for hiding this comment

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

I haven't tested it yet. The code changes look good and surprisingly simple though 👍

Copy link
Contributor

@AdamPettinger AdamPettinger left a comment

Choose a reason for hiding this comment

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

Seems very straightforward and elegant. I played with this a bunch starting from moveit/moveit2_tutorials#421 as an example, and it looks really good! Namely: single orientation, single position (with volume == 0 and volume > 0), and 1 position + 1 orientation all seem to be planning successfully very quickly.

@moveit moveit deleted a comment from mergify bot Jun 15, 2022
@AndyZe AndyZe merged commit c491ac1 into moveit:main Jun 15, 2022
peterdavidfagan pushed a commit to peterdavidfagan/moveit2 that referenced this pull request Jul 14, 2022
* Add support for mixed constraints

* Update moveit_planners/ompl/ompl_interface/src/detail/ompl_constraints.cpp

Co-authored-by: AndyZe <[email protected]>

Co-authored-by: AndyZe <[email protected]>
Co-authored-by: AndyZe <[email protected]>
@jih189
Copy link

jih189 commented Sep 5, 2022

The code looks strange to me. In the planning_context_manager.cpp, line 539 to line 544,, it still said "Mixed constraints are not supported", and it only allows one constraint based on the if statement.
if (constrained_planning_iterator != pc->second.config.end() && boost::lexical_cast<bool>(constrained_planning_iterator->second) && ((req.path_constraints.position_constraints.size() == 1) != (req.path_constraints.orientation_constraints.size() == 1)))

I think it is wrong. If so, what have you tested?

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.

4 participants