ENH:VTK_To_USD and ConvertVTKToUSD consolidation #44
ENH:VTK_To_USD and ConvertVTKToUSD consolidation #44aylward merged 4 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
WalkthroughRefactored VTK→USD conversion: removed the low-level Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant ConvertVTKToUSD
participant FileSystem as "VTK files"
participant TopologyValidator
participant USDStage as "USD Stage/Writer"
User->>ConvertVTKToUSD: call from_files(data_basename, vtk_files, ...)
ConvertVTKToUSD->>FileSystem: read VTK frames
FileSystem-->>ConvertVTKToUSD: MeshData sequence
ConvertVTKToUSD->>TopologyValidator: validate_time_series_topology(mesh_sequence)
TopologyValidator-->>ConvertVTKToUSD: validation result / warnings
User->>ConvertVTKToUSD: call convert(output_usd, static_merge?, separate_by?)
ConvertVTKToUSD->>USDStage: create stage / ensure prims
ConvertVTKToUSD->>USDStage: write meshes (time-varying or static)
USDStage-->>User: saved USD file / stage handle
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% 20.31% +6.77%
==========================================
Files 46 45 -1
Lines 6412 6202 -210
==========================================
+ Hits 868 1260 +392
+ Misses 5544 4942 -602
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
…prims Populate _cached_mesh_data in from_files() so _convert_unified() reuses the MeshData built during topology validation instead of converting VTK a second time. Remove the single-frame shortcut that called create_mesh(); create_time_varying_mesh() is now used unconditionally so every prim carries explicit time samples. Add 12 synthetic tests covering the cache (Gap B), time-codes validation (Gap A), single-frame time samples (Gap C), static-merge prim naming (Gap E), and mask_ids label filtering (Gap D).
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/test_convert_vtk_to_usd.py (2)
506-539: Good coverage of the label time-code filtering bug.The 3-frame setup (atrium present at t=0 and t=2 but absent at t=1) precisely exercises the
label_frame_indicespath in_convert_with_labels, and the assertionlist(a_samples) == [0.0, 2.0]will fail if anyone regresses to using the fullself._time_codeslist. One small note:converter._time_codes = [...]again bypasses the public API — ifConvertVTKToUSD.__init__acceptedtime_codesdirectly (likefrom_filesdoes) this could be set cleanly; worth considering as a follow-up to__init__.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_convert_vtk_to_usd.py` around lines 506 - 539, The test is directly mutating the private attribute converter._time_codes; modify ConvertVTKToUSD.__init__ to accept an optional time_codes parameter (e.g., def __init__(..., time_codes: Optional[List[float]] = None) and set self._time_codes = time_codes if provided (falling back to existing behavior), ensure from_files continues to construct instances with its existing time-code handling, and update the test test_mask_ids_missing_label_filters_time_codes to pass time_codes into the constructor instead of assigning converter._time_codes directly.
454-480: Brittle: test pokes private_is_static_mergeinstead of using the public entry point.
static_mergeis a publicfrom_files()option; reaching intoconverter._is_static_mergecouples the test to an internal attribute name and will silently pass (but test the wrong thing) if the attribute is renamed. Sincefrom_files()requires disk files, consider either (a) writing the two meshes totmp_pathas.vtpand usingConvertVTKToUSD.from_files(..., static_merge=True)so the static-merge code path is exercised end-to-end, or (b) adding astatic_mergekwarg on__init__so this can be set via the public API.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_convert_vtk_to_usd.py` around lines 454 - 480, The test mutates the private attribute converter._is_static_merge; change it to use the public API instead by either (A) writing mesh_a and mesh_b to tmp_path as .vtp files and calling ConvertVTKToUSD.from_files(..., static_merge=True) to exercise the real static-merge code path end-to-end (ensuring from_files accepts static_merge and the file list points to the written files), or (B) add a public static_merge kwarg to ConvertVTKToUSD.__init__ (store it on the instance as e.g. self.static_merge and have existing logic read that instead of _is_static_merge) and then construct the converter with ConvertVTKToUSD(data_basename="Organ", input_polydata=[mesh_a, mesh_b], static_merge=True) in the test; update references from _is_static_merge to the public attribute or from_files accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/test_convert_vtk_to_usd.py`:
- Around line 506-539: The test is directly mutating the private attribute
converter._time_codes; modify ConvertVTKToUSD.__init__ to accept an optional
time_codes parameter (e.g., def __init__(..., time_codes: Optional[List[float]]
= None) and set self._time_codes = time_codes if provided (falling back to
existing behavior), ensure from_files continues to construct instances with its
existing time-code handling, and update the test
test_mask_ids_missing_label_filters_time_codes to pass time_codes into the
constructor instead of assigning converter._time_codes directly.
- Around line 454-480: The test mutates the private attribute
converter._is_static_merge; change it to use the public API instead by either
(A) writing mesh_a and mesh_b to tmp_path as .vtp files and calling
ConvertVTKToUSD.from_files(..., static_merge=True) to exercise the real
static-merge code path end-to-end (ensuring from_files accepts static_merge and
the file list points to the written files), or (B) add a public static_merge
kwarg to ConvertVTKToUSD.__init__ (store it on the instance as e.g.
self.static_merge and have existing logic read that instead of _is_static_merge)
and then construct the converter with ConvertVTKToUSD(data_basename="Organ",
input_polydata=[mesh_a, mesh_b], static_merge=True) in the test; update
references from _is_static_merge to the public attribute or from_files
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 2b3f44f8-ae32-4e0b-872d-ec0219f3f945
📒 Files selected for processing (4)
docs/API_MAP.mdsrc/physiomotion4d/convert_vtk_to_usd.pytests/test_convert_vtk_to_usd.pytests/test_vtk_to_usd_library.py
✅ Files skipped from review due to trivial changes (1)
- tests/test_vtk_to_usd_library.py
🚧 Files skipped from review as they are similar to previous changes (1)
- src/physiomotion4d/convert_vtk_to_usd.py
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 12 out of 12 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if len(label_mesh_sequence) == 1: | ||
| label_mesh_sequence[0].material_id = material.name | ||
| material_mgr.get_or_create_material(material) | ||
| mesh_converter.create_mesh( | ||
| label_mesh_sequence[0], mesh_path, bind_material=True |
There was a problem hiding this comment.
In _convert_with_labels, the single-frame case uses create_mesh(..., time_code=None) which authors a static points value. When the overall stage is time-varying, this makes a label that appears in only one frame effectively present at all time codes, undermining the new time-code alignment logic. Consider always authoring time samples for label meshes (e.g., call create_time_varying_mesh with a single element and the correct time code, or pass an explicit time_code to create_mesh).
Summary by CodeRabbit
New Features
Refactor
Tests