Add Newton Support for MJCF parsing and actuation of Spatial Tendons #437
Conversation
…ewton-physics#233 Signed-off-by: Viktor Reutskyy <[email protected]>
📝 WalkthroughWalkthroughAdds full support for MJCF spatial tendons: parser updates, Model/Builder extensions for sites/tendons/tendon actuators, Control and Model control initialization for tendon targets, MuJoCo solver mapping and control application, ArticulationView tendon selection, tests, and an example script demonstrating tendon actuation and rendering. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant MJCF_Parser
participant ModelBuilder
participant Model
participant Control
participant MuJoCoSolver
participant Renderer
User->>MJCF_Parser: parse MJCF (sites, tendons, actuators)
MJCF_Parser->>ModelBuilder: add_site / add_tendon / add_tendon_actuator
ModelBuilder->>Model: finalize (populate arrays, counts, tendon_target)
User->>Control: create/set tendon_target
User->>MuJoCoSolver: init solver with Model
loop simulation step
Control->>MuJoCoSolver: provide tendon_target + other controls
MuJoCoSolver->>MuJoCoSolver: apply_mjc_tendon_control_kernel -> write MJ actuator controls
MuJoCoSolver->>Model: advance state
Model->>Renderer: provide state for visualization
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Assessment against linked issues
Suggested reviewers
✨ 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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
Status, Documentation and Community
|
vreutskyy
had a problem deploying
to
external-pr-approval
GitHub Actions
Error
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🔭 Outside diff range comments (2)
newton/sim/control.py (1)
80-91: Add tendon control clearing to theclear()method.The new tendon control attributes (
tendon_fandtendon_target) are not being reset in theclear()method. This could lead to unexpected behavior when clearing control inputs.Apply this diff to include tendon controls in the clearing logic:
def clear(self) -> None: """Reset the control inputs to zero.""" if self.joint_f is not None: self.joint_f.zero_() + if self.joint_target is not None: + self.joint_target.zero_() + if self.tendon_f is not None: + self.tendon_f.zero_() + if self.tendon_target is not None: + self.tendon_target.zero_() if self.tri_activations is not None: self.tri_activations.zero_() if self.tet_activations is not None: self.tet_activations.zero_() if self.muscle_activations is not None: self.muscle_activations.zero_()newton/utils/import_mjcf.py (1)
589-591: Remove debug print statement.This appears to be a debug statement that was accidentally left in the code.
- if body_name == "thigh_right": - print(geoms) visuals = []
🧹 Nitpick comments (13)
newton/utils/import_mjcf.py (2)
810-844: Consider using logging instead of print statements for warnings.The tendon parsing logic is well-implemented, but using
Consider using Python's logging module:
+import logging + +logger = logging.getLogger(__name__) + # In the tendon parsing section: - print(f"Warning: Site '{site_attr}' referenced in tendon '{tendon_name}' not found") + logger.warning(f"Site '{site_attr}' referenced in tendon '{tendon_name}' not found") - print(f"Warning: Tendon '{tendon_name}' has fewer than 2 sites, skipping") + logger.warning(f"Tendon '{tendon_name}' has fewer than 2 sites, skipping")
737-737: Clean up trailing whitespace on blank lines.Static analysis detected trailing whitespace on several blank lines. While minor, these should be cleaned up for consistency.
Remove trailing whitespace from the blank lines at the indicated line numbers.
Also applies to: 791-791, 795-795, 820-820, 824-824, 833-833, 859-859, 864-864, 872-872
newton/tests/test_tendon_control.py (1)
124-124: Clean up formatting issues.Fix trailing whitespace and add missing newline at end of file.
- Remove trailing whitespace from blank lines at the indicated line numbers
- Add a newline character at the end of the file (line 159)
Also applies to: 129-129, 136-136, 142-142, 144-144, 151-151, 159-159
newton/tests/test_import_mjcf.py (1)
150-150: Clean up trailing whitespace on blank lines.Remove trailing whitespace from the blank lines at the indicated line numbers for consistency with code style guidelines.
Also applies to: 155-155, 160-160, 177-177, 205-205, 209-209, 213-213, 220-220, 237-237, 244-244, 265-265, 268-268, 280-280
newton/examples/example_mjwarp_tendon.py (3)
27-27: Remove or explain the commented-out joint definition.The commented joint definition should either be removed or have a comment explaining why it's kept.
Either remove the line or add an explanatory comment:
- <!--joint name="swing" type="hinge" axis="1 0 0" pos="0 0 0.4"/--> + <!-- Alternative: use hinge joint instead of freejoint for constrained motion --> + <!--joint name="swing" type="hinge" axis="1 0 0" pos="0 0 0.4"/-->
64-64: Remove unnecessary f-string prefixes.These strings don't contain any placeholders, so the
fprefix is not needed.- print(f"Model statistics:") + print("Model statistics:") - print(f"\nFinal result:") + print("\nFinal result:")Also applies to: 195-195
18-18: Clean up formatting issues.Remove trailing whitespace from blank lines throughout the file.
Remove trailing whitespace from the blank lines at all the indicated line numbers for code consistency.
Also applies to: 24-24, 46-46, 68-68, 72-72, 77-77, 93-93, 99-99, 102-102, 107-107, 112-112, 116-116, 120-120, 128-128, 132-132, 141-141, 143-143, 151-151, 159-159, 163-163, 171-171, 173-173, 179-179, 183-183, 190-190, 194-194, 197-197, 202-202, 207-207, 219-219, 230-230, 232-232
newton/sim/model.py (1)
170-200: LGTM! Well-structured tendon system attributes with minor documentation clarification needed.The new attributes for sites, tendons, and tendon actuators are well-organized and follow the existing codebase patterns. The naming conventions are consistent and the documentation is clear.
Consider clarifying the
tendon_site_idsdocumentation to be more explicit about the nested list structure:- """List of site IDs for each tendon. For spatial tendons, this is a list of site indices.""" + """List of site ID arrays for each tendon. For spatial tendons, each entry is a list of site indices defining the tendon path."""newton/sim/builder.py (4)
1724-1749: Fix whitespace issues in blank lines.The
add_sitemethod implementation is correct and well-documented. However, there are whitespace issues on blank lines that should be addressed.Apply this diff to fix the whitespace issues:
- + - +
1751-1782: Fix whitespace issue in blank line.The
add_tendonmethod implementation is well-structured and properly documented. The handling of optional parameters and default values is appropriate.Apply this diff to fix the whitespace issue:
- +
1784-1815: Fix whitespace issues in blank lines.The
add_tendon_actuatormethod implementation is correct and follows good practices. The default force range handling with infinite limits is appropriate for unrestricted actuators.Apply this diff to fix the whitespace issues:
- + - +
3509-3528: Fix whitespace issues in blank lines.The finalization of tendon-related data is well-implemented with proper type conversions and null-safe handling for empty lists. The approach is consistent with the rest of the model finalization process.
Apply this diff to fix the whitespace issues:
- + - +newton/solvers/mujoco/solver_mujoco.py (1)
1246-1272: Consider adding support for tendon force control.The current implementation only handles
tendon_target(position control) but nottendon_f(force control). While the PR objectives mention only position control is implemented, the Control class in the Model does initialize both arrays.Would you like me to implement force control for tendon actuators to match the joint actuator functionality, or should we document this limitation?
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
newton/examples/example_mjwarp_tendon.py(1 hunks)newton/sim/builder.py(4 hunks)newton/sim/control.py(1 hunks)newton/sim/model.py(3 hunks)newton/solvers/mujoco/solver_mujoco.py(4 hunks)newton/tests/test_import_mjcf.py(1 hunks)newton/tests/test_tendon_control.py(1 hunks)newton/utils/import_mjcf.py(3 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: Kenny-Vilella
PR: newton-physics/newton#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.
newton/examples/example_mjwarp_tendon.py (1)
Learnt from: Kenny-Vilella
PR: newton-physics/newton#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.
newton/solvers/mujoco/solver_mujoco.py (1)
Learnt from: Kenny-Vilella
PR: newton-physics/newton#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.
🧬 Code Graph Analysis (4)
newton/utils/import_mjcf.py (2)
newton/utils/import_usd.py (2)
parse_vec(174-181)parse_float(134-141)newton/sim/builder.py (3)
add_site(1724-1749)add_tendon(1751-1782)add_tendon_actuator(1784-1815)
newton/examples/example_mjwarp_tendon.py (5)
newton/sim/builder.py (3)
ModelBuilder(83-3651)collapse_fixed_joints(1414-1688)finalize(3359-3619)newton/utils/import_mjcf.py (1)
parse_mjcf(33-882)newton/solvers/mujoco/solver_mujoco.py (2)
MuJoCoSolver(1004-2484)step(1122-1145)newton/sim/model.py (2)
state(399-432)control(434-472)newton/utils/render.py (2)
SimRendererOpenGL(950-999)SimRendererUsd(609-947)
newton/solvers/mujoco/solver_mujoco.py (2)
newton/sim/model.py (1)
control(434-472)newton/sim/builder.py (2)
add_site(1724-1749)add_tendon(1751-1782)
newton/sim/model.py (4)
newton/solvers/solver.py (1)
device(167-174)newton/sim/collide.py (1)
device(268-269)newton/sim/contacts.py (1)
device(83-84)newton/sim/state.py (1)
requires_grad(75-81)
🪛 Ruff (0.12.2)
newton/utils/import_mjcf.py
737-737: Blank line contains whitespace
Remove whitespace from blank line
(W293)
791-791: Blank line contains whitespace
Remove whitespace from blank line
(W293)
795-795: Blank line contains whitespace
Remove whitespace from blank line
(W293)
820-820: Blank line contains whitespace
Remove whitespace from blank line
(W293)
824-824: Blank line contains whitespace
Remove whitespace from blank line
(W293)
833-833: Blank line contains whitespace
Remove whitespace from blank line
(W293)
859-859: Blank line contains whitespace
Remove whitespace from blank line
(W293)
864-864: Blank line contains whitespace
Remove whitespace from blank line
(W293)
872-872: Blank line contains whitespace
Remove whitespace from blank line
(W293)
newton/tests/test_tendon_control.py
20-20: numpy imported but unused
Remove unused import: numpy
(F401)
61-61: mujoco imported but unused; consider using importlib.util.find_spec to test for availability
(F401)
62-62: mujoco_warp imported but unused; consider using importlib.util.find_spec to test for availability
(F401)
124-124: Blank line contains whitespace
Remove whitespace from blank line
(W293)
129-129: Blank line contains whitespace
Remove whitespace from blank line
(W293)
136-136: Blank line contains whitespace
Remove whitespace from blank line
(W293)
142-142: Blank line contains whitespace
Remove whitespace from blank line
(W293)
144-144: Blank line contains whitespace
Remove whitespace from blank line
(W293)
151-151: Blank line contains whitespace
Remove whitespace from blank line
(W293)
159-159: Trailing whitespace
Remove trailing whitespace
(W291)
159-159: No newline at end of file
Add trailing newline
(W292)
newton/examples/example_mjwarp_tendon.py
18-18: Blank line contains whitespace
Remove whitespace from blank line
(W293)
24-24: Blank line contains whitespace
Remove whitespace from blank line
(W293)
46-46: Blank line contains whitespace
Remove whitespace from blank line
(W293)
64-64: f-string without any placeholders
Remove extraneous f prefix
(F541)
68-68: Blank line contains whitespace
Remove whitespace from blank line
(W293)
72-72: Blank line contains whitespace
Remove whitespace from blank line
(W293)
77-77: Blank line contains whitespace
Remove whitespace from blank line
(W293)
93-93: Blank line contains whitespace
Remove whitespace from blank line
(W293)
99-99: Blank line contains whitespace
Remove whitespace from blank line
(W293)
102-102: Blank line contains whitespace
Remove whitespace from blank line
(W293)
107-107: Blank line contains whitespace
Remove whitespace from blank line
(W293)
112-112: Blank line contains whitespace
Remove whitespace from blank line
(W293)
116-116: Blank line contains whitespace
Remove whitespace from blank line
(W293)
120-120: Blank line contains whitespace
Remove whitespace from blank line
(W293)
128-128: Blank line contains whitespace
Remove whitespace from blank line
(W293)
132-132: Blank line contains whitespace
Remove whitespace from blank line
(W293)
141-141: Blank line contains whitespace
Remove whitespace from blank line
(W293)
143-143: Blank line contains whitespace
Remove whitespace from blank line
(W293)
151-151: Blank line contains whitespace
Remove whitespace from blank line
(W293)
159-159: Blank line contains whitespace
Remove whitespace from blank line
(W293)
163-163: Blank line contains whitespace
Remove whitespace from blank line
(W293)
171-171: Blank line contains whitespace
Remove whitespace from blank line
(W293)
173-173: Blank line contains whitespace
Remove whitespace from blank line
(W293)
179-179: Blank line contains whitespace
Remove whitespace from blank line
(W293)
183-183: Blank line contains whitespace
Remove whitespace from blank line
(W293)
190-190: Blank line contains whitespace
Remove whitespace from blank line
(W293)
194-194: Blank line contains whitespace
Remove whitespace from blank line
(W293)
195-195: f-string without any placeholders
Remove extraneous f prefix
(F541)
197-197: Blank line contains whitespace
Remove whitespace from blank line
(W293)
202-202: Blank line contains whitespace
Remove whitespace from blank line
(W293)
207-207: Blank line contains whitespace
Remove whitespace from blank line
(W293)
213-213: Do not use bare except
(E722)
219-219: Blank line contains whitespace
Remove whitespace from blank line
(W293)
230-230: Blank line contains whitespace
Remove whitespace from blank line
(W293)
232-232: Blank line contains whitespace
Remove whitespace from blank line
(W293)
newton/tests/test_import_mjcf.py
150-150: Blank line contains whitespace
Remove whitespace from blank line
(W293)
155-155: Blank line contains whitespace
Remove whitespace from blank line
(W293)
160-160: Blank line contains whitespace
Remove whitespace from blank line
(W293)
177-177: Blank line contains whitespace
Remove whitespace from blank line
(W293)
205-205: Blank line contains whitespace
Remove whitespace from blank line
(W293)
209-209: Blank line contains whitespace
Remove whitespace from blank line
(W293)
213-213: Blank line contains whitespace
Remove whitespace from blank line
(W293)
220-220: Blank line contains whitespace
Remove whitespace from blank line
(W293)
237-237: Blank line contains whitespace
Remove whitespace from blank line
(W293)
244-244: Blank line contains whitespace
Remove whitespace from blank line
(W293)
265-265: Blank line contains whitespace
Remove whitespace from blank line
(W293)
268-268: Blank line contains whitespace
Remove whitespace from blank line
(W293)
280-280: Blank line contains whitespace
Remove whitespace from blank line
(W293)
newton/solvers/mujoco/solver_mujoco.py
1245-1245: Blank line contains whitespace
Remove whitespace from blank line
(W293)
1251-1251: Blank line contains whitespace
Remove whitespace from blank line
(W293)
1261-1261: Local variable ctrl_idx is assigned to but never used
Remove assignment to unused variable ctrl_idx
(F841)
2043-2043: Blank line contains whitespace
Remove whitespace from blank line
(W293)
2048-2048: Blank line contains whitespace
Remove whitespace from blank line
(W293)
2055-2055: Blank line contains whitespace
Remove whitespace from blank line
(W293)
2061-2061: Blank line contains whitespace
Remove whitespace from blank line
(W293)
2071-2071: Blank line contains whitespace
Remove whitespace from blank line
(W293)
2076-2076: Blank line contains whitespace
Remove whitespace from blank line
(W293)
2080-2080: Blank line contains whitespace
Remove whitespace from blank line
(W293)
2085-2085: Blank line contains whitespace
Remove whitespace from blank line
(W293)
2094-2094: Blank line contains whitespace
Remove whitespace from blank line
(W293)
2100-2100: Blank line contains whitespace
Remove whitespace from blank line
(W293)
newton/sim/builder.py
1743-1743: Blank line contains whitespace
Remove whitespace from blank line
(W293)
1748-1748: Blank line contains whitespace
Remove whitespace from blank line
(W293)
1781-1781: Blank line contains whitespace
Remove whitespace from blank line
(W293)
1807-1807: Blank line contains whitespace
Remove whitespace from blank line
(W293)
1814-1814: Blank line contains whitespace
Remove whitespace from blank line
(W293)
3515-3515: Blank line contains whitespace
Remove whitespace from blank line
(W293)
3522-3522: Blank line contains whitespace
Remove whitespace from blank line
(W293)
🪛 GitHub Actions: Pull Request
newton/tests/test_tendon_control.py
[error] 107-107: RuntimeError: Item indexing is not supported on wp.array objects in test_tendon_control_integration.
🔇 Additional comments (8)
newton/utils/import_mjcf.py (1)
728-740: Site parsing implementation looks good!The site parsing correctly handles:
- Sites attached to bodies with relative transforms
- Worldbody sites with optional world transform application
- Proper scaling and transform composition
Also applies to: 783-798
newton/tests/test_import_mjcf.py (1)
121-285: Excellent test coverage for tendon-related MJCF parsing!The three new test methods comprehensively validate:
- Site parsing from bodies and worldbody
- Tendon parsing with properties and site connections
- Actuator parsing with control parameters
The tests are well-structured and cover both successful parsing and edge cases.
newton/examples/example_mjwarp_tendon.py (1)
101-102: Fix potential Warp array indexing issues.Similar to the test file, direct indexing of Warp arrays may cause issues.
# Record initial state - self.initial_angle = self.state_0.joint_q.numpy()[0] + joint_q_np = self.state_0.joint_q.numpy() + self.initial_angle = joint_q_np[0] if len(joint_q_np) > 0 else 0.0 # Print status every 0.5 seconds (30 frames at 60 FPS) if i % 30 == 0: - angle = self.state_0.joint_q.numpy()[0] - velocity = self.state_0.joint_qd.numpy()[0] + joint_q_np = self.state_0.joint_q.numpy() + joint_qd_np = self.state_0.joint_qd.numpy() + angle = joint_q_np[0] if len(joint_q_np) > 0 else 0.0 + velocity = joint_qd_np[0] if len(joint_qd_np) > 0 else 0.0Also applies to: 186-189
newton/sim/model.py (1)
330-335: LGTM! Counter attributes are properly implemented.The new counter attributes follow the established pattern and are correctly placed with other count attributes.
newton/sim/builder.py (2)
421-442: LGTM! Tendon data structure initialization follows established patterns.The initialization of tendon-related data structures is well-organized and consistent with the existing codebase patterns. All necessary attributes for sites, tendons, and tendon actuators are properly initialized.
3603-3605: LGTM! Count updates follow established patterns.The count updates for sites, tendons, and tendon actuators are correctly implemented and consistent with the existing codebase patterns.
newton/solvers/mujoco/solver_mujoco.py (2)
1657-1659: LGTM!The tendon actuator mapping initialization follows the established pattern and is correctly initialized to -1.
2041-2131: Well-structured implementation of sites, tendons, and tendon actuators!The implementation correctly:
- Handles site transforms relative to parent bodies
- Adds spatial tendons with proper site references
- Configures tendon actuators with position control parameters (kp, kv)
- Sets force ranges appropriately
The code follows MuJoCo API conventions and includes proper null checks.
Signed-off-by: Viktor Reutskyy <[email protected]>
vreutskyy
had a problem deploying
to
external-pr-approval
GitHub Actions
Error
| # return the index of the muscle | ||
| return len(self.muscle_start) - 1 | ||
|
|
||
| def add_site( |
There was a problem hiding this comment.
Does it make sense to add other site specifications here and also the model? e.g. site type/size ? would be nice for visualization paritly with mujoco
There was a problem hiding this comment.
Makes sense to me. But Philipp (@preist-nvidia) said Julia (@jvonmuralt) should check the implementation. Let's wait what she says.
…actuation-of-spatial-tendons
vreutskyy
had a problem deploying
to
external-pr-approval
GitHub Actions
Error
Signed-off-by: Viktor Reutskyy <[email protected]>
vreutskyy
had a problem deploying
to
external-pr-approval
GitHub Actions
Error
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
newton/solvers/mujoco/solver_mujoco.py (1)
1245-1283: Implementation is correct but could be optimized for performance.The tendon control application logic is sound and properly handles both MuJoCo-Warp and regular MuJoCo cases. The validation checks are appropriate.
Minor optimization opportunity: Consider minimizing numpy conversions by performing all necessary updates in numpy space first, then converting back to wp.array only once at the end.
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
newton/solvers/mujoco/solver_mujoco.py(4 hunks)newton/tests/test_tendon_control.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- newton/tests/test_tendon_control.py
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: Kenny-Vilella
PR: newton-physics/newton#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.
newton/solvers/mujoco/solver_mujoco.py (1)
Learnt from: Kenny-Vilella
PR: newton-physics/newton#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.
⏰ 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 (5)
newton/solvers/mujoco/solver_mujoco.py (5)
2053-2080: LGTM! Site addition logic is well implemented.The code correctly handles:
- Site attachment to both worldbody and regular bodies
- Proper quaternion format conversion from xyzw to wxyz
- Safe extraction of transform components from numpy arrays
2081-2107: LGTM! Tendon addition logic correctly handles spatial tendons.The implementation properly:
- Supports only spatial tendons as intended
- Adds appropriate warnings for unsupported tendon types (addresses previous review feedback)
- Uses the correct MuJoCo tendon API with
wrap_site- Safely sets tendon properties with null checks
2108-2149: LGTM! Tendon actuator addition is properly implemented.The code correctly:
- Uses the appropriate MuJoCo tendon actuator type (
mjTRN_TENDON)- Sets up position control with proper gain and bias parameters
- Handles force ranges safely with null checks
- Updates the tendon actuator mapping and count appropriately
2224-2229: LGTM! Actuator count calculations are now correct.The actuator count calculations properly handle:
- Joint actuator count by subtracting tendon actuators from total
- Total actuator count stored directly without double counting
- Proper mapping storage for tendon actuators
This addresses the previous concern about double counting actuators.
1669-1670: LGTM! Proper initialization of tendon actuator mapping.The tendon actuator mapping array is correctly initialized with -1 values to indicate unmapped actuators, following the same pattern as the joint axis mapping.
Signed-off-by: Viktor Reutskyy <[email protected]>
vreutskyy
had a problem deploying
to
external-pr-approval
GitHub Actions
Error
Signed-off-by: Viktor Reutskyy <[email protected]>
vreutskyy
temporarily deployed
to
external-pr-approval
GitHub Actions
Inactive
- Use 'key' instead of 'name' for method parameters and attributes - Rename tendon actuator gains: kp→ke, kv→kd (matching joint convention) Signed-off-by: Viktor Reutskyy <[email protected]>
…actuation-of-spatial-tendons
vreutskyy
had a problem deploying
to
external-pr-approval
GitHub Actions
Error
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
newton/sim/builder.py(4 hunks)newton/sim/model.py(4 hunks)newton/solvers/mujoco/solver_mujoco.py(4 hunks)newton/tests/test_import_mjcf.py(1 hunks)newton/tests/test_tendon_control.py(1 hunks)newton/utils/import_mjcf.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- newton/tests/test_import_mjcf.py
- newton/tests/test_tendon_control.py
- newton/utils/import_mjcf.py
- newton/sim/model.py
- newton/sim/builder.py
🧰 Additional context used
🧠 Learnings (1)
newton/solvers/mujoco/solver_mujoco.py (1)
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.
🧬 Code Graph Analysis (1)
newton/solvers/mujoco/solver_mujoco.py (2)
newton/sim/model.py (1)
control(438-475)newton/sim/builder.py (2)
add_site(1724-1749)add_tendon(1751-1782)
⏰ 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 (5)
newton/solvers/mujoco/solver_mujoco.py (5)
1246-1283: LGTM - Tendon control implementation is solid.The tendon control implementation correctly validates input dimensions, handles both MuJoCo-Warp and regular MuJoCo backends, and properly maps Newton tendon actuators to MuJoCo actuator indices. The previous review feedback appears to have been addressed.
1669-1670: LGTM - Proper mapping array initialization.The tendon actuator mapping array is correctly initialized with -1 values to indicate invalid mappings, following the same pattern as the joint axis mapping.
2053-2080: LGTM - Sites are correctly added to MuJoCo bodies.The implementation properly:
- Checks for site existence before processing
- Extracts transform data correctly from the numpy array
- Handles both worldbody and regular body attachments
- Converts quaternion format appropriately for MuJoCo
2108-2149: LGTM - Tendon actuators are properly created and configured.The tendon actuator implementation correctly:
- Creates MuJoCo actuators with the appropriate type (mjTRN_TENDON)
- Sets up position control parameters using gainprm and biasprm
- Handles force range limits properly
- Maintains proper mapping from Newton tendon actuators to MuJoCo actuators
2224-2229: LGTM - Actuator counts and mappings are correctly stored.The final storage of tendon actuator mappings and counts is correct:
- The tendon actuator mapping is properly stored in the model
- Joint actuator count correctly excludes tendon actuators
- Total actuator count includes all actuators
- Previous issues with double counting appear to be resolved
Signed-off-by: Viktor Reutskyy <[email protected]>
vreutskyy
had a problem deploying
to
external-pr-approval
GitHub Actions
Error
…y unused Signed-off-by: Viktor Reutskyy <[email protected]>
vreutskyy
had a problem deploying
to
external-pr-approval
GitHub Actions
Error
…actuation-of-spatial-tendons Signed-off-by: Viktor Reutskyy <[email protected]>
…actuation-of-spatial-tendons
vreutskyy
had a problem deploying
to
external-pr-approval
GitHub Actions
Error
…actuation-of-spatial-tendons
…actuation-of-spatial-tendons
Signed-off-by: Ryan Gresia <[email protected]>
Signed-off-by: Ryan Gresia <[email protected]>
Signed-off-by: Ryan Gresia <[email protected]>
…actuation-of-spatial-tendons Signed-off-by: Viktor Reutskyy <[email protected]>
…actuation-of-spatial-tendons
Signed-off-by: Viktor Reutskyy <[email protected]>
…ons-in-selection-api Add support for spatial tendons in selection api
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
newton/utils/selection.py (1)
457-461: Good defensive check for empty selection.The additional check
len(selected_tendon_ids) > 0prevents invalid slice creation when no tendons are selected.Consider applying the same defensive check to joints, links, and shapes for consistency:
-if self.joints_contiguous: +if self.joints_contiguous and len(selected_joint_ids) > 0: self._frequency_slices["joint"] = slice(selected_joint_ids[0], selected_joint_ids[-1] + 1)
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
newton/sim/model.py(5 hunks)newton/tests/test_tendon_control.py(1 hunks)newton/utils/selection.py(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- newton/tests/test_tendon_control.py
- newton/sim/model.py
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: Kenny-Vilella
PR: newton-physics/newton#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.
⏰ 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 Unit Tests on AWS EC2 (Pull Request)
- GitHub Check: Run GPU Benchmarks (Pull Request)
- GitHub Check: run-newton-tests / newton-unittests (ubuntu-latest)
- GitHub Check: run-newton-tests / newton-unittests (windows-latest)
🔇 Additional comments (7)
newton/utils/selection.py (7)
138-139: LGTM! Consistent parameter addition for tendon filtering.The new
include_tendonsandexclude_tendonsparameters follow the established pattern for joints and links, maintaining API consistency.
164-164: LGTM!Appropriately retrieves the tendon actuator count from the model.
182-183: LGTM! Proper initialization and population of tendon data.The tendon indices and names are collected following the same pattern as joints and links.
Also applies to: 198-200
271-299: LGTM! Robust tendon filtering implementation.The tendon inclusion and exclusion logic properly handles string patterns, integer indices, and type validation, maintaining consistency with joint and link filtering.
303-303: LGTM!Correctly computes the final selected tendon indices using set operations.
310-310: LGTM!Proper initialization of tendon-related lists as class attributes.
Also applies to: 320-320
420-420: LGTM!Correctly checks tendon index contiguity following the established pattern.
Signed-off-by: Viktor Reutskyy <[email protected]>
Signed-off-by: Viktor Reutskyy <[email protected]>
Signed-off-by: Viktor Reutskyy <[email protected]>
…actuation-of-spatial-tendons
…actuation-of-spatial-tendons
…actuation-of-spatial-tendons
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
newton/solvers/mujoco/solver_mujoco.py (1)
2243-2266: Consider using f-strings consistently for warning messages.While the implementation is correct, for consistency with other warning messages in the codebase, consider using f-strings throughout.
else: wp.utils.warn( - f"Tendon type '{tendon_type}' is not supported by MuJoCo solver, skipping tendon '{tendon_key}'" + f"Tendon type '{tendon_type}' is not supported by MuJoCo solver, skipping tendon '{tendon_key}'" )newton/sim/builder.py (1)
442-463: Consider documenting the tendon-related lists more thoroughly.While the initialization is correct, consider adding brief inline comments to clarify the purpose and expected data types for each tendon-related list, similar to how other model components are documented.
# sites (reference points on bodies for tendons) self.site_key = [] # list[str]: unique keys for sites self.site_body = [] # list[int]: body indices sites are attached to self.site_xform = [] # list[Transform]: site transforms relative to body # tendons self.tendon_key = [] # list[str]: unique keys for tendons self.tendon_type = [] # list[str]: tendon types (e.g., 'spatial') self.tendon_site_ids = [] # list[list[int]]: site indices for each tendon self.tendon_damping = [] # list[float]: damping coefficients self.tendon_stiffness = [] # list[float]: stiffness coefficients self.tendon_rest_length = [] # list[float]: rest lengths # tendon actuators self.tendon_actuator_tendon_id = [] # list[int]: tendon indices being actuated self.tendon_actuator_key = [] # list[str]: unique keys for actuators self.tendon_actuator_ke = [] # list[float]: elastic/position gains self.tendon_actuator_kd = [] # list[float]: damping/velocity gains self.tendon_actuator_force_range = [] # list[tuple[float, float]]: force limits
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
newton/sim/builder.py(4 hunks)newton/solvers/mujoco/solver_mujoco.py(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
newton/solvers/mujoco/solver_mujoco.py (3)
newton/sim/model.py (1)
control(505-542)newton/solvers/solver.py (1)
device(170-177)newton/sim/builder.py (2)
add_site(1915-1940)add_tendon(1942-1973)
⏰ 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 Unit Tests on AWS EC2 (Pull Request)
- GitHub Check: Run GPU Benchmarks (Pull Request)
- GitHub Check: run-newton-tests / newton-unittests (ubuntu-latest)
- GitHub Check: run-newton-tests / newton-unittests (windows-latest)
🔇 Additional comments (10)
newton/solvers/mujoco/solver_mujoco.py (5)
630-642: LGTM!The implementation of the
apply_mjc_tendon_control_kernelis correct and follows established Warp kernel patterns. The kernel properly maps tendon actuator indices to MuJoCo actuator indices and applies the control targets.
1339-1362: LGTM! Well-structured tendon control integration.The tendon control implementation is properly integrated into the existing control flow with appropriate validation and device-aware launching. The code correctly handles:
- Dimension validation with a clear error message
- Proper mapping of tendon actuators to MuJoCo actuators
- Efficient kernel launch pattern
2214-2239: LGTM! Thorough site parsing implementation.The site parsing correctly extracts transform components from numpy arrays and properly attaches sites to both the worldbody and regular bodies through the MuJoCo spec API.
2270-2308: LGTM! Complete tendon actuator integration.The tendon actuator parsing and creation is well-implemented with proper handling of:
- Actuator-to-tendon mapping
- PD control parameters (ke/kd)
- Force range constraints
- Proper actuator count tracking
2426-2431: LGTM! Model state properly updated with tendon actuator mappings.The code correctly stores the tendon actuator mapping and actuator counts in the Newton model, properly differentiating between joint actuators and total actuators.
newton/sim/builder.py (5)
1915-1940: LGTM! Clean and straightforward site implementation.The
add_sitemethod is well-implemented with proper defaults and follows the established pattern of other builder methods.
1942-1973: LGTM! Well-structured tendon creation method.The
add_tendonmethod follows best practices with sensible defaults and proper documentation.
1975-2007: LGTM! Complete tendon actuator implementation.The
add_tendon_actuatormethod properly handles force range defaults and follows the established pattern for actuator creation.
3827-3863: LGTM! Comprehensive tendon data finalization.The finalization properly transfers all tendon-related data to the Model with correct type conversions and conditional array creation. The initialization of tendon_target with zeros is appropriate for position control actuators.
4031-4033: LGTM! Count variables properly updated.The site, tendon, and tendon actuator counts are correctly computed and assigned to the model.
5 participants
Description
This PR adds support for parsing and actuating spatial tendons from MJCF files in Newton. Closes #233. The implementation enables Newton to handle tendon-based actuation systems, which are commonly used in robotics for cable-driven mechanisms, biomechanical simulations, and compliant actuators.
Key Features Added:
Extended Control System
tendon_targetarray to theControlclass for tendon position targetsModel/ModelBuilder Extensions
site_name,site_body,site_xform,site_counttendon_name,tendon_type,tendon_site_ids,tendon_damping,tendon_stiffness,tendon_rest_length,tendon_counttendon_actuator_tendon_id,tendon_actuator_name,tendon_actuator_kp,tendon_actuator_kv,tendon_actuator_force_range,tendon_actuator_countadd_site(),add_tendon(),add_tendon_actuator()MJCF Parser Updates
<site>elements from bodies and worldbody<spatial>tendons with site waypoint referencesMuJoCo Solver Integration
mjSpec.add_site())mjSpec.add_tendon()andtendon.wrap_site()Example and Tests
example_mjwarp_tendon.pydemonstrating a pendulum controlled by a spatial tendonLimitations:
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
New Features
Tests