Skip to content

Implement full MeshIO (OBJ/PLY)#17

Merged
csparker247 merged 50 commits into
developfrom
feature/mesh-io_20260323
Jun 24, 2026
Merged

Implement full MeshIO (OBJ/PLY)#17
csparker247 merged 50 commits into
developfrom
feature/mesh-io_20260323

Conversation

@csparker247

@csparker247 csparker247 commented Mar 25, 2026

Copy link
Copy Markdown
Member

Summary

This PR implements OBJ and PLY mesh IO for libcore — read and write, with three tiers (positions only; positions + UV map; positions + UV map + texture paths). A case-insensitive read_mesh / write_mesh facade dispatches by file extension. The mesh IO track (mesh-io_20260323) is complete.

MeshIO pipeline

  • io/MeshIO.hpp — facade with case-insensitive extension dispatch (.obj, .OBJ, .Ply all work)
  • io/MeshIO_OBJ.hpp — read/write OBJ + MTL with per-wedge UVs, multi-chart support via usemtl
  • io/MeshIO_PLY.hpp — read/write ASCII PLY, read binary little-endian PLY, per-wedge texcoord list + backward-compat per-vertex s/t
  • Vertex traits (has_normal, has_color) drive compile-time inclusion of normals and colors in both readers and writers
  • UVMap traits (has_chart) drive multi-chart OBJ output

Supporting additions

  • utils/MeshUtils.hppexpand_at_seams for formats needing per-vertex UVs
  • Mesh: num_vertices, num_faces, clear
  • String.hpp: zero-allocation split(string_view, vector&, pred) predicate overload, to_string / to_string_view (charconv-based, locale-independent, buffer-reusing), per-type charconv fallbacks
  • cmake/CheckCharconvFP.cmake replaces CheckToNumericFP.cmake — probes from_chars and to_chars independently for float, double, long double, emitting per-type EDUCE_CORE_NEED_* definitions

Faithful round-trips (no fabricated attributes)

