ami-iit / mujoco-urdf-loader

BSD 3-Clause "New" or "Revised" License
8 stars 0 forks source link

Add URDFtoMuJoCoLoader and MujocoWrapper class #7

Closed Giulero closed 1 month ago

Giulero commented 1 month ago

Summary of PR #7 Generated by beloved Copilot assistant

Files Changed

  1. examples/generate_ergoCub_xml.py (Added)

    • Demonstrates usage of URDFtoMuJoCoLoader and MujocoWrapper classes.
    • Example includes loading a URDF file, converting to MuJoCo XML, and initializing MuJoCo models.
  2. mujoco_urdf_loader/init.py (Modified)

    • Imports URDFtoMuJoCoLoader, URDFtoMuJoCoLoaderCfg, ControlMode, and MujocoWrapper.
  3. mujoco_urdf_loader/loader.py (Added)

    • Introduces URDFtoMuJoCoLoader class.
    • Handles loading and simplifying URDF, converting to MuJoCo XML, and setting control modes.

Summary of New Classes

For further details, you can view the full list of changes here.

traversaro commented 1 month ago

Cool! Could it make sense to update the README to advertise this class or at least link the example?

giotherobot commented 1 month ago

Also some tests would be nice :)

traversaro commented 1 month ago

Can you document in the PR add_position_actuator used to add actuators named <jointname>_motor and now is adding actuators named <jointname>_position_motor ? Thanks!

Giulero commented 1 month ago

Cool! Could it make sense to update the README to advertise this class or at least link the example?

Yup! I'll add some pointer there!

Also some tests would be nice :)

I agree! For now I can put one that checks that the conversion does not stop.

Giulero commented 1 month ago

Can you document in the PR add_position_actuator used to add actuators named <jointname>_motor and now is adding actuators named <jointname>_position_motor ? Thanks!

Actually, this is a point (and also a leftover) that I guess is worth to discuss. The order of the joints and the order of the actuators does not coincide. So I created a mapping between actuators and joints https://github.com/ami-iit/mujoco-urdf-loader/blob/51d484d513224a6f149ff76c55dccddac6922bba/mujoco_urdf_loader/wrapper.py#L20-L28

This mapping exploits the fact the the motors names are set with the same of the joints. I don't know if there is a smarter way to this mapping.

The use this mapping, also the <jointname>_position_motor should be changed to simply the joint name.

giotherobot commented 1 month ago

This mapping exploits the fact the the motors names are set with the same of the joints. I don't know if there is a smarter way to this mapping.

The use this mapping, also the <jointname>_position_motor should be changed to simply the joint name.

This should already work, if I'm not mistaken, since you are checking if the string joint_name is a subset of the actuator name.

traversaro commented 1 month ago

The existing heuristics as mentioned by @giotherobot seems to be working fine, but indeed it could could fail if some joints have some peculiar naming choice. URDFtoMuJoCoLoader knows the "real" mapping from joint to actuator, perhaps it can just pass it (optionally) to the MujocoWrapper, and if that is not present the MujocoWrapper fails back on the heuristics?

Giulero commented 1 month ago

The existing heuristics as mentioned by @giotherobot seems to be working fine, but indeed it could could fail if some joints have some peculiar naming choice. URDFtoMuJoCoLoader knows the "real" mapping from joint to actuator, perhaps it can just pass it (optionally) to the MujocoWrapper, and if that is not present the MujocoWrapper fails back on the heuristics?

Got it! Or another option could be to force who's using the URDFtoMuJoCoLoader to use as name of the actuators the one in the list of the controlled joints. A lazy solution to be honest.

Then I could set revert back the default name to <jointname>_motor in add_position_actuator, and just use the following lines to pass the name of the actuator (compliant with the list of actuated joints passed before)

https://github.com/ami-iit/mujoco-urdf-loader/blob/51d484d513224a6f149ff76c55dccddac6922bba/mujoco_urdf_loader/loader.py#L144-L160

Giulero commented 1 month ago

Reviving this PR!

Giulero commented 1 month ago

Ciao guys! Do you think there's something else to do for this PR?

traversaro commented 1 month ago

Ok for me!