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

added STOMP planner in moveit_planners #965

Closed

Conversation

raghavendersahdev
Copy link
Contributor

@raghavendersahdev 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
Copy link
Contributor Author

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

Copy link
Member

@davetcoleman davetcoleman left a comment

Choose a reason for hiding this comment

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

will review further once travis passes

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

Choose a reason for hiding this comment

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

- 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.
*/

Copy link
Member

Choose a reason for hiding this comment

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

Did you reference any of this documentation when writing moveit/moveit_tutorials#185?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

]

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@@ -0,0 +1,19 @@
<launch>
Copy link
Member

Choose a reason for hiding this comment

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

move this into the setup assistant template

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added !

@@ -0,0 +1,60 @@
stomp/manipulator:
Copy link
Member

Choose a reason for hiding this comment

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

move this into the setup assistant template

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Choose a reason for hiding this comment

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

change to new package name

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

@bmagyar
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
Copy link
Contributor Author

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

@v4hn
Copy link
Contributor

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

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

@henningkayser
Copy link
Member

@raghavendersahdev @davetcoleman @v4hn what's the current state on this?.
Apparently STOMP should fully remain in industrial_moveit or in a separate repo in the future.
Porting only the smoothing adapter or the moveit interface would add dependencies to stomp_core which need to be handled so I don't see the advantage in separating the packages.

@davetcoleman
Copy link
Member

I think @gavanderhoorn might have the latest update regarding creating a standalone repo to host stomp within ros-i...

@raghavendersahdev
Copy link
Contributor Author

I guess its waiting to get more info about the stomp repo depending on what ROS-industrial says . . I can resolve this as soon as we finalize how we want to have STOMP with MoveIt and Stomp core package..

This PR 965 is superseded / overridden by this PR 1081 which has the STOMP planning adapter also incorporated..

@jrgnicho
Copy link
Contributor

jrgnicho commented Mar 8, 2019

I think at this point all we need is a repository to put the stomp packages on (stomp_core, stomp_moveit and stomp_plugins). I'm not sure if the "stomp_test..." packages should go there as well since there's only one stomp specific yaml file that demonstrates how to configure the stomp planner.

@bmagyar
Copy link
Member

bmagyar commented Mar 10, 2019

If there's interest I can add one for PAL's Tiago and the rob@work (RaW) Fraunhofer robots

@jrgnicho
Copy link
Contributor

@bmagyar that would be nice, you (or anyone interested) may wanna join the ongoing discussion in this PR

@v4hn
Copy link
Contributor

v4hn commented Aug 21, 2019

Obsolete since @gavanderhoorn created the stomp_ros repository some time ago.

@v4hn v4hn closed this Aug 21, 2019
JafarAbdi pushed a commit to JafarAbdi/moveit that referenced this pull request Mar 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants