-
Notifications
You must be signed in to change notification settings - Fork 30
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
Add planner_dart component for DART planners #567
base: master
Are you sure you want to change the base?
Conversation
Hey @brianhou, can you take another look? Reviving this now, I've made some fixes so Travis should be satisfied. |
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.
Looks mostly good! However, what is our current policy on creating a new component? Is it mostly for better organization (in which case it does not have to be a component that can be compiled optionally) or is it based on the dependencies involved? Whichever it is, we should document this. Could you create an issue to discuss, track and add the decision to our code guidelines?
@@ -0,0 +1,62 @@ | |||
if(CMAKE_COMPILER_IS_GNUCXX) | |||
if(OMPL_VERSION VERSION_GREATER 1.2.0 OR OMPL_VERSION VERSION_EQUAL 1.2.0) | |||
if(Boost_VERSION VERSION_LESS 106501) |
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.
We do not need to check boost version here since this subdirectory does not have ompl code.
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 is related to the below comment as well: this PR is set up for #566. Sorry, I totally should have made that more clear: because the ultimate goal here is deprecating a lot of the old robot/util
stuff, we need to move some of the planTo
methods that use ompl into this new dart_planner
module. That's what #566 starts to do, but we set up the ompl dependency here 😄
"${PROJECT_NAME}_trajectory" | ||
"${PROJECT_NAME}_statespace" | ||
"${PROJECT_NAME}_planner" | ||
"${PROJECT_NAME}_planner_ompl" |
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.
do we need ompl here?
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.
See previous comment! 😸
@aditya-vk You have a good question about when to split off a new component. I think the motivation here is a combination of:
If these motivations sound good, I'm happy to create an issue to track this and update our AIKIDO guidelines. |
Codecov Report
@@ Coverage Diff @@
## master #567 +/- ##
==========================================
+ Coverage 75.59% 78.69% +3.09%
==========================================
Files 243 170 -73
Lines 5856 6406 +550
==========================================
+ Hits 4427 5041 +614
+ Misses 1429 1365 -64
|
This PR creates
planner_dart
component and builds all DART planners asplanner_dart
component.Before creating a pull request
make format
Before merging a pull request
CHANGELOG.md