Adding geom_gap as custom mujoco variable#1228
Conversation
Signed-off-by: Gordon Yeoman <gyeoman@nvidia.com>
📝 WalkthroughWalkthroughAdds a per-shape MuJoCo custom attribute Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Importer as Importer (MJCF/USD)
participant ModelBuilder as ModelBuilder / SolverMuJoCo.register_custom_attributes
participant Solver as SolverMuJoCo
participant Kernel as update_geom_properties_kernel
participant MJW as mjw_model (per-world arrays)
Note over Importer,ModelBuilder: Import phase
Importer->>ModelBuilder: produce shape-level attribute `geom_gap` (shape_geom_gap)
ModelBuilder->>Solver: store `shape_geom_gap` in model.mujoco fields
Note over Solver,Kernel: Conversion/update phase
Solver->>Kernel: call update_geom_properties_kernel(..., shape_geom_gap, ...)
Kernel-->>Solver: return geom_gap (per-geom 2D array)
Solver->>MJW: write mjw_model.geom_gap from kernel output
Note over Solver,MJW: Runtime update
Solver->>Solver: notify_model_changed -> re-run kernel with updated shape_geom_gap
Kernel-->>MJW: updated geom_gap values propagated to per-world arrays
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (3)📚 Learning: 2025-08-19T12:27:30.629ZApplied to files:
📚 Learning: 2025-11-24T08:05:21.390ZApplied to files:
📚 Learning: 2025-08-19T12:27:30.629ZApplied to files:
🧬 Code graph analysis (2)newton/_src/solvers/mujoco/solver_mujoco.py (1)
newton/tests/test_import_mjcf.py (2)
⏰ 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)
🔇 Additional comments (5)
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 |
…ding it back. Signed-off-by: Gordon Yeoman <gyeoman@nvidia.com>
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
Status, support, documentation and community
|
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
newton/_src/solvers/mujoco/kernels.py(3 hunks)newton/_src/solvers/mujoco/solver_mujoco.py(7 hunks)newton/tests/test_import_mjcf.py(1 hunks)newton/tests/test_import_usd.py(1 hunks)newton/tests/test_mujoco_solver.py(3 hunks)
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
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.
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/_src/sim/ik/ik_lbfgs_optimizer.py:739-752
Timestamp: 2025-09-22T21:03:39.624Z
Learning: The L-BFGS optimizer in newton/_src/sim/ik/ik_lbfgs_optimizer.py currently intentionally only supports additive updates (assuming n_coords == n_dofs). Velocity space integration for joints with mismatched coordinate/DOF dimensions (like free/ball joints) is planned for future work and should not be flagged as an issue in current reviews.
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/_src/sim/ik/ik_lbfgs_optimizer.py:638-648
Timestamp: 2025-09-22T21:03:18.367Z
Learning: The L-BFGS optimizer in newton/_src/sim/ik/ik_lbfgs_optimizer.py currently only supports additive updates, meaning it assumes n_coords == n_dofs. Velocity space integration for joints with mismatched coordinate/DOF dimensions (like free/ball joints) is planned for future work.
📚 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-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.pynewton/_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-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/kernels.pynewton/tests/test_mujoco_solver.pynewton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-08-19T12:27:30.629Z
Learnt from: preist-nvidia
Repo: newton-physics/newton PR: 579
File: newton/examples/example_mujoco.py:350-354
Timestamp: 2025-08-19T12:27:30.629Z
Learning: In Newton examples, there's a distinction between solver parameters and Example class attributes. The Example class can have its own use_mujoco attribute for controlling example-level behavior (like CUDA graphs, rendering logic), while the solver uses use_mujoco_cpu for backend selection. These serve different purposes and should not be conflated during API renames.
Applied to files:
newton/tests/test_mujoco_solver.pynewton/_src/solvers/mujoco/solver_mujoco.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-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_mujoco_solver.py
📚 Learning: 2025-08-19T12:27:30.629Z
Learnt from: preist-nvidia
Repo: newton-physics/newton PR: 579
File: newton/examples/example_mujoco.py:350-354
Timestamp: 2025-08-19T12:27:30.629Z
Learning: In Newton examples, Example.use_mujoco is a high-level attribute that controls whether to use MuJoCo solver vs other solvers (like XPBD), while SolverMuJoCo.use_mujoco_cpu controls the backend within MuJoCo (CPU vs Warp). These are separate concepts serving different purposes - the PR rename only applies to the solver parameter, not the Example class attributes.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
🧬 Code graph analysis (3)
newton/tests/test_import_usd.py (2)
newton/tests/test_import_mjcf.py (2)
test_geom_gap_parsing(1431-1472)test_geom_margin_parsing(1474-1515)newton/_src/solvers/mujoco/solver_mujoco.py (2)
SolverMuJoCo(80-2504)register_custom_attributes(154-323)
newton/tests/test_import_mjcf.py (2)
newton/tests/test_import_usd.py (2)
test_geom_gap_parsing(1312-1426)test_geom_margin_parsing(1429-1543)newton/_src/solvers/mujoco/solver_mujoco.py (2)
SolverMuJoCo(80-2504)register_custom_attributes(154-323)
newton/tests/test_mujoco_solver.py (1)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
register_custom_attributes(154-323)
🪛 Ruff (0.14.8)
newton/tests/test_import_usd.py
1314-1314: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
1431-1431: 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 Tests / Run GPU Unit Tests on AWS EC2
- GitHub Check: Run GPU Benchmarks / Run GPU Benchmarks on AWS EC2
🔇 Additional comments (7)
newton/_src/solvers/mujoco/kernels.py (2)
1194-1195: LGTM! Kernel signature properly extended for geom_gap and geom_margin.The additions follow the established pattern for custom MuJoCo shape attributes (matching the shape_geom_solimp/shape_geom_solmix convention).
Also applies to: 1205-1206
1249-1256: LGTM! Attribute propagation logic is correct.The conditional blocks properly propagate shape-level gap and margin attributes to per-geom outputs, following the same pattern as the existing solimp/solmix handling.
newton/tests/test_import_usd.py (2)
1311-1427: LGTM! USD geom_gap parsing test is correct.The test properly validates that the
mjc:gapUSD attribute is parsed and exposed viamodel.mujoco.geom_gap, with both explicit values (0.8, 0.7) and defaults (0.0) being verified.Note: The static analysis hint about unused
noqadirective on line 1314 can be safely ignored - this is a common pattern for conditional imports throughout the file.
1428-1544: LGTM! USD geom_margin parsing test is correct.The test properly validates that the
mjc:marginUSD attribute is parsed and exposed viamodel.mujoco.geom_margin, with both explicit values (0.8, 0.7) and defaults (0.0) being verified.Note: The static analysis hint about unused
noqadirective on line 1431 can be safely ignored - this is a common pattern for conditional imports throughout the file.newton/tests/test_mujoco_solver.py (2)
2401-2477: Geom-gap test correctly exercises multi-world mapping and runtime updatesThe
test_geom_gap_conversion_and_updateflow (per-world initialization viashape_world, verification throughmjc_geom_to_newton_shape, andnotify_model_changed(SHAPE_PROPERTIES)updates) is consistent with existing solimp/solmix tests and should robustly catch mapping/indexing bugs forgeom_gap. No issues spotted.Also applies to: 2481-2497
2499-2595: Geom-margin test mirrors the gap coverage and looks sound
test_geom_margin_conversion_and_updatemirrors the gap test structure with per-world initialization, conversion verification, and runtime update checks forgeom_margin. The use ofshape_worldandmjc_geom_to_newton_shapekeeps it robust for multi-world setups. Implementation looks correct.newton/_src/solvers/mujoco/solver_mujoco.py (1)
207-230: geom_gap/geom_margin integration is consistent across registration, conversion, expansion, and updatesThe new
geom_gap/geom_marginattributes are:
- Declared as SHAPE/MODEL custom attributes with appropriate defaults and USD/MJCF names.
- Pulled via
get_custom_attributeand threaded intogeom_paramsasgap/marginduring_convert_to_mjc, matching MuJoCo’s per-geom fields.- Included in
expand_model_fieldsso they tile correctly acrossnworld.- Read in
update_geom_propertiesas optional per-shape arrays and written intomjw_model.geom_gap/geom_marginalongside existing friction and solimp/solmix properties.This matches the existing solimp/solmix patterns and should behave correctly for multi-world models, as exercised by the new tests. No issues found.
Also applies to: 1133-1134, 1372-1375, 1995-1997, 2252-2253, 2275-2277, 2287-2288
Signed-off-by: Gordon Yeoman <gyeoman@nvidia.com>
Signed-off-by: Gordon Yeoman <gyeoman@nvidia.com>
… a separate PR. Signed-off-by: Gordon Yeoman <gyeoman@nvidia.com>
adenzler-nvidia
left a comment
There was a problem hiding this comment.
one small nitpick, thanks a lot.
|
also, the failing tests are not your fault, looks like they have been flaky. I hope a retry fixes this. |
|
I removed everything related to geom_margin and renamed the PR. I'll need to work out what work needs to be done for geom_margin. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
newton/tests/test_import_mjcf.py (1)
1431-1473: Minor: update comment to mentiongaprather thansolimpThe test logic looks correct, but the inline comment above
expected_valuesstill talks about “solimp” values; it should describe thegapattribute and the default of0.0for shapes without an explicitgapto avoid confusion for future readers.newton/tests/test_import_usd.py (1)
1314-1314: Clean up unused noqa directive.The static analysis tool flags this
noqadirective as unused. Remove it for cleaner code.Apply this diff:
- """Test that geom_gap attribute is parsed correctly from USD.""" - from pxr import Usd # noqa: PLC0415 + """Test that geom_gap attribute is parsed correctly from USD.""" + from pxr import Usd
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
newton/_src/solvers/mujoco/kernels.py(3 hunks)newton/_src/solvers/mujoco/solver_mujoco.py(7 hunks)newton/tests/test_import_mjcf.py(1 hunks)newton/tests/test_import_usd.py(1 hunks)newton/tests/test_mujoco_solver.py(1 hunks)
🧰 Additional context used
🧠 Learnings (10)
📚 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-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.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-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, despite the general warp documentation suggesting copies are made by default.
Applied to files:
newton/_src/solvers/mujoco/kernels.py
📚 Learning: 2025-08-18T15:56:26.587Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 552
File: newton/_src/solvers/mujoco/solver_mujoco.py:0-0
Timestamp: 2025-08-18T15:56:26.587Z
Learning: In Newton's MuJoCo solver, when transforming joint axes from Newton's internal frame to MuJoCo's expected frame, use wp.quat_rotate(joint_rot, axis) not wp.quat_rotate_inv(joint_rot, axis). The joint_rot represents rotation from joint-local to body frame, so forward rotation is correct.
Applied to files:
newton/_src/solvers/mujoco/kernels.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/kernels.pynewton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-08-19T12:27:30.629Z
Learnt from: preist-nvidia
Repo: newton-physics/newton PR: 579
File: newton/examples/example_mujoco.py:350-354
Timestamp: 2025-08-19T12:27:30.629Z
Learning: In Newton examples, Example.use_mujoco is a high-level attribute that controls whether to use MuJoCo solver vs other solvers (like XPBD), while SolverMuJoCo.use_mujoco_cpu controls the backend within MuJoCo (CPU vs Warp). These are separate concepts serving different purposes - the PR rename only applies to the solver parameter, not the Example class attributes.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-08-19T12:27:30.629Z
Learnt from: preist-nvidia
Repo: newton-physics/newton PR: 579
File: newton/examples/example_mujoco.py:350-354
Timestamp: 2025-08-19T12:27:30.629Z
Learning: In Newton examples, there's a distinction between solver parameters and Example class attributes. The Example class can have its own use_mujoco attribute for controlling example-level behavior (like CUDA graphs, rendering logic), while the solver uses use_mujoco_cpu for backend selection. These serve different purposes and should not be conflated during API renames.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
🧬 Code graph analysis (2)
newton/tests/test_import_usd.py (1)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
register_custom_attributes(154-311)
newton/_src/solvers/mujoco/solver_mujoco.py (2)
newton/_src/sim/builder.py (3)
add_custom_attribute(640-685)ModelBuilder(70-5934)CustomAttribute(312-402)newton/_src/sim/model.py (2)
ModelAttributeFrequency(49-70)ModelAttributeAssignment(31-46)
🪛 Ruff (0.14.8)
newton/tests/test_import_usd.py
1314-1314: 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 Tests / Run GPU Unit Tests on AWS EC2
- GitHub Check: Run GPU Benchmarks / Run GPU Benchmarks on AWS EC2
🔇 Additional comments (7)
newton/tests/test_mujoco_solver.py (1)
2401-2497: geom_gap conversion/update test is consistent and well-scopedThis test mirrors the existing per‑shape geom_solimp/geom_solmix coverage, correctly exercises multi‑world mapping via
mjc_geom_to_newton_shape, and validates both initial conversion andnotify_model_changed(SHAPE_PROPERTIES)updates intogeom_gap. I don’t see any functional issues here.newton/_src/solvers/mujoco/kernels.py (1)
1194-1205: geom_gap wiring inupdate_geom_properties_kernelmatches existing patterns; verify call sitesAdding
shape_geom_gapas an input and writing it into per‑worldgeom_gapmirrors howshape_geom_solimp/shape_geom_solmixare handled, and it lines up withcontact_params/convert_newton_contacts_to_mjwarp_kernelusinggeom_gapto computemargin - gapforcontact_includemargin. The kernel logic itself looks sound; the main thing to double‑check is that every launch ofupdate_geom_properties_kernelhas been updated to pass the newshape_geom_gapandgeom_gaparguments in the correct order so you don’t hit runtime signature mismatches.You can quickly list the call sites with:
#!/bin/bash # List all calls to update_geom_properties_kernel to verify their argument lists. rg -n "update_geom_properties_kernel" newton -SAlso applies to: 1247-1249
newton/_src/solvers/mujoco/solver_mujoco.py (5)
207-218: Verify that custom attribute is the correct approach for geom_gap.Based on a past review comment,
geom_marginshould be populated fromshape_contact_marginrather than as a custom attribute. Before finalizing this PR, confirm whethergeom_gapshould similarly be populated from an existing Newton shape property, or if the custom attribute approach is intentional and correct.As per past review comments, verify that custom attribute registration is the intended approach for this property.
1121-1121: LGTM!Correctly retrieves the custom attribute following the established pattern.
1359-1360: LGTM!Correctly passes geom_gap to MuJoCo geom parameters when available, following the established pattern.
1981-1981: LGTM!Correctly adds geom_gap to the expansion list for multi-world support.
2237-2237: LGTM!Correctly wires geom_gap through the update_geom_properties flow, maintaining consistency with other shape-level custom attributes (geom_solimp, geom_solmix).
Also applies to: 2259-2259, 2270-2270
d66ef03
Signed-off-by: Gordon Yeoman gyeoman@nvidia.com
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
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.