Writers no longer fabricate default attribute values for a trait-capable mesh that has none actually set. Previously a WithNormal / WithColor mesh with empty attributes wrote vn 0 0 0 / inline 0 0 0 (OBJ) and declared nx/ny/nz / red/green/blue with zeros (PLY); reading such a file back materialised unwanted zero normals / black colors, so a capable-but-empty mesh did not round-trip as empty.

  • New runtime guards in types/Mesh.hpp: has_any_normal(mesh) and has_any_color(mesh) — runtime companions to the compile-time has_normal / has_color traits, returning true only when at least one vertex actually carries the attribute.
  • OBJ: build_normal_index emits a compact vn pool referenced per-corner (decoupled from vertex index), and inline RGB is gated per-vertex on color.has_value(). Normal-less / color-less vertices emit no attribute data, and partially-attributed meshes round-trip exactly with no fabrication.
  • PLY: the nx/ny/nz and red/green/blue header declarations (and matching data columns) are gated on has_any_normal / has_any_color. Because a PLY element has fixed properties this is all-or-nothing per attribute — a fully-empty attribute is omitted entirely, while a partial attribute still zero-fills gaps (intrinsic to the format).
  • Trait-author convention (documented on has_any_normal, with @note pointers from WithNormal, WithColor, and WithChart): traits whose storage carries an unset state (std::optional, or a type like Color with its own empty state) need a has_any_<trait> guard in the writers; plain defaulted-value traits whose default is valid (e.g. WithChart's std::size_t chart, default 0 = first chart) do not.

Hardening

  • Binary PLY: stream state checked after every read; truncated files throw instead of returning garbage
  • OBJ: face vertex indices bounds-checked before insert_face
  • PLY header: element count capped at 500M; malformed element / property lines throw instead of being silently skipped; property before any element throws
  • PLY face records: texcoord and unknown list property counts capped at 1024 (vertex_indices still capped at 256)
  • PLY colors: stored in their native precision (U8C3 for uchar, U16C3 for ushort, F32C3 for float/double) instead of being forced through a lossy uchar cast
  • OBJ mtllib with no argument now throws instead of dereferencing out of bounds
  • Write-error detection on every write_obj / write_ply tier (catches silent I/O failures like disk full)
  • Stream error check after OBJ read loop distinguishes I/O errors from normal EOF

Tests

61 cases in tests/src/TestMeshIO.cpp covering round-trips (positions, normals, colors, UVs, textures, multi-chart, N-gons, empty meshes), facade dispatch, all hardening cases (truncated binary PLY, out-of-bounds indices, excessive element / texcoord / list counts, malformed PLY headers, bare mtllib, case-insensitive extensions, native-precision color storage), and faithful round-trips with no fabricated attributes (capable-but-empty meshes emit nothing and round-trip empty; partial normals / colors round-trip exactly). New has_any_normal / has_any_color unit tests in tests/src/TestMesh.cpp.

csparker247 and others added 8 commits March 25, 2026 06:38
…ring

Extends String.hpp with utilities needed for locale-independent mesh IO
serialization and efficient whitespace parsing of OBJ/PLY files.

- split(): add predicate overload (single O(n) pass); no-arg overload now
  splits on any whitespace via the predicate path; string-delimiter overload
  fast-paths single-char delimiters through the predicate overload
- to_string_view(buf, val): caller-supplied buffer variant of to_chars with
  no heap allocation; intended for hot paths (e.g. per-vertex coordinate
  output in a write loop)
- to_string(val): convenience wrapper over to_string_view for one-off
  conversions; allocates a std::string

Replaces CheckToNumericFP.cmake + CheckToCharsFP.cmake with a unified
CheckCharconvFP.cmake that probes from_chars and to_chars separately.
EDUCE_CORE_NEED_CHARCONV_FP is now set only by the from_chars probe (the
only path with a compile-time fallback). Updates README and conductor
artifacts for the mesh-io_20260323 track.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Replace split_ws references with split() (no-arg whitespace form)
- Update to_string_view usage to caller-provided buffer pattern:
  to_string_view(buf, val) instead of to_string_view(val)
- Record preparatory String.hpp commit (2f67926) in plan overview
- Bump updated date to 2026-03-25

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…h-io_20260323)

Add compile-time detection traits has_normal<V>, has_color<V>, and
has_chart<UVMapT> in detail/MeshTraits.hpp using the std::void_t detection
idiom. IO functions use these via if constexpr to conditionally read/write
normal, color, and chart data without extra overloads.

Register TestMeshIO.cpp and add MeshTraits.hpp to installed headers.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add read_obj / write_obj across all three tiers in MeshIO_OBJ.hpp.
Write uses to_string_view(buf, val) for zero-allocation numeric output.
Read uses split() + parse_face_ref() for locale-independent parsing.

Detection traits (has_normal, has_color, has_chart) drive all
conditional IO via if constexpr. Multi-chart write enforces has_chart
via static_assert. Normals are written/read per-vertex; UVs are
per-wedge. MTL texture paths are recovered from map_Kd in
material-declaration order.

Also adds Mesh::num_vertices() / num_faces() required for iteration.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…0260323)

- Add MeshUtils.hpp with expand_at_seams: deduplicates (vertex, uv-pool)
  pairs across face corners, splitting seam vertices and returning a flat
  per-vertex UV array suitable for PLY output
- Add MeshIO_PLY.hpp with write_ply (Tiers 1–3) and read_ply (Tiers 1–2);
  supports ASCII write, ASCII and binary-little-endian read, per-vertex
  s/t UV properties, and comment TextureFile header (MeshLab convention)
- Add expand_at_seams and PLY round-trip tests (Tasks 3.1, 3.3) covering
  no-seam, single-seam, binary PLY, UV expansion, texture path round-trip,
  and missing-file error cases
- Register MeshIO_PLY.hpp and MeshUtils.hpp as installed public headers

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Comment thread include/educelab/core/io/MeshIO_OBJ.hpp
csparker247 and others added 5 commits March 25, 2026 15:27
…-io_20260323)

