Skip to content

[Draft] Cable joint to support minimal eval fk, ik - #3200

Draft
jumyungc wants to merge 16 commits into
newton-physics:mainfrom
jumyungc:vbd-evalfk
Draft

[Draft] Cable joint to support minimal eval fk, ik#3200
jumyungc wants to merge 16 commits into
newton-physics:mainfrom
jumyungc:vbd-evalfk

Conversation

@jumyungc

@jumyungc jumyungc commented Jun 17, 2026

Copy link
Copy Markdown
Member

Description

This PR gives JointType.CABLE a standard kinematic state layout:

  • joint_q: 7-coordinate relative pose
  • joint_qd: 6-DoF relative twist

Cable joints now use the existing relative-pose FK, IK, and Jacobian paths, making them easier to consume from general kinematic and reduced-coordinate
workflows.

Details

  • Initialize Cable coordinates from the authored parent/child poses and joint anchors.
  • Represent the tangent with three linear and three angular axes.
  • Preserve the scalar stretch and bend material API across those axis blocks.
  • Keep existing VBD Cable dynamics and its two internal stretch/bend constraint slots unchanged.
  • Preserve the current inverse-dynamics rejection for Cable joints.
  • Update documentation and tests for the q7/qd6 layout.

SolverVBD continues to advance maximal body_q/body_qd state. When joint state is needed after VBD stepping, eval_ik synchronizes it before eval_fk or other
reduced-coordinate consumers use it.

Checklist

  • New or existing tests cover these changes
  • The documentation is up to date with these changes
  • CHANGELOG.md has been updated (if user-facing change)

Test plan

Bug fix

Steps to reproduce:

Minimal reproduction:

import newton

# Code that demonstrates the bug

New feature / API change

import newton

# Code that demonstrates the new capability

Summary by CodeRabbit

Summary

  • New Features

    • Cable joints now participate in forward kinematics, inverse kinematics, and 6-column Jacobians using a 7D relative anchor pose (joint_q) and 6-DoF relative twist (joint_qd).
  • Bug Fixes

    • Corrected cable motion/Jacobian conventions and rebuilt cable FK body pose/velocity state consistently.
    • Improved cable stiffness/damping and target layout handling.
  • Tests

    • Expanded cable FK/IK/Jacobian and IK→FK synchronization coverage, plus added an inverse-dynamics force check for a rotated cable root.
  • Documentation

    • Updated cable joint semantics and clarified inverse-dynamics support limitations.

Signed-off-by: JC <jumyungc@nvidia.com>
@jumyungc
jumyungc marked this pull request as draft June 17, 2026 10:48
@coderabbitai

coderabbitai Bot commented Jun 17, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: dcc9090c-d5ce-4dd6-af7a-432e463b6391

📥 Commits

Reviewing files that changed from the base of the PR and between 0a23aa5 and 7e9881e.

📒 Files selected for processing (1)
  • CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (1)
  • CHANGELOG.md

📝 Walkthrough

Walkthrough

JointType.CABLE now uses a seven-coordinate relative anchor pose and six-dimensional relative twist. Builder initialization, FK/IK reconstruction, motion-subspace handling, VBD penalty mapping, tests, and documentation were updated.

Changes

CABLE kinematics and dynamics integration

Layer / File(s) Summary
CABLE coordinate and builder contract
newton/_src/sim/enums.py, newton/_src/sim/builder.py, newton/tests/test_model.py
Defines six velocity DoF and seven position coordinates, initializes quaternion state and relative anchor poses, creates three linear plus three angular DOF configurations, and updates target-coordinate conversion and validation.
CABLE FK, IK, and motion subspace support
newton/_src/sim/articulation.py, newton/tests/test_cable.py
Processes CABLE joints in FK and IK, reconstructs body state from relative joint state, uses the six-dimensional motion subspace, and tests Jacobian and synchronization behavior.
CABLE VBD penalty and force handling
newton/_src/solvers/vbd/solver_vbd.py, newton/_src/sim/articulation.py, newton/tests/test_import_usd_deformable_cable.py, newton/tests/test_inverse_dynamics.py
Validates six-axis cable material entries, maps linear and angular stiffness and damping blocks to solver penalty slots, and extends rotated-parent force accumulation to CABLE.
CABLE semantics and documentation
docs/concepts/articulations.rst, CHANGELOG.md, newton/_src/sim/model.py
Documents the updated CABLE pose and twist representation, FK/IK behavior, and unsupported full inverse dynamics.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant ModelBuilder
  participant JointState
  participant eval_fk
  participant eval_ik
  participant MotionSubspace
  ModelBuilder->>JointState: Initialize CABLE relative pose and twist coordinates
  JointState->>eval_fk: Provide joint_q and joint_qd
  eval_fk->>JointState: Reconstruct cable body transforms and velocities
  JointState->>eval_ik: Provide cable body transforms and velocities
  eval_ik->>JointState: Recover relative pose and twist coordinates
  MotionSubspace->>MotionSubspace: Build six CABLE motion-subspace columns
