Pilz planner merge #1893

ct2034 · 2020-02-12T14:30:02Z

Description

Merging pilz trajectory generation into moveit as suggested here: #1800

Related PRs

merge before this:

merge after this:

Checklist

Required by CI: Code is auto formatted using clang-format
Extend the tutorials / documentation reference -> see PR
Document API changes relevant to the user in the MIGRATION.md notes
Create tests, which fail without this PR reference
Include a screenshot if changing a GUI
While waiting for someone to review your request, please help review another open pull request to support the maintainers

...it_planners/pilz_trajectory_generation/test/test_robots/abb_irb2400/launch/move_group.launch

to match moveit/moveit#1893

davetcoleman

Really excited to see this PR, thank you!

moveit_planners/trapezoidal/CMakeLists.txt

moveit_planners/trapezoidal/README.md

moveit_planners/trapezoidal/doc/MotionBlendAlgorithmDescription.tex

moveit_planners/trapezoidal/include/trapezoidal_trajectory_generation/cartesian_limit.h

moveit_planners/trapezoidal/package.xml

moveit_planners/trapezoidal/src/trajectory_functions.cpp

davetcoleman · 2020-02-27T18:40:34Z

Should the "WIP" be removed at this point?
Why is Travis still failing?

jschleicher · 2020-02-28T09:02:04Z

moveit/moveit_msgs#65 is required first. So travis currently is failing because of missing message(s).
PilzDE/pilz_robots#316 needs to be (at least partially) merged for a successful moveit build as well, see travis test build.

felixvd · 2020-03-10T09:27:38Z

~~I'm unclear on the name change?~~
You explained in the other PR.

jschleicher · 2020-03-19T10:32:35Z

Ready for review now @mamoll @v4hn.

Still moveit/moveit_msgs#65 needs to be merged first to make travis pass.

davetcoleman

Great integration tests!
Great documentation with latex!

I expanded all the files and skimmed them quickly with Page Down. There is so much code!

moveit_planners/pilz_industrial_motion_planner/CMakeLists.txt

moveit_planners/pilz_industrial_motion_planner/doc/MotionBlendAlgorithmDescription.fls

moveit_planners/pilz_industrial_motion_planner/package.xml

moveit_planners/trajopt/package.xml

rhaschke

I have seen some more files, I'm not sure that they should be merged here:

There are several image files: moveit_planners/pilz_industrial_motion_planner/test/acceptance_tests/img/acceptance_test_lin_img*.png
What is their role?
In moveit_planners/pilz_industrial_motion_planner/test/test_robots/frankaemika_panda you seem to have a copy of panda_moveit_config / moveit_resources/panda_moveit_config. Please use the latter one instead of introducing yet another copy here.
This PR includes moveit_planners/trajopt/package.xml, which is unrelated.

Generally, I'm not convinced, why this huge repo should be merged with the core MoveIt repo at all (@davetcoleman, can you please comment on this?)
Merging this here, I see the risk that it will become harder for Pilz to merge changes, because they need to pass our review process. On the other hand, Pilz will need to continue maintaining this code base, as I think no current MoveIt maintainer will sign to be responsible for this new code base.
I didn't actually look into the source files (because this PR is so huge), but only skimmed through the files on a very high level.

Apart from this, the planner is a great contribution and we should definitely work on increasing the visibility of this package. However, to increase visibility, it would suffice to provide tutorials and interlink with the remaining MoveIt documentation - which needs to be done anyway.

moveit_planners/pilz_industrial_motion_planner/package.xml

moveit_planners/pilz_industrial_motion_planner_testutils/package.xml

davetcoleman · 2020-04-23T16:41:21Z

@rhaschke the discussion on whether to add this to the moveit repo was already decided on in past maintainer meetings and here:
#1800

The benefits are outlined there. Something I didn't mention there but is also relevant: I don't see why the scope of MoveIt, as well as the scope of core ROS packages, must be limited to what was created at Willow Garage. I believe adding new major features and tightly integrating them into MoveIt, as we're doing here, is beneficial for consistent maintenance, usability, and discoverability.

Overall I think this PR is huge as we're importing a whole project, and our best approach to changes is to do follow up PRs for fixups.

Merging this here, I see the risk that it will become harder for Pilz to merge changes, because they need to pass our review process. On the other hand, Pilz will need to continue maintaining this code base, as I think no current MoveIt maintainer will sign to be responsible for this new code base.

For this reason we have plans here for the following:

