feat(MeshIO): binary PLY write and byte-order-correct read (#25) - #28
Merged
Conversation
csparker247
added a commit
that referenced
this pull request
Sep 4, 2026
…ry-io_20260904) The sentence added in this branch said scalars are written as float32 whatever T is. That is true of the binary path only. The ASCII path writes T at full decimal precision under a 'property float x' declaration, so a Mesh3d keeps 0.1234567890123456 through ASCII and narrows it to 0.12345679104328156 through Binary - the same call differing only in the format argument. Replaced with a structure-vs-precision distinction and a @warning naming the asymmetry. The ASCII behavior is long-standing and deliberately unchanged: this branch holds ASCII output byte-identical to e9635ab. Found by code review of #28. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
csparker247
added a commit
that referenced
this pull request
Sep 5, 2026
#29) ## What this is Three cases where `read_ply` or `write_ply` produced a **wrong result instead of an error**. All three predate #28 and are independent of binary write and endianness — found by code review of that PR, fixed here so #28's scope stays settled. Branched off `develop`. ## The bugs ### 1. Unbounded list count on an unknown element → wrong geometry, no error A PLY may contain elements `read_ply` doesn't understand (`edge`, `tristrips`, application-defined). It skips them by reading each list's count and jumping that many bytes. That count had no bound. PLY permits a **signed** count type — `property list char double junk` is legal. A count byte of `0xFF` reads as `-1`; as an unsigned byte total that becomes ~1.8e19, multiplying wraps it back to just below zero, and the cast to `std::streamsize` gives **-8**. A seek of -8 skips nothing, so the reader stays inside the element it meant to step over and parses that payload as vertex data. With enough bytes remaining, nothing fails: ``` truth: v0=(1,2,3) v1=(4,5,6) before char count 0xFF: v0=(0,-2.54688,0) v1=(-2.51562,0,-2.46875) NO ERROR after char count 0xFF: throws "list property count 18446744073709551615 exceeds maximum of 1024" ``` `uchar 0xFF` (255) was already safe — it runs off the end of the file and throws. Only a signed count type reaches the silent path. **Fix:** bound the count before multiplying. `kMaxFaceVertices` and `kMaxFaceListLength` move from duplicated function-local constants in the two face helpers to namespace scope, so all three skip sites share one bound. ### 2. List property on the vertex element → first vertex right, rest garbage The binary vertex reader sizes each record once by summing its properties' scalar widths, then does one `read` per vertex. A list property occupies a count plus N values, so the record size comes out short and every read after the first is misaligned. The ASCII path has the same flaw by a different route — it indexes tokens by property position. ``` truth: v0=(1,2,3) v1=(4,5,6) before: v0=(1,2,3) v1=(-0,-1.08421e-19,-2.00002) NO ERROR after: throws "list property 'extra' on the vertex element is not supported" ``` **Fix:** refuse the file. Lists on a `vertex` element are legal but rare — lists are conventionally a face thing — and refusing cannot break a working case, because no such file was ever read correctly. ### 3. `write_ply` reported success on a write that failed at flush Each tier ended with `if (!file) throw;` — but that ran while the tail of the data was still in the stream buffer. The final flush happens when the `ofstream` is destroyed, and a failure there is swallowed, so a write that failed *only* at flush time returned normally on an incomplete file. **Fix:** `file.close()` before the check, in all three tiers. `close()` performs that flush and records its failure. ## Testing Four failing tests first, then the fixes; two additional guard tests so the fixes can't be over-strict (a well-formed unknown element still skips correctly, and a genuinely huge count still fails). `TestMeshIO` 71 → 77 tests. Green in Debug and Release. Bug 3 needed care to test honestly. My first attempt passed both before and after the fix — worse than no test, since it looks like coverage. The real test isolates the failure to the flush and nowhere earlier: `RLIMIT_FSIZE` at 0 makes every write to the file fail, and a mesh smaller than the stream buffer means no write is attempted until `close()` — so the stream is still good when the old check ran, and only the flush fails. It's `#if defined(__unix__) || defined(__APPLE__)` with the limit and `SIGXFSZ` disposition restored by an RAII guard, and skips via `GTEST_SKIP` where the limit can't be lowered. There is no portable way to provoke this. ## Interaction with #28 #28 also hoists `kMaxFaceVertices` / `kMaxFaceListLength` to namespace scope (so its `validate_ply_face_lists` can `static_assert` the writer's limits against the reader's caps), and it adds a `needs_swap` argument to the `skip_binary_prop` lambda touched here. **Expect a conflict in `MeshIO_PLY.hpp` on whichever merges second**; the resolution is mechanical — keep the hoisted constants once, and keep both the bound and the `needs_swap` argument. If this merges first, #28 rebases and drops its duplicate hoist. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
The feature plan for PLY binary write and header-declared endianness on read. Docs only — no library code, no tests, no build changes. Merging this records the plan; it does not start implementation. Tracks #25. Precedes #19 (multi-chart PLY write), which rewrites the same two functions. Depends on #24, merged as 2eaba49. 5 phases / 22 tasks, ordered reader-before-writer: the reader holds the existing bug, is the smaller change, and is the only instrument that can check the writer. Phases 1–2 stand alone as a correctness fix. Three constraints found in the source that the issue did not state: - std::endian (C++20) and std::byteswap (C++23) are unavailable — the library targets cxx_std_17 — so host-order detection and the swap are hand-rolled. Phase 1 exists for this. - Three call sites need the swap flag, not the two the issue names: read_ply_face_binary and the skip_binary_prop lambda reach the choke points independently of the batched vertex path. - PLYTest.BinaryBigEndian_Throws asserts the behavior being removed and has to be replaced, not added around. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…nary-io_20260904) Task 1.1. The helpers are PLY-local: only binary PLY IO needs them, and a public utils/Endian.hpp would buy permanent public surface, an install entry, and a new test target for a single consumer. detail can be promoted later; a public header cannot be withdrawn. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… (ply-binary-io_20260904) Task 1.2, red phase: detail::host_is_little_endian and detail::swap_bytes do not exist yet, so TestMeshIO does not compile. Task 1.3 adds them. Covers all four widths the PLY format uses (1, 2, 4, 8), including the 1-byte no-op and float/double bit patterns, plus a runtime probe of host order that does not reuse the compile-time macros under test. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…wap (ply-binary-io_20260904) Task 1.3. Adds detail::host_is_little_endian() and detail::swap_bytes() to MeshIO_PLY.hpp. C++17 has neither std::endian nor std::byteswap, so host order is read from __BYTE_ORDER__ with a _WIN32 fallback and the swap is dispatched on sizeof across the format's four widths. host_is_little_endian() is constexpr so callers can hoist the comparison against the file's declared order out of their read loops. Nothing consumes the helpers yet; Phase 2 wires them into the reader. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Tasks 2.1 and 2.2, red phase: all three fail on the binary_big_endian rejection in read_ply_impl. BinaryBigEndian_Read covers every scalar width the format uses - float positions, double normals, ushort colors, uchar list count, int32 list values - with non-palindromic values so an unswapped read cannot pass by accident. The two SwapPrecedesCast tests pin the ordering the spec calls out, one per choke point: read_ply_prop_from_buf via the batched vertex record into a Mesh3d, and read_ply_binary_prop via the face index list. Fixture bytes are reversed by the test itself rather than by detail::swap_bytes, so a wrong swap cannot agree with itself. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…(ply-binary-io_20260904) Tasks 2.3 and 2.4, committed together because the signature change and the call-site threading do not compile apart. read_ply_binary_prop and read_ply_prop_from_buf take a runtime bool needs_swap and reverse the raw fixed-width value before casting to DestT. The parameter carries no default, so the compiler names every call site that forgets it - it found all fifteen. Each function now does one raw read or memcpy through a single lambda, so the swap cannot be omitted in one case of the width dispatch. read_ply_impl resolves the flag once from hdr.format against host_is_little_endian() and threads it to the batched vertex path, read_ply_face_binary, and the skip_binary_prop lambda. The binary_big_endian rejection is gone; binary_little_endian now means what it says instead of "native, labeled little-endian". Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… support (ply-binary-io_20260904) Task 2.5. BinaryBigEndian_Throws is replaced by BinaryBigEndian_Read from Task 2.1. The old test was already passing for the wrong reason: its fixture declares one face but writes no face data, so once the rejection was removed it threw on truncation instead of on the format - a test that would have kept reporting green no matter what the byte-order code did. read_ply's Doxygen no longer claims binary-little-endian only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ary-io_20260904) Task 3.1, red phase: the three throw tests fail, the three boundary tests already pass and exist to catch an over-strict guard. Walks both uchar limits from both sides - 256 and 255 corners without UVs, 128 and 127 with them - plus 128 corners without UVs to pin that the two limits are independent, and a tier-3 case because a missed call site there would fail silently rather than at compile time. Each throw test also asserts the output file does not exist, which fixes the validation ahead of the stream rather than mid-write. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ary-io_20260904) Tasks 3.2 and 3.3. detail::validate_ply_face_lists runs in all three write_ply tiers before the output stream is opened, so a mesh that cannot be written leaves no truncated file behind. Limits are 255 corners for vertex_indices and 127 with UVs, since texcoord writes 2*N; the message names which limit fired and the offending face index. For Task 3.3 the reader's caps are now named at namespace scope instead of being redeclared inside read_ply_face_binary and read_ply_face_ascii, and validate_ply_face_lists static_asserts its limits against them. Raising either without revisiting the other is now a compile error rather than a file the writer emits and the reader refuses. This also guards the ASCII path, whose decimal counts do not truncate but are declared uchar all the same. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ly-binary-io_20260904)
Task 4.1, behavior-neutral. PLYFormat { ASCII, Binary } is threaded through
all three write_ply tiers into write_ply_header and write_ply_data, which
ignore it for now so Tasks 4.2-4.5 have something to compile against.
Defaults to ASCII at every tier. Overload resolution was the risk: an
explicit third argument makes tier 1 and tier 2 both viable, but tier 1's
concrete PLYFormat parameter is more specialized than tier 2's deduced
UVMapT, so partial ordering picks it. Verified for all three tiers with and
without an explicit format.
Kept distinct from detail::PLYHeader::Format so no detail type appears in a
public signature.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Tasks 4.2 through 4.5, red phase: the four byte-level tests fail. The round-trips pass already, because PLYFormat::Binary still writes ASCII and read_ply reads it back happily - the exact blind spot the spec warns about, and the reason the byte-level tests exist. They become real regression tests once Tasks 4.6-4.7 land. Byte-level expectations are hand-derived IEEE-754 and two's-complement literals written big-endian and converted to host order by hand, so they encode the format rather than libcore's opinion of it. Covered: the exact vertex and face bytes of a triangle, float32 scalars from a Mesh3d asserted byte-identical to the Mesh3f write, the 27-byte normals-and-colors record, and the uchar-counted texcoord list including the (-1,-1) unmapped sentinel. Also pins that ASCII stays the default and that PLYFormat::ASCII is the same code path the default takes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Tasks 4.6, 4.7 and 4.8. write_ply_header emits binary_little_endian or binary_big_endian to match the host; the property declarations are unchanged, because binary writes the same float32 scalars, uchar colors and int32 indices the ASCII header already declared. write_ply_data_binary mirrors the reader's batching: the vertex layout is fixed by the mesh's traits, so offsets are resolved once and each vertex is a single write into a 27-byte stack buffer. Face records vary in length and get one write each from a reused vector. Widths never follow T - a Mesh3d and a Mesh3f produce byte-identical files. Unmapped UV corners keep the ASCII path's (-1,-1) sentinel, and the uchar list counts are safe narrowing casts only because validate_ply_face_lists has already run. All three tiers now open with std::ios::binary. On POSIX this is a no-op: ASCII output is byte-identical to the pre-track writer across all three tiers, checked by diffing against e9635ab's header. Windows ASCII callers stop getting CRLF line endings - read_ply already trims \r, so nothing regresses on read. Worth calling out in the PR description. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…sh omission (ply-binary-io_20260904) Phase 5. Task 5.1: @throws on all three write_ply tiers and all three write_mesh dispatchers. Tier 1 carries only the 255-corner vertex_indices limit; tiers 2 and 3 carry both, since texcoord writes 2*N values per face. Task 5.2: @PARAM format on each write_ply overload, and a @note on write_mesh recording that the absence of a format parameter is a decision - a PLY-only value is meaningless for half of write_mesh's inputs - deferred to #26. Brief lines no longer say "ASCII PLY file". Task 5.3: Doxygen is back to the pre-track warning count. Phases 1-4 had introduced four warnings: an undocumented parameter on validate_ply_face_lists, a trailing colon swallowed into a \ref, and the two reader caps hoisted to namespace scope without doc comments. All fixed; the 22 that remain predate the track. spec.md's acceptance criteria are annotated in place with the task and test that satisfies each. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
All 5 phases, 22 tasks. Records the Phase 5 checkpoint, the PR notes the plan asked for, and the outstanding manual MeshLab check. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…-io_20260904) Manual MeshLab check done. A binary tier-1 mesh loads correctly, closing the Phase 4 criterion. It also surfaced a pre-existing limitation: MeshLab rejects any libcore PLY that pairs a texcoord list with a face of more than 3 corners, in ASCII as well as binary. Not a regression - the failing ASCII tier-3 file is byte-identical (md5 639a03ea) to what e9635ab wrote before this track. The cause is in MeshLab's importer rather than libcore's output: its bundled libio_base.so carries vcglib's import_ply.h error table including "Face with no 6 texture coordinates", so per-wedge texcoord is hard-coded to 3 corners and vcglib's polygonal path is unavailable once texcoords are present. The same files parse correctly against an independent PLY reader. Deferred to #19, which already rewrites the texcoord write path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…at-independent (ply-binary-io_20260904) Manual verification in MeshLab 2025.07 against a 9-file matrix varying only face arity and the presence of a texcoord list. Triangles load with and without texcoord in both formats; a quad loads without texcoord in both formats; only the pair fails, and it fails identically in ASCII and binary. Binary therefore loads wherever ASCII does and fails only where ASCII already did, so the binary writer introduces no MeshLab incompatibility - which is what the Phase 4 criterion asked. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… a libcore limitation (ply-binary-io_20260904) The previous wording filed this under "Known Limitation" and floated triangulating on write, which had it backwards. write_ply emits valid PLY: texcoord is a list property and a 2*N count on an N-corner face is what the format allows. vcglib hard-codes per-wedge texcoord to 6 floats, so MeshLab cannot read that one combination. Nothing to fix in libcore, and no libcore issue to file. Kept as an interoperability note so the next person to hit the MeshLab error does not go hunting for a bug here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ry-io_20260904) The sentence added in this branch said scalars are written as float32 whatever T is. That is true of the binary path only. The ASCII path writes T at full decimal precision under a 'property float x' declaration, so a Mesh3d keeps 0.1234567890123456 through ASCII and narrows it to 0.12345679104328156 through Binary - the same call differing only in the format argument. Replaced with a structure-vs-precision distinction and a @warning naming the asymmetry. The ASCII behavior is long-standing and deliberately unchanged: this branch holds ASCII output byte-identical to e9635ab. Found by code review of #28. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ary-io_20260904) The manual MeshLab check was completed; the note still said it was outstanding. Records what it found and points at #29 for the robustness fixes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
csparker247
force-pushed
the
feat/ply-binary-io_20260904
branch
from
September 5, 2026 10:31
b95bf13 to
5e5ac86
Compare
…y-binary-io_20260904) Quality pass. No behavior change: ASCII and binary output are byte-identical before and after, and all 106 tests pass in Debug and Release. The three write_ply tiers were three copies of the same body, and this branch had widened the duplication - it added a validate call and threaded format into two calls in each of the three. detail::write_ply_impl now holds the sequence once and the tiers are one-liners, mirroring how the three read_ply tiers already share read_ply_impl. That collapse makes two documented cross-function invariants hold by construction instead of by each tier remembering them: validate_ply_face_lists runs before write_ply_data_binary's narrowing list-count casts, and the has_normals/has_colors passed to the header are the same values the data writer sees. has_uvs is now derived once as (uvmap != nullptr) rather than spelled independently in three places per tier. Also: - write_ply_data becomes write_ply_data_ascii, a sibling of write_ply_data_binary rather than its wrapper. The format branch moves up to write_ply_impl, so neither data writer takes a PLYFormat and the ASCII scratch buffer is local to the path that uses it instead of a parameter the binary path ignored. - validate_ply_face_lists checks one bound instead of two. The texcoord limit is half the vertex_indices one, so with UVs the 255 check could never fire. - Dropped [[maybe_unused]] from write_ply_data_binary's has_normals/has_colors, which are read unconditionally to size the vertex record. - Tests: make_ngon uses educelab::PI<float> rather than a literal, and the make_triangle() expected-byte body is built by one helper instead of twice. Co-Authored-By: Claude Opus 5 <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
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.
What this is
PLY binary write support, plus the endianness fix on read. Tracks #25.
This PR started as the docs-only feature plan; the branch now carries the
full implementation of all 5 phases / 22 tasks. Depends on #24 (merged as
2eaba49). Precedes #19.What changed
read_plynow honors the byte order declared in the header instead of reinterpreting raw bytes as native. Thebinary_big_endianrejection is gone.PLYFormat { ASCII, Binary }on all threewrite_plytiers, defaulting toASCII. Binary writes native order and labels the header to match.write_plyrejects faces whoseucharlist counts cannot be expressed: 255 corners forvertex_indices, 127 with UVs sincetexcoordwrites2*N.@throwsandPLYFormaton thewrite_plyoverloads and thewrite_meshdispatchers.Read before write, as the plan ordered it: the reader held the existing bug, is
the smaller change, and is the only instrument that can check the writer.
Phases 1–2 are the standalone correctness fix; 3–5 add the feature.
Behavior changes to know about
std::ios::binaryunconditionally. On POSIX this is a no-op — ASCII output isbyte-identical to the pre-track writer across all three tiers, verified by
compiling the same generator against
e9635ab's header and diffing. Nothingregresses on read:
read_plyalready trims\r(
PLYTest.ReadCommentTextureFile_CRLFLineEndings).binary_little_endianpreviously meant "native, labeled little-endian".On the little-endian hosts EduceLab runs on this was invisible, so no existing
file changes meaning — it was still wrong.
detailsignature changes — #19 will need to rebaseBoth tracks rewrite
write_ply_headerandwrite_ply_data.write_ply_headerPLYFormat formatwrite_ply_dataPLYFormat format; delegates to newwrite_ply_data_binaryread_ply_binary_propbool needs_swap— no default, by designread_ply_prop_from_bufbool needs_swap— no default, by designread_ply_face_binarybool needs_swapafterload_texcoordskMaxFaceVertices,kMaxFaceListLengthconstexprin two functions to namespace scope indetailOmitting the default on
needs_swapwas deliberate: it made the compilerenumerate all fifteen call sites rather than letting one silently misread a
property.
Testing
TestMeshIOgoes from 82 to 100 tests. Green in Debug and Release; Doxygen atthe pre-track warning count (22, unchanged, zero new).
Round-trips deliberately do not anchor this work — a byte-order mistake
shared by reader and writer round-trips perfectly, which is how the sized-alias
bug in #24 survived. Instead:
binary_big_endianfixtures with bytes reversed bythe test itself, covering every scalar width the format uses (float, double,
ushort, uchar count, int32 list). Values are non-palindromic so an unswapped
read cannot pass by accident.
3F 80 00 00must read as
1.0, not the denormal 4.6e-41.the tests encode the format rather than libcore's opinion of it. A
Mesh3dwrite is asserted byte-identical to the
Mesh3fone.empty mesh, and the maximal legal faces.
Two findings worth flagging:
PLYTest.BinaryBigEndian_Throwswas passing for the wrong reason — itsfixture declares a face but writes no face bytes, so after the rejection was
removed it threw on truncation instead. It would have stayed green whatever
the byte-order code did. Replaced with a positive read test.
static_assert: the writer'slimits are now checked against the reader's caps at compile time, so raising
either without revisiting the other is a build error rather than a file
libcore writes and refuses to read.
Manual verification
Verified in MeshLab 2025.07 against a matrix varying only face arity and the
presence of a
texcoordlist:Binary loads wherever ASCII loads and fails only where ASCII already failed, so
the binary writer introduces no incompatibility.
The quad +
texcoordrow is a MeshLab constraint, not a libcore one:texcoordis a list property and a2*Ncount on an N-corner face is what theformat allows. MeshLab's bundled
libio_base.socarries vcglib'simport_ply.herror table including "Face with no 6 texture coordinates" —per-wedge
texcoordis hard-coded to 6 floats, so vcglib's polygonal path isunavailable once texcoords are present. Those files parse correctly against an
independently written PLY reader and round-trip through
read_ply, and thefailing ASCII file is byte-identical to what
e9635abwrote. Recorded as aninteroperability note in
spec.md; nothing to change here.Out of scope
write_mesh— a PLY-only value is meaningless for halfits inputs. Deferred to write_mesh: options struct for backend-specific behavior #26;
write_meshgains only@throwsdocs.uchar;configurable scalar width. See
spec.mdfor the rationale on each.