Use generate parameters library in PlanningPipelineClass + general cleanups#2288
Merged
sjahr merged 23 commits intomoveit:mainfrom Oct 9, 2023
Merged
Use generate parameters library in PlanningPipelineClass + general cleanups#2288sjahr merged 23 commits intomoveit:mainfrom
sjahr merged 23 commits intomoveit:mainfrom
Conversation
|
This pull request is in conflict. Could you fix it @sjahr? |
sjahr
force-pushed
the
pr-clean_up_planning_pipeline_class
branch
from
e295aca to
6332f31
Compare
sjahr
force-pushed
the
pr-clean_up_planning_pipeline_class
branch
from
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. |
sea-bass
reviewed
Sep 12, 2023
Contributor
sea-bass
left a comment
sea-bass
left a comment
There was a problem hiding this comment.
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?
sea-bass
approved these changes
Sep 14, 2023
Abishalini
approved these changes
Sep 19, 2023
Contributor
Abishalini
left a comment
Abishalini
left a comment
There was a problem hiding this comment.
The changes look good to me. I just have one question
This was referenced Oct 5, 2023
m-elwin
pushed a commit
to m-elwin/moveit2
that referenced
this pull request
Dec 4, 2023
…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
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
To avoid breaking MTC moveit/moveit_task_constructor#450 needs to be merged.
nodiscardto return valuesChecklist