Loading

Possibly related PRs

Suggested labels: kamino

Suggested reviewers: adenzler-nvidia

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title is concise and clearly describes the main change: adding FK/IK support for cable joints.
Docstring Coverage ✅ Passed Docstring coverage is 84.21% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments (2)
newton/_src/sim/enums.py (1)

169-171: ⚡ Quick win

Trim redundant inline narration in the CABLE branch.

This comment restates what the docstring and assignments already convey. Please shorten or remove it to keep inline comments focused on non-obvious constraints only.

Proposed edit
-            # DOF entries stay as VBD stretch + bend/twist slots, while position
-            # coordinates hold the full relative anchor pose (3 translation + 4
-            # quaternion) so forward kinematics can reconstruct the rod.
+            # Keep CABLE velocity DoFs axis-driven while storing full anchor pose for FK reconstruction.
As per coding guidelines, “Code comments should be brief and only for non-obvious code… not what the code already shows.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@newton/_src/sim/enums.py` around lines 169 - 171, The inline comment spanning
lines 169-171 in the CABLE branch is redundant as it restates information
already conveyed by the docstring and variable assignments above it. Trim this
comment to remove the narration about DOF entries, VBD stretch, bend/twist
slots, and the full relative anchor pose details, keeping only any non-obvious
constraints or implementation notes that are not already apparent from the code
itself. If nothing non-obvious remains, remove the comment entirely.

Source: Coding guidelines

newton/tests/test_cable.py (1)

3555-3563: ⚡ Quick win

Add an explicit assertion that eval_ik preserves CABLE joint_qd.

Line 3559 validates joint_q recovery, but this test does not currently lock in the paired contract that CABLE joint_qd must remain untouched.

Suggested test addition
     # Wipe joint_q so the round-trip can't lean on the builder's seeding.
     state.joint_q.zero_()

+    joint_qd_before = state.joint_qd.numpy().copy()
+
     # eval_ik: recover joint_q (relative anchor pose) from the built body_q.
     newton.eval_ik(model, state, state.joint_q, state.joint_qd)

     jq = state.joint_q.numpy()
     test.assertTrue(np.any(np.abs(jq) > 1.0e-6), msg="eval_ik should populate CABLE joint_q")
+    np.testing.assert_allclose(
+        state.joint_qd.numpy(),
+        joint_qd_before,
+        rtol=0.0,
+        atol=0.0,
+        err_msg="eval_ik should leave CABLE joint_qd unchanged",
+    )
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@newton/tests/test_cable.py` around lines 3555 - 3563, After the eval_ik
function call in the test, capture the current state of joint_qd by converting
it to a numpy array similar to how joint_q is captured, then add an assertion to
verify that joint_qd remains unchanged (all values equal to zero or the original
values) after the eval_ik call, ensuring that eval_ik does not modify joint_qd
as part of the contract for CABLE joint handling.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@newton/_src/sim/enums.py`:
- Around line 169-171: The inline comment spanning lines 169-171 in the CABLE
branch is redundant as it restates information already conveyed by the docstring
and variable assignments above it. Trim this comment to remove the narration
about DOF entries, VBD stretch, bend/twist slots, and the full relative anchor
pose details, keeping only any non-obvious constraints or implementation notes
that are not already apparent from the code itself. If nothing non-obvious
remains, remove the comment entirely.

In `@newton/tests/test_cable.py`:
- Around line 3555-3563: After the eval_ik function call in the test, capture
the current state of joint_qd by converting it to a numpy array similar to how
joint_q is captured, then add an assertion to verify that joint_qd remains
unchanged (all values equal to zero or the original values) after the eval_ik
call, ensuring that eval_ik does not modify joint_qd as part of the contract for
CABLE joint handling.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 5ad934ac-b29b-44ef-9eb8-38fc869d938c

📥 Commits

Reviewing files that changed from the base of the PR and between 3deb7ee and e6f27ae.

📒 Files selected for processing (5)
  • CHANGELOG.md
  • newton/_src/sim/articulation.py
  • newton/_src/sim/builder.py
  • newton/_src/sim/enums.py
  • newton/tests/test_cable.py

Signed-off-by: JC <jumyungc@nvidia.com>
@codecov

codecov Bot commented Jun 17, 2026

Copy link
Copy Markdown

Codecov Report

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

📢 Thoughts on this report? Let us know!

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@newton/_src/sim/articulation.py`:
- Around line 1382-1549: Update eval_inverse_dynamics_force() to reject models
containing JointType.CABLE, matching the existing guard in
Model.inverse_dynamics(). Perform this validation before launching
eval_articulation_inverse_dynamics_force_kernel so CABLE models cannot produce
an invalid tau.