Add MeshIO.hpp with read_mesh and write_mesh overloads (Tiers 1–3) that
dispatch to OBJ or PLY IO by file extension; throw std::runtime_error for
unsupported extensions. Add facade dispatch tests for both formats and all
three tiers. Register MeshIO.hpp as an installed public header.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…20260323)

Security:
- parse_face_ref: throw on zero or negative OBJ face index
- read_ply_impl: cap per-face vertex count at 256; validate face indices
  against n_vertices; throw on binary big-endian format

Performance:
- String::split(): thread_local static locale eliminates per-call mutex
- Add split_into() overload; OBJ/PLY ASCII parsers reuse token vector
- read_obj_impl: hoist face_verts/face_vts/face_vns outside loop
- expand_at_seams: std::unordered_map with documented Fibonacci pair hash

Architecture:
- write_obj: extract write_obj_vertices/faces helpers; Tiers 1-3a share them
- write_ply: extract write_ply_header/data helpers; all tiers share them
- Add read_ply(path, mesh, uvmap) Tier 2 overload
- read_mesh Tier 2 calls new read_ply Tier 2 directly (no throwaway vector)

Testing:
- Unique temp dirs via this-pointer hex suffix in all three fixtures
- Full 3x3 position verification in all position round-trip tests
- Verify all three vertex colors in color round-trip tests
- Write-failure tests for write_obj, write_ply, write_mesh
- PLY UV coordinate value verification
- v/vt/vn combined face format test
- PLY malformed header test
- NCMesh (normals+colors) OBJ round-trip test
- BinaryBE throw test
- expand_at_seams empty-mesh test

Platform:
- CheckCharconvFP.cmake: add long double to_chars probe
- String.hpp: to_string_view<long double> fallback via double cast when
  EDUCE_CORE_NEED_TO_CHARS_LONG_DOUBLE_FALLBACK is defined (macOS-26)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Replace the single EDUCE_CORE_NEED_CHARCONV_FP flag with 6 independent
per-type probes (from_chars × {float,double,long double} and to_chars ×
{float,double,long double}).  Each fallback specialisation is now gated
on its own definition, so a platform where only long double is missing
does not regress float/double paths.

to_string_view fallbacks use snprintf (float: %.9g, double: %.17g);
long double casts to double (same representation on Apple Silicon).

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
@csparker247

Copy link
Copy Markdown
Member Author

@copilot Review the PR implementation

Copilot AI and others added 2 commits March 26, 2026 00:17

Copilot AI commented Mar 26, 2026

Copy link
Copy Markdown
Contributor

@copilot Review the PR implementation

Reviewed the implementation and fixed issues found:

  1. color_to_rgb (OBJ) and color_to_u8c3 (PLY) — Added missing support for U16C1, U16C3, U16C4, U8C4, and F32C4 color types (4c51c7c)
  2. Duplicate using Vertex declaration in write_obj Tier 3b removed (658cdb5)

All 39 tests pass.

Copilot AI and others added 2 commits March 26, 2026 00:35
…tive dispatch, and edge-case tests

Co-authored-by: csparker247 <1434526+csparker247@users.noreply.github.com>
Agent-Logs-Url: https://github.com/educelab/libcore/sessions/50bb342d-a00b-4bf5-9c2b-7c2af9957968
Co-authored-by: csparker247 <1434526+csparker247@users.noreply.github.com>
Agent-Logs-Url: https://github.com/educelab/libcore/sessions/50bb342d-a00b-4bf5-9c2b-7c2af9957968
Copilot AI changed the title Feature/mesh io 20260323 Harden mesh IO: stream validation, bounds checks, case-insensitive dispatch Mar 26, 2026
csparker247 and others added 11 commits April 21, 2026 19:58
…ispatch

