Skip to content

Port OMPL orientation constraints to MoveIt2#1273

Merged
AndyZe merged 12 commits intomoveit:mainfrom
stephanie-eng:seng/ompl-orientation-constraints
Jun 7, 2022
Merged

Port OMPL orientation constraints to MoveIt2#1273
AndyZe merged 12 commits intomoveit:mainfrom
stephanie-eng:seng/ompl-orientation-constraints

Conversation

@stephanie-eng
Copy link
Copy Markdown
Contributor

@stephanie-eng stephanie-eng commented May 24, 2022 •
edited
Loading

Description

Port of the OMPL orientation constraints. Update to the tutorial is in progress here: moveit/moveit2_tutorials#386

@codecov
Copy link
Copy Markdown

codecov bot commented May 24, 2022 •
edited
Loading

Codecov Report

Merging #1273 (c396e55) into main (4502659) will decrease coverage by 0.06%.
The diff coverage is 2.44%.

@@            Coverage Diff             @@
##             main    #1273      +/-   ##
==========================================
- Coverage   61.46%   61.41%   -0.05%     
==========================================
  Files         274      274              
  Lines       24940    24963      +23     
==========================================
  Hits        15328    15328              
- Misses       9612     9635      +23
Impacted Files Coverage Δ
...de/moveit/ompl_interface/detail/ompl_constraints.h 0.00% <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%> (-0.21%) ⬇️
...kinematic_constraints/src/kinematic_constraint.cpp 74.24% <100.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 4502659...c396e55. Read the comment docs.

@stephanie-eng stephanie-eng mentioned this pull request May 24, 2022
Merged
4 tasks
@stephanie-eng stephanie-eng marked this pull request as ready for review May 25, 2022 12:25
@stephanie-eng stephanie-eng requested a review from AndyZe May 25, 2022 12:25
@stephanie-eng stephanie-eng force-pushed the seng/ompl-orientation-constraints branch from e364eea to 1170c2e Compare May 25, 2022 12:30
@AndyZe AndyZe mentioned this pull request May 25, 2022
Closed
AndyZe
AndyZe reviewed May 25, 2022
View reviewed changes
moveit_planners/ompl/ompl_interface/src/detail/ompl_constraints.cpp
if (num_pos_con > 0 && num_ori_con > 0)
{
RCLCPP_ERROR(LOGGER, "Combining position and orientation constraints not implemented yet for OMPL's constrained "
RCLCPP_ERROR(LOGGER, "Combining position and orientation constraints is not implemented yet for OMPL's constrained "
Copy link
Copy Markdown
Member

@AndyZe AndyZe May 25, 2022

Choose a reason for hiding this comment

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

Oh, this kind of sucks. Oh well. We can work with it.

Copy link
Copy Markdown
Contributor

@mamoll mamoll May 27, 2022

Choose a reason for hiding this comment

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

@JeroenDM what is the difficulty here? The underlying OMPL code shouldn't care. Can you describe what is needed to support this?

@AndyZe AndyZe mentioned this pull request May 26, 2022
Closed
@stephanie-eng @AndyZe
Apply suggestions from code review
3856cdb 
Co-authored-by: AndyZe <andyz@utexas.edu>
@stephanie-eng
Copy link
Copy Markdown
Contributor Author

stephanie-eng commented May 26, 2022

@AndyZe I've started incorporating your suggestions - I noted some of them will modify the orientation constraints in a way that's not consistent with the equality or box constraints (like removing the RCLCPP_DEBUG lines or changing some function arguments. I'd prefer to keep the consistency, so if you agree, I'm going to also apply your suggestions to the other, preexisting constraint types.

@AndyZe
Copy link
Copy Markdown
Member

AndyZe commented May 26, 2022 •
edited
Loading

That sounds good to me. Let's check if somebody else agrees (@henningkayser @JafarAbdi @vatanaksoytezer)

@stephanie-eng stephanie-eng mentioned this pull request Jun 1, 2022
Merged
6 tasks
sjahr
sjahr approved these changes Jun 2, 2022
View reviewed changes
Copy link
Copy Markdown
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, I just left some minor comments

henningkayser
henningkayser approved these changes Jun 2, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@henningkayser henningkayser left a comment

Choose a reason for hiding this comment

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

Besides @sjahr's requests for more comments, I'd say this is good to go

@stephanie-eng stephanie-eng mentioned this pull request Jun 7, 2022
Merged
6 tasks
@AndyZe AndyZe merged commit 99c73e6 into moveit:main Jun 7, 2022
@henningkayser henningkayser mentioned this pull request Jun 13, 2022
Closed
peterdavidfagan pushed a commit to peterdavidfagan/moveit2 that referenced this pull request Jul 14, 2022
@stephanie-eng @JeroenDM
@AndyZe @peterdavidfagan
Port OMPL orientation constraints to MoveIt2 (moveit#1273)
d08a2ab 
* add orientation constraints to ompl interface

* also use constrained planner for a single orientation constraint

* Port orientation constraints to ROS2

* Update oreintation constraints for to use Bounds properly

* Fix warning text

* Use correct parameterization type

* Clang-tidy fixes and cleanup

* Apply suggestions from code review

Co-authored-by: AndyZe <andyz@utexas.edu>

* Removed all RCLCPP_DEBUG lines for consistency

* Add extra comments for doxygen, apply const &

* Fix logic for using the ConstrainedPlanningStateSpace

Co-authored-by: JeroenDM <jeroendemaeyer@live.be>
Co-authored-by: AndyZe <andyz@utexas.edu>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mamoll mamoll mamoll left review comments

@henningkayser henningkayser henningkayser approved these changes

@AndyZe AndyZe AndyZe approved these changes

@sjahr sjahr sjahr approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@stephanie-eng @AndyZe @mamoll @henningkayser @sjahr @JeroenDM