ENH: Refactored VTK-to-USD class, workflow, and tutorial.#85
Merged
Conversation
Decouple VTK-to-USD workflow from file I/O; rename times_per_second to frames_per_second - WorkflowConvertVTKToUSD now takes in-memory PyVista/VTK meshes (input_meshes) with explicit usd_project_name/output_directory and optional time_codes/static_merge, replacing file-path input and regex-based time-series filename discovery. - Renamed times_per_second -> frames_per_second across ConvertVTKToUSD, ConversionSettings, CLI --fps, and workflows for clarity. - Renamed run() -> process() to match WorkflowConvertImageToUSD's convention. - download_data CLI no longer defaults to Slicer-Heart-CT; an explicit dataset argument is now required (prints help otherwise). - Updated docs, tutorials, experiments, and tests accordingly.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughThe PR migrates VTK-to-USD workflows from file-based inputs to in-memory PyVista/VTK meshes, renames animation-rate parameters, adds static-merge handling, changes dataset CLI behavior, and updates documentation, experiments, tutorials, and tests. ChangesVTK-to-USD migration
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant PyVista
participant WorkflowConvertVTKToUSD
participant ConvertVTKToUSD
participant USDStage
CLI->>PyVista: read VTK input meshes
PyVista->>WorkflowConvertVTKToUSD: provide input_meshes
WorkflowConvertVTKToUSD->>ConvertVTKToUSD: convert input_polydata
ConvertVTKToUSD->>USDStage: create USD scene and time samples
WorkflowConvertVTKToUSD->>USDStage: apply appearance
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Pull request overview
Refactors the VTK-to-USD conversion workflow to accept in-memory meshes (decoupling conversion from file I/O), aligns naming with other workflows (run() → process()), and renames times_per_second → frames_per_second across the library/CLIs/docs. It also changes download_data so an explicit dataset argument is required.
Changes:
WorkflowConvertVTKToUSDnow consumesinput_meshesand writes{usd_project_name}.usdintooutput_directory, with optionalstatic_merge/time_codes.- Renames
times_per_second→frames_per_secondacross converter settings, workflows, CLIs, tests, docs, and experiments. download_dataCLI no longer has an implicit default dataset; calling it with no args prints help and exits non-zero.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tutorials/tutorial_05_vtk_to_usd.py | Updates tutorial to read a mesh via PyVista and call workflow.process(). |
| tests/test_vtk_to_usd_library.py | Updates converter callsite to use frames_per_second. |
| tests/test_download_data_cli.py | Updates expectations for download_data when no args are provided. |
| tests/test_cli_smoke.py | Updates CLI smoke test expectations for frames_per_second. |
| src/physiotwin4d/workflow_convert_vtk_to_usd.py | Refactors workflow to accept in-memory meshes, adds static_merge/time_codes, renames to process(). |
| src/physiotwin4d/workflow_convert_image_to_usd.py | Renames workflow parameter to frames_per_second and forwards it. |
| src/physiotwin4d/vtk_to_usd/data_structures.py | Renames ConversionSettings.times_per_second to frames_per_second. |
| src/physiotwin4d/data_download_tools.py | Adds download progress/status print()s during dataset acquisition. |
| src/physiotwin4d/convert_vtk_to_usd.py | Adds static_merge/time_codes to constructor and renames FPS parameter to frames_per_second. |
| src/physiotwin4d/cli/download_data.py | Requires explicit dataset argument; prints help and exits when omitted. |
| src/physiotwin4d/cli/convert_vtk_to_usd.py | Updates CLI to load meshes in-memory and call workflow.process(), adds --static-merge, renames --fps plumbing. |
| src/physiotwin4d/cli/convert_image_to_usd.py | Renames --fps plumbing to frames_per_second. |
| experiments/Heart-GatedCT_To_USD/3-transform_dynamic_and_static_contours.py | Renames converter argument to frames_per_second. |
| experiments/Convert_VTK_To_USD/convert_chop_tpv25_valve_to_usd.py | Renames converter argument to frames_per_second. |
| experiments/Convert_VTK_To_USD/convert_chop_heart_vtk_to_usd.py | Updates workflow usage to in-memory meshes and process(). |
| experiments/Convert_VTK_To_USD/convert_chop_alterra_valve_to_usd.py | Renames converter argument to frames_per_second. |
| docs/tutorials.rst | Updates tutorial docs for in-memory meshes + process(). |
| docs/developer/usd_generation.rst | Renames times_per_second → frames_per_second in examples. |
| docs/cli_scripts/vtk_to_usd.rst | Documents ordered time series vs --static-merge; clarifies Python API inputs are meshes. |
| docs/cli_scripts/download_data.rst | Updates docs to reflect required dataset argument. |
| docs/cli_scripts/byod_tutorials.rst | Updates BYOD docs for new workflow/converter parameters and usage. |
| docs/architecture.rst | Updates workflow description to “in-memory meshes”. |
| docs/api/workflows.rst | Updates workflow API example to in-memory meshes + process(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/cli_scripts/byod_tutorials.rst (1)
46-53: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the
physiotwin4d-download-dataexample to pass a dataset name.docs/cli_scripts/byod_tutorials.rst:50-57The bare command only prints help and exits; usephysiotwin4d-download-data Slicer-Heart-CTso the surrounding text matches the actual behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/cli_scripts/byod_tutorials.rst` around lines 46 - 53, Update the `physiotwin4d-download-data` command example in the BYOD tutorial to pass the required dataset argument `Slicer-Heart-CT`, keeping the surrounding description consistent with the command’s actual behavior.
🧹 Nitpick comments (2)
src/physiotwin4d/data_download_tools.py (1)
75-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
logginginstead of
DataDownloadToolsis a utility class in a library module, not a standalone script. As per coding guidelines, Python code should use theloggingmodule instead oflogging.info(...)for these progress messages so callers can control log levels and output destinations.♻️ Suggested refactor: replace print with logging
+import logging + +_logger = logging.getLogger(__name__) + # ... in each method: - print(f"Downloaded {DataDownloadTools.SLICER_HEART_CT_FILENAME}") + _logger.info("Downloaded %s", DataDownloadTools.SLICER_HEART_CT_FILENAME)- print( - f"Downloaded {index:02d}.vtk ({index}/{DataDownloadTools.KCL_HEART_MODEL_MESH_COUNT})" - ) + _logger.info( + "Downloaded %02d.vtk (%d/%d)", + index, + index, + DataDownloadTools.KCL_HEART_MODEL_MESH_COUNT, + )- print("Downloaded average_mesh.vtk") + _logger.info("Downloaded average_mesh.vtk")- print(f"Downloaded {subdir_name} ({asset_name})") + _logger.info("Downloaded %s (%s)", subdir_name, asset_name)Also applies to: 126-128, 137-137, 215-215
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/physiotwin4d/data_download_tools.py` at line 75, Replace the download-progress print statements in DataDownloadTools with logging.info calls, including the occurrences around SLICER_HEART_CT_FILENAME and the other referenced download messages; import and use Python’s logging module so callers can configure output and log levels.Source: Coding guidelines
src/physiotwin4d/workflow_convert_vtk_to_usd.py (1)
31-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the missing constructor return annotation.
__init__needs-> Nonefor strict mypy compliance.Proposed fix
- ): + ) -> None:As per coding guidelines, “provide full type hints (
mypystrict / no untyped defs).”🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/physiotwin4d/workflow_convert_vtk_to_usd.py` around lines 31 - 50, Add the explicit -> None return annotation to the __init__ constructor shown in the diff, preserving all existing parameters and behavior to satisfy strict mypy typing.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/architecture.rst`:
- Around line 87-89: Update the workflow wrapper description in the architecture
documentation to state that it converts in-memory PyVista/VTK meshes to both
static and animated USD scenes, while retaining the distinction from the
advanced primitives in the physiotwin4d.vtk_to_usd package.
In `@src/physiotwin4d/cli/convert_vtk_to_usd.py`:
- Around line 200-210: Validate args.output_usd before constructing
WorkflowConvertVTKToUSD, requiring its suffix to be exactly “.usd” and rejecting
unsupported extensions with a clear CLI error; keep the existing
Path.stem/output-directory behavior aligned with this constraint.
- Around line 202-205: The exception handler surrounding `pv.read()` in the CLI
conversion flow should catch the file-loading errors it can raise, including
`OSError`, `FileNotFoundError`, and `TypeError`, in addition to `ValueError.
Update the `try` block around `input_meshes` construction and preserve the
existing CLI error handling so malformed or unreadable VTK inputs do not produce
an uncaught traceback.
In `@src/physiotwin4d/data_download_tools.py`:
- Around line 126-128: In the download progress output within DataDownloadTools,
reformat the overlong f-string to comply with the 88-character limit, for
example by splitting the expression across lines while preserving the existing
message.
In `@src/physiotwin4d/workflow_convert_vtk_to_usd.py`:
- Around line 117-122: The time code selection in the workflow conversion logic
incorrectly treats an explicit empty list as missing. In the time_codes
initialization within ConvertVTKToUSD, replace the truthiness fallback with an
explicit None check so generated codes are used only when time_codes is None,
while [] is preserved for length validation.
---
Outside diff comments:
In `@docs/cli_scripts/byod_tutorials.rst`:
- Around line 46-53: Update the `physiotwin4d-download-data` command example in
the BYOD tutorial to pass the required dataset argument `Slicer-Heart-CT`,
keeping the surrounding description consistent with the command’s actual
behavior.
---
Nitpick comments:
In `@src/physiotwin4d/data_download_tools.py`:
- Line 75: Replace the download-progress print statements in DataDownloadTools
with logging.info calls, including the occurrences around
SLICER_HEART_CT_FILENAME and the other referenced download messages; import and
use Python’s logging module so callers can configure output and log levels.
In `@src/physiotwin4d/workflow_convert_vtk_to_usd.py`:
- Around line 31-50: Add the explicit -> None return annotation to the __init__
constructor shown in the diff, preserving all existing parameters and behavior
to satisfy strict mypy typing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 54899e8b-b282-4a73-b5b1-b8fd003f633b
📒 Files selected for processing (23)
docs/api/workflows.rstdocs/architecture.rstdocs/cli_scripts/byod_tutorials.rstdocs/cli_scripts/download_data.rstdocs/cli_scripts/vtk_to_usd.rstdocs/developer/usd_generation.rstdocs/tutorials.rstexperiments/Convert_VTK_To_USD/convert_chop_alterra_valve_to_usd.pyexperiments/Convert_VTK_To_USD/convert_chop_heart_vtk_to_usd.pyexperiments/Convert_VTK_To_USD/convert_chop_tpv25_valve_to_usd.pyexperiments/Heart-GatedCT_To_USD/3-transform_dynamic_and_static_contours.pysrc/physiotwin4d/cli/convert_image_to_usd.pysrc/physiotwin4d/cli/convert_vtk_to_usd.pysrc/physiotwin4d/cli/download_data.pysrc/physiotwin4d/convert_vtk_to_usd.pysrc/physiotwin4d/data_download_tools.pysrc/physiotwin4d/vtk_to_usd/data_structures.pysrc/physiotwin4d/workflow_convert_image_to_usd.pysrc/physiotwin4d/workflow_convert_vtk_to_usd.pytests/test_cli_smoke.pytests/test_download_data_cli.pytests/test_vtk_to_usd_library.pytutorials/tutorial_05_vtk_to_usd.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Decouple VTK-to-USD workflow from file I/O; rename times_per_second to frames_per_second
Summary by CodeRabbit