SolverMuJoCo: remove global condim override by adenzler-nvidia · Pull Request #1052 · newton-physics/newton

adenzler-nvidia · 2025-11-07T12:56:22Z

Can now be done in fine-grained fashion be adjusting the custom solver attributes.

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

Refactor
- Removed geom_condim parameter to streamline MuJoCo solver configuration.
New Features
- Implemented custom attributes registration mechanism for MuJoCo solver, enabling enhanced integration capabilities in benchmarks and example workflows.

Signed-off-by: Alain Denzler <adenzler@nvidia.com>

coderabbitai · 2025-11-07T12:56:36Z

📝 Walkthrough

Walkthrough

The PR refactors MuJoCo solver integration by removing the geom_condim parameter from the _convert_to_mjc function and introducing a register_custom_attributes registration pattern. This pattern is now applied across multiple example files and benchmarks before MJCF parsing.

Changes

Cohort / File(s)	Summary
MuJoCo Solver Core Changes `newton/_src/solvers/mujoco/solver_mujoco.py`	Removed `geom_condim: int = 3` parameter from `_convert_to_mjc` function signature and deleted corresponding `defaults.geom.condim` assignment.
Custom Attributes Registration Calls `asv/benchmarks/benchmark_mujoco.py`, `newton/examples/example_recording.py`, `newton/examples/selection/example_selection_articulations.py`, `newton/examples/sensors/example_sensor_contact.py`	Added `newton.solvers.SolverMuJoCo.register_custom_attributes()` calls with appropriate target objects (articulation_builder or world) during initialization, before MJCF asset processing.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Review focus areas:
- Verify all call sites of _convert_to_mjc have been updated to remove geom_condim argument passing
- Confirm register_custom_attributes is invoked at the correct initialization point in each example
- Ensure the parameter removal doesn't have downstream dependencies or side effects outside the diff

Possibly related PRs

Improve the logic with njmax and nconmax #787: Modifies _convert_to_mjc function signature in the MuJoCo solver—may have overlapping parameter changes or related refactoring.
Improve SolverMuJoCo setup performance #778: Updates MuJoCo solver implementation and SolverMuJoCo usage—likely shares related architectural changes.

Suggested reviewers

shi-eric
eric-heiden

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	You can run `@coderabbitai generate docstrings` to improve docstring coverage.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically describes the main change: removing the global condim override from SolverMuJoCo. It is concise, directly related to the primary modification evident across multiple files.

✨ 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 a39fc71 and 3387767.

📒 Files selected for processing (5)

asv/benchmarks/benchmark_mujoco.py (1 hunks)
newton/_src/solvers/mujoco/solver_mujoco.py (0 hunks)
newton/examples/example_recording.py (1 hunks)
newton/examples/selection/example_selection_articulations.py (1 hunks)
newton/examples/sensors/example_sensor_contact.py (1 hunks)

💤 Files with no reviewable changes (1)

newton/_src/solvers/mujoco/solver_mujoco.py

🧰 Additional context used

🧠 Learnings (4)

📓 Common learnings

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.

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.

📚 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:

asv/benchmarks/benchmark_mujoco.py
newton/examples/sensors/example_sensor_contact.py
newton/examples/example_recording.py
newton/examples/selection/example_selection_articulations.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:

asv/benchmarks/benchmark_mujoco.py
newton/examples/sensors/example_sensor_contact.py
newton/examples/example_recording.py
newton/examples/selection/example_selection_articulations.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/examples/example_recording.py

🧬 Code graph analysis (4)

asv/benchmarks/benchmark_mujoco.py (1)

newton/_src/solvers/mujoco/solver_mujoco.py (1)

register_custom_attributes (152-168)

newton/examples/sensors/example_sensor_contact.py (1)

newton/_src/solvers/mujoco/solver_mujoco.py (1)

register_custom_attributes (152-168)

newton/examples/example_recording.py (1)

newton/_src/solvers/mujoco/solver_mujoco.py (1)

register_custom_attributes (152-168)

newton/examples/selection/example_selection_articulations.py (2)

newton/_src/solvers/mujoco/solver_mujoco.py (1)

register_custom_attributes (152-168)

newton/_src/solvers/solver.py (1)

register_custom_attributes (307-314)

⏰ 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 Unit Tests on AWS EC2 (Pull Request)
GitHub Check: Run GPU Benchmarks (Pull Request)

🔇 Additional comments (4)

newton/examples/sensors/example_sensor_contact.py (1)

101-101: LGTM! Custom attribute registration follows the correct pattern.