Add @jschleicher as a MoveIt maintainer so he can continue to maintain this feature. I think he has proven his skills with the development of pilz_industrial_motion
Add @ct2034 as a MoveIt Core Contributor so he can also get more involved in maintenance

ct2034 · 2020-04-23T16:52:32Z

Thanks, @rhaschke for raising the point of making it harder for us to get our changes into the code. We have also had this discussion internally bu agreed that under the condition of us being maintainer/core contributor it will be manageable. While we expect some support in terms of maintenance we are of cause willing to do our fair share in terms of maintenance, just the same way we maintain all our other code.

I also want to apologize for the sometimes sloppy job with the inclusion of wrong files but it is also a big PR for us.

traversaro · 2020-04-23T17:21:28Z

moveit_planners/pilz_industrial_motion_planner/CMakeLists.txt

+## Add support for C++11, supported in ROS Kinetic and newer
+add_definitions(-std=c++11)
+add_definitions(-Wall)
+add_definitions(-Wextra)
+add_definitions(-Wno-unused-parameter)
+add_definitions(-Werror)


To maintain Windows compatibility, it probably make sense to only add this GCC/Clang-specific flags under an if, see for example https://github.com/ros-planning/moveit/blob/379acf91aa3630f320db77ba85b1ca294310b0bc/moveit_core/CMakeLists.txt#L8 .

cc @seanyen

could we do this in a separate PR afterwards?

could we do this in a separate PR afterwards?

I guess this is up to the maintainers, I just noticed this by looking up the (great) content added.

rhaschke · 2020-04-23T17:48:41Z

I'm fine to merge this PR in general, just wanted to raise some concerns, because I stumbled over the issue and repo links pointing to the old github repository. As far as I understand, Pilz will move maintenance to this repo only. Having @jschleicher and @ct2034 in the maintainer/contributor team would be great. @ct2034, could you please comment on the (seamingly) superfluous files mentioned in #1893 (review)?

added during testing

to copy the templates for auto-generated packages. Changing parameters via MSA-gui is left for future PR(s).

jschleicher · 2020-10-15T14:05:55Z

Thank you very much @henningkayser ! The latest build failure seems to be an unrelated docker issue.

I'd suggest a squash-merge.

henningkayser · 2020-10-15T16:11:34Z

...oveit_config_pkg_template/launch/pilz_industrial_motion_planner_planning_pipeline.launch.xml

+  <param name="planning_plugin" value="$(arg planning_plugin)" />
+  <param name="request_adapters" value="$(arg planning_adapters)" />
+  <param name="start_state_max_bounds_error" value="$(arg start_state_max_bounds_error)" />
+


Shouldn't this have the new capabilities, like here?

Yep, absolutely. To allow passing in user-defined capabilities as well, I'd suggest to pass the capabilities argument down to the included pipeline launch file(s) and append the planner-specific ones there: 99c059f

I'll re-generate the configuration inside moveit_resources based on this template.

...oveit_config_pkg_template/launch/pilz_industrial_motion_planner_planning_pipeline.launch.xml

…unch/pilz_industrial_motion_planner_planning_pipeline.launch.xml Co-authored-by: Henning Kayser <henningkayser@picknik.ai>

append the planner specific templates to the user-argument. Therefore we pass the user-defined capabilities down to the planning pipeline launch files via arguments.

davetcoleman · 2020-11-02T15:31:33Z

The RPBT PR is merged! I think this is ready to go.

@jschleicher would still be nice to improve usability for future users via @henningkayser's suggestion:

For that, please consider adding a template for the planning pipeline launch file and configs as requested here, or add corresponding TODO+issue and plan for a follow-up. The same applies for the new config cartesian_limits.yaml.

henningkayser · 2020-11-02T15:42:06Z

The RPBT PR is merged! I think this is ready to go.

@jschleicher would still be nice to improve usability for future users via @henningkayser's suggestion:

For that, please consider adding a template for the planning pipeline launch file and configs as requested here, or add corresponding TODO+issue and plan for a follow-up. The same applies for the new config cartesian_limits.yaml.

@davetcoleman Thanks for merging moveit_resources. Actually, the templates have already been added here.
@jschleicher please update the rosinstall so that we can merge this.

felixvd · 2020-11-02T15:51:45Z

Absolutely cannot wait to see this with multiple planning pipelines. Super excited for these features <3

ct2034 · 2020-11-03T10:41:27Z

Thank you very much. @henningkayser

gonzalocasas · 2020-11-03T10:41:58Z

🍾

