Site based constraints #1291
Site based constraints #1291
Conversation
📝 WalkthroughWalkthroughAdded a safe body-name helper in the MuJoCo solver and implemented site-based handling for MJCF Changes
Sequence Diagram(s)(Skipped — changes are parsing/naming/tests without a new multi-component sequential control flow.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 |
![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 (3)
newton/_src/utils/import_mjcf.py (2)
987-1025: Behavior of site-based CONNECT constraints and missing-site handlingThe new
site1/site2branch for<connect>correctly maps:
site1→body1_idxand anchor, and- optional
site2→body2_idx,falling back to
body2(or world via-1) whensite2is omitted, and skipping the constraint if a referenced site cannot be resolved.Two points worth tightening:
- Anchor frame consistency: In the site path,
anchor_veccomes from the site transform, while in the non‑site path it comes directly from the MJCFanchorattribute. If MuJoCo’sconnectanchor is interpreted in world coordinates, confirm thatanchor_vecfrom the site path is in that same frame (see prior comment onget_site_body_and_anchor).- Error visibility for bad site2: When
site2is specified but missing, the constraint is silently dropped (aside from an optional verbose print). For malformed MJCF this can be hard to notice. Consider raising a clearer error or at least including the constraint name in the warning to make debugging easier.The overall structure looks good; I’d just verify the frame conventions and consider improving diagnostics around unresolved sites.
1043-1095: Site-based WELD constraints: anchor semantics and type conversionThe new
site1 or site2branch for<weld>covers:
site1→(body1_idx, anchor_vec)via the site, or falls back to(body1_name, anchor)whensite1is absent.site2→body2_idxvia the site, or falls back tobody2_name.A couple of details to validate:
Anchor definition vs. MuJoCo semantics:
add_equality_constraint_weldis documented as takinganchor“relative to body2”, while here:
- When
site1is used,anchor_vecis derived fromsite1(on body1 or world), not body2.- When only
site2is used,anchor_veccomes from theanchorattribute.Please confirm the intended frame of
anchorin the Newton equality model and ensure it matches how_convert_to_mjcinterpretseq_constraint_anchorwhen fillingeq.data[0:3]formjEQ_WELD. If the frame is world‑space, it may be fine; if it is body‑2‑local, you might need to convert site/world positions accordingly.
torquescaletype:torquescaleis passed through as the raw string attribute value (same as the existing non‑site path). Ifadd_equality_constraint_weldexpects a float, consider normalizing here (float(torquescale)) for consistency and to fail fast on invalid inputs.Functionally the structure is sound; I’d just make the anchor frame and
torquescaletyping explicit and verify them against MuJoCo/Newton weld semantics.newton/tests/test_import_mjcf.py (1)
1669-1781: Extend site-based equality tests to cover names and anchorsThese tests nicely exercise the new site-based CONNECT/WELD paths end‑to‑end (through
SolverMuJoCo) and confirmneqandeq_type. They don’t currently check:
- That
eq_name1/eq_name2resolve as expected (e.g.,"world"for world sites, proper body names for site‑attached bodies), or- That
eq_data’s position entries match the corresponding site positions.Adding a couple of assertions per test (e.g., on
solver.mj_model.eq_obj1id/eq_obj2idviamj_name2id,eq_name1/2, andeq_data[0:3]vs. the known site coordinates) would give much stronger coverage of the newget_body_nameandget_site_body_and_anchorlogic without significantly increasing test complexity.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
newton/_src/solvers/mujoco/solver_mujoco.pynewton/_src/utils/import_mjcf.pynewton/tests/test_import_mjcf.py
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
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.
Learnt from: adenzler-nvidia
Repo: newton-physics/newton PR: 1107
File: newton/_src/solvers/mujoco/kernels.py:973-974
Timestamp: 2025-11-24T08:05:21.390Z
Learning: In Newton's MuJoCo solver integration (newton/_src/solvers/mujoco/), the mapping between Newton joints and MuJoCo joints is not 1-to-1. Instead, each Newton DOF maps to a distinct MuJoCo joint. This means that for multi-DOF joints (like D6 with 6 DOFs), there will be 6 corresponding MuJoCo joints, each with its own properties (margin, solimp, solref, etc.). The mapping is done via dof_to_mjc_joint array, ensuring each DOF's properties are written to its own MuJoCo joint without overwriting.
Learnt from: gdaviet
Repo: newton-physics/newton PR: 631
File: newton/examples/mpm/example_anymal_c_walk_on_sand.py:164-169
Timestamp: 2025-08-25T11:07:47.818Z
Learning: In Newton's MuJoCo solver implementation, setting TARGET_POSITION mode on floating base DOFs (first 6 DOFs) does not overconstrain the base - the solver handles this appropriately and maintains expected behavior for floating robots.
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.
Learnt from: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/_src/sim/ik/ik_lbfgs_optimizer.py:638-648
Timestamp: 2025-09-22T21:03:18.367Z
Learning: The L-BFGS optimizer in newton/_src/sim/ik/ik_lbfgs_optimizer.py currently only supports additive updates, meaning it assumes n_coords == n_dofs. Velocity space integration for joints with mismatched coordinate/DOF dimensions (like free/ball joints) is planned for future work.
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: dylanturpin
Repo: newton-physics/newton PR: 806
File: newton/_src/sim/ik/ik_lbfgs_optimizer.py:739-752
Timestamp: 2025-09-22T21:03:39.624Z
Learning: The L-BFGS optimizer in newton/_src/sim/ik/ik_lbfgs_optimizer.py currently intentionally only supports additive updates (assuming n_coords == n_dofs). Velocity space integration for joints with mismatched coordinate/DOF dimensions (like free/ball joints) is planned for future work and should not be flagged as an issue in current reviews.
📚 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-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/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/utils/import_mjcf.py
🧬 Code graph analysis (3)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
newton/_src/sim/joints.py (1)
EqType(82-97)
newton/_src/utils/import_mjcf.py (1)
newton/_src/sim/builder.py (3)
add_equality_constraint_connect(2683-2715)key(410-412)add_equality_constraint_weld(2751-2789)
newton/tests/test_import_mjcf.py (1)
newton/_src/sim/builder.py (1)
add_mjcf(1338-1431)
⏰ 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 (2)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
1686-1691: World body name handling for equality constraints looks correctMapping
body_idx == -1to"world"viaget_body_name()avoids out-of-range indexing onmodel.body_keyand aligns MuJoCo’s expectation that the world body is addressed by name"world". The refactor keeps behavior unchanged for regular bodies and should make world‑referencing CONNECT/WELD constraints robust.Also applies to: 1698-1699, 1718-1719
newton/_src/utils/import_mjcf.py (1)
971-985: Clarify site transform frame used for anchors
get_site_body_and_anchor()pulls the anchor frombuilder.shape_transform[site_idx]and directly uses its first three components as a position. This assumes that:
builder.shape_transformfor sites exposes translation in indices0:3, and- that this translation is in the frame expected by equality constraints (same convention as the non‑site
anchorattribute path).If
shape_transformis stored body‑local for non‑world bodies (which is common) while non‑site anchors are interpreted in world coordinates, this could make site‑based anchors inconsistent with body/anchor‑based constraints.Please double‑check:
- The exact representation of
builder.shape_transformfor sites (world vs body‑local, and indexing layout).- That
eq_constraint_anchoris consumed in the same frame in_convert_to_mjcfor both site‑based and non‑site equalities.If necessary, adjust to compute a world‑space anchor (e.g., by composing the site’s local transform with the body’s world transform) so that site‑based and anchor‑based equalities are consistent.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
newton/_src/utils/import_mjcf.py (1)
971-985: Consider verifying that the retrieved shape is actually a site.The function looks up
site_nameinbuilder.shape_key, which contains names for all shapes (collision, visual, and sites). If a collision or visual shape happens to have the same name as a site, this function could return incorrect data.Consider adding a type check to verify the shape is actually a site, for example by checking
builder.shape_geo_type[site_idx]or another indicator that distinguishes sites from other shapes.💡 Suggested verification
Run the following script to check if there's a way to distinguish sites from other shapes in the builder:
#!/bin/bash # Search for site-related attributes in the builder that could be used for type checking rg -n "shape_geo_type|GeoType\.|add_site" --type=py -C 3
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
newton/_src/utils/import_mjcf.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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.
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.
🧬 Code graph analysis (1)
newton/_src/utils/import_mjcf.py (1)
newton/_src/sim/builder.py (3)
add_equality_constraint_connect(2683-2715)key(410-412)add_equality_constraint_weld(2751-2789)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run GPU Tests / Run GPU Unit Tests on AWS EC2
- GitHub Check: Run GPU Benchmarks / Run GPU Benchmarks on AWS EC2
- GitHub Check: run-newton-tests / newton-unittests (windows-latest)
🔇 Additional comments (2)
newton/_src/utils/import_mjcf.py (2)
996-1026: Site-based connect constraint implementation looks good.The logic correctly handles the site-based connect constraint workflow:
- Uses site1 to determine body1 and anchor
- Optionally uses site2 to determine body2, falling back to body2_name
- Proper error handling when sites are not found
- Maintains backward compatibility with body-name-based constraints
1054-1098: Site-based weld constraint implementation looks good.The logic correctly implements site-based weld constraints:
- Uses site1 to determine body1 (if provided)
- Uses site2 to determine body2 and anchor in body2's frame (if provided)
- Properly parses relpose and creates the transform
- Maintains backward compatibility with body-name-based constraints
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
newton/_src/utils/import_mjcf.pynewton/tests/test_equality_constraints.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
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.
📚 Learning: 2025-08-12T18:04:06.287Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/_src/utils/import_usd.py:364-365
Timestamp: 2025-08-12T18:04:06.287Z
Learning: The quat_between_axes function in Newton accepts AxisType parameters, which includes strings like "X", "Y", "Z" as well as Axis enum values and integers. The function internally handles conversion using Axis.from_any(), so passing strings directly to quat_between_axes is valid and correct behavior.
Applied to files:
newton/_src/utils/import_mjcf.py
🪛 GitHub Actions: Pull Request
newton/tests/test_equality_constraints.py
[error] 1-1: MuJoCo solver compilation failed due to NaN/Inf or unstable state in QACC at DOF 0. Time = 0.0000.
🪛 Ruff (0.14.10)
newton/tests/test_equality_constraints.py
418-418: Local variable body_remap is assigned to but never used
Remove assignment to unused variable body_remap
(F841)
⏰ 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 (10)
newton/_src/utils/import_mjcf.py (4)
29-29: LGTM!The
ShapeFlagsimport is correctly added to support site detection in the new helper function.
1019-1047: LGTM!The site-based connect constraint logic correctly resolves both sites, uses the anchor from
site1, and provides appropriate warnings when inputs are incomplete.
1096-1100: The defaultrelposeis not identity—it's "0 1 0 0 0 0 0" (position offset of (0, 1, 0) with identity rotation).The code at lines 1096-1100 parses the
relposeXML attribute with the default "0 1 0 0 0 0 0", which doesn't represent an identity pose. This unusual default (position offset in the y-direction) suggests either a bug or an undocumented design choice. Verify that this default relpose is intentional and matches the expected behavior for site-based weld constraints in MuJoCo.
971-989: No issues found. The code correctly extracts the anchor position fromshape_transformusing direct indexing of the translation components ([0],[1],[2]), which is the standard pattern in the codebase.newton/tests/test_equality_constraints.py (6)
82-125: LGTM!The body-based robot creation helper is well-structured with clear documentation and correct constraint setup.
127-154: LGTM!The site-based robot creation helper correctly mirrors the body-based version using MJCF with appropriate site definitions.
156-229: LGTM!Comprehensive test that verifies constraint handling for both body-based and site-based scenarios across multiple worlds. The assertions properly validate constraint counts and body index remapping.
231-311: LGTM!Well-structured helper that creates a chain suitable for testing
collapse_fixed_jointswith equality constraints. The returned info dict enables detailed verification of remapping behavior.
313-345: LGTM!The site-based chain helper correctly creates an equivalent structure using MJCF. The absence of a joint equality constraint (compared to body-based) is appropriate since joint constraints don't support site-based definitions.
402-414: No evidence of pipeline failures or solver initialization issues related to this code. The search for the referenced error message "QACC at DOF" returns no results in the codebase, and the test itself is properly validating constraint counts after solver creation. The relpose value at line 340 is a benign constraint offset and not problematic.Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
newton/_src/utils/import_mjcf.py (1)
1096-1100: Consider auto-computing relpose for site-based welds.The current implementation uses the XML
relposeattribute directly for site-based welds (defaulting to identity). In a site-based workflow, users might expect the relpose to be automatically derived from the relative transforms of site1 and site2 when not explicitly specified.If this is intentional to maintain parity with the body-based path, consider documenting this behavior in verbose output.
newton/tests/test_equality_constraints.py (2)
132-185: Clarify test scenario naming.The method is named
_create_robot_site_basedand docstring says "site-based equality constraints," but lines 176-184 use body indices directly (body1=base, body2=link2) rather than site-based constraint creation. The sites are added but not used by the constraints.This appears to test models with sites present, not actual site-based constraint resolution. Consider renaming to
_create_robot_with_sitesor updating the docstring to clarify the test intent.
316-403: Consider reducing duplication between chain builders.
_create_chain_site_basedis nearly identical to_create_chain_body_based, differing only in the addition of sites. While explicit test code can be valuable for clarity, this could be refactored to a single helper with anadd_sitesparameter to reduce maintenance burden.Same observation as the robot helpers: the "site_based" name is misleading since constraints still use body indices directly.
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
newton/_src/utils/import_mjcf.pynewton/tests/test_equality_constraints.py
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
Learnt from: preist-nvidia
Repo: newton-physics/newton PR: 701
File: CONTRIBUTING.md:7-7
Timestamp: 2025-09-02T13:59:02.072Z
Learning: The newton-physics/newton-governance repository's CONTRIBUTING.md file contains both "Legal Requirements" (line 73, anchor #legal-requirements) and "Project Members" (line 108, anchor #project-members) sections that can be properly referenced from other repositories' CONTRIBUTING.md files.
📚 Learning: 2025-08-12T17:51:37.474Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/geometry.py:16-22
Timestamp: 2025-08-12T17:51:37.474Z
Learning: In the Newton public API refactor, common geometry symbols like ParticleFlags and ShapeFlags are now exposed at the top level in newton/__init__.py rather than through newton.geometry. The newton/geometry.py module is intentionally focused only on broad-phase collision detection classes (BroadPhaseAllPairs, BroadPhaseExplicit, BroadPhaseSAP).
Applied to files:
newton/_src/utils/import_mjcf.py
📚 Learning: 2025-08-12T18:04:06.287Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/_src/utils/import_usd.py:364-365
Timestamp: 2025-08-12T18:04:06.287Z
Learning: The quat_between_axes function in Newton accepts AxisType parameters, which includes strings like "X", "Y", "Z" as well as Axis enum values and integers. The function internally handles conversion using Axis.from_any(), so passing strings directly to quat_between_axes is valid and correct behavior.
Applied to files:
newton/_src/utils/import_mjcf.py
🧬 Code graph analysis (2)
newton/tests/test_equality_constraints.py (1)
newton/_src/sim/builder.py (4)
key(435-437)add_equality_constraint_connect(2785-2817)add_equality_constraint_joint(2819-2851)add_equality_constraint_weld(2853-2891)
newton/_src/utils/import_mjcf.py (2)
newton/_src/geometry/flags.py (1)
ShapeFlags(30-48)newton/_src/sim/builder.py (3)
add_equality_constraint_connect(2785-2817)key(435-437)add_equality_constraint_weld(2853-2891)
⏰ 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 (8)
newton/_src/utils/import_mjcf.py (4)
29-29: LGTM!The
ShapeFlagsimport is correctly added to support the site detection logic viaShapeFlags.SITEflag check in the newget_site_body_and_anchorhelper.
971-989: LGTM!The helper correctly validates site existence and type before extracting body index and anchor position. The guard
site_name not in builder.shape_keypreventsValueErrorfromindex()call.
1019-1047: LGTM!The site-based connect implementation correctly:
- Resolves both sites and validates their existence
- Uses site1's anchor (appropriate for connect constraints where anchor is on body1)
- Guards the warning with
if verbose- Uses
continueto skip invalid constraints gracefully
1085-1118: LGTM!The site-based weld implementation correctly:
- Uses site2's anchor position (matching the builder API where anchor is "relative to body2")
- Properly parses and applies relpose transformation
- Guards all warnings with
if verbose(past review issue has been addressed)newton/tests/test_equality_constraints.py (4)
82-130: LGTM!The helper correctly creates a robot with body-based equality constraints, establishing a clear baseline for comparison with the site-based variant.
187-229: LGTM!Good parameterized test structure using
subTestto verify constraint handling in both scenarios. The assertions correctly validate that:
- Newton model aggregates constraints across all worlds (6 total)
- MuJoCo solver with
separate_worlds=Trueonly includes per-world constraints (2)
231-314: LGTM!Comprehensive helper that returns both the builder and a detailed info dictionary for verification. This enables thorough testing of index remapping after
collapse_fixed_joints.
405-497: LGTM!Comprehensive test that verifies:
- Body count reduction after collapse
- Constraint preservation
- Valid body/joint index bounds
- Correct remapping of constraint references
- Proper transformation of anchor points and relpose
Note: The
body_remapvariable (line 443) is now used at lines 451-453, so the previous review comment about it being unused no longer applies.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
newton/tests/test_equality_constraints.py (2)
124-129: Consider using joint variables instead of hardcoded indices.The hardcoded indices
joint1=1andjoint2=2work correctly, but using the actual joint variable indices would be clearer and more maintainable:🔎 Proposed fix
robot.add_equality_constraint_joint( - joint1=1, - joint2=2, + joint1=joint2, + joint2=joint3, polycoef=[1.0, -1.0, 0, 0, 0], key="joint_constraint", )
179-184: Same hardcoded index issue applies here.For consistency with the suggestion on
_create_robot_body_based, consider using joint variables:🔎 Proposed fix
robot.add_equality_constraint_joint( - joint1=1, - joint2=2, + joint1=joint2, + joint2=joint3, polycoef=[1.0, -1.0, 0, 0, 0], key="joint_constraint", )
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
newton/_src/utils/import_mjcf.pynewton/tests/test_equality_constraints.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
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.
Learnt from: preist-nvidia
Repo: newton-physics/newton PR: 701
File: CONTRIBUTING.md:7-7
Timestamp: 2025-09-02T13:59:02.072Z
Learning: The newton-physics/newton-governance repository's CONTRIBUTING.md file contains both "Legal Requirements" (line 73, anchor #legal-requirements) and "Project Members" (line 108, anchor #project-members) sections that can be properly referenced from other repositories' CONTRIBUTING.md files.
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.
📚 Learning: 2025-08-12T18:04:06.287Z
Learnt from: nvlukasz
Repo: newton-physics/newton PR: 519
File: newton/_src/utils/import_usd.py:364-365
Timestamp: 2025-08-12T18:04:06.287Z
Learning: The quat_between_axes function in Newton accepts AxisType parameters, which includes strings like "X", "Y", "Z" as well as Axis enum values and integers. The function internally handles conversion using Axis.from_any(), so passing strings directly to quat_between_axes is valid and correct behavior.
Applied to files:
newton/_src/utils/import_mjcf.py
🧬 Code graph analysis (1)
newton/_src/utils/import_mjcf.py (2)
newton/_src/geometry/flags.py (1)
ShapeFlags(30-48)newton/_src/sim/builder.py (3)
add_equality_constraint_connect(2785-2817)key(435-437)add_equality_constraint_weld(2853-2891)
🔇 Additional comments (7)
newton/_src/utils/import_mjcf.py (3)
971-989: LGTM - Site lookup helper is well-implemented.The function correctly guards against missing sites and non-site shapes, extracts the anchor position from the shape transform, and provides helpful verbose warnings.
1019-1047: LGTM - Site-based connect constraint handling.The implementation correctly resolves both sites, uses site1's position as the anchor point (matching MuJoCo semantics), and properly guards the warning with the verbose flag.
1085-1118: LGTM - Site-based weld constraint handling.The implementation correctly resolves both sites, uses anchor from site2 (matching MuJoCo semantics where anchor is relative to body2), and the warning is now properly guarded by
if verbose:. The relpose handling follows the same pattern as body-based welds.newton/tests/test_equality_constraints.py (4)
187-229: LGTM - Well-structured test for constraint duplication.Good use of
subTestfor testing both scenarios, and clear assertions for expected constraint counts in both Newton model and MuJoCo solver contexts.
231-314: LGTM - Well-designed helper with comprehensive info dict.The helper returns all relevant indices and transforms needed for detailed post-collapse validation, enabling thorough testing of constraint remapping.
316-403: LGTM - Consistent with body-based helper.The site-based chain helper follows the same structure as the body-based version, adding sites to each link while maintaining the same info dict return format for unified validation.
405-497: LGTM - Comprehensive post-collapse validation.The test thoroughly verifies constraint remapping including body/joint index updates, merged body tracking, and proper transformation of anchor points and relpose. The past review comment about unused
body_remapno longer applies as it's now used in the validation logic (lines 451-453).
adenzler-nvidia
left a comment
adenzler-nvidia
left a comment
There was a problem hiding this comment.
Change looks good, could you please add a test for this?
Thanks, test_multiple_constraints does check that constraints with sites are satisfied. I added now more explicit testing that parsing worked correctly for them |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
newton/tests/test_equality_constraints.py (2)
42-58: Add error handling for missing constraints or use more descriptive assertions.The test directly calls
eq_keys.index("c_site")andeq_keys.index("w_site")without verifying these constraints exist in the loaded asset. If the asset file is missing, malformed, or doesn't contain these constraints, the test will fail with a genericValueErrorrather than a clear assertion message.Consider adding explicit assertions before indexing:
self.assertIn("c_site", eq_keys, "Expected 'c_site' constraint not found in model") self.assertIn("w_site", eq_keys, "Expected 'w_site' constraint not found in model")Alternatively, use dictionary-style access with
.get()and assert the result is not None, or wrap the index calls in a try-except with a more descriptive error message.
82-82: Consider adding a comment explaining the expected constraint count.The assertion expects exactly 5 equality constraints, but there's no context in the test about why 5 is the expected value. Adding a brief comment would make the test more maintainable when the asset file changes.
Example:
# constraints.xml contains: c_site, w_site, and 3 others self.assertEqual(self.solver.mj_model.eq_type.shape[0], 5)
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
newton/tests/test_equality_constraints.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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.
Learnt from: preist-nvidia
Repo: newton-physics/newton PR: 701
File: CONTRIBUTING.md:7-7
Timestamp: 2025-09-02T13:59:02.072Z
Learning: The newton-physics/newton-governance repository's CONTRIBUTING.md file contains both "Legal Requirements" (line 73, anchor #legal-requirements) and "Project Members" (line 108, anchor #project-members) sections that can be properly referenced from other repositories' CONTRIBUTING.md files.
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.
⏰ 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-newton-tests / newton-unittests (windows-latest)
🔇 Additional comments (2)
newton/tests/test_equality_constraints.py (2)
97-224: LGTM! Comprehensive test for separate_worlds constraint handling.The test thoroughly verifies:
- Correct constraint count in both Newton and MuJoCo models
- Proper index remapping for bodies and joints across multiple worlds
- That separate_worlds=True prevents constraint duplication
The logic and math for offset calculations (3 bodies/joints per world) are correct.
225-368: The test expectation is correct. The anchor should not be transformed in this case.The implementation in
collapse_fixed_joints(builder.py lines 3363-3385) explicitly handles WELD constraints differently depending on which body is merged:
- When
body1_was_merged(link2 merges into link1): only relpose is transformed, anchor remains unchanged- When
body2_was_merged: both anchor and relpose are transformedIn your test, link2 (body1) is merged into link1, so the first condition applies. The anchor is defined in the body1 (link2) frame and semantically belongs to that body, so it remains unchanged when body1 is merged. Only the relative pose (
relpose) needs transformation to maintain the spatial relationship between the merged body and body2.The test at line 299 correctly expects
expected_anchor = original_anchor, which matches the implementation behavior.
|
Thanks for the test. Could you merge latest main such the the dependencies are correct for the tests? Otherwise I think it looks good. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
1733-1748: Consider usingget_body_namehelper for consistency.The loop-closure joints (lines 1737, 1738, 1745, 1746) directly access
model.body_key[joint_parent[j]]andmodel.body_key[joint_child[j]]without using theget_body_namehelper. While loop joints may not reference the world body in practice, using the helper would:
- Be consistent with explicit equality constraint handling (lines 1700-1701, 1720-1721)
- Provide defensive handling if world-referenced loop joints are ever encountered
- Make the code's intent clearer
♻️ Proposed refactor for consistency
eq = spec.add_equality(objtype=mujoco.mjtObj.mjOBJ_BODY) eq.type = mujoco.mjtEq.mjEQ_CONNECT eq.active = True - eq.name1 = model.body_key[joint_parent[j]] - eq.name2 = model.body_key[joint_child[j]] + eq.name1 = get_body_name(joint_parent[j]) + eq.name2 = get_body_name(joint_child[j]) eq.data[0:3] = joint_parent_xform[j][:3] mjc_eq_to_newton_jnt[eq.id] = j eq = spec.add_equality(objtype=mujoco.mjtObj.mjOBJ_BODY) eq.type = mujoco.mjtEq.mjEQ_CONNECT eq.active = True - eq.name1 = model.body_key[joint_child[j]] - eq.name2 = model.body_key[joint_parent[j]] + eq.name1 = get_body_name(joint_child[j]) + eq.name2 = get_body_name(joint_parent[j]) eq.data[0:3] = joint_child_xform[j][:3] mjc_eq_to_newton_jnt[eq.id] = j
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
newton/_src/solvers/mujoco/solver_mujoco.py
🧰 Additional context used
🧠 Learnings (3)
📚 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
⏰ 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 (ubuntu-latest)
- GitHub Check: run-newton-tests / newton-unittests (windows-latest)
🔇 Additional comments (3)
newton/_src/solvers/mujoco/solver_mujoco.py (3)
1688-1692: LGTM! Clean helper for world body name resolution.The helper correctly handles the special case of the world body (index -1) by returning "world" instead of attempting to access
model.body_key[-1], which would cause incorrect behavior. The implementation is straightforward and well-documented.
1700-1701: Correct usage of the helper for CONNECT constraints.Properly uses
get_body_nameto handle both body references, ensuring that world body (-1) is correctly resolved to "world" string for MuJoCo constraint creation.
1720-1721: Correct usage of the helper for WELD constraints.Consistent with the CONNECT constraint handling above, properly resolving body names including world body references.
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
3 participants
Description
Newton Migration Guide
Please ensure the migration guide for warp.sim users is up-to-date with the changes made in this PR.
docs/migration.rstis up-to dateBefore your PR is "Ready for review"
newton/tests/test_examples.py)pre-commit run -aSummary by CodeRabbit
Bug Fixes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.