-
Notifications
You must be signed in to change notification settings - Fork 226
Enhanced URDF Model #240
base: melodic
Are you sure you want to change the base?
Enhanced URDF Model #240
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for updating the model. I am looking forward to a model description with the proper visual representation. The PR essentially adds a new "visual" mesh and updating the xacro accordingly.
My main comments are that we should not add redundant blender files to the repo and that it should also not contain multiple xacro files for different versions. Just add the new "visual" model, preferably as OBJ or STL for better compatibility, and only update the old XACRO file.
I think we can also use the union of boxes, that was previously used as visual mode, as collision mesh. There is no need for a dedicated collision mesh file.
Finally, please squash your commits. I think one commit that adds the new mesh and updated the XACRO would be sufficient.
urdf/azure_kinect_macro.urdf.xacro
Outdated
<xacro:macro name="azure_kinect" params="prefix:='' parent:='' gazebo:=false *origin"> | ||
|
||
<xacro:if value="${parent != ''}"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make sure that the indentation is aligned and follows the same style as the old xacro file.
urdf/azure_kinect_macro.urdf.xacro
Outdated
</xacro:if> | ||
|
||
</xacro:macro> | ||
</robot> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a new line at the end of the file.
urdf/azure_kinect_macro.urdf.xacro
Outdated
<collision> | ||
<origin xyz="0 0 0" rpy="0 0 0" /> | ||
<geometry> | ||
<mesh filename="package://azure_kinect_ros_driver/meshes/collision.dae"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From a simulation viewpoint, it is better to use a simplified collision mesh.
<xacro:azure_kinect prefix="" parent="" gazebo="false"> | ||
<origin xyz="0 0 0" rpy="0 0 0" /> | ||
</xacro:azure_kinect> | ||
</robot> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should have a newline at the end.
@@ -0,0 +1,132 @@ | |||
<?xml version="1.0"?> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file should replace the original xacro.
It would be great to fix this bug in this change - #69 |
Another issue that should be addressed in the URDF update #177 |
Please also add to the URDF (libgazebo_ros_openni_kinect.so section) |
In your original PR (#238) you mention that you rely on the original CAD model ( |
I wouldn't exactly call the blender file redundant. By exporting the mesh to collada, quite some information is lost. The material descriptions is simplified and all quads are split into triangles. As such working directly on the *.dae file always requires some manual cleanup, before one can nicely work on it, in case of future updates.
The collision mesh holds heavily simplified version of the visual model, basically reducing it to two convex boxes with rounded corners: I guess we can now have a long discussion whether this is better or worse than having two individual, plain boxes, but I'd say that having either of them defined in a separate file is definitely the cleanest solution to #69, since the materials are embedded in the mesh file and thus cannot cause naming clashes anymore. edit: Regarding the old model, I can of course delete the previous version.
Imho it is common practice to have at least two URDF files. One providing a xacro macro, which can be used to insert the device description as part of a larger system description (e.g. when defining a robot system equipped with a camera) and a minimal model, which just calls the macro and can be used when working with a standalone sensor. This approach can for instance be found in the packages of the PhotoNeo PhoXi or the KUKA LBR iiwa.
I'm not sure whether that's a great idea in general. The new model is centered on the tripod mounting screw, which should allow easy positioning of it when placing the model in larger setups (e.g. on a tripod model). I also see, that having the individual microphone poses might be helpful for audio processing, but having the position of each individual screw in the URDF model is imho a way of unnecessary inflating the URDF model quite heavily. I have also never seen anything like this in any other URDF... I would however say, that it migth be helpful to include the calibrated pieces being published during runtime as
I also figured this out and already have that changes in my local version for a while. I'll include it in the updated PR ;-) |
Sorry, the comment came in while I was typing my previous answer above. The issue with CAD files is, that they nicely define geometric bodies by curves, and how to combine them by boolean operations, but that they contain no information about the mesh structure, so we need to run them through some exporter (which was FreeCAD in my case). Moreover those models are meant for mechanic assembly, which isn't really optimal for visualization. There were a bunch of (almost) invisible structures (e.g. screw threads on the inside) having huge polygon counts. Also due to engineering tolerances, all parts were individual bodies with tiny gaps in between.
For the collision model I did even more simplifications (see screenshot in the post above). |
184ffe6
to
d6e69db
Compare
Thanks for continuing with this. Do you actually use the |
Nope, I just use the macro to mount it to a robot as shown in #237. Just used it for a day or two when the sensor was new and I was playing around with the software settings. It was, however, a little annoying that the the stand is a little tilted and thus the pointcloud was always rotated. |
Is this ready for review? |
I was waiting for you to make a call on the model of the stand... Other than that it's ready. |
I wonder, if a 2.78 trillion USD company is not able to work through pull requests in a timely fashion, who can? |
This PR is basically the same as #238, but I had to change the source branch, since I already did quite some more changes to our repo.
Fixes
Description of the changes:
Required before submitting a Pull Request:
I tested changes on: