Support USD parsing from physX fixed tendons and plumb through to MJWarp #654 #688
Conversation
…arp newton-physics#654 Signed-off-by: Viktor Reutskyy <[email protected]>
Signed-off-by: Viktor Reutskyy <[email protected]>
📝 WalkthroughWalkthroughAdds fixed tendon support end-to-end: builder data structures and API (add_tendon), model fields, USD import extraction/creation, MuJoCo conversion, examples demonstrating usage, and tests plus a USD asset. Builder merging/replication and finalize/export paths are extended to carry tendon arrays. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant USD as USD file
participant Importer as USD Importer
participant Builder as ModelBuilder
participant Model as Model
participant Solver as SolverMuJoCo
participant MjSpec as MuJoCo Spec
User->>Importer: import_usd(path)
Importer->>USD: Read joints
Importer->>Importer: extract_tendon_info_from_joint()
Importer->>Builder: add_joint*(...)
loop per tendon with ≥2 joints
Importer->>Builder: add_tendon(name, joint_ids, gearings, params)
end
User->>Builder: finalize()
Builder->>Model: Export tendon_* arrays and tendon_count
User->>Solver: convert_to_mjc(Model)
alt model.tendon_count > 0
Solver->>Solver: For each tendon
Solver->>MjSpec: spec.add_tendon(name, props)
Solver->>MjSpec: tendon.wrap_joint(joint_name, gearing)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. ✨ 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (9)
newton/examples/robot/example_robot_shadowhand_tendon.py (3)
37-37: Consider parameterizing default arguments.The
disable_tendonsparameter defaults toTrue, which seems counterintuitive for a tendon demonstration example. Consider defaulting toFalseto showcase the tendon functionality by default.- def __init__(self, viewer, num_envs=4, disable_tendons=True, mass_scale=1.0): + def __init__(self, viewer, num_envs=4, disable_tendons=False, mass_scale=1.0):
167-168: Remove commented-out debug code.These commented lines appear to be leftover debug code and should be removed for clarity.
- # tendon_joints = self.model.tendon_joints.numpy() if hasattr(self.model.tendon_joints, 'numpy') else self.model.tendon_joints - # tendon_gearings = self.model.tendon_gearings.numpy() if hasattr(self.model.tendon_gearings, 'numpy') else self.model.tendon_gearings
288-294: Enhance finger joint detection for robustness.The current logic looks for joints with both a prefix (FFJ, MFJ, etc.) AND "J2" in the name. This might miss joints or be fragile to naming variations. Consider making the detection more flexible.
for i, name in enumerate(joint_names): for prefix in finger_prefixes: - if prefix in name and "J2" in name: # J2 is typically the main flexion joint + # Match any joint with the finger prefix and J2, or just the prefix if no J2 exists + if prefix in name and ("J2" in name or f"{prefix}2" in name): self.finger_joints.append(i) print(f"Found finger joint: {name} at index {i}") breaknewton/_src/solvers/mujoco/solver_mujoco.py (1)
2309-2311: Add explicit shape validation for springlength_array
Before assigning, assert thatspringlength_array.shape == (2, 1)to guard against accidental changes to its dimensions. For example, just above line 2309:if springlength_array.shape != (2, 1): raise ValueError(f"springlength must be shape (2,1), got {springlength_array.shape}")This ensures the MuJoCo API receives the expected 2×1 array.
newton/tests/test_mujoco_solver.py (1)
1452-1457: Clarify tolerance wording.Message says “2% tolerance” but uses an absolute 0.02 rad. Make the text consistent to avoid confusion.
- 0.02, # 2% tolerance for numerical stability + 0.02, # 0.02 rad absolute tolerance for numerical stabilitynewton/_src/utils/import_usd.py (1)
35-67: Tendon attribute extraction is sensible; minor robustness nits.Consider restricting attribute match to exact prefix "physxTendon:{name}:" to avoid accidental substring hits in edge cases (current code already does this implicitly). Otherwise LGTM.
newton/_src/sim/builder.py (1)
1191-1208: Make tendon_start offset logic simpler and safer.Current loop assumes builder.tendon_start[0] == 0. Prefer explicit offsetting of all entries and document the assumption.
Apply:
- existing_tendon_joints = len(self.tendon_joints) - for i in range(len(builder.tendon_start)): - if i == 0: - self.tendon_start.append(existing_tendon_joints) - else: - self.tendon_start.append(existing_tendon_joints + builder.tendon_start[i]) + existing_tendon_joints = len(self.tendon_joints) + # builder.tendon_start must be relative to builder.tendon_joints and typically starts at 0 + self.tendon_start.extend( + existing_tendon_joints + np.array(builder.tendon_start, dtype=np.int32) + )If builder.tendon_start can legally start > 0, keep the new code; it remains correct.
newton/tests/assets/tendons_test.usda (2)
17-21: Cube sizing inconsistency (size vs extent).Base Cube authors both size and extent; Link Cubes author only extent. For robustness, prefer authoring the canonical Cube attribute (size) consistently across all links, unless your importer explicitly relies on extent.
Would you like me to normalize these to size everywhere?
Also applies to: 33-36
41-66: Tendon root/participant usage reads correctly; consider explicit comments.The root is declared once per tendon (tendon1 on Joint1, tendon2 on Joint2). This is fine; add a short comment noting roots are unique and participants add gearing only.
Also applies to: 83-107, 125-141
📜 Review details
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (11)
newton/_src/sim/builder.py(5 hunks)newton/_src/sim/model.py(2 hunks)newton/_src/solvers/mujoco/solver_mujoco.py(1 hunks)newton/_src/utils/import_usd.py(6 hunks)newton/examples/__init__.py(1 hunks)newton/examples/robot/example_robot_shadowhand_tendon.py(1 hunks)newton/examples/robot/example_robot_simple_tendon.py(1 hunks)newton/tests/assets/tendons_test.usda(1 hunks)newton/tests/test_fixed_tendons.py(1 hunks)newton/tests/test_import_usd.py(2 hunks)newton/tests/test_mujoco_solver.py(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-08-27T19:05:44.678Z
Learnt from: Milad-Rakhsha-NV
PR: newton-physics/newton#535
File: newton/tests/test_examples.py:320-414
Timestamp: 2025-08-27T19:05:44.678Z
Learning: In newton/examples/__init__.py, the robot policy example is registered with the key "robot_policy" (not "robot.example_robot_policy"), so tests should reference it as name="robot_policy".
Applied to files:
newton/examples/__init__.py
🧬 Code graph analysis (7)
newton/examples/robot/example_robot_shadowhand_tendon.py (5)
newton/examples/robot/example_robot_simple_tendon.py (5)
Example(47-316)capture(236-241)simulate(243-254)step(256-289)render(291-295)newton/_src/sim/builder.py (6)
ModelBuilder(68-4559)add_usd(720-818)collapse_fixed_joints(2077-2374)replicate(628-650)finalize(4154-4511)copy(193-194)newton/_src/solvers/mujoco/solver_mujoco.py (1)
step(1273-1296)newton/_src/sim/model.py (2)
state(453-487)control(489-522)newton/_src/viewer/viewer.py (1)
log_contacts(101-162)
newton/tests/test_fixed_tendons.py (1)
newton/_src/sim/builder.py (5)
ModelBuilder(68-4559)add_body(1225-1287)add_joint_revolute(1404-1486)add_tendon(2410-2461)finalize(4154-4511)
newton/_src/solvers/mujoco/solver_mujoco.py (1)
newton/_src/sim/builder.py (1)
add_tendon(2410-2461)
newton/tests/test_import_usd.py (1)
newton/_src/sim/builder.py (1)
joint_count(532-536)
newton/_src/utils/import_usd.py (1)
newton/_src/sim/builder.py (2)
joint_count(532-536)add_tendon(2410-2461)
newton/examples/robot/example_robot_simple_tendon.py (2)
newton/_src/sim/builder.py (3)
ModelBuilder(68-4559)add_tendon(2410-2461)copy(193-194)newton/_src/solvers/mujoco/solver_mujoco.py (1)
SolverMuJoCo(1096-2656)
newton/tests/test_mujoco_solver.py (3)
newton/_src/sim/builder.py (2)
add_tendon(2410-2461)finalize(4154-4511)newton/_src/sim/model.py (2)
state(453-487)control(489-522)newton/_src/solvers/mujoco/solver_mujoco.py (1)
step(1273-1296)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Run GPU Benchmarks (Pull Request)
- GitHub Check: Run GPU Unit Tests on AWS EC2 (Pull Request)
🔇 Additional comments (19)
newton/_src/sim/model.py (2)
208-217: LGTM! Tendon attributes are well-documented and consistently structured.The new tendon-related attributes follow the established pattern for other constraints (springs, muscles) with appropriate documentation, shape descriptions, and data types.
371-372: LGTM! Count attributes properly added.The
tendon_countattribute follows the established pattern for other system counts and is correctly documented.newton/examples/__init__.py (1)
202-203: LGTM! Example entries correctly added.The new tendon example entries follow the established naming convention and are properly mapped to their respective modules.
newton/_src/solvers/mujoco/solver_mujoco.py (1)
2243-2256: LGTM! Proper handling of optional tendon data.The code correctly checks for the existence of tendon data and handles both numpy arrays and other array types appropriately.
newton/examples/robot/example_robot_simple_tendon.py (1)
144-148: Good tendon coupling design!The negative gearing values (-0.8) create an antagonistic coupling where joint2 moves in the opposite direction to joint1, simulating realistic finger tendon mechanics.
newton/tests/test_import_usd.py (2)
367-382: Good fix: use 'extent' (singular) for Cube visuals.Correctly switches to the USD 'extent' attribute; matches USD schema and avoids mis-sized visuals.
515-602: USD tendon import test is thorough and well-structured.Covers joint mapping, gearings, and parameter extraction end-to-end.
newton/tests/test_fixed_tendons.py (2)
29-67: Solid basic creation/validation coverage.Validates key fields and error cases clearly.
101-169: Finalize-path assertions look correct.Verifies sentinel close in tendon_start and parameter defaults/limits.
newton/_src/utils/import_usd.py (4)
366-382: Nice: visual Cube uses 'extent' reliably now.This aligns visuals with collision sizing.
551-554: Returning None for disabled joints is correct and handled downstream.This prevents bogus tendon/joint bookkeeping.
755-762: Tendon plumbing end-to-end looks good; confirm units for angular joints.PhysX USD often specifies revolute limits/targets in degrees, while Newton/MuJoCo expect radians. Tendon restLength/lowerLimit/upperLimit must be consistent with sum(gearing[i] * q[i]) units. If any participating DOF is angular (revolute or rotational D6), ensure imported tendon params are in radians or convert them here; otherwise constraints/limits will be off by ~57.3x.
I can add a guarded conversion based on participating DOF types if desired.
Also applies to: 974-1062
1303-1307: Good fix: inv_mass set to 0 for mass<=0.Avoids infs/NaNs in dynamics.
newton/_src/sim/builder.py (4)
402-408: Tendon arrays added: layout is consistent with finalize.Naming and separation (start/params/joints/gearings/key) match later export; no issues spotted.
1184-1186: Copy semantics: adding tendon_params and tendon_key is correct.These belong in the bulk attribute copy; starts/joints/gearings are handled below. LGTM.
4313-4325: Finalize/export of tendon arrays: LGTM.Sentinel-closing, array dtypes, and tendon_count assignment are consistent with other multi-entity structures.
Also applies to: 4496-4496
953-967: Resolve: edge-case impossible—add_tendon enforces ≥2 joints.
builder.add_tendon raises a ValueError for fewer than two joint IDs (see test_fixed_tendons.py) and import_usd only invokes add_tendon whenlen(joints) >= 2, so a builder with tendons always contains at least two joints.newton/tests/assets/tendons_test.usda (2)
41-66: Angle limits units: confirm radians vs degrees.physics:lowerLimit/upperLimit are set to -90/90. Ensure the USD importer converts degrees to radians or that these are radians already; otherwise limits will be off by ~57.3x.
23-24: Zero-mass Base: intentional?Base has physics:mass = 0.0. If this is to anchor the articulation, fine; otherwise dynamics may differ from expectations.
| def add_tendon( | ||
| self, | ||
| name: str, | ||
| joint_ids: list[int], | ||
| gearings: list[float], | ||
| stiffness: float = 0.0, | ||
| damping: float = 0.0, | ||
| rest_length: float = 0.0, | ||
| lower_limit: float = float("-inf"), | ||
| upper_limit: float = float("inf"), | ||
| ) -> int: | ||
| """Adds a fixed tendon constraint between multiple joints. | ||
| Fixed tendons couple the motion of multiple joints through a linear | ||
| constraint on their positions. The tendon length is computed as: | ||
| L = rest_length + sum(gearing[i] * joint_pos[i]) | ||
| Args: | ||
| name: A unique identifier for the tendon | ||
| joint_ids: List of joint indices that this tendon connects | ||
| gearings: Gearing coefficient for each joint (transmission ratio) | ||
| stiffness: Elastic stiffness of the tendon (0 for hard constraint) | ||
| damping: Damping coefficient | ||
| rest_length: Rest length of the tendon | ||
| lower_limit: Lower limit for tendon length | ||
| upper_limit: Upper limit for tendon length | ||
| Returns: | ||
| The index of the tendon in the model | ||
| """ | ||
| if len(joint_ids) != len(gearings): | ||
| raise ValueError("Number of joint IDs must match number of gearings") | ||
|
|
||
| if len(joint_ids) < 2: | ||
| raise ValueError("Tendon must connect at least 2 joints") | ||
|
|
||
| # Store the start index for this tendon's joint data | ||
| self.tendon_start.append(len(self.tendon_joints)) | ||
|
|
||
| # Store tendon parameters | ||
| self.tendon_params.append((stiffness, damping, rest_length, lower_limit, upper_limit)) | ||
|
|
||
| # Store the name/key | ||
| self.tendon_key.append(name) | ||
|
|
||
| # Store joint indices and gearings | ||
| for joint_id, gearing in zip(joint_ids, gearings, strict=False): | ||
| self.tendon_joints.append(joint_id) | ||
| self.tendon_gearings.append(gearing) | ||
|
|
||
| # Return the index of the tendon | ||
| return len(self.tendon_start) - 1 | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
add_tendon lacks essential validation (ids, dof, limits, degeneracy, duplicate name).
Invalid joint ids or non-scalar joints will explode later in the solver. Limits can be inverted; all-zero gearings yield a no-op tendon.
Apply:
def add_tendon(
self,
name: str,
joint_ids: list[int],
gearings: list[float],
stiffness: float = 0.0,
damping: float = 0.0,
rest_length: float = 0.0,
lower_limit: float = float("-inf"),
upper_limit: float = float("inf"),
) -> int:
@@
- if len(joint_ids) != len(gearings):
+ if len(joint_ids) != len(gearings):
raise ValueError("Number of joint IDs must match number of gearings")
- if len(joint_ids) < 2:
+ if len(joint_ids) < 2:
raise ValueError("Tendon must connect at least 2 joints")
+ # Basic parameter sanity
+ if lower_limit > upper_limit:
+ raise ValueError(f"lower_limit ({lower_limit}) must be <= upper_limit ({upper_limit})")
+ if name in self.tendon_key:
+ raise ValueError(f"Duplicate tendon name: {name!r}")
+ if len(set(joint_ids)) != len(joint_ids):
+ raise ValueError("Duplicate joint indices in tendon are not allowed")
+ if not any(abs(g) > 0.0 for g in gearings):
+ raise ValueError("All gearings are zero; tendon would be degenerate")
+
+ # Validate joint indices and that each is 1-DoF (revolute or prismatic)
+ invalid = [j for j in joint_ids if j < 0 or j >= self.joint_count]
+ if invalid:
+ raise IndexError(f"Invalid joint indices for tendon {name}: {invalid}")
+ for j in set(joint_ids):
+ lin, ang = self.joint_dof_dim[j]
+ if (lin + ang) != 1:
+ raise ValueError(
+ f"Tendon {name} expects scalar joints; joint {j} has {lin+ang} DoFs"
+ )
+
# Store the start index for this tendon's joint data
self.tendon_start.append(len(self.tendon_joints))
@@
- for joint_id, gearing in zip(joint_ids, gearings, strict=False):
+ for joint_id, gearing in zip(joint_ids, gearings, strict=False):
self.tendon_joints.append(joint_id)
self.tendon_gearings.append(gearing)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def add_tendon( | |
| self, | |
| name: str, | |
| joint_ids: list[int], | |
| gearings: list[float], | |
| stiffness: float = 0.0, | |
| damping: float = 0.0, | |
| rest_length: float = 0.0, | |
| lower_limit: float = float("-inf"), | |
| upper_limit: float = float("inf"), | |
| ) -> int: | |
| """Adds a fixed tendon constraint between multiple joints. | |
| Fixed tendons couple the motion of multiple joints through a linear | |
| constraint on their positions. The tendon length is computed as: | |
| L = rest_length + sum(gearing[i] * joint_pos[i]) | |
| Args: | |
| name: A unique identifier for the tendon | |
| joint_ids: List of joint indices that this tendon connects | |
| gearings: Gearing coefficient for each joint (transmission ratio) | |
| stiffness: Elastic stiffness of the tendon (0 for hard constraint) | |
| damping: Damping coefficient | |
| rest_length: Rest length of the tendon | |
| lower_limit: Lower limit for tendon length | |
| upper_limit: Upper limit for tendon length | |
| Returns: | |
| The index of the tendon in the model | |
| """ | |
| if len(joint_ids) != len(gearings): | |
| raise ValueError("Number of joint IDs must match number of gearings") | |
| if len(joint_ids) < 2: | |
| raise ValueError("Tendon must connect at least 2 joints") | |
| # Store the start index for this tendon's joint data | |
| self.tendon_start.append(len(self.tendon_joints)) | |
| # Store tendon parameters | |
| self.tendon_params.append((stiffness, damping, rest_length, lower_limit, upper_limit)) | |
| # Store the name/key | |
| self.tendon_key.append(name) | |
| # Store joint indices and gearings | |
| for joint_id, gearing in zip(joint_ids, gearings, strict=False): | |
| self.tendon_joints.append(joint_id) | |
| self.tendon_gearings.append(gearing) | |
| # Return the index of the tendon | |
| return len(self.tendon_start) - 1 | |
| def add_tendon( | |
| self, | |
| name: str, | |
| joint_ids: list[int], | |
| gearings: list[float], | |
| stiffness: float = 0.0, | |
| damping: float = 0.0, | |
| rest_length: float = 0.0, | |
| lower_limit: float = float("-inf"), | |
| upper_limit: float = float("inf"), | |
| ) -> int: | |
| """Adds a fixed tendon constraint between multiple joints. | |
| Fixed tendons couple the motion of multiple joints through a linear | |
| constraint on their positions. The tendon length is computed as: | |
| L = rest_length + sum(gearing[i] * joint_pos[i]) | |
| Args: | |
| name: A unique identifier for the tendon | |
| joint_ids: List of joint indices that this tendon connects | |
| gearings: Gearing coefficient for each joint (transmission ratio) | |
| stiffness: Elastic stiffness of the tendon (0 for hard constraint) | |
| damping: Damping coefficient | |
| rest_length: Rest length of the tendon | |
| lower_limit: Lower limit for tendon length | |
| upper_limit: Upper limit for tendon length | |
| Returns: | |
| The index of the tendon in the model | |
| """ | |
| if len(joint_ids) != len(gearings): | |
| raise ValueError("Number of joint IDs must match number of gearings") | |
| if len(joint_ids) < 2: | |
| raise ValueError("Tendon must connect at least 2 joints") | |
| # Basic parameter sanity | |
| if lower_limit > upper_limit: | |
| raise ValueError( | |
| f"lower_limit ({lower_limit}) must be <= upper_limit ({upper_limit})" | |
| ) | |
| if name in self.tendon_key: | |
| raise ValueError(f"Duplicate tendon name: {name!r}") | |
| if len(set(joint_ids)) != len(joint_ids): | |
| raise ValueError("Duplicate joint indices in tendon are not allowed") | |
| if not any(abs(g) > 0.0 for g in gearings): | |
| raise ValueError("All gearings are zero; tendon would be degenerate") | |
| # Validate joint indices and that each is 1-DoF (revolute or prismatic) | |
| invalid = [j for j in joint_ids if j < 0 or j >= self.joint_count] | |
| if invalid: | |
| raise IndexError(f"Invalid joint indices for tendon {name}: {invalid}") | |
| for j in set(joint_ids): | |
| lin, ang = self.joint_dof_dim[j] | |
| if (lin + ang) != 1: | |
| raise ValueError( | |
| f"Tendon {name} expects scalar joints; joint {j} has {lin+ang} DoFs" | |
| ) | |
| # Store the start index for this tendon's joint data | |
| self.tendon_start.append(len(self.tendon_joints)) | |
| # Store tendon parameters | |
| self.tendon_params.append( | |
| (stiffness, damping, rest_length, lower_limit, upper_limit) | |
| ) | |
| # Store the name/key | |
| self.tendon_key.append(name) | |
| # Store joint indices and gearings | |
| for joint_id, gearing in zip(joint_ids, gearings, strict=False): | |
| self.tendon_joints.append(joint_id) | |
| self.tendon_gearings.append(gearing) | |
| # Return the index of the tendon | |
| return len(self.tendon_start) - 1 |
| all_joints_selected = True | ||
| for j in range(start_idx, end_idx): | ||
| joint_idx = tendon_joints[j] | ||
| if joint_idx not in selected_joints: | ||
| all_joints_selected = False | ||
| break | ||
|
|
||
| # Skip this tendon if not all joints are selected | ||
| if not all_joints_selected: | ||
| continue |
There was a problem hiding this comment.
Add bounds checking for joint indices.
The code should validate that joint indices are within valid bounds before using them to avoid potential crashes or undefined behavior.
for j in range(start_idx, end_idx):
joint_idx = tendon_joints[j]
+ if joint_idx < 0 or joint_idx >= model.joint_count:
+ print(f"Warning: Tendon {i} references invalid joint index {joint_idx}, skipping tendon")
+ all_joints_selected = False
+ break
if joint_idx not in selected_joints:
all_joints_selected = False
break📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| all_joints_selected = True | |
| for j in range(start_idx, end_idx): | |
| joint_idx = tendon_joints[j] | |
| if joint_idx not in selected_joints: | |
| all_joints_selected = False | |
| break | |
| # Skip this tendon if not all joints are selected | |
| if not all_joints_selected: | |
| continue | |
| all_joints_selected = True | |
| for j in range(start_idx, end_idx): | |
| joint_idx = tendon_joints[j] | |
| if joint_idx < 0 or joint_idx >= model.joint_count: | |
| print(f"Warning: Tendon {i} references invalid joint index {joint_idx}, skipping tendon") | |
| all_joints_selected = False | |
| break | |
| if joint_idx not in selected_joints: | |
| all_joints_selected = False | |
| break | |
| # Skip this tendon if not all joints are selected | |
| if not all_joints_selected: | |
| continue |
| joint_idx = tendon_joints[j] | ||
| gearing = tendon_gearings[j] | ||
| joint_name = model.joint_key[joint_idx] | ||
|
|
||
| # In MuJoCo spec API, use wrap_joint to add a joint to the tendon | ||
| # This is the Python equivalent of mjs_wrapJoint in C API | ||
| tendon.wrap_joint(joint_name, gearing) |
There was a problem hiding this comment.
Add error handling for missing joint names.
The code assumes joint names exist in the model but doesn't handle the case where a joint might not have a name.
for j in range(start_idx, end_idx):
joint_idx = tendon_joints[j]
gearing = tendon_gearings[j]
- joint_name = model.joint_key[joint_idx]
+ joint_name = model.joint_key[joint_idx] if joint_idx < len(model.joint_key) else f"joint_{joint_idx}"
# In MuJoCo spec API, use wrap_joint to add a joint to the tendon
# This is the Python equivalent of mjs_wrapJoint in C API
- tendon.wrap_joint(joint_name, gearing)
+ try:
+ tendon.wrap_joint(joint_name, gearing)
+ except Exception as e:
+ print(f"Warning: Failed to wrap joint '{joint_name}' to tendon '{name}': {e}")Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In newton/_src/solvers/mujoco/solver_mujoco.py around lines 2322 to 2328, the
code dereferences model.joint_key[joint_idx] without checking that a name
exists; update it to validate that joint_idx is in range and that joint_name is
not None/empty before calling tendon.wrap_joint. If the name is missing, either
log a clear warning including joint_idx and tendon identifier and skip wrapping
that joint, or raise a descriptive exception if a missing name should be
considered fatal; ensure logging uses the existing logger and that control flow
continues safely when skipping.
|
|
||
| ## Apply mass scaling if requested | ||
| # if mass_scale != 1.0: | ||
| # print(f"\nApplying mass scale factor: {mass_scale}") | ||
| # # Scale all body masses (which means scaling inverse masses down) | ||
| # self.model.body_inv_mass = self.model.body_inv_mass / mass_scale | ||
| # # Also scale inertias | ||
| # self.model.body_inv_inertia = self.model.body_inv_inertia / mass_scale | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Uncomment mass scaling code or remove it.
This commented-out mass scaling code should either be uncommented (if functional) or removed to avoid confusion. The mass_scale parameter is accepted but not used.
- ## Apply mass scaling if requested
- # if mass_scale != 1.0:
- # print(f"\nApplying mass scale factor: {mass_scale}")
- # # Scale all body masses (which means scaling inverse masses down)
- # self.model.body_inv_mass = self.model.body_inv_mass / mass_scale
- # # Also scale inertias
- # self.model.body_inv_inertia = self.model.body_inv_inertia / mass_scale
+ # Apply mass scaling if requested
+ if mass_scale != 1.0:
+ print(f"\nApplying mass scale factor: {mass_scale}")
+ # Scale all body masses (which means scaling inverse masses down)
+ self.model.body_inv_mass = self.model.body_inv_mass / mass_scale
+ # Also scale inertias
+ self.model.body_inv_inertia = self.model.body_inv_inertia / mass_scale📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ## Apply mass scaling if requested | |
| # if mass_scale != 1.0: | |
| # print(f"\nApplying mass scale factor: {mass_scale}") | |
| # # Scale all body masses (which means scaling inverse masses down) | |
| # self.model.body_inv_mass = self.model.body_inv_mass / mass_scale | |
| # # Also scale inertias | |
| # self.model.body_inv_inertia = self.model.body_inv_inertia / mass_scale | |
| # Apply mass scaling if requested | |
| if mass_scale != 1.0: | |
| print(f"\nApplying mass scale factor: {mass_scale}") | |
| # Scale all body masses (which means scaling inverse masses down) | |
| self.model.body_inv_mass = self.model.body_inv_mass / mass_scale | |
| # Also scale inertias | |
| self.model.body_inv_inertia = self.model.body_inv_inertia / mass_scale |
🤖 Prompt for AI Agents
In newton/examples/robot/example_robot_shadowhand_tendon.py around lines 142 to
150, the mass scaling block is commented out while a mass_scale parameter is
accepted but unused; either re-enable this scaling logic or remove it to avoid
confusion. If re-enabling, uncomment the block, ensure mass_scale is validated
(e.g., != 1.0) before applying, and update self.model.body_inv_mass and
self.model.body_inv_inertia by dividing by mass_scale as shown; if removing,
delete the commented lines and any unused mass_scale parameter or related
references and update function signatures/docs accordingly.
| gearings=[1.0, -0.6], # Joint3 moves 80% with joint2 | ||
| stiffness=100.0, # Moderate stiffness |
There was a problem hiding this comment.
Fix comment to match the gearing value.
The comment says "Joint3 moves 80% with joint2" but the gearing is -0.6 (60%) relative to joint1.
- gearings=[1.0, -0.6], # Joint3 moves 80% with joint2
+ gearings=[1.0, -0.6], # Joint3 moves 60% opposite to joint1📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| gearings=[1.0, -0.6], # Joint3 moves 80% with joint2 | |
| stiffness=100.0, # Moderate stiffness | |
| gearings=[1.0, -0.6], # Joint3 moves 60% opposite to joint1 | |
| stiffness=100.0, # Moderate stiffness |
🤖 Prompt for AI Agents
In newton/examples/robot/example_robot_simple_tendon.py around lines 155 to 156,
the inline comment for gearing is incorrect: it says "Joint3 moves 80% with
joint2" but the gearing value is -0.6 (60% relative to joint1 and negative
direction). Update the comment to reflect the actual gearing (e.g., "Joint3
moves 60% in opposite direction relative to joint1" or similar), keeping it
concise and accurate.
| # The gearing is [1.0, 0.8], so we expect q3 ≈ 0.8 * q2 | ||
| if abs(q2) > 0.01: # Only check if joint2 has moved | ||
| ratio = q3 / q2 | ||
| print(f"Finger {finger}: q1={q1:.3f}, q2={q2:.3f}, q3={q3:.3f}, ratio={ratio:.3f}") |
There was a problem hiding this comment.
Fix the test verification logic.
The test checks if abs(q2) > 0.01 but then calculates ratio = q3 / q2. The comment about gearing [1.0, 0.8] doesn't match the actual gearing values used in the tendons, and the expected ratio calculation is incorrect.
The test should verify the actual tendon constraints:
- Tendon1:
joint1 * 1.0 + joint2 * (-0.8) = rest_length - Tendon2:
joint1 * 1.0 + joint3 * (-0.6) = rest_length
- # Verify that joint2 and joint3 maintain approximate ratio
- # The gearing is [1.0, 0.8], so we expect q3 ≈ 0.8 * q2
- if abs(q2) > 0.01: # Only check if joint2 has moved
- ratio = q3 / q2
- print(f"Finger {finger}: q1={q1:.3f}, q2={q2:.3f}, q3={q3:.3f}, ratio={ratio:.3f}")
+ # Verify that joints maintain tendon constraints
+ # Tendon1: q1 * 1.0 + q2 * (-0.8) = 0 (rest_length=0)
+ # Tendon2: q1 * 1.0 + q3 * (-0.6) = 0 (rest_length=0)
+ if abs(q1) > 0.01: # Only check if joint1 has moved
+ expected_q2 = q1 / 0.8 # From tendon1 constraint
+ expected_q3 = q1 / 0.6 # From tendon2 constraint
+ print(f"Finger {finger}: q1={q1:.3f}, q2={q2:.3f} (expected {expected_q2:.3f}), "
+ f"q3={q3:.3f} (expected {expected_q3:.3f})")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # The gearing is [1.0, 0.8], so we expect q3 ≈ 0.8 * q2 | |
| if abs(q2) > 0.01: # Only check if joint2 has moved | |
| ratio = q3 / q2 | |
| print(f"Finger {finger}: q1={q1:.3f}, q2={q2:.3f}, q3={q3:.3f}, ratio={ratio:.3f}") | |
| # Verify that joints maintain tendon constraints | |
| # Tendon1: q1 * 1.0 + q2 * (-0.8) = 0 (rest_length=0) | |
| # Tendon2: q1 * 1.0 + q3 * (-0.6) = 0 (rest_length=0) | |
| if abs(q1) > 0.01: # Only check if joint1 has moved | |
| expected_q2 = q1 / 0.8 # From tendon1 constraint | |
| expected_q3 = q1 / 0.6 # From tendon2 constraint | |
| print(f"Finger {finger}: q1={q1:.3f}, q2={q2:.3f} (expected {expected_q2:.3f}), " | |
| f"q3={q3:.3f} (expected {expected_q3:.3f})") |
| def test_import_single_joint_tendon(self): | ||
| """Test that tendons with only one joint are skipped during import.""" | ||
| # Create a USDA with a tendon that only has one joint | ||
| usda_content = """#usda 1.0 | ||
| ( | ||
| upAxis = "Z" | ||
| metersPerUnit = 1.0 | ||
| ) | ||
| def Xform "World" | ||
| { | ||
| def Xform "Articulation" ( | ||
| prepend apiSchemas = ["PhysicsArticulationRootAPI"] | ||
| ) | ||
| { | ||
| def Xform "Base" ( | ||
| prepend apiSchemas = ["PhysicsRigidBodyAPI", "PhysicsMassAPI"] | ||
| ) | ||
| { | ||
| float physics:mass = 0.0 | ||
| } | ||
| def Xform "Link" ( | ||
| prepend apiSchemas = ["PhysicsRigidBodyAPI", "PhysicsMassAPI"] | ||
| ) | ||
| { | ||
| double3 xformOp:translate = (0, 0, 1) | ||
| uniform token[] xformOpOrder = ["xformOp:translate"] | ||
| float physics:mass = 1.0 | ||
| float3 physics:diagonalInertia = (0.1, 0.1, 0.1) | ||
| def PhysicsRevoluteJoint "Joint" ( | ||
| prepend apiSchemas = ["PhysxTendonAxisRootAPI:invalid_tendon"] | ||
| ) | ||
| { | ||
| uniform token physics:axis = "X" | ||
| rel physics:body0 = </World/Articulation/Base> | ||
| rel physics:body1 = </World/Articulation/Link> | ||
| float physics:lowerLimit = -90 | ||
| float physics:upperLimit = 90 | ||
| # Single joint tendon - should be skipped | ||
| float[] physxTendon:invalid_tendon:gearing = [1.0] | ||
| float physxTendon:invalid_tendon:stiffness = 100.0 | ||
| } | ||
| } | ||
| } | ||
| } | ||
| """ | ||
|
|
||
| # Write to temporary file and import | ||
| # Create temporary file | ||
| fd, tmp_path = tempfile.mkstemp(suffix=".usda", text=True) | ||
| try: | ||
| # Write content and close file descriptor | ||
| with os.fdopen(fd, "w") as tmp: | ||
| tmp.write(usda_content) | ||
|
|
||
| # Import USD | ||
| builder = newton.ModelBuilder() | ||
| builder.add_usd(tmp_path, verbose=False) | ||
| model = builder.finalize() | ||
|
|
||
| # Should have no tendons since a tendon with only one joint is invalid | ||
| self.assertEqual(model.tendon_count, 0) | ||
| finally: | ||
| try: | ||
| os.unlink(tmp_path) | ||
| except Exception: | ||
| pass # Ignore errors on cleanup | ||
|
|
||
| def test_import_tendons_no_params(self): | ||
| """Test that tendons with missing parameters use defaults.""" |
There was a problem hiding this comment.
Gate test on USD availability.
This path uses builder.add_usd under the hood; skip when usd-core is missing.
- def test_import_single_joint_tendon(self):
+ @unittest.skipUnless(USD_AVAILABLE, "Requires usd-core")
+ def test_import_single_joint_tendon(self):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_import_single_joint_tendon(self): | |
| """Test that tendons with only one joint are skipped during import.""" | |
| # Create a USDA with a tendon that only has one joint | |
| usda_content = """#usda 1.0 | |
| ( | |
| upAxis = "Z" | |
| metersPerUnit = 1.0 | |
| ) | |
| def Xform "World" | |
| { | |
| def Xform "Articulation" ( | |
| prepend apiSchemas = ["PhysicsArticulationRootAPI"] | |
| ) | |
| { | |
| def Xform "Base" ( | |
| prepend apiSchemas = ["PhysicsRigidBodyAPI", "PhysicsMassAPI"] | |
| ) | |
| { | |
| float physics:mass = 0.0 | |
| } | |
| def Xform "Link" ( | |
| prepend apiSchemas = ["PhysicsRigidBodyAPI", "PhysicsMassAPI"] | |
| ) | |
| { | |
| double3 xformOp:translate = (0, 0, 1) | |
| uniform token[] xformOpOrder = ["xformOp:translate"] | |
| float physics:mass = 1.0 | |
| float3 physics:diagonalInertia = (0.1, 0.1, 0.1) | |
| def PhysicsRevoluteJoint "Joint" ( | |
| prepend apiSchemas = ["PhysxTendonAxisRootAPI:invalid_tendon"] | |
| ) | |
| { | |
| uniform token physics:axis = "X" | |
| rel physics:body0 = </World/Articulation/Base> | |
| rel physics:body1 = </World/Articulation/Link> | |
| float physics:lowerLimit = -90 | |
| float physics:upperLimit = 90 | |
| # Single joint tendon - should be skipped | |
| float[] physxTendon:invalid_tendon:gearing = [1.0] | |
| float physxTendon:invalid_tendon:stiffness = 100.0 | |
| } | |
| } | |
| } | |
| } | |
| """ | |
| # Write to temporary file and import | |
| # Create temporary file | |
| fd, tmp_path = tempfile.mkstemp(suffix=".usda", text=True) | |
| try: | |
| # Write content and close file descriptor | |
| with os.fdopen(fd, "w") as tmp: | |
| tmp.write(usda_content) | |
| # Import USD | |
| builder = newton.ModelBuilder() | |
| builder.add_usd(tmp_path, verbose=False) | |
| model = builder.finalize() | |
| # Should have no tendons since a tendon with only one joint is invalid | |
| self.assertEqual(model.tendon_count, 0) | |
| finally: | |
| try: | |
| os.unlink(tmp_path) | |
| except Exception: | |
| pass # Ignore errors on cleanup | |
| def test_import_tendons_no_params(self): | |
| """Test that tendons with missing parameters use defaults.""" | |
| @unittest.skipUnless(USD_AVAILABLE, "Requires usd-core") | |
| def test_import_single_joint_tendon(self): | |
| """Test that tendons with only one joint are skipped during import.""" | |
| # Create a USDA with a tendon that only has one joint | |
| usda_content = """#usda 1.0 | |
| ( | |
| upAxis = "Z" | |
| metersPerUnit = 1.0 | |
| ) | |
| def Xform "World" | |
| { | |
| def Xform "Articulation" ( | |
| prepend apiSchemas = ["PhysicsArticulationRootAPI"] | |
| ) | |
| { | |
| def Xform "Base" ( | |
| prepend apiSchemas = ["PhysicsRigidBodyAPI", "PhysicsMassAPI"] | |
| ) | |
| { | |
| float physics:mass = 0.0 | |
| } | |
| def Xform "Link" ( | |
| prepend apiSchemas = ["PhysicsRigidBodyAPI", "PhysicsMassAPI"] | |
| ) | |
| { | |
| double3 xformOp:translate = (0, 0, 1) | |
| uniform token[] xformOpOrder = ["xformOp:translate"] | |
| float physics:mass = 1.0 | |
| float3 physics:diagonalInertia = (0.1, 0.1, 0.1) | |
| def PhysicsRevoluteJoint "Joint" ( | |
| prepend apiSchemas = ["PhysxTendonAxisRootAPI:invalid_tendon"] | |
| ) | |
| { | |
| uniform token physics:axis = "X" | |
| rel physics:body0 = </World/Articulation/Base> | |
| rel physics:body1 = </World/Articulation/Link> | |
| float physics:lowerLimit = -90 | |
| float physics:upperLimit = 90 | |
| # Single joint tendon - should be skipped | |
| float[] physxTendon:invalid_tendon:gearing = [1.0] | |
| float physxTendon:invalid_tendon:stiffness = 100.0 | |
| } | |
| } | |
| } | |
| } | |
| """ | |
| # Write to temporary file and import | |
| # Create temporary file | |
| fd, tmp_path = tempfile.mkstemp(suffix=".usda", text=True) | |
| try: | |
| # Write content and close file descriptor | |
| with os.fdopen(fd, "w") as tmp: | |
| tmp.write(usda_content) | |
| # Import USD | |
| builder = newton.ModelBuilder() | |
| builder.add_usd(tmp_path, verbose=False) | |
| model = builder.finalize() | |
| # Should have no tendons since a tendon with only one joint is invalid | |
| self.assertEqual(model.tendon_count, 0) | |
| finally: | |
| try: | |
| os.unlink(tmp_path) | |
| except Exception: | |
| pass # Ignore errors on cleanup |
🤖 Prompt for AI Agents
In newton/tests/test_import_usd.py around lines 603 to 675, the test uses
builder.add_usd and should be skipped when the usd-core dependency is missing;
add a gate at the start of test_import_single_joint_tendon to skip the test if
usd-core isn't available (e.g., call pytest.importorskip with the usd-core
module name or attempt an import and call pytest.skip on ImportError), and
ensure pytest is imported at the top of the file if not already present.
| def test_import_tendons_no_params(self): | ||
| """Test that tendons with missing parameters use defaults.""" | ||
| # Create a USDA with minimal tendon parameters | ||
| usda_content = """#usda 1.0 | ||
| ( | ||
| upAxis = "Z" | ||
| metersPerUnit = 1.0 | ||
| ) | ||
| def Xform "World" | ||
| { | ||
| def Xform "Articulation" ( | ||
| prepend apiSchemas = ["PhysicsArticulationRootAPI"] | ||
| ) | ||
| { | ||
| def Xform "Body0" ( | ||
| prepend apiSchemas = ["PhysicsRigidBodyAPI", "PhysicsMassAPI"] | ||
| ) | ||
| { | ||
| float physics:mass = 0.0 | ||
| } | ||
| def Xform "Body1" ( | ||
| prepend apiSchemas = ["PhysicsRigidBodyAPI", "PhysicsMassAPI"] | ||
| ) | ||
| { | ||
| double3 xformOp:translate = (0, 0, 1) | ||
| uniform token[] xformOpOrder = ["xformOp:translate"] | ||
| float physics:mass = 1.0 | ||
| float3 physics:diagonalInertia = (0.1, 0.1, 0.1) | ||
| def PhysicsRevoluteJoint "Joint" ( | ||
| prepend apiSchemas = ["PhysxTendonAxisRootAPI:minimal_tendon"] | ||
| ) | ||
| { | ||
| uniform token physics:axis = "X" | ||
| rel physics:body0 = </World/Articulation/Body0> | ||
| rel physics:body1 = </World/Articulation/Body1> | ||
| float physics:lowerLimit = -90 | ||
| float physics:upperLimit = 90 | ||
| # Root joint with only gearing - no other parameters | ||
| float[] physxTendon:minimal_tendon:gearing = [1.0] | ||
| } | ||
| } | ||
| def Xform "Body2" ( | ||
| prepend apiSchemas = ["PhysicsRigidBodyAPI", "PhysicsMassAPI"] | ||
| ) | ||
| { | ||
| double3 xformOp:translate = (0, 0, 2) | ||
| uniform token[] xformOpOrder = ["xformOp:translate"] | ||
| float physics:mass = 1.0 | ||
| float3 physics:diagonalInertia = (0.1, 0.1, 0.1) | ||
| def PhysicsRevoluteJoint "Joint" ( | ||
| prepend apiSchemas = ["PhysxTendonAxisAPI:minimal_tendon"] | ||
| ) | ||
| { | ||
| uniform token physics:axis = "X" | ||
| rel physics:body0 = </World/Articulation/Body1> | ||
| rel physics:body1 = </World/Articulation/Body2> | ||
| float physics:lowerLimit = -90 | ||
| float physics:upperLimit = 90 | ||
| # Participant joint | ||
| float[] physxTendon:minimal_tendon:gearing = [-1.0] | ||
| } | ||
| } | ||
| } | ||
| } | ||
| """ | ||
|
|
||
| # Write to temporary file and import | ||
| # Create temporary file | ||
| fd, tmp_path = tempfile.mkstemp(suffix=".usda", text=True) | ||
| try: | ||
| # Write content and close file descriptor | ||
| with os.fdopen(fd, "w") as tmp: | ||
| tmp.write(usda_content) | ||
|
|
||
| # Import USD | ||
| builder = newton.ModelBuilder() | ||
| builder.add_usd(tmp_path, verbose=False) | ||
| model = builder.finalize() | ||
|
|
||
| # Should have one tendon | ||
| self.assertEqual(model.tendon_count, 1) | ||
|
|
||
| # Check that default parameters were used | ||
| params = model.tendon_params.numpy()[0] | ||
| self.assertAlmostEqual(params[0], 0.0, places=5) # default stiffness | ||
| self.assertAlmostEqual(params[1], 0.0, places=5) # default damping | ||
| self.assertAlmostEqual(params[2], 0.0, places=5) # default rest_length | ||
| self.assertTrue(np.isinf(params[3]) and params[3] < 0) # default lower_limit (-inf) | ||
| self.assertTrue(np.isinf(params[4]) and params[4] > 0) # default upper_limit (+inf) | ||
| finally: | ||
| try: | ||
| os.unlink(tmp_path) | ||
| except Exception: | ||
| pass # Ignore errors on cleanup | ||
|
|
There was a problem hiding this comment.
Also gate minimal-params tendon test on USD availability.
Prevents ImportError on environments without pxr.
- def test_import_tendons_no_params(self):
+ @unittest.skipUnless(USD_AVAILABLE, "Requires usd-core")
+ def test_import_tendons_no_params(self):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def test_import_tendons_no_params(self): | |
| """Test that tendons with missing parameters use defaults.""" | |
| # Create a USDA with minimal tendon parameters | |
| usda_content = """#usda 1.0 | |
| ( | |
| upAxis = "Z" | |
| metersPerUnit = 1.0 | |
| ) | |
| def Xform "World" | |
| { | |
| def Xform "Articulation" ( | |
| prepend apiSchemas = ["PhysicsArticulationRootAPI"] | |
| ) | |
| { | |
| def Xform "Body0" ( | |
| prepend apiSchemas = ["PhysicsRigidBodyAPI", "PhysicsMassAPI"] | |
| ) | |
| { | |
| float physics:mass = 0.0 | |
| } | |
| def Xform "Body1" ( | |
| prepend apiSchemas = ["PhysicsRigidBodyAPI", "PhysicsMassAPI"] | |
| ) | |
| { | |
| double3 xformOp:translate = (0, 0, 1) | |
| uniform token[] xformOpOrder = ["xformOp:translate"] | |
| float physics:mass = 1.0 | |
| float3 physics:diagonalInertia = (0.1, 0.1, 0.1) | |
| def PhysicsRevoluteJoint "Joint" ( | |
| prepend apiSchemas = ["PhysxTendonAxisRootAPI:minimal_tendon"] | |
| ) | |
| { | |
| uniform token physics:axis = "X" | |
| rel physics:body0 = </World/Articulation/Body0> | |
| rel physics:body1 = </World/Articulation/Body1> | |
| float physics:lowerLimit = -90 | |
| float physics:upperLimit = 90 | |
| # Root joint with only gearing - no other parameters | |
| float[] physxTendon:minimal_tendon:gearing = [1.0] | |
| } | |
| } | |
| def Xform "Body2" ( | |
| prepend apiSchemas = ["PhysicsRigidBodyAPI", "PhysicsMassAPI"] | |
| ) | |
| { | |
| double3 xformOp:translate = (0, 0, 2) | |
| uniform token[] xformOpOrder = ["xformOp:translate"] | |
| float physics:mass = 1.0 | |
| float3 physics:diagonalInertia = (0.1, 0.1, 0.1) | |
| def PhysicsRevoluteJoint "Joint" ( | |
| prepend apiSchemas = ["PhysxTendonAxisAPI:minimal_tendon"] | |
| ) | |
| { | |
| uniform token physics:axis = "X" | |
| rel physics:body0 = </World/Articulation/Body1> | |
| rel physics:body1 = </World/Articulation/Body2> | |
| float physics:lowerLimit = -90 | |
| float physics:upperLimit = 90 | |
| # Participant joint | |
| float[] physxTendon:minimal_tendon:gearing = [-1.0] | |
| } | |
| } | |
| } | |
| } | |
| """ | |
| # Write to temporary file and import | |
| # Create temporary file | |
| fd, tmp_path = tempfile.mkstemp(suffix=".usda", text=True) | |
| try: | |
| # Write content and close file descriptor | |
| with os.fdopen(fd, "w") as tmp: | |
| tmp.write(usda_content) | |
| # Import USD | |
| builder = newton.ModelBuilder() | |
| builder.add_usd(tmp_path, verbose=False) | |
| model = builder.finalize() | |
| # Should have one tendon | |
| self.assertEqual(model.tendon_count, 1) | |
| # Check that default parameters were used | |
| params = model.tendon_params.numpy()[0] | |
| self.assertAlmostEqual(params[0], 0.0, places=5) # default stiffness | |
| self.assertAlmostEqual(params[1], 0.0, places=5) # default damping | |
| self.assertAlmostEqual(params[2], 0.0, places=5) # default rest_length | |
| self.assertTrue(np.isinf(params[3]) and params[3] < 0) # default lower_limit (-inf) | |
| self.assertTrue(np.isinf(params[4]) and params[4] > 0) # default upper_limit (+inf) | |
| finally: | |
| try: | |
| os.unlink(tmp_path) | |
| except Exception: | |
| pass # Ignore errors on cleanup | |
| @unittest.skipUnless(USD_AVAILABLE, "Requires usd-core") | |
| def test_import_tendons_no_params(self): | |
| """Test that tendons with missing parameters use defaults.""" | |
| # Create a USDA with minimal tendon parameters | |
| usda_content = """#usda 1.0 | |
| ( | |
| upAxis = "Z" | |
| metersPerUnit = 1.0 | |
| ) | |
| def Xform "World" | |
| { | |
| def Xform "Articulation" ( | |
| prepend apiSchemas = ["PhysicsArticulationRootAPI"] | |
| ) | |
| { | |
| def Xform "Body0" ( | |
| prepend apiSchemas = ["PhysicsRigidBodyAPI", "PhysicsMassAPI"] | |
| ) | |
| { | |
| float physics:mass = 0.0 | |
| } | |
| def Xform "Body1" ( | |
| prepend apiSchemas = ["PhysicsRigidBodyAPI", "PhysicsMassAPI"] | |
| ) | |
| { | |
| double3 xformOp:translate = (0, 0, 1) | |
| uniform token[] xformOpOrder = ["xformOp:translate"] | |
| float physics:mass = 1.0 | |
| float3 physics:diagonalInertia = (0.1, 0.1, 0.1) | |
| def PhysicsRevoluteJoint "Joint" ( | |
| prepend apiSchemas = ["PhysxTendonAxisRootAPI:minimal_tendon"] | |
| ) | |
| { | |
| uniform token physics:axis = "X" | |
| rel physics:body0 = </World/Articulation/Body0> | |
| rel physics:body1 = </World/Articulation/Body1> | |
| float physics:lowerLimit = -90 | |
| float physics:upperLimit = 90 | |
| # Root joint with only gearing - no other parameters | |
| float[] physxTendon:minimal_tendon:gearing = [1.0] | |
| } | |
| } | |
| def Xform "Body2" ( | |
| prepend apiSchemas = ["PhysicsRigidBodyAPI", "PhysicsMassAPI"] | |
| ) | |
| { | |
| double3 xformOp:translate = (0, 0, 2) | |
| uniform token[] xformOpOrder = ["xformOp:translate"] | |
| float physics:mass = 1.0 | |
| float3 physics:diagonalInertia = (0.1, 0.1, 0.1) | |
| def PhysicsRevoluteJoint "Joint" ( | |
| prepend apiSchemas = ["PhysxTendonAxisAPI:minimal_tendon"] | |
| ) | |
| { | |
| uniform token physics:axis = "X" | |
| rel physics:body0 = </World/Articulation/Body1> | |
| rel physics:body1 = </World/Articulation/Body2> | |
| float physics:lowerLimit = -90 | |
| float physics:upperLimit = 90 | |
| # Participant joint | |
| float[] physxTendon:minimal_tendon:gearing = [-1.0] | |
| } | |
| } | |
| } | |
| } | |
| """ | |
| # Write to temporary file and import | |
| # Create temporary file | |
| fd, tmp_path = tempfile.mkstemp(suffix=".usda", text=True) | |
| try: | |
| # Write content and close file descriptor | |
| with os.fdopen(fd, "w") as tmp: | |
| tmp.write(usda_content) | |
| # Import USD | |
| builder = newton.ModelBuilder() | |
| builder.add_usd(tmp_path, verbose=False) | |
| model = builder.finalize() | |
| # Should have one tendon | |
| self.assertEqual(model.tendon_count, 1) | |
| # Check that default parameters were used | |
| params = model.tendon_params.numpy()[0] | |
| self.assertAlmostEqual(params[0], 0.0, places=5) # default stiffness | |
| self.assertAlmostEqual(params[1], 0.0, places=5) # default damping | |
| self.assertAlmostEqual(params[2], 0.0, places=5) # default rest_length | |
| self.assertTrue(np.isinf(params[3]) and params[3] < 0) # default lower_limit (-inf) | |
| self.assertTrue(np.isinf(params[4]) and params[4] > 0) # default upper_limit (+inf) | |
| finally: | |
| try: | |
| os.unlink(tmp_path) | |
| except Exception: | |
| pass # Ignore errors on cleanup |
🤖 Prompt for AI Agents
In newton/tests/test_import_usd.py around lines 674 to 775, the test assumes the
USD (pxr) Python bindings are present and will raise ImportError in environments
without pxr; update the test to skip when USD is unavailable by checking for the
pxr/newton USD import at the start of the test and calling self.skipTest("USD
not available") (or use unittest.skipUnless) if the import fails, so the test is
gated on USD availability rather than causing an ImportError.
| # Create MuJoCo solver | ||
| try: | ||
| solver = SolverMuJoCo(model, iterations=50, ls_iterations=50) | ||
| except ImportError as e: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Disable contacts to speed up and avoid stale-contact usage.
These tendon tests don’t rely on contacts; disabling them reduces flakiness and runtime.
- solver = SolverMuJoCo(model, iterations=50, ls_iterations=50)
+ solver = SolverMuJoCo(model, iterations=50, ls_iterations=50, disable_contacts=True)- solver = SolverMuJoCo(model, iterations=50, ls_iterations=50)
+ solver = SolverMuJoCo(model, iterations=50, ls_iterations=50, disable_contacts=True)Also applies to: 1514-1517
🤖 Prompt for AI Agents
In newton/tests/test_mujoco_solver.py around lines 1424-1427 (and similarly at
1514-1517), the MuJoCo solver is created without disabling contacts, which makes
tendon tests flaky and slower; before instantiating SolverMuJoCo disable
contacts (either by passing disable_contacts=True to the SolverMuJoCo
constructor if supported, or by calling the model API to turn off contacts prior
to solver creation — e.g., model.disable_contacts() or setting the model/contact
option that disables contacts), then create the solver as before so the tests
run faster and avoid stale-contact usage.
| solver = newton.solvers.SolverMuJoCo(model) | ||
|
|
There was a problem hiding this comment.
Skip gracefully when MuJoCo is unavailable (prevents hard failures).
Wrap solver creation in test_simple_tendon_constraint with ImportError handling like other tests.
- solver = newton.solvers.SolverMuJoCo(model)
+ try:
+ solver = newton.solvers.SolverMuJoCo(model, disable_contacts=True)
+ except ImportError as e:
+ self.skipTest(f"MuJoCo or deps not installed. Skipping test: {e}")
+ return📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| solver = newton.solvers.SolverMuJoCo(model) | |
| try: | |
| solver = newton.solvers.SolverMuJoCo(model, disable_contacts=True) | |
| except ImportError as e: | |
| self.skipTest(f"MuJoCo or deps not installed. Skipping test: {e}") | |
| return |
🤖 Prompt for AI Agents
In newton/tests/test_mujoco_solver.py around lines 1606-1607, the test currently
does solver = newton.solvers.SolverMuJoCo(model) which will hard-fail if MuJoCo
is not installed; wrap this creation in ImportError handling like the other
tests: attempt to construct the solver inside a try/except ImportError block and
call pytest.skip("MuJoCo unavailable") (or raise unittest.SkipTest) from the
except branch so the test is skipped gracefully when MuJoCo cannot be imported.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Blocking this until #974 is in. |
3 participants
Description
This PR adds support for parsing PhysX fixed tendons from USD files and plumbs them through to the MuJoCo solver. Issue #654. The implementation includes:
What's Done:
PhysxTendonAxisRootAPI,PhysxTendonAxisAPI) inimport_usd.pytest_import_usd.pyfor tendon parsing from USD filestest_mujoco_solver.pyto verify tendon functionality in simulationexample_robot_simple_tendon.pydemonstrating a 3-segment robot with coupled joints via tendonsextentsattribute instead ofextent, causing unit cubes to appear around modelsWhat's Not Done / Known Issues:
example_robot_shadowhand_tendon.pyis included but has stability issues:Technical Details:
sum(joint_pos[i] * gear[i]) = rest_lengthke), damping (kd), and rest length parametersFuture Work:
Newton Migration Guide
Please ensure the migration guide for warp.sim users is up-to-date with the changes made in this PR.
docs/migration.rstis up to dateBefore your PR is "Ready for review"
newton/tests/test_examples.py)pre-commit run -aSummary by CodeRabbit