feat(MeshIO): binary PLY write and byte-order-correct read (#25)

conductor/index.md

@@ -17,6 +17,8 @@ Navigation hub for project context.
 - [Mesh IO](./tracks/mesh-io_20260323/index.md) — `mesh-io_20260323` ✓ complete
 - [Multi-Chart PLY Write Support](./tracks/ply-multichart_20260624/index.md) — `ply-multichart_20260624` ⋯ pending
 - [ACVD Remeshing](./tracks/acvd_20260904/index.md) — `acvd_20260904` ⋯ pending
+- [PLY Binary IO](./tracks/ply-binary-io_20260904/index.md) — `ply-binary-io_20260904` ⋯ pending
+- [write_mesh Options Struct](./tracks/mesh-io-options_20260904/index.md) — `mesh-io-options_20260904` ⋯ pending
 
 ## Getting Started
 

conductor/tracks.md

@@ -7,7 +7,7 @@
 | [x] | mesh-io_20260323 | Mesh IO | 2026-03-23 | 2026-06-24 |
 | [ ] | ply-multichart_20260624 | Multi-Chart PLY Write Support | 2026-06-24 | 2026-06-24 |
 | [ ] | acvd_20260904 | ACVD Remeshing | 2026-09-04 | 2026-09-04 |
-| [ ] | ply-binary-io_20260904 | PLY Binary IO | 2026-09-04 | 2026-09-04 |
+| [x] | ply-binary-io_20260904 | PLY Binary IO | 2026-09-04 | 2026-09-04 |
 | [ ] | mesh-io-options_20260904 | write_mesh Options Struct | 2026-09-04 | 2026-09-04 |
 
 <!-- Tracks registered by /conductor:new-track -->

conductor/tracks/ply-binary-io_20260904/index.md

@@ -5,8 +5,13 @@
 
 ## Documents
 
-- Specification — not yet written
-- Implementation Plan — not yet written
+- [Specification](./spec.md)
+- [Implementation Plan](./plan.md)
+
+## Progress
+
+- Phases: 0/5 complete
+- Tasks: 0/22 complete
 
 ## Summary
 

conductor/tracks/ply-binary-io_20260904/metadata.json

@@ -2,16 +2,33 @@
   "id": "ply-binary-io_20260904",
   "title": "PLY Binary IO",
   "type": "feature",
-  "status": "pending",
+  "status": "complete",
   "created": "2026-09-04T00:00:00+0200",
-  "updated": "2026-09-04T00:00:00+0200",
+  "updated": "2026-09-05T00:00:00Z",
   "issue": 25,
+  "current_phase": 5,
+  "current_task": "5.3",
   "phases": {
-    "total": 0,
-    "completed": 0
+    "total": 5,
+    "completed": 5
   },
   "tasks": {
-    "total": 0,
-    "completed": 0
-  }
+    "total": 22,
+    "completed": 22
+  },
+  "commits": [
+    "b28d606: docs(conductor): record byte-order helper placement in detail",
+    "802863c: test(MeshIO): byte-order helper tests for host detection and swapping",
+    "5861ee1: feat(MeshIO): hand-rolled byte-order detection and width-dispatched swap",
+    "f8940e4: test(MeshIO): big-endian PLY read fixtures",
+    "c185fc9: fix(MeshIO): honor the byte order declared in the PLY header on read",
+    "3483f88: test(MeshIO): drop the big-endian rejection test, document byte-order support",
+    "549b2ed: test(MeshIO): face list-count limit tests for the PLY writer",
+    "7583422: feat(MeshIO): reject face list counts a uchar cannot express",
+    "18e21b8: feat(MeshIO): add public PLYFormat and thread it through write_ply",
+    "ab25dc0: test(MeshIO): binary PLY write tests",
+    "398c7d7: feat(MeshIO): binary PLY write",
+    "bfc4aa2: docs(MeshIO): document PLYFormat, list-count throws, and the write_mesh omission"
+  ],
+  "notes": "Phase 4's MeshLab check was completed manually: a binary mesh loads correctly. It also surfaced that MeshLab cannot open any PLY pairing a texcoord list with a face of more than 3 corners, in ASCII or binary alike - a vcglib importer constraint, not a libcore one. Reader/writer robustness bugs found by code review of #28 are fixed separately in #29."
 }

conductor/tracks/ply-binary-io_20260904/plan.md

@@ -0,0 +1,267 @@
+# Implementation Plan: PLY Binary IO
+
+**Track ID:** ply-binary-io_20260904
+**Spec:** [spec.md](./spec.md)
+**Created:** 2026-09-04
+**Status:** [x] Complete
+
+> Task detail is at planning granularity. Per-task test lists get filled in when
+> each phase starts, per the strict tests-first workflow. Design detail lives in
+> [spec.md](./spec.md) and [educelab/libcore#25](https://github.com/educelab/libcore/issues/25).
+
+## Overview
+
+Read before write. The reader is where the existing bug is, it is the smaller
+change, and a correct reader is the only instrument that can check the writer —
+so it lands first and the writer's tests get to lean on it. Validation sits
+between them because it is a precondition of the binary writer being safe, not a
+polish step.
+
+**Phases 1–2 stand alone**: they fix a real correctness bug (endianness ignored
+on read) and are mergeable without any writer work. Phases 3–4 add the feature.
+
+All work is in `include/educelab/core/io/MeshIO_PLY.hpp` and
+`tests/src/TestMeshIO.cpp`, with doc-only touches to
+`include/educelab/core/io/MeshIO.hpp`. No new files, no new test target.
+
+## Checkpoints
+
+| Phase   | Checkpoint SHA | Date | Status  |
+| ------- | -------------- | ---- | ------- |
+| Phase 1 | `5861ee1` | 2026-09-04 | verified |
+| Phase 2 | `3483f88` | 2026-09-04 | verified |
+| Phase 3 | `7583422` | 2026-09-04 | verified |
+| Phase 4 | `398c7d7` | 2026-09-04 | verified* |
+| Phase 5 | `bfc4aa2` | 2026-09-04 | verified |
+
+---
+
+## Phase 1: Byte-order foundation
+
+The library targets `cxx_std_17`, so `std::endian` (C++20) and `std::byteswap`
+(C++23) are both unavailable. Host-order detection and a width-dispatched swap
+have to be written by hand, and they are worth isolating from the PLY logic that
+consumes them.
+
+### Tasks
+
+- [x] **Task 1.1**: Decide where the helpers live — `namespace detail` in
+      `MeshIO_PLY.hpp` (assumed: PLY-local, adds no public surface) or a new
+      public `utils/Endian.hpp`. Record the decision and rationale in
+      `spec.md`.
+- [x] **Task 1.2**: Tests for host-order detection and byte swapping across all
+      four widths the format uses (1, 2, 4, 8 bytes), including the 1-byte
+      no-op and a `double` case.
+- [x] **Task 1.3**: Implement host-endianness detection (`__BYTE_ORDER__` where
+      the compiler defines it, `_WIN32` fallback) and the width-dispatched
+      swap. `<cstring>`/`<cstdint>` only — no new dependency, no new include of
+      anything platform-specific.
+
+### Verification
+
+- [x] Helper tests pass
+- [x] Build succeeds in Debug and Release
+- [x] Full existing suite still green (nothing consumes the helpers yet)
+
+---
+
+## Phase 2: Reader honors declared endianness
+
+Removes the `binary_big_endian` rejection and makes `binary_little_endian` mean
+what it says. Swapping enters at the two choke points every binary scalar read
+passes through.
+
+### Tasks
+
+- [x] **Task 2.1**: Hand-crafted `binary_big_endian` fixture test — bytes
+      reversed by the test itself, positions plus an `int32` index list, read
+      back on an LE host. Exercises more than the 4-byte width so the dispatch
+      is covered, not just the common case.
+- [x] **Task 2.2**: Test that the swap precedes the cast: a BE `float`
+      `3F 80 00 00` must read as 1.0, not 4.6e-41. This is the failure mode the
+      spec calls out as unrecoverable if the order is inverted, so it gets its
+      own test rather than riding along inside a fixture assertion.
+- [x] **Task 2.3**: Add a runtime `bool` swap parameter to
+      `read_ply_binary_prop` and `read_ply_prop_from_buf`, swapping the raw
+      fixed-width value **before** `static_cast<DestT>`. Not a template
+      parameter — see spec.
+- [x] **Task 2.4**: In `read_ply_impl`, compute the flag from `hdr.format`
+      against host order, delete the `BinaryBE` rejection, and thread it to
+      every call site — the batched vertex path, `read_ply_face_binary`, and
+      the `skip_binary_prop` lambda. Missing one leaves a silently misread
+      property.
+- [x] **Task 2.5**: Replace `PLYTest.BinaryBigEndian_Throws` with the positive
+      read test from Task 2.1, and update the `read_ply` Doxygen that currently
+      says "Supports ASCII and binary-little-endian PLY".
+
+### Verification
+
+- [x] BE fixture and pre-cast-swap tests pass
+- [x] `BinaryLittleEndian_Read` and `SizedTypeAliases_BinaryLittleEndian_Read`
+      still pass unmodified — the LE path must not have moved
+- [x] No remaining reference to the BE rejection in code or docs
+- [x] Full suite green, Debug and Release
+
+---
+
+## Phase 3: Face list-count validation
+
+A precondition of the binary writer, not polish: `uchar` list counts silently
+truncate, so a 300-corner face would write a header the reader cannot make sense
+of. ASCII has the same latent problem and gets the same guard.
+
+### Tasks
+
+- [x] **Task 3.1**: Tests for both limits and both boundaries — a 256-corner
+      face throws naming `vertex_indices` and the face index; a 128-corner face
+      with a UV map throws naming `texcoord`; 255 corners without UVs and 127
+      corners with UVs both write cleanly.
+- [x] **Task 3.2**: Implement the two limits in the write path. The message
+      names which limit fired and the offending face index.
+- [x] **Task 3.3**: Confirm coherence with the reader's existing caps
+      (`kMaxFaceVertices` = 256, `kMaxFaceListLength` = 1024 in
+      `read_ply_face_binary`) — everything the writer now permits must still be
+      readable. Verify, do not assume.
+
+### Verification
+
+- [x] Limit tests pass, including both non-throwing boundary cases
+- [x] A maximal legal face round-trips through ASCII (`FaceWith255Corners_*`,
+      `FaceWith127CornersWithUVs_*`); binary added in Task 4.5
+- [x] Full suite green, Debug and Release
+
+---
+
+## Phase 4: Binary write
+
+The feature itself. `PLYFormat` and the plumbing land first as a no-op signature
+change so the tests that follow have something to compile against; the ASCII
+path is untouched until Task 4.6.
+
+### Tasks
+
+- [x] **Task 4.1**: Add public `enum class PLYFormat { ASCII, Binary }` and
+      thread it through the three `write_ply` tiers (defaulting to `ASCII`) into
+      `write_ply_header` and `write_ply_data`, which ignore it for now.
+      Behavior-neutral; kept distinct from `detail::PLYHeader::Format`.
+- [x] **Task 4.2**: Byte-level writer test — a small positions-only mesh
+      written as `Binary`, asserted against a hand-derived byte literal
+      (header line, then exact vertex and face bytes). The test encodes the
+      format, not libcore's opinion of it.
+- [x] **Task 4.3**: Test that scalars are `float32` regardless of `T` — write a
+      `Mesh<double, 3>` and assert 4-byte scalars and a `property float x`
+      declaration.
+- [x] **Task 4.4**: Test that `ASCII` remains the default — an unqualified
+      `write_ply` call still produces `format ascii 1.0`, asserted explicitly
+      rather than left to the existing header-grepping tests.
+- [x] **Task 4.5**: Round-trip tests for structural breadth — n-gon faces, UVs,
+      colors, normals, empty mesh — where hand-computing bytes stops paying.
+- [x] **Task 4.6**: `write_ply_header` emits the `format` line matching the
+      requested format and the host's byte order.
+- [x] **Task 4.7**: Binary path in `write_ply_data` — precomputed property
+      offsets, one `write` per vertex record, `float32` scalars, `uchar`
+      colors, `uchar`-prefixed `vertex_indices` and `texcoord` lists. Mirrors
+      the reader's batching rather than writing per property.
+- [x] **Task 4.8**: Open all three tiers with `std::ios::binary`
+      unconditionally. Note in the PR that Windows ASCII callers stop getting
+      CRLF; `read_ply` already trims `\r`, so nothing regresses on read.
+
+### Verification
+
+- [x] Byte-level, `float32`-width, default-ASCII and round-trip tests all pass
+- [x] Existing ASCII tests pass unmodified, and ASCII output is byte-identical
+      to the pre-track writer across all three tiers (verified by diffing the
+      output of `e9635ab`'s header against the current one)
+- [x] A binary file written by libcore opens correctly in MeshLab — **verified
+      manually in MeshLab 2025.07**, against a 9-file matrix varying only face
+      arity and the presence of a `texcoord` list:
+
+      | # | Mesh | Format | texcoord | MeshLab |
+      | - | ---- | ------ | -------- | ------- |
+      | 01 | triangles | ASCII  | no  | loads |
+      | 02 | triangles | Binary | no  | loads |
+      | 03 | triangles | ASCII  | yes | loads |
+      | 04 | triangles | Binary | yes | loads |
+      | 05 | triangles | Binary | yes | loads |
+      | 06 | quad      | ASCII  | no  | loads |
+      | 07 | quad      | Binary | no  | loads |
+      | 08 | quad      | ASCII  | yes | **fails** |
+      | 09 | quad      | Binary | yes | **fails** |
+
+      Binary loads wherever ASCII does, and fails only where ASCII fails
+      identically — so the binary writer introduces no MeshLab incompatibility.
+      Rows 08/09 are a **MeshLab** constraint, not a libcore one. What
+      `write_ply` emits is 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; see the interoperability note
+      in [spec.md](./spec.md).
+- [x] Full suite green, Debug and Release
+
+---
+
+## Phase 5: Documentation
+
+### Tasks
+
+- [x] **Task 5.1**: `@throws` for the list-count limits on the tier-2 and
+      tier-3 `write_ply` overloads and the corresponding `write_mesh`
+      dispatchers in `MeshIO.hpp`. Tier 1 carries only the 255 limit.
+- [x] **Task 5.2**: Document `PLYFormat` on the `write_ply` overloads, and note
+      that `write_mesh` deliberately gains no format parameter — deferred to
+      [#26](https://github.com/educelab/libcore/issues/26) — so the omission
+      reads as a decision rather than an oversight.
+- [x] **Task 5.3**: Doxygen builds cleanly with no new warnings.
+
+### Verification
+
+- [x] Doxygen clean — 22 warnings, byte-identical to the pre-track baseline at
+      `e9635ab`. Zero new. (The 22 are pre-existing and out of this track's
+      scope; four were introduced by Phases 1–4 and are fixed here.)
+- [x] Every acceptance criterion in `spec.md` maps to a passing test or a
+      merged doc change — all 15 annotated in place in `spec.md`
+
+---
+
+## Final Verification
+
+- [x] All acceptance criteria in [spec.md](./spec.md) met — all 15 annotated
+      in place with the task and test that satisfies each
+- [x] Full test suite passes in Debug and Release — 17/17 targets each,
+      `TestMeshIO` at 100 tests (82 → 100 over the track)
+- [x] Doxygen updated for `PLYFormat`, `@throws`, and BE read support; warning
+      count identical to the pre-track baseline
+- [x] PR notes the Windows CRLF change for ASCII callers — see below
+- [x] Ready for review; #19 unblocked to rebase onto the new
+      `write_ply_header` / `write_ply_data` signatures (both gained a trailing
+      `PLYFormat format` parameter, and `write_ply_data` now delegates to
+      `write_ply_data_binary`)
+- [x] MeshLab open — verified manually; see the Phase 4 note for the matrix
+
+## For the PR description
+
+- **Windows ASCII callers stop getting CRLF.** All three `write_ply` tiers now
+  open with `std::ios::binary` unconditionally (Task 4.8). On POSIX this is a
+  no-op — ASCII output is byte-identical to the pre-track writer. Nothing
+  regresses on read: `read_ply` already trims `\r`
+  (`PLYTest.ReadCommentTextureFile_CRLFLineEndings`).
+- **`binary_little_endian` previously 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.
+- **`detail` signature changes for #19:** `write_ply_header` and
+  `write_ply_data` take a trailing `PLYFormat`, `read_ply_binary_prop` and
+  `read_ply_prop_from_buf` take a trailing `bool needs_swap` (no default, by
+  design), and `read_ply_face_binary` takes `needs_swap` after
+  `load_texcoords`.
+- **`kMaxFaceVertices` / `kMaxFaceListLength`** moved from function-local
+  `constexpr` in two functions to namespace scope in `detail`, so
+  `validate_ply_face_lists` can `static_assert` the writer's limits against
+  them.
+
+---
+
+_Generated by Conductor. Tasks will be marked [~] in progress and [x] complete._
+
+_*Phase 4 verified except the manual MeshLab open, which cannot be automated
+(MeshLab 2025.07 ships no CLI). Validated against an independently written PLY
+parser instead; sample files listed in the Phase 4 notes._