Skip to content

Reduce default contact stiffness and damping#1285

Merged
adenzler-nvidia merged 8 commits into
newton-physics:mainfrom
camevor:shape-contact-stiffness
Jan 8, 2026
Merged

Reduce default contact stiffness and damping#1285
adenzler-nvidia merged 8 commits into
newton-physics:mainfrom
camevor:shape-contact-stiffness

Conversation

@camevor

@camevor camevor commented Dec 22, 2025

Copy link
Copy Markdown
Member

Description

This PR adjusts the default contact stiffness and damping values in ShapeConfig to resolve instability issues introduced in #1085.

Since #1085, the ke and kd values of ShapeConfig enter geom_solref; however, this change reduced the time constant by a factor of 10, resulting in unstable collisions with SolverMuJoCo and 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 with SolverMuJoCo.

This change also propagates geom_solref into 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.

  • The migration guide in docs/migration.rst is up-to date

Before your PR is "Ready for review"

  • Necessary tests have been added and new examples are tested (see newton/tests/test_examples.py)
  • Documentation is up-to-date
  • Code passes formatting and linting checks with pre-commit run -a

Summary by CodeRabbit

  • Improvements
    • Lowered default contact stiffness and damping for gentler, more stable collisions
    • Unified conversion of contact parameters for the MuJoCo solver for consistent solver behavior
  • Tests
    • Added end-to-end validation for contact dynamics and a rigid-body drop scenario
    • Tests include explicit overrides of shape defaults to preserve legacy expectations
  • Bug Fixes
    • Removed duplicate test registration to prevent redundant executions (addressed)

✏️ Tip: You can customize this high-level summary in your review settings.

@coderabbitai

coderabbitai Bot commented Dec 22, 2025

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

Defaults for shape contact stiffness/damping were lowered in ShapeConfig; a new convert_solref() helper centralizes conversion of ke/kd → MuJoCo solref; solver geometry setup now uses per-shape ke/kd to set geom solref; tests updated and a new rigid-contact test was added (with duplicated entries).

Changes

Cohort / File(s) Summary
Default parameter adjustment
newton/_src/sim/builder.py
Lowered ShapeConfig defaults: ke from 1.0e51.0e3, kd from 1000.0100.0.
Solref conversion helper
newton/_src/solvers/mujoco/kernels.py
Added convert_solref(ke,kd,d_width,d_r) to compute MuJoCo (timeconst, dampratio); replaced inline per-geom solref computation with this helper.
Solver geometry pipeline
newton/_src/solvers/mujoco/solver_mujoco.py
Read shape_ke/shape_kd into arrays, import convert_solref, and assign computed solref for each geom during add_geoms() (used instead of prior inline math).
Test configuration
newton/tests/test_cloth.py
Test harness sets builder.default_shape_cfg.ke=1.0e5, kd=1.0e3 in two locations to restore previous cloth defaults.
New contact test
newton/tests/test_rigid_contact.py
Added test_box_drop() with gravity-based velocity assertions and rest checks; duplicate function and registration blocks were also introduced and need deduplication.

Sequence Diagram(s)

mermaid
sequenceDiagram
autonumber
participant Builder as ModelBuilder (spec)
participant SolverSetup as solver_mujoco.add_geoms
participant Kernels as kernels.convert_solref
participant MuJoCo as MuJoCo geom params
Builder->>SolverSetup: provide per-shape shape_ke, shape_kd
SolverSetup->>Kernels: call convert_solref(ke,kd,d_width,d_r)
Kernels-->>SolverSetup: return (timeconst, dampratio)
SolverSetup->>MuJoCo: set geom.solref = returned values
Note right of MuJoCo #DDEBF7: geom solref now present in model spec

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested labels

bug

Suggested reviewers

  • adenzler-nvidia
  • eric-heiden

Pre-merge checks and finishing touches

❌ Failed checks (2 warnings)
Check name Status Explanation Resolution
Out of Scope Changes check ⚠️ Warning The test_rigid_contact.py file contains duplicate test definitions and registration blocks for test_box_drop, and test_cloth.py sets hardcoded ke/kd values inconsistent with the reduced defaults. Remove duplicate test_box_drop definition and registration blocks; update test_cloth.py to use the new default values or document why overrides are necessary.
Docstring Coverage ⚠️ Warning Docstring coverage is 55.56% which is insufficient. The required threshold is 80.00%. You can run @coderabbitai generate docstrings to improve docstring coverage.
✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Reduce default contact stiffness and damping' clearly and concisely describes the main change: lowering ke and kd default values in ShapeConfig as shown in the code modifications.
Linked Issues check ✅ Passed The PR addresses issue #1243 by introducing convert_solref helper, propagating shape_ke/kd into geom_solref during spec construction, ensuring solref values appear in exported MJCF models as required.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d233cc3 and b492c5a.

📒 Files selected for processing (1)
  • newton/_src/solvers/mujoco/solver_mujoco.py
🚧 Files skipped from review as they are similar to previous changes (1)
  • newton/_src/solvers/mujoco/solver_mujoco.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). (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)

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@codecov

codecov Bot commented Dec 22, 2025

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ All tests successful. No failed tests found.

📢 Thoughts on this report? Let us know!

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

📜 Review details

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e5f6493 and d233cc3.

📒 Files selected for processing (5)
  • newton/_src/sim/builder.py
  • newton/_src/solvers/mujoco/kernels.py
  • newton/_src/solvers/mujoco/solver_mujoco.py
  • newton/tests/test_cloth.py
  • newton/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.py
  • newton/_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.py
  • 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-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 subsequent add_cloth_mesh calls 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 consistent

Lowering ke to 1.0e3 and kd to 100.0 aligns 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_solref aligns with its usage in add_geoms below.


1122-1123: LGTM!

Extracting shape_ke and shape_kd from the model to propagate material stiffness/damping into MuJoCo's solref is consistent with the PR objective.


1374-1376: LGTM!

Using convert_solref to derive solref from shape stiffness/damping with fixed d_width=1.0 and d_r=1.0 appropriately 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_solref helper 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_solref helper, 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:

  1. Using an energy-based velocity bound (v_max = sqrt(2*g*max_drop)) to verify contacts properly dissipate energy
  2. 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).

Comment thread newton/_src/solvers/mujoco/solver_mujoco.py Outdated
@adenzler-nvidia

Copy link
Copy Markdown
Member

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?

@adenzler-nvidia

Copy link
Copy Markdown
Member

answering myself, we don't have MJCF parsing yet, planned in #1125

@adenzler-nvidia adenzler-nvidia added this pull request to the merge queue Jan 8, 2026
Merged via the queue into newton-physics:main with commit 91cee52 Jan 8, 2026
20 checks passed
@camevor camevor deleted the shape-contact-stiffness branch January 19, 2026 09:31
@coderabbitai coderabbitai Bot mentioned this pull request Jan 27, 2026
4 tasks
eric-heiden pushed a commit to eric-heiden/newton that referenced this pull request Jan 28, 2026
Co-authored-by: adenzler-nvidia <adenzler@nvidia.com>
mmacklin pushed a commit to mmacklin/newton that referenced this pull request Apr 7, 2026
Co-authored-by: adenzler-nvidia <adenzler@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

add geom_solref to spec model build

3 participants