[WIP][Rough Locomotion] Part 3: G1 Newton fixes#5312
[WIP][Rough Locomotion] Part 3: G1 Newton fixes#5312hujc7 wants to merge 28 commits intoisaac-sim:developfrom
Conversation
The ShapeConfig field is ``margin``, not ``contact_margin``. The incorrect name silently created a dead attribute so the intended 1 cm default shape margin was never applied, degrading contact quality on triangle-mesh terrain for lightweight robots.
Consolidate per-env Newton solver settings into a shared RoughPhysicsCfg in the base LocomotionVelocityRoughEnvCfg, removing duplicated config from each robot's rough_env_cfg. Per-robot overrides: - Go1: armature=0.02 Newton preset for training stability - Go2: njmax=400/nconmax=200 to prevent constraint budget overflow - All non-"base" robots: body name overrides for base_com event Also adds a Hydra preset regression test for the Go1 armature config.
…into jichuanh/rough-terrain-clean
- 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.
Go2 Newton was training to +6.86 on 1500 iter vs +13.73 PhysX (50% parity). Missing armature preset caused under-damped joint oscillation: dof_acc_l2 penalty was -0.35 (vs Go1 -0.12), 30% base_contact terminations, terrain curriculum stuck at 1.22. Mirroring Go1's existing preset (armature=0.02 for newton, default 0.0) lifts Go2 Newton to +18.58 on 1500 iter — 135% of PhysX baseline, 98% of Go1 Newton. PhysX path unchanged. Also drops the redundant 'physx=0.0' key from Go1's preset since it restates the default. The preset is now Newton-only, consistent with convention.
github-actions
Bot
added
documentation
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR #5312 Code Review: G1 Rough-Terrain Tuning (WIP) + FrameView + Newton Improvements
This PR contains three interconnected workstreams:
- G1 rough-terrain locomotion tuning (WIP) with Newton-specific armature/damping presets
- FrameView architecture refactoring to abstract backend-specific frame views
- 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 dRecommendation: 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 typeGood - 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-5These magic constants lack documentation explaining their purpose:
- Why
0.1, 0.0, 0.05specifically for child offset? - Is
ATOL=1e-5appropriate 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-floatingThe 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:
ScalarPresetwith 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
-
FrameView Architecture: The
BaseFrameView→FrameView→ backend-specific implementations pattern is clean and follows good OOP principles. -
preset()Hydra Integration: The concept of backend-conditional values (preset(default=X, newton=Y)) is elegant for multi-backend configs. -
Newton Shape Margin Fix: The CHANGELOG entry for
contact_margin→margintypo fix is well-documented. -
Test Infrastructure: The expanded
test_hydra.pywith 1200+ lines of tests shows good coverage of the new preset system. -
Warp Compatibility: The systematic
wp.to_torch()wrapping in MDP functions enables Newton backend support.
Recommended Actions
- Clarify
preset()function signature - consider explicit dict API - Document magic constants in
usd_frame_view.py - Fix stale comment about 37 DOF in G1 config
- Add migration notes for RayCaster changes
- File follow-up issue for Newton duplicate body entries
- Add missing test cases for ScalarPreset edge cases
Reviewed at commit: a67b001
There was a problem hiding this comment.
🟡 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.
There was a problem hiding this comment.
🟡 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.
There was a problem hiding this comment.
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_contactregression 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.
| # terminations | ||
| self.terminations.base_contact.params["sensor_cfg"].body_names = "torso_link" | ||
|
|
||
| # G1 H1: remove finger joints from the action space. 14 finger joints contribute |
There was a problem hiding this comment.
Minor typo: "G1 H1" should be "G1" - H1 doesn't have finger joints (H1 only has legs, feet, and arms actuator groups).
| # 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 |
There was a problem hiding this comment.
🟡 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.
There was a problem hiding this comment.
🟡 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.
There was a problem hiding this comment.
✅ 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.
The feet_slide change in 07265de added `asset_cfg.body_ids[:contacts.shape[1]]` to work around an assumed Newton body-name duplication for closed-loop bodies. Verified empirically: USD has 2 bodies matching `.*_leg_toe_roll` on both backends; the regex does not over-match `.*_rod_roll` auxiliary joints. The slicing was unnecessary and crashed with the default `SceneEntityCfg('robot')` (where body_ids is `slice(None)`) — flagged by review bot. Fixes: - rewards.py: restore `body_lin_vel_w[:, asset_cfg.body_ids, :2]` - digit/rough_env_cfg.py: restore regex `body_names='.*_leg_toe_roll'` matching the sensor_cfg pattern; remove misleading comment. Tested: Digit PhysX 10-iter and Digit Newton 5-iter both run feet_slide without crashes.
Bump Newton pin from 2684d75 (1.1.0) to 381781c2 (1.2.0) to fix biped contact quality on triangle-mesh terrain. H1 and Cassie fail on 1.1.0 but train on 1.2.0. Per-biped fixes (restore overrides dropped in 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. - Cassie: add leg armature=0.02 for stability on rough terrain. Out of scope (pre-existing config issues, fail on PhysX too): - G1: barely trains on PhysX (reward=0.14, 300 iter) — finger joints in action space dilute learning. Needs separate PR. - Digit: falls in 7 steps on both Newton and PhysX — closed-loop kinematics / init pose issue. Needs separate PR.
…on armature/damping presets Reduces action_rate_l2 penalty from -0.6 to -0.25 by restricting the 14 finger joints from the action vector; locomotion-relevant joints only (hips, knees, ankles, shoulders, elbows, torso). Adds Newton-only armature (0.03) and damping (20.0) presets on the legs actuator to stabilize G1 under the MJWarp solver where the USD baseline values (armature=0.01, damping=5.0) produce excessive oscillation. Measured results (1500 iter, 4096 envs, last30 avg reward): - G1 PhysX baseline: +11.68 - G1 PhysX (fingers-out-of-action): +20.54 (beats H1 PhysX +18.15) - G1 Newton baseline: -2.33 - G1 Newton (fingers-out-of-action): +5.35 - G1 Newton (fingers + armature + damping): +9.61 (47% of PhysX) Newton still lags PhysX on G1; residual gap is physics-level and out of scope for this PR. Marking WIP pending further tuning or upstream Newton improvements.
hujc7
force-pushed
the
jichuanh/g1-rough-terrain-wip
branch
from
a67b001 to
a4d0ebc
Compare
isaaclab-review-bot
Bot
dismissed
their stale review
Dismissing: bot incorrectly used CHANGES_REQUESTED. Bot policy is COMMENT-only — never APPROVE or REQUEST_CHANGES.
hujc7
changed the title
2 participants
This PR records work-in-progress G1 rough-terrain tuning. Not ready for
merge. Tracking record of current best results and the residual Newton gap.
Depends on: #5298 (Newton pin bump +
mujoco-warp 3.6) — G1 Newton won'tload without those. Base branch shows both PRs' changes combined here on GitHub's
"Files changed" tab; the actual PR delta is a single edit to
g1/rough_env_cfg.py.Changes (single file)
source/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/g1/rough_env_cfg.py:PhysX path is unchanged (presets resolve to
default).Measured results (1500 iter, 4096 envs, last30 avg reward)
Why fingers out of action
G1 has 14 finger joints that participate in the action space. At 1500 iter the
action_rate_l2reward reached −0.60 (3× H1's −0.20), purely from theextra dims. Removing them cuts the penalty to −0.248 and the locomotion
policy becomes learnable. PhysX goes from struggling to beating H1.
Why Newton-only armature + damping
After the action-space fix, G1 PhysX is solved but Newton still plateaus
near 40% of PhysX. The gap is physics-level: higher
base_contactrate(13.6% Newton vs 5% PhysX), terrain curriculum progresses more slowly.
Go2's armature preset pattern (PR #5248) closes similar under-damped
joint issues; applied here with Newton-only presets to avoid affecting
PhysX.
Damping bump from USD's 5.0 to 20.0 adds further stabilization but
with diminishing returns (+1.5 reward,
base_contactrate regresses7% → 18.7%). Included here for completeness of the tuning record;
reviewers may prefer to drop the damping change if the base_contact
regression is considered worse than the reward gain.
Remaining Newton gap (47% of PhysX)
After all the fixes in this PR, G1 Newton reaches ~+9.6 vs G1 PhysX's
+20.5. The residual gap is not addressable by further tuning of the
knobs explored here. Candidates for future work:
mujoco-warpimprovements in kinematics representationStatus
(cleanly valuable on its own; Newton work can follow independently)
{armature 0.02/0.03/0.05} × {damping 5/15/20/30}
Dependencies
mujoco-warp 3.6); required for G1 Newton to even loadpreset()helper + RoughPhysicsCfg)