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

Simulation Screen for the SA #956

Merged

Conversation

Projects
None yet
4 participants
@MohmadAyman
Copy link
Member

commented Jun 20, 2018

Description

This PR is a part of improving the integration between gazebo, moveit and ros issue 894.

For a robot to be rendered in gazebo it has to has an inertial element for each link, this PR tells the user if the urdf didn't contain them.
and views the new urdf on the simulation window for the user.


Added elements which the user should add to his urdf are colored in green.

Once this PR is merged, the [gazebo.launch](#936) will point to this new urdf.

The documentation of the setup assistant are updated in this PR.

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)
@gavanderhoorn

This comment has been minimized.

Copy link
Member

commented Jun 21, 2018

Is this really something we want to do?

Seems rather invasive for the setup assistant to go in and change a urdf. It also seems to use default (?) values, which may not necessarily work.

@MohmadAyman

This comment has been minimized.

Copy link
Member Author

commented Jun 21, 2018

Well once the new controllers window is added, the gazebo_ros_control plugin would work without having to do anything manually, that is the plan.

For the inertia values, we tell the user that those default inertial values are defaults and they can edit them if they wanted to. and later we should be able to calculate the inertia of simple shapes like ignition-math does here .

@gavanderhoorn

This comment has been minimized.

Copy link
Member

commented Jun 21, 2018

Personally I think I'd like a checkbox somewhere to make this configurable. That would at least leave it up to the user to decide whether he wants the MSA changing his URDF or not.

And something else I realised: what if my robot is actually modelled using xacro? The MSA converts that to URDF, but that URDF will not be used by the launch files. Saving a new URDF with the added inertial elements does not make sense in those cases.

Tbh, I would think that providing a proper URDF to the MSA is the responsibility of the user. The MSA setting up the required configuration files for gazebo_ros_control to work is something different. We don't add gazebo_ros_control elements to the URDF either, do we?

@MohmadAyman

This comment has been minimized.

Copy link
Member Author

commented Jun 21, 2018

A check box sounds good 👍

The newly created urdf file will be added to a new directory in the config package, and will be referenced only by the gazebo.launch that we are adding in PR#936, so there is no harm done to the user, if he wants to simulate in gazebo, they will have a urdf ready for that.

I'm planning to add the gazebo_ros_control elements to the urdf once the ros controllers PR is merged.

@gavanderhoorn

This comment has been minimized.

Copy link
Member

commented Jun 21, 2018

@MohmadAyman wrote:

The newly created urdf file will be added to a new directory in the config package, and will be referenced only by the gazebo.launch that we are adding in PR#936, so there is no harm done to the user, if he wants to simulate in gazebo, they will have a urdf ready for that.

forgive me if I misunderstand, but that would mean that the MSA must be rerun for every change made to the xacro backing the urdf then, correct? That seems like quite a big change and rather expensive.

I'm planning to add the gazebo_ros_control elements to the urdf once the ros controllers PR is merged.

As I commented earlier in #894, I'm not aware of what the GSoC assignment / description of work contained specifically, but the direction you're taking this in seems to increase coupling between MoveIt-MSA and Gazebo quite a bit. As Gazebo is just one of the simulators that can be used with MoveIt, is that desirable?

@MohmadAyman MohmadAyman referenced this pull request Jun 21, 2018

Closed

Setup Assistant 2 #894

6 of 6 tasks complete
@MohmadAyman

This comment has been minimized.

Copy link
Member Author

commented Jun 21, 2018

@gavanderhoorn

forgive me if I misunderstand, but that would mean that the MSA must be rerun for every change made to the xacro backing the urdf then, correct? That seems like quite a big change and rather expensive.

Correct, If the xacro were to be changed often, then yes this would be very expensive, but that doesn't happen often, right?

As I commented earlier in #894, I'm not aware of what the GSoC assignment / description of work contained specifically, but the direction you're taking this in seems to increase coupling between MoveIt-MSA and Gazebo quite a bit. As Gazebo is just one of the simulators that can be used with MoveIt, is that desirable?

