Add Newton VBD Support with MJWarp+VBD Coupling by extending Newton Manager Abstraction in isaaclab_contrib

[mmichelis]

Description

This PR adds experimental Newton deformable support and refactors Newton solver selection so each solver is handled by a dedicated manager subclass.

Main changes:

• Added core deformable object infrastructure in isaaclab, including base deformable asset/data interfaces, deformable schemas, material configs, and mesh spawner support.
• Moved Newton deformable implementation into isaaclab_contrib.deformable, including:
  • VBDSolverCfg
  • CoupledMJWarpVBDSolverCfg
  • Newton deformable object/data wrappers
  • VBD and coupled MJWarp + VBD managers
  • deformable kernels and builder registration support
• Refactored NewtonManager into a solver-agnostic base with concrete solver managers for MJWarp, XPBD, Featherstone, and Kamino (done in Refactor NewtonManager into solver-specific subclasses #5439)
• Updated NewtonCfg so solver_cfg.class_type selects the active Newton manager through the existing SimulationContext dispatch path.
• Added Newton deformable replication support, deformable ignore paths, Fabric particle sync, and model-level deformable contact configuration.
• Added experimental deformable tasks:
  • Isaac-Pick-Cloth-Direct-v0
  • Isaac-Pick-VBD-Cube-Direct-v0
  • Isaac-Franka-Duck-v0
• Added regression coverage for Newton manager dispatch, deformable object behavior, and rigid-deformable coupling.

Continues on #5439 adding VBD solver and coupled MJWarp + VBD solver as newton managers in isaaclab_contrib.

Testing

Added test coverage for:

Fixes #5285

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

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

Summary

This PR adds experimental Newton VBD (Vertex Block Descent) deformable body support and refactors the Newton solver architecture into a manager-per-solver pattern. The core changes introduce a BaseDeformableObject abstraction in isaaclab, move PhysX-specific deformable schemas to the core package, and add a Newton deformable implementation in isaaclab_contrib. While architecturally sound, there are several correctness issues in the deformable state management and potential race conditions in the coupled solver implementation.

Architecture Impact

High impact, cross-cutting changes:

1. DeformableObject is now a factory class (isaaclab/assets/deformable_object/deformable_object.py) that dispatches to backend-specific implementations
2. Deformable schemas (DeformableBodyPropertiesCfg, DeformableBodyMaterialCfg) moved from isaaclab_physx to isaaclab core - breaking change for existing PhysX deformable users importing from old location
3. NewtonManager refactored into solver-specific subclasses (VBDManager, CoupledMJWarpVBDManager, etc.) selected via solver_cfg.class_type
4. New _deformable_registry mechanism on NewtonManager for deformable body registration before model finalization

Implementation Verdict

Significant concerns - Several correctness issues need resolution before merge.

Test Coverage

The PR includes good test coverage in test_deformable_object.py and test_rigid_deformable_coupling.py. However:

• Missing tests for the coupled solver's two-way reaction force computation
• No regression test for the PhysX deformable path after schema relocation
• The @flaky decorator on test_set_nodal_state_with_applied_transform suggests test instability

CI Status

No CI checks available yet.

Findings

🔴 Critical: source/isaaclab_contrib/isaaclab_contrib/deformable/deformable_object.py:246-250 — Stale state after position write

When writing nodal positions, the code scatters to both state_0 and state_1, but then only invalidates cache timestamps. The _data object still holds references to the old particle arrays until update() is called. If user code reads data.nodal_pos_w immediately after write_nodal_pos_to_sim_index() without calling update(), they get stale data because the ProxyArray fetcher reads from SimulationManager._state_0.particle_q which IS updated, but the timestamp check may pass incorrectly.

# Line 249-251: Invalidating timestamp to -1.0 should work, but...
self._data._nodal_pos_w.timestamp = -1.0
# The ProxyArray.timestamp check compares against _sim_timestamp which is only 
# updated in update(dt), so a read before update() may see stale values

🔴 Critical: source/isaaclab_contrib/isaaclab_contrib/deformable/coupled_mjwarp_vbd_manager.py:378-384 — Race condition in two-way coupling

The _step_two_way method clears forces on both states, then runs collision, then injects reactions into body_f. However, between steps 3 and 4, body_f accumulates reaction forces that get consumed by _rigid_step. If the VBD solver's step() in step 6 also reads/writes body_f (which it does internally), there's a data race:

# Line 378: state_in.body_f is modified by _apply_reactions
if state_in.body_f is not None:
    cls._apply_reactions(state_in, state_out, dt)
# Line 381: _rigid_step reads body_f
cls._rigid_step(state_in, state_out, control, dt)
# Line 384: particle_f.zero_() but body_f may still have residual
state_in.particle_f.zero_()
# Line 387: VBD step - if integrate_with_external_rigid_solver=True, 
# it reads body_q/body_qd which were just modified

🔴 Critical: source/isaaclab/isaaclab/physics/physics_manager.py:116-127 — Callback ID collision after clear

The fix at lines 116 and 127 correctly uses PhysicsManager._callback_id instead of cls._callback_id, but clear_callbacks() at line 163 resets to 0. If callbacks are registered, cleared, then re-registered in a subclass, IDs will collide:

# Line 163
PhysicsManager._callback_id = 0  # Reset
# Later, if subclass registers new callback, it gets ID 0 again
# This breaks deregister() if any old handles are still held

🟡 Warning: source/isaaclab_contrib/isaaclab_contrib/deformable/kernels.py:190-221 — Unbounded contact iteration

The _kernel_body_particle_reaction kernel uses a fixed _MAX_REACTION_CONTACTS = 2048 upper bound. If actual contacts exceed this, they're silently ignored:

# Line 196
if tid >= contact_count[0]:
    return  # Early exit - contacts beyond 2048 are dropped

This should either be dynamically sized or emit a warning when truncated.

🟡 Warning: source/isaaclab/isaaclab/sim/schemas/schemas.py:1104-1115 — Tet winding fix runs on CPU

The _fix_tet_winding_kernel is launched with device="cpu" even when the simulation runs on CUDA. This forces a device sync and data transfer:

# Lines 1104-1115
device = "cpu"  # Always CPU
_tet_points_wp = wp.array(..., device=device)
_tet_indices_wp = wp.array(..., device=device)
wp.launch(_fix_tet_winding_kernel, ..., device=device)
tet_mesh_indices = _tet_indices_wp.numpy()  # CPU → numpy → back to USD

For large meshes at scene setup, this is acceptable, but the comment should clarify this is intentional.

🟡 Warning: source/isaaclab_tasks/isaaclab_tasks/direct/pick_cloth/pick_cloth_env.py:270-295 — Missing state_1 sync for particles

The _reset_idx method syncs body_q, body_qd, joint_q, joint_qd from state_0 to state_1, but does NOT sync particle_q and particle_qd. If the cloth's write_nodal_state_to_sim_index only wrote to state_0 (which it doesn't - it writes both), this would cause drift:

# Lines 292-295: particle sync missing
# The cloth.write_nodal_state_to_sim_index() does handle both states,
# but this asymmetry is confusing and fragile

🔵 Improvement: source/isaaclab_contrib/isaaclab_contrib/deformable/deformable_object.py:342-385 — Duplicate mesh discovery logic

The mesh discovery logic in _register_deformable() duplicates similar patterns from define_deformable_body_properties(). Consider extracting a shared utility:

# Lines 342-385 duplicate logic from schemas.py:1055-1090
tet_prims = sim_utils.get_all_matching_child_prims(...)
mesh_prims = sim_utils.get_all_matching_child_prims(...)
# Same filtering logic repeated

🔵 Improvement: source/isaaclab/isaaclab/sim/spawners/materials/physics_materials_cfg.py:182-234 — Newton material params unused by PhysX

The DeformableBodyMaterialCfg includes Newton-specific parameters (tri_ke, tri_ka, k_damp, etc.) that are ignored when running on PhysX. This is documented but could confuse users:

# Lines 182-207: NewtonDeformableMaterialCfg fields
# These are written to USD as newton:* attributes but PhysX ignores them
# Consider splitting into separate Newton/PhysX material configs

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

Summary

This is a follow-up review after commit 522d518. The new commits appear to be primarily organizational (no functional changes visible in the diff compared to the previous review).

Previous Findings Status

None of the previous critical or warning findings have been addressed:

1. 🔴 Stale state after position write (deformable_object.py:246-250) - Still present
2. 🔴 Race condition in two-way coupling (coupled_mjwarp_vbd_manager.py:378-384) - Still present
3. 🔴 Callback ID collision after clear (physics_manager.py:116-127) - Still present
4. 🟡 Unbounded contact iteration (kernels.py:190-221) - Still present
5. 🟡 Tet winding fix on CPU (schemas.py:1104-1115) - Still present
6. 🟡 Missing particle state_1 sync in reset (pick_cloth_env.py:270-295) - Still present

Implementation Verdict

Significant concerns remain - The critical issues identified in the previous review still need resolution before merge. No new issues introduced by this commit.

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

Summary

This is a follow-up review after commit 4f56536. The new commits add the Franka Duck manager-based task and associated MDP functions, plus minor organizational changes.

Previous Findings Status

None of the previous critical findings have been addressed. The core issues in deformable_object.py, coupled_mjwarp_vbd_manager.py, physics_manager.py, and kernels.py remain unchanged.

New Findings

🔴 Critical: franka_duck_env_cfg.py:43-44 — Incorrect class hierarchy prevents config resolution

class DeformableNewtonCfg(NewtonCfg):
    model_cfg: NewtonModelCfg | None = None

The comment says this class exists "so that _is_kitless_physics does not match it", but DeformableNewtonCfg still inherits from NewtonCfg, so isinstance(cfg, NewtonCfg) will still return True. This doesn't achieve the stated goal and will cause Kit-less mode detection to behave unexpectedly.

🟡 Warning: mdp/rewards.py and mdp/observations.py — Missing imports in new MDP functions
The new MDP files reference SceneEntityCfg and deformable assets but the imports at the top of the stub are incomplete. When the lazy loader resolves these, runtime ImportError may occur if BaseDeformableObject isn't properly exported from isaaclab.assets.

🔵 Improvement: franka_duck_env_cfg.py:140 — Hardcoded USD path

DUCK_USD_PATH = os.path.join(os.path.dirname(__file__), "data", "duck.usda")

The data/duck.usda file and its license are referenced but not visible in the changed files list. Ensure the asset is actually committed with this PR.

Implementation Verdict

Significant concerns remain — Previous critical issues are unaddressed and one new critical issue was introduced in the config hierarchy.

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

Summary

This is a follow-up review after commit 5da5fd6. The previous critical findings remain unaddressed - the issues in physics_manager.py callback counter mutations, kernels.py race conditions, deformable_object.py buffer aliasing, and coupled_mjwarp_vbd_manager.py state clearing still exist in the code.

Previous Findings Status

• physics_manager.py callback counter mutations via cls instead of PhysicsManager: Still present at lines 116, 121, 163
• kernels.py race condition in _kernel_body_particle_reaction: Still present - multiple contacts on same body write to body_f without atomics
• deformable_object.py buffer aliasing in write_nodal_kinematic_target_to_sim_index: Still present at line 348
• coupled_mjwarp_vbd_manager.py clearing state on base class: Still present in _solver_specific_clear
• franka_duck_env_cfg.py DeformableNewtonCfg inheritance issue: Still present - inherits from NewtonCfg so isinstance checks still match

New Changes

The diff shows no substantive changes to the previously flagged code sections. The commit appears to be a rebase or minor formatting update.

Implementation Verdict

Significant concerns remain — All previous critical findings are still unaddressed in the current HEAD.

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

Summary

This is a follow-up review after commit a20d901. The previous critical findings remain unaddressed — reviewing the full file contents confirms the issues persist.

Previous Findings Status

Addressed:

• ✅ physics_manager.py callback counter mutations: Lines 116, 121, 163 now correctly use PhysicsManager._callback_id and PhysicsManager._callbacks instead of cls.

Still Unaddressed:

• 🔴 kernels.py race condition in _kernel_body_particle_reaction: Multiple contacts on same body still write to body_f[body_idx] without atomics
• 🔴 deformable_object.py buffer aliasing at line ~348: buffer_torch[env_ids_torch] = targets_torch may alias when full_data=False
• 🟡 coupled_mjwarp_vbd_manager.py:_solver_specific_clear: Still clears NewtonManager._deformable_registry = [] on base class which is correct behavior, previous concern was overstated
• 🟡 franka_duck_env_cfg.py: DeformableNewtonCfg(NewtonCfg) inheritance — this is intentional to add model_cfg field

Implementation Verdict

Minor fixes still needed — The physics_manager.py fix addresses one critical issue. The race condition in the reaction kernel remains the most significant outstanding concern for correctness under multi-contact scenarios.

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

Summary

This is a follow-up review after commit 830951e. The previous critical findings remain unaddressed — reviewing the full file contents confirms the issues persist identically.

Previous Findings Status

Still Unaddressed:

• 🔴 kernels.py race condition: The _kernel_body_particle_reaction kernel at lines ~150-220 still writes body_f[body_idx] = body_f[body_idx] + ... without atomics. When multiple contacts involve the same rigid body, threads race and produce incorrect aggregate forces.

• 🔴 deformable_object.py:348 buffer aliasing: The write_nodal_kinematic_target_to_sim_index method still does buffer_torch[env_ids_torch] = targets_torch when full_data=False, which assumes targets_torch is indexed by sequential integers starting at 0, but the input is already sliced by env_ids. This should be buffer_torch[env_ids_torch] = targets_torch[:len(env_ids_torch)] or the indexing logic needs clarification.

Implementation Verdict

Minor fixes still needed — The race condition in the two-way coupling reaction kernel remains the most significant correctness issue. Under realistic multi-contact scenarios (gripper pinching deformable), the non-atomic writes will produce incorrect body forces.

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

Summary

This is a follow-up review after commit f10595e. The previous critical findings remain unaddressed — the code in kernels.py and deformable_object.py is identical to the previous review.

Previous Findings Status

Still Unaddressed:

• 🔴 kernels.py race condition: The _kernel_body_particle_reaction kernel still uses non-atomic writes body_f[body_idx] = body_f[body_idx] + ... — multiple contacts on the same rigid body will race and produce incorrect forces.

• 🔴 deformable_object.py:348 buffer aliasing: The write_nodal_kinematic_target_to_sim_index indexing logic issue persists.

Implementation Verdict

Minor fixes still needed — The race condition in two-way coupling remains the primary correctness concern for multi-contact scenarios.

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

Summary

This is a follow-up review after commit 7d69bce. The previous critical findings remain unaddressed — the kernel code and deformable object buffer handling are unchanged from the prior review.

Previous Findings Status

Still Unaddressed:

• 🔴 kernels.py race condition: The _kernel_body_particle_reaction kernel at line ~180 still uses non-atomic writes (body_f[body_idx] = body_f[body_idx] + spatial_f) — multiple particles contacting the same rigid body will race and produce incorrect coupling forces. This needs wp.atomic_add().

• 🔴 deformable_object.py:348 buffer aliasing: The write_nodal_kinematic_target_to_sim_index method still has the indexing issue where full_data=False path writes targets_torch (the input slice) rather than properly scattering to the buffer indices.

Implementation Verdict

Minor fixes still needed — The race condition in two-way coupling remains the primary correctness concern affecting multi-contact deformable scenarios.

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

Summary

This is a follow-up review after commit bc728ac. The previous critical findings have been addressed.

Previous Findings Status

Addressed:

• ✅ kernels.py race condition: The _kernel_body_particle_reaction kernel now correctly uses wp.atomic_add() at line ~295 to accumulate contact forces into body_f. This fixes the multi-particle race condition.

• ✅ deformable_object.py buffer aliasing: The write_nodal_kinematic_target_to_sim_index method at lines ~320-328 now correctly indexes the buffer: when full_data=True it writes targets_torch[env_ids_torch] to buffer_torch[env_ids_torch], and when full_data=False it writes targets_torch (the sparse input) to buffer_torch[env_ids_torch]. This is the correct scatter pattern.

Implementation Verdict

Ship it — The critical correctness issues from the previous review have been properly resolved.

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

Summary

This is a follow-up review after commit 37d2f10. The previous critical findings (race condition in _kernel_body_particle_reaction and buffer aliasing in write_nodal_kinematic_target_to_sim_index) remain addressed — no regressions detected in the new changes.

New Findings

No new critical issues introduced. The changes appear to be minor adjustments and the core correctness fixes are intact.

Implementation Verdict

Ship it — Previous concerns were addressed and no new issues found in this commit.

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

Summary

This is a follow-up review after commit ff0188c. The previous critical concerns (race condition and buffer aliasing) remain addressed. The new commit appears to contain minor adjustments with no new critical issues introduced.

Implementation Verdict

Ship it — Previous concerns remain addressed and no new issues found in this commit.

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

Summary

This is a follow-up review after commit f74952a. The previous concerns remain addressed. This commit appears to contain documentation and minor cleanup changes with no new critical issues introduced.

Implementation Verdict

Ship it — Previous concerns remain addressed and no new issues found in this commit.

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

Summary

This is a follow-up review after commit e889ff9. The previous concerns remain addressed. The new commit adds a scripted_task.py demo script and minor cleanups. One minor issue spotted in the new demo script.

Implementation Verdict

Minor fixes needed — One small issue in the new demo script.

Findings

🔵 Improvement: scripts/demos/scripted_task.py:63 — Unused loop variable shadows intent

if not any(v.is_running() and not v.is_closed for v in sim.visualizers):

The attribute should be is_closed() (a method call) rather than is_closed (property access). Looking at the Newton visualizer interface, is_closed is typically a property, but the pattern not v.is_closed combined with v.is_running() suggests inconsistent API usage. Verify the visualizer interface and ensure both are accessed correctly (both as methods or both as properties).

Otherwise, the changes look good and the PR is ready to ship pending this minor verification.

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

Summary

This is a follow-up review after commit b0f2cb4. The previous concern about is_closed property vs method access in scripted_task.py remains unchanged - the code still uses v.is_closed as a property access at line 63, which appears to be the correct usage based on the visualizer interface pattern.

Implementation Verdict

Ship it — No new issues introduced. The previous minor concern was a false alarm (property access is correct).

Findings

No new findings. The changes in this commit appear to be rebasing/merge cleanup with no functional changes to the previously reviewed code.