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.

Sign up for GitHub

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

Jump to bottom

Port OMPL orientation constraints to MoveIt2 #1273

Merged
merged 12 commits into from
Jun 7, 2022
Merged

Port OMPL orientation constraints to MoveIt2 #1273

merged 12 commits into from
Jun 7, 2022

Conversation

stephanie-eng
Copy link
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

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/include/moveit/ompl_interface/detail/ompl_constraints.h Outdated Show resolved Hide resolved
moveit_planners/ompl/ompl_interface/include/moveit/ompl_interface/detail/ompl_constraints.h Outdated Show resolved Hide resolved
moveit_planners/ompl/ompl_interface/include/moveit/ompl_interface/detail/ompl_constraints.h Outdated Show resolved Hide resolved
moveit_planners/ompl/ompl_interface/include/moveit/ompl_interface/detail/ompl_constraints.h Outdated Show resolved Hide resolved
moveit_planners/ompl/ompl_interface/include/moveit/ompl_interface/detail/ompl_constraints.h Outdated Show resolved Hide resolved
moveit_planners/ompl/ompl_interface/src/detail/ompl_constraints.cpp Outdated Show resolved Hide resolved
moveit_planners/ompl/ompl_interface/src/detail/ompl_constraints.cpp Show resolved Hide resolved
moveit_planners/ompl/ompl_interface/src/detail/ompl_constraints.cpp Show resolved Hide resolved
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
Member

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
Contributor

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?

moveit_planners/ompl/ompl_interface/src/detail/ompl_constraints.cpp Outdated Show resolved Hide resolved
@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
Contributor Author

@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
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
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

moveit_planners/ompl/ompl_interface/include/moveit/ompl_interface/detail/ompl_constraints.h Show resolved Hide resolved
moveit_planners/ompl/ompl_interface/include/moveit/ompl_interface/detail/ompl_constraints.h Outdated Show resolved Hide resolved
moveit_planners/ompl/ompl_interface/include/moveit/ompl_interface/detail/ompl_constraints.h Outdated Show resolved Hide resolved
henningkayser
henningkayser approved these changes Jun 2, 2022
View reviewed changes
Copy link
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

moveit_planners/ompl/ompl_interface/src/planning_context_manager.cpp Outdated Show resolved Hide resolved
@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.

None yet
6 participants
@stephanie-eng @AndyZe @mamoll @henningkayser @sjahr @JeroenDM