Skip to content

Adds cable (1D rod) assets via Newton#5641

Open
mmichelis wants to merge 26 commits into
isaac-sim:developfrom
mmichelis:mym/cable
Open

Adds cable (1D rod) assets via Newton#5641
mmichelis wants to merge 26 commits into
isaac-sim:developfrom
mmichelis:mym/cable

Conversation

@mmichelis
Copy link
Copy Markdown
Collaborator

@mmichelis mmichelis commented May 15, 2026

Description

Adds end-to-end support for cable / rod assets driven by Newton, layered on top of the existing deformable contrib pattern.

  • isaaclab.sim.spawners.shapes: new CableCfg / spawn_cable that author cables as UsdGeomBasisCurves prims. Physics is materialized by the Newton replicate hook, not at spawn time.
  • isaaclab_newton: new NewtonCableMaterialCfg (stretch / bend stiffness, damping, density) and a per-cable ignore-paths block in newton_physics_replicate so add_usd skips cable BasisCurves.
  • isaaclab_contrib.cable:
    • new CableObject (subclass of the Newton Articulation) + CableObjectCfg, plus the replicate-hook plumbing (CableRegistryEntry, add_cable_entry_to_builder, add_registered_cables_to_builder, install_cable_builder_hooks) mirroring the deformable contrib layout.
    • CableObject.reset(env_ids=…) — new public method that snaps the cable's per-env body slice back to its spawn pose. Restores state.body_q and solver.body_q_prev from model.body_q, and zeros state.body_qd and solver.body_inertia_q. Resetting body_q alone leaves stale solver.body_q_prev, which feeds AVBD's implicit velocity (body_q − body_q_prev)/dt and produced ~700 m/s spurious velocities on the next step.
    • USD-loadable cablesspawn_cable now authors an int2[] connections attribute on the UsdGeomBasisCurves prim, and CableObject._register_cable reads it to recover the edge topology. Cables can now be loaded from arbitrary curve USDs (e.g. authored in Blender) via UsdFileCfg, as long as the curve carries this attribute.
  • VBD manager:
    • Overrides forward() to mask cable articulations out of eval_fk, which has no JointType.CABLE case and was collapsing rod segments onto their parent anchors on every Kit pre-render FK pass (cable explosion fix). Open Bug in Newton 2887.
  • Kit / Fabric sync: cable UsdGeomBasisCurves points are refreshed from Newton cable body transforms at render cadence (CPU only for now).
  • Demo + tests: scripts/demos/cables.py, test_spawn_cable.py, test_cable_object.py.

