Adding Hydroelastic and SDF collision configuration to IsaacLab#5160
Adding Hydroelastic and SDF collision configuration to IsaacLab#5160vidurv-nvidia wants to merge 11 commits intoisaac-sim:developfrom
Conversation
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot
Summary
This PR adds SDF (Signed Distance Field) and hydroelastic collision configuration to the Newton physics backend. It introduces three new config dataclasses (SDFCfg, HydroelasticCfg, NewtonCollisionPipelineCfg), integrates SDF mesh building into the model builder pipeline, modifies the cloner to preserve triangle meshes for SDF-targeted shapes, and creates a configurable collision pipeline factory. The overall design is sound but there is one bug that will crash at runtime when collision_cfg is provided.
Design Assessment
Problem: Newton backend needs SDF-based mesh collision and hydroelastic contacts, requiring pre-built SDF on mesh geometry and a configurable collision pipeline.
The PR places SDF config application at the right lifecycle phase — NewtonManager._apply_sdf_config() operates on the ModelBuilder before finalize(), which is correct. Pattern-based matching (regex on body/shape labels) is consistent with how other Newton features configure per-shape properties. The cloner integration to skip convex hull approximation for SDF shapes is architecturally necessary since approximation destroys source geometry needed for mesh.build_sdf().
The coupling between the cloner and PhysicsManager._cfg is a pragmatic tradeoff — threading the SDF config as an explicit parameter would be cleaner but would require API changes to newton_physics_replicate() and all its callers.
Design is sound.
Architecture Impact
New fields collision_cfg and sdf_cfg on NewtonCfg default to None. All access uses getattr(cfg, "sdf_cfg", None), so existing configs without these attributes are safe. The collision pipeline creation path changes from hardcoded to configurable, but default behavior ("explicit" broad phase) is preserved. No cross-module breakage.
Implementation Verdict
Significant concerns — 1 bug that will crash at runtime when collision_cfg is used with default None fields, plus a misleading docstring and minor code quality issues.
Test Coverage
The test file (test_sdf_config.py) has 12 well-structured tests covering _build_sdf_on_mesh and _apply_sdf_config. Good use of mocking and clear assertions. Missing coverage:
_create_collision_pipelineis untested (where the bug is)_create_sdf_collision_from_visualis untested- Hydroelastic flag application path in
_apply_sdf_configis untested - Cloner SDF skip logic in
newton_replicate.pyis untested
A test for _create_collision_pipeline with default config would have caught the None kwargs bug.
CI Status
Only labeler has run (✅). No pre-commit, linter, or test CI checks have executed yet.
Findings
🔴 Critical: newton_manager.py:570 — to_dict() passes None values to CollisionPipeline, will crash at runtime
🟡 Warning: newton_manager_cfg.py:111 — target_voxel_size docstring claims precedence that code doesn't enforce
🟡 Warning: newton_replicate.py:70 — import re belongs at module level
🔵 Improvement: test_sdf_config.py — Missing tests for _create_collision_pipeline, _create_sdf_collision_from_visual, hydroelastic flags, and cloner skip logic
source/isaaclab_newton/isaaclab_newton/physics/newton_manager.py
Outdated
Show resolved
Hide resolved
source/isaaclab_newton/isaaclab_newton/cloner/newton_replicate.py
Outdated
Show resolved
Hide resolved
Greptile SummaryThis PR adds three new configuration classes ( Key changes:
Confidence Score: 4/5Safe to merge after addressing the One P1 defect:
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[NewtonCfg with sdf_cfg / collision_cfg] --> B{Replicate path?}
B -- Yes --> C[_build_newton_builder_from_mapping]
C --> D{has_sdf_patterns?}
D -- Yes --> E[Filter SDF shapes from convex-hull approx]
D -- No --> F[approximate_meshes all MESH shapes]
E --> G[approximate_meshes non-SDF shapes only]
F --> H[_apply_sdf_config per prototype]
G --> H
H --> I[set_builder global_builder]
B -- No --> J[start_simulation]
J --> K{_builder is None?}
K -- Yes --> L[instantiate_builder_from_stage]
L --> M[_apply_sdf_config builder]
M --> N[set_builder]
K -- No --> O[builder already set - SDF already applied]
N --> P[builder.finalize Model]
O --> P
I --> J
P --> Q[initialize_solver]
Q --> R{needs_collision_pipeline?}
R -- Yes --> S[_create_collision_pipeline]
S --> T{hydro_cfg?}
T -- Yes --> U[CollisionPipeline with HydroelasticSDF.Config]
T -- No --> V[CollisionPipeline with pipeline_kwargs]
R -- No --> W[MuJoCo native contacts]
Reviews (2): Last reviewed commit: "Raise TypeError for invalid collision_cf..." | Re-trigger Greptile |
| # Pipeline parameters from collision_cfg (or defaults) | ||
| collision_cfg = getattr(cfg, "collision_cfg", None) | ||
| if collision_cfg is not None: | ||
| pipeline_kwargs = {k: v for k, v in collision_cfg.to_dict().items() if v is not None} if hasattr(collision_cfg, "to_dict") else {} |
There was a problem hiding this comment.
Silent drop of
collision_cfg parameters when to_dict() is absent
When collision_cfg is not None but the hasattr(collision_cfg, "to_dict") guard returns False, pipeline_kwargs is set to {} and CollisionPipeline(model) is instantiated with no parameters. The user's NewtonCollisionPipelineCfg settings (broad_phase, reduce_contacts, max_triangle_pairs, etc.) are silently ignored.
configclass in IsaacLab should always expose to_dict(), so this case is unlikely in practice. However, the defensive else: {} fallback creates a confusing failure mode: instead of raising a clear AttributeError, the pipeline silently reverts to Newton defaults, which may differ from what was configured.
Consider either removing the guard (and letting an AttributeError surface a missing method) or explicitly raising:
if collision_cfg is not None:
if not hasattr(collision_cfg, "to_dict"):
raise TypeError(
f"collision_cfg {type(collision_cfg)} does not implement to_dict(). "
"Only @configclass instances are supported."
)
pipeline_kwargs = {k: v for k, v in collision_cfg.to_dict().items() if v is not None}
else:
pipeline_kwargs = {"broad_phase": "explicit"}| shape_cfg_kwargs = dict( | ||
| density=0.0, | ||
| has_shape_collision=True, | ||
| has_particle_collision=True, | ||
| is_visible=True, | ||
| ) | ||
| if sdf_cfg.margin is not None: | ||
| shape_cfg_kwargs["margin"] = sdf_cfg.margin | ||
| if hydro_cfg is not None: | ||
| shape_cfg_kwargs["is_hydroelastic"] = True | ||
| shape_cfg_kwargs["kh"] = hydro_cfg.k_hydro |
There was a problem hiding this comment.
Newly-created collision shape is also marked visible, rendering collision geometry
When use_visual_meshes=True, a new collision shape is added with is_visible=True. This means the collision mesh will be rendered on top of the existing visual mesh, resulting in duplicated (and likely z-fighting) geometry in the viewport.
Unless the intent is to explicitly show collision shapes for debugging purposes, the collision shape should not be visible:
| shape_cfg_kwargs = dict( | |
| density=0.0, | |
| has_shape_collision=True, | |
| has_particle_collision=True, | |
| is_visible=True, | |
| ) | |
| if sdf_cfg.margin is not None: | |
| shape_cfg_kwargs["margin"] = sdf_cfg.margin | |
| if hydro_cfg is not None: | |
| shape_cfg_kwargs["is_hydroelastic"] = True | |
| shape_cfg_kwargs["kh"] = hydro_cfg.k_hydro | |
| shape_cfg_kwargs = dict( | |
| density=0.0, | |
| has_shape_collision=True, | |
| has_particle_collision=True, | |
| is_visible=False, | |
| ) |
If visualisation of SDF collision shapes is a desired feature, it should be opt-in and documented in SDFCfg.
There was a problem hiding this comment.
✅ Addressed in caca3c3 — now correctly set to .
| # Create builder from USD stage if not provided | ||
| if cls._builder is None: | ||
| cls.instantiate_builder_from_stage() | ||
| else: | ||
| # Builder was set externally (e.g. by newton_replicate) — still apply SDF | ||
| cls._apply_sdf_config(cls._builder) | ||
|
|
There was a problem hiding this comment.
Double
_apply_sdf_config call when instantiate_builder_from_stage is invoked directly
instantiate_builder_from_stage is a public class method that already calls _apply_sdf_config(builder) before set_builder(builder). If a caller invokes instantiate_builder_from_stage() first (setting cls._builder), and start_simulation is then called, cls._builder is not None triggers the else branch and _apply_sdf_config runs a second time on the already-patched builder.
The consequences:
- Every matching collision mesh has
clear_sdf()+build_sdf()called again (wasted GPU/CPU work). - With
use_visual_meshes=True,_create_sdf_collision_from_visualis re-entered; the second pass gracefully skips adding duplicate shapes (bodies already have collision shapes), but the SDF is rebuilt on the newly-created shapes from the first pass.
The cleanest fix is to make _apply_sdf_config idempotency-safe by tracking whether it has already run, or to guard the call in start_simulation by verifying it has not been applied yet.
A simpler option: move _apply_sdf_config out of instantiate_builder_from_stage and call it only from start_simulation (in both branches), or mark instantiate_builder_from_stage as internal with a leading underscore.
| if body_patterns is not None: | ||
| for body_idx in range(len(builder.body_label)): | ||
| if any(p.search(builder.body_label[body_idx]) for p in body_patterns): | ||
| for si in range(builder.shape_count): | ||
| if builder.shape_body[si] == body_idx and builder.shape_type[si] == GeoType.MESH: | ||
| sdf_shape_indices.add(si) |
There was a problem hiding this comment.
O(bodies × shapes) nested scan when collecting SDF shape indices by body pattern
For each matched body, the code performs a full linear scan over all shapes (range(builder.shape_count)). For a large model with many bodies and shapes this is O(B·S) at startup. Building a reverse index (body_idx → [shape_indices]) once before the loop would reduce this to O(S) total:
# Build reverse map once
body_to_shapes: dict[int, list[int]] = {}
for si in range(builder.shape_count):
if builder.shape_type[si] == GeoType.MESH:
body_to_shapes.setdefault(builder.shape_body[si], []).append(si)
if body_patterns is not None:
for body_idx in range(len(builder.body_label)):
if any(p.search(builder.body_label[body_idx]) for p in body_patterns):
sdf_shape_indices.update(body_to_shapes.get(body_idx, []))This also avoids the redundant shape_type check inside the inner loop.
There was a problem hiding this comment.
Addressed in caca3c3 — reverse map body_to_shapes implemented as suggested, eliminating the O(B*S) scan for both body and shape pattern matching.
| # Build SDF on prototype before add_builder copies it N times. | ||
| # Mesh objects are shared by reference, so SDF is built once and | ||
| # all environments inherit it. | ||
| NewtonManager._apply_sdf_config(p) |
There was a problem hiding this comment.
_apply_sdf_config crashes when PhysicsManager._cfg is None
_apply_sdf_config(p) is called unconditionally for every prototype here, but inside _apply_sdf_config, the first two lines are:
cfg = PhysicsManager._cfg
sdf_cfg = cfg.sdf_cfg # AttributeError if cfg is NoneThere is no None guard on cfg. Meanwhile, the code added just above in this same function (lines 77–78) explicitly guards against cfg being None:
cfg = PhysicsManager._cfg
sdf_cfg = getattr(cfg, "sdf_cfg", None) if cfg is not None else NoneThis inconsistency means: in the exact scenario the replicate code considers valid (where PhysicsManager._cfg is None), calling _apply_sdf_config(p) unconditionally will raise AttributeError: 'NoneType' object has no attribute 'sdf_cfg'.
The simplest fix is to add the same guard inside _apply_sdf_config:
@classmethod
def _apply_sdf_config(cls, builder: ModelBuilder):
"""Apply SDF collision and optional hydroelastic flags to matching mesh shapes."""
from newton import GeoType, ShapeFlags
cfg = PhysicsManager._cfg
if cfg is None:
return
sdf_cfg = cfg.sdf_cfg
if sdf_cfg is None:
return
...Co-authored-by: isaaclab-review-bot[bot] <270793704+isaaclab-review-bot[bot]@users.noreply.github.com> Signed-off-by: vidurv-nvidia <vidurv@nvidia.com>
When collision_cfg is not None but lacks to_dict(), the previous code silently fell back to empty kwargs, ignoring all user settings. Now raises a clear TypeError so misconfiguration surfaces immediately.
The cloner calls _apply_sdf_config unconditionally on each prototype. If PhysicsManager._cfg is None at that point, cfg.sdf_cfg would raise AttributeError. Add an early return when cfg is None, matching the guard already used in the cloner's pattern compilation code.
22d6673 to
b7c2c76
Compare
Description
Adds SDF (Signed Distance Field) collision and hydroelastic contact support to Newton's IsaacLab integration. Since Newton cannot read SDF designation from USD schemas, SDF is configured entirely through runtime config and applied at the
ModelBuilderlevel before model finalization.New config classes
SDFCfg— configures SDF mesh collision via Newton'smesh.build_sdf()API:max_resolution/target_voxel_size— SDF grid resolutionbody_patterns/shape_patterns— regex patterns to select which bodies/shapes get SDF (matching againstbody_labelandshape_labelrespectively)pattern_resolutions— per-pattern resolution overridesuse_visual_meshes— optionally create collision shapes from visual meshes for bodies lacking collision geometryhydroelastic_cfg— nested hydroelastic contact configurationHydroelasticCfg— configures distributed surface contacts via SDF overlap:k_hydro,reduce_contacts,normal_matching,moment_matching, buffer multipliers, etc.NewtonCollisionPipelineCfg— configures Newton's collision pipeline parameters:broad_phase,reduce_contacts,rigid_contact_max,max_triangle_pairs, etc.Usage on
NewtonCfg:Implementation
_build_sdf_on_mesh— helper that builds SDF on a mesh with per-pattern resolution overrides_apply_sdf_config— collects matching shapes via body/shape patterns, patches collision meshes with SDF, optionally creates collision from visual meshes_create_sdf_collision_from_visual— creates collision shapes from visual meshes for matched bodies that lack collision geometry_create_collision_pipeline— constructsCollisionPipelinefromcollision_cfg+ hydroelastic config fromsdf_cfgadd_builderreplication) so it is built once and shared across all environmentsinstantiate_builder_from_stageinitialize_solverforces Newton collision pipeline whensdf_cfgorcollision_cfgis configuredCloner integration
Shapes matching SDF patterns are excluded from convex hull approximation in
_build_newton_builder_from_mapping, preserving original triangle meshes formesh.build_sdf().Type of change
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there