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

Exposed planners from latest ompl release. #338

Merged
merged 4 commits into from Nov 11, 2016

Conversation

Projects
None yet
3 participants
@rbbg
Contributor

rbbg commented Nov 1, 2016

Description

This PR exposes the following OMPL planners:

geometric::FMT
geometric::BFMT
geometric::PDST
geometric::STRIDE
geometric::BiTRRT
geometric::LBTRRT
geometric::BiEST
geometric::ProjEST
geometric::LazyPRM
geometric::LazyPRMstar
geometric::SPARS
geometric::SPARStwo

Several other planners are available in OMPL, but from I found those to not work out of the box so some work would be required in integrating those.

To test: I have a forked moveit_resources (over here) that has a ompl_planning.yaml with (parameter-less) configurations for these planners.

Due to the difference in OMPL version between kinetic and jade/indigo, this should not be cherry picked (I think).

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Nov 1, 2016

Member

Awesome, thank you!

Can you also add default parameter values for these planners in the Setup Assistant in this PR so that its easier for new users to configure these planners?

Member

davetcoleman commented Nov 1, 2016

Awesome, thank you!

Can you also add default parameter values for these planners in the Setup Assistant in this PR so that its easier for new users to configure these planners?

@rbbg

This comment has been minimized.

Show comment
Hide comment
@rbbg

rbbg Nov 1, 2016

Contributor

I would like to, but I honestly have no clue what would be logical settings for most of these parameters, so I don't really know how to go about it. I figured it was perhaps best let to OMPL to select the defaults. Thoughts on how to go about this?

Contributor

rbbg commented Nov 1, 2016

I would like to, but I honestly have no clue what would be logical settings for most of these parameters, so I don't really know how to go about it. I figured it was perhaps best let to OMPL to select the defaults. Thoughts on how to go about this?

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Nov 1, 2016

Member

In the Setup Assistant defaults, just copy the default already provided in each planner's config. e.g. for FMT you can see its defaults in the constructor

This allows users to easily tweak the settings in the future, and keeps the planner setup consistent in MoveIt!

Member

davetcoleman commented Nov 1, 2016

In the Setup Assistant defaults, just copy the default already provided in each planner's config. e.g. for FMT you can see its defaults in the constructor

This allows users to easily tweak the settings in the future, and keeps the planner setup consistent in MoveIt!

@rbbg

This comment has been minimized.

Show comment
Hide comment
@rbbg

rbbg Nov 4, 2016

Contributor

I'm working on this but am running into a couple of problems so it might be a couple of days.

Contributor

rbbg commented Nov 4, 2016

I'm working on this but am running into a couple of problems so it might be a couple of days.

rbbg added some commits Oct 7, 2016

Exposed planners from latest ompl release.
Removed VFRRT.

Fixed BFMT typo.

Removed problematic planners.

Removed LazyLBTRRT due to error with InfSampler.
@rbbg

This comment has been minimized.

Show comment
Hide comment
@rbbg

rbbg Nov 10, 2016

Contributor

For some reason FMT and BFMT are not finding any solutions at the moment, so I need to see where that is coming from, I pretty sure they were working before so not sure what that is about. Also, my clang-format plugin seems to have some problems :(

Contributor

rbbg commented Nov 10, 2016

For some reason FMT and BFMT are not finding any solutions at the moment, so I need to see where that is coming from, I pretty sure they were working before so not sure what that is about. Also, my clang-format plugin seems to have some problems :(

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Nov 10, 2016

Member

For some reason FMT and BFMT are not finding any solutions at the moment, so I need to see where that is coming from

Is a parameter set incorrectly in theses defaults? I'm ok merging these new features now and worrying about tweaking individual parameters later.

Also, my clang-format plugin seems to have some problems :(

I'm curious - did you have to manually fix the formatting then? What version are you using?

Member

davetcoleman commented Nov 10, 2016

For some reason FMT and BFMT are not finding any solutions at the moment, so I need to see where that is coming from

Is a parameter set incorrectly in theses defaults? I'm ok merging these new features now and worrying about tweaking individual parameters later.

Also, my clang-format plugin seems to have some problems :(

I'm curious - did you have to manually fix the formatting then? What version are you using?

@rbbg

This comment has been minimized.

Show comment
Hide comment
@rbbg

rbbg Nov 11, 2016

Contributor

Is a parameter set incorrectly in theses defaults? I'm ok merging these new features now and worrying about tweaking individual parameters later.

I don't think it's a parameter. I tried with an empty configuration but that didn't work either. I'll try to take a better look at it as soon as possible.

I'm curious - did you have to manually fix the formatting then? What version are you using?

I was using clang-format-3.8 but that isn't compatible as demonstrated by #344. As a matter of fact, the formatting changes I saw in the diff of that PR were identical to the ones I was having in my moveit_config_data.cpp file. Switching to clang-format-3.6 fixed it.

Contributor

rbbg commented Nov 11, 2016

Is a parameter set incorrectly in theses defaults? I'm ok merging these new features now and worrying about tweaking individual parameters later.

I don't think it's a parameter. I tried with an empty configuration but that didn't work either. I'll try to take a better look at it as soon as possible.

I'm curious - did you have to manually fix the formatting then? What version are you using?

I was using clang-format-3.8 but that isn't compatible as demonstrated by #344. As a matter of fact, the formatting changes I saw in the diff of that PR were identical to the ones I was having in my moveit_config_data.cpp file. Switching to clang-format-3.6 fixed it.

@davetcoleman davetcoleman merged commit 26459b0 into ros-planning:kinetic-devel Nov 11, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Nov 11, 2016

Member

Thanks so much! Please open another PR when you get a chance to look at FMT / BFMT

Member

davetcoleman commented Nov 11, 2016

Thanks so much! Please open another PR when you get a chance to look at FMT / BFMT

@gavanderhoorn

This comment has been minimized.

Show comment
Hide comment
@gavanderhoorn

gavanderhoorn Nov 11, 2016

Member

@rbbg: is the 'regular' binary distributed OMPL release (on Kinetic) enough for this, or do we need a source install?

Member

gavanderhoorn commented Nov 11, 2016

@rbbg: is the 'regular' binary distributed OMPL release (on Kinetic) enough for this, or do we need a source install?

@davetcoleman

This comment has been minimized.

Show comment
Hide comment
@davetcoleman

davetcoleman Nov 11, 2016

Member

@gavanderhoorn the binary should work because Travis passed that uses OMPL from binary

Member

davetcoleman commented Nov 11, 2016

@gavanderhoorn the binary should work because Travis passed that uses OMPL from binary

@gavanderhoorn

This comment has been minimized.

Show comment
Hide comment
@gavanderhoorn

gavanderhoorn Nov 11, 2016

Member

Right. Missed that. Thanks.

Member

gavanderhoorn commented Nov 11, 2016

Right. Missed that. Thanks.

@rbbg rbbg deleted the rbbg:kinetic-ompl-planners branch Nov 11, 2016

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