-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Refactors pose and velocities to link frame and COM frame APIs #966
Conversation
RigidObjectData
and ArticulationData
root_state_w
and body_state_w
to link frameThere 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.
Looks good, minor comments on additional commenting and potential to pull out common method
source/extensions/omni.isaac.lab/omni/isaac/lab/assets/articulation/articulation_data.py
Outdated
Show resolved
Hide resolved
source/extensions/omni.isaac.lab/omni/isaac/lab/assets/articulation/articulation_data.py
Outdated
Show resolved
Hide resolved
source/extensions/omni.isaac.lab/omni/isaac/lab/assets/articulation/articulation_data.py
Outdated
Show resolved
Hide resolved
source/extensions/omni.isaac.lab/omni/isaac/lab/assets/articulation/articulation_data.py
Outdated
Show resolved
Hide resolved
source/extensions/omni.isaac.lab/omni/isaac/lab/assets/articulation/articulation_data.py
Outdated
Show resolved
Hide resolved
source/extensions/omni.isaac.lab/omni/isaac/lab/assets/rigid_object/rigid_object_data.py
Outdated
Show resolved
Hide resolved
source/extensions/omni.isaac.lab/omni/isaac/lab/assets/rigid_object/rigid_object_data.py
Outdated
Show resolved
Hide resolved
Couple of points:
|
|
@Mayankm96 are you envisioning that we have a deprecation warning for the If so what we do we want to do with the sub properties |
source/extensions/omni.isaac.lab/omni/isaac/lab/assets/articulation/articulation.py
Show resolved
Hide resolved
Signed-off-by: Kelly Guo <[email protected]>
I guess we'll also need to extend this for RigidObjectCollection class now? |
Oh yeah I guess so |
@kellyguo11 These have been addressed. |
Signed-off-by: Kelly Guo <[email protected]>
…saacLab into fix/root_state_w_vel_at_link
…ate_w_vel_at_link
…ate_w_vel_at_link
…saacLab into fix/root_state_w_vel_at_link
hmm after updating the environments to the new APIs, training seems to be slowing down quite a bit. For |
I pushed a change to use the physics APIs directly for link pose and com vel. @jtigue-bdai let me know if this looks ok to you. I'll let this sit for another day before we merge it in. |
Hey @kellyguo11, I was worried about that, those transforms are costly. Its currently a problem for the IMU too. It seems to be passing the tests. Does your change make is faster? How much of a hit do we take? |
yup tests are passing now. the latest change helps bring back perf to pretty much where it was before, but it would only be the case if only link pose and com velocities are used. If com pose and link velocities are required (or the full states for either of them), then we'll still have to go through the transform computations, which maybe it's ok for now since we can't really work around that. |
OK cool. That sounds good for now. II think in general we need to figure out a faster way to do transforms. Maybe we can ask Physx team for getters for com_pose and link_velocities. That way the transforms are faster. Or we figure out a faster way to do transforms in IsaacLab. |
Description
Currently the root_physx_views of the rigid bodies and articulations output linear and angular velocities of the com of bodies rather than the link frame. This PR transforms the velocities and accelerations to the link frame of the body.
Fixes #942
Type of change
Checklist
pre-commit
checks with./isaaclab.sh --format
config/extension.toml
fileCONTRIBUTORS.md
or my name already exists there