[Rough Locomotion] Part 2: H1/Cassie bipeds on Newton#5298
Conversation
Greptile SummaryThis PR restores biped-specific Confidence Score: 5/5Safe to merge; all changes are targeted config overrides with no P0/P1 findings. All changes are incremental config-level overrides following established patterns already present for other robots. No logic errors, type errors, or missing guards were found. The only finding is a P2 CHANGELOG omission. No files require special attention beyond the CHANGELOG omission. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
Base["LocomotionVelocityRoughEnvCfg\n(EventsCfg defaults)\nreset_robot_joints: position_range=(0.5,1.5)\nadd_base_mass: body='base', scale=(1/1.25,1.25)"]
Base --> H1["H1RoughEnvCfg\n+ body_names='torso_link'\n+ position_range=(1.0,1.0)\n(inherits shared scale)"]
Base --> Cassie["CassieRoughEnvCfg\n+ body_names='pelvis'\n+ scale=(1.0,1.25) asymmetric\n+ armature=preset(default=0.0,newton=0.02)\n+ position_range=(1.0,1.0)"]
Base --> Digit["DigitRoughEnvCfg\n+ body_names='torso_base'\n+ position_range=(1.0,1.0)"]
Base --> G1["G1RoughEnvCfg\n+ add_base_mass=None\n+ position_range=(1.0,1.0)"]
Reviews (4): Last reviewed commit: "Re-enable add_base_mass on H1/Cassie wit..." | Re-trigger Greptile |
| # scene | ||
| self.scene.robot = CASSIE_CFG.replace(prim_path="{ENV_REGEX_NS}/Robot") | ||
| # Cassie armature for biped stability on rough terrain | ||
| self.scene.robot.actuators["legs"].armature = 0.02 |
There was a problem hiding this comment.
actuators["legs"].armature = 0.02 is set unconditionally, so it also modifies the PhysX backend's Cassie env. Every other robot that uses a backend-specific armature (Go1, AnymalC) wraps the value in preset(default=0.0, newton=0.02, physx=0.0). If this is intentional for PhysX too, a comment explaining why would help; if it's Newton-only, it should use a preset to avoid silently changing the existing PhysX training baseline.
| self.scene.robot.actuators["legs"].armature = 0.02 | |
| self.scene.robot.actuators["legs"].armature = preset(default=0.0, newton=0.02, physx=0.0) |
| ^^^^^ | ||
|
|
||
| * Added Newton rough terrain support for all remaining locomotion velocity | ||
| envs (Go1, Go2, A1, H1, Cassie, Anymal-B/C, G1, Digit). |
There was a problem hiding this comment.
The entry says Newton rough terrain is added for G1 and Digit, but the PR description documents that both still fail on the current Newton build (G1: ep_len=899 / reward=-6.97; Digit: ep_len=7 / reward=-3.25) due to pre-existing config issues. Including them in an "Added" bullet misleads users into thinking those envs are ready. Consider revising the bullet to only credit H1 and Cassie, and add a note for G1/Digit as a known issue.
| Changelog | ||
| --------- | ||
|
|
||
| 0.5.14 (2026-04-16) | ||
| ~~~~~~~~~~~~~~~~~~~ | ||
|
|
||
| Fixed | ||
| ^^^^^ | ||
|
|
||
| * Fixed incorrect attribute name ``contact_margin`` on Newton | ||
| ``ShapeConfig`` in | ||
| :meth:`~isaaclab_newton.physics.NewtonManager.create_builder`. The | ||
| correct field is ``margin``. The typo created a dead attribute so the | ||
| intended 1 cm default shape margin was never applied. |
There was a problem hiding this comment.
setup.py bumps the Newton dependency from commit 2684d75 (1.1.0.dev0) to 381781c2 (1.2.0.dev0), but the current isaaclab_newton changelog top entry (0.5.14) only documents the contact_margin typo fix. Per the project's guidelines, every source-directory change requires a new version heading and entry. The pin bump — particularly the breaking API changes it carries (rigid_contact_normal direction flip, body_qd realignment, shape_heightfield_data rename, SensorContact.measure_total, new wp.array[X] syntax) — warrants its own Changed section in a new 0.5.15 release.
- Drop Go2 njmax/nconmax override claim (not in rough_env_cfg code). - Narrow scope to quadrupeds; biped support is in follow-up PR isaac-sim#5298.
- Drop Go2 njmax/nconmax override claim (not in rough_env_cfg code). - Narrow scope to quadrupeds; biped support is in follow-up PR isaac-sim#5298.
There was a problem hiding this comment.
Code Review: PR #5298 — Enable H1/Cassie rough terrain on Newton (pin bump)
Thank you for this substantial PR that enables H1 and Cassie bipeds on rough terrain for Newton with an impressive +127-132% PhysX parity score! The new FrameView architecture is well-designed. Below is a detailed review covering correctness, simplicity, and completeness.
✅ What Looks Good
-
FrameView Architecture — Clean abstract base class (
BaseFrameView) with well-defined contracts and backend-specific implementations (USD, Fabric, Newton). The factory pattern inFrameViewautomatically selects the correct backend at runtime. -
Comprehensive Contract Tests — The
test_frame_view_contract.pydefines invariants that all backends must satisfy, with separate test files for each backend. This is excellent for ensuring parity. -
Newton GPU Kernels — The Warp kernels in
NewtonSiteFrameVieware correctly designed for GPU-resident pose computation with proper handling of world-attached sites (site_body == -1). -
Sensor Refactoring — The
_resolve_and_spawnhelper insensor_base.pyelegantly handles the physics-body-to-child-prim redirection with a clear behavior matrix documented in the docstring. -
New Terminations — The
body_lin_vel_out_of_limitandbody_ang_vel_out_of_limitterminations correctly check for NaN velocities (solver singularities) in addition to magnitude limits. -
Config Consolidation — Moving startup events into the base
EventsCfgreduces duplication across robot configs and makes the preset system more maintainable.
🔶 Suggestions / Minor Issues
1. Deprecation Warning Removal for attach_yaw_only
The deprecation warning code for attach_yaw_only was removed from ray_caster.py, but the attribute definition and its docstring remain in ray_caster_cfg.py. This leaves a deprecated attribute that now silently does nothing.
Suggestion: Either:
- Keep the deprecation warning code to give users a migration path, OR
- Remove the attribute entirely from
RayCasterCfg(breaking change, but cleaner)
# In ray_caster_cfg.py — attribute still exists but is now a no-op:
attach_yaw_only: bool | None = None2. Cassie-Specific Armature Setting
The armature adjustment for Cassie is applied directly to the actuator config:
# cassie/rough_env_cfg.py
self.scene.robot.actuators["legs"].armature = 0.02This modifies the shared CASSIE_CFG object, which could leak to other configs if they reuse the same config instance.
Suggestion: Use .replace() or ensure CASSIE_CFG is always copied:
self.scene.robot = CASSIE_CFG.replace(prim_path="{ENV_REGEX_NS}/Robot")
# Then modify the copy3. Test Coverage for _resolve_and_spawn
The new _resolve_and_spawn method in sensor_base.py has a well-documented behavior matrix, but I didn't see dedicated unit tests for all the documented paths (physics body redirect, non-physics existing prim, spawn at non-existing path, etc.).
Suggestion: Add unit tests covering each row of the behavior matrix to prevent regressions.
4. Potential Race Condition in FabricFrameView
In FabricFrameView.__init__, there's a physics event callback registration:
self._physics_event_callback = sim_ctx.physics_manager.subscribe_physics_event(...)If the view is created after physics stepping has already begun, ensure the initial state is properly synchronized.
🔍 Questions for Clarification
-
Breaking API Change: The removal of
ray_cast_utils.pyand_obtain_trackable_prim_viewremoves the automatic ArticulationView/RigidBodyView creation logic. Is this intentional? Users who relied on this auto-detection will now need to update their code. -
Newton Collision Pipeline: The new
NewtonCollisionPipelineCfg(max_triangle_pairs=2_500_000)is a significant memory allocation. Is this value empirically tuned for rough terrain, or should it be documented as a tuning parameter? -
PhysX Parity at Higher Iterations: The parity results show Newton at 127-132% of PhysX reward. Was this with matched solver iteration counts? It would be helpful to note if Newton required different solver settings to achieve stability.
📋 Checklist
- Newton pin bump version verified (
381781c2→ 1.2.0.dev0) - mujoco-warp version updated to 3.6.0
- FrameView contract tests pass for all backends
- Biped reset position_range fixed to (1.0, 1.0)
- Documentation updated for new terminations
- CI labeler check passes
Summary
This is a well-structured PR that successfully enables biped locomotion on rough terrain for Newton. The FrameView abstraction is a solid architectural improvement. The main suggestions are around cleaning up the deprecated attach_yaw_only attribute and adding test coverage for the new _resolve_and_spawn helper.
Recommendation: Approve with minor suggestions addressed.
43520bc to
7f8a773
Compare
…verrides) Change EventsCfg.add_base_mass from additive (-5, 5) kg uniform to multiplicative (1/1.25, 1.25) log-uniform scale with distribution="log_uniform" and operation="scale". The range is symmetric in log space (geometric mean = 1.0), so the randomization has no net mass bias and is scale-invariant across robot sizes — a single default works for the 2 kg pelvis of Cassie and the 32 kg base of Anymal-D without per-robot tuning. Drop the per-robot (-1.0, 3.0) additive overrides on A1, Go1, Go2 (introduced in PR isaac-sim#3308 [Newton] Locomotion Velocity-Flat envs, 2025-09-18) which have a +1 kg mean shift that biases controllers toward always-heavier nominal — a 10% bias on A1's 10 kg base that was actively hurting convergence. ## Why the change The old (-5, 5) kg additive default was a reasonable tune for Anymal-D (≈16% range) but too wide for A1 (≈50%) and too narrow for H1 (≈10%). PR isaac-sim#3308 addressed this with per-robot (-1, 3) overrides on small quadrupeds, but the range remained asymmetric (mean shifted +1 kg) and tuning was per-robot. A multiplicative scale is more physically meaningful — mass scales in ratio with actuator torque and gravity response, so the perturbation's effect on gait is a fixed fraction regardless of robot mass. Log-uniform over [1/1.25, 1.25] gives symmetric perturbation in log space with geometric mean = 1.0, so the training distribution is unbiased. ## Ablation results (1500-iter Newton unless noted) | Robot | old default | new log-uniform | Δ | |-------------|------------:|----------------:|------------:| | A1 @300 | 3.25 | 10.00 | 3.08x | | Go1 @300 | 16.30 | 22.29 | 1.37x | | Anymal-B @300 | 10.92 | 12.47 | 1.14x | | Anymal-C @300 | 12.31 | 14.64 | 1.19x | | Go2 @1499 | 18.58 | 24.71 | 1.33x | | Anymal-D @1499 | 15.62 | 16.09 | 1.03x | A1's 3x gain is the bias-removal effect: the old (-1, 3) applied +10% mean mass shift on a 10 kg robot, forcing the controller to spend capacity compensating for a nonphysical average instead of learning the nominal gait. The new default's geometric mean 1.0 removes that bias entirely. Biped configurations (H1, Cassie) are handled in the follow-up PR isaac-sim#5298. Cassie specifically requires an asymmetric heavier-bias override due to its closed-loop Achilles coupling being mass-sensitive; that override ships alongside the biped changes.
7f8a773 to
09e0418
Compare
…verrides) Change EventsCfg.add_base_mass from additive (-5, 5) kg uniform to multiplicative (1/1.25, 1.25) log-uniform scale with distribution="log_uniform" and operation="scale". The range is symmetric in log space (geometric mean = 1.0), so the randomization has no net mass bias and is scale-invariant across robot sizes — a single default works for the 2 kg pelvis of Cassie and the 32 kg base of Anymal-D without per-robot tuning. Drop the per-robot (-1.0, 3.0) additive overrides on A1, Go1, Go2 (introduced in PR isaac-sim#3308 [Newton] Locomotion Velocity-Flat envs, 2025-09-18) which have a +1 kg mean shift that biases controllers toward always-heavier nominal — a 10% bias on A1's 10 kg base that was actively hurting convergence. ## Why the change The old (-5, 5) kg additive default was a reasonable tune for Anymal-D (≈16% range) but too wide for A1 (≈50%) and too narrow for H1 (≈10%). PR isaac-sim#3308 addressed this with per-robot (-1, 3) overrides on small quadrupeds, but the range remained asymmetric (mean shifted +1 kg) and tuning was per-robot. A multiplicative scale is more physically meaningful — mass scales in ratio with actuator torque and gravity response, so the perturbation's effect on gait is a fixed fraction regardless of robot mass. Log-uniform over [1/1.25, 1.25] gives symmetric perturbation in log space with geometric mean = 1.0, so the training distribution is unbiased. ## Ablation results (1500-iter Newton unless noted) | Robot | old default | new log-uniform | Δ | |-------------|------------:|----------------:|------------:| | A1 @300 | 3.25 | 10.00 | 3.08x | | Go1 @300 | 16.30 | 22.29 | 1.37x | | Anymal-B @300 | 10.92 | 12.47 | 1.14x | | Anymal-C @300 | 12.31 | 14.64 | 1.19x | | Go2 @1499 | 18.58 | 24.71 | 1.33x | | Anymal-D @1499 | 15.62 | 16.09 | 1.03x | A1's 3x gain is the bias-removal effect: the old (-1, 3) applied +10% mean mass shift on a 10 kg robot, forcing the controller to spend capacity compensating for a nonphysical average instead of learning the nominal gait. The new default's geometric mean 1.0 removes that bias entirely. Biped configurations (H1, Cassie) are handled in the follow-up PR isaac-sim#5298. Cassie specifically requires an asymmetric heavier-bias override due to its closed-loop Achilles coupling being mass-sensitive; that override ships alongside the biped changes.
bf724d2 to
a2998e5
Compare
|
@greptileai Review — please re-review the latest commits (log-uniform mass randomization on #5248; biped H1/Cassie re-enable on #5298). Thanks. |
| "mujoco-warp==3.5.0.2", | ||
| "PyOpenGL-accelerate==3.1.10", | ||
| "newton @ git+https://github.com/newton-physics/newton.git@2684d75bfa4bb8b058a93b81c458a74b7701c997", | ||
| "newton @ git+https://github.com/newton-physics/newton.git@381781c2", |
There was a problem hiding this comment.
mujoco-warp pin conflicts with Newton 1.2.0 requirement
setup.py still pins mujoco-warp==3.5.0.2 while Newton 381781c2 (1.2.0.dev0) declares mujoco-warp~=3.6.0 in its own pyproject.toml. pip's modern resolver will raise ResolutionImpossible when both constraints are active, causing pip install -e ".[all]" to fail outright. The --no-deps footgun the PR description mentions is a symptom of the same root conflict.
| "mujoco-warp==3.5.0.2", | |
| "PyOpenGL-accelerate==3.1.10", | |
| "newton @ git+https://github.com/newton-physics/newton.git@2684d75bfa4bb8b058a93b81c458a74b7701c997", | |
| "newton @ git+https://github.com/newton-physics/newton.git@381781c2", | |
| "mujoco==3.5.0", | |
| "mujoco-warp~=3.6.0", | |
| "PyOpenGL-accelerate==3.1.10", | |
| "newton @ git+https://github.com/newton-physics/newton.git@381781c2", |
| asset_cfg: SceneEntityCfg = SceneEntityCfg("robot"), | ||
| ) -> torch.Tensor: |
There was a problem hiding this comment.
wp.to_torch() breaks on PhysX where body_lin_vel_w is already a torch tensor
wp.to_torch() accepts only Warp arrays — it does not accept torch tensors. On the PhysX backend asset.data.body_lin_vel_w is a regular torch.Tensor, so wp.to_torch(tensor) raises a TypeError at runtime. The same pattern applies to body_ang_vel_out_of_limit below. Since this termination is wired into TerminationsCfg shared by all robots and all backends (velocity_env_cfg.py:body_lin_vel), every PhysX training run that hits this termination code path will crash.
Replace with a direct tensor slice:
body_vel = asset.data.body_lin_vel_w[:, asset_cfg.body_ids]There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
This PR enables H1 and Cassie bipeds on rough terrain with Newton backend (achieving 127-132% of PhysX reward at 1500 iterations), introduces the new FrameView architecture with backend-agnostic pose queries, and re-enables base-mass randomization on bipeds with appropriate overrides. The implementation is solid with comprehensive training parity data.
Architecture Impact
FrameView refactoring: The new FrameView factory pattern (BaseFrameView → UsdFrameView, FabricFrameView, NewtonSiteFrameView) is a significant architectural improvement. The XformPrimView alias maintains backward compatibility. Multiple callers updated: isaaclab_mimic, terrain tests, ray caster sensors, interactive scene.
Biped config changes: H1 and Cassie now inherit from the consolidated EventsCfg base with log-uniform mass randomization. The position_range=(1.0, 1.0) fix for biped init poses is applied to all four bipeds (H1, Cassie, G1, Digit).
Cross-module impact: Low risk — changes are additive (new terminations, new FrameView backends) or consolidating existing patterns (mass randomization). The Newton pin bump (1.1.0 → 1.2.0) is required but API-compatible per the CHANGELOG notes.
Implementation Verdict
Ship it with conflict resolution — Clean implementation with well-documented training results. The FrameView architecture is well-designed with proper contract tests. The biped-specific overrides (Cassie armature, asymmetric mass randomization) are backed by empirical ablation data.
Test Coverage
✅ Contract tests: test_frame_view_contract.py defines invariants all backends must satisfy (world/local pose consistency, indexed operations, parent-child relationships). Each backend (USD, Fabric, Newton) has its own test file importing these shared tests.
✅ New terminations: body_lin_vel_out_of_limit and body_ang_vel_out_of_limit correctly check for NaN velocities (solver singularities) in addition to magnitude limits.
✅ Hydra preset test: The existing test_hydra.py now tests the Go1 armature Newton preset.
🟡 Missing: The new _resolve_and_spawn helper in sensor_base.py has a well-documented behavior matrix but no dedicated unit tests covering all paths (physics body redirect, non-physics existing prim, spawn at non-existing path).
CI Status
- labeler: ✅ pass
- pre-commit/Build and Test: Not shown — may be pending or running on a different workflow
Branch Status
CONFLICTING / DIRTY merge state). The PR cannot be merged until conflicts are resolved. The author should rebase or merge from develop to resolve.
Findings
🔵 Improvement: source/isaaclab/isaaclab/sensors/sensor_base.py — Missing test coverage for _resolve_and_spawn
The new helper has a 5-row behavior matrix documented in the docstring, but no dedicated unit tests. Consider adding tests for each documented path to prevent regressions.
🟢 Note: Cassie armature applied directly to CASSIE_CFG.actuators
In cassie/rough_env_cfg.py:
self.scene.robot.actuators["legs"].armature = preset(default=0.0, newton=0.02)This modifies the actuator config on the replaced robot config. Since CASSIE_CFG.replace() returns a copy and preset() creates a new PresetCfg, this is safe. No action needed.
🟢 Note: G1 disables mass randomization entirely
G1 sets self.events.add_base_mass = None while H1 uses the shared default and Cassie uses an asymmetric override. This is appropriate per the PR description (G1 needs separate tuning work tracked in #5312).
🟢 Note: Digit base_com references .default attribute
self.events.base_com.default.params["asset_cfg"].body_names = "torso_base"This works because base_com is a PresetCfg with default=EventTerm(...), newton=None. Accessing .default is intentional to modify the PhysX-only event. Correct.
Recommendation: Resolve merge conflicts and this is ready to ship. The previous review's suggestions (deprecation warning for attach_yaw_only, test coverage for _resolve_and_spawn) remain valid but are minor improvements, not blockers.
a2998e5 to
e25f8ac
Compare
e25f8ac to
5810b24
Compare
5810b24 to
b6fd87d
Compare
e73fe0d to
1d2669f
Compare
…eam dataclasses (#5365) # Description Adds `isaaclab.utils.checked_apply` for forwarding the declared fields of one dataclass onto another, raising `AttributeError` if the target is missing a declared field. The use case is Isaac Lab configclasses that mirror an upstream library's dataclass (for example, Newton's `ShapeConfig`). Bare `setattr` loops are fragile: if upstream renames or removes a field, every write becomes a silent no-op (the failure mode PR #5289 fixed for `ShapeConfig.contact_margin` → `margin`). With `checked_apply`, the failure surfaces at startup with a clear message instead of degrading runtime behavior. ## API ```python from isaaclab.utils import checked_apply @configclass class NewtonShapeCfg: margin: float = 0.0 gap: float = 0.01 # at apply site (one line, no per-field setattr noise) checked_apply(cfg.default_shape_cfg, builder.default_shape_cfg) ``` Internally: 1. Iterates `dataclasses.fields(src)` — single source of truth for declared fields. 2. Raises `AttributeError` if `target` lacks a declared field. 3. Rejects non-dataclass `src` with `TypeError`. ## What's included 1. `source/isaaclab/isaaclab/utils/configclass.py` — `checked_apply` function (lives next to `@configclass` since it operates on dataclasses). 2. `source/isaaclab/isaaclab/utils/__init__.pyi` — export. 3. `source/isaaclab/test/utils/test_configclass.py` — three tests (forwards all fields, raises on missing target field, rejects non-dataclass src). 4. `source/isaaclab/docs/CHANGELOG.rst` — `4.6.13` entry. 5. `source/isaaclab/config/extension.toml` — version bump. ## Dependents This PR is a dependency for the rough-terrain Newton stack: 1. PR #5248 — quadrupeds rough terrain, uses `checked_apply` to forward `NewtonShapeCfg` onto Newton's upstream `ShapeConfig`. Without it, `default_shape_cfg.margin` is left at Newton's upstream default of `0.0`, breaking all non-Anymal-D robots on triangle-mesh terrain. 2. PR #5298 — bipeds (chains on #5248). 3. PR #5312 — G1 (chains on #5298). ## Type of change - New feature (non-breaking). ## Checklist - [x] Tests added (3 new in `test_configclass.py`) - [x] Pre-commit checks pass - [x] CHANGELOG + extension.toml bumped - [x] No new dependencies --------- Signed-off-by: Kelly Guo <kellyg@nvidia.com> Co-authored-by: Kelly Guo <kellyg@nvidia.com>
… on Newton (#5248) # 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](https://github.com/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](#5225 (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 #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: ```python @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 - [x] Pre-commit checks pass - [x] CHANGELOG + extension.toml bumped on both `isaaclab_newton` and `isaaclab_tasks` - [x] Co-author credit for [@ooctipus](https://github.com/ooctipus) on the cherry-pick commit - [x] Ablation evidence cited in commit messages --------- Signed-off-by: Kelly Guo <kellyg@nvidia.com> Co-authored-by: Octi Zhang <zhengyuz@nvidia.com> Co-authored-by: Kelly Guo <kellyg@nvidia.com>
1d2669f to
0ee439a
Compare
|
@greptileai Review — please re-review on the freshly rebased HEAD. Stack collapsed: #5365 (
6 files, +41/-5 vs develop. CHANGELOG bumped to 1.5.29. Earlier P0/P1 findings have been resolved or are stale (Newton pin bump, mujoco-warp conflict, |
…eam dataclasses (isaac-sim#5365) # Description Adds `isaaclab.utils.checked_apply` for forwarding the declared fields of one dataclass onto another, raising `AttributeError` if the target is missing a declared field. The use case is Isaac Lab configclasses that mirror an upstream library's dataclass (for example, Newton's `ShapeConfig`). Bare `setattr` loops are fragile: if upstream renames or removes a field, every write becomes a silent no-op (the failure mode PR isaac-sim#5289 fixed for `ShapeConfig.contact_margin` → `margin`). With `checked_apply`, the failure surfaces at startup with a clear message instead of degrading runtime behavior. ## API ```python from isaaclab.utils import checked_apply @configclass class NewtonShapeCfg: margin: float = 0.0 gap: float = 0.01 # at apply site (one line, no per-field setattr noise) checked_apply(cfg.default_shape_cfg, builder.default_shape_cfg) ``` Internally: 1. Iterates `dataclasses.fields(src)` — single source of truth for declared fields. 2. Raises `AttributeError` if `target` lacks a declared field. 3. Rejects non-dataclass `src` with `TypeError`. ## What's included 1. `source/isaaclab/isaaclab/utils/configclass.py` — `checked_apply` function (lives next to `@configclass` since it operates on dataclasses). 2. `source/isaaclab/isaaclab/utils/__init__.pyi` — export. 3. `source/isaaclab/test/utils/test_configclass.py` — three tests (forwards all fields, raises on missing target field, rejects non-dataclass src). 4. `source/isaaclab/docs/CHANGELOG.rst` — `4.6.13` entry. 5. `source/isaaclab/config/extension.toml` — version bump. ## Dependents This PR is a dependency for the rough-terrain Newton stack: 1. PR isaac-sim#5248 — quadrupeds rough terrain, uses `checked_apply` to forward `NewtonShapeCfg` onto Newton's upstream `ShapeConfig`. Without it, `default_shape_cfg.margin` is left at Newton's upstream default of `0.0`, breaking all non-Anymal-D robots on triangle-mesh terrain. 2. PR isaac-sim#5298 — bipeds (chains on isaac-sim#5248). 3. PR isaac-sim#5312 — G1 (chains on isaac-sim#5298). ## Type of change - New feature (non-breaking). ## Checklist - [x] Tests added (3 new in `test_configclass.py`) - [x] Pre-commit checks pass - [x] CHANGELOG + extension.toml bumped - [x] No new dependencies --------- Signed-off-by: Kelly Guo <kellyg@nvidia.com> Co-authored-by: Kelly Guo <kellyg@nvidia.com>
… on Newton (isaac-sim#5248) 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](https://github.com/ooctipus)'s Anymal-D foundation work, cherry-picked from PR isaac-sim#5225. PR isaac-sim#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](isaac-sim#5225 (comment))) the required changes from isaac-sim#5225 are cherry-picked here so the rough-terrain stack can move forward without depending on his still-open WIP PR. - PR isaac-sim#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. Commit authored by Octi Zhang with `Co-authored-by` trailer. Subset of | # | 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 isaac-sim#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 isaac-sim#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 | 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 isaac-sim#5365: ```python @configclass class NewtonShapeCfg: margin: float = 0.0 gap: float = 0.01 default_shape_cfg: NewtonShapeCfg = NewtonShapeCfg() 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)`. 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. - **Go1**: leg armature preset for joint stability on lightweight quadruped. 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 isaac-sim#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 isaac-sim#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). - `isaaclab_newton` 0.5.21 → 0.5.22 - `isaaclab_tasks` 1.5.24 → 1.5.25 - New feature (non-breaking). - [x] Pre-commit checks pass - [x] CHANGELOG + extension.toml bumped on both `isaaclab_newton` and `isaaclab_tasks` - [x] Co-author credit for [@ooctipus](https://github.com/ooctipus) on the cherry-pick commit - [x] Ablation evidence cited in commit messages --------- Signed-off-by: Kelly Guo <kellyg@nvidia.com> Co-authored-by: Octi Zhang <zhengyuz@nvidia.com> Co-authored-by: Kelly Guo <kellyg@nvidia.com>
Restore per-biped overrides dropped by parent PR's consolidation: - All bipeds (H1, Cassie, G1, Digit): restore reset_robot_joints.params["position_range"] = (1.0, 1.0). Bipeds have precise init poses that should not be randomly scaled on reset. - Cassie: add leg armature=0.02 for stability on rough terrain. Out of scope (pre-existing issues that fail on PhysX too): - G1: finger joints dilute learning; addressed in follow-up PR. - Digit: closed-loop kinematics fail in 7 steps on both backends.
Restore base-mass randomization on H1 and Cassie that was disabled with add_base_mass = None in their rough-terrain configs (pre-existing biped convention from 2024-06 PR isaac-sim#444, later reinforced by PR isaac-sim#4165's Newton NaN TODO). The parent PR isaac-sim#5248 switches the shared default to log-uniform scale (1/1.25, 1.25), which is safer for bipeds than the old additive (-5, 5) kg (effectively ±25% on H1's torso vs ±100% on Cassie's pelvis). - H1 inherits the shared default (symmetric ±25% scale, body_names="torso_link"). - Cassie overrides to (1.0, 1.25) asymmetric heavier-bias: lighter-than- nominal pelvis destabilizes Cassie's closed-loop Achilles rod coupling and hip PD response, while heavier-than-nominal dampens dynamics. | Variant | reward | ep len | vs disabled | |--------------------------------|-------:|-------:|------------:| | None (disabled, prior default) | 23.93 | 910 | 1.00x | | (1/1.25, 1.25) sym25 | 14.15 | 850 | 0.59x x | | (1/1.10, 1.10) tight10 | 14.53 | 831 | 0.61x x | | **(1.0, 1.25) heavier25** | **21.50** | **932** | **0.90x** | Tightening the range symmetrically did not help (tight10 ~ sym25) — the lighter side is what destabilizes, not the magnitude. Restricting to [1.0, 1.25] (never lighter, up to +25% heavier) preserves most of the randomization benefit while avoiding the failure mode. Episode length also improves (932 vs 910), indicating the randomized policy is actively more stable during episodes — the lower aggregate reward comes from extra dof-torque regularizer paid to handle heavier instances, not degraded task completion. H1's larger base mass (≈15 kg torso) means ±25% on the default scale (11-19 kg range) is well within the controller's robustness margin. H1 reward at iter 1499: 24.02 with mass rand on vs 23.58 with it disabled (1.02x, essentially equal). Re-enabling provides sim-to-real robustness at negligible training cost.
0ee439a to
f5a903e
Compare
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
This is a drastically reduced PR scope — now containing only the biped-specific config overrides (position_range, mass randomization, Cassie armature) without the FrameView refactoring from the previous review. The changes are minimal and well-documented.
Architecture Impact
Self-contained — only biped locomotion configs are modified. No cross-module impact.
Implementation Verdict
Ship it
Test Coverage
Config-only changes that inherit from the already-tested shared EventsCfg. The PR description includes empirical training results validating the parameter choices.
CI Status
Most checks pending/skipped, but pre-commit ✅ and Build Wheel ✅ passed.
Findings
🔴 Critical: source/isaaclab_tasks/docs/CHANGELOG.rst:58 — Duplicate version number
The CHANGELOG has two entries for 1.5.28:
1.5.28 (2026-04-24)
~~~~~~~~~~~~~~~~~~~
...
1.5.28 (2026-04-27)
~~~~~~~~~~~~~~~~~~~One of these should be 1.5.29. This will cause confusion in version history tracking.
Previous findings about _resolve_and_spawn test coverage and FrameView are no longer applicable — that code is not in this PR. The scope is now purely biped config overrides, which are correct and well-justified by the training ablation data in the PR description.
The 'Re-enable add_base_mass on H1/Cassie' rebase resolved the CHANGELOG conflict by inserting our new 1.5.33 entry on top, preserving the content of every existing develop entry verbatim — but mislabeled what should have been '1.5.29 (2026-04-27)' as '1.5.28 (2026-04-24)' (a leftover heading from the PR's pre-rebase view of CHANGELOG). Net change is a single-line heading rename so develop's full version history is preserved correctly. Diff vs origin/develop is now exactly the new 1.5.33 entry at the top of the file, nothing else.
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
Previous concern about the duplicate 1.5.28 CHANGELOG entries has been addressed — the entries are now correctly versioned as 1.5.33, 1.5.32, etc.
Implementation Verdict
Ship it
The biped config overrides are correct, well-documented with training ablation data, and the CHANGELOG versioning is now consistent. No new issues introduced.
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
Follow-up review confirms all previous concerns have been addressed. The CHANGELOG versioning is now correct (1.5.33), and the biped reset overrides are properly implemented across H1, Cassie, Digit, and G1 configurations.
Implementation Verdict
Ship it
The changes are clean, well-documented with ablation data, and consistent across all biped configs. No new issues introduced in this commit.
1. Summary
Restores biped-specific reset overrides on H1, Cassie, Digit, G1 that were lost when the parent PR (#5248) consolidated startup events into the shared
EventsCfg. Re-enablesadd_base_massrandomization on H1 and Cassie with the new log-uniform scale default.This PR contains only the biped-level deltas — Newton physics, shape margin, and quadruped enablement all live in #5248.
2. Dependencies
checked_applyhelper.RoughPhysicsCfg,NewtonShapeCfg(margin=0.01).3. Changes
3.1 Restore biped reset overrides
Bipeds have precise initial poses that should not be randomly scaled on reset. The shared
EventsCfg.reset_robot_jointsusesposition_range = (0.5, 1.5); bipeds override to(1.0, 1.0):position_range = (1.0, 1.0)position_range = (1.0, 1.0)+ legarmature = 0.02for stability on rough terrainposition_range = (1.0, 1.0)position_range = (1.0, 1.0)3.2 Re-enable
add_base_masson H1 and CassiePer-env
add_base_mass = Noneoverrides on H1 and Cassie (pre-existing biped convention from PR #444, reinforced by PR #4165's Newton NaN TODO) are removed. The parent PR's new log-uniform scale default(1/1.25, 1.25)is safer for bipeds than the old additive(-5, 5)kg (which was effectively ±25% on H1's torso vs ±100% on Cassie's pelvis).body_names="torso_link").(1.0, 1.25)asymmetric heavier-bias: lighter-than-nominal pelvis destabilizes Cassie's closed-loop Achilles rod coupling and hip PD response, while heavier-than-nominal dampens dynamics.= None)(1/1.25, 1.25))(1.0, 1.25)H1 reward at iter 1499:
24.02with mass rand on vs23.58with it disabled — essentially equal; re-enabling provides sim-to-real robustness at negligible training cost.4. PhysX / Newton parity (1500 iter, 4096 envs, last30 avg)
Both bipeds reach parity or better on Newton at full 1500-iter training.
5. Versions
isaaclab_tasks1.5.25 → 1.5.26Type of change