-
Notifications
You must be signed in to change notification settings - Fork 383
Add toddlerbot_2xc and toddlerbot_2xm #202
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
base: main
Are you sure you want to change the base?
Conversation
louislelay
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice PR, thanks for sharing this! Could you remove all URDF files and follow the same structure used for other assets? That includes adding a readme in the asset folder describing how you created the XML. You should also consider adding entries for your robots in the global readme. And, are the fixed XML files really necessary?
- Remove all URDF files - Remove the fixed XML files - Add a readme in the asset folder - Add a license - Add an entry in the humanoids table of the global readme
|
Hi Louis, thanks for looking into this! I have updated the PR based on your suggestions. I added an entry in the humanoids table in the global readme, but I didn't add a thumbnail in the gallery since it seems like they're following a specific style. Are there any instructions to create such thumbnails? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice! Some more general comments ;)
Could you also change the images by screenshot from MuJoCo similarly to other assets. As for the thumbmails, i remember seeing in an older PR that Kevin Zakka add them himself as he have a script for it!
| </default> | ||
| <worldbody> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you format your xml files? See here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes. I followed the instructions to format the files.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok! Could you add a blank line between the big blocks? Look at the other assets to see how it’s done, it would improve readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be fixed in the latest commit
toddlerbot_2xm/toddlerbot_2xm.xml
Outdated
| size="0.02045 0.02325 0.023" name="left_shoulder_roll_link_collision" | ||
| material="shoulder_roll_link_visual_material"/> | ||
| <body name="left_shoulder_gear_drive" pos="0.019 0.024 -0.019" quat="1 0 0 0"> | ||
| <joint axis="0.0 0.0 1.0" name="left_shoulder_yaw_drive" type="hinge" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you have axis 0 0 1, it's not necessary to write it as it's already the default value. Please remove them and verify if you've other parameters with default values that you can remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be fixed now
Co-authored-by: Louis LE LAY <[email protected]>
- Remove robot.yml - Remove default values in the XML files - Update the photos
toddlerbot_2xm/toddlerbot_2xm.xml
Outdated
| mesh="waist_gear_drive_visual" material="waist_gear_drive_visual_material"/> | ||
| </body> | ||
| <body name="waist_gear_drive_2" pos="-0.03275 -0.0 -0.0196" quat="1 0 0 0"> | ||
| <joint axis="0.0 -0.0 1.0" name="waist_act_2" range="-4.698838925616426 4.656125865073181" class="XC330"/> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe you’re still missing some due to the way it’s written. It might be good to remove any unnecessary decimals and also remove the minus signs from zero values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice catch! Just fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! Maybe you could generalize the removal of unnecessary decimals and minus signs to other parameters as well, such as quat, pos, etc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback! I updated all the other parameters. I also found the script generate_gallery.py to render the thumbnail.
| return arena | ||
|
|
||
|
|
||
| MODEL_XMLS = [pathlib.Path(f"../{k}.xml") for k in MODEL_MAP.keys()] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to be a typo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes it does!
louislelay
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thanks for taking the time to go through my comments. I believe we cleared up a good amount of work for the maintainers :)
There are still a few things I’m a bit mixed up about, but the maintainers will have better judgment on those than I do.
Description
This PR adds the model of toddlerbot_2xc and toddlerbot_2xm
Fixes:
Checklist
Please check off each item (
[x]) once complete, or mark it as[N/A]if it doesn't apply:CONTRIBUTORS.md(alphabetically by first name)CHANGELOG.md:pytest test/locally and ensured all tests passRefer to the contributing guide if you're unsure about any of the steps.