* Add pilz_industrial_motion_planner to moveit_planners * Update license, document consent to this change by contributers * Fix formatting, code style, catkin_lint, clang-tidy * Add and update tests, use new prvt_moveit_config from moveit_resources * New codeowners for pilz_industrial_motion planner * Add pipeline configuration teimplates to MSA Co-authored-by: Joachim Schleicher <J.Schleicher@pilz.de> Co-authored-by: rfeistenauer <r.feistenauer@pilz.de> Co-authored-by: Giuseppe Sansone <g.sansone@pilz.de> Co-authored-by: Immanuel Martini <i.martini@pilz.de>

* Add Pilz industrial motion planner (#1893) * Add pilz_industrial_motion_planner to moveit_planners * Update license, document consent to this change by contributers * Fix formatting, code style, catkin_lint, clang-tidy * Add and update tests, use new prvt_moveit_config from moveit_resources * New codeowners for pilz_industrial_motion planner * Add pipeline configuration teimplates to MSA * Fix file not found error (adjust to melodic `moveit::core::PlanningScene::isEmpty()` functions) Co-authored-by: Christian Henkel <post@henkelchristian.de> Co-authored-by: Joachim Schleicher <J.Schleicher@pilz.de> Co-authored-by: rfeistenauer <r.feistenauer@pilz.de> Co-authored-by: Giuseppe Sansone <g.sansone@pilz.de>

Changing Planning Pipeline Reference to match moveit/moveit#1893

gavanderhoorn reviewed Feb 12, 2020

View reviewed changes

...it_planners/pilz_trajectory_generation/test/test_robots/abb_irb2400/launch/move_group.launch Outdated Show resolved Hide resolved

ct2034 added a commit to PilzDE/pilz_application_templates that referenced this pull request Feb 19, 2020

Changing Planning Pipeline Reference

de8e510

to match moveit/moveit#1893

ct2034 mentioned this pull request Feb 19, 2020

Changing Planning Pipeline Reference PilzDE/pilz_application_templates#15

Merged

2 tasks

davetcoleman reviewed Feb 20, 2020

View reviewed changes

This was referenced Feb 21, 2020

Remove outdated/superfluous documents PilzDE/pilz_industrial_motion#250

Merged

Adding messages for sequences moveit/moveit_msgs#65

Merged

jschleicher mentioned this pull request Feb 24, 2020

pilz_industrial_motion_planner moveit/moveit_tutorials#461

Merged

jschleicher reviewed Feb 24, 2020

View reviewed changes

moveit_planners/trapezoidal/src/trajectory_functions.cpp Outdated Show resolved Hide resolved

ct2034 mentioned this pull request Mar 6, 2020

Reenable Panda Test PilzDE/pilz_industrial_motion#176

Closed

ct2034 marked this pull request as ready for review March 12, 2020 13:09

ct2034 requested review from mamoll, rhaschke and v4hn as code owners March 12, 2020 13:09

ct2034 changed the title ~~WIP Pilz planner merge~~ Pilz planner merge Mar 12, 2020

ct2034 mentioned this pull request Mar 12, 2020

Print position limit error reason. PilzDE/pilz_industrial_motion#246

Closed

JeroenDM mentioned this pull request Mar 19, 2020

Integration of Descartes for Cartesian Path Planning #467

Open

7 tasks

v4hn added the planning Related the the motion planning stack (OMPL, CHOMP, etc.) label Mar 23, 2020

davetcoleman approved these changes Apr 16, 2020

View reviewed changes

ct2034 requested a review from davetcoleman April 20, 2020 12:53

This was referenced Apr 21, 2020

Convert tex to rst PilzDE/pilz_industrial_motion#259

Closed

Clean CMakeLists PilzDE/pilz_industrial_motion#260

Closed

rhaschke reviewed Apr 23, 2020

View reviewed changes

moveit_planners/pilz_industrial_motion_planner/package.xml Outdated Show resolved Hide resolved

moveit_planners/pilz_industrial_motion_planner_testutils/package.xml Outdated Show resolved Hide resolved

traversaro reviewed Apr 23, 2020

View reviewed changes

jschleicher and others added 3 commits October 13, 2020 08:09

remove notes

a97f489

added during testing

fix link

d707e21

add pipeline configuration files to MSA

9a18721

to copy the templates for auto-generated packages. Changing parameters via MSA-gui is left for future PR(s).

henningkayser reviewed Oct 15, 2020

View reviewed changes

...oveit_config_pkg_template/launch/pilz_industrial_motion_planner_planning_pipeline.launch.xml Outdated Show resolved Hide resolved

ct2034 and others added 4 commits October 15, 2020 22:15

Update moveit_setup_assistant/templates/moveit_config_pkg_template/la…

f389a89

…unch/pilz_industrial_motion_planner_planning_pipeline.launch.xml Co-authored-by: Henning Kayser <henningkayser@picknik.ai>

add pilz capabilities to templates

99c059f

append the planner specific templates to the user-argument. Therefore we pass the user-defined capabilities down to the planning pipeline launch files via arguments.

add option execution_type for fake_execution

a4527bc

simplify panda tests using new moveit_resources files

a0f3c65

using ressources master

d38d1a9

henningkayser approved these changes Nov 3, 2020

View reviewed changes

henningkayser merged commit cf21887 into moveit:master Nov 3, 2020

matteo-pacher mentioned this pull request Nov 3, 2020

Test and add examples of Pilz motion planner usage on ROS Noetic / MoveIt compas-dev/compas_fab#204

Open

4 tasks

jschleicher mentioned this pull request Nov 10, 2020

Changing references to pilz_command_planner into pilz_industrial_motion_planner in moveit PilzDE/pilz_robots#316

Closed

3 tasks

rhaschke mentioned this pull request Nov 21, 2020

Noetic changelogs and version bump (1.1.2) #2423

Closed

jschleicher mentioned this pull request Jan 8, 2021

MoveGroupSequence: find out which source command the trajectory output belongs to PilzDE/pilz_industrial_motion#319

Closed

martiniil mentioned this pull request Feb 3, 2021

[backport] Pilz planner merge #2507

Merged

6 tasks

tylerjw mentioned this pull request Apr 9, 2021

Noetic Release 1.1.2 #2593

Closed

v4hn mentioned this pull request Apr 14, 2021

Let users override fake execution type from demo.launch #2602

Merged

tylerjw mentioned this pull request Apr 29, 2021

Melodic Backports #2630

Merged

gavanderhoorn mentioned this pull request Jun 21, 2021

melodic: 'param' unexpected in 'include' in move_group.launch #2728

Closed

martiniil pushed a commit to PilzDE/pilz_application_templates that referenced this pull request Jul 22, 2021

Changing Planning Pipeline Reference (#15)

9196598

Changing Planning Pipeline Reference to match moveit/moveit#1893

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pilz planner merge #1893

Pilz planner merge #1893

ct2034 commented Feb 12, 2020 •

edited

davetcoleman left a comment

davetcoleman commented Feb 27, 2020

jschleicher commented Feb 28, 2020

felixvd commented Mar 10, 2020 •

edited

jschleicher commented Mar 19, 2020

davetcoleman left a comment

rhaschke left a comment

davetcoleman commented Apr 23, 2020

ct2034 commented Apr 23, 2020

traversaro Apr 23, 2020

ct2034 Apr 23, 2020

traversaro Apr 23, 2020

rhaschke commented Apr 23, 2020

jschleicher commented Oct 15, 2020

henningkayser Oct 15, 2020

jschleicher Oct 16, 2020

davetcoleman commented Nov 2, 2020

henningkayser commented Nov 2, 2020

felixvd commented Nov 2, 2020

ct2034 commented Nov 3, 2020

gonzalocasas commented Nov 3, 2020

Pilz planner merge #1893

Pilz planner merge #1893

Conversation

ct2034 commented Feb 12, 2020 • edited

Description

Related PRs

merge before this:

merge after this:

Checklist

davetcoleman left a comment

Choose a reason for hiding this comment

davetcoleman commented Feb 27, 2020

jschleicher commented Feb 28, 2020

felixvd commented Mar 10, 2020 • edited

jschleicher commented Mar 19, 2020

davetcoleman left a comment

Choose a reason for hiding this comment

rhaschke left a comment

Choose a reason for hiding this comment

davetcoleman commented Apr 23, 2020

ct2034 commented Apr 23, 2020

traversaro Apr 23, 2020

Choose a reason for hiding this comment

ct2034 Apr 23, 2020

Choose a reason for hiding this comment

traversaro Apr 23, 2020

Choose a reason for hiding this comment

rhaschke commented Apr 23, 2020

jschleicher commented Oct 15, 2020

henningkayser Oct 15, 2020

Choose a reason for hiding this comment

jschleicher Oct 16, 2020

Choose a reason for hiding this comment

davetcoleman commented Nov 2, 2020

henningkayser commented Nov 2, 2020

felixvd commented Nov 2, 2020

ct2034 commented Nov 3, 2020

gonzalocasas commented Nov 3, 2020

ct2034 commented Feb 12, 2020 •

edited

felixvd commented Mar 10, 2020 •

edited