f1d77aa293
Closes the gap "no code-quality / convention gate" from the structural
review. Three new artefacts, all local-only (CI promotion deferred
until the existing tree is 100 % clean under each):
1. .clang-format — project's existing style mechanically captured
(4-space indent, opening brace on new line for class/struct/function,
left-aligned pointer/reference modifiers, aligned `using = ...` blocks,
100-col loose limit, no include re-ordering — matches code/include/
today).
2. scripts/quality/clang-format.sh — drift detector. Dry-run mode by
default (always exits 0); --strict to fail on drift; --fix to apply
suggested changes in place. Skips code/deps/ and macOS-duplicate
files.
3. scripts/quality/cgal-conventions.py — checker for the CGAL idioms
that clang-format/clang-tidy cannot express:
CGAL-1 include-guard format `CGAL_<DIRS>_<FILE>_H`
CGAL-2 every public header has a `\\file` Doxygen brief
CGAL-3 no nested namespaces beyond the allowed set
(CGAL::parameters, CGAL::Conformal_map, internal_np, IO)
CGAL-4 named-parameter tag types end in `_t`; value object does not
CGAL-5 no `using namespace ...` at file scope (header leakage)
CGAL-6 no #define beyond CGAL_* / include-guard
Result on the current tree: 6 CGAL public headers, 0 violations.
The checker therefore doubles as documentation of the conventions
we already follow.
Both are wired into scripts/quality/run-all.sh's fast subset (~5 s
combined wall time). README.md updated to split the gates into a
"style/convention" group (cheap, run-on-every-commit material) and a
"correctness/quality" group (slow, run-before-tag material).
The reviewer-facing locked-vs-flexible.md gains another "✅ Closed"
row documenting both gates and the 0-violation baseline.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
310 lines
19 KiB
Markdown
310 lines
19 KiB
Markdown
# Locked-in vs flexible architecture decisions
|
||
|
||
> **Purpose.** External collaborators (especially mathematicians
|
||
> evaluating whether to extend the library) need to know which
|
||
> design choices are **load-bearing** (changing them is expensive
|
||
> across the whole codebase) and which are **opportunistic** (made
|
||
> on first principles and easy to revisit).
|
||
>
|
||
> **Why this matters now.** A v0.9.0 + Springborn-Bobenko-alumnus
|
||
> external review (May 2026) is the right moment to surface these
|
||
> decisions before they ossify further. If any of the *locked*
|
||
> decisions need revisiting, this is the cheapest moment in the
|
||
> project's life to do so.
|
||
|
||
Three tiers:
|
||
|
||
```
|
||
🔴 LOAD-BEARING changing requires repo-wide refactoring
|
||
🟡 SEMI-FIXED changing affects multiple subsystems; possible but not casual
|
||
🟢 OPPORTUNISTIC changing is one-PR work
|
||
```
|
||
|
||
---
|
||
|
||
## 1. Mesh data structure → `CGAL::Surface_mesh<P>`
|
||
|
||
| | |
|
||
|--|--|
|
||
| Status | 🔴 load-bearing |
|
||
| Locked since | Phase 3 (2025) |
|
||
| Alternative considered | OpenMesh / pmp-library / custom halfedge / Java `CoHDS` literal port |
|
||
| Why locked | Every header `code/include/*.hpp` uses `ConformalMesh = CGAL::Surface_mesh<Point3>` and CGAL property maps explicitly. Phase 8a Traits provide a *concept* abstraction but the Default model is `Surface_mesh`-only. |
|
||
| Cost to change | ~3 weeks: rewrite traits, every functional, every test mesh factory. Touched in ~30 headers + ~25 test files. |
|
||
| Mitigation | Traits-based design (Phase 8a) means user-supplied mesh types CAN be added as Default-traits specialisations without touching algorithms — Phase 8a.2 plan documents this. But the Default model is Surface_mesh. |
|
||
| When to revisit | If a major user wants Polyhedron_3 or OpenMesh as default. Currently no concrete request → keep. |
|
||
|
||
**Recommended posture for an external contributor:** use the existing
|
||
Surface_mesh-based API for any new functional. Adding generic-FaceGraph
|
||
support is a single architectural step that doesn't need to be repeated
|
||
per functional — wait for one user to need it.
|
||
|
||
---
|
||
|
||
## 2. Floating-point kernel → `CGAL::Simple_cartesian<double>`
|
||
|
||
| | |
|
||
|--|--|
|
||
| Status | 🟡 semi-fixed |
|
||
| Locked since | Phase 3 (deliberate decision: conformal geometry doesn't need exact predicates) |
|
||
| Cost to change | Per-functional template parameterisation. The Phase 8 MVP wrapper already deduces the kernel from the mesh point type, so user code can specify any CGAL kernel — but the **legacy** `code/include/*.hpp` headers are hardcoded. |
|
||
| When to revisit | If a user reports floating-point catastrophic cancellation in the half-tangent angle formula on extreme meshes. Hasn't happened in 250 tests over 9 phases. |
|
||
|
||
**Recommended posture:** stay with `Simple_cartesian<double>`. If a
|
||
specific algorithm needs `Exact_predicates_inexact_constructions_kernel`
|
||
for robustness, parameterise that one algorithm — don't refactor the
|
||
whole codebase.
|
||
|
||
---
|
||
|
||
## 3. Header-only, no compiled library
|
||
|
||
| | |
|
||
|--|--|
|
||
| Status | 🔴 load-bearing |
|
||
| Locked since | Phase 1 |
|
||
| Cost to change | Significant: would need to introduce `.cpp` files, link order, ABI compatibility decisions. But everything in `code/include/*.hpp` is `inline` or template, so a header-only-to-compiled migration is mechanical. |
|
||
| Why locked | CGAL package convention is also header-only. Forking would break that goal. |
|
||
| When to revisit | If compilation times become prohibitive (currently < 30 s clean build with all 5 functionals). Or if a future cyclic dependency between functionals forces it. Neither is on the horizon. |
|
||
|
||
**Recommended posture:** stay header-only. This is also the de-facto
|
||
norm for CGAL packages of comparable scope.
|
||
|
||
---
|
||
|
||
## 4. Five DCE models on the same mesh
|
||
|
||
| | |
|
||
|--|--|
|
||
| Status | 🟡 semi-fixed |
|
||
| Locked since | Phase 9a (2026-05) — when CP-Euclidean introduced face-DOFs |
|
||
| Why semi-fixed | Each model has its own `*Maps` bundle with its own property-map name prefix (`ev:`, `sv:`, `v:`, `cf:`/`ce:`, `iv:`/`ie:`) so all five can coexist on the same `CGAL::Surface_mesh`. Adding a sixth model means picking a new prefix + writing a new `*Maps` struct + Default trait. |
|
||
| Cost to add a sixth model | ~1 week (CP-Euclidean took ~3 days, Inversive-Distance ~3 days, Hessian + Newton + CGAL entry add another ~3 days). |
|
||
| When to revisit | If a unified base-Maps abstraction would actually win something (currently it would not — the property-map sets differ in *kind*, not just in name). |
|
||
|
||
**Recommended posture for adding a new functional:**
|
||
1. Pick a 2-letter prefix not in `{ev, sv, v, cf, ce, iv, ie}`.
|
||
2. Define your `*Maps` struct with that prefix.
|
||
3. Define your `Default_*_traits<Surface_mesh, K>` class in a new
|
||
header `code/include/CGAL/Discrete_*.h`.
|
||
4. Wire it into `newton_solver.hpp` (template-copy from
|
||
`newton_inversive_distance` is the closest pattern for vertex-DOFs;
|
||
`newton_cp_euclidean` for face-DOFs).
|
||
5. Add a CGAL entry in the new header.
|
||
6. Add tests following [`add-inversive-distance.md`](../tutorials/add-inversive-distance.md).
|
||
|
||
This recipe has been validated three times now (CP-Euclidean, Inversive
|
||
Distance, and the four Phase-8b-Lite wrappers).
|
||
|
||
---
|
||
|
||
## 5. Newton solver with line search + SparseQR fallback
|
||
|
||
| | |
|
||
|--|--|
|
||
| Status | 🟡 semi-fixed |
|
||
| Locked since | Phase 4 |
|
||
| Cost to change | Each `newton_*` function is ~50-80 lines; replacing the solver across all five is ~2 days. |
|
||
| When to revisit | If a future functional needs trust-region or BFGS. None of the five currently does — Newton converges quadratically near the optimum and the line search handles bad initial points. |
|
||
|
||
**Recommended posture:** Newton with line search is enough for any
|
||
strictly-convex variational problem. For non-convex variants, consider
|
||
adding a `newton_with_trust_region()` helper alongside, not replacing.
|
||
|
||
---
|
||
|
||
## 6. Eigen as the linear-algebra back-end
|
||
|
||
| | |
|
||
|--|--|
|
||
| Status | 🔴 load-bearing |
|
||
| Locked since | Phase 4 |
|
||
| Alternative considered | PETSc/Tao (Java original) / Boost.uBLAS / Blaze |
|
||
| Why locked | Eigen is header-only (no external dependency at build time), bundled as a CGAL dependency, fast, and offers `SimplicialLDLT + SparseQR` which the gauge-singular-mesh case needs. |
|
||
| Cost to change | Significant — every Hessian header (`*_hessian.hpp`) and every Newton solver uses `Eigen::SparseMatrix` and `Eigen::VectorXd` directly. ~2 weeks repo-wide. |
|
||
| When to revisit | If a sparse-solver feature (e.g. parallel Cholesky) is needed that Eigen doesn't offer. |
|
||
|
||
**Recommended posture:** stay with Eigen.
|
||
|
||
---
|
||
|
||
## 7. CGAL public-API surface layout
|
||
|
||
| | |
|
||
|--|--|
|
||
| Status | 🟡 semi-fixed (Phase 8a-MVP design decision, 2026-05-19) |
|
||
| Locked since | PR #6 (v0.9.0) |
|
||
| Strategy chosen | "Strategy C" — functional-specific Default traits, one entry function per functional, no fat unified trait. |
|
||
| Cost to change to unified trait | ~1 week — refactor `Default_*_traits<>` into a single `Default_conformal_map_traits<>` with all property-map fields. Existing 8 tests would need updating. |
|
||
| When to revisit | When the first cross-functional algorithm (e.g. a hybrid functional that uses both face and vertex DOFs) lands. Speculation today. |
|
||
|
||
**Recommended posture:** stay with Strategy C. CGAL's own
|
||
`Polygon_mesh_processing` package follows the same convention — one
|
||
default trait per algorithm family.
|
||
|
||
---
|
||
|
||
## 8. Named-parameter mechanism
|
||
|
||
| | |
|
||
|--|--|
|
||
| Status | 🟢 opportunistic |
|
||
| Locked since | Phase 8 MVP (2026-05-19) |
|
||
| Current state | Six tags: `vertex_curvature_map`, `fixed_vertex_map`, `gradient_tolerance`, `max_iterations`, `output_uv_map`, `normalise_layout`. Pipe-operator `|` chaining shipped (in lieu of `.member()` chaining which would require CGAL upstream modifications). |
|
||
| Cost to extend with `.member()` chaining | ~2 days IF CGAL upstream is forked / patched; otherwise the pipe-operator workaround is the maintainable path. |
|
||
| When to revisit | At any time; this is the lowest-risk change in the codebase. Tutorials [`add-output-uv-map.md`](../tutorials/add-output-uv-map.md) §4 explains the mechanism. |
|
||
|
||
**Recommended posture:** the pipe-operator `|` is already shipped and
|
||
sufficient. Add `.member()` chaining only if a concrete user pushes for
|
||
the CGAL-canonical syntax AND we are willing to fork CGAL upstream.
|
||
|
||
---
|
||
|
||
## 9. Property-map name conventions
|
||
|
||
| | |
|
||
|--|--|
|
||
| Status | 🟢 opportunistic |
|
||
| Locked since | Phase 3 + 9a (prefix `ev:`/`sv:`/`v:`/`cf:`/`ce:`/`iv:`/`ie:` set when each functional was introduced) |
|
||
| Cost to change | One sed-replace + recompile. No user-visible effect because the names are an *internal* convention; the CGAL public API never exposes them. |
|
||
| When to revisit | If a future functional reuses an existing letter prefix. Already discussed in [`locked-vs-flexible.md`](#4-five-dce-models-on-the-same-mesh). |
|
||
|
||
---
|
||
|
||
## 10. Tests: GTest, not CGAL's own test format
|
||
|
||
| | |
|
||
|--|--|
|
||
| Status | 🟡 semi-fixed |
|
||
| Locked since | Phase 1 |
|
||
| Cost to change | ~1 week — rewrite test harnesses to CGAL's `test/Conformal_map/` convention. This is Phase 8d (planned for CGAL submission). |
|
||
| Why GTest now | Faster development cycle, IDE-friendly (Xcode / VSCode / CLion all have native GTest support). No CGAL submission is in progress yet. |
|
||
| When to revisit | When committing to CGAL submission (Phase 8c-d, decided to be a future commitment, see [`release-policy.md`](../release-policy.md)). |
|
||
|
||
**Recommended posture:** keep GTest as primary. When/if CGAL submission
|
||
happens, add a `test/Conformal_map/` shim that calls into the GTest
|
||
suite — both formats can coexist.
|
||
|
||
---
|
||
|
||
## 11. License: MIT
|
||
|
||
| | |
|
||
|--|--|
|
||
| Status | 🔴 load-bearing |
|
||
| Locked since | Project inception |
|
||
| Cost to change | High organisational cost (requires consent of all contributors); ~no code cost. |
|
||
| Why locked | MIT was chosen for academic friendliness (citing, modifying, embedding). CGAL upstream requires LGPL for submitted packages. |
|
||
| Trade-off | Submitting to CGAL upstream is not possible without re-licensing. The codebase architecture is "CGAL-style" but the project would publish independently. |
|
||
| When to revisit | If/when a concrete CGAL upstream submission is decided. See [`release-policy.md`](../release-policy.md) for the formal policy. |
|
||
|
||
**Recommended posture:** stay with MIT. Build the "CGAL-style package
|
||
for external distribution" as the primary deliverable. Re-license only
|
||
when the CGAL editorial board commits to accepting the submission.
|
||
|
||
---
|
||
|
||
## 12. Documentation pattern: Markdown + Doxygen
|
||
|
||
| | |
|
||
|--|--|
|
||
| Status | 🟢 opportunistic |
|
||
| Locked since | Phase 7.5 (2026-05) |
|
||
| Current state | `code/include/*.hpp` carry Doxygen-style `///` comments (87% coverage); `doc/*.md` for prose; `Doxyfile` generates HTML in `doc/doxygen/`. |
|
||
| Cost to add more | Per-file basis; ~1 hour per header for full Doxygen. |
|
||
| When to revisit | When chasing CGAL-submission readiness (need `PackageDescription.txt` + `User_manual.md`). |
|
||
|
||
**Recommended posture:** keep adding `///` comments incrementally with
|
||
each new public function.
|
||
|
||
---
|
||
|
||
## Summary table
|
||
|
||
| Decision | Tier | Cost to change |
|
||
|-----------------------------------------|----------------|----------------------|
|
||
| 1. CGAL::Surface_mesh as default mesh | 🔴 load-bearing | ~3 weeks |
|
||
| 2. Simple_cartesian<double> kernel | 🟡 semi-fixed | per-functional |
|
||
| 3. Header-only architecture | 🔴 load-bearing | medium (mechanical) |
|
||
| 4. Five DCE models, separate Maps | 🟡 semi-fixed | ~1 week per new model |
|
||
| 5. Newton + line search + SparseQR | 🟡 semi-fixed | ~2 days |
|
||
| 6. Eigen back-end | 🔴 load-bearing | ~2 weeks |
|
||
| 7. Strategy C (per-functional traits) | 🟡 semi-fixed | ~1 week |
|
||
| 8. Named-parameter mechanism | 🟢 opportunistic | pipe ✅; .member() ~2 days |
|
||
| 9. Property-map name conventions | 🟢 opportunistic | ~1 hour |
|
||
| 10. GTest, not CGAL test format | 🟡 semi-fixed | ~1 week |
|
||
| 11. MIT license | 🔴 load-bearing | organisational |
|
||
| 12. Markdown + Doxygen | 🟢 opportunistic | per-file |
|
||
|
||
**Key insight:** the **load-bearing decisions are all good in 2026**.
|
||
Surface_mesh + Eigen + header-only + MIT are the right defaults for a
|
||
research-quality CGAL-style package. The **semi-fixed decisions are
|
||
all behind one concrete blocker** (single user request, CGAL submission
|
||
commitment, etc.). The **opportunistic decisions are cheap to revisit
|
||
any time**.
|
||
|
||
The architecture is in a good place for the v0.9.0 → v0.10.0 transition.
|
||
No "expensive corner" has been painted into; every locked decision
|
||
matches the project's three-goal hierarchy in
|
||
[`research-track.md`](../roadmap/research-track.md).
|
||
|
||
---
|
||
|
||
## Known limitations (state at the time of the reviewer meeting)
|
||
|
||
These are deliberate, honestly-flagged gaps in the v0.9.0 snapshot the
|
||
reviewer will see. None of them are load-bearing — each is a small,
|
||
mechanical next step rather than a missing piece of theory.
|
||
|
||
| Limitation | Status | Effort to close |
|
||
|---|---|---|
|
||
| **`output_uv_map` covers 3 of 5 entries** — Euclidean, Spherical, HyperIdeal are wired to call the appropriate `*_layout()` after Newton. CP-Euclidean (face-based) and Inversive-Distance (vertex-based) entries still require the user to call `*_layout()` by hand. | tutorial + plumbing in place; just needs to be copy-pasted into the two remaining entry headers | ~0.5 day |
|
||
| **Named-parameter chaining: `\|`-operator only, no `.a().b().c()`.** Member-style chaining would need a patch to CGAL's upstream `parameters_interface.h`, which we treat as a read-only vendored dependency. The pipe operator is documented, ADL-discoverable, and equivalent in expressive power. | pipe shipped, member-chain deferred until upstream extension point exists | ~2 days (only if upstream PR is accepted) |
|
||
| **Phase 9b-analytic: derivation complete, code uses block-FD.** The Schläfli-based analytic HyperIdeal Hessian is fully derived in [`hyperideal-hessian-derivation.md`](../math/hyperideal-hessian-derivation.md) (805 lines, all sign pitfalls covered). The shipped code still uses per-face block-FD (already 96× faster than the legacy full-FD path). | research-ready writeup; implementation gated on the reviewer's view of whether the additional ~6× is worth it | ~2 weeks |
|
||
| **Doxygen `WARN_IF_UNDOCUMENTED = NO`.** With `EXTRACT_ALL = YES`, every symbol is in the generated HTML — live at <https://tmoussa.codeberg.page/ConformalLabpp/> (auto-published from `main` by `.gitea/workflows/doxygen-pages.yml`) — but symbols without explicit doc comments show only their signature. Public API surface (entry functions, named-parameter helpers, traits typedefs) has hand-written Doxygen; internal helpers vary. | clean (0 warnings) under current policy; not yet enforced "no undocumented symbol"; pursued on a separate branch | ~3 days to drive `WARN_IF_UNDOCUMENTED = YES` to zero |
|
||
| **`check-test-counts.sh` not wired into CI.** ✅ Closed by branch `ci/structural-tests`. Step is now part of `.gitea/workflows/cpp-tests.yml` (re-uses the just-built `build/` dir; ~5 s overhead). | gate active on every PR | done |
|
||
| **End-to-end smoke (`try_it.sh`) not in CI.** ✅ Closed by `ci/structural-tests`. Added as a step after the CGAL job. | gate active on every PR | done |
|
||
| **Internal markdown link checker.** ✅ Closed by `ci/structural-tests`. New `.gitea/workflows/markdown-links.yml` runs on every PR that touches a `*.md` file, plus a weekly cron for external link rot. | gate active on every PR + weekly | done |
|
||
| **Local quality gates (sanitizers, coverage, clang-tidy, multi-compiler, CGAL-version-matrix, reproducible-build, license-headers).** Eight scripts under `scripts/quality/`, driven by `run-all.sh`. Documented in `scripts/quality/README.md` with promotion-to-CI checklist. | local-only by design; promotion gated on policy text in release-policy.md | done as scripts; 60 SPDX headers missing in `code/include/` will be a follow-up |
|
||
| **Code-style / convention gates (clang-format + CGAL-conventions checker).** ✅ Closed by `ci/structural-tests`. Adds `.clang-format` (project style mechanically captured) + `clang-format.sh` (drift detector, `--fix` mode); `cgal-conventions.py` enforces 6 CGAL-specific idioms (include-guard format, `\file` brief, namespace nesting, named-parameter tag `_t` suffix, no `using namespace`, no stray `#define`). Both are intentionally local-only — promotion to CI once the existing tree passes `--strict` (today CGAL-conventions does pass: 0/6 violations across 6 CGAL public headers; clang-format drift TBD pending toolchain install). | done as scripts; promotion deferred | done |
|
||
| **CP-Euclidean and Inversive-Distance research-track entries.** Both ship a working DCE solver, but lack the auxiliary utilities the Euclidean / HyperIdeal entries have (curvature inspection helpers, edge-flip Delaunay maintenance for ID). Out of port scope; listed in [`research-track.md`](../roadmap/research-track.md). | research-track, not blocking | per-utility |
|
||
| **`StereographicUnwrapper`, `CircleDomainUnwrapper`, `CuttingUtility`, `KoebePolyhedron`.** Mentioned in roadmap + research-track docs but not yet ported. Java versions still authoritative. | documented as Phase 11+ / optional | weeks each — explicit "out of port scope unless requested" |
|
||
|
||
The honest framing for the meeting: **the porting layer hits its target
|
||
for the 5 Phase-8b-Lite DCE entries; the hackability layer is
|
||
demonstrably in place (3 tutorials, named-parameter chaining,
|
||
Doxygen-HTML); the research-track items are scoped but not built.**
|
||
|
||
---
|
||
|
||
## Open questions for the external reviewer
|
||
|
||
Items where the project would benefit from a second opinion:
|
||
|
||
1. **Phase 9c (4g-polygon) algorithm choice.** Two routes:
|
||
* Port the Java `FundamentalPolygonUtility` + `CanonicalFormUtility`
|
||
literally (~2 weeks).
|
||
* Or: re-derive from Springborn 2020 §5 using the existing
|
||
`cut_graph.hpp` + holonomy infrastructure (~3 weeks, cleaner
|
||
architecture).
|
||
Which is preferred? See [`phases.md`](../roadmap/phases.md) §Phase 9c.
|
||
|
||
2. **Phase 10a (forms) priorities.** Three sub-items
|
||
(`DiscreteHarmonicFormUtility`, `DiscreteHolomorphicFormUtility`,
|
||
`CanonicalBasisUtility`) interlock. Which to start with?
|
||
|
||
3. **Analytic Hessian payoff.** The Schläfli-based analytic HyperIdeal
|
||
Hessian (Phase 9b-analytic — derivation already written:
|
||
[`hyperideal-hessian-derivation.md`](../math/hyperideal-hessian-derivation.md))
|
||
would add another ~6× over block-FD. Is that worth ~2 weeks of
|
||
implementation effort for a working-mesh size on which?
|
||
|
||
4. **CGAL upstream vs independent distribution.** Does the reviewer
|
||
know a CGAL editor / has personal opinion on the LGPL-vs-MIT
|
||
trade-off?
|
||
|
||
5. **geometry-central cross-validation (GC-1).** Two libraries solve
|
||
the same DCE problem from different algorithmic directions
|
||
(Newton-on-mesh vs Ptolemaic-flips-on-intrinsic-triangulation). An
|
||
independent comparison would be a nice paper. Interested?
|