openrr / urdf-rs

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

[BUG] If <origin> is below <geometry> in a urdf file, then <origin> doesn't load. #94

Open rydb opened 6 months ago

rydb commented 6 months ago

E.G: the below code will cause right_leg have its origin set to xyz = [0, 0, 0] and rpy = [0, 0, 0], even though it is a valid xml that does not error when loaded by urdf_rs

invalid

<?xml version="1.0"?>
<robot name="origins">
  <link name="base_link">
    <visual>
      <geometry>
        <cylinder length="0.6" radius="0.2"/>
      </geometry>
    </visual>
  </link>

  <link name="right_leg">
    <visual>
      <geometry>
        <box size="0.6 0.1 0.2"/>
      </geometry>
      <origin rpy="0 1.57075 0" xyz="0 0 -0.3"/>
    </visual>
  </link>

  <joint name="base_to_right_leg" type="fixed">
    <parent link="base_link"/>
    <child link="right_leg"/>
    <origin xyz="0 -0.22 0.25"/>
  </joint>

</robot>

valid

<?xml version="1.0"?>
<robot name="origins">
  <link name="base_link">
    <visual>
      <geometry>
        <cylinder length="0.6" radius="0.2"/>
      </geometry>
    </visual>
  </link>

  <link name="right_leg">
    <visual>
      <origin rpy="0 1.57075 0" xyz="0 0 -0.3"/>
      <geometry>
        <box size="0.6 0.1 0.2"/>
      </geometry>
    </visual>
  </link>

  <joint name="base_to_right_leg" type="fixed">
    <parent link="base_link"/>
    <child link="right_leg"/>
    <origin xyz="0 -0.22 0.25"/>
  </joint>

</robot>
taiki-e commented 6 months ago

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 commented 6 months ago

Can you check if this branch fixes it?

rydb commented 6 months ago

Can you check if this branch fixes it?

Can you make the version requirement of thiserror looser? I can't test urdf-rs due to the below cargo error.

cargo check failed to start: Cargo watcher failed, the command produced no valid metadata (exit code: ExitStatus(unix_wait_status(25856))):

Updating git repository `[https://github.com/luca-della-vedova/urdf-rs`](vscode-file://vscode-app/snap/code/148/usr/share/code/resources/app/out/vs/code/electron-sandbox/workbench/workbench.html)

Updating crates.io index

error: failed to select a version for thiserror.

... required by package `urdf-rs v0.8.0 ([https://github.com/luca-della-vedova/urdf-rs?rev=cb42f5a#cb42f5aa)`](vscode-file://vscode-app/snap/code/148/usr/share/code/resources/app/out/vs/code/electron-sandbox/workbench/workbench.html)

... which satisfies git dependency `urdf-rs` of package `bevy_serialization_extras v0.2.1 (/home/ry/Projects/bevy_serialization_extras)`

versions that meet the requirements ^1.0.56 are: 1.0.56

all possible versions conflict with previously selected packages.

previously selected package thiserror v1.0.50

... which satisfies dependency `thiserror = "^1.0"` (locked to 1.0.50) of package `bevy_obj v0.12.0`

... which satisfies dependency `bevy_obj = "^0.12"` (locked to 0.12.0) of package `bevy_serialization_extras v0.2.1 (/home/ry/Projects/bevy_serialization_extras)`

failed to select a version for thiserror which could resolve this conflict

Failed to load workspaces.

luca-della-vedova commented 6 months ago

Ah you are working with bevy! That's awesome, I worked with bevy_obj as well :)

Since bevy_obj just needs 1.0 I believe a simple cargo update should fix your build error.

Edit: Regarding the making the version looser, I noticed that urdf-rs also fixes a patch version so I just updated that, I'm not sure if it really needs 1.0.7 or if we can just fix it to 1.0

taiki-e commented 6 months ago

I noticed that urdf-rs also fixes a patch version

No, "1.0.7" means ">= 1.0.7, < 2.0.0" (not "=1.0.7") because the cargo's default version requirement strategy is caret requirements.

https://github.com/openrr/urdf-rs/blob/e07b0ea013b47223213708a15430a2350c4ef02e/Cargo.toml#L16

I'm not sure if it really needs 1.0.7 or if we can just fix it to 1.0

1.0.7+ is needed because we use a feature only available since https://github.com/dtolnay/thiserror/releases/tag/1.0.7

https://github.com/openrr/urdf-rs/blob/e07b0ea013b47223213708a15430a2350c4ef02e/src/errors.rs#L7

luca-della-vedova commented 6 months ago

OK thanks! I reverted the update since it's not necessary.

taiki-e commented 6 months ago

As for an error about thiserror: I think the real problem is that one of bevy_obj's dependencies (or itself) is pinning a proc-macro related dependency (I remember wgpu doing that before), but it's okay to remove the dependency on thiserror to avoid the problem. It would not be too complicated even if we wrote that manually.