Exposes Newton collision pipeline configuration through NewtonCfg#5219
Conversation
Greptile SummaryThis PR adds Confidence Score: 5/5Safe to merge — the only finding is a minor style preference on a truthiness check that works correctly in practice. All remaining findings are P2 style suggestions. The logic for building CollisionPipeline from the configclass, the mutual-exclusion guard between use_mujoco_contacts and collision_cfg, the clear/reset paths, and the public API exports are all implemented correctly. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[NewtonCfg] -->|solver_cfg| B{SolverMuJoCo?}
A -->|collision_cfg| C[NewtonCollisionPipelineCfg]
C --> C1[broad_phase]
C --> C2[reduce_contacts]
C --> C3[rigid_contact_max]
C --> C4[max_triangle_pairs]
C --> C5[soft_contact_*]
C --> C6[sdf_hydroelastic_config]
C6 -->|None| G[Hydroelastic disabled]
C6 -->|HydroelasticSDFCfg| H[HydroelasticSDF.Config]
B -->|Yes + use_mujoco_contacts=True| D{collision_cfg set?}
D -->|Yes| E[ValueError raised]
D -->|No| F[MuJoCo internal contacts]
B -->|Yes + use_mujoco_contacts=False| I[Newton CollisionPipeline]
B -->|No: XPBD / Featherstone| I
C -->|to_dict + HydroelasticSDF.Config| I
I --> J[CollisionPipeline.collide on each step]
Reviews (1): Last reviewed commit: "line break" | Re-trigger Greptile |
| if hydro_cfg: | ||
| cfg_dict["sdf_hydroelastic_config"] = HydroelasticSDF.Config(**hydro_cfg) |
There was a problem hiding this comment.
is not None over truthiness check on dict
if hydro_cfg: is falsy for an empty dict {}. While HydroelasticSDFCfg.to_dict() will never produce an empty dict in practice (it always has 6 fields), an explicit is not None guard makes the intent clearer and is more robust against future changes to the configclass.
| if hydro_cfg: | |
| cfg_dict["sdf_hydroelastic_config"] = HydroelasticSDF.Config(**hydro_cfg) | |
| if hydro_cfg is not None: | |
| cfg_dict["sdf_hydroelastic_config"] = HydroelasticSDF.Config(**hydro_cfg) |
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!
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
This PR adds a new NewtonCollisionPipelineCfg configclass to expose Newton's CollisionPipeline parameters (broad phase, contact limits, hydroelastic mode) through NewtonCfg.collision_cfg. It also adds a MJWarpSolverCfg.tolerance field and cleans up some solver config access patterns. The design is sound and follows established Isaac Lab config patterns. A few implementation details need attention.
Design Assessment
Design is sound. The collision_cfg placement on NewtonCfg (rather than on individual solver configs) is the right level — collision pipeline configuration is orthogonal to solver choice. The mutual exclusivity validation with use_mujoco_contacts=True is appropriate and well-documented. The HydroelasticSDFCfg nested config mirrors Newton's own HydroelasticSDF.Config structure cleanly.
The removal of the defensive hasattr/isinstance(dict)/getattr pattern for accessing solver_cfg.use_mujoco_contacts (replaced with direct attribute access) is a good simplification — the config is always a typed dataclass at this point.
Findings
🟡 Warning: Falsy check on hydro_cfg dict may skip explicit default config — newton_manager.py:541
hydro_cfg = cfg_dict.pop("sdf_hydroelastic_config", None)
if hydro_cfg:
cfg_dict["sdf_hydroelastic_config"] = HydroelasticSDF.Config(**hydro_cfg)The if hydro_cfg: check uses truthiness. If configclass.to_dict() ever returns an empty dict for HydroelasticSDFCfg() (e.g. if a future to_dict() implementation filters out fields matching defaults), a user who explicitly sets sdf_hydroelastic_config=HydroelasticSDFCfg() to enable hydroelastic mode with all defaults would silently get no hydroelastic contacts. Consider using if hydro_cfg is not None: for an explicit None check:
if hydro_cfg is not None:
This is defensive and costs nothing.
🟡 Warning: MJWarpSolverCfg.tolerance added without changelog mention — newton_manager_cfg.py:128
The tolerance field is a user-facing addition to MJWarpSolverCfg but the CHANGELOG.rst entry only mentions NewtonCollisionPipelineCfg. Since this changes the default solver behavior (users can now tune convergence tolerance), it should be documented in the changelog:
* Added :attr:`~isaaclab_newton.physics.MJWarpSolverCfg.tolerance` for solver convergence control.
🟡 Warning: collision_cfg validation only runs for MuJoCo solver path — newton_manager.py:617-622
The mutual exclusivity check (collision_cfg + use_mujoco_contacts=True → error) is good, but there's no validation for non-MuJoCo solvers. If a user passes an invalid broad_phase value (e.g. a typo like "sapp"), the error would surface deep inside CollisionPipeline.__init__ during _initialize_contacts() rather than at config validation time. Consider adding an __post_init__ validator on NewtonCollisionPipelineCfg or at least a note that invalid values will error at pipeline construction time. This is minor since the Literal type hint provides static checking, but runtime validation would help users who don't use type checkers.
🔵 Suggestion: collision_cfg on NewtonCfg is accessible for non-MuJoCo solver configs that don't have collision_cfg — newton_manager_cfg.py:243
This is a design observation, not an issue. Since collision_cfg is on NewtonCfg (the top-level physics config), it applies to all solver types. The docstring correctly documents which solver configurations activate the collision pipeline. The design is correct — just confirming it's intentional and well-documented.
🔵 Suggestion: Consider adding __all__ to newton_collision_cfg.py — newton_collision_cfg.py
The new module doesn't define __all__. While the .pyi stub correctly exports both HydroelasticSDFCfg and NewtonCollisionPipelineCfg, adding __all__ to the source module would be consistent with other modules in the package. This is a minor style suggestion.
Test Coverage
Coverage Summary: The PR author has explicitly left the tests checkbox unchecked ("I have added tests that prove my fix is effective or that my feature works"). No test files are included in this PR.
🟡 Missing: Config validation tests — Criticality: 6/10
- What: Tests that
NewtonCollisionPipelineCfgandHydroelasticSDFCfgcan be instantiated with valid values, serialized viato_dict(), and round-tripped correctly. - Why: Config serialization is critical for the
to_dict()→CollisionPipeline(**cfg_dict)path. A serialization issue would cause silent misconfiguration.
🟡 Missing: Mutual exclusivity validation test — Criticality: 6/10
- What: Test that setting
collision_cfgwithuse_mujoco_contacts=TrueraisesValueError. - Why: This is an important user-facing validation that could regress if the initialization order changes.
🟡 Missing: Integration test with collision pipeline — Criticality: 5/10
- What: Test that
NewtonCfg(solver_cfg=MJWarpSolverCfg(use_mujoco_contacts=False), collision_cfg=NewtonCollisionPipelineCfg())correctly creates aCollisionPipelinewith the specified parameters. - Why: The
to_dict()→ constructor unpacking path is the core functionality and should be verified.
Verdict:
- Feature PR: Coverage — No (no tests included)
- The PR would benefit from at minimum the config validation and mutual exclusivity tests. Integration tests are harder without a full Newton runtime but config-level tests should be straightforward.
CI Status
- pre-commit: ✅ pass
- license-check: ✅ pass
- Build Latest Docs: ✅ pass
- Check for Broken Links: ✅ pass
- Build Base Docker Image: ⏳ pending
Verdict
COMMENT
This is a well-structured feature addition with clean config design, good documentation, and appropriate validation. The main areas for improvement are: (1) the falsy dict check on hydro_cfg should use is not None, (2) the tolerance field should be mentioned in the changelog, and (3) tests should be added for config validation and the mutual exclusivity check. None of these are blocking, but addressing them would strengthen the PR.
Review by Isaac Lab Review Bot — [Expert Analysis] + [Error Handling Audit] + [Test Coverage Check]
| if cls._collision_pipeline is None: | ||
| cls._collision_pipeline = CollisionPipeline(cls._model, broad_phase="explicit") | ||
| if cls._collision_cfg is not None: | ||
| cfg_dict = cls._collision_cfg.to_dict() |
There was a problem hiding this comment.
🟡 Warning: The if hydro_cfg: truthiness check could skip an explicitly set HydroelasticSDFCfg() with all defaults if to_dict() ever returns an empty dict. Consider if hydro_cfg is not None: for safety.
| The solver iterates until the residual drops below this threshold or | ||
| ``iterations`` is reached. Lower values give more precise constraint | ||
| satisfaction at the cost of more iterations. MuJoCo default is ``1e-8``; | ||
| Newton default is ``1e-6``. |
There was a problem hiding this comment.
🟡 Warning: This new tolerance field is a user-facing addition but isn't mentioned in the CHANGELOG.rst entry. Consider adding it to the 0.5.11 changelog.
|
Clean, minimal PR that exposes Newton's Specific feedback
|
Newton HydroelasticSDF.Config field |
Exposed? |
|---|---|
reduce_contacts |
Yes |
buffer_fraction |
Yes |
normal_matching |
Yes |
anchor_contact |
Yes |
margin_contact_area |
Yes |
output_contact_surface |
Yes |
pre_prune_contacts |
No |
moment_matching |
No |
buffer_mult_broad |
No |
buffer_mult_iso |
No |
buffer_mult_contact |
No |
contact_buffer_fraction |
No |
grid_size |
No |
Of these, moment_matching and the buffer_mult_* multipliers are practically important — moment_matching affects friction accuracy in contact reduction, and buffer_mult_* are the primary knobs users reach for when they hit overflow warnings. Suggest adding at least moment_matching, buffer_mult_broad, buffer_mult_iso, buffer_mult_contact, and grid_size.
Since HydroelasticSDF.Config has all 13 params, exposing only 6 means users have to fall back to constructing HydroelasticSDF.Config directly for advanced tuning, bypassing the config system.
_initialize_contacts — to_dict() may include None values
cfg_dict = cls._collision_cfg.to_dict()
hydro_cfg = cfg_dict.pop("sdf_hydroelastic_config", None)
if hydro_cfg:
cfg_dict["sdf_hydroelastic_config"] = HydroelasticSDF.Config(**hydro_cfg)
cls._collision_pipeline = CollisionPipeline(cls._model, **cfg_dict)If to_dict() includes fields with None values (e.g. rigid_contact_max=None, soft_contact_max=None, requires_grad=None), passing them as **cfg_dict works because CollisionPipeline.__init__ accepts None for those params. But worth verifying that to_dict() doesn't serialize None fields differently than expected (some configclass implementations skip None fields, others include them).
tolerance field on MJWarpSolverCfg — unrelated to this PR?
The PR adds a tolerance: float = 1e-6 field to MJWarpSolverCfg. This doesn't seem related to collision pipeline configuration. If it's intentional, it should be in a separate commit or mentioned in the PR description. If it slipped in, consider removing to keep the PR focused.
No tests
The PR checklist shows tests unchecked. Even a basic test that creates a NewtonCollisionPipelineCfg, sets collision_cfg on NewtonCfg, and verifies the CollisionPipeline is created with the right params would catch regressions. The to_dict() -> CollisionPipeline(**cfg_dict) path is the most important thing to test.
|
Hi @ooctipus, I have some more feedback on this PR. Maybe you already planned to address it on separete PR but it's worth noting a potential issue on the setup of SDF/Hydroelastic shapes. ContextPR #5219 correctly exposes The ProblemFor hydroelastic contacts to activate, three things must happen before
With PR #5219 alone, setting Suggested Feedback1. Add a warning when hydroelastic config has no effectIn if cls._collision_cfg is not None:
cfg_dict = cls._collision_cfg.to_dict()
hydro_cfg = cfg_dict.pop("sdf_hydroelastic_config", None)
if hydro_cfg:
cfg_dict["sdf_hydroelastic_config"] = HydroelasticSDF.Config(**hydro_cfg)
cls._collision_pipeline = CollisionPipeline(cls._model, **cfg_dict)
# Warn if hydroelastic was requested but no shapes qualify
if hydro_cfg and cls._collision_pipeline.hydroelastic_sdf is None:
logger.warning(
"HydroelasticSDFCfg was set but no hydroelastic shape pairs were found. "
"Ensure shapes have SDF built (mesh.build_sdf()) and is_hydroelastic=True "
"in their ShapeConfig. Falling back to point contacts."
)2. Document the prerequisite in
|
Add NewtonCollisionPipelineCfg and HydroelasticSDFCfg to expose Newton's collision pipeline parameters (broad phase, contact limits, hydroelastic mode) through NewtonCfg.collision_cfg. Add MJWarpSolverCfg.tolerance for solver convergence control. Fix validation order so collision_cfg is stored before the use_mujoco_contacts consistency check runs. Reset _collision_cfg in clear() to avoid stale state across reset cycles. Fall back to a default CollisionPipeline when collision_cfg is None.
Move config resolution out of NewtonManager into NewtonCollisionPipelineCfg.to_pipeline_args(), following the Kamino to_solver_config() pattern. Fix truthiness check on hydroelastic config dict to use explicit `is not None`. Add missing changelog entry for MJWarpSolverCfg.tolerance. Use specific return type hint dict[str, Any].
2a269b6 to
9191147
Compare
Description
Expose Newton's
CollisionPipelineparameters through a newNewtonCollisionPipelineCfgconfigclass, allowing users to customize broad phase, contact limits, and hydroelastic mode viaNewtonCfg.collision_cfg.Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there