[WIP][Rough Locomotion] Part 3: G1 on Newton (max_iterations 3000→5000, no physics tuning)

[hujc7]

⚠️ WIP / draft

Tracking record for G1 rough-terrain on Newton. Single-line preset bump on max_iterations. Keep open until reviewed.

1. Summary

Enable G1 rough-terrain training on Newton. The only engine-specific change is a ~1.7× bump on max_iterations (Newton = 5000, PhysX = 3000). No physics, solver, reward, or action-space tuning — the G1 rough env config is identical on both backends.

2. Dependencies

1. PR Add checked_apply helper for forwarding configclass fields onto upstream dataclasses #5365 — checked_apply helper.
2. PR [Newton] Add Rough terrain locomotion Part 1: Foundation + quadrupeds on Newton #5248 — quadruped Newton support, shared RoughPhysicsCfg, NewtonShapeCfg(margin=0.01).
3. PR [Rough Locomotion] Part 2: H1/Cassie bipeds on Newton #5298 — biped reset overrides (G1 inherits the position_range = (1.0, 1.0) from there).

3. Core finding — PhysX saturates at iter 3000; Newton matches by iter 5000

Ran both backends for 7500 iter on identical configs. Reward alone is misleading — PhysX reward oscillates +16 to +19 across its entire run past iter 3000 without improving. Episode length confirms the plateau:

iter	PhysX reward	PhysX ep_len	Newton reward	Newton ep_len
3000	+18.14	983	+6.21	979
4000	+19.38	986	+10.65	983
5000	+18.04	978	+16.01	984
5500	+18.19	983	+16.60	985
6000	+17.07	981	+18.86	996
6500	+16.71	989	+19.80	996
7000	+16.27	968	+18.10	976
7500	+15.81	—	+17.56	969

• PhysX plateau: both metrics stable at iter 3000 (+18.14 / 983). No meaningful gain past iter 3000.
• Newton at iter 5000: (+16.01 / 984) — matches PhysX quality on both reward and ep_len.
• Newton at iter 6000+: equals or exceeds PhysX on both metrics.

Episode length > 960 everywhere means the robot is stable (not falling) even when reward is low — reward alone is not a sufficient convergence signal.

4. Ablation record

Tested at L40, 4096 envs, seed=42, Newton @ 381781c2 (1.2.0.dev0):

Variant	iter	Reward	Verdict
A0 — Vanilla Newton	3000	+6.21	Newton slower but learning
A1 — Newton armature 0.01	3000	+5.14	No help
A2 — Newton armature 0.03	3000	+6.43	No help
D0 — Vanilla Newton (extended)	5000	+16.01	Matches PhysX plateau quality
E0 — Vanilla PhysX	3000	+18.14	PhysX plateau

Key observations:

• Armature tuning (0.01, 0.03) does not change Newton's convergence rate on G1.
• Damping preset (5 → 20) and finger-removal also tested in earlier rounds; no durable benefit once Newton is given enough iterations.
• Newton catches PhysX by iter 5000 on vanilla config, so the framework-level max_iterations bump is sufficient on its own.

5. Change

source/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/g1/agents/rsl_rl_ppo_cfg.py:

from isaaclab_tasks.utils import preset

class G1RoughPPORunnerCfg(RslRlOnPolicyRunnerCfg):
    # Newton needs ~1.7x the PPO iterations to match PhysX on G1. PhysX saturates near iter 3000
    # (reward ≈ +18, ep_len ≈ 980) and does not meaningfully improve on either metric past that.
    # Newton reaches the same (reward, ep_len) quality at iter 5000 (+16 / 984). The gap is
    # sample-efficiency, not a ceiling — no physics or reward tuning closes it.
    max_iterations = preset(default=3000, newton=5000)

G1 rough_env_cfg.py is unchanged vs the parent bipeds branch — no finger removal, no armature preset, no damping preset.

Precedent for per-robot max_iterations tuning: Allegro Hand (5000), Spot (20000).

6. Versions

• isaaclab_tasks 1.5.26 → 1.5.27

Type of change

• New feature (non-breaking).

[isaaclab-review-bot]

PR #5312 Code Review: G1 Rough-Terrain Tuning (WIP) + FrameView + Newton Improvements

This PR contains three interconnected workstreams:

1. G1 rough-terrain locomotion tuning (WIP) with Newton-specific armature/damping presets
2. FrameView architecture refactoring to abstract backend-specific frame views
3. Newton backend improvements including shape margin fixes and MDP warp compatibility

Overall Assessment: REQUEST_CHANGES

