Migrate to quick-xml

[luca-della-vedova]

Opening for visibility, it "works for me" but I haven't had time yet to write additional extensive tests yet.

quick-xml uses #[serde(rename)] by prepending a @ for attributes so it still allows specifying what should be serialize / deserialized into an attribute and what into a child element. Preexisting unit tests work, I extended the test to cover #95 and #94 as well, with only one minor API breakage as denoted here.

We can remove a whole chunk of code for manual serialization / deserialization since quick-xml "just works" and doesn't need any of the workaround that either of the previous libraries needed (reordering elements for serde-xml-rs and serializing complex structs for yaserde).

I'd be happy to have some feedback on what to do with mandatory vs optional fields as denoted in https://github.com/openrr/urdf-rs/issues/95#issuecomment-1884423182, I can see arguments both ways so anything works really

[taiki-e]

Thanks!

I have tested this with some URDFs, and I found the following two issues:

• Serializing https://github.com/openrr/openrr/blob/38b67711531ff339b4a037badd4e9f4150c6436b/openrr-planner/sample.urdf produces the wrong URDF.
• Deserializing https://github.com/ToyotaResearchInstitute/hsr_description/blob/9281be02b759e96107183b8c78d944ed8144d61b/robots/hsrb4s.urdf fails with "UrdfError(Other("Expecting Event::Start"))" error.

I believe the first one can be easily fixed (https://github.com/openrr/urdf-rs/pull/98#discussion_r1481388967), but I'm not sure what the second one is yet.

I'd be happy to have some feedback on what to do with mandatory vs optional fields as denoted in #95 (comment), I can see arguments both ways so anything works really

@OTL and I talked about this a while ago and agreed that what is written in the ROS wiki is not a very strict specification and that it is reasonable to follow what is actually supported in the ROS C++ parser.

So I think it is okay to allow for the omission of the effort attribute.

[luca-della-vedova]

Thanks for the feedback! I added a serde(default) to effort so files without it should deserialize just fine.

I had a look at the second Urdf you found and found the root cause, it is a very sneaky one... here. If you look very closely there is a ` character. It was so small that I thought it was my screen being dirty! Removing it makes the deserialization work, or at least not return an error, now the question is how do other libraries handle this but my feeling is that it's an issue with the urdf?

[taiki-e]

I had a look at the second Urdf you found and found the root cause, it is a very sneaky one... here. If you look very closely there is a ` character. It was so small that I thought it was my screen being dirty! Removing it makes the deserialization work, or at least not return an error, now the question is how do other libraries handle this but my feeling is that it's an issue with the urdf?

Wow, well spotted! That is indeed somewhat odd as XML/URDF.

That said, neither serde-xml-rs, yaserde, nor roxmltree seem to treat that case as an error, and I would definitely like to support that case since that URDF is the one used in the downstream's gallery.

[luca-della-vedova]

I'm a bit lost at the options here, there seems to be a PR in the repo about this issue, claiming that it doesn't work with robot_state_publisher and trying to fix it but the repo seems semi-abandoned. I was going to try this locally but noticed that the repo is only on ROS 1, apparently last released on kinetic and I don't have the right OS to actually try that out (or enough bandwidth, at least now to migrate the package to ROS 2 and verify that it works in its current state with the rest of the ROS 2 stack). My instinct was to try to fix the upstream repo since it's clearly of typo of some sorts that (according to the linked PR) causes at least some breakage, but given how abandoned the repo seems I'm not sure that would really end anywhere, unless the repo is forked over the medium-long term.