Fix Newton mesh approximation import controls

[ooctipus]

Summary

• add the missing nested mesh-collision property field to collision configs so collision props can dispatch authored mesh approximation settings
• update Newton replication to honor per-collider physics:approximation tokens instead of approximating every imported mesh as a convex hull
• add a Newton schema regression test for nested mesh-collision config dispatch

Tests

• git diff --cached --check before commit
• syntax parsed modified Python files with ast.parse
• Not run: ./isaaclab.sh -p -m pytest source/isaaclab_newton/test/sim/test_newton_schemas.py -k test_newton_mesh_collision_nested_cfg_written -q cannot collect locally because Python is missing lazy_loader

[greptile-apps]

Greptile Summary

This PR fixes two related gaps in Newton collision handling: it adds a mesh_collision_property nested field to CollisionBaseCfg so collision configs can dispatch authored mesh-approximation settings, and it updates Newton's replication path to honor per-collider physics:approximation tokens instead of blindly approximating every mesh as a convex hull.

• CollisionBaseCfg.mesh_collision_property: adds MeshCollisionBaseCfg | None = None field; modify_collision_properties dispatches it to modify_mesh_collision_properties and pops it from the dict before the generic attribute writer runs, preventing a double-write.
• Newton replication behavior change: p.approximate_meshes("convex_hull", ...) post-processing is removed; instead, skip_mesh_approximation=not simplify_meshes is passed directly to add_usd, so each shape's authored physics:approximation token is used. Meshes without an authored token will no longer receive convex-hull approximation by default.
• New regression test: test_newton_mesh_collision_nested_cfg_written verifies that a NewtonCollisionPropertiesCfg carrying a nested NewtonMeshCollisionPropertiesCfg correctly writes physics:approximation, newton:maxHullVertices, and applies NewtonMeshCollisionAPI.

Confidence Score: 3/5

The cfg-dispatch side of the change is well-structured and tested, but the Newton replication change silently alters the default collision behavior for any asset that does not carry explicit physics:approximation tokens.

The simplify_meshes=True default previously guaranteed convex-hull approximation for every mesh collider via approximate_meshes("convex_hull"). After this PR the same default delegates entirely to per-shape USD token authoring. Any existing asset that relied on the blanket approximation without explicit tokens will now receive the Newton importer's raw-mesh default, which can produce visibly different or unstable collision behavior with no warning to the caller. The cfg dispatch and test additions are correct, but the replication change carries a meaningful backward-compatibility risk that isn't captured in the changelog.

source/isaaclab_newton/isaaclab_newton/cloner/newton_replicate.py deserves careful review, specifically whether existing callers that pass simplify_meshes=True and have assets without authored physics:approximation will still get functionally equivalent collision shapes.

Important Files Changed