While this PR contains valuable architectural improvements, there are several issues that need addressing before merge.

---

🔴 Critical Issues

1. preset() Function Signature Ambiguity

File: source/isaaclab_tasks/isaaclab_tasks/utils/hydra.py

The preset() function uses **kwargs with a magic default key, but this creates ambiguity when users want a preset named "default":

def preset(default=_MISSING, **kwargs):
    if default is _MISSING and "default" not in kwargs:
        raise ValueError("preset() requires 'default=' or a kwarg named 'default'")

Issue: The function allows both preset(default=X) and preset(newton=Y, default=X) but the semantics differ - in the first case default is positional, in the second it's a kwarg. This is confusing.

Recommendation: Use explicit dict construction: preset({"default": X, "newton": Y}) or require all keys be kwargs.

2. ScalarPreset Serialization Asymmetry

File: source/isaaclab_tasks/isaaclab_tasks/utils/hydra.py:46-73

The ScalarPreset class implements to_dict() but from_dict() would reconstruct a plain scalar, not a ScalarPreset. This breaks round-trip serialization:

sp = preset(default=0.01, newton=0.03)
d = sp.to_dict()  # Returns resolved scalar
# Cannot reconstruct ScalarPreset from d

Recommendation: Either document this limitation clearly or implement proper serialization.

3. Missing Type Annotations in New Termination Functions

File: source/isaaclab/isaaclab/envs/mdp/terminations.py:161-185

New functions body_lin_vel_out_of_limit and body_ang_vel_out_of_limit are missing return type annotations and have inconsistent docstring format:

def body_lin_vel_out_of_limit(
    env: ManagerBasedRLEnv,
    max_velocity: float,
    asset_cfg: SceneEntityCfg = SceneEntityCfg("robot"),
) -> torch.Tensor:  # ✓ Has return type

Good - these are correct. But the docstrings lack Args/Returns sections that other functions in this file have.

---

🟡 Medium Issues

4. Hardcoded CHILD_OFFSET and ATOL Constants

File: source/isaaclab/isaaclab/sim/views/usd_frame_view.py:29-30

CHILD_OFFSET = (0.1, 0.0, 0.05)
ATOL = 1e-5

These magic constants lack documentation explaining their purpose:

• Why 0.1, 0.0, 0.05 specifically for child offset?
• Is ATOL=1e-5 appropriate for all use cases?

Recommendation: Add docstrings explaining these values' origins and when they might need adjustment.

5. RayCaster Major Refactoring Without Migration Path

File: source/isaaclab/isaaclab/sensors/ray_caster/ray_caster.py

The complete removal of _obtain_trackable_prim_view() (75+ lines) and switch to FrameView is a breaking change. The old code had specific handling for:

• ArticulationView detection
• RigidBodyView detection
• XformPrimView fallback with warnings

Issue: The new FrameView abstraction assumes the backend handles this, but there's no deprecation warning or migration guide.

Recommendation: Add a CHANGELOG entry and migration notes for users relying on the old behavior.

6. feet_slide Body ID Slicing Hack

File: source/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/mdp/rewards.py:87-88

# Use sensor body count to slice asset velocities — ensures matching dimensions
# even when Newton reports duplicate body entries for closed-loop constraints.
body_vel = wp.to_torch(asset.data.body_lin_vel_w)[:, asset_cfg.body_ids[:contacts.shape[1]], :2]

Issue: This is a workaround for a Newton backend inconsistency, not a proper fix. The comment acknowledges Newton "reports duplicate body entries" - this should be fixed in Newton, not papered over in reward functions.

Recommendation: File a follow-up issue to fix Newton's body reporting. This workaround should be temporary.

7. G1 Action Space Comment Mismatch

File: source/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/g1/rough_env_cfg.py:35-36

# We need to include the joints above as well, to get the 37 DOF action space.
# This is less problematic since we can let the finger joints free-floating

The PR removes 14 finger joints from the action space (37 → 23 DOF), but the comment still references "37 DOF action space". This is stale documentation.

---

🟢 Minor Issues / Suggestions

8. Unused Import in interactive_scene.py

File: source/isaaclab/isaaclab/scene/interactive_scene.py

The PR removes several imports but the TYPE_CHECKING block imports DeformableObject and SurfaceGripper which may not be used if those backends aren't active.

9. Test Coverage for ScalarPreset

File: source/isaaclab_tasks/test/test_hydra.py

The test file is comprehensive (1200+ lines) but I don't see explicit tests for:

