newton/_src/sim/builder.py (1)

3205-3258: Equality-constraint remap and frame transforms are consistent; consider adding coverage for anchors/relpose

The post-collapse remap of equality constraints looks logically sound:

• Bodies are mapped either directly via body_remap or via body_merged_parent (including merges into world via -1), so no body index is left dangling.
• For weld/connect constraints, the anchor and relpose updates implement the expected frame change:
  • If body1 is merged: anchor' = T1 * anchor, relpose' = T1 * relpose.
  • If body2 is merged (weld): relpose' = relpose * inv(T2).
  • Together this matches relpose' = T1 * relpose * T2^{-1} when both sides are merged.
• Joint indices are remapped via joint_remap, and constraints referencing removed joints are safely disabled via equality_constraint_enabled.

Given the complexity, it would be worthwhile to extend tests to cover non-zero anchors and non-identity weld relpose (e.g., rotated/translated bodies) to validate that the transformed anchor/relpose behave as expected after collapsing.

newton/tests/test_equality_constraints.py (1)

210-332: Good coverage of index remap; you may want to also validate anchor/relpose transforms

This test nicely exercises collapse_fixed_joints with connect, joint, and weld equality constraints, and verifies that body/joint indices and counts are remapped as expected both on the builder and the finalized model.

To fully cover the new remap logic in collapse_fixed_joints, consider extending this test (or adding a sibling test) to:

• Use a non-zero weld anchor and a non-identity relpose between link2 and link3, and
• Assert that the post-collapse equality_constraint_anchor and equality_constraint_relpose match the analytically expected transforms after link2 is merged into link1.

That would catch any future regressions in the frame-change math, not just index remapping.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

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

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: eric-heiden
Repo: newton-physics/newton PR: 587
File: newton/_src/sim/builder.py:656-661
Timestamp: 2025-08-20T18:02:36.099Z
Learning: In Newton physics, collisions between shapes with body index -1 (world-attached/global shapes) are automatically skipped by the collision system, so no manual collision filter pairs need to be added between global shapes from different builders.

newton/tests/test_equality_constraints.py (1)

210-331: New collapse/equality test is well-structured; consider tightening assertions and removing prints

The scenario (fixed–revolute–fixed–revolute chain plus connect/joint/weld constraints) is a good minimal repro for the new collapse logic and validates remapping of both body and joint indices, as well as post-finalize counts.

Two optional refinements:

• Add explicit checks that the three constraints remain enabled after collapse, e.g. self.assertTrue(builder.equality_constraint_enabled[eq_connect]) (similarly for eq_joint and eq_weld) to guard against future changes that might silently disable them while still keeping the count at 3.
• Drop the print(...) at lines 331 and nearby or convert them to comments; they add noise to test output without affecting coverage.

newton/_src/sim/builder.py (1)

2967-3005: Equality-referenced bodies blocking world merges looks correct; optional tweak re disabled constraints

The bodies_in_constraints set and should_skip_merge condition:

bodies_in_constraints = set()
for i in range(len(self.equality_constraint_body1)):
    ...
should_skip_merge = child_body in bodies_in_constraints and last_dynamic_body == -1

correctly implement “don’t collapse a fixed joint into world when its child body is mentioned by any equality constraint”, matching the PR intent and the new test.

One optional consideration: currently even constraints with equality_constraint_enabled[i] == False still prevent collapse into world, since you don’t filter on the enabled flag when populating bodies_in_constraints. That’s a safe, conservative choice; if the intent is that disabled constraints should be completely ignored for topology decisions, you could restrict the set to enabled constraints only.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

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

Learnt from: eric-heiden
Repo: newton-physics/newton PR: 587
File: newton/_src/sim/builder.py:656-661
Timestamp: 2025-08-20T18:02:36.099Z
Learning: In Newton physics, collisions between shapes with body index -1 (world-attached/global shapes) are automatically skipped by the collision system, so no manual collision filter pairs need to be added between global shapes from different builders.

newton/tests/test_equality_constraints.py (1)

210-331: Good targeted regression test; consider also validating anchors/relpose.

The scenario and assertions around body/joint remapping after collapse_fixed_joints() look solid and exercise the new equality-constraint handling. To fully cover the new logic, you might also assert that the CONNECT/WELD anchors and the WELD relpose are transformed consistently with the expected merge transforms (especially for the link2→link1 weld), since that code is subtle and easy to regress.

newton/_src/sim/builder.py (1)

2967-3005: Skip‑merge heuristic for constrained bodies looks reasonable; consider its scope.

Using bodies_in_constraints to prevent collapsing a constrained child body directly into world (last_dynamic_body == -1) makes sense and matches the goal of preserving bodies that appear explicitly in equality constraints. A couple of points to keep in mind:

• This logic currently ignores equality_constraint_enabled; even disabled constraints will block world merges. That’s conservative but may be more restrictive than needed if users programmatically disable constraints before collapsing.
• The heuristic is intentionally narrow: constrained bodies can still be merged into another dynamic body, relying on the post‑pass remap to keep constraints consistent. That matches the new tests, but it’d be good to double‑check that this is the intended behavior for all constraint types (CONNECT/WELD/JOINT) and that no callers rely on constrained bodies staying as separate links in those cases.

If that behavior is intentional, a short comment near should_skip_merge clarifying “only protect from merging into world; merges into other dynamics are remapped later” would make the design intent explicit.

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

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.