-
Notifications
You must be signed in to change notification settings - Fork 26
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
Apply external forces #124
Apply external forces #124
Conversation
7a3a657
to
a98480d
Compare
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.
Minor comments that do not block merge.
ignition/src/IgnitionRobot.cpp
Outdated
const std::array<double, 3>& torque) | ||
{ | ||
LinkEntity linkEntity = pImpl->getLinkEntity(linkName); | ||
assert(linkEntity != ignition::gazebo::kNullEntity); |
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.
The user of assert here means that if the linkname is wrong, the Python interpreter will crash if the plugin has been compiled in debug, or it will silently fail if it is compiled in Release. Could it make sense to propagate the error instead?
assert(linkEntity != ignition::gazebo::kNullEntity); | |
if (linkEntity != ignition::gazebo::kNullEntity) { | |
return false; | |
} |
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 makes sense, and it is a pattern that was used in the same file:
gym-ignition/ignition/src/IgnitionRobot.cpp
Lines 980 to 983 in 7292f50
JointEntity jointEntity = pImpl->getJointEntity(jointName); | |
if (jointEntity == ignition::gazebo::kNullEntity) { | |
return false; | |
} |
However, for some reason there are other methods that use the assert. I will change this line here, and will address the other ones in a separated PR. Thanks!
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.
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.
Check this diff for the changes https://github.com/robotology/gym-ignition/compare/a98480d2d0ce9647f81ea7de49263d8fe2281611..af4c9d750f953f87da4eeede155e9ab9aa1ea43d
Perfect!
|
||
The force and the torque are applied at the link origin. | ||
The force is expressed in the world frame. | ||
The torque is expressed with the orientation of the world frame. |
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.
To clarify possible confusion with an API that permanently set the force (in the sense that after you set it, you need it to reset to zero) could it make sense to add:
The torque is expressed with the orientation of the world frame. | |
The torque is expressed with the orientation of the world frame. | |
The external force and torque is only applied for the next simulation step, | |
and then is automatically reset to zero. To apply a force for a more then one step, | |
call `apply_external_force` before each step. |
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! I agree that this is a important piece of information.
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.
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.
Check this diff for the changes https://github.com/robotology/gym-ignition/compare/a98480d2d0ce9647f81ea7de49263d8fe2281611..af4c9d750f953f87da4eeede155e9ab9aa1ea43d
Perfect!
a98480d
to
af4c9d7
Compare
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.
Totally unrelated, but eventually it would be great if it would be possible to visualize this external force.
I agree. And beyond visualizing, also applying forces through the GUI would be great. Right now that could be done through ignition transport, not the best way but at least it's compatible with online usage. |
cube = utils.CubeGazeboRobot(model_file=model_file, | ||
gazebo=gazebo.simulator, | ||
initial_position=np.array([0, 0, 1.0])) | ||
|
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.
Just out of curiosity: Why has it been chosen to use serialization and then pass a file to CubeGazeboRobot
in this case, and not pass directly the utils.get_cube_urdf()
output?
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.
If you check https://github.com/robotology/gym-ignition/pull/124/files/af4c9d750f953f87da4eeede155e9ab9aa1ea43d#diff-2fa3603fe004643ef39d4d16c04a3003R132, the get_cube_urdf
returnes a string. Robot objects like CubeGazeboRobot
want a model_file
argument containing a file name.
I didn't want to create an extra file stored in the repository only for the tests. Since the urdf in this case is trivial and the computation of different quantities like inertia is done on the fly, I preferred to store it as a string and serialize it to a temporary file when needed.
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, I see. Thanks.
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.
No changes
I agree. |
This PR adds the support of applying external forces and torques to model's links. The component read by the Physics system unfortunately is a transport message, and due to this choice the way to fill the data handled by the component is not that clean.
Closes #121