[Newton] Add Rough terrain locomotion Part 1: Foundation + quadrupeds on Newton

[hujc7]

Description

Enables Newton rough-terrain locomotion training on all locomotion velocity envs (Go1, Go2, A1, Anymal-B/C/D, H1, Cassie, Digit, G1) on top of @ooctipus's Anymal-D foundation work, cherry-picked from PR #5225.

Why this PR exists

PR #5225 (Octi's draft) added Newton support for Anymal-D rough terrain. The other 9 locomotion envs were left out of scope. Octi is away and his PR has CI failures, so per maintainer guidance (Kelly Guo's comment) the required changes from #5225 are cherry-picked here so the rough-terrain stack can move forward without depending on his still-open WIP PR.

Dependencies

• PR Add checked_apply helper for forwarding configclass fields onto upstream dataclasses #5365 — adds isaaclab.utils.checked_apply, used by NewtonManager.create_builder to forward NewtonShapeCfg onto Newton's upstream ShapeConfig. Required for stable rough-terrain contact.

1. Cherry-pick from #5225 (614ea2dbb74)

Commit authored by Octi Zhang with Co-authored-by trailer. Subset of #5225 — what's kept and dropped:

#	Item	Status	Reason
1	anymal_d/rough_env_cfg.py Anymal-D Newton config	KEPT (then hoisted into shared parent in commit 2 below)	Defines the Newton physics shape used for rough terrain
2	velocity_env_cfg.py — hoist physics_material, add_base_mass, base_com startup events into shared EventsCfg	KEPT	All envs need them
3	base_com guard preset(default=..., newton=None)	KEPT	Ablation A4 on #5225 (posted 2026-04-17): without the gate, 99.99% of episodes terminate from body_lin_vel runaway on Newton. Upstream Newton fix newton-physics/newton#2332 will let us drop the guard once it ships
4	velocity_env_cfg.py — collider_offsets startup event	DROPPED	(a) Greptile P1 on #5225: PhysX-only root_view methods, would AttributeError on Newton without a guard. (b) Ablation A3: clobbers the 1cm shape margin set by RoughPhysicsCfg (event resets gap = max(0, contact_offset − margin) = 0). Removing it gives +3.71 reward on Anymal-D Newton (+16.38 vs A0 baseline +12.47)
5	terminations.py — body_lin_vel_out_of_limit / body_ang_vel_out_of_limit + __init__.pyi exports	DROPPED	Were a NaN guard for the Newton body_lin_vel runaway when base_com was unguarded. With the preset(newton=None) gate (item 3), the runaway no longer occurs and the guards are unused
6	terrain_generator.py subdivided flat-grid border	DROPPED	Was a workaround for Newton triangle-collision failures on the box-primitive border. Newton has since improved triangle handling, so the workaround is no longer needed

2. New work — 2a532d1f745

2.1 NewtonShapeCfg + checked_apply wiring

The single most important Newton setting for rough terrain is shape margin. Without a nonzero margin, non-Anymal-D robots fail to learn stable contact on triangle-mesh terrain. The previous NewtonManager.create_builder only set gap = 0.01 and left margin at Newton's upstream default of 0.0.

This PR adds NewtonShapeCfg (the Isaac Lab wrapper) exposing margin and gap, and forwards it onto Newton's upstream ShapeConfig via checked_apply from PR #5365:

@configclass
class NewtonShapeCfg:
    margin: float = 0.0
    gap: float = 0.01

# in NewtonCfg
default_shape_cfg: NewtonShapeCfg = NewtonShapeCfg()

# in NewtonManager.create_builder
shape_cfg = cfg.default_shape_cfg if isinstance(cfg, NewtonCfg) else NewtonShapeCfg()
checked_apply(shape_cfg, builder.default_shape_cfg)

RoughPhysicsCfg opts in to default_shape_cfg=NewtonShapeCfg(margin=0.01).

2.2 Hoist RoughPhysicsCfg into shared base

Octi's per-env Anymal-D RoughPhysicsCfg (MJWarp solver + collision pipeline) is hoisted into LocomotionVelocityRoughEnvCfg.sim so every rough-terrain env inherits identical Newton physics. Per-env files become minimal robot-only deltas.

2.3 Per-env Newton-only tweak

• Go1: leg armature preset for joint stability on lightweight quadruped.

2.4 Mass randomization rewrite

Replace EventsCfg.add_base_mass's additive (-5, 5) kg default with multiplicative (1/1.25, 1.25) log-uniform scale (operation="scale", distribution="log_uniform"). Scale-invariant across robot sizes, geometric mean 1.0, no per-robot kg overrides needed.

Per-robot ablation, Newton, 1500-iter (small quadrupeds early-aborted at iter 300):

