Files
ConformalLabpp/doc/reviewer/usability-audit-2026-05-31.md
Tarik Moussa 37b538aae4
Some checks failed
C++ Tests / test-fast (push) Has been cancelled
C++ Tests / test-cgal (push) Has been cancelled
C++ Tests / quality-gates (push) Has been cancelled
API Docs / doc-build (push) Has been cancelled
Markdown link check / check (push) Has been cancelled
C++ Tests / test-fast (pull_request) Successful in 2m10s
C++ Tests / quality-gates (pull_request) Has been skipped
C++ Tests / test-cgal (pull_request) Has been skipped
docs: Layout2D index semantics, CLI table, CLI roadmap (U9+U10+U11)
U9 (layout.hpp + example_layout.cpp)
  Layout2D.uv and .halfedge_uv now have explicit Doxygen docs stating:
    - indexing: v.idx() / h.idx() (raw integer index)
    - length: mesh.number_of_vertices() / number_of_halfedges()
    - precondition: no vertex removal / collect_garbage() after loading
    - access pattern example in the doc comment
  example_layout.cpp: access site comment + static_cast<size_t>(v.idx())

U10 (getting-started.md)
  New 'CLI parameter reference' table (7 rows) added directly below the
  CLI usage examples; cross-references --help as the canonical source

U11 (doc/roadmap/phases.md)
  New Phase 9h 'CLI usability extensions' section inserted before Phase 10:
    9h.1  --tol / --max-iter solver-tuning params  (~30 min, no deps)
    9h.2  -g cp_euclidean / -g inversive_distance  (~2-4 h, needs 9a )
  Each sub-task has effort estimate, implementation sketch, and
  acceptance criteria so a future session can pick it up cold.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-05-31 01:51:04 +02:00

557 lines
20 KiB
Markdown
Raw Blame History

This file contains ambiguous Unicode characters

This file contains Unicode characters that might be confused with other characters. If you think that this is intentional, you can safely ignore this warning. Use the Escape button to reveal them.

