openrr / urdf-rs

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

Migrate to yaserde and add support for urdf serialization #64

Closed luca-della-vedova closed 1 year ago

luca-della-vedova commented 1 year ago

:warning: This PR is a pretty large change to the library itself and I believe it might need careful review / changes to make sure it is OK from both a functionality and style point of view. :warning:

TLDR: Switch implementation from serde-xml-rs to yaserde and derive serialization traits to allow users to also save their structs into a urdf file, as opposed to what it was before that was only allowing deserialization.

Why yaserde?

I'm working on a "urdf editor" project, so while deserializing urdf is great I would need to be able to also serialize it back to save my changes. When trying to derive Serialize I noticed that it's currently impossible to tell serde whether a field should be saved in a child element or in an attribute, for example if we had a struct:

struct Element {
    pub child: String
}

It could be serialized in any of the following ways, depending on whether child should be serialized as a child element or as an attribute:

<element>
    <child>content</child>
</element>

Or

<element child="content" />

Serde does not allow tagging fields to change the serialization behavior so the serializer can't be told exactly what to do. It defaults to using child elements (first behavior) but that is not enough for urdf that makes extensive use of attributes. The discussion started here, with the serde maintainer mentioning that xml should use a different crate, which sparked the creation of yaserde that is specific for xml.

yaserde allows specifying for each field whether it is an attribute or a child by adding a feature [yaserde(attribute)], child is implied. The rest of the APIs, such as flattening, renaming, are mostly similar to the serde crate.

The good

The bad

yaserde seems to have a few issues when generating serialization code for enums with struct fields, as documented in this issue. This forced me to rewrite serialization and deserialization for geometries manually. Unit tests pass but the surface for bugs is a lot higher. The code should be able to be removed once the upstream issue is fixed.

Migration Guide

// Before
let axis: [f64; 3] = joint.axis.xyz;

// After, either will work
let axis: [f64; 3] = *joint.axis.xyz;
let axis: [f64; 3] = joint.axis.xyz.0;
OTL commented 1 year ago

If k is compiled with this change, I feel it's almost OK. I tried it, but I can't do that easily.

Could you try to fix k? https://github.com/openrr/k

The number of errors is not so many.

❯ cargo build
   Compiling k v0.29.1 (/Users/ogura/rust/k)
error[E0277]: the trait bound `link::Geometry<T>: From<GeometrySerde>` is not satisfied
   --> src/urdf.rs:115:13
    |
115 |             urdf_visual.geometry.into(),
    |             ^^^^^^^^^^^^^^^^^^^^ ---- required by a bound introduced by this call
    |             |
    |             the trait `From<GeometrySerde>` is not implemented for `link::Geometry<T>`
    |
    = help: the trait `From<urdf_rs::Geometry>` is implemented for `link::Geometry<T>`
    = note: required for `GeometrySerde` to implement `Into<link::Geometry<T>>`

error[E0277]: the trait bound `link::Geometry<T>: From<GeometrySerde>` is not satisfied
   --> src/urdf.rs:129:13
    |
129 |             urdf_collision.geometry.into(),
    |             ^^^^^^^^^^^^^^^^^^^^^^^ ---- required by a bound introduced by this call
    |             |
    |             the trait `From<GeometrySerde>` is not implemented for `link::Geometry<T>`
    |
    = help: the trait `From<urdf_rs::Geometry>` is implemented for `link::Geometry<T>`
    = note: required for `GeometrySerde` to implement `Into<link::Geometry<T>>`

error[E0769]: tuple variant `urdf_rs::Geometry::Box` written as struct variant
   --> src/urdf.rs:140:13
    |