#	Robot	new log-uniform	old additive baseline	ratio	verdict
1	A1 (iter 300)	10.00	3.25	3.08×	pass — driven by bias removal (old (-1, 3) had +10% mean bias on A1's 10 kg base)
2	Go1 (iter 300)	22.29	16.30	1.37×	pass
3	Anymal-B (iter 300)	12.47	10.92	1.14×	pass
4	Anymal-C (iter 300)	14.64	12.31	1.19×	pass
5	Go2 @ 1499	24.71	18.58	1.33×	pass
6	Anymal-D @ 1499	16.09	15.62	1.03×	pass
7	H1 @ 1499 (biped)	24.02	23.58	1.02×	pass
8	Cassie @ 1499 sym25	14.15	23.93 (mass rand off)	0.59×	regression — fixed in dependent PR #5298 with (1.0, 1.25) heavier-only override

Cassie sensitivity is closed-loop Achilles + hip PD response: lighter-than-nominal pelvis destabilizes; heavier-only (1.0, 1.25) recovers 90% of the reward and pushes ep_len higher (932 vs 910 baseline). Sub-ablation table is in #5298.

Raw logs, checkpoints, and config snapshots preserved at ~/workspaces/data/2026-04-21_mass-rand-scale/ (per-robot <robot>_newton_1500.log, key.md, status.md, run.sh reproducer).

Versions

• isaaclab_newton 0.5.21 → 0.5.22
• isaaclab_tasks 1.5.24 → 1.5.25

Type of change

• New feature (non-breaking).

Checklist

• Pre-commit checks pass
• CHANGELOG + extension.toml bumped on both isaaclab_newton and isaaclab_tasks
• Co-author credit for @ooctipus on the cherry-pick commit
• Ablation evidence cited in commit messages

[greptile-apps]

Greptile Summary

This PR hoists shared Newton rough-terrain physics (RoughPhysicsCfg, startup events, NewtonShapeCfg) into the base LocomotionVelocityRoughEnvCfg, simplifying all 9 per-robot configs from multi-class hierarchies into single __post_init__ overrides. It also introduces checked_apply to forward Isaac Lab wrapper fields onto upstream Newton objects with an API-drift guard.

• P0 blocker: TerminationsCfg.body_lin_vel references mdp.body_lin_vel_out_of_limit (line 327 of velocity_env_cfg.py), a function that does not exist anywhere in the codebase. The PR description (item 5) explicitly states this function was dropped; its presence in TerminationsCfg is contradictory and will cause AttributeError on instantiation of every rough-terrain environment.

Confidence Score: 1/5

Not safe to merge — every rough-terrain env will crash on instantiation due to a missing MDP function.

A single P0 bug (mdp.body_lin_vel_out_of_limit does not exist) breaks all 10 environments at config-resolution time. Everything else in the PR — checked_apply, NewtonShapeCfg, RoughPhysicsCfg hoist, per-env simplifications — is clean and well-structured.

source/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/velocity_env_cfg.py (line 327) and source/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/mdp/terminations.py (missing function definition).

Important Files Changed

Filename	Overview
source/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/velocity_env_cfg.py	Adds shared RoughPhysicsCfg preset, hoists startup events, and adds a body_lin_vel termination term — but the termination function mdp.body_lin_vel_out_of_limit does not exist, causing AttributeError on instantiation.
source/isaaclab/isaaclab/utils/configclass.py	Adds checked_apply helper to forward dataclass fields onto an upstream object with an AttributeError guard; well-tested and correctly implemented.
source/isaaclab_newton/isaaclab_newton/physics/newton_manager_cfg.py	Adds NewtonShapeCfg (margin + gap) and wires it into NewtonCfg.default_shape_cfg; clean addition.
source/isaaclab_newton/isaaclab_newton/physics/newton_manager.py	create_builder now forwards NewtonShapeCfg onto builder.default_shape_cfg via checked_apply; falls back to wrapper defaults in non-Newton contexts.
source/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/go2/rough_env_cfg.py	Simplified by removing per-env physics preset (now in shared base) and per-env EventsCfg subclasses; adds Newton armature preset for Go2.
source/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/digit/rough_env_cfg.py	Removes DigitEventsCfg hierarchy; now mutates shared EventsCfg in post_init. Enables base_com randomization on PhysX path for Digit (previously disabled), which is a behavioral change vs other bipeds.
source/isaaclab_tasks/test/test_hydra.py	Adds regression test for Go1 Newton armature preset; correct and clear.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[LocomotionVelocityRoughEnvCfg] --> B[sim: SimulationCfg\nphysics=RoughPhysicsCfg]
    A --> C[events: EventsCfg\nphysics_material\nadd_base_mass\nbase_com preset\nnewton=None]
    A --> D[terminations: TerminationsCfg\nbody_lin_vel ❌ missing fn]

    B --> E{preset resolve}
    E -- PhysX --> F[PhysxCfg\ngpu_max_rigid_patch_count]
    E -- Newton --> G[NewtonCfg\nMJWarpSolverCfg\nNewtonShapeCfg margin=0.01]

    G --> H[NewtonManager.create_builder]
    H --> I[checked_apply\nNewtonShapeCfg → builder.default_shape_cfg]

    A --> J[Per-robot subclass\n__post_init__ only]
    J --> K[Anymal B/C/D\nGo1/Go2/A1\nH1/G1/Cassie/Digit]

Loading

_{Reviews (3): Last reviewed commit: "Enable rough terrain on all locomotion e..." | Re-trigger Greptile}

[greptile-apps]

RoughPhysicsCfg duplicated across all 9 configs

The identical RoughPhysicsCfg class body is copy-pasted verbatim into every rough terrain config: anymal_b, anymal_c, anymal_d, a1, go1, go2, h1, g1, cassie, and digit. Changing Newton solver settings (e.g. raising njmax/nconmax for G1) will require editing 9+ files. Consider extracting this to a shared module (e.g. velocity_env_cfg.py or a new rough_physics_cfg.py) and importing it in each file.

[greptile-apps]

Hardcoded conda environment name

conda run -n env_isaaclab_develop hardcodes the author's local environment name. Anyone else running this script needs to edit the file. Consider reading the name from an environment variable with a fallback, or using ./isaaclab.sh -p (the project's standard wrapper) as the invocation instead of a bare python call.

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

