Add FFW SG2 MuJoCo Model #3
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @jinw00-1, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request introduces a new MuJoCo model for the FFW SG2 robot, expanding the existing collection of robot models. It includes the necessary XML definition for the robot's structure and behavior, and an update to the README for visualization and discoverability.
Highlights
- New MuJoCo Model Added: I've added the FFW SG2 robot model to the robotis_mujoco_menagerie.
- MuJoCo XML Definition: The core of this addition is the robotis_ffw/ffw_sg2.xml file, which defines the robot's structure, joints, and actuators for simulation.
- README Documentation: The README.md has been updated to include a new section and an image for the FFW SG2 model, making it easily discoverable.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments or fill out our survey to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This PR adds a new MuJoCo model for the FFW SG2 robot. The changes look good overall, introducing the model's XML definition and updating the README. I've found a few issues in the XML file, including a potentially critical error in an inertia value, several inconsistencies between joint ranges and actuator control ranges, and some minor maintainability improvements. Please see my detailed comments.
| <worldbody> | ||
|
|
||
| <body name="base_link" pos="0 0 0.15"> | ||
| <inertial pos="-0.0457 0.0066 0.1943" mass="35.99" diaginertia="17254 39837 28461"/> |
There was a problem hiding this comment.
The diaginertia values for base_link appear to be extremely large for a mass of 35.99 kg. In MuJoCo (using SI units), inertia is typically in the order of mass * radius^2. These values (17254, 39837, 28461) might cause unrealistic simulation behavior. Please verify if these values are in the correct units (kg·m²) or if they need to be scaled. For instance, if they were specified in kg·mm², they would need to be divided by 1,000,000.
| arm_r_joint7 -> range=-1.046 1.046 | ||
| --> | ||
| <position name="actuator_arm_r_joint1" joint="arm_r_joint1" class="robot" ctrlrange="-3.14 3.14"/> | ||
| <position name="actuator_arm_r_joint2" joint="arm_r_joint2" class="robot" ctrlrange="0 3.14"/> |
There was a problem hiding this comment.
The actuator ctrlrange="0 3.14" is inconsistent with the joint's range="-3.14 0" (defined on line 217). The signs are opposite, which will likely lead to incorrect control of the joint. This seems to be a copy-paste error from the left arm. Please correct the ctrlrange to match the joint's range.
<position name="actuator_arm_r_joint2" joint="arm_r_joint2" class="robot" ctrlrange="-3.14 0"/>
| head_joint2 -> range=-0.6 0.6 | ||
| --> | ||
| <position name="actuator_head_joint1" joint="head_joint1" class="robot" ctrlrange="0 0.4"/> | ||
| <position name="actuator_head_joint2" joint="head_joint2" class="robot" ctrlrange="-0.6 0.6"/> |
There was a problem hiding this comment.
The actuator ctrlrange="-0.6 0.6" is wider than the corresponding joint's range="-0.35 0.35" (defined on line 135). While MuJoCo will clamp the values, it's best practice to have the ctrlrange be within the joint's range to avoid confusion and unexpected behavior. The comment on line 316 also seems to reflect the incorrect wider range.
<position name="actuator_head_joint2" joint="head_joint2" class="robot" ctrlrange="-0.35 0.35"/>
| <position name="actuator_arm_r_joint1" joint="arm_r_joint1" class="robot" ctrlrange="-3.14 3.14"/> | ||
| <position name="actuator_arm_r_joint2" joint="arm_r_joint2" class="robot" ctrlrange="0 3.14"/> | ||
| <position name="actuator_arm_r_joint3" joint="arm_r_joint3" class="robot" ctrlrange="-1.57 1.57"/> | ||
| <position name="actuator_arm_r_joint4" joint="arm_r_joint4" class="robot" ctrlrange="-1.86 1.86"/> |
There was a problem hiding this comment.
The actuator ctrlrange="-1.86 1.86" is not a subset of the joint's range="-2.9361 1.0786" (defined on line 227). The upper limit of the control range (1.86) is outside the joint's physical limit (1.0786). Please adjust the ctrlrange to be within the joint's range.
<position name="actuator_arm_r_joint4" joint="arm_r_joint4" class="robot" ctrlrange="-1.86 1.0786"/>
| --> | ||
| <position name="actuator_arm_l_joint1" joint="arm_l_joint1" class="robot" ctrlrange="-3.14 3.14"/> | ||
| <position name="actuator_arm_l_joint2" joint="arm_l_joint2" class="robot" ctrlrange="0 3.14"/> | ||
| <position name="actuator_arm_l_joint3" joint="arm_l_joint3" class="robot" ctrlrange="-6.28 6.28"/> |
There was a problem hiding this comment.
The actuator ctrlrange="-6.28 6.28" is wider than the joint's range="-3.14 3.14" (defined on line 153). The control range should be within the joint's physical limits.
<position name="actuator_arm_l_joint3" joint="arm_l_joint3" class="robot" ctrlrange="-3.14 3.14"/>
| A collection of models for the MuJoCo physics engine from ROBOTIS | ||
|
|
||
| ## AI Worker (FFW-SG2) | ||
| <img src="https://github.com/jinw00-1/robotis_mujoco_menagerie/raw/main/media/ffw_sg2.png" width="300"> |
There was a problem hiding this comment.
The image URL points to a personal fork (jinw00-1). For long-term maintainability, it's better to use a URL that points to the main repository, similar to the other images in this file. Once this PR is merged, the image will be available in the main repository.
| <img src="https://github.com/jinw00-1/robotis_mujoco_menagerie/raw/main/media/ffw_sg2.png" width="300"> | |
| <img src="https://raw.githubusercontent.com/ROBOTIS-GIT/robotis_mujoco_menagerie/main/media/ffw_sg2.png" width="300"> |
| <freejoint name="floating_base"/> | ||
| <geom type="mesh" rgba="0.7 0.7 0.7 1" mesh="base_mobile_assy"/> | ||
|
|
||
| <!-- 왼쪽 앞 바퀴 (조향+구동) --> |
There was a problem hiding this comment.
Some comments are in Korean, while most of the file uses English. For consistency and to make the model accessible to a wider audience, it would be best to use English for all comments. This also applies to lines 86, 97, 299, and 303.
<!-- Left front wheel (steering+driving) -->
| <!-- | ||
| <joint name="lift_joint" type="slide" axis="0 0 1" range="-0.5 0"/> | ||
| <inertial pos="-0.0460732 -2.28101e-05 -0.0270208" quat="0.295276 0.295528 -0.642514 0.642379" mass="8.72176" diaginertia="0.106112 0.0836519 0.0576143"/> | ||
| <geom type="mesh" rgba="1 1 1 1" mesh="body_arm_assy"/> | ||
| --> |
There was a problem hiding this comment.
This block of code is commented out. If it's no longer needed, it should be removed to improve code clarity and reduce clutter.
| --> | ||
|
|
||
| <inertial pos="-0.0460732 -2.28101e-05 -0.0270208" quat="0.295276 0.295528 -0.642514 0.642379" mass="8.72176" diaginertia="0.106112 0.0836519 0.0576143"/> | ||
| <joint name="lift_joint" class="slider" pos="0 0 0" axis="0 0 1" type="slide"/> |
There was a problem hiding this comment.
The type="slide" attribute is redundant here because the slider class (defined on lines 27-34) already specifies type="slide" for the joint. Removing it will make the code cleaner.
<joint name="lift_joint" class="slider" pos="0 0 0" axis="0 0 1"/>
| Right arm joints: | ||
| arm_r_joint1 -> range=-3.14 3.14 | ||
| arm_r_joint2 -> range=0 3.14 | ||
| arm_r_joint3 -> range=-1.57 1.57 | ||
| arm_r_joint4 -> range=-1.86 1.86 | ||
| arm_r_joint5 -> range=-1.57 1.57 | ||
| arm_r_joint6 -> range=-1.57 1.57 | ||
| arm_r_joint7 -> range=-1.046 1.046 | ||
| --> |
There was a problem hiding this comment.
The joint ranges listed in these comments do not match the actual range attributes defined for the joints in the <worldbody> section. For example, arm_r_joint3 is commented as range=-1.57 1.57, but defined with range="-3.14 3.14" on line 222. This is confusing and can lead to errors. Please update the comments to reflect the actual joint ranges or vice-versa to ensure consistency. This applies to other arm joints as well.
1 participant
Add FFW SG2 MuJoCo Model
This PR adds a new MuJoCo model for **FFW SG2 ** to the menagerie.
Description
The model includes: