Skip to content

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

Open
willstott101 wants to merge 2 commits into
openrr:mainfrom
willstott101:feature/sdf
Open

WIP: Towards adding an SDFormat parser to this repo.#32
willstott101 wants to merge 2 commits into
openrr:mainfrom
willstott101:feature/sdf

Conversation

@willstott101

@willstott101 willstott101 commented Apr 12, 2022

Copy link
Copy Markdown

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.

@OTL OTL left a comment

Copy link
Copy Markdown
Collaborator

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.)

Comment thread README.md

```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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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()

Comment thread src/common/mod.rs
@@ -0,0 +1,5 @@
mod elements;

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

Comment thread src/ros_utils.rs
@@ -1,6 +1,9 @@
use crate::deserialize::Robot;
//! ROS-specific functions that make use of `rosrun` and `rospack` as

Copy link
Copy Markdown
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.

Comment thread src/sdf/funcs.rs
use crate::sdf::deserialize::*;

use std::path::Path;

Copy link
Copy Markdown
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
Copy Markdown
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

frenzox commented Oct 9, 2023

Copy link
Copy Markdown

Any updates on this?

@willstott101

Copy link
Copy Markdown
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