diff --git a/doc/reviewer/usability-audit-2026-05-31.md b/doc/reviewer/usability-audit-2026-05-31.md new file mode 100644 index 0000000..6caddb9 --- /dev/null +++ b/doc/reviewer/usability-audit-2026-05-31.md @@ -0,0 +1,455 @@ +# Usability & Documentation Audit β€” ConformalLabpp v0.10.0 + +**Date:** 2026-05-31 +**Auditor:** External reviewer (Claude Sonnet 4.6) +**Branch:** `review/usability-audit-2026-05-31` +**Base:** `main` (post external-audit-2026-05-30 merge) +**Focus:** Documentation quality, API usability, new-user experience + +This document is self-contained. A new session can pick up any finding +and act on it without prior context. Each finding includes exact file paths, +the concrete problem, and a precise fix with acceptance criteria. + +Status legend: πŸ”΄ Usability bug Β· 🟑 Doc error/stale Β· 🟠 Missing content + +--- + +## How to read this document in a new session + +```bash +git checkout review/usability-audit-2026-05-31 + +# No build needed for most fixes β€” documentation and example changes only. +# For changes that touch compilable code (examples), verify with: +cmake -S code -B build-cgal -DWITH_CGAL_TESTS=ON -DCONFORMALLAB_LOW_MEMORY_BUILD=ON +cmake --build build-cgal --target conformallab_cgal_tests -j1 +ctest --test-dir build-cgal -R '^cgal\.' --output-on-failure +``` + +--- + +## FINDING-U1 β€” πŸ”΄ CRITICAL: All examples show the trivial identity map, not real conformal flattening + +### Location +- `README.md` lines 100–121 ("Minimal usage") +- `code/examples/example_euclidean.cpp` lines 72–83 (Step 4) +- `code/examples/example_layout.cpp` lines 40–49 (`set_natural_theta`) +- `code/examples/example_hyper_ideal.cpp` lines 89–98 (Step 3) + +### Problem + +Every example uses the "natural theta" trick: +```cpp +auto G0 = euclidean_gradient(mesh, x0, maps); +for (auto v : mesh.vertices()) + if (maps.v_idx[v] >= 0) + maps.theta_v[v] -= G0[maps.v_idx[v]]; +``` + +This sets Θ_v to the *actual angle sums at x=0*, making x*=0 the equilibrium. +The result: `u_v β‰ˆ 0` everywhere β€” **no deformation**. The map is identity. + +Natural theta is a valid test pattern (it proves Newton converges in 0 or 1 +steps and guards against solver regressions). But as the *only* pattern shown to +a new user, it gives the false impression that the library produces trivial +output. The real use case β€” **conformally flatten a surface to the plane** β€” +is never demonstrated. + +### What is missing + +A "real-world" example that shows the actual use case: + +```cpp +// Conformally flatten a mesh to the plane. +// Set Θ_v = 2Ο€ (regular interior vertex, zero cone angle defect) for all +// free vertices; enforce_gauss_bonnet makes the assignment topologically +// consistent. The result is a flat conformal map with minimal angle distortion. +for (auto v : mesh.vertices()) + maps.theta_v[v] = conformallab::TWO_PI; +conformallab::enforce_gauss_bonnet(mesh, maps); +auto res = conformallab::newton_euclidean(mesh, x0, maps); +auto layout = conformallab::euclidean_layout(mesh, res.x, maps); +// layout.uv[v.idx()] now holds 2D coordinates of the flattening +``` + +### Fix + +1. **Add a new example** `code/examples/example_flatten.cpp`: + - Takes a real mesh from `code/data/` (e.g. `cathead.obj` or `torus_8x8.off`) + - Sets Θ_v = 2Ο€, calls `enforce_gauss_bonnet`, solves Newton, computes layout + - Saves a UV-mapped OFF file + - Shows non-trivial u_v values at output + +2. **Update README "Minimal usage"** to show the flatten use case instead of + (or in addition to) natural theta. Add a comment explaining what natural + theta is and when to use it. + +3. **Add a comment** to all existing examples explaining that natural theta + is a *testing convenience*, not a typical use case. + +### Acceptance criteria +- [ ] `example_flatten.cpp` compiles and runs with a real mesh input +- [ ] Output shows non-trivial `u_v β‰  0` and a visible UV parameterisation +- [ ] README "Minimal usage" demonstrates real flattening (not identity) +- [ ] Existing examples have a comment: "Note: natural theta β†’ x* = 0 (testing + pattern). For real flattening, set theta_v = TWO_PI and call enforce_gauss_bonnet." + +--- + +## FINDING-U2 β€” πŸ”΄ CRITICAL: No example for the CGAL public API + +### Location +- `code/include/CGAL/Discrete_conformal_map.h` (the "user-facing entry") +- `code/examples/` (no example uses CGAL::discrete_conformal_map_euclidean) + +### Problem + +`Discrete_conformal_map.h` is documented as "User-facing entry point for the +Discrete_conformal_map package." The Doxygen doc shows a minimal usage snippet: + +```cpp +auto result = CGAL::discrete_conformal_map_euclidean(mesh); +``` + +But this snippet only exists in a Doxygen comment β€” no compilable example +demonstrates it. All four `code/examples/*.cpp` use the *internal* API +(`setup_euclidean_maps + newton_euclidean`). A CGAL user who reaches this +library via the CGAL ecosystem has no runnable starting point. + +The CGAL API and the internal API also produce *different result types*: +- Internal: `NewtonResult.x` β€” DOF-index-indexed `std::vector` +- CGAL: `Conformal_map_result.u_per_vertex` β€” vertex-index-indexed `std::vector` + +This discrepancy is not explained anywhere for users. + +### Fix + +Add `code/examples/example_cgal_api.cpp`: +```cpp +// Demonstrates the CGAL public API (Discrete_conformal_map.h). +// Contrast with example_euclidean.cpp which uses the internal API directly. + +#include +#include +#include +// ... +auto result = CGAL::discrete_conformal_map_euclidean(mesh); +// result.u_per_vertex[v.idx()] β€” per-vertex scale factor +// result.converged, result.iterations, result.gradient_norm +``` + +Also add to README: a short paragraph explaining when to use CGAL API +vs internal API (CGAL API: simpler, less control; internal API: full pipeline, +holonomy, period matrix). + +### Acceptance criteria +- [ ] `example_cgal_api.cpp` compiles with `-DWITH_CGAL=ON` +- [ ] Example uses `CGAL::discrete_conformal_map_euclidean` (not internal API) +- [ ] README explains the two API levels and when to use each + +--- + +## FINDING-U3 β€” 🟑 DOC ERROR: `contracts.md` incorrect after Finding-B + +### Location +`doc/api/contracts.md` line 16 + +### Problem + +```markdown +| `check_gauss_bonnet()` | `theta_v[v]` set for all vertices | + Throws `std::runtime_error` if Ξ£(2Ο€βˆ’Ξ˜α΅₯) β‰  2π·χ(M) | +``` + +After the external-audit-2026-05-30 Finding-B fix, calling +`check_gauss_bonnet(mesh, hyper_ideal_maps)` is a **compile error** +(the overload is `= delete`). The contract table implies it works for +all map types β€” which is now false. + +Also missing from the table: the HyperIdeal functional needs no +Gauss-Bonnet pre-check because its energy is strictly convex +(Springborn 2020 Theorem 1.3) β€” Newton converges for any target angles. + +### Fix + +```markdown +| `check_gauss_bonnet()` | `theta_v[v]` set Β· **Euclidean or Spherical maps only** | + Throws if Ξ£(2Ο€βˆ’Ξ˜α΅₯) β‰  2π·χ(M). **Not applicable to HyperIdealMaps** β€” + deleted overload; HyperIdeal needs no pre-check (strictly convex energy). | +``` + +Also add a row for `enforce_gauss_bonnet`: +```markdown +| `enforce_gauss_bonnet()` | `theta_v[v]` set Β· **Euclidean or Spherical maps only** | + Shifts all Θα΅₯ by Ξ΄ = (lhsβˆ’rhs)/V so that GB holds exactly. + **Not applicable to HyperIdealMaps** β€” deleted overload. | +``` + +### Acceptance criteria +- [ ] `check_gauss_bonnet` row notes "Euclidean/Spherical only, not HyperIdeal" +- [ ] `enforce_gauss_bonnet` row added with same note +- [ ] A brief explanation why HyperIdeal doesn't need GB: "strictly convex energy" + +--- + +## FINDING-U4 β€” 🟑 DOC ERROR: Stale version in README (v0.9.0) + +### Location +`README.md` line 17 + +### Problem + +```markdown +**Status:** v0.9.0 β€” Phases 1–9a complete, Phase 8b-Lite CGAL API surface. +``` + +The current release is **v0.10.0** (tagged on `main`; CLAUDE.md confirms this). +CLAUDE.md was updated to v0.10.0 but the README was not. + +### Fix + +```markdown +**Status:** v0.10.0 β€” Phases 1–9b complete. Newton solvers for five DCE +models (Euclidean / Spherical / HyperIdeal / CP-Euclidean / Inversive-Distance), +priority-BFS layout in ℝ²/SΒ²/PoincarΓ© disk, Gauss–Bonnet, tree-cotree cut +graph, MΓΆbius holonomy, period matrix (genus 1), fundamental domain, +halfedge_uv texture atlas, JSON/XML serialisation, CLI app. +277 tests passing, 0 skipped β€” see [`doc/api/tests.md`](doc/api/tests.md). +``` + +### Acceptance criteria +- [ ] README status line shows v0.10.0 and the correct feature list +- [ ] Test count updated to 277 + +--- + +## FINDING-U5 β€” 🟑 DOC ERROR: CGAL header comment describes superseded state + +### Location +`code/include/CGAL/Discrete_conformal_map.h` lines 14–16 + +### Problem + +```cpp +/// This header provides a single function β€” `discrete_conformal_map_euclidean` +/// β€” that computes a Euclidean discrete-conformal flattening … Spherical and +/// hyperbolic variants are scheduled for Phase 8b.2 once the Euclidean +/// pattern is validated +``` + +But the file already contains `discrete_conformal_map_spherical()` and +`discrete_conformal_map_hyper_ideal()` (and the other models). The comment +describes Phase 8a state; the file is at Phase 8b-Lite. + +### Fix + +Update the `\file` Doxygen block at the top of `Discrete_conformal_map.h`: + +```cpp +/*! +\file CGAL/Discrete_conformal_map.h +\ingroup PkgConformalMapRef + +User-facing entry points for the Discrete_conformal_map package. + +This header provides five functions covering all three DCE geometries: + - `CGAL::discrete_conformal_map_euclidean` (flat / ℝ²) + - `CGAL::discrete_conformal_map_spherical` (SΒ², genus 0) + - `CGAL::discrete_conformal_map_hyper_ideal` (HΒ², genus β‰₯ 1) + - `CGAL::discrete_circle_packing_euclidean` β†’ Discrete_circle_packing.h + - `CGAL::discrete_inversive_distance_map` β†’ Discrete_inversive_distance.h +... +*/ +``` + +### Acceptance criteria +- [ ] `\file` doc block lists all five entry functions +- [ ] No forward-reference to "planned Phase 8b.2" β€” it is already implemented + +--- + +## FINDING-U6 β€” 🟑 STALE EXAMPLES: New gauge-vertex overload not reflected + +### Location +- `README.md` lines 107–110 +- `code/examples/example_euclidean.cpp` lines 62–68 +- `code/examples/example_layout.cpp` lines 52–59 (`pin_first` helper) + +### Problem + +Finding-D (external-audit-2026-05-30) added a clean gauge-vertex overload: +```cpp +assign_euclidean_vertex_dof_indices(mesh, maps, gauge_vertex); +``` + +But all examples still use the old verbose loop: +```cpp +auto vit = mesh.vertices().begin(); +maps.v_idx[*vit++] = -1; // pinned +int idx = 0; +for (; vit != mesh.vertices().end(); ++vit) + maps.v_idx[*vit] = idx++; +``` + +A new user copying from the examples will write the old error-prone pattern +instead of the new clean overload β€” defeating the purpose of the fix. + +### Fix + +Replace the old loop in all three locations with the new overload: +```cpp +// Pin the first vertex, assign sequential DOF indices to the rest. +int n = assign_euclidean_vertex_dof_indices(mesh, maps, + *mesh.vertices().begin()); +``` + +Same for `assign_vertex_dof_indices` (spherical) and +`assign_inversive_distance_vertex_dof_indices` wherever they appear in examples. + +### Acceptance criteria +- [ ] README "Minimal usage" uses the overload +- [ ] `example_euclidean.cpp` uses the overload +- [ ] `example_layout.cpp` `pin_first` helper replaced with the overload +- [ ] Old manual loop removed from all user-facing code + +--- + +## FINDING-U7 β€” 🟠 MISSING: `CONFORMALLAB_LOW_MEMORY_BUILD` absent from `getting-started.md` + +### Location +`doc/getting-started.md` β€” "Build modes" section + +### Problem + +`getting-started.md` documents five compile-time modes but does not list +`CONFORMALLAB_LOW_MEMORY_BUILD=ON`. A Raspberry Pi or low-RAM user who +reads `getting-started.md` (the natural first stop) cannot find the flag. +It only appears in CLAUDE.md and the README table. + +### Fix + +Add to `getting-started.md` after the `CONFORMALLAB_FAST_TEST_BUILD` entry: + +```markdown +### Low-memory build (RAM-constrained CI / Raspberry Pi) + +For machines with ≀ 4 GB RAM, the default CGAL build (PCH + -O3) OOMs +the compiler. `LOW_MEMORY_BUILD` uses `-O0`, disables PCH, and sets +unity batch size to 1, keeping `cc1plus` peak at ~150-200 MB per TU: + +```bash +cmake -S code -B build -DWITH_CGAL_TESTS=ON \ + -DCONFORMALLAB_LOW_MEMORY_BUILD=ON +cmake --build build --target conformallab_cgal_tests -j1 +``` + +Tests run ~15Γ— slower at -O0 but all pass. Useful for CI runners with +limited memory. +``` + +### Acceptance criteria +- [ ] `getting-started.md` documents `CONFORMALLAB_LOW_MEMORY_BUILD` +- [ ] Example command shows `-j1` (required for memory safety) + +--- + +## FINDING-U8 β€” 🟠 MISSING: `LOW_MEMORY_BUILD` absent from README compile-time table + +### Location +`README.md` lines 53–82 ("Compile-time workflow modes") + +### Problem + +The README code block shows five compile-time patterns but omits +`CONFORMALLAB_LOW_MEMORY_BUILD`. This is the flag that re-enabled CI +and is directly relevant to anyone building on resource-constrained hardware. + +### Fix + +Add to the README compile-time examples: +```bash +# Low-memory build: -O0, no PCH, unity batch 1 (~150 MB peak per cc1plus). +# Use on Raspberry Pi / CI runners with ≀ 4 GB RAM. Tests run ~15Γ— slower. +cmake -S code -B build -DWITH_CGAL_TESTS=ON -DCONFORMALLAB_LOW_MEMORY_BUILD=ON +cmake --build build --target conformallab_cgal_tests -j1 +``` + +### Acceptance criteria +- [ ] README shows `LOW_MEMORY_BUILD` usage with a one-line explanation + +--- + +## FINDING-U9 β€” 🟠 MISSING: `layout.uv` indexing semantics not documented + +### Location +`code/examples/example_layout.cpp` line 127 +`code/include/layout.hpp` β€” `Layout2D` struct definition + +### Problem + +```cpp +auto& p = layout.uv[v.idx()]; // example_layout.cpp:127 +``` + +`Layout2D.uv` is a `std::vector` indexed by the raw integer `.idx()` of +`Vertex_index`. This is undocumented: nowhere is it stated that the vector +length equals `num_vertices(mesh)` and that the index is contiguous. In CGAL, +`vertex.idx()` is contiguous on a fresh `Surface_mesh` but may have gaps after +vertex removals. + +The `halfedge_uv` field (also in `Layout2D`) has the same issue for halfedge +indices. + +### Fix + +In `layout.hpp`, add explicit documentation to the `Layout2D` struct: + +```cpp +struct Layout2D { + /// UV coordinate of vertex v in the layout plane. + /// Indexed by `v.idx()` β€” length equals `mesh.number_of_vertices()`. + /// \pre No vertices have been removed from `mesh` since construction + /// (i.e., `mesh.is_valid(false)` and no compaction was performed). + std::vector uv; + + /// UV of source(h) as seen from face(h), indexed by `h.idx()`. + /// At seam halfedges the two opposite halfedges carry different UV values, + /// enabling proper per-halfedge UV for GPU texture atlasing. + std::vector halfedge_uv; + ... +}; +``` + +Also update `example_layout.cpp` to add a comment: +```cpp +// layout.uv is indexed by v.idx() β€” valid as long as no vertices were +// removed from the mesh after loading. +auto& p = layout.uv[v.idx()]; +``` + +### Acceptance criteria +- [ ] `Layout2D.uv` and `Layout2D.halfedge_uv` have explicit Doxygen docs + stating the index semantics and the no-compaction precondition +- [ ] `example_layout.cpp` has an inline comment at the access site + +--- + +## Summary table + +| ID | File | Type | Severity | Status | +|----|------|------|----------|--------| +| U1 | README + 4 examples | Usability | πŸ”΄ Critical | 🟑 Open | +| U2 | examples/ (missing) | Usability | πŸ”΄ Critical | 🟑 Open | +| U3 | `contracts.md:16` | Doc error | 🟑 Medium | 🟑 Open | +| U4 | `README.md:17` | Stale | 🟑 Medium | 🟑 Open | +| U5 | `Discrete_conformal_map.h:14` | Stale | 🟑 Medium | 🟑 Open | +| U6 | README + 2 examples | Stale | 🟑 Medium | 🟑 Open | +| U7 | `getting-started.md` | Missing | 🟠 Minor | 🟑 Open | +| U8 | `README.md` | Missing | 🟠 Minor | 🟑 Open | +| U9 | `layout.hpp` + example | Missing | 🟠 Minor | 🟑 Open | + +**Priority order for fixing:** +1. U1 + U2 (new examples β€” most impactful for new users) +2. U3 (correctness β€” contracts.md wrong after Finding-B) +3. U4 + U5 + U6 (stale content β€” quick fixes) +4. U7 + U8 + U9 (missing docs β€” minor)