ENH: Add low-level VTK-to-USD facade and clarify API boundary#46
ENH: Add low-level VTK-to-USD facade and clarify API boundary#46aylward merged 3 commits intoProject-MONAI:mainfrom
Conversation
Add vtk_to_usd.convert_vtk_file() as the stable advanced file-conversion facade, while keeping ConvertVTKToUSD as the in-repo API for experiments, workflows, and tests. Refactor VTK-to-USD tests to validate behavior through ConvertVTKToUSD, replace experiment diagnostics with ConvertVTKToUSD.inspect_file(), and update docs/API map to remove stale converter APIs.
|
Warning Rate limit exceeded
To continue reviewing without waiting, purchase usage credits in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (9)
WalkthroughThis PR replaces the prior VTK-to-USD converter surface with a new ConvertVTKToUSD-centered API and adds a low-level file facade ChangesVTK-to-USD API Refactor
Sequence DiagramsequenceDiagram
participant User
participant HighLevel as ConvertVTKToUSD<br/>(High-Level)
participant Facade as converter.py<br/>(convert_vtk_file)
participant VTK as VTK Reader
participant Material as MaterialManager
participant USD as USD Stage
User->>HighLevel: from_files(data_basename, vtk_files, time_codes...)
activate HighLevel
HighLevel->>VTK: read VTK files (read_vtk_file)
VTK-->>HighLevel: mesh data / arrays
HighLevel->>HighLevel: prepare mesh/time sampling, materials
HighLevel-->>User: configured converter instance
deactivate HighLevel
User->>HighLevel: convert(output_usd_file)
activate HighLevel
HighLevel->>USD: create stage, set metadata (meters/upAxis)
HighLevel->>Material: ensure/create material
Material-->>HighLevel: material prim
HighLevel->>USD: create mesh prim(s), write primvars/time samples
HighLevel->>USD: save stage
USD-->>HighLevel: stage saved
HighLevel-->>User: USD stage
deactivate HighLevel
alt Direct file facade
User->>Facade: convert_vtk_file(vtk_file, output_usd_file, ...)
activate Facade
Facade->>VTK: read_vtk_file(vtk_file)
VTK-->>Facade: mesh data
Facade->>Material: prepare/ensure material
Material-->>Facade: material prim
Facade->>USD: create stage and mesh prim, write, save
USD-->>Facade: stage
Facade-->>User: USD stage
deactivate Facade
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #46 +/- ##
==========================================
+ Coverage 20.06% 22.11% +2.04%
==========================================
Files 45 46 +1
Lines 6214 6268 +54
==========================================
+ Hits 1247 1386 +139
+ Misses 4967 4882 -85
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Add vtk_to_usd.convert_vtk_file() as the stable advanced low-level single-file conversion facade while keeping ConvertVTKToUSD as the in-repo API for experiments, workflows, and tests. Add ConvertVTKToUSD.inspect_file() for public diagnostics, update experiments to use it, refactor tests to exercise conversion through ConvertVTKToUSD, and refresh docs/API map to remove stale converter APIs. Handle empty mesh inspection without raising.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/api/usd/tetmesh.rst (1)
1-21:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAll three files—
polymesh.rst,tetmesh.rst, andvtk_conversion.rst—document the same class with duplicate.. autoclass:: ConvertVTKToUSDdirectives.This causes Sphinx to emit duplicate object description warnings. Additionally, only
vtk_conversion.rstis listed inindex.rsttoctree;polymesh.rstandtetmesh.rstare orphaned from the main navigation.Consolidate API documentation in one location. Either:
- Delete
polymesh.rstandtetmesh.rst, keep onlyvtk_conversion.rst- Or replace autoclass directives in
polymesh.rstandtetmesh.rstwith a single-sentence note +:doc:link tovtk_conversion.rstfor backwards compatibility🤖 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/api/usd/tetmesh.rst` around lines 1 - 21, The three files (polymesh.rst, tetmesh.rst, vtk_conversion.rst) all declare the same autoclass ConvertVTKToUSD causing duplicate-object warnings; fix by consolidating docs: either delete polymesh.rst and tetmesh.rst and keep the canonical vtk_conversion.rst, or edit polymesh.rst and tetmesh.rst to remove the autoclass:: ConvertVTKToUSD block and replace it with a single-sentence notice that points readers to vtk_conversion.rst via a :doc: link (and if you keep the stub files, ensure index.rst/toctree includes them for navigation).
🧹 Nitpick comments (1)
tests/test_vtk_to_usd_library.py (1)
38-42: 💤 Low value
check_valve4d_data()appears unused in this file.It's defined alongside
check_kcl_heart_data()but isn't referenced by any fixture or test in this module. If it was intentionally kept for parity with the experiment scripts or anticipated valve4d tests, please add a brief comment; otherwise consider removing it to keep the test helpers tight.🤖 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 `@tests/test_vtk_to_usd_library.py` around lines 38 - 42, The helper function check_valve4d_data() is unused in the test module; either remove it to keep helpers minimal or document why it’s retained (e.g., placeholder for future Valve4D tests) and ensure it is correct if kept — fix the variable name bug (alterra_dir vs alterra_dir/alterra typo) inside check_valve4d_data() and add a one-line comment above it explaining its purpose if you choose to keep it, otherwise delete the function; reference: check_valve4d_data() and check_kcl_heart_data().
🤖 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 `@README.md`:
- Around line 351-356: The example uses ConversionSettings with
meters_per_unit=1.0 but doesn't clarify units; update the snippet so it either
keeps the library default (meters_per_unit=0.001) to reflect the mm→m
conversion, or keep 1.0 but change the inline comment to explicitly state
"source mesh authored in meters (no scaling)" — adjust the example around
ConversionSettings and the meters_per_unit parameter accordingly so users won't
accidentally scale typical medical (mm) meshes 1000×.
In `@src/physiomotion4d/vtk_to_usd/README.md`:
- Around line 12-17: The README currently claims "VTK point and cell arrays as
USD primvars" while the low-level API (ConversionSettings.preserve_cell_arrays)
is still unimplemented; update the README entry to only advertise supported
functionality: change the line to indicate only VTK point arrays are exported as
USD primvars and either remove or explicitly mark cell-array primvars as "not
yet supported / coming soon." Reference the symbol
ConversionSettings.preserve_cell_arrays to guide future enabling of the feature
once implemented and keep the Materials/Coordinates lines unchanged.
---
Outside diff comments:
In `@docs/api/usd/tetmesh.rst`:
- Around line 1-21: The three files (polymesh.rst, tetmesh.rst,
vtk_conversion.rst) all declare the same autoclass ConvertVTKToUSD causing
duplicate-object warnings; fix by consolidating docs: either delete polymesh.rst
and tetmesh.rst and keep the canonical vtk_conversion.rst, or edit polymesh.rst
and tetmesh.rst to remove the autoclass:: ConvertVTKToUSD block and replace it
with a single-sentence notice that points readers to vtk_conversion.rst via a
:doc: link (and if you keep the stub files, ensure index.rst/toctree includes
them for navigation).
---
Nitpick comments:
In `@tests/test_vtk_to_usd_library.py`:
- Around line 38-42: The helper function check_valve4d_data() is unused in the
test module; either remove it to keep helpers minimal or document why it’s
retained (e.g., placeholder for future Valve4D tests) and ensure it is correct
if kept — fix the variable name bug (alterra_dir vs alterra_dir/alterra typo)
inside check_valve4d_data() and add a one-line comment above it explaining its
purpose if you choose to keep it, otherwise delete the function; reference:
check_valve4d_data() and check_kcl_heart_data().
🪄 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: 097c7242-279b-4060-9e15-7b0479d6b9ae
📒 Files selected for processing (20)
README.mddocs/API_MAP.mddocs/api/usd/index.rstdocs/api/usd/polymesh.rstdocs/api/usd/tetmesh.rstdocs/developer/architecture.rstdocs/developer/extending.rstdocs/developer/usd_generation.rstdocs/examples.rstdocs/quickstart.rstexperiments/Convert_VTK_To_USD/convert_chop_alterra_valve_to_usd.pyexperiments/Convert_VTK_To_USD/convert_chop_tpv25_valve_to_usd.pyexperiments/Convert_VTK_To_USD/convert_vtk_to_usd_using_class.pysrc/physiomotion4d/convert_vtk_to_usd.pysrc/physiomotion4d/vtk_to_usd/CLAUDE.mdsrc/physiomotion4d/vtk_to_usd/README.mdsrc/physiomotion4d/vtk_to_usd/__init__.pysrc/physiomotion4d/vtk_to_usd/converter.pysrc/physiomotion4d/vtk_to_usd/vtk_reader.pytests/test_vtk_to_usd_library.py
| # Advanced: custom settings and material | ||
| settings = ConversionSettings( | ||
| triangulate_meshes=True, | ||
| compute_normals=True, | ||
| meters_per_unit=0.001, # mm to meters | ||
| meters_per_unit=1.0, # coordinates are authored in meters | ||
| times_per_second=60.0, |
There was a problem hiding this comment.
Clarify the unit-scaling example before users copy it.
ConversionSettings.meters_per_unit is the VTK→USD scale factor, and the library’s default path is the mm→m conversion validated by tests/test_vtk_to_usd_library.py. Setting this to 1.0 here without explicitly saying the source mesh is already in meters will make typical medical meshes 1000× too large. Either keep the default 0.001 in the example or call out that 1.0 is only for meter-authored inputs.
📝 Suggested doc tweak
settings = ConversionSettings(
triangulate_meshes=True,
compute_normals=True,
- meters_per_unit=1.0, # coordinates are authored in meters
+ meters_per_unit=0.001, # typical VTK medical meshes are authored in millimeters
times_per_second=60.0,
)Or, if you want to keep 1.0, make the comment explicit:
- meters_per_unit=1.0, # coordinates are authored in meters
+ meters_per_unit=1.0, # only when the source VTK coordinates are already in meters📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # Advanced: custom settings and material | |
| settings = ConversionSettings( | |
| triangulate_meshes=True, | |
| compute_normals=True, | |
| meters_per_unit=0.001, # mm to meters | |
| meters_per_unit=1.0, # coordinates are authored in meters | |
| times_per_second=60.0, | |
| settings = ConversionSettings( | |
| triangulate_meshes=True, | |
| compute_normals=True, | |
| meters_per_unit=0.001, # typical VTK medical meshes are authored in millimeters | |
| times_per_second=60.0, |
🤖 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 `@README.md` around lines 351 - 356, The example uses ConversionSettings with
meters_per_unit=1.0 but doesn't clarify units; update the snippet so it either
keeps the library default (meters_per_unit=0.001) to reflect the mm→m
conversion, or keep 1.0 but change the inline comment to explicitly state
"source mesh authored in meters (no scaling)" — adjust the example around
ConversionSettings and the meters_per_unit parameter accordingly so users won't
accidentally scale typical medical (mm) meshes 1000×.
Remove duplicate ConvertVTKToUSD API pages, clean stale navigation links, clarify VTK-to-USD scaling and primvar documentation, and remove an unused Valve4D test helper.
1 participant
Add vtk_to_usd.convert_vtk_file() as the stable advanced file-conversion facade, while keeping ConvertVTKToUSD as the in-repo API for experiments, workflows, and tests.
Refactor VTK-to-USD tests to validate behavior through ConvertVTKToUSD, replace experiment diagnostics with ConvertVTKToUSD.inspect_file(), and update docs/API map to remove stale converter APIs.
Summary by CodeRabbit