Files
ConformalLabpp/doc/reviewer/usability-audit-2026-05-31.md
Tarik Moussa 02a3745b67 examples: add real conformal-flattening + CGAL-API examples (U1+U2)
Finding-U1 and Finding-U2 from doc/reviewer/usability-audit-2026-05-31.md.

The existing examples (example_euclidean, example_layout, example_hyper_ideal)
all used the "natural theta" pattern which makes x*=0 trivially the
equilibrium — u_v ≈ 0 everywhere, no deformation. A new user following
these examples saw solver output but not conformal geometry.

New: example_flatten.cpp
  - PRIMARY USE CASE: conformally flatten a mesh to the plane
  - Sets Θ_v = 2π for all interior vertices (flat target)
  - Pins boundary vertices (no Gauss-Bonnet check for open meshes)
  - Demonstrates non-trivial u_v (cathead.obj: range ≈ 2.96, 5 Newton iters)
  - Documents the difference from "natural theta" explicitly

New: example_cgal_api.cpp
  - Demonstrates CGAL::discrete_conformal_map_euclidean (Discrete_conformal_map.h)
  - First runnable CGAL public API example; contrast with internal API
  - Documents the "natural theta" default behaviour and explains why u_v=0
  - Explains when to use CGAL API vs internal API

Both examples registered in code/examples/CMakeLists.txt and compile
cleanly with -DWITH_CGAL=ON.

Updated:
  - example_euclidean.cpp: prominent "TESTING CONVENTION" warning
  - example_layout.cpp: same warning on set_natural_theta helper
  - doc/getting-started.md: example_flatten is now the recommended
    "start here" example; note on natural-theta behaviour added

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

456 lines
16 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
---
## 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 | 🟡 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)