Implement full MeshIO (OBJ/PLY)#17
Merged
Merged
Conversation
…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>
csparker247
commented
Mar 25, 2026
…-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>
Member
Author
|
@copilot Review the PR implementation |
Co-authored-by: csparker247 <1434526+csparker247@users.noreply.github.com> Agent-Logs-Url: https://github.com/educelab/libcore/sessions/d7bca254-3f1d-4647-abdb-4eb0f9586a21
…_obj Tier 3b Co-authored-by: csparker247 <1434526+csparker247@users.noreply.github.com> Agent-Logs-Url: https://github.com/educelab/libcore/sessions/d7bca254-3f1d-4647-abdb-4eb0f9586a21
Contributor
Reviewed the implementation and fixed issues found:
All 39 tests pass. |
…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
…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>
- 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 <stem> 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
commented
Apr 23, 2026
csparker247
left a comment
Member
Author
There was a problem hiding this comment.
@copilot Review the comments.
Copilot stopped work on behalf of
csparker247 due to an error
April 23, 2026 18:36
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>
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>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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_meshfacade 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,.Plyall work)io/MeshIO_OBJ.hpp— read/write OBJ + MTL with per-wedge UVs, multi-chart support viausemtlio/MeshIO_PLY.hpp— read/write ASCII PLY, read binary little-endian PLY, per-wedge texcoord list + backward-compat per-vertex s/thas_normal,has_color) drive compile-time inclusion of normals and colors in both readers and writershas_chart) drive multi-chart OBJ outputSupporting additions
utils/MeshUtils.hpp—expand_at_seamsfor formats needing per-vertex UVsMesh:num_vertices,num_faces,clearString.hpp: zero-allocationsplit(string_view, vector&, pred)predicate overload,to_string/to_string_view(charconv-based, locale-independent, buffer-reusing), per-type charconv fallbackscmake/CheckCharconvFP.cmakereplacesCheckToNumericFP.cmake— probesfrom_charsandto_charsindependently for float, double, long double, emitting per-typeEDUCE_CORE_NEED_*definitionsFaithful 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/WithColormesh with empty attributes wrotevn 0 0 0/ inline0 0 0(OBJ) and declarednx/ny/nz/red/green/bluewith 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.types/Mesh.hpp:has_any_normal(mesh)andhas_any_color(mesh)— runtime companions to the compile-timehas_normal/has_colortraits, returningtrueonly when at least one vertex actually carries the attribute.build_normal_indexemits a compactvnpool referenced per-corner (decoupled from vertex index), and inline RGB is gated per-vertex oncolor.has_value(). Normal-less / color-less vertices emit no attribute data, and partially-attributed meshes round-trip exactly with no fabrication.nx/ny/nzandred/green/blueheader declarations (and matching data columns) are gated onhas_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).has_any_normal, with@notepointers fromWithNormal,WithColor, andWithChart): traits whose storage carries an unset state (std::optional, or a type likeColorwith its own empty state) need ahas_any_<trait>guard in the writers; plain defaulted-value traits whose default is valid (e.g.WithChart'sstd::size_t chart, default0= first chart) do not.Hardening
insert_faceelement/propertylines throw instead of being silently skipped;propertybefore anyelementthrowsU8C3for uchar,U16C3for ushort,F32C3for float/double) instead of being forced through a lossy uchar castmtllibwith no argument now throws instead of dereferencing out of boundswrite_obj/write_plytier (catches silent I/O failures like disk full)Tests
61 cases in
tests/src/TestMeshIO.cppcovering 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, baremtllib, 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). Newhas_any_normal/has_any_colorunit tests intests/src/TestMesh.cpp.