perf(PLY): fast binary-LE reader via vendored miniply (5.5×, int32 cells)#75
Open
akaszynski wants to merge 3 commits into
Open
perf(PLY): fast binary-LE reader via vendored miniply (5.5×, int32 cells)#75akaszynski wants to merge 3 commits into
akaszynski wants to merge 3 commits into
Conversation
… swap) Swap vtkPLYReader's internals for a fast columnar path on the common case (binary little-endian PLY read from a file), keeping the outward API constant and the output byte-identical to the legacy reader. The legacy reader drives vtkPLY's per-row `ply_get_element` dispatch. The new `ReadPLYFast` uses the vendored miniply parser (MIT, Vilya Harvey) to bulk- extract whole property columns in one pass, then assembles the same vtkPolyData. Byte-exactness is guaranteed by construction: the fast path engages ONLY when every consumed property's stored type already equals its VTK destination type (float x/y/z, normals, tcoords; uchar colors/intensity; integer index lists), so each value is a verbatim little-endian copy. Point and face order are preserved (PLY faces index the point array, so order is load-bearing) and polygons are kept untriangulated. The miniply list path reproduces the exact offsets/connectivity vtkCellArray would build. Anything outside that narrow envelope declines (-1) and falls back to the legacy reader untouched: ASCII / big-endian (last-ULP parse risk), the per-face texcoord point-duplication path, non-matching stored types, list-valued vertex elements, face-before-vertex ordering, or a missing vertex_indices list. Comments are re-scanned from the header so GetComments() stays faithful. miniply.cxx is pulled out of source-unity batching (vendored third-party). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Apply the fvtk-wide width-relaxed rule to the fast PLY reader: build the cell array's offsets/connectivity as int32 by default, widening to int64 only when a value cannot fit in int32 (numPolys/total >= 2^31). Integer VALUES are identical to stock VTK; only the storage container narrows (stock defaults to int64), halving the cell-array footprint for the common case. Byte-exactness is now width-relaxed for integer arrays: values sacred, container width negotiable. Also add an env-gated (FVTK_PLY_FASTPATH_TRACE) stderr breadcrumb so validation can confirm which files engage the fast path; off by default, no API change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The int32-vs-int64 cell-array decision must guarantee EVERY stored value fits. Connectivity values are point indices in [0, numPts); offset values are in [0, total]. The previous check bounded numPolys, which does not bound the connectivity values -- a mesh with few faces but >2^31 points could overflow int32 connectivity. Bound numPts (covers connectivity) and total (covers offsets); numPolys <= total so it needs no separate check. Two unsigned compares, no per-element scan -- cheap and robust. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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.
Summary
Swaps
vtkPLYReader's internals for a fast columnar path on the common case (binary little-endian PLY read from a file), keeping the outward API constant and the output byte-identical to the legacy reader (width-relaxed: int32 cell storage, see below). Falls back to the legacy reader for anything outside the fast envelope.ply_get_elementdispatch.Correctness
The fast path engages only when every consumed property's stored type equals its VTK destination type (float x/y/z, normals, tcoords; uchar colors/intensity; integer index lists), so each value is a verbatim little-endian copy → byte-identical by construction. Point and face order are preserved (PLY faces index the point array), and polygons are not triangulated (miniply list path reproduces the exact offsets/connectivity).
Declines to the legacy reader for: ASCII / big-endian (last-ULP parse risk), the per-face texcoord point-duplication path, non-matching stored types, list-valued vertex elements, face-before-vertex ordering, missing
vertex_indices. Comments are re-scanned from the header soGetComments()stays faithful.Width-relaxed cell storage
Cell offsets/connectivity are stored as int32 when values fit (robust check: bounded by
numPtsfor connectivity andtotalfor offsets), per the fvtk int32-default rule — integer values identical to stock, container narrower (halves cell-array memory).Validation
18-file matrix (all property combos + decline cases), stock VTK 9.6.2 vs fvtk, in the manylinux_2_28 container: 18/18 byte-exact PASS (mesh + comments), fast path engaged on 14/14 binary-LE files, correctly declined 4, int32 cell storage confirmed.
🤖 Generated with Claude Code