Rhoban / onshape-to-robot

Converting OnShape assembly to robot definition (SDF or URDF) through OnShape API
MIT License
257 stars 59 forks source link

support continuous joints #104

Closed atticusrussell closed 1 year ago

atticusrussell commented 1 year ago

Cylindrical and Revolute Onshape mates with "continuous" or "wheel" in the name are created as continuous joints. Addresses part of #99.

I tested and confirmed the functionality of my changes using a known good Onshape assembly.

atticusrussell commented 1 year ago

Unsure how to properly request a review. @Gregwar does this look appropriate to merge?

DevnathNair commented 1 year ago

@atticusrussell Additionaly, It'd be cleaner if the continuous keyword was removed from the link name while writing to urdf/sdf in cases wherein they aren't strictly wheels. For example for a joint named dof_continuous_c_bore_l would be rendered as c_bore_l in the urdf/sdf.

Gregwar commented 1 year ago

Hello, sorry to be late here!

The change seems good but I agree with @DevnathNair that the property should not be in the dof names. Moreover, testing for it to be anywhere on the name might be dangerous

If we merge this, we would have 3 "modifiers" on the DOFs:

So we could have dof_shaft1_inv_continuous_speed, which seems really cumbersome

Maybe those could move to the config.json file

"joints": {
   "shaft1": {
      "invert": true,
      "continuous": true
   }
}

This could also be tweaked to merge this configuration so that custom joint limits or max efforts could be overwritten here. Maybe this is out of the scope, we can merge and open another issue to update the way it is handled in a future version.

What are your thoughts?

DevnathNair commented 1 year ago

@Gregwar I agree. Although it's very convenient to specify the type in the name of the link itself it'll become a code smell as more link specific overrides are added.

However, I suggest we keep the wheel keyword as it is both readable and functional. (I can't think of any case where a wheel link wouldn't be continuous).

As far as I can tell, we should be able to let dof_ and _inv live since they're at the first and last of the name. Anything else as you rightly suggested should be in the config.json.

Maybe a "joint_group" key could be added to the parser and config.json to assign properties en-masse like so:

{
    "joints": {
        "shaft1": {
            "invert": true,
            "continuous": true
        },
        "shaft2": {},
        "shaft3": {}
    },
    "joint_group": {
        "joints": ["shaft1", "shaft2", "shaft3"],
        "properties": {
            "continuous": true
        }
    }
}
Gregwar commented 1 year ago

Maybe another way to achieve this would be to have a "default" entry in the joints list that defines the fallback values for each joints:

{
  "joints": {
    "default": {
        "continous": true
    },
    "joint1": {
        "invert": true
    }
  }
}
Gregwar commented 1 year ago

Anyway, the PR is harmless, let's merge and discuss this in another thread