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.

Sign up for GitHub

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

Jump to bottom

Computes Jacobian in the root frame inside the DifferentialInverseKinematicsAction class #967

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Computes Jacobian in the root frame inside the DifferentialInverseKinematicsAction class #967

wants to merge 1 commit into from
+35 −3

Conversation

zoctipus
Copy link
Contributor

Description

This pull request fix the bug where jacobian returned by DifferentialInverseKinematicsAction._compute_fame_jacobian are not truly in robot base frame because it did not consider robot's base rotation. After the change, _compute_fame_jacobian returns the correct local frame jacobian. In addition, properties get_jacobian_w and get_jacobian_b is added to differentiate frame differences

Fixes #911

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Screenshots

image

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

Mayankm96
Mayankm96 reviewed Sep 10, 2024
View reviewed changes
source/extensions/omni.isaac.lab/omni/isaac/lab/envs/mdp/actions/task_space_actions.py Outdated
Comment on lines 118 to 131
@property
def get_jacobian_w(self) -> torch.Tensor:
return self._asset.root_physx_view.get_jacobians()[:, self._jacobi_body_idx, :, self._joint_ids]

@property
def get_jacobian_b(self) -> torch.Tensor:
jacobian = self.get_jacobian_w
base_rot = self._asset.data.root_quat_w
base_rot_matrix = math_utils.matrix_from_quat(math_utils.quat_inv(base_rot))
jacobian[:, :3, :] = torch.bmm(base_rot_matrix, jacobian[:, :3, :])
jacobian[:, 3:, :] = torch.bmm(base_rot_matrix, jacobian[:, 3:, :])
return jacobian
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason to make these properties? If it is only used at one place then you can just keep it there?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

jtigue suggested that the physx's get_jacobians() is not clear that the returned is in world frame, and keep it there solves the issue but introduced unstated feature, so I instead used property. I understand that use property in only one place is a bit overkill, and I can change it back if that makes more sense

Mayankm96
Mayankm96 requested changes Sep 10, 2024
View reviewed changes
Copy link
Contributor

@Mayankm96 Mayankm96 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@zoctipus Thanks a lot for the fix!

Could the unit test here be adapted to make sure the feature works as expected?

https://github.com/isaac-sim/IsaacLab/blob/main/source/extensions/omni.isaac.lab/test/controllers/test_differential_ik.py

Also, you don't need to post a screenshot of the git diff. The screenshot is only for showing results in behavior (not code).

@Mayankm96 Mayankm96 changed the title added base rotation to jacobian when compute_fame_jacobian in DifferentialInverseKinematicsAction Computes Jacobian in the root frame inside the DifferentialInverseKinematicsAction class Sep 10, 2024
@Mayankm96 Mayankm96 added the bug Something isn't working label Sep 10, 2024
@ozhanozen ozhanozen mentioned this pull request Sep 13, 2024
Open
6 tasks
@zoctipus
Copy link
Contributor Author

zoctipus commented Sep 22, 2024 •
edited
Loading

@Mayankm96
Thanks for Reviewing, and sorry for the delay, my last week schedule was overwhelming.

I have added tests for checking the implement, all tests are passing!
however, there are some concerns that worries me:

  1. the test is only for class:DifferentialIKController but not for class:DifferentialInverseKinematicsAction. DifferentialIKController doesn't calculates Jacobian but depends on Jacobian calculated by either physx or DifferentialInverseKinematicsAction. This bug fix in DifferentialInverseKinematicsAction, namely base frame's jacobian transformation formula, is not referenced by the test suite because test suite didn't use DifferentialInverseKinematicsAction. Instead this formula is re-implemented to make sure the base-frame-transformation is applied before given to DifferentialIKController. This worries me because there are two implementations of same formulat, one in unittest and the other one in DifferentialInverseKinematicsAction, and if at some day one formula changes, the other will not be sync and causing invalid test case or bugs. Perhaps there needs to be a test suite that specifically test for DifferentialInverseKinematicsAction?

  2. There are two unit tests for DifferentialIKController in https://github.com/isaac-sim/IsaacLab/blob/main/source/extensions/omni.isaac.lab/test/controllers/test_differential_ik.py , one for franka, and the other for UR10. UR 10 test is failing because UR10 is not able to be imported properly. Specifically,

# spawn single instance
prim = func(prim_paths[0], cfg, *args, **kwargs)

Will not populates UR10's links and joints into stage. Only an empty /World/envs/env_0/Robot will be created

Upon further investigation, it seems like this is an issue when ISAACLAB_NUCLEUS_DIR points to 4.2. Changing 4.2 to 4.1 resolves import issue.

Buggy usd path:
'http://omniverse-content-production.s3-us-west-2.amazonaws.com/Assets/Isaac/4.2/Isaac/IsaacLab/Robots/UniversalRobots/UR10/ur10_instanceable.usd'

OK usd path:
'http://omniverse-content-production.s3-us-west-2.amazonaws.com/Assets/Isaac/4.1/Isaac/IsaacLab/Robots/UniversalRobots/UR10/ur10_instanceable.usd'

with OK usd path, both tests can pass

@zoctipus
Copy link
Contributor Author

Thanks for the fix!

@zoctipus
Copy link
Contributor Author

I just rebased the branch to track the newest Isaac Iab and fixed the versioning,
Please have a view and let me know what else I can fix!
@lorenwel @Mayankm96 @jtigue-bdai
Thanks for your guys help!

@zoctipus
Copy link
Contributor Author

I have updated versioning in CHANGE LOG and extension.toml, please take a look when you have time! Thanks! @jsmith-bdai @Mayankm96

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lorenwel lorenwel lorenwel requested changes

@jsmith-bdai jsmith-bdai Awaiting requested review from jsmith-bdai jsmith-bdai is a code owner

@Dhoeller19 Dhoeller19 Awaiting requested review from Dhoeller19 Dhoeller19 is a code owner

@kellyguo11 kellyguo11 Awaiting requested review from kellyguo11 kellyguo11 is a code owner

@Mayankm96 Mayankm96 Awaiting requested review from Mayankm96 Mayankm96 is a code owner

@hhansen-bdai hhansen-bdai Awaiting requested review from hhansen-bdai

@pascal-roth pascal-roth Awaiting requested review from pascal-roth

@jtigue-bdai jtigue-bdai Awaiting requested review from jtigue-bdai

@nikitardn nikitardn Awaiting requested review from nikitardn

@masoudmoghani masoudmoghani Awaiting requested review from masoudmoghani

Requested changes must be addressed to merge this pull request.

Assignees

@zoctipus zoctipus

Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug Report] Ik Absolute seems to not work properly with rotated base frame
3 participants
@zoctipus @lorenwel @Mayankm96