Moveit: Moveit Treats Visual Mesh as Collision Mesh When No Collision Mesh is Specified

[mfbailey91]

Although, some may argue that this is a check for when URDFs are constructed improperly and links are not given collision meshes I would argue that promoting improper creation of URDF is a negative and it limits the modularity as moveit as a tool.

This prevents the use of virtual links which one may want to use as visual feedback in RVIZ. Examples of when you may want only a visual mesh is: if you want to use a virtual link visual mesh to outline a robots reachable work space, or specify some other visual indicator in the work space of the manipulator, or if you want some visual indicator that moves with the arm you may add a virtual fixed joint to a manipulator link.

Because the visual mesh is taken as the collision mesh if one wants to build in some visual feedback that intersects with the workspace they have to create some virtual collision mesh that then doesn't intersect with the workspace, which is a poor work around.

Links with visual meshes and no collision meshes should be treated as such and the visual mesh should not act as a collision mesh when no collision mesh is given.

[de-vri-es]

I ran into the same problem. I wanted a visual link without collision model, but had to resort to disabling collisions with the link and every other link in the allowed collision matrix of the SRDF.

Changing this unconditionally would break existing systems though, and especially in the case of collision detection you don't want to suddenly disable certain checks with a new version. So this would have to be an option that defaults to the old behaviour I think.

That leaves the question what would be a logical place for such an option. In the SRDF or URDF on a per-robot or per-link basis, or make it configurable in code and load it from the parameter server where applicable?

[gavanderhoorn]

@mfbailey91 wrote:

Links with visual meshes and no collision meshes should be treated as such and the visual mesh should not act as a collision mesh when no collision mesh is given.

