microsoft / Azure_Kinect_ROS_Driver

A ROS sensor driver for the Azure Kinect Developer Kit.
MIT License
304 stars 226 forks source link

Enhanced URDF Model #240

Open exo-core opened 2 years ago

exo-core commented 2 years ago

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:

ooeygui commented 2 years ago

It would be great to fix this bug in this change - https://github.com/microsoft/Azure_Kinect_ROS_Driver/issues/69

ooeygui commented 2 years ago

Another issue that should be addressed in the URDF update https://github.com/microsoft/Azure_Kinect_ROS_Driver/issues/177

uahic commented 2 years ago

Please also add to the URDF (libgazebo_ros_openni_kinect.so section) ` 0.25

2.88

` (this works for me, the values are identically with the clip-near and clip-far) to the camera plugin in Gazebo as you otherwise get a wall of points where the clipping area ends

christian-rauch commented 2 years ago

In your original PR (#238) you mention that you rely on the original CAD model (akdk_camera_cad.stp, see also https://github.com/microsoft/Azure_Kinect_ROS_Driver/issues/101#issuecomment-809455009). Could you document in the commit message or in this PR, how the dae is generated from the official stp file?

exo-core commented 2 years ago

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 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.

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.

The collision mesh holds heavily simplified version of the visual model, basically reducing it to two convex boxes with rounded corners:

kinect_azure_models

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.

Finally, please squash your commits. I think one commit that adds the new mesh and updated the XACRO would be sufficient.

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.

Another issue that should be addressed in the URDF update #177

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 floating joints, so that the TF tree is at fully connected upon initialization.

Please also add to the URDF (libgazebo_ros_openni_kinect.so section) <pointCloudCutoff>0.25</pointCloudCutoff> <pointCloudCutoffMax>2.88</pointCloudCutoffMax> (this works for me, the values are identically with the clip-near and clip-far) to the camera plugin in Gazebo as you otherwise get a wall of points where the clipping area ends

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 ;-)

exo-core commented 2 years ago

In your original PR (#238) you mention that you rely on the original CAD model (akdk_camera_cad.stp, see also #101 (comment)). Could you document in the commit message or in this PR, how the dae is generated from the official stp file?

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. So, what I did was

  1. Merge all loose parts to a single mesh
  2. Deleting all structures on the inside or between parts
  3. Clean edges between the parts by closing the gaps
  4. Remove unnecessary vertices lying within planar surfaces. In all of that cleanup work I also did some reorganization of the mesh structure, by rearranging the edges and replacing triangles by quads, as they are way more comfortable to work with.
  5. Adjust the scale from millimeters to meters to match the ROS convention (I noticed some tools have ignore the scale parameter in the URDF).
  6. Shift the center of origin to the tripod screw, as this is usually the point with which to sensor is connected to other models.
  7. Apply colors and materials
  8. Finally I did some general simplifications on the model. I closed the screw holes on the side and most of the holes in the top cover. I only left six of them which lie on the positions of the microphones.

For the collision model I did even more simplifications (see screenshot in the post above).

exo-core commented 2 years ago

So, I did a bunch of cleaning up and also included some new frames and features:

  1. The URDF now holds additional parameters for the entire microphone array azure_microphone_frames
  2. I deleted the old description. There are now two new files. One with the macro, and another one for standalone usage, loading that macro onto a simple model of the stand shipping with the Azure. Both models come with a visual, and a collision mesh. azure_stand_visual azure_stand_collision
  3. All launch files were updated to load the new URDF version. I also introduced a new parameter publish_calibrated_poses (true by default) that allows to load the static CAD measures (left) instead of publishing the factory calibration parameters (right) during runtime. azure_cad-vs-calibration This is also necessary for simulation, as there is no device with factory parameters.

If I didn't overlook something this PR should now be ready for merging...

christian-rauch commented 2 years ago

Thanks for continuing with this. Do you actually use the stand_visual.dae in your setup. I think most people will mount the camera to a robot, or use it as-is without the "stand". Especially since the stand and the camera are not rigidly connected. I honestly think that this stand is not required.

exo-core commented 2 years ago

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. Major reason to include it, was that I wanted a somewhat "realistic" standalone file so that one can see, how the xacro macro is intended to be used. Thought having it doesn't do harm, but a cylinder would probably get the job done as well... It would, however, bring back material definitions and thus potential naming clashes.

ooeygui commented 2 years ago

Is this ready for review?

exo-core commented 2 years ago

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.

StohanzlMart commented 11 months ago

I wonder, if a 2.78 trillion USD company is not able to work through pull requests in a timely fashion, who can?