Reduce default contact stiffness and damping#1285
Conversation
📝 WalkthroughWalkthroughDefaults for shape contact stiffness/damping were lowered in ShapeConfig; a new Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ 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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (3)
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
newton/_src/sim/builder.pynewton/_src/solvers/mujoco/kernels.pynewton/_src/solvers/mujoco/solver_mujoco.pynewton/tests/test_cloth.pynewton/tests/test_rigid_contact.py
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: lenroe-nv
Repo: newton-physics/newton PR: 1248
File: newton/_src/geometry/contact_data.py:47-49
Timestamp: 2025-12-13T17:26:39.791Z
Learning: In ContactData (newton/_src/geometry/contact_data.py), the fields contact_stiffness, contact_damping, and contact_friction_scale use 0.0 as the "unset" sentinel value. This is intentional: Warp struct fields initialize to 0.0 by default, and solver-side code interprets 0.0 as "use default/unscaled" rather than "disable." This design avoids explicit initialization in contact generation kernels.
📚 Learning: 2025-12-13T17:26:39.791Z
Learnt from: lenroe-nv
Repo: newton-physics/newton PR: 1248
File: newton/_src/geometry/contact_data.py:47-49
Timestamp: 2025-12-13T17:26:39.791Z
Learning: In ContactData (newton/_src/geometry/contact_data.py), the fields contact_stiffness, contact_damping, and contact_friction_scale use 0.0 as the "unset" sentinel value. This is intentional: Warp struct fields initialize to 0.0 by default, and solver-side code interprets 0.0 as "use default/unscaled" rather than "disable." This design avoids explicit initialization in contact generation kernels.
Applied to files:
newton/_src/sim/builder.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-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.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-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
📚 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-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
🧬 Code graph analysis (2)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
newton/_src/solvers/mujoco/kernels.py (1)
convert_solref(169-183)
newton/tests/test_rigid_contact.py (1)
newton/tests/unittest_utils.py (1)
add_function_test(320-339)
🪛 Ruff (0.14.10)
newton/_src/solvers/mujoco/solver_mujoco.py
1389-1389: Undefined name ke
(F821)
⏰ 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). (3)
- GitHub Check: Run GPU Tests / Run GPU Unit Tests on AWS EC2
- GitHub Check: Run GPU Benchmarks / Run GPU Benchmarks on AWS EC2
- GitHub Check: run-newton-tests / newton-unittests (windows-latest)
🔇 Additional comments (9)
newton/tests/test_cloth.py (1)
304-306: Appropriate override to preserve cloth test stability.These explicit settings correctly preserve the previous default stiffness (
1.0e5) and damping (1.0e3) values for cloth tests after the global defaults were reduced by two orders of magnitude. The placement immediately after builder creation ensures all subsequentadd_cloth_meshcalls use these values.Consider adding a brief inline comment explaining these are the pre-PR#1285 defaults retained for cloth stability, so future maintainers understand the intent.
newton/_src/sim/builder.py (1)
149-154: ShapeConfig ke/kd default reductions look correct and consistentLowering
keto1.0e3andkdto100.0aligns with the PR goal of reducing contact stiffness/damping for stability, while keeping them positive and non-sentinel values (avoids any confusion with the 0.0 “unset” convention used in contact data). No additional changes are required in this file. Based on learnings, these defaults should remain non-zero.newton/_src/solvers/mujoco/solver_mujoco.py (3)
54-54: LGTM!The import of
convert_solrefaligns with its usage inadd_geomsbelow.
1122-1123: LGTM!Extracting
shape_keandshape_kdfrom the model to propagate material stiffness/damping into MuJoCo'ssolrefis consistent with the PR objective.
1374-1376: LGTM!Using
convert_solrefto derivesolreffrom shape stiffness/damping with fixedd_width=1.0andd_r=1.0appropriately maps Newton's material parameters to MuJoCo's time constant and damping ratio.newton/_src/solvers/mujoco/kernels.py (2)
168-183: LGTM!The
convert_solrefhelper correctly derives MuJoCo's time constant and damping ratio from stiffness/damping, with appropriate fallback to defaults when either parameter is non-positive. The formulas align with the MuJoCo solver parameter documentation.
1278-1280: LGTM!The updated kernel uses the unified
convert_solrefhelper, and the comment clarifies that positive solref values are used (rather than the negative stiffness/damping convention) to support solmix-based blending between shapes.newton/tests/test_rigid_contact.py (2)
724-789: LGTM!This test provides good coverage for the updated contact stiffness/damping defaults by:
- Using an energy-based velocity bound (
v_max = sqrt(2*g*max_drop)) to verify contacts properly dissipate energy- Checking final rest conditions (position constraints and low velocities)
The approach is physically grounded and will catch stability regressions from the parameter changes.
846-861: LGTM!The registration follows existing patterns and correctly filters solver/device combinations (skipping mujoco_warp on CPU and mujoco_cpu on CUDA).
|
Looks good from my side. Are there any importers setting these values? How do the defaults there differ from the ones you changed here? Is the MJCF -> MuJoCo roundtrip still correct? |
|
answering myself, we don't have MJCF parsing yet, planned in #1125 |
Co-authored-by: adenzler-nvidia <adenzler@nvidia.com>
Co-authored-by: adenzler-nvidia <adenzler@nvidia.com>
Description
This PR adjusts the default contact stiffness and damping values in
ShapeConfigto resolve instability issues introduced in #1085.Since #1085, the
keandkdvalues ofShapeConfigentergeom_solref; however, this change reduced the time constant by a factor of 10, resulting in unstable collisions withSolverMuJoCoand default shape settings. This resulted in a time constant of ~2 ms, which requires a simulation frequency of at least 1 kHz to remain stable withSolverMuJoCo.This change also propagates
geom_solrefinto the mjc spec.Resolves #1243
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.