# 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 100121 ("Minimal usage")
- `code/examples/example_euclidean.cpp` lines 7283 (Step 4)
- `code/examples/example_layout.cpp` lines 4049 (`set_natural_theta`)
- `code/examples/example_hyper_ideal.cpp` lines 8998 (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<double>`
- CGAL: `Conformal_map_result.u_per_vertex` — vertex-index-indexed `std::vector<FT>`
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 <CGAL/Simple_cartesian.h>
#include <CGAL/Surface_mesh.h>
#include <CGAL/Discrete_conformal_map.h>
// ...
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 δ = (lhsrhs)/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 19a 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 19b complete. Newton solvers for five DCE
models (Euclidean / Spherical / HyperIdeal / CP-Euclidean / Inversive-Distance),
priority-BFS layout in ℝ²/S²/Poincaré disk, GaussBonnet, 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 1416
### 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 107110
- `code/examples/example_euclidean.cpp` lines 6268
- `code/examples/example_layout.cpp` lines 5259 (`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 5382 ("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<Eigen::Vector2d> 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<Eigen::Vector2d> 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
---
## FINDING-U10 — 🟠 MISSING: CLI parameter reference not in README or getting-started
### Location
- `README.md` (has CLI examples but no parameter table)
- `doc/getting-started.md` (same)
- `code/src/apps/v0/conformallab_cli.cpp` lines 333340 (source of truth)
### Problem
The CLI app (`conformallab_core`) is the primary non-programmatic entry point
for the library. The README shows a few usage examples but no parameter
reference. A user running `--help` gets CLI11's auto-generated output, but
that output is not reproduced anywhere in the documentation.
Current parameters (from `conformallab_cli.cpp`):
| Flag | Default | Description |
|------|---------|-------------|
| `-i / --input` | *required* | Input mesh (OFF / OBJ / PLY) |
| `-o / --output` | *(none)* | Output layout OFF file |
| `-j / --json` | *(none)* | Serialise result to JSON |
| `-x / --xml` | *(none)* | Serialise result to XML |
| `-g / --geometry` | `euclidean` | Target geometry: `euclidean` \| `spherical` \| `hyper_ideal` |
| `-s / --show` | `false` | Open input mesh in interactive viewer |
| `-v / --verbose` | `false` | Print topology / DOF stats |
### Fix
Add a concise parameter table to `doc/getting-started.md` in the
"First run — CLI app" section. The README already shows the most common
invocations; the table can live in getting-started to avoid README bloat.
### Acceptance criteria
- [ ] `doc/getting-started.md` has a parameter table matching the source
- [ ] Each flag has a one-line description and notes the default
- [ ] Cross-reference to `--help` for the canonical up-to-date list
---
## FINDING-U11 — 🟠 ROADMAP: CLI missing three useful parameters + two models
### Location
`code/src/apps/v0/conformallab_cli.cpp`
### Problem (not a bug — a gap for a future session to fill)
**Missing solver-tuning parameters** (low effort, high value):
The Newton tolerance and iteration limit are hardcoded at their defaults
(`tol=1e-8`, `max_iter=200`). Any user who wants to solve a stiff mesh
more carefully or stop early has to recompile.
```bash
# Not yet possible:
./conformallab_core -i hard_mesh.off -g euclidean --tol 1e-12 --max-iter 1000
./conformallab_core -i quick_check.off --tol 1e-4 --max-iter 20
```
**Missing geometry modes** (medium effort):
The two Phase-9a functionals are not reachable from the CLI at all:
```bash
# Not yet possible — needs run_cp_euclidean() + run_inversive_distance() helpers:
./conformallab_core -i mesh.off -g cp_euclidean
./conformallab_core -i mesh.off -g inversive_distance
```
CP-Euclidean (`Discrete_circle_packing.h`) and Inversive-Distance
(`Discrete_inversive_distance.h`) are fully implemented in the library and
exposed via the CGAL public API, but a CLI user cannot reach them.
### Suggested implementation (for a future session)
1. **`--tol <double>` and `--max-iter <int>`:** one `app.add_option` each,
thread through all three `run_*` functions. ~15 lines.
2. **`-g cp_euclidean` and `-g inversive_distance`:** add two new
`run_cp_euclidean()` / `run_inversive_distance()` helpers following
the existing `run_euclidean()` pattern. ~60 lines each. Add to the
`-g` `CLI::IsMember` list. Register the two new geometry strings in
the dispatch block.
3. **Update README + getting-started** to list all five geometry modes.
### Effort estimate
- `--tol` + `--max-iter`: ~30 min
- Phase-9a CLI exposure: ~2 hours (mostly copy-paste + adaptation)
- Tests: add two CLI smoke tests (geometry converges on tetrahedron)
### Acceptance criteria (when implemented)
- [ ] `--tol` and `--max-iter` are accepted and forwarded to all three/five solvers
- [ ] `./conformallab_core -i mesh.off -g cp_euclidean -o out.off` works
- [ ] `./conformallab_core -i mesh.off -g inversive_distance -o out.off` works
- [ ] `--help` output lists all five geometry modes
---
## Summary table
| ID | File | Type | Severity | Status |
|----|------|------|----------|--------|
| U1 | README + 4 examples | Usability | 🔴 Critical | ✅ Fixed 2026-05-31 |
| U2 | examples/ (missing) | Usability | 🔴 Critical | ✅ Fixed 2026-05-31 |
| U3 | `contracts.md:16` | Doc error | 🟡 Medium | ✅ Fixed 2026-05-31 |
| U4 | `README.md:17` | Stale | 🟡 Medium | ✅ Fixed 2026-05-31 |
| U5 | `Discrete_conformal_map.h:14` | Stale | 🟡 Medium | ✅ Fixed 2026-05-31 |
| U6 | README + 2 examples | Stale | 🟡 Medium | ✅ Fixed 2026-05-31 |
| U7 | `getting-started.md` | Missing | 🟠 Minor | ✅ Fixed 2026-05-31 |
| U8 | `README.md` | Missing | 🟠 Minor | ✅ Fixed 2026-05-31 |
| U9 | `layout.hpp` + example | Missing | 🟠 Minor | ✅ Fixed 2026-05-31 |
| U10 | `getting-started.md` | Missing | 🟠 Minor | ✅ Fixed 2026-05-31 |
| U11 | `conformallab_cli.cpp` | Roadmap | 🟠 Minor | ✅ Added to phases.md (9h) |
**Priority order for fixing:**
1. ~~U1 + U2~~ ✅ done
2. U3 (correctness — contracts.md wrong after Finding-B)
3. U4 + U5 + U6 (stale content — quick fixes)
4. U7 + U8 + U9 + U10 (missing docs — minor)
5. U11 (roadmap — CLI extensions, not urgent)