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.md b/conductor/tracks.md index d0a5e93..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/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..532874e 100644 --- a/conductor/tracks/ply-binary-io_20260904/metadata.json +++ b/conductor/tracks/ply-binary-io_20260904/metadata.json @@ -2,16 +2,33 @@ "id": "ply-binary-io_20260904", "title": "PLY Binary IO", "type": "feature", - "status": "pending", + "status": "complete", "created": "2026-09-04T00:00:00+0200", - "updated": "2026-09-04T00:00:00+0200", + "updated": "2026-09-05T00:00:00Z", "issue": 25, + "current_phase": 5, + "current_task": "5.3", "phases": { - "total": 0, - "completed": 0 + "total": 5, + "completed": 5 }, "tasks": { - "total": 0, - "completed": 0 - } + "total": 22, + "completed": 22 + }, + "commits": [ + "b28d606: docs(conductor): record byte-order helper placement in detail", + "802863c: test(MeshIO): byte-order helper tests for host detection and swapping", + "5861ee1: feat(MeshIO): hand-rolled byte-order detection and width-dispatched swap", + "f8940e4: test(MeshIO): big-endian PLY read fixtures", + "c185fc9: fix(MeshIO): honor the byte order declared in the PLY header on read", + "3483f88: test(MeshIO): drop the big-endian rejection test, document byte-order support", + "549b2ed: test(MeshIO): face list-count limit tests for the PLY writer", + "7583422: feat(MeshIO): reject face list counts a uchar cannot express", + "18e21b8: feat(MeshIO): add public PLYFormat and thread it through write_ply", + "ab25dc0: test(MeshIO): binary PLY write tests", + "398c7d7: feat(MeshIO): binary PLY write", + "bfc4aa2: docs(MeshIO): document PLYFormat, list-count throws, and the write_mesh omission" + ], + "notes": "Phase 4's MeshLab check was completed manually: a binary mesh loads correctly. It also surfaced that MeshLab cannot open any PLY pairing a texcoord list with a face of more than 3 corners, in ASCII or binary alike - a vcglib importer constraint, not a libcore one. Reader/writer robustness bugs found by code review of #28 are fixed separately in #29." } 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..c866ef1 --- /dev/null +++ b/conductor/tracks/ply-binary-io_20260904/plan.md @@ -0,0 +1,267 @@ +# Implementation Plan: PLY Binary IO + +**Track ID:** ply-binary-io_20260904 +**Spec:** [spec.md](./spec.md) +**Created:** 2026-09-04 +**Status:** [x] Complete + +> Task detail is at planning granularity. Per-task test lists get filled in when +> each phase starts, per the strict tests-first workflow. Design detail lives in +> [spec.md](./spec.md) and [educelab/libcore#25](https://github.com/educelab/libcore/issues/25). + +## Overview + +Read before write. The reader is where the existing bug is, it is the smaller +change, and a correct reader is the only instrument that can check the writer — +so it lands first and the writer's tests get to lean on it. Validation sits +between them because it is a precondition of the binary writer being safe, not a +polish step. + +**Phases 1–2 stand alone**: they fix a real correctness bug (endianness ignored +on read) and are mergeable without any writer work. Phases 3–4 add the feature. + +All work is in `include/educelab/core/io/MeshIO_PLY.hpp` and +`tests/src/TestMeshIO.cpp`, with doc-only touches to +`include/educelab/core/io/MeshIO.hpp`. No new files, no new test target. + +## Checkpoints + +| Phase | Checkpoint SHA | Date | Status | +| ------- | -------------- | ---- | ------- | +| Phase 1 | `5861ee1` | 2026-09-04 | verified | +| Phase 2 | `3483f88` | 2026-09-04 | verified | +| Phase 3 | `7583422` | 2026-09-04 | verified | +| Phase 4 | `398c7d7` | 2026-09-04 | verified* | +| Phase 5 | `bfc4aa2` | 2026-09-04 | verified | + +--- + +## Phase 1: Byte-order foundation + +The library targets `cxx_std_17`, so `std::endian` (C++20) and `std::byteswap` +(C++23) are both unavailable. Host-order detection and a width-dispatched swap +have to be written by hand, and they are worth isolating from the PLY logic that +consumes them. + +### Tasks + +- [x] **Task 1.1**: Decide where the helpers live — `namespace detail` in + `MeshIO_PLY.hpp` (assumed: PLY-local, adds no public surface) or a new + public `utils/Endian.hpp`. Record the decision and rationale in + `spec.md`. +- [x] **Task 1.2**: Tests for host-order detection and byte swapping across all + four widths the format uses (1, 2, 4, 8 bytes), including the 1-byte + no-op and a `double` case. +- [x] **Task 1.3**: Implement host-endianness detection (`__BYTE_ORDER__` where + the compiler defines it, `_WIN32` fallback) and the width-dispatched + swap. ``/`` only — no new dependency, no new include of + anything platform-specific. + +### Verification + +- [x] Helper tests pass +- [x] Build succeeds in Debug and Release +- [x] Full existing suite still green (nothing consumes the helpers yet) + +--- + +## Phase 2: Reader honors declared endianness + +Removes the `binary_big_endian` rejection and makes `binary_little_endian` mean +what it says. Swapping enters at the two choke points every binary scalar read +passes through. + +### Tasks + +- [x] **Task 2.1**: Hand-crafted `binary_big_endian` fixture test — bytes + reversed by the test itself, positions plus an `int32` index list, read + back on an LE host. Exercises more than the 4-byte width so the dispatch + is covered, not just the common case. +- [x] **Task 2.2**: Test that the swap precedes the cast: a BE `float` + `3F 80 00 00` must read as 1.0, not 4.6e-41. This is the failure mode the + spec calls out as unrecoverable if the order is inverted, so it gets its + own test rather than riding along inside a fixture assertion. +- [x] **Task 2.3**: Add a runtime `bool` swap parameter to + `read_ply_binary_prop` and `read_ply_prop_from_buf`, swapping the raw + fixed-width value **before** `static_cast`. Not a template + parameter — see spec. +- [x] **Task 2.4**: In `read_ply_impl`, compute the flag from `hdr.format` + against host order, delete the `BinaryBE` rejection, and thread it to + every call site — the batched vertex path, `read_ply_face_binary`, and + the `skip_binary_prop` lambda. Missing one leaves a silently misread + property. +- [x] **Task 2.5**: Replace `PLYTest.BinaryBigEndian_Throws` with the positive + read test from Task 2.1, and update the `read_ply` Doxygen that currently + says "Supports ASCII and binary-little-endian PLY". + +### Verification + +- [x] BE fixture and pre-cast-swap tests pass +- [x] `BinaryLittleEndian_Read` and `SizedTypeAliases_BinaryLittleEndian_Read` + still pass unmodified — the LE path must not have moved +- [x] No remaining reference to the BE rejection in code or docs +- [x] Full suite green, Debug and Release + +--- + +## Phase 3: Face list-count validation + +A precondition of the binary writer, not polish: `uchar` list counts silently +truncate, so a 300-corner face would write a header the reader cannot make sense +of. ASCII has the same latent problem and gets the same guard. + +### Tasks + +- [x] **Task 3.1**: Tests for both limits and both boundaries — a 256-corner + face throws naming `vertex_indices` and the face index; a 128-corner face + with a UV map throws naming `texcoord`; 255 corners without UVs and 127 + corners with UVs both write cleanly. +- [x] **Task 3.2**: Implement the two limits in the write path. The message + names which limit fired and the offending face index. +- [x] **Task 3.3**: Confirm coherence with the reader's existing caps + (`kMaxFaceVertices` = 256, `kMaxFaceListLength` = 1024 in + `read_ply_face_binary`) — everything the writer now permits must still be + readable. Verify, do not assume. + +### Verification + +- [x] Limit tests pass, including both non-throwing boundary cases +- [x] A maximal legal face round-trips through ASCII (`FaceWith255Corners_*`, + `FaceWith127CornersWithUVs_*`); binary added in Task 4.5 +- [x] Full suite green, Debug and Release + +--- + +## Phase 4: Binary write + +The feature itself. `PLYFormat` and the plumbing land first as a no-op signature +change so the tests that follow have something to compile against; the ASCII +path is untouched until Task 4.6. + +### Tasks + +- [x] **Task 4.1**: Add public `enum class PLYFormat { ASCII, Binary }` and + thread it through the three `write_ply` tiers (defaulting to `ASCII`) into + `write_ply_header` and `write_ply_data`, which ignore it for now. + Behavior-neutral; kept distinct from `detail::PLYHeader::Format`. +- [x] **Task 4.2**: Byte-level writer test — a small positions-only mesh + written as `Binary`, asserted against a hand-derived byte literal + (header line, then exact vertex and face bytes). The test encodes the + format, not libcore's opinion of it. +- [x] **Task 4.3**: Test that scalars are `float32` regardless of `T` — write a + `Mesh` and assert 4-byte scalars and a `property float x` + declaration. +- [x] **Task 4.4**: Test that `ASCII` remains the default — an unqualified + `write_ply` call still produces `format ascii 1.0`, asserted explicitly + rather than left to the existing header-grepping tests. +- [x] **Task 4.5**: Round-trip tests for structural breadth — n-gon faces, UVs, + colors, normals, empty mesh — where hand-computing bytes stops paying. +- [x] **Task 4.6**: `write_ply_header` emits the `format` line matching the + requested format and the host's byte order. +- [x] **Task 4.7**: Binary path in `write_ply_data` — precomputed property + offsets, one `write` per vertex record, `float32` scalars, `uchar` + colors, `uchar`-prefixed `vertex_indices` and `texcoord` lists. Mirrors + the reader's batching rather than writing per property. +- [x] **Task 4.8**: Open all three tiers with `std::ios::binary` + unconditionally. Note in the PR that Windows ASCII callers stop getting + CRLF; `read_ply` already trims `\r`, so nothing regresses on read. + +### Verification + +- [x] Byte-level, `float32`-width, default-ASCII and round-trip tests all pass +- [x] Existing ASCII tests pass unmodified, and ASCII output is byte-identical + to the pre-track writer across all three tiers (verified by diffing the + output of `e9635ab`'s header against the current one) +- [x] A binary file written by libcore opens correctly in MeshLab — **verified + manually in MeshLab 2025.07**, against a 9-file matrix varying only face + arity and the presence of a `texcoord` list: + + | # | Mesh | Format | texcoord | MeshLab | + | - | ---- | ------ | -------- | ------- | + | 01 | triangles | ASCII | no | loads | + | 02 | triangles | Binary | no | loads | + | 03 | triangles | ASCII | yes | loads | + | 04 | triangles | Binary | yes | loads | + | 05 | triangles | Binary | yes | loads | + | 06 | quad | ASCII | no | loads | + | 07 | quad | Binary | no | loads | + | 08 | quad | ASCII | yes | **fails** | + | 09 | quad | Binary | yes | **fails** | + + Binary loads wherever ASCII does, and fails only where ASCII fails + identically — so the binary writer introduces no MeshLab incompatibility. + Rows 08/09 are a **MeshLab** constraint, not a libcore one. What + `write_ply` emits is valid PLY — `texcoord` is a list property and a + `2*N` count on an N-corner face is what the format allows. vcglib + hard-codes per-wedge `texcoord` to 6 floats, so MeshLab cannot read that + one combination. Nothing to fix in libcore; see the interoperability note + in [spec.md](./spec.md). +- [x] Full suite green, Debug and Release + +--- + +## Phase 5: Documentation + +### Tasks + +- [x] **Task 5.1**: `@throws` for the list-count limits on the tier-2 and + tier-3 `write_ply` overloads and the corresponding `write_mesh` + dispatchers in `MeshIO.hpp`. Tier 1 carries only the 255 limit. +- [x] **Task 5.2**: Document `PLYFormat` on the `write_ply` overloads, and note + that `write_mesh` deliberately gains no format parameter — deferred to + [#26](https://github.com/educelab/libcore/issues/26) — so the omission + reads as a decision rather than an oversight. +- [x] **Task 5.3**: Doxygen builds cleanly with no new warnings. + +### Verification + +- [x] Doxygen clean — 22 warnings, byte-identical to the pre-track baseline at + `e9635ab`. Zero new. (The 22 are pre-existing and out of this track's + scope; four were introduced by Phases 1–4 and are fixed here.) +- [x] Every acceptance criterion in `spec.md` maps to a passing test or a + merged doc change — all 15 annotated in place in `spec.md` + +--- + +## Final Verification + +- [x] All acceptance criteria in [spec.md](./spec.md) met — all 15 annotated + in place with the task and test that satisfies each +- [x] Full test suite passes in Debug and Release — 17/17 targets each, + `TestMeshIO` at 100 tests (82 → 100 over the track) +- [x] Doxygen updated for `PLYFormat`, `@throws`, and BE read support; warning + count identical to the pre-track baseline +- [x] PR notes the Windows CRLF change for ASCII callers — see below +- [x] Ready for review; #19 unblocked to rebase onto the new + `write_ply_header` / `write_ply_data` signatures (both gained a trailing + `PLYFormat format` parameter, and `write_ply_data` now delegates to + `write_ply_data_binary`) +- [x] MeshLab open — verified manually; see the Phase 4 note for the matrix + +## For the PR description + +- **Windows ASCII callers stop getting CRLF.** All three `write_ply` tiers now + open with `std::ios::binary` unconditionally (Task 4.8). On POSIX this is a + no-op — ASCII output is byte-identical to the pre-track writer. Nothing + regresses on read: `read_ply` already trims `\r` + (`PLYTest.ReadCommentTextureFile_CRLFLineEndings`). +- **`binary_little_endian` previously meant "native, labeled little-endian".** + On the little-endian hosts EduceLab runs on this was invisible, so no + existing file changes meaning; it was still wrong. +- **`detail` signature changes for #19:** `write_ply_header` and + `write_ply_data` take a trailing `PLYFormat`, `read_ply_binary_prop` and + `read_ply_prop_from_buf` take a trailing `bool needs_swap` (no default, by + design), and `read_ply_face_binary` takes `needs_swap` after + `load_texcoords`. +- **`kMaxFaceVertices` / `kMaxFaceListLength`** moved from function-local + `constexpr` in two functions to namespace scope in `detail`, so + `validate_ply_face_lists` can `static_assert` the writer's limits against + them. + +--- + +_Generated by Conductor. Tasks will be marked [~] in progress and [x] complete._ + +_*Phase 4 verified except the manual MeshLab open, which cannot be automated +(MeshLab 2025.07 ships no CLI). Validated against an independently written PLY +parser instead; sample files listed in the Phase 4 notes._ 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..214320c --- /dev/null +++ b/conductor/tracks/ply-binary-io_20260904/spec.md @@ -0,0 +1,258 @@ +# Specification: PLY Binary IO + +**Track ID:** ply-binary-io_20260904 +**Type:** Feature +**Created:** 2026-09-04 +**Status:** Implemented +**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 + +- [x] A public `educelab::PLYFormat { ASCII, Binary }` exists and is threaded + through all three `write_ply` tiers, defaulting to `ASCII`. + *(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. + *(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). + *(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. + *(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. + *(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 + +- [x] `read_ply` honors the endianness declared in the header, byte-swapping + when file order differs from host order. + *(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. + *(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). + *(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 + +- [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`). + *(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 + +### `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. + +### 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 +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. + +## Interoperability Note: MeshLab and per-wedge `texcoord` + +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. 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 + +- **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._ 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 97171bb..d5b7ece 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" @@ -21,9 +23,94 @@ 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 { +// ------------------------------------------------------------------------- +// 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 { @@ -310,63 +397,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: @@ -383,49 +484,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: @@ -443,6 +556,9 @@ auto read_ply_prop_from_buf(const char* buf, PLYType type) -> DestT // 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; @@ -456,12 +572,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) { @@ -473,8 +594,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) { @@ -485,8 +606,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 " + @@ -507,8 +628,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( @@ -639,11 +760,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) { @@ -718,8 +843,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. @@ -805,27 +930,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 } @@ -920,7 +1045,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; @@ -984,6 +1109,61 @@ 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 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 + */ +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 + + // 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"); + + 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 > max_corners) { + throw std::runtime_error( + "write_ply: face " + to_string(fi) + " has " + to_string(n) + + " 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")); + } + } +} + /** * @brief Write the PLY ASCII header to @p file * @@ -993,7 +1173,10 @@ void read_ply_impl( * 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( @@ -1002,12 +1185,20 @@ 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, + 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'; @@ -1044,19 +1235,148 @@ 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, + bool has_normals, + 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 * + * 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. + * + * 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, @@ -1064,6 +1384,9 @@ void write_ply_data( { using Vertex = typename Mesh::Vertex; + // 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); file << to_string_view(buf, v[0]) << ' ' @@ -1117,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 // ============================================================================= @@ -1124,7 +1508,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 @@ -1133,37 +1517,38 @@ 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 + * 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 + * @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( - 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"); - std::ofstream file(path); - 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); - detail::write_ply_data( - file, buf, mesh, static_cast*>(nullptr), - has_normals, has_colors); - - // 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); } // ============================================================================= @@ -1171,7 +1556,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 @@ -1179,37 +1564,28 @@ 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( 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"); - std::ofstream file(path); - 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); - detail::write_ply_data(file, buf, mesh, &uvmap, has_normals, has_colors); - - // 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); } // ============================================================================= @@ -1217,10 +1593,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 @@ -1230,39 +1606,31 @@ 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( 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"); - std::ofstream file(path); - 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); - detail::write_ply_data(file, buf, mesh, &uvmap, has_normals, has_colors); - - // 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); } // ============================================================================= @@ -1272,7 +1640,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 72f4a1a..a8541eb 100644 --- a/tests/src/TestMeshIO.cpp +++ b/tests/src/TestMeshIO.cpp @@ -1,11 +1,18 @@ #include +#include #include +#include #include #include +#include #include #include +#include #include +#include +#include +#include #if defined(__unix__) || defined(__APPLE__) #include @@ -16,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; @@ -176,6 +184,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 * PI * 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 //------------------------------------------------------------------------------ @@ -1115,6 +1151,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 //------------------------------------------------------------------------------ @@ -1435,6 +1586,668 @@ 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})); +} + +//------------------------------------------------------------------------------ +// 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); +} + +//------------------------------------------------------------------------------ +// 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"; +} + +// 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. + 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; + push_triangle_body(want); + + 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; + push_triangle_body(want); + want.push_back(0x06); // 2*N, still a uchar count + 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); +} + +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. @@ -1686,28 +2499,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 //------------------------------------------------------------------------------