fix: scale VTK mm coordinates to meters when exporting to OpenUSD#45
Merged
aylward merged 4 commits intoProject-MONAI:mainfrom May 6, 2026
Merged
Conversation
VTK/medical imaging uses millimeters; OpenUSD stages declaring metersPerUnit=1.0 require meter-scale geometry. Previously ras_points_to_usd() performed only a RAS→Y-up axis swap, writing raw mm values into USD while the stage metadata claimed they were meters — making a 100 mm structure appear as 100 meters in viewers. Changes: - ras_to_usd() and ras_points_to_usd(): apply * 0.001 (mm → m) during the axis-swap so all point coordinates in USD are in meters. - ras_normals_to_usd(): decoupled from ras_points_to_usd(); performs axis swap only with no unit scaling (normals are unit direction vectors). - usd_tools.py merge_usd_files / merge_usd_files_flattened: fix inconsistent metersPerUnit from 0.01 (centimeters) to 1.0 (meters) to match converter output. - save_usd_file_arrangement: update grid spacing from 400.0 mm to 0.4 m. - Add TestUnitScaling tests verifying point scaling, normal length preservation, and stage metersPerUnit metadata. Breaking change: all generated USD files will have coordinates 1000× smaller than before (meter scale instead of millimeter scale). https://claude.ai/code/session_01L8zowmsyVqXNkZaQivxgc8
WalkthroughUnit/coordinate handling was corrected across the VTK→USD pipeline: RAS point conversions now scale mm→m, normals are axis-swapped without scaling, USD stage metadata and grid spacing were adjusted to reflect meter units, time-sampled attributes ensure baseline values, and tests were added to validate these behaviors. ChangesUnit Scaling and Time-sampling Baselines
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 1
🧹 Nitpick comments (1)
src/physiomotion4d/vtk_to_usd/usd_utils.py (1)
68-73: ⚡ Quick winUse explicit float32 allocation for defensive robustness
While VTK data is always float64 and works correctly with the current code,
np.empty_like()inherits input dtype. For defensive programming, explicitly allocate float32 arrays to prevent silent truncation if non-float arrays are ever passed (e.g., if someone constructsMeshDatadirectly with integer coordinates in millimeters).Suggested improvement
- usd_points = np.empty_like(points) + usd_points = np.empty(points.shape, dtype=np.float32) usd_points[:, 0] = points[:, 0] * 0.001 usd_points[:, 1] = points[:, 2] * 0.001 usd_points[:, 2] = -points[:, 1] * 0.001 - return Vt.Vec3fArray.FromNumpy(usd_points.astype(np.float32)) + return Vt.Vec3fArray.FromNumpy(usd_points)- usd_normals = np.empty_like(normals) + usd_normals = np.empty(normals.shape, dtype=np.float32) usd_normals[:, 0] = normals[:, 0] usd_normals[:, 1] = normals[:, 2] usd_normals[:, 2] = -normals[:, 1] - return Vt.Vec3fArray.FromNumpy(usd_normals.astype(np.float32)) + return Vt.Vec3fArray.FromNumpy(usd_normals)Also applies to: 91–96
🤖 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/physiomotion4d/vtk_to_usd/usd_utils.py` around lines 68 - 73, The code uses np.empty_like(points) which inherits the input dtype and can silently truncate non-float inputs; change the allocation to explicitly create a float32 array (e.g., np.empty(points.shape, dtype=np.float32) or np.zeros(..., dtype=np.float32)), perform the millimeter-to-meter scaling into that float32 buffer, then return Vt.Vec3fArray.FromNumpy(usd_points) (no extra astype needed). Apply the same change to the other similar block referenced (around lines 91–96) so both places allocate usd_points as dtype=np.float32 before filling and returning via Vt.Vec3fArray.FromNumpy.
🤖 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 `@src/physiomotion4d/usd_tools.py`:
- Around line 288-289: The docstring for merge_usd_files() contains a stale note
claiming a "0.01 scale factor"; update that text to reflect the actual behavior
where stage.SetMetadata("metersPerUnit", 1.0) and the up axis is "Y" (i.e., no
0.01 scaling) so the docstring matches the implementation by replacing the 0.01
reference with metersPerUnit=1.0 and mentioning upAxis="Y" as appropriate.
---
Nitpick comments:
In `@src/physiomotion4d/vtk_to_usd/usd_utils.py`:
- Around line 68-73: The code uses np.empty_like(points) which inherits the
input dtype and can silently truncate non-float inputs; change the allocation to
explicitly create a float32 array (e.g., np.empty(points.shape,
dtype=np.float32) or np.zeros(..., dtype=np.float32)), perform the
millimeter-to-meter scaling into that float32 buffer, then return
Vt.Vec3fArray.FromNumpy(usd_points) (no extra astype needed). Apply the same
change to the other similar block referenced (around lines 91–96) so both places
allocate usd_points as dtype=np.float32 before filling and returning via
Vt.Vec3fArray.FromNumpy.
🪄 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: 17663ddd-e378-4383-9fa3-132de240a936
📒 Files selected for processing (3)
src/physiomotion4d/usd_tools.pysrc/physiomotion4d/vtk_to_usd/usd_utils.pytests/test_vtk_to_usd_library.py
There was a problem hiding this comment.
Pull request overview
This PR fixes unit correctness in the VTK→OpenUSD export pipeline by converting VTK’s millimeter-based coordinates into meter-based USD coordinates (consistent with metersPerUnit=1.0) and adds tests to prevent regressions.
Changes:
- Apply mm→m scaling (
* 0.001) during RAS→USD axis conversion for point coordinates, while keeping normal vectors unscaled. - Standardize
metersPerUnitmetadata to1.0in USD merge utilities and adjust grid layout spacing accordingly. - Add unit-scaling tests validating point scaling, normal-length preservation, and stage
metersPerUnitmetadata.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
tests/test_vtk_to_usd_library.py |
Adds TestUnitScaling to validate point scaling, normals behavior, and stage metadata. |
src/physiomotion4d/vtk_to_usd/usd_utils.py |
Updates RAS→USD conversion helpers to scale points mm→m and prevents normals from being scaled. |
src/physiomotion4d/usd_tools.py |
Aligns merge-stage metersPerUnit with meter-scale output and updates layout spacing for meter units. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+67
to
+71
| # Vectorized: USD(x, y, z) = RAS(x, z, -y) * 0.001 (mm → m) | ||
| usd_points = np.empty_like(points) | ||
| usd_points[:, 0] = points[:, 0] # X stays the same | ||
| usd_points[:, 1] = points[:, 2] # Y = Z | ||
| usd_points[:, 2] = -points[:, 1] # Z = -Y | ||
| usd_points[:, 0] = points[:, 0] * 0.001 | ||
| usd_points[:, 1] = points[:, 2] * 0.001 | ||
| usd_points[:, 2] = -points[:, 1] * 0.001 |
Comment on lines
286
to
289
| # Create new stage with meters as units (standard USD configuration) | ||
| stage = Usd.Stage.CreateNew(output_filename) | ||
| stage.SetMetadata("metersPerUnit", 0.01) | ||
| stage.SetMetadata("metersPerUnit", 1.0) | ||
| stage.SetMetadata("upAxis", "Y") |
Comment on lines
+173
to
176
| x_spacing = 0.4 | ||
| y_spacing = 0.4 | ||
| x_offset = -x_spacing * (n_cols - 1) / 2 | ||
| y_offset = -y_spacing * (n_rows - 1) / 2 |
Author default values for time-sampled mesh points, extents, and normals so Omniverse/default-time readers can load single-frame and mixed static/animated USD content correctly. Allocate float arrays during RAS-to-USD coordinate conversion to avoid truncating meter-scaled integer inputs, and update USD merge documentation to match metersPerUnit=1.0.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| # Set points (time-varying if time_code provided) | ||
| points_attr = mesh.CreatePointsAttr() | ||
| if time_code is not None: | ||
| points_attr.Set(usd_points) |
| extent = compute_mesh_extent(usd_points) | ||
| extent_attr = mesh.CreateExtentAttr() | ||
| if time_code is not None: | ||
| extent_attr.Set(extent) |
| normals_attr = mesh.CreateNormalsAttr() | ||
| normals_attr.SetMetadata("interpolation", UsdGeom.Tokens.vertex) | ||
| if time_code is not None: | ||
| normals_attr.Set(usd_normals) |
Comment on lines
+67
to
74
| # Vectorized: USD(x, y, z) = RAS(x, z, -y) * 0.001 (mm → m) | ||
| usd_points = np.empty(points.shape, dtype=np.float32) | ||
| usd_points[:, 0] = points[:, 0] * 0.001 | ||
| usd_points[:, 1] = points[:, 2] * 0.001 | ||
| usd_points[:, 2] = -points[:, 1] * 0.001 | ||
|
|
||
| # Convert to USD Vec3fArray | ||
| return Vt.Vec3fArray.FromNumpy(usd_points.astype(np.float32)) | ||
|
|
| usd_normals[:, 1] = normals[:, 2] | ||
| usd_normals[:, 2] = -normals[:, 1] | ||
|
|
||
| return Vt.Vec3fArray.FromNumpy(usd_normals.astype(np.float32)) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #45 +/- ##
==========================================
+ Coverage 19.92% 20.45% +0.52%
==========================================
Files 45 45
Lines 6202 6214 +12
==========================================
+ Hits 1236 1271 +35
+ Misses 4966 4943 -23
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.
🧹 Nitpick comments (2)
src/physiomotion4d/vtk_to_usd/usd_utils.py (1)
39-50: 💤 Low valueMinor: redundant
if/elsebranches inras_to_usd.Both branches of the
isinstance(point, (tuple, list))check execute identical code. NumPy arrays supportpoint[0],point[1],point[2]indexing just like sequences, so the conditional adds noise without behavioral difference. Optional simplification:♻️ Proposed simplification
- if isinstance(point, (tuple, list)): - return Gf.Vec3f( - float(point[0]) * 0.001, - float(point[2]) * 0.001, - float(-point[1]) * 0.001, - ) - else: - return Gf.Vec3f( - float(point[0]) * 0.001, - float(point[2]) * 0.001, - float(-point[1]) * 0.001, - ) + return Gf.Vec3f( + float(point[0]) * 0.001, + float(point[2]) * 0.001, + float(-point[1]) * 0.001, + )🤖 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/physiomotion4d/vtk_to_usd/usd_utils.py` around lines 39 - 50, The function ras_to_usd contains a redundant isinstance(point, (tuple, list)) branch since both branches build the same Gf.Vec3f from point[0], point[1], point[2]; remove the conditional and always construct and return Gf.Vec3f(float(point[0]) * 0.001, float(point[2]) * 0.001, float(-point[1]) * 0.001) directly so the function handles tuples, lists and NumPy arrays uniformly (update the ras_to_usd function to eliminate the duplicate if/else).src/physiomotion4d/vtk_to_usd/usd_mesh_converter.py (1)
121-131: 💤 Low valueConsider keeping the normals baseline guard consistent with the existing
displayColorbehavior.For
displayColor/displayOpacity(Lines 187, 200), baseline is only authored whentime_code == 0.0, but forpoints/extent/normalsyou author the baseline whenever the attribute hasn't been written yet. The new pattern is arguably more robust (works regardless of whether the first time_code is 0). If intentional, no action needed; otherwise consider aligning the color helpers with the sameGet() is Nonestyle for consistency across all time-sampled attributes.🤖 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/physiomotion4d/vtk_to_usd/usd_mesh_converter.py` around lines 121 - 131, The normals/points/extent code authors a baseline when the attribute hasn't been written yet using the Get() is None pattern, but displayColor/displayOpacity (tokens displayColor/displayOpacity) only author a baseline when time_code == 0.0; make them consistent by changing the displayColor/displayOpacity handling to use the same "if attr.Get() is None: attr.Set(baseline)" guard before time-sampled writes (use the same flow as normals_attr/points_attr: create the attribute, set interpolation metadata, then if time_code is not None check attr.Get() is None to write the baseline and call Set(value, time_code), else Set(value)).
🤖 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.
Nitpick comments:
In `@src/physiomotion4d/vtk_to_usd/usd_mesh_converter.py`:
- Around line 121-131: The normals/points/extent code authors a baseline when
the attribute hasn't been written yet using the Get() is None pattern, but
displayColor/displayOpacity (tokens displayColor/displayOpacity) only author a
baseline when time_code == 0.0; make them consistent by changing the
displayColor/displayOpacity handling to use the same "if attr.Get() is None:
attr.Set(baseline)" guard before time-sampled writes (use the same flow as
normals_attr/points_attr: create the attribute, set interpolation metadata, then
if time_code is not None check attr.Get() is None to write the baseline and call
Set(value, time_code), else Set(value)).
In `@src/physiomotion4d/vtk_to_usd/usd_utils.py`:
- Around line 39-50: The function ras_to_usd contains a redundant
isinstance(point, (tuple, list)) branch since both branches build the same
Gf.Vec3f from point[0], point[1], point[2]; remove the conditional and always
construct and return Gf.Vec3f(float(point[0]) * 0.001, float(point[2]) * 0.001,
float(-point[1]) * 0.001) directly so the function handles tuples, lists and
NumPy arrays uniformly (update the ras_to_usd function to eliminate the
duplicate if/else).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f27596f8-197b-4950-83ff-f83b9b9523af
📒 Files selected for processing (4)
docs/API_MAP.mdsrc/physiomotion4d/usd_tools.pysrc/physiomotion4d/vtk_to_usd/usd_mesh_converter.pysrc/physiomotion4d/vtk_to_usd/usd_utils.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.
VTK/medical imaging uses millimeters; OpenUSD stages declaring metersPerUnit=1.0 require meter-scale geometry. Previously ras_points_to_usd() performed only a RAS→Y-up axis swap, writing raw mm values into USD while the stage metadata claimed they were meters — making a 100 mm structure appear as 100 meters in viewers.
Changes:
Breaking change: all generated USD files will have coordinates 1000× smaller than before (meter scale instead of millimeter scale).
https://claude.ai/code/session_01L8zowmsyVqXNkZaQivxgc8
Summary by CodeRabbit
Bug Fixes
New Features
Refactor
Tests