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 gazebo launch file #936

Merged
merged 1 commit into from
Aug 9, 2018

Conversation

mayman99
Copy link
Contributor

@mayman99 mayman99 commented Jun 3, 2018

Description

Added a gazebo launch file template for the setup assistant.
For the ur10 robot it looks like that.

Checklist

  • Required: Code is auto formatted using clang-format
  • Extended the tutorials / documentation, if necessary reference
  • Include a screenshot if changing a GUI
  • Optional: Created tests, which fail without this PR reference
  • Optional: Decide if this should be cherry-picked to other current ROS branches (Indigo, Jade, Kinetic)

@mayman99 mayman99 mentioned this pull request Jun 4, 2018
@davetcoleman
Copy link
Member

Before we merge this can you create a new tutorial that explains how to launch this new file, complete with screenshots of the Franka Emika robot?

https://github.com/ros-planning/moveit_tutorials/

<!-- push robot_description to factory and spawn robot in gazebo -->
<node name="spawn_gazebo_model" pkg="gazebo_ros" type="spawn_model" args="-urdf -param robot_description -model robot -z 0.1" respawn="false" output="screen" />

<include file="$(find [GENERATED_PACKAGE_NAME])/launch/ros_controllers.launch"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the ros_controllers.launch file supposed to be generated automatically as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, ros_controllers.launch can now be generated automatically by the setup assistant.

@mayman99
Copy link
Contributor Author

mayman99 commented Jun 9, 2018 via email

@mayman99
Copy link
Contributor Author

mayman99 commented Jun 9, 2018 via email

file.file_name_ = "gazebo.launch";
file.rel_path_ = config_data_->appendPaths(launch_path, file.file_name_);
template_path = config_data_->appendPaths(template_launch_path, "gazebo.launch");
file.description_ = "gazebo launch file";
Copy link
Member

Choose a reason for hiding this comment

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

Please capitalize this description and explain in a sentence what it does, see the other examples in this file.

Also apply the same change to the ros_controllers.launch you added

@@ -0,0 +1,24 @@
<?xml version="1.0"?>
<launch>
<arg name="limited" default="false"/>
Copy link
Member

Choose a reason for hiding this comment

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

can you add a comment here about what limited does?

Copy link
Contributor

Choose a reason for hiding this comment

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

This sounds a lot like the limited arg that some of the universal_robot launch files have/use.

I don't believe it should be part of a template file introduced by this contribution.

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 did remove it as @gavanderhoorn suggestion makes sense.

<!-- send robot urdf to param server -->
<param unless="$(arg limited)" name="robot_description" [URDF_LOAD_ATTRIBUTE] />

<param if="$(arg limited)" name="robot_description" [URDF_LOAD_ATTRIBUTE] />
Copy link
Member

Choose a reason for hiding this comment

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

these two lines do the exact same thing, so the limited variable should be removed or the URDF_LOAD_ATTRIBUTE should change for some reason

<param if="$(arg limited)" name="robot_description" [URDF_LOAD_ATTRIBUTE] />

<!-- push robot_description to factory and spawn robot in gazebo -->
<node name="spawn_gazebo_model" pkg="gazebo_ros" type="spawn_model" args="-urdf -param robot_description -model robot -z 0.1" respawn="false" output="screen" />
Copy link
Member

Choose a reason for hiding this comment

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

please break this line into two (line too long - max is 120)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review, will fix the changes.

Btw I got to get franka to be spawned in gazebo, but I had to do some changes to the urdf file first. What would you recommend, do we make the changes that makes a urdf model gazebo combitable a part of this PR? Or do we better make it a separate one and make this one just for the launch file?

Copy link
Member

Choose a reason for hiding this comment

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

please move this discussion to the main issue or the gdoc, it does not belong inline with a PR code review :-)

@mayman99
Copy link
Contributor Author

@davetcoleman
Thanks for the review, will fix the changes.

Btw I got to get franka to be spawned in gazebo, but I had to do some changes to the urdf file first. What would you recommend, do we make the changes that makes a urdf model gazebo combitable a part of this PR? Or do we better make it a separate one and make this one just for the launch file?

@mayman99
Copy link
Contributor Author

mayman99 commented Jul 3, 2018

As @davetcoleman and @gavanderhoorn suggested,

gazebo.launch is no longer automatically generated, the simulation screen should be responsible to trigger its generation through a checkbox.

@mayman99 mayman99 mentioned this pull request Jul 9, 2018
6 tasks
<param name="robot_description" [URDF_LOAD_ATTRIBUTE] />

<!-- push robot_description to factory and spawn robot in gazebo -->
<node name="spawn_gazebo_model" pkg="gazebo_ros" type="spawn_model" args="-urdf -param robot_description -model robot -z 0.1" respawn="false" output="screen" />
Copy link
Member

Choose a reason for hiding this comment

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

do you understand why -z 0.1 is used?

i believe this may be a custom value for a particular robot that is not very generic. please add a comment in this launch file explaining the purpose of z (changing the start location of the robot). i think it should default to 0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes that is right, 0 as default makes better sense, thanks.

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 it, now robot spawn at the origin.

Copy link
Member

Choose a reason for hiding this comment

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

Actually can you leave the -z option there but set it to zero and document its usage?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we leave the z, shouldnt we also leave the -x and -y?

Copy link
Member

Choose a reason for hiding this comment

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

great idea!

@davetcoleman
Copy link
Member

@mlautman please review

@davetcoleman davetcoleman merged commit a08d75c into moveit:kinetic-devel Aug 9, 2018
davetcoleman pushed a commit that referenced this pull request Aug 9, 2018
@davetcoleman
Copy link
Member

Cherry-picked to melodic

mayman99 added a commit to mayman99/moveit that referenced this pull request Aug 25, 2018
pull bot pushed a commit to shadow-robot/moveit that referenced this pull request Sep 3, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants