Skip to content
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

Merged
merged 50 commits into from
Dec 17, 2024

Conversation

jtigue-bdai
Copy link
Collaborator

@jtigue-bdai jtigue-bdai commented Sep 9, 2024

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

  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

Checklist

  • I have run the pre-commit checks with ./isaaclab.sh --format
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the changelog and the corresponding version in the extension's config/extension.toml file
  • I have added my name to the CONTRIBUTORS.md or my name already exists there

@jtigue-bdai jtigue-bdai changed the title Transform RigidObjectData and ArticulationData root_state_w and body_state_w to link frame Transform root_physx_view linear velocities to link frame Sep 9, 2024
@jtigue-bdai jtigue-bdai self-assigned this Sep 10, 2024
@jtigue-bdai jtigue-bdai marked this pull request as ready for review September 10, 2024 12:59
Copy link
Collaborator

@jsmith-bdai jsmith-bdai left a 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

@Mayankm96
Copy link
Contributor

Mayankm96 commented Sep 20, 2024

Couple of points:

  • This is a breaking change from the framework point of view. I was wondering if we should make it explicit always to use root_state_link_w and root_state_com_w attributes and add a warning on root_state_w to make users switch to either of these.
  • The setters for the root state need to be adapted too. Should this now also expect velocities to be in the link frame? If so then we would need to add operations to set them in the com frame into the simulator.
  • torch.bmm is a slow operation from what I know. Did you see a perf difference because of it?
  • Missing tests for this change.

@jtigue-bdai
Copy link
Collaborator Author

Couple of points:

  • This is a breaking change from the framework point of view. I was wondering if we should make it explicit always to use root_state_link_w and root_state_com_w attributes and add a warning on root_state_w to make users switch to either of these.
  • The setters for the root state need to be adapted too. Should this now also expect velocities to be in the link frame? If so then we would need to add operations to set them in the com frame into the simulator.
  • torch.bmm is a slow operation from what I know. Did you see a perf difference because of it?
  • Missing tests for this change.
  1. Yeah we can do the separated properties. that is easy enough.
  2. I forgot about the setters, but yeah I think that would be beneficial
  3. Will torch.matmul be a better alternative? It seems like it should handle a batch matrix multiplication provided the right shape. Do we have a script/test that would give a good indication? If not I will look at it when I make the tests.
  4. Yeah I wanted to make sure an implementation was agreed on before make them. Now that we are going with the explicit properties I will work those tests out.

@jtigue-bdai
Copy link
Collaborator Author

jtigue-bdai commented Sep 25, 2024

@Mayankm96 are you envisioning that we have a deprecation warning for the root_state_w property and then from now on have the root_state_com_w and root_state_link_w properties? (and the same for body_state_w)

If so what we do we want to do with the sub properties root_pos_w, root_quat_w, root_vel_w, etc? Do we want deprecation warnings and _link _com versions on those too? This is going to get pretty kinda big and messy. I wonder if it will be more confusing from a user perspective.

@kellyguo11
Copy link
Contributor

I guess we'll also need to extend this for RigidObjectCollection class now?

@jtigue-bdai
Copy link
Collaborator Author

Oh yeah I guess so

@jtigue-bdai
Copy link
Collaborator Author

I guess we'll also need to extend this for RigidObjectCollection class now?

@kellyguo11 These have been addressed.

@kellyguo11 kellyguo11 changed the title Transform root_physx_view linear velocities to link frame Refactors pose and velocities to link frame and COM frame APIs Dec 15, 2024
@kellyguo11
Copy link
Contributor

hmm after updating the environments to the new APIs, training seems to be slowing down quite a bit. For Isaac-Repose-Cube-Shadow-Direct-v0, I'm seeing ~100k FPS before the change and ~30k FPS after. These transform computations seem to be pretty costly.

@kellyguo11
Copy link
Contributor

hmm after updating the environments to the new APIs, training seems to be slowing down quite a bit. For Isaac-Repose-Cube-Shadow-Direct-v0, I'm seeing ~100k FPS before the change and ~30k FPS after. These transform computations seem to be pretty costly.

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.

@jtigue-bdai
Copy link
Collaborator Author

jtigue-bdai commented Dec 16, 2024

hmm after updating the environments to the new APIs, training seems to be slowing down quite a bit. For Isaac-Repose-Cube-Shadow-Direct-v0, I'm seeing ~100k FPS before the change and ~30k FPS after. These transform computations seem to be pretty costly.

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?

@kellyguo11
Copy link
Contributor

hmm after updating the environments to the new APIs, training seems to be slowing down quite a bit. For Isaac-Repose-Cube-Shadow-Direct-v0, I'm seeing ~100k FPS before the change and ~30k FPS after. These transform computations seem to be pretty costly.

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.

@jtigue-bdai
Copy link
Collaborator Author

hmm after updating the environments to the new APIs, training seems to be slowing down quite a bit. For Isaac-Repose-Cube-Shadow-Direct-v0, I'm seeing ~100k FPS before the change and ~30k FPS after. These transform computations seem to be pretty costly.

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.

@kellyguo11 kellyguo11 merged commit 5d86a8b into main Dec 17, 2024
4 of 5 checks passed
@kellyguo11 kellyguo11 deleted the fix/root_state_w_vel_at_link branch December 17, 2024 04:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants