-
Notifications
You must be signed in to change notification settings - Fork 545
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
Use generate parameters library in PlanningPipelineClass + general cleanups #2288
Conversation
This pull request is in conflict. Could you fix it @sjahr? |
e295aca
to
6332f31
Compare
6332f31
to
e9d70d0
Compare
Codecov ReportAll modified lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2288 +/- ##
==========================================
- Coverage 50.84% 50.35% -0.49%
==========================================
Files 386 385 -1
Lines 31938 31767 -171
==========================================
- Hits 16237 15993 -244
- Misses 15701 15774 +73 ☔ View full report in Codecov by Sentry. |
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.
Code looks good, and I like the cleanup and extra error checks you've done along the way.
Mostly just have one question about changing a few function signatures and whether we should try do a more gradual change? But maybe the default arguments are most often left default and this doesn't have a huge impact?
moveit_ros/planning/planning_pipeline/include/moveit/planning_pipeline/planning_pipeline.h
Show resolved
Hide resolved
...line_interfaces/include/moveit/planning_pipeline_interfaces/planning_pipeline_interfaces.hpp
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.
The changes look good to me. I just have one question
moveit_planners/pilz_industrial_motion_planner/src/command_list_manager.cpp
Show resolved
Hide resolved
…eanups (moveit#2288) * Don't discard stuff * Move constants into source file * Move static consts into header * Don't ignore pipeline result * Use generate parameter library for planning pipeline parameters * Fix CI * More CI fixes * Remove more state from planning pipeline * Small cleanups * Assert planner_instance_ is not a nullptr * Remove valid variable * Simplify logic for trajectory printing * More helpful comments * Small logic simplification by using break * Fix clang-tidy * Pre-commit + Deprecate functions instead of removing them * Fix CI
Description
To avoid breaking MTC moveit/moveit_task_constructor#450 needs to be merged.
nodiscard
to return valuesChecklist