Filename	Overview
source/isaaclab_newton/isaaclab_newton/cloner/newton_replicate.py	Replaces blanket convex-hull post-processing with per-shape token dispatch; the semantics of simplify_meshes=True silently change for assets without authored physics:approximation tokens
source/isaaclab/isaaclab/sim/schemas/schemas_cfg.py	Adds `mesh_collision_property: MeshCollisionBaseCfg
source/isaaclab_newton/test/sim/test_newton_schemas.py	Adds test_newton_mesh_collision_nested_cfg_written that exercises the new nested-cfg dispatch path through define_collision_properties; test structure is consistent with existing suite
source/isaaclab/changelog.d/fix-nested-mesh-collision-cfg.rst	Changelog entry for the CollisionBaseCfg.mesh_collision_property addition
source/isaaclab_newton/changelog.d/fix-newton-mesh-approximation.rst	Changelog entry for the Newton replication mesh-approximation behavior change

Sequence Diagram

sequenceDiagram
    participant Caller
    participant define_collision_properties
    participant modify_collision_properties
    participant modify_mesh_collision_properties
    participant _apply_namespaced_schemas

    Caller->>define_collision_properties: "cfg=NewtonCollisionPropertiesCfg(mesh_collision_property=...)"
    define_collision_properties->>modify_collision_properties: prim_path, cfg
    modify_collision_properties->>modify_collision_properties: getattr(cfg, "mesh_collision_property")
    alt mesh_collision_property is not None
        modify_collision_properties->>modify_mesh_collision_properties: prim_path, mesh_collision_cfg
        modify_mesh_collision_properties-->>modify_collision_properties: apply MeshCollisionAPI + write physics:approximation, newton:maxHullVertices
    end
    modify_collision_properties->>modify_collision_properties: cfg_dict.pop("mesh_collision_property")
    modify_collision_properties->>_apply_namespaced_schemas: prim, cfg, cfg_dict (without mesh field)
    _apply_namespaced_schemas-->>modify_collision_properties: write remaining collision attrs

    Note over Caller,_apply_namespaced_schemas: Newton replication path
    Caller->>newton_replicate: "simplify_meshes=True (default)"
    newton_replicate->>add_usd: "skip_mesh_approximation=False (honors per-shape tokens)"
    Note right of add_usd: Old path called approximate_meshes(convex_hull) for all meshes after add_usd

Loading

_{Reviews (1): Last reviewed commit: "Fix Newton mesh approximation import con..." | Re-trigger Greptile}

[greptile-apps]

Breaking semantic change for callers relying on uniform convex-hull default

Before this PR, simplify_meshes=True (the default) unconditionally called p.approximate_meshes("convex_hull", keep_visual_shapes=True) after add_usd, so every mesh collider received convex-hull approximation regardless of authoring. Now simplify_meshes=True maps to skip_mesh_approximation=False, which honors the per-shape physics:approximation token. A mesh with no authored token will no longer receive any approximation — the Newton importer will use its internal default (raw triangle mesh). Existing workflows that never authored explicit physics:approximation tokens on their assets, and relied on simplify_meshes=True producing valid convex-hull colliders, will silently regress in simulation fidelity. This should be documented as a migration note in the changelog and ideally guarded with a deprecation period or a separate boolean parameter.

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!

[isaaclab-review-bot]

Code Review: Fix Newton mesh approximation import controls

Thanks for this PR — the shift from blanket convex-hull approximation to per-collider token dispatch is a welcome improvement for users with heterogeneous collision mesh needs.

Summary

1. Core IsaacLab (schemas_cfg.py): Adds mesh_collision_property: MeshCollisionBaseCfg | None to CollisionBaseCfg, enabling nested mesh-collision configuration to be dispatched through the existing define_collision_properties path (which already handles this field at schemas.py:549-556).
2. Newton replicate (newton_replicate.py): Replaces the two-step approach (skip_mesh_approximation=True + post-hoc approximate_meshes("convex_hull")) with a single skip_mesh_approximation=not simplify_meshes, delegating per-shape approximation to the USD importer.
3. Regression test: Validates the nested cfg dispatch writes expected prim attributes.

Strengths

• Clean integration following IsaacLab's schema composition pattern
• None default preserves existing behavior for users not setting the field
• Forward reference to MeshCollisionBaseCfg is fine thanks to from __future__ import annotations
• Existing dispatch wiring in schemas.py already consumes mesh_collision_property — no additional plumbing needed
• Test assertions are specific and verify both the attribute value and the applied schema

Findings

1. Semantic shift in simplify_meshes — potential breaking change (Medium)

File: source/isaaclab_newton/isaaclab_newton/cloner/newton_replicate.py

The behavioral change is:

• Before: simplify_meshes=True → every mesh gets convex-hull approximation (regardless of authored USD tokens)
• After: simplify_meshes=True → the USD importer reads each collider's physics:approximation token and applies whatever approximation is authored

For meshes that have no explicit physics:approximation token authored, the old code would still convex-hull them, but the new code may leave them un-approximated (depending on the Newton USD importer's default behavior for missing tokens). This could silently degrade simulation performance or correctness for existing scenes that relied on the blanket convex-hull behavior.

Suggestion: Consider documenting this behavioral change more explicitly (e.g., a migration note) or adding a fallback that applies convex hull when no physics:approximation token is authored and simplify_meshes=True.

2. No integration test for the replicate path change (Low)

File: source/isaaclab_newton/test/sim/test_newton_schemas.py

The new test (test_newton_mesh_collision_nested_cfg_written) validates the schema dispatch path — it confirms define_collision_properties with a nested mesh_collision_property writes the correct attributes. However, the behavioral change in _build_newton_builder_from_mapping (the actual fix for Newton replication) has no corresponding test.

The PR body notes the test could not be collected locally due to missing lazy_loader. If CI eventually runs the newton test suite, existing tests may cover this path implicitly. Worth confirming once CI passes.

3. CI blocked — newton tests haven't run (Info)

The Build Base Docker Image job failed, which gates all downstream test jobs including isaaclab_newton. The pre-commit and lint checks pass, but the functional validation hasn't executed yet. Would recommend re-running CI or confirming the Docker failure is unrelated before merge.

4. Docstring accuracy for simplify_meshes=False case (Nit)

File: source/isaaclab_newton/isaaclab_newton/cloner/newton_replicate.py, lines 40-42

The updated docstring says "Whether to honor per-shape physics:approximation tokens authored on colliders during import." When simplify_meshes=False, the importer skips all approximation (skip_mesh_approximation=True). This means authored tokens are ignored, not just "not honored." A minor wording tweak like "When False, all mesh approximation is skipped regardless of authored tokens" would make the False-path behavior clearer.

---

Overall this is a well-motivated fix with clean implementation. The main concern is the semantic shift for existing callers who expected blanket convex-hull behavior. Once CI passes and the behavioral implications are confirmed acceptable, this looks good to merge.