ENH:VTK_To_USD and ConvertVTKToUSD consolidation #44
ENH:VTK_To_USD and ConvertVTKToUSD consolidation #44aylward wants to merge 3 commits intoProject-MONAI:mainfrom
Conversation
- Add from_files() classmethod to ConvertVTKToUSD, accepting file paths and
loading via pv.read(); adds separate_by, solid_color, static_merge, time_codes
- Add _convert_static_merge() method for multi-file static scenes
- Add _convert_unified() splitting logic via split_mesh_data_by_connectivity /
split_mesh_data_by_cell_type imported from vtk_to_usd internals
- Rewrite WorkflowConvertVTKToUSD.run() to call ConvertVTKToUSD.from_files()
exclusively; update post-processing scan path from /World/Meshes to
/World/{mesh_name}
- Delete vtk_to_usd/converter.py (VTKToUSDConverter and helpers removed)
- Remove VTKToUSDConverter / convert_vtk_file / convert_vtk_sequence from
vtk_to_usd/__init__.py exports; update module docstring
- Remove vtk_to_usd from physiomotion4d top-level __init__.py public API
- Update three experiment scripts to use ConvertVTKToUSD.from_files()
- Rewrite test_vtk_to_usd_library.py tests to use ConvertVTKToUSD; update
all prim-path assertions from /World/Meshes/X to /World/X/Mesh
- Regenerate docs/API_MAP.md
https://claude.ai/code/session_01B16tgwah5XemvxzrGpzUZj
WalkthroughThe VTK→USD conversion API was refactored: the low-level Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
experiments/Convert_VTK_To_USD/convert_chop_alterra_valve_to_usd.py (1)
163-169: Avoid hardcoding a connectivity ordinal in the demo path.
AlterraValve_object3depends on the connected-component enumeration order, so a small mesh/preprocessing change can break the colormap step even when conversion succeeded. It would be safer to discover mesh paths under/World/AlterraValveand pick the target dynamically.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@experiments/Convert_VTK_To_USD/convert_chop_alterra_valve_to_usd.py` around lines 163 - 169, The code hardcodes a connectivity ordinal into vessel_path (e.g., "/World/AlterraValve/AlterraValve_object3"), which is brittle; update the logic around the separate_by check (where vessel_path is assigned) to list children under "/World/AlterraValve" at runtime and choose the appropriate prim dynamically (for example, select the child whose name startswith "AlterraValve_object" or matches the connectivity target, or fall back to a Mesh prim), so replace the fixed string assignments for vessel_path with a discovery step that finds the correct prim path based on existing children before proceeding.tests/test_vtk_to_usd_library.py (1)
329-347: Assert the requested diffuse color, not just material existence.This test will still pass if
solid_coloris ignored and a default material is created/bound. Please read the bound shader input and compare it to(0.9, 0.3, 0.3)so the new API path actually exercises the feature under test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_vtk_to_usd_library.py` around lines 329 - 347, The test currently only checks that a material exists and is bound but doesn't verify the diffuse color; update the assertions after ComputeBoundMaterial() to fetch the bound material's shader (via UsdShade.Shader on the bound_material prim or material_prim), read the relevant input (e.g., "diffuseColor" or the shader input name used by ConvertVTKToUSD for solid_color), convert the returned value to an RGB triple and assert it equals (0.9, 0.3, 0.3); use the existing symbols ConvertVTKToUSD.from_files, binding_api/ComputeBoundMaterial(), bound_material, and material_prim to locate where to add the shader input read and the color equality assertion.experiments/Convert_VTK_To_USD/convert_chop_tpv25_valve_to_usd.py (1)
162-168: Make the selected mesh path resilient to connectivity reordering.
TPV25Valve_object4is not a stable identifier; it comes from component enumeration order. If the extracted surface changes slightly, this notebook can fail in the post-processing step while the conversion itself is still correct.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@experiments/Convert_VTK_To_USD/convert_chop_tpv25_valve_to_usd.py` around lines 162 - 168, Hardcoded vessel path using "TPV25Valve_object4" is brittle; instead compute vessel_path dynamically from the USD stage by locating the child prim under "/World/TPV25Valve" that matches a stable criterion (e.g., prim type == "Mesh" or prim name contains the part/basename variable) based on the existing variables separate_by and part_name; update the logic that sets vessel_path (the block referencing separate_by and vessel_path) to query the stage's children and pick the intended prim name rather than assuming "TPV25Valve_object4", falling back to "/World/TPV25Valve/Mesh" if no match is found.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/physiomotion4d/convert_vtk_to_usd.py`:
- Around line 183-199: Validate time_codes in from_files before assigning to
instance: ensure that when time_codes is not None its length equals len(meshes)
and its values are sorted in non-decreasing order (so stage start <= end and
frame indexing won't break); if the checks fail raise a clear ValueError. After
validation assign resolved_time_codes to instance._time_codes as before so
_convert_unified() can assume correct length and ordering.
- Around line 552-559: label_time_codes currently uses self._time_codes (full
per-frame list) even when label_mesh_sequence only contains meshes for a subset
of frames, which misaligns samples; instead, derive label_time_codes to match
only the frames present in label_mesh_sequence: if self._time_codes is None,
keep the existing fallback (float range of len(label_mesh_sequence)); otherwise
build label_time_codes by selecting entries from self._time_codes corresponding
to the frames that produced the meshes in label_mesh_sequence (i.e., filter
using the same indices/condition used to populate label_mesh_sequence) before
calling mesh_converter.create_time_varying_mesh with bind_material=True.
In `@src/physiomotion4d/workflow_convert_vtk_to_usd.py`:
- Around line 176-186: The call to ConvertVTKToUSD.from_files in
workflow_convert_vtk_to_usd.py is dropping the up_axis and triangulate options
advertised by WorkflowConvertVTKToUSD.__init__; either forward these parameters
into ConvertVTKToUSD.from_files (pass up_axis=self.up_axis and
triangulate=self.triangulate) so the converter respects the requested
axes/triangulation, or if ConvertVTKToUSD.from_files does not support them yet,
fail fast by raising a clear error when up_axis or triangulate are set to
non-defaults; update the call site (ConvertVTKToUSD.from_files) accordingly and
keep references to up_axis and triangulate consistent with the
WorkflowConvertVTKToUSD constructor.
---
Nitpick comments:
In `@experiments/Convert_VTK_To_USD/convert_chop_alterra_valve_to_usd.py`:
- Around line 163-169: The code hardcodes a connectivity ordinal into
vessel_path (e.g., "/World/AlterraValve/AlterraValve_object3"), which is
brittle; update the logic around the separate_by check (where vessel_path is
assigned) to list children under "/World/AlterraValve" at runtime and choose the
appropriate prim dynamically (for example, select the child whose name
startswith "AlterraValve_object" or matches the connectivity target, or fall
back to a Mesh prim), so replace the fixed string assignments for vessel_path
with a discovery step that finds the correct prim path based on existing
children before proceeding.
In `@experiments/Convert_VTK_To_USD/convert_chop_tpv25_valve_to_usd.py`:
- Around line 162-168: Hardcoded vessel path using "TPV25Valve_object4" is
brittle; instead compute vessel_path dynamically from the USD stage by locating
the child prim under "/World/TPV25Valve" that matches a stable criterion (e.g.,
prim type == "Mesh" or prim name contains the part/basename variable) based on
the existing variables separate_by and part_name; update the logic that sets
vessel_path (the block referencing separate_by and vessel_path) to query the
stage's children and pick the intended prim name rather than assuming
"TPV25Valve_object4", falling back to "/World/TPV25Valve/Mesh" if no match is
found.
In `@tests/test_vtk_to_usd_library.py`:
- Around line 329-347: The test currently only checks that a material exists and
is bound but doesn't verify the diffuse color; update the assertions after
ComputeBoundMaterial() to fetch the bound material's shader (via UsdShade.Shader
on the bound_material prim or material_prim), read the relevant input (e.g.,
"diffuseColor" or the shader input name used by ConvertVTKToUSD for
solid_color), convert the returned value to an RGB triple and assert it equals
(0.9, 0.3, 0.3); use the existing symbols ConvertVTKToUSD.from_files,
binding_api/ComputeBoundMaterial(), bound_material, and material_prim to locate
where to add the shader input read and the color equality assertion.
🪄 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: 496c0958-d258-4c0f-8ffa-dfe6b6fa07cb
📒 Files selected for processing (10)
docs/API_MAP.mdexperiments/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/__init__.pysrc/physiomotion4d/convert_vtk_to_usd.pysrc/physiomotion4d/vtk_to_usd/__init__.pysrc/physiomotion4d/vtk_to_usd/converter.pysrc/physiomotion4d/workflow_convert_vtk_to_usd.pytests/test_vtk_to_usd_library.py
💤 Files with no reviewable changes (2)
- src/physiomotion4d/init.py
- src/physiomotion4d/vtk_to_usd/converter.py
| converter = ConvertVTKToUSD.from_files( | ||
| data_basename=self.mesh_name, | ||
| vtk_files=paths_ordered, | ||
| extract_surface=self.extract_surface, | ||
| separate_by=separate_by, | ||
| times_per_second=self.times_per_second, | ||
| solid_color=self.solid_color, | ||
| time_codes=time_codes if not is_static_merge else None, | ||
| static_merge=is_static_merge, | ||
| log_level=self.log_level, | ||
| ) |
There was a problem hiding this comment.
Don't silently drop up_axis and triangulate here.
WorkflowConvertVTKToUSD.__init__() still advertises these knobs, but this new path no longer applies them. With the current ConvertVTKToUSD implementation, callers asking for up_axis="Z" or triangulate=False will get Y-up + triangulated output anyway.
At minimum, fail fast until support is restored
+ if self.up_axis != "Y":
+ raise ValueError(
+ "WorkflowConvertVTKToUSD currently only supports up_axis='Y'"
+ )
+ if not self.triangulate:
+ raise ValueError(
+ "WorkflowConvertVTKToUSD currently requires triangulate=True"
+ )
+
converter = ConvertVTKToUSD.from_files(
data_basename=self.mesh_name,
vtk_files=paths_ordered,📝 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.
| converter = ConvertVTKToUSD.from_files( | |
| data_basename=self.mesh_name, | |
| vtk_files=paths_ordered, | |
| extract_surface=self.extract_surface, | |
| separate_by=separate_by, | |
| times_per_second=self.times_per_second, | |
| solid_color=self.solid_color, | |
| time_codes=time_codes if not is_static_merge else None, | |
| static_merge=is_static_merge, | |
| log_level=self.log_level, | |
| ) | |
| if self.up_axis != "Y": | |
| raise ValueError( | |
| "WorkflowConvertVTKToUSD currently only supports up_axis='Y'" | |
| ) | |
| if not self.triangulate: | |
| raise ValueError( | |
| "WorkflowConvertVTKToUSD currently requires triangulate=True" | |
| ) | |
| converter = ConvertVTKToUSD.from_files( | |
| data_basename=self.mesh_name, | |
| vtk_files=paths_ordered, | |
| extract_surface=self.extract_surface, | |
| separate_by=separate_by, | |
| times_per_second=self.times_per_second, | |
| solid_color=self.solid_color, | |
| time_codes=time_codes if not is_static_merge else None, | |
| static_merge=is_static_merge, | |
| log_level=self.log_level, | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/physiomotion4d/workflow_convert_vtk_to_usd.py` around lines 176 - 186,
The call to ConvertVTKToUSD.from_files in workflow_convert_vtk_to_usd.py is
dropping the up_axis and triangulate options advertised by
WorkflowConvertVTKToUSD.__init__; either forward these parameters into
ConvertVTKToUSD.from_files (pass up_axis=self.up_axis and
triangulate=self.triangulate) so the converter respects the requested
axes/triangulation, or if ConvertVTKToUSD.from_files does not support them yet,
fail fast by raising a clear error when up_axis or triangulate are set to
non-defaults; update the call site (ConvertVTKToUSD.from_files) accordingly and
keep references to up_axis and triangulate consistent with the
WorkflowConvertVTKToUSD constructor.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #44 +/- ##
==========================================
+ Coverage 13.53% 14.05% +0.52%
==========================================
Files 46 45 -1
Lines 6412 6202 -210
==========================================
+ Hits 868 872 +4
+ Misses 5544 5330 -214
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:
|
There was a problem hiding this comment.
Pull request overview
This PR consolidates VTK→USD conversion on the ConvertVTKToUSD API by removing the older VTKToUSDConverter interface and updating workflows/tests/experiments to use the unified converter and its new prim path layout.
Changes:
- Migrates conversion call sites (workflow, tests, experiment scripts) to
ConvertVTKToUSD.from_files(...).convert(...). - Removes the
src/physiomotion4d/vtk_to_usd/converter.pyhigh-level converter and stops exporting it fromphysiomotion4d.vtk_to_usd. - Updates documentation/API map and test expectations for the new USD prim paths (rooted at
/World/{data_basename}).
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/test_vtk_to_usd_library.py | Updates tests to use ConvertVTKToUSD and asserts new prim/material paths. |
| src/physiomotion4d/workflow_convert_vtk_to_usd.py | Switches workflow conversion to ConvertVTKToUSD.from_files and updates post-processing root path. |
| src/physiomotion4d/vtk_to_usd/converter.py | Removes the deprecated high-level converter implementation. |
| src/physiomotion4d/vtk_to_usd/init.py | Re-scopes vtk_to_usd as internal plumbing and removes public exports of the old converter API. |
| src/physiomotion4d/convert_vtk_to_usd.py | Adds from_files, splitting options (separate_by), static-merge mode, and solid color handling. |
| src/physiomotion4d/init.py | Stops re-exporting vtk_to_usd at the package top-level. |
| experiments/Convert_VTK_To_USD/convert_vtk_to_usd_using_class.py | Updates example script to use ConvertVTKToUSD and modern PyVista-based time-series creation. |
| experiments/Convert_VTK_To_USD/convert_chop_tpv25_valve_to_usd.py | Migrates conversion to ConvertVTKToUSD.from_files and updates expected prim paths. |
| experiments/Convert_VTK_To_USD/convert_chop_alterra_valve_to_usd.py | Migrates conversion to ConvertVTKToUSD.from_files and updates expected prim paths. |
| docs/API_MAP.md | Regenerates API map entries to reflect removed/added APIs and shifted line numbers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| label_time_codes = self._time_codes or [ | ||
| float(i) for i in range(len(label_mesh_sequence)) | ||
| ] | ||
| for md in label_mesh_sequence: | ||
| md.material_id = material.name |
There was a problem hiding this comment.
When labels are missing in some frames, label_mesh_sequence can be shorter than the original series, but label_time_codes uses self._time_codes unchanged. That can make create_time_varying_mesh() receive sequences with mismatched lengths. Build per-label time codes aligned to the frames actually included (similar to how _convert_unified() collects part_time_codes).
| ) | ||
| for i, vtk_mesh in enumerate(self.input_polydata): | ||
| mesh_data = self._vtk_to_mesh_data(vtk_mesh, i) | ||
| frame_name = f"Mesh_{i}" |
There was a problem hiding this comment.
In static-merge mode, the per-file prim base name is hard-coded as Mesh_{i}, which ignores data_basename and makes the resulting prim names less informative/inconsistent with the data_basename contract. Consider using something derived from self.data_basename (e.g., {data_basename}_{i}) so merged scenes are self-describing and stable across workflows.
| frame_name = f"Mesh_{i}" | |
| frame_name = f"{self.data_basename}_{i}" |
| converter = ConvertVTKToUSD.from_files( | ||
| data_basename=self.mesh_name, | ||
| vtk_files=paths_ordered, | ||
| extract_surface=self.extract_surface, | ||
| separate_by=separate_by, |
There was a problem hiding this comment.
WorkflowConvertVTKToUSD still exposes/stores up_axis and triangulate, but the new ConvertVTKToUSD.from_files(...) path doesn’t pass these through (and ConvertVTKToUSD.convert() currently hard-codes Y-up and always triangulates via its internal settings). Either remove these workflow parameters (and update the docstring) or plumb them through into ConvertVTKToUSD so the workflow options remain effective.
| instance = cls( | ||
| data_basename=data_basename, | ||
| input_polydata=meshes, | ||
| mask_ids=mask_ids, | ||
| separate_by=separate_by, |
There was a problem hiding this comment.
from_files() accepts extract_surface, but that choice isn’t propagated to the instance’s convert_to_surface setting. Since ConvertVTKToUSD.__init__ defaults convert_to_surface=True, calling from_files(..., extract_surface=False) will still extract surfaces later in _vtk_to_mesh_data(), so the flag can’t actually disable surface extraction. Consider passing convert_to_surface=extract_surface into cls(...) (or setting instance.convert_to_surface = extract_surface) and avoid doing extraction twice in two different places.
| resolved_time_codes = ( | ||
| time_codes | ||
| if time_codes is not None | ||
| else [float(i) for i in range(len(meshes))] | ||
| ) |
There was a problem hiding this comment.
time_codes isn’t validated against vtk_files length. If a caller passes a list with a different length than the number of meshes, later indexing in _convert_unified() will raise (or silently misalign samples). Add an explicit length check when time_codes is not None and raise a clear ValueError if it doesn’t match len(vtk_files).
…nment Validate time_codes length and non-decreasing order in from_files(); propagate extract_surface to convert_to_surface so surfaces are not extracted twice; align per-label time codes to the frames actually present rather than using the full series; use data_basename in static-merge prim names; remove dead up_axis/triangulate from WorkflowConvertVTKToUSD and its CLI; discover vessel prim dynamically in experiment scripts; add diffuseColor assertion to material test.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
experiments/Convert_VTK_To_USD/convert_chop_alterra_valve_to_usd.py (1)
166-181: Arbitrary component selection for colormap target.
candidates[-1]andtriangle_paths[0]implicitly rely on the ordering ofsplit_mesh_data_by_connectivity/split_mesh_data_by_cell_type. If those ordering heuristics change (e.g., ordering by component size vs. by first-encountered vertex), the colormap will silently land on a different sub-prim. Consider sorting deterministically (by point count or by name index) and adding a short log line of the chosenvessel_pathso regressions are easy to spot.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@experiments/Convert_VTK_To_USD/convert_chop_alterra_valve_to_usd.py` around lines 166 - 181, The current selection of vessel_path uses non-deterministic picks (candidates[-1], triangle_paths[0]) which can change if split_mesh_data_by_connectivity or split_mesh_data_by_cell_type alter ordering; update the logic in the branch that uses usd_tools.list_mesh_paths_under to deterministically choose a sub-prim (for example, sort candidates/triangle_paths by a stable key like prim name index or by point/vertex count obtained from the prim metadata) and set vessel_path to that sorted-first/last element instead of relying on list order, and add a short processLogger.info/processLogger.debug line that logs the chosen vessel_path so future regressions are easy to detect (refer to the variables separate_by, candidates, triangle_paths, vessel_path and the helper functions split_mesh_data_by_connectivity/split_mesh_data_by_cell_type).src/physiomotion4d/convert_vtk_to_usd.py (1)
190-195:extract_surfacebranch only handlesUnstructuredGrid.
pv.read()can also returnStructuredGrid,ImageData, orRectilinearGrid(all listed as supported insupports_mesh_type). None have.faces, so they will fall through unchanged and later fail in_vtk_to_mesh_data()with the"Mesh has no faces"error rather than being surfaced.Broaden the surface-extraction condition
- if extract_surface and isinstance(mesh, pv.UnstructuredGrid): - mesh = mesh.extract_surface(algorithm="dataset_surface") + if extract_surface and isinstance( + mesh, + (pv.UnstructuredGrid, pv.StructuredGrid, pv.ImageData, pv.RectilinearGrid), + ): + mesh = mesh.extract_surface()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/physiomotion4d/convert_vtk_to_usd.py` around lines 190 - 195, The extract_surface branch currently only handles pv.UnstructuredGrid which leaves StructuredGrid/ImageData/RectilinearGrid unprocessed and later causes "_vtk_to_mesh_data() Mesh has no faces" errors; update the condition in the loop that reads meshes (the pv.read(...) -> if extract_surface ... block) to detect and handle all supported grid types (pv.UnstructuredGrid, pv.StructuredGrid, pv.ImageData, pv.RectilinearGrid) and call mesh.extract_surface(algorithm="dataset_surface") for those types before appending to meshes so subsequent _vtk_to_mesh_data() receives geometry with faces.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/physiomotion4d/convert_vtk_to_usd.py`:
- Around line 216-228: The topology validation currently recomputes full mesh
conversion by calling _vtk_to_mesh_data() for each frame before
_convert_unified()/_convert_with_labels(), doubling work; modify the code to
cache the converted mesh data sequence on the instance (e.g., set
instance._cached_time_series_mesh_data = mesh_data_seq) after building
mesh_data_seq and use that cached list inside _convert_unified() and
_convert_with_labels() if present instead of recomputing; alternatively, if you
prefer a lighter check, replace validate_time_series_topology(mesh_data_seq)
with a cheap validator that reads vtk_files metadata (point/face counts) and
avoids calling _vtk_to_mesh_data(), but ensure either approach references
validate_time_series_topology, _vtk_to_mesh_data, _convert_unified, and
_convert_with_labels so the heavy conversion is not performed twice.
- Around line 469-480: The current branch calls mesh_converter.create_mesh(...)
when len(part_sequence) == 1, which creates a static prim that will appear at
all time codes; instead always call mesh_converter.create_time_varying_mesh(...)
(even for single-sample parts) so the prim is only populated at the actual
part_time_codes. Update the logic around part_sequence/mesh_path to remove the
special-case create_mesh call and invoke
mesh_converter.create_time_varying_mesh(part_sequence, mesh_path,
part_time_codes, bind_material=True) for both single- and multi-sample sequences
(retain mesh_path, part_sequence and bind_material usage).
---
Nitpick comments:
In `@experiments/Convert_VTK_To_USD/convert_chop_alterra_valve_to_usd.py`:
- Around line 166-181: The current selection of vessel_path uses
non-deterministic picks (candidates[-1], triangle_paths[0]) which can change if
split_mesh_data_by_connectivity or split_mesh_data_by_cell_type alter ordering;
update the logic in the branch that uses usd_tools.list_mesh_paths_under to
deterministically choose a sub-prim (for example, sort candidates/triangle_paths
by a stable key like prim name index or by point/vertex count obtained from the
prim metadata) and set vessel_path to that sorted-first/last element instead of
relying on list order, and add a short processLogger.info/processLogger.debug
line that logs the chosen vessel_path so future regressions are easy to detect
(refer to the variables separate_by, candidates, triangle_paths, vessel_path and
the helper functions
split_mesh_data_by_connectivity/split_mesh_data_by_cell_type).
In `@src/physiomotion4d/convert_vtk_to_usd.py`:
- Around line 190-195: The extract_surface branch currently only handles
pv.UnstructuredGrid which leaves StructuredGrid/ImageData/RectilinearGrid
unprocessed and later causes "_vtk_to_mesh_data() Mesh has no faces" errors;
update the condition in the loop that reads meshes (the pv.read(...) -> if
extract_surface ... block) to detect and handle all supported grid types
(pv.UnstructuredGrid, pv.StructuredGrid, pv.ImageData, pv.RectilinearGrid) and
call mesh.extract_surface(algorithm="dataset_surface") for those types before
appending to meshes so subsequent _vtk_to_mesh_data() receives geometry with
faces.
🪄 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: 77be8c23-077e-4c63-af85-208e513ce6d6
📒 Files selected for processing (7)
docs/API_MAP.mdexperiments/Convert_VTK_To_USD/convert_chop_alterra_valve_to_usd.pyexperiments/Convert_VTK_To_USD/convert_chop_tpv25_valve_to_usd.pysrc/physiomotion4d/cli/convert_vtk_to_usd.pysrc/physiomotion4d/convert_vtk_to_usd.pysrc/physiomotion4d/workflow_convert_vtk_to_usd.pytests/test_vtk_to_usd_library.py
💤 Files with no reviewable changes (1)
- src/physiomotion4d/cli/convert_vtk_to_usd.py
🚧 Files skipped from review as they are similar to previous changes (1)
- experiments/Convert_VTK_To_USD/convert_chop_tpv25_valve_to_usd.py
| # Validate topology consistency for multi-frame time series | ||
| if len(meshes) > 1 and not static_merge: | ||
| from .vtk_to_usd.vtk_reader import validate_time_series_topology | ||
|
|
||
| mesh_data_seq = [ | ||
| instance._vtk_to_mesh_data(m, i) for i, m in enumerate(meshes) | ||
| ] | ||
| try: | ||
| report = validate_time_series_topology(mesh_data_seq) | ||
| for w in report.get("warnings", []): | ||
| instance.log_warning("%s", w) | ||
| except Exception as exc: | ||
| instance.log_debug("Topology validation skipped: %s", exc) |
There was a problem hiding this comment.
Topology validation duplicates the full mesh-conversion pass.
_vtk_to_mesh_data() runs surface extraction, colormap application, primvar packing, etc. Here it is executed once per frame purely for validation, and then _convert_unified() re-invokes it on the same inputs at line 425-427. For a ~265-frame time series (Alterra) this roughly doubles conversion time.
Consider caching the sequence on the instance and reusing it in _convert_unified() / _convert_with_labels(), or swap in a lighter topology check that works directly on vtk_files metadata (point/face counts) without running the full conversion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/physiomotion4d/convert_vtk_to_usd.py` around lines 216 - 228, The
topology validation currently recomputes full mesh conversion by calling
_vtk_to_mesh_data() for each frame before
_convert_unified()/_convert_with_labels(), doubling work; modify the code to
cache the converted mesh data sequence on the instance (e.g., set
instance._cached_time_series_mesh_data = mesh_data_seq) after building
mesh_data_seq and use that cached list inside _convert_unified() and
_convert_with_labels() if present instead of recomputing; alternatively, if you
prefer a lighter check, replace validate_time_series_topology(mesh_data_seq)
with a cheap validator that reads vtk_files metadata (point/face counts) and
avoids calling _vtk_to_mesh_data(), but ensure either approach references
validate_time_series_topology, _vtk_to_mesh_data, _convert_unified, and
_convert_with_labels so the heavy conversion is not performed twice.
| if not part_sequence: | ||
| continue | ||
|
|
||
| # Create material | ||
| material = self._create_material_from_colormap("unified_material") | ||
| mesh_path = f"{root_path}/{part_name}" | ||
| if len(part_sequence) == 1: | ||
| mesh_converter.create_mesh( | ||
| part_sequence[0], mesh_path, bind_material=True | ||
| ) | ||
| else: | ||
| mesh_converter.create_time_varying_mesh( | ||
| part_sequence, mesh_path, part_time_codes, bind_material=True | ||
| ) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l src/physiomotion4d/convert_vtk_to_usd.pyRepository: Project-MONAI/physiomotion4d
Length of output: 116
🏁 Script executed:
sed -n '440,500p' src/physiomotion4d/convert_vtk_to_usd.pyRepository: Project-MONAI/physiomotion4d
Length of output: 2317
🏁 Script executed:
# Search for separate_by parameter and how it's used
rg "separate_by" src/physiomotion4d/convert_vtk_to_usd.py -B 2 -A 5Repository: Project-MONAI/physiomotion4d
Length of output: 3899
🏁 Script executed:
# Look for the _convert_unified function and parts_per_frame logic
rg "parts_per_frame|part_sequence" src/physiomotion4d/convert_vtk_to_usd.py -B 2 -A 3Repository: Project-MONAI/physiomotion4d
Length of output: 1871
🏁 Script executed:
# Check create_mesh and create_time_varying_mesh implementations
rg "def create_mesh|def create_time_varying_mesh" src/physiomotion4d/ -A 10Repository: Project-MONAI/physiomotion4d
Length of output: 1844
🏁 Script executed:
sed -n '1,50p' src/physiomotion4d/vtk_to_usd/usd_mesh_converter.pyRepository: Project-MONAI/physiomotion4d
Length of output: 1307
🏁 Script executed:
# Get full create_mesh implementation
rg "def create_mesh" src/physiomotion4d/vtk_to_usd/usd_mesh_converter.py -A 40Repository: Project-MONAI/physiomotion4d
Length of output: 1522
🏁 Script executed:
# Get full create_time_varying_mesh implementation
rg "def create_time_varying_mesh" src/physiomotion4d/vtk_to_usd/usd_mesh_converter.py -A 45Repository: Project-MONAI/physiomotion4d
Length of output: 1675
🏁 Script executed:
# Check for any documentation on time sampling in USD
rg "time_code|time sample" src/physiomotion4d/vtk_to_usd/usd_mesh_converter.py -B 2 -A 2Repository: Project-MONAI/physiomotion4d
Length of output: 5817
🏁 Script executed:
# Look at the full context of _convert_unified to understand mesh_data_sequence
sed -n '420,450p' src/physiomotion4d/convert_vtk_to_usd.pyRepository: Project-MONAI/physiomotion4d
Length of output: 1288
Single-frame part becomes a static prim visible at every time code in multi-frame workflows.
When separate_by="connectivity" or "cell_type" produces parts with differing names across frames (topology changes), a part appearing in only one frame hits the len(part_sequence) == 1 branch and calls create_mesh() without a time code. In USD, unsampled attributes resolve to their default at all times, so that part will appear at frames where it did not exist in the source data.
For a multi-frame stage it is safer to always route through create_time_varying_mesh() even with a single sample, so the prim is only populated at the frames that actually contributed:
Suggested adjustment
- mesh_path = f"{root_path}/{part_name}"
- if len(part_sequence) == 1:
- mesh_converter.create_mesh(
- part_sequence[0], mesh_path, bind_material=True
- )
- else:
- mesh_converter.create_time_varying_mesh(
- part_sequence, mesh_path, part_time_codes, bind_material=True
- )
+ mesh_path = f"{root_path}/{part_name}"
+ is_time_varying_stage = len(parts_per_frame) > 1
+ if len(part_sequence) == 1 and not is_time_varying_stage:
+ mesh_converter.create_mesh(
+ part_sequence[0], mesh_path, bind_material=True
+ )
+ else:
+ mesh_converter.create_time_varying_mesh(
+ part_sequence, mesh_path, part_time_codes, bind_material=True
+ )This scenario occurs when multiple frames are converted with separate_by="connectivity" or "cell_type" and topology actually varies across frames.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/physiomotion4d/convert_vtk_to_usd.py` around lines 469 - 480, The current
branch calls mesh_converter.create_mesh(...) when len(part_sequence) == 1, which
creates a static prim that will appear at all time codes; instead always call
mesh_converter.create_time_varying_mesh(...) (even for single-sample parts) so
the prim is only populated at the actual part_time_codes. Update the logic
around part_sequence/mesh_path to remove the special-case create_mesh call and
invoke mesh_converter.create_time_varying_mesh(part_sequence, mesh_path,
part_time_codes, bind_material=True) for both single- and multi-sample sequences
(retain mesh_path, part_sequence and bind_material usage).
Summary by CodeRabbit
New Features
Refactor