Update URDF parser to use Tesseract XML namespace

[marip8]

This PR addresses #1059 by removing the URDF tag tesseract_version and moving the custom geometry implementations to a tesseract XML namespace. This should allow us to make further Tesseract-specific URDF additions in a clearer manner. Users will need to specify this XML namespace as follows:

- <robot name="my_robot" xmlns:xacro="http://ros.org/wiki/xacro">
+ <robot name="my_robot" xmlns:xacro="http://ros.org/wiki/xacro" xmlns:tesseract="http://github.com/tesseract-robotics">
  ...
</robot>

I think the URL is somewhat arbitrary, but should generally point to a location that gives the user more information about what elements are available in this namespace.

This PR also removes the behavior in tesseract_version < 2 of automatically converting collision mesh files into into convex hulls. Now, if users want tesseract to automatically convert mesh files to convex hulls, they will need to add the attribute tesseract:make_convex="true|false" either at the global level (as an attribute in the robot element) or at the individual mesh level (i.e., as an attribute in the mesh element)

<collision>
  <geometry>
-    <mesh filename="" />  <!-- Previously, this mesh was automatically converted to a convex hull under the hood when `tesseract_version` was < 2 -->
+    <mesh filename="" tesseract:make_convex="true"/>  <!-- Now use this element to get the same behavior -->
  </geometry>
</collision>

[Levi-Armstrong]

This PR also removes the behavior in tesseract_version < 2 of automatically converting collision mesh files into into convex hulls. Now, if users want tesseract to automatically convert mesh files to convex hulls, they will need to use the <tesseract:convex_mesh file="" convert="true"/> element instead of just the mesh element.

I think more discussion is needed because this will have significant impact throughout Tesseract because most leverage xacro's outside of tesseract with no easy way to enable this convex hull functionality instead of duplicating and updating with the tesseract namespace to get same behavior. I think there still needs to be a global way to enable this.

[Levi-Armstrong]

It looks like you can namespace attributes so we could do the following to define global setting which can be overriden locally?

<robot name="my_robot" xmlns:xacro="http://ros.org/wiki/xacro" xmlns:tesseract="http://github.com/tesseract-robotics" tesseract:create_convex_hull="true">
<collision>
  <geometry>
    <mesh file="" /> This would use global setting
  </geometry>
</collision>
<collision>
  <geometry>
    <mesh file="" tesseract:create_convex_hull="true"/> Override global setting set to true
  </geometry>
</collision>
<collision>
  <geometry>
    <mesh file="" tesseract:create_convex_hull="false"/> Override global setting set to false
  </geometry>
</collision>
</robot>

[marip8]

