Fix Newton joint positions in DOF space, and enable the Digit velocity tasks by Double7sBurger · Pull Request #7520 · isaac-sim/IsaacLab

Double7sBurger · 2026-09-03T06:11:23Z

The problem

ArticulationData._sim_bind_joint_pos is bound straight to ArticulationView.get_dof_positions():

# isaaclab_newton/assets/articulation/articulation_data.py:1594
self._sim_bind_joint_pos = self._root_view.get_dof_positions(SimulationManager.get_state_0())[:, 0]

# newton/_src/utils/selection.py:1587
def get_dof_positions(self, source):
    """Get the joint coordinate positions (DoF positions) ..."""
    return self._get_attribute_values("joint_q", source)

The name says DOF, the return is joint_q which is coordinate space. Newton (like MuJoCo) stores a
ball joint as a 4-component unit quaternion against 3 DOFs, so for an articulation containing one the
array is wider than num_joints, while joint_names, joint_vel, default_joint_pos and every
joint gain stay in DOF space. On Agility Digit (6 ball joints) that is 56 coordinates against 50
DOFs.

PhysX is unaffected: its articulations are reduced-coordinate, a spherical joint is 3 scalars,
nq == nv, and the string joint_q does not appear anywhere in isaaclab_physx. The MDP layer was
written against that assumption and the Newton backend inherited it without a conversion.

IsaacLab addresses joints by DOF index everywhere in joint_names, find_joints,
SceneEntityCfg.joint_ids, so both directions break, and they break silently.

Reads

Every MDP term indexes both arrays with the same ids:

# isaaclab/envs/mdp/rewards.py:186
asset.data.joint_pos.torch[:, asset_cfg.joint_ids] - asset.data.default_joint_pos.torch[:, asset_cfg.joint_ids]

The two arrays are in different spaces, so past the first ball joint they refer to different joints.
On Digit joint_pos[13], left_leg_knee, reads a quaternion w, which barely leaves 1.0, so the
knee angle never reaches the policy or the rewards. sync_torque_telemetry carries the same offset.

Writes

write_joint_position_to_sim_* scatters DOF-indexed values into coordinate slots, so
reset_joints_by_scale corrupts the pose on every reset: the loop-closure residual goes from
0.0004 mm to 828 mm and the solver diverges within 10 steps.

The fix

joint_coordinates.py adds JointCoordinateMap, which builds the coordinate ↔ DOF index tables
once per articulation and converts with four small Warp kernels. Ball joints go through
rotation vector ↔ quaternion; the gather forces w >= 0 first, because a quaternion double-covers
SO(3) and without that a fixed pose can decode to +θ one step and -(2π − θ) the next, injecting
discontinuities into the observation. Rotation vector is the representation consistent with
joint_qd, which already holds angular velocity in the joint frame.

required is False when every joint has one coordinate per DOF — i.e. every articulation without a
ball joint. Those keep the existing zero-copy view onto joint_q and cost nothing.

Four call sites: the binding in articulation_data.py, a lazy _refresh_joint_positions() in
update() and the joint_pos property, a _flush_joint_positions() after each of the four joint
position writers in articulation.py, and a forced refresh in _post_actuator (it runs inside the
step, before update() bumps the timestamp).

The scatter is scoped to the environments each write touched. Resets are staggered, so an unscoped
scatter would round-trip every environment's ball joints through the log map on nearly every step,
and the float32 rounding lands on the loop-closure constraints.

The Digit tasks

DigitPhysicsCfg gains a newton_mjwarp branch, so presets=newton_mjwarp selects it on the
shipped tasks; the PhysX default is unchanged. Digit was never PhysX-only by design — that missing
branch was the whole implementation of it. Four items specific to digit_v4.usd, all gated on the
preset:

32 CollisionAPI prims, every one a /Visual/ decoration mesh on a RealSense camera mount. They
become 57% of the robot's shapes and produced 3e7 N contact forces 1.4 m above the ground.
Ten joints ship with armature below MJWarp's explicit-damping bound c·h/I < 2 — wrist_yaw at
7.86, eight more at 2.74. They get their own actuator group with
armature = preset(default=None, newton_mjwarp=0.10).
No articulation_props, so Newton filtered all 253 intra-articulation shape pairs and the legs
passed through each other.
entropy_coef = preset(default=0.01, newton_mjwarp=0.005). MJWarp is less forgiving of the action
tail than PhysX, and at 0.01 a seed occasionally diverges late in training while the policy itself
is still healthy.

Result on the task this came from

Digit was listed in docs/source/refs/issues.rst as "not currently validated" on Newton, attributed
to its closed kinematic loops. The loops import and solve fine; this was the actual blocker. With it
fixed (3 seeds each, 4096 envs, Metrics/success_rate at the final iteration):

	before	after
Velocity-Flat-Digit	0.000 / 0.000 / 0.000	1.000 / 1.000 / 1.000
Velocity-Rough-Digit	0.019 / 0.005 / 0.002	0.992 / 0.993 / 1.000

Episode length 985–1000 of 1000, tip-over termination 0.008 (was 1.000 on rough), terrain curriculum
climbing to 3.5 (was pinned at 0).

I have read and understood the contribution guidelines
bug fixed
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 added a changelog fragment under source/<pkg>/changelog.d/ for every touched package (do not edit CHANGELOG.rst or bump extension.toml — CI handles that)
I have added my name to the CONTRIBUTORS.md or my name already exists there

greptile-apps · 2026-09-03T06:15:37Z

Greptile Summary

The PR converts Newton ball-joint positions between coordinate and DOF representations and enables Digit velocity tasks on MJWarp.

Adds quaternion/rotation-vector gather and scatter paths for Newton articulation state.
Refreshes and flushes converted joint positions at simulation lifecycle and writer boundaries.
Adds Digit-specific MJWarp solver, actuator, collision, and training presets.
Replaces the process-wide visual-collider patch with an asset spawner, but its lexical path scoping can still affect same-prefix sibling assets.

Confidence Score: 4/5

The PR is not yet safe to merge because the replacement Digit collider cleanup can remove authored colliders from same-prefix sibling assets.