In `@newton/_src/sim/builder.py`:
- Around line 4414-4424: Update the xform remapping logic in
ModelBuilder.add_builder() so world-root CABLE joints are handled alongside FREE
joints. Recompute their 7-DOF joint_q pose using the transformed parent/child
world poses, keeping joint_q consistent with joint_X_p for add_world() and
replicate() copies.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 9f4dc073-6b63-4141-8d96-49b7e543fccc

📥 Commits

Reviewing files that changed from the base of the PR and between 20af5ab and d731ed7.

📒 Files selected for processing (7)
  • CHANGELOG.md
  • docs/concepts/articulations.rst
  • newton/_src/sim/articulation.py
  • newton/_src/sim/builder.py
  • newton/_src/sim/enums.py
  • newton/_src/sim/model.py
  • newton/_src/solvers/vbd/solver_vbd.py
🚧 Files skipped from review as they are similar to previous changes (3)
  • docs/concepts/articulations.rst
  • newton/_src/sim/enums.py
  • CHANGELOG.md

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Caution

Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.

Actionable comments posted: 2

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@newton/_src/sim/articulation.py`:
- Around line 1382-1549: Update eval_inverse_dynamics_force() to reject models
containing JointType.CABLE, matching the existing guard in
Model.inverse_dynamics(). Perform this validation before launching
eval_articulation_inverse_dynamics_force_kernel so CABLE models cannot produce
an invalid tau.

In `@newton/_src/sim/builder.py`:
- Around line 4414-4424: Update the xform remapping logic in
ModelBuilder.add_builder() so world-root CABLE joints are handled alongside FREE
joints. Recompute their 7-DOF joint_q pose using the transformed parent/child
world poses, keeping joint_q consistent with joint_X_p for add_world() and
replicate() copies.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 9f4dc073-6b63-4141-8d96-49b7e543fccc

📥 Commits

Reviewing files that changed from the base of the PR and between 20af5ab and d731ed7.

📒 Files selected for processing (7)
  • CHANGELOG.md
  • docs/concepts/articulations.rst
  • newton/_src/sim/articulation.py
  • newton/_src/sim/builder.py
  • newton/_src/sim/enums.py
  • newton/_src/sim/model.py
  • newton/_src/solvers/vbd/solver_vbd.py
🚧 Files skipped from review as they are similar to previous changes (3)
  • docs/concepts/articulations.rst
  • newton/_src/sim/enums.py
  • CHANGELOG.md
🛑 Comments failed to post (2)
newton/_src/sim/articulation.py (1)

1382-1549: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Map relevant symbols and locations.
rg -n "CABLE|inverse_dynamics_force|eval_inverse_dynamics_force|inverse_dynamics\(" newton/_src/sim/articulation.py

# Show surrounding context for the relevant sections, capped to reasonable line ranges.
nl -ba newton/_src/sim/articulation.py | sed -n '1,260p'
nl -ba newton/_src/sim/articulation.py | sed -n '1320,1605p'

Repository: newton-physics/newton

Length of output: 1123


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect any model/container-level guards for CABLE joints.
rg -n "CABLE|container|guard|reject|unsupported" newton/_src/sim -g '*.py'

# Read the inverse_dynamics path in the same file if present.
nl -ba newton/_src/sim/articulation.py | sed -n '900,1180p'

Repository: newton-physics/newton

Length of output: 6321


🏁 Script executed:

#!/bin/bash
set -euo pipefail

rg -n "CABLE|inverse_dynamics_force|eval_inverse_dynamics_force|inverse_dynamics\(" newton/_src/sim/articulation.py
nl -ba newton/_src/sim/articulation.py | sed -n '1320,1605p'
nl -ba newton/_src/sim/articulation.py | sed -n '900,1180p'

Repository: newton-physics/newton

Length of output: 1123


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the model guard and call sites for eval_inverse_dynamics_force.
sed -n '2178,2218p' newton/_src/sim/model.py
printf '\n---\n'
rg -n "eval_inverse_dynamics_force\(" newton/_src/sim -g '*.py'

Repository: newton-physics/newton

Length of output: 2196


🏁 Script executed:

#!/bin/bash
set -euo pipefail

rg -n "eval_inverse_dynamics_force" newton -g '*.py'
printf '\n--- exports ---\n'
sed -n '1,220p' newton/__init__.py

Repository: newton-physics/newton

Length of output: 6478


Guard eval_inverse_dynamics_force() against CABLE models. Model.inverse_dynamics() blocks CABLE, but this exported helper can still be called directly on a CABLE model and will leave the 6-DOF root wrench in the parent frame, producing a wrong tau. Add the same JointType.CABLE check here.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@newton/_src/sim/articulation.py` around lines 1382 - 1549, Update
eval_inverse_dynamics_force() to reject models containing JointType.CABLE,
matching the existing guard in Model.inverse_dynamics(). Perform this validation
before launching eval_articulation_inverse_dynamics_force_kernel so CABLE models
cannot produce an invalid tau.
newton/_src/sim/builder.py (1)