Pre-compute PropRole enum for every PLYProp in parse_ply_header based on
element context (vertex vs face). Replace the if-else name-comparison chains
in both binary and ASCII vertex/face reader inner loops with switch statements
on the pre-computed role, eliminating O(properties) string comparisons per
vertex/face. No behavior change; 17/17 tests pass. (mesh-io_20260323)
…reader

Pre-compute per-property byte offsets from the PLY header so the binary
vertex loop makes one istream::read() per vertex (O(vertices)) instead of
one read per property per vertex (O(properties × vertices)).

Adds read_ply_prop_from_buf<T> alongside the existing stream-based
read_ply_binary_prop<T>; unknown/skipped properties are read into the
buffer and their bytes are simply ignored, preserving correct stream
positioning without extra istream::ignore() calls.

Closes task 5.7 (mesh-io_20260323)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ly_impl

The face-element branch of read_ply_impl contained ~80 lines of nested
binary + ASCII parsing logic inside the element loop. Extract into two
inline detail functions:
- read_ply_face_binary: parses one binary face record from an istream
- read_ply_face_ascii: parses one ASCII face record from a token vector

Both take a reusable face_indices buffer and texcoords vector as
out-params, clearing them before filling. read_ply_impl now delegates
to these helpers and constructs the Face from the returned index buffer,
reducing the nesting depth and isolating per-face parsing logic.

Pre-compute load_texcoords = uvmap != nullptr && has_texcoord once
outside the face loop; both helpers receive it as a plain bool.

Closes task 5.8 (mesh-io_20260323)

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…_20260323)

Tasks were completed in Phase 5 (cb5ee97, 6b9d7f5); carry-forward
references in Phase 6 were not ticked at the time.
- PLYProp, PLYElement, PLYHeader and their members/enums: add @brief +
  inline ///<  comments so EXTRACT_PRIVATE builds cleanly
- ply_type_bytes, read_ply_binary_prop, read_ply_impl: add @brief
- Fix broken \ref read_ply(path,mesh) (used param names, not types)
- MeshIO_OBJ.hpp: escape <stem> as &lt;stem&gt; to suppress Doxygen
  xml/html tag warning
- product.md: add UVMap, IO, and trait detection to Key Goals
- tech-stack.md: add UVMap, MeshUtils, MeshIO, MeshIO_OBJ, MeshIO_PLY to component table
- README.md: add MeshIO header-only entries; add Mesh IO usage section
@csparker247 csparker247 changed the title Harden mesh IO: stream validation, bounds checks, case-insensitive dispatch Implement full MeshIO (OBJ/PLY) Apr 23, 2026

@csparker247 csparker247 left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot Review the comments.

Comment thread conductor/product.md Outdated
Comment thread include/educelab/core/utils/Math.hpp Outdated
Comment thread include/educelab/core/utils/String.hpp
Comment thread README.md Outdated
Comment thread README.md Outdated
Comment thread include/educelab/core/io/MeshIO_PLY.hpp Outdated
Comment thread include/educelab/core/io/MeshIO_PLY.hpp Outdated
Comment thread include/educelab/core/io/MeshIO_PLY.hpp
Comment thread README.md Outdated
Comment thread README.md Outdated
Copilot AI and others added 2 commits April 23, 2026 15:01
Addresses all unresolved review comments on the MeshIO implementation
PR.

## `MeshIO_PLY.hpp`
- **`PLYType` enum** replaces `std::string` for `PLYProp::type` and
`list_count_type`; `parse_ply_type()` converts header tokens at parse
time. `ply_type_bytes`, `read_ply_binary_prop`, and
`read_ply_prop_from_buf` now dispatch via `switch` instead of sequential
string comparisons
- **Color-type conditions** now match only `PLYType::UChar` /
`PLYType::UShort`; removed signed `char`/`short` cases that could
produce incorrect casts for negative values
- **`trim_right` (string_view)** replaces `trim_right_in_place` in all
three ASCII read loops; the trimmed `string_view` is passed directly to
`split`, avoiding a redundant cast