140 |             urdf_rs::Geometry::Box { size } => Geometry::Box {
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
help: use the tuple variant pattern syntax instead
    |
140 |             urdf_rs::Geometry::Box(size) => Geometry::Box {
    |                                   ~~~~~~

error[E0769]: tuple variant `urdf_rs::Geometry::Cylinder` written as struct variant
   --> src/urdf.rs:145:13
    |
145 |             urdf_rs::Geometry::Cylinder { radius, length } => Geometry::Cylinder {
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
help: use the tuple variant pattern syntax instead
    |
145 |             urdf_rs::Geometry::Cylinder(_) => Geometry::Cylinder {
    |                                        ~~~

error[E0769]: tuple variant `urdf_rs::Geometry::Capsule` written as struct variant
   --> src/urdf.rs:149:13
    |
149 |             urdf_rs::Geometry::Capsule { radius, length } => Geometry::Capsule {
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
help: use the tuple variant pattern syntax instead
    |
149 |             urdf_rs::Geometry::Capsule(_) => Geometry::Capsule {
    |                                       ~~~

error[E0769]: tuple variant `urdf_rs::Geometry::Sphere` written as struct variant
   --> src/urdf.rs:153:13
    |
153 |             urdf_rs::Geometry::Sphere { radius } => Geometry::Sphere {
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
help: use the tuple variant pattern syntax instead
    |
153 |             urdf_rs::Geometry::Sphere(radius) => Geometry::Sphere {
    |                                      ~~~~~~~~

error[E0769]: tuple variant `urdf_rs::Geometry::Mesh` written as struct variant
   --> src/urdf.rs:156:13
    |
156 |             urdf_rs::Geometry::Mesh { filename, scale } => {
    |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |
help: use the tuple variant pattern syntax instead
    |
156 |             urdf_rs::Geometry::Mesh(_) => {
    |                                    ~~~

error[E0308]: mismatched types
   --> src/urdf.rs:246:41
    |
246 |                         axis: axis_from(joint.axis.xyz),
    |                               --------- ^^^^^^^^^^^^^^ expected array `[f64; 3]`, found struct `Vec3`
    |                               |
    |                               arguments to this function are incorrect
    |
note: function defined here
   --> src/urdf.rs:199:4
    |
199 | fn axis_from<T>(array3: [f64; 3]) -> na::Unit<na::Vector3<T>>
    |    ^^^^^^^^^    ----------------
help: consider dereferencing the type
    |
246 |                         axis: axis_from(*joint.axis.xyz),
    |                                         +

error[E0308]: mismatched types
   --> src/urdf.rs:250:37
    |
250 |                     axis: axis_from(joint.axis.xyz),
    |                           --------- ^^^^^^^^^^^^^^ expected array `[f64; 3]`, found struct `Vec3`
    |                           |
    |                           arguments to this function are incorrect
    |
note: function defined here
   --> src/urdf.rs:199:4
    |
199 | fn axis_from<T>(array3: [f64; 3]) -> na::Unit<na::Vector3<T>>
    |    ^^^^^^^^^    ----------------
help: consider dereferencing the type
    |
250 |                     axis: axis_from(*joint.axis.xyz),
    |                                     +

Some errors have detailed explanations: E0277, E0308, E0769.
For more information about an error, try `rustc --explain E0277`.
error: could not compile `k` due to 9 previous errors

It will show the new usage of urdf-rs.

luca-della-vedova commented 1 year ago

Sounds good! I also added a migration guide to the PR description to describe the breaking changes and how to migrate. Draft PR in the k repo is up to show what they look like to crate users https://github.com/openrr/k/pull/112.

The Vec3 breakage is not strictly necessary, however when looking at the code I noticed that a Vec3 struct was actually created but was not being used anywhere where Vec3 are expected, making me think it was some sort of TODO to migrate, then since this type of PR would break API and require a version bump might as well address the implied TODO

OTL commented 1 year ago

Thanks a lot! I want to think a little bit more to confirm that there is no way to simplify the usage.

taiki-e commented 1 year ago

Would you update the following line to mention yaserde instead of serde?

https://github.com/openrr/urdf-rs/blob/490fc7654250b511520162f1d86c62e1863c9523/Cargo.toml#L12

luca-della-vedova commented 1 year ago

Would you update the following line to mention yaserde instead of serde?

https://github.com/openrr/urdf-rs/blob/490fc7654250b511520162f1d86c62e1863c9523/Cargo.toml#L12

Done! https://github.com/openrr/urdf-rs/pull/64/commits/aff68c8d6753d2336e19ee6fddc97824026aaac0

taiki-e commented 1 year ago

I noticed this PR used an old version of yaserde (the latest version is 0.8 which was released about 1 year ago).

Was it intentional? If so, could you explain why an old version is used?

luca-della-vedova commented 1 year ago

I noticed this PR used an old version of yaserde (the latest version is 0.8 which was released about 1 year ago).

Was it intentional? If so, could you explain why an old version is used?

It was... not... I think I used the changelog in their repo to figure out what version was the latest one but didn't check in crates.io.

The main change is that they made the xml import hidden and that could cause some issues if they decide it's not a public facing API and break it in a minor release, but still the port for now is pretty straightforward.

PR is here, nothing else changes and the upstream issues are still there, so not strictly necessary but it can be useful for future updates I guess

taiki-e commented 1 year ago

Hmm. This patch seems to have broken deserialization of JointLimit.

This can be reproduced by adding a check to deserialization test.

diff --git a/src/funcs.rs b/src/funcs.rs
index 317066b..b0a5a8b 100644
--- a/src/funcs.rs
+++ b/src/funcs.rs
@@ -163,6 +163,11 @@ fn check_robot(robot: &Robot) {
         assert_approx_eq!(xyz[0], 0.0);
         assert_approx_eq!(xyz[1], 1.0);
         assert_approx_eq!(xyz[2], -1.0);
+        let limit = &robot.joints[0].limit;
+        assert_approx_eq!(limit.lower, -1.0);
+        assert_approx_eq!(limit.upper, 1.0);
+        assert_approx_eq!(limit.effort, 0.0);
+        assert_approx_eq!(limit.velocity, 1.0);
     }

     #[test]
$ cargo test --lib -- funcs::tests::deserialization 
running 1 test
test funcs::tests::deserialization ... FAILED

failures:

---- funcs::tests::deserialization stdout ----
thread 'funcs::tests::deserialization' panicked at 'assertion failed: `(left !== right)` (left: `0.0`, right: `-1.0`, expect diff: `1e-6`, real diff: `1.0`)', src/funcs.rs:167:9
stack backtrace:
   0: rust_begin_unwind
             at /rustc/dbba594575ce75b1b57ccb1e4223b9909a28b1b8/library/std/src/panicking.rs:578:5
   1: core::panicking::panic_fmt
             at /rustc/dbba594575ce75b1b57ccb1e4223b9909a28b1b8/library/core/src/panicking.rs:67:14
   2: urdf_rs::funcs::tests::check_robot
             at ./src/funcs.rs:167:9
   3: urdf_rs::funcs::tests::deserialization
             at ./src/funcs.rs:234:9
   4: urdf_rs::funcs::tests::deserialization::{{closure}}
             at ./src/funcs.rs:174:26
   5: core::ops::function::FnOnce::call_once
             at /rustc/dbba594575ce75b1b57ccb1e4223b9909a28b1b8/library/core/src/ops/function.rs:250:5
   6: core::ops::function::FnOnce::call_once
             at /rustc/dbba594575ce75b1b57ccb1e4223b9909a28b1b8/library/core/src/ops/function.rs:250:5
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

failures:
    funcs::tests::deserialization

It can also be reproduced with yaserde 0.8.

taiki-e commented 1 year ago

I will revert this until the above bug is investigated and resolved. (#73)