The custom spawner traverses the entire USD stage and scopes removal with a lexical prefix, so a sibling such as Robot_extra can be mistaken for a Digit descendant and lose its collision behavior.

Files Needing Attention: source/isaaclab_tasks/isaaclab_tasks/contrib/velocity/config/digit/_strip_visual_colliders.py

Important Files Changed

Filename	Overview
source/isaaclab_newton/isaaclab_newton/assets/articulation/joint_coordinates.py	Adds the coordinate-to-DOF mapping and quaternion/rotation-vector conversion used for ball joints.
source/isaaclab_newton/isaaclab_newton/assets/articulation/articulation_data.py	Separates Newton coordinate storage from the public DOF-space position buffer and synchronizes the two representations.
source/isaaclab_newton/isaaclab_newton/assets/articulation/articulation.py	Flushes DOF-space position writes back into Newton coordinates for only the selected environments.
source/isaaclab_tasks/isaaclab_tasks/contrib/velocity/config/digit/_strip_visual_colliders.py	Introduces an asset-local collider cleanup, but stage-wide traversal with lexical prefix filtering can remove colliders from same-prefix siblings.
source/isaaclab_tasks/isaaclab_tasks/contrib/velocity/config/digit/rough_env_cfg.py	Adds the Digit MJWarp backend preset, actuator armature overrides, self-collision configuration, and custom spawner.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart LR
  A[Newton joint_q coordinates] --> B[JointCoordinateMap gather]
  B --> C[IsaacLab DOF-space joint_pos]
  C --> D[Observations and actuator telemetry]
  C --> E[Joint-position writers]
  E --> F[JointCoordinateMap scatter]
  F --> A

_{Reviews (2): Last reviewed commit: "Lower the Digit entropy coefficient on N..." | Re-trigger Greptile}

greptile-apps · 2026-09-03T06:15:40Z

+        q_start = model.joint_q_start.numpy()
+        qd_start = model.joint_qd_start.numpy()
+        coord, dof = 0, 0
+        for j in range(len(joint_type) - 1):


Coordinate map uses wrong topology

When a ball-joint articulation is not represented by the first topology in a heterogeneous Newton model, JointCoordinateMap scans the model-wide joint arrays from index zero rather than using the view's articulation IDs. The resulting map gathers and scatters against another articulation's layout, causing incorrect joint observations and corrupting position writes.

Knowledge Base Used: Newton backend

greptile-apps · 2026-09-03T06:15:40Z

+        return original(stage, *args, **kwargs)
+
+    _rep._build_newton_builder_from_mapping = patched
+    _INSTALLED = True


Collider patch leaks across scenes

After a Digit Newton configuration is loaded, this assignment permanently patches the process-wide Newton cloner. A later scene with an authored collider under a path containing both /Visual/ and camera_mount has that collider silently removed, causing objects to pass through contacts the asset defines.

Knowledge Base Used:

Newton backend

Task library

isaaclab-review-bot

Isaac Lab Review Bot

The Newton coordinate/DOF split preserves the intended joint_pos contract and zero-copy path for ordinary joints, but several concrete issues need correction: jointless articulations now fail during update, coordinate maps are built from the start of the global model rather than the current articulation, mask writes violate their graph-capture contract, and the Digit workaround installs a process-wide private-function monkeypatch. One new source file also lacks the required SPDX header.

Design and architecture: JointCoordinateMap is an appropriate abstraction for separating Newton coordinate space from Isaac Lab DOF space, but its tables must be scoped to the current articulation instead of starting at global joint index zero. The Digit collider workaround should also avoid permanently patching a private cloner function for every subsequently built scene.
API: The public joint_pos shape and write-method signatures remain intact. However, the documented graph-capable mask write paths now compact masks through Torch nonzero(), introducing a data-dependent host operation for ball-jointed articulations; the scatter path should consume the mask directly or gate a fixed-size launch.
Implementation: The quaternion conversion and environment-scoped scatter are well motivated. Before merge, initialize or guard the coordinate-map state for zero-joint articulations, derive mapping offsets from the articulation view, remove the process-wide collider hook in favor of an asset-scoped mechanism, and add the repository SPDX header to _strip_visual_colliders.py.

Significant concerns. Posted 5 actionable findings inline.

Automated review; human maintainers own approval decisions.

isaaclab-review-bot · 2026-09-03T06:17:10Z

@@ -120,6 +122,8 @@ def update(self, dt: float) -> None:
        """
        # update the simulation timestamp
        self._sim_timestamp += dt
+        # joint_q may have moved; re-derive the DOF-space view before anything reads it
+        self._refresh_joint_positions()


🔴 Critical · Implementation — Jointless articulations break on update

_refresh_joint_positions() dereferences self._joint_coord_map (and _joint_pos_timestamp), but both are assigned only inside the if self._num_joints > 0: branch of _create_simulation_bindings. The documented "No joints (e.g., free-floating rigid body)" branch leaves them unset, so update() and any joint_pos read raise AttributeError for such assets. Initialize a no-op map and timestamp in the zero-joint branch, or guard the refresh by joint count.

isaaclab-review-bot · 2026-09-03T06:17:10Z

+        if env_ids is not None:
+            selection = as_warp_indices(env_ids)
+        elif env_mask is not None:
+            selection = as_warp_indices(wp.to_torch(env_mask).nonzero().flatten().to(torch.int32))


🟡 Warning · Api — Mask writes lose graph-capture guarantee

write_joint_state_to_sim_mask and write_joint_position_to_sim_mask document the mask variants as the ones that allow graphed pipelines. The mask branch here converts through wp.to_torch(env_mask).nonzero(), a data-dependent, host-synchronizing allocation, so for ball-jointed articulations that documented capturable path no longer captures. Consume the boolean mask directly in the scatter kernels (or launch over all envs gated by the mask) instead of compacting indices on the host.

isaaclab-review-bot · 2026-09-03T06:17:10Z

+        q_start = model.joint_q_start.numpy()
+        qd_start = model.joint_qd_start.numpy()
+        coord, dof = 0, 0
+        for j in range(len(joint_type) - 1):


🟡 Warning · Design Architecture — Map assumes articulation is model's first

The tables are built by walking the global model.joint_type/joint_q_start from index 0 and breaking at num_dofs, so they always describe whichever articulation Newton registered first, while _sim_bind_joint_coords is this view's own slice. In a scene with another jointed asset ahead of the ball-jointed robot, required and the coord/DOF offsets are wrong. Derive the start joint from the view's articulation indices, as gravity_compensation_forces does with model.articulation_start.

isaaclab-review-bot · 2026-09-03T06:17:10Z

+                removed.append(path)
+        return original(stage, *args, **kwargs)
+
+    _rep._build_newton_builder_from_mapping = patched


🟡 Warning · Design Architecture — Import-time patch of private cloner function

install() runs at import of newton_env_cfg and permanently rebinds the private isaaclab_newton.cloner.replicate._build_newton_builder_from_mapping for the whole process, with no uninstall or asset check. Every scene built afterwards in that process loses CollisionAPI on prims matching /Visual/ and camera_mount, and the hook breaks silently if that private symbol moves. Express the collider removal through the Digit spawn/asset configuration instead. removed is also collected and never used.

isaaclab-review-bot · 2026-09-03T06:17:10Z

@@ -0,0 +1,41 @@
+"""Remove CollisionAPI from Digit's RealSense *visual* decoration meshes before the model is built.


