Preserve Blender conversion paths and failures - #7280
Conversation
Greptile SummaryThe PR makes Blender mesh conversion preserve whitespace-containing paths and propagate conversion failures.
Confidence Score: 5/5The PR appears safe to merge with no actionable correctness or security issues identified. The new argument-list invocation preserves each path as a literal subprocess argument, avoids shell interpretation, and intentionally propagates Blender lookup and conversion failures with regression coverage. Important Files Changed
Reviews (1): Last reviewed commit: "Preserve paths in Blender conversion sub..." | Re-trigger Greptile |
There was a problem hiding this comment.
Isaac Lab Review Bot
The Blender conversion helper now constructs argv directly so executable, script, input, and output paths containing whitespace remain intact, while subprocess failures are propagated through check=True. The regression tests cover both argument preservation and the missing-executable path.
- Design and architecture: Keeping invocation construction and failure handling in
run_blender_convert2objremains consistent with the script’s existing call flow. The test’s repository-root traversal is somewhat coupled to directory layout but is a non-blocking tradeoff for testing a standalone repository script. - API: The helper signature, CLI arguments, defaults, and output-path behavior remain unchanged. Failed Blender conversions now deliberately propagate instead of being silently ignored, and a missing executable produces an explicit
FileNotFoundError. - Implementation: The
parse_cli_argstoconvert_meshestorun_blender_convert2objpath was traced. Every path is passed as one argv element,check=Truestops processing after conversion failure, and the tests patch the exact module globals and subprocess function used. Adding a test beneathsource/isaaclab/testdoes not by itself establish that theisaaclabsource package changed or requires a package changelog fragment.
No blocking issues. No inline issue met the actionable-evidence threshold; the assessment above records the review feedback.
Automated review; human maintainers own approval decisions.
There was a problem hiding this comment.
AI review disclosure: This review was produced with assistance from multiple AI systems.
Thank you for putting this PR together.
Assessment: Argument-list invocation preserves whitespace and check=True correctly propagates conversion failures.
Findings: Managed isaaclab tests are touched without a required changelog fragment.
Requested changes: Add source/isaaclab/changelog.d/.skip and run the changelog gate.
Test scope: Coverage is focused and sufficient.
Documentation / AI skills: No additional documentation update is required: no public CLI syntax or documented workflow changed. No AI-skill update is required.
Description
Fixes Blender subprocess invocation in
scripts/tools/process_meshes_to_obj.py.The helper previously constructed the command as one formatted string and then called
.split(" "). Any valid Blender, input, output, or script path containing whitespace was therefore split into multiple command-line arguments.The subprocess was also launched without
check=True, so a failed Blender conversion could be silently treated as success by the wrapper.The helper now builds the subprocess argument list directly, preserves each path as one argument, requests
check=True, and raises a clear error when Blender cannot be found onPATH.Type of change
Validation
check=Trueis used.source/<pkg>/package.Checklist