4414-4424: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Inspect the relevant builder and articulation code paths.
git grep -n "JointType.CABLE\|JointType.FREE\|add_builder\|replicate\|add_world\|joint_q_start\|joint_X_p" -- newton/_src/sim/builder.py newton/_src/sim

# Show the surrounding code for the reported line range and the xform-offset branch.
sed -n '4388,4445p' newton/_src/sim/builder.py
sed -n '3535,3605p' newton/_src/sim/builder.py

# Find cable-related helpers/tests to see how world-anchored cable joints are used.
git grep -n "add_joint_cable\|cable" -- newton tests

Repository: newton-physics/newton

Length of output: 50378


🏁 Script executed:

#!/bin/bash
set -euo pipefail

# Read only the relevant slices around add_builder/add_joint_cable and the FK docs.
sed -n '3450,3615p' newton/_src/sim/builder.py
sed -n '4947,5175p' newton/_src/sim/builder.py
sed -n '4800,4845p' newton/_src/sim/builder.py
sed -n '500,530p' newton/_src/sim/articulation.py

# Check whether any tests or helpers explicitly cover world-anchored cable replication.
git grep -n "add_joint_cable(parent=-1\|replicate(.*cable\|add_world(.*cable\|world-anchored cable\|CABLE" -- tests newton/_src | head -n 200

Repository: newton-physics/newton

Length of output: 28101


Extend the xform remap to world-root CABLE joints in ModelBuilder.add_builder()

CABLE joints use the same 7-DOF joint_q pose layout as FREE, so copying a world-root cable with a non-identity xform leaves joint_q stale while joint_X_p is offset. That breaks eval_fk for add_world()/replicate() copies. Extend the FREE branch to cover CABLE as well, or document that world-root cables cannot be replicated with transforms.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@newton/_src/sim/builder.py` around lines 4414 - 4424, Update the xform
remapping logic in ModelBuilder.add_builder() so world-root CABLE joints are
handled alongside FREE joints. Recompute their 7-DOF joint_q pose using the
transformed parent/child world poses, keeping joint_q consistent with joint_X_p
for add_world() and replicate() copies.

Signed-off-by: JC <jumyungc@nvidia.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🧹 Nitpick comments (2)
newton/_src/solvers/vbd/solver_vbd.py (1)

1307-1312: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Use prefix-first names for the stiffness locals.

Rename ke_stretch and ke_bend to stretch_ke and bend_ke so the semantic qualifier comes first.

As per coding guidelines, use prefix-first naming for autocomplete.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@newton/_src/solvers/vbd/solver_vbd.py` around lines 1307 - 1312, Rename the
local stiffness variables in the joint initialization block from ke_stretch and
ke_bend to stretch_ke and bend_ke, and update all references in the assignments
to joint_k_max_np and joint_k_init_np accordingly.