🔵 Suggestion · Implementation — New source file missing SPDX header

This new module starts directly with its docstring, while every other file added in this change carries the repository copyright/SPDX header template, which the repository guidelines require for new source files. Add the same four-line BSD-3-Clause header used in joint_coordinates.py.

`ArticulationData._sim_bind_joint_pos` is bound straight to `ArticulationView.get_dof_positions()`, which despite its name returns Newton's `joint_q` -- coordinate space. A ball joint occupies 4 quaternion components against 3 DOFs, so for any articulation containing one the array is wider than `num_joints` while `joint_names`, `joint_vel`, `default_joint_pos` and every joint gain stay in DOF space. IsaacLab addresses joints by DOF index throughout, so both directions broke. MDP terms index `joint_pos` and `default_joint_pos` with the same ids, and past the first ball joint the two refer to different joints: on Agility Digit `joint_pos[13]`, `left_leg_knee`, returned a quaternion `w`, which barely leaves 1.0. `sync_torque_telemetry` carried the same offset. On the write side `write_joint_position_to_sim_*` scattered DOF-indexed values into coordinate slots, so `reset_joints_by_scale` corrupted the pose on every reset -- measured loop-closure residual 0.0004 mm to 828 mm, diverging within 10 steps. Adds `JointCoordinateMap`, which builds the coordinate <-> DOF index tables once per articulation and converts with four Warp kernels; ball joints go through rotation vector <-> quaternion, forcing `w >= 0` so the double cover cannot flip the decoded sign between steps. `required` is False when every joint has one coordinate per DOF -- every articulation without a ball joint -- and those keep the existing zero-copy view onto `joint_q`. The scatter is scoped to the environments each write touched. Resets are staggered, so an unscoped scatter round-trips every environment's ball joints on nearly every step, and the float32 rounding lands on the loop-closure constraints. Verified on Digit: the DOF view matches an independent recomputation exactly, a write round-trips exactly, and a partial write leaves the other environments bit-identical. On `Isaac-Velocity-Rough-H1`, `required` is False and `joint_pos` and `joint_q` share a pointer.

Digit was never PhysX-only by design: `DigitPhysicsCfg` simply had no `newton_mjwarp` entry, and that missing branch was the whole implementation of it. `docs/source/refs/issues.rst` attributes the status to Digit's closed kinematic loops, but the loops import and solve correctly -- the blocker was the joint coordinate space mismatch fixed in the preceding commit. Adds the branch, so `presets=newton_mjwarp` selects it. The PhysX default is unchanged. Three further items are specific to `digit_v4.usd`: The asset authors `UsdPhysics.CollisionAPI` on 32 prims, every one a `/Visual/` decoration mesh on a RealSense camera mount. They become 57% of the robot's shapes while the arms, hips and rods carry none, and produced 3e7 N contact forces on bodies 1.4 m above the ground. Ten joints ship with armature below MJWarp's explicit damping bound of `c * h / I < 2`: `wrist_yaw` at 0.01822 gives 7.86 and grows 6.9x per substep, eight more at 0.05228 give 2.74. A startup event raises those to 0.10. A scalar `ImplicitActuatorCfg.armature` cannot express this -- Digit has one `.*` actuator group, so it would also pull the joints that are already fine down to the floor. The asset authors no `articulation_props`, so Newton filtered all 253 intra-articulation shape pairs -- exactly C(23,2) for its 23 colliding shapes -- and the legs passed through each other. Which links carry colliders is left as the asset authored it. The armature event and the self-collision setting are gated on the preset, so the PhysX path keeps its current behaviour. Measured on 4096 environments, `Metrics/success_rate` at the final iteration: flat 1.000/1.000/1.000 reached by iteration 389-644, rough 0.992/0.993/1.000 by 573-734. Both were effectively zero before.

…nfig Scope `JointCoordinateMap` to the view's own articulation. It walked the model joint arrays from index zero, so in a scene where another jointed asset is registered first the tables described that asset's layout instead. The walk now starts at `articulation_start[articulation_ids[0]]`. Initialize the map in the jointless branch of `_create_simulation_bindings`. `_refresh_joint_positions` dereferenced it unconditionally, so a free-floating rigid body raised `AttributeError` on the first `update()`. Keep the mask write paths graph-capturable. The mask branch compacted through `wp.to_torch(...).nonzero()`, a data-dependent host allocation, on methods documented as the capturable ones. The scatter now takes the boolean mask and launches over every environment. Express the Digit armature floor as actuator configuration rather than a startup event. Splitting the ten joints below `c * h / I < 2` into their own group lets `armature = preset(default=None, newton_mjwarp=0.10)` carry it; `None` keeps the value the USD prim authors, so PhysX is unaffected. The two groups together cover exactly the joints the single group did. Replace the process-wide cloner monkeypatch with a spawner. Removing `CollisionAPI` from Digit's camera decoration meshes was hooked onto a private cloner function for the lifetime of the process, so any later scene with a prim path containing both `/Visual/` and `camera_mount` silently lost its colliders. The removal now runs as the asset's own spawn function, scoped to the prims it just created. Verified with the Newton preset: joint_pos 50 wide, ten joints at armature 0.10 with worst c*h/I 1.432, 23 collision shapes (was 55), and a partial write leaves the untouched environments bit-identical.