Summary

This PR enables all rough terrain environments on the Newton backend by (1) adding RoughPhysicsCfg(PresetCfg) with shared Newton solver settings to 10 robot env configs, (2) merging the StartupEventsCfg into the base EventsCfg, (3) fixing base_com body name mismatches for non-base robots, and (4) adding body velocity terminations for NaN/explosion detection. It also includes the XformPrimView → FrameView refactoring from PR #5179 (new BaseFrameView ABC, UsdFrameView, FabricFrameView, NewtonSiteFrameView), ray caster sensor spawner improvements, and a terrain border mesh fix for Newton compatibility.

The approach is sound — unifying events into the base class and using presets for backend-specific behavior is the right direction. However, several Newton-specific event customizations were silently removed during the refactoring, which constitutes a behavioral change that should be documented.

Design Assessment

Design is sound, with one structural concern. The move from per-robot *NewtonEventsCfg / *PhysxEventsCfg / *EventsCfg(PresetCfg) classes to a unified EventsCfg with preset() fields and __post_init__ overrides is a significant simplification. The FrameView abstraction hierarchy (BaseFrameView → UsdFrameView / FabricFrameView / NewtonSiteFrameView with FrameView factory) is well-designed. The main structural concern is RoughPhysicsCfg being copy-pasted identically across 10 files — this should be defined once and imported.

Findings

