Skip to content

Add planner_dart component for DART planners - #567

Open
gilwoolee wants to merge 6 commits into
masterfrom
gilwoo/planner_dart
Open

Add planner_dart component for DART planners#567
gilwoolee wants to merge 6 commits into
masterfrom
gilwoo/planner_dart

Conversation

@gilwoolee

@gilwoolee gilwoolee commented Mar 3, 2020

Copy link
Copy Markdown
Contributor

This PR creates planner_dart component and builds all DART planners as planner_dart component.


Before creating a pull request

  • Document new methods and classes
  • Format code with make format

Before merging a pull request

  • Set version target by selecting a milestone on the right side
  • Summarize this change in CHANGELOG.md
  • Add unit test(s) for this change

@gilwoolee gilwoolee added this to the Aikido 0.4.0 milestone Mar 3, 2020
@brianhou brianhou modified the milestones: Aikido 0.4.0, Aikido 0.5.0 Aug 27, 2020
@sniyaz

sniyaz commented Sep 25, 2020

Copy link
Copy Markdown

Hey @brianhou, can you take another look? Reviving this now, I've made some fixes so Travis should be satisfied.

@sniyaz
sniyaz requested review from egordon and removed request for sniyaz September 25, 2020 19:20
@sniyaz sniyaz self-assigned this Sep 25, 2020

@aditya-vk aditya-vk left a comment

Copy link
Copy Markdown
Contributor

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)

Copy link
Copy Markdown
Contributor

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.

Copy link
Copy Markdown

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"

Copy link
Copy Markdown
Contributor

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?

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See previous comment! 😸

@sniyaz

sniyaz commented Sep 25, 2020

Copy link
Copy Markdown

@aditya-vk You have a good question about when to split off a new component. I think the motivation here is a combination of:

  1. The amount of code in this component has grown large enough that splitting it off makes sense.
  2. The dependency here on OMPL means that none of our planners would build if OMPL was not installed correctly (assuming we were using a single component for all planners like we have now).

If these motivations sound good, I'm happy to create an issue to track this and update our AIKIDO guidelines.

@codecov

codecov Bot commented Sep 17, 2021

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.69%. Comparing base (8e2bcd9) to head (a3654af).
⚠️ Report is 22 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #567      +/-   ##
==========================================
+ Coverage   76.37%   78.69%   +2.31%     
==========================================
  Files         200      170      -30     
  Lines        7030     6406     -624     
==========================================
- Hits         5369     5041     -328     
+ Misses       1661     1365     -296     

see 32 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants