Fixes default shape margin field name in Newton manager

[hujc7]

Summary

Fix incorrect attribute name contact_margin → gap on Newton ShapeConfig in NewtonManager.create_builder().

Newton PR #1732 renamed contact_margin to gap (broad-phase AABB expansion). The IsaacLab code was never updated, creating a dead attribute that Python silently accepted. The intended 1 cm default shape gap was never applied.

Newton PR #1732 rename mapping

Old field	New field	Semantics
thickness	margin	Shape surface extension (affects contact forces)
contact_margin	gap	Broad-phase AABB expansion (detection range only)
rigid_contact_margin	rigid_gap	Builder-level default gap (already 0.1)

Timeline

Date	Newton	IsaacLab	contact_margin valid?
Feb 19	pin: 51ce35e (has contact_margin)	#4646 adds contact_margin = 0.01	Yes
Feb 24	PR #1732 renames contact_margin → gap	—	—
Mar 2	pin updated to v0.2.3 (after rename)	#4761 keeps contact_margin	No — broken
Mar 9	—	#4883 removes the line	Removed
Apr 13	—	#4822 re-adds contact_margin	No — still broken

Ablation: gap vs margin

We conducted an ablation study to understand the impact. Note: margin (shape surface
extension) is a different field from gap (broad-phase range). The original code intended
to set gap, but setting margin also has a significant positive effect on training for
rough terrain locomotion.

Setting	gap	margin	Go1 Reward (300 iter)	Effect
Dead field (baseline)	0.1 (default)	0.0	~1.0	—
gap=0.01 only	0.01	0.0	0.66	No training improvement
margin=0.01 only	0.1 (default)	0.01	18.77	Major improvement
gap=0.01 + margin=0.01	0.01	0.01	16.54	Similar to margin-only