• ScalarPreset with all-same-value presets (edge case)
• preset() with invalid/empty kwargs
• Round-trip serialization (which would catch issue #2)

10. torch.norm → torch.linalg.norm Migration

Throughout rewards.py and terminations.py, the PR migrates from deprecated torch.norm to torch.linalg.norm. This is good practice, but should be noted in the PR description for searchability.

---

✅ Positive Observations

1. FrameView Architecture: The BaseFrameView → FrameView → backend-specific implementations pattern is clean and follows good OOP principles.

2. preset() Hydra Integration: The concept of backend-conditional values (preset(default=X, newton=Y)) is elegant for multi-backend configs.

3. Newton Shape Margin Fix: The CHANGELOG entry for contact_margin → margin typo fix is well-documented.

4. Test Infrastructure: The expanded test_hydra.py with 1200+ lines of tests shows good coverage of the new preset system.

5. Warp Compatibility: The systematic wp.to_torch() wrapping in MDP functions enables Newton backend support.

---

Recommended Actions

1. Clarify preset() function signature - consider explicit dict API
2. Document magic constants in usd_frame_view.py
3. Fix stale comment about 37 DOF in G1 config
4. Add migration notes for RayCaster changes
5. File follow-up issue for Newton duplicate body entries
6. Add missing test cases for ScalarPreset edge cases

---

Reviewed at commit: a67b001

[isaaclab-review-bot]

🟡 Magic Constants: CHILD_OFFSET and ATOL lack documentation.

• Why (0.1, 0.0, 0.05) for CHILD_OFFSET?
• What is the origin of ATOL=1e-5?

Please add docstrings explaining these constants' purpose and when users might need to adjust them.

[isaaclab-review-bot]

🟡 Workaround for Newton Bug: The slicing asset_cfg.body_ids[:contacts.shape[1]] papers over a Newton backend issue.

The comment says Newton "reports duplicate body entries for closed-loop constraints" - this should be fixed in Newton itself, not worked around in reward functions.

Suggestion: File a follow-up issue to fix Newton's body reporting and mark this as a temporary workaround with a TODO.

[isaaclab-review-bot]

WIP PR Review: G1 Rough-Terrain Tuning

Reviewed the WIP commit a67b0017 which adds G1-specific rough terrain tuning on top of the parent PR #5298.

Summary

This is a well-documented WIP PR that addresses a real training issue (14 finger joints inflating action_rate_l2 penalty 3× vs H1) and applies the established Newton armature/damping preset pattern from Go2. The measured results (+20.54 PhysX, +9.61 Newton) demonstrate meaningful improvement.

Overall

For a WIP PR focused on recording tuning progress, this is clean and well-structured. The code follows established patterns (preset() for Newton-specific values, actuators["legs"] matching the G1_CFG definition). The detailed PR description with measured results is appreciated.

When promoting from WIP:

• Consider whether the damping bump (5.0→20.0) is worth the base_contact regression noted in the PR description (7%→18.7%)
• The armature-only variant (+8.10 reward) may be a cleaner landing point than armature+damping (+9.61 with contact regression)
• As noted in the PR, this could potentially be split: fingers-out-of-action fix (clean PhysX improvement) could land independently of the Newton tuning work

No blocking issues found. The one inline comment is a minor documentation fix.

[isaaclab-review-bot]

Minor typo: "G1 H1" should be "G1" - H1 doesn't have finger joints (H1 only has legs, feet, and arms actuator groups).

Suggested change

	# G1 H1: remove finger joints from the action space. 14 finger joints contribute
	# G1: remove finger joints from the action space. 14 finger joints contribute

[isaaclab-review-bot]

🟡 Stale Documentation: Comments in this file reference "37 DOF action space" but this PR removes 14 finger joints, reducing the action space to 23 DOF.

Please update the comments to reflect the actual action space dimensions after finger joint removal.

[isaaclab-review-bot]

🟡 Breaking Change: The complete removal of _obtain_trackable_prim_view() (75+ lines) is a significant refactoring.

The old code had specific handling for:

• ArticulationView detection via UsdPhysics.ArticulationRootAPI
• RigidBodyView detection via UsdPhysics.RigidBodyAPI
• XformPrimView fallback with helpful warning messages

Please add a CHANGELOG entry and migration notes for users who may have been relying on the old behavior or its warning messages.

[isaaclab-review-bot]

✅ Good Addition: The new body_lin_vel_out_of_limit and body_ang_vel_out_of_limit functions are useful safety checks that can prevent NaN propagation from solver singularities.

Minor suggestion: Consider adding Args/Returns sections to the docstrings for consistency with other functions in this file.

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

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

Summary

This PR (Part 3 of rough locomotion) adds Newton backend support for the G1 biped on rough terrain. The implementation is minimal and well-documented: a single preset(default=3000, newton=5000) on max_iterations in G1RoughPPORunnerCfg is the only G1-specific change. The PR description provides thorough ablation data showing Newton reaches PhysX-equivalent quality (reward ≈ +16, ep_len ≈ 984) at iter 5000 vs PhysX's plateau at iter 3000.

This is a clean, well-scoped change. The decision to bump iteration budget rather than tune physics/reward terms keeps the env config engine-agnostic, following precedent from Allegro Hand (5000 iter) and Spot (20000 iter).

Architecture Impact

No cross-module impact from the G1-specific changes. The max_iterations preset is localized to the PPO runner config. The rest of the PR stacks parent changes from #5298 (biped Newton support) and #5248 (quadruped Newton support), which have broader impact but were covered in prior reviews.

Implementation Verdict

Ship it — The G1-specific change (commit 99f50d7d) is correct and follows established patterns. The comment explains the rationale thoroughly.

Test Coverage

Acceptable for this change type. This is a hyperparameter tuning PR (iteration count), not a code behavior change. The existing rough-terrain training infrastructure provides implicit coverage. The PR description includes ablation data demonstrating the change achieves parity:

iter	PhysX reward	PhysX ep_len	Newton reward	Newton ep_len
3000	+18.14	983	+6.21	979
5000	+18.04	978	+16.01	984

The data validates the 5000 iteration preset is sufficient for Newton to match PhysX plateau quality.

CI Status

Only labeler check is present (passed ✅). No pre-commit/lint/test failures detected.

Branch Status

⚠️ This branch has diverged from develop (30 commits behind, 29 ahead). Consider rebasing before merge to pick up recent changes and ensure CI runs against current develop.

Findings

No new issues found in the G1-specific changes (commit 99f50d7d). The change is minimal, well-documented, and follows established patterns.

Prior review comments still applicable: The stacked commits from parent PRs have existing review comments (FrameView constants documentation, Newton body reporting workaround, etc.) that should be addressed in those parent PRs.

---

Reviewed at commit: 99f50d7

[hujc7]

@greptileai Review — please re-review. Force-pushed onto updated #5298 base (rebase only; no content change in this PR's own commit). Stack: #5365 → #5248 → #5298 → #5312.

[greptile-apps]

Greptile Summary

This PR is broader than its WIP title suggests: it delivers the checked_apply utility (PR #5365), NewtonShapeCfg with margin support (PR #5248), a major refactoring of all rough-terrain locomotion configs to a single shared RoughPhysicsCfg / EventsCfg base, and the headline G1 max_iterations preset (3000 → 5000 for Newton). The empirical convergence evidence in the PR description is thorough and the refactoring is clean.

• The new body_lin_vel termination at 20 m/s is silently added to all rough-terrain envs; worth confirming the threshold doesn't clip lightweight robots (Go1, A1) under aggressive push or reset conditions.
• Several Newton per-robot configs previously disabled push_robot; those overrides are now gone (behavioral change folded into the shared base).

Confidence Score: 5/5

Safe to merge; all remaining findings are P2 style/informational items that do not block correctness.

The core checked_apply utility is well-tested and the Newton shape config wiring is correct. The G1 max_iterations preset is the only engine-specific change and is backed by solid empirical data. The broad config refactoring appears intentional and the CHANGELOG captures all behavioral changes. No P0 or P1 issues found.

velocity_env_cfg.py — new body_lin_vel termination warrants a sanity check for lightweight quadrupeds under push events.

Important Files Changed

Filename	Overview
source/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/velocity_env_cfg.py	Major refactor: hoists shared RoughPhysicsCfg, moves startup events into base EventsCfg, adds body_lin_vel termination to all rough-terrain envs, and changes events: EventsCfg = MISSING → concrete default; broadly touches all robot training configs.
source/isaaclab/isaaclab/utils/configclass.py	Adds checked_apply helper that forwards declared dataclass fields onto an external target, raising AttributeError on mismatch; import ordering has a minor style issue.
source/isaaclab_newton/isaaclab_newton/physics/newton_manager.py	Replaces hard-coded builder.default_shape_cfg.gap = 0.01 with checked_apply(shape_cfg, ...), correctly falling back to NewtonShapeCfg() defaults when no NewtonCfg is active.
source/isaaclab_newton/isaaclab_newton/physics/newton_manager_cfg.py	Introduces NewtonShapeCfg (margin, gap) and adds default_shape_cfg: NewtonShapeCfg field to NewtonCfg; well-documented defaults.
source/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/g1/agents/rsl_rl_ppo_cfg.py	Core PR change: replaces max_iterations = 3000 with preset(default=3000, newton=5000) for G1 rough training, with well-documented empirical justification.
source/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/cassie/rough_env_cfg.py	Removes per-backend event classes; adds Newton armature preset and asymmetric (1.0, 1.25) mass-distribution override; minor formatting artifact (double blank line inside post_init).
source/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/go2/rough_env_cfg.py	Removes per-backend event classes and the Go2-specific PhysicsCfg (PhysX patch count is now preserved via the shared RoughPhysicsCfg); adds Newton armature preset.
source/isaaclab/test/utils/test_configclass.py	Adds three well-structured tests for checked_apply: forwards all fields, raises on missing target field, and rejects non-dataclass src.
source/isaaclab_tasks/test/test_hydra.py	Adds regression test verifying Go1 Newton armature preset resolves correctly to 0.02 without touching PhysX value.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["LocomotionVelocityRoughEnvCfg\n(sim: SimulationCfg(physics=RoughPhysicsCfg()))"]

    subgraph Physics["RoughPhysicsCfg (PresetCfg)"]
        P1["default/physx: PhysxCfg\n(gpu_max_rigid_patch_count=10x2^15)"]
        P2["newton: NewtonCfg\n(MJWarpSolverCfg + NewtonCollisionPipelineCfg\n+ NewtonShapeCfg(margin=0.01))"]
    end

    subgraph Events["EventsCfg (now shared base)"]
        E1["physics_material (startup)"]
        E2["add_base_mass (startup, log-uniform scale)"]
        E3["base_com (preset: PhysX=EventTerm, Newton=None)"]
        E4["push_robot (interval)"]
        E5["reset_base / reset_robot_joints (reset)"]
    end

    subgraph Terminations["TerminationsCfg"]
        T1["time_out"]
        T2["base_contact"]
        T3["body_lin_vel <= 20 m/s (NEW)"]
    end

    subgraph Robots["Per-robot overrides"]
        R1["G1: add_base_mass=None\nmax_iterations preset(3000->5000 Newton)"]
        R2["Cassie: mass_dist=(1.0,1.25), armature preset"]
        R3["H1: body_names=torso_link"]
        R4["Go1/Go2: armature preset(0->0.02 Newton)"]
        R5["Digit: explicit joint names"]
        R6["Anymal B/C/D: no overrides needed"]
    end

    subgraph CheckedApply["checked_apply"]
        CA["NewtonShapeCfg -> ModelBuilder.default_shape_cfg\nRaises AttributeError on field mismatch"]
    end

    A --> Physics
    A --> Events
    A --> Terminations
    A --> Robots
    P2 -->|create_builder| CA

Loading

Comments Outside Diff (3)

1. source/isaaclab/isaaclab/utils/configclass.py, line 64 (link)

   import dataclasses out of standard import position

   import dataclasses is inserted mid-block between from copy import deepcopy and from dataclasses import .... PEP 8 groups bare import X lines before from X import Y lines within the same stdlib section; placing it here breaks the existing convention in this file.

   Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

2. source/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/cassie/rough_env_cfg.py, line 708-711 (link)

   Spurious double blank line inside __post_init__

   Two consecutive blank lines between statements inside a function body violate PEP 8 (double blank lines are reserved for top-level definitions). Looks like a merge artifact.

3. source/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/go2/rough_env_cfg.py, line 29-30 (link)

   Spurious double blank line after super().__post_init__()

   Same double-blank-line artifact as in cassie/rough_env_cfg.py; should be a single blank line inside the method body.

_{Reviews (1): Last reviewed commit: "Bump G1 max_iterations to 5000 on Newton..." | Re-trigger Greptile}

[greptile-apps]

New body_lin_vel termination applies to every rough-terrain env

body_lin_vel_out_of_limit at 20 m/s is now a universal termination condition across all rough-terrain robots (Anymal, Go1/2, A1, H1, Cassie, Digit, G1). While 20 m/s is a generous safety cap and the intent (catching simulation explosions) is good, this is an additive behavioral change for all existing environments that wasn't present before. If any robot canonically reaches near that speed during certain reset conditions or in edge-case terrain interactions, the episode will silently terminate differently than before — worth verifying the threshold is safe for the lightest/fastest robots (Go1, A1) under aggressive push events.