Source: Coding guidelines

newton/tests/test_import_usd_deformable_cable.py (1)

242-246: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Assert the complete six-DOF material block.

This test checks only the first linear and angular entries, so incorrect values in the other four entries would go unnoticed. Assert all three linear and all three angular entries to match the documented replication contract.

Proposed test adjustment
-            self.assertAlmostEqual(ke[dof0], expected_stretch, delta=expected_stretch * 1e-3)
-            self.assertAlmostEqual(ke[dof0 + 3], expected_bend, delta=expected_bend * 1e-3)
+            for i in range(3):
+                self.assertAlmostEqual(ke[dof0 + i], expected_stretch, delta=expected_stretch * 1e-3)
+                self.assertAlmostEqual(ke[dof0 + 3 + i], expected_bend, delta=expected_bend * 1e-3)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@newton/tests/test_import_usd_deformable_cable.py` around lines 242 - 246,
Extend the assertions in the material-check section of the cable test around
builder.joint_target_ke so all six entries for j0 are validated: the three
linear DOFs must match expected_stretch and the three angular DOFs must match
expected_bend, using the existing tolerance convention.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@newton/_src/solvers/vbd/solver_vbd.py`:
- Around line 1307-1312: Rename the local stiffness variables in the joint
initialization block from ke_stretch and ke_bend to stretch_ke and bend_ke, and
update all references in the assignments to joint_k_max_np and joint_k_init_np
accordingly.

In `@newton/tests/test_import_usd_deformable_cable.py`:
- Around line 242-246: Extend the assertions in the material-check section of
the cable test around builder.joint_target_ke so all six entries for j0 are
validated: the three linear DOFs must match expected_stretch and the three
angular DOFs must match expected_bend, using the existing tolerance convention.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 1c2a9b11-e5e3-436b-b46c-7ae1b61315bf

📥 Commits

Reviewing files that changed from the base of the PR and between d731ed7 and b5ddea2.

📒 Files selected for processing (10)
  • CHANGELOG.md
  • docs/concepts/articulations.rst
  • newton/_src/sim/articulation.py
  • newton/_src/sim/builder.py
  • newton/_src/sim/enums.py
  • newton/_src/sim/model.py
  • newton/_src/solvers/vbd/solver_vbd.py
  • newton/tests/test_cable.py
  • newton/tests/test_import_usd_deformable_cable.py
  • newton/tests/test_inverse_dynamics.py
🚧 Files skipped from review as they are similar to previous changes (2)
  • newton/_src/sim/enums.py
  • newton/_src/sim/model.py

Signed-off-by: JC <jumyungc@nvidia.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
newton/_src/sim/builder.py (1)

4380-4410: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

CABLE target_pos values for linear/angular axes are silently dropped.

For BALL/FREE/DISTANCE, add_joint copies each axis's dim.target_pos into joint_target_q (linear axes directly, angular axes either via quaternion composition or raw copy) before setting the quat-w slot. The new elif joint_type == JointType.CABLE: branch only sets the quat-w padding to 1.0 and never writes any of the 6 linear/angular dim.target_pos values into joint_target_q. Any caller using the generic add_joint(JointType.CABLE, ...) API with non-zero target_pos on the linear/angular axes will have those targets silently discarded instead of raising an error or being honored, unlike every other multi-axis joint type. add_joint_cable itself never sets target_pos on the generated JointDofConfigs, so this doesn't manifest through that helper today, but it's a latent correctness gap in the general add_joint contract for CABLE.

