Add native mesh file I/O (OBJ, STL, PLY)#1369
Open
dhairyashilRG wants to merge 12 commits into
Open
Conversation
Add native OBJ, STL, and PLY mesh file parsers to eliminate USD dependency for simple test geometries and improve onboarding. * feat(io): Add wp.load_mesh(), wp.save_mesh(), wp.read_mesh() * feat(io): Add OBJ parser with quad triangulation * feat(io): Add STL parser (binary/ASCII) with vertex deduplication * feat(io): Add PLY parser (binary/ASCII, LE/BE) * feat(io): Add MeshData container and to_warp_mesh() method * test(io): Add 23 test cases across all formats * test(io): Add mesh asset generator script Resolves issue where users must install external packages (trimesh, open3d) or use heavy OpenUSD dependency for basic mesh loading. Signed-off-by: DhairyashilGhatage <dhairyashil25@gmail.com>
Address ruff findings from pre-commit checks. Non-functional style fixes only. * style(io): Remove unused imports (Path, warn) * style(io): Add encoding='utf-8' to text-mode open() calls * style(io): Add noqa for intentional lazy imports * style(io): Fix exception chaining with 'from None' * style(io): Replace legacy np.random with modern RNG API * style(io): Replace 'assert False' with raise AssertionError Signed-off-by: DhairyashilGhatage <dhairyashil25@gmail.com>
Kernel integration test was accessing mesh.points directly when mesh is passed as wp.uint64 handle, causing WarpCodegenAttributeError. Kernels must use mesh_query_point() for queries on mesh handles. Direct attribute access (mesh.points, mesh.indices) only works on Mesh objects in Python, not uint64 handles in kernel code. Signed-off-by: DhairyashilGhatage <dhairyashil25@gmail.com>
Validation script had hardcoded CPU device check that failed on CUDA systems. Use wp.get_device() for dynamic detection. * test(io): Fix str(mesh.device) assertion for CUDA * docs(io): Update implementation plan with CUDA test results * docs(io): Document AWS T4 (sm_75) testing completed * docs(io): Document all 69 existing mesh tests pass (no regressions) Testing completed on macOS ARM (CPU), Linux x86_64 (CPU), and Linux x86_64 + CUDA T4. Signed-off-by: DhairyashilGhatage <dhairyashil25@gmail.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds native mesh file I/O to Warp: OBJ, STL (binary/ASCII), and PLY (binary/ASCII) parsers/writers, a MeshData dataclass, and top-level wp.load_mesh(), wp.read_mesh(), and wp.save_mesh() with mesh-to-warp conversion, optional winding flips, and BVH construction hooks. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant LoadAPI as load_mesh()
participant Device as Device<br/>Resolution
participant ReadAPI as read_mesh()
participant Parser as Format<br/>Parser (OBJ/STL/PLY)
participant Converter as MeshData<br/>.to_warp_mesh()
participant Warp as wp.Mesh
App->>LoadAPI: load_mesh(filename, device=None, ...)
LoadAPI->>Device: resolve device
LoadAPI->>ReadAPI: read_mesh(filename, ...)
ReadAPI->>Parser: detect & dispatch by format
Parser-->>ReadAPI: return MeshData(points, indices, ...)
ReadAPI->>ReadAPI: apply flip_winding if requested
ReadAPI-->>LoadAPI: MeshData
LoadAPI->>Converter: to_warp_mesh(device, bvh_kwargs)
Converter->>Warp: construct wp.Mesh (arrays -> wp.array, build BVH)
Warp-->>LoadAPI: wp.Mesh
LoadAPI-->>App: wp.Mesh
sequenceDiagram
participant App as Application
participant SaveAPI as save_mesh()
participant Mesh as wp.Mesh
participant Converter as Extract<br/>Geometry (to NumPy)
participant Writer as Format Writer<br/>(OBJ/STL/PLY)
participant FS as File System
App->>SaveAPI: save_mesh(mesh, filename, binary=True)
SaveAPI->>SaveAPI: detect format from extension
SaveAPI->>Mesh: read mesh.points, mesh.indices
SaveAPI->>Converter: convert to NumPy arrays
Converter-->>SaveAPI: (points, indices, normals?)
SaveAPI->>Writer: dispatch to write_obj/write_stl/write_ply
Writer->>FS: write file (ASCII or binary)
FS-->>SaveAPI: success
SaveAPI-->>App: file saved
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 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)
Comment |
Greptile SummaryThis PR adds native, zero-dependency mesh I/O ( Confidence Score: 5/5Safe to merge; all remaining findings are non-blocking P2 style/consistency issues. All prior P1 bugs are confirmed resolved. The two remaining code issues (missing divisibility guard in write_ply/write_obj, and missing face-count check in read_ply) are P2: they only trigger on malformed inputs that don't arise from the normal save_mesh→load_mesh path. Tests are correctly registered in default_suite and cover the primary user flows. warp/_src/io/ply.py (write_ply missing guard + read_ply empty-face check); warp/_src/io/obj.py (write_obj missing guard); warp/tests/io/validate_mesh_io.py (bare asserts). Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["wp.load_mesh(filename)"] --> B["read_mesh()"]
A2["wp.read_mesh(filename)"] --> B
B --> C{Format detection}
C -->|".obj"| D["read_obj()"]
C -->|".stl"| E["read_stl()"]
C -->|".ply"| F["read_ply()"]
E --> G{_detect_stl_format}
G -->|binary| H["_read_binary_stl()"]
G -->|ascii| I["_read_ascii_stl()"]
H --> J["_deduplicate_stl_vertices()"]
I --> J
F --> K{PLY format header}
K -->|ascii| L["_read_ply_ascii()"]
K -->|binary| M["_read_ply_binary()"]
D & J & L & M --> N["MeshData\n(points, indices, normals?, uvs?, colors?)"]
N -->|"load_mesh path"| O["MeshData.to_warp_mesh()"]
O --> P["wp.Mesh (BVH built)"]
P --> Q["wp.save_mesh(mesh, filename)"]
Q --> R{ext}
R -->|".obj"| S["write_obj()"]
R -->|".stl"| T["write_stl()"]
R -->|".ply"| U["write_ply()"]
Reviews (9): Last reviewed commit: "lint: Move struct import to top level in..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (6)
warp/_src/io/stl.py (1)
35-63: Consider logging or narrowing exceptions in format detection.Static analysis flags the bare
except Exceptionclauses. While this is acceptable for format detection (where any failure should fall back to the next format), narrowing to specific exceptions (ValueError,struct.error,UnicodeDecodeError) would be more precise.📝 Optional: Narrow exception types
if file_size < 84: # Must be ASCII try: _read_ascii_stl(filename) return "ascii" - except Exception: + except (ValueError, RuntimeError, UnicodeDecodeError): pass# Try ASCII try: _read_ascii_stl(filename) return "ascii" - except Exception: + except (ValueError, RuntimeError, UnicodeDecodeError): raise RuntimeError(f"Unable to parse STL file: '{filename}'") from None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@warp/_src/io/stl.py` around lines 35 - 63, Replace the broad except Exception handlers in the STL format detection with narrower exception types: in the initial ASCII probe catch only UnicodeDecodeError, ValueError and OSError when calling _read_ascii_stl(filename); in the binary probe you already catch (struct.error, OSError, EOFError) so keep those; and in the final ASCII fallback catch the same narrow set (UnicodeDecodeError, ValueError, OSError) instead of Exception before raising RuntimeError; reference the functions/variables _read_ascii_stl, filename, file_size and the struct.error handling to locate the three try/except blocks to update.warp/_src/io/mesh.py (2)
68-79: Consider renamingformatparameter to avoid shadowing the builtin.Static analysis flags
formatas shadowing Python's builtin. While this works, renaming tofile_formatormesh_formatwould avoid the warning and potential confusion.📝 Optional: Rename parameter
def load_mesh( filename: str, device: DeviceLike | None = None, *, - format: str | None = None, + file_format: str | None = None, flip_winding: bool = False, ...Apply the same change to the function body and
read_mesh().🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@warp/_src/io/mesh.py` around lines 68 - 79, Rename the parameter named format in functions load_mesh and read_mesh to a non-builtin name (e.g., file_format or mesh_format) and update all uses inside their bodies and any callers in the diff to the new name; ensure function signatures, default handling, type hints (format: str | None -> file_format: str | None) and any keyword references (e.g., when passing format=...) are updated consistently to eliminate builtin shadowing warnings.
173-244: Code duplication withload_mesh()for validation and format detection.The validation logic (file existence, size check, format detection) is duplicated between
load_mesh()andread_mesh(). Consider extracting a shared helper or havingload_mesh()callread_mesh()internally.♻️ Optional: Refactor to reduce duplication
def load_mesh( filename: str, device: DeviceLike | None = None, *, format: str | None = None, flip_winding: bool = False, max_file_size_mb: float | None = 500.0, stl_merge_tolerance: float = 1e-6, support_winding_number: bool = False, bvh_constructor: BvhConstructor | str | None = None, bvh_leaf_size: int | None = None, ) -> wp.Mesh: # Resolve device if device is None: device = get_device() # Delegate to read_mesh for parsing data = read_mesh( filename, format=format, flip_winding=flip_winding, max_file_size_mb=max_file_size_mb, stl_merge_tolerance=stl_merge_tolerance, ) return data.to_warp_mesh( device=device, support_winding_number=support_winding_number, bvh_constructor=bvh_constructor, bvh_leaf_size=bvh_leaf_size, )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@warp/_src/io/mesh.py` around lines 173 - 244, The validation and format-detection logic in read_mesh is duplicated in load_mesh; update load_mesh to delegate parsing/validation to read_mesh by calling read_mesh(filename, format=format, flip_winding=flip_winding, max_file_size_mb=max_file_size_mb, stl_merge_tolerance=stl_merge_tolerance) and then convert the returned MeshData to a wp.Mesh via MeshData.to_warp_mesh(...) (preserve load_mesh's device resolution and parameters support_winding_number, bvh_constructor, bvh_leaf_size), removing the duplicated file existence/size/extension checks from load_mesh.warp/tests/io/validate_mesh_io.py (2)
159-194: Hardcoded relative path may fail depending on working directory.The path
"warp/tests/io/assets/cube.obj"assumes the script is run from the repository root. Consider usingPath(__file__).parent / "assets/cube.obj"for robustness.🔧 Proposed fix: Use path relative to script
def test_mesh_operations_integration(): """Test integration with Warp mesh operations.""" print("\nTesting Warp mesh operations integration...") # Load a test mesh - mesh = wp.load_mesh("warp/tests/io/assets/cube.obj") + cube_path = Path(__file__).parent / "assets" / "cube.obj" + mesh = wp.load_mesh(str(cube_path))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@warp/tests/io/validate_mesh_io.py` around lines 159 - 194, The test_mesh_operations_integration function uses a hardcoded relative path in the wp.load_mesh call which can break depending on CWD; change the mesh load to build the path relative to the test file (use Path(__file__).parent / "assets" / "cube.obj") and pass its string (str(...) or .as_posix()) into wp.load_mesh, and add an import for Path from pathlib at the top of the file if missing so mesh = wp.load_mesh(...) uses the robust path.
24-38: Test assumesreal_world_assetsdirectory exists with OBJ files.If this directory is empty or missing, the test passes silently (the glob returns nothing). Consider adding an assertion that at least one file was tested.
🛡️ Proposed fix: Ensure at least one file is tested
def test_real_world_meshes(): """Test loading real-world mesh files.""" print("Testing real-world meshes...") real_world_dir = Path(__file__).parent / "real_world_assets" - for mesh_file in real_world_dir.glob("*.obj"): + mesh_files = list(real_world_dir.glob("*.obj")) + if not mesh_files: + print(" No real-world OBJ files found, skipping...") + return + + for mesh_file in mesh_files: print(f" {mesh_file.name}...", end=" ")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@warp/tests/io/validate_mesh_io.py` around lines 24 - 38, The test_real_world_meshes function currently iterates real_world_dir.glob("*.obj") which can yield zero files and make the test vacuously pass; fix it by first collecting the matches (e.g., files = list(real_world_dir.glob("*.obj"))) and asserting that files is non-empty (assert len(files) > 0, "No real-world .obj test files found in real_world_assets") before the for loop, then iterate over that files list (referencing test_real_world_meshes, real_world_dir, mesh_file).warp/_src/io/obj.py (1)
111-163: Consider documenting that face indices assume per-vertex normals/UVs.The writer uses the same vertex index for position, UV, and normal (
face[0]+1/face[0]+1/face[0]+1), which assumes normals and UVs are per-vertex and aligned with the points array. This is a valid simplification for indexed meshes, but worth noting since the reader doesn't preserve this indexing relationship.📝 Optional: Add a note to the docstring
def write_obj( points: np.ndarray, indices: np.ndarray, filename: str, normals: np.ndarray | None = None, uvs: np.ndarray | None = None, ) -> None: """Write a mesh to an OBJ file. Args: points: Vertex positions, shape (N, 3). indices: Triangle indices, shape (M * 3,). filename: Output file path. - normals: Optional vertex normals, shape (N, 3). - uvs: Optional texture coordinates, shape (N, 2). + normals: Optional vertex normals, shape (N, 3). Must be per-vertex + (same count as points) if provided. + uvs: Optional texture coordinates, shape (N, 2). Must be per-vertex + (same count as points) if provided. """🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@warp/_src/io/obj.py` around lines 111 - 163, Update the write_obj docstring to explicitly state that normals and uvs are assumed to be per-vertex and aligned with the points array (i.e., the writer uses the same index for position, texture coordinate and normal when emitting faces), and mention that faces are written using those per-vertex indices (OBJ 1-based indexing); modify the docstring in the write_obj function to include this note so callers know the indexing assumption and limitations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@warp/_src/io/__init__.py`:
- Around line 6-7: Update the top-level docstring in this module to capitalize
the product name and format the API symbol as a code literal: change the plain
text "warp" to "Warp" and the plain text reference to wp.load_mesh to a
code-formatted ``wp.load_mesh`` in the module docstring so it follows docstring
conventions.
In `@warp/_src/io/stl.py`:
- Around line 145-193: The ASCII STL reader (_read_ascii_stl) currently returns
raw points/indices without applying vertex deduplication, causing format
inconsistency; after parsing points and indices but before returning MeshData,
run the same deduplication/merge logic used by the binary path (use the existing
merge_tolerance parameter or helper function used elsewhere in read_stl) to
merge nearby vertices and remap indices, then return MeshData with the
deduplicated points (np.float32) and updated indices (np.int32); ensure you
reuse the same dedupe helper or logic so ASCII and binary STL behave
identically.
In `@warp/tests/io/assets/generate_mesh_assets.py`:
- Around line 330-331: The two face writes f.write("f 1 7 2\n") and f.write("f 2
7 6\n") are wrong: vertex 1 is on x=0 so those lines do not reference the cube's
right face, and the triangle winding is not inverted as the docstring requires;
replace these two f.write calls with face definitions that use the four vertex
indices whose x==1 (the right-face vertices) and invert the winding by reversing
the vertex order for each triangle (i.e., ensure each triangle's indices are the
opposite order from the current orientation).
In `@warp/tests/io/validate_mesh_io.py`:
- Around line 52-58: Remove the two unused variables indices_u and indices_v
which are defined but never referenced; delete their declarations so only the
actual mesh generation variables (rng, points, indices) remain (look for the
indices_u and indices_v definitions in the test setup and remove those lines).
---
Nitpick comments:
In `@warp/_src/io/mesh.py`:
- Around line 68-79: Rename the parameter named format in functions load_mesh
and read_mesh to a non-builtin name (e.g., file_format or mesh_format) and
update all uses inside their bodies and any callers in the diff to the new name;
ensure function signatures, default handling, type hints (format: str | None ->
file_format: str | None) and any keyword references (e.g., when passing
format=...) are updated consistently to eliminate builtin shadowing warnings.
- Around line 173-244: The validation and format-detection logic in read_mesh is
duplicated in load_mesh; update load_mesh to delegate parsing/validation to
read_mesh by calling read_mesh(filename, format=format,
flip_winding=flip_winding, max_file_size_mb=max_file_size_mb,
stl_merge_tolerance=stl_merge_tolerance) and then convert the returned MeshData
to a wp.Mesh via MeshData.to_warp_mesh(...) (preserve load_mesh's device
resolution and parameters support_winding_number, bvh_constructor,
bvh_leaf_size), removing the duplicated file existence/size/extension checks
from load_mesh.
In `@warp/_src/io/obj.py`:
- Around line 111-163: Update the write_obj docstring to explicitly state that
normals and uvs are assumed to be per-vertex and aligned with the points array
(i.e., the writer uses the same index for position, texture coordinate and
normal when emitting faces), and mention that faces are written using those
per-vertex indices (OBJ 1-based indexing); modify the docstring in the write_obj
function to include this note so callers know the indexing assumption and
limitations.
In `@warp/_src/io/stl.py`:
- Around line 35-63: Replace the broad except Exception handlers in the STL
format detection with narrower exception types: in the initial ASCII probe catch
only UnicodeDecodeError, ValueError and OSError when calling
_read_ascii_stl(filename); in the binary probe you already catch (struct.error,
OSError, EOFError) so keep those; and in the final ASCII fallback catch the same
narrow set (UnicodeDecodeError, ValueError, OSError) instead of Exception before
raising RuntimeError; reference the functions/variables _read_ascii_stl,
filename, file_size and the struct.error handling to locate the three try/except
blocks to update.
In `@warp/tests/io/validate_mesh_io.py`:
- Around line 159-194: The test_mesh_operations_integration function uses a
hardcoded relative path in the wp.load_mesh call which can break depending on
CWD; change the mesh load to build the path relative to the test file (use
Path(__file__).parent / "assets" / "cube.obj") and pass its string (str(...) or
.as_posix()) into wp.load_mesh, and add an import for Path from pathlib at the
top of the file if missing so mesh = wp.load_mesh(...) uses the robust path.
- Around line 24-38: The test_real_world_meshes function currently iterates
real_world_dir.glob("*.obj") which can yield zero files and make the test
vacuously pass; fix it by first collecting the matches (e.g., files =
list(real_world_dir.glob("*.obj"))) and asserting that files is non-empty
(assert len(files) > 0, "No real-world .obj test files found in
real_world_assets") before the for loop, then iterate over that files list
(referencing test_real_world_meshes, real_world_dir, mesh_file).
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 62f385ee-ad9c-4fa6-b8e8-badd39573813
⛔ Files ignored due to path filters (7)
warp/tests/io/assets/cube.objis excluded by!**/*.objwarp/tests/io/assets/cube_quads.objis excluded by!**/*.objwarp/tests/io/assets/inverted_cube.objis excluded by!**/*.objwarp/tests/io/assets/negative_indices.objis excluded by!**/*.objwarp/tests/io/assets/triangle.objis excluded by!**/*.objwarp/tests/io/real_world_assets/suzanne.objis excluded by!**/*.objwarp/tests/io/real_world_assets/utah_teapot.objis excluded by!**/*.obj
📒 Files selected for processing (24)
CHANGELOG.mdwarp/__init__.pywarp/_src/io/__init__.pywarp/_src/io/mesh.pywarp/_src/io/obj.pywarp/_src/io/ply.pywarp/_src/io/stl.pywarp/examples/core/example_mesh.pywarp/tests/io/TESTING_STATUS.mdwarp/tests/io/__init__.pywarp/tests/io/assets/cube.plywarp/tests/io/assets/cube.stlwarp/tests/io/assets/cube_ascii.plywarp/tests/io/assets/cube_ascii.stlwarp/tests/io/assets/generate_mesh_assets.pywarp/tests/io/assets/triangle.plywarp/tests/io/assets/triangle.stlwarp/tests/io/assets/triangle_ascii.plywarp/tests/io/assets/triangle_ascii.stlwarp/tests/io/test_kernel_integration.pywarp/tests/io/test_mesh.pywarp/tests/io/test_mesh_queries.pywarp/tests/io/validate_mesh_io.pywarp/tests/unittest_suites.py
👮 Files not reviewed due to content moderation or server errors (8)
- warp/init.py
- warp/examples/core/example_mesh.py
- CHANGELOG.md
- warp/tests/io/test_kernel_integration.py
- warp/tests/io/test_mesh_queries.py
- warp/_src/io/ply.py
- warp/tests/io/test_mesh.py
- warp/tests/io/TESTING_STATUS.md
Fix 4 critical bugs, 2 important issues, and 9 nitpicks identified during code review. All 23 TestMeshIO tests pass. Critical fixes (P1): * ply(io): Fix ASCII normals/colors pre-allocation bug - Was allocating only 1 entry, now pre-allocates to vertex_count * ply(io): Fix binary write struct format string - Was appending multiple byte-order prefixes, now uses single prefix * stl(io): Remove per-face normals from MeshData - STL has per-face normals, not per-vertex; omit to avoid shape mismatch * warp: Export MeshData to public namespace - Add wp.MeshData alongside load_mesh/read_mesh/save_mesh Important fixes (P2): * stl(io): Apply vertex deduplication to ASCII STL reader - ASCII and binary now produce equivalent results * test(io): Use public wp.MeshData import instead of internal path Code quality improvements: * obj(io): Clarify docstring about per-vertex assumption for normals/uvs * stl(io): Narrow exception types in format detection * mesh(io): Rename 'format' parameter to 'file_format' (builtin shadow) * mesh(io): Refactor load_mesh to delegate to read_mesh (reduce duplication) * test(io): Fix hardcoded path to use Path(__file__).parent * test(io): Add assertions for missing test files * test(io): Remove unused variables indices_u/indices_v * gen: Fix inverted cube right-face triangles (correct vertices & winding) * io: Capitalize 'Warp' in module docstring Test assets regenerated with fixes. Signed-off-by: DhairyashilGhatage <dhairyashil25@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
warp/_src/io/stl.py (1)
271-279: Duplicated face normal computation.The normal computation logic is duplicated between binary and ASCII branches. Consider extracting to a helper for maintainability.
♻️ Optional extraction
def _compute_face_normal(v0: np.ndarray, v1: np.ndarray, v2: np.ndarray) -> np.ndarray: """Compute unit normal for a triangle face.""" edge1 = v1 - v0 edge2 = v2 - v0 normal = np.cross(edge1, edge2) norm_length = np.linalg.norm(normal) if norm_length > 0: return normal / norm_length return np.array([0.0, 0.0, 1.0])Then use
normal = _compute_face_normal(v0, v1, v2)in both branches.Also applies to: 298-306
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@warp/_src/io/stl.py` around lines 271 - 279, The face normal computation in warp/_src/io/stl.py is duplicated in the binary and ASCII parsing branches (blocks around the current diff and lines 298-306); extract this logic into a helper function (e.g., _compute_face_normal(v0, v1, v2)) that returns a unit normal (fallback to np.array([0.0,0.0,1.0]) on zero length) and replace both occurrences with calls to _compute_face_normal(v0, v1, v2) to remove duplication and improve maintainability.warp/_src/io/ply.py (2)
335-337: Consider validating indices length.
indices.reshape(-1, 3)will raise a confusing error iflen(indices)isn't divisible by 3. A pre-check with a clear error message would improve debuggability.🛡️ Optional validation
+ if len(indices) % 3 != 0: + raise ValueError(f"indices length ({len(indices)}) must be divisible by 3") indices_reshaped = indices.reshape(-1, 3)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@warp/_src/io/ply.py` around lines 335 - 337, Before calling indices.reshape(-1, 3) in this block (where indices_reshaped, num_verts, num_faces are computed), add a pre-check that len(indices) is divisible by 3 and raise a clear ValueError (or use the module's logger) if not; include the length of indices and a hint that faces must be triples so the error message pinpoints the bad input and avoids the confusing reshape exception.
119-130: Misleading guard conditions.The checks
normals is not Noneandcolors is not Noneare always True here since they're initialized as lists (empty or pre-allocated). The code works because the properties won't exist in the header ifhas_normals/has_colorsis False, but the intent would be clearer using the boolean flags directly.♻️ Suggested clarification
- if prop_name == "nx" and normals is not None: + if prop_name == "nx" and has_normals: normals[vertex_idx][0] = float(values[idx]) - elif prop_name == "ny" and normals is not None: + elif prop_name == "ny" and has_normals: normals[vertex_idx][1] = float(values[idx]) - elif prop_name == "nz" and normals is not None: + elif prop_name == "nz" and has_normals: normals[vertex_idx][2] = float(values[idx]) - elif prop_name == "red" and colors is not None: + elif prop_name == "red" and has_colors: colors[vertex_idx][0] = int(values[idx]) - elif prop_name == "green" and colors is not None: + elif prop_name == "green" and has_colors: colors[vertex_idx][1] = int(values[idx]) - elif prop_name == "blue" and colors is not None: + elif prop_name == "blue" and has_colors: colors[vertex_idx][2] = int(values[idx]) - elif prop_name == "alpha" and colors is not None: + elif prop_name == "alpha" and has_colors:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@warp/_src/io/ply.py` around lines 119 - 130, The guard conditions inside the PLY property parsing (checking `prop_name` and assigning to `normals[vertex_idx]` or `colors[vertex_idx]`) use `normals is not None` / `colors is not None`, which is misleading because those variables are always lists; update the conditionals to use the boolean flags `has_normals` and `has_colors` instead (e.g., replace `and normals is not None` with `and has_normals`, and `and colors is not None` with `and has_colors`) so the intent is clear when handling `prop_name` branches that assign to `normals`, `colors`, and their components using `vertex_idx` and `values`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@warp/_src/io/ply.py`:
- Around line 229-231: The code in _read_ply_binary directly indexes
prop_offsets["x"], ["y"], ["z"] when filling points, which will KeyError if
those vertex properties are missing; add a validation step before the vertex
loop that checks prop_offsets contains "x","y","z" and raise a clear,
descriptive exception (including the filename or other context) if any are
missing; update _read_ply_binary's signature to accept filename (or adjust the
error text) so the raised error can include file context and reference the
failing keys, and use that validation to avoid the unsafe indexing in the points
assignment block.
- Around line 257-263: The face-index reader currently hardcodes idx_format =
endian + "i" and misparses non-int32 PLYs; update the PLY header parser (the
code that reads property list lines) to capture both the count_type and
value_type for the face property (e.g., the tokens after "property list" such as
"uchar uint vertex_indices") and store them (e.g., face_count_type,
face_value_type). Add a small mapping from PLY types to struct format characters
(handling signed/unsigned and 8/16/32-bit variants) and use endian +
mapped_format when computing idx_format/idx_size and when reading the count
value; then read count using the mapped count format and loop to unpack that
many indices into face_verts using the mapped value format instead of
hard-coding "i". Ensure the code path that builds face_verts references the new
stored face_value_type and face_count_type.
In `@warp/_src/io/stl.py`:
- Around line 127-132: The code allocates and fills face_normals but never uses
it; remove the dead allocation and per-iteration assignment by deleting the
face_normals = np.empty(...) line and the face_normals[i] = normal assignment
inside the for i in range(num_tris) loop, and instead just advance the file
pointer to consume the 12-byte normal per triangle (e.g., f.read(12)) so the
loop still reads/skips the normal bytes without allocating memory.
---
Nitpick comments:
In `@warp/_src/io/ply.py`:
- Around line 335-337: Before calling indices.reshape(-1, 3) in this block
(where indices_reshaped, num_verts, num_faces are computed), add a pre-check
that len(indices) is divisible by 3 and raise a clear ValueError (or use the
module's logger) if not; include the length of indices and a hint that faces
must be triples so the error message pinpoints the bad input and avoids the
confusing reshape exception.
- Around line 119-130: The guard conditions inside the PLY property parsing
(checking `prop_name` and assigning to `normals[vertex_idx]` or
`colors[vertex_idx]`) use `normals is not None` / `colors is not None`, which is
misleading because those variables are always lists; update the conditionals to
use the boolean flags `has_normals` and `has_colors` instead (e.g., replace `and
normals is not None` with `and has_normals`, and `and colors is not None` with
`and has_colors`) so the intent is clear when handling `prop_name` branches that
assign to `normals`, `colors`, and their components using `vertex_idx` and
`values`.
In `@warp/_src/io/stl.py`:
- Around line 271-279: The face normal computation in warp/_src/io/stl.py is
duplicated in the binary and ASCII parsing branches (blocks around the current
diff and lines 298-306); extract this logic into a helper function (e.g.,
_compute_face_normal(v0, v1, v2)) that returns a unit normal (fallback to
np.array([0.0,0.0,1.0]) on zero length) and replace both occurrences with calls
to _compute_face_normal(v0, v1, v2) to remove duplication and improve
maintainability.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 4140c9a8-1cc5-45cb-8cf0-07e1d92daa31
📒 Files selected for processing (9)
warp/__init__.pywarp/_src/io/__init__.pywarp/_src/io/mesh.pywarp/_src/io/obj.pywarp/_src/io/ply.pywarp/_src/io/stl.pywarp/tests/io/assets/generate_mesh_assets.pywarp/tests/io/test_mesh.pywarp/tests/io/validate_mesh_io.py
✅ Files skipped from review due to trivial changes (3)
- warp/_src/io/init.py
- warp/tests/io/test_mesh.py
- warp/tests/io/validate_mesh_io.py
🚧 Files skipped from review as they are similar to previous changes (3)
- warp/tests/io/assets/generate_mesh_assets.py
- warp/_src/io/obj.py
- warp/_src/io/mesh.py
- Fix test_flip_winding_order to use file_format parameter instead of format - Remove unused face_normals allocation in _read_binary_stl - Parse face index types from PLY header instead of hardcoding int32 - Add validation for required x, y, z vertex properties in PLY parser - Delete test_kernel_integration.py and test_mesh_queries.py (not proper unittest.TestCase subclasses, functionality covered by existing tests) These changes address reviewer feedback on robustness, consistency, and adherence to project testing conventions. Signed-off-by: DhairyashilGhatage <dhairyashil25@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
warp/_src/io/stl.py (4)
55-56:EOFErroris not raised bystruct.unpackorf.read().
f.read()returns an empty bytes object at EOF rather than raisingEOFError. If the file is truncated,struct.unpackwill raisestruct.errorwhen it receives fewer bytes than expected. Consider removingEOFErrorfrom the exception tuple for clarity, or keep it for defensive coding if you prefer.♻️ Optional: Remove unreachable exception type
- except (struct.error, OSError, EOFError): + except (struct.error, OSError): pass🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@warp/_src/io/stl.py` around lines 55 - 56, The except block currently catches EOFError even though struct.unpack and f.read() do not raise EOFError; update the exception tuple in the except clause inside warp/_src/io/stl.py to remove EOFError and keep (struct.error, OSError) so only reachable exceptions are caught (or explicitly document/leave EOFError only if you intentionally want defensive, non-actionable catching). Locate the except handling around the struct.unpack/f.read logic and edit the tuple on that except line accordingly.
251-251: Consider validating that indices length is divisible by 3.If
indiceshas a length not divisible by 3,reshape(-1, 3)will raise a crypticValueError. An explicit check with a clear error message would improve debuggability.♻️ Optional: Add indices validation
+ if len(indices) % 3 != 0: + raise ValueError(f"Indices length ({len(indices)}) must be divisible by 3") indices_reshaped = indices.reshape(-1, 3)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@warp/_src/io/stl.py` at line 251, Before calling indices.reshape(-1, 3) in warp/_src/io/stl.py, validate that len(indices) % 3 == 0 and raise a ValueError with a clear message (e.g. "indices length must be divisible by 3, got {len(indices)}") if not; then proceed to perform indices_reshaped = indices.reshape(-1, 3). This check should be placed immediately before the existing indices_reshaped assignment so any malformed input fails with an explicit error instead of a cryptic reshape ValueError.
268-276: Face normal computation is duplicated.The normal computation logic (cross product, normalization, zero-length handling) is repeated in both binary and ASCII write paths. This is a minor duplication; extracting a helper is optional.
♻️ Optional: Extract helper function
def _compute_face_normal(v0: np.ndarray, v1: np.ndarray, v2: np.ndarray) -> np.ndarray: """Compute normalized face normal, defaulting to [0, 0, 1] for degenerate triangles.""" edge1 = v1 - v0 edge2 = v2 - v0 normal = np.cross(edge1, edge2) norm_length = np.linalg.norm(normal) if norm_length > 0: return normal / norm_length return np.array([0.0, 0.0, 1.0])Also applies to: 295-303
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@warp/_src/io/stl.py` around lines 268 - 276, Extract the duplicated face-normal logic into a single helper (e.g. _compute_face_normal(v0, v1, v2)) that computes the cross product, normalizes it, and returns a default np.array([0.0,0.0,1.0]) for degenerate triangles, then replace the repeated blocks in both the binary and ASCII STL write paths with calls to this helper (look for the blocks computing edge1=edge..., edge2=..., np.cross, np.linalg.norm and the zero-length branch). Ensure the helper is imported/defined in the same module so both write branches call it.
181-186: Vertex count per facet is not validated.A malformed ASCII STL with fewer or more than 3 vertices per facet would produce incorrect triangle indices. Consider tracking vertex count per facet and raising an error if it doesn't equal 3.
♻️ Optional: Add per-facet vertex count validation
in_solid = False in_facet = False + facet_vertex_count = 0 with open(filename, encoding="utf-8") as f: for line in f: ... elif keyword == "facet" and in_solid: in_facet = True + facet_vertex_count = 0 elif keyword == "endfacet": + if facet_vertex_count != 3: + raise ValueError(f"Invalid facet: expected 3 vertices, got {facet_vertex_count}") in_facet = False elif keyword == "vertex" and in_facet: if len(parts) >= 4: points.append([float(parts[1]), float(parts[2]), float(parts[3])]) + facet_vertex_count += 1 if len(points) % 3 == 0:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@warp/_src/io/stl.py` around lines 181 - 186, The parser currently appends vertices for the "vertex" keyword but never validates that each facet has exactly 3 vertices; add a per-facet counter (e.g., facet_vertex_count) that you increment whenever you append to points inside the "vertex" && in_facet branch, and validate when the facet ends (on the "endfacet" keyword or when in_facet becomes False): if facet_vertex_count != 3 raise a ValueError (or a clear parsing exception) and avoid updating indices/vertex_offset for malformed facets; reset facet_vertex_count to 0 at the start of each facet. Ensure the index-building logic that uses vertex_offset remains unchanged for valid facets and that malformed facets do not corrupt indices.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@warp/_src/io/ply.py`:
- Around line 46-49: The loops that scan for "end_header" currently treat an
empty read (EOF) as a blank line and spin; change each loop that does line =
f.readline().decode("ascii").strip() to first read the raw bytes (raw =
f.readline()), check if raw == b"" and raise a parse/EOF error (e.g.,
ValueError("Unexpected EOF while scanning for end_header")) before decoding,
then proceed with line = raw.decode("ascii").strip(); apply this same guard to
the two later header-skip loops (the other places that read and strip lines
searching for end_header).
- Around line 72-73: Replace the current any(...) checks that set
header_info["has_colors"] and header_info["has_normals"] with strict existence
checks so colors are only exposed when red, green, and blue are all present and
normals only when nx, ny, and nz are all present; update all code paths that
allocate or populate arrays (where np.empty(...) is used) — including the ASCII
parsing branch and the binary parsing branches that reference
header_info["has_colors"]/["has_normals"] — to only allocate and fill color
arrays when the full RGB set is present and only allocate/fill normal arrays
when all three normal components exist, avoiding fabrication of zero/garbage
channels when only a single component (e.g., nx or alpha) was in the header.
- Around line 74-77: The code is recording face list types for every "property
list" in a "face" element, which can pick up unrelated list properties; update
the branch inside warp/_src/io/ply.py where current_element == "face" so it only
sets header_info["face_index_count_type"] and
header_info["face_index_value_type"] when the property name (parts[4]) is the
face index field (e.g., "vertex_indices" or "vertex_index"); only assign those
two header_info keys in that conditional to avoid mis-parsing other face list
properties.
---
Nitpick comments:
In `@warp/_src/io/stl.py`:
- Around line 55-56: The except block currently catches EOFError even though
struct.unpack and f.read() do not raise EOFError; update the exception tuple in
the except clause inside warp/_src/io/stl.py to remove EOFError and keep
(struct.error, OSError) so only reachable exceptions are caught (or explicitly
document/leave EOFError only if you intentionally want defensive, non-actionable
catching). Locate the except handling around the struct.unpack/f.read logic and
edit the tuple on that except line accordingly.
- Line 251: Before calling indices.reshape(-1, 3) in warp/_src/io/stl.py,
validate that len(indices) % 3 == 0 and raise a ValueError with a clear message
(e.g. "indices length must be divisible by 3, got {len(indices)}") if not; then
proceed to perform indices_reshaped = indices.reshape(-1, 3). This check should
be placed immediately before the existing indices_reshaped assignment so any
malformed input fails with an explicit error instead of a cryptic reshape
ValueError.
- Around line 268-276: Extract the duplicated face-normal logic into a single
helper (e.g. _compute_face_normal(v0, v1, v2)) that computes the cross product,
normalizes it, and returns a default np.array([0.0,0.0,1.0]) for degenerate
triangles, then replace the repeated blocks in both the binary and ASCII STL
write paths with calls to this helper (look for the blocks computing
edge1=edge..., edge2=..., np.cross, np.linalg.norm and the zero-length branch).
Ensure the helper is imported/defined in the same module so both write branches
call it.
- Around line 181-186: The parser currently appends vertices for the "vertex"
keyword but never validates that each facet has exactly 3 vertices; add a
per-facet counter (e.g., facet_vertex_count) that you increment whenever you
append to points inside the "vertex" && in_facet branch, and validate when the
facet ends (on the "endfacet" keyword or when in_facet becomes False): if
facet_vertex_count != 3 raise a ValueError (or a clear parsing exception) and
avoid updating indices/vertex_offset for malformed facets; reset
facet_vertex_count to 0 at the start of each facet. Ensure the index-building
logic that uses vertex_offset remains unchanged for valid facets and that
malformed facets do not corrupt indices.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9286e847-0c94-4e02-87ea-61b911d390d0
📒 Files selected for processing (4)
warp/__init__.pyiwarp/_src/io/ply.pywarp/_src/io/stl.pywarp/tests/io/test_mesh.py
✅ Files skipped from review due to trivial changes (1)
- warp/tests/io/test_mesh.py
- Add EOF detection in PLY header parsing loops to prevent infinite spin on truncated files - Require complete normal/color component sets (all RGB, all xyz) instead of partial matches to avoid zero/garbage channels - Only capture face index types for vertex_indices property to avoid mis-parsing unrelated face list properties - Remove unreachable EOFError from STL exception tuple - Add indices length validation before reshape in write_stl - Extract duplicated face normal computation to _compute_face_normal - Validate exactly 3 vertices per facet in ASCII STL parser Signed-off-by: DhairyashilGhatage <dhairyashil25@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
warp/_src/io/stl.py (1)
19-27: Normalize the new docstrings to the repo's RST style.These docstrings still use plain text or quoted literals for code elements like
"binary","ascii",merge_tolerance, andMeshData. Please switch those to repo-standard inline markup so Sphinx/Pylance render them consistently.As per coding guidelines, "Use double backticks for code elements and parameter cross-references (RST syntax):
``data``,``.nvdb``—not italics (*data*)."Also applies to: 70-80, 112-120, 132-140, 171-177, 229-245, 274-283
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@warp/_src/io/stl.py` around lines 19 - 27, Update the docstrings in warp/_src/io/stl.py to use RST inline code markup (double backticks) instead of plain quotes or italics: replace occurrences like "binary", "ascii", merge_tolerance, MeshData, and any `.nvdb` references with ``binary``, ``ascii``, ``merge_tolerance``, ``MeshData`` and ``.nvdb`` respectively in the relevant docstrings (including the blocks around lines 19-27, 70-80, 112-120, 132-140, 171-177, 229-245, 274-283); ensure parameter cross-references and code elements follow the repo guideline of using double backticks throughout so Sphinx/Pylance render consistently and update any inline mentions in functions such as detect_stl_format and related docstrings accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@warp/_src/io/stl.py`:
- Around line 219-225: Before returning MeshData, add a guard that rejects
unterminated ASCII facets: check the parser state variables (e.g., in_facet and
facet_vertex_count) and if in_facet is True or facet_vertex_count is not 0 (or
not equal to expected 3), raise a RuntimeError mentioning the truncated STL and
filename; do this validation right before constructing/returning
MeshData(points=..., indices=...) so partially parsed facets are not silently
accepted.
---
Nitpick comments:
In `@warp/_src/io/stl.py`:
- Around line 19-27: Update the docstrings in warp/_src/io/stl.py to use RST
inline code markup (double backticks) instead of plain quotes or italics:
replace occurrences like "binary", "ascii", merge_tolerance, MeshData, and any
`.nvdb` references with ``binary``, ``ascii``, ``merge_tolerance``, ``MeshData``
and ``.nvdb`` respectively in the relevant docstrings (including the blocks
around lines 19-27, 70-80, 112-120, 132-140, 171-177, 229-245, 274-283); ensure
parameter cross-references and code elements follow the repo guideline of using
double backticks throughout so Sphinx/Pylance render consistently and update any
inline mentions in functions such as detect_stl_format and related docstrings
accordingly.
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 462de8dd-1a5a-4049-8159-cbab354830d0
📒 Files selected for processing (2)
warp/_src/io/ply.pywarp/_src/io/stl.py
🚧 Files skipped from review as they are similar to previous changes (1)
- warp/_src/io/ply.py
dhairyashilRG
force-pushed
the
feat/mesh-io-utilities
branch
from
31b0367 to
fd4bfdf
Compare
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
warp/_src/io/stl.py (2)
285-302: Consider validating index bounds before writing.If
indicescontains values>= len(points), an unclearIndexErrorwill occur during write. Adding bounds validation would provide a clearer error message.🛡️ Proposed validation
if len(indices) % 3 != 0: raise ValueError(f"indices length must be divisible by 3, got {len(indices)}") + if len(indices) > 0 and (indices.min() < 0 or indices.max() >= len(points)): + raise ValueError( + f"indices out of bounds: min={indices.min()}, max={indices.max()}, " + f"but points has {len(points)} vertices" + ) indices_reshaped = indices.reshape(-1, 3)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@warp/_src/io/stl.py` around lines 285 - 302, Validate index bounds before indexing points: check that all values in indices are >= 0 and < len(points) (e.g. via np.any or np.max) right after indices_reshaped is created and before the binary write loop, and raise a clear ValueError naming the offending max/min index and the points length. Update the block that creates indices_reshaped and the subsequent binary path (references: indices, indices_reshaped, points) to perform this bounds check and raise a descriptive error instead of letting an IndexError occur during v0/v1/v2 assignment.
142-156: Consider validating file size before reading triangles.If the file is truncated or
num_trisis corrupt,struct.unpackwill raise an unclear error. Pre-validating the expected size would provide a clearer error message.🛡️ Proposed validation
with open(filename, "rb") as f: - f.read(80) # Skip header + header = f.read(80) # Skip header + if len(header) < 80: + raise RuntimeError(f"Truncated STL file (incomplete header): '{filename}'") num_tris = struct.unpack("<I", f.read(4))[0] if num_tris == 0: raise RuntimeError(f"No triangles found in STL file: '{filename}'") + # Validate file has enough data for all triangles + expected_size = 84 + num_tris * 50 + f.seek(0, 2) # Seek to end + actual_size = f.tell() + if actual_size < expected_size: + raise RuntimeError( + f"Truncated STL file: expected {expected_size} bytes for {num_tris} triangles, " + f"got {actual_size}. File: '{filename}'" + ) + f.seek(84) # Seek back to triangle data + # Pre-allocate: worst case = 3 * num_tris unique vertices🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@warp/_src/io/stl.py` around lines 142 - 156, The code reads num_tris from the STL header but doesn't validate that the file contains enough bytes for num_tris triangles, so a truncated or corrupt file can cause unclear struct.unpack errors; compute the expected_size = 80(header) + 4(num_tris) + num_tris*50(bytes per triangle) (or use 84 + num_tris*50) and compare it to the actual file size (via os.path.getsize(filename) or f.seek(0,2)/f.tell()) before looping; also validate that num_tris is non-negative and not absurdly large, and if the check fails raise a clear RuntimeError referencing filename and both expected and actual sizes so callers get an explicit message (update the block that reads num_tris, raw_points and the triangle loop accordingly).warp/_src/io/ply.py (2)
78-87: Redundant condition on line 82.
len(parts) >= 5is already verified on line 80, so the duplicate check on line 82 is unnecessary.🧹 Remove duplicate condition
elif ( current_element == "face" and len(parts) >= 5 and parts[1] == "list" - and len(parts) >= 5 and parts[4] in ("vertex_indices", "vertex_index") ):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@warp/_src/io/ply.py` around lines 78 - 87, The conditional in the PLY header parsing branch redundantly checks len(parts) >= 5 twice; update the if in the block that examines current_element == "face" to remove the duplicate `and len(parts) >= 5` so the condition reads something like: current_element == "face" and len(parts) >= 5 and parts[1] == "list" and parts[4] in ("vertex_indices", "vertex_index"); keep the existing assignments to header_info["face_index_count_type"] and header_info["face_index_value_type"] unchanged.
299-310: Face index type map also missingint16/uint16.For consistency with the vertex
type_map, add the missing aliases here as well.🔧 Add missing aliases and consider deduplication
ply_to_struct = { "char": "b", "uchar": "B", "short": "h", "ushort": "H", "int": "i", "int8": "b", + "int16": "h", "int32": "i", "uint": "I", "uint8": "B", + "uint16": "H", "uint32": "I", }Alternatively, extract a shared constant at module level to avoid maintaining two near-identical mappings.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@warp/_src/io/ply.py` around lines 299 - 310, The face index type map dictionary ply_to_struct in warp/_src/io/ply.py is missing the int16/uint16 aliases; add entries "int16": "h" and "uint16": "H" to match the vertex type_map, and to avoid divergence consider deduplicating by extracting the shared type mapping into a single module-level constant (e.g., TYPE_TO_STRUCT) used by both vertex and face mapping code paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@warp/_src/io/ply.py`:
- Around line 143-146: In the loop that reads vertex lines (the block iterating
for vertex_idx in range(header["vertex_count"]) that reads line =
f.readline().decode("ascii").strip() and splits into values), validate that
len(values) >= 3 before converting to float; if not, raise a clear ValueError
(or custom exception) that includes the vertex_idx and the raw line contents so
malformed/truncated vertex lines produce a descriptive error instead of an
IndexError. Ensure the error message references the header["vertex_count"]
context and the offending line for easier debugging.
- Around line 152-163: The conditional checks use "normals is not None" /
"colors is not None" which are True for empty lists and cause IndexError when
partial components exist; update the conditions in the vertex parsing block (the
branches that handle "nx","ny","nz" and "red","green","blue") to reference the
existence flags has_normals and has_colors (or check truthiness/length) instead
of "is not None" so you only write into normals[vertex_idx][...] and
colors[vertex_idx][...] when those arrays actually contain allocated per-vertex
vectors.
- Around line 321-324: The loop that reads faces currently breaks silently when
count_bytes = f.read(count_size) returns empty, masking truncated/corrupt PLY
files; instead modify the face-reading loop in warp/_src/io/ply.py to raise a
descriptive exception (e.g., EOFError or ValueError) when count_bytes is empty
or shorter than count_size (include context such as the face index and expected
count_size), and similarly validate subsequent reads (like vertex index bytes)
to raise on short reads rather than breaking silently; update code paths that
relied on the break to handle the new exception or let it propagate.
- Around line 206-220: The type mappings are missing PLY aliases; update the
type_map dict and the ply_to_struct mapping to include "int16" -> "h", "uint16"
-> "H", and "float64" -> "d" so properties named int16/uint16/float64 are
handled the same as short/ushort/double; locate and modify the type_map
definition and the ply_to_struct variable in warp/_src/io/ply.py to add these
three aliases.
In `@warp/_src/io/stl.py`:
- Around line 82-87: The multiplication raw_points * scale can overflow int64
when tolerance is extremely small; before computing grid_points in the function
(the block that defines scale and grid_points), compute max_coord =
np.max(np.abs(raw_points)) and compare max_coord * scale against
np.iinfo(np.int64).max; if it would exceed int64 range, either reduce scale to
max_int64 / max_coord (so grid_points = np.round(raw_points * reduced_scale)) or
raise a clear ValueError instructing the caller to use a larger tolerance, and
update any error message to reference tolerance, scale, raw_points and
grid_points so the issue is discoverable.
---
Nitpick comments:
In `@warp/_src/io/ply.py`:
- Around line 78-87: The conditional in the PLY header parsing branch
redundantly checks len(parts) >= 5 twice; update the if in the block that
examines current_element == "face" to remove the duplicate `and len(parts) >= 5`
so the condition reads something like: current_element == "face" and len(parts)
>= 5 and parts[1] == "list" and parts[4] in ("vertex_indices", "vertex_index");
keep the existing assignments to header_info["face_index_count_type"] and
header_info["face_index_value_type"] unchanged.
- Around line 299-310: The face index type map dictionary ply_to_struct in
warp/_src/io/ply.py is missing the int16/uint16 aliases; add entries "int16":
"h" and "uint16": "H" to match the vertex type_map, and to avoid divergence
consider deduplicating by extracting the shared type mapping into a single
module-level constant (e.g., TYPE_TO_STRUCT) used by both vertex and face
mapping code paths.
In `@warp/_src/io/stl.py`:
- Around line 285-302: Validate index bounds before indexing points: check that
all values in indices are >= 0 and < len(points) (e.g. via np.any or np.max)
right after indices_reshaped is created and before the binary write loop, and
raise a clear ValueError naming the offending max/min index and the points
length. Update the block that creates indices_reshaped and the subsequent binary
path (references: indices, indices_reshaped, points) to perform this bounds
check and raise a descriptive error instead of letting an IndexError occur
during v0/v1/v2 assignment.
- Around line 142-156: The code reads num_tris from the STL header but doesn't
validate that the file contains enough bytes for num_tris triangles, so a
truncated or corrupt file can cause unclear struct.unpack errors; compute the
expected_size = 80(header) + 4(num_tris) + num_tris*50(bytes per triangle) (or
use 84 + num_tris*50) and compare it to the actual file size (via
os.path.getsize(filename) or f.seek(0,2)/f.tell()) before looping; also validate
that num_tris is non-negative and not absurdly large, and if the check fails
raise a clear RuntimeError referencing filename and both expected and actual
sizes so callers get an explicit message (update the block that reads num_tris,
raw_points and the triangle loop accordingly).
🪄 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: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 72bd9e95-01be-4b40-9c24-f92616457c88
📒 Files selected for processing (2)
warp/_src/io/ply.pywarp/_src/io/stl.py
- Add check for unterminated facets at EOF in ASCII STL parser - Reset facet_vertex_count to 0 after endfacet to avoid false positive - Update STL docstrings to use RST inline code markup (double backticks) - Fix OBJ negative index off-by-one error: len(points) + v_idx instead of len(points) + v_idx + 1 (OBJ spec: -1 is last vertex, not out of bounds) Signed-off-by: DhairyashilGhatage <dhairyashil25@gmail.com>
PLY fixes: - Validate vertex line has at least 3 values with descriptive error - Use has_normals/has_colors flags instead of 'is not None' to avoid IndexError when partial components exist - Raise RuntimeError on EOF during face reading instead of silent break - Add int16, uint16, float64 type aliases to type_map and ply_to_struct - Remove duplicate len(parts) >= 5 check in header parser STL fixes: - Guard against int64 overflow in vertex deduplication with tiny tolerances - Validate index bounds before writing to prevent cryptic IndexError - Validate file size matches num_tris in binary reader for truncated files Signed-off-by: DhairyashilGhatage <dhairyashil25@gmail.com>
OBJ files can have different vn (normal) count than v (vertex) count for hard-edge meshes (e.g., 24 normals for 8 cube vertices). The current code creates normals_array with vn count, violating MeshData.normals shape (N, 3) where N is vertex count. Only include normals when len(normals) == len(points) to ensure per-vertex alignment. A complete fix would require parsing face //vn indices and splitting vertices for non-manifold normals. Signed-off-by: DhairyashilGhatage <dhairyashil25@gmail.com>
OBJ files can have different vt (UV) count than v (vertex) count. Apply the same validation to uvs_array as normals_array to ensure per-vertex data alignment. This prevents MeshData.uvs.shape[0] from silently differing from MeshData.points.shape[0]. Signed-off-by: DhairyashilGhatage <dhairyashil25@gmail.com>
Fixes PLC0415 violations where 'import struct' was inside functions instead of at module level. All imports now at top of file. Signed-off-by: DhairyashilGhatage <dhairyashil25@gmail.com>
Author
One aspect appears to be a false positive. The CHANGELOG diff shows only additions (mesh I/O entry) with no deletions. No GH-1327 entry exists in the main branch CHANGELOG.md to have been accidentally removed. |
Author
|
I've addressed the initial feedback from the automated bots (fixed the keyword arguments, resolved the OBJ normals shape mismatch, and updated the PLY parsing). All local tests are passing. Ready for a human review whenever someone has time! |
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
1 participant
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.
Description
This PR adds native, zero-dependency mesh I/O utilities (
wp.load_mesh()andwp.save_mesh()) to Warp as per the planCurrently, loading basic test meshes in Warp requires either
pxr.UsdGeom(a massive dependency) or third-party packages liketrimesh. This forces many basic geometry tests and examples to be gated behindUSD_AVAILABLE.This PR implements lightweight, pure-Python parsers for standard mesh formats (Wavefront OBJ, ASCII/Binary STL, and ASCII/Binary PLY) directly into Warp. This is intended to drop the OpenUSD requirement for testing simple geometries and improve onboarding, not to replace high-performance C++ pipelines for massive assets.
Checklist
Test plan
warp/tests/io/test_mesh.py) for OBJ, STL (ASCII/Binary + vertex deduplication), and PLY formats.cube.obj,cube.stl, etc.).uv run --extra dev -m warp.tests -s autodetect -k TestMeshIONew feature / enhancement
Out of scope for this PR: Material/texture loading (.mtl files), animation/sequence loading, and GPU-accelerated parsing (file I/O is CPU-bound).
Summary by CodeRabbit
New Features
Documentation
Tests
Examples