Skip to content

fix(MeshIO): accept sized PLY type aliases on read - #24

Merged
csparker247 merged 2 commits into
developfrom
fix/ply-sized-type-aliases
Sep 4, 2026
Merged

fix(MeshIO): accept sized PLY type aliases on read#24
csparker247 merged 2 commits into
developfrom
fix/ply-sized-type-aliases

Conversation

@csparker247

@csparker247 csparker247 commented Sep 4, 2026

Copy link
Copy Markdown
Member

Problem

parse_ply_type accepted only the eight type names from Greg Turk's original PLY description — char uchar short ushort int uint float double. That is spec-conformant: those eight are the only ones the format defines. But a second set of spellings is in wide circulation. vcglib (and therefore MeshLab) has carried both for two decades, naming them typenames and newtypenames and accepting either; OpenMVS emits the sized set.

An OpenMVS refine_mesh.ply header reads:

format binary_little_endian 1.0
element vertex 5340268
property float32 x
property float32 y
property float32 z
element face 10661114
property list uint8 uint32 vertex_indices
end_header

so every such file threw:

read_ply: unrecognized property type 'float32'

This is a be-liberal-in-what-you-accept gap rather than a conformance bug, but the practical effect is that libcore cannot read the output of a tool our pipelines are built on.

Change

parse_ply_type now accepts int8/uint8/int16/uint16/int32/uint32/float32/float64 alongside the spec names. write_ply continues to emit the spec spellings, so libcore's own output is byte-identical to before.

Tests

The existing round-trip tests could not catch this — they only ever read what write_ply wrote, which is exactly why the gap was invisible from inside. Both new tests use hand-written headers:

  • SizedTypeAliases_ASCII_Read — positions, normals (float64), colors (uint8), and the face list.
  • SizedTypeAliases_BinaryLittleEndian_Read — mirrors the OpenMVS header shape. An alias mapped to the wrong byte width desynchronizes the whole data section rather than throwing, so the binary case needs its own coverage.

Verified in both directions: with the parse_ply_type change stashed, both new tests fail on unrecognized property type 'float32'; with it applied, all 71 MeshIO tests pass.

Follow-up

Binary PLY write is still missing, and the reader currently ignores the header's declared endianness (it reinterprets raw bytes as native). Both are scoped in the ply-binary-io_20260904 track.

🤖 Generated with Claude Code

@csparker247 @claude
fix(MeshIO): accept sized PLY type aliases on read
a334219 
parse_ply_type accepted only the eight type names from Greg Turk's
original PLY description (char, uchar, short, ushort, int, uint, float,
double). That is spec-conformant, but a second set of spellings is in
wide circulation: vcglib -- and therefore MeshLab -- has parsed both for
two decades, and OpenMVS emits the sized names.

An OpenMVS refine_mesh.ply declares:

    property float32 x
    property list uint8 uint32 vertex_indices

so every such file threw "read_ply: unrecognized property type 'float32'".

Accept int8/uint8/int16/uint16/int32/uint32/float32/float64 as aliases
for the corresponding spec names. write_ply continues to emit the spec
spellings, so libcore's own output is unchanged.

The existing round-trip tests could not catch this: they only ever read
what write_ply wrote. Both new tests use hand-written headers -- one
ASCII covering positions, normals, colors and the face list, and one
binary-little-endian mirroring the OpenMVS header shape, since an alias
mapped to the wrong byte width desynchronizes the data section rather
than throwing.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This was referenced Sep 4, 2026
Closed
Merged
csparker247 added a commit that referenced this pull request Sep 4, 2026
@csparker247 @claude
docs(conductor): register PLY Binary IO and write_mesh options tracks (…
ab36082 
…#27)

Registers two tracks from the PLY binary IO design discussion.

**`ply-binary-io_20260904` — PLY Binary IO** (#25)

Binary PLY write support, plus making `read_ply` honor the endianness
declared in the file header. These are one contract and land together:
`read_ply` currently reinterprets raw bytes as native regardless of what
the header says — native-endian reading labeled little-endian — so a
writer that can emit binary must not be able to produce files the reader
refuses or silently misreads.

Sequenced before #19 (Multi-Chart PLY Write Support). Both rewrite
`write_ply_header` and `write_ply_data`; #19 is Pending at 0/15, and
binary IO is the live need — `pgs-decimate` rewrites its mesh once per
improved search round and currently pays ASCII for it.

Depends on #24.

**`mesh-io-options_20260904` — write_mesh Options Struct** (#26)

Deferred from the above and deliberately unscoped. `PLYFormat` lands on
`write_ply` only, because a PLY-only value is meaningless for half of
`write_mesh`'s inputs and there's no good answer to what
`write_mesh("out.obj", mesh, PLYFormat::Binary)` should do. Registered
so the intention is discoverable rather than living only in a design
conversation.

Docs only — no code changes.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
@csparker247
csparker247 enabled auto-merge (squash) September 4, 2026 20:24
@csparker247
csparker247 merged commit 2eaba49 into develop Sep 4, 2026
5 checks passed
@csparker247
csparker247 deleted the fix/ply-sized-type-aliases branch September 4, 2026 20:25
@csparker247 csparker247 mentioned this pull request Sep 4, 2026
Merged
csparker247 added a commit that referenced this pull request Sep 5, 2026
@csparker247 @claude
docs(conductor): PLY binary IO spec and implementation plan
5a9a04a 
The feature plan for PLY binary write and header-declared endianness on
read. Docs only — no library code, no tests, no build changes. Merging
this records the plan; it does not start implementation.

Tracks #25. Precedes #19 (multi-chart PLY write), which rewrites the same
two functions. Depends on #24, merged as 2eaba49.

5 phases / 22 tasks, ordered reader-before-writer: the reader holds the
existing bug, is the smaller change, and is the only instrument that can
check the writer. Phases 1–2 stand alone as a correctness fix.

Three constraints found in the source that the issue did not state:

- std::endian (C++20) and std::byteswap (C++23) are unavailable — the
  library targets cxx_std_17 — so host-order detection and the swap are
  hand-rolled. Phase 1 exists for this.
- Three call sites need the swap flag, not the two the issue names:
  read_ply_face_binary and the skip_binary_prop lambda reach the choke
  points independently of the batched vertex path.
- PLYTest.BinaryBigEndian_Throws asserts the behavior being removed and
  has to be replaced, not added around.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
csparker247 added a commit that referenced this pull request Sep 6, 2026
@csparker247 @claude
feat(MeshIO): binary PLY write and byte-order-correct read (#25) (#28)
b11f68d 
## What this is

PLY binary write support, plus the endianness fix on read. Tracks #25.

This PR started as the docs-only feature plan; the branch now carries
the
**full implementation** of all 5 phases / 22 tasks. Depends on #24
(merged as
`2eaba49`). Precedes #19.

## What changed

| Area | Change |
|------|--------|
| **Read** | `read_ply` now honors the byte order declared in the header
instead of reinterpreting raw bytes as native. The `binary_big_endian`
rejection is gone. |
| **Write** | New public `PLYFormat { ASCII, Binary }` on all three
`write_ply` tiers, defaulting to `ASCII`. Binary writes native order and
labels the header to match. |
| **Validation** | `write_ply` rejects faces whose `uchar` list counts
cannot be expressed: 255 corners for `vertex_indices`, 127 with UVs
since `texcoord` writes `2*N`. |
| **Docs** | `@throws` and `PLYFormat` on the `write_ply` overloads and
the `write_mesh` dispatchers. |

```cpp
write_ply(path, mesh);                        // unchanged — ASCII
write_ply(path, mesh, PLYFormat::Binary);     // new
write_ply(path, mesh, uvmap, PLYFormat::Binary);
write_ply(path, mesh, uvmap, tex, PLYFormat::Binary);
```

Read before write, as the plan ordered it: the reader held the existing
bug, is
the smaller change, and is the only instrument that can check the
writer.
Phases 1–2 are the standalone correctness fix; 3–5 add the feature.

## Behavior changes to know about

- **Windows ASCII callers stop getting CRLF.** All three tiers now open
with
`std::ios::binary` unconditionally. On POSIX this is a no-op — ASCII
output is
byte-identical to the pre-track writer across all three tiers, verified
by
compiling the same generator against `e9635ab`'s header and diffing.
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 — #19 will need to rebase

Both tracks rewrite `write_ply_header` and `write_ply_data`.

| Function | Change |
|----------|--------|
| `write_ply_header` | trailing `PLYFormat format` |
| `write_ply_data` | trailing `PLYFormat format`; delegates to new
`write_ply_data_binary` |
| `read_ply_binary_prop` | trailing `bool needs_swap` — **no default, by
design** |
| `read_ply_prop_from_buf` | trailing `bool needs_swap` — **no default,
by design** |
| `read_ply_face_binary` | `bool needs_swap` after `load_texcoords` |
| `kMaxFaceVertices`, `kMaxFaceListLength` | moved from function-local
`constexpr` in two functions to namespace scope in `detail` |

Omitting the default on `needs_swap` was deliberate: it made the
compiler
enumerate all fifteen call sites rather than letting one silently
misread a
property.

## Testing

`TestMeshIO` goes from 82 to 100 tests. Green in Debug and Release;
Doxygen at
the pre-track warning count (22, unchanged, zero new).

Round-trips deliberately do **not** anchor this work — a byte-order
mistake
shared by reader and writer round-trips perfectly, which is how the
sized-alias
bug in #24 survived. Instead:

- **Reader:** hand-crafted `binary_big_endian` fixtures with bytes
reversed by
the test itself, covering every scalar width the format uses (float,
double,
ushort, uchar count, int32 list). Values are non-palindromic so an
unswapped
  read cannot pass by accident.
- **Swap-before-cast** gets its own test at each choke point — BE `3F 80
00 00`
  must read as `1.0`, not the denormal 4.6e-41.
- **Writer:** byte-level assertions against hand-derived IEEE-754
literals, so
the tests encode the format rather than libcore's opinion of it. A
`Mesh3d`
  write is asserted byte-identical to the `Mesh3f` one.
- **Round-trips** only for structural breadth: n-gons, UVs, colors,
normals,
  empty mesh, and the maximal legal faces.

Two findings worth flagging:

1. `PLYTest.BinaryBigEndian_Throws` was passing for the wrong reason —
its
fixture declares a face but writes no face bytes, so after the rejection
was
removed it threw on truncation instead. It would have stayed green
whatever
   the byte-order code did. Replaced with a positive read test.
2. Task 3.3's "verify, do not assume" became a `static_assert`: the
writer's
limits are now checked against the reader's caps at compile time, so
raising
either without revisiting the other is a build error rather than a file
   libcore writes and refuses to read.

## Manual verification

Verified in MeshLab 2025.07 against a matrix varying only face arity and
the
presence of a `texcoord` list:

| Mesh | ASCII | Binary |
|------|-------|--------|
| triangles, no texcoord | loads | loads |
| triangles + texcoord | loads | loads |
| quad, no texcoord | loads | loads |
| quad + texcoord | **fails** | **fails** |

Binary loads wherever ASCII loads and fails only where ASCII already
failed, so
the binary writer introduces no incompatibility.

The quad + `texcoord` row is a **MeshLab** constraint, not a libcore
one:
`texcoord` is a list property and a `2*N` count on an N-corner face is
what the
format allows. MeshLab's bundled `libio_base.so` carries vcglib's
`import_ply.h` error table including "Face with no 6 texture
coordinates" —
per-wedge `texcoord` is hard-coded to 6 floats, so vcglib's polygonal
path is
unavailable once texcoords are present. Those files parse correctly
against an
independently written PLY reader and round-trip through `read_ply`, and
the
failing ASCII file is byte-identical to what `e9635ab` wrote. Recorded
as an
interoperability note in `spec.md`; nothing to change here.

## Out of scope

- A format parameter on `write_mesh` — a PLY-only value is meaningless
for half
  its inputs. Deferred to #26; `write_mesh` gains only `@throws` docs.
- Writing non-native byte order; widening list counts beyond `uchar`;
  configurable scalar width. See `spec.md` for the rationale on each.

---------

Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@csparker247