Adds position to set external forces and torques #1680
Conversation
AntoineRichard
requested review from
jsmith-bdai,
kellyguo11,
Mayankm96 and
jtigue-bdai
as code owners
|
Thanks for adding in the wrench positions. I think your current implementation is good for now. We are trying to address the com/link frame issue from the physics side so that we can avoid the additional transformations on our end. |
| articulation.write_root_link_pose_to_sim(root_state[:, :7]) | ||
| articulation.write_root_com_velocity_to_sim(root_state[:, 7:]) |
There was a problem hiding this comment.
FYI update to the latest main you will find that we have reverted the "root_link" vs "root_com" deprecation. Please use the previous "root_" API.
| # Reset the external wrench after writing to the simulation. | ||
| self.uses_external_wrench_positions = False | ||
| # Fill the positions tensor with zeros to avoid applying the wrench at the same position in the next step. | ||
| self._external_wrench_positions_b.fill_(0.0) |
There was a problem hiding this comment.
Hm since the external force is applied over multiple simulation steps under the environment decimation loop, resetting here will cause issues.
| torque_data=self._external_torque_b.view(-1, 3), | ||
| position_data=None, | ||
| indices=self._ALL_INDICES, | ||
| is_global=False, |
There was a problem hiding this comment.
Shall we also expose is_global flag?
Related MR: #1223
|
Sorry for the delay on this I've been meaning to update it but I was waiting on 4.5 before rebasing, and now I'm stuck in IROS submissions. @Mayankm96 Yes, indeed! I fixed it on my branch, but we're still using 4.2. For the application in the global frame I'm wondering if it wouldn't be better to perform the transformation on IsaacLab's side, this way some indices could be global while others could be local? Let me know! |
AntoineRichard
force-pushed
the
add_position_to_set_external_forces_and_torques
branch
from
ff071b6 to
39fb0b9
Compare
kellyguo11
changed the title
|
Suggestion: Would it be possible to extend this functionality to rigid bodies as well? Specifically, it would involve updating |
AntoineRichard
force-pushed
the
add_position_to_set_external_forces_and_torques
branch
from
99c3e68 to
2781c43
Compare
| torque_data=self._external_torque_b.view(-1, 3), | ||
| position_data=self._external_wrench_positions_b.view(-1, 3), | ||
| indices=self._ALL_INDICES, | ||
| is_global=False, |
There was a problem hiding this comment.
should we expose this in the same PR for simplicity?
There was a problem hiding this comment.
I'm not fond of exposing the global flag. The main issue I have with this is say we do:
art.set_external_force_and_torque(force, torque, position=position, body_id = body_1, global=True)
art.set_external_force_and_torque(force, torque, position=position, body_id = body_2, global=False)Then it will use global = false for both. I think it's misleading.
There was a problem hiding this comment.
Or we could raise a warning / error if the global flag is changed more than once in a single simulation step for a given articulation / body(ies). But still it feels wrong.
There was a problem hiding this comment.
I see, would it make sense to have it as a parameter in either the simulation cfg or articulation cfg? so it can be specified on a per-sim level or per-articulation level
There was a problem hiding this comment.
Let's do it on a per articulation level. Default will be local.
There was a problem hiding this comment.
hmm, I guess the downside with this approach is we can't control the frame for rigid bodies that are not an articulation. would that make sense to support if we expose the flag for RigidObjectCfg and RigidObjectCollectionCfg as well?
There was a problem hiding this comment.
Oh yes, sorry it completely skipped my that I had to replicate it on the other assets...
… wrenches into the global frame.
… wrenches into the global frame.
…e to the Object and Object collections.
Description
This PR adds the option to set wrench positions to
set_external_force_and_torque. This is a non-breaking change as the positions are passed as an optional argument. When no positions are set, the function defaults to the original implementation, that is, no positions are passed to PhysX. The PR also adds tests to check that the position values are correctly set into their buffer, but does not check if the resulting wrenches are correct. I did test the Quadcopter task before and after this PR and the training results are exactly the same.As of now, the function follows the original layout. But it could make sense to offer the option to set the position in either the link frame or the CoM frame. This would follow the recent changes made to the set_pose and set_velocity methods for instance. However, this would be a breaking change. Hence, for now, this has not been implemented. One could also argue, that this could be done prior to feeding the positions outside this method. Please let me know what you feel is best, and I'll update the PR accordingly.
If one wanted to test the resulting wrenches, it would require a simple test articulation like a 1kg sphere that would be used to accurately compute the expected velocities. This is also feasible, but I feel like this test is more on the PhysX side of things, let me know.
This change will require an update of the API documentation to include the position argument.
Fixes #1678
Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there