search

openrr / urdf-rs

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

Revert switching to yaserde, use quick-xml for serialization #107

Closed taiki-e closed 1 week ago

taiki-e commented 1 week ago

See https://github.com/openrr/urdf-rs/issues/96 for more context.

We have switched to yaserde from serde-xml-rs in #64. 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.

This changes:

This is considered a breaking change because yaserde trait impls are removed, but it should not contain any other breaking changes.

I'm not very happy depending on 3 XML-related crates, but I believe there is no more reliable way to fix everything that is broken (#94, #95, and probably more), introduce no new regressions (https://github.com/openrr/urdf-rs/pull/98#issuecomment-1931956647), and maintain what we wanted in #64 (i.e., serialization) than this way at this time.

Eventually I will probably select roxmltree (that we already used in our mesh-loader) for deserialization, but we don't have the bandwidth to spend on a thorough rewrite of this crate at this time.

Fixes #95 Fixes #94 Fixes https://github.com/openrr/urdf-viz/issues/256 Closes #96 Closes #98

FYI, here is a diff with 0.6.9 (last release before switching to yaserde): https://github.com/openrr/urdf-rs/compare/v0.6.9...e4c3574c3126d6d23d0cd21d69e3a258298f79b7

Co-authored-by: Luca Della Vedova lucadv@intrinsic.ai

luca-della-vedova commented 1 week ago

Ah bummer that one character created so much trouble and forced this split, I was exploring your comment above about doing a preparsing to get rid of it and only using quick-xml but I guess it's too late now :sweat_smile: thanks for tackling this!

taiki-e commented 1 week ago

I was exploring your comment above about doing a preparsing to get rid of it and only using quick-xml but I guess it's too late now 😅

I think with proper documentation (added in https://github.com/openrr/urdf-rs/pull/108) we can do that without making it a breaking change.