MJWarp is less forgiving of the action tail than PhysX. At the shipped 0.01 the policy settles around std 0.59 on rough terrain against 0.40 at 0.005, a 45% wider action distribution over 5000 iterations of 4096 environments, and one seed in three hit a solver divergence past iteration 2000 while success rate was 1.000 and episode length full -- the policy was healthy, the action tail was not. Gated on the preset, so PhysX keeps 0.01.

greptile-apps · 2026-09-03T13:46:31Z

+    for child in prim.GetStage().Traverse():
+        path = child.GetPath().pathString
+        if not path.startswith(prim.GetPath().pathString):
+            continue


Collider scope includes siblings

If the scene contains a sibling such as /World/envs/env_0/Robot_extra, the stage-wide traversal accepts it because its path starts with the Digit root string. Matching camera-mount colliders on that sibling then lose CollisionAPI, causing its intended contacts to disappear.

Suggested change

for child in prim.GetStage().Traverse():

path = child.GetPath().pathString

if not path.startswith(prim.GetPath().pathString):

continue

root_path = prim.GetPath().pathString

for child in prim.GetStage().Traverse():

path = child.GetPath().pathString

if path != root_path and not path.startswith(f"{root_path}/"):

continue

AntoineRichard

🤖 AI-generated review. Produced by Claude (Opus/Fable 5) on behalf of @antoiner from a local checkout of this branch, Newton 1.5.1, the cached digit_v4.usd, and AGENTS.md. Every claim below was confirmed in code by at least two independent passes unless marked UNVERIFIED. Findings already raised by the bots and addressed in 54270fa5ed8 are not repeated. Treat as input, not a verdict.

Critical

1. Off-by-one drops the model's last joint — reproduced at num_envs=1.
joint_coordinates.py:187 iterates range(first_joint, len(joint_type) - 1), but Model.joint_type is [joint_count] while joint_q_start/joint_qd_start are [joint_count + 1] (sentinel; Newton model.py:1104,1153). With 2+ envs the next env's joints pad the walk and the dof >= num_dofs break rescues it; with a single env the last DOF-bearing joint is never tabulated, _sim_bind_joint_pos[:, -n:] stays 0.0 forever and writes to it never reach joint_q. Reproduced kitless with a ModelBuilder chain free→revolute→ball→revolute: num_envs=2 correct, num_envs=1 gathers 0.0 where -0.7 was set. Digit is masked only because its six excludeFromArticulation loop joints are appended after the tree joints. This is the --num_envs 1 / play.py case, and nothing asserts dof == num_dofs.

2. The collider strip is not preset-gated and changes the PhysX Digit baseline.
rough_env_cfg.py:283 sets spawn.func = spawn_digit unconditionally. In the scene flow the spawner is invoked at the concrete env_0 path (clone_plan.py:301) and ReplicateSession.__exit__ copies env_0 to every env afterwards (replicate_session.py:183-186), so all 32 colliders vanish under physx too. "All gated on the preset" (PR text) and "The PhysX default is unchanged" (fragment) are both false. DigitLocoManipEnvCfg(DigitRoughEnvCfg) (loco_manip_env_cfg.py:214) inherits the spawner and the new newton_mjwarp branch without validation, while issues.rst still lists it PhysX-only. Stripping on PhysX may well be right — but as a disclosed decision, not a side effect.

3. joint_pos is one step stale under non-identity joint ordering.
With ordering active, joint_pos proxies _joint_pos_user (articulation_data.py:2356,2388), refilled by _refresh_user_order_joint_state in a post-step callback (:2185-2195, registered articulation.py:3387). The new gather into _sim_bind_joint_pos runs later in update() and never republishes the reorder, so the user-order shadow holds the previous step's values. Latent for Digit (joint_ordering=None) but it is exactly the sim-to-sim transfer configuration, and it fails silently with plausible numbers.

Important

4. No tests (checklist unchecked; AGENTS.md requires a regression test that fails without the fix). The kernels are pure Warp and CPU-testable with no mocking. Minimal set:

