From 5a9a04adb56a4318c6e08c9eff903231e4fdffaa Mon Sep 17 00:00:00 2001 From: Seth Parker Date: Fri, 4 Sep 2026 16:49:09 -0400 Subject: [PATCH 01/23] docs(conductor): PLY binary IO spec and implementation plan MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- conductor/index.md | 2 + .../tracks/ply-binary-io_20260904/index.md | 9 +- .../ply-binary-io_20260904/metadata.json | 4 +- .../tracks/ply-binary-io_20260904/plan.md | 210 ++++++++++++++++++ .../tracks/ply-binary-io_20260904/spec.md | 192 ++++++++++++++++ 5 files changed, 413 insertions(+), 4 deletions(-) create mode 100644 conductor/tracks/ply-binary-io_20260904/plan.md create mode 100644 conductor/tracks/ply-binary-io_20260904/spec.md diff --git a/conductor/index.md b/conductor/index.md index 6a21a96..345d2c6 100644 --- a/conductor/index.md +++ b/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 diff --git a/conductor/tracks/ply-binary-io_20260904/index.md b/conductor/tracks/ply-binary-io_20260904/index.md index 6d82948..442b1c5 100644 --- a/conductor/tracks/ply-binary-io_20260904/index.md +++ b/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 diff --git a/conductor/tracks/ply-binary-io_20260904/metadata.json b/conductor/tracks/ply-binary-io_20260904/metadata.json index 27c2306..7aa0b71 100644 --- a/conductor/tracks/ply-binary-io_20260904/metadata.json +++ b/conductor/tracks/ply-binary-io_20260904/metadata.json @@ -7,11 +7,11 @@ "updated": "2026-09-04T00:00:00+0200", "issue": 25, "phases": { - "total": 0, + "total": 5, "completed": 0 }, "tasks": { - "total": 0, + "total": 22, "completed": 0 } } diff --git a/conductor/tracks/ply-binary-io_20260904/plan.md b/conductor/tracks/ply-binary-io_20260904/plan.md new file mode 100644 index 0000000..5f8b8bb --- /dev/null +++ b/conductor/tracks/ply-binary-io_20260904/plan.md @@ -0,0 +1,210 @@ +# Implementation Plan: PLY Binary IO + +**Track ID:** ply-binary-io_20260904 +**Spec:** [spec.md](./spec.md) +**Created:** 2026-09-04 +**Status:** [ ] Not Started + +> 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 | | | pending | +| Phase 2 | | | pending | +| Phase 3 | | | pending | +| Phase 4 | | | pending | +| Phase 5 | | | pending | + +--- + +## 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 + +- [ ] **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`. +- [ ] **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. +- [ ] **Task 1.3**: Implement host-endianness detection (`__BYTE_ORDER__` where + the compiler defines it, `_WIN32` fallback) and the width-dispatched + swap. ``/`` only — no new dependency, no new include of + anything platform-specific. + +### Verification + +- [ ] Helper tests pass +- [ ] Build succeeds in Debug and Release +- [ ] 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 + +- [ ] **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. +- [ ] **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. +- [ ] **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`. Not a template + parameter — see spec. +- [ ] **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. +- [ ] **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 + +- [ ] BE fixture and pre-cast-swap tests pass +- [ ] `BinaryLittleEndian_Read` and `SizedTypeAliases_BinaryLittleEndian_Read` + still pass unmodified — the LE path must not have moved +- [ ] No remaining reference to the BE rejection in code or docs +- [ ] 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 + +- [ ] **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. +- [ ] **Task 3.2**: Implement the two limits in the write path. The message + names which limit fired and the offending face index. +- [ ] **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 + +- [ ] Limit tests pass, including both non-throwing boundary cases +- [ ] A maximal legal face round-trips through both ASCII and (once Phase 4 + lands) binary +- [ ] 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 + +- [ ] **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`. +- [ ] **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. +- [ ] **Task 4.3**: Test that scalars are `float32` regardless of `T` — write a + `Mesh` and assert 4-byte scalars and a `property float x` + declaration. +- [ ] **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. +- [ ] **Task 4.5**: Round-trip tests for structural breadth — n-gon faces, UVs, + colors, normals, empty mesh — where hand-computing bytes stops paying. +- [ ] **Task 4.6**: `write_ply_header` emits the `format` line matching the + requested format and the host's byte order. +- [ ] **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. +- [ ] **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 + +- [ ] Byte-level, `float32`-width, default-ASCII and round-trip tests all pass +- [ ] Existing ASCII tests pass unmodified +- [ ] A binary file written by libcore opens correctly in MeshLab +- [ ] Full suite green, Debug and Release + +--- + +## Phase 5: Documentation + +### Tasks + +- [ ] **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. +- [ ] **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. +- [ ] **Task 5.3**: Doxygen builds cleanly with no new warnings. + +### Verification + +- [ ] Doxygen clean +- [ ] Every acceptance criterion in `spec.md` maps to a passing test or a + merged doc change + +--- + +## Final Verification + +- [ ] All acceptance criteria in [spec.md](./spec.md) met +- [ ] Full test suite passes in Debug and Release +- [ ] Doxygen updated for `PLYFormat`, `@throws`, and BE read support +- [ ] PR notes the Windows CRLF change for ASCII callers +- [ ] Ready for review; #19 unblocked to rebase onto the new + `write_ply_header` / `write_ply_data` signatures + +--- + +_Generated by Conductor. Tasks will be marked [~] in progress and [x] complete._ diff --git a/conductor/tracks/ply-binary-io_20260904/spec.md b/conductor/tracks/ply-binary-io_20260904/spec.md new file mode 100644 index 0000000..82aa8f2 --- /dev/null +++ b/conductor/tracks/ply-binary-io_20260904/spec.md @@ -0,0 +1,192 @@ +# Specification: PLY Binary IO + +**Track ID:** ply-binary-io_20260904 +**Type:** Feature +**Created:** 2026-09-04 +**Status:** Draft +**GitHub issue:** [educelab/libcore#25](https://github.com/educelab/libcore/issues/25) + +## Summary + +Add binary PLY write support, and make `read_ply` honor the endianness declared +in the file header rather than reinterpreting raw bytes as native. These land +together as one contract: a writer that can emit binary must not be able to +produce a file the reader refuses or silently misreads. + +## Context + +EduceLab Core provides the project's shared mesh IO. `write_ply` today emits +`format ascii 1.0` unconditionally (`MeshIO_PLY.hpp:984`). For a multi-million-face +mesh that is several times the size of the binary equivalent and correspondingly +slower to write — `pgs-decimate` writes its output once per improved search +round and pays that cost repeatedly. + +Separately, `read_ply` claims `binary_little_endian` support but does not honor +the declared byte order. `read_ply_binary_prop` (`MeshIO_PLY.hpp:318`) and +`read_ply_prop_from_buf` (`MeshIO_PLY.hpp:388`) reinterpret raw bytes as native +types — native-endian reading labeled little-endian — and `binary_big_endian` is +rejected outright at `MeshIO_PLY.hpp:638`. On the little-endian hosts EduceLab +runs on the mislabeling is invisible; it is still wrong, and it is the half of +the contract the writer would otherwise be able to violate. + +## User Story + +As an EduceLab developer writing large meshes repeatedly, I want `write_ply` to +emit binary PLY so that output is smaller and faster to write, and I want +`read_ply` to correctly read any binary PLY regardless of the byte order its +header declares. + +## Acceptance Criteria + +### Write + +- [ ] A public `educelab::PLYFormat { ASCII, Binary }` exists and is threaded + through all three `write_ply` tiers, defaulting to `ASCII`. +- [ ] `PLYFormat` is kept distinct from `detail::PLYHeader::Format` (three + values, "what I parsed") so no `detail` type leaks into a public signature. +- [ ] `Binary` writes native byte order and labels the header to match + (`format binary_little_endian 1.0` on an LE host). +- [ ] Scalars are always written as `float32` regardless of the mesh's `T`, + matching what the ASCII header already declares and what OpenMVS emits. + The on-disk format does not depend on a template parameter. +- [ ] The binary writer mirrors the reader's record batching — precomputed + property offsets, one `write` per vertex — not one `write` per property. +- [ ] All three tiers open with `std::ios::binary` unconditionally. +- [ ] Existing ASCII output is byte-for-byte unchanged on non-Windows hosts; + all existing header-grepping tests pass untouched. + +### Read + +- [ ] `read_ply` honors the endianness declared in the header, byte-swapping + when file order differs from host order. +- [ ] The `binary_big_endian` rejection at `MeshIO_PLY.hpp:638` is removed and + `PLYTest.BinaryBigEndian_Throws` is replaced by a positive read test. +- [ ] Swapping enters at the two choke points every binary scalar read passes + through: `read_ply_binary_prop` (stream) and `read_ply_prop_from_buf` + (buffer). +- [ ] The swap flag is a runtime `bool`, not a template parameter. +- [ ] The swap applies to the raw fixed-width value **before** the cast to the + destination type. + +### Validation + +- [ ] `write_ply` throws when a face exceeds the `uchar` list-count limits: 255 + corners for `vertex_indices`, 127 for `texcoord` (which writes `2*N`). +- [ ] The message names which limit fired and the offending face index. +- [ ] `@throws` is documented on the tier-2 and tier-3 `write_ply` overloads and + the corresponding `write_mesh` dispatchers; tier 1 carries only the 255 + limit. + +## Design Decisions + +### `PLYFormat` is a new public enum, not `detail::PLYHeader::Format` + +`PLYHeader::Format` has three values and answers "what did I parse". The writer +needs two and answers "what should I emit". Reusing it would put a `detail` type +in a public signature and would require the writer to reject one of its own +enum's values. + +### Binary writes native order only + +A caller who wants a specific byte order is not a caller libcore has. Writing +native and labeling honestly is correct, and the reader now handles either +direction, so libcore-written files remain readable everywhere. + +### `float32` on disk regardless of `T` + +The ASCII header already declares `property float x`. Making the binary width +follow the mesh's `T` would mean a `Mesh3d` and a `Mesh3f` produce structurally +different files from the same call, and would make the on-disk format a function +of a template parameter. `float32` matches OpenMVS and the existing declaration. + +### Runtime `bool` for the swap, not a template parameter + +The flag is loop-invariant, so the branch predicts perfectly. Templating on it +doubles the instantiated code for both choke points for no measurable gain. + +### Swap before the cast, never after + +This is the trap that makes the feature subtle. Big-endian `3F 80 00 00` +`memcpy`'d into a native `float` is 4.6e-41; no reversal of the widened `double` +recovers 1.0. The bytes must be reordered while the value is still its raw +fixed-width type. + +### `uchar` list counts stay `uchar` + +Widening `vertex_indices` to a `uint32` count would cost 3 bytes per face — some +30MB on a 10M-face mesh — to guard a case that does not occur. Throwing is the +right answer for a mesh that does hit it. + +### Endianness helpers are hand-rolled + +The library targets `cxx_std_17` (`CMakeLists.txt:69`). `std::endian` is C++20 +and `std::byteswap` is C++23, so neither is available; host-order detection and +a width-dispatched swap have to be written against `__BYTE_ORDER__` with a +`_WIN32` fallback. + +## Testing Strategy + +Round-trip tests cannot anchor this work. The sized-alias bug fixed in #24 +survived precisely because the suite only ever read what libcore wrote, and a +byte-order mistake shared by reader and writer round-trips just as happily. + +- **Reader:** a hand-crafted `binary_big_endian` fixture with bytes reversed by + the test, exercising the swap path on an LE host. Covers more than one scalar + width so the dispatch is exercised, not just the 4-byte case. +- **Writer:** byte-level assertion of a small mesh's binary body against a + hand-derived literal, so the test encodes the format rather than libcore's + opinion of it. +- **Round-trip:** used only for structural breadth (n-gons, UVs, colors, + normals, empty mesh) where hand-computing bytes stops paying. + +## Dependencies + +- **Depends on #24** (sized PLY type aliases on read) — merged as `2eaba49`. +- Depends on existing code: `detail::parse_ply_header`, `detail::PLYHeader`, + `detail::PLYType`, `detail::ply_type_bytes`, `detail::read_ply_impl`, + `detail::write_ply_header`, `detail::write_ply_data`, and the three public + `write_ply` / `read_ply` tiers, all in + `include/educelab/core/io/MeshIO_PLY.hpp`. +- Tests live in the existing `tests/src/TestMeshIO.cpp`, already registered in + `tests/CMakeLists.txt`. No new test target. + +## Sequencing + +Lands **before** [ply-multichart_20260624](../ply-multichart_20260624/index.md) +(#19). Both rewrite `write_ply_header` and `write_ply_data`; multichart is +Pending at 0/15, this is the live need, and this is the smaller and more +additive of the two. + +Precedes [mesh-io-options_20260904](../mesh-io-options_20260904/index.md) (#26), +which was filed out of this track's decision to leave `write_mesh` untouched. + +## Out of Scope + +- **A format parameter on `write_mesh`.** A PLY-only value is meaningless for + half of `write_mesh`'s inputs, and there is no good answer for + `write_mesh("out.obj", mesh, PLYFormat::Binary)`. Deferred to #26; `write_mesh` + gains only `@throws` documentation here. +- **Writing non-native byte order.** The writer emits native and labels it. +- **Widening list-count declarations** beyond `uchar`. +- **Binary OBJ**, which is not a format. +- **Multi-chart PLY write** (`texnumber`, multiple `TextureFile` comments) — + that is #19. +- **Configurable scalar width on write.** `float32` always. + +## Technical Notes + +- The reader's existing face-record caps (`kMaxFaceVertices` = 256, + `kMaxFaceListLength` = 1024 in `read_ply_face_binary`) sit above the writer's + new limits (255 corners, 254 texcoord values), so nothing the writer emits can + be refused on read. Verify this rather than assume it. +- Opening the ASCII tiers with `std::ios::binary` is a silent behavior change + for Windows callers, who stop getting CRLF line endings. Nothing regresses on + read: `read_ply` already trims `\r` (`PLYTest.ReadCommentTextureFile_CRLFLineEndings` + covers it). Call it out in the PR description. +- `read_ply_face_binary` and the `skip_binary_prop` lambda in `read_ply_impl` + both call `read_ply_binary_prop`; every call site needs the new flag threaded + through, not just the vertex path. + +--- + +_Generated by Conductor from educelab/libcore#25. Review and edit as needed._ From 4e6b299dd006d321eb5bbdb45e428846646cf86e Mon Sep 17 00:00:00 2001 From: Seth Parker Date: Fri, 4 Sep 2026 17:55:59 -0400 Subject: [PATCH 02/23] docs(conductor): record byte-order helper placement in detail (ply-binary-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 --- conductor/tracks.md | 2 +- .../tracks/ply-binary-io_20260904/metadata.json | 9 ++++++--- conductor/tracks/ply-binary-io_20260904/plan.md | 4 ++-- conductor/tracks/ply-binary-io_20260904/spec.md | 16 ++++++++++++++++ 4 files changed, 25 insertions(+), 6 deletions(-) diff --git a/conductor/tracks.md b/conductor/tracks.md index d0a5e93..9b5ffc2 100644 --- a/conductor/tracks.md +++ b/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 | +| [~] | 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 | diff --git a/conductor/tracks/ply-binary-io_20260904/metadata.json b/conductor/tracks/ply-binary-io_20260904/metadata.json index 7aa0b71..d25c082 100644 --- a/conductor/tracks/ply-binary-io_20260904/metadata.json +++ b/conductor/tracks/ply-binary-io_20260904/metadata.json @@ -2,10 +2,12 @@ "id": "ply-binary-io_20260904", "title": "PLY Binary IO", "type": "feature", - "status": "pending", + "status": "in_progress", "created": "2026-09-04T00:00:00+0200", - "updated": "2026-09-04T00:00:00+0200", + "updated": "2026-09-04T21:55:13Z", "issue": 25, + "current_phase": 1, + "current_task": "1.1", "phases": { "total": 5, "completed": 0 @@ -13,5 +15,6 @@ "tasks": { "total": 22, "completed": 0 - } + }, + "commits": [] } diff --git a/conductor/tracks/ply-binary-io_20260904/plan.md b/conductor/tracks/ply-binary-io_20260904/plan.md index 5f8b8bb..928ad78 100644 --- a/conductor/tracks/ply-binary-io_20260904/plan.md +++ b/conductor/tracks/ply-binary-io_20260904/plan.md @@ -3,7 +3,7 @@ **Track ID:** ply-binary-io_20260904 **Spec:** [spec.md](./spec.md) **Created:** 2026-09-04 -**Status:** [ ] Not Started +**Status:** [~] In Progress > 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 @@ -45,7 +45,7 @@ consumes them. ### Tasks -- [ ] **Task 1.1**: Decide where the helpers live — `namespace detail` in +- [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`. diff --git a/conductor/tracks/ply-binary-io_20260904/spec.md b/conductor/tracks/ply-binary-io_20260904/spec.md index 82aa8f2..1401d38 100644 --- a/conductor/tracks/ply-binary-io_20260904/spec.md +++ b/conductor/tracks/ply-binary-io_20260904/spec.md @@ -124,6 +124,22 @@ and `std::byteswap` is C++23, so neither is available; host-order detection and a width-dispatched swap have to be written against `__BYTE_ORDER__` with a `_WIN32` fallback. +### The helpers live in `detail`, not a public `utils/Endian.hpp` + +*(Task 1.1)* `host_is_little_endian()` and `swap_bytes()` go in +`namespace educelab::detail` in `MeshIO_PLY.hpp`, beside the other PLY-local +helpers, rather than in a new public `utils/Endian.hpp`. + +PLY binary IO is the only caller in the library, and byte order is not a problem +any other libcore component has. A public header would have to be added to +`public_hdrs` in `CMakeLists.txt`, installed, Doxygen-documented, and given its +own `TestEndian.cpp` target — permanent public surface and a support obligation +bought for one consumer. `detail` costs nothing and can be promoted the moment a +second caller appears; the reverse move is a breaking change. + +This also keeps the track's stated shape: no new files, no new test target, +tests in the existing `tests/src/TestMeshIO.cpp`. + ## Testing Strategy Round-trip tests cannot anchor this work. The sized-alias bug fixed in #24 From bff248d9626247301d395778960800233f0dfbf0 Mon Sep 17 00:00:00 2001 From: Seth Parker Date: Fri, 4 Sep 2026 17:57:05 -0400 Subject: [PATCH 03/23] test(MeshIO): byte-order helper tests for host detection and swapping (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 --- .../tracks/ply-binary-io_20260904/plan.md | 4 +- tests/src/TestMeshIO.cpp | 116 ++++++++++++++++++ 2 files changed, 118 insertions(+), 2 deletions(-) diff --git a/conductor/tracks/ply-binary-io_20260904/plan.md b/conductor/tracks/ply-binary-io_20260904/plan.md index 928ad78..f6e2aae 100644 --- a/conductor/tracks/ply-binary-io_20260904/plan.md +++ b/conductor/tracks/ply-binary-io_20260904/plan.md @@ -49,10 +49,10 @@ consumes them. `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`. -- [ ] **Task 1.2**: Tests for host-order detection and byte swapping across all +- [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. -- [ ] **Task 1.3**: Implement host-endianness detection (`__BYTE_ORDER__` where +- [~] **Task 1.3**: Implement host-endianness detection (`__BYTE_ORDER__` where the compiler defines it, `_WIN32` fallback) and the width-dispatched swap. ``/`` only — no new dependency, no new include of anything platform-specific. diff --git a/tests/src/TestMeshIO.cpp b/tests/src/TestMeshIO.cpp index 72f4a1a..ef4654c 100644 --- a/tests/src/TestMeshIO.cpp +++ b/tests/src/TestMeshIO.cpp @@ -3,6 +3,7 @@ #include #include #include +#include #include #include #include @@ -1115,6 +1116,121 @@ TEST(ExpandAtSeams, EmptyMesh_ReturnsEmptyExpanded) EXPECT_TRUE(flat.empty()); } +//------------------------------------------------------------------------------ +// Byte-order helpers (detail::host_is_little_endian, detail::swap_bytes) +// +// The byte-order foundation the binary PLY reader and writer both sit on. +// Tested directly rather than only through a PLY file, because a swap that is +// wrong in a way the writer shares would round-trip perfectly. +//------------------------------------------------------------------------------ + +// Independent probe of the host's byte order. Deliberately does not reuse the +// compile-time macros detail::host_is_little_endian() consults, so the two can +// disagree and the test can notice. +static auto probe_host_is_little_endian() -> bool +{ + const uint32_t v = 0x01020304U; + std::array bytes{}; + std::memcpy(bytes.data(), &v, 4); + return bytes[0] == 0x04; +} + +TEST(PLYByteOrder, HostOrderMatchesRuntimeProbe) +{ + EXPECT_EQ(detail::host_is_little_endian(), probe_host_is_little_endian()); +} + +TEST(PLYByteOrder, HostOrderIsCompileTimeConstant) +{ + // Used to derive a loop-invariant swap flag, so it must be usable in a + // constant expression rather than resolved per property read. + constexpr bool kLittle = detail::host_is_little_endian(); + EXPECT_EQ(kLittle, probe_host_is_little_endian()); +} + +TEST(PLYByteOrder, SwapBytes_OneByteIsNoOp) +{ + uint8_t v{0xAB}; + detail::swap_bytes(v); + EXPECT_EQ(v, 0xABu); + + int8_t sv{-2}; // 0xFE + detail::swap_bytes(sv); + EXPECT_EQ(sv, -2); +} + +TEST(PLYByteOrder, SwapBytes_TwoBytes) +{ + uint16_t v{0x1234}; + detail::swap_bytes(v); + EXPECT_EQ(v, 0x3412u); + + int16_t sv{0x0102}; + detail::swap_bytes(sv); + EXPECT_EQ(sv, static_cast(0x0201)); +} + +TEST(PLYByteOrder, SwapBytes_FourBytes) +{ + uint32_t v{0x12345678U}; + detail::swap_bytes(v); + EXPECT_EQ(v, 0x78563412U); + + int32_t sv{0x01020304}; + detail::swap_bytes(sv); + EXPECT_EQ(sv, 0x04030201); +} + +TEST(PLYByteOrder, SwapBytes_EightBytes) +{ + uint64_t v{0x0102030405060708ULL}; + detail::swap_bytes(v); + EXPECT_EQ(v, 0x0807060504030201ULL); +} + +TEST(PLYByteOrder, SwapBytes_Float) +{ + // 1.0f is 3F 80 00 00 big-endian; reversed it is 00 00 80 3F. + float f{1.f}; + detail::swap_bytes(f); + uint32_t bits{}; + std::memcpy(&bits, &f, 4); + // The assertion holds on either host: reversing the bytes and then + // reading them back as an integer of the same width is symmetric. + EXPECT_EQ(bits, 0x0000803FU); +} + +TEST(PLYByteOrder, SwapBytes_Double) +{ + // 1.0 is 3F F0 00 00 00 00 00 00 big-endian; reversed, 00 ... 00 F0 3F. + double d{1.}; + detail::swap_bytes(d); + uint64_t bits{}; + std::memcpy(&bits, &d, 8); + EXPECT_EQ(bits, 0x000000000000F03FULL); +} + +TEST(PLYByteOrder, SwapBytes_IsItsOwnInverse) +{ + // Two swaps must restore the original for every width the format uses. + uint16_t u16{0xBEEF}; + detail::swap_bytes(u16); + detail::swap_bytes(u16); + EXPECT_EQ(u16, 0xBEEFu); + + const float f_orig{-1234.5678f}; + float f{f_orig}; + detail::swap_bytes(f); + detail::swap_bytes(f); + EXPECT_EQ(f, f_orig); + + const double d_orig{3.14159265358979}; + double d{d_orig}; + detail::swap_bytes(d); + detail::swap_bytes(d); + EXPECT_EQ(d, d_orig); +} + //------------------------------------------------------------------------------ // PLY Round-Trip Test Fixture //------------------------------------------------------------------------------ From ba3a4c557f08b57f3e3d2414ede1d99d490e5294 Mon Sep 17 00:00:00 2001 From: Seth Parker Date: Fri, 4 Sep 2026 17:58:08 -0400 Subject: [PATCH 04/23] feat(MeshIO): hand-rolled byte-order detection and width-dispatched swap (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 --- .../tracks/ply-binary-io_20260904/plan.md | 8 +-- include/educelab/core/io/MeshIO_PLY.hpp | 71 +++++++++++++++++++ 2 files changed, 75 insertions(+), 4 deletions(-) diff --git a/conductor/tracks/ply-binary-io_20260904/plan.md b/conductor/tracks/ply-binary-io_20260904/plan.md index f6e2aae..99fb7e8 100644 --- a/conductor/tracks/ply-binary-io_20260904/plan.md +++ b/conductor/tracks/ply-binary-io_20260904/plan.md @@ -52,16 +52,16 @@ consumes them. - [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. -- [~] **Task 1.3**: Implement host-endianness detection (`__BYTE_ORDER__` where +- [x] **Task 1.3**: Implement host-endianness detection (`__BYTE_ORDER__` where the compiler defines it, `_WIN32` fallback) and the width-dispatched swap. ``/`` only — no new dependency, no new include of anything platform-specific. ### Verification -- [ ] Helper tests pass -- [ ] Build succeeds in Debug and Release -- [ ] Full existing suite still green (nothing consumes the helpers yet) +- [x] Helper tests pass +- [x] Build succeeds in Debug and Release +- [x] Full existing suite still green (nothing consumes the helpers yet) --- diff --git a/include/educelab/core/io/MeshIO_PLY.hpp b/include/educelab/core/io/MeshIO_PLY.hpp index 97171bb..27c04fc 100644 --- a/include/educelab/core/io/MeshIO_PLY.hpp +++ b/include/educelab/core/io/MeshIO_PLY.hpp @@ -10,6 +10,8 @@ #include #include #include +#include +#include #include #include "educelab/core/types/Color.hpp" @@ -24,6 +26,75 @@ namespace educelab namespace detail { +// ------------------------------------------------------------------------- +// Byte-order helpers +// ------------------------------------------------------------------------- +// +// The library targets cxx_std_17, where neither std::endian (C++20) nor +// std::byteswap (C++23) exists, so host-order detection and the swap are +// hand-rolled. They live here rather than in a public utils/ header because +// binary PLY IO is their only consumer. + +/** @brief True when the host stores multi-byte scalars least-significant + * byte first + * + * Resolved at compile time so callers can hoist the comparison against the + * file's declared order out of their read loops. + */ +inline constexpr auto host_is_little_endian() -> bool +{ +#if defined(__BYTE_ORDER__) && defined(__ORDER_LITTLE_ENDIAN__) && \ + defined(__ORDER_BIG_ENDIAN__) + static_assert( + __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__ or + __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__, + "MeshIO_PLY: mixed-endian hosts are not supported"); + return __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__; +#elif defined(_WIN32) + // MSVC defines no byte-order macro. Every Windows target (x86, x64, ARM, + // ARM64) is little-endian. + return true; +#else +#error "MeshIO_PLY: cannot determine host byte order" +#endif +} + +/** @brief Reverse the byte order of a fixed-width scalar in place + * + * Dispatched on @c sizeof(ScalarT) across the four widths the PLY format + * uses. A 1-byte scalar is a no-op, which keeps call sites free of a width + * check of their own. + * + * @tparam ScalarT Trivially copyable scalar of 1, 2, 4, or 8 bytes + */ +template +void swap_bytes(ScalarT& v) +{ + static_assert( + std::is_trivially_copyable_v, + "swap_bytes requires a trivially copyable type"); + static_assert( + sizeof(ScalarT) == 1 or sizeof(ScalarT) == 2 or + sizeof(ScalarT) == 4 or sizeof(ScalarT) == 8, + "swap_bytes supports 1-, 2-, 4-, and 8-byte scalars"); + + // Aliasing a scalar through unsigned char* is permitted; this reorders the + // object representation without forming a value of a narrower type. + auto* p = reinterpret_cast(&v); + if constexpr (sizeof(ScalarT) == 2) { + std::swap(p[0], p[1]); + } else if constexpr (sizeof(ScalarT) == 4) { + std::swap(p[0], p[3]); + std::swap(p[1], p[2]); + } else if constexpr (sizeof(ScalarT) == 8) { + std::swap(p[0], p[7]); + std::swap(p[1], p[6]); + std::swap(p[2], p[5]); + std::swap(p[3], p[4]); + } + // sizeof(ScalarT) == 1: nothing to reverse +} + /** @brief Convert a Color to {r, g, b} in [0, 255] uint8 range */ inline auto color_to_u8c3(const Color& c) -> std::array { From f48ccc49ccaac12cf0a56a62b2e2c6acf6ca9411 Mon Sep 17 00:00:00 2001 From: Seth Parker Date: Fri, 4 Sep 2026 17:58:17 -0400 Subject: [PATCH 05/23] chore(conductor): Phase 1 checkpoint (ply-binary-io_20260904) Co-Authored-By: Claude Opus 5 --- .../tracks/ply-binary-io_20260904/metadata.json | 16 ++++++++++------ conductor/tracks/ply-binary-io_20260904/plan.md | 2 +- 2 files changed, 11 insertions(+), 7 deletions(-) diff --git a/conductor/tracks/ply-binary-io_20260904/metadata.json b/conductor/tracks/ply-binary-io_20260904/metadata.json index d25c082..21fb94e 100644 --- a/conductor/tracks/ply-binary-io_20260904/metadata.json +++ b/conductor/tracks/ply-binary-io_20260904/metadata.json @@ -4,17 +4,21 @@ "type": "feature", "status": "in_progress", "created": "2026-09-04T00:00:00+0200", - "updated": "2026-09-04T21:55:13Z", + "updated": "2026-09-04T22:10:00Z", "issue": 25, - "current_phase": 1, - "current_task": "1.1", + "current_phase": 2, + "current_task": "2.1", "phases": { "total": 5, - "completed": 0 + "completed": 1 }, "tasks": { "total": 22, - "completed": 0 + "completed": 3 }, - "commits": [] + "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" + ] } diff --git a/conductor/tracks/ply-binary-io_20260904/plan.md b/conductor/tracks/ply-binary-io_20260904/plan.md index 99fb7e8..e0d42b1 100644 --- a/conductor/tracks/ply-binary-io_20260904/plan.md +++ b/conductor/tracks/ply-binary-io_20260904/plan.md @@ -28,7 +28,7 @@ All work is in `include/educelab/core/io/MeshIO_PLY.hpp` and | Phase | Checkpoint SHA | Date | Status | | ------- | -------------- | ---- | ------- | -| Phase 1 | | | pending | +| Phase 1 | `5861ee1` | 2026-09-04 | verified | | Phase 2 | | | pending | | Phase 3 | | | pending | | Phase 4 | | | pending | From 58b783b6d033e8932dd7114889b3348f95bbf70d Mon Sep 17 00:00:00 2001 From: Seth Parker Date: Fri, 4 Sep 2026 18:03:16 -0400 Subject: [PATCH 06/23] test(MeshIO): big-endian PLY read fixtures (ply-binary-io_20260904) 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 --- .../tracks/ply-binary-io_20260904/plan.md | 6 +- tests/src/TestMeshIO.cpp | 184 ++++++++++++++++++ 2 files changed, 187 insertions(+), 3 deletions(-) diff --git a/conductor/tracks/ply-binary-io_20260904/plan.md b/conductor/tracks/ply-binary-io_20260904/plan.md index e0d42b1..15bfaf5 100644 --- a/conductor/tracks/ply-binary-io_20260904/plan.md +++ b/conductor/tracks/ply-binary-io_20260904/plan.md @@ -73,15 +73,15 @@ passes through. ### Tasks -- [ ] **Task 2.1**: Hand-crafted `binary_big_endian` fixture test — bytes +- [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. -- [ ] **Task 2.2**: Test that the swap precedes the cast: a BE `float` +- [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. -- [ ] **Task 2.3**: Add a runtime `bool` swap parameter to +- [~] **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`. Not a template parameter — see spec. diff --git a/tests/src/TestMeshIO.cpp b/tests/src/TestMeshIO.cpp index ef4654c..5f03e51 100644 --- a/tests/src/TestMeshIO.cpp +++ b/tests/src/TestMeshIO.cpp @@ -1,5 +1,6 @@ #include +#include #include #include #include @@ -1551,6 +1552,189 @@ TEST_F(PLYTest, SizedTypeAliases_BinaryLittleEndian_Read) EXPECT_EQ(dst.face(0), (Mesh3f::Face{0, 1, 2})); } +// Append `v` to `f` most-significant byte first. +// +// Reverses the host layout by hand rather than calling detail::swap_bytes: +// the fixture has to encode big-endian independently of the code under test, +// or a wrong swap would agree with itself and the test would pass. +template +static void write_be(std::ostream& f, ScalarT v) +{ + std::array bytes{}; + std::memcpy(bytes.data(), &v, sizeof(ScalarT)); + if (probe_host_is_little_endian()) { + std::reverse(bytes.begin(), bytes.end()); + } + f.write(bytes.data(), sizeof(ScalarT)); +} + +TEST_F(PLYTest, BinaryBigEndian_Read) +{ + // A big-endian PLY read back on a little-endian host. Every scalar width + // the format uses appears: float (4) positions, double (8) normals, + // ushort (2) colors, uchar (1) list count, int (4) list values. + // + // Values are chosen so an unswapped read cannot accidentally pass: + // 1.0f is 3F 80 00 00, the normals are non-palindromic bit patterns, the + // colors are 0x0102 / 0x0304 / 0x0506, and the face indices 0/1/2 would + // become 0 / 16777216 / 33554432 and be rejected as out of range. + const auto path = ply("be_read"); + { + std::ofstream f(path, std::ios::binary); + f << "ply\n" + << "format binary_big_endian 1.0\n" + << "element vertex 3\n" + << "property float x\n" + << "property float y\n" + << "property float z\n" + << "property double nx\n" + << "property double ny\n" + << "property double nz\n" + << "property ushort red\n" + << "property ushort green\n" + << "property ushort blue\n" + << "element face 1\n" + << "property list uchar int vertex_indices\n" + << "end_header\n"; + + const float pos[3][3] = { + {0.f, 0.f, 0.f}, + {1.f, 0.f, 0.f}, + {0.f, 1.f, 0.f}}; + const double nrm[3][3] = { + {0.25, 0.5, 0.75}, + {-0.5, 0.25, 0.75}, + {0.75, -0.25, 0.5}}; + const uint16_t col[3][3] = { + {0x0102, 0x0304, 0x0506}, + {0x0708, 0x090A, 0x0B0C}, + {0x0D0E, 0x0F10, 0x1112}}; + + for (int vi = 0; vi < 3; ++vi) { + for (int c = 0; c < 3; ++c) { + write_be(f, pos[vi][c]); + } + for (int c = 0; c < 3; ++c) { + write_be(f, nrm[vi][c]); + } + for (int c = 0; c < 3; ++c) { + write_be(f, col[vi][c]); + } + } + + write_be(f, static_cast(3)); + write_be(f, static_cast(0)); + write_be(f, static_cast(1)); + write_be(f, static_cast(2)); + } + + NCMesh dst; + read_ply(path, dst); + + ASSERT_EQ(dst.num_vertices(), 3u); + ASSERT_EQ(dst.num_faces(), 1u); + + // 4-byte positions + EXPECT_NEAR(dst.vertex(1)[0], 1.f, 1e-6f); + EXPECT_NEAR(dst.vertex(1)[1], 0.f, 1e-6f); + EXPECT_NEAR(dst.vertex(2)[1], 1.f, 1e-6f); + + // 8-byte normals + ASSERT_TRUE(dst.vertex(0).normal.has_value()); + EXPECT_NEAR((*dst.vertex(0).normal)[0], 0.25f, 1e-6f); + EXPECT_NEAR((*dst.vertex(0).normal)[1], 0.5f, 1e-6f); + EXPECT_NEAR((*dst.vertex(0).normal)[2], 0.75f, 1e-6f); + ASSERT_TRUE(dst.vertex(2).normal.has_value()); + EXPECT_NEAR((*dst.vertex(2).normal)[1], -0.25f, 1e-6f); + + // 2-byte colors — preserved as U16C3, so the swap is visible in the value + ASSERT_TRUE(dst.vertex(0).color.has_value()); + EXPECT_EQ(dst.vertex(0).color.value()[0], 0x0102u); + EXPECT_EQ(dst.vertex(0).color.value()[1], 0x0304u); + EXPECT_EQ(dst.vertex(0).color.value()[2], 0x0506u); + EXPECT_EQ(dst.vertex(2).color.value()[2], 0x1112u); + + // 1-byte count and 4-byte list values + EXPECT_EQ(dst.face(0), (NCMesh::Face{0, 1, 2})); +} + +TEST_F(PLYTest, BinaryBigEndian_SwapPrecedesCast) +{ + // The trap the spec calls out. A big-endian float 1.0 is the byte sequence + // 3F 80 00 00. memcpy'd straight into a native little-endian float that is + // 4.6e-41 (a denormal), and no reversal of the widened double recovers + // 1.0 — the bytes have to be reordered while the value is still a float. + // + // The destination mesh is Mesh3d so the widening float -> double is real + // and the assertion would fail if the swap were applied after the cast. + const auto path = ply("be_precast"); + { + std::ofstream f(path, std::ios::binary); + f << "ply\n" + << "format binary_big_endian 1.0\n" + << "element vertex 1\n" + << "property float x\n" + << "property float y\n" + << "property float z\n" + << "element face 0\n" + << "property list uchar int vertex_indices\n" + << "end_header\n"; + // Literal big-endian bytes, not derived from a host float. + const std::array be_one{'\x3f', '\x80', '\x00', '\x00'}; + for (int i = 0; i < 3; ++i) { + f.write(be_one.data(), 4); + } + } + + Mesh3d dst; + read_ply(path, dst); + + ASSERT_EQ(dst.num_vertices(), 1u); + EXPECT_DOUBLE_EQ(dst.vertex(0)[0], 1.0); + EXPECT_DOUBLE_EQ(dst.vertex(0)[1], 1.0); + EXPECT_DOUBLE_EQ(dst.vertex(0)[2], 1.0); +} + +TEST_F(PLYTest, BinaryBigEndian_SwapPrecedesCast_StreamPath) +{ + // Same guarantee for the other choke point. read_ply_prop_from_buf serves + // the batched vertex record; read_ply_binary_prop serves the face record + // and the unknown-element skip. A face index list of int32 values reaches + // the stream path, so the same file exercises both. + const auto path = ply("be_precast_stream"); + { + std::ofstream f(path, std::ios::binary); + f << "ply\n" + << "format binary_big_endian 1.0\n" + << "element vertex 3\n" + << "property float x\n" + << "property float y\n" + << "property float z\n" + << "element face 1\n" + << "property list uchar int vertex_indices\n" + << "end_header\n"; + for (int vi = 0; vi < 3; ++vi) { + for (int c = 0; c < 3; ++c) { + write_be(f, 0.f); + } + } + // Big-endian int32 2, 1, 0 — literal bytes. Unswapped these read as + // 33554432, 16777216 and 0, and the reader would reject them. + const std::array face{ + '\x03', + '\x00', '\x00', '\x00', '\x02', + '\x00', '\x00', '\x00', '\x01', + '\x00', '\x00', '\x00', '\x00'}; + f.write(face.data(), face.size()); + } + + Mesh3f dst; + read_ply(path, dst); + + ASSERT_EQ(dst.num_faces(), 1u); + EXPECT_EQ(dst.face(0), (Mesh3f::Face{2, 1, 0})); +} + TEST_F(PLYTest, WriteWithUVMap_PerWedgeTexcoord) { // Two triangles sharing an edge with a UV seam. From 64ecde6f7e034b678a04b6116acd47d5e9ad7de8 Mon Sep 17 00:00:00 2001 From: Seth Parker Date: Fri, 4 Sep 2026 18:04:51 -0400 Subject: [PATCH 07/23] fix(MeshIO): honor the byte order declared in the PLY header on read (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 --- .../tracks/ply-binary-io_20260904/plan.md | 6 +- include/educelab/core/io/MeshIO_PLY.hpp | 145 +++++++++++------- 2 files changed, 93 insertions(+), 58 deletions(-) diff --git a/conductor/tracks/ply-binary-io_20260904/plan.md b/conductor/tracks/ply-binary-io_20260904/plan.md index 15bfaf5..0ca90dc 100644 --- a/conductor/tracks/ply-binary-io_20260904/plan.md +++ b/conductor/tracks/ply-binary-io_20260904/plan.md @@ -81,16 +81,16 @@ passes through. `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. -- [~] **Task 2.3**: Add a runtime `bool` swap parameter to +- [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`. Not a template parameter — see spec. -- [ ] **Task 2.4**: In `read_ply_impl`, compute the flag from `hdr.format` +- [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. -- [ ] **Task 2.5**: Replace `PLYTest.BinaryBigEndian_Throws` with the positive +- [~] **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". diff --git a/include/educelab/core/io/MeshIO_PLY.hpp b/include/educelab/core/io/MeshIO_PLY.hpp index 27c04fc..8641991 100644 --- a/include/educelab/core/io/MeshIO_PLY.hpp +++ b/include/educelab/core/io/MeshIO_PLY.hpp @@ -381,63 +381,77 @@ inline auto parse_ply_header(std::istream& file) -> PLYHeader } // ------------------------------------------------------------------------- -// Binary property reading (little-endian) +// Binary property reading // ------------------------------------------------------------------------- -/** @brief Read a single binary little-endian PLY scalar property from @p f */ +/** @brief Read a single binary PLY scalar property from @p f + * + * @p needs_swap must be true when the file's declared byte order differs from + * the host's. The swap is applied to the raw fixed-width value, **before** the + * cast to @c DestT: a big-endian float 1.0 (@c 3F @c 80 @c 00 @c 00) read + * natively is the denormal 4.6e-41, and no reversal of the widened value + * recovers it. + * + * It is a runtime parameter rather than a template one because the flag is + * loop-invariant, and it carries no default so a new call site cannot silently + * omit it. + */ template -auto read_ply_binary_prop(std::istream& f, PLYType type) -> DestT +auto read_ply_binary_prop(std::istream& f, PLYType type, bool needs_swap) + -> DestT { - const auto err = []() { - throw std::runtime_error("read_ply: unexpected end of binary data"); + // One raw read per width, so the swap cannot be forgotten in a single + // case of the dispatch below. + const auto read_raw = [&f, needs_swap](auto& v) { + f.read(reinterpret_cast(&v), sizeof(v)); + if (!f) { + throw std::runtime_error( + "read_ply: unexpected end of binary data"); + } + if (needs_swap) { + swap_bytes(v); + } }; + switch (type) { case PLYType::Float: { float v{}; - f.read(reinterpret_cast(&v), 4); - if (!f) err(); + read_raw(v); return static_cast(v); } case PLYType::Double: { double v{}; - f.read(reinterpret_cast(&v), 8); - if (!f) err(); + read_raw(v); return static_cast(v); } case PLYType::Int: { int32_t v{}; - f.read(reinterpret_cast(&v), 4); - if (!f) err(); + read_raw(v); return static_cast(v); } case PLYType::UInt: { uint32_t v{}; - f.read(reinterpret_cast(&v), 4); - if (!f) err(); + read_raw(v); return static_cast(v); } case PLYType::Short: { int16_t v{}; - f.read(reinterpret_cast(&v), 2); - if (!f) err(); + read_raw(v); return static_cast(v); } case PLYType::UShort: { uint16_t v{}; - f.read(reinterpret_cast(&v), 2); - if (!f) err(); + read_raw(v); return static_cast(v); } case PLYType::Char: { int8_t v{}; - f.read(reinterpret_cast(&v), 1); - if (!f) err(); + read_raw(v); return static_cast(v); } case PLYType::UChar: { uint8_t v{}; - f.read(reinterpret_cast(&v), 1); - if (!f) err(); + read_raw(v); return static_cast(v); } default: @@ -454,49 +468,61 @@ auto read_ply_binary_prop(std::istream& f, PLYType type) -> DestT * been read in one istream::read call. Each field is extracted by its * pre-computed byte offset within the buffer rather than via individual * istream::read calls. + * + * @p needs_swap follows the same contract as in @ref read_ply_binary_prop: + * true when the file's byte order differs from the host's, applied to the raw + * fixed-width value before the cast to @c DestT, and with no default. */ template -auto read_ply_prop_from_buf(const char* buf, PLYType type) -> DestT +auto read_ply_prop_from_buf(const char* buf, PLYType type, bool needs_swap) + -> DestT { + const auto load = [buf, needs_swap](auto& v) { + std::memcpy(&v, buf, sizeof(v)); + if (needs_swap) { + swap_bytes(v); + } + }; + switch (type) { case PLYType::Float: { float v; - std::memcpy(&v, buf, 4); + load(v); return static_cast(v); } case PLYType::Double: { double v; - std::memcpy(&v, buf, 8); + load(v); return static_cast(v); } case PLYType::Int: { int32_t v; - std::memcpy(&v, buf, 4); + load(v); return static_cast(v); } case PLYType::UInt: { uint32_t v; - std::memcpy(&v, buf, 4); + load(v); return static_cast(v); } case PLYType::Short: { int16_t v; - std::memcpy(&v, buf, 2); + load(v); return static_cast(v); } case PLYType::UShort: { uint16_t v; - std::memcpy(&v, buf, 2); + load(v); return static_cast(v); } case PLYType::Char: { int8_t v; - std::memcpy(&v, buf, 1); + load(v); return static_cast(v); } case PLYType::UChar: { uint8_t v; - std::memcpy(&v, buf, 1); + load(v); return static_cast(v); } default: @@ -527,12 +553,17 @@ constexpr std::size_t kMaxFaceListLength = 1024; * true, @p texcoords with raw float values. Both containers are cleared * before filling. Skips any list or scalar face properties that are not * vertex_indices or texcoord. + * + * @p needs_swap is forwarded to every scalar read; see + * @ref read_ply_binary_prop. Skipped properties are advanced over as bytes + * and need no swap. */ inline void read_ply_face_binary( std::istream& file, const PLYElement& elem, std::size_t n_vertices, bool load_texcoords, + bool needs_swap, std::vector& face, std::vector& texcoords) { @@ -544,8 +575,8 @@ inline void read_ply_face_binary( static_cast(ply_type_bytes(prop.type))); continue; } - const auto count = - read_ply_binary_prop(file, prop.list_count_type); + const auto count = read_ply_binary_prop( + file, prop.list_count_type, needs_swap); switch (prop.role) { case PropRole::VertexIndices: if (count > kMaxFaceVertices) { @@ -556,8 +587,8 @@ inline void read_ply_face_binary( } face.reserve(count); for (std::size_t k = 0; k < count; ++k) { - const auto idx = - read_ply_binary_prop(file, prop.type); + const auto idx = read_ply_binary_prop( + file, prop.type, needs_swap); if (idx >= n_vertices) { throw std::runtime_error( "read_ply: face vertex index " + @@ -578,8 +609,8 @@ inline void read_ply_face_binary( if (load_texcoords) { texcoords.resize(count); for (std::size_t k = 0; k < count; ++k) { - texcoords[k] = - read_ply_binary_prop(file, prop.type); + texcoords[k] = read_ply_binary_prop( + file, prop.type, needs_swap); } } else { file.ignore(static_cast( @@ -710,11 +741,15 @@ void read_ply_impl( } const auto hdr = parse_ply_header(file); - if (hdr.format == PLYHeader::Format::BinaryBE) { - throw std::runtime_error( - "read_ply: binary big-endian format is not supported"); - } - const bool binary = hdr.format == PLYHeader::Format::BinaryLE; + const bool binary = hdr.format == PLYHeader::Format::BinaryLE or + hdr.format == PLYHeader::Format::BinaryBE; + // Swap when the file's declared byte order differs from the host's. The + // flag is loop-invariant, so it is resolved once here and threaded to + // every binary scalar read: the batched vertex path, the face record, and + // the unknown-element skip. + const bool needs_swap = + binary and ((hdr.format == PLYHeader::Format::BinaryLE) != + host_is_little_endian()); // Populate texture paths from header if (texture_paths != nullptr) { @@ -789,8 +824,8 @@ void read_ply_impl( static_cast(ply_type_bytes(prop.type))); return; } - const auto count = - read_ply_binary_prop(file, prop.list_count_type); + const auto count = read_ply_binary_prop( + file, prop.list_count_type, needs_swap); // Bound before multiplying. A negative count from a signed count type // arrives here as a huge unsigned value, and the byte total would wrap // to a negative seek that skips nothing at all. @@ -876,27 +911,27 @@ void read_ply_impl( const char* pb = vbuf.data() + vert_offsets[pi]; switch (prop.role) { case PropRole::X: - x = read_ply_prop_from_buf(pb, prop.type); break; + x = read_ply_prop_from_buf(pb, prop.type, needs_swap); break; case PropRole::Y: - y = read_ply_prop_from_buf(pb, prop.type); break; + y = read_ply_prop_from_buf(pb, prop.type, needs_swap); break; case PropRole::Z: - z = read_ply_prop_from_buf(pb, prop.type); break; + z = read_ply_prop_from_buf(pb, prop.type, needs_swap); break; case PropRole::NX: - nx = read_ply_prop_from_buf(pb, prop.type); break; + nx = read_ply_prop_from_buf(pb, prop.type, needs_swap); break; case PropRole::NY: - ny = read_ply_prop_from_buf(pb, prop.type); break; + ny = read_ply_prop_from_buf(pb, prop.type, needs_swap); break; case PropRole::NZ: - nz = read_ply_prop_from_buf(pb, prop.type); break; + nz = read_ply_prop_from_buf(pb, prop.type, needs_swap); break; case PropRole::Red: - r = read_ply_prop_from_buf(pb, prop.type); break; + r = read_ply_prop_from_buf(pb, prop.type, needs_swap); break; case PropRole::Green: - g = read_ply_prop_from_buf(pb, prop.type); break; + g = read_ply_prop_from_buf(pb, prop.type, needs_swap); break; case PropRole::Blue: - b = read_ply_prop_from_buf(pb, prop.type); break; + b = read_ply_prop_from_buf(pb, prop.type, needs_swap); break; case PropRole::S: - s = read_ply_prop_from_buf(pb, prop.type); break; + s = read_ply_prop_from_buf(pb, prop.type, needs_swap); break; case PropRole::T: - t = read_ply_prop_from_buf(pb, prop.type); break; + t = read_ply_prop_from_buf(pb, prop.type, needs_swap); break; default: break; // unknown: in buffer, role ignored } @@ -991,7 +1026,7 @@ void read_ply_impl( for (std::size_t fi = 0; fi < elem.count; ++fi) { if (binary) { read_ply_face_binary( - file, elem, n_vertices, load_texcoords, + file, elem, n_vertices, load_texcoords, needs_swap, face_indices, texcoords); } else { std::string_view sv; From 99dbc5b9f8731af5836c3f89727743301b988dfe Mon Sep 17 00:00:00 2001 From: Seth Parker Date: Fri, 4 Sep 2026 18:05:44 -0400 Subject: [PATCH 08/23] test(MeshIO): drop the big-endian rejection test, document byte-order 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 --- .../tracks/ply-binary-io_20260904/plan.md | 10 ++++----- include/educelab/core/io/MeshIO_PLY.hpp | 4 +++- tests/src/TestMeshIO.cpp | 22 ------------------- 3 files changed, 8 insertions(+), 28 deletions(-) diff --git a/conductor/tracks/ply-binary-io_20260904/plan.md b/conductor/tracks/ply-binary-io_20260904/plan.md index 0ca90dc..cd9f4fc 100644 --- a/conductor/tracks/ply-binary-io_20260904/plan.md +++ b/conductor/tracks/ply-binary-io_20260904/plan.md @@ -90,17 +90,17 @@ passes through. every call site — the batched vertex path, `read_ply_face_binary`, and the `skip_binary_prop` lambda. Missing one leaves a silently misread property. -- [~] **Task 2.5**: Replace `PLYTest.BinaryBigEndian_Throws` with the positive +- [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 -- [ ] BE fixture and pre-cast-swap tests pass -- [ ] `BinaryLittleEndian_Read` and `SizedTypeAliases_BinaryLittleEndian_Read` +- [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 -- [ ] No remaining reference to the BE rejection in code or docs -- [ ] Full suite green, Debug and Release +- [x] No remaining reference to the BE rejection in code or docs +- [x] Full suite green, Debug and Release --- diff --git a/include/educelab/core/io/MeshIO_PLY.hpp b/include/educelab/core/io/MeshIO_PLY.hpp index 8641991..97a6868 100644 --- a/include/educelab/core/io/MeshIO_PLY.hpp +++ b/include/educelab/core/io/MeshIO_PLY.hpp @@ -1378,7 +1378,9 @@ void write_ply( /** * @brief Read a PLY file into a mesh (Tier 1 — positions only) * - * Supports ASCII and binary-little-endian PLY. Parses @c x @c y @c z vertex + * Supports ASCII and binary PLY in either byte order. Binary files are read + * according to the byte order declared in the header, byte-swapping when it + * differs from the host's. Parses @c x @c y @c z vertex * properties; populates @ref traits::WithNormal and @ref traits::WithColor * vertex fields when present in the file and the @c Vertex type supports them. * All other properties are skipped. diff --git a/tests/src/TestMeshIO.cpp b/tests/src/TestMeshIO.cpp index 5f03e51..0d526b4 100644 --- a/tests/src/TestMeshIO.cpp +++ b/tests/src/TestMeshIO.cpp @@ -1986,28 +1986,6 @@ TEST_F(PLYTest, MalformedHeader_Throws) EXPECT_THROW(read_ply(path, dst), std::runtime_error); } -TEST_F(PLYTest, BinaryBigEndian_Throws) -{ - const auto path = ply("big_endian"); - { - std::ofstream f(path, std::ios::binary); - f << "ply\n" - << "format binary_big_endian 1.0\n" - << "element vertex 3\n" - << "property float x\n" - << "property float y\n" - << "property float z\n" - << "element face 1\n" - << "property list uchar int vertex_indices\n" - << "end_header\n"; - // Some placeholder data (will never be read) - const float verts[9] = {}; - f.write(reinterpret_cast(verts), sizeof(verts)); - } - Mesh3f dst; - EXPECT_THROW(read_ply(path, dst), std::runtime_error); -} - //------------------------------------------------------------------------------ // Task 4.1 — read_mesh / write_mesh convenience facade tests //------------------------------------------------------------------------------ From ea448c12f66cff75c03094fb78f376b53060e9d9 Mon Sep 17 00:00:00 2001 From: Seth Parker Date: Fri, 4 Sep 2026 18:05:44 -0400 Subject: [PATCH 09/23] chore(conductor): Phase 2 checkpoint (ply-binary-io_20260904) Co-Authored-By: Claude Opus 5 --- .../tracks/ply-binary-io_20260904/metadata.json | 15 +++++++++------ conductor/tracks/ply-binary-io_20260904/plan.md | 2 +- 2 files changed, 10 insertions(+), 7 deletions(-) diff --git a/conductor/tracks/ply-binary-io_20260904/metadata.json b/conductor/tracks/ply-binary-io_20260904/metadata.json index 21fb94e..abb56f9 100644 --- a/conductor/tracks/ply-binary-io_20260904/metadata.json +++ b/conductor/tracks/ply-binary-io_20260904/metadata.json @@ -4,21 +4,24 @@ "type": "feature", "status": "in_progress", "created": "2026-09-04T00:00:00+0200", - "updated": "2026-09-04T22:10:00Z", + "updated": "2026-09-04T22:30:00Z", "issue": 25, - "current_phase": 2, - "current_task": "2.1", + "current_phase": 3, + "current_task": "3.1", "phases": { "total": 5, - "completed": 1 + "completed": 2 }, "tasks": { "total": 22, - "completed": 3 + "completed": 8 }, "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" + "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" ] } diff --git a/conductor/tracks/ply-binary-io_20260904/plan.md b/conductor/tracks/ply-binary-io_20260904/plan.md index cd9f4fc..5f7b24b 100644 --- a/conductor/tracks/ply-binary-io_20260904/plan.md +++ b/conductor/tracks/ply-binary-io_20260904/plan.md @@ -29,7 +29,7 @@ All work is in `include/educelab/core/io/MeshIO_PLY.hpp` and | Phase | Checkpoint SHA | Date | Status | | ------- | -------------- | ---- | ------- | | Phase 1 | `5861ee1` | 2026-09-04 | verified | -| Phase 2 | | | pending | +| Phase 2 | `3483f88` | 2026-09-04 | verified | | Phase 3 | | | pending | | Phase 4 | | | pending | | Phase 5 | | | pending | From 4404d64fa87687aeff645ea1a5a11e9be381591b Mon Sep 17 00:00:00 2001 From: Seth Parker Date: Fri, 4 Sep 2026 18:07:55 -0400 Subject: [PATCH 10/23] test(MeshIO): face list-count limit tests for the PLY writer (ply-binary-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 --- .../tracks/ply-binary-io_20260904/plan.md | 4 +- tests/src/TestMeshIO.cpp | 151 ++++++++++++++++++ 2 files changed, 153 insertions(+), 2 deletions(-) diff --git a/conductor/tracks/ply-binary-io_20260904/plan.md b/conductor/tracks/ply-binary-io_20260904/plan.md index 5f7b24b..07ec51a 100644 --- a/conductor/tracks/ply-binary-io_20260904/plan.md +++ b/conductor/tracks/ply-binary-io_20260904/plan.md @@ -112,11 +112,11 @@ of. ASCII has the same latent problem and gets the same guard. ### Tasks -- [ ] **Task 3.1**: Tests for both limits and both boundaries — a 256-corner +- [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. -- [ ] **Task 3.2**: Implement the two limits in the write path. The message +- [~] **Task 3.2**: Implement the two limits in the write path. The message names which limit fired and the offending face index. - [ ] **Task 3.3**: Confirm coherence with the reader's existing caps (`kMaxFaceVertices` = 256, `kMaxFaceListLength` = 1024 in diff --git a/tests/src/TestMeshIO.cpp b/tests/src/TestMeshIO.cpp index 0d526b4..2a2e40a 100644 --- a/tests/src/TestMeshIO.cpp +++ b/tests/src/TestMeshIO.cpp @@ -2,12 +2,16 @@ #include #include +#include #include #include #include #include #include #include +#include +#include +#include #if defined(__unix__) || defined(__APPLE__) #include @@ -178,6 +182,34 @@ static auto make_triangle_uvmap(const Mesh3f& /*m*/) -> UVMap2f return uv; } +// Single n-gon face with `n` corners on a unit circle. Used to walk the uchar +// list-count boundaries the PLY writer has to enforce. +static auto make_ngon(std::size_t n) -> Mesh3f +{ + Mesh3f m; + Mesh3f::Face idx(n); + for (std::size_t i = 0; i < n; ++i) { + const auto a = 2.f * 3.14159265f * static_cast(i) / + static_cast(n); + (void)m.insert_vertex(std::cos(a), std::sin(a), 0.f); + idx[i] = i; + } + (void)m.insert_face(idx); + return m; +} + +// Per-wedge UVs for the single face of make_ngon(n) — every corner mapped. +static auto make_ngon_uvmap(std::size_t n) -> UVMap2f +{ + UVMap2f uv; + for (std::size_t i = 0; i < n; ++i) { + const auto t = static_cast(i) / static_cast(n); + const auto pool = uv.insert(t, 1.f - t); + uv.map(0, i, pool); + } + return uv; +} + //------------------------------------------------------------------------------ // Task 2.1 — OBJ Round-Trip Tests //------------------------------------------------------------------------------ @@ -1735,6 +1767,125 @@ TEST_F(PLYTest, BinaryBigEndian_SwapPrecedesCast_StreamPath) EXPECT_EQ(dst.face(0), (Mesh3f::Face{2, 1, 0})); } +//------------------------------------------------------------------------------ +// Face list-count limits +// +// write_ply declares both of its list properties with a `uchar` count: +// vertex_indices writes N, texcoord writes 2*N. A count above 255 would +// truncate silently and produce a file no reader can make sense of, so the +// writer rejects it. The limits are therefore 255 corners without UVs and 127 +// with them. +//------------------------------------------------------------------------------ + +TEST_F(PLYTest, FaceOver255Corners_Throws) +{ + const auto src = make_ngon(256); + const auto path = ply("ngon256"); + + try { + write_ply(path, src); + FAIL() << "expected write_ply to reject a 256-corner face"; + } catch (const std::runtime_error& e) { + const std::string msg = e.what(); + EXPECT_NE(msg.find("vertex_indices"), std::string::npos) << msg; + EXPECT_NE(msg.find("face 0"), std::string::npos) << msg; + EXPECT_NE(msg.find("256"), std::string::npos) << msg; + } + + // Validated before the stream is opened, so a mesh that cannot be written + // leaves no truncated file behind. + EXPECT_FALSE(fs::exists(path)); +} + +TEST_F(PLYTest, FaceWith255Corners_WritesAndReadsBack) +{ + // The largest face the uchar count can express. Also the Task 3.3 + // coherence check: the reader's own cap is kMaxFaceVertices = 256, so + // everything the writer now permits stays readable. + const auto src = make_ngon(255); + const auto path = ply("ngon255"); + ASSERT_NO_THROW(write_ply(path, src)); + + Mesh3f dst; + read_ply(path, dst); + ASSERT_EQ(dst.num_vertices(), 255u); + ASSERT_EQ(dst.num_faces(), 1u); + EXPECT_EQ(dst.face(0).size(), 255u); + EXPECT_EQ(dst.face(0).back(), 254u); +} + +TEST_F(PLYTest, FaceOver127CornersWithUVs_Throws) +{ + // 128 corners is legal for vertex_indices but its texcoord list would be + // 256 values, one past the uchar count. + const auto src = make_ngon(128); + const auto uv = make_ngon_uvmap(128); + const auto path = ply("ngon128_uv"); + + try { + write_ply(path, src, uv); + FAIL() << "expected write_ply to reject a 128-corner face with UVs"; + } catch (const std::runtime_error& e) { + const std::string msg = e.what(); + EXPECT_NE(msg.find("texcoord"), std::string::npos) << msg; + EXPECT_NE(msg.find("face 0"), std::string::npos) << msg; + } + + EXPECT_FALSE(fs::exists(path)); +} + +TEST_F(PLYTest, FaceOver127CornersWithUVs_Tier3_Throws) +{ + // Tier 3 takes the same guard. Asserted separately because a missed call + // site there would fail silently rather than at compile time. + const auto src = make_ngon(128); + const auto uv = make_ngon_uvmap(128); + const auto path = ply("ngon128_uv_tex"); + + try { + write_ply(path, src, uv, fs::path("tex.png")); + FAIL() << "expected write_ply tier 3 to reject the same face"; + } catch (const std::runtime_error& e) { + const std::string msg = e.what(); + EXPECT_NE(msg.find("texcoord"), std::string::npos) << msg; + } + + EXPECT_FALSE(fs::exists(path)); +} + +TEST_F(PLYTest, FaceWith127CornersWithUVs_WritesAndReadsBack) +{ + // The largest face that can carry UVs: 127 corners, 254 texcoord values. + // The reader's texcoord cap is kMaxFaceListLength = 1024, well clear. + const auto src = make_ngon(127); + const auto uv = make_ngon_uvmap(127); + const auto path = ply("ngon127_uv"); + ASSERT_NO_THROW(write_ply(path, src, uv)); + + Mesh3f dst; + UVMap2f dst_uv; + read_ply(path, dst, dst_uv); + ASSERT_EQ(dst.num_faces(), 1u); + EXPECT_EQ(dst.face(0).size(), 127u); + ASSERT_TRUE(dst_uv.has(0, 0)); + ASSERT_TRUE(dst_uv.has(0, 126)); + EXPECT_NEAR(dst_uv.at(dst_uv.get(0, 0))[0], 0.f, 1e-5f); + EXPECT_NEAR(dst_uv.at(dst_uv.get(0, 126))[0], 126.f / 127.f, 1e-5f); +} + +TEST_F(PLYTest, FaceWith128CornersNoUVs_WritesCleanly) +{ + // 128 corners only trips the texcoord limit, so tier 1 must still accept + // it — the two limits are independent. + const auto src = make_ngon(128); + const auto path = ply("ngon128"); + ASSERT_NO_THROW(write_ply(path, src)); + + Mesh3f dst; + read_ply(path, dst); + EXPECT_EQ(dst.face(0).size(), 128u); +} + TEST_F(PLYTest, WriteWithUVMap_PerWedgeTexcoord) { // Two triangles sharing an edge with a UV seam. From 18844bf7042133608a2771a4f5a6b841d92f78c3 Mon Sep 17 00:00:00 2001 From: Seth Parker Date: Fri, 4 Sep 2026 18:09:19 -0400 Subject: [PATCH 11/23] feat(MeshIO): reject face list counts a uchar cannot express (ply-binary-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 --- .../tracks/ply-binary-io_20260904/plan.md | 12 ++-- include/educelab/core/io/MeshIO_PLY.hpp | 63 +++++++++++++++++++ 2 files changed, 69 insertions(+), 6 deletions(-) diff --git a/conductor/tracks/ply-binary-io_20260904/plan.md b/conductor/tracks/ply-binary-io_20260904/plan.md index 07ec51a..7fb3afb 100644 --- a/conductor/tracks/ply-binary-io_20260904/plan.md +++ b/conductor/tracks/ply-binary-io_20260904/plan.md @@ -116,19 +116,19 @@ of. ASCII has the same latent problem and gets the same guard. 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. -- [~] **Task 3.2**: Implement the two limits in the write path. The message +- [x] **Task 3.2**: Implement the two limits in the write path. The message names which limit fired and the offending face index. -- [ ] **Task 3.3**: Confirm coherence with the reader's existing caps +- [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 -- [ ] Limit tests pass, including both non-throwing boundary cases -- [ ] A maximal legal face round-trips through both ASCII and (once Phase 4 - lands) binary -- [ ] Full suite green, Debug and Release +- [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 --- diff --git a/include/educelab/core/io/MeshIO_PLY.hpp b/include/educelab/core/io/MeshIO_PLY.hpp index 97a6868..a2c5dc1 100644 --- a/include/educelab/core/io/MeshIO_PLY.hpp +++ b/include/educelab/core/io/MeshIO_PLY.hpp @@ -540,6 +540,9 @@ auto read_ply_prop_from_buf(const char* buf, PLYType type, bool needs_swap) // PLY permits a signed count type, and a negative count converted to an // unsigned byte total wraps to a seek that skips nothing, leaving the reader // misaligned inside the element it meant to step over. +// +// Named at namespace scope so @ref validate_ply_face_lists can assert the +// writer's own limits stay inside them; the two must not drift apart. /** @brief Largest @c vertex_indices count @ref read_ply will accept */ constexpr std::size_t kMaxFaceVertices = 256; @@ -1090,6 +1093,60 @@ void read_ply_impl( } } +/** + * @brief Reject faces whose PLY list counts would not fit in a @c uchar + * + * Both list properties @ref write_ply emits declare a @c uchar count: + * @c vertex_indices writes @c N and @c texcoord writes @c 2*N. A count above + * 255 cannot be expressed, so it is rejected rather than silently truncated + * into a file no reader can make sense of. The limits are 255 corners without + * UVs and 127 with them. + * + * Called by every @c write_ply tier *before* the output stream is opened, so a + * mesh that cannot be written leaves no truncated file behind. The extra pass + * over the faces reads only @c size() and is negligible beside the write it + * guards. + * + * @param has_uvs True when a @c texcoord list will be written (tiers 2 and 3) + * @throws std::runtime_error naming the limit that fired and the face index + */ +template +void validate_ply_face_lists( + const Mesh& mesh, bool has_uvs) +{ + // Widest value a uchar list count can express. + constexpr std::size_t kMaxListCount = 255; + constexpr std::size_t kMaxUVCorners = kMaxListCount / 2; // 2*N per face + + // Nothing the writer permits may be refused on read. Checked here rather + // than trusted, so raising either limit without revisiting the reader's + // caps is a compile error instead of an unreadable file. + static_assert( + kMaxListCount <= kMaxFaceVertices, + "write_ply's vertex_indices limit exceeds read_ply's face vertex cap"); + static_assert( + 2 * kMaxUVCorners <= kMaxFaceListLength, + "write_ply's texcoord limit exceeds read_ply's face list cap"); + + for (std::size_t fi = 0; fi < mesh.num_faces(); ++fi) { + const auto n = mesh.face(fi).size(); + if (n > kMaxListCount) { + throw std::runtime_error( + "write_ply: face " + to_string(fi) + " has " + to_string(n) + + " corners, exceeding the " + to_string(kMaxListCount) + + " a uchar vertex_indices list count can express"); + } + if (has_uvs and n > kMaxUVCorners) { + throw std::runtime_error( + "write_ply: face " + to_string(fi) + " has " + to_string(n) + + " corners, whose texcoord list of " + to_string(2 * n) + + " values exceeds the " + to_string(kMaxListCount) + + " a uchar list count can express (" + + to_string(kMaxUVCorners) + " corners max with UVs)"); + } + } +} + /** * @brief Write the PLY ASCII header to @p file * @@ -1247,6 +1304,8 @@ void write_ply( { static_assert(Dims >= 3, "write_ply requires Dims >= 3"); + detail::validate_ply_face_lists(mesh, false); + std::ofstream file(path); if (!file) { throw std::runtime_error( @@ -1295,6 +1354,8 @@ void write_ply( { static_assert(Dims >= 3, "write_ply requires Dims >= 3"); + detail::validate_ply_face_lists(mesh, true); + std::ofstream file(path); if (!file) { throw std::runtime_error( @@ -1347,6 +1408,8 @@ void write_ply( { static_assert(Dims >= 3, "write_ply requires Dims >= 3"); + detail::validate_ply_face_lists(mesh, true); + std::ofstream file(path); if (!file) { throw std::runtime_error( From 64eba76850ad4945b9a5c5b6e7692ac0ba883394 Mon Sep 17 00:00:00 2001 From: Seth Parker Date: Fri, 4 Sep 2026 18:09:19 -0400 Subject: [PATCH 12/23] chore(conductor): Phase 3 checkpoint (ply-binary-io_20260904) Co-Authored-By: Claude Opus 5 --- .../tracks/ply-binary-io_20260904/metadata.json | 14 ++++++++------ conductor/tracks/ply-binary-io_20260904/plan.md | 2 +- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/conductor/tracks/ply-binary-io_20260904/metadata.json b/conductor/tracks/ply-binary-io_20260904/metadata.json index abb56f9..cd41673 100644 --- a/conductor/tracks/ply-binary-io_20260904/metadata.json +++ b/conductor/tracks/ply-binary-io_20260904/metadata.json @@ -4,17 +4,17 @@ "type": "feature", "status": "in_progress", "created": "2026-09-04T00:00:00+0200", - "updated": "2026-09-04T22:30:00Z", + "updated": "2026-09-04T22:45:00Z", "issue": 25, - "current_phase": 3, - "current_task": "3.1", + "current_phase": 4, + "current_task": "4.1", "phases": { "total": 5, - "completed": 2 + "completed": 3 }, "tasks": { "total": 22, - "completed": 8 + "completed": 11 }, "commits": [ "b28d606: docs(conductor): record byte-order helper placement in detail", @@ -22,6 +22,8 @@ "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" + "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" ] } diff --git a/conductor/tracks/ply-binary-io_20260904/plan.md b/conductor/tracks/ply-binary-io_20260904/plan.md index 7fb3afb..fe42339 100644 --- a/conductor/tracks/ply-binary-io_20260904/plan.md +++ b/conductor/tracks/ply-binary-io_20260904/plan.md @@ -30,7 +30,7 @@ All work is in `include/educelab/core/io/MeshIO_PLY.hpp` and | ------- | -------------- | ---- | ------- | | Phase 1 | `5861ee1` | 2026-09-04 | verified | | Phase 2 | `3483f88` | 2026-09-04 | verified | -| Phase 3 | | | pending | +| Phase 3 | `7583422` | 2026-09-04 | verified | | Phase 4 | | | pending | | Phase 5 | | | pending | From 7f3a3a5fc3749c08802d80e5d6bdc50e48b03569 Mon Sep 17 00:00:00 2001 From: Seth Parker Date: Fri, 4 Sep 2026 18:13:08 -0400 Subject: [PATCH 13/23] feat(MeshIO): add public PLYFormat and thread it through write_ply (ply-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 --- .../tracks/ply-binary-io_20260904/plan.md | 10 ++-- include/educelab/core/io/MeshIO_PLY.hpp | 54 ++++++++++++++----- 2 files changed, 47 insertions(+), 17 deletions(-) diff --git a/conductor/tracks/ply-binary-io_20260904/plan.md b/conductor/tracks/ply-binary-io_20260904/plan.md index fe42339..f52ae96 100644 --- a/conductor/tracks/ply-binary-io_20260904/plan.md +++ b/conductor/tracks/ply-binary-io_20260904/plan.md @@ -140,21 +140,21 @@ path is untouched until Task 4.6. ### Tasks -- [ ] **Task 4.1**: Add public `enum class PLYFormat { ASCII, Binary }` and +- [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`. -- [ ] **Task 4.2**: Byte-level writer test — a small positions-only mesh +- [~] **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. -- [ ] **Task 4.3**: Test that scalars are `float32` regardless of `T` — write a +- [~] **Task 4.3**: Test that scalars are `float32` regardless of `T` — write a `Mesh` and assert 4-byte scalars and a `property float x` declaration. -- [ ] **Task 4.4**: Test that `ASCII` remains the default — an unqualified +- [~] **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. -- [ ] **Task 4.5**: Round-trip tests for structural breadth — n-gon faces, UVs, +- [~] **Task 4.5**: Round-trip tests for structural breadth — n-gon faces, UVs, colors, normals, empty mesh — where hand-computing bytes stops paying. - [ ] **Task 4.6**: `write_ply_header` emits the `format` line matching the requested format and the host's byte order. diff --git a/include/educelab/core/io/MeshIO_PLY.hpp b/include/educelab/core/io/MeshIO_PLY.hpp index a2c5dc1..fd9dfb0 100644 --- a/include/educelab/core/io/MeshIO_PLY.hpp +++ b/include/educelab/core/io/MeshIO_PLY.hpp @@ -23,6 +23,22 @@ namespace educelab { +/** + * @brief Encoding of a PLY file's data section, selected on write + * + * @c Binary writes the host's native byte order and labels the header to + * match, so a file written on a little-endian host declares + * @c "format binary_little_endian 1.0". @ref read_ply honors either order, so + * the choice does not limit who can read the result. + * + * Kept distinct from @c detail::PLYHeader::Format, which has a third value and + * answers "what did I parse" rather than "what should I emit". + */ +enum class PLYFormat { + ASCII, ///< ASCII text + Binary ///< Binary, in the host's native byte order +}; + namespace detail { @@ -1156,7 +1172,10 @@ void validate_ply_face_lists( * declared on the face element. The @p has_normals flag (from * @ref has_any_normal) controls whether @c nx/ny/nz are declared; the * @p has_colors flag (from @ref has_any_color) controls whether - * @c red/green/blue are declared. + * @c red/green/blue are declared. @p format selects the @c format line; the + * property declarations are identical either way, since binary writes the same + * @c float32 scalars, @c uchar colors and @c int32 indices the ASCII header + * already declares. */ template void write_ply_header( @@ -1165,7 +1184,8 @@ void write_ply_header( const std::string& texture_comment, bool has_uvs, [[maybe_unused]] bool has_normals, - [[maybe_unused]] bool has_colors) + [[maybe_unused]] bool has_colors, + [[maybe_unused]] PLYFormat format) { using Vertex = typename Mesh::Vertex; @@ -1223,7 +1243,8 @@ void write_ply_data( const Mesh& mesh, const UVMapT* uvmap, [[maybe_unused]] bool has_normals, - [[maybe_unused]] bool has_colors) + [[maybe_unused]] bool has_colors, + [[maybe_unused]] PLYFormat format) { using Vertex = typename Mesh::Vertex; @@ -1300,7 +1321,9 @@ void write_ply_data( */ template void write_ply( - const std::filesystem::path& path, const Mesh& mesh) + const std::filesystem::path& path, + const Mesh& mesh, + PLYFormat format = PLYFormat::ASCII) { static_assert(Dims >= 3, "write_ply requires Dims >= 3"); @@ -1315,10 +1338,11 @@ void write_ply( std::array buf{}; const bool has_normals = has_any_normal(mesh); const bool has_colors = has_any_color(mesh); - detail::write_ply_header(file, mesh, "", false, has_normals, has_colors); + detail::write_ply_header( + file, mesh, "", false, has_normals, has_colors, format); detail::write_ply_data( file, buf, mesh, static_cast*>(nullptr), - has_normals, has_colors); + has_normals, has_colors, format); // Close before checking. The stream may still hold buffered data at this // point; the final flush happens when `file` is destroyed, and a failure @@ -1350,7 +1374,8 @@ template void write_ply( const std::filesystem::path& path, const Mesh& mesh, - const UVMapT& uvmap) + const UVMapT& uvmap, + PLYFormat format = PLYFormat::ASCII) { static_assert(Dims >= 3, "write_ply requires Dims >= 3"); @@ -1365,8 +1390,10 @@ void write_ply( std::array buf{}; const bool has_normals = has_any_normal(mesh); const bool has_colors = has_any_color(mesh); - detail::write_ply_header(file, mesh, "", true, has_normals, has_colors); - detail::write_ply_data(file, buf, mesh, &uvmap, has_normals, has_colors); + detail::write_ply_header( + file, mesh, "", true, has_normals, has_colors, format); + detail::write_ply_data( + file, buf, mesh, &uvmap, has_normals, has_colors, format); // Close before checking. The stream may still hold buffered data at this // point; the final flush happens when `file` is destroyed, and a failure @@ -1404,7 +1431,8 @@ void write_ply( const std::filesystem::path& path, const Mesh& mesh, const UVMapT& uvmap, - const std::filesystem::path& texture_path) + const std::filesystem::path& texture_path, + PLYFormat format = PLYFormat::ASCII) { static_assert(Dims >= 3, "write_ply requires Dims >= 3"); @@ -1420,8 +1448,10 @@ void write_ply( const bool has_normals = has_any_normal(mesh); const bool has_colors = has_any_color(mesh); detail::write_ply_header( - file, mesh, texture_path.string(), true, has_normals, has_colors); - detail::write_ply_data(file, buf, mesh, &uvmap, has_normals, has_colors); + file, mesh, texture_path.string(), true, has_normals, has_colors, + format); + detail::write_ply_data( + file, buf, mesh, &uvmap, has_normals, has_colors, format); // Close before checking. The stream may still hold buffered data at this // point; the final flush happens when `file` is destroyed, and a failure From 673c6a6435999e68936c40d102e646009c815b48 Mon Sep 17 00:00:00 2001 From: Seth Parker Date: Fri, 4 Sep 2026 18:14:39 -0400 Subject: [PATCH 14/23] test(MeshIO): binary PLY write tests (ply-binary-io_20260904) 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 --- .../tracks/ply-binary-io_20260904/plan.md | 10 +- tests/src/TestMeshIO.cpp | 355 ++++++++++++++++++ 2 files changed, 360 insertions(+), 5 deletions(-) diff --git a/conductor/tracks/ply-binary-io_20260904/plan.md b/conductor/tracks/ply-binary-io_20260904/plan.md index f52ae96..7f650c8 100644 --- a/conductor/tracks/ply-binary-io_20260904/plan.md +++ b/conductor/tracks/ply-binary-io_20260904/plan.md @@ -144,19 +144,19 @@ path is untouched until Task 4.6. 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`. -- [~] **Task 4.2**: Byte-level writer test — a small positions-only mesh +- [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. -- [~] **Task 4.3**: Test that scalars are `float32` regardless of `T` — write a +- [x] **Task 4.3**: Test that scalars are `float32` regardless of `T` — write a `Mesh` and assert 4-byte scalars and a `property float x` declaration. -- [~] **Task 4.4**: Test that `ASCII` remains the default — an unqualified +- [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. -- [~] **Task 4.5**: Round-trip tests for structural breadth — n-gon faces, UVs, +- [x] **Task 4.5**: Round-trip tests for structural breadth — n-gon faces, UVs, colors, normals, empty mesh — where hand-computing bytes stops paying. -- [ ] **Task 4.6**: `write_ply_header` emits the `format` line matching the +- [~] **Task 4.6**: `write_ply_header` emits the `format` line matching the requested format and the host's byte order. - [ ] **Task 4.7**: Binary path in `write_ply_data` — precomputed property offsets, one `write` per vertex record, `float32` scalars, `uchar` diff --git a/tests/src/TestMeshIO.cpp b/tests/src/TestMeshIO.cpp index 2a2e40a..715623e 100644 --- a/tests/src/TestMeshIO.cpp +++ b/tests/src/TestMeshIO.cpp @@ -8,6 +8,7 @@ #include #include #include +#include #include #include #include @@ -1886,6 +1887,360 @@ TEST_F(PLYTest, FaceWith128CornersNoUVs_WritesCleanly) EXPECT_EQ(dst.face(0).size(), 128u); } +//------------------------------------------------------------------------------ +// Binary write +// +// The byte-level tests below assert the format itself, not libcore's opinion +// of it: expectations are spelled as hand-derived IEEE-754 / two's-complement +// literals in big-endian order and converted to host order by hand. A +// round-trip cannot anchor this work, because a byte-order mistake the reader +// and writer share round-trips perfectly. +//------------------------------------------------------------------------------ + +// Split a written PLY into its header text and its raw data bytes. +static void split_ply( + const fs::path& path, + std::string& header, + std::vector& body) +{ + std::ifstream f(path, std::ios::binary); + ASSERT_TRUE(f.good()) << "cannot open " << path; + // Extra parens on the first argument: without them this is a function + // declaration, not a vector (most vexing parse). + const std::vector all( + (std::istreambuf_iterator(f)), + std::istreambuf_iterator()); + + static const std::string kEnd = "end_header\n"; + const std::string text(all.begin(), all.end()); + const auto pos = text.find(kEnd); + ASSERT_NE(pos, std::string::npos) << "no end_header in " << path; + header.assign(text, 0, pos + kEnd.size()); + body.assign(all.begin() + static_cast(pos + kEnd.size()), all.end()); +} + +// Append a hand-derived big-endian byte spelling in the host's own order. +static void push_host( + std::vector& out, std::initializer_list be) +{ + std::vector b(be); + if (probe_host_is_little_endian()) { + std::reverse(b.begin(), b.end()); + } + out.insert(out.end(), b.begin(), b.end()); +} + +// The format line a binary write must emit on this host. +static auto expected_binary_format_line() -> std::string +{ + return probe_host_is_little_endian() + ? "format binary_little_endian 1.0\n" + : "format binary_big_endian 1.0\n"; +} + +TEST_F(PLYTest, BinaryWrite_ByteLevel) +{ + // make_triangle(): v0=(0,0,0) v1=(1,0,0) v2=(0,1,0), face 0 1 2. + const auto src = make_triangle(); + const auto path = ply("bin_bytes"); + write_ply(path, src, PLYFormat::Binary); + + std::string header; + std::vector body; + split_ply(path, header, body); + + // Property declarations are the same as ASCII; only the format line moves. + const std::string want_header = + "ply\n" + expected_binary_format_line() + + "element vertex 3\n" + "property float x\n" + "property float y\n" + "property float z\n" + "element face 1\n" + "property list uchar int vertex_indices\n" + "end_header\n"; + EXPECT_EQ(header, want_header); + + // 3 vertices x 3 float32, then a uchar count and 3 int32 indices. + std::vector want; + const std::initializer_list zero{0x00, 0x00, 0x00, 0x00}; + const std::initializer_list one{0x3F, 0x80, 0x00, 0x00}; + push_host(want, zero); push_host(want, zero); push_host(want, zero); + push_host(want, one); push_host(want, zero); push_host(want, zero); + push_host(want, zero); push_host(want, one); push_host(want, zero); + want.push_back(0x03); // uchar list count needs no swap + push_host(want, {0x00, 0x00, 0x00, 0x00}); + push_host(want, {0x00, 0x00, 0x00, 0x01}); + push_host(want, {0x00, 0x00, 0x00, 0x02}); + + ASSERT_EQ(body.size(), want.size()); + EXPECT_EQ(body, want); +} + +TEST_F(PLYTest, BinaryWrite_ScalarsAreFloat32RegardlessOfT) +{ + // The on-disk format must not follow the mesh's template parameter: a + // Mesh3d and a Mesh3f have to produce the same bytes from the same call. + Mesh3d src; + (void)src.insert_vertex(0., 0., 0.); + (void)src.insert_vertex(1., 0., 0.); + (void)src.insert_vertex(0., 1., 0.); + (void)src.insert_face(0u, 1u, 2u); + + const auto path = ply("bin_double"); + write_ply(path, src, PLYFormat::Binary); + + std::string header; + std::vector body; + split_ply(path, header, body); + + EXPECT_NE(header.find("property float x\n"), std::string::npos) << header; + EXPECT_EQ(header.find("float64"), std::string::npos) << header; + EXPECT_EQ(header.find("property double"), std::string::npos) << header; + + // 3 x 3 x 4 bytes of positions + 1 count byte + 3 x 4 index bytes. + EXPECT_EQ(body.size(), 3u * 3u * 4u + 1u + 3u * 4u); + + // Byte-identical to the Mesh3f write of the same geometry. + const auto f_path = ply("bin_float"); + write_ply(f_path, make_triangle(), PLYFormat::Binary); + std::string f_header; + std::vector f_body; + split_ply(f_path, f_header, f_body); + EXPECT_EQ(header, f_header); + EXPECT_EQ(body, f_body); +} + +TEST_F(PLYTest, BinaryWrite_NormalsAndColorsAreDeclaredWidths) +{ + // Normals are float32 and colors uchar, as the ASCII header declares. + // A NCMesh vertex record is therefore 12 + 12 + 3 = 27 bytes. + NCMesh src; + for (int i = 0; i < 3; ++i) { + const auto vi = src.insert_vertex( + static_cast(i), 0.f, 0.f); + src.vertex(vi).normal = Vec{0.f, 0.f, 1.f}; + src.vertex(vi).color = Color::U8C3{0x10, 0x20, 0x30}; + } + (void)src.insert_face(0u, 1u, 2u); + + const auto path = ply("bin_nc"); + write_ply(path, src, PLYFormat::Binary); + + std::string header; + std::vector body; + split_ply(path, header, body); + + EXPECT_NE(header.find("property float nx\n"), std::string::npos) << header; + EXPECT_NE(header.find("property uchar red\n"), std::string::npos) << header; + EXPECT_EQ(body.size(), 3u * 27u + 1u + 3u * 4u); + + // The color bytes sit at the tail of each vertex record, unswapped. + EXPECT_EQ(body[24], 0x10u); + EXPECT_EQ(body[25], 0x20u); + EXPECT_EQ(body[26], 0x30u); +} + +TEST_F(PLYTest, BinaryWrite_TexcoordList) +{ + // texcoord is a uchar-counted list of 2*N float32 values, with (-1,-1) + // for an unmapped corner. + const auto src = make_triangle(); + UVMap2f uv; + (void)uv.insert(0.f, 0.f); + (void)uv.insert(1.f, 0.f); + uv.map(0, 0, 0); + uv.map(0, 1, 1); + // corner 2 deliberately left unmapped + + const auto path = ply("bin_uv"); + write_ply(path, src, uv, PLYFormat::Binary); + + std::string header; + std::vector body; + split_ply(path, header, body); + EXPECT_NE( + header.find("property list uchar float texcoord\n"), + std::string::npos) + << header; + + std::vector want; + const std::initializer_list zero{0x00, 0x00, 0x00, 0x00}; + const std::initializer_list one{0x3F, 0x80, 0x00, 0x00}; + const std::initializer_list neg_one{0xBF, 0x80, 0x00, 0x00}; + push_host(want, zero); push_host(want, zero); push_host(want, zero); + push_host(want, one); push_host(want, zero); push_host(want, zero); + push_host(want, zero); push_host(want, one); push_host(want, zero); + want.push_back(0x03); + push_host(want, {0x00, 0x00, 0x00, 0x00}); + push_host(want, {0x00, 0x00, 0x00, 0x01}); + push_host(want, {0x00, 0x00, 0x00, 0x02}); + want.push_back(0x06); // 2*N, still a uchar count + push_host(want, zero); push_host(want, zero); // corner 0 (0,0) + push_host(want, one); push_host(want, zero); // corner 1 (1,0) + push_host(want, neg_one); push_host(want, neg_one); // corner 2 unmapped + + ASSERT_EQ(body.size(), want.size()); + EXPECT_EQ(body, want); +} + +TEST_F(PLYTest, DefaultFormatIsASCII) +{ + // Asserted explicitly rather than left to the header-grepping tests, so + // an accidental flip of the default is caught here. + const auto src = make_triangle(); + const auto path = ply("default_fmt"); + write_ply(path, src); + + std::ifstream f(path, std::ios::binary); + std::string line1, line2; + ASSERT_TRUE(std::getline(f, line1)); + ASSERT_TRUE(std::getline(f, line2)); + EXPECT_EQ(line1, "ply"); + EXPECT_EQ(line2, "format ascii 1.0"); +} + +TEST_F(PLYTest, ExplicitASCIIMatchesDefault) +{ + // PLYFormat::ASCII must be the same code path the default takes. + const auto src = make_triangle(); + const auto a = ply("ascii_default"); + const auto b = ply("ascii_explicit"); + write_ply(a, src); + write_ply(b, src, PLYFormat::ASCII); + + const auto slurp = [](const fs::path& p) { + std::ifstream f(p, std::ios::binary); + return std::string( + std::istreambuf_iterator(f), + std::istreambuf_iterator()); + }; + EXPECT_EQ(slurp(a), slurp(b)); +} + +//------------------------------------------------------------------------------ +// Binary write round-trips — structural breadth, where hand-computing bytes +// stops paying. +//------------------------------------------------------------------------------ + +TEST_F(PLYTest, BinaryRoundTrip_NGonAndNormalsAndColors) +{ + NCMesh src; + for (std::size_t i = 0; i < 5; ++i) { + const auto vi = src.insert_vertex( + static_cast(i), static_cast(2 * i), 0.5f); + src.vertex(vi).normal = Vec{0.f, 0.f, 1.f}; + src.vertex(vi).color = + Color::U8C3{static_cast(10 * i), 0x40, 0x80}; + } + (void)src.insert_face(NCMesh::Face{0, 1, 2, 3, 4}); // pentagon + + const auto path = ply("bin_rt_ngon"); + write_ply(path, src, PLYFormat::Binary); + + NCMesh dst; + read_ply(path, dst); + ASSERT_EQ(dst.num_vertices(), 5u); + ASSERT_EQ(dst.num_faces(), 1u); + EXPECT_EQ(dst.face(0), (NCMesh::Face{0, 1, 2, 3, 4})); + EXPECT_NEAR(dst.vertex(4)[0], 4.f, 1e-6f); + EXPECT_NEAR(dst.vertex(4)[1], 8.f, 1e-6f); + EXPECT_NEAR(dst.vertex(4)[2], 0.5f, 1e-6f); + ASSERT_TRUE(dst.vertex(3).normal.has_value()); + EXPECT_NEAR((*dst.vertex(3).normal)[2], 1.f, 1e-6f); + ASSERT_TRUE(dst.vertex(3).color.has_value()); + EXPECT_EQ(dst.vertex(3).color.value()[0], 30u); + EXPECT_EQ(dst.vertex(3).color.value()[2], 0x80u); +} + +TEST_F(PLYTest, BinaryRoundTrip_UVsWithSeam) +{ + const auto src = make_triangle(); + const auto uv = make_triangle_uvmap(src); + + const auto path = ply("bin_rt_uv"); + write_ply(path, src, uv, PLYFormat::Binary); + + Mesh3f dst; + UVMap2f dst_uv; + read_ply(path, dst, dst_uv); + ASSERT_EQ(dst.num_faces(), 1u); + for (std::size_t ci = 0; ci < 3; ++ci) { + ASSERT_TRUE(dst_uv.has(0, ci)) << "corner " << ci; + const auto& want = uv.at(uv.get(0, ci)); + const auto& got = dst_uv.at(dst_uv.get(0, ci)); + EXPECT_NEAR(got[0], want[0], 1e-6f) << "corner " << ci; + EXPECT_NEAR(got[1], want[1], 1e-6f) << "corner " << ci; + } +} + +TEST_F(PLYTest, BinaryRoundTrip_TexturePath) +{ + const auto src = make_triangle(); + const auto uv = make_triangle_uvmap(src); + const fs::path tex{"texture.png"}; + + const auto path = ply("bin_rt_tex"); + write_ply(path, src, uv, tex, PLYFormat::Binary); + + Mesh3f dst; + UVMap2f dst_uv; + std::vector paths; + read_ply(path, dst, dst_uv, paths); + ASSERT_EQ(paths.size(), 1u); + EXPECT_EQ(paths[0], tex); + EXPECT_EQ(dst.num_faces(), 1u); +} + +TEST_F(PLYTest, BinaryRoundTrip_EmptyMesh) +{ + const Mesh3f src; + const auto path = ply("bin_rt_empty"); + write_ply(path, src, PLYFormat::Binary); + + Mesh3f dst; + read_ply(path, dst); + EXPECT_EQ(dst.num_vertices(), 0u); + EXPECT_EQ(dst.num_faces(), 0u); +} + +TEST_F(PLYTest, BinaryRoundTrip_MaximalFaces) +{ + // The Phase 3 coherence check on the binary path: the largest faces the + // writer permits must still be readable. + { + const auto path = ply("bin_rt_255"); + write_ply(path, make_ngon(255), PLYFormat::Binary); + Mesh3f dst; + read_ply(path, dst); + ASSERT_EQ(dst.num_faces(), 1u); + EXPECT_EQ(dst.face(0).size(), 255u); + EXPECT_EQ(dst.face(0).back(), 254u); + } + { + const auto path = ply("bin_rt_127_uv"); + write_ply(path, make_ngon(127), make_ngon_uvmap(127), + PLYFormat::Binary); + Mesh3f dst; + UVMap2f dst_uv; + read_ply(path, dst, dst_uv); + ASSERT_EQ(dst.num_faces(), 1u); + EXPECT_EQ(dst.face(0).size(), 127u); + ASSERT_TRUE(dst_uv.has(0, 126)); + EXPECT_NEAR(dst_uv.at(dst_uv.get(0, 126))[0], 126.f / 127.f, 1e-5f); + } +} + +TEST_F(PLYTest, BinaryWrite_RejectsOversizeFaces) +{ + // The list-count guard is format-independent. + const auto path = ply("bin_ngon256"); + EXPECT_THROW( + write_ply(path, make_ngon(256), PLYFormat::Binary), + std::runtime_error); + EXPECT_FALSE(fs::exists(path)); +} + TEST_F(PLYTest, WriteWithUVMap_PerWedgeTexcoord) { // Two triangles sharing an edge with a UV seam. From 8adad09223528b5f092a6c2609ccbd8521cc7369 Mon Sep 17 00:00:00 2001 From: Seth Parker Date: Fri, 4 Sep 2026 18:17:07 -0400 Subject: [PATCH 15/23] feat(MeshIO): binary PLY write (ply-binary-io_20260904) 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 --- .../tracks/ply-binary-io_20260904/plan.md | 20 ++- include/educelab/core/io/MeshIO_PLY.hpp | 152 +++++++++++++++++- 2 files changed, 158 insertions(+), 14 deletions(-) diff --git a/conductor/tracks/ply-binary-io_20260904/plan.md b/conductor/tracks/ply-binary-io_20260904/plan.md index 7f650c8..9cd4901 100644 --- a/conductor/tracks/ply-binary-io_20260904/plan.md +++ b/conductor/tracks/ply-binary-io_20260904/plan.md @@ -156,22 +156,28 @@ path is untouched until Task 4.6. 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. -- [~] **Task 4.6**: `write_ply_header` emits the `format` line matching the +- [x] **Task 4.6**: `write_ply_header` emits the `format` line matching the requested format and the host's byte order. -- [ ] **Task 4.7**: Binary path in `write_ply_data` — precomputed property +- [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. -- [ ] **Task 4.8**: Open all three tiers with `std::ios::binary` +- [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 -- [ ] Byte-level, `float32`-width, default-ASCII and round-trip tests all pass -- [ ] Existing ASCII tests pass unmodified -- [ ] A binary file written by libcore opens correctly in MeshLab -- [ ] Full suite green, Debug and Release +- [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) +- [~] A binary file written by libcore opens correctly in MeshLab — **awaiting + manual check**. MeshLab 2025.07 ships no CLI (`meshlabserver` was removed + upstream), so this cannot be automated here. Validated instead against an + independently written PLY parser (structure, all scalar values, per-wedge + texcoords, no trailing bytes) for tiers 1–3. +- [x] Full suite green, Debug and Release --- diff --git a/include/educelab/core/io/MeshIO_PLY.hpp b/include/educelab/core/io/MeshIO_PLY.hpp index fd9dfb0..b720314 100644 --- a/include/educelab/core/io/MeshIO_PLY.hpp +++ b/include/educelab/core/io/MeshIO_PLY.hpp @@ -1185,12 +1185,19 @@ void write_ply_header( bool has_uvs, [[maybe_unused]] bool has_normals, [[maybe_unused]] bool has_colors, - [[maybe_unused]] PLYFormat format) + PLYFormat format) { using Vertex = typename Mesh::Vertex; - file << "ply\n" - << "format ascii 1.0\n"; + file << "ply\n"; + if (format == PLYFormat::Binary) { + // Native byte order, labeled honestly. read_ply handles either + // direction, so the file stays portable. + file << (host_is_little_endian() ? "format binary_little_endian 1.0\n" + : "format binary_big_endian 1.0\n"); + } else { + file << "format ascii 1.0\n"; + } if (!texture_comment.empty()) { file << "comment TextureFile " << texture_comment << '\n'; @@ -1227,6 +1234,129 @@ void write_ply_header( file << "end_header\n"; } +/** + * @brief Write PLY binary vertex and face data to @p file + * + * Mirrors the reader's record batching: the vertex layout is fixed by the + * mesh's traits, so property offsets are precomputed once and each vertex + * costs a single @c write rather than one per property. Face records vary in + * length and get one @c write each. + * + * Widths match what @ref write_ply_header declares and never follow @c T: + * @c float32 positions and normals, @c uchar colors, @c int32 vertex indices, + * @c float32 texcoords, and @c uchar list counts. Byte order is the host's, + * which the header labels. + * + * @p uvmap may be @c nullptr (no texcoord list). Unmapped corners are written + * as @c (-1,-1), the same sentinel the ASCII path uses. @p has_normals and + * @p has_colors must match the values passed to @ref write_ply_header. + * + * @warning Assumes @ref validate_ply_face_lists has already run: the @c uchar + * list counts below are narrowing casts that only hold because every + * face is known to be within 255 corners, or 127 when UVs are + * written. + */ +template +void write_ply_data_binary( + std::ostream& file, + const Mesh& mesh, + const UVMapT* uvmap, + [[maybe_unused]] bool has_normals, + [[maybe_unused]] bool has_colors) +{ + using Vertex = typename Mesh::Vertex; + + constexpr std::size_t kF32 = sizeof(float); + static_assert(kF32 == 4, "PLY float32 output requires a 4-byte float"); + + // Vertex record layout, resolved once outside the loop. + const std::size_t normal_off = 3 * kF32; + const std::size_t color_off = normal_off + (has_normals ? 3 * kF32 : 0); + const std::size_t vert_rec_size = color_off + (has_colors ? 3 : 0); + + // Widest possible record: 3 positions + 3 normals as float32, 3 uchar + // colors. Fixed size, so no allocation in the vertex loop. + std::array vbuf{}; + + const auto put_f32 = [&vbuf](std::size_t off, auto value) { + const auto f = static_cast(value); + std::memcpy(vbuf.data() + off, &f, kF32); + }; + + for (std::size_t vi = 0; vi < mesh.num_vertices(); ++vi) { + const auto& v = mesh.vertex(vi); + put_f32(0, v[0]); + put_f32(kF32, v[1]); + put_f32(2 * kF32, v[2]); + + if constexpr (traits::has_normal::value) { + if (has_normals) { + // PLY's fixed-property element forces a value for every + // vertex; gaps in a partially-normalled mesh fall back to + // zero, as in the ASCII path. + const auto n = v.normal.value_or(Vec{}); + put_f32(normal_off, n[0]); + put_f32(normal_off + kF32, n[1]); + put_f32(normal_off + 2 * kF32, n[2]); + } + } + if constexpr (traits::has_color::value) { + if (has_colors) { + // Gaps fall back to black, as in the ASCII path. + const auto [r, g, b] = detail::color_to_u8c3(v.color); + vbuf[color_off] = static_cast(r); + vbuf[color_off + 1] = static_cast(g); + vbuf[color_off + 2] = static_cast(b); + } + } + + file.write( + vbuf.data(), static_cast(vert_rec_size)); + } + + // Face records vary with corner count, so the buffer grows to the largest + // face seen and is then reused. + std::vector fbuf; + for (std::size_t fi = 0; fi < mesh.num_faces(); ++fi) { + const auto& face = mesh.face(fi); + const auto n = face.size(); + + std::size_t rec_size = 1 + n * sizeof(int32_t); + if (uvmap != nullptr) { + rec_size += 1 + 2 * n * kF32; + } + if (fbuf.size() < rec_size) { + fbuf.resize(rec_size); + } + + std::size_t off = 0; + fbuf[off++] = static_cast(static_cast(n)); + for (const auto vi : face) { + const auto idx = static_cast(vi); + std::memcpy(fbuf.data() + off, &idx, sizeof(int32_t)); + off += sizeof(int32_t); + } + + if (uvmap != nullptr) { + fbuf[off++] = static_cast(static_cast(2 * n)); + for (std::size_t ci = 0; ci < n; ++ci) { + float u{-1.f}, w{-1.f}; + if (uvmap->has(fi, ci)) { + const auto& uv = uvmap->at(uvmap->get(fi, ci)); + u = static_cast(uv[0]); + w = static_cast(uv[1]); + } + std::memcpy(fbuf.data() + off, &u, kF32); + off += kF32; + std::memcpy(fbuf.data() + off, &w, kF32); + off += kF32; + } + } + + file.write(fbuf.data(), static_cast(rec_size)); + } +} + /** * @brief Write PLY ASCII vertex and face data to @p file * @@ -1235,6 +1365,9 @@ void write_ply_header( * written as @c -1,-1 (sentinel for "no UV assignment"). @p has_normals and * @p has_colors must match the values passed to @ref write_ply_header so the * data matches the declared properties. + * + * @p format selects the encoding; @c PLYFormat::Binary delegates to + * @ref write_ply_data_binary and @p buf is then unused. */ template void write_ply_data( @@ -1244,10 +1377,15 @@ void write_ply_data( const UVMapT* uvmap, [[maybe_unused]] bool has_normals, [[maybe_unused]] bool has_colors, - [[maybe_unused]] PLYFormat format) + PLYFormat format) { using Vertex = typename Mesh::Vertex; + if (format == PLYFormat::Binary) { + write_ply_data_binary(file, mesh, uvmap, has_normals, has_colors); + return; + } + for (std::size_t vi = 0; vi < mesh.num_vertices(); ++vi) { const auto& v = mesh.vertex(vi); file << to_string_view(buf, v[0]) << ' ' @@ -1329,7 +1467,7 @@ void write_ply( detail::validate_ply_face_lists(mesh, false); - std::ofstream file(path); + std::ofstream file(path, std::ios::binary); if (!file) { throw std::runtime_error( "write_ply: cannot open file: " + path.string()); @@ -1381,7 +1519,7 @@ void write_ply( detail::validate_ply_face_lists(mesh, true); - std::ofstream file(path); + std::ofstream file(path, std::ios::binary); if (!file) { throw std::runtime_error( "write_ply: cannot open file: " + path.string()); @@ -1438,7 +1576,7 @@ void write_ply( detail::validate_ply_face_lists(mesh, true); - std::ofstream file(path); + std::ofstream file(path, std::ios::binary); if (!file) { throw std::runtime_error( "write_ply: cannot open file: " + path.string()); From 205998e2cec74b7a07c4dfd12fe7b54e8a125104 Mon Sep 17 00:00:00 2001 From: Seth Parker Date: Fri, 4 Sep 2026 18:21:28 -0400 Subject: [PATCH 16/23] docs(MeshIO): document PLYFormat, list-count throws, and the write_mesh 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 --- .../tracks/ply-binary-io_20260904/plan.md | 14 +++-- .../tracks/ply-binary-io_20260904/spec.md | 59 ++++++++++++++----- include/educelab/core/io/MeshIO.hpp | 31 +++++++++- include/educelab/core/io/MeshIO_PLY.hpp | 49 ++++++++++++--- 4 files changed, 121 insertions(+), 32 deletions(-) diff --git a/conductor/tracks/ply-binary-io_20260904/plan.md b/conductor/tracks/ply-binary-io_20260904/plan.md index 9cd4901..869a15a 100644 --- a/conductor/tracks/ply-binary-io_20260904/plan.md +++ b/conductor/tracks/ply-binary-io_20260904/plan.md @@ -185,20 +185,22 @@ path is untouched until Task 4.6. ### Tasks -- [ ] **Task 5.1**: `@throws` for the list-count limits on the tier-2 and +- [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. -- [ ] **Task 5.2**: Document `PLYFormat` on the `write_ply` overloads, and note +- [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. -- [ ] **Task 5.3**: Doxygen builds cleanly with no new warnings. +- [x] **Task 5.3**: Doxygen builds cleanly with no new warnings. ### Verification -- [ ] Doxygen clean -- [ ] Every acceptance criterion in `spec.md` maps to a passing test or a - merged doc change +- [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` --- diff --git a/conductor/tracks/ply-binary-io_20260904/spec.md b/conductor/tracks/ply-binary-io_20260904/spec.md index 1401d38..e7ffea9 100644 --- a/conductor/tracks/ply-binary-io_20260904/spec.md +++ b/conductor/tracks/ply-binary-io_20260904/spec.md @@ -3,7 +3,7 @@ **Track ID:** ply-binary-io_20260904 **Type:** Feature **Created:** 2026-09-04 -**Status:** Draft +**Status:** Implemented **GitHub issue:** [educelab/libcore#25](https://github.com/educelab/libcore/issues/25) ## Summary @@ -40,42 +40,69 @@ header declares. ### Write -- [ ] A public `educelab::PLYFormat { ASCII, Binary }` exists and is threaded +- [x] A public `educelab::PLYFormat { ASCII, Binary }` exists and is threaded through all three `write_ply` tiers, defaulting to `ASCII`. -- [ ] `PLYFormat` is kept distinct from `detail::PLYHeader::Format` (three + *(Task 4.1; `PLYTest.DefaultFormatIsASCII`, + `PLYTest.ExplicitASCIIMatchesDefault`)* +- [x] `PLYFormat` is kept distinct from `detail::PLYHeader::Format` (three values, "what I parsed") so no `detail` type leaks into a public signature. -- [ ] `Binary` writes native byte order and labels the header to match + *(Task 4.1; separate enum declared outside `namespace detail`)* +- [x] `Binary` writes native byte order and labels the header to match (`format binary_little_endian 1.0` on an LE host). -- [ ] Scalars are always written as `float32` regardless of the mesh's `T`, + *(Task 4.6; `PLYTest.BinaryWrite_ByteLevel` asserts the exact line)* +- [x] Scalars are always written as `float32` regardless of the mesh's `T`, matching what the ASCII header already declares and what OpenMVS emits. The on-disk format does not depend on a template parameter. -- [ ] The binary writer mirrors the reader's record batching — precomputed + *(Task 4.7; `PLYTest.BinaryWrite_ScalarsAreFloat32RegardlessOfT` asserts + a `Mesh3d` write is byte-identical to the `Mesh3f` one)* +- [x] The binary writer mirrors the reader's record batching — precomputed property offsets, one `write` per vertex — not one `write` per property. -- [ ] All three tiers open with `std::ios::binary` unconditionally. -- [ ] Existing ASCII output is byte-for-byte unchanged on non-Windows hosts; + *(Task 4.7; `detail::write_ply_data_binary`, one `write` per vertex and + per face record)* +- [x] All three tiers open with `std::ios::binary` unconditionally. + *(Task 4.8)* +- [x] Existing ASCII output is byte-for-byte unchanged on non-Windows hosts; all existing header-grepping tests pass untouched. + *(Verified by diffing all three tiers' output against `e9635ab`'s writer + for a mesh with normals, partial colors, partial UVs and mixed face + arity — identical. No existing test was modified.)* ### Read -- [ ] `read_ply` honors the endianness declared in the header, byte-swapping +- [x] `read_ply` honors the endianness declared in the header, byte-swapping when file order differs from host order. -- [ ] The `binary_big_endian` rejection at `MeshIO_PLY.hpp:638` is removed and + *(Task 2.4; `PLYTest.BinaryBigEndian_Read`)* +- [x] The `binary_big_endian` rejection at `MeshIO_PLY.hpp:638` is removed and `PLYTest.BinaryBigEndian_Throws` is replaced by a positive read test. -- [ ] Swapping enters at the two choke points every binary scalar read passes + *(Tasks 2.4 and 2.5. The old test was passing for the wrong reason — its + fixture declares a face but writes no face bytes, so it threw on + truncation once the rejection was gone.)* +- [x] Swapping enters at the two choke points every binary scalar read passes through: `read_ply_binary_prop` (stream) and `read_ply_prop_from_buf` (buffer). -- [ ] The swap flag is a runtime `bool`, not a template parameter. -- [ ] The swap applies to the raw fixed-width value **before** the cast to the + *(Task 2.3; each has one raw-read lambda so the swap cannot be dropped in + a single case of the width dispatch)* +- [x] The swap flag is a runtime `bool`, not a template parameter. + *(Task 2.3; `needs_swap`, deliberately with no default so the compiler + names every call site that omits it — it found all fifteen)* +- [x] The swap applies to the raw fixed-width value **before** the cast to the destination type. + *(Task 2.3; `PLYTest.BinaryBigEndian_SwapPrecedesCast` for the buffer + path and `..._SwapPrecedesCast_StreamPath` for the stream path)* ### Validation -- [ ] `write_ply` throws when a face exceeds the `uchar` list-count limits: 255 +- [x] `write_ply` throws when a face exceeds the `uchar` list-count limits: 255 corners for `vertex_indices`, 127 for `texcoord` (which writes `2*N`). -- [ ] The message names which limit fired and the offending face index. -- [ ] `@throws` is documented on the tier-2 and tier-3 `write_ply` overloads and + *(Task 3.2; `PLYTest.FaceOver255Corners_Throws`, + `FaceOver127CornersWithUVs_Throws`, plus `..._Tier3_Throws` and the two + non-throwing boundary cases)* +- [x] The message names which limit fired and the offending face index. + *(Task 3.2; asserted on the message text)* +- [x] `@throws` is documented on the tier-2 and tier-3 `write_ply` overloads and the corresponding `write_mesh` dispatchers; tier 1 carries only the 255 limit. + *(Task 5.1)* ## Design Decisions diff --git a/include/educelab/core/io/MeshIO.hpp b/include/educelab/core/io/MeshIO.hpp index 36de380..e912eba 100644 --- a/include/educelab/core/io/MeshIO.hpp +++ b/include/educelab/core/io/MeshIO.hpp @@ -40,8 +40,19 @@ void read_mesh( /** * @brief Write a mesh to a file, dispatching by extension * - * Supported extensions: `.obj`, `.ply` (case-insensitive). Throws - * `std::runtime_error` for any other extension. + * Supported extensions: `.obj`, `.ply` (case-insensitive). + * + * @note `write_mesh` takes no PLY format parameter. A PLY-only value would be + * meaningless for half of its inputs, and there is no good answer for + * `write_mesh("out.obj", mesh, PLYFormat::Binary)`. Call @ref write_ply + * directly to write binary PLY. Giving `write_mesh` a general + * format-options mechanism is deferred to + * [educelab/libcore#26](https://github.com/educelab/libcore/issues/26); + * the omission is a decision, not an oversight. + * + * @throws std::runtime_error for an unsupported extension, if the file cannot + * be opened, or, for `.ply`, if any face has more than 255 corners, + * which a `uchar` `vertex_indices` list count cannot express */ template void write_mesh( @@ -78,6 +89,14 @@ void read_mesh( /** * @brief Write a mesh and UV map to a file, dispatching by extension + * + * @note No PLY format parameter — see the two-argument @ref write_mesh + * overload. + * + * @throws std::runtime_error for an unsupported extension, if the file cannot + * be opened, or, for `.ply`, if a face exceeds either `uchar` + * list-count limit: 255 corners for `vertex_indices`, or 127 corners + * here, since `texcoord` writes `2*N` values per face */ template void write_mesh( @@ -123,6 +142,14 @@ void read_mesh( * * OBJ: emits a `.mtl` with one `map_Kd` entry.\n * PLY: emits a `comment TextureFile` line in the header. + * + * @note No PLY format parameter — see the two-argument @ref write_mesh + * overload. + * + * @throws std::runtime_error for an unsupported extension, if the file cannot + * be opened, or, for `.ply`, if a face exceeds either `uchar` + * list-count limit: 255 corners for `vertex_indices`, or 127 corners + * here, since `texcoord` writes `2*N` values per face */ template void write_mesh( diff --git a/include/educelab/core/io/MeshIO_PLY.hpp b/include/educelab/core/io/MeshIO_PLY.hpp index b720314..ec45cb7 100644 --- a/include/educelab/core/io/MeshIO_PLY.hpp +++ b/include/educelab/core/io/MeshIO_PLY.hpp @@ -485,7 +485,7 @@ auto read_ply_binary_prop(std::istream& f, PLYType type, bool needs_swap) * pre-computed byte offset within the buffer rather than via individual * istream::read calls. * - * @p needs_swap follows the same contract as in @ref read_ply_binary_prop: + * @p needs_swap follows the same contract as in @ref read_ply_binary_prop — * true when the file's byte order differs from the host's, applied to the raw * fixed-width value before the cast to @c DestT, and with no default. */ @@ -1123,6 +1123,7 @@ void read_ply_impl( * over the faces reads only @c size() and is negligible beside the write it * guards. * + * @param mesh Mesh whose faces are checked * @param has_uvs True when a @c texcoord list will be written (tiers 2 and 3) * @throws std::runtime_error naming the limit that fired and the face index */ @@ -1446,7 +1447,7 @@ void write_ply_data( // ============================================================================= /** - * @brief Write a mesh to an ASCII PLY file + * @brief Write a mesh to a PLY file * * Emits @c x @c y @c z vertex properties. If @c Vertex carries * @ref traits::WithNormal @em and at least one vertex has a normal set, also @@ -1455,7 +1456,18 @@ void write_ply_data( * color set, also emits @c red @c green @c blue properties (@c uchar, 0–255; * a color-less mesh declares none). * - * @throws std::runtime_error if the file cannot be opened + * Scalars are written as @c float32 whatever @c T is, so the on-disk layout + * never depends on the mesh's template parameters. + * + * @param path Output file path + * @param mesh Mesh to write + * @param format Data-section encoding; defaults to @ref PLYFormat::ASCII. + * @ref PLYFormat::Binary is smaller and faster to write and is + * labeled with the host's byte order — see @ref PLYFormat. + * + * @throws std::runtime_error if the file cannot be opened, if writing fails, + * or if any face has more than 255 corners, which a @c uchar + * @c vertex_indices list count cannot express */ template void write_ply( @@ -1498,7 +1510,7 @@ void write_ply( // ============================================================================= /** - * @brief Write a mesh and UV map to an ASCII PLY file + * @brief Write a mesh and UV map to a PLY file * * Writes per-wedge UV coordinates as a @c texcoord list property on the face * element. No vertex duplication is performed. Adds @@ -1506,7 +1518,17 @@ void write_ply( * Corners with no UV assignment are written as @c -1,-1. * No @c comment @c TextureFile line is written. * - * @throws std::runtime_error if the file cannot be opened + * @param path Output file path + * @param mesh Mesh to write + * @param uvmap Per-wedge UV coordinates + * @param format Data-section encoding; defaults to @ref PLYFormat::ASCII. + * See @ref PLYFormat. + * + * @throws std::runtime_error if the file cannot be opened, if writing fails, + * or if a face exceeds either @c uchar list-count limit: 255 corners + * for @c vertex_indices, or 127 corners here, since @c texcoord + * writes @c 2*N values per face. The message names which limit fired + * and the offending face index. */ template void write_ply( @@ -1549,10 +1571,10 @@ void write_ply( // ============================================================================= /** - * @brief Write a mesh, UV map, and texture path to an ASCII PLY file + * @brief Write a mesh, UV map, and texture path to a PLY file * * Identical to the Tier 2 overload but also emits a - * @c "comment TextureFile " line immediately after @c "format ascii 1.0" + * @c "comment TextureFile " line immediately after the @c format line * (MeshLab convention). * * @note PLY write supports only a **single** texture/chart: there is no @@ -1562,7 +1584,18 @@ void write_ply( * (@c read_ply will still recover multiple @c "comment TextureFile" lines * into its @c texture_paths out-parameter when reading such files.) * - * @throws std::runtime_error if the file cannot be opened + * @param path Output file path + * @param mesh Mesh to write + * @param uvmap Per-wedge UV coordinates + * @param texture_path Path emitted in the @c comment @c TextureFile line + * @param format Data-section encoding; defaults to + * @ref PLYFormat::ASCII. See @ref PLYFormat. + * + * @throws std::runtime_error if the file cannot be opened, if writing fails, + * or if a face exceeds either @c uchar list-count limit: 255 corners + * for @c vertex_indices, or 127 corners here, since @c texcoord + * writes @c 2*N values per face. The message names which limit fired + * and the offending face index. */ template void write_ply( From 42ab5f4da08d3eece3bdf29a4fbad7d17ce9b264 Mon Sep 17 00:00:00 2001 From: Seth Parker Date: Fri, 4 Sep 2026 18:22:21 -0400 Subject: [PATCH 17/23] chore(conductor): complete PLY Binary IO track (ply-binary-io_20260904) 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 --- conductor/tracks.md | 2 +- .../ply-binary-io_20260904/metadata.json | 21 ++++---- .../tracks/ply-binary-io_20260904/plan.md | 48 +++++++++++++++---- 3 files changed, 53 insertions(+), 18 deletions(-) diff --git a/conductor/tracks.md b/conductor/tracks.md index 9b5ffc2..de64cdb 100644 --- a/conductor/tracks.md +++ b/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 | diff --git a/conductor/tracks/ply-binary-io_20260904/metadata.json b/conductor/tracks/ply-binary-io_20260904/metadata.json index cd41673..7835838 100644 --- a/conductor/tracks/ply-binary-io_20260904/metadata.json +++ b/conductor/tracks/ply-binary-io_20260904/metadata.json @@ -2,19 +2,19 @@ "id": "ply-binary-io_20260904", "title": "PLY Binary IO", "type": "feature", - "status": "in_progress", + "status": "complete", "created": "2026-09-04T00:00:00+0200", - "updated": "2026-09-04T22:45:00Z", + "updated": "2026-09-04T23:20:00Z", "issue": 25, - "current_phase": 4, - "current_task": "4.1", + "current_phase": 5, + "current_task": "5.3", "phases": { "total": 5, - "completed": 3 + "completed": 5 }, "tasks": { "total": 22, - "completed": 11 + "completed": 22 }, "commits": [ "b28d606: docs(conductor): record byte-order helper placement in detail", @@ -24,6 +24,11 @@ "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" - ] + "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 manual MeshLab check is outstanding: MeshLab 2025.07 ships no CLI, so it was validated against an independently written PLY parser instead." } diff --git a/conductor/tracks/ply-binary-io_20260904/plan.md b/conductor/tracks/ply-binary-io_20260904/plan.md index 869a15a..eff7727 100644 --- a/conductor/tracks/ply-binary-io_20260904/plan.md +++ b/conductor/tracks/ply-binary-io_20260904/plan.md @@ -3,7 +3,7 @@ **Track ID:** ply-binary-io_20260904 **Spec:** [spec.md](./spec.md) **Created:** 2026-09-04 -**Status:** [~] In Progress +**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 @@ -31,8 +31,8 @@ All work is in `include/educelab/core/io/MeshIO_PLY.hpp` and | Phase 1 | `5861ee1` | 2026-09-04 | verified | | Phase 2 | `3483f88` | 2026-09-04 | verified | | Phase 3 | `7583422` | 2026-09-04 | verified | -| Phase 4 | | | pending | -| Phase 5 | | | pending | +| Phase 4 | `398c7d7` | 2026-09-04 | verified* | +| Phase 5 | `bfc4aa2` | 2026-09-04 | verified | --- @@ -206,13 +206,43 @@ path is untouched until Task 4.6. ## Final Verification -- [ ] All acceptance criteria in [spec.md](./spec.md) met -- [ ] Full test suite passes in Debug and Release -- [ ] Doxygen updated for `PLYFormat`, `@throws`, and BE read support -- [ ] PR notes the Windows CRLF change for ASCII callers -- [ ] Ready for review; #19 unblocked to rebase onto the new - `write_ply_header` / `write_ply_data` signatures +- [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`) +- [~] MeshLab open — manual, see the Phase 4 note + +## 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._ From 614ec49942accc0bc3bca5a237f6566dd9cfff29 Mon Sep 17 00:00:00 2001 From: Seth Parker Date: Fri, 4 Sep 2026 18:28:27 -0400 Subject: [PATCH 18/23] docs(conductor): record MeshLab texcoord/n-gon limitation (ply-binary-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 --- .../tracks/ply-binary-io_20260904/plan.md | 27 +++++++++++++++---- .../tracks/ply-binary-io_20260904/spec.md | 22 +++++++++++++++ 2 files changed, 44 insertions(+), 5 deletions(-) diff --git a/conductor/tracks/ply-binary-io_20260904/plan.md b/conductor/tracks/ply-binary-io_20260904/plan.md index eff7727..80b6210 100644 --- a/conductor/tracks/ply-binary-io_20260904/plan.md +++ b/conductor/tracks/ply-binary-io_20260904/plan.md @@ -172,11 +172,28 @@ path is untouched until Task 4.6. - [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) -- [~] A binary file written by libcore opens correctly in MeshLab — **awaiting - manual check**. MeshLab 2025.07 ships no CLI (`meshlabserver` was removed - upstream), so this cannot be automated here. Validated instead against an - independently written PLY parser (structure, all scalar values, per-wedge - texcoords, no trailing bytes) for tiers 1–3. +- [x] A binary file written by libcore opens correctly in MeshLab — **checked + manually**. A binary tier-1 mesh (positions, normals, colors, mixed + triangle/quad arity) loads correctly. +- [!] **Pre-existing limitation found, not a regression.** Any PLY libcore + writes with a `texcoord` list *and* a face of more than 3 corners is + rejected by MeshLab with "Face with more than 3 vertices" — in **both** + ASCII and binary. The ASCII tier-3 file that fails is byte-identical + (md5 `639a03ea…`) to what `e9635ab` wrote before this track, so the + behavior predates the work here and the binary path is no worse than the + ASCII one. + + Cause is in MeshLab's importer, not libcore's output: the error table in + `libio_base.so` carries vcglib's `import_ply.h` strings, including + "Face with no 6 texture coordinates" — per-wedge `texcoord` support is + hard-coded to 6 floats, i.e. 3 corners, so vcglib's polygonal path is + unavailable whenever texcoords are present. The files parse correctly + against an independently written PLY reader. + + Out of scope for this track (it changes neither the endianness fix nor + the binary writer). Belongs with + [ply-multichart_20260624](../ply-multichart_20260624/index.md) (#19), + which already rewrites the `texcoord` write path. - [x] Full suite green, Debug and Release --- diff --git a/conductor/tracks/ply-binary-io_20260904/spec.md b/conductor/tracks/ply-binary-io_20260904/spec.md index e7ffea9..adc66d2 100644 --- a/conductor/tracks/ply-binary-io_20260904/spec.md +++ b/conductor/tracks/ply-binary-io_20260904/spec.md @@ -203,6 +203,28 @@ additive of the two. Precedes [mesh-io-options_20260904](../mesh-io-options_20260904/index.md) (#26), which was filed out of this track's decision to leave `write_mesh` untouched. +## Known Limitation (found during implementation) + +MeshLab cannot open **any** PLY libcore writes that combines a `texcoord` list +with a face of more than 3 corners — ASCII or binary, and regardless of this +track. It reports "Face with more than 3 vertices". + +The limitation is in MeshLab's importer. Its bundled `libio_base.so` carries +vcglib's `import_ply.h` error table, including "Face with no 6 texture +coordinates": per-wedge `texcoord` is hard-coded to 6 floats — 3 corners — so +vcglib's polygonal face path is disabled once texcoords are present. Files that +MeshLab refuses still parse correctly against an independent PLY reader, and +`read_ply` round-trips them. + +Verified not to be a regression: the failing ASCII tier-3 file is byte-identical +(md5 `639a03ea…`) to the output of `e9635ab`, the commit this track branched +from. Triangle-only meshes — what the EduceLab pipelines actually write — are +unaffected. + +Deferred to [ply-multichart_20260624](../ply-multichart_20260624/index.md) +(#19), which already rewrites the `texcoord` write path and is the right place +to decide between triangulating on write, warning, or documenting. + ## Out of Scope - **A format parameter on `write_mesh`.** A PLY-only value is meaningless for From 5006146b3529486c41e1782c5d790e1c5074ffaa Mon Sep 17 00:00:00 2001 From: Seth Parker Date: Fri, 4 Sep 2026 18:30:30 -0400 Subject: [PATCH 19/23] docs(conductor): confirm MeshLab limitation is texcoord + n-gon, format-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 --- .../tracks/ply-binary-io_20260904/plan.md | 39 +++++++++++++------ .../tracks/ply-binary-io_20260904/spec.md | 5 +++ 2 files changed, 32 insertions(+), 12 deletions(-) diff --git a/conductor/tracks/ply-binary-io_20260904/plan.md b/conductor/tracks/ply-binary-io_20260904/plan.md index 80b6210..3e2592b 100644 --- a/conductor/tracks/ply-binary-io_20260904/plan.md +++ b/conductor/tracks/ply-binary-io_20260904/plan.md @@ -172,23 +172,38 @@ path is untouched until Task 4.6. - [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 — **checked - manually**. A binary tier-1 mesh (positions, normals, colors, mixed - triangle/quad arity) loads correctly. -- [!] **Pre-existing limitation found, not a regression.** Any PLY libcore - writes with a `texcoord` list *and* a face of more than 3 corners is - rejected by MeshLab with "Face with more than 3 vertices" — in **both** - ASCII and binary. The ASCII tier-3 file that fails is byte-identical - (md5 `639a03ea…`) to what `e9635ab` wrote before this track, so the - behavior predates the work here and the binary path is no worse than the - ASCII 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. +- [!] **Pre-existing limitation confirmed, not a regression.** Rows 08/09 pin + the trigger as `texcoord` *combined with* a face of more than 3 corners: + texcoord alone is fine (03–05), a quad alone is fine (06–07), and the + pair fails in ASCII and binary alike. The failing ASCII file is + byte-identical (md5 `639a03ea…`) to what `e9635ab` wrote before this + track. Cause is in MeshLab's importer, not libcore's output: the error table in `libio_base.so` carries vcglib's `import_ply.h` strings, including "Face with no 6 texture coordinates" — per-wedge `texcoord` support is hard-coded to 6 floats, i.e. 3 corners, so vcglib's polygonal path is unavailable whenever texcoords are present. The files parse correctly - against an independently written PLY reader. + against an independently written PLY reader and round-trip through + `read_ply`. Out of scope for this track (it changes neither the endianness fix nor the binary writer). Belongs with @@ -234,7 +249,7 @@ path is untouched until Task 4.6. `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`) -- [~] MeshLab open — manual, see the Phase 4 note +- [x] MeshLab open — verified manually; see the Phase 4 note for the matrix ## For the PR description diff --git a/conductor/tracks/ply-binary-io_20260904/spec.md b/conductor/tracks/ply-binary-io_20260904/spec.md index adc66d2..3028245 100644 --- a/conductor/tracks/ply-binary-io_20260904/spec.md +++ b/conductor/tracks/ply-binary-io_20260904/spec.md @@ -209,6 +209,11 @@ MeshLab cannot open **any** PLY libcore writes that combines a `texcoord` list with a face of more than 3 corners — ASCII or binary, and regardless of this track. It reports "Face with more than 3 vertices". +Isolated in MeshLab 2025.07 with a matrix varying only arity and `texcoord`: +triangles load with and without texcoord, a quad loads without texcoord, and +only the pair fails — in ASCII and binary identically. So the binary writer +adds no incompatibility of its own. + The limitation is in MeshLab's importer. Its bundled `libio_base.so` carries vcglib's `import_ply.h` error table, including "Face with no 6 texture coordinates": per-wedge `texcoord` is hard-coded to 6 floats — 3 corners — so From cb27f4ac013583fe62caeda5bf475994b7bdc3bd Mon Sep 17 00:00:00 2001 From: Seth Parker Date: Fri, 4 Sep 2026 18:32:30 -0400 Subject: [PATCH 20/23] docs(conductor): reframe the MeshLab texcoord finding as interop, not 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 --- .../tracks/ply-binary-io_20260904/plan.md | 25 +++--------- .../tracks/ply-binary-io_20260904/spec.md | 40 +++++++++---------- 2 files changed, 24 insertions(+), 41 deletions(-) diff --git a/conductor/tracks/ply-binary-io_20260904/plan.md b/conductor/tracks/ply-binary-io_20260904/plan.md index 3e2592b..c866ef1 100644 --- a/conductor/tracks/ply-binary-io_20260904/plan.md +++ b/conductor/tracks/ply-binary-io_20260904/plan.md @@ -190,25 +190,12 @@ path is untouched until Task 4.6. Binary loads wherever ASCII does, and fails only where ASCII fails identically — so the binary writer introduces no MeshLab incompatibility. -- [!] **Pre-existing limitation confirmed, not a regression.** Rows 08/09 pin - the trigger as `texcoord` *combined with* a face of more than 3 corners: - texcoord alone is fine (03–05), a quad alone is fine (06–07), and the - pair fails in ASCII and binary alike. The failing ASCII file is - byte-identical (md5 `639a03ea…`) to what `e9635ab` wrote before this - track. - - Cause is in MeshLab's importer, not libcore's output: the error table in - `libio_base.so` carries vcglib's `import_ply.h` strings, including - "Face with no 6 texture coordinates" — per-wedge `texcoord` support is - hard-coded to 6 floats, i.e. 3 corners, so vcglib's polygonal path is - unavailable whenever texcoords are present. The files parse correctly - against an independently written PLY reader and round-trip through - `read_ply`. - - Out of scope for this track (it changes neither the endianness fix nor - the binary writer). Belongs with - [ply-multichart_20260624](../ply-multichart_20260624/index.md) (#19), - which already rewrites the `texcoord` write path. + 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 --- diff --git a/conductor/tracks/ply-binary-io_20260904/spec.md b/conductor/tracks/ply-binary-io_20260904/spec.md index 3028245..214320c 100644 --- a/conductor/tracks/ply-binary-io_20260904/spec.md +++ b/conductor/tracks/ply-binary-io_20260904/spec.md @@ -203,32 +203,28 @@ additive of the two. Precedes [mesh-io-options_20260904](../mesh-io-options_20260904/index.md) (#26), which was filed out of this track's decision to leave `write_mesh` untouched. -## Known Limitation (found during implementation) +## Interoperability Note: MeshLab and per-wedge `texcoord` -MeshLab cannot open **any** PLY libcore writes that combines a `texcoord` list -with a face of more than 3 corners — ASCII or binary, and regardless of this -track. It reports "Face with more than 3 vertices". +Not a libcore limitation. What `write_ply` emits is valid PLY: `texcoord` is a +list property, and a `2*N` count on an N-corner face is exactly what the format +allows. The constraint is in MeshLab's importer. + +MeshLab 2025.07 refuses any PLY that pairs a `texcoord` list with a face of more +than 3 corners, reporting "Face with more than 3 vertices". Its bundled +`libio_base.so` carries vcglib's `import_ply.h` error table, including "Face +with no 6 texture coordinates" — vcglib hard-codes per-wedge `texcoord` to 6 +floats, so its polygonal face path is unavailable once texcoords are present. Isolated in MeshLab 2025.07 with a matrix varying only arity and `texcoord`: triangles load with and without texcoord, a quad loads without texcoord, and -only the pair fails — in ASCII and binary identically. So the binary writer -adds no incompatibility of its own. - -The limitation is in MeshLab's importer. Its bundled `libio_base.so` carries -vcglib's `import_ply.h` error table, including "Face with no 6 texture -coordinates": per-wedge `texcoord` is hard-coded to 6 floats — 3 corners — so -vcglib's polygonal face path is disabled once texcoords are present. Files that -MeshLab refuses still parse correctly against an independent PLY reader, and -`read_ply` round-trips them. - -Verified not to be a regression: the failing ASCII tier-3 file is byte-identical -(md5 `639a03ea…`) to the output of `e9635ab`, the commit this track branched -from. Triangle-only meshes — what the EduceLab pipelines actually write — are -unaffected. - -Deferred to [ply-multichart_20260624](../ply-multichart_20260624/index.md) -(#19), which already rewrites the `texcoord` write path and is the right place -to decide between triangulating on write, warning, or documenting. +only the pair fails — in ASCII and binary identically. The same files parse +correctly against an independently written PLY reader and round-trip through +`read_ply`, and the failing ASCII file is byte-identical (md5 `639a03ea…`) to +what `e9635ab` wrote before this track. + +Recorded here only so the next person who sees that MeshLab error does not go +looking for a bug in libcore. Triangle meshes — what the EduceLab pipelines +write — are unaffected. ## Out of Scope From 09419b5af7276aa8935f118d0a08a3dbb0c78cb3 Mon Sep 17 00:00:00 2001 From: Seth Parker Date: Fri, 4 Sep 2026 18:46:26 -0400 Subject: [PATCH 21/23] docs(MeshIO): correct the float32 claim on write_ply tier 1 (ply-binary-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 --- include/educelab/core/io/MeshIO_PLY.hpp | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/include/educelab/core/io/MeshIO_PLY.hpp b/include/educelab/core/io/MeshIO_PLY.hpp index ec45cb7..171b4e4 100644 --- a/include/educelab/core/io/MeshIO_PLY.hpp +++ b/include/educelab/core/io/MeshIO_PLY.hpp @@ -1456,8 +1456,17 @@ void write_ply_data( * color set, also emits @c red @c green @c blue properties (@c uchar, 0–255; * a color-less mesh declares none). * - * Scalars are written as @c float32 whatever @c T is, so the on-disk layout - * never depends on the mesh's template parameters. + * The header declares @c float positions and normals and @c uchar colors + * whatever @c T is, so the file's *structure* never depends on the mesh's + * template parameters. + * + * @warning The two formats do not carry the same precision. + * @ref PLYFormat::Binary writes exactly the declared @c float32, + * while the ASCII path writes @c T at full decimal precision — so a + * @c Mesh3d keeps its @c double values through @c ASCII and narrows + * them to @c float through @c Binary. ASCII text wider than the + * declared @c float is long-standing behavior and unchanged here; a + * reader that honors the declaration narrows it anyway. * * @param path Output file path * @param mesh Mesh to write From 5e5ac862d3537ab49171fde4c9932948d9d5046e Mon Sep 17 00:00:00 2001 From: Seth Parker Date: Sat, 5 Sep 2026 06:25:42 -0400 Subject: [PATCH 22/23] chore(conductor): clear stale MeshLab note in track metadata (ply-binary-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 --- conductor/tracks/ply-binary-io_20260904/metadata.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/conductor/tracks/ply-binary-io_20260904/metadata.json b/conductor/tracks/ply-binary-io_20260904/metadata.json index 7835838..532874e 100644 --- a/conductor/tracks/ply-binary-io_20260904/metadata.json +++ b/conductor/tracks/ply-binary-io_20260904/metadata.json @@ -4,7 +4,7 @@ "type": "feature", "status": "complete", "created": "2026-09-04T00:00:00+0200", - "updated": "2026-09-04T23:20:00Z", + "updated": "2026-09-05T00:00:00Z", "issue": 25, "current_phase": 5, "current_task": "5.3", @@ -30,5 +30,5 @@ "398c7d7: feat(MeshIO): binary PLY write", "bfc4aa2: docs(MeshIO): document PLYFormat, list-count throws, and the write_mesh omission" ], - "notes": "Phase 4's manual MeshLab check is outstanding: MeshLab 2025.07 ships no CLI, so it was validated against an independently written PLY parser instead." + "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." } From a68c04cbde558d02711eb14370f4c08f699ddad1 Mon Sep 17 00:00:00 2001 From: Seth Parker Date: Sat, 5 Sep 2026 06:43:40 -0400 Subject: [PATCH 23/23] refactor(MeshIO): share one write_ply core across the three tiers (ply-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 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 --- include/educelab/core/io/MeshIO_PLY.hpp | 189 +++++++++++------------- tests/src/TestMeshIO.cpp | 55 ++++--- 2 files changed, 120 insertions(+), 124 deletions(-) diff --git a/include/educelab/core/io/MeshIO_PLY.hpp b/include/educelab/core/io/MeshIO_PLY.hpp index 171b4e4..d5b7ece 100644 --- a/include/educelab/core/io/MeshIO_PLY.hpp +++ b/include/educelab/core/io/MeshIO_PLY.hpp @@ -1131,6 +1131,10 @@ template void validate_ply_face_lists( const Mesh& mesh, bool has_uvs) { + // One bound, not two: the texcoord limit is half the vertex_indices one, + // so when UVs are written it is always the binding constraint and the + // 255 check below it could never fire. + // Widest value a uchar list count can express. constexpr std::size_t kMaxListCount = 255; constexpr std::size_t kMaxUVCorners = kMaxListCount / 2; // 2*N per face @@ -1145,21 +1149,17 @@ void validate_ply_face_lists( 2 * kMaxUVCorners <= kMaxFaceListLength, "write_ply's texcoord limit exceeds read_ply's face list cap"); + const std::size_t max_corners = has_uvs ? kMaxUVCorners : kMaxListCount; for (std::size_t fi = 0; fi < mesh.num_faces(); ++fi) { const auto n = mesh.face(fi).size(); - if (n > kMaxListCount) { - throw std::runtime_error( - "write_ply: face " + to_string(fi) + " has " + to_string(n) + - " corners, exceeding the " + to_string(kMaxListCount) + - " a uchar vertex_indices list count can express"); - } - if (has_uvs and n > kMaxUVCorners) { + if (n > max_corners) { throw std::runtime_error( "write_ply: face " + to_string(fi) + " has " + to_string(n) + - " corners, whose texcoord list of " + to_string(2 * n) + - " values exceeds the " + to_string(kMaxListCount) + - " a uchar list count can express (" + - to_string(kMaxUVCorners) + " corners max with UVs)"); + " corners, exceeding the " + to_string(max_corners) + + (has_uvs + ? " a uchar texcoord list count allows, since texcoord" + " writes 2 values per corner" + : " a uchar vertex_indices list count can express")); } } } @@ -1262,8 +1262,8 @@ void write_ply_data_binary( std::ostream& file, const Mesh& mesh, const UVMapT* uvmap, - [[maybe_unused]] bool has_normals, - [[maybe_unused]] bool has_colors) + bool has_normals, + bool has_colors) { using Vertex = typename Mesh::Vertex; @@ -1361,31 +1361,31 @@ void write_ply_data_binary( /** * @brief Write PLY ASCII vertex and face data to @p file * + * Text is written at @c T's full precision, which is wider than the + * @c property @c float the header declares; @ref write_ply_data_binary writes + * exactly the declared @c float32. See the @c write_ply overloads. + * * @p uvmap may be @c nullptr (no UV output). When non-null, each face record * is followed by a @c texcoord list of 2*N floats. Unmapped corners are * written as @c -1,-1 (sentinel for "no UV assignment"). @p has_normals and * @p has_colors must match the values passed to @ref write_ply_header so the * data matches the declared properties. * - * @p format selects the encoding; @c PLYFormat::Binary delegates to - * @ref write_ply_data_binary and @p buf is then unused. + * Sibling of @ref write_ply_data_binary; @ref write_ply_impl picks between + * them. */ template -void write_ply_data( +void write_ply_data_ascii( std::ostream& file, - std::array& buf, const Mesh& mesh, const UVMapT* uvmap, [[maybe_unused]] bool has_normals, - [[maybe_unused]] bool has_colors, - PLYFormat format) + [[maybe_unused]] bool has_colors) { using Vertex = typename Mesh::Vertex; - if (format == PLYFormat::Binary) { - write_ply_data_binary(file, mesh, uvmap, has_normals, has_colors); - return; - } + // Scratch for to_string_view; used only by this path. + std::array buf{}; for (std::size_t vi = 0; vi < mesh.num_vertices(); ++vi) { const auto& v = mesh.vertex(vi); @@ -1440,6 +1440,67 @@ void write_ply_data( } } +/** + * @brief Internal PLY writer shared by all public @ref write_ply overloads + * + * Mirrors @ref read_ply_impl on the read side: the three public tiers differ + * only in what they pass here. Keeping the sequence in one place is what makes + * the two cross-function invariants hold by construction rather than by each + * tier remembering them — @ref validate_ply_face_lists runs before + * @ref write_ply_data_binary's narrowing list-count casts, and the + * @c has_normals / @c has_colors given to the header are the same values the + * data writer sees. + * + * @p uvmap may be @c nullptr (no UVs); @p texture_comment may be empty (no + * @c comment @c TextureFile line). + * + * @throws std::runtime_error if the file cannot be opened, if writing fails, + * or if a face exceeds the @c uchar list-count limits + */ +template +void write_ply_impl( + const std::filesystem::path& path, + const Mesh& mesh, + const UVMapT* uvmap, + const std::string& texture_comment, + PLYFormat format) +{ + // Single source of truth: a texcoord list is written exactly when there is + // a UV map, so the validator, the header and the data writer cannot + // disagree about it. + const bool has_uvs = uvmap != nullptr; + + // Before the stream is opened, so a mesh that cannot be written leaves no + // truncated file behind. + validate_ply_face_lists(mesh, has_uvs); + + std::ofstream file(path, std::ios::binary); + if (!file) { + throw std::runtime_error( + "write_ply: cannot open file: " + path.string()); + } + + const bool has_normals = has_any_normal(mesh); + const bool has_colors = has_any_color(mesh); + + write_ply_header( + file, mesh, texture_comment, has_uvs, has_normals, has_colors, format); + if (format == PLYFormat::Binary) { + write_ply_data_binary(file, mesh, uvmap, has_normals, has_colors); + } else { + write_ply_data_ascii(file, mesh, uvmap, has_normals, has_colors); + } + + // Close before checking. The stream may still hold buffered data here; the + // final flush happens when `file` is destroyed and its failure would be + // swallowed, so write_ply would return normally on an incomplete file. + file.close(); + if (!file) { + throw std::runtime_error( + "write_ply: I/O error while writing file: " + path.string()); + } +} + } // namespace detail // ============================================================================= @@ -1486,32 +1547,8 @@ void write_ply( { static_assert(Dims >= 3, "write_ply requires Dims >= 3"); - detail::validate_ply_face_lists(mesh, false); - - std::ofstream file(path, std::ios::binary); - if (!file) { - throw std::runtime_error( - "write_ply: cannot open file: " + path.string()); - } - - std::array buf{}; - const bool has_normals = has_any_normal(mesh); - const bool has_colors = has_any_color(mesh); - detail::write_ply_header( - file, mesh, "", false, has_normals, has_colors, format); - detail::write_ply_data( - file, buf, mesh, static_cast*>(nullptr), - has_normals, has_colors, format); - - // Close before checking. The stream may still hold buffered data at this - // point; the final flush happens when `file` is destroyed, and a failure - // there would be swallowed, so write_ply would return normally on an - // incomplete file. close() performs that flush and records its failure. - file.close(); - if (!file) { - throw std::runtime_error( - "write_ply: I/O error while writing file: " + path.string()); - } + detail::write_ply_impl( + path, mesh, static_cast*>(nullptr), "", format); } // ============================================================================= @@ -1548,31 +1585,7 @@ void write_ply( { static_assert(Dims >= 3, "write_ply requires Dims >= 3"); - detail::validate_ply_face_lists(mesh, true); - - std::ofstream file(path, std::ios::binary); - if (!file) { - throw std::runtime_error( - "write_ply: cannot open file: " + path.string()); - } - - std::array buf{}; - const bool has_normals = has_any_normal(mesh); - const bool has_colors = has_any_color(mesh); - detail::write_ply_header( - file, mesh, "", true, has_normals, has_colors, format); - detail::write_ply_data( - file, buf, mesh, &uvmap, has_normals, has_colors, format); - - // Close before checking. The stream may still hold buffered data at this - // point; the final flush happens when `file` is destroyed, and a failure - // there would be swallowed, so write_ply would return normally on an - // incomplete file. close() performs that flush and records its failure. - file.close(); - if (!file) { - throw std::runtime_error( - "write_ply: I/O error while writing file: " + path.string()); - } + detail::write_ply_impl(path, mesh, &uvmap, "", format); } // ============================================================================= @@ -1616,32 +1629,8 @@ void write_ply( { static_assert(Dims >= 3, "write_ply requires Dims >= 3"); - detail::validate_ply_face_lists(mesh, true); - - std::ofstream file(path, std::ios::binary); - if (!file) { - throw std::runtime_error( - "write_ply: cannot open file: " + path.string()); - } - - std::array buf{}; - const bool has_normals = has_any_normal(mesh); - const bool has_colors = has_any_color(mesh); - detail::write_ply_header( - file, mesh, texture_path.string(), true, has_normals, has_colors, - format); - detail::write_ply_data( - file, buf, mesh, &uvmap, has_normals, has_colors, format); - - // Close before checking. The stream may still hold buffered data at this - // point; the final flush happens when `file` is destroyed, and a failure - // there would be swallowed, so write_ply would return normally on an - // incomplete file. close() performs that flush and records its failure. - file.close(); - if (!file) { - throw std::runtime_error( - "write_ply: I/O error while writing file: " + path.string()); - } + detail::write_ply_impl( + path, mesh, &uvmap, texture_path.string(), format); } // ============================================================================= diff --git a/tests/src/TestMeshIO.cpp b/tests/src/TestMeshIO.cpp index 715623e..a8541eb 100644 --- a/tests/src/TestMeshIO.cpp +++ b/tests/src/TestMeshIO.cpp @@ -23,6 +23,7 @@ #include "educelab/core/io/MeshIO_PLY.hpp" #include "educelab/core/types/Mesh.hpp" #include "educelab/core/types/UVMap.hpp" +#include "educelab/core/utils/Math.hpp" #include "educelab/core/utils/MeshUtils.hpp" namespace fs = std::filesystem; @@ -190,8 +191,8 @@ static auto make_ngon(std::size_t n) -> Mesh3f Mesh3f m; Mesh3f::Face idx(n); for (std::size_t i = 0; i < n; ++i) { - const auto a = 2.f * 3.14159265f * static_cast(i) / - static_cast(n); + const auto a = + 2.f * PI * static_cast(i) / static_cast(n); (void)m.insert_vertex(std::cos(a), std::sin(a), 0.f); idx[i] = i; } @@ -1938,6 +1939,29 @@ static auto expected_binary_format_line() -> std::string : "format binary_big_endian 1.0\n"; } +// Hand-derived IEEE-754 spellings, big-endian, for the values make_triangle() +// and its UV map use. +static constexpr std::initializer_list kZeroF32{ + 0x00, 0x00, 0x00, 0x00}; +static constexpr std::initializer_list kOneF32{ + 0x3F, 0x80, 0x00, 0x00}; +static constexpr std::initializer_list kNegOneF32{ + 0xBF, 0x80, 0x00, 0x00}; + +// The binary body of make_triangle(): v0=(0,0,0) v1=(1,0,0) v2=(0,1,0) as +// float32, then the uchar-counted vertex_indices list 0 1 2. Shared so a +// change to make_triangle() is a one-place fixup. +static void push_triangle_body(std::vector& want) +{ + push_host(want, kZeroF32); push_host(want, kZeroF32); push_host(want, kZeroF32); + push_host(want, kOneF32); push_host(want, kZeroF32); push_host(want, kZeroF32); + push_host(want, kZeroF32); push_host(want, kOneF32); push_host(want, kZeroF32); + want.push_back(0x03); // uchar list count needs no swap + push_host(want, {0x00, 0x00, 0x00, 0x00}); + push_host(want, {0x00, 0x00, 0x00, 0x01}); + push_host(want, {0x00, 0x00, 0x00, 0x02}); +} + TEST_F(PLYTest, BinaryWrite_ByteLevel) { // make_triangle(): v0=(0,0,0) v1=(1,0,0) v2=(0,1,0), face 0 1 2. @@ -1963,15 +1987,7 @@ TEST_F(PLYTest, BinaryWrite_ByteLevel) // 3 vertices x 3 float32, then a uchar count and 3 int32 indices. std::vector want; - const std::initializer_list zero{0x00, 0x00, 0x00, 0x00}; - const std::initializer_list one{0x3F, 0x80, 0x00, 0x00}; - push_host(want, zero); push_host(want, zero); push_host(want, zero); - push_host(want, one); push_host(want, zero); push_host(want, zero); - push_host(want, zero); push_host(want, one); push_host(want, zero); - want.push_back(0x03); // uchar list count needs no swap - push_host(want, {0x00, 0x00, 0x00, 0x00}); - push_host(want, {0x00, 0x00, 0x00, 0x01}); - push_host(want, {0x00, 0x00, 0x00, 0x02}); + push_triangle_body(want); ASSERT_EQ(body.size(), want.size()); EXPECT_EQ(body, want); @@ -2065,20 +2081,11 @@ TEST_F(PLYTest, BinaryWrite_TexcoordList) << header; std::vector want; - const std::initializer_list zero{0x00, 0x00, 0x00, 0x00}; - const std::initializer_list one{0x3F, 0x80, 0x00, 0x00}; - const std::initializer_list neg_one{0xBF, 0x80, 0x00, 0x00}; - push_host(want, zero); push_host(want, zero); push_host(want, zero); - push_host(want, one); push_host(want, zero); push_host(want, zero); - push_host(want, zero); push_host(want, one); push_host(want, zero); - want.push_back(0x03); - push_host(want, {0x00, 0x00, 0x00, 0x00}); - push_host(want, {0x00, 0x00, 0x00, 0x01}); - push_host(want, {0x00, 0x00, 0x00, 0x02}); + push_triangle_body(want); want.push_back(0x06); // 2*N, still a uchar count - push_host(want, zero); push_host(want, zero); // corner 0 (0,0) - push_host(want, one); push_host(want, zero); // corner 1 (1,0) - push_host(want, neg_one); push_host(want, neg_one); // corner 2 unmapped + push_host(want, kZeroF32); push_host(want, kZeroF32); // corner 0 (0,0) + push_host(want, kOneF32); push_host(want, kZeroF32); // corner 1 (1,0) + push_host(want, kNegOneF32); push_host(want, kNegOneF32); // corner 2 unmapped ASSERT_EQ(body.size(), want.size()); EXPECT_EQ(body, want);