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 arguments to launch file templates #1397

Merged
merged 4 commits into from
Mar 26, 2019

Conversation

jschleicher
Copy link
Contributor

@jschleicher jschleicher commented Mar 14, 2019

If one installed a moveit_config package, there should be enough
arguments, to use move_group.launch. Otherwise every user of the robot
would have to fork the robot package.

This commit adds some arguments that allow an application package to build
onto a robot's moveit_config:

  • load_robot_description - if set to false, one can load a modified
    robot model that contains e.g. additional fixed objects
    (see pilz_robots tutorial for an example use-case)
  • pipeline - allows to choose between different planning pipelines
    (minimal version of Allow pipeline to be loaded from another package #1025)
  • rviz config_file - path to config file is way more flexible than
    defaulting to an empty setting. Especially since the
    save config as default option was dropped in RViz.

Checklist

  • Document API changes relevant to the user in the moveit/MIGRATION.md notes
  • I'd suggest to cherry-pick this to melodic-devel

@welcome
Copy link

welcome bot commented Mar 14, 2019

Thanks for helping in improving MoveIt!

Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

Thanks a lot for this improvement. This was a long-standing shortcoming.

@rhaschke rhaschke added the awaits 2nd review one maintainer approved this request label Mar 19, 2019
jschleicher added a commit to PilzDE/pilz_robots that referenced this pull request Mar 20, 2019
According to @rfeistenauer it is preferred to hava a rviz-config in
an application package and not just a user-default.
(See moveit/moveit#1397)

Re-generated demo.launch from upstream template.
Copy link
Contributor

@rhaschke rhaschke left a comment

Choose a reason for hiding this comment

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

Thanks. LGTM.

@@ -1,5 +1,8 @@
<launch>

<!-- specify the planning pipeline -->
<arg name="pipeline" default="ompl" />
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a little bit philosophical: While the tutorials introduce the argument planner, here the argument pipeline is introduced for the very same purpose.
I vote for keeping pipeline because this better indicates that you could have different pipeline configs for the same planner plugin, e.g. ompl1 and ompl2. However, a follow-up PR should be filed for the tutorials to align them.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with keeping pipeline, is planner used in this case anywhere else but the tutorials?

Copy link
Member

Choose a reason for hiding this comment

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

I love long variable names and would even suggest planning_pipeline, but that's for another PR

@jschleicher do you mind updating the tutorials per @rhaschke ?
https://github.com/ros-planning/moveit_tutorials

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not a panda expert; updating the tutorials should also re-generate the panda_moveit_config. Opened moveit/moveit_tutorials#300 as follow-up-issue.

Copy link
Contributor

@BryceStevenWilley BryceStevenWilley left a comment

Choose a reason for hiding this comment

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

LGTM; very useful change, I've definitely made it in my own robot packages.

@@ -1,5 +1,8 @@
<launch>

<!-- specify the planning pipeline -->
<arg name="pipeline" default="ompl" />
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with keeping pipeline, is planner used in this case anywhere else but the tutorials?

@BryceStevenWilley
Copy link
Contributor

Looks like the clang-tidy-fix job timed out because this build was just incredibly slow.

jschleicher added a commit to PilzDE/pilz_robots that referenced this pull request Mar 20, 2019
According to @rfeistenauer it is preferred to hava a rviz-config in
an application package and not just a user-default.
(See moveit/moveit#1397)

Re-generated demo.launch from upstream template.
@@ -1,5 +1,8 @@
<launch>

<!-- specify the planning pipeline -->
<arg name="pipeline" default="ompl" />
Copy link
Member

Choose a reason for hiding this comment

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

I love long variable names and would even suggest planning_pipeline, but that's for another PR

@jschleicher do you mind updating the tutorials per @rhaschke ?
https://github.com/ros-planning/moveit_tutorials

<arg unless="$(arg config)" name="command_args" value="" />
<arg if="$(arg config)" name="command_args" value="-d $(find [GENERATED_PACKAGE_NAME])/launch/moveit.rviz" />
<arg name="rviz_config" default="" />
<arg if="$(eval rviz_config=='')" name="command_args" value="" />
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<arg if="$(eval rviz_config=='')" name="command_args" value="" />
<arg if="$(eval rviz_config=='')" name="command_args" value="-d $(find [GENERATED_PACKAGE_NAME])/launch/moveit.rviz" />

We want to keep the default behavior

Copy link
Contributor Author

@jschleicher jschleicher Mar 21, 2019

Choose a reason for hiding this comment

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

I'd put the default two lines above the line you referenced: Imho it should still be possible, to set an empty config and pass nothing to rviz (which currently loads ~/.rviz/default.rviz). Don't you think so? The former default is still set in demo.launch:52.

What about setting

<arg name="rviz_config" default="$(find [GENERATED_PACKAGE_NAME])/launch/moveit.rviz" />

and add a rviz_config == 'none' option similar to what @rhaschke suggested above?

If I just add this default, it isn't possible to roslaunch pkg moveit_rviz.launch rviz_config:="" since this auto-sets the default and doesn't preserve the empty string.

Copy link
Contributor

Choose a reason for hiding this comment

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

The approach suggested in #1397 (comment) should handle all cases, including the default:

  • In moveit_rviz.launch the default of config=False would be kept.
  • demo.launch overrides this default by passing the package's moveit.rviz.
  • Launching moveit_rviz.launch with an explicit file argument for config, will load another rviz config.

As far as I can see, you didn't exposed the rviz_config / config argument from demo.launch.

Copy link
Contributor Author

@jschleicher jschleicher Mar 21, 2019

Choose a reason for hiding this comment

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

@rhaschke @davetcoleman The initial and current suggestion does not change default behaviour:

  • The former default False in moveit_rviz.launch did not pass any argument to rviz; same as the default rviz_config=='' now.
  • The default in demo.launch was to overwrite the argument with True to set the packages rviz-config which is the same now.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed. I think, @davetcoleman comment in #1397 (comment), refers to not change the semantics of the variable "config", which was previously available to enable/disable the package's rviz config.

Copy link
Member

Choose a reason for hiding this comment

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

ah i see, my bad.

@jschleicher jschleicher requested a review from v4hn as a code owner March 21, 2019 13:14
If one has an installed moveit_config package, there should be enough
arguments, to use move_group.launch. Otherwise every user of the robot
would have to fork the robot package.

This commit adds some options that allow an application package to build
onto a robot's moveit_config:

* load_robot_description - if set to false, one can load a modified
  robot model that contains e.g. additional fixed objects
* pipeline - allows to choose between different planning pipelines
  (minimal version of moveit#1025)
* rviz config_file - path to config file is way more flexible than
  defaulting to an empty setting. Especially since the
  `save config as default` option was dropped in RViz.
@jschleicher
Copy link
Contributor Author

Rebased onto current master.
@davetcoleman Pleas re-review.
@v4hn The auto-assignment requested another review; I dont' think, that is necessary...

MIGRATION.md Outdated Show resolved Hide resolved
Co-Authored-By: jschleicher <j.schleicher@pilz.de>
<arg unless="$(arg config)" name="command_args" value="" />
<arg if="$(arg config)" name="command_args" value="-d $(find [GENERATED_PACKAGE_NAME])/launch/moveit.rviz" />
<arg name="rviz_config" default="" />
<arg if="$(eval rviz_config=='')" name="command_args" value="" />
Copy link
Member

Choose a reason for hiding this comment

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

ah i see, my bad.

@davetcoleman davetcoleman merged commit 261a0c4 into moveit:master Mar 26, 2019
@welcome
Copy link

welcome bot commented Mar 26, 2019

Congrats on getting your first MoveIt! pull request merged and improving open source robotics!

rhaschke pushed a commit to ubi-agni/moveit that referenced this pull request May 11, 2019
* add arguments to launch file templates

If one has an installed moveit_config package, there should be enough
arguments, to use move_group.launch. Otherwise every user of the robot
would have to fork the robot package.

This commit adds some options that allow an application package to build
onto a robot's moveit_config:

* load_robot_description - if set to false, one can load a modified
  robot model that contains e.g. additional fixed objects
* pipeline - allows to choose between different planning pipelines
  (minimal version of moveit#1025)
* rviz config_file - path to config file is way more flexible than
  defaulting to an empty setting. Especially since the
  `save config as default` option was dropped in RViz.

* rename argument to rviz_config

* add rviz_config argument to MIGRATION.md

* Update MIGRATION.md

Co-Authored-By: jschleicher <j.schleicher@pilz.de>
ipa-kut added a commit to ipa-kut/moveit that referenced this pull request Dec 3, 2019
…unch/demo_gazebo.launch


The new argument requires a config file instead of boolean value. See moveit#1397.

Co-Authored-By: jschleicher <j.schleicher@pilz.de>
sjahr pushed a commit to sjahr/moveit that referenced this pull request Jun 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
awaits 2nd review one maintainer approved this request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants