Add stable VBD rigid coupling controls - #6733
Conversation
Greptile SummaryAdds stable rigid-contact controls and generalized-coordinate synchronization for VBD physics flows.
Confidence Score: 5/5The PR appears safe to merge, with no concrete changed-code failure identified. The new configuration fields are forwarded through existing constructor filtering, builder coloring remains immediately before finalization, and post-solve IK writes into the output state before the established state-buffer swap while excluding unsupported cable articulations. Important Files Changed
Sequence DiagramsequenceDiagram
participant Manager as VBD/Coupler Manager
participant Solver as VBD/Coupled Solver
participant State as Output State
participant IK as Newton eval_ik
Manager->>Solver: step(state_in, state_out, control, contacts, dt)
Solver-->>State: update maximal body state
alt supported VBD-owned non-cable articulation
Manager->>IK: reconstruct joint_q and joint_qd
IK-->>State: publish synchronized generalized state
else no eligible articulation
Manager-->>State: skip IK synchronization
end
Reviews (1): Last reviewed commit: "Add stable VBD rigid coupling controls" | Re-trigger Greptile |
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
Isaac Lab Review Bot
Summary
This PR exposes new SolverVBD rigid-contact stability knobs on VBDSolverCfg, adds contact-matching options to NewtonCollisionPipelineCfg, moves VBD builder coloring from instantiate_builder_from_stage into the shared _prepare_builder_for_finalize hook, and adds a post-step eval_ik synchronization of generalized coordinates for VBD-owned, non-cable articulations (both standalone VBD and coupled managers). The mask construction and step wiring are clear and unit-tested at the helper level. Two areas need attention: the coupled mask selects an articulation when any of its joints is VBD-owned, which can rewrite generalized state owned by another entry under ADMM coupling; and several new config fields are forwarded through name-based kwarg filtering without a test that the names actually match the Newton constructors, so a typo would ship as a silently ignored knob.
Architecture impact
Moving builder.color() into _prepare_builder_for_finalize is a sensible consolidation and lets the coupled manager color only when a VBD entry is present. However, NewtonCouplerManager._prepare_builder_for_finalize now no longer calls super(), and NewtonVBDManager._prepare_builder_for_finalize does not delegate upward either, so any future logic added to the base hook would be silently skipped on the coupled path. The coupler's _step_solver override also duplicates the inherited VBD implementation verbatim, creating two copies that can drift.
Test coverage
New tests cover mask construction (including cable exclusion), the _step_solver call shape, and config field round-tripping. The gaps are at the integration boundaries: no test proves the new VBDSolverCfg / NewtonCollisionPipelineCfg fields survive kwarg filtering into SolverVBD / CollisionPipeline (both assertions currently hold for any declared dataclass field), and no test drives _build_solver to verify the IK mask is actually installed or cleared, nor the coupler's VBD-only coloring branch.
Implementation verdict
Minor fixes needed. Posted 7 actionable findings inline.
Automated comment-only review; human maintainers own approval decisions.
| strict=True, | ||
| ) | ||
| ): | ||
| mask[articulation_index] &= any(joint in vbd_owned_joints for joint in range(int(start), int(end))) |
There was a problem hiding this comment.
🟡 Warning — Partially VBD-owned articulations get full-articulation IK
The mask is enabled when any joint in [start, end) belongs to a VBD entry, but eval_ik operates at articulation granularity and rewrites joint_q/joint_qd for every joint in the selected articulation. ADMM coupling explicitly allows an articulation to be split across entries (see test_cross_entry_joint_is_left_unowned_for_admm_attachment), so a single VBD-owned joint would cause the generalized coordinates just integrated by another solver to be overwritten after each step, with no error. Consider requiring full VBD ownership (all(...)) or explicitly rejecting split articulations that contain VBD-owned joints.
| mask[articulation_index] &= any(joint in vbd_owned_joints for joint in range(int(start), int(end))) | |
| mask[articulation_index] &= all(joint in vbd_owned_joints for joint in range(int(start), int(end))) |
| """Normalize kinematic colliders when a coupled entry uses implicit MPM.""" | ||
| super()._prepare_builder_for_finalize(builder) | ||
| solver_cfg = getattr(PhysicsManager._cfg, "solver_cfg", None) | ||
| if any(isinstance(entry.solver_cfg, VBDSolverCfg) for entry in getattr(solver_cfg, "entries", ())): |
There was a problem hiding this comment.
🟡 Warning — Base _prepare_builder_for_finalize is no longer invoked on the coupled path
The super()._prepare_builder_for_finalize(builder) call was removed and replaced with explicit conditional dispatch to the VBD and MPM hooks. Any preparation performed by NewtonManager._prepare_builder_for_finalize is now skipped for coupled scenes and will keep being skipped silently if the base hook gains logic later. The same applies to NewtonVBDManager._prepare_builder_for_finalize, which only calls builder.color() without delegating upward. Consider invoking the base hook explicitly before the solver-specific branches, and having the VBD override call super() first.
|
|
||
|
|
||
| def test_vbd_cfg_exposes_rigid_contact_stability_controls(): | ||
| """Coupled rigid scenes can configure the public SolverVBD stability knobs.""" |
There was a problem hiding this comment.
🟡 Warning — New VBD rigid-contact knobs are not proven to reach SolverVBD
This test only reads back values it just assigned to the configclass, so it passes for any declared field. The new fields reach the solver solely through _filter_solver_kwargs(SolverVBD, solver_cfg), which drops keys that do not match the constructor signature — a misspelled name (e.g. rigid_avbd_contact_alpha vs. the solver's actual parameter) would ship as a silently ignored knob while this test stays green. Please assert the filtered kwargs contain each new field with the configured value, and that the declared defaults match SolverVBD's own defaults (the defaults are always forwarded, so a mismatch changes behavior for every existing VBD scene).
|
|
||
| def test_collision_cfg_forwards_contact_matching_controls(): | ||
| """Collision history users can request Newton's native matching buffers.""" | ||
| cfg = NewtonCollisionPipelineCfg( |
There was a problem hiding this comment.
🟡 Warning — Contact-matching forwarding test cannot fail on a name mismatch
to_pipeline_args() is essentially to_dict() with the hydroelastic entry replaced, so every declared configclass field appears in the result. This test therefore passes even if newton.CollisionPipeline.__init__ does not accept contact_matching, contact_matching_pos_threshold, or contact_matching_normal_dot_threshold — the real failure (a TypeError at pipeline construction) is not covered. Consider asserting the new keys against inspect.signature(CollisionPipeline.__init__).parameters, or constructing a pipeline with these arguments.
| NewtonManager._needs_collision_pipeline = needs_collision_pipeline | ||
|
|
||
| @classmethod | ||
| def _step_solver( |
There was a problem hiding this comment.
🔵 Suggestion — Coupler _step_solver duplicates the inherited VBD implementation
NewtonCouplerManager inherits from NewtonVBDManager, and this override is byte-identical to the parent's _step_solver (the mask is already a class attribute resolved through cls). Removing the override would keep behavior identical and avoid two copies that can drift apart. If the override is kept for clarity, consider delegating the solver step via super()._step_solver(...) rather than calling cls._solver.step(...) directly.
| NewtonManager._solver = cls._create_solver(model, solver_cfg) | ||
| NewtonManager._use_single_state = False | ||
| NewtonManager._needs_collision_pipeline = True | ||
| articulation_mask = cls._make_ik_articulation_mask(model) |
There was a problem hiding this comment.
🔵 Suggestion — IK-mask installation in _build_solver is untested
The gate cls._ik_articulation_mask = articulation_mask if bool(np.any(...)) else None (and its counterpart in the coupler) decides whether eval_ik runs at all after every substep, but tests exercise only _make_ik_articulation_mask and _step_solver in isolation with a manually installed sentinel mask. An inverted gate, a dropped assignment, or a stale mask carried over from a previous run would leave every added test green. A _build_solver test with a cable-only model (expect None) and one with a revolute articulation (expect a non-None mask, used by the next step) would close this.
| assert PHYSICS_CONTEXT is NewtonReplicateContext | ||
|
|
||
|
|
||
| def test_vbd_manager_colors_replicated_builder_before_finalize(): |
There was a problem hiding this comment.
🔵 Suggestion — Coloring move and the coupler's VBD-only branch are only tested by direct hook calls
builder.color() was removed from instantiate_builder_from_stage, so VBD now depends on the finalize hook being reached on both the stage-built and replicated paths; this test calls the hook directly and would still pass if the hook were never invoked. Likewise, the only coupler lifecycle test (test_mpm_entry_reuses_builder_lifecycle_hooks) uses an MPM-only entry and passes both before and after the new VBD branch. Adding a coupled case with a VBDSolverCfg entry (asserting the VBD hook runs) and a non-VBD case (asserting it does not) would cover the new dispatch.
mmichelis
left a comment
mmichelis
left a comment
There was a problem hiding this comment.
Thanks for the additions for the VBD solver for rigid bodies! The config additions would absolutely be helpful to add, but I think the IK mask can be a separate PR, and will likely need a bit more generalization. We would also want more understanding from the Newton side how this will be handled moving forward, perhaps you could simplify this PR down to just the VBD config changes so we have better rigid solver control?
|
|
||
| cls._validate_config(solver_cfg) | ||
| resolved_entries = [cls._resolve_entry(model, entry) for entry in solver_cfg.entries] | ||
| vbd_entries = [entry for entry in resolved_entries if isinstance(entry.config.solver_cfg, VBDSolverCfg)] |
There was a problem hiding this comment.
very specific VBD addition in the coupler, ideally we can decouple this and have a manager specific call that handles the IK masking (since it isn't VBD specific, but Kamino might also want/need this)
| rigid_avbd_beta: float = 0.0 | ||
| """Penalty ramp rate per rigid-body AVBD iteration; zero disables ramping.""" |
There was a problem hiding this comment.
rigid_avbd_beta docstring omits the rigid_contact_k_start interaction, and it is the direction that matters.
Newton documents the dependency on the other field: "rigid_contact_k_start: Body-body and body-particle contact penalty seed for AVBD ramping. Used when rigid_avbd_linear_beta (or rigid_avbd_beta fallback) is greater than zero. When the linear beta is 0, k is fixed at the contact stiffness regardless of this value." (newton/_src/solvers/vbd/solver_vbd.py:358-360 at the pinned commit d7581b7).
Since the default added here is rigid_avbd_beta = 0.0, the pre-existing rigid_contact_k_start field on line 117 is a no-op by default, and its docstring ("Initial stiffness seed for all rigid body contacts [N/m]") is now actively misleading: this PR exposes the field that gates it without cross-referencing in either direction. A user reading line 117 in isolation will tune a value that does nothing.
Two smaller things dropped from Newton's own docs for this field:
- Linear (meters) and angular (radians) constraints have different units, so Newton recommends the
rigid_avbd_linear_beta/rigid_avbd_angular_betaoverrides for production tuning. Worth stating even though those overrides are not exposed on this configclass. - A concrete non-zero magnitude. Newton suggests e.g.
1e5;0.0alone gives no scale hint.
Also, sibling fields in this class consistently carry unit brackets ([N/m], [N*s/m], [m]); this one has none.
| rigid_avbd_beta: float = 0.0 | |
| """Penalty ramp rate per rigid-body AVBD iteration; zero disables ramping.""" | |
| rigid_avbd_beta: float = 0.0 | |
| """Penalty ramp rate per rigid-body AVBD iteration; zero disables ramping. | |
| :attr:`rigid_contact_k_start` only takes effect when this is greater than | |
| zero; at zero the penalty stiffness is fixed at the contact stiffness. | |
| Set to e.g. ``1e5`` to enable ramping. Shared by linear [m] and angular | |
| [rad] constraints, which have different units. | |
| """ |
2 participants
Description
Expose the SolverVBD rigid-contact stability controls needed by coupled rigid/deformable scenes and keep VBD-owned articulation generalized coordinates synchronized after each solve.
This also moves VBD builder coloring into the shared pre-finalize hook so coupled managers color only when they contain a VBD entry, and exposes Newton collision-pipeline contact matching in
NewtonCollisionPipelineCfg.The changes are bounded to VBD-owned non-cable articulations; cable-only and non-VBD coupling entries skip the inverse-kinematics synchronization.
Type of change
Validation
uv run --locked --extra test pytest -q source/isaaclab_contrib/test/coupling/test_coupler.py source/isaaclab_contrib/test/deformable/test_deformable_builder_hooks.py source/isaaclab_newton/test/physics/test_newton_manager_abstraction.py— 145 passeduv run --locked isaaclab -f— passeduv run --locked python tools/changelog/cli.py check develop— passedScreenshots
Not applicable; this changes physics-manager behavior and configuration.
Checklist
uv run isaaclab -fsource/<pkg>/changelog.d/for every touched packageCONTRIBUTORS.md