The main objective of this project, as was discussed with @davetcoleman before GSoC, as mentioned here on the GSoC website, was to improve the integration between gazebo and moveit!.

I do agree with you we shouldn't increase the coupling/dependency between gazebo and moveit!. What we are doing should be something extra for the setup assistant, the idea is to make simulating a robot in gazebo while using moveit! a more automated process through the setup assistant. As gazebo maybe the most popular simulator used with ros, this should benefit a lot of users.

I can't forget to mention your comment on the main issue to make the SA plugin based, which sounds very cool and I guess everyone would be thrilled by that. In the future this extra optional step for simulation could be just a plugin.

@gavanderhoorn

This comment has been minimized.

Copy link
Member

commented Jun 21, 2018

@MohmadAyman wrote:

Correct, If the xacro were to be changed often, then yes this would be very expensive, but that doesn't happen often, right?

My workflow while developing can include quite some round-trip editing of xacros, which with this PR would now have to include the MSA as well. But I may not be representative of typical usage.

@davetcoleman

This comment has been minimized.

Copy link
Member

commented Jun 21, 2018

@gavanderhoorn I think you're bringing up some great points that I've been struggling with also. I totally agree we should have a checkbox for generating the suggested Gazebo tags and we should educate the user very carefully about what is happening behind scenes.

I'm uncomfortable with having Gazebo use a different URDF also. Rather, I'd like the MSA to provide to the user suggested XML to add to their URDF/XACO, but that step must be manual. We'd link out to a help page that explains the details of that.

@MohmadAyman

This comment has been minimized.

Copy link
Member Author

commented Jun 21, 2018

@gavanderhoorn
Well I've the idea of making this a part of xacros package itself, by maybe adding an option called -gazebo_compatible or -add_inertia, this would then be as easy as running a command.

@davetcoleman
It is not suggested to either create a new urdf, and we can't edit the existing one in most cases. So if we are going to apply the suggestion of promoting the user with the xml that they should add to their urdf, that does probably mean we would need a new screen, a simulation screen, do you suggest another way?

@davetcoleman

This comment has been minimized.

Copy link
Member

commented Jun 21, 2018

we can't edit the existing one in most cases.

That's not necessarily true. We just have to educate the user that a debian package can be replaced with a local git clone checkout. This education can take place outside of the MSA on a help page.

that does probably mean we would need a new screen, a simulation screen

Yes, I think that might be needed.

@MohmadAyman

This comment has been minimized.

Copy link
Member Author

commented Jun 22, 2018

that does probably mean we would need a new screen, a simulation screen

Yes, I think that might be needed.

Then I will rename this PR to simulation screen, and instead of writing a new urdf, we just show the user what elements they need to add to their urdf and where.

I'll start drawing a mock-up to add it to the description.

@MohmadAyman

This comment has been minimized.

Copy link
Member Author

commented Jun 23, 2018

This is the mock-up

Do we continue the discussion about the Simulation screen here, or do we open an issue for that?
@davetcoleman

@davetcoleman

This comment has been minimized.

Copy link
Member

commented Jun 25, 2018

I'd rather have as few text boxs as necessary for the screen. Can we have just one large one that creates the new URDF, but then the user has to copy into the actual file location?

@MohmadAyman MohmadAyman force-pushed the MohmadAyman:urdf_gazebo_compatible branch 2 times, most recently from bdabd6a to 910e9ef Jun 29, 2018

@MohmadAyman MohmadAyman changed the title urdf gazebo compatible Simulation Screen for the SA Jun 29, 2018

@MohmadAyman MohmadAyman force-pushed the MohmadAyman:urdf_gazebo_compatible branch 3 times, most recently from cc6c8d2 to f176317 Jun 29, 2018

@MohmadAyman

This comment has been minimized.

Copy link
Member Author

commented Jun 29, 2018

description updated.

@MohmadAyman MohmadAyman force-pushed the MohmadAyman:urdf_gazebo_compatible branch from f176317 to 9516c9e Jul 2, 2018

@MohmadAyman MohmadAyman referenced this pull request Jul 3, 2018

Merged

Added gazebo launch file #936

0 of 5 tasks complete