I may be a bit conservative, but I wouldn't be comfortable with changing MoveIt's default behaviour here. Collision checking is part of MoveIt's 'safety features', and I've seen enough configs that actually (implicitly) depend on MoveIt defaulting to using the visual geometry for the collision mesh. That is certainly not nice (and the author of the URDF shouldn't have done that), but with such a large installed base I don't think removing this behaviour is an option.

So it would need to be configurable, as @de-vri-es suggests.

That leaves the question what would be a logical place for such an option. In the SRDF or URDF on a per-robot or per-link basis, or make it configurable in code and load it from the parameter server where applicable?

The ACM can already be configured by changing / adding appropriate entries in the SRDF (disable_collisions) and through the Setup Assistant. A limitation that would need to be removed is that it can only configure entries where both links are known. That makes re-using it for what @mfbailey91 describes impossible, as additional collision geometry may be added at run-time (OctoMap, manually added Collision Objects, etc), for which the ACM would then have to be updated as well (manually, and that's rather tedious / requires a lot of boilerplate).

Perhaps we could extend the set of allowed values for either link1 or link2 in disable_collisions elements with a special value that supports disabling collisions between a link and everything else, now and in the future.

[v4hn]

Changing the default is no option.
Given the current behavior of MoveIt, I would expect a flag to disable this functionality.
How about having a new optional item in the srdf that marks links as virtual/never-colliding?
Something like

<virtual_link name="my_virtual_link" />

I would welcome support for this.

opinions?

[gavanderhoorn]

@v4hn wrote:

How about having a new optional item in the srdf that marks links as virtual/never-colliding?

isn't that what I (admittedly while using more words) proposed? :)

[v4hn]

You pretty much got me there. :)
The difference is that in one case this would extend the existing disable_collisions interface and in the other we would add a new one. I wanted to have both versions up for discussion.

I'm not sure which one is better, actually. Both would need support in the setup assistant.
Either way, I would prefer a mechanism that is explicitly interpreted as "this link never collides"
instead of "this link never collides _with other links_", so expanding the special value you propose to all known links is no option.

[de-vri-es]

I do think disable_collision makes sense. It perfectly describes the wanted behaviour.

Maybe something alone the lines of one of these:

<disable_collision all="name-of-link" /> <!-- alternative attribute instead of link1, link2 pair ->
<disable_all_collisions link="name-of-link" /> <!-- admitted, this is a new tag -->
<disable_collision link1="name-of-link" /> <!-- absent link2 attribute means everything, probably bad idea -->

I like the looks of the first one most. The third one seems like a pretty bad idea in case someone forgets a link2 attribute.

[gavanderhoorn]

@v4hn wrote:

Either way, I would prefer a mechanism that is explicitly interpreted as "this link never collides"
instead of "this link never collides with other links", so expanding the special value you propose to all known links is no option.

I'm actually always a fan of the strictest and most pedantic options if given a choice, but in this case I don't really see a difference between the two options you describe: in the context of a MoveIt planning scene, what other entities are there than _links_ (with associated geometry)?

I do think disable_collision makes sense. It perfectly describes the wanted behaviour.

Well, yes, which is why I suggested it. But I do see the value of having a new tag for it, as it removes any possibility of users accidentally configuring a link to be always ignored.

Another option: <disable_collisions link1="name-of-link" link2="*" reason="Never" />.

[de-vri-es]

I'm actually always a fan of the strictest and most pedantic options if given a choice, but in this case I don't really see a difference between the two options you describe: in the context of a MoveIt planning scene, what other entities are there than links (with associated geometry)?

Another option: .

Well, attached collision objects and octomaps can also cause collisions. You don't want those to have collisions with these kind of links. That makes link2="*" less logical, since the other object is not necessarily a link.

It would also make '' a problematic link name. I think '' is a pretty bad link name as it is, but the URDF wiki page [1] doesn't say anything about what is legal in a link name.

[1] http://wiki.ros.org/urdf/XML/link

[gavanderhoorn]

It would also make '_' a problematic link name. I think '_' is a pretty bad link name as it is, but the URDF wiki page [1] doesn't say anything about what is legal in a link name.

I didn't suggest it was a link name. I merely used it as an example for a 'special value' that would signify _all links_ or _everything in the scene / world_.

[de-vri-es]

I didn't suggest it was a link name. I merely used it as an example for a 'special value' that would signify all links or everything in the scene / world.

Well, that is what I would avoid. Special values should not be legal link names in my opinion. Since the URDF specification doesn't seem to impose any restrictions on link names, that doesn't leave any room for special values.

[AndyZe]

:+1: for the idea of

It's really tedious to go through the ACM and disable each pair, otherwise.

[rhaschke]

As pointed out in #1907, the fact that MoveIt doesn't comply with the standard way how ROS programs should handle the collision and visual geometries in a URDF, results in an inconsistency.
For this reason, in my opinion, the only solution is to adhere to the standards in the long run.
I agree that we cannot introduce such a safety-critical change overnight. My suggestion is to deprecate the current behavior and introduce a _global_ ROS parameter, moveit_populate_empty_collision_geometry, that - for now - defaults to True (maintaining the current behavior, but issuing a deprecation warning), but if set to False adheres to the standard behavior in ROS.

[v4hn]

As pointed out in #1907, the fact that MoveIt doesn't comply with the standard way how ROS programs should handle the collision and visual geometries in a URDF, results in an inconsistency.

Can you link the documented standard, or at least name some other projects interpreting it differently, please?

Other aspects might be:

• Just keep the current MoveIt semantics of falling back to the visual geometry,
  but add a warning/error when no collision geometry is specified.
• trigger a fatal error to avoid safety issues, but go for a required collision-tag.

• generate a convex hull for collision checking on the fly

• Do we currently allow for visual meshes that are not supposed to have collisions associated, e.g., to indicate zones of interest?

I agree that we cannot introduce such a safety-critical change overnight. My suggestion is to deprecate the current behavior and introduce a _global_ ROS parameter, moveit_populate_empty_collision_geometry, that - for now - defaults to True (maintaining the current behavior, but issuing a deprecation warning), but if set to False adheres to the standard behavior in ROS.

It will be a hard change one way or the other, but yes, we should have a deprecation period.
I'm not a fan of keeping transition parameters in the code base, so why add it in the first place?

[rhaschke]

1907 was specifically referring to rviz, which strictly separates collision and visual geometries.

The URDF specification doesn't mention that visuals are taken as collision geometries and vice versa.

Just keep the current MoveIt semantics of falling back to the visual geometry,
but add a warning/error when no collision geometry is specified.

Isn't that what I suggested? Plus the possibility to suppress the (deprecation) warning message by setting a corresponding global ROS parameter.

Do we currently allow for visual meshes that are not supposed to have collisions associated, e.g., to indicate zones of interest?

One key critics of #1907 is that this is currently _not_ possible.

I'm not a fan of keeping transition parameters in the code base, so why add it in the first place?

To enable backwards compatibility at all. Isn't that obvious?

[rhaschke]

Let's discuss this at the next maintainer meeting.
Obviously, this change will require many URDFs out there to be adapted to the "standard".

[v4hn]

The difference I meant to point out is between

• trigger a warning/error but go on. Later make the error fatal.

  • No transition parameter required, because users fix the urdfs to get rid of the warning.

• make the error fatal and force the user to fix it, but provide the parameter to allow users to override it during the transition.

  • Pragmatic users (so most) will add the flag to their moveit config, go on, and forget about it until we remove/deprecate the parameter.

For the last few years, MoveIt usually picked the first approach to support migration.

[rhaschke]

The first approach doesn't work here, because we explicitly want to allow empty collision geometries while visuals are defined - for example, to indicate the pose of a virtual joint (e.g. mimicking a ball joint as a combination of three rotational ones). However, this empty collision geometry would trigger the warning then. To indicate to MoveIt that the new standard should be adopted, we need the extra parameter.
My proposal is fundamentally different from your option 2: Without the parameter, MoveIt will issue a deprecation warning, indicating that empty collision models will not be populated from visual geometries in the future. The user can silent this warning by defining the parameter. However, this will also imply that the collision model is not populated anymore, thus adhering to the new default.

[Dale-Koenig]

RobotModel is not part of moveit_ros, so it should not be reading ros params, and overall it seems Moveit avoids use of params except for plugins and a couple of more peripheral packages (jog_arm, grasps), so I think it would be fairly awkward to go pass a rosparam to RobotModelLoader, then pass that through to the RobotModel (Adding an argument to a bunch of RobotModel functions only to remove it again after full deprecation).

For this reason I think it's worth considering the first approach of just adding a warning and later making it fatal, since moveit does not support empty collision geometry at the moment anyway.