I like the idea of using namespace attributes where possible better than adding new elements. That way existing tools can still function (mostly) correctly without needing to understand Tesseract-specific attributes (e.g., Rviz would still render <mesh/> elements whereas it wouldn't know how to render <tesseract:convex_mesh/> elements).

Is the global attribute really that useful? I wonder if it would be more confusing rather than helpful, especially when you start thinking about composing xacros where one xacro might want it to be on and another off, but at the highest level there can only be one value of that attribute that gets applied to all underlying xacros. I would rather just specify it at the individual mesh level to be very explicit for clarity and consistency

[Levi-Armstrong]

Is the global attribute really that useful? I wonder if it would be more confusing rather than helpful, especially when you start thinking about composing xacros where one xacro might want it to be on and another off, but at the highest level there can only be one value of that attribute that gets applied to all underlying xacros. I would rather just specify it at the individual mesh level to be very explicit for clarity and consistency

While I agree that is it better to be explicit, though I do not think there is an easy way around having a global attribute which does not add significant burden to the developer. This is not easy to accomplish if you are using third party packages like fanuc, motoman, etc., because without a global attribute you would have to copy all third party xacros and modify them when you want to use convex hulls. I think we can provide the global option but change the default to false so now the default behavior is to leverage detailed meshes and they must explicitly set this to true to get the current behavior.

[marip8]

Yeah, I've run into a similar problem; I ended up running xacro to create a URDF of the fully defined system, then manually added all of the custom namespace elements/attributes that I needed, which is somewhat tedious. I'll add the global attribute and set the default to not convert mesh to convex hulls, and I'll add the mesh element specific override attributes.

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 88.54%. Comparing base (9658a37) to head (477fc89).
Report is 1 commits behind head on master.

Additional details and impacted files

@@           Coverage Diff           @@
##           master    #1081   +/-   ##
=======================================
  Coverage   88.54%   88.54%           
=======================================
  Files         293      292    -1     
  Lines       16778    16729   -49     
=======================================
- Hits        14856    14813   -43     
+ Misses       1922     1916    -6     

see 27 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[Levi-Armstrong]

Also thinking we should just make the default true, because I think it is fine that we are different as long as there is ability to change the behavior. This would also require less changes possible throughout the repos.

[marip8]

Also thinking we should just make the default true, because I think it is fine that we are different as long as there is ability to change the behavior. This would also require less changes possible throughout the repos.

I would prefer to keep the default to be false (i.e., the global option is false when the attribute is not present) and make users specify that attribute explicitly. Automatically converting all mesh collision geometry to convex hulls without the users explicit instruction can result in collision checking errors that are very confusing and hard to explain for someone who doesn't know that this conversion is going on under the hood.

I can add this attribute to the rest of the URDFs in tesseract_support to make sure the downstream demos and tests still run the same way. I'm not sure this is really necessary though because I think all of the geometries are already convex hulls

[Levi-Armstrong]

My concern is that tesseract has had this default behavior since the beginning and this change is likely to break a lot existing implementation. It could go unnoticed and difficult to track down for them. If we go with false then I would recommend that it is required that the urdf contain the global attribute and have no default.

[marip8]

Okay, let's make tesseract:make_convex required then, and in the case that it doesn't exist, I'll add an error message saying that it is missing and that the previous default value was true.

[Levi-Armstrong]

@marip8 Do think that we should remove convex_mesh support in the urdf and just use mesh with the attribute?

[marip8]

@marip8 Do think that we should remove convex_mesh support in the urdf and just use mesh with the attribute?

Yes, I completely removed the convex_mesh element and added the tesseract:make_convex attribute both globally and in the mesh element

[marip8]

After adding b681a5c to fix the Windows build, I see the following error in clang-tidy and MacOS:

error: lambda capture 'global_make_convex' is not required to be captured for this use [clang-diagnostic-unused-lambda-capture]

@Levi-Armstrong any idea how to proceed?

[Levi-Armstrong]

After adding b681a5c to fix the Windows build, I see the following error in clang-tidy and MacOS:

error: lambda capture 'global_make_convex' is not required to be captured for this use [clang-diagnostic-unused-lambda-capture]

@Levi-Armstrong any idea how to proceed?

Does the following work?

const auto parse_collision_fn = [global_make_convex = false](const tinyxml2::XMLElement* xml_element,
                                                        const tesseract_common::ResourceLocator& locator) {
    return tesseract_urdf::parseCollision(xml_element, locator, global_make_convex);
  };

[marip8]

It appears that I fixed the issue by only providing & in the capture instead of &global_make_convex. Not sure why including everything by reference is okay but capturing a single variable by reference is not...

[Levi-Armstrong]

I think the issue is that it is const.

[Levi-Armstrong]

Lets hold off merging this until I finish fixing 0.28 release.

[rjoomen]

Good initiative. Will the new style Tesseract URDF satisfy the official urdfdom XSD? See ros/urdfdom#200 for the latest version (WIP).

[marip8]

Good initiative. Will the new style Tesseract URDF satisfy the official urdfdom XSD? See ros/urdfdom#200 for the latest version (WIP).

Yes, I believe so. These changes do not overwrite any elements or attributes that are already specified in the urdfdom XSD. It only adds a few new namespaced geometry elements (e.g., tesseract:capsule, tesseract:octree, etc.) and a new namespaced attributed (tesseract:make_convex) for the existing robot and mesh elements

[Levi-Armstrong]

@marip8 You good if I squash merge this?

[rjoomen]

Good initiative. Will the new style Tesseract URDF satisfy the official urdfdom XSD? See ros/urdfdom#200 for the latest version (WIP).

Yes, I believe so. These changes do not overwrite any elements or attributes that are already specified in the urdfdom XSD. It only adds a few new namespaced geometry elements (e.g., tesseract:capsule, tesseract:octree, etc.) and a new namespaced attributed (tesseract:make_convex) for the existing robot and mesh elements

Great. Would it be possible to add this to the unit tests?

[marip8]

@marip8 You good if I squash merge this?

Yes it should be ready for merge now and squashing the history is fine with me

[marip8]

Would it be possible to add this to the unit tests?

What do you mean exactly? There are unit tests that parse all of these elements with their new names and test the tesseract:make_convex attribute. Do you mean testing compatibility with urdfdom XSD?

[rjoomen]

Would it be possible to add this to the unit tests?

What do you mean exactly? There are unit tests that parse all of these elements with their new names and test the tesseract:make_convex attribute. Do you mean testing compatibility with urdfdom XSD?

Yes