Skip to content

Raise warning about orphan joints in USD importer#1480

Merged
eric-heiden merged 4 commits into
newton-physics:mainfrom
eric-heiden:usd-orphan-joints
Jan 29, 2026
Merged

Raise warning about orphan joints in USD importer#1480
eric-heiden merged 4 commits into
newton-physics:mainfrom
eric-heiden:usd-orphan-joints

Conversation

@eric-heiden

@eric-heiden eric-heiden commented Jan 29, 2026

Copy link
Copy Markdown
Member

Raise a warning if joints are parsed from USD but no ArticulationRoot prim was found.

Summary by CodeRabbit

  • Bug Fixes
    • Detects and reports orphan joints when joints are present but no articulation root is found. A consolidated warning lists the orphan joint keys and advises how to proceed or skip joint validation.

✏️ Tip: You can customize this high-level summary in your review settings.

Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
@coderabbitai

coderabbitai Bot commented Jan 29, 2026

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

Adds detection and consolidated warning for orphan joints during USD import when joints are parsed but no articulation root exists; updates the test to expect that warning. No public signatures changed; changes are limited to messaging and tests.

Changes

Cohort / File(s) Summary
USD import logic
newton/_src/utils/import_usd.py
Collects parsed joints into an orphan_joints list when no articulation root is found and emits a single consolidated UserWarning advising ModelBuilder.finalize(skip_validation_joints=True) where appropriate.
Tests
newton/tests/test_import_usd.py
Updates test to assert that a UserWarning is emitted containing the orphan-joints message while keeping existing functional assertions (joint/body counts).

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Suggested reviewers

  • kaloca
🚥 Pre-merge checks | ✅ 2 | ❌ 1
❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly and clearly describes the main change: adding a warning when orphan joints are encountered during USD import without an articulation root.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings

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

❤️ Share

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

Signed-off-by: Eric Heiden <eric-heiden@outlook.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR adds a warning to the USD importer to alert users when joints are found without any articulation root. The warning helps users understand that they need to either add an articulation root prim or bypass joint validation when finalizing the model.

Changes:

  • Added a warning in the USD importer when orphan joints (joints without an articulation root) are detected
  • Updated the existing test to verify that the warning is properly raised with the expected message

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
newton/_src/utils/import_usd.py Added orphan_joints list tracking and warning when non-articulated joints are found
newton/tests/test_import_usd.py Updated test_import_non_articulated_joints to assert that a warning is raised with the correct message
Comments suppressed due to low confidence (1)

newton/_src/utils/import_usd.py:1358

  • The loop does not check for only_load_enabled_joints before adding joints to orphan_joints. According to the has_joints check at line 1329 and parse_joint function at line 514, disabled joints should be filtered when only_load_enabled_joints is True. This inconsistency means that disabled joints may be counted in the warning message even though they weren't actually parsed.

Add a check similar to line 1329: if not joint_desc.jointEnabled and only_load_enabled_joints: continue

        for joint_key, joint_desc in joint_descriptions.items():
            if any(re.match(p, joint_key) for p in ignore_paths):
                continue
            if str(joint_desc.body0) in ignored_body_paths or str(joint_desc.body1) in ignored_body_paths:
                continue

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread newton/_src/utils/import_usd.py Outdated
Comment thread newton/_src/utils/import_usd.py Outdated
@eric-heiden eric-heiden self-assigned this Jan 29, 2026
@codecov

codecov Bot commented Jan 29, 2026

Copy link
Copy Markdown

Codecov Report

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

📢 Thoughts on this report? Let us know!

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@newton/_src/utils/import_usd.py`:
- Around line 1373-1374: The warning message built in import_usd.py is
suggesting the wrong parameter value; update the text that references
ModelBuilder.finalize to recommend calling
ModelBuilder.finalize(skip_validation_joints=True) instead of False so it
correctly tells users to skip joint validation to avoid the ValueError for
orphan joints—look for the warn_str construction around ModelBuilder.finalize
and adjust the parameter text accordingly.
🧹 Nitpick comments (1)
newton/_src/utils/import_usd.py (1)

1359-1364: Consider moving the append inside the try block for accuracy.

Currently, orphan_joints.append(joint_key) executes regardless of whether parse_joint succeeds or raises a ValueError. This means joints that failed to parse are still counted in the warning message that says they "were parsed."

If you want to count only successfully parsed joints, move the append inside the try block. If the current behavior is intentional (counting all attempted joints), consider rewording the warning from "were parsed" to "were found" or "were encountered."

♻️ Option A: Count only successfully parsed joints
             try:
                 parse_joint(joint_desc, incoming_xform=incoming_world_xform)
+                orphan_joints.append(joint_key)
             except ValueError as exc:
                 if verbose:
                     print(f"Skipping joint {joint_key}: {exc}")
-            orphan_joints.append(joint_key)
♻️ Option B: Reword the warning message for accuracy
         if len(orphan_joints) > 0:
             warn_str = (
-                f"No articulation was found but {len(orphan_joints)} joints were parsed: [{', '.join(orphan_joints)}]. "
+                f"No articulation was found but {len(orphan_joints)} joints were encountered: [{', '.join(orphan_joints)}]. "
             )

Comment thread newton/_src/utils/import_usd.py Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 1

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)

1359-1364: Bug: Joints that fail to parse are still counted as "parsed".

The orphan_joints.append(joint_key) call at line 1364 is outside the try-except block, meaning joints that raise ValueError during parse_joint() are still added to the orphan list. The warning message then incorrectly claims these joints "were parsed" when they actually failed.

Move the append inside the try block to only count successfully parsed joints.

🐛 Proposed fix
         try:
             parse_joint(joint_desc, incoming_xform=incoming_world_xform)
+            orphan_joints.append(joint_key)
         except ValueError as exc:
             if verbose:
                 print(f"Skipping joint {joint_key}: {exc}")
-        orphan_joints.append(joint_key)
🤖 Fix all issues with AI agents
In `@newton/_src/utils/import_usd.py`:
- Around line 1373-1374: The warning string in import_usd.py incorrectly tells
users to call ModelBuilder.finalize(skip_validation_joints=False) which would
not skip validation; update the message built in the warn_str construction to
instruct calling ModelBuilder.finalize(skip_validation_joints=True) instead so
it matches the intended behavior and the test (see test_import_usd.py) and
refers to the ModelBuilder.finalize method name in the text.

Comment thread newton/_src/utils/import_usd.py Outdated
eric-heiden and others added 2 commits January 28, 2026 22:10
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
@eric-heiden eric-heiden marked this pull request as ready for review January 29, 2026 06:11
@eric-heiden eric-heiden added this pull request to the merge queue Jan 29, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jan 29, 2026
@eric-heiden eric-heiden added this pull request to the merge queue Jan 29, 2026
Merged via the queue into newton-physics:main with commit 6453ea7 Jan 29, 2026
22 checks passed
@coderabbitai coderabbitai Bot mentioned this pull request Feb 5, 2026
4 tasks
mmacklin pushed a commit to mmacklin/newton that referenced this pull request Apr 7, 2026
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants