Add support for geom contact margin #1253
Conversation
Signed-off-by: Gordon Yeoman <[email protected]>
…vidia/newton into dev/gyeoman/geom_margin
Signed-off-by: Gordon Yeoman <[email protected]>
…vidia/newton into dev/gyeoman/geom_margin
📝 WalkthroughWalkthroughPropagates a per-shape contact margin (shape_contact_margin → geom_margin) from MJCF/USD import through ModelBuilder into SolverMuJoCo and the MuJoCo kernel; adds parsing, multi-world data expansion, kernel I/O changes, and unit tests for parsing and runtime updates. Changes
Sequence Diagram(s)sequenceDiagram
participant MJCF as MJCF / USD Import
participant Builder as ModelBuilder
participant Solver as SolverMuJoCo
participant Kernel as update_geom_properties_kernel
participant MuJoCo as MuJoCo Model
MJCF->>Builder: parse shapes (include contact_margin)
Builder->>Solver: export per-shape fields (shape_contact_margin)
Solver->>Solver: expand per-shape fields for multi-worlds -> geom_margin array
Solver->>Kernel: invoke update_geom_properties_kernel(..., shape_contact_margin, ...)
Kernel->>Kernel: geom_margin[world, geom_idx] = shape_contact_margin[shape_idx]
Kernel->>MuJoCo: write geom_margin into MuJoCo geom properties
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (8)📓 Common learnings📚 Learning: 2025-10-10T10:47:41.082ZApplied to files:
📚 Learning: 2025-12-12T08:45:51.908ZApplied to files:
📚 Learning: 2025-10-24T07:56:14.792ZApplied to files:
📚 Learning: 2025-08-20T18:02:36.099ZApplied to files:
📚 Learning: 2025-11-24T08:05:21.390ZApplied to files:
📚 Learning: 2025-08-12T17:51:37.474ZApplied to files:
📚 Learning: 2025-08-25T20:10:59.536ZApplied to files:
🪛 GitHub Actions: Pull Request - AWS GPUnewton/_src/solvers/mujoco/solver_mujoco.py[warning] 434-434: UserWarning: Value for nconmax is changed from 6 to 8 following an MjWarp requirement. [warning] 434-434: UserWarning: Value for njmax is changed from 24 to 32 following an MjWarp requirement. 🔇 Additional comments (8)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
newton/_src/utils/import_usd.py (1)
1372-1377: USD contact margin resolution is sound; consider adding verbose flag for consistencyResolving
contact_marginviaR.get_value(..., default=builder.rigid_contact_margin)nicely mirrors other per-shape properties and honors scene/builder defaults. To keep diagnostics consistent with the rest of the file, you might want to passverbose=verbosehere as you do for otherR.get_valuecalls.newton/tests/test_import_mjcf.py (1)
1431-1472: Margin behavior test is correct; fix stale numeric in commentThe test correctly exercises explicit geom margins and the fallback to
builder.rigid_contact_margin(0.17). The inline comment on Line 1463 still mentions a default margin of0.1, which no longer matches the test setup or expectations—worth updating to avoid confusion.newton/tests/test_import_usd.py (1)
1311-1412: Solid USD geom margin test; drop unusednoqadirectives.The construction and assertions in
test_geom_margin_parsinglook consistent with the existing USD/MuJoCo attribute tests and correctly verify:
- explicit per‑geom margins (0.82, 0.71),
- default margin staying at 0.0 and independent of
builder.rigid_contact_margin.Only nit: Ruff flags the
# noqa: PLC0415comments on the local imports as unused (RUF100). Since these imports are already inside the test function, thenoqais unnecessary here—please remove the# noqa: PLC0415on lines 1314 and 1316.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
newton/_src/solvers/mujoco/kernels.py(3 hunks)newton/_src/solvers/mujoco/solver_mujoco.py(5 hunks)newton/_src/utils/import_mjcf.py(1 hunks)newton/_src/utils/import_usd.py(1 hunks)newton/tests/test_import_mjcf.py(1 hunks)newton/tests/test_import_usd.py(1 hunks)newton/tests/test_mujoco_solver.py(2 hunks)
🧰 Additional context used
🧠 Learnings (11)
📓 Common learnings
Learnt from: shi-eric
Repo: newton-physics/newton PR: 521
File: newton/examples/example_cloth_hanging.py:36-36
Timestamp: 2025-08-12T05:17:34.423Z
Learning: The Newton migration guide (docs/migration.rst) is specifically for documenting how to migrate existing warp.sim functionality to Newton equivalents. New Newton-only features that didn't exist in warp.sim do not need migration documentation.
📚 Learning: 2025-08-12T17:51:37.474Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/geometry.py:16-22
Timestamp: 2025-08-12T17:51:37.474Z
Learning: In the Newton public API refactor, common geometry symbols like ParticleFlags and ShapeFlags are now exposed at the top level in newton/__init__.py rather than through newton.geometry. The newton/geometry.py module is intentionally focused only on broad-phase collision detection classes (BroadPhaseAllPairs, BroadPhaseExplicit, BroadPhaseSAP).
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.pynewton/_src/solvers/mujoco/kernels.pynewton/_src/utils/import_usd.py
📚 Learning: 2025-11-24T08:05:21.390Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 1107
File: newton/_src/solvers/mujoco/kernels.py:973-974
Timestamp: 2025-11-24T08:05:21.390Z
Learning: In Newton's MuJoCo solver integration (newton/_src/solvers/mujoco/), the mapping between Newton joints and MuJoCo joints is not 1-to-1. Instead, each Newton DOF maps to a distinct MuJoCo joint. This means that for multi-DOF joints (like D6 with 6 DOFs), there will be 6 corresponding MuJoCo joints, each with its own properties (margin, solimp, solref, etc.). The mapping is done via dof_to_mjc_joint array, ensuring each DOF's properties are written to its own MuJoCo joint without overwriting.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-08-25T20:10:59.536Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 634
File: newton/examples/ik/example_ik_franka.py:0-0
Timestamp: 2025-08-25T20:10:59.536Z
Learning: In Newton's IK examples, when creating solver arrays with `wp.array(source_array, shape=new_shape)`, this creates a view into the same underlying memory rather than a copy. This means updates to the reshaped array are automatically reflected in the original array without needing explicit copy operations like `wp.copy()`.
Applied to files:
newton/_src/solvers/mujoco/kernels.py
📚 Learning: 2025-09-22T21:08:31.901Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/examples/ik/example_ik_franka.py:121-123
Timestamp: 2025-09-22T21:08:31.901Z
Learning: In the newton physics framework, when creating warp arrays for IK solver joint variables using wp.array(self.model.joint_q, shape=(1, coord_count)), the resulting array acts as a reference/pointer to the original model's joint coordinates, so updates from the IK solver automatically reflect in the model's joint_q buffer used for rendering.
Applied to files:
newton/_src/solvers/mujoco/kernels.py
📚 Learning: 2025-08-20T03:30:16.037Z
Learnt from: eric-heiden
Repo: newton-physics/newton PR: 584
File: newton/_src/utils/import_urdf.py:159-167
Timestamp: 2025-08-20T03:30:16.037Z
Learning: In the URDF importer (newton/_src/utils/import_urdf.py), cylinders are intentionally created using add_shape_capsule() instead of add_shape_cylinder() because cylinder collisions are not fully supported yet. This is a deliberate workaround until proper cylinder collision support is implemented.
Applied to files:
newton/_src/utils/import_usd.py
📚 Learning: 2025-10-10T10:47:41.082Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 899
File: newton/tests/test_rigid_contact.py:214-268
Timestamp: 2025-10-10T10:47:41.082Z
Learning: In `newton/tests/test_rigid_contact.py`, using `add_shape_plane(width=0, length=0)` for an infinite plane works correctly with the collision system and does not cause degenerate AABB issues in the test scenario `test_shape_collisions_gjk_mpr_multicontact`.
Applied to files:
newton/_src/utils/import_usd.pynewton/tests/test_mujoco_solver.py
📚 Learning: 2025-10-24T07:56:14.792Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 981
File: newton/_src/geometry/collision_convex.py:180-289
Timestamp: 2025-10-24T07:56:14.792Z
Learning: In newton/_src/geometry/collision_convex.py, the single-contact solver (create_solve_convex_single_contact) intentionally returns count=1 even when shapes are separated (signed_distance > contact_threshold). The calling code is responsible for deciding whether to accept the contact based on the signed distance value. This design pushes filtering responsibility to the caller.
Applied to files:
newton/_src/utils/import_usd.py
📚 Learning: 2025-09-24T00:26:54.486Z
Learnt from: eric-heiden
Repo: newton-physics/newton PR: 710
File: newton/_src/utils/import_usd.py:597-599
Timestamp: 2025-09-24T00:26:54.486Z
Learning: In the Newton ModelBuilder class, `joint_count`, `body_count`, `articulation_count`, and `shape_count` are implemented as property decorated methods, so they should be accessed as attributes (e.g., `builder.joint_count`) rather than called as methods (e.g., `builder.joint_count()`).
Applied to files:
newton/tests/test_mujoco_solver.py
📚 Learning: 2025-08-12T18:07:17.267Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
Applied to files:
newton/tests/test_import_usd.py
📚 Learning: 2025-08-12T18:07:17.267Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions (lines 109-111 and 1289-1291) rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
Applied to files:
newton/tests/test_import_usd.py
🧬 Code graph analysis (1)
newton/_src/utils/import_usd.py (2)
newton/_src/usd/schema_resolver.py (3)
get_value(101-120)get_value(196-247)PrimType(33-49)newton/_src/sim/builder.py (1)
key(395-397)
🪛 Ruff (0.14.8)
newton/tests/test_import_usd.py
1314-1314: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
1316-1316: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
🔇 Additional comments (9)
newton/_src/utils/import_mjcf.py (1)
330-333: MJCF per-geom margin wiring looks correctReading
geom_attrib["margin"]intoshape_cfg.contact_marginonly when authored cleanly extends ShapeConfig without disturbing existing defaults; no issues spotted.newton/_src/solvers/mujoco/kernels.py (1)
1177-1205: Per-geom margin propagation through MuJoCo kernel is consistent and minimalAdding
shape_contact_marginas an input and copying it intogeom_margin[world, geom_idx]mirrors how other shape properties are exported (friction, solimp/solmix, etc.) and cleanly feedscontact_params’ existing use ofgeom_marginwithout altering control flow. Implementation looks correct assuming caller updates the kernel launch signature accordingly.If not already done, double‑check the corresponding kernel launch in
solver_mujoco.pypassesshape_contact_marginandgeom_marginin the updated order to avoid subtle argument misalignment.Also applies to: 1247-1248
newton/tests/test_mujoco_solver.py (2)
2499-2591: geom_margin multi‑world conversion and update test looks correct.The new
test_geom_margin_conversion_and_updatefollows the established patterns for geom_* tests:
- assigns per‑shape
shape_contact_marginusingshape_world,- verifies
mjw_model.geom_marginviamjc_geom_to_newton_shape,- exercises runtime updates through
notify_model_changed(SHAPE_PROPERTIES).Indexing and multi‑world expansion logic are consistent with the neighboring geom property tests.
3582-3583: Explicitly zeroing rigid_contact_margin in mocap test is appropriate.Setting
builder.rigid_contact_margin = 0.0intest_mocap_body_transform_updates_collision_geomskeeps this scenario independent of global contact margins, which is helpful now that per‑geom margins are supported. The change is localized to this test and should not affect other cases.newton/_src/solvers/mujoco/solver_mujoco.py (5)
1093-1093: LGTM: Contact margin retrieval follows established pattern.The retrieval of
shape_contact_marginis consistent with how other core shape properties are accessed (e.g.,shape_mu,shape_torsional_friction). This appropriately treats contact margin as a core Newton shape attribute rather than a MuJoCo-specific custom attribute.
1347-1348: LGTM: Margin parameter correctly set during geom creation.The conditional setting of the margin parameter follows the same pattern as other shape properties (e.g.,
shape_condim,shape_priority,shape_geom_solimp) and correctly indexes into the shape-specific margin value.
1968-1968: LGTM: Geom margin correctly included in multi-world expansion.The addition of
"geom_margin"tomodel_fields_to_expandensures that margin values are properly replicated across multiple worlds in the MuJoCo Warp model, consistent with other geometry properties likegeom_frictionandgeom_solmix.
2236-2236: LGTM: Contact margin correctly passed to update kernel.The
shape_contact_margininput toupdate_geom_properties_kernelenables dynamic updates of margin values through thenotify_model_changedsystem, consistent with other shape properties.
2257-2257: LGTM: Geom margin properly output to MuJoCo model.The output of
geom_marginto the MuJoCo Warp model completes the data flow from Newton's shape properties through to MuJoCo's collision detection system.
Signed-off-by: Gordon Yeoman <[email protected]>
Signed-off-by: Gordon Yeoman <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
newton/_src/utils/import_mjcf.py (1)
336-338: Minor: Redundant default parameter in parse_float call.The
parse_float(geom_attrib, "margin", 0.0)call on line 338 will never use the default value0.0because the code already checksif "margin" in geom_attrib:on line 337. When margin is not specified, the code correctly preservesshape_cfg.contact_marginfrombuilder.default_shape_cfg(line 316).This works correctly, but for clarity, consider one of these alternatives:
Option 1: Remove the if-check and let parse_float handle the missing attribute (but would need to change default):
-# Parse MJCF margin (contact margin for collision detection) -if "margin" in geom_attrib: - shape_cfg.contact_margin = parse_float(geom_attrib, "margin", 0.0) +# Parse MJCF margin (contact margin for collision detection) +# If not specified, parse_float returns current value from shape_cfg +if "margin" in geom_attrib: + shape_cfg.contact_margin = parse_float(geom_attrib, "margin", shape_cfg.contact_margin)Option 2: Make the unused default explicit:
- shape_cfg.contact_margin = parse_float(geom_attrib, "margin", 0.0) + shape_cfg.contact_margin = parse_float(geom_attrib, "margin", 0.0) # default unused; already checked key existsHowever, since this follows the same pattern as the friction parsing above (lines 322-334), maintaining consistency is reasonable.
newton/tests/test_mujoco_solver.py (2)
2597-2689: LGTM! Comprehensive test for geom_margin feature.The test correctly validates per-shape contact margin conversion to MuJoCo and runtime updates across multiple worlds. The structure follows established patterns and covers both initial conversion and dynamic updates via
notify_model_changed.Optional: Consider adding a clarifying comment to explain why this test doesn't call
SolverMuJoCo.register_custom_attributes()unlike similar tests (e.g.,test_geom_gap_conversion_and_update). This would help future maintainers understand thatshape_contact_marginis a standard Newton Model attribute rather than a MuJoCo-specific custom attribute.For example:
def test_geom_margin_conversion_and_update(self): """Test per-shape geom_margin conversion to MuJoCo and dynamic updates across multiple worlds.""" + # Note: shape_contact_margin is a standard Newton Model attribute, not a MuJoCo custom attribute, + # so no register_custom_attributes() call is needed (unlike geom_gap, geom_solmix, etc.) # Create a model with custom attributes registered
2792-2792: Clarify the reason for setting rigid_contact_margin to zero.This line sets the contact margin to zero, likely to isolate the test from margin-related behavior. Adding a brief comment would help future maintainers understand why this configuration is necessary.
Apply this diff to add a clarifying comment:
builder = newton.ModelBuilder() builder.default_shape_cfg.ke = 1e4 builder.default_shape_cfg.kd = 1000.0 - builder.rigid_contact_margin = 0.0 + builder.rigid_contact_margin = 0.0 # Disable contact margin to ensure clean contact behavior
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
newton/_src/solvers/mujoco/kernels.py(3 hunks)newton/_src/solvers/mujoco/solver_mujoco.py(5 hunks)newton/_src/utils/import_mjcf.py(1 hunks)newton/tests/test_import_mjcf.py(1 hunks)newton/tests/test_import_usd.py(1 hunks)newton/tests/test_mujoco_solver.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- newton/_src/solvers/mujoco/solver_mujoco.py
- newton/tests/test_import_usd.py
🧰 Additional context used
🧠 Learnings (5)
📓 Common learnings
Learnt from: shi-eric
Repo: newton-physics/newton PR: 521
File: newton/examples/example_cloth_hanging.py:36-36
Timestamp: 2025-08-12T05:17:34.423Z
Learning: The Newton migration guide (docs/migration.rst) is specifically for documenting how to migrate existing warp.sim functionality to Newton equivalents. New Newton-only features that didn't exist in warp.sim do not need migration documentation.
Learnt from: nvtw
Repo: newton-physics/newton PR: 899
File: newton/tests/test_rigid_contact.py:214-268
Timestamp: 2025-10-10T10:47:41.082Z
Learning: In `newton/tests/test_rigid_contact.py`, using `add_shape_plane(width=0, length=0)` for an infinite plane works correctly with the collision system and does not cause degenerate AABB issues in the test scenario `test_shape_collisions_gjk_mpr_multicontact`.
📚 Learning: 2025-08-12T17:51:37.474Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/geometry.py:16-22
Timestamp: 2025-08-12T17:51:37.474Z
Learning: In the Newton public API refactor, common geometry symbols like ParticleFlags and ShapeFlags are now exposed at the top level in newton/__init__.py rather than through newton.geometry. The newton/geometry.py module is intentionally focused only on broad-phase collision detection classes (BroadPhaseAllPairs, BroadPhaseExplicit, BroadPhaseSAP).
Applied to files:
newton/_src/solvers/mujoco/kernels.pynewton/tests/test_mujoco_solver.py
📚 Learning: 2025-08-25T20:10:59.536Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 634
File: newton/examples/ik/example_ik_franka.py:0-0
Timestamp: 2025-08-25T20:10:59.536Z
Learning: In Newton's IK examples, when creating solver arrays with `wp.array(source_array, shape=new_shape)`, this creates a view into the same underlying memory rather than a copy. This means updates to the reshaped array are automatically reflected in the original array without needing explicit copy operations like `wp.copy()`.
Applied to files:
newton/_src/solvers/mujoco/kernels.py
📚 Learning: 2025-10-10T10:47:41.082Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 899
File: newton/tests/test_rigid_contact.py:214-268
Timestamp: 2025-10-10T10:47:41.082Z
Learning: In `newton/tests/test_rigid_contact.py`, using `add_shape_plane(width=0, length=0)` for an infinite plane works correctly with the collision system and does not cause degenerate AABB issues in the test scenario `test_shape_collisions_gjk_mpr_multicontact`.
Applied to files:
newton/tests/test_mujoco_solver.py
📚 Learning: 2025-09-24T00:26:54.486Z
Learnt from: eric-heiden
Repo: newton-physics/newton PR: 710
File: newton/_src/utils/import_usd.py:597-599
Timestamp: 2025-09-24T00:26:54.486Z
Learning: In the Newton ModelBuilder class, `joint_count`, `body_count`, `articulation_count`, and `shape_count` are implemented as property decorated methods, so they should be accessed as attributes (e.g., `builder.joint_count`) rather than called as methods (e.g., `builder.joint_count()`).
Applied to files:
newton/tests/test_mujoco_solver.py
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run GPU Tests / Run GPU Unit Tests on AWS EC2
- GitHub Check: Run GPU Benchmarks / Run GPU Benchmarks on AWS EC2
🔇 Additional comments (2)
newton/tests/test_import_mjcf.py (1)
1500-1542: LGTM! Test correctly validates per-geom margin parsing and default handling.The test properly verifies:
- Explicit per-geom margin values are parsed and applied (0.5, 0.8)
- Missing margin attributes fall back to
builder.rigid_contact_margin(0.17) instead of MuJoCo's default (0.0)- The
shape_contact_marginattribute is correctly exposed on the finalized modelThe comment on line 1535 helpfully clarifies Newton's design decision to use its own default margin rather than MuJoCo's convention.
newton/_src/solvers/mujoco/kernels.py (1)
1183-1183: LGTM! Kernel changes correctly propagate per-shape contact margin.The implementation:
- Adds
shape_contact_margininput andgeom_marginoutput parameters to the kernel signature- Writes the per-shape margin to the per-world geom array (lines 1252-1253)
- Follows the same pattern as existing properties like
geom_gap(lines 1249-1250)- Benefits from the existing shape_idx bounds check on line 1215
The changes integrate cleanly with the existing update path and maintain consistency with the codebase patterns.
Also applies to: 1205-1205, 1252-1253
…om pre-commit.ci Signed-off-by: Gordon Yeoman <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
newton/tests/test_import_usd.py (3)
1431-1433: Clean up unusednoqadirectives.The static analysis tool indicates these
noqadirectives are unnecessary since the PLC0415 rule is not enabled. While this is a minor issue, removing them would clean up the code.Apply this diff to remove the unused directives:
- from pxr import Usd # noqa: PLC0415 + from pxr import Usd - from newton._src.usd.schemas import SchemaResolverMjc # noqa: PLC0415 + from newton._src.usd.schemas import SchemaResolverMjc
1520-1523: Clarify thatbuilder.rigid_contact_marginis ignored for USD imports.Setting
builder.rigid_contact_margin = 0.2here is potentially misleading because USD imports withSchemaResolverMjcuse the hardcoded default of 0.0 from the schema resolver, not the builder's setting. This differs from MJCF imports where the builder's margin is used as the default (seetest_import_mjcf.py::test_geom_margin_parsing).Consider either removing this line or adding a comment explaining that it's intentionally set to a different value to verify that USD parsing ignores it.
Option 1 - Remove the misleading assignment:
builder = newton.ModelBuilder() - builder.rigid_contact_margin = 0.2 builder.add_usd(stage, schema_resolvers=[SchemaResolverMjc()])Option 2 - Add clarifying comment:
builder = newton.ModelBuilder() + # Note: This setting is ignored for USD imports with SchemaResolverMjc, + # which uses its own hardcoded default of 0.0 builder.rigid_contact_margin = 0.2 builder.add_usd(stage, schema_resolvers=[SchemaResolverMjc()])
1534-1537: Improve comment formatting.The inline comment on line 1536 is lengthy and disrupts readability. Consider moving it to a separate line above the assignment for better clarity.
Apply this diff:
# Check that we have shapes with expected values expected_explicit_1 = 0.82 - expected_default = 0.0 # default Note that we default to 0.0 when parsing USD and that builder.rigid_contact_margin plays no role. + # Note: USD parsing with SchemaResolverMjc defaults to 0.0, ignoring builder.rigid_contact_margin + expected_default = 0.0 expected_explicit_2 = 0.71
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
newton/tests/test_import_usd.py(1 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: shi-eric
Repo: newton-physics/newton PR: 521
File: newton/examples/example_cloth_hanging.py:36-36
Timestamp: 2025-08-12T05:17:34.423Z
Learning: The Newton migration guide (docs/migration.rst) is specifically for documenting how to migrate existing warp.sim functionality to Newton equivalents. New Newton-only features that didn't exist in warp.sim do not need migration documentation.
Learnt from: nvtw
Repo: newton-physics/newton PR: 899
File: newton/tests/test_rigid_contact.py:214-268
Timestamp: 2025-10-10T10:47:41.082Z
Learning: In `newton/tests/test_rigid_contact.py`, using `add_shape_plane(width=0, length=0)` for an infinite plane works correctly with the collision system and does not cause degenerate AABB issues in the test scenario `test_shape_collisions_gjk_mpr_multicontact`.
📚 Learning: 2025-08-12T18:07:17.267Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions (lines 109-111 and 1289-1291) rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
Applied to files:
newton/tests/test_import_usd.py
📚 Learning: 2025-08-12T18:07:17.267Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/utils.py:30-33
Timestamp: 2025-08-12T18:07:17.267Z
Learning: In newton/_src/utils/import_usd.py, the PXR/USD modules are lazily loaded using try-except blocks within the functions rather than at module import time, making it safe to import parse_usd at the module level without causing import-time failures for users without USD dependencies.
Applied to files:
newton/tests/test_import_usd.py
📚 Learning: 2025-10-10T10:47:41.082Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 899
File: newton/tests/test_rigid_contact.py:214-268
Timestamp: 2025-10-10T10:47:41.082Z
Learning: In `newton/tests/test_rigid_contact.py`, using `add_shape_plane(width=0, length=0)` for an infinite plane works correctly with the collision system and does not cause degenerate AABB issues in the test scenario `test_shape_collisions_gjk_mpr_multicontact`.
Applied to files:
newton/tests/test_import_usd.py
🧬 Code graph analysis (1)
newton/tests/test_import_usd.py (2)
newton/tests/test_import_mjcf.py (1)
test_geom_margin_parsing(1500-1541)newton/_src/usd/schemas.py (1)
SchemaResolverMjc(245-315)
🪛 Ruff (0.14.8)
newton/tests/test_import_usd.py
1431-1431: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
1433-1433: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run GPU Benchmarks / Run GPU Benchmarks on AWS EC2
- GitHub Check: Run GPU Tests / Run GPU Unit Tests on AWS EC2
… value is there: shape_cfg.contact_margin = float(geom_attrib["margin"]) Signed-off-by: Gordon Yeoman <[email protected]>
Signed-off-by: Gordon Yeoman <[email protected]>
newton/_src/utils/import_mjcf.py
Outdated
|
|
||
| # Parse MJCF margin (contact margin for collision detection) | ||
| if "margin" in geom_attrib: | ||
| shape_cfg.contact_margin = float(geom_attrib["margin"]) |
There was a problem hiding this comment.
I think you could just use parse_float here.
newton/tests/test_import_mjcf.py
Outdated
| geom_margin = model.shape_contact_margin.numpy() | ||
| self.assertEqual(model.shape_count, 3, "Should have 3 shapes") | ||
|
|
||
| # Expected values: shape 0 has margin=0.5, shape 1 has default margin=0.1 (builder.rigid_contact_margin), shape 2 has margin=0.8 |
There was a problem hiding this comment.
nitpick: comment is outdated (1 vs 1.7)
| model = builder.finalize() | ||
|
|
||
| # shape_contact_margin is a built-in Newton attribute, not a custom mujoco attribute | ||
| self.assertTrue(hasattr(model, "shape_contact_margin"), "Model should have shape_contact_margin attribute") |
There was a problem hiding this comment.
I'm not sure if that assert is really needed, the attribute should always be there and the test is going to fail anyway if you're going to access it. We never really do this test for other built-ins. But it's a subjective issue, leaving the decision up to you.
There was a problem hiding this comment.
It's really just a reminder that we are not working with a custom attribute. It's useful for me, even if for nobody else.
Signed-off-by: Gordon Yeoman <[email protected]>
Signed-off-by: Gordon Yeoman <[email protected]>
Signed-off-by: Gordon Yeoman <[email protected]>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Signed-off-by: Gordon Yeoman <[email protected]>
Signed-off-by: Gordon Yeoman <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
newton/tests/test_rigid_contact.py (1)
40-53: Consider using a dictionary for cleaner margin lookup.The current if-elif chain works correctly, but a dictionary-based approach would be more maintainable and concise.
🔎 Optional refactoring to use dictionary lookup:
-def get_contact_margin(solver_name): - if solver_name == "featherstone": - return 100.0 - elif solver_name == "mujoco_cpu": - return 0.0 - elif solver_name == "mujoco_warp": - return 0.0 - elif solver_name == "xpbd": - return 100.0 - elif solver_name == "semi_implicit": - return 100.0 - else: - return 0.0 +def get_contact_margin(solver_name): + margins = { + "featherstone": 100.0, + "mujoco_cpu": 0.0, + "mujoco_warp": 0.0, + "xpbd": 100.0, + "semi_implicit": 100.0, + } + return margins.get(solver_name, 0.0)
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
newton/_src/utils/import_mjcf.py(1 hunks)newton/_src/utils/import_usd.py(1 hunks)newton/examples/robot/example_robot_allegro_hand.py(1 hunks)newton/tests/test_anymal_reset.py(1 hunks)newton/tests/test_import_mjcf.py(1 hunks)newton/tests/test_rigid_contact.py(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- newton/_src/utils/import_mjcf.py
- newton/tests/test_import_mjcf.py
- newton/_src/utils/import_usd.py
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: shi-eric
Repo: newton-physics/newton PR: 521
File: newton/examples/example_cloth_hanging.py:36-36
Timestamp: 2025-08-12T05:17:34.423Z
Learning: The Newton migration guide (docs/migration.rst) is specifically for documenting how to migrate existing warp.sim functionality to Newton equivalents. New Newton-only features that didn't exist in warp.sim do not need migration documentation.
📚 Learning: 2025-12-12T08:45:43.428Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 1221
File: newton/examples/example_sdf.py:277-287
Timestamp: 2025-12-12T08:45:43.428Z
Learning: In Newtown (Newton) example code, specifically files under newton/examples, computing contacts once per frame and reusing them across all substeps is an intentional design choice, not a bug. Reviewers should verify that contacts are computed before the substep loop and reused for every substep within the same frame. This pattern reduces redundant work and preserves frame-consistency; do not flag as a regression unless the behavior is changed for correctness or performance reasons.
Applied to files:
newton/examples/robot/example_robot_allegro_hand.py
📚 Learning: 2025-10-10T10:47:41.082Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 899
File: newton/tests/test_rigid_contact.py:214-268
Timestamp: 2025-10-10T10:47:41.082Z
Learning: In `newton/tests/test_rigid_contact.py`, using `add_shape_plane(width=0, length=0)` for an infinite plane works correctly with the collision system and does not cause degenerate AABB issues in the test scenario `test_shape_collisions_gjk_mpr_multicontact`.
Applied to files:
newton/tests/test_rigid_contact.py
📚 Learning: 2025-10-24T07:56:14.792Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 981
File: newton/_src/geometry/collision_convex.py:180-289
Timestamp: 2025-10-24T07:56:14.792Z
Learning: In newton/_src/geometry/collision_convex.py, the single-contact solver (create_solve_convex_single_contact) intentionally returns count=1 even when shapes are separated (signed_distance > contact_threshold). The calling code is responsible for deciding whether to accept the contact based on the signed distance value. This design pushes filtering responsibility to the caller.
Applied to files:
newton/tests/test_rigid_contact.py
🪛 Ruff (0.14.8)
newton/tests/test_rigid_contact.py
66-66: Unused function argument: test
(ARG001)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run GPU Tests / Run GPU Unit Tests on AWS EC2
- GitHub Check: Run GPU Benchmarks / Run GPU Benchmarks on AWS EC2
🔇 Additional comments (5)
newton/tests/test_anymal_reset.py (1)
39-39: LGTM! Contact margin explicitly configured for test consistency.Setting
rigid_contact_margin = 0.0ensures deterministic contact behavior for the reset stability tests. The placement before model finalization is correct.newton/examples/robot/example_robot_allegro_hand.py (1)
96-96: LGTM! Contact margin properly configured for the example.Setting
rigid_contact_margin = 0.0is appropriate for the MuJoCo solver used in this example (line 135). The placement in the builder configuration sequence is correct.newton/tests/test_rigid_contact.py (3)
31-37: LGTM! Multi-solver test infrastructure is well-structured.The
solversdictionary provides a clean, extensible way to test across different solver backends. The lambda-based lazy initialization is appropriate.
66-66: LGTM! Solver-specific margin handling is correctly implemented.The
solver_nameparameter enables dynamic margin configuration per solver type. The margin is correctly set before adding shapes (line 128), as required.Note: The static analysis warning about unused
testparameter is a false positive—the parameter is used for assertions throughout the function (e.g., line 205, 207).Also applies to: 128-128
754-767: LGTM! Test registration correctly wires solver-specific parameters.The test registration loop properly passes
solver_nameto enable per-solver margin configuration. The device filtering logic (lines 756-759) appropriately skips incompatible solver-device combinations.
…th xpbd. Signed-off-by: Gordon Yeoman <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
newton/tests/test_rigid_contact.py (1)
55-55: Unusedsolver_nameparameter (likely intentional).The
solver_nameparameter is not used within the function body. This appears intentional for test registration purposes (passed toadd_function_testat line 755 for test naming), which is an acceptable pattern in test frameworks.If this is the intended design, consider adding a comment or suppressing the linting warning. Alternatively, you can prefix unused parameters with an underscore (e.g.,
_solver_name) to indicate they're intentionally unused.🔎 Optional: Suppress linting warning
-def test_shapes_on_plane(test, device, solver_fn, solver_name): +def test_shapes_on_plane(test, device, solver_fn, solver_name): # noqa: ARG001Or prefix with underscore to indicate intentional:
-def test_shapes_on_plane(test, device, solver_fn, solver_name): +def test_shapes_on_plane(test, device, solver_fn, _solver_name):
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
newton/tests/test_rigid_contact.py
🧰 Additional context used
🧠 Learnings (7)
📓 Common learnings
Learnt from: shi-eric
Repo: newton-physics/newton PR: 521
File: newton/examples/example_cloth_hanging.py:36-36
Timestamp: 2025-08-12T05:17:34.423Z
Learning: The Newton migration guide (docs/migration.rst) is specifically for documenting how to migrate existing warp.sim functionality to Newton equivalents. New Newton-only features that didn't exist in warp.sim do not need migration documentation.
Learnt from: nvtw
Repo: newton-physics/newton PR: 899
File: newton/tests/test_rigid_contact.py:214-268
Timestamp: 2025-10-10T10:47:41.082Z
Learning: In `newton/tests/test_rigid_contact.py`, using `add_shape_plane(width=0, length=0)` for an infinite plane works correctly with the collision system and does not cause degenerate AABB issues in the test scenario `test_shape_collisions_gjk_mpr_multicontact`.
📚 Learning: 2025-10-10T10:47:41.082Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 899
File: newton/tests/test_rigid_contact.py:214-268
Timestamp: 2025-10-10T10:47:41.082Z
Learning: In `newton/tests/test_rigid_contact.py`, using `add_shape_plane(width=0, length=0)` for an infinite plane works correctly with the collision system and does not cause degenerate AABB issues in the test scenario `test_shape_collisions_gjk_mpr_multicontact`.
Applied to files:
newton/tests/test_rigid_contact.py
📚 Learning: 2025-09-18T07:05:56.836Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 750
File: newton/_src/solvers/implicit_mpm/solve_rheology.py:969-986
Timestamp: 2025-09-18T07:05:56.836Z
Learning: In newton/_src/solvers/implicit_mpm/solve_rheology.py, transposed_strain_mat parameter cannot be None - the type signature was corrected to reflect this guarantee, eliminating the need for None checks when accessing transposed_strain_mat.offsets.
Applied to files:
newton/tests/test_rigid_contact.py
📚 Learning: 2025-10-24T07:56:14.792Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 981
File: newton/_src/geometry/collision_convex.py:180-289
Timestamp: 2025-10-24T07:56:14.792Z
Learning: In newton/_src/geometry/collision_convex.py, the single-contact solver (create_solve_convex_single_contact) intentionally returns count=1 even when shapes are separated (signed_distance > contact_threshold). The calling code is responsible for deciding whether to accept the contact based on the signed distance value. This design pushes filtering responsibility to the caller.
Applied to files:
newton/tests/test_rigid_contact.py
📚 Learning: 2025-08-12T17:51:37.474Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/geometry.py:16-22
Timestamp: 2025-08-12T17:51:37.474Z
Learning: In the Newton public API refactor, common geometry symbols like ParticleFlags and ShapeFlags are now exposed at the top level in newton/__init__.py rather than through newton.geometry. The newton/geometry.py module is intentionally focused only on broad-phase collision detection classes (BroadPhaseAllPairs, BroadPhaseExplicit, BroadPhaseSAP).
Applied to files:
newton/tests/test_rigid_contact.py
📚 Learning: 2025-08-12T17:58:16.929Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/tests/test_ik.py:25-26
Timestamp: 2025-08-12T17:58:16.929Z
Learning: In the Newton physics engine codebase, it's acceptable for tests to import and use private `_src` internal modules and functions when needed for testing purposes, even though this breaks the public API boundary. This is an intentional project decision for test code.
Applied to files:
newton/tests/test_rigid_contact.py
📚 Learning: 2025-08-20T18:02:36.099Z
Learnt from: eric-heiden
Repo: newton-physics/newton PR: 587
File: newton/_src/sim/builder.py:656-661
Timestamp: 2025-08-20T18:02:36.099Z
Learning: In Newton physics, collisions between shapes with body index -1 (world-attached/global shapes) are automatically skipped by the collision system, so no manual collision filter pairs need to be added between global shapes from different builders.
Applied to files:
newton/tests/test_rigid_contact.py
🪛 Ruff (0.14.10)
newton/tests/test_rigid_contact.py
55-55: Unused function argument: test
(ARG001)
55-55: Unused function argument: solver_name
(ARG001)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: run-newton-tests / newton-unittests (windows-latest)
🔇 Additional comments (4)
newton/tests/test_rigid_contact.py (4)
31-37: LGTM!The module-scope solvers dictionary is well-structured for test parameterization. The lambda-based lazy instantiation is appropriate, and the cleanup of the previously duplicated dictionary block improves code organization.
42-52: LGTM!The conditional contact computation logic correctly handles the different solver backends. MuJoCo solvers handle collision detection internally (contacts=None), while other solvers require explicit collision computation via
model.collide().
116-117: LGTM!Setting
rigid_contact_margin = 0.0is appropriate for this test, which verifies that shapes settle to precise expected positions (lines 129, 140, 149, 157, 165) matching their geometric dimensions. The comment correctly notes the ordering constraint.
743-756: LGTM!The test registration loop correctly integrates with the module-scope
solversdictionary and passessolver_namefor test naming. The device/solver compatibility filtering (skipping mujoco_warp on CPU and mujoco_cpu on CUDA) is appropriate.
Signed-off-by: Gordon Yeoman <[email protected]>
Signed-off-by: Gordon Yeoman <[email protected]>
|
@gyeomannvidia should we close this for now? |
|
Definitely close this. |
|
closing as per recent margin-related discussions. |
2 participants
Description
Newton Migration Guide
Please ensure the migration guide for warp.sim users is up-to-date with the changes made in this PR.
docs/migration.rstis up-to dateBefore your PR is "Ready for review"
newton/tests/test_examples.py)pre-commit run -aSummary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.