-
Notifications
You must be signed in to change notification settings - Fork 495
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
[main] Port CHOMP to MoveIt2 #809
Conversation
moveit_planners/chomp/chomp_interface/src/chomp_planning_context.cpp
Outdated
Show resolved
Hide resolved
moveit_planners/chomp/chomp_interface/src/chomp_planning_context.cpp
Outdated
Show resolved
Hide resolved
moveit_planners/chomp/chomp_motion_planner/src/chomp_planner.cpp
Outdated
Show resolved
Hide resolved
moveit_planners/chomp/chomp_optimizer_adapter/src/chomp_optimizer_adapter.cpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks really good so far, thanks a lot again for your work. I have left some minor fixes that should be addressed before merging, but otherwise I think this is good to go. Before merging: on what robot have you tested this (if anything else than the Panda)? I'm going to give this a couple runs and see if I find any issues.
I also noticed that there is still a bunch of commented code in this package (the parts not from you). I'm inclined to review these and simply remove commented code if it doesn't seem useful. Most of it seems to be related to profiling and debugging. Since you apparently use CHOMP currently for your project, feel free to re-enable and modify what you find or make suggestions for cleanup.
Thanks again for your review! It has been tested on a 6-DOF industrial robot arm and there have been no issues. I'm not using or planing to use the commented code at the moment so I agree that the profiling/debugging parts can be removed. However, it seems that the Hamiltonian Monte Carlo method has never been thoroughly tested (see this comment), so I would suggest to make that a github issue to be fixed in the future. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All right, to me this looks ready to get merged. @tylerjw or @JafarAbdi could either of you give this a quick review for the second approval?
Codecov Report
@@ Coverage Diff @@
## main #809 +/- ##
==========================================
- Coverage 56.69% 56.67% -0.01%
==========================================
Files 200 200
Lines 21660 21660
==========================================
- Hits 12277 12274 -3
- Misses 9383 9386 +3
Continue to review full report at Codecov.
|
Looks like I accidentally merged |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the port, just left a few comments then I think it's ready to be merged
moveit_planners/chomp/chomp_motion_planner/src/chomp_optimizer.cpp
Outdated
Show resolved
Hide resolved
moveit_planners/chomp/chomp_motion_planner/src/chomp_optimizer.cpp
Outdated
Show resolved
Hide resolved
moveit_planners/chomp/chomp_motion_planner/src/chomp_planner.cpp
Outdated
Show resolved
Hide resolved
moveit_planners/chomp/chomp_motion_planner/src/chomp_planner.cpp
Outdated
Show resolved
Hide resolved
moveit_planners/chomp/chomp_motion_planner/src/chomp_planner.cpp
Outdated
Show resolved
Hide resolved
moveit_planners/chomp/chomp_motion_planner/src/chomp_planner.cpp
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just curious, there are a few reordering of include files and adding/removing black lines, any reason for that ?
No reason. I'm not exactly sure what caused this, maybe some code formatting tool I used. If you prefer a specific ordering, let me know. |
@andreas-botbuilt I open a PR to fix very nit stuff, could you please review it ? thanks |
Sorry I forgot to run |
This pull request is in conflict. Could you fix it @andreas-botbuilt? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Co-authored-by: Henning Kayser <henningkayser@picknik.ai> Co-authored-by: Jafar Abdi <cafer.abdi@gmail.com> (cherry picked from commit 0458208) # Conflicts: # moveit_planners/chomp/chomp_interface/package.xml # moveit_planners/chomp/chomp_interface/src/chomp_planning_context.cpp # moveit_planners/chomp/chomp_interface/src/chomp_plugin.cpp # moveit_planners/chomp/chomp_motion_planner/package.xml # moveit_planners/chomp/chomp_optimizer_adapter/package.xml
Description
This PR makes CHOMP available in MoveIt2. CHOMP can be used as a stand-alone motion planner plugin (instead of OMPL) or as a post-processor for OMPL (where the seed trajectory for CHOMP is the output of OMPL).
Checklist
Changes
main