Builds on top of mym/deformable_experimental (#5443 ) because cables build on the in-progress Newton deformable contrib stack.

Test plan

  • ./isaaclab.sh -p -m pytest source/isaaclab/test/sim/test_spawn_cable.py
  • ./isaaclab.sh -p -m pytest source/isaaclab_contrib/test/cable/test_cable_object.py
  • ./isaaclab.sh -p scripts/demos/cables.py — visual sanity check in Kit
  • ./isaaclab.sh -p scripts/demos/deformables.py — confirm no regression in mixed scenes
  • ./isaaclab.sh -f

Type of change

  • New feature (non-breaking change which adds functionality)

Checklist

  • I have read and understood the contribution guidelines
  • I have run the pre-commit checks with ./isaaclab.sh --format
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the changelog and the corresponding version in the extension's config/extension.toml file
  • I have added my name to the CONTRIBUTORS.md or my name already exists there

@github-actions github-actions Bot added documentation Improvements or additions to documentation isaac-lab Related to Isaac Lab team labels May 15, 2026
Copy link
Copy Markdown

@isaaclab-review-bot isaaclab-review-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

This PR adds experimental cable (1D rod) asset support via Newton physics backend, introducing a significant new asset type alongside the existing deformable body system. This is a substantial contribution of 141 files with comprehensive documentation, demos, and testing.

Architecture Assessment

Strengths:

  • Well-structured separation of concerns: CableObject inherits from Articulation (since Newton implements rods as articulations), while deformables remain separate
  • Clean registry pattern (CableRegistryEntry) mirrors the existing deformable registry approach
  • Proper hook installation via install_cable_builder_hooks() integrates cleanly with Newton's replicate pipeline
  • Deprecation warnings for renamed methods (write_nodal_state_to_simwrite_nodal_state_to_sim_index) maintain backward compatibility

Documentation quality is excellent:

  • New VBD solver documentation with comprehensive parameter tables
  • Newton manager abstraction guide for future solver implementations
  • Updated migration guides for deformable backend split

Findings

1. Cable Material Validation Gap (Medium)

File: source/isaaclab/isaaclab/sim/spawners/shapes/shapes.py (lines 263-275)

The cable spawner validates that physics_material is a NewtonCableMaterialCfg, but the error message could be more actionable:

if not isinstance(cfg.physics_material, NewtonCableMaterialCfg):
    raise ValueError(
        "CableCfg requires `physics_material` to be a NewtonCableMaterialCfg instance,"
        f" got {type(cfg.physics_material).__name__}."
    )

Suggestion: Consider adding guidance about the Newton-only constraint in the error message, e.g., "Cables are currently only supported with the Newton physics backend."

2. Tetrahedral Mesh Winding Fix Using CPU Device (Low)

File: source/isaaclab/isaaclab/sim/schemas/schemas.py (lines 1105-1120)

The _fix_tet_winding_kernel is always launched on CPU:

device = "cpu"
_tet_points_wp = wp.array(tet_mesh_points.astype(np.float32), dtype=wp.vec3, device=device)

This is likely intentional since tetrahedralization happens at spawn time (before GPU simulation), but documenting this choice would help future maintainers understand it's not a performance oversight.

3. Test Coverage for Cable Physics (Low)

Files: source/isaaclab_contrib/test/cable/test_cable_object.py

The test file includes good unit tests for hook idempotency and registry behavior. Consider adding an integration test that validates cable physics behavior (e.g., cable settling under gravity) to catch regressions in Newton's add_rod_graph integration.

4. MeshSquareCfg → MeshRectangleCfg Rename (Breaking Change - Documented)

File: source/isaaclab/isaaclab/sim/spawners/meshes/meshes_cfg.py

The rename from MeshSquareCfg to MeshRectangleCfg with size changing from float to tuple[float, float] is a breaking change. The changelog documents this well, and the updated size semantics (X and Y axis lengths) provide more flexibility.

Note: Existing code using MeshSquareCfg(size=1.5) must migrate to MeshRectangleCfg(size=(1.5, 1.5)).

5. Deformable Object Cfg Location (API Design)

File: source/isaaclab/isaaclab/assets/deformable_object/deformable_object_cfg.py

Good decision to move DeformableObjectCfg to the backend-neutral isaaclab.assets package while keeping backend-specific implementations (PhysxDeformableObject, NewtonDeformableObject) in their respective packages. This maintains clean import paths for users.

Overall Assessment

This PR represents a well-designed extension of Isaac Lab's physics capabilities. The cable asset implementation follows established patterns (deformable registry, builder hooks), the documentation is thorough, and the test coverage is reasonable. The breaking changes are clearly documented in changelogs.

Recommendation: This PR is ready for detailed code review by maintainers. The architectural decisions are sound, and the implementation quality is high.


Update (456e5f5): Reviewed incremental changes from merge with develop and new cable-specific fixes.

New Changes Reviewed

1. Camera/Renderer Migration to Warp (Major Infrastructure)

Camera data and renderer buffers have migrated to warp arrays (ProxyArray). This impacts test files across the codebase (test_camera.py, test_tiled_camera.py, etc.) where assertions now check wp.uint8 / wp.float32 instead of torch.uint8 / torch.float.

The ProxyArray deprecation bridge in proxy_array.py now forwards unknown attribute access to the torch view with a deprecation warning, maintaining backward compatibility.

2. configclass Import Refactoring (Cleanup)

All modules now import configclass directly from isaaclab.utils.configclass rather than isaaclab.utils. This fixes lazy import issues and provides a cleaner import pattern across 80+ files.

3. Cable Kit Visualization (Key Fix)

File: source/isaaclab_contrib/isaaclab_contrib/deformable/vbd_manager.py

Added Fabric sync for cable curves via _sync_cable_curve_points kernel. The cable body transforms from Newton are now properly written back to UsdGeomBasisCurves.points for viewport visualization. The sync runs on CPU because Kit/Hydra reads CPU Fabric for runtime-spawned curves.

4. Newton eval_fk Cable Joint Workaround (Important Fix)

File: source/isaaclab_contrib/isaaclab_contrib/deformable/vbd_manager.py (lines 394-420)

Added _non_cable_articulation_mask to skip cable articulations during FK evaluation. This prevents cable segments from collapsing onto their parent anchors when Kit triggers a pre-render FK pass, since Newton's eval_fk has no case for JointType.CABLE.

Note: Comment indicates this can be removed once Newton patches cable joints in eval_fk.

5. Zero-Particle Scene Fix (Bug Fix)

File: source/isaaclab_contrib/isaaclab_contrib/deformable/vbd_manager.py (line 499)

Fixed AttributeError in cable-only scenes by guarding the rebuild_bvh call with getattr(cls._solver, "particle_enable_self_contact", False). Newton's SolverVBD skips _init_particle_system for zero-particle scenes, leaving the attribute unset.

6. URDF/MJCF Importer Updates (Breaking Change)

Significant updates to urdf_converter.py and mjcf_converter.py for Isaac Sim 4.5/5.0 importer compatibility. The merge_fixed_joints utility moved from isaaclab to isaacsim.asset.importer.urdf.impl.urdf_utils. Self-collision now writes to newton:selfCollisionEnabled on articulation roots.

7. Frame Stacking Support (New Feature)

Added stacked_image observation term with ring-buffer channel stacking for temporal information. Tests cover warmup, channel ordering, partial env reset, and long runs.

Overall Update Assessment

The incremental changes are well-integrated. The cable visualization fix is critical for usability, and the eval_fk workaround is properly documented as temporary pending upstream Newton support. No new architectural concerns identified.


Update (2f6591d): Reviewed documentation additions and safety hardening for cable articulation handling.

New Changes Reviewed

1. Cable Documentation (Major Addition)

File: docs/source/experimental-features/newton-physics-integration/using-cables.rst

Comprehensive 240-line documentation covering:

  • Quick start demo (scripts/demos/cables.py)
  • Cable authoring with CableCfg and CableObjectCfg
  • Solver selection (VBD required for cable joints)
  • Material parameter reference table (stretch/bend stiffness/damping, density)
  • Spawner parameter documentation
  • Kit/Fabric visualization details
  • Limitations section (Newton-only, no actuators, eval_fk workaround)

Quality assessment: Excellent user-facing documentation. The material parameter table with units and descriptions is particularly helpful.

2. Validation Moved Downstream (Design Improvement)

File: source/isaaclab/isaaclab/sim/spawners/shapes/shapes.py

The NewtonCableMaterialCfg validation has been moved from spawn_cable to CableObject._register_cable. This:

  • Removes the hard dependency on isaaclab_newton in the core isaaclab package
  • Allows cable USD to be authored on non-Newton backends (renders as inert geometry)
  • Matches the documentation note that cables "load as inert geometry under a PhysX backend"

The corresponding test test_spawn_cable_validation_wrong_material was correctly removed.

3. eval_fk Safety Hardening (Critical Fix)

File: source/isaaclab_contrib/isaaclab_contrib/deformable/vbd_manager.py

_build_non_cable_articulation_mask now raises RuntimeError in edge cases:

  • Cables registered but model state incomplete (missing joint_type/joint_articulation)
  • Cables registered but finalized model has zero articulations
  • Cables registered but no JointType.CABLE joints found (hook didn't run)

forward() also now raises if cables are registered but the mask is None, preventing silent corruption.

Assessment: These defensive checks are important — the previous silent fallthrough to unmasked eval_fk would corrupt cable body_q without any visible error.

4. Regression Test Added (Testing)

File: source/isaaclab_contrib/test/cable/test_cable_object.py

New test_forward_preserves_cable_body_q test validates:

  • Mask is built when cables are registered
  • forward() does not alter body_q (verifies cable articulations are excluded from FK)

This directly tests the eval_fk workaround and will catch regressions if the mask logic breaks.

5. Manager Registry Cleanup (Bug Fix)

Files: coupled_featherstone_vbd_manager.py, coupled_mjwarp_vbd_manager.py, vbd_manager.py

Added _solver_specific_clear() method that properly resets:

  • _cable_registry
  • _deformable_registry
  • _per_world_builder_hooks

This ensures registries are cleared when the manager is reset, preventing stale entries from previous scenes.

6. Import Cleanup (Minor)

Fixed configclass imports in cable_object_cfg.py and franka_soft_env_cfg.py to use the direct isaaclab.utils.configclass path.

Overall Update Assessment

This update addresses the earlier review finding about validation messaging by moving validation downstream with proper documentation of the Newton-only constraint. The safety hardening for the eval_fk workaround is well-designed — fail-fast with clear error messages is the right approach for a temporary workaround. The new regression test directly validates the critical cable body_q preservation.

Status: All previous findings have been addressed or documented. PR continues to be well-designed and thoroughly tested.


Update (46850b5): Previous concerns addressed. No new issues introduced.

Changes Since Last Review

This push contains maintenance and cleanup changes unrelated to the core cable functionality:

  1. Test file cleanups - Removed unnecessary AppLauncher/simulation_app imports from ~15 test files that don't require simulation context (test_terrain_generator.py, test_mock_sensors.py, various utils tests)

  2. Kernel test refactoring - Renamed fused kernels in test_ray_caster_kernels.py to reflect combined operations (e.g., compute_distance_to_image_plane_to_image_masked_kernel)

  3. Deformable properties test fix - Updated test_newton_deformable_cfgs_use_core_schema_and_material_functions to assert _usd_namespace == "newton" instead of checking attribute absence

  4. OVRTX renderer optimization - Avoided disk I/O by exporting USD to memory (ExportToString) instead of temporary files

  5. RSL-RL version bump - Updated from 3.1.2 to 5.0.1 in python_packages.toml

  6. CI tooling - Docker build improvements for conda support

Assessment: All changes are infrastructure/test maintenance. No cable-specific logic was modified. The core cable implementation reviewed previously remains unchanged.


Update (4312a72): Reviewed USD-loadable cables and self-collision prevention.

New Changes Reviewed

1. USD-Loadable Cables via connections Attribute (Key Feature)

File: source/isaaclab/isaaclab/sim/spawners/shapes/shapes.py

Cables now author edge topology as a custom int2[] attribute named connections on the BasisCurves prim. This enables two use cases:

  1. Programmatic cables: spawn_cable writes [(0,1), (1,2), ...] automatically
  2. USD-loaded cables: Users can load pre-authored cable USDs via UsdFileCfg as long as the curve prim includes this attribute

The comment correctly notes this is a workaround until UsdGeomBasisCurves natively supports curve topology — the Newton replicate hook reads connections to build the rod graph.

File: source/isaaclab_contrib/isaaclab_contrib/cable/cable_object.py

_register_cable now:

  • Discovers BasisCurves via descendant traversal (works for both spawn_cable and arbitrary USD structures)
  • Reads edge topology from connections instead of assuming linear order
  • Raises helpful ValueError if connections is missing

This is a good API decision — it decouples cable authoring from cable loading.

2. Cable Self-Collision Prevention (Critical Fix)

File: source/isaaclab_contrib/isaaclab_contrib/cable/cable_object.py

add_cable_entry_to_builder now assigns a unique negative collision_group to each cable:

shape_cfg.collision_group = -(1 + cable_idx)

Newton's collision filtering rule: same negative group = filtered, negative-vs-positive = collides. This means:

  • Cable segments don't collide with themselves (self-collision filtered)
  • Cables still collide with ground, rigid bodies, and other cables

This fixes cables "exploding" or tangling with themselves on first contact.

3. start_simulation eval_fk Corruption Fix (Critical Fix)

File: source/isaaclab_contrib/isaaclab_contrib/deformable/vbd_manager.py

The start_simulation method now rebuilds state_0/state_1 from model.state() after the base finalize step, then re-runs the masked forward(). This addresses a subtle bug:

  • NewtonManager.start_simulation ends with an unmasked eval_fk
  • For straight cables this is harmless (identity output matches add_rod_graph)
  • For curved cables, eval_fk collapses segments onto the root's +Z axis

The fix restores body_q from model.body_q (untouched by eval_fk) for cable bodies.

Test Coverage: test_start_simulation_preserves_curved_cable_body_q validates this fix with a deliberately curved 3-node cable where adjacent edges point in different directions.

4. AVBD Beta Parameter (New Feature)

File: source/isaaclab_contrib/isaaclab_contrib/deformable/newton_manager_cfg.py

Added rigid_avbd_beta parameter to VBDSolverCfg:

  • Controls per-iteration penalty-stiffness ramping for AVBD (Adaptive VBD)
  • Each iteration grows constraint penalty k by beta * |C|
  • Improves Hessian conditioning with small iteration budgets
  • Set to 0 to disable ramping (pin k at ceiling)

The demo uses rigid_avbd_beta=1e2 which suggests this was tuned for the cable pile scenario.

5. Demo Updates (Minor)

File: scripts/demos/cables.py

  • Added examples loading cables from USD files (demonstrates new UsdFileCfg pathway)
  • Tuned parameters (smaller cables, higher spawn point, lower density)
  • Integrated NewtonVisualizerCfg for camera setup
  • Increased rigid_body_contact_buffer_size to 1024 for larger pile simulations

Note: The hardcoded paths (/home/mmichelis/Documents/...) should be replaced with relative paths or asset references before merge.

Quality Assessment

This update represents significant functional improvements:

  1. USD-loadable cables — enables authoring cables in DCC tools and importing them
  2. Self-collision prevention — fixes a critical usability issue
  3. Curved cable support — fixes silent corruption for non-linear cable layouts
  4. AVBD tuning — provides solver control for stability/performance tradeoff

All changes include appropriate test coverage. The only concern is the hardcoded absolute paths in the demo, which should be parameterized or use asset references.

Status: Ready for maintainer review. Core functionality is complete and well-tested.


Update (786e28a): Reviewed eval_fk initialization refactor — cleaner approach, minor style issue.

New Changes Reviewed (b317f29, 786e28a)

1. eval_fk Initialization Moved to Base Class (Architecture Improvement)

File: source/isaaclab_newton/isaaclab_newton/physics/newton_manager.py

The base NewtonManager.start_simulation() now calls cls.forward() instead of eval_fk() directly:

# Before:
eval_fk(cls._model, cls._state_0.joint_q, cls._state_0.joint_qd, cls._state_0, None)
# After:
cls.forward()

This allows subclasses like VBDManager to override forward() with masked FK logic, eliminating the need for post-hoc state reconstruction.

2. VBDManager start_simulation Cleanup (Simplification)

File: source/isaaclab_contrib/isaaclab_contrib/deformable/vbd_manager.py

Removed 13 lines of workaround code from start_simulation():

# REMOVED:
cls._build_non_cable_articulation_mask()
if cls._non_cable_articulation_mask is not None and cls._model is not None:
    cls._state_0 = cls._model.state()
    cls._state_1 = cls._model.state()
    cls.forward()

This workaround is no longer needed because the base class now delegates to forward(), which handles cable masking correctly.

3. Lazy Mask Building in forward() (Behavior Change)

File: source/isaaclab_contrib/isaaclab_contrib/deformable/vbd_manager.py

The forward() method now lazily builds the cable mask if cables are registered but the mask hasn't been built yet:

if cls._non_cable_articulation_mask is None:
    if cls._cable_registry:
        cls._build_non_cable_articulation_mask()
    else:
        super().forward()
        return

This replaces the previous defensive RuntimeError that required callers to ensure start_simulation() ran first.

Assessment: This is a reasonable change — lazy initialization is more forgiving of call order, though it does mean the mask-building cost shifts to the first forward() call rather than occurring deterministically in start_simulation(). For cables, this is fine since forward() is called during start_simulation() anyway via the base class change.

4. Minor Style Issue (Low)

File: source/isaaclab_contrib/isaaclab_contrib/deformable/vbd_manager.py (line 332)

There's a whitespace inconsistency at line 332:

       if cls._non_cable_articulation_mask is None:  # 7 spaces instead of 8

This appears to be an accidental dedent. The code will still work (Python allows inconsistent indentation within a file as long as blocks are consistent), but it should be fixed for consistency.

Overall Update Assessment

This is a clean architectural improvement. Moving the FK call into forward() at the base class level and letting subclasses override it is the right approach — it eliminates the "fix up after base class corruption" pattern from the previous implementation.

The lazy mask building is a minor tradeoff (determinism vs. flexibility) but acceptable for this use case.

Minor fix needed: Correct the indentation at line 332 of vbd_manager.py.

Status: LGTM with minor style fix.


Update (92a3b3d): Indentation fix applied — style issue resolved.

Change Reviewed

Commit: 92a3b3df — "Fix: typo"

The whitespace inconsistency at line 332 of vbd_manager.py (7 spaces instead of 8) has been corrected:

-       if cls._non_cable_articulation_mask is None:
+        if cls._non_cable_articulation_mask is None:

This addresses the minor style issue noted in the previous review update.

Status: All feedback addressed. LGTM. ✅

Copy link
Copy Markdown

@isaaclab-review-bot isaaclab-review-bot Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

This is a massive PR (147 files) that adds experimental 1D cable/rod asset support via Newton physics, along with significant deformable body infrastructure improvements. The PR introduces a new asset type and refactors the deformable system to support multiple physics backends (PhysX and Newton).

High-Level Architecture Assessment

Strengths:

  • Clean separation of concerns: CableObject inherits from Articulation since Newton implements rods as articulations
  • Registry pattern (CableRegistryEntry) mirrors the existing deformable registry approach
  • Backend-neutral DeformableObjectCfg in isaaclab.assets with backend-specific implementations
  • Comprehensive documentation with VBD solver guide, cable usage guide, and Newton manager abstraction docs

Key Design Decisions:

  • Cables are Newton-only — USD authored on any backend but physics only on Newton
  • Cables have no actuators — stiffness is material-like, not controllable
  • Self-collision prevention via negative collision_group per cable

High-Impact Findings

1. Pre-commit Check Failing (CI)

The pre-commit CI check is failing. This should be resolved before merge.

2. Cable connections Attribute Documentation (Medium)

File: source/isaaclab/isaaclab/sim/spawners/shapes/shapes.py (lines 290-295)

The custom int2[] connections attribute is critical for cable topology but its requirement for user-imported USD files needs clearer documentation. The error message in _register_cable is helpful, but the spawner docstring could emphasize this requirement earlier.

Suggestion: Add to the CableCfg docstring: "User-imported cable USDs must include a connections attribute (int2[]) listing edge pairs."

3. Hardcoded Demo Paths ✅ Addressed

File: scripts/demos/cables.py

The demo references hardcoded absolute paths like /home/mmichelis/Documents/....

Update (22f1a86): The demo code with hardcoded paths is now commented out.

4. eval_fk Workaround is Well-Documented (Informational)

File: source/isaaclab_contrib/isaaclab_contrib/deformable/vbd_manager.py

The _non_cable_articulation_mask workaround for Newton's missing JointType.CABLE case in eval_fk is properly documented with clear comments indicating it can be removed once Newton patches cable joints. Good defensive coding with RuntimeError if mask building fails.

5. Test Coverage is Good (Positive)

  • test_spawn_cable.py covers basic spawning, validation, and material authoring
  • test_cable_object.py covers hook idempotency and registry behavior
  • test_forward_preserves_cable_body_q validates the FK workaround

New in 22f1a86: CableObject.reset() Implementation

The new reset() method (43 lines) properly snaps cable bodies back to spawn pose:

  • Restores state.body_q and solver.body_q_prev from model.body_q
  • Zeroes state.body_qd and solver.body_inertia_q
  • Includes excellent docstring explaining the body_q_prev requirement for AVBD

Minor note: The per-call wp.zeros() allocations in the loop could be moved outside the loop for efficiency, but this is a minor optimization concern for a reset path.


Update (ee75425): Additional Changes Reviewed

New commits add:

  • NewtonCableMaterialCfg: New configclass for cable rod material parameters (stretch/bend stiffness, damping, density)
  • Graph coloring refactor: Moved from post-replicate hooks to solver-cfg-driven (requires_graph_coloring flag)
  • Newton replicate refactor: Cable registry now included in ignore-paths alongside deformable registry
  • start_simulation refactor: Uses forward() instead of direct eval_fk call
  • Substep state swap fix: Changed cls._state_0 to NewtonManager._state_0 for correct class reference
  • Additional regression tests: test_start_simulation_preserves_curved_cable_body_q, test_cable_object_reset_restores_body_state

The cable implementation continues to look solid. The test coverage for edge cases (curved cables, body_q corruption, reset behavior) is excellent.


Overall Assessment

This PR represents a well-designed extension of Isaac Lab's physics capabilities. The cable implementation follows established patterns, documentation is thorough, and test coverage is solid. The main blocker is the failing pre-commit check.

Recommendation: Fix the pre-commit failure, then this PR is ready for detailed maintainer review.

@isaaclab-review-bot
Copy link
Copy Markdown

🔄 Review Update (376ad2e)

Following up on my previous review after new commits were pushed.

Status of Previous Findings

Finding Status
Pre-commit CI ⏳ Pending (only labeler check shows completed)
connections attribute docs 📝 Still applicable (minor suggestion)
Hardcoded demo paths ✅ Fixed (commented out)
eval_fk workaround docs ✅ Well-documented
Test coverage ✅ Excellent — new comprehensive tests added

Incremental Changes (22f1a86376ad2e)

No new issues identified. The incremental changes primarily add unrelated features (OVPhysX contact sensor, Hydra preset improvements, MCAP teleop replay) plus changelog entries for the cable feature.

New cable tests are comprehensive:

  • test_forward_preserves_cable_body_q — FK patch regression test
  • test_start_simulation_preserves_curved_cable_body_q — Body restoration test
  • test_cable_object_reset_restores_body_state — Reset behavior test

Overall

The cable implementation remains solid. Once CI checks pass, this PR is ready for maintainer merge.

@AntoineRichard AntoineRichard changed the title Add cable (1D rod) assets via Newton Adds cable (1D rod) assets via Newton May 20, 2026
@kellyguo11 kellyguo11 moved this to In review in Isaac Lab May 20, 2026
Copy link
Copy Markdown
Collaborator

@AntoineRichard AntoineRichard left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Mike for the cool work! Couple changes are required, but I think it's a really good step in the right direction. @kellyguo11 if we need this now, this can be merged as is and all the comments could be addressed in a follow-up however some are quite important to get right before that.

:meth:`newton.ModelBuilder.add_rod_graph`. A cable is spawned as a
``UsdGeomBasisCurves`` prim, and the cable's physics (per-segment capsules,
inter-segment cable joints, stretch / bend stiffness, damping, density) is
materialized at Newton model-build time by a contrib replicate hook.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very technical phrasing that doesn't add much. It might be better to rephrase that into regular human lingo.

Comment on lines +16 to +23
.. note::
Cables are currently **only supported on the Newton physics backend**.
The spawner authors valid USD on any backend (so the scene loads in PhysX
or PhysX-Fabric viewports), but the resulting cable is not registered with
a PhysX articulation. :class:`~isaaclab.sim.spawners.shapes.CableCfg`
requires ``physics_material`` to be a
:class:`~isaaclab_newton.sim.spawners.materials.NewtonCableMaterialCfg`
and rejects ``rigid_props`` / ``mass_props`` up front.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should that be the case, I feel like we should raise an error immediately if a cable is defined in PhysX to not deceive the users.

Comment on lines +26 to +39
Quick Start: The Cable Demo
---------------------------

Before adding cables to a task, run the standalone demo to confirm that the
spawner, the cable replicate hook, the VBD solver, and the Kit / Fabric
viewport sync are all working in your environment:

.. code-block:: bash

./isaaclab.sh -p scripts/demos/cables.py
./isaaclab.sh -p scripts/demos/cables.py --num_cables 40

The demo spawns a pile of randomly oriented cables onto a ground plane under
the Newton VBD solver. Source: ``scripts/demos/cables.py``.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe add something around the lines of: "Since this feature is currently experimental, a good sanity check is to ...." Otherwise it sounds like we don't know what we are doing.

Comment on lines +45 to +49
A cable is configured as a :class:`~isaaclab.sim.spawners.shapes.CableCfg`
plus a Newton-specific physics material. The cfg's ``positions`` field is a
list of at least two control points in the cable's local frame; adjacent pairs
become individual rod segments, each materialized as a capsule body of
diameter ``width`` and joined to its neighbour by a Newton cable joint.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like this could be phrased better for people to understand.

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not add a table like done elsewhere? That might be more informative to give precise information to the users while allowing for a more technically relaxed description.

Comment on lines +51 to +68
.. code-block:: python

import isaaclab.sim as sim_utils
from isaaclab_newton.sim.spawners.materials import NewtonCableMaterialCfg

cable_spawn = sim_utils.CableCfg(
positions=[(i * 0.1, 0.0, 0.0) for i in range(10)],
width=0.03,
visual_material=sim_utils.PreviewSurfaceCfg(diffuse_color=(0.7, 0.2, 0.2)),
physics_material=NewtonCableMaterialCfg(
stretch_stiffness=1.0e6,
bend_stiffness=1.0e-4,
stretch_damping=1.0e-4,
bend_damping=1.0e-4,
density=1000.0,
),
collision_props=sim_utils.CollisionPropertiesCfg(),
)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this code block be documented? Create a cable with 20 elements all spaced by 10cm....

Comment on lines +8 to +11
from isaaclab.app import AppLauncher

# launch omniverse app
simulation_app = AppLauncher(headless=True).app
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is newton only, do we need to start kit?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really parsed or is it for show only? If it's not parsed yet, we should advertize this clearly?

NewtonManager._state_1 = cls._model.state()
NewtonManager._control = cls._model.control()
eval_fk(cls._model, cls._state_0.joint_q, cls._state_0.joint_qd, cls._state_0, None)
cls.forward()
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Who is defining that now? I don't see an explicit forward method, was it already defined? If so was it the only thing it did? If not should we be concerned about perf implications?

Comment on lines 1000 to 1019
# Setup USD/Fabric sync for Kit viewport rendering
if not cls._clone_physics_only:
import usdrt

body_paths = getattr(cls._model, "body_label", None) or getattr(cls._model, "body_key", None)
if not body_paths:
logger.warning(
"NewtonManager: model has no rigid bodies (body_label/body_key is empty). "
"USD/Fabric body sync for RTX is skipped. "
"Particle-only scenes (e.g. cloth) must register their own USD mesh update."
)
NewtonManager._usdrt_stage = None
else:
NewtonManager._usdrt_stage = get_current_stage(fabric=True)
for i, prim_path in enumerate(body_paths):
prim = cls._usdrt_stage.GetPrimAtPath(prim_path)
# Cable segment bodies have no per-body USD prim.
if not prim.IsValid():
continue
prim.CreateAttribute(cls._newton_index_attr, usdrt.Sdf.ValueTypeNames.UInt, True)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dear lord, we're parsing the stage here now :'(. @ooctipus we really need to do the cloner thing. We have artifacts everywhere :/.

Comment on lines +210 to +225
if not cls._clone_physics_only and cls._cable_registry:
import re

import usdrt

if NewtonManager._usdrt_stage is None:
NewtonManager._usdrt_stage = get_current_stage(fabric=True)

stage = get_current_stage()
curves_registered = False
for entry in cls._cable_registry:
curve_template_path = entry.curve_prim_path or f"{entry.prim_path}/geometry/mesh"
for inst_idx, body_offset in enumerate(entry.body_offsets):
resolved = re.sub(r"(?<=[Ee]nv_)\.\*", str(inst_idx), curve_template_path)
resolved = re.sub(r"\.\*", str(inst_idx), resolved)
curve_prim = stage.GetPrimAtPath(resolved)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering how much this kind of methods could be externalized to better isolate the code. The risk here is that this code might need to be duplicated to XPBD, or any other solver, and then that as we add more things (cloth etc...) it just ends up bloating the code base making the sinulation start completely unreadable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation infrastructure isaac-lab Related to Isaac Lab team

Projects

Status: In review

Development

Successfully merging this pull request may close these issues.

3 participants