[pull] develop from isaac-sim:develop - #62
Merged
Merged
Conversation
# Description Follow-up to #7967, which fixed this for PhysX. The OVPhysX joint-wrench sensor had the same extra frame transform. OVPhysX reports `LINK_INCOMING_JOINT_FORCE` in the **child-side joint frame at the joint anchor**, the same convention as PhysX. `isaaclab_ov/sensors/joint_wrench/kernels.py` treated that reading as "body1's frame at body1's origin" and shifted/rotated it again by the child joint pose (`localPos1`/`localRot1`). Readings were therefore wrong whenever that pose was not identity. This PR mirrors the PhysX correction in #7967 and makes the physical contract explicit across all three backends: - The OVPhysX split kernel passes force and torque through; unused USD joint-frame buffers are removed. - The shared `test_joint_wrench_frame` lives in `source/isaaclab/test/sensors/joint_wrench_contract.py`. Newton, PhysX, and OVPhysX import it directly into their existing sensor suites and provide their existing `sim` fixtures, following the FrameView contract-test pattern. - The runtime test utility, forwarding test functions, and circular OVPhysX frame-transform test are removed. The same physical scene and analytic force/torque expectations remain shared. - A small structural test checks that every joint-wrench backend imports the shared tests, rejects backend dependencies in the contract, and prevents restoration of the old utility. - OVPhysX uses the existing `device_split` CI marker so CPU and CUDA cases run in separate processes instead of silently skipping one device. ## Measurement (OVPhysX 0.6.3, before this PR) The setup is a 0.5 kg tool welded to an arm. The tool's COM is 0.15 m past the wrist anchor. I authored the child and parent frames of the weld as below and compared the tool's reading with the analytic wrench. | Child joint frame | Sensor force | Sensor torque | Analytic (joint frame, at anchor) | Raw OVPhysX | |---|---|---|---|---| | identity | (0, 0, 4.905) | (0, -0.736, 0) | (0, 0, 4.905) / (0, -0.736, 0) | = analytic | | 90° about z | (0, 0, 4.905) | (0, **+0.736**, 0) | (0, 0, 4.905) / (-0.736, 0, 0) | = analytic | | anchor offset (lever 0.25 m) | (0, 0, 4.905) | (0, **-1.717**, 0) | (0, 0, 4.905) / (0, -1.226, 0) | = analytic | | 90° about z + offset | (0, 0, 4.905) | (**-0.491**, **1.226**, 0) | (0, 0, 4.905) / (-1.226, 0, 0) | = analytic | | 90° about x | (0, 0, **-4.905**) | (0, **0.736**, 0) | (0, 4.905, 0) / (0, 0, 0.736) | = analytic | In every case the raw OVPhysX tensor already matches the analytic wrench, and only the extra transform makes the sensor output wrong. With a 90° rotation about x, the reported force even points the wrong way. ## Type of change - Bug fix (non-breaking change which fixes an issue) - Readings change only for joints whose child frame is not identity. - The documented `incoming_joint_frame` convention is unchanged. ## Tests Local validation with OVPhysX 0.6.3, running backends in separate processes (CUDA on an RTX 5090): - Newton joint-wrench suite: 14 passed. - PhysX joint-wrench suite: 16 passed. - OVPhysX CPU pass (`-k 'cpu or not cuda'`): 12 passed, including the device-independent pre-init test. - OVPhysX CUDA pass (`-k cuda`): 11 passed. - Shared-contract structural gate: 1 passed. Removing the OVPhysX contract import makes it fail. - Restoring the original OVPhysX sensor and kernel makes the shared physical test fail on both CPU and CUDA with a maximum force-component error of 19.62 N. - File-by-file architecture audit and `uv run --no-sync isaaclab -f` passed. The physical scenario count is unchanged; the existing per-backend frame tests now collect the same test function. Full PR CI remains pending. ## Checklist - [x] I have read and understood the contribution guidelines - [x] I have run the `pre-commit` checks with `./isaaclab.sh --format` - [x] I have made corresponding changes to the documentation (kernel docstring) - [x] I have added tests that prove my fix is effective or that my feature works - [x] I have added a changelog fragment for every touched package - [x] I have added my name to the `CONTRIBUTORS.md` or my name already exists there cc @ooctipus --------- Signed-off-by: Lynn <lynnhe02@gmail.com> Co-authored-by: Octi Zhang <zhengyuz@nvidia.com>
# Description `DifferentialInverseKinematicsAction._compute_frame_jacobian` and `OperationalSpaceControllerAction._compute_ee_jacobian` shift the body-origin Jacobian to the `body_offset` frame. `jacobian_b` is the body-origin geometric Jacobian expressed in the **root** frame. The previous correction had two problems: 1. **Translational rows.** The correction used `-[r]× J_w` with `r = body_offset.pos` in the **body** frame. The offset point moves with `v + w × (R_body_b r)`, so the lever arm has to be rotated into the root frame first. The two only coincide when the body's orientation in the root frame is identity. For the Franka IK tasks (`pos=(0, 0, 0.107)`, hand pointing down), the lever arm's sign is effectively flipped. 2. **Angular rows.** They were multiplied by `R(body_offset.rot)`. The offset frame is rigidly attached to the body, so its angular velocity in the root frame equals the body's, and the angular rows must stay unchanged. The same terms already compute the offset frame's **pose** correctly (`combine_frame_transforms(link_pose_b, offset)`). The OSC term also computes its **velocity** correctly (`v + w × quat_apply(link_quat_b, offset_pos)`, with a comment that angular velocity is unaffected). So the Jacobian was inconsistent with both. Both action terms apply the correction inline. They compute the body orientation in the root frame directly from the asset data, so the result does not depend on whether `_compute_ee_pose` ran earlier in the step: the OSC `apply_actions` computes the Jacobian before the pose. ## Type of change - Bug fix (non-breaking change which fixes an issue) ## Tests - Extended `source/isaaclab/test/envs/test_diffik_jacobian_aliasing.py` with the body-offset regression (kit-free `unit`, parametrized over DiffIK and OSC), reusing its existing stub. - Setup: one revolute joint about z, with the body rotated 90° about z. - With offset `(1, 0, 0)`, the expected linear column is `z × (0, 1, 0) = (-1, 0, 0)`. With a 90° offset rotation about x, the angular column must stay `(0, 0, 1)`. - **Updated** `test_diffik_jacobian_aliasing.py::test_compute_frame_jacobian_applies_offset_once`. - Its out-of-place reference encoded the previous formula. It now uses non-trivial root and body orientations and the rigid-offset formula. - The idempotence and owned-buffer tests are unchanged and still pass. ``` pytest -q source/isaaclab/test/envs/test_diffik_jacobian_aliasing.py # without the fix: 3 failed, 2 passed # with the fix: 5 passed ``` **Finite-difference check** (script not included in the PR): - Setup: random 7-joint serial chain in float64, offset `(0, 0, 0.107)`, 0.7 rad offset rotation. - Comparison: the Jacobian of the offset frame against finite differences of its pose. | | max linear error | max angular error | |---|---|---| | previous formula | 7.9e-2 | 6.1e-1 | | this PR | 4.7e-7 | 2.4e-7 | ## Notes for reviewers - **Changed behavior for existing tasks.** Tasks that set `body_offset` (e.g. the Franka lift/stack/cabinet IK tasks) will now use the correct Jacobian. DiffIK is closed-loop, so the previous error mostly degraded convergence rather than causing outright failures. For OSC with inertial decoupling, the mismatch between `J` and the (correct) EE velocity is removed. - **Scope.** I kept the change local to the two action terms. It should not overlap with the controller consolidation in #7854, which does not touch these methods. ## Checklist - [x] I have read and understood the contribution guidelines - [x] I have run the `pre-commit` checks with `./isaaclab.sh --format` - [x] I have added tests that prove my fix is effective or that my feature works - [x] I have added a changelog fragment under `source/isaaclab/changelog.d/` - [x] I have added my name to the `CONTRIBUTORS.md` or my name already exists there --------- Signed-off-by: Lynn <lynnhe02@gmail.com> Co-authored-by: Octi Zhang <zhengyuz@nvidia.com>
github-actions
Bot
added
documentation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.4)
Can you help keep this open source service alive? 💖 Please sponsor : )