🐛 Proposed fix: fold CABLE into the FREE/DISTANCE branch
-        if joint_type == JointType.BALL or joint_type == JointType.FREE or joint_type == JointType.DISTANCE:
+        if (
+            joint_type == JointType.BALL
+            or joint_type == JointType.FREE
+            or joint_type == JointType.DISTANCE
+            or joint_type == JointType.CABLE
+        ):
             if joint_type == JointType.BALL:
                 quat_offset = target_q_offset
             else:
                 for i, dim in enumerate(linear_axes):
                     self.joint_target_q[target_q_offset + i] = dim.target_pos
                 quat_offset = target_q_offset + 3
 
             import newton  # noqa: PLC0415
 
             if newton.use_coord_layout_targets:
                 qx, qy, qz, qw = self._quat_from_axis_targets(
                     angular_axes[0].target_pos,
                     angular_axes[1].target_pos,
                     angular_axes[2].target_pos,
                 )
                 self.joint_target_q[quat_offset + 0] = qx
                 self.joint_target_q[quat_offset + 1] = qy
                 self.joint_target_q[quat_offset + 2] = qz
                 self.joint_target_q[quat_offset + 3] = qw
             else:
                 for i, dim in enumerate(angular_axes):
                     self.joint_target_q[quat_offset + i] = dim.target_pos
                 self.joint_target_q[quat_offset + 3] = 1.0
-        elif joint_type == JointType.CABLE:
-            self.joint_target_q[target_q_offset + 6] = 1.0
         elif joint_type != JointType.FIXED:
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@newton/_src/sim/builder.py` around lines 4380 - 4410, Update the add_joint
branch handling JointType.CABLE so it follows the same target_pos copying
behavior as the FREE/DISTANCE multi-axis path: write all linear and angular axis
target_pos values into joint_target_q, then set the quaternion padding component
to 1.0. Preserve the existing BALL quaternion handling and CABLE target layout.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@newton/_src/sim/builder.py`:
- Around line 4380-4410: Update the add_joint branch handling JointType.CABLE so
it follows the same target_pos copying behavior as the FREE/DISTANCE multi-axis
path: write all linear and angular axis target_pos values into joint_target_q,
then set the quaternion padding component to 1.0. Preserve the existing BALL
quaternion handling and CABLE target layout.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 412640d4-49d0-4bff-9895-53de82f3753c

📥 Commits

Reviewing files that changed from the base of the PR and between b5ddea2 and b44940e.

📒 Files selected for processing (6)
  • CHANGELOG.md
  • docs/concepts/articulations.rst
  • newton/_src/sim/builder.py
  • newton/_src/sim/enums.py
  • newton/_src/solvers/vbd/solver_vbd.py
  • newton/tests/test_cable.py
🚧 Files skipped from review as they are similar to previous changes (4)
  • CHANGELOG.md
  • docs/concepts/articulations.rst
  • newton/_src/sim/enums.py
  • newton/_src/solvers/vbd/solver_vbd.py

Signed-off-by: JC <jumyungc@nvidia.com>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
newton/tests/test_model.py (1)

1663-1688: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win

Align the docstring with ModelBuilder._quat_from_axis_targets

newton/tests/test_model.py:1666-1688 still references :meth:\ModelBuilder._quat_from_euler_zyx`, but the test and builder use ModelBuilder._quat_from_axis_targets`. Update the docstring so it matches the helper the assertion actually exercises.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@newton/tests/test_model.py` around lines 1663 - 1688, Update the test
docstring describing the Flag=True coordinate conversion to reference
ModelBuilder._quat_from_axis_targets instead of
ModelBuilder._quat_from_euler_zyx, matching the helper used by expected_quat and
the exercised builder behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@newton/tests/test_model.py`:
- Around line 1663-1688: Update the test docstring describing the Flag=True
coordinate conversion to reference ModelBuilder._quat_from_axis_targets instead
of ModelBuilder._quat_from_euler_zyx, matching the helper used by expected_quat
and the exercised builder behavior.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yml

Review profile: CHILL

Plan: Pro

Run ID: 63790f77-9392-46aa-9dd8-4e883912f7b5

📥 Commits

Reviewing files that changed from the base of the PR and between b44940e and 834493b.

📒 Files selected for processing (6)
  • newton/_src/sim/articulation.py
  • newton/_src/sim/builder.py
  • newton/_src/solvers/vbd/solver_vbd.py
  • newton/tests/test_import_usd_deformable_cable.py
  • newton/tests/test_inverse_dynamics.py
  • newton/tests/test_model.py
🚧 Files skipped from review as they are similar to previous changes (3)
  • newton/tests/test_import_usd_deformable_cable.py
  • newton/_src/solvers/vbd/solver_vbd.py
  • newton/_src/sim/builder.py

@jumyungc jumyungc self-assigned this Jul 28, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant