Add Newton implicit MPM manager#5697
Conversation
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
🔍 Code Review: Add Newton implicit MPM manager
Thank you for this well-structured addition of implicit MPM support to Isaac Lab! The implementation follows the established manager pattern cleanly. Here are my findings:
✅ Strengths
-
Clean Architecture: The
NewtonMPMManager/MPMSolverCfgpattern mirrors existing managers (Kamino, MJWarp, etc.) and integrates naturally with the baseNewtonManagerclass. -
Thorough Test Coverage: Adding MPM to the existing
test_newton_manager_abstraction.pyensures the new solver path is validated alongside the others. -
Well-Documented Demo: The teapot-pour demo (
newton_cup_pour_mpm.py) is extensive and includes helpful CLI parameters for experimentation. -
Correct Custom Attribute Registration: The
_register_solver_custom_attributeshook ensures MPM-specific particle attributes (mpm:viscosity,mpm:friction, etc.) are registered before particles are added.
🔸 Suggestions
1. Hardcoded Local Path in Demo ✅ RESOLVED
File: scripts/demos/newton_cup_pour_mpm.py
This absolute path will not work for other contributors.
Update: This has been addressed in commit 7c3f4c9. The demo now uses ISAACLAB_ASSETS_DATA_DIR for proper asset resolution, and the teapot USD has been added to isaaclab_assets.
2. Missing NaN/Divergence Checks in MPM Manager (Medium)
File: source/isaaclab_newton/isaaclab_newton/physics/mpm_manager.py
Implicit MPM solvers can silently diverge under certain conditions (e.g., extreme velocities, bad voxel sizing). Consider adding:
- A diagnostic check for
NaNorInfvalues in particle positions/velocities after stepping - Optional logging when iteration limits are hit without convergence
This could be a follow-up enhancement rather than a blocker.
3. Config Attribute Mapping Silently Ignores Unknown Fields (Low)
File: source/isaaclab_newton/isaaclab_newton/physics/mpm_manager_cfg.py, lines 99-105
def to_solver_config(self) -> SolverImplicitMPM.Config:
cfg = SolverImplicitMPM.Config()
for key, value in self.to_dict().items():
if hasattr(cfg, key):
setattr(cfg, key, value)
return cfgFields that exist in MPMSolverCfg but not in SolverImplicitMPM.Config are silently ignored. This could mask typos or version mismatches. Consider logging a warning for unmapped fields, or at minimum documenting this behavior.
4. Demo Particle Seeding Magic Numbers (Low)
File: scripts/demos/newton_cup_pour_mpm.py, lines 595-598
z_lo = mesh_lo[2] + 0.22 * mesh_extent[2] + bottom_clearance
z_hi = mesh_lo[2] + 0.72 * mesh_extent[2] - top_clearanceThe 0.22 and 0.72 fractions appear to be empirically determined for this specific teapot mesh. A brief comment explaining these values (e.g., "skip bottom 22% for teapot base, top 28% for lid/spout area") would help future maintainers.
❓ Questions
-
Test coverage for particle count validation: The
NewtonMPMManager._build_solvercorrectly raises ifmodel.particle_count == 0. Is this exercised by a negative test case, or just covered implicitly via the valid particle test? -
CUDA graph compatibility: Is the implicit MPM solver compatible with CUDA graph capture when
use_cuda_graph=True? The demo defaults to--disable-cuda-graphbeingFalse, suggesting it should work, but this might warrant a note in the docstring.
📋 Summary
This is a solid contribution that follows Isaac Lab conventions well. The main actionable item is the hardcoded teapot asset path, which should be addressed before merge. The other suggestions are improvements that can be tracked separately.
Recommendation: Approve after addressing the hardcoded path issue.
🔄 Update (7c3f4c9)
The author has addressed the main blocking issue:
Changes in this push:
- ✅ Replaced hardcoded teapot path with proper asset resolution via
ISAACLAB_ASSETS_DATA_DIR - ✅ Added the teapot USD asset (
teapot_hollow_separate_lid.usdc) toisaaclab_assets/data/Props/Teapot/ - ✅ Added
.usdcto.gitattributesfor LFS tracking - ✅ Added changelog entry for the new asset
The demo now properly resolves the teapot asset from the package data directory, making it work for all contributors without manual path configuration.
Updated Recommendation: ✅ Approve — The blocking issue has been resolved. Remaining suggestions (NaN checks, unmapped field warnings, magic number comments) are minor improvements that can be addressed in follow-up PRs.
🔄 Update (bee2da2)
The author has pivoted the asset resolution strategy:
Changes in this push:
- 🔄 Asset moved to Nucleus: The teapot USD is now fetched from
ISAACLAB_NUCLEUS_DIR/Mimic/nut_pour_task/nut_pour_assets/instead of being bundled in the repo - 🔄 Uses
retrieve_file_path(): The demo now uses Isaac Lab's asset retrieval utility for proper caching/download - ➖ Removed LFS-tracked asset: The
.usdcfile and LFS entry have been removed from the repo - ➖ Removed changelog entry: Since no new asset is being added to
isaaclab_assets, the changelog entry was removed
Assessment:
This is a cleaner approach — using existing Nucleus-hosted assets avoids bloating the repo with large binary files. The retrieve_file_path() utility handles caching transparently, and the error message now guides users to either upload to Nucleus or provide a local path via --teapot-usd.
The error handling with the explicit from exc chain is a nice touch for debugging.
Updated Recommendation: ✅ Approve — The asset resolution is now properly integrated with Isaac Lab's Nucleus infrastructure. This is the preferred pattern for large assets.
🔄 Update (3d8218a)
No substantive changes since the previous review. The author rebased onto the latest develop branch, which incorporated commits from other PRs. The MPM-specific code remains unchanged and continues to use the Nucleus-hosted teapot asset approach reviewed previously.
Status: ✅ Ready to merge — no additional review findings.
🔄 Update (d3355bc)
Excellent improvements in this push! The author has addressed one of my previous suggestions:
✅ Fixed: Config Attribute Mapping (Suggestion #3)
The to_solver_config() method now explicitly iterates over a whitelist of valid Newton solver fields (_SOLVER_CONFIG_FIELDS) rather than using to_dict(). This is a cleaner solution than logging warnings — it simply ensures only the relevant fields are mapped, and Isaac Lab metadata fields (like class_type, solver_type) are never accidentally forwarded.
Additional improvements:
- 🐛 Fixed variable shadowing:
solver_cfg→solver_cfg_itemin the MPM detection loop - ✨ Added idempotency guard:
has_custom_attribute(\"mpm:young_modulus\")prevents duplicate attribute registration - 🧪 Added two targeted unit tests covering both the field filtering and registration idempotency
Status: ✅ Ready to merge — this is a well-polished contribution.
🔄 Update (684dc9f)
Minor cleanup in the demo script:
- ➖ Removed
--teapot-scaleand--teapot-roll-correction-degCLI arguments - ➖ Removed
_bake_mesh_roll_correction()function and associated logic - 🧹 Simplified
open_teapot_stage()by removing dynamic scale operations
This is a sensible cleanup — the Nucleus-hosted teapot asset is apparently already in the correct orientation and scale, so the manual transformation code is no longer needed. This reduces code complexity without affecting functionality.
Status: ✅ Ready to merge — no new concerns.
🔄 Update (5022487)
Further simplification of the demo's visual scene setup:
Changes in this push:
- ➖ Removed
--table-usd,--bowl-usd,--table-visual-z, and--asset-scaleCLI arguments - ➖ Removed
AssetPathsclass andresolve_asset_paths()function - 🔄 Changed teapot asset path from
teapot_hollow_separate_lid.usdctoteapot.usdc - ✨ Rewrote
setup_kit_scene()to generate table/bowl visuals procedurally via_spawn_collision_shape_visuals()using primitive shapes and mesh generation
Assessment:
This is a nice cleanup that removes external USD dependencies for the table and bowl. By constructing these visuals directly (CuboidCfg for table, generated mesh for bowl), the demo becomes more self-contained and avoids potential Nucleus availability issues for secondary assets. The collision geometry and visual geometry now match exactly, which is cleaner for a physics demo.
The only external asset requirement is now the teapot itself (renamed to teapot.usdc), which is appropriate since that's the centerpiece of the demo.
Status: ✅ Ready to merge — cleaner and more self-contained.
Greptile SummaryThis PR adds Newton implicit MPM simulation support to Isaac Lab, including an
Confidence Score: 3/5The core manager and config classes are well-structured, but the demo ships with a hardcoded developer-local asset path that makes it unusable out of the box for anyone else. The NewtonMPMManager, MPMSolverCfg, and test additions are clean and follow existing patterns. The demo TEAPOT_USD_PATH points to /home/maximiliank/Work/IsaacLab/... — anyone running it without --teapot-usd will hit a FileNotFoundError with no guidance on obtaining the asset. The library code itself is safe; the demo needs the asset path resolved before it can be used by anyone other than the author. scripts/demos/newton_cup_pour_mpm.py — the hardcoded default teapot asset path must be replaced before the demo is usable by anyone other than the author. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant SimContext as SimulationContext
participant NM as NewtonManager
participant NMPM as NewtonMPMManager
participant Builder as ModelBuilder
participant Solver as SolverImplicitMPM
User->>NM: create_builder()
NM->>NM: _register_solver_custom_attributes(builder)
NM-->>User: builder
User->>Builder: add_particles(...)
User->>NM: set_builder(builder)
User->>SimContext: reset()
SimContext->>NM: start_simulation()
NM->>NM: _register_solver_custom_attributes(_builder)
NM->>Builder: finalize(device)
Builder-->>NM: model
NM->>NMPM: _build_solver(model, solver_cfg)
NMPM->>Solver: SolverImplicitMPM(model, config)
Solver-->>NMPM: solver
NMPM->>NM: _solver, _use_single_state, _needs_collision_pipeline
User->>SimContext: step()
SimContext->>NM: step(dt)
NM->>Solver: step(model, state_0, state_0, dt)
Reviews (1): Last reviewed commit: "Add Newton implicit MPM manager" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| BOWL_BASE_POS = (0.22, 0.0, TABLE_TOP_Z + 0.02) | ||
| BOWL_HEIGHT = 0.13 | ||
| BOWL_RIM_Z = BOWL_BASE_POS[2] + BOWL_HEIGHT |
There was a problem hiding this comment.
TEAPOT_USD_PATH is set to /home/maximiliank/Work/IsaacLab/... — a path that exists only on the author's machine. Every other user who runs the demo without --teapot-usd will immediately hit a FileNotFoundError with no guidance on where to obtain the asset. Either publish the asset to a Nucleus path and update this constant, or make --teapot-usd a required argument.
| def to_solver_config(self) -> SolverImplicitMPM.Config: | ||
| """Build a :class:`SolverImplicitMPM.Config` from this configuration. | ||
|
|
||
| Returns: | ||
| A ``SolverImplicitMPM.Config`` instance ready for solver construction. | ||
| """ | ||
| from newton.solvers import SolverImplicitMPM | ||
|
|
||
| cfg = SolverImplicitMPM.Config() | ||
| for key, value in self.to_dict().items(): | ||
| if hasattr(cfg, key): | ||
| setattr(cfg, key, value) | ||
| return cfg |
There was a problem hiding this comment.
to_solver_config iterates over self.to_dict() — which includes class_type, solver_type, and entries inherited from NewtonSolverCfg — and silently sets any field that also exists on SolverImplicitMPM.Config. Currently safe because those names don't appear in Newton's config, but a future Newton Config field with a colliding name would be silently injected with an Isaac Lab value. An explicit exclusion set would make the mapping robust against upstream Newton changes.
| if any(isinstance(solver_cfg, MPMSolverCfg) for solver_cfg in solver_cfgs): | ||
| from newton.solvers import SolverImplicitMPM | ||
|
|
||
| SolverImplicitMPM.register_custom_attributes(builder) |
There was a problem hiding this comment.
register_custom_attributes called multiple times on the same builder
_register_solver_custom_attributes is invoked at three distinct call sites (create_builder, start_simulation, and both builder branches of instantiate_builder_from_stage), so the same ModelBuilder can receive SolverImplicitMPM.register_custom_attributes twice. Correctness currently depends on that call being idempotent. A guard like if not builder.has_custom_attribute("mpm:young_modulus") would make the invocation explicitly safe.
| if any(isinstance(solver_cfg, MPMSolverCfg) for solver_cfg in solver_cfgs): | |
| from newton.solvers import SolverImplicitMPM | |
| SolverImplicitMPM.register_custom_attributes(builder) | |
| if any(isinstance(solver_cfg, MPMSolverCfg) for solver_cfg in solver_cfgs): | |
| from newton.solvers import SolverImplicitMPM | |
| if not builder.has_custom_attribute("mpm:young_modulus"): | |
| SolverImplicitMPM.register_custom_attributes(builder) |
| solver_cfg = getattr(cfg, "solver_cfg", None) | ||
| solver_cfgs = [solver_cfg] | ||
| solver_cfgs.extend(getattr(entry, "solver_cfg", entry) for entry in getattr(solver_cfg, "entries", ()) or ()) | ||
| from .mpm_manager_cfg import MPMSolverCfg | ||
|
|
||
| if any(isinstance(solver_cfg, MPMSolverCfg) for solver_cfg in solver_cfgs): |
There was a problem hiding this comment.
solver_cfg is used both as the outer local variable and as the iteration variable inside the any(...) generator expression. Python 3 generator expressions carry their own scope so this is not a runtime bug, but a reader skimming the function may expect the isinstance check to test only the single top-level solver_cfg rather than every element of solver_cfgs.
| solver_cfg = getattr(cfg, "solver_cfg", None) | |
| solver_cfgs = [solver_cfg] | |
| solver_cfgs.extend(getattr(entry, "solver_cfg", entry) for entry in getattr(solver_cfg, "entries", ()) or ()) | |
| from .mpm_manager_cfg import MPMSolverCfg | |
| if any(isinstance(solver_cfg, MPMSolverCfg) for solver_cfg in solver_cfgs): | |
| top_solver_cfg = getattr(cfg, "solver_cfg", None) | |
| solver_cfgs = [top_solver_cfg] | |
| solver_cfgs.extend(getattr(entry, "solver_cfg", entry) for entry in getattr(top_solver_cfg, "entries", ()) or ()) | |
| from .mpm_manager_cfg import MPMSolverCfg | |
| if any(isinstance(sc, MPMSolverCfg) for sc in solver_cfgs): |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
Need to add USD asset to Nucleus |
maxkra15
force-pushed
the
max/newton-mpm-manager
branch
from
bee2da2 to
3d8218a
Compare
1 participant
Description
Adds initial Isaac Lab support for Newton implicit MPM simulations.
This PR adds:
MPMSolverCfg, an Isaac Lab config wrapper for Newton'sSolverImplicitMPM.Config.NewtonMPMManager, a physics manager specialization for standalone implicit MPM simulations.NewtonManagerbefore builder/model finalization.isaaclab_newton.The MPM manager does not use Isaac Lab's rigid-body external collision pipeline because Newton's implicit MPM solver handles particle/collider
interaction internally.
Dependencies:
newton[sim] @ git+https://github.com/newton-physics/newton.git@v1.2.0.Fixes # N/A
Type of change
GIF
Validation
Ran formatting checks:
Checklist
pre-commitchecks with./isaaclab.sh --format