Raise warning about orphan joints in USD importer#1480
Conversation
Signed-off-by: Eric Heiden <eric-heiden@outlook.com>
📝 WalkthroughWalkthroughAdds 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
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 <eric-heiden@outlook.com>
There was a problem hiding this comment.
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.
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
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 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 whetherparse_jointsucceeds or raises aValueError. 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
tryblock. 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)}]. " )
There was a problem hiding this comment.
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 raiseValueErrorduringparse_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.
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>
Signed-off-by: Eric Heiden <eric-heiden@outlook.com> Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Raise a warning if joints are parsed from USD but no ArticulationRoot prim was found.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.