SolverMuJoCo: custom attributes for MJCF/MJC schema options #1211
Conversation
Signed-off-by: Alain Denzler <[email protected]>
Signed-off-by: Alain Denzler <[email protected]>
Signed-off-by: Alain Denzler <[email protected]>
Signed-off-by: Alain Denzler <[email protected]>
Signed-off-by: Alain Denzler <[email protected]>
Signed-off-by: Alain Denzler <[email protected]>
Signed-off-by: Alain Denzler <[email protected]>
Signed-off-by: Alain Denzler <[email protected]>
📝 WalkthroughWalkthroughThis PR adds per-world solver option support to the MuJoCo solver integration, specifically for the "impratio" parameter. It introduces optional parsing of solver options from MJCF and USD imports, wires per-world values through model expansion using a new kernel, and makes impratio a constructor-overridable parameter in SolverMuJoCo. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ImportUtil as Import Utility
participant Builder as ModelBuilder
participant Solver as SolverMuJoCo
participant Kernel as Warp Kernel
participant MuJoCo as MuJoCo Warp Model
User->>ImportUtil: parse_mjcf/parse_usd(parse_options=True)
ImportUtil->>ImportUtil: Extract impratio from source
ImportUtil->>Builder: Set custom_attributes["impratio"]
User->>Solver: __init__(impratio=value or None)
Solver->>Builder: Resolve impratio: constructor → custom attribute → default
Solver->>Solver: _convert_to_mjc()
Solver->>MuJoCo: Apply impratio to opt
Solver->>Solver: expand_model_fields() (per-world)
Solver->>Solver: _update_solver_options()
Solver->>Kernel: Launch update_solver_options_kernel()
Kernel->>MuJoCo: Compute opt.impratio_invsqrt per world
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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! |
![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 (4)
newton/tests/test_import_usd.py (1)
2193-2259: LGTM! Well-structured test for USD impratio parsing.The test correctly validates that
impratiois parsed from the USD PhysicsScene attributemjc:option:impratioand exposed onmodel.mujoco.impratio. The test follows the established pattern from the MJCF equivalent test and properly registers custom attributes before parsing.Minor linting: Remove unused noqa directive.
Line 2196 has an unused
noqa: PLC0415directive since PLC0415 is not enabled in your linter configuration.Apply this diff:
- from pxr import Usd # noqa: PLC0415 + from pxr import Usdnewton/tests/test_mujoco_solver.py (1)
3512-3632: WORLD-frequency impratio tests look correct and align with solver semanticsBoth tests follow existing patterns (template builder + replicate + register_custom_attributes) and exercise the key behaviors:
test_impratio_multiworld_conversion:
- Confirms the WORLD-frequency attribute exists, has per-world length, and that
SolverMuJoCopropagates distinct per-world values intomjw_model.opt.impratio.- The way you assign via
model.mujoco.impratio.assign(initial_impratio)and then assert onsolver.mjw_model.opt.impratio.numpy()matches how other scalar custom-attribute tests are written.
test_impratio_constructor_override:
- Correctly verifies that a constructor
impratio=3.0wins over model attributes for bothmj_model.opt.impratioandmjw_model.opt.impratio.- Using
float(mjw_impratio.flat[0])is fine; if you ever want stronger coverage, you could also assert all entries equal 3.0 for multi-world, but that’s optional.I don’t see functional issues here; these tests give good coverage of the new option path and precedence rules.
newton/_src/solvers/mujoco/solver_mujoco.py (2)
871-895: Impratio resolution and application order is correct; single-world CPU vs multi-world Warp behavior is clearThe new
_convert_to_mjchandling forimpratiohas good semantics:
impratio_overridden = impratio is not Nonecleanly captures “constructor vs model attribute” precedence.- If no constructor value is given, you fall back to
model.mujoco.impratio(when registered), usingnumpy()[0]as the scalar to seedspec.option.impratio. That’s appropriate for the single global MuJoCo option (CPU path) and as a base value for the Warp model before per‑world expansion.- If neither ctor arg nor custom attribute is present, you leave
spec.option.impratioat MuJoCo’s own default.Setting
spec.option.impratio = impratioonly when the value is non‑None avoids unintentionally overriding MuJoCo defaults.One minor nuance: in multi‑world Warp runs, the scalar from world 0 is only used as an initial value, then overwritten by
update_solver_optionswith the full WORLD-frequency array, which matches the tests and your design.Overall this logic looks correct and matches the documented precedence.
Also applies to: 973-981, 989-991
1760-1766: update_solver_options correctly gates constructor overrides and avoids None inputs; no runtime updates by design
- Calling
self.update_solver_options(impratio_overridden=impratio_overridden)immediately afterexpand_model_fieldsis the right spot: the Warp model has been tiled tonworld, butnotify_model_changedhasn’t yet run.- In
update_solver_options:
- You re-fetch
model.mujoco.impratioonly when it exists andimpratiowas not overridden by constructor, so constructor values remain authoritative.all_noneis used to guard the kernel launch, soupdate_solver_options_kernelis never called with aNoneinput.dim=nworldmatchesself.model.num_worlds, which equals the Warpnworldin all valid configurations (multi-world requiresseparate_worlds=True; single-world impliesnum_worlds == 1).By not wiring this into
notify_model_changed, you’re explicitly making solver options static after construction (no runtime updates from attributes), which matches the PR description and avoids conflicts with constructor overrides.Looks functionally sound.
Also applies to: 1891-1919
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
newton/_src/sim/builder.py(2 hunks)newton/_src/sim/model.py(1 hunks)newton/_src/solvers/mujoco/kernels.py(1 hunks)newton/_src/solvers/mujoco/solver_mujoco.py(10 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(1 hunks)
🧰 Additional context used
🧠 Learnings (15)
📓 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.
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 479
File: newton/solvers/mujoco/solver_mujoco.py:2147-2149
Timestamp: 2025-07-28T15:04:54.360Z
Learning: In the Newton MuJoCo solver implementation, the ls_parallel option may not be available in the pure MuJoCo mjOption structure, despite being documented in newer MuJoCo versions. The user adenzler-nvidia confirmed that mj_model does not have this attribute in their implementation, so ls_parallel should only be set on mjw_model.opt.
📚 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/_src/sim/model.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.pynewton/_src/solvers/mujoco/kernels.pynewton/tests/test_mujoco_solver.py
📚 Learning: 2025-08-12T05:17:34.423Z
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.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.pynewton/_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, 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.pynewton/tests/test_mujoco_solver.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.pynewton/_src/solvers/mujoco/kernels.py
📚 Learning: 2025-08-12T05:17:34.423Z
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 STYLE3D solver in Newton is a new solver that did not exist in warp.sim, so it does not require any migration documentation.
Applied to files:
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
📚 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-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-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-07-14T03:57:29.670Z
Learnt from: Kenny-Vilella
Repo: newton-physics/newton PR: 398
File: newton/examples/example_mujoco.py:352-352
Timestamp: 2025-07-14T03:57:29.670Z
Learning: The use_mujoco option in newton/examples/example_mujoco.py is currently unsupported and causes crashes. The code automatically disables this option with a warning message when users attempt to enable it. This is intentionally kept as a placeholder for future implementation.
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
📚 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_import_usd.py
🧬 Code graph analysis (4)
newton/_src/sim/builder.py (1)
newton/_src/sim/model.py (1)
ModelAttributeFrequency(49-72)
newton/tests/test_import_mjcf.py (1)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
register_custom_attributes(152-298)
newton/_src/utils/import_usd.py (3)
newton/_src/sim/builder.py (3)
ModelBuilder(70-5757)CustomAttribute(308-398)key(391-393)newton/_src/sim/model.py (1)
ModelAttributeFrequency(49-72)newton/_src/usd/utils.py (1)
get_custom_attribute_values(451-475)
newton/tests/test_import_usd.py (2)
newton/tests/test_import_mjcf.py (1)
test_option_impratio_parsing(1388-1413)newton/_src/solvers/mujoco/solver_mujoco.py (2)
SolverMuJoCo(78-2198)register_custom_attributes(152-298)
🪛 Ruff (0.14.7)
newton/tests/test_import_usd.py
2196-2196: 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 (11)
newton/_src/utils/import_mjcf.py (1)
146-157: LGTM! Option parsing implementation is correct.The implementation correctly parses MJCF option tags for solver options (ONCE and WORLD frequency attributes). The code appropriately:
- Filters for relevant attribute frequencies
- Checks for element and attribute existence before processing
- Updates the builder's custom attributes at index 0 (with WORLD attributes being remapped later during
add_world()as noted in the comment)This follows the same pattern as other attribute parsing in the file.
newton/tests/test_import_mjcf.py (1)
1388-1463: LGTM! Comprehensive test coverage for option parsing.Both test functions effectively validate the new option parsing functionality:
test_option_impratio_parsingverifies single-world parsing from MJCF option tagstest_option_impratio_per_worldvalidates that per-world impratio values are correctly preserved when merging separate ModelBuildersThe tests appropriately register custom attributes, parse MJCF content, and verify the resulting model attributes. This provides good coverage for both basic parsing and the more complex multi-world scenario.
newton/_src/sim/model.py (1)
71-72: LGTM! Enum addition is correct.The new
WORLDenum value is properly defined with:
- Sequential value (7) following the existing pattern
- Clear docstring referencing the corresponding
Model.num_worldsattribute- Consistent formatting with other enum entries
This enables per-world attribute frequency tracking as intended by the PR.
newton/_src/sim/builder.py (2)
1772-1827: WORLD-frequency custom attribute remapping inadd_builderlooks correctUsing
self.current_worldas the offset forModelAttributeFrequency.WORLDcleanly remaps a sub-builder’s local world index 0 to the destination builder’s actual world index, which is exactly what you want foradd_world/replicate-style usage. Thecurrent_world < 0 -> 0fallback is also a reasonable default for “no explicit world yet” cases, given thatfinalize()will clampnum_worldsto at least 1.
5632-5651: WORLD-frequency sizing infinalizeis consistent with the new semanticsTreating
ModelAttributeFrequency.WORLDas lengthm.num_worldsaligns with the new WORLD category and with the offsetting logic inadd_builder.build_array(count, ...)will now correctly materialize one element per world, and ONCE/BODY/SHAPE/JOINT*/ARTICULATION paths remain unchanged.newton/_src/solvers/mujoco/kernels.py (1)
875-892: Fixupdate_solver_options_kerneloptional-array handling (if newton_impratio)In a
@wp.kernel,newton_impratiois always awp.array; the docstring comment “None if overridden” and theif newton_impratio:check don’t line up with Warp’s type model:
- Kernels can’t be launched with
Nonefor anwp.array-typed argument; the host code must gate the launch instead.- Testing an array in boolean context (
if newton_impratio:) is at best redundant and at worst a compile-time/runtime error depending on Warp’s codegen, and it doesn’t vary per-thread anyway.Given the caller already knows whether a Newton-side
impratiois available, the kernel can safely assume the array exists and just perform the per-world copy. A minimal, safer version:@wp.kernel def update_solver_options_kernel( - newton_impratio: wp.array(dtype=float), - # outputs - opt_impratio: wp.array(dtype=float), + newton_impratio: wp.array(dtype=float), + # outputs + opt_impratio: wp.array(dtype=float), ): @@ - worldid = wp.tid() - - # Only update if Newton array exists (None means overridden or not available) - if newton_impratio: - opt_impratio[worldid] = newton_impratio[worldid] + worldid = wp.tid() + opt_impratio[worldid] = newton_impratio[worldid]and rely on the host-side logic to skip launching this kernel when solver-constructor options override model attributes. If you truly need optional behavior inside the kernel, pass an explicit
has_newton_impratio: boolflag instead of testing the array object itself.⛔ Skipped due to learnings
Learnt from: adenzler-nvidia Repo: newton-physics/newton PR: 1047 File: newton/_src/solvers/mujoco/solver_mujoco.py:1723-1749 Timestamp: 2025-11-10T12:33:57.428Z Learning: In Warp, it is valid to pass None as a kernel input parameter for optional arrays. Inside the kernel, use `if array:` to check whether the array was provided before accessing it. This pattern eliminates the need for separate presence flags and dummy arrays.Learnt from: eric-heiden Repo: newton-physics/newton PR: 818 File: newton/_src/viewer/viewer_usd.py:249-259 Timestamp: 2025-09-25T16:48:54.358Z Learning: In Warp kernels and functions, use `if array: ...` to check for arrays being not None, rather than explicit `if array is not None:` checks. This is the correct and idiomatic way in the Warp ecosystem.Learnt from: WenchaoHuang Repo: newton-physics/newton PR: 835 File: newton/_src/solvers/style3d/collision/collision.py:49-53 Timestamp: 2025-09-26T05:58:21.013Z Learning: In newton/_src/solvers/style3d/collision/collision.py, the broad_phase buffers (broad_phase_ee, broad_phase_ef, broad_phase_vf) created with wp.array() don't need zero-initialization because they follow a write-first-read-later pattern: BVH query kernels in frame_begin() populate these buffers completely (write-only operations), and collision kernels only read from them afterward in accumulate_contact_force(), ensuring no undefined reads occur.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()`.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.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.Learnt from: nvlukasz Repo: newton-physics/newton PR: 519 File: newton/_src/solvers/featherstone/kernels.py:75-75 Timestamp: 2025-08-12T18:04:06.577Z Learning: The Newton physics framework requires nightly Warp builds, which means compatibility concerns with older stable Warp versions (like missing functions such as wp.spatial_adjoint) are not relevant for this project.Learnt from: WenchaoHuang Repo: newton-physics/newton PR: 835 File: newton/_src/solvers/style3d/collision/collision.py:49-53 Timestamp: 2025-09-26T05:58:21.013Z Learning: In newton/_src/solvers/style3d/collision/collision.py, the broad_phase buffers (broad_phase_ee, broad_phase_ef, broad_phase_vf) created with wp.array() don't need zero-initialization because the BVH query kernels that use them always write the count to query_results[0, tid] = 0 first before populating any data, ensuring no undefined reads occur.Learnt from: nvlukasz Repo: newton-physics/newton PR: 519 File: newton/_src/utils/import_mjcf.py:226-231 Timestamp: 2025-08-12T17:52:15.009Z Learning: In the Newton codebase, when passing array-like objects (numpy arrays, lists, tuples) to wp.vec3(), the consistent pattern is to use the unpacking operator: wp.vec3(*array) rather than wp.vec3(array). This pattern is used throughout newton/_src/utils/import_mjcf.py and newton/_src/core/types.py.Learnt from: gdaviet Repo: newton-physics/newton PR: 750 File: newton/_src/solvers/implicit_mpm/render_grains.py:142-160 Timestamp: 2025-09-09T12:50:42.929Z Learning: The `update_render_grains` function in newton/_src/solvers/implicit_mpm/render_grains.py should return early when `state.velocity_field is None` because its purpose is specifically to advect grains in the velocity field. Without a velocity field, the function should be a no-op.newton/_src/utils/import_usd.py (1)
959-973: LGTM! Correct implementation of WORLD frequency attribute support.The changes properly extend USD import to handle both ONCE and WORLD frequency custom attributes from the PhysicsScene prim. Setting
values[0]for WORLD attributes is correct here—this represents the first world's value, which will be remapped to the appropriate world index duringModelBuilder.add_world()as shown in the builder context. The implementation aligns with the MJCF parsing approach and is well-documented with clear comments.newton/_src/solvers/mujoco/solver_mujoco.py (4)
64-65: Kernel import aligned with new solver-option update pathImporting
update_solver_options_kernelhere is consistent with the later usage inupdate_solver_options. No issues from a dependency or layering perspective.
300-353: Constructor impratio parameter + docstring match behavior and precedence design
- Adding
impratio: float | None = Noneto__init__and documenting it as “constructor > model custom attribute > MuJoCo default (1.0)” is consistent with the implementation in_convert_to_mjc.- Passing
impratiothrough into_convert_to_mjcensures bothmj_modelandmjw_modelsee the same override behavior.- No ordering issues with other parameters; backward compatibility is preserved for callers using keywords.
No functional issues here.
286-298: WORLD-frequency impratio custom attribute registration is correctThe registration is consistent with MuJoCo USD schema conventions:
frequency=ModelAttributeFrequency.WORLDandassignment=ModelAttributeAssignment.MODELcorrectly represent per-world optionsdtype=wp.float32with default1.0matches the option semanticsmjcf_attribute_name="impratio"aligns with the MJCF field nameusd_attribute_name="mjc:option:impratio"is the correct USD schema name per MuJoCo's official USD schema mapping (mjc:option namespace maps 1:1 to MJCF<option>fields)This registration block is solid.
1845-1857: Tiling mjw_model.opt.impratio for multi-world is consistent with other expanded fieldsExtending
expand_model_fieldswithopt_fields_to_expand = {"impratio"}and tilingmj_model.opt.impratioacrossnworldis consistent with how you already expand other per-world fields (e.g.,body_*,geom_*).MuJoCo Warp's
Model.optsupports a leading batch dimension for per-world option values—subfields likeopt.impratiocan be represented as Warp arrays with a leading batch axis, wheremujoco_warp.put_modelindexes per-world usingworldid % batch_size. This matches the existing multi-world expansion pattern usingrepeat_array_kernel, providedopt.impratiostarts with its leading dimension equal to 1 (the standard single-world model case).
Signed-off-by: Alain Denzler <[email protected]>
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 (1)
newton/_src/solvers/mujoco/solver_mujoco.py(10 hunks)
🧰 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.
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 479
File: newton/solvers/mujoco/solver_mujoco.py:2147-2149
Timestamp: 2025-07-28T15:04:54.360Z
Learning: In the Newton MuJoCo solver implementation, the ls_parallel option may not be available in the pure MuJoCo mjOption structure, despite being documented in newer MuJoCo versions. The user adenzler-nvidia confirmed that mj_model does not have this attribute in their implementation, so ls_parallel should only be set on mjw_model.opt.
📚 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-08-12T05:17:34.423Z
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.
Applied to files:
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). (2)
- GitHub Check: Run GPU Benchmarks / Run GPU Benchmarks on AWS EC2
- GitHub Check: Run GPU Tests / Run GPU Unit Tests on AWS EC2
🔇 Additional comments (4)
newton/_src/solvers/mujoco/solver_mujoco.py (4)
313-313: LGTM! Clean precedence implementation.The optional parameter type and resolution logic correctly implement the documented precedence: constructor arguments > model custom attributes > MuJoCo default (1.0).
Reading
impratio.numpy()[0]at line 978 is appropriate here because we're building the template model from the first world. Later,update_solver_optionspropagates per-world values whenimpratio_overridden=False.Also applies to: 339-339, 972-989
1844-1889: LGTM! Proper per-world expansion.The addition of
opt_fields_to_expandand the tiling logic for solver option fields correctly handles per-world replication. Whenimpratiois set via constructor,tile()ensures all worlds receive the same value. When not overridden,update_solver_options(called next) propagates per-world values from the Newton model.
1759-1761: LGTM! Correct placement and clear intent.The call to
update_solver_optionsis correctly placed afterexpand_model_fields(which handles tiling of constructor-overridden values) and beforenotify_model_changed(which propagates other model properties). The comment clearly explains the conditional behavior.
287-297: Verify MJCF parsing support for impratio attribute.The custom attribute registration is well-structured with WORLD frequency, enabling per-world solver option values. The default of 1.0 matches MuJoCo's documented default.
However, the attribute lacks
mjcf_attribute_namewhile the PR title mentions "MJCF/MJC schema options". Other attributes likegeom_priorityandgravcompincludemjcf_attribute_namefor MJCF parsing. Please verify that MJCF files can specify this option, either through the importer handling<option impratio="...">directly or by addingmjcf_attribute_name="impratio".
newton/_src/utils/import_mjcf.py
Outdated
| option_attrs = parse_custom_attributes(option_elem.attrib, builder_custom_attr_option, "mjcf") | ||
| for key, value in option_attrs.items(): | ||
| if key in builder.custom_attributes: | ||
| builder.custom_attributes[key].values[0] = value |
There was a problem hiding this comment.
the parsing support here will overwrite the option values in case there are multiple MJCFs imported per world. I don't think there is a good way to work around this, it's just a limitation of the approach. Similar things happen when you combine multiple MJCFs natively, although the precedence is likely defined somewhere.
I hope it's fine to get some basic parsing support in here but let's consider this PR to be mostly about getting the model conversion right, and iron out any details and issues in parsing at a later point.
There was a problem hiding this comment.
I think what you have is a great first step, users can simply pass parse_options=False if they don't want the other MJCF's to overwrite the per-world settings.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
newton/_src/solvers/mujoco/kernels.py (1)
900-917: Kernel implementation is correct; doc/optional guard could be simplifiedThe per‑world copy
opt_impratio[worldid] = newton_impratio[worldid]is straightforward and aligns with how other kernels index worlds. Given the host now skips launching this kernel whennewton_impratioisNone, theif newton_impratio:guard and the “(None if overridden)” part of the docstring are effectively redundant; you can either keep them for future optional‑array use or drop them for clarity.newton/_src/solvers/mujoco/solver_mujoco.py (3)
289-300: WORLD-scopedimpratiocustom attribute wiring looks consistentRegistering
impratioas aModelAttributeFrequency.WORLDattribute in themujoconamespace with default1.0matches the intended per‑world solver option semantics and the later access viamodel.mujoco.impratio. The USD namemjc:option:impratioalso aligns with the stated schema path.If you expect MJCF
<option impratio="...">to populate this as well, double‑check that the MJCF importer uses the same name so both paths converge on this attribute.
316-343: Constructor and conversion impratio precedence chain is well-structuredThe precedence
constructor impratio → model.mujoco.impratio[0] → MuJoCo defaultis clear and matches the new docstring. Using only the first WORLD entry at spec time and then pushing full per‑world values later viaupdate_solver_optionsis a reasonable split between global MJCF options and per‑world MjWarp state.Minor nits:
mujoco_attrsis fetched twice in_convert_to_mjc(lines 1016 and 1099); you can reuse the earlier value.- Consider a brief comment noting that at MJCF level only a single scalar
option.impratioexists, and per‑world variation is applied post‑conversion from the Newton attribute.Also applies to: 910-1030
2061-2089: Host-sideupdate_solver_optionslogic correctly respects constructor overridesThe host logic cleanly enforces the intended precedence:
- If the constructor overrode
impratio, you skip reading the custom attribute and leave the tiled constructor value inmjw_model.opt.impratio.- Otherwise, when
model.mujoco.impratioexists, you launch the kernel once overnum_worldsto copy the per‑world values.Two small suggestions:
- Since you now avoid launching the kernel when
newton_impratioisNone, the “kernel checks for None” comment is stale; consider rephrasing to reflect the current guard.- For robustness if
update_solver_optionsis ever called outside the_convert_to_mjcScopedDeviceblock, you might want to adddevice=self.model.deviceto thewp.launchcall, matching other launches in this class.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
newton/_src/sim/builder.py(2 hunks)newton/_src/sim/model.py(1 hunks)newton/_src/solvers/mujoco/kernels.py(1 hunks)newton/_src/solvers/mujoco/solver_mujoco.py(10 hunks)newton/tests/test_mujoco_solver.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- newton/_src/sim/model.py
- newton/tests/test_mujoco_solver.py
🧰 Additional context used
🧠 Learnings (12)
📓 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.
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 479
File: newton/solvers/mujoco/solver_mujoco.py:2147-2149
Timestamp: 2025-07-28T15:04:54.360Z
Learning: In the Newton MuJoCo solver implementation, the ls_parallel option may not be available in the pure MuJoCo mjOption structure, despite being documented in newer MuJoCo versions. The user adenzler-nvidia confirmed that mj_model does not have this attribute in their implementation, so ls_parallel should only be set on mjw_model.opt.
📚 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/kernels.pynewton/_src/solvers/mujoco/solver_mujoco.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-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-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-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, 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-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-11-10T12:33:57.428Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 1047
File: newton/_src/solvers/mujoco/solver_mujoco.py:1723-1749
Timestamp: 2025-11-10T12:33:57.428Z
Learning: In Warp, it is valid to pass None as a kernel input parameter for optional arrays. Inside the kernel, use `if array:` to check whether the array was provided before accessing it. This pattern eliminates the need for separate presence flags and dummy arrays.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-09-09T12:50:42.929Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 750
File: newton/_src/solvers/implicit_mpm/render_grains.py:142-160
Timestamp: 2025-09-09T12:50:42.929Z
Learning: The `update_render_grains` function in newton/_src/solvers/implicit_mpm/render_grains.py should return early when `state.velocity_field is None` because its purpose is specifically to advect grains in the velocity field. Without a velocity field, the function should be a no-op.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.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/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-09-09T11:03:41.928Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 750
File: newton/_src/solvers/implicit_mpm/solve_rheology.py:892-895
Timestamp: 2025-09-09T11:03:41.928Z
Learning: In newton/_src/solvers/implicit_mpm/solve_rheology.py, passing None for compliance_mat_diagonal is intentional to avoid loading zeros from global memory when compliance_mat is None, rather than creating a zero matrix and extracting its diagonal.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
🧬 Code graph analysis (1)
newton/_src/sim/builder.py (1)
newton/_src/sim/model.py (1)
ModelAttributeFrequency(49-72)
⏰ 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 (4)
newton/_src/sim/builder.py (2)
1805-1860: WORLD-frequency offset inadd_buildercorrectly maps sub-builder world indicesUsing
offset = self.current_worldforModelAttributeFrequency.WORLDensures that a sub-builder’s local world index 0 is remapped to the actual destination world index when composing viaadd_world/replicate, while defaulting to 0 whencurrent_world == -1keeps the single-world/global case consistent. The index-shift logic aligns with howbody_world/shape_worldare overridden inadd_builder, so WORLD-scoped custom attributes will line up with the finalnum_worldslayout.
5810-5835: WORLD-frequency attributes are now sized per world infinalizeTreating
ModelAttributeFrequency.WORLDwithcount = m.num_worldsinfinalizematches the enum’s contract (“follows the number of worlds”) and the builder merge logic, so WORLD-scoped custom attributes (e.g., per-world solver options) will materialize as one entry per world in the finalizedModel. This wiring looks consistent and complete.newton/_src/solvers/mujoco/solver_mujoco.py (2)
45-68: New kernel import is consistent with usageImporting
update_solver_options_kernelhere matches its sole use inupdate_solver_optionsand keeps all MuJoCo‑specific kernels centralized in this module.
2015-2059: Expandingopt.impratioacross worlds matches existing tiling pattern; ensure shape assumptions holdAdding
"impratio"toopt_fields_to_expandand reusingtile()mirrors how other per‑world fields are expanded from a single template world. As long asmj_model.opt.impratiocomes out ofmujoco_warp.put_modelwith a leading size‑1 world dimension (likeopt.gravity/opt.timestep), this will correctly broadcast the scalar option to all worlds.To be safe, please verify on a multi‑world model that:
self.mjw_model.opt.impratio.shape[0] == 1beforeexpand_model_fields, and- After calling
expand_model_fields, its first dimension equalsnworldand each world receives the expected initial value.
Signed-off-by: Alain Denzler <[email protected]>
|
@eric-heiden what's your stance on all the assumptions/decisions I outlined in the description? I would like to move this PR along so I think we should agree on this first. |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
newton/tests/test_mujoco_solver.py (1)
3922-4042: Strengthen assertions for constructor‑overrideimpratioand align comment with per‑world semanticsThe new tests nicely cover WORLD‑frequency
impratioand constructor precedence. Intest_impratio_constructor_override, you currently only checkmjw_impratio.flat[0]and note thatopt.impratiois scalar, whiletest_impratio_multiworld_conversiontreatsmjw_model.opt.impratioas a per‑world array.Given the WORLD‑frequency design and the kernel tiling logic, this test would be more robust if it asserted that all worlds in
mjw_model.opt.impratiosee the overridden value (and that its length matchesnum_worlds), and if the comment clarified that the conceptual MuJoCo option is scalar even though the Warp backing array is per‑world.For example:
- # Verify that mjw_model (mujoco_warp) also has the correct value - # Note: opt.impratio is a scalar option, not expanded per-world like model arrays - mjw_impratio = solver.mjw_model.opt.impratio.numpy() - self.assertAlmostEqual( - float(mjw_impratio.flat[0]), - 3.0, - places=4, - msg="MuJoCo Warp impratio should be 3.0", - ) + # Verify that mjw_model (mujoco_warp) also has the overridden value in all worlds. + # Conceptually MuJoCo's option is scalar, but the Warp backing array is per-world. + mjw_impratio = solver.mjw_model.opt.impratio.numpy() + self.assertEqual( + mjw_impratio.shape[0], + num_worlds, + "MuJoCo Warp opt.impratio should have one entry per world", + ) + for world_idx in range(num_worlds): + self.assertAlmostEqual( + mjw_impratio[world_idx], + 3.0, + places=4, + msg=f"MuJoCo Warp impratio[{world_idx}] should be 3.0 when overridden by constructor", + )This keeps the test aligned with the WORLD‑frequency behavior you’re already exercising in
test_impratio_multiworld_conversionand guards against regressions in multi‑world tiling of constructor overrides.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
newton/_src/solvers/mujoco/kernels.py(1 hunks)newton/_src/solvers/mujoco/solver_mujoco.py(10 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(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- newton/_src/utils/import_mjcf.py
- newton/_src/solvers/mujoco/kernels.py
🧰 Additional context used
🧠 Learnings (10)
📓 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.
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 479
File: newton/solvers/mujoco/solver_mujoco.py:2147-2149
Timestamp: 2025-07-28T15:04:54.360Z
Learning: The ls_parallel option is specific to MuJoCo Warp and is not available in the pure MuJoCo mjOption structure. It should only be set on mjw_model.opt, not mj_model.opt.
📚 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/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-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-11-10T12:33:57.428Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 1047
File: newton/_src/solvers/mujoco/solver_mujoco.py:1723-1749
Timestamp: 2025-11-10T12:33:57.428Z
Learning: In Warp, it is valid to pass None as a kernel input parameter for optional arrays. Inside the kernel, use `if array:` to check whether the array was provided before accessing it. This pattern eliminates the need for separate presence flags and dummy arrays.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-09-09T12:50:42.929Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 750
File: newton/_src/solvers/implicit_mpm/render_grains.py:142-160
Timestamp: 2025-09-09T12:50:42.929Z
Learning: The `update_render_grains` function in newton/_src/solvers/implicit_mpm/render_grains.py should return early when `state.velocity_field is None` because its purpose is specifically to advect grains in the velocity field. Without a velocity field, the function should be a no-op.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.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/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-09-09T11:03:41.928Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 750
File: newton/_src/solvers/implicit_mpm/solve_rheology.py:892-895
Timestamp: 2025-09-09T11:03:41.928Z
Learning: In newton/_src/solvers/implicit_mpm/solve_rheology.py, passing None for compliance_mat_diagonal is intentional to avoid loading zeros from global memory when compliance_mat is None, rather than creating a zero matrix and extracting its diagonal.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.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
📚 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
🧬 Code graph analysis (4)
newton/tests/test_import_mjcf.py (2)
newton/tests/test_import_usd.py (1)
test_option_impratio_parsing(2428-2492)newton/_src/solvers/mujoco/solver_mujoco.py (2)
SolverMuJoCo(81-2564)register_custom_attributes(155-324)
newton/tests/test_mujoco_solver.py (1)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
register_custom_attributes(155-324)
newton/_src/solvers/mujoco/solver_mujoco.py (3)
newton/_src/solvers/mujoco/kernels.py (1)
update_solver_options_kernel(914-929)newton/_src/sim/builder.py (3)
add_custom_attribute(640-685)ModelBuilder(70-5940)CustomAttribute(312-402)newton/_src/sim/model.py (2)
ModelAttributeFrequency(49-72)ModelAttributeAssignment(31-46)
newton/tests/test_import_usd.py (2)
newton/tests/test_import_mjcf.py (1)
test_option_impratio_parsing(1388-1414)newton/_src/solvers/mujoco/solver_mujoco.py (2)
SolverMuJoCo(81-2564)register_custom_attributes(155-324)
🪛 Ruff (0.14.8)
newton/tests/test_import_usd.py
2430-2430: 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 (6)
newton/tests/test_import_usd.py (1)
2427-2492: USD impratio parsing test is consistent with WORLD-frequency semanticsThe test constructs a minimal USD stage with
MjcSceneAPIandmjc:option:impratio = 1.5, registers MuJoCo custom attributes, and verifiesmodel.mujoco.impratiohas a single world value equal to 1.5. This mirrors the MJCF-side coverage and matches the intended WORLD-scopedimpratiobehavior.newton/_src/solvers/mujoco/solver_mujoco.py (4)
45-68: Kernel import wiring for solver options is correctAdding
update_solver_options_kernelto the local kernel imports cleanly hooks up the new per-world solver-option update path; no issues here.
313-324: WORLD-scopedimpratiocustom attribute is defined appropriatelyDefining
impratioas:
frequency=ModelAttributeFrequency.WORLDassignment=ModelAttributeAssignment.MODELdtype=wp.float32,default=1.0namespace="mujoco",usd_attribute_name="mjc:option:impratio"matches the intended “per-world solver option with MuJoCo-schema binding” design and lines up with the new USD test expectations. No changes needed.
340-352: Constructorimpratioargument and docstring align with intended fallback behaviorThe new keyword-only
impratio: float | None = Noneparameter, with docstring explaining “constructor arg → model custom attributemujoco:impratio→ MuJoCo default (1.0)”, matches how_convert_to_mjcresolves and applies the option below. The wiring through to_convert_to_mjc(…, impratio=impratio, …)looks consistent.Also applies to: 366-379
935-936:impratioprecedence and application to MuJoCo options are correctThe resolution logic implements the desired precedence: constructor argument > model custom attribute > MuJoCo default (1.0). The code:
- Records
impratio_overridden = impratio is not Noneto track constructor override status (line 1043)- Falls back to the first WORLD entry from
model.mujoco.impratiowhen the constructor argument isNone(lines 1044–1047)- Sets
spec.option.impratioonly when a concrete value exists, leaving MuJoCo's default untouched otherwise (lines 1057–1058)For multi-world MuJoCo Warp runs, world-0's value initializes the compiled
MjModel, while per-world values are applied viaupdate_solver_options, which is consistent with the single-world CPU backend and multi-world Warp backend behavior.newton/tests/test_import_mjcf.py (1)
1388-1464: MJCFoption.impratioparsing and per‑world remapping tests look solidThe two new tests cleanly cover both single‑world parsing and per‑world remapping when merging builders, and they follow the existing custom‑attribute test patterns in this file. Assertions on presence, length, and per‑world values of
model.mujoco.impratioare appropriate and should catch regressions in WORLD‑frequency handling and builder world‑merging logic.
| @unittest.skipUnless(USD_AVAILABLE, "Requires usd-core") | ||
| def test_option_impratio_parsing(self): | ||
| """Test parsing of impratio from USD PhysicsScene with mjc:option:impratio attribute.""" | ||
| from pxr import Usd # noqa: PLC0415 |
There was a problem hiding this comment.
Remove unused # noqa: PLC0415 per Ruff hint
Ruff reports the # noqa: PLC0415 on this import as unused. Since this rule isn’t enabled, you can drop the directive:
- from pxr import Usd # noqa: PLC0415
+ from pxr import UsdThis should clear the warning without changing behavior.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from pxr import Usd # noqa: PLC0415 | |
| from pxr import Usd |
🧰 Tools
🪛 Ruff (0.14.8)
2430-2430: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
🤖 Prompt for AI Agents
In newton/tests/test_import_usd.py around line 2430, the import line includes an
unnecessary inline noqa comment ("# noqa: PLC0415") flagged by Ruff as unused;
remove the trailing "# noqa: PLC0415" from the from pxr import Usd import so the
directive is dropped while leaving the import statement unchanged.
Signed-off-by: Alain Denzler <[email protected]>
Signed-off-by: Alain Denzler <[email protected]>
| self.expand_model_fields(self.mjw_model, nworld) | ||
|
|
||
| # update solver options from Newton model (only if not overridden by constructor) | ||
| self.update_solver_options(impratio_overridden=impratio_overridden) |
There was a problem hiding this comment.
The whole override tracking needs a better mechanism, but I'll defer this to when we parse more than 1 option.
There was a problem hiding this comment.
Yeah, we might be able to come up with some kind of Options class approach if it becomes worth it.
Signed-off-by: Alain Denzler <[email protected]>
|
PR is now updated with guards for option parsing. It's not great yet but I think a starting point. Especially for USD we will only really be able to fine-tune it once we have the schemas ready. |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
newton/_src/sim/builder.py (2)
1214-1237:add_usdsignature change is API‑breaking for positional callersYou inserted
parse_mujoco_optionsbeforemesh_maxhullvert, which shifts all subsequent positional arguments. Any existing code callingadd_usd(...)positionally up tomesh_maxhullvert(orschema_resolvers) will now silently pass values intoparse_mujoco_optionsand misalign the rest of the arguments.To preserve backwards compatibility:
- Either append the new parameter at the end of the signature, or
- Keep it here but only feed it into
parse_usdvia a keyword, and strongly document that downstream callers must treat it as keyword‑only.A BC‑friendly adjustment (requires aligning
parse_usdaccordingly) could look like:- hide_collision_shapes: bool = False, - parse_mujoco_options: bool = True, - mesh_maxhullvert: int = MESH_MAXHULLVERT, - schema_resolvers: list[SchemaResolver] | None = None, + hide_collision_shapes: bool = False, + mesh_maxhullvert: int = MESH_MAXHULLVERT, + schema_resolvers: list[SchemaResolver] | None = None, + parse_mujoco_options: bool = True, @@ - load_visual_shapes, - hide_collision_shapes, - parse_mujoco_options, - mesh_maxhullvert, - schema_resolvers, + load_visual_shapes, + hide_collision_shapes, + mesh_maxhullvert, + schema_resolvers, + parse_mujoco_options=parse_mujoco_options,Also applies to: 1264-1265, 1316-1339
1341-1371:add_mjcfinsertingparse_optionsbeforeup_axisalso breaks positional callsSimilarly,
parse_optionswas added beforeup_axis, changing the positional meaning ofup_axisand all following parameters. Any user code that passedup_axis(or later args) positionally will now feed that value intoparse_optionsand shift the rest, which is a silent and hard‑to‑debug break.Safer pattern is to append the new toggle at the end and/or pass it to
parse_mjcfas a keyword:- parse_sites: bool = True, - parse_visuals: bool = True, - parse_options: bool = True, - up_axis: AxisType = Axis.Z, + parse_sites: bool = True, + parse_visuals: bool = True, + up_axis: AxisType = Axis.Z, @@ - mesh_maxhullvert: int = MESH_MAXHULLVERT, + mesh_maxhullvert: int = MESH_MAXHULLVERT, + parse_options: bool = True, @@ - parse_sites, - parse_visuals, - parse_options, - up_axis, + parse_sites, + parse_visuals, + up_axis, @@ - mesh_maxhullvert, + mesh_maxhullvert, + parse_options=parse_options,(Assumes
parse_mjcfwas updated to accept aparse_optionskeyword.)Also applies to: 1387-1388, 1407-1437
🧹 Nitpick comments (2)
newton/_src/utils/import_usd.py (1)
961-980: Consider case-insensitive namespace check.The attribute extraction and filtering logic is well-structured. The namespace filtering at lines 970-971 correctly distinguishes MuJoCo-specific attributes from others in USD scenes.
However, consider whether the namespace check
attr.namespace != "mujoco"should be case-insensitive to handle potential variations in attribute authoring (e.g., "MuJoCo", "MUJOCO"). If the schema enforces lowercase, this is fine; otherwise, consider using.lower().If case-insensitive matching is needed:
# Filter out MuJoCo attributes if parse_mujoco_options is False if not parse_mujoco_options: - builder_custom_attr_model = [attr for attr in builder_custom_attr_model if attr.namespace != "mujoco"] + builder_custom_attr_model = [attr for attr in builder_custom_attr_model if attr.namespace and attr.namespace.lower() != "mujoco"]newton/_src/sim/builder.py (1)
1824-1882: WORLD‑frequency custom attribute remapping: semantics are reasonable but assumptions should be made explicitThe new branch
elif attr.frequency == ModelAttributeFrequency.WORLD: offset = self.current_world if self.current_world >= 0 else 0nicely handles the
add_world(builder)case where a sub‑builder encodes per‑world attributes at local index 0 and you want them remapped to the actual world index on the parent. Repeated replications correctly accumulate{world_idx: value}entries.However, this assumes:
- Sub‑builders used with
add_worldare effectively single‑world blueprints (WORLD indices local to that world, usually just{0: ...}).add_builderin global context (current_world == -1) is not used with multi‑world sub‑builders carrying WORLD attributes, otherwise higher WORLD indices may be silently ignored atfinalizetime (sincecount = m.num_worldslimits visible indices).I’d suggest either:
- Adding a small assertion/guard (e.g.,
assert builder.num_worlds <= 1when merging WORLD attributes underadd_world), or- Documenting this assumption in the
add_builderdocstring so future users don’t accidentally misuse WORLD attributes with multi‑world sub‑builders.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
newton/_src/sim/builder.py(8 hunks)newton/_src/sim/model.py(1 hunks)newton/_src/solvers/mujoco/kernels.py(1 hunks)newton/_src/solvers/mujoco/solver_mujoco.py(10 hunks)newton/_src/utils/import_mjcf.py(3 hunks)newton/_src/utils/import_usd.py(3 hunks)newton/tests/test_import_mjcf.py(2 hunks)newton/tests/test_import_usd.py(1 hunks)newton/tests/test_mujoco_solver.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- newton/_src/sim/model.py
- newton/_src/solvers/mujoco/kernels.py
🧰 Additional context used
🧠 Learnings (25)
📓 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.
📚 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.pynewton/_src/utils/import_usd.pynewton/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, 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.pynewton/_src/utils/import_usd.pynewton/tests/test_mujoco_solver.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-11-10T12:33:57.428Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 1047
File: newton/_src/solvers/mujoco/solver_mujoco.py:1723-1749
Timestamp: 2025-11-10T12:33:57.428Z
Learning: In Warp, it is valid to pass None as a kernel input parameter for optional arrays. Inside the kernel, use `if array:` to check whether the array was provided before accessing it. This pattern eliminates the need for separate presence flags and dummy arrays.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-09-09T12:50:42.929Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 750
File: newton/_src/solvers/implicit_mpm/render_grains.py:142-160
Timestamp: 2025-09-09T12:50:42.929Z
Learning: The `update_render_grains` function in newton/_src/solvers/implicit_mpm/render_grains.py should return early when `state.velocity_field is None` because its purpose is specifically to advect grains in the velocity field. Without a velocity field, the function should be a no-op.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.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/_src/solvers/mujoco/solver_mujoco.py
📚 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/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-09-09T11:03:41.928Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 750
File: newton/_src/solvers/implicit_mpm/solve_rheology.py:892-895
Timestamp: 2025-09-09T11:03:41.928Z
Learning: In newton/_src/solvers/implicit_mpm/solve_rheology.py, passing None for compliance_mat_diagonal is intentional to avoid loading zeros from global memory when compliance_mat is None, rather than creating a zero matrix and extracting its diagonal.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-12-12T17:41:15.097Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 1221
File: newton/_src/sim/builder.py:5556-5587
Timestamp: 2025-12-12T17:41:15.097Z
Learning: newton._src.geometry.sdf_utils.compute_sdf accepts max_resolution=None (e.g., when sdf_target_voxel_size is provided), so passing None from ModelBuilder.finalize is valid and should not be flagged.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-08-12T18:04:06.577Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/_src/solvers/featherstone/kernels.py:75-75
Timestamp: 2025-08-12T18:04:06.577Z
Learning: The Newton physics framework requires nightly Warp builds, which means compatibility concerns with older stable Warp versions (like missing functions such as wp.spatial_adjoint) are not relevant for this project.
Applied to files:
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/solver_mujoco.py
📚 Learning: 2025-07-28T15:04:54.360Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 479
File: newton/solvers/mujoco/solver_mujoco.py:2147-2149
Timestamp: 2025-07-28T15:04:54.360Z
Learning: In the Newton MuJoCo solver implementation, the ls_parallel option may not be available in the pure MuJoCo mjOption structure, despite being documented in newer MuJoCo versions. The user adenzler-nvidia confirmed that mj_model does not have this attribute in their implementation, so ls_parallel should only be set on mjw_model.opt.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-09-26T05:58:21.013Z
Learnt from: WenchaoHuang
Repo: newton-physics/newton PR: 835
File: newton/_src/solvers/style3d/collision/collision.py:49-53
Timestamp: 2025-09-26T05:58:21.013Z
Learning: In newton/_src/solvers/style3d/collision/collision.py, the broad_phase buffers (broad_phase_ee, broad_phase_ef, broad_phase_vf) created with wp.array() don't need zero-initialization because they follow a write-first-read-later pattern: BVH query kernels in frame_begin() populate these buffers completely (write-only operations), and collision kernels only read from them afterward in accumulate_contact_force(), ensuring no undefined reads occur.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-07-28T15:04:54.360Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 479
File: newton/solvers/mujoco/solver_mujoco.py:2147-2149
Timestamp: 2025-07-28T15:04:54.360Z
Learning: The ls_parallel option is specific to MuJoCo Warp and is not available in the pure MuJoCo mjOption structure. It should only be set on mjw_model.opt, not mj_model.opt.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-07-14T03:57:29.670Z
Learnt from: Kenny-Vilella
Repo: newton-physics/newton PR: 398
File: newton/examples/example_mujoco.py:352-352
Timestamp: 2025-07-14T03:57:29.670Z
Learning: The use_mujoco option in newton/examples/example_mujoco.py is currently unsupported and causes crashes. The code automatically disables this option with a warning message when users attempt to enable it. This is intentionally kept as a placeholder for future implementation.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.pynewton/tests/test_import_mjcf.py
📚 Learning: 2025-10-06T18:14:18.582Z
Learnt from: daedalus5
Repo: newton-physics/newton PR: 868
File: newton/examples/diffsim/example_diffsim_bear.py:82-96
Timestamp: 2025-10-06T18:14:18.582Z
Learning: Warp's tile operations (`wp.tile_store()` and `wp.tile_load()`) have built-in boundary checking. Out-of-bounds writes are automatically skipped and out-of-bounds reads are zero-filled, so tiled kernels are safe even when the array size is not a multiple of the tile size.
Applied to files:
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.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-08-12T05:17:34.423Z
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.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-09-25T16:14:22.033Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/_src/sim/ik/ik_objectives.py:0-0
Timestamp: 2025-09-25T16:14:22.033Z
Learning: wp.transform in NVIDIA Warp supports indexing using numerical indices (e.g., body_tf[0], body_tf[3], etc.) to access translation and rotation components. The indexing approach used in the Newton codebase is valid and compiles correctly.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-12-01T17:00:28.889Z
Learnt from: camevor
Repo: newton-physics/newton PR: 1161
File: newton/_src/solvers/mujoco/kernels.py:1268-1268
Timestamp: 2025-12-01T17:00:28.889Z
Learning: MuJoCo Warp spatial vectors (e.g., cacc, cvel) use the convention (angular, linear), which is opposite to Newton's (linear, angular) convention. When converting from MuJoCo Warp to Newton format, spatial_top() on MuJoCo Warp data extracts the angular component, and spatial_bottom() extracts the linear component.
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/solver_mujoco.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/_src/utils/import_usd.pynewton/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/_src/utils/import_usd.pynewton/tests/test_import_usd.py
📚 Learning: 2025-11-07T01:42:36.906Z
Learnt from: daniela-hase
Repo: newton-physics/newton PR: 1044
File: newton/_src/sensors/tiled_camera_sensor.py:224-256
Timestamp: 2025-11-07T01:42:36.906Z
Learning: In newton/_src/sensors/tiled_camera_sensor.py, the `# noqa: PLC0415` directives on the local PIL imports (lines 224 and 256 in save_color_image and save_depth_image methods) should be kept because they are required by the pre-commit configuration. The local imports are intentional for optional dependency handling.
Applied to files:
newton/tests/test_import_usd.py
🧬 Code graph analysis (3)
newton/_src/solvers/mujoco/solver_mujoco.py (3)
newton/_src/solvers/mujoco/kernels.py (1)
update_solver_options_kernel(914-929)newton/_src/sim/builder.py (2)
ModelBuilder(70-6085)CustomAttribute(327-417)newton/_src/sim/model.py (2)
ModelAttributeFrequency(49-74)ModelAttributeAssignment(31-46)
newton/_src/utils/import_usd.py (2)
newton/_src/sim/model.py (1)
ModelAttributeFrequency(49-74)newton/_src/usd/utils.py (1)
get_custom_attribute_values(451-475)
newton/_src/sim/builder.py (2)
newton/_src/viewer/wind.py (2)
frequency(172-174)frequency(177-179)newton/_src/sim/model.py (1)
ModelAttributeFrequency(49-74)
🪛 GitHub Actions: Pull Request
newton/_src/solvers/mujoco/solver_mujoco.py
[error] 2156-2156: AttributeError: 'Option' object has no attribute 'impratio' encountered during SolverMuJoCo._update_solver_options; code attempted to access self.mjw_model.opt.impratio, causing MuJoCo solver initialization failures across multiple tests.
🪛 Ruff (0.14.8)
newton/tests/test_import_usd.py
2430-2430: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
2497-2497: 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). (1)
- GitHub Check: Run GPU Benchmarks / Run GPU Benchmarks on AWS EC2
🔇 Additional comments (15)
newton/_src/utils/import_mjcf.py (2)
49-49: LGTM! Parameter addition is well-documented.The
parse_optionsparameter and its documentation are clear and consistent with the similarparse_mujoco_optionsparameter inimport_usd.py. Defaulting toTruemaintains backward compatibility.Also applies to: 83-83
154-166: The option parsing implementation correctly handles per-world custom attributes with proper remapping. However, clarify the scope of explicit validation.The code correctly implements the three-tier precedence (constructor arg > parsed custom attribute > MuJoCo default) in
SolverMuJoCo.__init__(lines 1056–1062), not in the parsing code itself. Per-world handling works as designed—WORLD frequency attributes stored at index 0 are remapped per-world duringadd_world(), which is tested and verified intest_option_impratio_per_world.The only potential concern is that
parse_warp_value_from_stringprovides type validation but no domain bounds checking (e.g., valid ranges forimpratio). Confirm whether domain validation is expected at parse time or should be delegated to the MuJoCo engine.newton/_src/utils/import_usd.py (1)
58-58: LGTM! Parameter addition is appropriate for USD context.The
parse_mujoco_optionsparameter and documentation are clear. The naming explicitly mentions "MuJoCo" which is appropriate since USD can contain attributes from multiple physics engines, unlike MJCF which is inherently MuJoCo-specific.Also applies to: 88-88
newton/_src/sim/builder.py (1)
5953-5980: WORLD frequency handling infinalizelooks consistent withModelAttributeFrequency.WORLDYou now map WORLD‑frequency attributes to
count = m.num_worlds, and then rely onCustomAttribute.build_arrayto fill[0..count-1]usingvalues.get(i, default). That matches the documented contract for WORLD (“follows the number of worlds”) and should correctly:
- Produce one entry per world index.
- Fall back to defaults for worlds without explicit overrides.
This also complements the WORLD remapping in
add_builder, so per‑world attributes survive replication/finalization.No changes needed here.
newton/tests/test_import_usd.py (1)
2427-2544: USD impratio option tests align with WORLD‑scoped and gating semanticsBoth tests correctly exercise:
- WORLD‑frequency storage on
model.mujoco.impratio(single vs default value),- the new
mjc:option:impratioPhysicsScene path when parsing is enabled, and- that
parse_mujoco_options=Falseleaves the solver option at its default despite authored USD values.This gives good coverage for the new USD option path and the intended precedence/gating behavior.
newton/tests/test_mujoco_solver.py (1)
4034-4155: New TestMuJoCoOptions class cleanly validates per‑world impratio and constructor precedenceThe two tests accurately cover:
- registration and per‑world storage of
impratioon the Newton model,- expansion into
mjw_model.opt.impratiofor multi‑world setups, and- that the
SolverMuJoCo(impratio=...)constructor argument overrides any model‑authored values (both inmj_modelandmjw_model).The implementation matches patterns used elsewhere in this file and directly exercises the intended precedence contract.
newton/tests/test_import_mjcf.py (3)
1414-1440: LGTM! Test validates single-world impratio parsing.The test correctly verifies that
impratiofrom the MJCF<option>tag is parsed and exposed asmodel.mujoco.impratiowith the expected value (1.5) for a single world.
1442-1489: LGTM! Test validates per-world impratio preservation across world merging.The test correctly verifies that when merging separate
ModelBuilderinstances with differentimpratiovalues viaadd_world, the final model preserves distinct per-world values (1.5 and 2.0) as expected.
1745-1764: LGTM! Test validates parse_options control flag.The test correctly verifies that when
parse_options=False, the MJCF<option impratio="99.0"/>is ignored and the model retains the default value (1.0).newton/_src/solvers/mujoco/solver_mujoco.py (6)
326-337: LGTM! Custom attribute registration follows established patterns.The
impratioattribute is correctly registered withWORLDfrequency for per-world values, proper default (1.0), and consistent naming conventions.
353-353: LGTM! Constructor signature change is well-documented.The
impratioparameter is correctly typed asfloat | Noneand the docstring clearly documents the fallback hierarchy: constructor argument → model custom attribute → MuJoCo default (1.0).Also applies to: 379-380
1056-1073: LGTM! Impratio resolution logic correctly implements fallback hierarchy.The code properly resolves
impratiowith constructor argument taking precedence over the model custom attribute, tracks override status viaimpratio_overridden, and only assigns tospec.option.impratiowhen a concrete value exists.
2083-2127: LGTM! Solver option field expansion follows established tiling pattern.The new
opt_fields_to_expandset and corresponding loop at lines 2124-2127 correctly handle per-world expansion of solver options nested inmj_model.opt, consistent with the existingmodel_fields_to_expandpattern.
1998-1999: LGTM! Integration point for solver option updates is correctly placed.The call to
_update_solver_optionsoccurs after model field expansion and correctly passesimpratio_overriddento control whether per-world values from the Newton model should be propagated.
2129-2157: The code correctly handlesopt.impratioexpansion.expand_model_fields()is called before_update_solver_options()and explicitly tilesimpratiofrom a scalar to per-world arrays via theopt_fields_to_expandloop. Theput_model()call creates the initial scalar attribute, andexpand_model_fields()converts it to a per-world array when needed. Tests confirmmjw_model.opt.impratiois accessible as an array and works correctly. The suggested manual array creation is unnecessary.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
2083-2157: Add hasattr guard in_update_solver_optionsto handle missingopt.impratiofield (fixes CI AttributeError).The pipeline error (
AttributeError: 'Option' object has no attribute 'impratio') occurs because_update_solver_optionsunconditionally accessesself.mjw_model.opt.impratiowithout checking field availability. In contrast, theexpand_model_fieldsmethod already guards alloptfield access withhasattr. Option fields are version-dependent and may not exist in all MuJoCo Warp builds.Add a feature-detection guard before the kernel launch:
def _update_solver_options(self, impratio_overridden: bool = False): """Update solver options from Newton model to MuJoCo Warp. Copies per-world values from Newton custom attributes to the MuJoCo Warp model. If a value was overridden by constructor, tile() already handled expansion so we skip it. Args: impratio_overridden: If True, impratio was set by constructor and tile() handled it. """ mujoco_attrs = getattr(self.model, "mujoco", None) nworld = self.model.num_worlds + # Skip if mjw_model.opt.impratio is not available (version-dependent) + if not hasattr(self.mjw_model.opt, "impratio"): + return + # Get Newton arrays - pass None if overridden or not available (kernel checks for None) if not impratio_overridden and mujoco_attrs and hasattr(mujoco_attrs, "impratio"): newton_impratio = mujoco_attrs.impratio else: newton_impratio = NoneThis restores compatibility with MuJoCo Warp versions where
impratiois unavailable, unblocking CI while preserving per-world impratio support when the field exists.
♻️ Duplicate comments (1)
newton/tests/test_import_usd.py (1)
2430-2430: Remove unusednoqadirective.Ruff reports the
# noqa: PLC0415directive as unused because the rule is not enabled in your configuration.Apply this diff:
- from pxr import Usd # noqa: PLC0415 + from pxr import Usd
🧹 Nitpick comments (2)
newton/_src/sim/builder.py (1)
1215-1237: Parameter insertion is technically sound, but no existing positional usage breaksThe concern about inserting
parse_mujoco_optionsbeforemesh_maxhullvertis technically correct—code callingadd_usd()with positional arguments for tail parameters would break. However, a search of the repository shows all current uses ofadd_usd()andadd_mjcf()already pass tail parameters by keyword (e.g.,collapse_fixed_joints=True,load_sites=False,up_axis="Z"), so this change does not break existing code. Given Newton's active alpha development status where the API is unstable and breaking changes are likely to occur frequently and without notice, this is acceptable. Making these new parameters keyword-only would be a good practice for future-proofing the API, but is not required.newton/_src/solvers/mujoco/solver_mujoco.py (1)
950-973:impratioresolution and propagation path is coherent.
_convert_to_mjccorrectly:
- Tracks constructor override (
impratio_overridden),- Falls back to
model.mujoco.impratio[0]when unset,- Only writes
spec.option.impratiowhen a concrete value exists, and- Calls
_update_solver_options(impratio_overridden=...)afterexpand_model_fieldsto push per‑world values into MJWarp.This matches the stated precedence and keeps constructor overrides taking priority over model attributes.
If you want to be extra explicit for future multi‑world users, you could assert (or at least document) that
model.mujoco.impratiois expected to be uniform across worlds when used to seedspec.option.impratio(since you currently sample index 0).Also applies to: 1056-1063, 1072-1073, 1995-1999
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
newton/_src/sim/builder.py(8 hunks)newton/_src/sim/model.py(1 hunks)newton/_src/solvers/mujoco/kernels.py(1 hunks)newton/_src/solvers/mujoco/solver_mujoco.py(10 hunks)newton/_src/utils/import_mjcf.py(3 hunks)newton/_src/utils/import_usd.py(3 hunks)newton/tests/test_import_mjcf.py(2 hunks)newton/tests/test_import_usd.py(1 hunks)newton/tests/test_mujoco_solver.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- newton/_src/utils/import_mjcf.py
- newton/_src/sim/model.py
- newton/_src/solvers/mujoco/kernels.py
- newton/tests/test_mujoco_solver.py
🧰 Additional context used
🧠 Learnings (24)
📓 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: nvtw
Repo: newton-physics/newton PR: 1221
File: newton/examples/example_sdf.py:189-191
Timestamp: 2025-12-12T08:46:29.875Z
Learning: In Newton, SDF data structures are shared across replicated world instances (e.g., when using ModelBuilder.replicate()). Therefore, the memory cost of SDF generation scales with sdf_max_resolution but does not scale with the number of worlds—100 worlds with sdf_max_resolution=512 uses the same SDF memory as 1 world with sdf_max_resolution=512.
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 479
File: newton/solvers/mujoco/solver_mujoco.py:2147-2149
Timestamp: 2025-07-28T15:04:54.360Z
Learning: The ls_parallel option is specific to MuJoCo Warp and is not available in the pure MuJoCo mjOption structure. It should only be set on mjw_model.opt, not mj_model.opt.
📚 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/_src/utils/import_usd.pynewton/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/_src/utils/import_usd.pynewton/tests/test_import_usd.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/utils/import_usd.pynewton/_src/sim/builder.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/_src/utils/import_usd.pynewton/_src/sim/builder.pynewton/_src/solvers/mujoco/solver_mujoco.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-11-10T12:33:57.428Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 1047
File: newton/_src/solvers/mujoco/solver_mujoco.py:1723-1749
Timestamp: 2025-11-10T12:33:57.428Z
Learning: In Warp, it is valid to pass None as a kernel input parameter for optional arrays. Inside the kernel, use `if array:` to check whether the array was provided before accessing it. This pattern eliminates the need for separate presence flags and dummy arrays.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-09-09T12:50:42.929Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 750
File: newton/_src/solvers/implicit_mpm/render_grains.py:142-160
Timestamp: 2025-09-09T12:50:42.929Z
Learning: The `update_render_grains` function in newton/_src/solvers/implicit_mpm/render_grains.py should return early when `state.velocity_field is None` because its purpose is specifically to advect grains in the velocity field. Without a velocity field, the function should be a no-op.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.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/_src/solvers/mujoco/solver_mujoco.py
📚 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/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-09-09T11:03:41.928Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 750
File: newton/_src/solvers/implicit_mpm/solve_rheology.py:892-895
Timestamp: 2025-09-09T11:03:41.928Z
Learning: In newton/_src/solvers/implicit_mpm/solve_rheology.py, passing None for compliance_mat_diagonal is intentional to avoid loading zeros from global memory when compliance_mat is None, rather than creating a zero matrix and extracting its diagonal.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-08-12T18:04:06.577Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/_src/solvers/featherstone/kernels.py:75-75
Timestamp: 2025-08-12T18:04:06.577Z
Learning: The Newton physics framework requires nightly Warp builds, which means compatibility concerns with older stable Warp versions (like missing functions such as wp.spatial_adjoint) are not relevant for this project.
Applied to files:
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/solver_mujoco.py
📚 Learning: 2025-07-28T15:04:54.360Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 479
File: newton/solvers/mujoco/solver_mujoco.py:2147-2149
Timestamp: 2025-07-28T15:04:54.360Z
Learning: In the Newton MuJoCo solver implementation, the ls_parallel option may not be available in the pure MuJoCo mjOption structure, despite being documented in newer MuJoCo versions. The user adenzler-nvidia confirmed that mj_model does not have this attribute in their implementation, so ls_parallel should only be set on mjw_model.opt.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-09-26T05:58:21.013Z
Learnt from: WenchaoHuang
Repo: newton-physics/newton PR: 835
File: newton/_src/solvers/style3d/collision/collision.py:49-53
Timestamp: 2025-09-26T05:58:21.013Z
Learning: In newton/_src/solvers/style3d/collision/collision.py, the broad_phase buffers (broad_phase_ee, broad_phase_ef, broad_phase_vf) created with wp.array() don't need zero-initialization because they follow a write-first-read-later pattern: BVH query kernels in frame_begin() populate these buffers completely (write-only operations), and collision kernels only read from them afterward in accumulate_contact_force(), ensuring no undefined reads occur.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-07-28T15:04:54.360Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 479
File: newton/solvers/mujoco/solver_mujoco.py:2147-2149
Timestamp: 2025-07-28T15:04:54.360Z
Learning: The ls_parallel option is specific to MuJoCo Warp and is not available in the pure MuJoCo mjOption structure. It should only be set on mjw_model.opt, not mj_model.opt.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-07-14T03:57:29.670Z
Learnt from: Kenny-Vilella
Repo: newton-physics/newton PR: 398
File: newton/examples/example_mujoco.py:352-352
Timestamp: 2025-07-14T03:57:29.670Z
Learning: The use_mujoco option in newton/examples/example_mujoco.py is currently unsupported and causes crashes. The code automatically disables this option with a warning message when users attempt to enable it. This is intentionally kept as a placeholder for future implementation.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.pynewton/tests/test_import_mjcf.py
📚 Learning: 2025-10-06T18:14:18.582Z
Learnt from: daedalus5
Repo: newton-physics/newton PR: 868
File: newton/examples/diffsim/example_diffsim_bear.py:82-96
Timestamp: 2025-10-06T18:14:18.582Z
Learning: Warp's tile operations (`wp.tile_store()` and `wp.tile_load()`) have built-in boundary checking. Out-of-bounds writes are automatically skipped and out-of-bounds reads are zero-filled, so tiled kernels are safe even when the array size is not a multiple of the tile size.
Applied to files:
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.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-08-12T05:17:34.423Z
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.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-09-25T16:14:22.033Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/_src/sim/ik/ik_objectives.py:0-0
Timestamp: 2025-09-25T16:14:22.033Z
Learning: wp.transform in NVIDIA Warp supports indexing using numerical indices (e.g., body_tf[0], body_tf[3], etc.) to access translation and rotation components. The indexing approach used in the Newton codebase is valid and compiles correctly.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-12-01T17:00:28.889Z
Learnt from: camevor
Repo: newton-physics/newton PR: 1161
File: newton/_src/solvers/mujoco/kernels.py:1268-1268
Timestamp: 2025-12-01T17:00:28.889Z
Learning: MuJoCo Warp spatial vectors (e.g., cacc, cvel) use the convention (angular, linear), which is opposite to Newton's (linear, angular) convention. When converting from MuJoCo Warp to Newton format, spatial_top() on MuJoCo Warp data extracts the angular component, and spatial_bottom() extracts the linear component.
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/solver_mujoco.py
📚 Learning: 2025-11-07T01:42:36.906Z
Learnt from: daniela-hase
Repo: newton-physics/newton PR: 1044
File: newton/_src/sensors/tiled_camera_sensor.py:224-256
Timestamp: 2025-11-07T01:42:36.906Z
Learning: In newton/_src/sensors/tiled_camera_sensor.py, the `# noqa: PLC0415` directives on the local PIL imports (lines 224 and 256 in save_color_image and save_depth_image methods) should be kept because they are required by the pre-commit configuration. The local imports are intentional for optional dependency handling.
Applied to files:
newton/tests/test_import_usd.py
🧬 Code graph analysis (3)
newton/_src/sim/builder.py (1)
newton/_src/sim/model.py (1)
ModelAttributeFrequency(49-74)
newton/_src/solvers/mujoco/solver_mujoco.py (2)
newton/_src/solvers/mujoco/kernels.py (1)
update_solver_options_kernel(914-929)newton/_src/sim/model.py (2)
ModelAttributeFrequency(49-74)ModelAttributeAssignment(31-46)
newton/tests/test_import_usd.py (2)
newton/tests/test_import_mjcf.py (1)
test_option_impratio_parsing(1414-1440)newton/_src/solvers/mujoco/solver_mujoco.py (2)
SolverMuJoCo(82-2628)register_custom_attributes(156-337)
🪛 GitHub Actions: Pull Request
newton/_src/solvers/mujoco/solver_mujoco.py
[error] 2156-2156: AttributeError: 'Option' object has no attribute 'impratio' when updating MuJoCo solver options (self.mjw_model.opt.impratio) in _update_solver_options.
🪛 GitHub Actions: Pull Request - AWS GPU
newton/_src/solvers/mujoco/solver_mujoco.py
[error] 2156-2156: AttributeError: 'Option' object has no attribute 'impratio' while updating MuJoCo solver options in SolverMuJoCo. This causes widespread test failures across MuJoCo-related tests.
🪛 Ruff (0.14.8)
newton/tests/test_import_usd.py
2430-2430: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
2497-2497: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
🔇 Additional comments (10)
newton/_src/sim/builder.py (2)
1825-1847: WORLD‑frequency custom attribute handling in merge/finalize looks consistentThe new WORLD branch in
add_builder(offset = current_worldwith a global fallback of 0) matches the documented semantics ofadd_builder/add_world, and will correctly remap per‑world indices when replicating builders or composing worlds. Likewise, sizing WORLD attributes infinalizeviacount = m.num_worldsis consistent withModelAttributeFrequency.WORLDand ensures the resulting arrays cover all worlds.No issues spotted here; the WORLD custom‑attribute pipeline from merge → finalize appears sound.
Also applies to: 5953-5961
1341-1354: parse_options parameter placement does not break backward compatibilityThe concern about positional argument misrouting is unfounded. All existing usages of
add_mjcfthroughout tests and examples employ keyword arguments for optional parameters (e.g.,add_mjcf(source, up_axis="Z"),add_mjcf(source, floating=False)). No code relies on positional ordering forup_axisor later parameters. The parameter is correctly placed and forwarded toparse_mjcf; no action required.Likely an incorrect or invalid review comment.
newton/tests/test_import_usd.py (1)
2427-2492: LGTM! The test correctly verifies USD option parsing.The test properly:
- Registers MuJoCo custom attributes before USD import
- Parses
mjc:option:impratiofrom PhysicsScene- Validates the attribute namespace and value
newton/_src/utils/import_usd.py (2)
58-58: LGTM! Theparse_mujoco_optionsparameter addition is well-designed.The parameter:
- Defaults to
Truefor backward compatibility- Is clearly documented
- Follows the same pattern as the MJCF parser's
parse_optionsAlso applies to: 88-88
961-980: LGTM! The attribute extraction logic correctly implements the parse_mujoco_options feature.The implementation:
- Extends extraction to include WORLD frequency attributes (line 966) for per-world solver options
- Filters out MuJoCo attributes when
parse_mujoco_options=False(lines 970-971)- Properly assigns extracted values to
builder.custom_attributes[key].values[0](lines 976-979)- Follows the same pattern as MJCF parsing
The comment on line 962 correctly notes that WORLD attributes use index 0 temporarily and get remapped during world replication.
newton/_src/solvers/mujoco/solver_mujoco.py (2)
326-337: WORLD‑frequencyimpratiocustom attribute looks consistent.The new
mujoco:impratioattribute (WORLD frequency, default 1.0, USD namemjc:option:impratio) matches the intended per‑world solver option semantics and follows existing custom‑attribute patterns.
353-380: Constructor and docstring semantics forimpratioalign with resolve logic.Treating
impratioasfloat | Nonewith precedence “constructor > model custom attribute > MuJoCo default” matches the resolver block in_convert_to_mjc, and the docstring correctly reflects that behavior.newton/tests/test_import_mjcf.py (3)
1414-1441: MJCF<option>impratio parsing test is well‑scoped.This test cleanly asserts that a single‑world
<option impratio="...">is exposed asmodel.mujoco.impratio[0], matching the new WORLD‑frequency attribute contract.
1442-1489: Per‑world impratio merge test covers important multi‑world behavior.Using two builders with different
<option impratio>values and merging viaadd_worldis a good way to lock in the WORLD‑frequency semantics formujoco.impratio.
1745-1764:parse_options=Falsebehavior is clearly specified and tested.Ensuring that
mujoco.impratiostays at its default (1.0) whenparse_options=Falseprevents surprising side effects from MJCF<option>tags and defines the flag’s intent well.
| @unittest.skipUnless(USD_AVAILABLE, "Requires usd-core") | ||
| def test_parse_mujoco_options_disabled(self): | ||
| """Test that MuJoCo options from PhysicsScene are not parsed when parse_mujoco_options=False.""" | ||
| from pxr import Usd # noqa: PLC0415 |
There was a problem hiding this comment.
Remove unused noqa directive.
Ruff reports the # noqa: PLC0415 directive as unused because the rule is not enabled in your configuration.
Apply this diff:
- from pxr import Usd # noqa: PLC0415
+ from pxr import Usd📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| from pxr import Usd # noqa: PLC0415 | |
| from pxr import Usd |
🧰 Tools
🪛 Ruff (0.14.8)
2497-2497: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
🤖 Prompt for AI Agents
In newton/tests/test_import_usd.py around line 2497, the import statement "from
pxr import Usd # noqa: PLC0415" contains an unused noqa directive; remove the
trailing "# noqa: PLC0415" from that line so the import stands without the
unnecessary directive, then run the linter to confirm the warning is resolved.
Signed-off-by: Alain Denzler <[email protected]>
Signed-off-by: Alain Denzler <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
newton/tests/test_import_usd.py (1)
2430-2430: Drop unused# noqa: PLC0415directives on pxr importsRuff reports these
# noqa: PLC0415markers as unused becausePLC0415is not enabled in the config. You can safely remove the directives and keep the imports as-is:- from pxr import Usd # noqa: PLC0415 + from pxr import UsdIf your lint configuration changes in the future to re‑enable
PLC0415, you may want to reintroduce targetednoqawhere necessary.Also applies to: 2497-2497
🧹 Nitpick comments (2)
newton/_src/solvers/mujoco/solver_mujoco.py (2)
2005-2081: Verify whetheropt.impratio_invsqrtalso needs tiling alongsideopt.impratio
expand_model_fieldsnow tiles selectedmj_modelroot fields and, underopt_fields_to_expand, tilesmj_model.opt.impratiovia the sharedtile()helper. Later,_update_solver_optionswrites per‑world values intoself.mjw_model.opt.impratio_invsqrt.Depending on how
mujoco_warp.put_model/put_datainitializeopt.impratio_invsqrt, there are two possibilities:
- If
impratio_invsqrtis already allocated with lengthnworld, onlyimprationeeds tiling and the current code is fine.- If
impratio_invsqrtstarts as length 1 (single template world), it may also need to be expanded to lengthnworldbefore the kernel write, or the kernel could index beyond the array.I’d suggest double‑checking mujoco_warp’s
Model.optlayout and, if needed, either:- opt_fields_to_expand = { - # "timestep", - "impratio", - # ... - } + opt_fields_to_expand = { + # "timestep", + "impratio", + "impratio_invsqrt", + # ... + }or relying solely on
_update_solver_optionsto write the fully-sizedimpratio_invsqrtwhile letting mujoco_warp handle the initial expansion.
2129-2157:_update_solver_optionscorrectly gates constructor overrides and uses WORLD attributesThe helper:
- Looks up
model.mujoco.impratioonly when it wasn’t overridden by the constructor and when the attribute exists.- Short‑circuits if all options are
None, avoiding a pointless kernel launch.- Launches
update_solver_options_kernelovernworld, passingmujoco_attrs.impratio(WORLD array) and writing intoself.mjw_model.opt.impratio_invsqrt.This matches the design that constructor values take precedence, while model attributes provide per‑world defaults when constructors don’t override. Two small, optional tweaks you might consider:
- wp.launch( + wp.launch( update_solver_options_kernel, dim=nworld, inputs=[newton_impratio], outputs=[self.mjw_model.opt.impratio_invsqrt], - ) + device=self.model.device, + )
- Explicitly passing
device=self.model.devicefor consistency with other launches.- Adding a brief note in the docstring that solver options are only synced at construction time, not via
notify_model_changed, to make that policy obvious to users reading the code.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
newton/_src/solvers/mujoco/kernels.py(1 hunks)newton/_src/solvers/mujoco/solver_mujoco.py(10 hunks)newton/tests/test_import_usd.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- newton/_src/solvers/mujoco/kernels.py
🧰 Additional context used
🧠 Learnings (24)
📓 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.
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 479
File: newton/solvers/mujoco/solver_mujoco.py:2147-2149
Timestamp: 2025-07-28T15:04:54.360Z
Learning: In the Newton MuJoCo solver implementation, the ls_parallel option may not be available in the pure MuJoCo mjOption structure, despite being documented in newer MuJoCo versions. The user adenzler-nvidia confirmed that mj_model does not have this attribute in their implementation, so ls_parallel should only be set on mjw_model.opt.
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 479
File: newton/solvers/mujoco/solver_mujoco.py:2147-2149
Timestamp: 2025-07-28T15:04:54.360Z
Learning: The ls_parallel option is specific to MuJoCo Warp and is not available in the pure MuJoCo mjOption structure. It should only be set on mjw_model.opt, not mj_model.opt.
📚 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-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-11-10T12:33:57.428Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 1047
File: newton/_src/solvers/mujoco/solver_mujoco.py:1723-1749
Timestamp: 2025-11-10T12:33:57.428Z
Learning: In Warp, it is valid to pass None as a kernel input parameter for optional arrays. Inside the kernel, use `if array:` to check whether the array was provided before accessing it. This pattern eliminates the need for separate presence flags and dummy arrays.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-09-09T12:50:42.929Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 750
File: newton/_src/solvers/implicit_mpm/render_grains.py:142-160
Timestamp: 2025-09-09T12:50:42.929Z
Learning: The `update_render_grains` function in newton/_src/solvers/implicit_mpm/render_grains.py should return early when `state.velocity_field is None` because its purpose is specifically to advect grains in the velocity field. Without a velocity field, the function should be a no-op.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.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/_src/solvers/mujoco/solver_mujoco.py
📚 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/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-09-09T11:03:41.928Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 750
File: newton/_src/solvers/implicit_mpm/solve_rheology.py:892-895
Timestamp: 2025-09-09T11:03:41.928Z
Learning: In newton/_src/solvers/implicit_mpm/solve_rheology.py, passing None for compliance_mat_diagonal is intentional to avoid loading zeros from global memory when compliance_mat is None, rather than creating a zero matrix and extracting its diagonal.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-08-12T18:04:06.577Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/_src/solvers/featherstone/kernels.py:75-75
Timestamp: 2025-08-12T18:04:06.577Z
Learning: The Newton physics framework requires nightly Warp builds, which means compatibility concerns with older stable Warp versions (like missing functions such as wp.spatial_adjoint) are not relevant for this project.
Applied to files:
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/solver_mujoco.py
📚 Learning: 2025-07-28T15:04:54.360Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 479
File: newton/solvers/mujoco/solver_mujoco.py:2147-2149
Timestamp: 2025-07-28T15:04:54.360Z
Learning: In the Newton MuJoCo solver implementation, the ls_parallel option may not be available in the pure MuJoCo mjOption structure, despite being documented in newer MuJoCo versions. The user adenzler-nvidia confirmed that mj_model does not have this attribute in their implementation, so ls_parallel should only be set on mjw_model.opt.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-09-26T05:58:21.013Z
Learnt from: WenchaoHuang
Repo: newton-physics/newton PR: 835
File: newton/_src/solvers/style3d/collision/collision.py:49-53
Timestamp: 2025-09-26T05:58:21.013Z
Learning: In newton/_src/solvers/style3d/collision/collision.py, the broad_phase buffers (broad_phase_ee, broad_phase_ef, broad_phase_vf) created with wp.array() don't need zero-initialization because they follow a write-first-read-later pattern: BVH query kernels in frame_begin() populate these buffers completely (write-only operations), and collision kernels only read from them afterward in accumulate_contact_force(), ensuring no undefined reads occur.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-07-28T15:04:54.360Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 479
File: newton/solvers/mujoco/solver_mujoco.py:2147-2149
Timestamp: 2025-07-28T15:04:54.360Z
Learning: The ls_parallel option is specific to MuJoCo Warp and is not available in the pure MuJoCo mjOption structure. It should only be set on mjw_model.opt, not mj_model.opt.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-07-14T03:57:29.670Z
Learnt from: Kenny-Vilella
Repo: newton-physics/newton PR: 398
File: newton/examples/example_mujoco.py:352-352
Timestamp: 2025-07-14T03:57:29.670Z
Learning: The use_mujoco option in newton/examples/example_mujoco.py is currently unsupported and causes crashes. The code automatically disables this option with a warning message when users attempt to enable it. This is intentionally kept as a placeholder for future implementation.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-10-06T18:14:18.582Z
Learnt from: daedalus5
Repo: newton-physics/newton PR: 868
File: newton/examples/diffsim/example_diffsim_bear.py:82-96
Timestamp: 2025-10-06T18:14:18.582Z
Learning: Warp's tile operations (`wp.tile_store()` and `wp.tile_load()`) have built-in boundary checking. Out-of-bounds writes are automatically skipped and out-of-bounds reads are zero-filled, so tiled kernels are safe even when the array size is not a multiple of the tile size.
Applied to files:
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.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-08-12T05:17:34.423Z
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.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-09-25T16:14:22.033Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/_src/sim/ik/ik_objectives.py:0-0
Timestamp: 2025-09-25T16:14:22.033Z
Learning: wp.transform in NVIDIA Warp supports indexing using numerical indices (e.g., body_tf[0], body_tf[3], etc.) to access translation and rotation components. The indexing approach used in the Newton codebase is valid and compiles correctly.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-12-01T17:00:28.889Z
Learnt from: camevor
Repo: newton-physics/newton PR: 1161
File: newton/_src/solvers/mujoco/kernels.py:1268-1268
Timestamp: 2025-12-01T17:00:28.889Z
Learning: MuJoCo Warp spatial vectors (e.g., cacc, cvel) use the convention (angular, linear), which is opposite to Newton's (linear, angular) convention. When converting from MuJoCo Warp to Newton format, spatial_top() on MuJoCo Warp data extracts the angular component, and spatial_bottom() extracts the linear component.
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/solver_mujoco.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
📚 Learning: 2025-11-07T01:42:36.906Z
Learnt from: daniela-hase
Repo: newton-physics/newton PR: 1044
File: newton/_src/sensors/tiled_camera_sensor.py:224-256
Timestamp: 2025-11-07T01:42:36.906Z
Learning: In newton/_src/sensors/tiled_camera_sensor.py, the `# noqa: PLC0415` directives on the local PIL imports (lines 224 and 256 in save_color_image and save_depth_image methods) should be kept because they are required by the pre-commit configuration. The local imports are intentional for optional dependency handling.
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_option_impratio_parsing(1414-1440)newton/_src/solvers/mujoco/solver_mujoco.py (2)
SolverMuJoCo(82-2628)register_custom_attributes(156-337)
🪛 Ruff (0.14.8)
newton/tests/test_import_usd.py
2430-2430: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
2497-2497: 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
🔇 Additional comments (6)
newton/tests/test_import_usd.py (1)
2427-2493: USD impratio parsing tests look correct and aligned with schemaThe new tests correctly:
- Use
mjc:option:impratioonPhysicsScene(matching the registered USD name).- Verify the value lands in
model.mujoco.impratioas a single WORLD-scoped entry.- Cover the disabled path via
parse_mujoco_options=Falsewith the default 1.0 expectation.This matches the new WORLD-frequency custom attribute and importer behavior.
newton/_src/solvers/mujoco/solver_mujoco.py (5)
45-69: Kernel import wiring for solver options looks consistentImporting
update_solver_options_kernelalongside the other MuJoCo kernels and using it only in_update_solver_optionskeeps the solver‑option synchronization localized and avoids touching the hot step path. No issues here.
326-337: WORLD-scopedimpratiocustom attribute is well-structuredDefining
impratioas:
frequency=ModelAttributeFrequency.WORLD(per-world),assignment=ModelAttributeAssignment.MODEL,dtype=wp.float32with default1.0,usd_attribute_name="mjc:option:impratio",matches the new USD tests and gives the solver a clean place to read per-world options. The lack of an explicit
mjcf_attribute_nameis fine as long as the MJCF importer handlesoption impratiodirectly, which your MJCF tests appear to exercise.
339-381: Constructorimpratioargument and precedence semantics look correctThe new
impratio: float | None = Noneargument, its docstring, and passing it through to_convert_to_mjcalign with the intended precedence:
- If the ctor arg is provided, it overrides any model attribute.
- If
None,_convert_to_mjcfalls back to the modelmujoco.impratioattribute (if present) or MuJoCo’s default.This preserves the existing behavior when users explicitly configure
SolverMuJoCo(impratio=...)while enabling model-driven defaults when left asNone.
936-1074:impratioresolution and spec.option wiring follow the documented precedenceThe resolver block:
- Captures
impratio_overridden = impratio is not Nonebefore consulting model attributes.- When
impratio is None, readsmodel.mujoco.impratio(WORLD array) and collapses the first element to a scalar forspec.option.impratio.- Only assigns
spec.option.impratioif a concrete value is available.This matches the “constructor > model attribute > MuJoCo default” rule and keeps the CPU backend (which only sees a single world) in sync with the same scalar value the Warp model starts from. Subsequent per-world adjustments via
_update_solver_optionsthen refine the Warp-onlyoptstate without disturbing the base MuJoCo spec.
1995-2003: One-time_update_solver_optionscall is placed correctlyCalling
self._update_solver_options(impratio_overridden=impratio_overridden):
- Happens after
put_model,put_data, andexpand_model_fields, soself.mjw_modeland itsoptarrays exist at the finalnworldsize.- Runs only once during construction, avoiding the runtime “live update” semantics you explicitly wanted to avoid.
- Honors the override flag so constructor-provided
impratiovalues continue to be sourced fromspec.option.impratio/ mujoco_warp’s internal initialization.This placement looks appropriate and low‑risk.
Signed-off-by: Alain Denzler <[email protected]>
Signed-off-by: Alain Denzler <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
326-337: Consider adding MJCF attribute mapping for completeness.The custom attribute registration correctly uses WORLD frequency for per-world solver options. However,
mjcf_attribute_nameis not specified. If MJCF schemas support specifyingimpratio(e.g., as<option impratio="..."/>), consider adding it for consistency with other attributes.If MJCF supports this, apply this diff:
builder.add_custom_attribute( ModelBuilder.CustomAttribute( name="impratio", frequency=ModelAttributeFrequency.WORLD, assignment=ModelAttributeAssignment.MODEL, dtype=wp.float32, default=1.0, namespace="mujoco", usd_attribute_name="mjc:option:impratio", + mjcf_attribute_name="impratio", ) )
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
newton/_src/solvers/mujoco/solver_mujoco.py(10 hunks)newton/tests/test_mujoco_solver.py(1 hunks)
🧰 Additional context used
🧠 Learnings (21)
📓 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: adenzler-nvidia
Repo: newton-physics/newton PR: 479
File: newton/solvers/mujoco/solver_mujoco.py:2147-2149
Timestamp: 2025-07-28T15:04:54.360Z
Learning: The ls_parallel option is specific to MuJoCo Warp and is not available in the pure MuJoCo mjOption structure. It should only be set on mjw_model.opt, not mj_model.opt.
📚 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.pynewton/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, 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.pynewton/tests/test_mujoco_solver.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-12T05:17:34.423Z
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.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-11-10T12:33:57.428Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 1047
File: newton/_src/solvers/mujoco/solver_mujoco.py:1723-1749
Timestamp: 2025-11-10T12:33:57.428Z
Learning: In Warp, it is valid to pass None as a kernel input parameter for optional arrays. Inside the kernel, use `if array:` to check whether the array was provided before accessing it. This pattern eliminates the need for separate presence flags and dummy arrays.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-09-09T12:50:42.929Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 750
File: newton/_src/solvers/implicit_mpm/render_grains.py:142-160
Timestamp: 2025-09-09T12:50:42.929Z
Learning: The `update_render_grains` function in newton/_src/solvers/implicit_mpm/render_grains.py should return early when `state.velocity_field is None` because its purpose is specifically to advect grains in the velocity field. Without a velocity field, the function should be a no-op.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.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/_src/solvers/mujoco/solver_mujoco.py
📚 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/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-09-09T11:03:41.928Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 750
File: newton/_src/solvers/implicit_mpm/solve_rheology.py:892-895
Timestamp: 2025-09-09T11:03:41.928Z
Learning: In newton/_src/solvers/implicit_mpm/solve_rheology.py, passing None for compliance_mat_diagonal is intentional to avoid loading zeros from global memory when compliance_mat is None, rather than creating a zero matrix and extracting its diagonal.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-08-12T18:04:06.577Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/_src/solvers/featherstone/kernels.py:75-75
Timestamp: 2025-08-12T18:04:06.577Z
Learning: The Newton physics framework requires nightly Warp builds, which means compatibility concerns with older stable Warp versions (like missing functions such as wp.spatial_adjoint) are not relevant for this project.
Applied to files:
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/solver_mujoco.py
📚 Learning: 2025-07-28T15:04:54.360Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 479
File: newton/solvers/mujoco/solver_mujoco.py:2147-2149
Timestamp: 2025-07-28T15:04:54.360Z
Learning: In the Newton MuJoCo solver implementation, the ls_parallel option may not be available in the pure MuJoCo mjOption structure, despite being documented in newer MuJoCo versions. The user adenzler-nvidia confirmed that mj_model does not have this attribute in their implementation, so ls_parallel should only be set on mjw_model.opt.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-09-26T05:58:21.013Z
Learnt from: WenchaoHuang
Repo: newton-physics/newton PR: 835
File: newton/_src/solvers/style3d/collision/collision.py:49-53
Timestamp: 2025-09-26T05:58:21.013Z
Learning: In newton/_src/solvers/style3d/collision/collision.py, the broad_phase buffers (broad_phase_ee, broad_phase_ef, broad_phase_vf) created with wp.array() don't need zero-initialization because they follow a write-first-read-later pattern: BVH query kernels in frame_begin() populate these buffers completely (write-only operations), and collision kernels only read from them afterward in accumulate_contact_force(), ensuring no undefined reads occur.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-07-28T15:04:54.360Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 479
File: newton/solvers/mujoco/solver_mujoco.py:2147-2149
Timestamp: 2025-07-28T15:04:54.360Z
Learning: The ls_parallel option is specific to MuJoCo Warp and is not available in the pure MuJoCo mjOption structure. It should only be set on mjw_model.opt, not mj_model.opt.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-07-14T03:57:29.670Z
Learnt from: Kenny-Vilella
Repo: newton-physics/newton PR: 398
File: newton/examples/example_mujoco.py:352-352
Timestamp: 2025-07-14T03:57:29.670Z
Learning: The use_mujoco option in newton/examples/example_mujoco.py is currently unsupported and causes crashes. The code automatically disables this option with a warning message when users attempt to enable it. This is intentionally kept as a placeholder for future implementation.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-10-06T18:14:18.582Z
Learnt from: daedalus5
Repo: newton-physics/newton PR: 868
File: newton/examples/diffsim/example_diffsim_bear.py:82-96
Timestamp: 2025-10-06T18:14:18.582Z
Learning: Warp's tile operations (`wp.tile_store()` and `wp.tile_load()`) have built-in boundary checking. Out-of-bounds writes are automatically skipped and out-of-bounds reads are zero-filled, so tiled kernels are safe even when the array size is not a multiple of the tile size.
Applied to files:
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.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-09-25T16:14:22.033Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/_src/sim/ik/ik_objectives.py:0-0
Timestamp: 2025-09-25T16:14:22.033Z
Learning: wp.transform in NVIDIA Warp supports indexing using numerical indices (e.g., body_tf[0], body_tf[3], etc.) to access translation and rotation components. The indexing approach used in the Newton codebase is valid and compiles correctly.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-12-01T17:00:28.889Z
Learnt from: camevor
Repo: newton-physics/newton PR: 1161
File: newton/_src/solvers/mujoco/kernels.py:1268-1268
Timestamp: 2025-12-01T17:00:28.889Z
Learning: MuJoCo Warp spatial vectors (e.g., cacc, cvel) use the convention (angular, linear), which is opposite to Newton's (linear, angular) convention. When converting from MuJoCo Warp to Newton format, spatial_top() on MuJoCo Warp data extracts the angular component, and spatial_bottom() extracts the linear component.
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/solver_mujoco.py
🧬 Code graph analysis (1)
newton/_src/solvers/mujoco/solver_mujoco.py (4)
newton/_src/solvers/mujoco/kernels.py (1)
update_solver_options_kernel(914-930)newton/_src/sim/builder.py (3)
add_custom_attribute(649-694)ModelBuilder(70-6085)CustomAttribute(327-417)newton/_src/viewer/wind.py (2)
frequency(172-174)frequency(177-179)newton/_src/sim/model.py (2)
ModelAttributeFrequency(49-74)ModelAttributeAssignment(31-46)
🪛 GitHub Actions: Pull Request
newton/tests/test_mujoco_solver.py
[error] 4153-4153: NameError: name 'expected_invsqrt' is not defined in test_impratio_constructor_override.
🪛 Ruff (0.14.8)
newton/tests/test_mujoco_solver.py
4153-4153: Undefined name expected_invsqrt
(F821)
4155-4155: Undefined name expected_invsqrt
(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). (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 (6)
newton/_src/solvers/mujoco/solver_mujoco.py (6)
68-68: LGTM!The import of
update_solver_options_kernelis correctly added and necessary for the new per-world solver option synchronization functionality.
353-353: LGTM!The constructor signature and docstring correctly document the optional
impratioparameter and its fallback behavior (constructor → custom attribute → MuJoCo default).Also applies to: 379-379
1056-1062: LGTM!The option resolution logic correctly implements the precedence order (constructor → custom attribute → MuJoCo default) and properly tracks whether the value was overridden for later use in
_update_solver_options.Also applies to: 1072-1073
2083-2095: LGTM!The opt field expansion logic correctly mirrors the pattern used for model field expansion. The use of
impratio_invsqrtis correct, as MuJoCo Warp stores the inverse square root of impratio for performance reasons.Also applies to: 2124-2127
2129-2157: LGTM!The
_update_solver_optionsmethod correctly implements per-world solver option synchronization from Newton model attributes to MuJoCo Warp. The pattern of passingNonefor optional arrays is valid in Warp, as confirmed by the kernel implementation that checksif newton_impratio:before accessing the array.Based on learnings, this follows the established pattern from PR #1047.
1998-1999: LGTM!The call to
_update_solver_optionsis correctly placed after model field expansion and beforenotify_model_changed. This ensures that per-world solver options from the Newton model are synchronized to MuJoCo Warp when not overridden by constructor arguments.
Signed-off-by: Alain Denzler <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
newton/tests/test_mujoco_solver.py (1)
4103-4163: Previous critical issue has been fixed; consider strengthening test with heterogeneous initial values.The undefined variable
expected_invsqrtissue from the previous review has been resolved. The variable is now correctly defined at line 4150 before being used at line 4160.The test successfully verifies that constructor-provided
impratio=3.0overrides the model's per-world values for bothmj_modelandmjw_model.Minor suggestion: At line 4128, consider setting different per-world values (e.g.,
[1.5, 2.0]) instead of identical values. This would strengthen the test by demonstrating that the constructor override uniformly affects all worlds, regardless of their initial heterogeneous values.Apply this diff if desired:
- initial_impratio = np.array([1.5, 1.5], dtype=np.float32) + initial_impratio = np.array([1.5, 2.0], dtype=np.float32)Then update the assertions at lines 4133-4134 to match:
- self.assertAlmostEqual(impratio[0], 1.5, places=4) - self.assertAlmostEqual(impratio[1], 1.5, places=4) + self.assertAlmostEqual(impratio[0], 1.5, places=4) + self.assertAlmostEqual(impratio[1], 2.0, places=4)
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
newton/tests/test_mujoco_solver.py(1 hunks)
🧰 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.
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 479
File: newton/solvers/mujoco/solver_mujoco.py:2147-2149
Timestamp: 2025-07-28T15:04:54.360Z
Learning: The ls_parallel option is specific to MuJoCo Warp and is not available in the pure MuJoCo mjOption structure. It should only be set on mjw_model.opt, not mj_model.opt.
📚 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/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, 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.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/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 (1)
newton/tests/test_mujoco_solver.py (1)
4037-4101: LGTM! Comprehensive test for per-world impratio conversion.The test correctly verifies that WORLD-frequency impratio values are:
- Registered and stored in the model
- Propagated per-world to MuJoCo Warp as
impratio_invsqrt- Computed correctly (1/sqrt(impratio))
The test structure is clear and validates each world individually.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
newton/_src/utils/import_usd.py (1)
58-58: LGTM! Good backward compatibility.The new
parse_mujoco_optionsparameter is well-positioned and defaults toTrue, maintaining backward compatibility.Optional: Consider clarifying the docstring wording
The docstring on line 88 states "custom attributes retain their default values" when
parse_mujoco_optionsis False. This could be more precise—attributes retain their current values (which may differ from defaults if set elsewhere, e.g., via constructor arguments). Consider:- parse_mujoco_options (bool): Whether MuJoCo solver options from the PhysicsScene should be parsed. If False, solver options are not loaded and custom attributes retain their default values. Default is True. + parse_mujoco_options (bool): Whether MuJoCo solver options from the PhysicsScene should be parsed. If False, MuJoCo-specific custom attributes from the PhysicsScene are not loaded and retain their existing values. Default is True.Also applies to: 88-88
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
newton/_src/utils/import_usd.py(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 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: adenzler-nvidia
Repo: newton-physics/newton PR: 479
File: newton/solvers/mujoco/solver_mujoco.py:2147-2149
Timestamp: 2025-07-28T15:04:54.360Z
Learning: The ls_parallel option is specific to MuJoCo Warp and is not available in the pure MuJoCo mjOption structure. It should only be set on mjw_model.opt, not mj_model.opt.
📚 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/_src/utils/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/_src/utils/import_usd.py
🧬 Code graph analysis (1)
newton/_src/utils/import_usd.py (3)
newton/_src/sim/builder.py (3)
ModelBuilder(70-6085)CustomAttribute(327-417)key(410-412)newton/_src/sim/model.py (1)
ModelAttributeFrequency(49-74)newton/_src/usd/utils.py (1)
get_custom_attribute_values(469-493)
⏰ 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/_src/utils/import_usd.py (2)
966-977: LGTM! Clear and correct filtering logic.The two-step approach (build list of ONCE/WORLD attributes, then conditionally filter MuJoCo namespace) is clear and correct. The comment documenting WORLD attribute remapping behavior is helpful.
981-984: LGTM! Correct value assignment with good defensive check.The assignment of values at index 0 for both ONCE and WORLD frequencies is correct (WORLD attributes are documented to be remapped during
add_world()on line 967). The defensive check on line 983 is good practice, though by construction all keys inscene_custom_attrsshould exist inbuilder.custom_attributes.
I think this looks great @adenzler-nvidia! The rule constructor arg > custom attribute > MuJoCo default makes total sense to me. |
Signed-off-by: Alain Denzler <[email protected]>
Signed-off-by: Alain Denzler <[email protected]>
|
PR is updated - should be good to go now. I suggest moving forward with this and then I will add more options after that, and we can see how well this strategy works or whether we need an options class? |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
newton/_src/utils/import_mjcf.py (1)
36-66: Potential breaking change: new parameter inserted mid-signature can break positional callers.
Addingparse_optionsat Line 49 shifts positions forup_axisand everything after it. If any downstream code callsparse_mjcf(...)with positional args beyondparse_visuals, it will silently mis-bind arguments.Suggested fix: move
parse_optionsto the end or make everything aftersourcekeyword-only.Proposed fix (move new arg near end to preserve positional ordering)
def parse_mjcf( builder: ModelBuilder, source: str, xform: Transform | None = None, floating: bool | None = None, base_joint: dict | str | None = None, armature_scale: float = 1.0, scale: float = 1.0, hide_visuals: bool = False, parse_visuals_as_colliders: bool = False, parse_meshes: bool = True, parse_sites: bool = True, parse_visuals: bool = True, - parse_options: bool = True, up_axis: AxisType = Axis.Z, ignore_names: Sequence[str] = (), ignore_classes: Sequence[str] = (), visual_classes: Sequence[str] = ("visual",), collider_classes: Sequence[str] = ("collision",), no_class_as_colliders: bool = True, force_show_colliders: bool = False, enable_self_collisions: bool = False, ignore_inertial_definitions: bool = True, ensure_nonstatic_links: bool = True, static_link_mass: float = 1e-2, collapse_fixed_joints: bool = False, verbose: bool = False, skip_equality_constraints: bool = False, convert_3d_hinge_to_ball_joints: bool = False, mesh_maxhullvert: int = MESH_MAXHULLVERT, + parse_options: bool = True, ):Also applies to: 70-84
newton/_src/utils/import_usd.py (1)
38-61: Potential breaking change:parse_mujoco_optionsinserted beforemesh_maxhullvertcan break positional callers.
Any call using positional args up throughmesh_maxhullvertwill now mis-bind. Consider moving the new flag to the end (or making most args keyword-only).Also applies to: 88-91
newton/_src/sim/builder.py (1)
1531-1627: Same issue foradd_mjcf:parse_optionsinserted beforeup_axisbreaks positional callers; also prefer keyword pass-through.
A positional call that previously passedup_axisas the next arg will now bind it toparse_options.Proposed fix (preserve positional behavior + reduce coupling)
def add_mjcf( self, source: str, xform: Transform | None = None, floating: bool | None = None, base_joint: dict | str | None = None, armature_scale: float = 1.0, scale: float = 1.0, hide_visuals: bool = False, parse_visuals_as_colliders: bool = False, parse_meshes: bool = True, parse_sites: bool = True, parse_visuals: bool = True, - parse_options: bool = True, up_axis: AxisType = Axis.Z, @@ mesh_maxhullvert: int = MESH_MAXHULLVERT, + parse_options: bool = True, ): @@ - return parse_mjcf( - self, - source, - xform, - floating, - base_joint, - armature_scale, - scale, - hide_visuals, - parse_visuals_as_colliders, - parse_meshes, - parse_sites, - parse_visuals, - parse_options, - up_axis, - ignore_names, - ignore_classes, - visual_classes, - collider_classes, - no_class_as_colliders, - force_show_colliders, - enable_self_collisions, - ignore_inertial_definitions, - ensure_nonstatic_links, - static_link_mass, - collapse_fixed_joints, - verbose, - skip_equality_constraints, - convert_3d_hinge_to_ball_joints, - mesh_maxhullvert, - ) + return parse_mjcf( + self, + source=source, + xform=xform, + floating=floating, + base_joint=base_joint, + armature_scale=armature_scale, + scale=scale, + hide_visuals=hide_visuals, + parse_visuals_as_colliders=parse_visuals_as_colliders, + parse_meshes=parse_meshes, + parse_sites=parse_sites, + parse_visuals=parse_visuals, + parse_options=parse_options, + up_axis=up_axis, + ignore_names=ignore_names, + ignore_classes=ignore_classes, + visual_classes=visual_classes, + collider_classes=collider_classes, + no_class_as_colliders=no_class_as_colliders, + force_show_colliders=force_show_colliders, + enable_self_collisions=enable_self_collisions, + ignore_inertial_definitions=ignore_inertial_definitions, + ensure_nonstatic_links=ensure_nonstatic_links, + static_link_mass=static_link_mass, + collapse_fixed_joints=collapse_fixed_joints, + verbose=verbose, + skip_equality_constraints=skip_equality_constraints, + convert_3d_hinge_to_ball_joints=convert_3d_hinge_to_ball_joints, + mesh_maxhullvert=mesh_maxhullvert, + )
🧹 Nitpick comments (4)
newton/_src/utils/import_mjcf.py (1)
154-166: WORLD-frequency options should likely key offbuilder.current_world(not hard-coded index 0).
Line 165 always writes to.values[0]. That matches the “template world” use-case, but if someone imports MJCF into a builder that already has multiple worlds, this will populate world 0 (not the active world).Proposed tweak (use current world for WORLD attrs)
option_attrs = parse_custom_attributes(option_elem.attrib, builder_custom_attr_option, "mjcf") for key, value in option_attrs.items(): if key in builder.custom_attributes: - builder.custom_attributes[key].values[0] = value + attr = builder.custom_attributes[key] + idx = builder.current_world if attr.frequency == ModelAttributeFrequency.WORLD else 0 + attr.values[idx] = valuenewton/_src/utils/import_usd.py (1)
981-1001: When syncing WORLD-frequency scene attrs back to the builder, avoid hard-coding index 0.
Line 1000 writesvalues[0]for both ONCE and WORLD. That matches the “single-world template” convention, but it’s easy to get wrong when importing into an already-multiworld builder.Proposed tweak (parallel to MJCF suggestion)
# Set values on builder's custom attributes (similar to MJCF parsing) for key, value in scene_custom_attrs.items(): if key in builder.custom_attributes: - builder.custom_attributes[key].values[0] = value + attr = builder.custom_attributes[key] + idx = builder.current_world if attr.frequency == ModelAttributeFrequency.WORLD else 0 + attr.values[idx] = valuenewton/_src/sim/builder.py (1)
1404-1529: Parameter pass-through robustness: use keyword arguments to decouple wrapper from importer signatures.The positional pass-through to
parse_usd()andparse_mjcf()creates unnecessary fragility. While parameter names currently align correctly and all observed call sites use keyword arguments (avoiding immediate breakage), future signature changes to the importers will silently mis-bind arguments.Additionally,
add_mjcf()has a parameter naming inconsistency: it declaresparse_mujoco_optionsbutparse_mjcf()expectsparse_optionsat that position. This API inconsistency is confusing and best resolved through explicit keyword pass-through.To improve maintainability, convert the calls from positional to keyword pass-through:
Suggested fix for add_usd()
return parse_usd( - self, - source, - xform, - only_load_enabled_rigid_bodies, - only_load_enabled_joints, - joint_drive_gains_scaling, - verbose, - ignore_paths, - cloned_world, - collapse_fixed_joints, - enable_self_collisions, - apply_up_axis_from_stage, - root_path, - joint_ordering, - bodies_follow_joint_ordering, - skip_mesh_approximation, - load_sites, - load_visual_shapes, - hide_collision_shapes, - parse_mujoco_options, - mesh_maxhullvert, - schema_resolvers, + builder=self, + source=source, + xform=xform, + only_load_enabled_rigid_bodies=only_load_enabled_rigid_bodies, + only_load_enabled_joints=only_load_enabled_joints, + joint_drive_gains_scaling=joint_drive_gains_scaling, + verbose=verbose, + ignore_paths=ignore_paths, + cloned_world=cloned_world, + collapse_fixed_joints=collapse_fixed_joints, + enable_self_collisions=enable_self_collisions, + apply_up_axis_from_stage=apply_up_axis_from_stage, + root_path=root_path, + joint_ordering=joint_ordering, + bodies_follow_joint_ordering=bodies_follow_joint_ordering, + skip_mesh_approximation=skip_mesh_approximation, + load_sites=load_sites, + load_visual_shapes=load_visual_shapes, + hide_collision_shapes=hide_collision_shapes, + parse_mujoco_options=parse_mujoco_options, + mesh_maxhullvert=mesh_maxhullvert, + schema_resolvers=schema_resolvers, )Suggested fix for add_mjcf() — also align parameter name to match parse_mjcf()
def add_mjcf( self, source: str, xform: Transform | None = None, floating: bool | None = None, base_joint: dict | str | None = None, armature_scale: float = 1.0, scale: float = 1.0, hide_visuals: bool = False, parse_visuals_as_colliders: bool = False, parse_meshes: bool = True, parse_sites: bool = True, parse_visuals: bool = True, - parse_mujoco_options: bool = True, + parse_options: bool = True, up_axis: AxisType = Axis.Z, ignore_names: Sequence[str] = (), ignore_classes: Sequence[str] = (), visual_classes: Sequence[str] = ("visual",), collider_classes: Sequence[str] = ("collision",), no_class_as_colliders: bool = True, force_show_colliders: bool = False, enable_self_collisions: bool = False, ignore_inertial_definitions: bool = True, ensure_nonstatic_links: bool = True, static_link_mass: float = 1e-2, collapse_fixed_joints: bool = False, verbose: bool = False, skip_equality_constraints: bool = False, convert_3d_hinge_to_ball_joints: bool = False, mesh_maxhullvert: int = MESH_MAXHULLVERT, ): """ Parses MuJoCo XML (MJCF) file and adds the bodies and joints to the given ModelBuilder. Args: source (str): The filename of the MuJoCo file to parse, or the MJCF XML string content. ... - parse_mujoco_options (bool): Whether solver options from the MJCF `<option>` tag should be parsed. If False, solver options are not loaded and custom attributes retain their default values. Default is True. + parse_options (bool): Whether solver options from the MJCF `<option>` tag should be parsed. If False, solver options are not loaded and custom attributes retain their default values. Default is True. ... """ from ..utils.import_mjcf import parse_mjcf # noqa: PLC0415 return parse_mjcf( + builder=self, + source=source, + xform=xform, + floating=floating, + base_joint=base_joint, + armature_scale=armature_scale, + scale=scale, + hide_visuals=hide_visuals, + parse_visuals_as_colliders=parse_visuals_as_colliders, + parse_meshes=parse_meshes, + parse_sites=parse_sites, + parse_visuals=parse_visuals, + parse_options=parse_options, + up_axis=up_axis, + ignore_names=ignore_names, + ignore_classes=ignore_classes, + visual_classes=visual_classes, + collider_classes=collider_classes, + no_class_as_colliders=no_class_as_colliders, + force_show_colliders=force_show_colliders, + enable_self_collisions=enable_self_collisions, + ignore_inertial_definitions=ignore_inertial_definitions, + ensure_nonstatic_links=ensure_nonstatic_links, + static_link_mass=static_link_mass, + collapse_fixed_joints=collapse_fixed_joints, + verbose=verbose, + skip_equality_constraints=skip_equality_constraints, + convert_3d_hinge_to_ball_joints=convert_3d_hinge_to_ball_joints, + mesh_maxhullvert=mesh_maxhullvert, )newton/_src/solvers/mujoco/solver_mujoco.py (1)
2393-2421: Consider adding explicit device parameter for consistency.The
wp.launchcall omits thedevice=parameter, relying on the enclosingwp.ScopedDevicecontext. While this works correctly, otherwp.launchcalls in this file explicitly passdevice=self.model.devicefor clarity.♻️ Suggested change
wp.launch( update_solver_options_kernel, dim=nworld, inputs=[newton_impratio], outputs=[self.mjw_model.opt.impratio_invsqrt], + device=self.model.device, )
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
newton/_src/sim/builder.pynewton/_src/solvers/mujoco/kernels.pynewton/_src/solvers/mujoco/solver_mujoco.pynewton/_src/utils/import_mjcf.pynewton/_src/utils/import_usd.pynewton/tests/test_import_usd.pynewton/tests/test_mujoco_solver.py
🚧 Files skipped from review as they are similar to previous changes (1)
- newton/_src/solvers/mujoco/kernels.py
🧰 Additional context used
🧠 Learnings (26)
📓 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-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/_src/utils/import_usd.pynewton/_src/sim/builder.pynewton/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/_src/utils/import_usd.pynewton/_src/sim/builder.pynewton/tests/test_import_usd.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/sim/builder.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, 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/sim/builder.pynewton/tests/test_mujoco_solver.pynewton/_src/solvers/mujoco/solver_mujoco.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/tests/test_mujoco_solver.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/tests/test_mujoco_solver.pynewton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-08-12T05:17:34.423Z
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.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-11-10T12:33:57.428Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 1047
File: newton/_src/solvers/mujoco/solver_mujoco.py:1723-1749
Timestamp: 2025-11-10T12:33:57.428Z
Learning: In Warp, it is valid to pass None as a kernel input parameter for optional arrays. Inside the kernel, use `if array:` to check whether the array was provided before accessing it. This pattern eliminates the need for separate presence flags and dummy arrays.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-09-09T12:50:42.929Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 750
File: newton/_src/solvers/implicit_mpm/render_grains.py:142-160
Timestamp: 2025-09-09T12:50:42.929Z
Learning: The `update_render_grains` function in newton/_src/solvers/implicit_mpm/render_grains.py should return early when `state.velocity_field is None` because its purpose is specifically to advect grains in the velocity field. Without a velocity field, the function should be a no-op.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.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/_src/solvers/mujoco/solver_mujoco.py
📚 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/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-09-09T11:03:41.928Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 750
File: newton/_src/solvers/implicit_mpm/solve_rheology.py:892-895
Timestamp: 2025-09-09T11:03:41.928Z
Learning: In newton/_src/solvers/implicit_mpm/solve_rheology.py, passing None for compliance_mat_diagonal is intentional to avoid loading zeros from global memory when compliance_mat is None, rather than creating a zero matrix and extracting its diagonal.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-12-12T17:41:15.097Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 1221
File: newton/_src/sim/builder.py:5556-5587
Timestamp: 2025-12-12T17:41:15.097Z
Learning: newton._src.geometry.sdf_utils.compute_sdf accepts max_resolution=None (e.g., when sdf_target_voxel_size is provided), so passing None from ModelBuilder.finalize is valid and should not be flagged.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-08-12T18:04:06.577Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/_src/solvers/featherstone/kernels.py:75-75
Timestamp: 2025-08-12T18:04:06.577Z
Learning: The Newton physics framework requires nightly Warp builds, which means compatibility concerns with older stable Warp versions (like missing functions such as wp.spatial_adjoint) are not relevant for this project.
Applied to files:
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/solver_mujoco.py
📚 Learning: 2025-07-28T15:04:54.360Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 479
File: newton/solvers/mujoco/solver_mujoco.py:2147-2149
Timestamp: 2025-07-28T15:04:54.360Z
Learning: In the Newton MuJoCo solver implementation, the ls_parallel option may not be available in the pure MuJoCo mjOption structure, despite being documented in newer MuJoCo versions. The user adenzler-nvidia confirmed that mj_model does not have this attribute in their implementation, so ls_parallel should only be set on mjw_model.opt.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-09-26T05:58:21.013Z
Learnt from: WenchaoHuang
Repo: newton-physics/newton PR: 835
File: newton/_src/solvers/style3d/collision/collision.py:49-53
Timestamp: 2025-09-26T05:58:21.013Z
Learning: In newton/_src/solvers/style3d/collision/collision.py, the broad_phase buffers (broad_phase_ee, broad_phase_ef, broad_phase_vf) created with wp.array() don't need zero-initialization because they follow a write-first-read-later pattern: BVH query kernels in frame_begin() populate these buffers completely (write-only operations), and collision kernels only read from them afterward in accumulate_contact_force(), ensuring no undefined reads occur.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-07-28T15:04:54.360Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 479
File: newton/solvers/mujoco/solver_mujoco.py:2147-2149
Timestamp: 2025-07-28T15:04:54.360Z
Learning: The ls_parallel option is specific to MuJoCo Warp and is not available in the pure MuJoCo mjOption structure. It should only be set on mjw_model.opt, not mj_model.opt.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-07-14T03:57:29.670Z
Learnt from: Kenny-Vilella
Repo: newton-physics/newton PR: 398
File: newton/examples/example_mujoco.py:352-352
Timestamp: 2025-07-14T03:57:29.670Z
Learning: The use_mujoco option in newton/examples/example_mujoco.py is currently unsupported and causes crashes. The code automatically disables this option with a warning message when users attempt to enable it. This is intentionally kept as a placeholder for future implementation.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-10-06T18:14:18.582Z
Learnt from: daedalus5
Repo: newton-physics/newton PR: 868
File: newton/examples/diffsim/example_diffsim_bear.py:82-96
Timestamp: 2025-10-06T18:14:18.582Z
Learning: Warp's tile operations (`wp.tile_store()` and `wp.tile_load()`) have built-in boundary checking. Out-of-bounds writes are automatically skipped and out-of-bounds reads are zero-filled, so tiled kernels are safe even when the array size is not a multiple of the tile size.
Applied to files:
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.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-09-25T16:14:22.033Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/_src/sim/ik/ik_objectives.py:0-0
Timestamp: 2025-09-25T16:14:22.033Z
Learning: wp.transform in NVIDIA Warp supports indexing using numerical indices (e.g., body_tf[0], body_tf[3], etc.) to access translation and rotation components. The indexing approach used in the Newton codebase is valid and compiles correctly.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-12-01T17:00:28.889Z
Learnt from: camevor
Repo: newton-physics/newton PR: 1161
File: newton/_src/solvers/mujoco/kernels.py:1268-1268
Timestamp: 2025-12-01T17:00:28.889Z
Learning: MuJoCo Warp spatial vectors (e.g., cacc, cvel) use the convention (angular, linear), which is opposite to Newton's (linear, angular) convention. When converting from MuJoCo Warp to Newton format, spatial_top() on MuJoCo Warp data extracts the angular component, and spatial_bottom() extracts the linear component.
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/solver_mujoco.py
📚 Learning: 2025-11-07T01:42:36.906Z
Learnt from: daniela-hase
Repo: newton-physics/newton PR: 1044
File: newton/_src/sensors/tiled_camera_sensor.py:224-256
Timestamp: 2025-11-07T01:42:36.906Z
Learning: In newton/_src/sensors/tiled_camera_sensor.py, the `# noqa: PLC0415` directives on the local PIL imports (lines 224 and 256 in save_color_image and save_depth_image methods) should be kept because they are required by the pre-commit configuration. The local imports are intentional for optional dependency handling.
Applied to files:
newton/tests/test_import_usd.py
🧬 Code graph analysis (3)
newton/_src/utils/import_usd.py (3)
newton/_src/sim/builder.py (3)
ModelBuilder(70-6754)CustomAttribute(352-516)key(464-466)newton/_src/sim/model.py (1)
ModelAttributeFrequency(49-74)newton/_src/usd/utils.py (1)
get_custom_attribute_values(469-493)
newton/_src/utils/import_mjcf.py (3)
newton/_src/sim/builder.py (4)
ModelBuilder(70-6754)CustomAttribute(352-516)get_custom_attributes_by_frequency(812-826)key(464-466)newton/_src/sim/model.py (1)
ModelAttributeFrequency(49-74)newton/_src/utils/import_utils.py (1)
parse_custom_attributes(106-149)
newton/tests/test_mujoco_solver.py (2)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
register_custom_attributes(163-452)newton/_src/solvers/solver.py (1)
register_custom_attributes(307-314)
🪛 Ruff (0.14.10)
newton/tests/test_import_usd.py
2529-2529: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
2596-2596: 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). (4)
- 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)
- GitHub Check: run-newton-tests / newton-unittests (ubuntu-latest)
🔇 Additional comments (11)
newton/tests/test_import_usd.py (1)
2526-2643: Keep the# noqa: PLC0415directives—they are necessary.PLC0415 is explicitly enabled in the ruff configuration (
[tool.ruff.lint] selectincludes "PLC"), so thesenoqacomments suppress a real linting violation for local imports inside function scopes. Removing them would cause pre-commit to fail. This pattern is used consistently throughout the codebase (100+ occurrences) and is intentional for optional dependency handling.Likely an incorrect or invalid review comment.
newton/_src/solvers/mujoco/solver_mujoco.py (9)
70-70: LGTM!Import is correctly added for the new kernel required by
_update_solver_options.
552-552: LGTM!The optional
impratioparameter and its docstring clearly communicate the fallback semantics: constructor value takes precedence over model custom attributes, which take precedence over MuJoCo defaults.Also applies to: 578-578
1207-1207: LGTM!Parameter correctly propagated to the internal conversion method.
1312-1319: LGTM!The resolution logic correctly implements the precedence rule. Reading
numpy()[0]is appropriate here since it's for the single-world MuJoCo spec template; per-world values are applied later via_update_solver_options.
1328-1329: LGTM!Correctly guards against setting
spec.option.impratiowhen no value is available.
2262-2264: LGTM!The call is correctly placed after
expand_model_fields(which tiles arrays to the correct shape) and beforenotify_model_changed. Theimpratio_overriddenflag correctly controls whether per-world values from Newton model should overwrite the tiled values.
2347-2360: LGTM!Well-structured addition with commented fields serving as documentation for future extensibility. Using
impratio_invsqrt(MuJoCo's internal representation) is correct since that's what MuJoCo Warp exposes.
2388-2392: LGTM!The
hasattrcheck provides good forward/backward compatibility with different mujoco_warp versions. Reusing the sametile()function ensures consistent expansion behavior.
333-344: No action needed. MJCF parsing forimpratioworks correctly without explicitmjcf_attribute_name.The custom attribute registration in lines 333-344 is correct. The
parse_custom_attributesfunction (import_utils.py:106) has a fallback mechanism: whenmjcf_attribute_nameis None, it uses thenamefield ("impratio") to look up the attribute in MJCF's<option>element. Since the name matches the MJCF attribute name, parsing succeeds automatically. This is confirmed by existing tests (test_import_mjcf.py:1414-1489) demonstrating that<option impratio="1.5"/>is correctly parsed and assigned to the custom attribute.Likely an incorrect or invalid review comment.
newton/tests/test_mujoco_solver.py (1)
4078-4208: LGTM! Well-structured tests for the new impratio WORLD-frequency option.The tests comprehensively verify:
- Custom attribute registration and per-world array shape
- Correct propagation of per-world values to MuJoCo Warp (
impratio_invsqrt = 1/sqrt(impratio))- Constructor override precedence (as documented in the PR objectives)
The pattern of using
template_builder+replicateis consistent with other multi-world tests in this file.Note: The AI summary mentions duplicate
TestMuJoCoOptionsblocks, but only one class definition exists in the provided code—this appears to be a summary artifact, not an actual code issue.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
newton/_src/solvers/mujoco/kernels.py (1)
972-990: The Warp optional array pattern is correct and idiomatic; add defensive clamping for domain safety.The
if newton_impratio:check is the correct and idiomatic way to detect optional arrays in Warp kernels within Newton. However, add a clamp before1.0 / wp.sqrt()to defend against edge cases whereimpratio ≤ 0:Proposed fix
if newton_impratio: # MuJoCo stores impratio as inverse square root - opt_impratio_invsqrt[worldid] = 1.0 / wp.sqrt(newton_impratio[worldid]) + val = wp.max(newton_impratio[worldid], MJ_MINVAL) + opt_impratio_invsqrt[worldid] = 1.0 / wp.sqrt(val)newton/_src/utils/import_usd.py (1)
981-1001: Usebuilder.get_custom_attributes_by_frequency()for consistency with MJCF parsing.The USD import manually filters ONCE/WORLD attributes with a list comprehension, while MJCF already uses
builder.get_custom_attributes_by_frequency()for the same purpose. This inconsistency should be resolved by using the dedicated method in both places.Proposed fix
@@ - builder_custom_attr_model: list[ModelBuilder.CustomAttribute] = [ - attr - for attr in builder.custom_attributes.values() - if attr.frequency in (ModelAttributeFrequency.ONCE, ModelAttributeFrequency.WORLD) - ] + builder_custom_attr_model: list[ModelBuilder.CustomAttribute] = builder.get_custom_attributes_by_frequency( + [ModelAttributeFrequency.ONCE, ModelAttributeFrequency.WORLD] + )
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
newton/_src/sim/builder.pynewton/_src/solvers/mujoco/kernels.pynewton/_src/solvers/mujoco/solver_mujoco.pynewton/_src/utils/import_mjcf.pynewton/_src/utils/import_usd.pynewton/tests/test_import_usd.pynewton/tests/test_mujoco_solver.py
🚧 Files skipped from review as they are similar to previous changes (2)
- newton/_src/utils/import_mjcf.py
- newton/_src/sim/builder.py
🧰 Additional context used
🧠 Learnings (26)
📓 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-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/_src/utils/import_usd.pynewton/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/_src/utils/import_usd.pynewton/tests/test_import_usd.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.pynewton/tests/test_mujoco_solver.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-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.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.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/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-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/tests/test_mujoco_solver.pynewton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-08-12T05:17:34.423Z
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.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-11-10T12:33:57.428Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 1047
File: newton/_src/solvers/mujoco/solver_mujoco.py:1723-1749
Timestamp: 2025-11-10T12:33:57.428Z
Learning: In Warp, it is valid to pass None as a kernel input parameter for optional arrays. Inside the kernel, use `if array:` to check whether the array was provided before accessing it. This pattern eliminates the need for separate presence flags and dummy arrays.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-09-09T12:50:42.929Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 750
File: newton/_src/solvers/implicit_mpm/render_grains.py:142-160
Timestamp: 2025-09-09T12:50:42.929Z
Learning: The `update_render_grains` function in newton/_src/solvers/implicit_mpm/render_grains.py should return early when `state.velocity_field is None` because its purpose is specifically to advect grains in the velocity field. Without a velocity field, the function should be a no-op.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.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/_src/solvers/mujoco/solver_mujoco.py
📚 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/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-09-09T11:03:41.928Z
Learnt from: gdaviet
Repo: newton-physics/newton PR: 750
File: newton/_src/solvers/implicit_mpm/solve_rheology.py:892-895
Timestamp: 2025-09-09T11:03:41.928Z
Learning: In newton/_src/solvers/implicit_mpm/solve_rheology.py, passing None for compliance_mat_diagonal is intentional to avoid loading zeros from global memory when compliance_mat is None, rather than creating a zero matrix and extracting its diagonal.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-12-12T17:41:15.097Z
Learnt from: nvtw
Repo: newton-physics/newton PR: 1221
File: newton/_src/sim/builder.py:5556-5587
Timestamp: 2025-12-12T17:41:15.097Z
Learning: newton._src.geometry.sdf_utils.compute_sdf accepts max_resolution=None (e.g., when sdf_target_voxel_size is provided), so passing None from ModelBuilder.finalize is valid and should not be flagged.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-08-12T18:04:06.577Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/_src/solvers/featherstone/kernels.py:75-75
Timestamp: 2025-08-12T18:04:06.577Z
Learning: The Newton physics framework requires nightly Warp builds, which means compatibility concerns with older stable Warp versions (like missing functions such as wp.spatial_adjoint) are not relevant for this project.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-07-28T15:04:54.360Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 479
File: newton/solvers/mujoco/solver_mujoco.py:2147-2149
Timestamp: 2025-07-28T15:04:54.360Z
Learning: In the Newton MuJoCo solver implementation, the ls_parallel option may not be available in the pure MuJoCo mjOption structure, despite being documented in newer MuJoCo versions. The user adenzler-nvidia confirmed that mj_model does not have this attribute in their implementation, so ls_parallel should only be set on mjw_model.opt.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-09-26T05:58:21.013Z
Learnt from: WenchaoHuang
Repo: newton-physics/newton PR: 835
File: newton/_src/solvers/style3d/collision/collision.py:49-53
Timestamp: 2025-09-26T05:58:21.013Z
Learning: In newton/_src/solvers/style3d/collision/collision.py, the broad_phase buffers (broad_phase_ee, broad_phase_ef, broad_phase_vf) created with wp.array() don't need zero-initialization because they follow a write-first-read-later pattern: BVH query kernels in frame_begin() populate these buffers completely (write-only operations), and collision kernels only read from them afterward in accumulate_contact_force(), ensuring no undefined reads occur.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-07-28T15:04:54.360Z
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 479
File: newton/solvers/mujoco/solver_mujoco.py:2147-2149
Timestamp: 2025-07-28T15:04:54.360Z
Learning: The ls_parallel option is specific to MuJoCo Warp and is not available in the pure MuJoCo mjOption structure. It should only be set on mjw_model.opt, not mj_model.opt.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-07-14T03:57:29.670Z
Learnt from: Kenny-Vilella
Repo: newton-physics/newton PR: 398
File: newton/examples/example_mujoco.py:352-352
Timestamp: 2025-07-14T03:57:29.670Z
Learning: The use_mujoco option in newton/examples/example_mujoco.py is currently unsupported and causes crashes. The code automatically disables this option with a warning message when users attempt to enable it. This is intentionally kept as a placeholder for future implementation.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-10-06T18:14:18.582Z
Learnt from: daedalus5
Repo: newton-physics/newton PR: 868
File: newton/examples/diffsim/example_diffsim_bear.py:82-96
Timestamp: 2025-10-06T18:14:18.582Z
Learning: Warp's tile operations (`wp.tile_store()` and `wp.tile_load()`) have built-in boundary checking. Out-of-bounds writes are automatically skipped and out-of-bounds reads are zero-filled, so tiled kernels are safe even when the array size is not a multiple of the tile size.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-09-25T16:14:22.033Z
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/_src/sim/ik/ik_objectives.py:0-0
Timestamp: 2025-09-25T16:14:22.033Z
Learning: wp.transform in NVIDIA Warp supports indexing using numerical indices (e.g., body_tf[0], body_tf[3], etc.) to access translation and rotation components. The indexing approach used in the Newton codebase is valid and compiles correctly.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-12-01T17:00:28.889Z
Learnt from: camevor
Repo: newton-physics/newton PR: 1161
File: newton/_src/solvers/mujoco/kernels.py:1268-1268
Timestamp: 2025-12-01T17:00:28.889Z
Learning: MuJoCo Warp spatial vectors (e.g., cacc, cvel) use the convention (angular, linear), which is opposite to Newton's (linear, angular) convention. When converting from MuJoCo Warp to Newton format, spatial_top() on MuJoCo Warp data extracts the angular component, and spatial_bottom() extracts the linear component.
Applied to files:
newton/_src/solvers/mujoco/solver_mujoco.py
📚 Learning: 2025-11-07T01:42:36.906Z
Learnt from: daniela-hase
Repo: newton-physics/newton PR: 1044
File: newton/_src/sensors/tiled_camera_sensor.py:224-256
Timestamp: 2025-11-07T01:42:36.906Z
Learning: In newton/_src/sensors/tiled_camera_sensor.py, the `# noqa: PLC0415` directives on the local PIL imports (lines 224 and 256 in save_color_image and save_depth_image methods) should be kept because they are required by the pre-commit configuration. The local imports are intentional for optional dependency handling.
Applied to files:
newton/tests/test_import_usd.py
🧬 Code graph analysis (2)
newton/_src/utils/import_usd.py (2)
newton/_src/sim/model.py (1)
ModelAttributeFrequency(49-74)newton/_src/usd/utils.py (1)
get_custom_attribute_values(469-493)
newton/tests/test_import_usd.py (1)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
register_custom_attributes(163-452)
🪛 Ruff (0.14.10)
newton/tests/test_import_usd.py
2529-2529: Unused noqa directive (non-enabled: PLC0415)
Remove unused noqa directive
(RUF100)
2596-2596: 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 (13)
newton/tests/test_import_usd.py (1)
2526-2643: Fix Ruff RUF100: remove unused# noqa: PLC0415on local USD imports.Ruff reports the
# noqa: PLC0415directives are unused here (rule not enabled), which will fail lint. The tests can keep local imports (they’re already guarded byUSD_AVAILABLE) without the unused noqa.Proposed diff
@@ - from pxr import Usd # noqa: PLC0415 + from pxr import Usd @@ - from pxr import Usd # noqa: PLC0415 + from pxr import Usd⛔ Skipped due to learnings
Learnt from: daniela-hase Repo: newton-physics/newton PR: 1044 File: newton/_src/sensors/tiled_camera_sensor.py:224-256 Timestamp: 2025-11-07T01:42:36.906Z Learning: In newton/_src/sensors/tiled_camera_sensor.py, the `# noqa: PLC0415` directives on the local PIL imports (lines 224 and 256 in save_color_image and save_depth_image methods) should be kept because they are required by the pre-commit configuration. The local imports are intentional for optional dependency handling.newton/_src/utils/import_usd.py (1)
38-61: Parameter is correctly propagated and documented.The
parse_mujoco_options: bool = Trueparameter is consistently implemented acrossparse_usd()andModelBuilder.add_usd()with proper forwarding (newton/_src/sim/builder.py:1506–1527). Documentation clearly explains behavior: "Whether MuJoCo solver options from the PhysicsScene should be parsed. If False, solver options are not loaded and custom attributes retain their default values."newton/tests/test_mujoco_solver.py (1)
4078-4207: LGTM! Well-structured tests for per-world impratio option support.The test class appropriately validates:
- Custom attribute registration and array shape (one value per world)
- Per-world value storage in Newton model
- Correct expansion to MuJoCo Warp (
impratio_invsqrt = 1/sqrt(impratio))- Constructor override precedence (aligns with PR's documented behavior: constructor arg > custom attribute > MuJoCo default)
The tests follow established patterns from other property tests in this file and provide good coverage for the new functionality.
newton/_src/solvers/mujoco/solver_mujoco.py (10)
333-344: LGTM - Consider addingmjcf_attribute_namefor MJCF import support.The custom attribute registration is correct with
WORLDfrequency for per-world values. However,mjcf_attribute_nameis not specified, which means this won't be automatically parsed from MJCF<option>elements. This may be intentional given the PR notes that finer tuning for USD/MJCF awaits finalized schemas.
552-552: LGTM!The
impratioparameter is properly typed asfloat | Noneto allow the fallback mechanism. The docstring clearly documents the resolution priority (constructor → model attribute → MuJoCo default).Also applies to: 566-571, 584-584
1213-1213: LGTM!The
_convert_to_mjcmethod signature and documentation are consistent with the constructor. The resolution priority documentation is appropriately duplicated here since this is a separate entry point.Also applies to: 1230-1265
1347-1356: LGTM - resolution logic is correct.The logic correctly implements the priority order:
- If
impratiois provided via constructor,impratio_overridden=Trueensures per-world values aren't overwritten later- If not provided, falls back to the first world's value from
model.mujoco.impratiofor the MuJoCo spec- Per-world values are then applied via
_update_solver_optionswhen not overridden
1365-1366: LGTM!Correctly guards setting
spec.option.impratioto preserve MuJoCo's default when no value is specified.
2299-2300: LGTM!Correctly placed after
expand_model_fields(which tiles the arrays) and beforenotify_model_changed. Theimpratio_overriddenflag ensures per-world values from model attributes are only applied when the constructor didn't override.
2384-2397: LGTM!Good separation of solver option fields (
opt_fields_to_expand) from model fields. Note that MuJoCo Warp internally usesimpratio_invsqrt(1/√impratio) rather thanimpratiodirectly - the kernel in_update_solver_optionshandles this conversion.
2425-2428: LGTM!Correctly extends the tiling logic to handle nested
optfields. Thehasattrcheck provides defensive handling for optional fields.
2430-2459: LGTM - well-structured solver options update mechanism.The method correctly:
- Skips kernel launch when constructor override was used (
impratio_overridden=True) sinceexpand_model_fieldsalready tiled the value- Uses the valid Warp pattern of passing
Nonefor optional arrays (per learnings)- Only launches the kernel when there are per-world values to apply
- The kernel handles the conversion from
impratiotoimpratio_invsqrt(1/√impratio)The
all_nonecheck with the comment# and other_option is None and ...provides a clear extension pattern for future solver options.
70-70: LGTM! Import is correctly added for the new kernel used in_update_solver_options.
|
I made an intermediate PR #1327 to address some coderabbit feedback. Will wait with this PR until 1327 is through. |
2 participants
This PR is a first step at adding custom attribute support for parsing MuJoCo options from MJCF/MJC schemas. I just focused on impration to have an example and get the infrastructure working.
This will introduce a duplicate way to specify solver options, and comes with the price of us having to figure out whether options have been overriden. The PR makes the following assumptions/decisions:
Before your PR is "Ready for review"
newton/tests/test_examples.py)pre-commit run -aSummary by CodeRabbit
Release Notes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.