🟡 Warning: RoughPhysicsCfg is duplicated identically across 10 env config files — source/isaaclab_tasks/.../config/*/rough_env_cfg.py

All 10 robot env configs define an identical RoughPhysicsCfg(PresetCfg) class with the same Newton solver parameters (njmax=200, nconmax=100, pyramidal, implicitfast, max_triangle_pairs=2_500_000). If any Newton parameter needs tuning, all 10 files must be updated in sync. Consider defining this once in velocity_env_cfg.py (or a shared module) and importing it. Robot-specific overrides (e.g., if G1 eventually needs higher njmax for its NaN issue) can still be done via __post_init__.

🟡 Warning: Newton-specific event customizations were silently removed — Multiple env config files

The old per-robot *NewtonEventsCfg classes contained Newton-specific overrides that are no longer present in the new code:

1. push_robot = None was set for all 6 non-Anymal robots (A1, Go1, Go2, G1, H1, Cassie) on Newton. Now push_robot is active on Newton for all robots.
2. reset_robot_joints.params["position_range"] = (1.0, 1.0) constrained joint resets to exact default positions on Newton. This override is now removed.
3. reset_base.params zeroed out velocity randomization on Newton. This override is now removed.

These are intentional simplifications backed by the parity test results (6/7 envs show parity), but they change training dynamics on Newton. Recommend documenting these behavioral changes in the PR description so downstream users know the Newton event configuration has changed.

🟡 Warning: base_com randomization is newly enabled for several robots on PhysX — velocity_env_cfg.py:183

Previously, A1, Go1, Go2, Digit, Cassie, G1, and H1 all set base_com = None in their PhysX event configs (disabling COM randomization). The new code enables it for A1, Go1, Go2, and Digit (with corrected body names) and keeps it disabled for Cassie, G1, H1 (bipeds). This is a training-behavior change for the quadrupeds — the PR description mentions the anymal_c parity gap is related to this, but the change applies to other robots too. Worth noting in the PR description.

🟡 Warning: New collider_offsets startup event added to all rough terrain envs — velocity_env_cfg.py:199

A new randomize_rigid_body_collider_offsets startup event is added to the base EventsCfg with contact_offset_distribution_params=(0.01, 0.01). This event was not present in any of the old configurations. While the identical min/max suggests it applies a fixed offset (not truly random), this is a new event that affects all rough terrain environments. Should be mentioned in the PR description / changelog.

🔵 Suggestion: body_lin_vel_out_of_limit checks all bodies every step — velocity_env_cfg.py:298

The new body_lin_vel termination uses body_names=".*" which checks the linear velocity of every body in the articulation every simulation step. For robots with many bodies (e.g., Digit with 30+ bodies), this computes a norm per body per env. While 20 m/s is a reasonable threshold for catching NaN/explosions, consider whether checking only a subset of bodies (e.g., the base body) would be sufficient and more performant. The NaN check is the most valuable part — an Inf/NaN in any body would propagate to the base anyway.

🔵 Suggestion: Terrain border mesh generation could be pre-validated — source/isaaclab/isaaclab/terrains/terrain_generator.py

The new _add_terrain_border implementation using subdivided grid strips is a good fix for Newton collision detection with large triangles. However, when horizontal_scale is very small relative to border_width, this could generate a very large number of vertices ((border_width/hs + 1)^2 per strip). Consider adding a sanity check or logging a warning when the generated mesh exceeds a threshold vertex count.

Test Coverage

• New code: The FrameView refactoring has good test coverage with test_frame_view_contract.py (359 lines), backend-specific tests for Newton and Fabric, and a ray caster regression test (test_raycaster_offset_does_not_affect_pos_w).
• Env configs: The env config changes are tested via the author's parity test (7 envs × 2 backends × 300 iterations), documented in the PR description. No automated regression tests for env config behavioral changes, but this is consistent with the project's testing approach.
• Gaps: The removed Newton event customizations (push_robot, reset params) and new events (collider_offsets, body_lin_vel) lack unit tests, but these are config-level changes exercised by integration tests.
• Feature PR: Coverage adequate? Yes for the framework changes, Partial for the env config behavioral changes.

CI Status

Only a labeler check is visible and passing. No lint, build, or test CI results are shown — this may be expected for this repo's CI configuration or the checks may be running elsewhere.

Verdict

Minor fixes needed

The core approach — unifying events, adding Newton solver configs, and the FrameView abstraction — is well-executed with demonstrated parity results. The main concerns are:

1. The 10× code duplication of RoughPhysicsCfg (easy to fix by extracting to a shared module)
2. Silent behavioral changes from removed Newton event customizations should be documented
3. New events (collider_offsets, body_lin_vel) should be mentioned in the PR description

None of these are blocking, but addressing them would make the PR cleaner and help downstream users understand what changed. The parity test results provide confidence that the changes work correctly at a training level.

[isaaclab-review-bot]

🟡 Warning: Duplicated RoughPhysicsCfg — This exact class is copy-pasted identically across all 10 robot env configs. Consider defining it once in a shared module (e.g., velocity_env_cfg.py) and importing it. This would make future Newton solver parameter changes a single-file edit.

[isaaclab-review-bot]

🟡 Warning: New event not present in old configs. collider_offsets is a new startup event added to all rough terrain environments. The identical min/max (0.01, 0.01) makes this a fixed offset rather than randomized. Was this intentional? If so, worth noting in the PR description as a behavioral change.

[isaaclab-review-bot]

🔵 Suggestion: body_names=".*" checks every body in the articulation each step. For robots with many bodies this computes num_bodies × num_envs norms per step. Consider whether checking just the base body would be sufficient — NaN in any body will eventually propagate to the root state. The NaN detection is the most valuable part of this termination.

[isaaclab-review-bot]

🔵 Note: Changed from events: EventsCfg = MISSING to events: EventsCfg = EventsCfg(). This means per-robot env configs no longer must provide events — they inherit the base class defaults. Good simplification, but means any robot that previously relied on MISSING to force explicit configuration will now silently get defaults.

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot — Updated Review

Summary

This PR enables all rough terrain locomotion environments on the Newton physics backend through:

1. FrameView Abstraction — Major refactoring from XformPrimView to a backend-dispatching FrameView factory with implementations for USD, Fabric (PhysX), and Newton (GPU-resident site transforms via Warp kernels)
2. NewtonCfg.default_shape_margin — New config field for per-env shape collision margin control
3. Go1/Go2 armature presets — Newton-specific armature values (0.02) for training stability
4. RayCaster improvements — Uses new FrameView for transform tracking, removes complex physics view detection
5. Terrain border mesh fix — Subdivided grid mesh instead of large box primitives for Newton/MuJoCo collision compatibility
6. New termination functions — body_lin_vel_out_of_limit and body_ang_vel_out_of_limit for detecting solver singularities

Architecture Assessment

The FrameView design is excellent. The abstraction hierarchy is well-structured:

BaseFrameView (ABC)
├── UsdFrameView (USD XformCache backend)
├── FabricFrameView (Fabric GPU arrays, PhysX)  
└── NewtonSiteFrameView (Warp kernels, Newton GPU state)

FrameView (factory) ─→ selects backend at runtime

The factory pattern in FrameView.__new__ correctly dispatches to the appropriate backend based on the active physics manager. Contract tests in test_frame_view_contract.py ensure behavioral consistency across backends.

Detailed Findings

🟢 Excellent: Contract test suite

The test_frame_view_contract.py (359 lines) defines invariants that all backends must satisfy:

• World pose = parent + local offset
• Local pose stability after parent moves
• World pose tracking when parent moves
• Indexed get/set operations
• Set/get roundtrip verification

This is the right approach for ensuring backend parity.

🟢 Good: RayCaster offset handling

The new test test_raycaster_offset_does_not_affect_pos_w explicitly validates that the sensor offset doesn't leak into data.pos_w. This catches a subtle bug that existed in the old implementation.

🟡 Warning: Backward compatibility alias may hide issues

# source/isaaclab/isaaclab/sim/views/xform_prim_view.py
XformPrimView = FrameView

While this preserves backward compatibility, it means:

1. Existing code using XformPrimView will silently start using the new factory
2. The API differs (FrameView returns wp.array, old XformPrimView returned torch.Tensor)
3. Consider adding a deprecation warning via __getattr__ or similar

🟡 Warning: Newton site discovery depends on USD path matching

In NewtonSiteFrameView.__init__, sites are discovered via USD regex matching, then body indices are looked up by parsing the body name from the USD path. If the USD hierarchy doesn't match the Newton model body naming, this could fail silently:

# newton_site_frame_view.py line ~280
body_name = site_parent.GetName()
# ... 
if body_name in body_name_to_idx:
    ...
else:
    logger.warning(f"Body '{body_name}' not found in Newton model for site {site_path}")

The warning-only approach is appropriate for flexibility, but the site will be attached to the world frame (body_index=-1) which may not be the intended behavior. Consider whether this should raise an error in strict mode.

🟡 Warning: Terrain border memory consumption

The new subdivided grid border mesh generation creates (width/hs + 1) * (length/hs + 1) vertices per strip. For a typical config with border_width=20 and horizontal_scale=0.05:

vertices per strip ≈ (20/0.05 + 1) * (length/0.05 + 1) = 401 * length_factor

This is significantly more vertices than the old box approach but necessary for Newton collision compatibility. Consider adding a note about memory implications in the docstring.

🔵 Suggestion: _gather_scales kernel is O(N × M)

The _gather_scales kernel iterates through all shapes to find ones matching the body:

for s in range(num_shapes):
    if shape_body[s] == bid and found == 0:
        out_scales[i] = shape_scale[s]
        found = 1

For large models with many shapes, this is O(num_sites × num_shapes). Consider building a body→first_shape index map during initialization for O(1) lookups. This is a minor optimization but could matter for complex robots.

🔵 Suggestion: Document quaternion convention

The code uses (w, x, y, z) quaternion ordering in some places and (x, y, z, w) in others. While the Warp kernels correctly handle the conversion, explicit documentation of the expected convention at API boundaries would help:

def get_world_poses(self, ...) -> tuple[wp.array, wp.array]:
    """...
    Returns:
        orientations: Quaternions in (w, x, y, z) order.  # ← Add this
    """

Test Coverage Assessment

Component	Tests	Coverage
FrameView contract	test_frame_view_contract.py	✅ Excellent
Newton backend	Contract tests + newton-specific	✅ Good
Fabric backend	Contract tests + fabric-specific	✅ Good
RayCaster integration	test_raycaster_offset_does_not_affect_pos_w	✅ Good
Termination functions	None visible	⚠️ Missing
Terrain border mesh	None visible	⚠️ Missing

The new termination functions (body_lin_vel_out_of_limit, body_ang_vel_out_of_limit) should have unit tests verifying they correctly detect NaN velocities and threshold violations.

CI Status

Only labeler check visible and passing. Full CI suite (tests, lint) status unknown.

Verdict

Approve with minor suggestions

This is a well-designed PR that brings significant improvements:

1. ✅ Clean abstraction layer for multi-backend transform queries
2. ✅ Proper contract testing ensures backend parity
3. ✅ Newton-specific fixes (shape margin, border mesh) address real collision issues
4. ✅ Armature presets enable Newton rough terrain training

The warnings above are non-blocking observations for potential improvement. The code is production-ready.

---

Reviewed commit: fdd2cbd

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot — Additional Review

Critical Bug Found

The previous review approved this PR, but I've identified a critical bug that will crash at runtime.

🔴 Bug: feet_slide crashes when asset_cfg.body_ids is default slice(None)

The change on line 87 of rewards.py introduces a regression:

body_vel = wp.to_torch(asset.data.body_lin_vel_w)[:, asset_cfg.body_ids[:contacts.shape[1]], :2]

When asset_cfg uses the default body_ids = slice(None), this will crash with:

TypeError: 'slice' object is not subscriptable

Reproduction:

s = slice(None)
s[:5]  # TypeError: 'slice' object is not subscriptable

Root cause: The PR description mentions this handles "Newton's ball-joint rod bodies (Digit)" but the implementation doesn't handle the common case where body_ids is a slice.

Suggested fix: Convert the slice to indices first, or check the type:

if isinstance(asset_cfg.body_ids, slice):
    # slice(None) means all bodies, so use all up to contact count
    body_ids = list(range(contacts.shape[1]))
else:
    body_ids = asset_cfg.body_ids[:contacts.shape[1]]
body_vel = wp.to_torch(asset.data.body_lin_vel_w)[:, body_ids, :2]

Verdict

Request Changes — The feet_slide bug will crash any locomotion env using the default asset_cfg for this reward. This must be fixed before merge.

---

Reviewed commit: fdd2cbd

[isaaclab-review-bot]

🔴 Bug: slice object is not subscriptable

When asset_cfg uses the default body_ids = slice(None), this line crashes with TypeError: 'slice' object is not subscriptable.

>>> slice(None)[:5]
TypeError: 'slice' object is not subscriptable

The comment mentions handling Newton's duplicate body entries, but this breaks the common case where no explicit body_ids are configured.

Suggested fix:

Suggested change

	# even when Newton reports duplicate body entries for closed-loop constraints.
	# Use sensor body count to slice asset velocities — ensures matching dimensions
	# even when Newton reports duplicate body entries for closed-loop constraints.
	if isinstance(asset_cfg.body_ids, slice):
	# Default slice(None) means all bodies — just take the first N matching sensor count
	body_ids = slice(contacts.shape[1])
	else:
	body_ids = asset_cfg.body_ids[:contacts.shape[1]]
	body_vel = wp.to_torch(asset.data.body_lin_vel_w)[:, body_ids, :2]

Dismissing: bot incorrectly used CHANGES_REQUESTED. Bot policy is COMMENT-only — never APPROVE or REQUEST_CHANGES.

[greptile-apps]

mdp.body_lin_vel_out_of_limit does not exist — every env will crash on instantiation

TerminationsCfg.body_lin_vel calls mdp.body_lin_vel_out_of_limit, but that function is not defined anywhere in the codebase. The local terminations.py only contains terrain_out_of_bounds, and neither the core isaaclab.envs.mdp.terminations module nor the lazy_export()-driven mdp.__init__ exposes a body_lin_vel_out_of_limit symbol.

The PR description (item 5) explicitly says this function was dropped from #5225, yet the code adds a DoneTerm using it. Every LocomotionVelocityRoughEnvCfg subclass will raise AttributeError: module 'mdp' has no attribute 'body_lin_vel_out_of_limit' as soon as the config is resolved. Either define the function in the local terminations.py, import it from the correct module, or remove the body_lin_vel termination term.

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

Summary

This PR enables Newton rough-terrain locomotion training across all 10 locomotion envs by hoisting Octi's Anymal-D foundation into the shared base config, adding NewtonShapeCfg to expose collision margin/gap settings, and introducing checked_apply for safe config forwarding. The implementation is architecturally sound but has one critical bug where accessing base_com.default on a preset() wrapper fails when Newton is active (returns None), and the mass randomization rewrite changes training behavior without deprecation.

Architecture Impact

• checked_apply is a new utility in isaaclab.utils that mirrors upstream dataclass fields onto external objects with strict validation. Used by NewtonManager.create_builder to forward NewtonShapeCfg onto Newton's ShapeConfig. Low blast radius, well-isolated.
• RoughPhysicsCfg is hoisted into velocity_env_cfg.py as the shared physics preset. All 10 per-robot rough env configs now inherit this instead of defining their own physics. This is a significant simplification.
• EventsCfg consolidation moves physics_material, add_base_mass, and base_com from per-env classes into the shared base. The mass randomization changes from additive (-5, 5) kg to multiplicative log-uniform (1/1.25, 1.25).

Implementation Verdict

Significant concerns — one critical bug in per-env configs accessing base_com.default.params on a preset() wrapper, plus missing test coverage for the new rough terrain configs.

Test Coverage

• ✅ checked_apply has thorough unit tests covering happy path, missing field detection, and non-dataclass rejection
• ✅ test_go1_rough_newton_armature_preset covers the Go1 armature preset
• 🔴 No tests for the mass randomization behavior change (additive → multiplicative)
• 🔴 No tests verifying the per-env base_com.default.params access pattern works with the preset() wrapper

CI Status

Most checks are pending or successful. Pre-commit passed, wheel built successfully.

Findings

🔴 Critical: source/isaaclab_tasks/.../go1/rough_env_cfg.py:35 — AttributeError when accessing base_com.default.params on Newton preset

The preset() helper returns a PresetCfg wrapper, not the actual EventTerm. When base_com = preset(default=EventTerm(...), newton=None), accessing self.events.base_com.default.params in __post_init__ requires the .default attribute on the PresetCfg wrapper. However, looking at the full velocity_env_cfg.py:

base_com = preset(
    default=EventTerm(...),
    newton=None,
)

Then in go1/rough_env_cfg.py:35:

self.events.base_com.default.params["asset_cfg"].body_names = "trunk"

This assumes base_com is a PresetCfg with a .default attribute. After Hydra resolves presets to the active backend, base_com becomes either the EventTerm (for PhysX) or None (for Newton). If this code runs after preset resolution, this will raise AttributeError: 'EventTerm' object has no attribute 'default' on PhysX, or AttributeError: 'NoneType' object has no attribute 'default' on Newton.

The same pattern appears in:

• a1/rough_env_cfg.py:32
• digit/rough_env_cfg.py:236
• anymal_c/rough_env_cfg.py:24 (only sets armature but similar pattern)

🔴 Critical: source/isaaclab_tasks/.../velocity_env_cfg.py:215-223 — Mass randomization semantic change without deprecation

The mass randomization changed from:

"mass_distribution_params": (-5.0, 5.0),
"operation": "add",

to:

"mass_distribution_params": (1 / 1.25, 1.25),
"operation": "scale",
"distribution": "log_uniform",

This is a behavioral change that affects training reproducibility for existing users. A -5kg perturbation on a 50kg robot is -10%, but on a 12kg Go1 is -42%. The new scale-invariant approach is better, but this should be documented as a breaking change or gated behind a version flag for reproducibility.

🟡 Warning: source/isaaclab_newton/.../newton_manager.py:437-440 — Fallback NewtonShapeCfg() uses different defaults than documented

cfg = PhysicsManager._cfg
shape_cfg = cfg.default_shape_cfg if isinstance(cfg, NewtonCfg) else NewtonShapeCfg()

When cfg is not a NewtonCfg, the fallback NewtonShapeCfg() uses margin=0.0 (the class default), not margin=0.01 (the rough-terrain value). This means non-Newton contexts don't get the rough-terrain margin as the docstring claims. The docstring says "rough-terrain margin/gap still apply during early construction" but the code applies margin=0.0.

🟡 Warning: source/isaaclab_tasks/.../digit/rough_env_cfg.py:237-244 — Actuator override may break PhysX

The Digit config now overrides the entire actuators dict:

self.scene.robot.actuators = {
    "legs_arms": ImplicitActuatorCfg(
        joint_names_expr=LEG_JOINT_NAMES + ARM_JOINT_NAMES,
        stiffness=None,
        damping=None,
    ),
}

This replaces whatever actuators were defined in DIGIT_V4_CFG. If DIGIT_V4_CFG has backend-specific actuator settings that differ between PhysX and Newton, this override loses them. The comment explains this is for Newton's ball joint representation, but it applies unconditionally to both backends.

🔵 Improvement: source/isaaclab/isaaclab/utils/configclass.py:662-668 — checked_apply could be more defensive

The function iterates dataclasses.fields(src) but doesn't handle the case where target might have the attribute as a property or descriptor that raises on setattr. Consider catching and re-raising with context:

try:
    setattr(target, f.name, getattr(src, f.name))
except Exception as e:
    raise type(e)(f"Failed to apply {f.name}: {e}") from e

🔵 Improvement: source/isaaclab_newton/.../newton_manager_cfg.py:227 — NewtonShapeCfg.gap default differs from docstring

gap: float = 0.01
"""Default per-shape contact gap [m]. Newton's upstream default is ``None``."""

The docstring says Newton's upstream default is None, but the wrapper default is 0.01. This is intentional but the docstring should clarify that Isaac Lab's wrapper default differs from upstream for rough-terrain stability.

🔵 Improvement: Missing CHANGELOG entry for mass randomization change

The isaaclab_tasks CHANGELOG mentions the new feature but doesn't document that mass randomization changed from additive to multiplicative, which affects training reproducibility for existing rough-terrain experiments.

[kellyguo11]

why was the randomization changed from the previous settings?

[hujc7]

The goal is to avoid per-rboto tuning for the original fixed addition method. The robot has different weight and I thought making it weight based should be better. Ablation study was also done to show that it behaves about the same or better, except Cassie. A1 is lighter and it trains better with this. Cassie seems to prefer being heavier so only it has specific tuning.

Ablation study seems to be dropped from PR. It's added back now.

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

Summary

This PR enables Newton rough-terrain locomotion training across all locomotion velocity environments by hoisting physics configuration into a shared base class and adding NewtonShapeCfg with checked_apply for safe config forwarding. The core implementation is correct, but there are several issues around test coverage, a potential runtime error in the Digit config, and missing handling of a preset wrapper access pattern.

Architecture Impact

• checked_apply in isaaclab/utils/configclass.py is a new public utility that will be used by Newton and potentially other physics backends to forward wrapper configs onto upstream dataclasses. Low risk, well-tested.
• RoughPhysicsCfg hoisted into velocity_env_cfg.py now affects all 10 robot rough-terrain configs. Any change to this shared preset propagates everywhere.
• EventsCfg changes (mass randomization from additive to multiplicative) affect training dynamics for all robots — this is an intentional behavioral change but could cause regressions if existing trained policies relied on the old mass distribution.
• Per-env configs are now significantly simpler, delegating physics to the shared base. Cross-module coupling increased but duplication decreased.

Implementation Verdict

Minor fixes needed

Test Coverage

• ✅ checked_apply has thorough unit tests covering happy path, missing field detection, and non-dataclass rejection
• ✅ Go1 armature preset test added
• 🟡 No tests for the new RoughPhysicsCfg preset resolution or NewtonShapeCfg forwarding in create_builder
• 🟡 No regression tests for the mass randomization behavior change (additive → multiplicative)

CI Status

Several jobs still pending (Installation Tests, Build Docker Images, license-check). Pre-commit and core tests passed.

Findings

🔴 Critical: source/isaaclab_tasks/.../digit/rough_env_cfg.py:230 — Accessing .default on preset without checking if preset is active

self.events.base_com.default.params["asset_cfg"].body_names = "torso_base"

When running with newton preset active via Hydra, self.events.base_com will be None (per the preset(default=..., newton=None) in the base class). This line will raise AttributeError: 'NoneType' object has no attribute 'default'. The A1 and Go1 configs have the same pattern but they don't set base_com = None for Newton in their inheritance path — however Digit inherits the shared EventsCfg which does. Fix: guard with if self.events.base_com is not None: or access via the preset wrapper pattern used elsewhere.

🔴 Critical: source/isaaclab_tasks/.../a1/rough_env_cfg.py:32 — Same .default access issue

self.events.base_com.default.params["asset_cfg"].body_names = "trunk"

Same issue as Digit. When Newton preset is active, base_com is None. This will crash at config instantiation time for Newton runs.

🔴 Critical: source/isaaclab_tasks/.../go1/rough_env_cfg.py:35 — Same pattern

self.events.base_com.default.params["asset_cfg"].body_names = "trunk"

Same NoneType crash for Newton preset.

🟡 Warning: source/isaaclab_newton/.../newton_manager.py:448-450 — Fallback to NewtonShapeCfg() may not match intent

cfg = PhysicsManager._cfg
shape_cfg = cfg.default_shape_cfg if isinstance(cfg, NewtonCfg) else NewtonShapeCfg()

When PhysicsManager._cfg is None (early construction before initialize() is called), this falls back to NewtonShapeCfg() which has margin=0.0. However, the PR description states margin is critical for rough terrain. If create_builder is called before config is set, the margin won't be applied. Consider whether this fallback should use a margin-enabled default or if the caller should be required to pass the config explicitly.

🟡 Warning: source/isaaclab_tasks/.../velocity_env_cfg.py:211-216 — base_com preset wrapper may confuse downstream code

base_com = preset(
    default=EventTerm(...),
    newton=None,
)

This creates a PresetCfg wrapper object at class level. The per-env configs then try to access self.events.base_com.default.params which only works if the preset hasn't been resolved yet. After resolve_cfg_presets() runs (which happens in env creation), base_com becomes either the EventTerm or None directly. The access pattern is fragile and depends on timing.

🔵 Improvement: source/isaaclab/test/utils/test_configclass.py:1148-1208 — Tests use local @configclass instead of testing actual integration
The checked_apply tests define local test classes rather than testing the actual NewtonShapeCfg → Newton ShapeConfig forwarding. Consider adding an integration test that imports the real Newton types to verify field names match.

🔵 Improvement: source/isaaclab_tasks/.../cassie/rough_env_cfg.py:65 — Inconsistent body name pattern

self.events.base_external_force_torque.params["asset_cfg"].body_names = ".*pelvis"

Uses regex ".*pelvis" while other configs use exact names like "trunk" or "torso_link". This inconsistency could match unintended bodies if naming conventions change. Consider using "pelvis" for consistency.

🔵 Improvement: source/isaaclab_newton/docs/CHANGELOG.rst:18 — Changelog mentions PR #5289 but this is PR #5248

the bug class PR #5289 fixed for Newton ``ShapeConfig.contact_margin`` → ``margin``

The changelog references PR #5289 but this PR is #5248. If #5289 is a dependency or related PR, this is fine, but if it's a typo it should be corrected.