Skip to content
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

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

gilwoolee
Copy link
Contributor

@gilwoolee gilwoolee commented Mar 3, 2020

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
Copy link

sniyaz commented Sep 25, 2020

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
Copy link
Contributor

@aditya-vk aditya-vk left a 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)
Copy link
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

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
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

Choose a reason for hiding this comment

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

See previous comment! 😸

@sniyaz
Copy link

sniyaz commented Sep 25, 2020

@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
Copy link

codecov bot commented Sep 17, 2021

Codecov Report

Merging #567 (a3654af) into master (62dfb15) will increase coverage by 3.09%.
The diff coverage is 76.71%.

@@            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     
Impacted Files Coverage Δ
include/aikido/common/BSpline.hpp 100.00% <ø> (ø)
include/aikido/common/RNG.hpp 100.00% <ø> (ø)
include/aikido/common/Spline.hpp 100.00% <ø> (ø)
...clude/aikido/common/detail/ExecutorThread-impl.hpp 100.00% <ø> (ø)
include/aikido/common/detail/Spline-impl.hpp 96.96% <ø> (+0.22%) ⬆️
...lude/aikido/common/detail/metaprogramming-impl.hpp 80.00% <ø> (ø)
include/aikido/common/detail/pair-impl.hpp 100.00% <ø> (ø)
include/aikido/constraint/Differentiable.hpp 100.00% <ø> (ø)
...clude/aikido/constraint/DifferentiableSubspace.hpp 100.00% <ø> (ø)
include/aikido/constraint/Projectable.hpp 100.00% <ø> (ø)
... and 244 more

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