Proxy-coupled MJWarp (Articulation) + VBD (Particles) Solver#5751
Proxy-coupled MJWarp (Articulation) + VBD (Particles) Solver#5751mmichelis wants to merge 19 commits into
Conversation
github-actions
Bot
added
documentation
![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 — Automated PR Review
Summary
This PR adds NewtonProxyCoupledMJWarpVBDManager wrapping Newton's SolverCoupledProxy, enabling partitioned simulation between MuJoCo Warp (rigids/articulations) and VBD (particles/deformables) with selected MJWarp bodies exposed as proxies in the VBD view via lagged impulses. The implementation includes flexible body selectors supporting both SceneEntityCfg entries and raw USD prim-path regex strings, comprehensive tests, and documentation updates.
🏗️ Architecture & Design (Isaac Lab Expert)
Strengths:
- Clean separation of concerns: The
NewtonProxyCoupledMJWarpVBDManagerextendsNewtonVBDManagerappropriately, reusing builder hooks and deformable infrastructure - Well-designed selector system: Supporting both
SceneEntityCfgand raw prim-path regex strings provides flexibility for assets that may not be registered as named scene entities - Good use of the existing
_filter_solver_kwargshelper (centralized inNewtonManager) reduces code duplication across all solver managers - The
CoupledNewtonCfgwithscene_cfgfield elegantly solves the resolver-at-build-time problem - Proxy configuration parameters (
proxy_mode,proxy_iterations,proxy_collide_interval,proxy_mass_scale) are well-documented and expose the underlying Newton solver capabilities appropriately
Suggestions:
-
Consider documenting the performance trade-offs — The docs mention proxy coupling "typically scales better" but a brief note about when to prefer alternating coupling vs proxy coupling would help users choose
-
Line 46-48 in
proxy_coupled_mjwarp_vbd_manager.py— Thescene_cfglookup:outer_cfg = PhysicsManager._cfg scene_cfg = outer_cfg.scene_cfg if isinstance(outer_cfg, CoupledNewtonCfg) else None
This relies on accessing the physics manager's internal
_cfg. Consider adding a comment explaining this coupling, as it's a non-obvious dependency
🔍 Silent Failure Analysis
Potential Issues Found:
-
Empty partition warning could be clearer (Line 57-62 in
proxy_coupled_mjwarp_vbd_manager.py):if solver_cfg.proxy_bodies and not proxy_body_ids: logger.warning(...)
✅ Good: Warning is logged when proxy_bodies are specified but none match
⚠️ Consideration: Whenproxy_bodies=[](empty list), there's no warning. This is intentional but might surprise users who expect some default proxies -
Body partition validation (Lines 189-199 in
proxy_coupled_mjwarp_vbd_manager.py):if overlap := sorted(mjc_owned & vbd_owned): raise ValueError(...) unclaimed = [b for b in range(int(model.body_count)) if b not in mjc_owned and b not in vbd_owned] if unclaimed: raise ValueError(...)
✅ Excellent: Both overlapping and unclaimed bodies raise clear errors with body labels for debugging
-
SceneEntityCfg proxy_bodies validation (Lines 257-260):
if isinstance(spec, SceneEntityCfg) and spec.body_names is None: raise ValueError(...)
✅ Good: Requires explicit
body_namesfor SceneEntityCfg entries in proxy_bodies -
Regex pattern matching edge case — Line 141 uses
asset_re.match(lbl)which matches from string start. This is correct but note that patterns inbody_namesusefullmatchon short names. The different matching semantics are intentional but could be documented -
vbd_bodiesreceiving particles automatically (Lines 70-71):vbd_particles = list(range(model.particle_count))
All particles go to VBD entry — this is the correct design but users might not realize particles cannot be partitioned. A docstring note would help
🧪 Test Coverage Analysis
What's Tested (362 lines in test_proxy_coupled_mjwarp_vbd.py):
- ✅
_resolve_entity_to_body_ids: no body_names returns all, body_names filtering, missing asset error, unmatched patterns error - ✅
_partition_model_by_entities: correct split of bodies/joints/shapes, overlap detection, unclaimed detection - ✅
_select_proxy_bodies: COLLIDE_SHAPES filtering, body_names requirement, empty input, deduplication - ✅ Raw prim-path string selectors: prefix matching, single body, no matches error, mixed with SceneEntityCfg
What Could Be Added:
-
Static shape handling — Test that shapes with
body == -1always go to VBD (currently covered implicitly byextra_static_shape=Truetest but could be more explicit) -
Joint inheritance — Test that joints correctly inherit from their child body's partition (partially covered but edge cases like joints spanning partitions aren't tested)
-
Integration smoke test — The file notes that end-to-end tests rely on
test_environments.py. Consider adding a minimal integration test that verifies_build_solvercompletes without error with valid inputs -
Multi-environment body labels — Test that
env_0,env_1, etc. patterns work correctly with multiple environment clones
📝 Documentation Review
Strengths:
- Comprehensive RST documentation explaining proxy-coupling concept and when to use it
- Good parameter table in
using-vbd-solver.rst - Changelog fragment added
- Clear distinction between alternating and proxy coupling approaches
Minor Suggestions:
- The docs reference
newton_mjwarp_vbd_proxypreset but the default is now proxy-coupled. Users might be confused about which is which
🔧 Minor Code Quality Observations
-
Consistent rename from
deformabletoobject— The rename in rewards/observations/events is thorough and improves generality. Good migration approach -
Viewer config migration — Moving from
self.viewer.*tovisualizer_cfgslist is a clean pattern -
State machine demo simplification — The generic
parse_env_cfgapproach inlift_franka_soft.pyremoves task-specific branching — cleaner
✅ Overall Assessment
This is a well-designed, thoroughly tested PR that adds significant new functionality for proxy-coupled simulation. The architecture follows Isaac Lab patterns, error handling is robust with informative messages, and the test coverage is comprehensive for the static partitioning logic.
Recommendations:
- Address the CI "Check changelog fragments" failure (appears to be running)
- Consider the minor documentation clarifications noted above
- The code is ready for human review focusing on the Newton solver integration specifics
This review was generated by the Isaac Lab Review Bot using ensemble multi-model analysis.
Update (e05cf4f):
✅ Silent failure concern addressed — Empty proxy_bodies matching now raises ValueError instead of logging a warning. This directly addresses my earlier observation about potential silent failure.
Additional improvements in this commit:
- Added explicit
ValueErrorfor unknowncoupling_modevalues in both coupled managers (removes potential silent fall-through) - Fixed docstring references to use correct Newton API path (
newton.solvers.experimental.coupled.SolverCoupledProxy) - Removed unused
loggingimport
These changes improve robustness and code quality. 👍
Update (a372856):
✅ Changelog fragments added — This commit adds the missing changelog entries that were flagged by CI:
isaaclab_contrib: Documents the newProxyCoupledMJWarpVBDSolverCfgand managerisaaclab_newton: Documents the_filter_solver_kwargshelper extractionisaaclab_tasks: Documents the breaking rename fromdeformable→objectin lift_franka_soft
CI checks should now pass. LGTM 🚀
Update (ca5ef5d):
✅ No new issues — This commit focuses on documentation updates and unrelated improvements. Changes analyzed:
Documentation:
- Moved visualization docs from
/source/features/to/source/overview/core-concepts/(path reorganization) - Fixed relative image paths in visualization.rst
- Added
newton_kaminosolver support to preset tables - Reformatted environment preset tables for clarity with explicit
physics=,renderer=,presets=labels - Added new RLinf (GR00T VLA) integration documentation
Bug fixes (unrelated to proxy coupling):
- Fixed OVRTX log file path to use cross-platform
tempfile.gettempdir()instead of hardcoded/tmp - Fixed flaky noise tests using
torch.onesfor scale operations to avoid 0/0=NaN
Testing:
- Re-enabled rendering correctness tests in normal CI
- Removed redundant render correctness tests from shadow hand vision tests
All proxy-coupled solver changes from previous commits remain intact. LGTM 🚀
mmichelis
requested review from
Mayankm96,
jtigue-bdai,
kellyguo11 and
ooctipus
as code owners
Greptile SummaryThis PR introduces
Confidence Score: 3/5The proxy-body cross-validation gap means a misconfigured proxy_bodies list silently passes incorrect body IDs to SolverCoupledProxy, producing wrong simulation physics with no error. The new manager introduces a missing cross-check: bodies resolved by _select_proxy_bodies are never validated to be a subset of mjc_bodies. If a user configures proxy_bodies to select VBD-partition bodies, the coupled solver receives body IDs from the wrong entry and generates physically incorrect proxy forces with no diagnostic. Additionally, vbd_bodies=[] in the shipped franka_soft_env_cfg.py relies on an undocumented assumption about which assets produce Newton rigid-body entries, so a scene change could cause a runtime crash. proxy_coupled_mjwarp_vbd_manager.py (proxy body cross-validation) and franka_soft_env_cfg.py (implicit vbd_bodies assumption) deserve a second look before merge. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[_build_solver called] --> B[_partition_model_by_entities]
B --> C{any overlap?}
C -- yes --> ERR1[ValueError: overlap]
C -- no --> D{any unclaimed?}
D -- yes --> ERR2[ValueError: unclaimed]
D -- no --> E[mjc_bodies / vbd_bodies / joints / shapes split]
E --> F[_select_proxy_bodies]
F --> G{proxy_bodies empty?}
G -- yes --> H[proxy_ids = empty list]
G -- no --> I[resolve each spec to body IDs]
I --> J[filter: keep only COLLIDE_SHAPES bodies]
J --> K{resolved to zero?}
K -- yes --> ERR3[ValueError: zero collidable proxies]
K -- no --> L[proxy_ids = filtered list]
H --> M[Build SolverCoupledProxy entries mjc + vbd]
L --> M
M --> N[Build SolverCoupledProxy.Proxy source=mjc dest=vbd]
N --> O[NewtonManager._solver = SolverCoupledProxy]
O --> P[_use_single_state = False, _needs_collision_pipeline = False]
Reviews (1): Last reviewed commit: "Docs: Add changelog fragments" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| proxy_body_ids = cls._select_proxy_bodies(model, solver_cfg.proxy_bodies, scene_cfg) | ||
| if solver_cfg.proxy_bodies and not proxy_body_ids: | ||
| raise ValueError( | ||
| f"ProxyCoupledMJWarpVBDSolverCfg.proxy_bodies={solver_cfg.proxy_bodies!r} resolved to " | ||
| "zero bodies after filtering for `ShapeFlags.COLLIDE_SHAPES`. Rigid bodies would not be " | ||
| "visible to VBD; check that the selected bodies own at least one collidable shape." | ||
| ) |
There was a problem hiding this comment.
_select_proxy_bodies returns body IDs without checking whether they belong to mjc_bodies. The docstring specifies proxies must be MuJoCo bodies exposed to VBD, but nothing prevents a user from listing bodies that are actually in the VBD partition. When those IDs are passed to SolverCoupledProxy as the proxy source in the "mjc" entry, the coupled solver operates on body IDs that are owned by the "vbd" entry — producing physically incorrect proxy forces with no diagnostic. A guard like invalid = [b for b in proxy_body_ids if b not in set(mjc_bodies)] before constructing the SolverCoupledProxy.Proxy object would catch this at configuration time.
| if patterns is not None: | ||
| unmatched = [p for p, ok in zip(patterns, matched) if not ok] | ||
| if unmatched: | ||
| raise ValueError( | ||
| f"ProxyCoupledMJWarpVBDSolverCfg.{field}: {spec_repr} has no bodies matching {unmatched}." | ||
| ) | ||
| elif isinstance(spec, str) and not body_ids: | ||
| # Strings have no asset-cfg safety net — zero matches is almost always a typo. | ||
| raise ValueError( | ||
| f"ProxyCoupledMJWarpVBDSolverCfg.{field}: {spec_repr} matched no bodies in " | ||
| f"`model.body_label` (labels are full post-clone prim paths)." | ||
| ) |
There was a problem hiding this comment.
SceneEntityCfg with no matching bodies
When spec is a SceneEntityCfg with body_names=None and the asset's prim_path matches no bodies in the model (e.g., a typo in the asset name resolves to the wrong prim path on scene_cfg), the function returns an empty body_ids list without raising. The string-spec branch correctly raises for zero matches, but the SceneEntityCfg branch doesn't. The downstream "unclaimed bodies" error from _partition_model_by_entities will eventually fire, but with a misleading count rather than pointing at the offending selector.
| newton_mjwarp_vbd_proxy: CoupledNewtonCfg = CoupledNewtonCfg( | ||
| solver_cfg=ProxyCoupledMJWarpVBDSolverCfg( | ||
| mjwarp_cfg=MJWarpSolverCfg( | ||
| cone="elliptic", | ||
| ls_parallel=True, | ||
| ls_iterations=20, | ||
| integrator="implicitfast", | ||
| ), | ||
| vbd_cfg=VBDSolverCfg( | ||
| iterations=10, | ||
| ), | ||
| mjwarp_bodies=["/World/envs/env_.*/Robot"], | ||
| proxy_bodies=[ | ||
| "/World/envs/env_.*/Robot/panda_hand", | ||
| "/World/envs/env_.*/Robot/panda_(left|right)finger", | ||
| ], | ||
| proxy_collide_interval=5, | ||
| ), | ||
| model_cfg=NewtonModelCfg( | ||
| soft_contact_ke=1e4, | ||
| soft_contact_kd=1e-5, | ||
| soft_contact_mu=5.0, | ||
| shape_material_ke=4e4, | ||
| shape_material_kd=1e-5, | ||
| shape_material_mu=5.0, | ||
| ), | ||
| num_substeps=10, | ||
| ) |
There was a problem hiding this comment.
vbd_bodies left at default [] — relies on undocumented assumption
ProxyCoupledMJWarpVBDSolverCfg defaults vbd_bodies=[], so _partition_model_by_entities requires that mjwarp_bodies=["/World/envs/env_.*/Robot"] captures every rigid body in the Newton model. If the table USD (loaded via UsdFileCfg) or ground plane creates a Newton rigid body entry (body index ≥ 0), it would be unclaimed and trigger a runtime ValueError. This assumption is not documented in the config comment. A brief inline comment explaining why the scene's static assets don't produce Newton body entries would prevent future confusion when the scene changes.
Drop the local DeformableNewtonCfg in franka_soft_env_cfg.py and use the shared CoupledNewtonCfg from isaaclab_contrib.deformable.newton_manager_cfg instead, so the existing soft/cloth envs match the proxy-coupled manager.
Rename the scene entity, command, event, reward, termination, and mdp
function identifiers from 'deformable' to 'object' in the lift_franka_soft
environments and the lift_franka_soft state machine demo, so the proxy
coupled solver's SceneEntityCfg("object") selectors and the rest of the
lift task family share one asset key. Asset types (DeformableObjectCfg)
and USD prim paths are unchanged.
mmichelis
force-pushed
the
feat/proxycoupledsolver
branch
from
a372856 to
ca5ef5d
Compare
1 participant
Description
Summary
Adds a new
NewtonProxyCoupledMJWarpVBDManagerthat wrapsnewton.solvers.SolverCoupledProxy, splitting simulation between MuJoCo Warp (rigids/articulations) and VBD (particles/deformables) with selected MJWarp bodies exposed as proxies in the VBD view. Migrates the Franka soft lift environments to use this manager by default.Builds on top of Newton Coupling in gdaviet/coupled-solver-framework.
Changes
NewtonProxyCoupledMJWarpVBDManager+ matchingProxyCoupledMJWarpVBDSolverCfg, partitioning bodies/joints/shapes between the two sub-solvers and collecting proxy body IDs for the coupling.mjwarp_bodies,vbd_bodies, andproxy_bodiesaccept bothSceneEntityCfgentries and raw USD prim-path regex strings (e.g."/World/envs/env_.*/MyCube"), making it possible to claim assets that aren't registered as named scene entities.lift_franka_soft(rigid + cloth variants) now default toCoupledNewtonCfgwith proxy coupling; cleanup pass on the env configs, observations, and rewards (deformable → object rename).test_proxy_coupled_mjwarp_vbd.pycovering body partitioning, proxy selection, and the prim-path-string selector path.Type of change
Test plan
./isaaclab.sh -p -m pytest source/isaaclab_contrib/test/deformable/test_proxy_coupled_mjwarp_vbd.pylift_franka_softstate-machine demo and confirm rigid/soft coupling behaves as expected.Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there