039cc26e36
Some checks failed
C++ Tests / test-fast (push) Successful in 1m58s
C++ Tests / test-fast (pull_request) Successful in 2m33s
API Docs / doc-build (pull_request) Successful in 51s
C++ Tests / test-cgal (push) Has been skipped
C++ Tests / test-cgal (pull_request) Failing after 10m32s
Adds `operator|` in `namespace CGAL` so package-local named parameters
can be combined left-to-right without modifying CGAL upstream:
auto p = CGAL::parameters::gradient_tolerance(1e-12)
| CGAL::parameters::max_iterations(500)
| CGAL::parameters::output_uv_map(uv);
CGAL::discrete_conformal_map_euclidean(mesh, p);
Why not the canonical `.a().b().c()` syntax
───────────────────────────────────────────
CGAL's standard chaining mechanism requires registering each named
parameter as a member function on `Named_function_parameters` via the
`CGAL_add_named_parameter` macro in
`CGAL/STL_Extension/internal/parameters_interface.h` — a vendored
upstream file that conformallab++ deliberately treats as read-only.
Adding member-function chainers for our package-local tags would
require either forking CGAL or modifying the vendored copy. Neither
is acceptable for a library that wants to remain portable across
future CGAL releases.
The pipe-operator achieves the same compositional semantics via a
free function in `namespace CGAL` (so ADL finds it for
`Named_function_parameters` operands). Implementation: rebuild the
right-hand-side `Named_function_parameters` with the left-hand-side
as its `Base`, producing an indistinguishable chain that every entry
function accepts unchanged.
Implementation: `code/include/CGAL/Conformal_map/internal/parameters.h`
lines 158-187. The operator is constrained to right-hand-sides with
`No_property` base (i.e. fresh single-parameter packs from the helper
functions), so it never collides with any future CGAL operator on the
same type.
Tests (2 new, total Phase-8b-Lite suite 15 → 17)
────────────────────────────────────────────────
* CGALPhase8bLite.NamedParamPipe_MultipleParamsTakeEffect
Chain three parameters; verify all three take effect (tight
tolerance respected + UV pmap populated + iteration cap honoured).
* CGALPhase8bLite.NamedParamPipe_TwoParams
Chain two parameters; verify max_iterations(0) blocks the loop
even when combined with another param.
Full CGAL suite: 234/234 PASSED, 0 SKIPPED (was 232).
Total: 257/257 PASSED, 0 SKIPPED (was 255).
scripts/check-test-counts.sh: OK.
Documentation updates
─────────────────────
* doc/tutorials/add-output-uv-map.md §3.4: "Current limitation: no
chaining" → "Chaining: use the pipe operator `|`". Explains why
CGAL's `.member()` syntax isn't available and shows the `|`
workaround with a working code example.
* doc/architecture/locked-vs-flexible.md §8: chaining now flagged as
shipped via pipe; recommended posture says `.member()` chaining
only if a user pushes for the CGAL-canonical syntax.
* doc/roadmap/porting-status.md §5: API limitations table updated.
* doc/api/tests.md: CGALPhase8bLite row 15 → 17, total 232 → 234.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
283 lines
14 KiB
Markdown
283 lines
14 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).
|
||
|
||
---
|
||
|
||
## 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?
|