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

Update URDF parser to use Tesseract XML namespace #1081

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

marip8
Copy link
Contributor

@marip8 marip8 commented Dec 13, 2024

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 use the <tesseract:convex_mesh file="" convert="true"/> element instead of just the mesh element.

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

@Levi-Armstrong
Copy link
Contributor

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
Copy link
Contributor

Levi-Armstrong commented Dec 14, 2024

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
Copy link
Contributor Author

marip8 commented Dec 16, 2024

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
Copy link
Contributor

Levi-Armstrong commented Dec 16, 2024

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
Copy link
Contributor Author

marip8 commented Dec 16, 2024

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.

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.

2 participants