Switch to velocity-space ik to support all joint types#450
Conversation
📝 WalkthroughWalkthroughThe changes refactor the inverse kinematics (IK) system to consistently use joint degrees of freedom (DoFs) instead of joint coordinates for all solver, objective, and kernel computations. The interactive IK example is updated to operate in world coordinates, adjust joint indexing, and streamline solver invocation. New and expanded tests validate IK convergence for models with free and revolute joints. Additionally, a benchmark example disables quaternion error canonicalization in rotation objectives. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Example (interactive IK)
participant IKSolver
participant Model
User->>Example: Interact with gizmo (drag target)
Example->>Example: Update target position (world coordinates)
Example->>IKSolver: Call solve() with current joint_q and targets
IKSolver->>Model: Evaluate FK with joint DoFs
IKSolver->>IKSolver: Compute residuals/Jacobian (DoF-based)
IKSolver->>IKSolver: Integrate dq_dof into joint_q
IKSolver-->>Example: Return new joint_q
Example-->>User: Update robot pose in UI
Estimated code review effort🎯 4 (Complex) | ⏱️ ~90 minutes Possibly related PRs
Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: .coderabbit.yml 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (2)📓 Common learnings📚 Learning: in newton's ik system, the jointlimitobjective class intentionally only handles joints where dof cou...Applied to files:
⏰ 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)
🔇 Additional comments (7)
✨ Finishing Touches
🧪 Generate 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
newton/tests/test_ik.py (2)
65-68: Section header formatting consistencyConsider using consistent formatting for section headers. The other section uses a simple comment style (line 29), while this uses a more elaborate box style.
-# ---------------------------------------------------------------------------- -# helpers - FREE-REV -# ---------------------------------------------------------------------------- - +# ---------------------------------------------------------------------------- +# helpers: free-revolute baseline +# ----------------------------------------------------------------------------
220-220: Consider making iteration count configurableThe free joint convergence test uses 60 iterations while the planar test uses 40. Consider making this a parameter or adding a comment explaining why free joints need more iterations.
Add a comment explaining the iteration count difference:
+ # Free joints have more DOFs and typically require more iterations to converge solver.solve(iterations=60)newton/examples/example_ik_interactive.py (1)
217-218: Document the purpose of epsilon offsetAdding 1e-3 to all joint angles needs explanation. Is this for numerical stability or to avoid singularities?
+ # Add small epsilon to avoid numerical issues at joint limits or singularities for i in range(len(articulation_builder.joint_q)): articulation_builder.joint_q[i] += 1e-3newton/sim/ik.py (2)
356-356: Redundant zero operationThe
qd_zeroarray is already initialized to zeros and passed as zero velocities. Zeroing it again after integration seems unnecessary.device=self.device, ) - self.qd_zero.zero_()
1177-1179: Magic number for joint limit range checkThe threshold
9.9e5for treating joint ranges as unlimited should be a named constant.+ UNLIMITED_JOINT_RANGE = 9.9e5 # Threshold for treating joint limits as unlimited + # treat huge ranges as no limit - if upper - lower > 9.9e5: + if upper - lower > UNLIMITED_JOINT_RANGE: return
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 6fc6074 and 5b3640bd89c24da3bdee6e8ecfb253c01f5edda2.
📒 Files selected for processing (3)
newton/examples/example_ik_interactive.py(12 hunks)newton/sim/ik.py(26 hunks)newton/tests/test_ik.py(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
newton/sim/ik.py (3)
newton/sim/state.py (1)
joint_dof_count(105-109)newton/sim/builder.py (1)
joint_count(442-443)newton/solvers/featherstone/kernels.py (2)
jcalc_integrate(406-537)jcalc_motion(228-327)
⏰ 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 Unit Tests on AWS EC2 (Pull Request)
🔇 Additional comments (6)
newton/tests/test_ik.py (1)
354-363: Test organization looks goodThe test registration is well-organized with clear sections for planar convergence, free-joint convergence, and Jacobian equality tests.
newton/examples/example_ik_interactive.py (2)
118-118: Significant weight increase for joint limitsThe joint limit objective weight increased from 0.1 to 1.0 (10x increase). This could significantly affect the solver behavior and convergence. Please verify this is intentional.
474-474: _solve signature verified – no step_size parameter
I confirmed that_solve(self)innewton/examples/example_ik_interactive.pyis defined without any parameters. Removingstep_sizefrom its invocation is therefore correct—no changes are needed.Likely an incorrect or invalid review comment.
newton/sim/ik.py (3)
492-495: Excellent documentation for the integration kernelThe docstring clearly explains the velocity-space integration approach and its universal support for all joint types.
529-544: Clever use of jcalc_integrate for velocity-space updatesThe implementation correctly treats the DOF update vector as acceleration with dt=1, effectively implementing velocity-space integration. This is an elegant solution that reuses existing Featherstone integration logic.
979-993: Efficient analytic Jacobian computationThe analytic Jacobian computation correctly uses motion subspace columns and properly handles world-space transformations for position objectives. The use of DOF masking with
affects_dofis an efficient optimization.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
newton/sim/ik.py (1)
1286-1296: LGTM! The DOF-to-coordinate mapping is appropriately scoped.Based on the retrieved learning, this 1:1 mapping is intentionally limited to joints where it makes sense (revolute, prismatic). Complex joints like ball/free joints with quaternion representations aren't expected to use joint limits.
🧹 Nitpick comments (3)
newton/tests/test_ik.py (1)
191-229: Consider adjusting the convergence threshold for free joints.The convergence test for free joints uses a less strict error threshold (1e-3) compared to the planar test (1e-4), and requires more iterations (60 vs 40). While this may be appropriate given the increased complexity of free joints, consider documenting why different thresholds are necessary.
Add a comment explaining the rationale:
- solver.solve(iterations=60) + # Free joints require more iterations and have a higher error tolerance + # due to their 6 DOF complexity (3 translation + 3 rotation) + solver.solve(iterations=60)newton/examples/example_ik_interactive.py (1)
222-224: Document the purpose of the epsilon addition.Adding 1e-3 to all joint angles for floating base models needs clarification. This could be to avoid singularities or numerical issues.
if floating: + # Add small epsilon to avoid potential singularities in quaternion representation for i in range(len(articulation_builder.joint_q)): articulation_builder.joint_q[i] += 1e-3newton/sim/ik.py (1)
1154-1183: Verify joint limit behavior for large ranges.The kernel treats ranges larger than 9.9e5 as "no limit", which seems like a magic number. Consider documenting this threshold or making it configurable.
Consider defining this as a constant:
+# Joint limit range threshold - ranges larger than this are treated as unbounded +JOINT_LIMIT_UNBOUNDED_THRESHOLD = wp.constant(1e6) + @wp.kernel def _limit_residuals( # ... parameters ... ): # ... code ... - # treat huge ranges as no limit - if upper - lower > 9.9e5: + # treat huge ranges as no limit + if upper - lower > JOINT_LIMIT_UNBOUNDED_THRESHOLD: return
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 5b3640bd89c24da3bdee6e8ecfb253c01f5edda2 and 878bbaf1108221d051bf57018412441bf17adba1.
📒 Files selected for processing (3)
newton/examples/example_ik_interactive.py(14 hunks)newton/sim/ik.py(26 hunks)newton/tests/test_ik.py(5 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: dylanturpin
PR: newton-physics/newton#450
File: newton/sim/ik.py:1286-1296
Timestamp: 2025-07-21T19:11:04.055Z
Learning: In Newton's IK system, the JointLimitObjective class intentionally only handles joints where DOF count equals coordinate count (1-1 mapping). This is appropriate because joint limits are typically only meaningful for simple joint types like revolute and prismatic joints, not complex joints like ball or free joints where DOF count != coordinate count.
newton/examples/example_ik_interactive.py (2)
Learnt from: Kenny-Vilella
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.
Learnt from: dylanturpin
PR: #450
File: newton/sim/ik.py:1286-1296
Timestamp: 2025-07-21T19:11:04.055Z
Learning: In Newton's IK system, the JointLimitObjective class intentionally only handles joints where DOF count equals coordinate count (1-1 mapping). This is appropriate because joint limits are typically only meaningful for simple joint types like revolute and prismatic joints, not complex joints like ball or free joints where DOF count != coordinate count.
newton/sim/ik.py (1)
Learnt from: dylanturpin
PR: #450
File: newton/sim/ik.py:1286-1296
Timestamp: 2025-07-21T19:11:04.055Z
Learning: In Newton's IK system, the JointLimitObjective class intentionally only handles joints where DOF count equals coordinate count (1-1 mapping). This is appropriate because joint limits are typically only meaningful for simple joint types like revolute and prismatic joints, not complex joints like ball or free joints where DOF count != coordinate count.
🧬 Code Graph Analysis (1)
newton/sim/ik.py (3)
newton/sim/state.py (1)
joint_dof_count(105-109)newton/sim/builder.py (1)
joint_count(442-443)newton/solvers/featherstone/kernels.py (2)
jcalc_integrate(406-537)jcalc_motion(228-327)
⏰ 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 Unit Tests on AWS EC2 (Pull Request)
🔇 Additional comments (12)
newton/tests/test_ik.py (2)
29-30: LGTM!The comment clearly distinguishes the planar 2-revolute baseline helper functions from the new free joint helpers.
65-102: LGTM!The helper function correctly builds a model with a free joint followed by a revolute joint, which is essential for testing the velocity-space IK solver with complex joint types.
newton/examples/example_ik_interactive.py (4)
91-96: LGTM!The variable renaming from
num_links/num_coordstobody_count/num_dofscorrectly aligns with the IK solver's transition to DOF-based indexing.
208-224: LGTM! The joint offset adjustment is correct.The conditional joint index offset (
joint_q_offset = 0 if floating else -7) properly accounts for the 7 additional coordinates (3 position + 4 quaternion) present in floating base models.
269-273: LGTM! The coordinate system handling is well-designed.The conditional logic correctly handles world coordinates for floating base models and local coordinates for fixed base models, maintaining consistency with the solver's expectations.
103-103: Initial pose copy for IK example is intentional
Copyingmodel.joint_qinto the solver’s initial guess ensures the IK routine starts from the URDF/MJCF–imported default configuration rather than an all-zero pose. This provides a more natural seed for interactive use. The test suite still explicitly zeroes joint arrays where needed, so no further changes are required.newton/sim/ik.py (6)
78-79: LGTM!The tile dimensions correctly use DOFs instead of coordinates, aligning with the velocity-space approach.
139-139: LGTM!The zero velocity array
qd_zerois appropriately initialized for FK evaluation when actual velocities aren't needed.
335-356: LGTM! The velocity-space integration is well-implemented.The
_integrate_dqmethod correctly:
- Launches the integration kernel for all joints across all problems
- Passes the velocity update
dq_dofas the integration delta- Zeros out the velocity array after integration to maintain consistency
479-544: Excellent implementation of velocity-space integration!The
_integrate_dq_dofkernel is well-designed:
- Supports all joint types through
jcalc_integrate- Correctly interprets
dq_dofas a velocity update withdtintegration- Proper thread indexing for parallel problem solving
- Clear documentation of the integration approach
956-993: LGTM! The analytic position Jacobian correctly uses DOF indexing.The kernel properly:
- Iterates over DOFs instead of coordinates
- Uses the
affects_dofmask to skip irrelevant DOFs- Extracts motion subspace columns for velocity computation
- Writes Jacobian entries at the correct DOF indices
1435-1461: LGTM! The rotation Jacobian correctly handles DOF-based computation.The analytic rotation Jacobian kernel properly:
- Uses DOF indexing throughout
- Applies the
affects_dofmask to skip irrelevant DOFs- Extracts only the angular velocity components from motion subspace
- Correctly maps to Jacobian DOF columns
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
newton/sim/ik.py (1)
1286-1296: Appropriate DOF-to-coordinate mapping for joint limits.The mapping correctly handles only joints where DOF count equals coordinate count, which aligns with the design intent that joint limits are meaningful only for simple joint types like revolute and prismatic joints.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between 5b3640bd89c24da3bdee6e8ecfb253c01f5edda2 and 878bbaf1108221d051bf57018412441bf17adba1.
📒 Files selected for processing (3)
newton/examples/example_ik_interactive.py(14 hunks)newton/sim/ik.py(26 hunks)newton/tests/test_ik.py(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- newton/tests/test_ik.py
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: dylanturpin
PR: newton-physics/newton#450
File: newton/sim/ik.py:1286-1296
Timestamp: 2025-07-21T19:11:04.055Z
Learning: In Newton's IK system, the JointLimitObjective class intentionally only handles joints where DOF count equals coordinate count (1-1 mapping). This is appropriate because joint limits are typically only meaningful for simple joint types like revolute and prismatic joints, not complex joints like ball or free joints where DOF count != coordinate count.
newton/examples/example_ik_interactive.py (1)
Learnt from: dylanturpin
PR: #450
File: newton/sim/ik.py:1286-1296
Timestamp: 2025-07-21T19:11:04.055Z
Learning: In Newton's IK system, the JointLimitObjective class intentionally only handles joints where DOF count equals coordinate count (1-1 mapping). This is appropriate because joint limits are typically only meaningful for simple joint types like revolute and prismatic joints, not complex joints like ball or free joints where DOF count != coordinate count.
newton/sim/ik.py (1)
Learnt from: dylanturpin
PR: #450
File: newton/sim/ik.py:1286-1296
Timestamp: 2025-07-21T19:11:04.055Z
Learning: In Newton's IK system, the JointLimitObjective class intentionally only handles joints where DOF count equals coordinate count (1-1 mapping). This is appropriate because joint limits are typically only meaningful for simple joint types like revolute and prismatic joints, not complex joints like ball or free joints where DOF count != coordinate count.
🧬 Code Graph Analysis (1)
newton/examples/example_ik_interactive.py (3)
newton/sim/model.py (1)
state(362-395)newton/sim/state.py (2)
joint_coord_count(98-102)joint_dof_count(105-109)newton/sim/ik.py (2)
set_target_positions(1079-1086)set_target_position(1070-1077)
⏰ 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 Unit Tests on AWS EC2 (Pull Request)
🔇 Additional comments (9)
newton/examples/example_ik_interactive.py (4)
75-75: LGTM! Clean implementation of floating base support.The addition of the
floatingparameter is well-integrated throughout the initialization, properly propagated to model building, and follows consistent naming conventions.Also applies to: 81-81, 96-96, 99-99
222-224: Verify the necessity of the epsilon offset for floating joints.Adding a small epsilon (1e-3) to all joint angles when floating seems like a workaround. This could potentially mask underlying numerical issues or create unexpected initial poses.
Could you explain why this epsilon offset is necessary for floating base models? Consider documenting this requirement or investigating if there's a more principled approach.
269-273: Well-structured coordinate frame handling.The conditional logic correctly manages target positions based on the floating mode - world coordinates for floating base, local coordinates for fixed base. The conversion is clean and the intent is clear.
398-404: Consistent coordinate transformation in dragging callbacks.The position dragging logic correctly maintains the same coordinate frame convention as initialization - world space for floating base, local space for fixed base.
newton/sim/ik.py (5)
78-78: Clean refactoring to DOF-based dimensions.The consistent replacement of coordinate-based dimensions with DOF-based dimensions throughout the solver initialization enables proper support for all joint types, including those where DOF count ≠ coordinate count (e.g., ball and free joints).
Also applies to: 83-84, 116-117, 139-139, 149-150
479-544: Excellent implementation of velocity-space integration.The new integration kernel properly leverages Featherstone's
jcalc_integrateto handle all joint types. The approach of treatingdq_dofas acceleration withdtis mathematically sound and enables support for quaternion-based joints (ball, free) that were previously unsupported.
313-333: Well-implemented motion subspace computation.The motion subspace computation correctly extracts the spatial motion vectors needed for analytic Jacobian calculations. Using
jcalc_motionensures compatibility with all joint types.Also applies to: 546-594
1029-1030: Consistent DOF-based updates in PositionObjective.The objective correctly updates all lookups and masks to use DOFs instead of coordinates. The
affects_dofmask anddof_to_jointmapping ensure proper Jacobian sparsity.Also applies to: 1033-1059, 1141-1142
1503-1504: Complete DOF-based refactoring in RotationObjective.The rotation objective properly updates all DOF-related computations, maintaining consistency with the solver's new velocity-space approach.
Also applies to: 1509-1513, 1531-1532
|
Hi Eric. Wouldn't be the first time! But I think it's all there. Just changes to...
|
Signed-off-by: Dylan Turpin <dylanturpin@gmail.com>
…s#450) Signed-off-by: Dylan Turpin <dylanturpin@gmail.com> Co-authored-by: Eric Heiden <eheiden@nvidia.com>
…ewton-physics#450) # Description Adds user guide on CloudXR Teleoperation on Kubernetes cluster. ## Type of change - This change is a documentation update ## Screenshots n/a Did a proof read in a local build. ## Checklist - [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with `./isaaclab.sh --format` - [x] I have made corresponding changes to the documentation - [ ] My changes generate no new warnings - [ ] I have added tests that prove my fix is effective or that my feature works - [ ] I have updated the changelog and the corresponding version in the extension's `config/extension.toml` file - [ ] I have added my name to the `CONTRIBUTORS.md` or my name already exists there
…s#450) Signed-off-by: Dylan Turpin <dylanturpin@gmail.com> Co-authored-by: Eric Heiden <eheiden@nvidia.com>
Description
Upgrades Newton’s IK to a velocity-space optimization. Δq is now produced in joint‑velocity space (like
joint_qd) and integrated through Featherstone'sjcalc_integrate(), letting us:Testing:
test_ik.pynow tests convergence for a free joint.Interactive example:
example_ik_interactive.pycan be run with--floatingto include a free joint connecting the base of the h1 robot to the world so the torso can move (as seen below).Newton Migration Guide
Please ensure the migration guide for warp.sim users is up-to-date with the changes made in this MR.
docs/migration.rstis up-to dateBefore your PR is "Ready for review"
newton/tests/test_examples.py)pre-commit run -aSummary by CodeRabbit
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Tests