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

added STOMP planner in moveit_planners #965

Open
wants to merge 5 commits into
base: kinetic-devel
from

Conversation

Projects
None yet
4 participants
@raghavendersahdev
Copy link
Contributor

raghavendersahdev commented Jun 29, 2018

Addition of STOMP Planner into moveit planner

This PR is a work done by Raghavender as a part of 2018 Google summer of code. This PR adds the functionality of STOMP as implemented in ros-industrial/industrial_moveit repo and adds it to moveIt. It merges stomp_moveit and stomp_plugins packages into one package. Only the stomp_moveit and stomp_plugins packages are taken from industrial_moveit. All other packages stay in industrial_moveit and stomp_core package is a pre-requisite to run STOMP in moveit planners

Checklist

  • addition of STOMP motion planner into moveit
  • merges the 2 packages stomp_moveit and stomp_plugins into one package
  • stomp_core package is a pre-requisite dependency to run STOMP in moveIt. stomp_core must be built from source as its not released as a debian yet.
  • addition of STOMP configuration files (stomp_planning_pipeline.launch.xml and stomp_planning.yaml generated from the setup assistant code) into the moveit setup assisant
@raghavendersahdev

This comment has been minimized.

Copy link
Contributor

raghavendersahdev commented Jun 29, 2018

I need look at this PR more closely, travis seems to be failing currently.

@davetcoleman
Copy link
Member

davetcoleman left a comment

will review further once travis passes

0.1.0 (2017-03-14)
------------------
* Initial release
* Contributors: Jonathan Meyer, Levi Armstrong, Jorge Nicho

This comment has been minimized.

- joint_update_rates: The rates at which to update the joints during numerical ik computations.
- max_ik_iterations: Limit on the number of iterations allowed for numerical ik computations.
*/

This comment has been minimized.

@davetcoleman

davetcoleman Jun 29, 2018

Member

Did you reference any of this documentation when writing ros-planning/moveit_tutorials#185?

This comment has been minimized.

@raghavendersahdev

raghavendersahdev Jul 1, 2018

Contributor

@davetcoleman yes I addressed these here, here and here . I think I should change these links to belong to the new moveit link of this file once this gets merged and open a small new PR for this... Or alternatively I could just link it to this file right now. But this would cause travis to fail.

num_iterations_after_valid: 0
num_rollouts: 10
max_rollouts: 100
initialization_method: 1 #[1 : LINEAR_INTERPOLATION, 2 : CUBIC_POLYNOMIAL, 3 : MININUM_CONTROL_COST

This comment has been minimized.

@davetcoleman

This comment has been minimized.

@raghavendersahdev
@@ -0,0 +1,19 @@
<launch>

This comment has been minimized.

@davetcoleman

davetcoleman Jun 29, 2018

Member

move this into the setup assistant template

This comment has been minimized.

@raghavendersahdev

raghavendersahdev Jul 5, 2018

Contributor

added !

@@ -0,0 +1,60 @@
stomp/manipulator:

This comment has been minimized.

@davetcoleman

davetcoleman Jun 29, 2018

Member

move this into the setup assistant template

This comment has been minimized.

@raghavendersahdev

raghavendersahdev Jul 5, 2018

Contributor

integrated this into the moveit setup assistant in my commit here

* limitations under the License.
*/
#ifndef INDUSTRIAL_MOVEIT_STOMP_MOVEIT_INCLUDE_STOMP_MOVEIT_COST_FUNCTIONS_COLLISION_CHECK_H_
#define INDUSTRIAL_MOVEIT_STOMP_MOVEIT_INCLUDE_STOMP_MOVEIT_COST_FUNCTIONS_COLLISION_CHECK_H_

This comment has been minimized.

@davetcoleman

davetcoleman Jun 29, 2018

Member

change to new package name

This comment has been minimized.

@raghavendersahdev

raghavendersahdev Jul 1, 2018

Contributor

changed this one to MOVEIT_STOMP_MOVEIT_INCLUDE_STOMP_MOVEIT_COST_FUNCTIONS_COLLISION_CHECK_H_ and also in all other header files.

@bmagyar

This comment has been minimized.

Copy link
Member

bmagyar commented Jun 30, 2018

Probably stating the obvious but the CI clearly fails due to stomp_core not being available. I don't think it would be too much of an overhaul to bring that here too. CHOMP also has a separation of planner and plugin, STOMP could follow the same setup.

@raghavendersahdev

This comment has been minimized.

Copy link
Contributor

raghavendersahdev commented Jul 1, 2018

Thanks @bmagyar , yes stomp_core can also be brought here if required ! @davetcoleman @mamoll

@v4hn

This comment has been minimized.

Copy link
Member

v4hn commented Jul 2, 2018

I'm confused.

Didn't we agree with @Levi-Armstrong @jrgnicho and @gavanderhoorn that the stomp planner is maintained in the separate ROS-I-maintained repository until they decide that they do not want to maintain it anymore?

This proposal would add a lot of new code to the repository which very few of the current maintainers ever looked at.

I fail to see the advantage in merging this.

@davetcoleman

This comment has been minimized.

Copy link
Member

davetcoleman commented Jul 4, 2018

Reasoning/Advantages:

  • Easier to use stomp with moveit and other planners
  • Easier to fix bugs if moveit API changes
  • We are only moving the moveit interface of stomp

But certainly I'd like to hear Levi's et al's opinion

@raghavendersahdev raghavendersahdev force-pushed the raghavendersahdev:stomp_planner branch 4 times, most recently from bfbc3c1 to 1f133f5 Jul 5, 2018

@raghavendersahdev raghavendersahdev force-pushed the raghavendersahdev:stomp_planner branch from 94bf5e9 to f860444 Aug 2, 2018

@raghavendersahdev raghavendersahdev force-pushed the raghavendersahdev:stomp_planner branch from f860444 to 2a0bb93 Aug 10, 2018

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