kitless test_joint_coordinates.py: tables at num_envs∈{1,2} (catches #1); gather∘scatter round trip vs scipy Rotation.from_rotvec; negated-quaternion hemisphere; index/mask parity.
test_articulation.py: a small .usda with a PhysicsSphericalJoint, asserting joint_pos.shape[1] == num_joints (fails on base) and write-then-read without update().
test_hydra.py: resolve DigitRoughEnvCfg/DigitRoughPPORunnerCfg under newton_mjwarp — first test of a list-valued preset().
No smoke suite runs Digit on Newton: test_environments_newton.py filters tier="core", contrib tests run PhysX only.

5. Any joint with n_coords != n_dofs is decoded as a quaternion. joint_coordinates.py:192-198 has no JointType.BALL check; DISTANCE is 6 DOF / 7 coords (enums.py:170) and is not in Lab's exclude_joint_types=[FREE, FIXED]. Offsets stay aligned, so it corrupts silently. elif type == BALL … else: raise NotImplementedError.

6. docs/source/refs/issues.rst:67-82 still says the Digit tasks are PhysX-only and that presets=newton_mjwarp "is rejected". The diff touches no docs.

7. joint_target_q layout is unguarded. The binding at articulation_data.py:1664 is DOF-shaped only because newton.use_coord_layout_targets defaults to False. Newton already emits a DeprecationWarning for Digit (builder.py:11992; suppressed by Python's default filter) and will flip the default; then this array becomes 56-wide against 50 DOFs — the same bug this PR fixes, on a second array. One shape assert after binding makes that loud. Related: under the legacy layout a ball joint's 3 targets are extrinsic ZYX Euler (builder.py:2522) while joint_pos is now a rotation vector — inert for Digit (rod joints are passive) but worth stating.

8. Prose accuracy.

isaaclab_tasks fragment: "the asset authors no articulation_props" (grammar; the USD itself authors enabledSelfCollisions=False); "PhysX default is unchanged" (false); omits the legs_arms regroup and entropy_coef change.
rough_env_cfg.py:288 ".* mis-indexes Digit's ball-joint DoFs" is now a stale rationale — the remaining reason is not driving the passive tarsus/toe/rod joints.
rough_env_cfg.py:33 asserts "MJWarp integrates actuator damping explicitly". MJWarp's implicitfast path does include the velocity-actuator biasprm term in qDeriv (derivative.py), so the mechanism is UNVERIFIED as stated. The c·h/I numbers are internally consistent (they imply c·h = 0.1432), but neither c nor h is written down, so the bound rots silently when sim.dt/num_substeps change. State the observation and constants rather than the mechanism.

Suggestions

_strip_visual_colliders.py:61: startswith without a trailing / also matches Robot_extra; the loop proceeds silently when it strips zero prims.
articulation_data.py:167: getattr(self, "_all_env_indices", None) is dead (all four callers pass _resolve_env_ids() output) and duplicates _ALL_INDICES; as_warp_indices does not belong in joint_coordinates.
JointCoordinateMap.inert() via cls.__new__ yields an object on which two of three methods raise AttributeError; required is stored rather than derived.
wp.clamp(w, -1, 1) before atan2 is a no-op; the 1e-8 / 1e-9 thresholds differ for no reason.
Docstring joint_coordinates.py:163 attributes the free-root exclusion to get_dof_positions; it is Lab's exclude_joint_types.

Design: this is not the minimal shape

The PR re-derives, with a model walk and caching machinery, information the surrounding code already has:

PR builds	Already exists	Deleting it removes
Model walk over `joint_type`/`q_start`/`qd_start` from `articulation_start[articulation_ids[0]]`, `dof >= num_dofs` break	`view.joint_dof_counts` / `view.joint_coord_counts` per selected joint, in column order, already excluding FREE/FIXED and loop joints (`selection.py:903-943, 574`)	`first_joint`, both `.numpy()` readbacks, the break and its "loop closures" comment, `inert()`, bug #1
`_joint_pos_timestamp`, `force=`, hooks in `update()` and the property	The post-step callback slot `_refresh_user_order_state` already uses, inside the captured region (`newton_manager.py:2489`)	All lazy machinery; gather-then-reorder in one callback fixes bug #3 structurally. Keep one unconditional gather in `_post_actuator` for telemetry.
6 kernels, `_scatter_ball`, `env_index`/`env_mask` dual API	`wp.quat_to_axis_angle` (identical math incl. the `w<0` branch), `wp.quat_from_axis_angle`, `Articulation._env_ids_to_mask` (`articulation.py:2255`)	4 kernels, `as_warp_indices`, `_all_env_indices`; env scoping preserved via the mask
`_strip_visual_colliders.py` + `spawn.func` override	`UsdFileCfg.collision_props={"/.camera_mount/realsense/Visual/.": [UsdPhysicsCollisionCfg(collision_enabled=False)]}` — applied inside the spawner before replication (`from_files.py:336`), honored natively by PhysX and by Newton's `_is_enabled_collider` (`import_usd.py:657`); wrap in `preset()` if the PhysX baseline must stay	The whole file and bug #2 (verified by code reading, not executed)

Genuinely unavoidable: a separate DOF buffer + one gather per step when a ball joint exists (Newton stores quaternions); a scatter after each of the four writers; a second gather per decimation on the native-actuator path for sync_torque_telemetry; a documented convention choice. That is ~80–100 lines in joint_coordinates.py, ~30 in articulation_data.py, four one-liners in articulation.py, one in actuator_control.py, and config-only Digit changes.

On the convention: rotation vector is the right family. PhysX's eSPHERICAL position is PxExp(twist, swing1, swing2) projected onto the DOF axes (PxArticulationJointReducedCoordinate.h; DyFeatherstoneArticulation.cpp propagateTransform), and MJWarp's quat_to_vel is the same log map. Sign and axis-order parity with PhysX on Digit is UNVERIFIED — a numeric side-by-side belongs in this PR, since cross-backend joint_pos parity is the point. Longer term this belongs in Newton's ArticulationView (get_dof_positions returns joint_q while its own docstring says "DoF positions"); the Lab layer should stay small enough to delete when that lands.

What is right here

The diagnosis is correct and important, and the fix sits in the right layer with a true zero-cost path for every non-ball asset. The w >= 0 canonicalization, env-scoped scatter, graph-safe masked kernels and the forced refresh in _post_actuator are all correctly reasoned; the kernel math checks out to 1e-15 against a float64 reference. Each Digit asset finding (camera-glass colliders, armature floor, self-collision authored off, 253 = C(23,2) filtered pairs) was independently reproduced from the cached USD. Preset resolution on every shipped entry point is sound.

Suggested order

Build tables from the view (or at minimum drop the - 1 and assert dof == num_dofs); add the BALL type check (#1, #5).
Move the gather to the post-step callback ahead of the ordering reorder; delete the timestamp machinery (#3).
Replace the spawner with collision_props; decide explicitly whether PhysX keeps the colliders and say so in the fragment; address LocoManip (#2, #8).
Add the three tests, update issues.rst, add the joint_target_q shape guard (#4, #6, #7).

The restructured PR should come out smaller than the current diff.

AntoineRichard

🤖 AI-generated inline pointers, companion to my earlier review (Claude, on behalf of @antoiner). Each comment marks something this PR re-implements and names the existing API (with file:line) to rely on instead. Net effect if all are taken: joint_coordinates.py shrinks to ~2 kernels + a ~30-line class, the timestamp machinery and _strip_visual_colliders.py disappear, and the num_envs=1 and joint-ordering bugs fall out structurally.

AntoineRichard · 2026-09-04T09:54:39Z

+        q_start = model.joint_q_start.numpy()
+        qd_start = model.joint_qd_start.numpy()
+        coord, dof = 0, 0
+        for j in range(first_joint, len(joint_type) - 1):


Duplicates ArticulationView's per-joint counts. The view already computed, for exactly the joints Lab selected, joint_coord_counts and joint_dof_counts in the same order as the columns of get_dof_positions() / get_dof_velocities() (Newton selection.py:903-943). It already excludes FREE/FIXED (Lab passes exclude_joint_types, articulation.py:3304) and loop-closing joints (include_loop_closing_joints=False, selection.py:574).

Rely on that instead of walking model.joint_type/joint_q_start/joint_qd_start:

coord = dof = 0 for n_c, n_d in zip(view.joint_coord_counts, view.joint_dof_counts): if n_c == n_d: single_dof += range(dof, dof + n_d); single_coord += range(coord, coord + n_c) elif (n_c, n_d) == (4, 3): ball_dof.append(dof); ball_coord.append(coord) else: raise NotImplementedError(f"{n_c} coords / {n_d} DOFs is not a ball joint") coord += n_c; dof += n_d

This removes first_joint, num_dofs, the break on L201 and its "loop closures" comment — and the len(joint_type) - 1 bound, which skips the model's last joint (joint_type is [joint_count], the start arrays are [joint_count + 1]); with num_envs=1 the last DOF-bearing joint is silently never mapped.

AntoineRichard · 2026-09-04T09:54:39Z

+            # coordinate walk has to start at this view's own first joint.
+            first_art = int(self._root_view.articulation_ids.numpy().reshape(-1)[0])
+            first_joint = int(model.articulation_start.numpy()[first_art])
+            self._joint_coord_map = JointCoordinateMap(model, self._num_joints, first_joint, self.device)


Only needed because of the model walk above. With view-derived tables this becomes

self._joint_coord_map = JointCoordinateMap( self._root_view.joint_coord_counts, self._root_view.joint_dof_counts, self.device )

and L1647-1648 (articulation_ids.numpy(), articulation_start.numpy()) go away, along with the heterogeneous-scene assumption they encode.

AntoineRichard · 2026-09-04T09:54:39Z

+    """
+
+    @classmethod
+    def inert(cls) -> JointCoordinateMap:


inert() becomes unnecessary once the constructor takes the view's count lists: a jointless view yields empty lists → no ball entries → required=False, and L1699 can call the same constructor. That also removes the partially-initialised state (an inert() instance raises AttributeError from gather/scatter).

AntoineRichard · 2026-09-04T09:54:39Z

+
+
+@wp.kernel(enable_backward=False)
+def gather_ball_dofs(


Hand-rolled log map; Warp ships it. wp.quat_to_axis_angle(quat) -> (vec3, float) (Warp 1.16 stubs.py:2308; native quat.h:167-172) already does the hemisphere handling this kernel does by hand: it flips the axis sign with w and returns angle = 2·atan2(|v|, |w|) ∈ [0, π], and normalize of a zero vector yields zero so the identity is safe.

q = wp.quat(coords[env, c + 0], coords[env, c + 1], coords[env, c + 2], coords[env, c + 3]) axis, angle = wp.quat_to_axis_angle(q) rv = axis * angle

(wp.clamp(w, -1, 1) before atan2 is a no-op either way.)

AntoineRichard · 2026-09-04T09:54:39Z

+
+
+@wp.func
+def _scatter_ball(


Same for the exp map: wp.quat_from_axis_angle(axis, angle) (stubs.py:2303). Keep the small-angle guard, then

coords[env, c:c+4] = wp.quat_from_axis_angle(axis / angle, angle) # component-wise

AntoineRichard · 2026-09-04T09:54:40Z

        self.joint_acc
        self.body_com_acc_w

+    def _refresh_joint_positions(self, force: bool = False) -> None:


Freshness tracking duplicates a mechanism this class already uses. The joint-ordering code solved "publish a derived joint buffer after every step, graph-capturably" with a post-step callback: _refresh_user_order_state is registered via SimulationManager.register_post_step_callback (articulation.py:3387-3389; hook contract at newton_manager.py:3272-3290) and runs inside the stepped/captured region after the last substep, with no Python freshness guard by design.

Register the coordinate gather in that same slot — before _refresh_user_order_joint_state, so the reorder reads fresh DOFs (as written, under non-identity ordering the reorder runs before update() gathers, so joint_pos is one step stale). Then _joint_pos_timestamp, force=, the hook in update() (L125) and in the property (L1083) are all unnecessary; the only other gather needed is the unconditional one in _post_actuator for sync_torque_telemetry.

AntoineRichard · 2026-09-04T09:54:40Z

        # update the simulation timestamp
        self._sim_timestamp += dt
+        # joint_q may have moved; re-derive the DOF-space view before anything reads it
+        self._refresh_joint_positions()


Goes away if the gather is a post-step callback (see L134): by the time update() runs, the buffer is already current.

AntoineRichard · 2026-09-04T09:54:40Z

        Shape is (num_instances, num_joints), dtype = wp.float32. In torch this resolves to
        (num_instances, num_joints).
        """
+        self._refresh_joint_positions()


Same — no per-read check needed once the buffer is refreshed post-step. Writers land in DOF space directly, so post-write reads are already fresh without this.

AntoineRichard · 2026-09-04T09:54:40Z

        def _post_actuator() -> None:
+            # Runs inside the step, before ArticulationData.update() bumps the timestamp, so the
+            # DOF-space view has to be forced rather than left to the usual lazy refresh.
+            articulation._data._refresh_joint_positions(force=True)


Keep a gather here (telemetry reads _sim_bind_joint_pos inside the decimation loop), but it can be the plain unconditional gather() — force= exists only to bypass the timestamp gate, which the post-step design removes.

AntoineRichard · 2026-09-04T09:54:40Z

        self.scene.robot = DIGIT_V4_CFG.replace(prim_path="{ENV_REGEX_NS}/Robot")
+        # digit_v4.usd applies CollisionAPI to 32 RealSense camera decoration meshes; this spawner
+        # clears it on the prims it just created. See :mod:`._strip_visual_colliders`.
+        self.scene.robot.spawn.func = spawn_digit


A custom spawner duplicates an existing spawner feature. UsdFileCfg.collision_props (spawner_cfg.py:142) takes a regex-suffix → fragment mapping, and UsdPhysicsCollisionCfg(collision_enabled=False) (schemas_cfg.py:255,268) writes physics:collisionEnabled=false. It is applied in _apply_body_schema_properties (from_files.py:336-346) on the env_0 prototype before replication, so every env gets it on both backends; PhysX honours collisionEnabled natively and Newton drops the shape from collision via _is_enabled_collider (import_usd.py:657).

self.scene.robot.spawn.collision_props = preset( default=None, # or apply to PhysX too — but then say so in the fragment newton_mjwarp={"/.*camera_mount/realsense/Visual/.*": [UsdPhysicsCollisionCfg(collision_enabled=False)]}, )

That deletes _strip_visual_colliders.py and the import on L31, and fixes the un-gated PhysX baseline change (verified by code reading, not executed — worth one run to confirm the Newton shape count matches).

@AntoineRichard

…ting existing APIs Follows @AntoineRichard's review. Net effect is 25 fewer lines plus seven tests. `JointCoordinateMap` now takes `ArticulationView.joint_coord_counts` / `joint_dof_counts` instead of walking `Model.joint_type`. Those lists are already in the column order of `get_dof_positions()` and already exclude the free root, fixed joints and loop-closing joints. That removes the `articulation_start` lookup, both `.numpy()` readbacks, `inert()`, and with them an off-by-one: `joint_type` is `[joint_count]` while `joint_q_start` is `[joint_count + 1]`, so `range(first, len(joint_type) - 1)` dropped the last DOF-bearing joint. At two or more environments the next environment's joints padded the walk; at `num_envs=1` -- the `play.py` case -- those DOFs read 0.0 forever and writes to them never reached `joint_q`. Digit was masked only because its loop joints are appended after the tree joints. A joint whose counts differ in any way other than 4-against-3 now raises instead of being decoded as a quaternion; a distance joint is 7 against 6. The gather moves into the post-step callback slot the ordering shadows already use, ahead of `_refresh_user_order_joint_state`. Under non-identity ordering the reorder ran before `update()` gathered, so `joint_pos` published the previous step's values. That also deletes `_joint_pos_timestamp`, the `force=` flag and both refresh hooks. The kernels use `wp.quat_to_axis_angle` / `wp.quat_from_axis_angle`, which already do the hemisphere handling that was written out by hand, and the index scatter variants are gone: `_env_ids_to_mask` converts, so one masked kernel per shape covers both write paths. `_build_env_mask_kernel` accepts any integer width, since `_resolve_env_ids` is documented to return a torch tensor. The Digit collider removal uses `UsdFileCfg.collision_props` rather than a custom spawner, deleting `_strip_visual_colliders.py`. The previous spawner ran on the env_0 prototype before replication, so it stripped the colliders on PhysX too while the changelog claimed otherwise. Keeping that behaviour, now as a stated decision: colliders on a camera's glass are an authoring error on either backend. `issues.rst` is updated, and a shape guard fires if Newton ever returns coordinate-layout joint targets. Verified: the map covers all 50 DOFs and round-trips to 7e-9 at `num_envs` 1 and 2; the seven new tests fail against the previous implementation.

The shape guard added in the previous commit fired on the CI image: from Newton 1.6 `use_coord_layout_targets` defaults to True, so `joint_target_q` is coordinate-shaped exactly like `joint_q`, and the actuators were writing 50 DOF-indexed targets into a 56-wide array. Same defect as the one this branch fixes, on the control array rather than the state array. Actuators now write into a DOF-shaped staging buffer and the post-actuator callback scatters it through the existing `JointCoordinateMap`; a ball joint's three target values are read as a rotation vector, the convention `joint_pos` already uses. Verified on Digit under 1.6.0rc1: a target set on `left_leg_knee` (DOF 13) lands on coordinate 14 and on `right_arm_shoulder_pitch` (DOF 25) on coordinate 28. The workspace pin moves to `newton[sim]==1.6.0rc1` and `warp-lang==1.17.0`, matching develop. Under 1.5.1 the targets were already DOF-shaped and the conversion is inert, so this is not a version-specific branch.

The flush was hooked onto `_post_actuator`, which is registered inside `finalize_native_actuators` and therefore never fires for an articulation using Lab-side implicit actuators -- which is what Digit uses. The DOF-indexed targets reached the staging buffer and stopped there, so Newton saw a constant target array: measured `|coords|` pinned at 1.0 across every step while the staging buffer varied, and flat training sat at 50-step episodes for 700 iterations before the policy's std underflowed. `submit_commands` now wraps its three exit paths and flushes in a `finally`. Measured over six stepped iterations: every single-coordinate DOF target matches its coordinate slot exactly, and a second explicit scatter is a no-op.

Double7sBurger · 2026-09-05T10:08:19Z

Thanks — this was a genuinely useful review. Everything below is on 7644a77bc3.

Critical

1. Off-by-one at num_envs=1. Confirmed, and it is gone structurally: JointCoordinateMap now takes view.joint_coord_counts / joint_dof_counts instead of walking the model, so there is no len(joint_type) - 1 to get wrong and no articulation_start lookup. first_joint, both .numpy() readbacks and inert() are deleted. Verified: all 50 DOFs tabulated and a write round-trips to 7e-9 at num_envs 1 and 2.

2. Collider strip was not preset-gated. You were right on the mechanism and on the prose being false. Replaced with UsdFileCfg.collision_props, which deletes _strip_visual_colliders.py. I kept it applying on both backends — colliders on a camera's glass are an authoring error either way — but it is now a stated decision: the changelog says the PhysX contact behaviour of the Digit tasks changes. LocoManip is called out in issues.rst as inheriting the preset without validation.

3. joint_pos stale under non-identity ordering. The gather moved into the post-step callback slot, ahead of _refresh_user_order_joint_state. _joint_pos_timestamp, force= and both hooks are gone.

Important

4. Tests. Added test_joint_coordinates.py: tables at num_envs ∈ {1, 2}, scatter∘gather round trip against an independent exp map, negated-quaternion hemisphere, unsupported-layout rejection, and mask scoping. Seven tests; they fail 7/7 against the previous implementation.

5. BALL check. (n_coords, n_dofs) != (4, 3) now raises NotImplementedError instead of being decoded as a quaternion.

6. issues.rst updated.

7. joint_target_q — this one had already happened. The shape guard you suggested fired on the CI image within minutes: develop pins newton[sim]==1.6.0rc1, where use_coord_layout_targets defaults to True, so the actuators were writing 50 DOF-indexed targets into a 56-wide coordinate array. Same defect as this PR fixes, on the control array. Targets now go through a DOF-shaped staging buffer and the same map. Two follow-on notes:

The workspace pin moves to newton[sim]==1.6.0rc1 / warp-lang==1.17.0 to match develop. Under 1.5.1 the targets are DOF-shaped and the conversion is inert, so this is not a version-specific branch.
My first attempt hooked the flush onto _post_actuator, which is registered inside finalize_native_actuators and so never fires for Lab-side implicit actuators. Newton then saw a constant target array and flat training sat at 50-step episodes for 700 iterations before the policy's std underflowed. It now flushes from submit_commands in a finally, covering all three exits.

8. Prose. The c·h/I comment now states the observation and the constants (c = 57.3, h = sim.dt / num_substeps = 0.0025) rather than a mechanism I had not verified, and says to re-measure if either changes. The stale ball-joint rationale on the actuator split is corrected, and the changelog no longer claims PhysX is unchanged.

Suggestions

All taken: wp.quat_to_axis_angle / wp.quat_from_axis_angle replace the hand-rolled log and exp maps, _env_ids_to_mask replaces the index scatter variants (so as_warp_indices, _all_env_indices and the dual signature are gone), and the clamp and mismatched epsilons went with them. One addition: _build_env_mask_kernel was typed int32 and had no callers, so it raised on the torch tensors _resolve_env_ids is documented to return — it now takes any integer width.

Net effect on the backend files is −25 lines plus the tests, so the diff did come out smaller.

Validation

Five seeds per arm, 4096 environments, on 7644a77bc3 with Newton 1.6.0rc1:

	complete	final `Metrics/success_rate`
Velocity-Flat-Digit, 3000 iters	5/5	1.000 ×5, crossing 0.95 at iteration 390–562
Velocity-Rough-Digit, 5000 iters	5/5	1.000 / 0.990 / 1.000 / 1.000 / 1.000

Not addressed

Sign and axis-order parity with PhysX on Digit is still UNVERIFIED — I have not run the numeric side-by-side you asked for, and isaaclab_physx needs omni.physics, which this environment does not have. Happy to add it if you want it in this PR rather than as follow-up. I agree the durable fix is in Newton's ArticulationView; this layer is small enough to delete when that lands.

Double7sBurger requested a review from a team September 3, 2026 06:11

Double7sBurger requested review from AntoineRichard, StafaH, hujc7, kellyguo11 and ooctipus as code owners September 3, 2026 06:11

github-actions Bot added bug Something isn't working isaac-lab Related to Isaac Lab team labels Sep 3, 2026

Double7sBurger changed the title ~~Fix/newton ball joint dof space~~ Fix Digit Flat/Rough task training and Ball joint bug Sep 3, 2026

greptile-apps Bot reviewed Sep 3, 2026

View reviewed changes

isaaclab-review-bot Bot reviewed Sep 3, 2026

View reviewed changes

Double7sBurger added 2 commits September 3, 2026 14:23

Double7sBurger force-pushed the fix/newton-ball-joint-dof-space branch from 1431842 to 561bbbc Compare September 3, 2026 06:25

Double7sBurger marked this pull request as draft September 3, 2026 06:42

Double7sBurger added 2 commits September 3, 2026 14:50

Double7sBurger changed the title ~~Fix Digit Flat/Rough task training and Ball joint bug~~ Fix Newton joint positions in DOF space, and enable the Digit velocity tasks Sep 3, 2026

Double7sBurger marked this pull request as ready for review September 3, 2026 13:43

greptile-apps Bot reviewed Sep 3, 2026

View reviewed changes

kellyguo11 added this to Isaac Lab Sep 3, 2026

kellyguo11 moved this to In review in Isaac Lab Sep 3, 2026

kellyguo11 assigned AntoineRichard Sep 3, 2026

AntoineRichard reviewed Sep 4, 2026

View reviewed changes

Double7sBurger added 3 commits September 5, 2026 13:28

github-actions Bot added the documentation Improvements or additions to documentation label Sep 5, 2026

github-actions Bot added the infrastructure label Sep 5, 2026

		@@ -0,0 +1,41 @@
		"""Remove CollisionAPI from Digit's RealSense visual decoration meshes before the model is built.

Uh oh!

Conversation

Double7sBurger commented Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

The problem

Reads

Writes

The fix

The Digit tasks

Result on the task this came from

Uh oh!

greptile-apps Bot commented Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Greptile Summary

Confidence Score: 4/5

Important Files Changed

Flowchart

Uh oh!

greptile-apps Bot Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

isaaclab-review-bot Bot left a comment

Choose a reason for hiding this comment

Isaac Lab Review Bot

Uh oh!

isaaclab-review-bot Bot Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

isaaclab-review-bot Bot Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

isaaclab-review-bot Bot Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

isaaclab-review-bot Bot Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

isaaclab-review-bot Bot Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

greptile-apps Bot Sep 3, 2026

Choose a reason for hiding this comment

Uh oh!

AntoineRichard left a comment

Choose a reason for hiding this comment

Critical

Important

Suggestions

Design: this is not the minimal shape

What is right here

Suggested order

Uh oh!

AntoineRichard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Double7sBurger commented Sep 5, 2026

Critical

Double7sBurger commented Sep 3, 2026 •

edited

Loading

greptile-apps Bot commented Sep 3, 2026 •

edited

Loading