This PR fixes the correct field migration (contact_margin → gap). The margin setting
for rough terrain contact quality will be addressed separately in the rough terrain env PR
(#5248) via a new default_shape_margin config field on NewtonCfg.

Test plan

• Verified contact_margin is not a field on ShapeConfig (Newton 1.1.0.dev0)
• Verified gap is the correct replacement field per Newton PR [Proposal] Incorrect clipping of actions #1732
• Confirmed by camevor (Newton developer)
• Ablation study: gap alone doesn't change training, so existing tests should pass

[greptile-apps]

Greptile Summary

Fixes a silent typo in NewtonManager.create_builder() where default_shape_cfg.contact_margin = 0.01 created a dead Python attribute instead of setting the real margin field, causing the intended 1 cm default shape margin to never be applied to the Newton model. The changelog and version bump follow project conventions correctly.

Confidence Score: 5/5

Safe to merge — single-line corrective fix with no API changes, validated on multiple robots.

The change is a one-line typo fix, extensively validated by the author on five robot platforms with quantitative training metrics. Changelog and version bump follow project rules. The only remaining finding (P2) is a pre-existing inconsistency in instantiate_builder_from_stage that is not introduced by this PR.

No files require special attention for merge.

Important Files Changed

Filename	Overview
source/isaaclab_newton/isaaclab_newton/physics/newton_manager.py	Single-line fix: default_shape_cfg.contact_margin → default_shape_cfg.margin in create_builder() at line 425; the old attribute name silently created a dead attribute, leaving the intended 1 cm margin unapplied.
source/isaaclab_newton/docs/CHANGELOG.rst	New version 0.5.14 heading added with a well-formed "Fixed" entry documenting the margin field name correction; RST formatting and version bump are correct.
source/isaaclab_newton/config/extension.toml	Version bumped from 0.5.13 to 0.5.14, matching the new changelog entry.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A[NewtonManager.create_builder] -->|Creates| B[ModelBuilder]
    B -->|Before fix| C["default_shape_cfg.contact_margin = 0.01\n(dead attribute, margin stays 0.0)"]
    B -->|After fix| D["default_shape_cfg.margin = 0.01\n(margin correctly applied)"]
    D --> E[builder.finalize → Model]
    C --> F[builder.finalize → Model]
    E -->|shape margin = 0.01 m| G[Physics Simulation]
    F -->|shape margin = 0.0 m| H[Degraded contact quality]

    style D fill:#90EE90
    style C fill:#FFB6C1
    style H fill:#FFB6C1
    style G fill:#90EE90

Loading

Comments Outside Diff (1)

1. source/isaaclab_newton/isaaclab_newton/physics/newton_manager.py, line 663-677 (link)

   Builders in instantiate_builder_from_stage bypass the 1 cm margin

   create_builder() is now the only place that sets default_shape_cfg.margin = 0.01, but instantiate_builder_from_stage() constructs both the main builder and the prototype directly via ModelBuilder(up_axis=...) (lines 663 and 677), bypassing that helper. Users who reach the auto-stage path will get margin = 0.0 instead of the intended default.

   Consider routing through create_builder() or manually applying the same default here for consistency:

   builder = cls.create_builder(up_axis=up_axis)
   # and
   proto = cls.create_builder(up_axis=up_axis)

_{Reviews (1): Last reviewed commit: "Fix default shape margin field name in N..." | Re-trigger Greptile}

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

Summary

Clean, well-diagnosed one-line bug fix. The create_builder() method was setting default_shape_cfg.contact_margin = 0.01, but Newton's ModelBuilder.ShapeConfig defines the field as margin — not contact_margin. Python silently accepted the dynamic attribute, so the intended 1 cm default shape margin was never applied to the actual simulation. The fix correctly renames the attribute to margin.

Design Assessment

Design is sound. This is the right fix at the right level — correcting the field name where the default is applied. The approach is minimal and surgical. Version bump and CHANGELOG entry are included and follow the existing conventions.

Findings

🔵 Suggestion: Defensive guard against future typos — newton_manager.py:425

This class of bug (Python silently accepting dynamic attribute assignment on a config object) is insidious because there's no runtime error. The PR author's investigation confirms this went undetected across multiple commits (#4646, #4761, #4822). Consider whether ShapeConfig (upstream in Newton) could benefit from __slots__ or a frozen dataclass to prevent silent attribute creation — though that's outside this PR's scope and would be an upstream Newton change.

Test Coverage

This is a field-name typo fix with no new logic paths. The author's manual test plan is thorough (Go1, Go2, A1, H1, Cassie on rough terrain) and demonstrates a dramatic improvement (reward 1.0 → 18.77 at 300 iter). An automated regression test here would require a full Newton simulation harness and is disproportionate for a one-character-class fix. No new tests needed.

CI Status

✅ All checks passing (labeler: pass).

Verdict

Ship it 🟢

Textbook bug fix: clear diagnosis, minimal change, proper documentation, version bump, and validated across multiple robot configurations. The CHANGELOG entry is well-written and explains both the cause and the impact. Nice work.

[AntoineRichard]

LGTM, thanks @hujc7! As mentioned, make a separate PR for testing these fields!

[AntoineRichard]

@camevor do you know why the newton tests fails? It looks like the force reported are larger than expected when the contact margin are large?

[camevor]

ShapeConfig.margin is not the same thing as ShapeConfig.contact_margin used to be. You want to set ShapeConfig.gap or ModelBuilder.rigid_gap (fallback) instead.

[camevor]

Please note that this change does not migrate the former contact_margin correctly to gap, because the semantics of these terms has been changed in Newton. Please take a look at the Shape Configuration Documentation.

[camevor]

@camevor do you know why the newton tests fails? It looks like the force reported are larger than expected when the contact margin are large?

This is likely a test stability issue: The contact sensor is inherently quite noisy, so we should add some averaging for these quantitative tests. I'll prepare a PR with averaging to make the tests less flaky. It's also worth considering adding this to the sensor itself (e.g. averaging over solver substeps).

[hujc7]

Thanks, @camevor ! Running a test with updated field name. It did previous improve training so I will also investigate the impact of setting margin. Will check back

[hujc7]

gap alone does not fix rough env but should still remain the right fix for the intended field.