## CMake charconv probes
- `CheckCharconvFP.cmake` now appends needed definitions to
`EDUCE_CORE_CHARCONV_DEFS` instead of calling `add_compile_definitions`
- `CMakeLists.txt` applies them via `target_compile_definitions(core
PUBLIC ...)`, so they propagate automatically to any downstream target
via `target_link_libraries`

```cmake
# No manual propagation needed anymore
target_link_libraries(foo PRIVATE educelab::core)
```

## Miscellaneous
- `Math.hpp`: `cross()` error message → `"Inputs must be 3-dimensional"`
- `String.hpp`: comment at predicate-dispatch call site clarifying it
routes to the `is_invocable_r_v`-constrained overload, not back into the
variadic template
- `README.md`: charconv section updated with correct per-type definition
names (`EDUCE_CORE_NEED_FROM/TO_CHARS_{FLOAT,DOUBLE,LONG_DOUBLE}`) and
revised cmake example reflecting automatic propagation
- `conductor/product.md`: trimmed goal #4 per suggestion

---------

Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
Co-authored-by: csparker247 <1434526+csparker247@users.noreply.github.com>
csparker247 and others added 8 commits April 23, 2026 18:56
Flip if/else blocks with a small false branch into early
continue/return/skip, hoisting the main body out one indent level.

- PLY: read_ply_face_ascii inner loop (matches read_ply_face_binary)
- PLY: skip_binary_prop lambda
- OBJ: read_obj_impl MTL parse block (early return)
- OBJ: write_obj Tier 3b chart-grouping loop

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
write_obj previously emitted one vn per vertex via value_or(zero) with the
vn index tied to the vertex index, so a WithNormal mesh with no normals set
got fabricated "vn 0 0 0" lines (and v//vn refs), and partial meshes got
zeros in the gaps.

Decouple the vn pool from the v pool: build_normal_index assigns a compact
1-based vn index in vertex order only to vertices that carry a normal. The
vn block and all face writers (shared + Tier 3b chart-grouped) reference it
per corner, so normal-less meshes emit no vn at all and partial meshes
reference normals only where present.

Add regression tests for the no-normals and partial-normals cases.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
A WithColor-capable mesh with no colors set previously wrote fabricated
black: OBJ appended "r g b" of 0,0,0 to every v line; PLY declared
red/green/blue and wrote 0 0 0 per vertex. Mirrors the earlier normals fix.

Add has_any_color(mesh) to Mesh.hpp as the runtime companion to the
compile-time traits::has_color, alongside has_any_normal. OBJ gates inline
RGB per vertex (its v lines carry color positionally and the reader detects
it per line), so partial meshes round-trip without fabrication. PLY's
fixed-property element forces an all-or-nothing header decision: declare
red/green/blue only when some color exists (partial meshes still zero-fill
gaps, intrinsic to PLY).

Add has_any_color unit test plus OBJ/PLY no-color and OBJ partial-color
regression tests.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Record why optional-bearing traits (WithNormal, WithColor) need a
has_any_<trait> runtime guard in the I/O writers while plain defaulted-value
traits (WithChart) do not. The authoritative "trait authors" note lives on
has_any_normal; WithNormal/WithColor/WithChart carry @note pointers to it so
the rule is discoverable from wherever a new trait gets defined.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…work

Reconcile stale track artifacts with the completed work: mark the Mesh IO
track Complete (deps resolved), correct progress to 7/7 phases / 39/39 tasks,
and clear the "⚠ blocked" marker from the conductor index. Add Phase 7 to the
plan documenting the no-fabrication round-trip work (has_any_normal /
has_any_color, compact OBJ vn pool, per-vertex inline RGB, gated PLY attribute
declarations, trait-author convention docs).

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@csparker247 csparker247 marked this pull request as ready for review June 24, 2026 00:15
@csparker247 csparker247 merged commit 8eb070c into develop Jun 24, 2026
5 checks passed
@csparker247 csparker247 deleted the feature/mesh-io_20260323 branch June 24, 2026 00:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants