[Newton] Backend-agnostic task-space accessors for IK/OSC#5400
Conversation
Task-space controllers (IK, OSC, RMPFlow) previously called PhysX-only
methods directly on `asset.root_view` — `get_jacobians`,
`get_generalized_mass_matrices`, `get_gravity_compensation_forces`. On
Newton these crashed with AttributeError because the corresponding
view (`newton.selection.ArticulationView`) exposes a different API
(`eval_jacobian`, `eval_mass_matrix`, no gravity-compensation
primitive).
Add three abstract methods on `BaseArticulation`:
* `get_jacobians()` — per-env spatial Jacobians.
* `get_mass_matrix()` — per-env joint-space mass matrix.
* `get_gravity_compensation_forces()` — per-env joint torques to hold
against gravity.
PhysX implements all three as thin passthroughs. Newton wraps
`eval_jacobian` and `eval_mass_matrix`, with all source / output /
scratch buffers pre-allocated in `_create_buffers` (CUDA-graph-capture
safe). Two Warp gather kernels copy the rows owned by each
ArticulationView out of Newton's model-sized output, applying a
`link_offset` of 1 for fixed-base articulations to drop Newton's
fixed-root row and match the PhysX shape contract. Newton's gravity
stub raises NotImplementedError; ovphysx mirrors with stubs since it
has no view-level access at all.
Migrate every direct caller to the new accessors:
* Action terms: DifferentialInverseKinematicsAction,
OperationalSpaceControllerAction, RMPFlowTaskSpaceAction,
PinkTaskSpaceAction.
* Direct-workflow envs: factory, automate (assembly + disassembly),
deploy events.
* Tests: test_differential_ik, test_operational_space.
Gate OSC's per-step fetches in `_compute_dynamic_quantities`: mass
matrix only when `inertial_dynamics_decoupling` or
`nullspace_control != "none"`; gravity comp only when
`gravity_compensation`. Previously both were fetched unconditionally,
which (a) crashed on Newton even when the values were unused and
(b) was wasted work on PhysX.
Remove the `self.sim.physics = PhysxCfg(...)` workaround from
`Isaac-Reach-Franka-{IK-Abs,IK-Rel,OSC}-v0`. The previous comment
*"{IK,OSC} control is not supported with Newton physics; use PhysX
only"* no longer holds. All three tasks now inherit the
`ReachPhysicsCfg` preset, so `presets=newton` selects `NewtonCfg` and
the env runs under Newton without further overrides. The default OSC
config keeps `gravity_compensation=False` so the Newton path doesn't
hit the upstream-blocked gravity primitive.
Pin the upstream Newton gap with a regression test
(`test_get_gravity_compensation_forces_not_implemented_on_newton`)
that asserts `NotImplementedError` is raised. When upstream Newton
ships an `eval_gravity_compensation` API and the wrapper switches to
a real implementation, the test fails with `DID NOT RAISE` — directing
the maintainer to remove the stub and the OSC config-time guidance.
Tighten the floating-base OSC indexing regression test by enabling
both `inertial_dynamics_decoupling=True` and
`gravity_compensation=True` in its OSCCfg, so the new flag-gating
doesn't short-circuit the indexing checks the test was written to
verify.
Validation:
* PhysX: test_differential_ik 2/2, test_operational_space 18/18
(including the floating-base regression).
* Newton: Isaac-Reach-Franka-{IK-Abs,IK-Rel,OSC}-v0 random_agent
smokes ran zero-error through thousands of physics substeps.
* Codex review pass (4 P1/P2 findings — all addressed).
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
This PR introduces backend-agnostic task-space accessors (get_jacobians, get_mass_matrix, get_gravity_compensation_forces) on BaseArticulation to enable IK/OSC/RMPFlow controllers to work under both PhysX and Newton backends. The implementation is well-designed with proper CUDA graph capture safety, pre-allocated buffers, and correct gating of dynamic quantity fetches. The core approach is sound, though there are a few issues worth addressing.
Architecture Impact
High impact, well-contained. The changes span the articulation abstraction layer (BaseArticulation), all three backend implementations (PhysX, Newton, ovphysx), the task-space action terms, and downstream task configs. The Newton implementation introduces new gather kernels that extract view-specific rows from model-sized buffers. The OSC action term now conditionally fetches mass matrix and gravity compensation based on controller config flags, which is the correct pattern for backend compatibility.
Implementation Verdict
Minor fixes needed — One potential correctness issue with the mass matrix gating logic and a few smaller concerns.
Test Coverage
- ✅
test_get_gravity_compensation_forces_not_implemented_on_newtonpins the known Newton gap - ✅
test_operational_space.pyupdated to enablegravity_compensation=Truefor floating-base test ⚠️ Missing: No direct unit test forget_jacobians()orget_mass_matrix()on Newton verifying shape/value correctness against PhysX⚠️ Missing: No test exercising OSC withinertial_dynamics_decoupling=Falseandnullspace_control="position"to verify the new gating logic
CI Status
No CI checks available yet — recommend waiting for CI before merge.
Findings
🔴 Critical: source/isaaclab/isaaclab/envs/mdp/actions/task_space_actions.py:659-663 — Mass matrix gating may skip required data for nullspace control
The condition needs_mass_matrix = self.cfg.controller_cfg.inertial_dynamics_decoupling or (self.cfg.controller_cfg.nullspace_control != "none") is correct, but the _mass_matrix tensor is initialized to zeros in __init__ (line 409) and never cleared between steps. If a user toggles config at runtime (unlikely but possible), or if the controller internally assumes the mass matrix is always fresh, stale zeros could cause issues. More importantly, if needs_mass_matrix is False, _mass_matrix remains zeros and gets passed to self._osc.compute() at line 538. The OSC controller's compute() method receives mass_matrix unconditionally — verify the controller handles zero mass matrices gracefully when neither flag is set.
# Line 538 always passes self._mass_matrix regardless of gating
self._joint_efforts[:] = self._osc.compute(
...
mass_matrix=self._mass_matrix, # Could be zeros if not fetched
...
)🟡 Warning: source/isaaclab_newton/isaaclab_newton/assets/articulation/articulation.py:226-240 — Redundant jacobian evaluation in get_mass_matrix
When get_jacobians() and get_mass_matrix() are called in the same step (common for OSC), eval_jacobian is called twice on the same state. The comment acknowledges this ("any earlier get_jacobians this step is overwritten with identical data"), but this is still a performance overhead. Consider caching the jacobian evaluation per-state or adding a combined accessor.
🟡 Warning: source/isaaclab_newton/isaaclab_newton/assets/articulation/kernels.py:581 — gather_jacobian_rows uses int instead of wp.int32
def gather_jacobian_rows(
...
link_offset: int, # Should be wp.int32 for Warp kernel type consistency
...
):While Warp may implicitly handle Python int, using wp.int32 would be more consistent with other kernel signatures in the codebase.
🟡 Warning: source/isaaclab_newton/isaaclab_newton/assets/articulation/articulation.py:3320-3325 — Jacobian view buffer shape uses max_links from model
The Newton implementation uses model.max_joints_per_articulation for max_links, but the comment at line 3320 says "Source buffer covers every articulation in the model." For scenes with multiple articulation types (e.g., Franka + mobile base), max_links and max_dofs are model-wide maximums. This is correct for the source buffer, but the view buffer at line 3340 should ideally use this view's actual link/dof count rather than model maximums to avoid over-allocation. This isn't a bug (the gather kernel respects bounds), but wastes memory.
🔵 Improvement: source/isaaclab/isaaclab/envs/mdp/actions/task_space_actions.py:654-668 — Consider caching needs_mass_matrix flag
The needs_mass_matrix computation happens every step in _compute_dynamic_quantities(). Since self.cfg.controller_cfg is immutable after init, this boolean could be computed once in __init__ and stored as self._needs_mass_matrix and self._needs_gravity.
🔵 Improvement: source/isaaclab_newton/test/assets/test_articulation.py:2604-2640 — Test should also verify jacobian/mass_matrix shapes
The new test only checks get_gravity_compensation_forces raises NotImplementedError. Consider adding companion tests that verify get_jacobians() and get_mass_matrix() return tensors with the expected shapes (num_instances, num_jacobi_bodies, 6, num_jacobi_joints) and (num_instances, num_jacobi_joints, num_jacobi_joints) respectively, matching the PhysX contract.
🔵 Improvement: source/isaaclab_physx/isaaclab_physx/assets/articulation/articulation.py:182-190 — PhysX method naming inconsistency
The base class defines get_mass_matrix() but PhysX's underlying view method is get_generalized_mass_matrices() (plural). The passthrough is correct, but a brief comment noting this naming difference would help maintainability:
def get_mass_matrix(self) -> wp.array:
# PhysX view uses plural "matrices" naming convention
return self._root_view.get_generalized_mass_matrices()
|
@greptileai review |
Greptile SummaryThis PR adds three backend-agnostic abstract accessors ( Confidence Score: 5/5Safe to merge; all findings are P2 style/optimisation suggestions with no correctness impact. The design is sound: fixed-base link_offset handling is correct, eval_mass_matrix's J-as-input gotcha is addressed, buffers are pre-allocated for CUDA-graph capture safety, and the xfail(strict=True) test correctly pins the Newton gravity-comp gap. The only findings are a redundant eval_jacobian dispatch when OSC fetches both Jacobian and mass matrix in the same step (P2 perf), an unguarded dtype assumption on articulation_ids (P2 defensive), and a missing clarifying comment on gather_mass_matrix_rows (P2 docs). source/isaaclab_newton/isaaclab_newton/assets/articulation/articulation.py — double eval_jacobian and articulation_ids dtype; both are P2. Important Files Changed
Sequence DiagramsequenceDiagram
participant AT as ActionTerm (IK/OSC/RMPFlow)
participant BA as BaseArticulation
participant NW as Newton Articulation
participant PX as PhysX Articulation
participant WP as Warp Kernel
AT->>BA: get_jacobians()
alt Newton backend
BA->>NW: get_jacobians()
NW->>NW: eval_jacobian(state, J=_jacobian_buf_flat)
NW->>WP: gather_jacobian_rows(src, art_ids, link_offset)
WP-->>NW: _jacobian_view_buf
NW-->>AT: (N, num_bodies-1, 6, num_joints)
else PhysX backend
BA->>PX: get_jacobians()
PX->>PX: root_view.get_jacobians()
PX-->>AT: (N, num_bodies-1, 6, num_joints)
end
AT->>BA: get_mass_matrix()
alt Newton backend
BA->>NW: get_mass_matrix()
NW->>NW: eval_jacobian (redundant if get_jacobians already called)
NW->>NW: eval_mass_matrix(H, J=_jacobian_buf_flat)
NW->>WP: gather_mass_matrix_rows(src, art_ids)
WP-->>NW: _mass_matrix_view_buf
NW-->>AT: (N, num_joints, num_joints)
else PhysX backend
BA->>PX: get_mass_matrix()
PX->>PX: root_view.get_generalized_mass_matrices()
PX-->>AT: (N, num_joints, num_joints)
end
AT->>BA: get_gravity_compensation_forces()
alt Newton backend
BA->>NW: get_gravity_compensation_forces()
NW-->>AT: NotImplementedError
else PhysX backend
BA->>PX: get_gravity_compensation_forces()
PX->>PX: root_view.get_gravity_compensation_forces()
PX-->>AT: (N, num_joints)
end
Reviews (5): Last reviewed commit: "Merge remote-tracking branch 'origin/dev..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if self._asset.is_fixed_base: | ||
| gravity = torch.zeros_like( | ||
| wp.to_torch(self._asset.root_view.get_gravity_compensation_forces())[ | ||
| :, self._controlled_joint_ids_tensor | ||
| ] | ||
| wp.to_torch(self._asset.get_gravity_compensation_forces())[:, self._controlled_joint_ids_tensor] | ||
| ) |
There was a problem hiding this comment.
get_gravity_compensation_forces() call on fixed-base
_apply_gravity_compensation calls get_gravity_compensation_forces() on a fixed-base robot solely to get the tensor shape for torch.zeros_like. On Newton, this immediately raises NotImplementedError, so any fixed-base robot using PinkInverseKinematicsAction with gravity not disabled (rigid_props.disable_gravity=False) will crash at runtime — even though the intended result is all zeros and the underlying value is never used.
| if self._asset.is_fixed_base: | |
| gravity = torch.zeros_like( | |
| wp.to_torch(self._asset.root_view.get_gravity_compensation_forces())[ | |
| :, self._controlled_joint_ids_tensor | |
| ] | |
| wp.to_torch(self._asset.get_gravity_compensation_forces())[:, self._controlled_joint_ids_tensor] | |
| ) | |
| if self._asset.is_fixed_base: | |
| gravity = torch.zeros( | |
| (self._asset.num_instances, len(self._controlled_joint_ids_tensor)), | |
| device=self.device, | |
| ) |
There was a problem hiding this comment.
I do think crash is the right behavior, otheriwise it's hiding problem if user turns on gravity comp with newton.
| gravity = wp.to_torch(self._asset.get_gravity_compensation_forces())[ | ||
| :, self._controlled_joint_ids_tensor + self._physx_floating_joint_indices_offset | ||
| ] |
There was a problem hiding this comment.
_physx_floating_joint_indices_offset embeds a PhysX convention (6 virtual free-floating DOFs prepended to gravity-comp output). When Newton eventually implements get_gravity_compensation_forces(), its output likely won't include those 6 virtual DOFs — so applying this offset here would silently read the wrong joints. Consider adding a comment or an assertion that ties this indexing to the PhysX backend, so a future implementor knows to revisit it.
* pink_task_space_actions.py: fixed-base gravity-comp path now constructs the zeros tensor directly. Previously called get_gravity_compensation_forces() solely to feed torch.zeros_like for shape, which crashes on Newton. * task_space_actions.py: cache _needs_mass_matrix / _needs_gravity at init (controller cfg is immutable). Pass None into OSC.compute() when those flags are False instead of forwarding the stale-zero buffer; the controller's existing None-checks then raise immediately on any misconfiguration rather than silently operating on zeros. * Newton gather_jacobian_rows: type link_offset as wp.int32 to match the kernel-signature convention used elsewhere in this module. * test_get_gravity_compensation_forces_not_implemented_on_newton: switch from pytest.raises to pytest.mark.xfail(strict=True, raises=...). Same contract (test fails when Newton ships the primitive), cleaner CI semantics — pytest reports XFAIL/XPASS instead of looking like a real failure when the gap is closed. * PhysX passthrough: comment noting that get_generalized_mass_matrices (plural) is PhysX's view-side name; the singular IsaacLab API (get_mass_matrix) hides the difference.
|
@greptileai review |
* Newton get_jacobians / get_mass_matrix output buffers were sized using ``model.max_joints_per_articulation`` and ``model.max_dofs_per_articulation`` — the model-wide maxima. In a homogeneous scene this matched per-asset counts, but in a heterogeneous scene where another articulation type has more joints/DoFs, this view's returned tensor would carry zero-padded rows/cols that don't belong to it. The padded mass-matrix was singular under torch.inverse. Output buffers now use ``self.num_bodies`` / ``self.num_joints`` so the returned shape always matches THIS view's asset, restoring the cross-backend contract. Source and scratch buffers stay model-sized — the kernel needs that to match Newton's writes. * Bump isaaclab_ovphysx 0.1.1 -> 0.1.2 with a CHANGELOG entry covering the new BaseArticulation.get_* stubs added in the previous commit. Per the per-extension changelog/version rule.
|
@greptileai review |
Pin the public shape contract for ``get_jacobians`` and
``get_mass_matrix`` so future regressions fail at test time instead of
in production.
* Newton: 5 new tests in test_articulation.py
- test_get_jacobians_shape_fixed_base — Franka fixed-base, locks
(N, num_bodies-1, 6, num_joints) and the link_offset=1 fix.
- test_get_mass_matrix_shape_and_nonsingular_fixed_base — Franka
fixed-base, locks (N, num_joints, num_joints) and that the matrix
has positive diagonals (a padded zero row would surface here).
- test_get_jacobians_shape_floating_base — Anymal-C floating-base,
locks (N, num_bodies, 6, num_joints) and link_offset=0 path.
- test_get_mass_matrix_shape_floating_base — Anymal-C, mass-matrix
shape on floating-base.
- test_heterogeneous_scene_per_view_shapes — co-resident Franka +
Anymal-C in one Newton model. Direct regression test for the
Codex round-2 finding: each view's output uses its own
num_bodies/num_joints, NOT model.max_*. ``num_per_type=1`` keeps
the test off Newton's pre-existing actuator-default replication
quirk in multi-instance multi-type scenes.
* PhysX: 3 new tests in test_articulation.py
- Mirror the Franka fixed-base + Anymal floating-base shape tests
so the cross-backend contract is locked at both ends. The
heterogeneous case is Newton-specific (PhysX views are
view-scoped at the engine level), so no PhysX-side equivalent.
Mass-matrix non-singularity check uses positive-diagonal rather than
``torch.linalg.det()``: a real Franka mass matrix has det~1e-13 by
numerical conditioning even when SPD, so the determinant threshold
would false-fail. Padded zero rows (the bug we want to catch) show up
clearly as zero diagonals.
|
@greptileai review |
* BaseArticulation.get_jacobians / get_mass_matrix / get_gravity_compensation_forces are now concrete with ``raise NotImplementedError`` bodies instead of ``@abstractmethod``. Out-of-tree subclasses no longer break at instantiation when these accessors are added — they fail only when the methods are actually invoked, matching AGENTS.md's deprecation policy. The in-tree PhysX / Newton / ovphysx subclasses still override explicitly. * BaseArticulation.get_jacobians docstring documents the floating-base joint-dim convention difference between PhysX (prepends 6 virtual DoFs to the joint dim, ``num_joints`` counts only actuated) and Newton (``ArticulationView.joint_dof_count`` already includes the base 6, so ``num_joints`` is the total). The total joint-dim is identical on both backends; only how ``num_joints`` is reported differs. Floating-base task-space callers should verify their joint indexing against the active backend's DoF ordering. No floating-base IK/OSC env exists today so no caller is affected.
Reverting the round-1 change that bypassed ``get_gravity_compensation_forces()`` for fixed-base by constructing the zero tensor directly. The earlier rationale was "the result is always zeros, so don't crash on Newton just for shape inference," but that silently no-ops a feature the user explicitly opted into. If a user sets ``enable_gravity_compensation=True`` on a Newton fixed-base robot, they expect compensation; getting zeros without any signal is worse than a crash. Now the call goes through the new wrapper, which raises ``NotImplementedError`` on Newton (no upstream primitive). PhysX still returns zeros via the existing ``torch.zeros_like(...)`` quirk in this fixed-base branch — pre-existing semantic, not changed here. Loud failure on Newton is the desired contract: users see immediately that gravity comp is not supported and must set ``enable_gravity_compensation=False`` until upstream Newton ships ``eval_gravity_compensation``. Coverage of the failure mode is already pinned by ``test_get_gravity_compensation_forces_not_implemented_on_newton`` (strict xfail) at the wrapper level; the Pink action term inherits that contract via the wrapper call.
hujc7
requested review from
Mayankm96,
jtigue-bdai,
kellyguo11 and
ooctipus
as code owners
…-compat-mvp # Conflicts: # source/isaaclab/config/extension.toml # source/isaaclab/docs/CHANGELOG.rst # source/isaaclab/test/controllers/test_differential_ik.py # source/isaaclab_newton/config/extension.toml # source/isaaclab_newton/docs/CHANGELOG.rst # source/isaaclab_ovphysx/docs/CHANGELOG.rst # source/isaaclab_physx/config/extension.toml # source/isaaclab_physx/docs/CHANGELOG.rst # source/isaaclab_tasks/config/extension.toml # source/isaaclab_tasks/docs/CHANGELOG.rst # source/isaaclab_tasks/isaaclab_tasks/direct/automate/assembly_env.py # source/isaaclab_tasks/isaaclab_tasks/direct/automate/disassembly_env.py # source/isaaclab_tasks/isaaclab_tasks/direct/factory/factory_env.py
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
|
Thanks for the Newton IK compatibility scaffold. I tested it locally on Franka reach and wanted to sanity-check two things with you:
Does that sound right to you? |
|
@aravindskumar-nvidia Thanks! I do miss some testing in this PR. Let me take a look once my machine is back. |
Newton's eval_jacobian returns linear-velocity rows referenced at each link's COM (per Newton's public "COM/world body-twist" convention), while IsaacLab's task-space controllers (IK, OSC, RMPFlow) and the body_offset shift in DifferentialInverseKinematicsAction expect the linear rows to reference the link origin (matching body_link_pos_w). On Franka under Newton this gap surfaced as a steady-state EE tracking offset of roughly the panda hand COM offset. Apply the standard v_origin = v_com - omega x (R . body_com_pos_b) shift per Jacobian column on the gathered, view-sized buffer in a new shift_jacobian_com_to_origin Warp kernel. The model-sized _jacobian_buf_flat stays COM-referenced so eval_mass_matrix consumes it unchanged (the joint-space mass matrix is reference-invariant). Tightens the BaseArticulation.get_jacobians docstring to state the link-origin / world-frame contract and the J . q_dot == [body_link_lin_vel_w; body_link_ang_vel_w] identity that controllers silently rely on. Adds: * test_get_jacobians_link_origin_contract on PhysX, asserting the contract numerically. Mirrors the convention test on the Newton side is deferred until the lazy buffer staleness on body_link_*_vel_w is resolved separately; the math is validated by a standalone Newton reproducer. * test_get_gravity_compensation_forces_static_equilibrium on PhysX: apply only the gravity-comp torque to a non-trivial Franka pose and assert the manipulator stays static, pinning the gravity-comp accessor in isolation. Updates the Newton-side xfail rationale for get_gravity_compensation_forces to link the upstream Newton inverse-dynamics tracker (issues isaac-sim#2497, isaac-sim#2529) and the floating-base bug in MuJoCo Warp (isaac-sim#2625), explaining why we deliberately do not roll our own J^T.m.g shim in this PR.
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
All previous concerns have been addressed in this follow-up commit. The PR is ready to ship.
Previous Findings Status
- ✅ Mass matrix gating (Critical): Fixed —
_compute_dynamic_quantitiesnow only fetches when needed, and_osc.compute()receivesNoneinstead of stale zeros when not needed (lines 540-541:mass_matrix=self._mass_matrix if self._needs_mass_matrix else None) - ✅ Caching needs_mass_matrix flag: Fixed — flags are now computed once in
__init__(lines 382-388) and reused - ✅ Test coverage for shapes: Fixed — comprehensive shape tests added for both backends (
test_get_jacobians_shape_fixed_base,test_get_mass_matrix_shape_and_nonsingular_fixed_base, etc.) - ✅ Kernel type annotation: Fixed —
link_offset: wp.int32is now used consistently in kernels - ✅ PhysX naming comment: Fixed — comment added at line 185-187 explaining the plural naming convention
- ✅ View buffer sizing: Fixed — view buffers now use per-articulation counts (
self.num_bodies,self.num_joints) rather than model-wide maximums (lines 3339-3343)
New Additions Review
The new shift_jacobian_com_to_origin kernel correctly implements the COM-to-link-origin Jacobian shift using the standard v_origin = v_com - ω × (R · body_com_pos_b) identity. The test_get_jacobians_link_origin_contract test on PhysX properly pins the J · q_dot == [body_link_lin_vel_w; body_link_ang_vel_w] contract.
Implementation Verdict
Ship it — Clean implementation with thorough test coverage across both backends.
Mirror the PhysX-side test_get_jacobians_link_origin_contract on the Newton side. Both backends inject a non-trivial q_dot, step once, and assert J · q_dot's linear rows equal v_origin = v_com - omega x c_world. Newton-side reads ground-truth (v_com, omega) directly from ArticulationView.get_link_velocities (Newton's canonical (v_com, omega) spatial twist) rather than data.body_com_lin_vel_w, bypassing the IsaacLab lazy velocity-buffer chain that has been observed to read all-zero post-step on Newton in some configurations. The shape returned by get_link_velocities is (num_instances, 1, num_bodies, 6) — squeeze the inner 1-dim. The world-frame COM offset is computed as body_com_pos_w - body_link_pos_w from the data layer to avoid quaternion-convention pitfalls in matrix_from_quat. Also drops the IK accuracy test from the Newton side. Newton's MJWarp solver has an actuator-tracking floor of ~3-4 cm regardless of target distance, plus 2% non-determinism from CUDA kernel ordering — neither limit is in scope for this PR's controller-bridge contract. The Newton contract test directly pins the COM->origin shift correctness without depending on full sim-loop convergence. Updates the 0.5.26 changelog "Added" entry to describe the contract test more precisely (v_origin identity check via state.body_qd) and removes the IK accuracy test reference that no longer exists.
Per the user's request for full accuracy coverage across both backends (3 controllers x 2 backends), add Newton-side tracking sentinels to mirror the existing PhysX-side tests in source/isaaclab/test/controllers/test_differential_ik.py and test_operational_space.py. These tests are NOT correctness oracles — they are regression sentinels with loose thresholds well above the Newton MJWarp solver's actuator tracking floor (~3-5 cm) but below "totally broken" (>20 cm). A wrong- reference-frame Jacobian or DoF-ordering bug would push the steady- state error past these bounds and trip the test. Tight correctness is pinned by test_get_jacobians_link_origin_contract (5 mm tolerance). * test_franka_ik_tracking_accuracy: IK to a 5 cm relative target, scene gravity off. Bound: pos_min < 15 cm. Typical Newton: ~4 cm. * test_franka_osc_tracking_accuracy: OSC pose-abs with inertial_dynamics_decoupling=True (exercises get_jacobians AND get_mass_matrix), gravity_compensation=False, scene gravity off. Bound: pos_min < 20 cm. Typical Newton: ~2 cm. Adds the OperationalSpaceController, DifferentialIKController, FRANKA_PANDA_HIGH_PD_CFG, and math-util imports that the convention- resolution lost during conflict resolution at branch checkout. Both tests print IK_METRIC / OSC_METRIC lines on stdout via -s so stress-test scripts can capture pos_min / pos_mean / rot_min / rot_mean regardless of pass/fail.
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
All previous concerns remain addressed. The new commit (498205f) appears to be a rebase/merge commit with no substantive code changes from the previously reviewed state. The implementation is correct and comprehensive.
Implementation Verdict
Ship it — No new issues found. The PR correctly implements backend-agnostic task-space accessors with proper COM-to-origin Jacobian shifting on Newton, appropriate gating for mass matrix and gravity compensation fetches, and thorough test coverage across both backends.
Stress testing the new Newton IK and OSC accuracy tests for 13+ consecutive runs each shows that with the small-relative-target setup (target = initial_ee_pos + 5 cm in x) the steady-state error is deterministic to 5 decimal places run to run: * IK: pos_min = 0.04136 m (4.136 cm) every run * OSC: pos_min = 0.01778 m (1.778 cm) every run The earlier non-determinism we observed (~2% non-convergence over 100 runs) was specific to long-distance targets (60 cm traversal) that admit multiple local minima. With short trajectories Newton's MJWarp solver is bit-for-bit reproducible. Tighten the thresholds to give a sharper bridge-regression signal: * IK: 15 cm -> 5 cm (slack ~0.86 cm above the deterministic floor) * OSC: 20 cm -> 3 cm (slack ~1.22 cm above the deterministic floor) A wrong-frame Jacobian or missing COM->origin shift would push the steady-state error past these bounds because the bias compounds with Newton's own actuator floor over 800 IK iterations. The previous loose bounds would have silently passed any regression smaller than 10 cm (IK) or 15 cm (OSC) which doesn't catch realistic bridge bugs.
Three user-facing scripts still consumed the engine-level ``robot.root_view.get_jacobians()`` / ``get_generalized_mass_matrices()`` / ``get_gravity_compensation_forces()`` methods directly with raw ``arm_joint_ids`` / ``ee_body_idx``. Under the new shape contract the J / M / g DoF axis is ``num_joints + num_base_dofs``; the old code silently produced wrong results for floating-base robots. - ``scripts/tutorials/05_controllers/run_osc.py``: read through ``robot.data.body_link_jacobian_w`` / ``mass_matrix`` / ``gravity_compensation_forces``; shift actuated-joint ids by ``+ robot.num_base_dofs``. - ``scripts/tutorials/05_controllers/run_diff_ik.py``: same migration for the Jacobian path. - ``scripts/demos/haply_teleoperation.py``: same; plus pre-compute ``ee_jacobi_body_idx`` (drops the fixed-root row when fixed-base) so the body-axis index matches the new wrapper layout. Also updates a stale comment in ``source/isaaclab_newton/test/assets/test_articulation.py`` referencing the removed ``get_jacobians`` / ``get_mass_matrix`` names.
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
Follow-up review on commit b0d179f. This commit adds a _ensure_fk_fresh() helper method to Newton's ArticulationData and refactors the FK freshness check to use it consistently across body_link_pose_w, body_com_jacobian_w, body_link_jacobian_w, and mass_matrix properties. The previous concern about SIM_CFGs mutation was already addressed with deepcopy.
Implementation Verdict
Ship it
Findings
The refactoring correctly consolidates the FK freshness pattern. The new _ensure_fk_fresh() method at line 130-141 is a clean extraction of the existing logic from body_link_pose_w (now at line 676). All Jacobian and mass matrix properties now correctly call this helper before accessing state that depends on body_q. No new issues introduced.
| offset = self._asset.num_base_dofs | ||
| self._jacobi_joint_ids = [j + offset for j in self._joint_ids] |
| offset = self._asset.num_base_dofs | ||
| self._jacobi_joint_ids = [j + offset for j in self._joint_ids] |
The base-class and per-backend property docstrings for the four new task-space accessors had grown to include rationale prose, industry- library citations, and implementation guidance for backend authors — content that belongs in the PR description / changelog, not the API documentation users read at code-completion time. Trim each docstring to: shape + units + indexing convention + math contract. Per-backend overrides defer to the base contract via ``See :attr:`BaseArticulationData.<name>``` and add only what's specific to that backend (engine call used, any backend-only caveat). No behavior change.
…-compat-mvp Resolve conflict in source/isaaclab_physx/.../articulation_data.py: develop's PR isaac-sim#5458 moved body_incoming_joint_wrench_b out of ArticulationData into a sensor module; this branch's data-layer migration had introduced a ProxyArray wrapper for that property. Drop the orphaned _body_incoming_joint_wrench_b_ta declaration in _pin_proxy_arrays since the property and its underlying buffer no longer exist; keep the new _body_link_jacobian_w_ta initializer introduced by this branch. Post-merge tests pass: PhysX 15/15, Newton 16 + 1 expected XFAIL on the articulation suite.
Inline review (PR isaac-sim#5400 review threads on commit 2a814c1): combine the two-step ``if/else`` setting ``_jacobi_body_idx`` followed by the separate ``_jacobi_joint_ids`` arithmetic into one tight block per action term, since both pieces describe the same Jacobian-axis mapping convention. Affected: - DifferentialInverseKinematicsAction - OperationalSpaceControllerAction - RMPFlowAction No behavior change.
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
Follow-up review on commit 11f62c5. This commit addresses the previous concern about missing FK freshness in the body_link_jacobian_w shift kernel — the property now reads self.body_link_pose_w.warp (which triggers _ensure_fk_fresh()) instead of the raw _sim_bind_body_link_pose_w directly. The comment at line 900-903 explicitly documents this design choice.
Implementation Verdict
Ship it
Findings
Previous concern about FK freshness in body_link_jacobian_w has been addressed. The property now correctly uses the accessor path that triggers FK refresh before the shift kernel runs. No new issues identified in this commit.
Two PhysX-side tests pinning the OSC ↔ gravity_compensation_forces pipeline end-to-end on Franka under scene gravity: - ``test_franka_osc_gravity_compensation_holds_under_gravity``: with ``OperationalSpaceControllerCfg.gravity_compensation=True`` and ``osc.compute(gravity=robot.data.gravity_compensation_forces.torch[...])``, the EE holds the initial pose under gravity. Pins the OSC indexing (``+ num_base_dofs`` shift), OSC's ``compute()`` correctly adding ``g(q)`` to its torque output, and the data-property reachability that the existing ``test_get_gravity_compensation_forces_static_equilibrium`` does not exercise. - ``test_franka_osc_no_gravity_compensation_sags_under_gravity``: same setup with ``gravity_compensation=False`` and ``gravity=None``. Sanity check that gravity is genuinely loading the arm (and the with-comp test pass is meaningful, not a no-op-under-no-gravity false positive). Measured magnitudes on Franka home pose (zero actuator PD, scene gravity ON, 100 steps): - with comp: pos_mean ≈ 0.12 mm, rot_mean ≈ 0.00017 rad - without comp: pos_mean ≈ 20.6 mm (~2 cm), rot_mean ≈ 0.036 rad (~2°) A ~170× position-accuracy delta confirms gravity comp is real-loading the arm and being correctly cancelled by the OSC + ``g(q)`` pipeline. Also extends ``_setup_franka_at_home_pose`` with an ``enable_rigid_body_gravity`` flag — ``FRANKA_PANDA_HIGH_PD_CFG`` defaults ``disable_gravity=True`` for the existing IK/OSC tracking-accuracy tests (which use ``gravity_enabled=False``); gravity-related tests need to override both layers.
|
Note for reviewers — current CI red is upstream, not in this PR's code. The failing tests ( Root cause: a transitional regression in Isaac Sim's Repro scope:
The upstream fix is already implemented in the property-query path; awaiting it landing in the rolling tag. No code change to this PR is needed. Will re-trigger CI once the upstream fix lands. |
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
Makes IK/OSC/RMPFlow task-space controllers backend-agnostic by adding abstract properties to BaseArticulationData (body_link_jacobian_w, body_com_jacobian_w, mass_matrix, gravity_compensation_forces) with implementations on both Newton and PhysX backends. Adds BaseArticulation.num_base_dofs for consistent Jacobian column indexing. Refactors all five action terms to use these properties instead of PhysX-specific root_view.get_*() calls. Result: Isaac-Reach-Franka-{IK-Abs,IK-Rel,OSC}-v0 now run on either backend.
Design Assessment
Design is sound. The abstract base → backend implementation pattern is clean and follows established Isaac Lab conventions. The Jacobian body-axis convention (jacobi_body_idx = body_idx - 1 for fixed-base, body_idx for floating-base) and DoF-axis convention (leading num_base_dofs columns for free-base, then actuated joints) are consistent with industry standards (Pinocchio, Drake, MuJoCo) and verified across all call sites. The COM→link-origin shift kernel is correctly applied on both backends.
Findings
No critical or warning-level issues found. The implementation is thorough and correct.
🔵 Suggestion: factory_env.py / assembly_env.py still hardcode fixed-base indexing — source/isaaclab_tasks/isaaclab_tasks/direct/factory/factory_env.py
These files use body_idx - 1 and [:, 0:7] slicing rather than the new num_base_dofs abstraction. Correct for their Franka (fixed-base) scenario, but a follow-up could standardize them for consistency. Not a correctness issue.
🔵 Suggestion: pink_task_space_actions.py calls gravity_compensation_forces for fixed-base even though it zeros the result — This would raise NotImplementedError on Newton if disable_gravity=False for a fixed-base robot. The docstring correctly warns users to set enable_gravity_compensation=False on Newton, but the code path could be simplified to skip the property access entirely for fixed-base.
Test Coverage
- Jacobian shape contracts (fixed + floating): ✅ Both backends
- Link-origin Jacobian contract (
J·q̇ = body_vel): ✅ Both backends - Mass matrix symmetry + positive-definiteness: ✅ Both backends
- FK refresh after manual joint writes: ✅ Both backends
- Gravity compensation static equilibrium: ✅ PhysX;
xfail(strict=True)on Newton - End-to-end IK tracking (800 steps, <5mm): ✅ Both backends
- End-to-end OSC tracking (800 steps, <5mm): ✅ Both backends
- Heterogeneous scene shapes: ✅ Newton
- Cross-backend numerical comparison:
⚠️ Not directly tested (independent ground-truth validation on each) - Test-to-source ratio: ~1488 lines / ~500 lines new code — excellent
CI Status
Checks pending.
Verdict
Ship it ✅
Excellent engineering — the Jacobian conventions are consistent across all 5 action terms and both backends, the num_base_dofs abstraction correctly handles fixed/floating base, gravity compensation is properly guarded with a strict xfail sentinel, and the test coverage is comprehensive. The Franka reach envs can now run on Newton without workarounds.
📝 Update (44280a9)
New commits reviewed: 1f5b5c9a → 44280a95
Changes in this update:
- Fixed spurious Carbonite null-client error logs in
_fabric_notices.pyand_cubric.py - Updated Newton dependencies:
newton==1.2.0rc3,mujoco-warp==3.8.0.2,warp-lang>=1.13.0 - Migrated Dexsuite object primitives from
CuboidCfg/SphereCfgtoMeshCuboidCfg/MeshSphereCfg(required for Newton multi-asset spawning) - Consolidated physics and event configs into base
dexsuite_env_cfg.py - Extended CI test timeout for
test_articulation.py(1500→3000s)
Previous concerns status:
- 🔵 P1 (pink_task_space_actions.py fixed-base gravity crash): Not addressed in this update — file unchanged. This remains a minor edge-case issue (workaround: set
enable_gravity_compensation=False). - 🔵 P2 (PhysX-specific index offset comment): Not addressed — file unchanged.
New issues: None found.
Verdict: Still ship it ✅
📝 Update (7471978)
New commits reviewed: 44280a95 → 74719782
Changes in this update:
- Fixed
ContactSensormetadata extraction for Newton 1.1 migration (changedsensing_obj_type/counterpart_typefrom per-row enums to scalar strings,counterpart_indicesfrom flat to per-row) - Added new
test_sensor_metadatatest covering body-mode, body-mode-with-filter, and shape-mode configurations - Consolidated changelog fragments into main CHANGELOG.rst files across packages
- Version bumps: isaaclab 5.1.1, isaaclab_newton 0.8.1, isaaclab_ov 0.1.8, isaaclab_physx 0.6.4, isaaclab_tasks 1.5.38
Previous concerns status:
- 🔵 P1 (pink_task_space_actions.py fixed-base gravity crash): Unchanged
- 🔵 P2 (PhysX-specific index offset comment): Unchanged
New issues: None found. The contact sensor fix is clean and well-tested.
Verdict: Still ship it ✅
generate_articulation_cfg("panda") returns the module-level
FRANKA_PANDA_CFG by reference. test_initialization_fixed_base_made_
floating_base then mutated fix_root_link=False directly on that shared
object, leaving the singleton in a floating-base state for every later
test in the same pytest session. Symptoms included wrong mass-matrix
shape (15x15 vs 9x9) on later fixed-base panda tests and the inverse
failure on the IK/OSC accuracy tests.
The companion test (floating-to-fixed) already copies the anymal cfg
before mutating it; this aligns the panda test with that pattern on
both the PhysX and Newton backends.
NewtonArticulationData._create_simulation_bindings rebound
_sim_bind_root_com_vel_w / _sim_bind_body_com_vel_w on every reset,
including when get_root_velocities / get_link_velocities returned None
for fixed-base articulations. The first call left a wp.zeros fallback
allocated by _create_buffers, but every subsequent sim.reset() then
overwrote that fallback with None, so _pin_proxy_arrays crashed:
TypeError: ProxyArray expects a warp.array, got NoneType.
Reproduces with any panda test that calls sim.reset() twice (e.g.
test_write_joint_frictions_to_sim) once the Newton articulation tests
were re-enabled in the regular CI suite by isaac-sim#5557.
Only assign the binding when the solver actually exposes one; otherwise
let the existing wp.zeros fallback persist.
Also wire model.articulation_count / max_joints_per_articulation /
max_dofs_per_articulation / joint_dof_count / body_count into the mock
NewtonManager used by test_articulation_iface; the task-space scratch
buffers added in this PR consume those sizes during data init and would
otherwise pick up MagicMock placeholders, triggering reshape mismatches
in wp.zeros / wp.array.reshape.
AntoineRichard
left a comment
AntoineRichard
left a comment
There was a problem hiding this comment.
If we need it now, this is fine, I didn't dig too deep, didn't have time. But it looks like some features here could be interesting to newton in general.
There was a problem hiding this comment.
Why is the gravity comp mixed into this PR?
There was a problem hiding this comment.
Just to unify the API to provide it in a backend agnostic way if newton side has plan to provide it in the physx way. Can be removed if there's no such plan
| PhysX implementation: passthrough of ``_root_view.get_jacobians()``, which is | ||
| natively COM-referenced. | ||
| """ | ||
| return ProxyArray(self._root_view.get_jacobians()) |
There was a problem hiding this comment.
Did you check if this pointer was stable/fixed? This way we could avoid creating a proxy array everytime?
There was a problem hiding this comment.
Fixed all to follow existing pattern.
|
|
||
| PhysX implementation: passthrough of ``_root_view.get_generalized_mass_matrices()``. | ||
| """ | ||
| return ProxyArray(self._root_view.get_generalized_mass_matrices()) |
There was a problem hiding this comment.
Did you check if this pointer was stable/fixed? This way we could avoid creating a proxy array everytime?
|
|
||
| PhysX implementation: passthrough of ``_root_view.get_gravity_compensation_forces()``. | ||
| """ | ||
| return ProxyArray(self._root_view.get_gravity_compensation_forces()) |
There was a problem hiding this comment.
Did you check if this pointer was stable/fixed? This way we could avoid creating a proxy array everytime?
| def _ensure_fk_fresh(self) -> None: | ||
| """Run forward kinematics if joint state has changed since the last FK update. | ||
|
|
||
| Newton's ``state.body_q`` (per-body world transforms) is updated by ``eval_fk``, | ||
| invoked here through ``SimulationManager.forward()``. After a manual joint or root | ||
| write that bypassed the sim step (``write_*_to_sim_*``), ``_fk_timestamp`` is set | ||
| to ``-1.0`` to force a refresh on the next read of any property that depends on | ||
| body poses (``body_link_pose_w``, the Jacobian properties, ``mass_matrix``). | ||
| """ | ||
| if self._fk_timestamp < self._sim_timestamp: | ||
| SimulationManager.forward() | ||
| self._fk_timestamp = self._sim_timestamp | ||
|
|
There was a problem hiding this comment.
Don't we already have something like that? If not can we harmonize to other articulation props like the bodies links and the writers?
| @property | ||
| def gravity_compensation_forces(self) -> ProxyArray: | ||
| """See :attr:`isaaclab.assets.BaseArticulationData.gravity_compensation_forces`. | ||
|
|
||
| Newton implementation: raises :class:`NotImplementedError` — Newton's | ||
| ``ArticulationView`` exposes only ``eval_fk`` / ``eval_jacobian`` / | ||
| ``eval_mass_matrix``. Use PhysX, or set the controller's | ||
| ``gravity_compensation=False`` until upstream Newton adds the primitive. | ||
| """ | ||
| raise NotImplementedError( | ||
| "Newton has no gravity-compensation primitive. Use PhysX, or set the controller's" | ||
| " ``gravity_compensation=False`` until upstream Newton adds an" | ||
| " ``eval_gravity_compensation`` API." | ||
| ) | ||
|
|
There was a problem hiding this comment.
Did we open a ticket for this? @preist-nvidia for viz.
There was a problem hiding this comment.
@AntoineRichard could I ask you (or your AI agent) to file a ticket for us at https://github.com/newton-physics/newton/issues , specifying precisely how you'd like us to provide Jacobians and mass matrices (and anything else) and in which format?
A paragraph or two are fine, but it should include acceptance criteria and ideally a way to test it, which shouldn't be hard given this MR here. We'll then triage and assign it accordingly.
Thank you!
| # -- dynamics quantities for task-space controllers | ||
| # Newton's eval_jacobian / eval_mass_matrix write into model-sized scratch buffers | ||
| # spanning every articulation in the model; the gather kernels below extract the rows | ||
| # belonging to this view. The output buffers are sized using THIS articulation's body / | ||
| # DoF counts (not the model-wide ``max_*``) so heterogeneous scenes do not leak | ||
| # zero-padded rows / cols into the returned tensor. The DoF axis includes | ||
| # ``num_base_dofs`` floating-base columns up front (0 for fixed-base, 6 for floating- | ||
| # base), matching the cross-library industry convention used by PhysX, Pinocchio, | ||
| # Drake, MuJoCo, RBDL, OCS2, and iDynTree. | ||
| model = SimulationManager.get_model() | ||
| max_links = model.max_joints_per_articulation | ||
| max_dofs = model.max_dofs_per_articulation | ||
| self._jacobian_buf_flat = wp.zeros( | ||
| (model.articulation_count, max_links * 6, max_dofs), dtype=wp.float32, device=self.device | ||
| ) | ||
| # 4-D reshape view (zero-copy); used as kernel input. | ||
| self._jacobian_buf = self._jacobian_buf_flat.reshape((model.articulation_count, max_links, 6, max_dofs)) | ||
| # Motion-subspace scratch (Featherstone ``S``, spatial frame). | ||
| self._jacobian_joint_S_s_buf = wp.zeros(model.joint_dof_count, dtype=wp.spatial_vector, device=self.device) | ||
| # Link-row offset distinguishes fixed-base (skip the 1 fixed-root row) from floating- | ||
| # base (keep the root row), matching Newton's per-articulation eval_jacobian layout. | ||
| self._jacobian_link_offset = 1 if self._root_view.is_fixed_base else 0 | ||
| num_jacobi_bodies = self._num_bodies - self._jacobian_link_offset | ||
| # ``num_base_dofs`` matches the leading free-root DoF columns Newton fills for | ||
| # floating-base articulations. For fixed-base it's 0 (all cols are actuated). | ||
| num_base_dofs = 0 if self._root_view.is_fixed_base else 6 | ||
| # COM-referenced view-sized Jacobian (output of the gather kernel). Pre-allocated | ||
| # for CUDA-graph capture safety; overwritten on every read (no caching). | ||
| self._body_com_jacobian_w_buf = wp.zeros( | ||
| (self._num_instances, num_jacobi_bodies, 6, self._num_joints + num_base_dofs), | ||
| dtype=wp.float32, | ||
| device=self.device, | ||
| ) | ||
| # Link-origin Jacobian (output of the shift kernel applied to body_com_jacobian_w). | ||
| self._body_link_jacobian_w_buf = wp.zeros( | ||
| (self._num_instances, num_jacobi_bodies, 6, self._num_joints + num_base_dofs), | ||
| dtype=wp.float32, | ||
| device=self.device, | ||
| ) | ||
| # Mass-matrix scratch and view-sized output. Same gather pattern as the Jacobian | ||
| # buffers, just one dimension smaller. ``_mass_matrix_body_I_s_buf`` holds per-step | ||
| # body spatial inertias, written by ``eval_mass_matrix``. | ||
| self._mass_matrix_buf = wp.zeros( | ||
| (model.articulation_count, max_dofs, max_dofs), dtype=wp.float32, device=self.device | ||
| ) | ||
| self._mass_matrix_body_I_s_buf = wp.zeros(model.body_count, dtype=wp.spatial_matrix, device=self.device) | ||
| self._mass_matrix_buf_view = wp.zeros( | ||
| (self._num_instances, self._num_joints + num_base_dofs, self._num_joints + num_base_dofs), | ||
| dtype=wp.float32, | ||
| device=self.device, | ||
| ) |
There was a problem hiding this comment.
Can we spin this out as a different method: _create_ jacobian_buffers or something along these lines?
| # -- dynamics quantities for task-space controllers | ||
| # Newton's eval_jacobian / eval_mass_matrix write into model-sized scratch buffers | ||
| # spanning every articulation in the model; the gather kernels below extract the rows | ||
| # belonging to this view. The output buffers are sized using THIS articulation's body / | ||
| # DoF counts (not the model-wide ``max_*``) so heterogeneous scenes do not leak | ||
| # zero-padded rows / cols into the returned tensor. The DoF axis includes | ||
| # ``num_base_dofs`` floating-base columns up front (0 for fixed-base, 6 for floating- | ||
| # base), matching the cross-library industry convention used by PhysX, Pinocchio, | ||
| # Drake, MuJoCo, RBDL, OCS2, and iDynTree. |
There was a problem hiding this comment.
In general it looks like this is something that may not fall on lab considering how important this looks? @preist-nvidia @kellyguo11 for viz.
There was a problem hiding this comment.
@camevor should the selection API provide tools for this similarly to how it provides per articulation root and FK?
PhysX: pre-cache the body_com_jacobian_w, mass_matrix, and gravity_compensation_forces ProxyArrays in _create_buffers. The PhysX tensor-view getters return pointer-stable buffers for the articulation's lifetime (verified across sim.step, manual joint write, and sim.reset), so a single wrap survives every property read instead of allocating a fresh ProxyArray each time. Newton: extract the dynamics-scratch allocation out of _create_buffers into a private _create_jacobian_buffers method and section the buffers by which property each one feeds (shared scratch, per-view gather config, body_com_jacobian_w, body_link_jacobian_w, mass_matrix). Rename the buffers so the names reflect each one's physical role: - _jacobian_joint_S_s_buf -> _joint_S_s_buf (motion subspace is shared between eval_jacobian and eval_mass_matrix; the prefix misled about scope) - _mass_matrix_buf -> _mass_matrix_full_buf (model-wide scratch) - _mass_matrix_buf_view -> _mass_matrix_buf (per-view output, now matches the property name like _body_com_jacobian_w_buf) Cite upstream Newton issues 2497, 2529, and 2625 in the Newton gravity_compensation_forces docstring and NotImplementedError so the upstream gap is visible at the point of failure.
The same 5-line comment about jacobi_body_idx and jacobi_joint_id* appeared verbatim in three places (rmpflow_task_space_actions.py and two sites in task_space_actions.py). BaseArticulation.num_base_dofs already documents column = j + num_base_dofs, so the local re-explanation is pure duplication.
The three PhysX-side passthrough properties (body_com_jacobian_w, mass_matrix, gravity_compensation_forces) were caching the ProxyArray once at init and returning it on every read. The PhysX tensor-view getters they wrap (_root_view.get_jacobians, get_generalized_mass_matrices, get_gravity_compensation_forces) are side-effecting — calling them is what makes PhysX refresh the buffer behind the returned pointer. Skipping the call left the buffer at its initial values and broke every consumer that relies on the property reflecting the current joint or root state after a manual write. Restore correctness by adopting the TimestampedBuffer + timestamp-gated lazy ProxyArray pattern used by body_link_pose_w, joint_pos, and ~20 other properties in the file: - Each property gates the getter call by ``timestamp < _sim_timestamp`` and lazy-inits its ProxyArray once. - Writers that change joint state (write_joint_state_to_sim_index, write_joint_position_to_sim_index) invalidate all three timestamps. - Writers that change root pose (write_root_link_pose_to_sim_index, write_root_com_pose_to_sim_index) invalidate body_com_jacobian_w and gravity_compensation_forces; mass_matrix is configuration-space and root pose does not affect it. Local verification: 9/9 PhysX task-space tests pass, including the manual-write freshness, gravity-comp static-equilibrium, and IK/OSC tracking-accuracy sentinels.
5 participants
1. Summary
Make IK / OSC / RMPFlow task-space controllers backend-agnostic so Franka manipulation envs run under Newton.
The action terms previously called PhysX-only methods on
asset.root_viewdirectly, which crashed under Newton withAttributeError. The Franka reach envs worked around this by hardcodingself.sim.physics = PhysxCfg(...)with the comment "{IK,OSC} control is not supported with Newton physics; use PhysX only". This PR removes that workaround.End-to-end result:
Isaac-Reach-Franka-{IK-Abs,IK-Rel,OSC}-v0run on either backend, picked viapresets=newton.2. Design
Four new properties on
BaseArticulationData:body_link_jacobian_w— geometric Jacobian, linear rows at the link origin (USD prim frame). What IK / OSC consumers want.body_com_jacobian_w— geometric Jacobian, linear rows at the center of mass. Engine-natural form; useful for dynamics reasoning.mass_matrix— joint-space generalized mass matrixM(q).gravity_compensation_forces— joint-space gravity-loading torquesg(q).Plus one metadata property on
BaseArticulation:num_base_dofs— number of free DoFs of the floating base.0for fixed-base,6for floating-base. Maps an actuated-joint idjto its column in J / M / g viaj + num_base_dofs.All four data properties are concrete-with-
NotImplementedErrordefaults; backends override what they support.2.1 DoF axis:
num_joints + num_base_dofs(industry-standard)The DoF axis prepends
num_base_dofscolumns at the front of the joint axis:0for fixed-base,6for floating-base. The 6 columns are the floating-base spatial velocity in world frame, ordered[lin_x, lin_y, lin_z, ang_x, ang_y, ang_z].This matches every major rigid-body library:
nv; reduced-coordinatenv = 6 + n_actuated.pinocchio::computeJointJacobianswrites(6, nv).MultibodyPlantephemeral floating joint;MultibodyPlant::CalcJacobianSpatialVelocityreturns(6, nv)with leading 6 free-floating cols.<freejoint/>introduces 6 DoFs at the front ofqvel;mj_jac*returns(_, nv).JointTypeFloatingBaseadds 6 todof_count;CalcPointJacobianreturns(_, dof_count).generalizedCoordinatesNum = 6 + actuatedJointsNumfor floating-base robots.getFreeFloatingMassMatrixreturns(6 + dofs, 6 + dofs).Consumers operating in actuated-joint space (action terms keyed by
joint_ids) compute[j + asset.num_base_dofs for j in joint_ids]once at init — same application-level reduction as every surveyed library.2.2 Output shapes by backend
Concrete shapes for each engine output and the wrapper transform applied.
N= num_instances,B= num_bodies (full count incl. root),J= num_joints (actuated only). "Default" is the engine-native per-view shape (post-gather). "passthrough" = wrapper applies no shape change. The COM→origin shift is values-only (does not change shape).body_link_jacobian_w(N, B, 6, J)⟶ drop fixed-root row + COM→origin shift(N, B−1, 6, J)⟶ COM→origin shift(N, B−1, 6, J)body_link_jacobian_w(N, B, 6, J+6)⟶ COM→origin shift(N, B, 6, J+6)⟶ COM→origin shift(N, B, 6, J+6)body_com_jacobian_w(N, B, 6, J)⟶ drop fixed-root row(N, B−1, 6, J)⟶ passthrough(N, B−1, 6, J)body_com_jacobian_w(N, B, 6, J+6)⟶ passthrough(N, B, 6, J+6)⟶ passthrough(N, B, 6, J+6)mass_matrix(N, J, J)⟶ passthrough(N, J, J)⟶ passthrough(N, J, J)mass_matrix(N, J+6, J+6)⟶ passthrough(N, J+6, J+6)⟶ passthrough(N, J+6, J+6)gravity_compensation_forcesNotImplementedError(N, J)⟶ passthrough(N, J)gravity_compensation_forcesNotImplementedError(N, J+6)⟶ passthrough(N, J+6)The aligned column collapses across base type using two derived symbols:
num_base_dofs=0fixed |6floating — exposed asBaseArticulation.num_base_dofsnum_jacobi_bodies=B − 1fixed |Bfloating — fixed-root row excluded for fixed-basebody_link_jacobian_w,body_com_jacobian_w(N, num_jacobi_bodies, 6, J + num_base_dofs)mass_matrix(N, J + num_base_dofs, J + num_base_dofs)gravity_compensation_forces(N, J + num_base_dofs)Two observations:
eval_jacobianincludes a zero row for the fixed-root joint that the wrapper drops. PhysX's engine already drops it.J + num_base_dofsform on both engines natively. The only DoF-axis asymmetry isgravity_compensation_forces(Newton NIE pending upstream primitive).2.3 Why on
BaseArticulationData, notBaseArticulationBaseArticulationDataalready exposes per-body / per-joint state as cached lazy@propertyaccessors withTimestampedBufferinvalidation (body_link_pose_w,joint_pos, etc.). The four new accessors fit the same shape — read-only state indexed by body / joint, refreshed per sim step — so they reuse that infrastructure and let consumer code stay symmetric:3. Fixed: latent PhysX IK / OSC frame mismatch
PhysX's
_root_view.get_jacobians()returns linear rows referenced at each body's center of mass, not the link origin. Undocumented behavior — verified empirically by bypassing the IsaacLab wrapper and confirmingJ · q̇matchesbody_com_lin_vel_wto 1e-8 and differs frombody_link_lin_vel_wby exactlyω × r_com_world(the rigid-body shift). The PhysX data layer already encoded this convention for velocities:body_com_vel_wis the raw passthrough of_root_view.get_link_velocities();body_link_vel_wis derived via a shift kernel.Before this PR, IK / OSC / RMPFlow action terms on PhysX consumed
_root_view.get_jacobians()directly while usingdata.body_link_pose_was the EE pose setpoint. The frame mismatch contributesω × r_com_worldper body to the linear-row contract — undetected in CI because no existing test comparedJ · q̇againstbody_link_lin_vel_wdirectly. The new contract testtest_get_jacobians_link_origin_contractparametrized toanymalcatches it explicitly: 0.32 m/s residual on PhysX without the fix.After this PR, PhysX's
body_link_jacobian_wapplies the same COM→origin shift kernel that Newton uses, mirroring the existingbody_link_vel_wderivation. Both backends now satisfybody_link_jacobian_w · q̇ == body_link_vel_wto numerical precision (test tolerance 5e-3 absolute, 1e-2 relative).The COM-referenced sibling
body_com_jacobian_wis exposed for callers that intentionally want the engine-native form (e.g. dynamics-side reasoning at the COM).Also: the three PhysX passthrough properties (
body_com_jacobian_w,mass_matrix,gravity_compensation_forces) now pin a singleProxyArrayin_create_buffersinstead of allocating one per property read. PhysX tensor-view getters return pointer-stable buffers for the articulation's lifetime — verified manually acrosssim.step, a manual joint write, andsim.reset.4. Newton-side details
4.1 Pre-allocation for capture safety
Dynamics-scratch allocation is grouped in a private
ArticulationData._create_jacobian_buffers(model)helper called from_create_buffers. The helper is sectioned by which property each buffer feeds, and names reflect each buffer's physical role:Jinput to skip a re-compute):_jacobian_buf_flat,_joint_S_s_buf(Featherstone motion subspace — shared between the two evals, hence the property-prefix-free name)._jacobian_link_offset(fixed-base row-0 skip),_jacobian_view_art_ids(flattened view-to-model index map).body_com_jacobian_w:_jacobian_buf(zero-copy 4-D view of_jacobian_buf_flat, kernel input) and_body_com_jacobian_w_buf(gather output, per-view).body_link_jacobian_w:_body_link_jacobian_w_buf(Center-Of-Mass-to-link-origin shift kernel output, per-view).mass_matrix:_mass_matrix_full_buf(model-wideHscratch — Composite Rigid Body Algorithm (CRBA) output),_mass_matrix_body_I_s_buf(Featherstone per-body spatial inertia aux, CRBA-only),_mass_matrix_buf(gather output, per-view).Properties are allocation-free at step time. The kernel-launch sequence runs against fixed buffer pointers, which is what makes the per-step path safe under CUDA-graph capture.
4.2 View-level row gather
Newton's
eval_jacobian/eval_mass_matrixwrite every articulation in the model into a single buffer (shape includesmodel.articulation_count), regardless of whichArticulationViewinvoked them. PhysX returns view-scoped data already. Two Warp kernels (gather_jacobian_rows4-D,gather_mass_matrix_rows3-D) gather just this view's rows into a contiguous view-sized destination so the caller-facing shape contract matches PhysX. The view-to-model index map is reused across both gathers.4.3
eval_mass_matrix"J as input" gotchaNewton's
eval_mass_matrix(state, H, J=None, body_I_s=None, joint_S_s=None)treatsJas input when provided — it skips the internaleval_jacobianand uses the buffer as-is. Passing an empty pre-allocatedJproducesH = J^T·M·J = 0→ singular →LinAlgErrorin OSC'storch.inverse(mass_matrix). The wrapper explicitly populatesJby callingeval_jacobianfirst; we reuse_jacobian_buf_flat(same shape) so no separate scratch is needed.4.4 COM→origin shift on
body_link_jacobian_wNewton's
eval_jacobianwrites linear-velocity rows at each link's center of mass. Aftergather_jacobian_rows, theshift_jacobian_com_to_originWarp kernel appliesv_origin = v_com - ω × (R · body_com_pos_b)per(env, body, dof)thread, writing to_body_link_jacobian_w_buf. The COM-referenced source buffer is reused as-is forbody_com_jacobian_wandmass_matrix.4.5 FK staleness
eval_jacobianandeval_mass_matrixreadstate.body_q(per-body world transforms). After a manualwrite_joint_position_to_sim_*(no sim step),state.joint_qis updated butstate.body_qis stale untileval_fkruns. The new properties match the existingbody_link_pose_wconvention — refresh FK lazily via_ensure_fk_fresh()(Python-guardedSimulationManager.forward()) before invoking the eval kernels. PhysX has its own internal refresh on the equivalent getters (verified empirically by the new manual-write tests passing on PhysX without an explicit trigger).5. Action-term gating
OperationalSpaceControllerAction._compute_dynamic_quantitiespreviously fetched mass matrix and gravity-compensation forces unconditionally on every step. Under the new abstraction this would still call Newton's gravity-comp stub even when the user disabled gravity compensation in the controller config.The fetches are now gated to match what the controller actually consumes:
inertial_dynamics_decoupling=Trueornullspace_control != "none"(the null-space torque term inOperationalSpaceController.compute()consumes mass matrix independently of inertial decoupling).gravity_compensation=True.Side benefit: skips a per-step engine call on PhysX when neither flag is set.
6. Known limitation: gravity compensation on Newton
Newton's
ArticulationViewhas no gravity-compensation primitive (onlyeval_fk/eval_jacobian/eval_mass_matrix).gravity_compensation_forcesraisesNotImplementedErroron Newton; OSC users on Newton must setgravity_compensation=Falseuntil upstream lands the primitive (newton-physics/newton#2497, #2529, #2625). The strict-xfail testtest_get_gravity_compensation_forces_not_implemented_on_newtonflips to XPASS when that happens, signaling the maintainer to remove the wrapper stub and OSC guidance.7. Reach-env cfg cleanup
Removed the
self.sim.physics = PhysxCfg(bounce_threshold_velocity=0.2)override fromIsaac-Reach-Franka-{IK-Abs,IK-Rel,OSC}-v0. Tasks now inherit the parentReachPhysicsCfgpreset, sopresets=newtonselectsNewtonCfgandpresets=physx(the default) keeps the previous behavior — samebounce_threshold_velocity=0.2lives onReachPhysicsCfg.default = PhysxCfg(bounce_threshold_velocity=0.2). No information lost.8. Test plan
8.1 Existing controller tests migrated
test_differential_ik.pymigrated torobot.data.body_link_jacobian_w.torch. Passes on PhysX (2/2).test_operational_space.pymigrated torobot.data.body_link_jacobian_w.torch,robot.data.mass_matrix.torch,robot.data.gravity_compensation_forces.torch. PhysX (12/18 — the 6 failures are pre-existingContactSensorenv issues unrelated to this PR; affected tests fail onomni.physics.tensors.apiimport which is independent of the bridge).test_floating_base_osc_action_term_indexingcfg unchanged.8.2 New tests in this PR
Shape contracts — lock the public DoF / body axes:
test_get_jacobians_shape_fixed_basebody_link_jacobian_wdrops the fixed-root row →(N, B−1, 6, J).test_get_jacobians_shape_floating_base(N, B, 6, J+6).test_get_mass_matrix_shape_and_nonsingular_fixed_base(N, J, J)+ strictly positive diagonal (catches the model-wide-padding bug that masks heterogeneous-scene mismatch as a singular matrix).test_get_mass_matrix_shape_floating_base(N, J+6, J+6)— floating-base includes the 6 free-root DoFs.test_heterogeneous_scene_per_view_shapesmodel.max_*. Direct regression test for the heterogeneous-padding bug.Math / physics contracts — values, not just shapes:
test_get_jacobians_link_origin_contract[panda | anymal]J · q̇ == body_link_lin_vel_widentity (sharp J reference-point check). Anymal exercise catches the latent COM/link mismatch this PR fixes.test_get_jacobians_link_origin_contract[panda | anymal]state.body_qdminus the COM offset shift.test_get_mass_matrix_symmetry_pd[panda | anymal]M(q)is square, symmetric, positive-definite.Freshness contracts — Forward Kinematics (FK) refresh after manual writes:
test_jacobian_refreshes_after_manual_joint_write[panda | anymal]write_joint_position_to_sim_index(no sim step), reading the Jacobian reflects the new joint state. Locks in the FK-staleness contract on the manual-write code path.test_mass_matrix_refreshes_after_manual_joint_write[panda | anymal]Gravity compensation — accessor + Operational Space Control (OSC) integration + negative control:
test_get_gravity_compensation_forces_static_equilibriumτ_gcto a non-trivial Franka pose; assert it stays static. Pins the accessor in isolation, no controller masking.test_franka_osc_gravity_compensation_holds_under_gravitygravity=g(q)under scene gravity holds the EE pose. Pins (a)_jacobi_joint_idx + num_base_dofsindexing, (b)OSC.compute(gravity=...)torque math, (c) reachability through the action-term pipeline.test_franka_osc_no_gravity_compensation_sags_under_gravityg(q)=0.test_get_gravity_compensation_forces_not_implemented_on_newtonEnd-to-end Inverse Kinematics (IK) and OSC accuracy — production sentinels:
test_franka_ik_tracking_accuracytest_franka_osc_tracking_accuracyLatest CI:
isaaclab_physxandisaaclab_newtonboth green on the most recent push.8.3 Determinism and stability
The accuracy sentinels are bit-for-bit deterministic on both backends at the 5 cm short-target setup: 20× consecutive runs each give the same
pos_meanto 5 decimal places. Both tests assert on tail mean rather than tail min — the latter is the bottom of any oscillation envelope and can pass spuriously while the actual tracking error is much larger. Tight regression sentinels rather than flaky bounds. Nopytest-rerunfailuresretry decoration — a CI failure should be a real regression, not noise to retry away.8.4 Smoke runs
random_agentonIsaac-Reach-Franka-IK-Abs-v0under Newton: 5,205 physics substeps zero-error.random_agentonIsaac-Reach-Franka-IK-Rel-v0under Newton: 306 substeps zero-error.random_agentonIsaac-Reach-Franka-OSC-v0under Newton: 290 substeps zero-error.8.5 Test-setup notes
The accuracy tests need two fixes that the standalone path doesn't get for free (production envs do):
init_state.joint_pospost-sim.reset(). Without it, the robot sits at the URDF-neutral pose where Franka's wrist axes nearly align — rank-deficient Jacobian, multi-cm DLS plateau.sim_cfg.gravity = (0, 0, 0)in the Newtonsimfixture (build_simulation_context(gravity_enabled=False)is silently ignored when an explicitsim_cfgis passed).The OSC test additionally zeros actuator PD gains so OSC's joint-effort output isn't opposed by
kp·(target − q)(same way OSC is wired in production action terms). With these in place, Newton hits machine precision and PhysX hits ~10 µm. Newton's PD does have a realg_torque/kpgravity-sag that PhysX's TGS masks via constraint projection — surfaces only when gravity is on without gravity compensation, which is the upstream gap in § 6.9. Files
Touched five extensions; per-package changelog fragments under
source/<pkg>/changelog.d/jichuanh-ik-newton-compat-mvp*.rst:isaaclab(minor): four new properties onBaseArticulationData;num_base_dofsonBaseArticulation; controller-action gating + migration to data-layer accessors; doc updates.isaaclab_physx: data-layer impls +shift_jacobian_com_to_originkernel; docs the latent frame-mismatch fix.isaaclab_newton(minor): data-layer impls (eval + gather + shift); model-sized scratch and view-sized output buffers migrated from articulation to data layer; gravity-comp NIE stub.isaaclab_ovphysx: bridge methods removed (inherits NIE).isaaclab_tasks: hardcodedPhysxCfgremovals + direct-workflow caller migrations to data-layer accessors.