Add Newton SDF collision schema config#5926
Conversation
Signed-off-by: Maximilian Krause <maximiliank@nvidia.com>
There was a problem hiding this comment.
Review: Add Newton SDF collision schema config
Overall this is a clean, well-structured addition. The new NewtonSDFCollisionPropertiesCfg integrates smoothly into the existing schema hierarchy and the shim/export paths are wired correctly. Tests and docs are comprehensive.
Findings
1. Minor robustness concern in test helper _has_authored_api_schema (low severity)
In test_newton_schemas.py, the helper iterates over ("explicitItems", "prependedItems", "appendedItems", "addedItems") via getattr. If the SdfListOp variant returned by GetMetadata("apiSchemas") doesn't have one of these attributes, an AttributeError would be raised. Consider guarding with hasattr:
return any(
schema_name in getattr(api_schemas, item_list, [])
for item_list in ("explicitItems", "prependedItems", "appendedItems", "addedItems")
)2. No bounds validation on physical fields (informational)
Fields like sdf_max_resolution, sdf_target_voxel_size, sdf_padding, and hydroelastic_stiffness accept any numeric value including negatives or zero. While this is consistent with the existing PropertiesCfg pattern in the codebase (validation happens at the Newton runtime level), consider documenting acceptable ranges in the docstrings — especially since sdf_max_resolution "must be divisible by 8" per the docstring but this constraint isn't enforced.
3. sdf_max_resolution divisibility-by-8 constraint (suggestion)
The docstring states "Newton requires this value to be divisible by 8". A __post_init__ check (or a note that Newton's importer raises if violated) would help users catch misconfigurations early. Not blocking, but would improve DX.
4. Formatting-only changes inflate the diff (nit)
The frozenset({...}) → multi-line reformatting in sim/__init__.py and sim/schemas/__init__.py is style-only (presumably from ruff format). This is fine but makes the diff harder to review. In future PRs, separating formatter-driven changes from functional ones helps reviewers.
5. Test coverage is solid ✓
Three test cases covering:
- Full attribute write + API schema application
- Base-only fields don't trigger SDF schema
- All-None config applies no schema
This covers the key behavioral contract. The shim tests also correctly verify Newton forwarding end-to-end.
Summary
| Aspect | Status |
|---|---|
| API design | ✅ Clean single-inheritance from NewtonCollisionPropertiesCfg |
| USD schema wiring | ✅ _usd_namespace, _usd_applied_schema correctly set |
| Shim exports | ✅ All 4 export paths updated consistently |
| Type safety | ✅ Literal type for texture format |
| Tests | ✅ Comprehensive |
| Docs | ✅ RST + hierarchy diagram updated |
| CI | ⏳ Some checks still pending |
Verdict: Looks good. The suggestions above are non-blocking improvements. Nice work! 👍
Update (6c6b2fd): Reviewed 1 new commit. This is a documentation-only fix to schema_cfgs.rst that adds the missing NewtonMeshCollisionPropertiesCfg entry to the collision schema hierarchy diagram, making it consistent with the actual class structure. The tree now correctly shows both NewtonMeshCollisionPropertiesCfg and NewtonSDFCollisionPropertiesCfg as children of NewtonCollisionPropertiesCfg. LGTM — no concerns with this change.
Greptile SummaryIntroduces
Confidence Score: 4/5The PR is safe to merge; all changes are additive and well-isolated. The main area worth a second look is how sdf_texture_format is written to USD. The implementation is consistent with all existing Newton schema patterns and the shim forwarding is correct across all four export paths. Two non-blocking concerns exist: sdf_texture_format is written via safe_set_attribute_on_usd_prim which creates String-typed attributes for Python str values — if Newton's USD importer expects a Token-typed attribute, this would silently produce the wrong Sdf type (the tests only verify the value, not the type); and the documented divisibility-by-8 requirement on sdf_max_resolution has no enforcement, so an invalid value is not caught until Newton import time. source/isaaclab_newton/isaaclab_newton/sim/schemas/schemas_cfg.py — specifically the sdf_texture_format field routing and the missing sdf_max_resolution guard. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["NewtonSDFCollisionPropertiesCfg(...)"] --> B["define_collision_properties(prim_path, cfg)"]
B --> C["UsdPhysics.CollisionAPI.Apply(prim)"]
B --> D["modify_collision_properties(prim_path, cfg)"]
D --> E["_apply_namespaced_schemas(prim, cfg, cfg_dict)"]
E --> F{"Per-declaring-class MRO routing"}
F -->|"CollisionBaseCfg fields\ncollision_enabled"| G["physics:* namespace\nno schema applied"]
F -->|"NewtonCollisionPropertiesCfg fields\ncontact_margin, contact_gap"| H["newton:* namespace\nAddAppliedSchema NewtonCollisionAPI"]
F -->|"NewtonSDFCollisionPropertiesCfg fields\nsdf_max_resolution, sdf_texture_format, etc."| I["newton:* namespace\nAddAppliedSchema NewtonSDFCollisionAPI"]
H --> J["safe_set_attribute_on_usd_prim\nnewton:contactMargin, newton:contactGap"]
I --> K["safe_set_attribute_on_usd_prim\nnewton:sdfMaxResolution, newton:sdfTextureFormat\nnewton:hydroelasticEnabled, etc."]
Reviews (1): Last reviewed commit: "Add Newton SDF collision schema config" | Re-trigger Greptile |
| sdf_texture_format: Literal["uint8", "uint16", "float32"] | None = None | ||
| """Subgrid texture storage format for generated SDFs. | ||
|
|
||
| Written to ``newton:sdfTextureFormat`` via ``NewtonSDFCollisionAPI``. | ||
| """ |
There was a problem hiding this comment.
sdf_texture_format may be authored as a String attribute instead of a Token
safe_set_attribute_on_usd_prim maps Python str values to Sdf.ValueTypeNames.String (see prims.py:371-372). If NewtonSDFCollisionAPI defines newton:sdfTextureFormat as a token-typed attribute (the typical USD convention for enumerated choices), the attribute will be created with the wrong Sdf type when it does not already exist on the prim. Newton's importer may then silently ignore or misread it. The tests only assert prim.GetAttribute("newton:sdfTextureFormat").Get() == "float32", which passes for both String and Token typed attributes and does not catch the mismatch. Consider routing sdf_texture_format via _usd_field_exceptions with an explicit Token-typed write path (as drive_type is handled for UsdPhysics.DriveAPI), or add an assertion on GetTypeName() in the test.
| sdf_max_resolution: int | None = None | ||
| """Maximum SDF grid dimension. | ||
|
|
||
| Newton requires this value to be divisible by 8. If | ||
| :attr:`sdf_target_voxel_size` is also authored, Newton uses the target voxel | ||
| size and ignores this resolution. | ||
| Written to ``newton:sdfMaxResolution`` via ``NewtonSDFCollisionAPI``. | ||
| """ |
There was a problem hiding this comment.
The docstring explicitly states "Newton requires this value to be divisible by 8", but there is no runtime guard. A caller who passes
sdf_max_resolution=65 will silently write an invalid value to USD and only discover the error at Newton import time. A cheap __post_init__ check would surface the problem at config construction time where the stack trace is much more actionable.
| sdf_max_resolution: int | None = None | |
| """Maximum SDF grid dimension. | |
| Newton requires this value to be divisible by 8. If | |
| :attr:`sdf_target_voxel_size` is also authored, Newton uses the target voxel | |
| size and ignores this resolution. | |
| Written to ``newton:sdfMaxResolution`` via ``NewtonSDFCollisionAPI``. | |
| """ | |
| sdf_max_resolution: int | None = None | |
| """Maximum SDF grid dimension. | |
| Newton requires this value to be divisible by 8. If | |
| :attr:`sdf_target_voxel_size` is also authored, Newton uses the target voxel | |
| size and ignores this resolution. | |
| Written to ``newton:sdfMaxResolution`` via ``NewtonSDFCollisionAPI``. | |
| """ | |
| def __post_init__(self): | |
| if self.sdf_max_resolution is not None and self.sdf_max_resolution % 8 != 0: | |
| raise ValueError( | |
| f"'sdf_max_resolution' must be divisible by 8, got {self.sdf_max_resolution}." | |
| ) |
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com> Signed-off-by: ooctipus <zhengyuz@nvidia.com>
Description
Adds a Newton SDF collision schema config surface to IsaacLab.
This PR introduces
NewtonSDFCollisionPropertiesCfg, allowing IsaacLab users to author Newton SDF and hydroelastic collisionUSD attributes from Python configs. The new config writes
NewtonSDFCollisionAPImetadata and the correspondingnewton:*attributes for SDF resolution, narrow band, voxel size, texture format, padding, hydroelastic enablement, and hydroelastic
stiffness.
The change also wires the new config through the existing public export paths:
isaaclab_newton.sim.schemasisaaclab.simisaaclab.sim.schemasisaaclab.sim.schemas.schemas_cfgDocumentation, shim tests, Newton schema tests, and changelog fragments are included.
Type of change
Screenshots
Not applicable.
Checklist
contributing.html)
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists thereValidation
Ran the following checks locally:
git diff --checkruff checkruff format --checkpre-commit run --files ...python3 -m py_compile ...pytest source/isaaclab/test/sim/test_schemas_shim.py145 passedpytest source/isaaclab_newton/test/sim/test_newton_schemas.py17 passed