Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Towards adding an SDFormat parser to this repo. #32

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

willstott101
Copy link

@willstott101 willstott101 commented Apr 12, 2022

Refactor to move urdf functions and types under urdf_rs::urdf, in order to start implementing SDF parsing under urdf_rs::sdf.

I haven't actually started writing the SDF code yet.

N.B. My interest here is still experimental - I'm not on the road to using urdf_rs in a product and as such may not be very responsive. That being said, I'm very excited about Rust's possibilities in Robotics.

Copy link
Collaborator

@OTL OTL left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added some comments. Basically, it seems that good direction for me.
(I wrote comments before knowing that WIP: in the title.)

@@ -11,7 +11,7 @@ Only [link](http://wiki.ros.org/urdf/XML/link) and [joint](http://wiki.ros.org/u
You can access urdf elements like below example.

```rust
let urdf_robo = urdf_rs::read_file("sample.urdf").unwrap();
let urdf_robo = urdf_rs::urdf::read_file("samples/sample.urdf").unwrap();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I want to keep urdf_rs::read_file() interface.
I think it's acceptable to move to urdf_rs::urdf::read_file, but urdf_rs::read_file should be kept.
How about reading sdf file if the extension is .sdf in urdf_rs::read_file ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would have to manage an enum return type then but we could add that.

Another option might be urdf_rs::read_urdf_file but IDK if that's any better really.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I understand about the type. I prefer to keep the current API.
I mean adding urdf_rs::read_stf_file()

@@ -0,0 +1,5 @@
mod elements;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want to use mod.rs, I think it's an old style.
I think it should be src/common.rs in modern style.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hm, is there another way to split up into multiple files? IDK how much this module will grow

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh I see - this style is new to me, didn't realise common.rs can have the same contents of common/mod.rs. I can see how that might be clearer in tabbed editors. I'll switch.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We want to use rust2021 edition style if possible.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI, there are clippy lints to force such module styles.
https://rust-lang.github.io/rust-clippy/master/index.html#module_files

@@ -1,6 +1,9 @@
use crate::deserialize::Robot;
//! ROS-specific functions that make use of `rosrun` and `rospack` as
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This rename also changes the interface, but it might be accepted. Because it's reasonable.

use crate::errors::*;
use crate::sdf::deserialize::*;

use std::path::Path;

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At least, bellow sort_link_joint() seems to be a duplication of urdf/funcs:sort_link_joint(). It can be avoided.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah this file in particular I haven't really looked at at all.

@frenzox
Copy link

frenzox commented Oct 9, 2023

Any updates on this?

@willstott101
Copy link
Author

No updates, we're currently using sdformat python bindings, and it's working acceptably. I'm not likely to put any time into this soon, but would use it if it existed 🤷

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants