Color in material doesn't seem to parse.

[tylerjw]

Using this test urdf:

let urdf = r#"<robot name="test">"
  <joint name="j1" type="fixed">
    <parent link="l1"/>
    <child link="l2"/>
  </joint>
  <joint name="j2" type="fixed">
    <parent link="l1"/>
    <child link="l2"/>
  </joint>
  <link name="l1">
    <visual>
      <geometry>
        <sphere radius="1.349"/>
      </geometry>
      <material name="">
        <color rgba="1.0 0.65 0.0 0.01" />
      </material>
    </visual>
    <inertial>
      <mass value="8.4396"/>
      <inertia ixx="0.087" ixy="0.14" ixz="0.912" iyy="0.763" iyz="0.0012" izz="0.908"/>
    </inertial>
  </link>
  <link name="l2">
    <visual>
      <geometry>
        <cylinder radius="3.349" length="7.5490"/>
      </geometry>
      <material name="red ish">
        <color rgba="1 0.0001 0.0 1" />
      </material>
    </visual>
  </link>
</robot>"#;

I parse calling your read_from_string function, the color value is always a None. Here is a debug output of each of the visual objects after the parse:

&l.visual = [
    Visual {
        name: Some(
            "",
        ),
        origin: Pose {
            xyz: Vec3(
                [
                    0.0,
                    0.0,
                    0.0,
                ],
            ),
            rpy: Vec3(
                [
                    0.0,
                    0.0,
                    0.0,
                ],
            ),
        },
        geometry: Sphere {
            radius: 1.349,
        },
        material: Some(
            Material {
                name: "",
                color: None,
                texture: None,
            },
        ),
    },
]
&l.visual = [
    Visual {
        name: Some(
            "red ish",
        ),
        origin: Pose {
            xyz: Vec3(
                [
                    0.0,
                    0.0,
                    0.0,
                ],
            ),
            rpy: Vec3(
                [
                    0.0,
                    0.0,
                    0.0,
                ],
            ),
        },
        geometry: Cylinder {
            radius: 3.349,
            length: 7.549,
        },
        material: Some(
            Material {
                name: "",
                color: None,
                texture: None,
            },
        ),
    },
]

[luca-della-vedova]

Hello!

I admit not knowing much about yaserde but I can reproduce in some cases, I played with the unit test and got something similar to yours to "work" by editing the visual to include an <origin> tag. Can you give it a try? Now that is not the solution but I wonder if yaserde tries to deserialize the urdf, doesn't find the origin element even though it is currently required, and for this reason falls back to Default. If this is the case I believe that even if your material had a name it wouldn't be parsed, so it's really about material being Material::default() rather than a color parsing issue. If origin is indeed optional it could be considered to make the struct an Option rather than a mandatory field, but that would be API breaking and require a new major release.

Might be related to #94 that is affecting the same struct.

[taiki-e]

Thanks for the report! This is another regression introduced in https://github.com/openrr/urdf-rs/pull/64. I confirmed urdf-rs 0.6 (that uses serde-xml-rs) doesn't have this problem.

---

@luca-della-vedova

Thanks for the investigation! Since the main branch already has a breaking change (https://github.com/openrr/urdf-rs/pull/92), there is no problem with making a new breaking change.

Now that is not the solution but I wonder if yaserde tries to deserialize the urdf, doesn't find the origin element even though it is currently required

If this is the cause of this problem, it seems that yaserde is behaving quite badly in not treating the lack of the required element as an error.

[tylerjw]

Thank you for looking into this. I can confirm adding the required origin element fixes this behavior. I'm building a new library that depends on this so feel free to ping me to ask for help with changes here. I'm happy to test any new changes.

[luca-della-vedova]

I wonder if we should reconsider what fields are mandatory and what are options, for example we currently set origin, or even name, as mandatory, i.e. here then default to the origin if this is not provided, which kinda follows the specification where it states:

<origin> (optional: defaults to identity if not specified)

This creates a bit of an issue since it is not possible, for a user, to figure out if a pose in the origin was specified or if it was just the derived default. This creates a bit of confusion in deserialization use cases like the above, since there is a lot of empty data that is autogenerated, and is actively harmful in serialization since a round trip will add a lot of unnecessary defaults and noise to the URDF file. Should we reconsider and make all these fields Option instead? We can (and should) still implement Default so users can unwrap_or_default() the option if needed, which is a bit of API noise but technically more correct.

What do y'all think?

[luca-della-vedova]

@tylerjw can you give a try to this branch and see if it works for you?

[tylerjw]

Tested and got it to compile with some small changes around the GeometryTag type. It seems effort is a required argument to limit tags?

UrdfError(Other("missing field `@effort`"))

[luca-della-vedova]

I followed the specification http://wiki.ros.org/urdf/XML/joint which says that while lower and upper are optional, effort and velocity are required, are there a lot of urdfs in the wild that don't have one of the two and are successfully parsed by other parsers?

[tylerjw]

I think it is likely you are correct and my test urdfs are malformed. I'll go look at the actual urdf files we have for robots to see if this is the case. The ones in my tests are copied from the C++ parser's tests in ROS.