Skip to content

Preserve Blender conversion paths and failures - #7280

Open
sylvesterkaczmarek wants to merge 2 commits into
isaac-sim:developfrom
sylvesterkaczmarek:fix/process-meshes-blender-args
Open

Preserve Blender conversion paths and failures#7280
sylvesterkaczmarek wants to merge 2 commits into
isaac-sim:developfrom
sylvesterkaczmarek:fix/process-meshes-blender-args

Conversation

@sylvesterkaczmarek

Copy link
Copy Markdown

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 on PATH.

Type of change

  • Bug fix

Validation

  • Added a unit regression test with Blender, input, and output paths containing spaces.
  • The test verifies each path remains one subprocess argument and check=True is used.
  • Added coverage for the missing-Blender error path.
  • No Blender installation is required by the tests.
  • No package changelog fragment is required because this changes a repository script, not a source/<pkg>/ package.

Checklist

  • I have read and understood the contribution guidelines
  • I have added regression tests
  • No new dependencies

@sylvesterkaczmarek
sylvesterkaczmarek requested a review from a team August 21, 2026 16:31
@github-actions github-actions Bot added bug Something isn't working isaac-lab Related to Isaac Lab team labels Aug 21, 2026
@greptile-apps

greptile-apps Bot commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

The PR makes Blender mesh conversion preserve whitespace-containing paths and propagate conversion failures.

  • Builds the Blender command as an argument list instead of splitting a formatted string.
  • Reports a missing Blender executable clearly and enables subprocess exit-status checking.
  • Adds unit coverage for whitespace-containing paths and missing-Blender behavior.

Confidence Score: 5/5

The 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

Filename Overview
scripts/tools/process_meshes_to_obj.py Safely replaces whitespace-sensitive command splitting with literal argv construction and surfaces missing executables and failed conversions.
source/isaaclab/test/tools/test_process_meshes_to_obj.py Adds focused unit regression coverage using a dynamically loaded script and mocked subprocess invocation.

Reviews (1): Last reviewed commit: "Preserve paths in Blender conversion sub..." | Re-trigger Greptile

@isaaclab-review-bot isaaclab-review-bot 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.

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_convert2obj remains 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_args to convert_meshes to run_blender_convert2obj path was traced. Every path is passed as one argv element, check=True stops processing after conversion failure, and the tests patch the exact module globals and subprocess function used. Adding a test beneath source/isaaclab/test does not by itself establish that the isaaclab source 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.

@AntoineRichard AntoineRichard left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working isaac-lab Related to Isaac Lab team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants