Fix Newton joint positions in DOF space, and enable the Digit velocity tasks - #7520
Fix Newton joint positions in DOF space, and enable the Digit velocity tasks#7520Double7sBurger wants to merge 7 commits into
Conversation
github-actions
Bot
added
bug
Double7sBurger
changed the title
Greptile SummaryThe PR converts Newton ball-joint positions between coordinate and DOF representations and enables Digit velocity tasks on MJWarp.
Confidence Score: 4/5The 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 Files Needing Attention: source/isaaclab_tasks/isaaclab_tasks/contrib/velocity/config/digit/_strip_visual_colliders.py Important Files Changed
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[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| 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): |
There was a problem hiding this comment.
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
| return original(stage, *args, **kwargs) | ||
|
|
||
| _rep._build_newton_builder_from_mapping = patched | ||
| _INSTALLED = True |
There was a problem hiding this comment.
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:
![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
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:
JointCoordinateMapis 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_posshape and write-method signatures remain intact. However, the documented graph-capable mask write paths now compact masks through Torchnonzero(), 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.
| @@ -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() | |||
There was a problem hiding this comment.
🔴 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.
| 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)) |
There was a problem hiding this comment.
🟡 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.
| 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): |
There was a problem hiding this comment.
🟡 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.
| removed.append(path) | ||
| return original(stage, *args, **kwargs) | ||
|
|
||
| _rep._build_newton_builder_from_mapping = patched |
There was a problem hiding this comment.
🟡 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.
| @@ -0,0 +1,41 @@ | |||
| """Remove CollisionAPI from Digit's RealSense *visual* decoration meshes before the model is built. | |||
There was a problem hiding this comment.
🔵 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.
1431842 to
561bbbc
Compare
…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.
Double7sBurger
changed the title
| for child in prim.GetStage().Traverse(): | ||
| path = child.GetPath().pathString | ||
| if not path.startswith(prim.GetPath().pathString): | ||
| continue |
There was a problem hiding this comment.
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.
| 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
left a comment
AntoineRichard
left a comment
There was a problem hiding this comment.
🤖 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, andAGENTS.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 in54270fa5ed8are 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 atnum_envs∈{1,2}(catches #1); gather∘scatter round trip vsscipy Rotation.from_rotvec; negated-quaternion hemisphere; index/mask parity. test_articulation.py: a small.usdawith aPhysicsSphericalJoint, assertingjoint_pos.shape[1] == num_joints(fails on base) and write-then-read withoutupdate().test_hydra.py: resolveDigitRoughEnvCfg/DigitRoughPPORunnerCfgundernewton_mjwarp— first test of a list-valuedpreset().
No smoke suite runs Digit on Newton:test_environments_newton.pyfilterstier="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_tasksfragment: "the asset authors noarticulation_props" (grammar; the USD itself authorsenabledSelfCollisions=False); "PhysX default is unchanged" (false); omits thelegs_armsregroup andentropy_coefchange.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:33asserts "MJWarp integrates actuator damping explicitly". MJWarp'simplicitfastpath does include the velocity-actuatorbiasprmterm inqDeriv(derivative.py), so the mechanism is UNVERIFIED as stated. Thec·h/Inumbers are internally consistent (they implyc·h = 0.1432), but neithercnorhis written down, so the bound rots silently whensim.dt/num_substepschange. State the observation and constants rather than the mechanism.
Suggestions
_strip_visual_colliders.py:61:startswithwithout a trailing/also matchesRobot_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_indicesdoes not belong injoint_coordinates.JointCoordinateMap.inert()viacls.__new__yields an object on which two of three methods raiseAttributeError;requiredis stored rather than derived.wp.clamp(w, -1, 1)beforeatan2is a no-op; the 1e-8 / 1e-9 thresholds differ for no reason.- Docstring
joint_coordinates.py:163attributes the free-root exclusion toget_dof_positions; it is Lab'sexclude_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
- 1and assertdof == 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 thejoint_target_qshape guard (#4, #6, #7).
The restructured PR should come out smaller than the current diff.
AntoineRichard
left a comment
AntoineRichard
left a comment
There was a problem hiding this comment.
🤖 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.pyshrinks to ~2 kernels + a ~30-line class, the timestamp machinery and_strip_visual_colliders.pydisappear, and thenum_envs=1and joint-ordering bugs fall out structurally.
| 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): |
There was a problem hiding this comment.
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_dThis 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.
| # 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) |
There was a problem hiding this comment.
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.
| """ | ||
|
|
||
| @classmethod | ||
| def inert(cls) -> JointCoordinateMap: |
There was a problem hiding this comment.
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).
|
|
||
|
|
||
| @wp.kernel(enable_backward=False) | ||
| def gather_ball_dofs( |
There was a problem hiding this comment.
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.)
|
|
||
|
|
||
| @wp.func | ||
| def _scatter_ball( |
There was a problem hiding this comment.
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| self.joint_acc | ||
| self.body_com_acc_w | ||
|
|
||
| def _refresh_joint_positions(self, force: bool = False) -> None: |
There was a problem hiding this comment.
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.
| # 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() |
There was a problem hiding this comment.
Goes away if the gather is a post-step callback (see L134): by the time update() runs, the buffer is already current.
| Shape is (num_instances, num_joints), dtype = wp.float32. In torch this resolves to | ||
| (num_instances, num_joints). | ||
| """ | ||
| self._refresh_joint_positions() |
There was a problem hiding this comment.
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.
| 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) |
There was a problem hiding this comment.
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.
| 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 |
There was a problem hiding this comment.
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).
…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.
github-actions
Bot
added
the
documentation
|
Thanks — this was a genuinely useful review. Everything below is on Critical1. Off-by-one at 2. Collider strip was not preset-gated. You were right on the mechanism and on the prose being false. Replaced with 3. Important4. Tests. Added 5. BALL check. 6. 7.
8. Prose. The SuggestionsAll taken: Net effect on the backend files is −25 lines plus the tests, so the diff did come out smaller. ValidationFive seeds per arm, 4096 environments, on
Not addressedSign and axis-order parity with PhysX on Digit is still UNVERIFIED — I have not run the numeric side-by-side you asked for, and |
3 participants
The problem
ArticulationData._sim_bind_joint_posis bound straight toArticulationView.get_dof_positions():The name says DOF, the return is
joint_qwhich is coordinate space. Newton (like MuJoCo) stores aball joint as a 4-component unit quaternion against 3 DOFs, so for an articulation containing one the
array is wider than
num_joints, whilejoint_names,joint_vel,default_joint_posand everyjoint 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 stringjoint_qdoes not appear anywhere inisaaclab_physx. The MDP layer waswritten 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:
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 quaternionw, which barely leaves 1.0, so theknee angle never reaches the policy or the rewards.
sync_torque_telemetrycarries the same offset.Writes
write_joint_position_to_sim_*scatters DOF-indexed values into coordinate slots, soreset_joints_by_scalecorrupts the pose on every reset: the loop-closure residual goes from0.0004 mm to 828 mm and the solver diverges within 10 steps.
The fix
joint_coordinates.pyaddsJointCoordinateMap, which builds the coordinate ↔ DOF index tablesonce per articulation and converts with four small Warp kernels. Ball joints go through
rotation vector ↔ quaternion; the gather forces
w >= 0first, because a quaternion double-coversSO(3) and without that a fixed pose can decode to
+θone step and-(2π − θ)the next, injectingdiscontinuities into the observation. Rotation vector is the representation consistent with
joint_qd, which already holds angular velocity in the joint frame.requiredis False when every joint has one coordinate per DOF — i.e. every articulation without aball joint. Those keep the existing zero-copy view onto
joint_qand cost nothing.Four call sites: the binding in
articulation_data.py, a lazy_refresh_joint_positions()inupdate()and thejoint_posproperty, a_flush_joint_positions()after each of the four jointposition writers in
articulation.py, and a forced refresh in_post_actuator(it runs inside thestep, 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
DigitPhysicsCfggains anewton_mjwarpbranch, sopresets=newton_mjwarpselects it on theshipped 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 thepreset:
CollisionAPIprims, every one a/Visual/decoration mesh on a RealSense camera mount. Theybecome 57% of the robot's shapes and produced 3e7 N contact forces 1.4 m above the ground.
c·h/I < 2—wrist_yawat7.86, eight more at 2.74. They get their own actuator group with
armature = preset(default=None, newton_mjwarp=0.10).articulation_props, so Newton filtered all 253 intra-articulation shape pairs and the legspassed through each other.
entropy_coef = preset(default=0.01, newton_mjwarp=0.005). MJWarp is less forgiving of the actiontail 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.rstas "not currently validated" on Newton, attributedto 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_rateat the final iteration):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).
pre-commitchecks with./isaaclab.sh --formatsource/<pkg>/changelog.d/for every touched package (do not editCHANGELOG.rstor bumpextension.toml— CI handles that)CONTRIBUTORS.mdor my name already exists there