Unity-Technologies / URDF-Importer

URDF importer
Apache License 2.0
230 stars 71 forks source link

Cylinder collider lost when imported URDF isturned into prefab #82

Closed Th-Havy closed 3 years ago

Th-Havy commented 3 years ago

Hi,

There is a problem with imported urdf robots that cannot be properly made into prefabs. Indeed, upon importing a URDF with cylinder colliders, a custom cylinder mesh is created and assigned to a mesh collider. This is totally fine when the robot is used in a scene right after being imported. However, Unity does not serialize Mesh objects, which means that custom meshes (e.g. cylinder colliders) that are not saved as Assets are lost when the object is turned into a prefab.

To reproduce the bug, you can use the following URDF: import it, save it as a prefab, and instantiate the prefab in the scene. The mesh collider then has no mesh associated, thus the cylinder collider is lost.

<?xml version="1.0" encoding="utf-8"?>
<robot name="Robot">
  <link name="base_link">
    <inertial>
      <origin rpy="0 0 0" xyz="0 0 0" />
      <mass value="1" />
      <inertia ixx="1" ixy="0" ixz="0" iyy="1" iyz="0" izz="1" />
    </inertial>
  </link>
  <link name="link">
    <inertial>
      <origin rpy="0 0 0" xyz="0 0 0" />
      <mass value="1" />
      <inertia ixx="0.395378053188324" ixy="0" ixz="0" iyy="0.395378053188324" iyz="0" izz="0.124089486896992" />
    </inertial>
    <visual>
      <geometry>
        <cylinder length="2" radius="0.5" />
      </geometry>
    </visual>
    <collision>
      <geometry>
        <cylinder length="2" radius="0.5" />
      </geometry>
    </collision>
  </link>
  <joint name="" type="fixed">
    <parent link="base_link" />
    <child link="link" />
  </joint>
</robot>

I do not really understand the need to create a custom mesh (public static Mesh CreateCylinderMesh(Link.Geometry.Cylinder cylinder)), for each cylinder. when the default unity cylinder could be used. I assume the goal is to have base circles with more sides to provide better rolling effects?

Anyways a single mesh should be created, and shared by all the cylinders, as changing only the scale of the parent "unnamed" is enough to shape the cylinder correctly.

The default Unity cylinder mesh could be retrieve as follows, or another more detailled cylinder mesh could maybe be stored in the URDF importer package (I am not really familiar with Unity packages, maybe this is not possible). Last resort is to save also the cylinder mesh in the meshes folder upon importing, so that it is also an asset and can thus not be lost when the robot itself is made into an asset.

LaurieCheers-unity commented 3 years ago

Hi, thanks for bringing this to our attention. I've made a ticket for the issue internally. [AIRO-621]

at669 commented 3 years ago

Hi @Th-Havy! You've brought up some great points--I'll address them individually.

To reproduce the bug, you can use the following URDF

Thank you for this sample!

Default Unity cylinder

The default Unity cylinder primitive is only a cylinder visually, and actually contains a capsule collider that will not be physically accurate. You can find more information on the primitives here. This is the motivation for generating a custom mesh for cylinder geometries.

Custom meshes (e.g. cylinder colliders) that are not saved as Assets are lost when the object is turned into a prefab

We've made fixes to be able to save the non-cylindrical meshes generated (i.e. reading from the geometry mesh file and saving a new asset), and I have linked a PR to resolve the cylinder meshes issue. In the meantime, you can checkout the PR's branch to view the changes for a current workaround and you can follow the PR to stay up-to-date with when this is merged into dev. Let me know if this resolves your issue!

hyounesy commented 3 years ago

The issue is fixed in PR 91 and will be available in the next release. In the meantime you may use the dev branch by opening the Packages/manifest.json in a text editor and modifying the entry for the urdf-importer package as following: "com.unity.robotics.urdf-importer": "https://github.com/Unity-Technologies/URDF-Importer.git?path=/com.unity.robotics.urdf-importer#dev"

hyounesy commented 3 years ago

closing the issue. feel free to open if the issue persists.