The call to register_custom_attributes is correctly placed after ModelBuilder creation and before adding MJCF assets, ensuring MuJoCo-specific custom attributes (like condim) are declared before they're needed during asset parsing.

asv/benchmarks/benchmark_mujoco.py (1)

384-384: LGTM! Registration correctly precedes asset loading.

The custom attribute registration is properly positioned before any robot setup functions that load MJCF/USD/URDF assets, ensuring MuJoCo-specific attributes are available during asset parsing.

newton/examples/selection/example_selection_articulations.py (1)

98-98: LGTM! Correct placement for custom attribute registration.

The registration is properly positioned after ModelBuilder creation and before MJCF asset loading, following the established pattern across the codebase.

newton/examples/example_recording.py (1)

60-60: LGTM! Registration correctly positioned before MJCF parsing.

The custom attribute registration is properly placed before parse_mjcf, ensuring MuJoCo-specific attributes are declared before the MJCF asset is parsed and the builder is subsequently reused in the replication loop.

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 · 2025-11-07T13:02:30Z

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!

Signed-off-by: Alain Denzler <adenzler@nvidia.com>

remove default condim override from MuJoCo solver initialization

3387767

Signed-off-by: Alain Denzler <adenzler@nvidia.com>

Merge branch 'main' into dev/adenzler/remove-condim-option

cf6ac96

eric-heiden approved these changes Nov 10, 2025

View reviewed changes

eric-heiden enabled auto-merge (squash) November 10, 2025 16:20

eric-heiden and others added 2 commits November 10, 2025 08:20

Merge branch 'main' into dev/adenzler/remove-condim-option

12bb946

Merge branch 'main' into dev/adenzler/remove-condim-option

1154da9

eric-heiden merged commit 561126b into newton-physics:main Nov 10, 2025
18 checks passed

This was referenced Nov 13, 2025

Move geom_solref to negative convention with shape stiffness/damping #1065 #1085

Merged

SolverMuJoCo: SolimpLimit parsing and update support #1047

Merged

This was referenced Dec 8, 2025

SolverMuJoCo: custom attributes for MJCF/MJC schema options #1211

Merged

Add custom attribute for geom_solmix #1223

Merged

coderabbitai Bot mentioned this pull request Dec 10, 2025

SolverMuJoCo: Custom attributes for contact pair #1234

Merged

4 tasks

coderabbitai Bot mentioned this pull request Dec 25, 2025

Test SensorContact with SolverMuJoCo #1292

Merged

4 tasks

coderabbitai Bot mentioned this pull request Jan 20, 2026

Call SolverMuJoCo.register_custom_attributes() from add_mjcf() #1411

Merged

eric-heiden pushed a commit to eric-heiden/newton that referenced this pull request Jan 28, 2026

SolverMuJoCo: remove global condim override (newton-physics#1052)

7ac8bfa

Signed-off-by: Alain Denzler <adenzler@nvidia.com>

coderabbitai Bot mentioned this pull request Feb 4, 2026

USD Plumbing MJC solver attributes through resolver and custom attribute framework #1463

Merged

4 tasks

coderabbitai Bot mentioned this pull request Feb 24, 2026

Clean up unused and internal-only kwargs in SolverMuJoCo #1766

Merged

This was referenced Mar 5, 2026

Document Newton model to MuJoCo model conversions #1943

Merged

Improve MuJoCo contact handling in ik_cube_stacking example #1964

Merged

mmacklin pushed a commit to mmacklin/newton that referenced this pull request Apr 7, 2026

SolverMuJoCo: remove global condim override (newton-physics#1052)

cb6c4ba

Signed-off-by: Alain Denzler <adenzler@nvidia.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SolverMuJoCo: remove global condim override#1052

SolverMuJoCo: remove global condim override#1052
eric-heiden merged 4 commits into
newton-physics:mainfrom
adenzler-nvidia:dev/adenzler/remove-condim-option

adenzler-nvidia commented Nov 7, 2025 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Nov 7, 2025 •

edited

Loading

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested reviewers

Uh oh!

codecov Bot commented Nov 7, 2025 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

adenzler-nvidia commented Nov 7, 2025 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Before your PR is "Ready for review"

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Estimated code review effort

Possibly related PRs

Suggested reviewers

Pre-merge checks and finishing touches

Uh oh!

codecov Bot commented Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

adenzler-nvidia commented Nov 7, 2025 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Nov 7, 2025 •

edited

Loading

codecov Bot commented Nov 7, 2025 •

edited

Loading