search

openrr / urdf-rs

URDF parser for Rust
Apache License 2.0
30 stars 11 forks source link

Consider moving away from yaserde #96

Closed taiki-e closed 1 month ago

taiki-e commented 9 months ago

We have switched to yaserde from serde-xml-rs in #64 (urdf-rs 0.7.0). However, many bugs have been introduced since then, some of which have not yet been fixed.

I'm no longer happy with continuing to use this difficult-to-use crate. I'm considering moving away from it, at least on deserialization.

Candidates for alternatives in my mind are serde-xml-rs, which was used previously, and roxmltree, which is currently used in mesh-loader. We should not use anything we have no experience with or are not familiar with, like yaserde. Both support deserialization only; as for serialization, we can continue to use yaserde, or we can consider replacing it with our own. Anyway, it is important to fix deserialization, which obviously has correctness problems.

FYI @luca-della-vedova

luca-della-vedova commented 9 months ago

I can see the frustration and I obviously feel partially guilty for this. I guess the only testing I did was to see if it works for my use cases and that it passed the pre-existing unit tests but that proved out to be insufficient since these don't really capture the wild variety of URDF files out there.

I'd like to try to look at these issues first but I will understand if you want to move away from yaserde. I suppose having serde-xml-rs and yaserde and having structures [derive(Deserialize, YaSerialize)] could bring the best of both worlds (robust deserialization and at least some serialization support), but that would have the cost of having double the number of dependencies. I'll ultimately leave the call up to you as the maintainer, happy to help whichever direction you choose to go.

luca-della-vedova commented 8 months ago

For reference I've been playing with removing yaserde in favor of quick-xml here. I reached the point where all the existing unit tests work (including serialization / deserialization round trip), but given the history I'll work on adding a regression test for every issue that has been opened, as well as trying to improve coverage, before really taking the result too seriously. There is only one "minor" API breakage, I had to add another struct to wrap geometry as denoted here. I added a Deref and DerefMut implementation so the only user facing difference is adding a dereference operator when accessing geometry object, shown here.

Do you think this exploration is OK or would you rather just revert to the previous option? For what it's worth on crates.io quick-xml has much more activity / downloads than serde-xml-rs, although I'll be honest I can't remember why I chose yaserde for the serialization implementation, this really just looks a lot better. image

taiki-e commented 8 months ago

For reference I've been playing with removing yaserde in favor of quick-xml here. I reached the point where all the existing unit tests work (including serialization / deserialization round trip), but given the history I'll work on adding a regression test for every issue that has been opened, as well as trying to improve coverage, before really taking the result too seriously. There is only one "minor" API breakage, I had to add another struct to wrap geometry as denoted here. I added a Deref and DerefMut implementation so the only user facing difference is adding a dereference operator when accessing geometry object, shown here.

Thanks! At first glance, it looks great to me. Being able to handle problematic cases is great, and not having to manually parse enums is also great.

And this workaround that was necessary when serde-xml-rs was used is no longer needed?

luca-della-vedova commented 8 months ago

And this workaround that was necessary when serde-xml-rs was used is no longer needed?

No it is not necessary anymore. I believe that case is already captured by the unit tests since we check that all the joints and links are parsed correctly here but they are in order of "link-joint-link-link-joint" here. Specifically for this crate, there is a (default off, that I enabled) feature to deal with non continuous lists, at the expense of extra parsing time.