Handle reversed joints in USD #1392
Conversation
Signed-off-by: Eric Heiden <[email protected]>
Signed-off-by: Eric Heiden <[email protected]>
Signed-off-by: Eric Heiden <[email protected]>
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughUndirected topological sorting was added to the USD import pipeline; parse_usd now records per-joint reversal and propagates joint path maps through replication. Topology utilities were generalized and exported, tests for reversed joints were added, and docstrings/API docs were updated. Changes
Sequence Diagram(s)sequenceDiagram
participant USD as "USD File"
participant Parser as "parse_usd()"
participant Topo as "topological_sort_undirected()"
participant JointProc as "Joint Processor"
participant Builder as "ModelBuilder"
USD->>Parser: load bodies, shapes, joints
Parser->>Topo: provide joint edges (undirected)
Topo->>Topo: traverse graph -> joint_order + reversed_joints
Topo-->>Parser: return joint_order, reversed_joints
Parser->>JointProc: process each joint in joint_order with reverse flag
JointProc->>JointProc: if reversed -> validate type & adjust_axis (negate)
JointProc-->>Parser: processed joint descriptors
Parser->>Builder: add bodies/shapes/joints with corrected axes/order
Parser-->>Parser: record path_body_map, path_shape_map, path_joint_map, reversed_joint_ids
Parser->>Caller: return parsed data and maps
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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. Comment |
Signed-off-by: Eric Heiden <[email protected]>
Signed-off-by: Eric Heiden <[email protected]>
There was a problem hiding this comment.
Pull request overview
This pull request implements support for handling reversed joints in USD import, where physics:body0 relates to the joint child and physics:body1 relates to the joint parent. The PR addresses this by detecting reversed joints during topological sorting and appropriately swapping parent/child transforms and negating joint axes for supported joint types (revolute, prismatic, and fixed). For unsupported joint types (D6, spherical), the code raises an error.
Changes:
- Added a new
topological_sort_undirectedfunction that treats the joint graph as undirected and detects which joints need to be reversed during traversal - Modified USD import logic to handle reversed joints by swapping transforms and negating axes for supported joint types
- Updated documentation to clarify joint axis and transform frame conventions
- Added tests to verify correct handling of reversed joints and appropriate error raising for unsupported types
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
newton/_src/utils/topology.py |
Added new topological_sort_undirected function with type annotations for handling undirected joint graphs and tracking reversed edges |
newton/_src/utils/import_usd.py |
Modified parse_joint to handle reversed joints, added axis adjustment logic, updated documentation for return values including reversed_joint_ids |
newton/_src/sim/builder.py |
Updated documentation to clarify joint transforms and axes are defined in joint parent anchor frame |
newton/_src/sim/articulation.py |
Updated docstring for reconstruct_angular_q_qd to clarify frame conventions |
newton/_src/utils/__init__.py |
Exported new topological_sort_undirected function |
newton/tests/test_import_usd.py |
Added tests for reversed joint handling, updated existing tests to account for new joint ordering behavior, modified mass calculation test to sort before comparing |
docs/api/newton_utils.rst |
Added documentation entries for new utilities |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@newton/_src/sim/builder.py`:
- Around line 307-308: Update the documentation to use consistent frame wording:
change the field comment for ModelBuilder.joint_axis to match the new inline
docstring that says the axis is expressed in the joint parent anchor frame (or
alternatively update the inline docstring if you intend the axis to be in the
child joint frame). Locate the stored field comment for ModelBuilder.joint_axis
and the assignment using self.axis = wp.normalize(axis_to_vec3(axis)) and ensure
both describe the same frame (explicitly mention "joint parent anchor frame" if
keeping the new docstring), so the docstring and field comment are reconciled.
♻️ Duplicate comments (1)
newton/_src/utils/import_usd.py (1)
1271-1338:reversed_joint_idsshould store global joint indices (not local joint_edges indices).
Right nowreversed_joint_idsis extended with indices intojoint_edges. The return contract and downstream checks expect global builder joint indices. This can silently mislabel joints, especially once a free joint is inserted or when articulations are stacked. Populatereversed_joint_idsat the point whereparse_jointreturns the actual builder joint index (and consider offsetting duringcloned_worldreplication as well).🐛 Proposed fix (record global joint indices)
- reversed_joint_ids.extend(reversed_joint_list) reversed_joints_set = set(reversed_joint_list) @@ if joint is not None: articulation_joint_indices.append(joint) + if i in reversed_joints_set: + reversed_joint_ids.append(joint)
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@docs/api/newton_utils.rst`:
- Around line 12-15: The autosummary in docs/api/newton_utils.rst lists internal
symbols that are not exported from the public newton.utils API (MeshAdjacency,
MeshEdge, bourke_color_map, copy_rgb_frame_uint8, solidify_mesh); either remove
those names from the autosummary or add public re-exports in the newton.utils
package (e.g., import and expose these symbols from their internal modules in
newton.utils.__init__ or the package's public module) so that importing
newton.utils.<symbol> succeeds; update the docs or the public re-export code
accordingly to ensure the documented symbols match the actual public API.
In `@newton/_src/utils/__init__.py`:
- Around line 23-24: Add topological_sort_undirected to the public API surface:
in newton/utils.py, update the import from newton._src.utils to include
topological_sort_undirected alongside topological_sort, and add
"topological_sort_undirected" to the module's __all__ list so it is re-exported
for external users; ensure the name matches the function in
newton._src.utils.topology.
In `@newton/_src/utils/import_usd.py`:
- Line 153: Remove the unused noqa suppression on the import of
topological_sort_undirected in import_usd.py: replace "from .topology import
topological_sort_undirected # noqa: PLC0415 (circular import)" with a normal
import and, if desired, a plain comment like "# circular import" but without the
"noqa: PLC0415" tag so Ruff no longer reports an unused suppression; ensure the
import remains exactly "from .topology import topological_sort_undirected" (or
equivalent) to preserve behavior.
♻️ Duplicate comments (1)
newton/_src/utils/import_usd.py (1)
1243-1281: Populatereversed_joint_idswith actual builder joint indices.
reversed_joint_ids.extend(reversed_joint_list)records local indices intojoint_edges, which won’t matchModelBuilderjoint indices and can include skipped joints. Append the createdjointindex when the joint was reversed instead.🛠️ Proposed fix
- reversed_joint_ids.extend(reversed_joint_list) reversed_joints_set = set(reversed_joint_list) @@ - if joint is not None: - articulation_joint_indices.append(joint) + if joint is not None: + articulation_joint_indices.append(joint) + if i in reversed_joints_set: + reversed_joint_ids.append(joint)Also applies to: 1322-1338, 1755-1774
🧹 Nitpick comments (2)
newton/tests/test_import_usd.py (2)
269-321: Well-structured test for reversed joint axis negation.The test correctly verifies that when a USD joint has its body0/body1 reversed relative to the articulation tree, the joint axis is negated appropriately.
Minor: The
noqa: PLC0415directive at line 272 appears to be unused per static analysis. Consider removing it if the linter doesn't require it.
1848-1891: Verify the parent/child validation logic for edge cases.The logic handles normal and reversed joints, but I want to highlight the control flow for clarity:
- Normal match (lines 1872-1873): Passes silently when actual matches expected.
- Swapped match (lines 1874-1881): Allows swapping only for FIXED, REVOLUTE, and PRISMATIC joints; fails for other types.
- Mismatch with reversed flag (lines 1882-1886): Fails if the joint was expected to be reversed but indices don't match.
- General mismatch (lines 1887-1891): Fails for any other case.
One consideration: On line 1882, when
j in reversed_joint_ids, the assertion failure message references "expected reversed parent/child indices" but the actual comparison being made is against(expected_child, expected_parent). This seems inverted from what the message suggests—the check is that actual values should equal(expected_child, expected_parent), but this condition is only reached when they don't match the swapped expectation (since line 1874 already passed through).This logic appears correct but is complex. Consider adding a brief inline comment explaining the control flow.
Signed-off-by: Eric Heiden <[email protected]>
…le-joint-articulation
Signed-off-by: Eric Heiden <[email protected]>
Signed-off-by: Eric Heiden <[email protected]>
Signed-off-by: Eric Heiden <[email protected]>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3b608cd9a1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@newton/_src/utils/import_usd.py`:
- Around line 1338-1340: reversed_joint_ids is computed against original joint
indices and goes stale after index remaps from collapse_fixed_joints() and
cloned-world replication; change the logic to record reversed_joint_paths (store
each joint's unique scene/path when you append to articulation_joint_indices and
reversed_joint_ids), then after any index-remapping step (e.g., inside
collapse_fixed_joints and the clone/replication routine) rebuild
reversed_joint_ids by resolving those stored paths to the new indices and
applying any clone offset to the resolved index; update code references where
reversed_joint_ids is created (the block appending to articulation_joint_indices
and reversed_joint_ids) and where remaps occur so the rebuild runs immediately
after remapping.
♻️ Duplicate comments (1)
newton/_src/utils/import_usd.py (1)
153-153: Remove unusednoqasuppression.Ruff already flags this as an unused suppression.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
newton/_src/utils/import_usd.py (1)
1699-1754:reversed_joint_idsnot propagated to cloned worlds.When
cloned_worldis used,path_joint_mapis correctly updated with offset indices for replicated worlds (lines 1728-1730), butreversed_joint_idsremains unchanged. This means the list only contains joint indices from the first world, making it incorrect for replicated worlds.Consider offsetting
reversed_joint_idsin the clone loop:🔧 Suggested fix
for path, joint_id in path_joint_map.items(): new_path = path.replace(cloned_world, world_path) path_joint_map_updates[new_path] = joint_id + joint_count + # Add these lines after the path_joint_map loop: + reversed_joint_ids_updates = [] + for jid in reversed_joint_ids: + reversed_joint_ids_updates.append(jid + joint_count)And after the loop:
path_joint_map = path_joint_map_updates + reversed_joint_ids.extend(reversed_joint_ids_updates)
♻️ Duplicate comments (1)
newton/_src/utils/import_usd.py (1)
153-153: Remove unusednoqadirective.The
# noqa: PLC0415suppression is unused as indicated by static analysis. Keep a plain comment if documenting the circular import is desired.🧹 Suggested fix
- from .topology import topological_sort_undirected # noqa: PLC0415 + from .topology import topological_sort_undirected # circular import
🧹 Nitpick comments (1)
newton/_src/utils/import_usd.py (1)
828-843: Minor inconsistency:adjust_axisapplied only to angular axes in D6 joints.The
adjust_axishelper is applied to angular axes but not to linear axes (line 779). This is currently harmless since D6 joints are excluded from reversed joint support (raising an error at line 559). However, if D6 support is added in the future, both linear and angular axes would need consistent handling.
Signed-off-by: Eric Heiden <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@newton/_src/utils/import_usd.py`:
- Around line 1243-1251: The code currently raises a ValueError when any
reversed joint is detected in topological_sort_undirected (reversed_joint_list),
which blocks supported types; instead, collect the reversed joints into a set
(e.g., reversed_joint_set populated from reversed_joint_list using joint_names
or joint ids) and remove the ValueError block so import continues; then pass
that reversed_joint_set into parse_joint(...) so parse_joint can swap
parent/child ids and transforms, negate axes for revolute/prismatic as needed,
and only raise for truly unsupported types (D6/spherical/distance). Ensure
references to topological_sort_undirected,
reversed_joint_list/reversed_joint_paths/reversed_joint_names, and parse_joint
are updated accordingly.
In `@newton/tests/test_import_usd.py`:
- Around line 269-320: The test test_reversed_joints_in_articulation_raise is
asserting a ValueError for reversed revolute joints but the PR intends to
support reversed body0/body1 for revolute/prismatic/fixed joints; update the
test to call ModelBuilder.add_usd() expecting success (no exception), then
inspect the resulting model to verify the joint at "/World/Articulation/Joint1"
was imported with body0/body1 swapped as needed (e.g., child/parent relationship
adjusted) and that the joint axis/sign was corrected when required; reference
the ModelBuilder.add_usd method and the joint named "/World/Articulation/Joint1"
when implementing the assertions.
♻️ Duplicate comments (1)
newton/_src/utils/import_usd.py (1)
148-148: Remove unusednoqaon local import.
Ruff flags this as an unused suppression.🧹 Proposed tidy-up
-from .topology import topological_sort_undirected # noqa: PLC0415 +from .topology import topological_sort_undirected
camevor
left a comment
camevor
left a comment
There was a problem hiding this comment.
Thanks, looks good to me! I added one comment about an edge case.
| if reversed_joint_list: | ||
| reversed_joint_paths = [joint_names[joint_id] for joint_id in reversed_joint_list] | ||
| reversed_joint_names = ", ".join(reversed_joint_paths) | ||
| raise ValueError( |
There was a problem hiding this comment.
One special case arises for fixed joints to world that are reversed, which are swapped silently in resolve_joint_parent_child at line 515, which would prevent the raise here.
There was a problem hiding this comment.
Thanks for flagging this! I've added another test (test_reversed_fixed_root_joint_to_world_is_allowed) to double check this and we do still support these silently reversed root joints. This is because the joint_edges list we use for determining the reversed joints via topological_sort_undirected has already the correct [parent, child] ordering (thanks to calling resolve_joint_parent_child to fill in the joint edges) for these root joints, so there is no further reversal necessary.
Signed-off-by: Eric Heiden <[email protected]>
2 participants
In some cases the
physics:body0relates to the joint child andphysics:body1related to the joint parent.Allowing flipped joints (body0 being the topological child and body1 being the parent) would need runtime handling in several places: Joint targets would have to be transformed to keep the semantics of body0 → body1 and and body parent forces would also have to be inverted and reported at the parent link COM (alternatively, this could happen in the selection API, but this would introduce even more confusion).
Fixes #700.
Fixes #1377.
Summary by CodeRabbit
New Features
Documentation
Tests
✏️ Tip: You can customize this high-level summary in your review settings.