@MohmadAyman MohmadAyman force-pushed the MohmadAyman:urdf_gazebo_compatible branch from 9516c9e to 11c2afa Jul 9, 2018

@MohmadAyman MohmadAyman force-pushed the MohmadAyman:urdf_gazebo_compatible branch 3 times, most recently from 54d47a2 to 11c2afa Jul 24, 2018

catch (YAML::ParserException& e) // Catch errors
{
ROS_ERROR_STREAM(e.what());
return "";

This comment has been minimized.

Copy link
@mcevoyandy

mcevoyandy Jul 25, 2018

I think this should be return std::string();

return printer.CStr();
}

return "";

This comment has been minimized.

Copy link
@mcevoyandy

mcevoyandy Jul 25, 2018

return std::string();?

This comment has been minimized.

Copy link
@MohmadAyman

MohmadAyman Jul 25, 2018

Author Member

I agree it seems be better to cast them to std::string.


// Top Header Area ------------------------------------------------

HeaderWidget* header = new HeaderWidget("Simulation", "Shows what changes that should be made for the robot urdf to "

This comment has been minimized.

Copy link
@mcevoyandy

mcevoyandy Jul 25, 2018

change ... changes that should ... to ... changes should ...

"be spawned and rendered "
"correctly in gazebo simulator. "
" Green colored text are the changes.",
this);

This comment has been minimized.

Copy link
@mcevoyandy

mcevoyandy Jul 25, 2018

Could be shorter "The URDF changes needed for Gazebo compatibility are shown in green text."

@MohmadAyman MohmadAyman force-pushed the MohmadAyman:urdf_gazebo_compatible branch from 9b261a0 to d471ec9 Jul 25, 2018

@MohmadAyman MohmadAyman force-pushed the MohmadAyman:urdf_gazebo_compatible branch from d471ec9 to b46e7d2 Jul 25, 2018

* added
* \ return true if parsed into string with no errors
*/
bool urdfGazeboCompatible();

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Jul 27, 2018

Member

add a verb to this function name, e.g. makeURDFGazeboCompatibile

/**
* \brief Parses the existing urdf and constructs a string from it with the elements required by gazebo simulator
* added
* \ return true if parsed into string with no errors

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Jul 30, 2018

Member

it does not return true, it returns an empty string...

for (TiXmlElement* link_element = urdf_document.RootElement()->FirstChildElement(); link_element != NULL;
link_element = link_element->NextSiblingElement())
{
if (((std::string)link_element->Value()).find("link") != std::string::npos)

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Jul 30, 2018

Member

make this if statement be == and then continue, to prevent deeply nestled if statements

if (link_element->FirstChildElement("inertial") == NULL && link_element->FirstChildElement("collision") != NULL)
{
new_urdf_needed = true;
TiXmlElement inertial("inertial");

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Jul 30, 2018

Member

inertial vs inertia is very confusing. instead say inertia_link vs inertia_joint

inertia.SetAttribute("ixz", "0.0");
inertia.SetAttribute("iyz", "0.0");

inertial.InsertEndChild(mass);

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Jul 30, 2018

Member

for "mass" and "inertial" you can construct those directly in the insert function:

inertial.InsertEndChild(TiXmlElement("mass"));

This comment has been minimized.

Copy link
@MohmadAyman

MohmadAyman Jul 31, 2018

Author Member

If mass as TiXmlElement were to be constructed directly in the insert function, I can't think of a simple way to add attributes to it afterwards.

}
catch (YAML::ParserException& e) // Catch errors
{
ROS_ERROR_STREAM(e.what());

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Jul 30, 2018

Member

add _NAMED

@MohmadAyman MohmadAyman force-pushed the MohmadAyman:urdf_gazebo_compatible branch from b46e7d2 to 1dc8b34 Jul 31, 2018

@MohmadAyman

This comment has been minimized.

Copy link
Member Author

commented Jul 31, 2018

@davetcoleman
Addressed the comments, thanks for the review.

@davetcoleman
Copy link
Member

left a comment

please rebase and fix conflicts

@davetcoleman

This comment has been minimized.

Copy link
Member

commented Jul 31, 2018

@mcevoyandy please review

@MohmadAyman MohmadAyman force-pushed the MohmadAyman:urdf_gazebo_compatible branch from 1dc8b34 to be588a1 Aug 5, 2018

/**
* \brief Parses the existing urdf and constructs a string from it with the elements required by gazebo simulator
* added
* \return std::string - gazebo compatible urdf or empty if error encountered

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Aug 5, 2018

Member

no need to duplicate the "std::string - " part, you can remove that

}
catch (YAML::ParserException& e) // Catch errors
{
ROS_ERROR_STREAM_NAMED("Setup Assistant", e.what());

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Aug 5, 2018

Member

"Setup Assistant" -> "moveit_config_data"

The ROS logging will already tell users what package the log is coming from, but its helpful to add the filename to the logging namespace


HeaderWidget* header =
new HeaderWidget("Simulation With Gazebo", "The following tool will auto-generate the URDF changes needed "
"for Gazebo compatibility with ROS-Control and MoveIt!. The "

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Aug 5, 2018

Member

no dash needed between "ROS-Control"

void SimulationWidget::generateURDFClick()
{
simulation_text_->setVisible(true);
std::string gazebo_compatible_urdf_string_ = config_data_->getGazeboCompatibleURDF();

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Aug 5, 2018

Member

local variables should not end with underscore "_", only class member variables

inertial_closing_matches.push_back(it->position());
}

for (size_t i = 0; i < inertial_opening_matches.size(); i++)

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Aug 5, 2018

Member

std::size_t

// ******************************************************************************************
void SimulationWidget::focusGiven()
{
}

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Aug 5, 2018

Member

if blank, please remove

/* Author: Mohamad Ayman */

