Move standalone VBD support into Newton core - #6937
Conversation
github-actions
Bot
added
bug
Greptile SummaryThe PR moves standalone VBD configuration and manager support from
Confidence Score: 5/5The PR appears safe to merge; no concrete changed-code failure remains after checking the VBD lifecycle, replication, optional contrib integration, and configuration migration. The migrated contact settings remain attached to the outer Newton configuration, optional contrib integration is isolated behind guarded imports, and the new VBD replication path preserves per-world transforms, hooks, sites, and deformable exclusions. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
Launcher["Launcher / task preset"] --> NewtonCfg["NewtonCfg"]
NewtonCfg --> VBDCfg["VBDSolverCfg"]
NewtonCfg --> ContactCfg["NewtonSoftContactCfg"]
VBDCfg --> VBDManager["NewtonVBDManager"]
VBDManager --> CoreBuilder["Core Newton builder and VBD solver"]
Contrib["Optional isaaclab_contrib deformable integration"] -. registers builder and Fabric hooks .-> VBDManager
VBDManager --> Model["Finalized Newton model"]
ContactCfg --> Model
Reviews (1): Last reviewed commit: "Correct coupled VBD migration note" | 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
The VBD manager/config relocation into isaaclab_newton.physics, outer NewtonCfg.soft_contact_cfg, and migrated launcher, coupling, task, documentation, and test paths are internally consistent. Before merge, preserve the removed contrib APIs through a deprecation cycle and document the changed damping default in the contrib migration guidance.
- Design and architecture: Moving standalone VBD ownership into Newton core while retaining deformable-object integration and coupled solvers in contrib is coherent. Hoisting global soft-contact parameters onto
NewtonCfgalso removes the solver-specific model-config shim cleanly. - API: The new core exports and API documentation are wired correctly. However, the previously public contrib VBD symbols are deleted without the prior deprecation required by repository policy; forwarding aliases should remain for a release. The contrib changelog must also state that
soft_contact_kdchanges from0.01to10.0and explain how to preserve the old behavior. - Implementation: The model-finalization soft-contact application, VBD stage-import exclusions and replication hooks, BVH rebuild ordering, launcher path, and migrated coupling consumers align with the new API. The required implementation change is compatibility forwarding at the old contrib import paths; the associated migration note should cover the materially changed damping default.
Minor fixes needed. Posted 2 actionable findings inline.
Automated review; human maintainers own approval decisions.
| Changed | ||
| ^^^^^^^ | ||
|
|
||
| * **Breaking:** Moved the standalone VBD solver from |
There was a problem hiding this comment.
🟡 Warning · Api — Contrib VBD symbols removed without prior deprecation
VBDSolverCfg, NewtonModelCfg, NewtonModelSolverCfg, and vbd_manager.NewtonVBDManager were exported from isaaclab_contrib.deformable.__init__.pyi and documented via autoclass; this change deletes them so existing imports fail immediately. Repository rules require deprecating public symbols in a prior release before removal. Keep thin forwarding aliases at the old paths that emit DeprecationWarning and resolve to the core types for one release.
| ``isaaclab_contrib.deformable`` to :mod:`isaaclab_newton.physics`. Import | ||
| :class:`~isaaclab_newton.physics.NewtonVBDManager` and | ||
| :class:`~isaaclab_newton.physics.VBDSolverCfg` from their new location, and | ||
| move ``NewtonModelCfg`` and ``NewtonModelSolverCfg`` soft-contact settings to |
There was a problem hiding this comment.
🔵 Suggestion · Api — Migration note omits soft_contact_kd default change
NewtonModelCfg.soft_contact_kd defaulted to 1.0e-2 while NewtonSoftContactCfg.soft_contact_kd defaults to 10.0, so users who simply move settings to soft_contact_cfg silently get 1000x the contact damping. Changed entries must carry migration guidance; add the default-value change and the explicit soft_contact_kd=0.01 restoration note to this fragment.
AntoineRichard
left a comment
AntoineRichard
left a comment
There was a problem hiding this comment.
AI-generated review
Requesting changes for the code-quality and documentation issues called out inline. The largest concerns are the duplicated Newton stage-import pipeline, public configuration docstrings that lost functional value semantics during the move, and tests that either duplicate the manager-matrix coverage or do not exercise the new soft-contact application path.
The two existing inline findings about removing public contrib APIs without a deprecation cycle and omitting the soft_contact_kd migration guidance also remain unresolved and are merge-blocking under the repository rules.
Validation performed on the exact PR head (0a5fedf1a0):
uv run isaaclab -f: passed.- 84 focused non-GPU VBD, launcher, coupling, and manager-wiring tests: passed.
uv run --isolated --extra test -- make -C docs current-docs: passed.
| setup_registered_deformable_fabric_sync(cls) | ||
|
|
||
| @classmethod | ||
| def instantiate_builder_from_stage(cls): |
There was a problem hiding this comment.
[AI-generated review] Avoid duplicating the complete stage-import pipeline. This method copies nearly all of NewtonManager.instantiate_builder_from_stage (environment discovery, global/prototype imports, visual restoration, site injection, pose extraction, replication, and manager-state publication) solely to add deformable ignore paths and coloring. Any correction to the generic importer now has to be made in two places, and the copies have already drifted in structure. Extract the shared import/replication flow and provide a protected hook for solver-specific ignore paths; VBD should only supply its deformable exclusions and coloring requirement.
There was a problem hiding this comment.
VBD now uses the shared NewtonManager.instantiate_builder_from_stage() flow and only overrides _get_usd_import_ignore_paths() to provide deformable exclusions. Coloring remains VBD-specific without duplicating the import pipeline.
| """Self-contact detection margin [m].""" | ||
|
|
||
| particle_collision_detection_interval: int = -1 | ||
| """How often particle self-contact detection runs.""" |
There was a problem hiding this comment.
[AI-generated review] Document the accepted value semantics. The default is -1, but "how often" does not tell users what negative, zero, or positive values do. The moved contrib API documented the actual contract: < 0 runs once before initialization, 0 runs before and after initialization, and k >= 1 runs before every k VBD iterations. Restore that concise mapping so this public field is functional rather than merely descriptive.
There was a problem hiding this comment.
Addressed. Restored the concise < 0, 0, and k >= 1 behavior mapping from the contrib API documentation.
| """Body-particle contact damping [N*s/m].""" | ||
|
|
||
| soft_contact_mu: float = 0.5 | ||
| """Body-particle contact friction coefficient [dimensionless].""" |
There was a problem hiding this comment.
[AI-generated review] Preserve the solver semantics when moving this public config. The removed NewtonModelCfg documentation stated that effective friction is sqrt(soft_contact_mu * shape_mu); this replacement now reads as though soft_contact_mu is used directly. Restore the concise formula and its interaction with per-shape material friction. The same principle applies to soft_contact_ke, whose effective contact stiffness is averaged with the rigid shape stiffness. These are functional contracts, not tuning rationale.
There was a problem hiding this comment.
Restored the effective body-particle stiffness and friction formulas, including their interaction with the rigid shape material properties in d7708fed1b.
| from isaaclab_newton.physics import NewtonCfg, NewtonManager | ||
|
|
||
|
|
||
| def test_vbd_symbols_are_exported_from_core(): |
There was a problem hiding this comment.
[AI-generated review] Remove this duplicate test. test_newton_manager_abstraction.py now imports both public symbols, includes VBD in SOLVER_MATRIX (covering class_type resolution), and includes NewtonVBDManager in the subclass contract test. Consequently every assertion here is already exercised elsewhere; keeping this adds maintenance without guarding an additional regression.
There was a problem hiding this comment.
Addressed in 33c4862066. Removed the duplicate test; the abstraction suite already covers public exports, class resolution, and subclass contracts.
| assert issubclass(physics.NewtonVBDManager, NewtonManager) | ||
|
|
||
|
|
||
| def test_soft_contact_cfg_defaults_match_newton(): |
There was a problem hiding this comment.
[AI-generated review] Test the new behavior, not copied constants. These assertions pass even if the new assignments in NewtonManager.start_simulation are deleted. Add a behavioral test that supplies non-default NewtonSoftContactCfg values, finalizes a model (or a focused fake), and verifies that all three values reach the finalized model; also cover that None preserves Newton defaults. The literal-default assertions can then be removed or kept only if the defaults themselves are an intentional compatibility contract.
There was a problem hiding this comment.
Addressed in 713f6a07bc. Replaced the literal-default assertions with behavioral coverage for custom value propagation and preservation of Newton defaults when the config is None.
| :class:`~isaaclab_contrib.deformable.VBDSolverCfg` directly. Robot or | ||
| rigid-body scenes can use either: | ||
| :class:`~isaaclab_newton.physics.VBDSolverCfg` directly. Robot or | ||
| rigid-body scenes can use: |
There was a problem hiding this comment.
[AI-generated review] Keep the supported-coupling list consistent with this page. This wording makes the following list exhaustive, but the Custom MJWarp + VBD Parameters section later on this page still documents CoupledMJWarpVBDSolverCfg as a supported opt-in rigid/deformable path. Either retain that third bullet here or narrow this sentence to the named-entry couplers so the overview does not contradict its own detailed section.
There was a problem hiding this comment.
Addressed in c3da4fd677. Narrowed the overview list to named-entry coupling options so it no longer excludes the custom shared-model path documented later.
| * Soft bodies and particles are available through the experimental VBD path in | ||
| :mod:`isaaclab_contrib.deformable`; other non-rigid PhysX features are not | ||
| yet covered. | ||
| * Soft bodies and particles use the core VBD solver with the contrib deformable |
There was a problem hiding this comment.
[AI-generated review] This is a capability, not a known gap. The same capability is already stated in the package and solver-coverage sections above, so repeating it here obscures the actual limitations and creates another statement to keep synchronized. Remove it from Known Gaps, or rewrite it to name the concrete non-rigid features that remain unsupported.
There was a problem hiding this comment.
Addressed in 7bb11dd89f. Removed the duplicated capability statement so Known Gaps now contains only actual limitations.
|
Hi @mmichelis — thanks for addressing the duplicated stage-import pipeline and preserving the VBD coloring flow. Could you please clarify the plan for the other review comments that are still open? In particular:
Some of these may be intentionally deferred or not planned for this PR. If so, could you briefly explain the rationale and, where applicable, whether they will be handled in follow-up work? That will help me understand which threads can be closed. Thanks! |
7bb11dd to
6fb7a62
Compare
Add a narrow solver hook for USD import exclusions so VBD can reuse the generic stage importer. Move VBD coloring to finalization preparation and retain focused import-path coverage.
Keep stage import shared while preserving separate coloring for stage-created and prebuilt VBD builders.
Document the accepted interval values and trim redundant VBD coloring test scaffolding.
Restore the effective stiffness and friction formulas for body-particle soft contacts.
Rely on the Newton manager abstraction suite for public exports, class resolution, and subclass contracts.
Exercise configured overrides and preservation of finalized model defaults through the startup lifecycle.
Scope the overview list to named-entry coupling while retaining the custom shared-model path in its dedicated section.
Carry the new rigid-particle buffer option into the core VBD config and keep rigid-force capability coverage after removing the contrib manager.
Update the cable examples to use the core VBD configuration and mark the contrib API removal as a major change.
23ac3ba to
ab29bc3
Compare
3 participants
Description
Moves the standalone Newton VBD solver configuration and manager from
isaaclab_contrib.deformableintoisaaclab_newton.physics. The core VBD API can now be imported withoutisaaclab_contrib. When the contrib deformable integration is installed,NewtonVBDManageruses its registered deformable builder hook and Fabric synchronization.NewtonVBDManager,VBDSolverCfg, andNewtonSoftContactCfg.solver_cfg.model_cfgtoNewtonCfg.soft_contact_cfg.NewtonModelCfg,NewtonModelSolverCfg,VBDSolverCfg, andNewtonVBDManagersymbols. Import the standalone VBD types fromisaaclab_newton.physicsinstead.No new external dependencies are required.
The removed symbols are experimental contrib APIs. This change intentionally does not retain compatibility aliases at the old import paths.
Migration
Import the standalone VBD types from
isaaclab_newton.physics:Global soft-contact parameters move from
solver_cfg.model_cfgto the outerNewtonCfg.soft_contact_cfg.NewtonSoftContactCfg.soft_contact_kddefaults to Newton's10.0; set it explicitly if you relied on the previous contrib default of0.01.Type of change
Validation
developchanges.Pytest warnings were limited to third-party PyTorch deprecation warnings and existing Newton warnings.
Checklist
./isaaclab.sh -f.CONTRIBUTORS.md.