openrr / urdf-rs

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

[Feature Request] JointLimit should be inferred to be unlimited if a urdf has no explicitely defined JointLimit. #99

Open rydb opened 5 months ago

rydb commented 5 months ago

Looking at deserialize.rs


#[derive(Debug, YaDeserialize, YaSerialize, Default, Clone)]
pub struct JointLimit {
    #[yaserde(attribute)]
    pub lower: f64,
    #[yaserde(attribute)]
    pub upper: f64,
    #[yaserde(attribute)]
    pub effort: f64,
    #[yaserde(attribute)]
    pub velocity: f64,
}

f64 defaults to 0.0, this means the max lower and upper bound is inferred to be 0.0. This means, if a urdf has no explicitely defined joint limit, urdf-rs defaults to the joint not being able to move at all

I think Default for JointLimit should be changed to the below to fix that.

impl Default for JointLimit {
    fn default() -> Self {
        Self {
            lower: f64::MIN,
            upper: f64::MAX,
            effort: f64::MAX,
            velocity: f64::MAX,
        }
    }
}
luca-della-vedova commented 5 months ago

I'm not too sure about this default, it seems the docs specify that the default should be 0 if unspecified (here), This also seems to be the behavior for the ROS parser

rydb commented 5 months ago

I'm not too sure about this default, it seems the docs specify that the default should be 0 if unspecified (here), This also seems to be the behavior for the ROS parser

looking at the docs them selves:

<limit> (required only for revolute and prismatic joint)

    An element can contain the following attributes:

    lower (optional, defaults to 0)

        An attribute specifying the lower joint limit (in radians for revolute joints, in meters for prismatic joints). Omit if joint is continuous. 

    upper (optional, defaults to 0)

        An attribute specifying the upper joint limit (in radians for revolute joints, in meters for prismatic joints). Omit if joint is continuous. 
 Omit if joint is continuous. 

this implies there are instances where ROS assumes there are no limits like on continuous joints. I suppose it makes sense from a safety sense, but its still inconvenient.

Maybe there should be a JointLimit::unlimited() then?

luca-della-vedova commented 5 months ago

Yap there are continuous joints, it's a separate type (not revolute), have a look here. Now someone could make the case that we could exploit Rust's enum system better and make sure JointLimit is only contained if the joint is of a variant where it makes sense, but that would make serialization / deserialization a fair bit trickier

taiki-e commented 5 months ago

One option would be to add a method that returns Option<Range> based on JointType like this.

Given that it would complicate the parsing code, it would not necessarily be easy to embed the limit in the JointType. However, it would probably be the most type-safe way to do it.