#ifndef MOVEIT_MOVEIT_SETUP_ASSISTANT_WIDGETS_SIMULATION_WIDGET_
#define MOVEIT_MOVEIT_SETUP_ASSISTANT_WIDGETS_SIMULATION_WIDGET_

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Aug 5, 2018

Member

H at end


QTextEdit* simulation_text_;
QLabel* no_changes_label_;
QLabel* copy_urdf_;

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Aug 5, 2018

Member

make these private


QTextEdit* instructions_command = new QTextEdit(this);
instructions_command->setText(
std::string("roscd " + config_data->srdf_->srdf_model_->getName() + "_description").c_str());

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Aug 5, 2018

Member

config_data_->urdf_pkg_name_ is more reliable for getting the exact package name, do not assume their robot follows the "_description" format

@@ -127,6 +127,7 @@ SetupAssistantWidget::SetupAssistantWidget(QWidget* parent, boost::program_optio
nav_name_list_ << "End Effectors";
nav_name_list_ << "Passive Joints";
nav_name_list_ << "Perception";
nav_name_list_ << "Simulation with Gazebo";

This comment has been minimized.

Copy link
@davetcoleman

davetcoleman Aug 5, 2018

Member

I think you misunderstood me - I like this name for the top title, but I think its a little too long for the navigation bar. Can you please restore it to simply "Simulation"?

@MohmadAyman MohmadAyman force-pushed the MohmadAyman:urdf_gazebo_compatible branch 5 times, most recently from c172b68 to 04d3c0e Aug 6, 2018

@davetcoleman

This comment has been minimized.

Copy link
Member

commented Aug 9, 2018

Checks are still failing

@MohmadAyman MohmadAyman force-pushed the MohmadAyman:urdf_gazebo_compatible branch from 04d3c0e to f97baaf Aug 9, 2018

@MohmadAyman

This comment has been minimized.

Copy link
Member Author

commented Aug 10, 2018

@davetcoleman Travis happy.

@davetcoleman
Copy link
Member

left a comment

Great job!

@mcevoyandy
Copy link

left a comment

looks good to me.

@davetcoleman davetcoleman merged commit 48e577d into ros-planning:kinetic-devel Aug 13, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

davetcoleman added a commit that referenced this pull request Aug 13, 2018

@davetcoleman

This comment has been minimized.

Copy link
Member

commented Aug 13, 2018

cherry-picked to melodic

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.