From aefbf4a111a033631531ff01b606284d0f061607 Mon Sep 17 00:00:00 2001 From: Seth Parker Date: Sat, 20 Jun 2026 10:13:44 -0400 Subject: [PATCH 01/20] test(packing): add F2 PackCharts suite (TDD red) + header scaffold Add TestChartPacking.cpp covering edge cases, absolute/normalize scaling, non-overlap, extent, padding, and an end-to-end tear/extract/flatten/pack test with per-wedge recovery via the back-maps. ChartPacking.hpp provides PackOptions/PackResult and a stubbed PackCharts so the suite compiles and fails (red); implementation follows in Phase 3. Co-Authored-By: Claude Opus 4.8 --- conductor/tracks/F2/plan.md | 14 +- include/OpenABF/ChartPacking.hpp | 144 ++++++++++++++++ tests/CMakeLists.txt | 1 + tests/src/TestChartPacking.cpp | 288 +++++++++++++++++++++++++++++++ 4 files changed, 440 insertions(+), 7 deletions(-) create mode 100644 include/OpenABF/ChartPacking.hpp create mode 100644 tests/src/TestChartPacking.cpp diff --git a/conductor/tracks/F2/plan.md b/conductor/tracks/F2/plan.md index 011c8d5..9c15e78 100644 --- a/conductor/tracks/F2/plan.md +++ b/conductor/tracks/F2/plan.md @@ -11,13 +11,13 @@ Design resolved 2026-06-20 (see spec.md → Design Decisions). - [x] 1.6 Resolve edge cases: empty list no-op, zero-area placed, null/empty throws, Dim>=2 ## Phase 2: Tests (write first) -- [ ] 2.1 Synthetic 2D charts: bbox computation correctness (min/max per mesh) -- [ ] 2.2 Assert packed chart bounding boxes do not overlap (padding respected) -- [ ] 2.3 Assert absolute-mode preserves relative chart sizes (no per-chart distortion) -- [ ] 2.4 Assert normalize=true fits all UVs within [0,1]² via single global scale -- [ ] 2.5 Assert returned PackResult extent bounds all packed charts -- [ ] 2.6 Degenerate cases: empty list, single chart, zero-area chart, null/empty throw -- [ ] 2.7 End-to-end: tear → extract_connected_components → LSCM → PackCharts, and +- [x] 2.1 Synthetic 2D charts: bbox computation correctness (min/max per mesh) +- [x] 2.2 Assert packed chart bounding boxes do not overlap (padding respected) +- [x] 2.3 Assert absolute-mode preserves relative chart sizes (no per-chart distortion) +- [x] 2.4 Assert normalize=true fits all UVs within [0,1]² via single global scale +- [x] 2.5 Assert returned PackResult extent bounds all packed charts +- [x] 2.6 Degenerate cases: empty list, single chart, zero-area chart, null/empty throw +- [x] 2.7 End-to-end: tear → extract_connected_components → LSCM → PackCharts, and verify per-wedge recovery via (face_map[f], vertex_map[corner.vertex.idx]) ## Phase 3: Implementation diff --git a/include/OpenABF/ChartPacking.hpp b/include/OpenABF/ChartPacking.hpp new file mode 100644 index 0000000..641b924 --- /dev/null +++ b/include/OpenABF/ChartPacking.hpp @@ -0,0 +1,144 @@ +/* +OpenABF +https://gitlab.com/educelab/OpenABF + +Copyright 2025 EduceLab + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ +#pragma once + +#include +#include +#include +#include + +#include "OpenABF/Vec.hpp" + +namespace OpenABF +{ + +namespace detail +{ +/** @brief Deduce the dimensionality of a Vec type */ +template +struct VecDimensions; + +template +struct VecDimensions> { + static constexpr std::size_t value = N; +}; +} // namespace detail + +/** + * @brief Options controlling PackCharts behavior + * + * @tparam T Floating-point scalar type + */ +template +struct PackOptions { + /** + * @brief Fit the packed atlas into the unit square `[0,1]^2` + * + * When `false` (default), charts keep their absolute (physical) scale and + * are only translated. When `true`, a single global uniform scale is + * applied after layout so the whole atlas fits in `[0,1]^2`. A single + * global factor preserves relative chart sizes and cross-chart texel + * density; only the absolute units change. + */ + bool normalize{false}; + + /** + * @brief Target shelf width; overrides the `sqrt(total area)` heuristic + * + * Charts wrap to a new shelf when a row would exceed this width. If unset, + * the width defaults to `sqrt(sum of per-chart bounding-box areas)`, which + * yields a roughly square atlas. + */ + std::optional target_width{}; + + /** @brief Gutter added around each chart, in chart/absolute units */ + T padding{T(0)}; +}; + +/** + * @brief The bounding box of the packed atlas + * + * @tparam T Floating-point scalar type + */ +template +struct PackResult { + /** @brief Lower corner of the packed atlas */ + Vec min; + /** @brief Upper corner of the packed atlas */ + Vec max; +}; + +/** + * @brief Pack a set of parameterized charts into a shared coordinate frame + * + * Lays out a list of already-parameterized charts (2D meshes whose vertex + * `pos` holds `{u, v, ...}`) into a single shared frame using shelf packing, + * so that no two charts' bounding boxes overlap. Operates purely on geometry: + * each chart's vertex positions are translated (and, when `normalize` is set, + * uniformly scaled) **in place**. Topology, vertex indices, and face indices + * are untouched, so any `ExtractedComponent` back-maps a caller holds remain + * valid after packing. + * + * @par Scaling + * By default charts keep their absolute scale and are only translated; the + * returned extent is meaningful in physical units. With `opts.normalize`, one + * global uniform scale maps the packed atlas into `[0,1]^2`. + * + * @par Building a per-wedge UV map + * This function does not own a UV-map type. To build a per-corner ("wedge") + * UV map from the packed charts, key each wedge by **vertex identity**, not by + * corner position: a face's corner order is not stable (the half-edge mesh may + * reverse a mis-wound face at insertion time, and that permutation is not + * recorded). Given an `ExtractedComponent` `ec` for a chart, the robust key is: + * @code + * for (const auto& face : chart->faces()) { + * for (const auto& edge : *face) { // walk the face's corners + * auto origFace = ec.face_map[face->idx]; + * auto origVert = ec.vertex_map[edge->vertex->idx]; + * auto uv = edge->vertex->pos; // packed UV + * // wedge (origFace, origVert) -> uv + * } + * } + * @endcode + * + * @tparam MeshType A HalfEdgeMesh specialization + * @param charts Charts to pack; each chart's vertex positions are modified + * @param opts Packing options + * @return The bounding box of the packed atlas + * + * @throws std::invalid_argument If a chart pointer is null or a chart has no + * vertices. + */ +template +auto PackCharts(std::vector& charts, + const PackOptions& opts = + PackOptions{}) -> PackResult +{ + using T = typename MeshType::type; + static_assert( + detail::VecDimensions().pos)>::value >= 2, + "PackCharts requires mesh vertices with at least 2 position dimensions"); + + // STUB (TDD red phase): real layout is implemented in Phase 3. + (void)charts; + (void)opts; + return PackResult{Vec{T(0), T(0)}, Vec{T(0), T(0)}}; +} + +} // namespace OpenABF diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index 9287893..e381070 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -16,6 +16,7 @@ set(tests src/TestParameterization.cpp src/TestMeshIOUtils.cpp src/TestMeshIO.cpp + src/TestChartPacking.cpp ) foreach(src ${tests}) diff --git a/tests/src/TestChartPacking.cpp b/tests/src/TestChartPacking.cpp new file mode 100644 index 0000000..d9d3c43 --- /dev/null +++ b/tests/src/TestChartPacking.cpp @@ -0,0 +1,288 @@ +#include +#include +#include +#include +#include + +#include + +#include "OpenABF/ChartPacking.hpp" +#include "OpenABF/OpenABF.hpp" + +using Mesh = OpenABF::HalfEdgeMesh; +using OpenABF::PackCharts; +using OpenABF::PackOptions; +using OpenABF::PackResult; + +namespace +{ + +/** @brief Build a w x h rectangle chart (two triangles) at the origin */ +auto MakeRectChart(float w, float h) -> Mesh::Pointer +{ + auto m = Mesh::New(); + m->insert_vertices({{0.f, 0.f, 0.f}, {w, 0.f, 0.f}, {w, h, 0.f}, {0.f, h, 0.f}}); + m->insert_faces({{0, 1, 2}, {0, 2, 3}}); + return m; +} + +struct BBox { + float minx{std::numeric_limits::max()}; + float miny{std::numeric_limits::max()}; + float maxx{std::numeric_limits::lowest()}; + float maxy{std::numeric_limits::lowest()}; + [[nodiscard]] auto width() const -> float { return maxx - minx; } + [[nodiscard]] auto height() const -> float { return maxy - miny; } +}; + +template +auto ChartBBox(const MeshPtr& m) -> BBox +{ + BBox b; + for (const auto& v : m->vertices()) { + b.minx = std::min(b.minx, v->pos[0]); + b.miny = std::min(b.miny, v->pos[1]); + b.maxx = std::max(b.maxx, v->pos[0]); + b.maxy = std::max(b.maxy, v->pos[1]); + } + return b; +} + +/** @brief True if two boxes overlap by more than eps (touching is allowed) */ +auto Overlaps(const BBox& a, const BBox& b, float eps) -> bool +{ + return (a.minx + eps < b.maxx) && (b.minx + eps < a.maxx) && (a.miny + eps < b.maxy) && + (b.miny + eps < a.maxy); +} + +} // namespace + +// --- Edge cases ------------------------------------------------------------ + +TEST(ChartPacking, EmptyListReturnsEmptyExtent) +{ + std::vector charts; + auto extent = PackCharts(charts); + EXPECT_FLOAT_EQ(extent.min[0], 0.f); + EXPECT_FLOAT_EQ(extent.min[1], 0.f); + EXPECT_FLOAT_EQ(extent.max[0], 0.f); + EXPECT_FLOAT_EQ(extent.max[1], 0.f); +} + +TEST(ChartPacking, NullChartThrows) +{ + std::vector charts{MakeRectChart(1.f, 1.f), nullptr}; + EXPECT_THROW(PackCharts(charts), std::invalid_argument); +} + +TEST(ChartPacking, EmptyChartThrows) +{ + std::vector charts{MakeRectChart(1.f, 1.f), Mesh::New()}; + EXPECT_THROW(PackCharts(charts), std::invalid_argument); +} + +TEST(ChartPacking, ZeroAreaChartIsPlacedWithoutThrowing) +{ + // A flat (collinear) chart has zero area but is a valid mesh: distinct + // vertices, non-zero edges. PackCharts must place it without crashing. + auto flat = Mesh::New(); + flat->insert_vertices({{0.f, 0.f, 0.f}, {1.f, 0.f, 0.f}, {2.f, 0.f, 0.f}}); + flat->insert_faces({{0, 1, 2}}); + std::vector charts{MakeRectChart(1.f, 1.f), flat}; + EXPECT_NO_THROW(PackCharts(charts)); +} + +// --- Absolute (default) mode ---------------------------------------------- + +TEST(ChartPacking, SingleChartMapsMinToOrigin) +{ + std::vector charts{MakeRectChart(3.f, 2.f)}; + auto extent = PackCharts(charts); + auto b = ChartBBox(charts[0]); + EXPECT_FLOAT_EQ(b.minx, 0.f); + EXPECT_FLOAT_EQ(b.miny, 0.f); + EXPECT_FLOAT_EQ(b.width(), 3.f); + EXPECT_FLOAT_EQ(b.height(), 2.f); + EXPECT_FLOAT_EQ(extent.max[0], 3.f); + EXPECT_FLOAT_EQ(extent.max[1], 2.f); +} + +TEST(ChartPacking, AbsoluteModePreservesChartSizes) +{ + std::vector ws{1.f, 2.f, 0.5f, 3.f}; + std::vector hs{1.f, 1.5f, 2.f, 0.7f}; + std::vector charts; + for (std::size_t i = 0; i < ws.size(); ++i) { + charts.push_back(MakeRectChart(ws[i], hs[i])); + } + PackCharts(charts); + for (std::size_t i = 0; i < charts.size(); ++i) { + auto b = ChartBBox(charts[i]); + EXPECT_FLOAT_EQ(b.width(), ws[i]) << "chart " << i; + EXPECT_FLOAT_EQ(b.height(), hs[i]) << "chart " << i; + } +} + +TEST(ChartPacking, ChartsDoNotOverlap) +{ + std::vector charts; + for (int i = 0; i < 6; ++i) { + charts.push_back(MakeRectChart(1.f + 0.3f * static_cast(i), 1.f)); + } + PackCharts(charts); + std::vector boxes; + for (const auto& c : charts) { + boxes.push_back(ChartBBox(c)); + } + for (std::size_t i = 0; i < boxes.size(); ++i) { + for (std::size_t j = i + 1; j < boxes.size(); ++j) { + EXPECT_FALSE(Overlaps(boxes[i], boxes[j], 1e-4f)) << "charts " << i << " and " << j; + } + } +} + +TEST(ChartPacking, ExtentBoundsAllCharts) +{ + std::vector charts{MakeRectChart(1.f, 1.f), MakeRectChart(2.f, 0.5f), + MakeRectChart(0.5f, 3.f)}; + auto extent = PackCharts(charts); + for (const auto& c : charts) { + for (const auto& v : c->vertices()) { + EXPECT_GE(v->pos[0], extent.min[0] - 1e-4f); + EXPECT_GE(v->pos[1], extent.min[1] - 1e-4f); + EXPECT_LE(v->pos[0], extent.max[0] + 1e-4f); + EXPECT_LE(v->pos[1], extent.max[1] + 1e-4f); + } + } +} + +TEST(ChartPacking, PaddingSeparatesChartsInSingleRow) +{ + // Force a single row with a large target width; padding should appear as a + // gap between the two charts, so the atlas width is w0 + padding + w1. + std::vector charts{MakeRectChart(1.f, 1.f), MakeRectChart(1.f, 1.f)}; + PackOptions opts; + opts.target_width = 1000.f; + opts.padding = 0.5f; + auto extent = PackCharts(charts, opts); + EXPECT_NEAR(extent.max[0] - extent.min[0], 2.5f, 1e-4f); + + auto b0 = ChartBBox(charts[0]); + auto b1 = ChartBBox(charts[1]); + auto gap = std::max(b1.minx - b0.maxx, b0.minx - b1.maxx); + EXPECT_GE(gap, 0.5f - 1e-4f); +} + +// --- Normalize mode -------------------------------------------------------- + +TEST(ChartPacking, NormalizeFitsUnitSquare) +{ + std::vector charts{MakeRectChart(2.f, 1.f), MakeRectChart(1.f, 3.f), + MakeRectChart(0.5f, 0.5f)}; + PackOptions opts; + opts.normalize = true; + auto extent = PackCharts(charts, opts); + + float maxCoord = 0.f; + for (const auto& c : charts) { + for (const auto& v : c->vertices()) { + EXPECT_GE(v->pos[0], -1e-4f); + EXPECT_GE(v->pos[1], -1e-4f); + EXPECT_LE(v->pos[0], 1.f + 1e-4f); + EXPECT_LE(v->pos[1], 1.f + 1e-4f); + maxCoord = std::max({maxCoord, v->pos[0], v->pos[1]}); + } + } + // The atlas should fill at least one axis of the unit square. + EXPECT_NEAR(maxCoord, 1.f, 1e-3f); + EXPECT_LE(extent.max[0], 1.f + 1e-4f); + EXPECT_LE(extent.max[1], 1.f + 1e-4f); +} + +TEST(ChartPacking, NormalizePreservesRelativeChartSizes) +{ + // One chart twice the linear size of the other: ratio must survive a single + // global uniform scale. + std::vector charts{MakeRectChart(1.f, 1.f), MakeRectChart(2.f, 2.f)}; + PackOptions opts; + opts.normalize = true; + PackCharts(charts, opts); + auto b0 = ChartBBox(charts[0]); + auto b1 = ChartBBox(charts[1]); + EXPECT_NEAR(b1.width() / b0.width(), 2.f, 1e-3f); + EXPECT_NEAR(b1.height() / b0.height(), 2.f, 1e-3f); +} + +// --- End-to-end pipeline + per-wedge recovery ------------------------------ + +TEST(ChartPacking, EndToEndTearExtractFlattenPackAndWedgeRecovery) +{ + using ABF = OpenABF::ABFPlusPlus; + using LSCM = OpenABF::AngleBasedLSCM; + + // 3x3 grid (matches the MultiChartFlatten example). + auto mesh = ABF::Mesh::New(); + mesh->insert_vertices({ + {0.f, 0.f, 0.f}, + {1.f, 0.f, 0.f}, + {2.f, 0.f, 0.f}, + {0.f, 1.f, 0.f}, + {1.f, 1.f, 0.f}, + {2.f, 1.f, 0.f}, + {0.f, 2.f, 0.f}, + {1.f, 2.f, 0.f}, + {2.f, 2.f, 0.f}, + }); + mesh->insert_faces({ + {0, 3, 1}, + {1, 3, 4}, + {1, 4, 2}, + {2, 4, 5}, + {3, 6, 4}, + {4, 6, 7}, + {4, 7, 5}, + {5, 7, 8}, + }); + + // Tear into two charts and extract. + mesh->split_path({1, 4, 7}); + auto ccs = mesh->extract_connected_components(); + ASSERT_EQ(ccs.size(), 2u); + + // Flatten each chart and collect the meshes for packing. + std::vector chartMeshes; + for (auto& cc : ccs) { + std::size_t iters{0}; + float grad{OpenABF::INF}; + ABF::Compute(cc.mesh, iters, grad); + LSCM::Compute(cc.mesh); + chartMeshes.push_back(cc.mesh); + } + + auto extent = PackCharts(chartMeshes); + + // Charts must not overlap after packing. + std::vector boxes; + for (const auto& c : chartMeshes) { + boxes.push_back(ChartBBox(c)); + } + EXPECT_FALSE(Overlaps(boxes[0], boxes[1], 1e-4f)); + + // Per-wedge recovery via the back-maps, keyed on vertex identity. Every + // (original face, original vertex) wedge must be unique, proving the + // documented recipe yields a well-formed per-wedge map. + std::set> wedges; + std::size_t corners = 0; + for (auto& cc : ccs) { + for (const auto& face : cc.mesh->faces()) { + for (const auto& edge : *face) { + auto origFace = cc.face_map[face->idx]; + auto origVert = cc.vertex_map[edge->vertex->idx]; + wedges.emplace(origFace, origVert); + ++corners; + } + } + } + EXPECT_EQ(corners, 8u * 3u); // 8 faces, 3 corners each + EXPECT_EQ(wedges.size(), corners); +} From 9612c612c69b34084762b58b6657eea9c4acc276 Mon Sep 17 00:00:00 2001 From: Seth Parker Date: Sat, 20 Jun 2026 10:17:45 -0400 Subject: [PATCH 02/20] feat(packing): implement PackCharts shelf packing (TDD green) Implement multi-chart UV packing as a geometry-only free function that translates (and optionally uniformly scales) each chart's vertex positions in place. Charts are laid out with shelf packing using a sqrt-area target width (overridable); absolute scale is preserved by default with opt-in normalize to [0,1]^2 via a single global scale. Returns the packed atlas extent. Documents the vertex-identity per-wedge recipe and complexity. Wire ChartPacking.hpp into OpenABF.hpp and regenerate the single header. All 12 PackCharts tests pass (full suite 7/7). Co-Authored-By: Claude Opus 4.8 --- conductor/tracks/F2/plan.md | 21 +-- include/OpenABF/ChartPacking.hpp | 112 ++++++++++++- include/OpenABF/OpenABF.hpp | 2 + single_include/OpenABF/OpenABF.hpp | 252 +++++++++++++++++++++++++++++ 4 files changed, 373 insertions(+), 14 deletions(-) diff --git a/conductor/tracks/F2/plan.md b/conductor/tracks/F2/plan.md index 9c15e78..f86fe03 100644 --- a/conductor/tracks/F2/plan.md +++ b/conductor/tracks/F2/plan.md @@ -21,15 +21,16 @@ Design resolved 2026-06-20 (see spec.md → Design Decisions). verify per-wedge recovery via (face_map[f], vertex_map[corner.vertex.idx]) ## Phase 3: Implementation -- [ ] 3.1 Create `include/OpenABF/ChartPacking.hpp` with PackOptions, PackResult -- [ ] 3.2 Implement per-chart bbox + sqrt-area target width + shelf placement -- [ ] 3.3 Implement absolute (translate-only) and normalize (global uniform scale) modes -- [ ] 3.4 Implement padding, degenerate-input handling, static_assert(Dim>=2) -- [ ] 3.5 Document the vertex-identity per-wedge recipe in the header + complexity notes -- [ ] 3.6 Add include to `include/OpenABF/OpenABF.hpp` -- [ ] 3.7 Update `single_include.json` and run amalgamation script +- [x] 3.1 Create `include/OpenABF/ChartPacking.hpp` with PackOptions, PackResult +- [x] 3.2 Implement per-chart bbox + sqrt-area target width + shelf placement +- [x] 3.3 Implement absolute (translate-only) and normalize (global uniform scale) modes +- [x] 3.4 Implement padding, degenerate-input handling, static_assert(Dim>=2) +- [x] 3.5 Document the vertex-identity per-wedge recipe in the header + complexity notes +- [x] 3.6 Add include to `include/OpenABF/OpenABF.hpp` +- [x] 3.7 Update single-header via amalgamation script (single_include.json unchanged — + it already tracks OpenABF.hpp transitively) ## Phase 4: Verify -- [ ] 4.1 Run `ctest` — all tests pass -- [ ] 4.2 Run clang-format on changed files -- [ ] 4.3 Confirm single-header build matches multi-header +- [x] 4.1 Run `ctest` — all 7 suites pass (incl. new OpenABF_TestChartPacking) +- [x] 4.2 Run clang-format on changed files +- [x] 4.3 Confirm single-header build compiles and runs diff --git a/include/OpenABF/ChartPacking.hpp b/include/OpenABF/ChartPacking.hpp index 641b924..e5c73db 100644 --- a/include/OpenABF/ChartPacking.hpp +++ b/include/OpenABF/ChartPacking.hpp @@ -18,8 +18,13 @@ limitations under the License. */ #pragma once +#include +#include #include +#include +#include #include +#include #include #include @@ -117,6 +122,11 @@ struct PackResult { * } * @endcode * + * @par Complexity + * `O(n log n)` in the number of charts `n` (dominated by the height sort) plus + * `O(V)` in the total vertex count `V` (two passes: one to measure bounding + * boxes, one to apply the transform). Memory overhead is `O(n)`. + * * @tparam MeshType A HalfEdgeMesh specialization * @param charts Charts to pack; each chart's vertex positions are modified * @param opts Packing options @@ -135,10 +145,104 @@ auto PackCharts(std::vector& charts, detail::VecDimensions().pos)>::value >= 2, "PackCharts requires mesh vertices with at least 2 position dimensions"); - // STUB (TDD red phase): real layout is implemented in Phase 3. - (void)charts; - (void)opts; - return PackResult{Vec{T(0), T(0)}, Vec{T(0), T(0)}}; + PackResult result{Vec{T(0), T(0)}, Vec{T(0), T(0)}}; + if (charts.empty()) { + return result; + } + const std::size_t n = charts.size(); + + // Validate inputs and compute each chart's 2D bounding box (origin + size). + std::vector minX(n); + std::vector minY(n); + std::vector width(n); + std::vector height(n); + for (std::size_t i = 0; i < n; ++i) { + const auto& chart = charts[i]; + if (not chart or chart->num_vertices() == 0) { + throw std::invalid_argument("PackCharts: chart is null or has no vertices"); + } + auto mnX = std::numeric_limits::max(); + auto mnY = std::numeric_limits::max(); + auto mxX = std::numeric_limits::lowest(); + auto mxY = std::numeric_limits::lowest(); + for (const auto& v : chart->vertices()) { + mnX = std::min(mnX, v->pos[0]); + mnY = std::min(mnY, v->pos[1]); + mxX = std::max(mxX, v->pos[0]); + mxY = std::max(mxY, v->pos[1]); + } + minX[i] = mnX; + minY[i] = mnY; + width[i] = mxX - mnX; + height[i] = mxY - mnY; + } + + // Place taller charts first so shelves pack tightly. + std::vector order(n); + std::iota(order.begin(), order.end(), std::size_t{0}); + std::sort(order.begin(), order.end(), + [&](std::size_t a, std::size_t b) { return height[a] > height[b]; }); + + // Target shelf width: caller override, else sqrt(sum of padded chart areas) + // for a roughly square atlas. + const T pad = opts.padding; + T targetWidth; + if (opts.target_width) { + targetWidth = *opts.target_width; + } else { + auto areaSum = T(0); + for (std::size_t i = 0; i < n; ++i) { + areaSum += (width[i] + pad) * (height[i] + pad); + } + targetWidth = std::sqrt(areaSum); + } + + // Shelf layout. Charts are placed left-to-right; a row wraps to a new shelf + // once it would exceed targetWidth (a chart wider than targetWidth still + // gets placed alone at the start of a shelf). Every chart's bbox-min is + // mapped to a cursor position >= 0, so the packed atlas's lower corner is + // the origin. + std::vector offsetX(n); + std::vector offsetY(n); + auto cursorX = T(0); + auto cursorY = T(0); + auto shelfHeight = T(0); + auto atlasMaxX = T(0); + auto atlasMaxY = T(0); + for (const auto i : order) { + if (cursorX > T(0) and cursorX + width[i] > targetWidth) { + cursorX = T(0); + cursorY += shelfHeight + pad; + shelfHeight = T(0); + } + offsetX[i] = cursorX - minX[i]; + offsetY[i] = cursorY - minY[i]; + atlasMaxX = std::max(atlasMaxX, cursorX + width[i]); + atlasMaxY = std::max(atlasMaxY, cursorY + height[i]); + cursorX += width[i] + pad; + shelfHeight = std::max(shelfHeight, height[i]); + } + + // Optional normalization: a single global uniform scale that fits the + // packed atlas into [0,1]^2, preserving relative chart sizes. + auto scale = T(1); + if (opts.normalize) { + const auto extentMax = std::max(atlasMaxX, atlasMaxY); + if (extentMax > T(0)) { + scale = T(1) / extentMax; + } + } + + // Apply translation (+ optional scale about the origin) in place. + for (std::size_t i = 0; i < n; ++i) { + for (const auto& v : charts[i]->vertices()) { + v->pos[0] = (v->pos[0] + offsetX[i]) * scale; + v->pos[1] = (v->pos[1] + offsetY[i]) * scale; + } + } + + result.max = Vec{atlasMaxX * scale, atlasMaxY * scale}; + return result; } } // namespace OpenABF diff --git a/include/OpenABF/OpenABF.hpp b/include/OpenABF/OpenABF.hpp index 89e16b1..23754bb 100644 --- a/include/OpenABF/OpenABF.hpp +++ b/include/OpenABF/OpenABF.hpp @@ -30,5 +30,7 @@ limitations under the License. #include "OpenABF/AngleBasedLSCM.hpp" #include "OpenABF/HierarchicalLSCM.hpp" +#include "OpenABF/ChartPacking.hpp" + #include "OpenABF/MeshIO.hpp" // clang-format on diff --git a/single_include/OpenABF/OpenABF.hpp b/single_include/OpenABF/OpenABF.hpp index b21d488..c8ca501 100644 --- a/single_include/OpenABF/OpenABF.hpp +++ b/single_include/OpenABF/OpenABF.hpp @@ -4893,6 +4893,258 @@ class HierarchicalLSCM } // namespace OpenABF +// #include "OpenABF/ChartPacking.hpp" +/* +OpenABF +https://gitlab.com/educelab/OpenABF + +Copyright 2025 EduceLab + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + + +#include +#include +#include +#include +#include +#include +#include +#include +#include + +// #include "OpenABF/Vec.hpp" + + +namespace OpenABF +{ + +namespace detail +{ +/** @brief Deduce the dimensionality of a Vec type */ +template +struct VecDimensions; + +template +struct VecDimensions> { + static constexpr std::size_t value = N; +}; +} // namespace detail + +/** + * @brief Options controlling PackCharts behavior + * + * @tparam T Floating-point scalar type + */ +template +struct PackOptions { + /** + * @brief Fit the packed atlas into the unit square `[0,1]^2` + * + * When `false` (default), charts keep their absolute (physical) scale and + * are only translated. When `true`, a single global uniform scale is + * applied after layout so the whole atlas fits in `[0,1]^2`. A single + * global factor preserves relative chart sizes and cross-chart texel + * density; only the absolute units change. + */ + bool normalize{false}; + + /** + * @brief Target shelf width; overrides the `sqrt(total area)` heuristic + * + * Charts wrap to a new shelf when a row would exceed this width. If unset, + * the width defaults to `sqrt(sum of per-chart bounding-box areas)`, which + * yields a roughly square atlas. + */ + std::optional target_width{}; + + /** @brief Gutter added around each chart, in chart/absolute units */ + T padding{T(0)}; +}; + +/** + * @brief The bounding box of the packed atlas + * + * @tparam T Floating-point scalar type + */ +template +struct PackResult { + /** @brief Lower corner of the packed atlas */ + Vec min; + /** @brief Upper corner of the packed atlas */ + Vec max; +}; + +/** + * @brief Pack a set of parameterized charts into a shared coordinate frame + * + * Lays out a list of already-parameterized charts (2D meshes whose vertex + * `pos` holds `{u, v, ...}`) into a single shared frame using shelf packing, + * so that no two charts' bounding boxes overlap. Operates purely on geometry: + * each chart's vertex positions are translated (and, when `normalize` is set, + * uniformly scaled) **in place**. Topology, vertex indices, and face indices + * are untouched, so any `ExtractedComponent` back-maps a caller holds remain + * valid after packing. + * + * @par Scaling + * By default charts keep their absolute scale and are only translated; the + * returned extent is meaningful in physical units. With `opts.normalize`, one + * global uniform scale maps the packed atlas into `[0,1]^2`. + * + * @par Building a per-wedge UV map + * This function does not own a UV-map type. To build a per-corner ("wedge") + * UV map from the packed charts, key each wedge by **vertex identity**, not by + * corner position: a face's corner order is not stable (the half-edge mesh may + * reverse a mis-wound face at insertion time, and that permutation is not + * recorded). Given an `ExtractedComponent` `ec` for a chart, the robust key is: + * @code + * for (const auto& face : chart->faces()) { + * for (const auto& edge : *face) { // walk the face's corners + * auto origFace = ec.face_map[face->idx]; + * auto origVert = ec.vertex_map[edge->vertex->idx]; + * auto uv = edge->vertex->pos; // packed UV + * // wedge (origFace, origVert) -> uv + * } + * } + * @endcode + * + * @par Complexity + * `O(n log n)` in the number of charts `n` (dominated by the height sort) plus + * `O(V)` in the total vertex count `V` (two passes: one to measure bounding + * boxes, one to apply the transform). Memory overhead is `O(n)`. + * + * @tparam MeshType A HalfEdgeMesh specialization + * @param charts Charts to pack; each chart's vertex positions are modified + * @param opts Packing options + * @return The bounding box of the packed atlas + * + * @throws std::invalid_argument If a chart pointer is null or a chart has no + * vertices. + */ +template +auto PackCharts(std::vector& charts, + const PackOptions& opts = + PackOptions{}) -> PackResult +{ + using T = typename MeshType::type; + static_assert( + detail::VecDimensions().pos)>::value >= 2, + "PackCharts requires mesh vertices with at least 2 position dimensions"); + + PackResult result{Vec{T(0), T(0)}, Vec{T(0), T(0)}}; + if (charts.empty()) { + return result; + } + const std::size_t n = charts.size(); + + // Validate inputs and compute each chart's 2D bounding box (origin + size). + std::vector minX(n); + std::vector minY(n); + std::vector width(n); + std::vector height(n); + for (std::size_t i = 0; i < n; ++i) { + const auto& chart = charts[i]; + if (not chart or chart->num_vertices() == 0) { + throw std::invalid_argument("PackCharts: chart is null or has no vertices"); + } + auto mnX = std::numeric_limits::max(); + auto mnY = std::numeric_limits::max(); + auto mxX = std::numeric_limits::lowest(); + auto mxY = std::numeric_limits::lowest(); + for (const auto& v : chart->vertices()) { + mnX = std::min(mnX, v->pos[0]); + mnY = std::min(mnY, v->pos[1]); + mxX = std::max(mxX, v->pos[0]); + mxY = std::max(mxY, v->pos[1]); + } + minX[i] = mnX; + minY[i] = mnY; + width[i] = mxX - mnX; + height[i] = mxY - mnY; + } + + // Place taller charts first so shelves pack tightly. + std::vector order(n); + std::iota(order.begin(), order.end(), std::size_t{0}); + std::sort(order.begin(), order.end(), + [&](std::size_t a, std::size_t b) { return height[a] > height[b]; }); + + // Target shelf width: caller override, else sqrt(sum of padded chart areas) + // for a roughly square atlas. + const T pad = opts.padding; + T targetWidth; + if (opts.target_width) { + targetWidth = *opts.target_width; + } else { + auto areaSum = T(0); + for (std::size_t i = 0; i < n; ++i) { + areaSum += (width[i] + pad) * (height[i] + pad); + } + targetWidth = std::sqrt(areaSum); + } + + // Shelf layout. Charts are placed left-to-right; a row wraps to a new shelf + // once it would exceed targetWidth (a chart wider than targetWidth still + // gets placed alone at the start of a shelf). Every chart's bbox-min is + // mapped to a cursor position >= 0, so the packed atlas's lower corner is + // the origin. + std::vector offsetX(n); + std::vector offsetY(n); + auto cursorX = T(0); + auto cursorY = T(0); + auto shelfHeight = T(0); + auto atlasMaxX = T(0); + auto atlasMaxY = T(0); + for (const auto i : order) { + if (cursorX > T(0) and cursorX + width[i] > targetWidth) { + cursorX = T(0); + cursorY += shelfHeight + pad; + shelfHeight = T(0); + } + offsetX[i] = cursorX - minX[i]; + offsetY[i] = cursorY - minY[i]; + atlasMaxX = std::max(atlasMaxX, cursorX + width[i]); + atlasMaxY = std::max(atlasMaxY, cursorY + height[i]); + cursorX += width[i] + pad; + shelfHeight = std::max(shelfHeight, height[i]); + } + + // Optional normalization: a single global uniform scale that fits the + // packed atlas into [0,1]^2, preserving relative chart sizes. + auto scale = T(1); + if (opts.normalize) { + const auto extentMax = std::max(atlasMaxX, atlasMaxY); + if (extentMax > T(0)) { + scale = T(1) / extentMax; + } + } + + // Apply translation (+ optional scale about the origin) in place. + for (std::size_t i = 0; i < n; ++i) { + for (const auto& v : charts[i]->vertices()) { + v->pos[0] = (v->pos[0] + offsetX[i]) * scale; + v->pos[1] = (v->pos[1] + offsetY[i]) * scale; + } + } + + result.max = Vec{atlasMaxX * scale, atlasMaxY * scale}; + return result; +} + +} // namespace OpenABF + + // #include "OpenABF/MeshIO.hpp" From 3ef7a75404beaa0b9d2365050093a4848efb5360 Mon Sep 17 00:00:00 2001 From: Seth Parker Date: Sat, 20 Jun 2026 10:20:06 -0400 Subject: [PATCH 03/20] fix(build): install ChartPacking.hpp in multiheader install set The multiheader install enumerates public headers explicitly; ChartPacking.hpp was missing, so the installed OpenABF.hpp failed to find it (CI install-test, Multiheader=ON). Add it to the install list. Co-Authored-By: Claude Opus 4.8 --- CMakeLists.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/CMakeLists.txt b/CMakeLists.txt index d4f572e..67c547e 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -29,6 +29,7 @@ if(OPENABF_MULTIHEADER) include/OpenABF/HalfEdgeMesh.hpp include/OpenABF/HierarchicalLSCM.hpp include/OpenABF/AngleBasedLSCM.hpp + include/OpenABF/ChartPacking.hpp include/OpenABF/Math.hpp include/OpenABF/Vec.hpp include/OpenABF/MeshIO.hpp From af2e52ca69af36904934a0b7777a77652f366a5a Mon Sep 17 00:00:00 2001 From: Seth Parker Date: Sat, 20 Jun 2026 10:23:45 -0400 Subject: [PATCH 04/20] fix(test): include only OpenABF.hpp in TestChartPacking The direct include of OpenABF/ChartPacking.hpp broke the single-header build (Multiheader=OFF), where only the amalgamated OpenABF.hpp exists. ChartPacking is included transitively via OpenABF.hpp, matching the other test files; drop the direct include. Co-Authored-By: Claude Opus 4.8 --- tests/src/TestChartPacking.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/tests/src/TestChartPacking.cpp b/tests/src/TestChartPacking.cpp index d9d3c43..68aabce 100644 --- a/tests/src/TestChartPacking.cpp +++ b/tests/src/TestChartPacking.cpp @@ -6,7 +6,6 @@ #include -#include "OpenABF/ChartPacking.hpp" #include "OpenABF/OpenABF.hpp" using Mesh = OpenABF::HalfEdgeMesh; From 85290a87688e7438952224492f2a5da2e3caf08d Mon Sep 17 00:00:00 2001 From: Seth Parker Date: Sat, 20 Jun 2026 10:28:39 -0400 Subject: [PATCH 05/20] docs(example): pack multi-chart flatten output into a single atlas Update MultiChartFlatten to demonstrate PackCharts: flatten each connected component, pack the charts into a shared [0,1]^2 frame, merge them into one mesh, and write a single packed-atlas .obj instead of one file per chart. Co-Authored-By: Claude Opus 4.8 --- examples/src/MultiChartFlatten.cpp | 61 ++++++++++++++++++++++++------ 1 file changed, 50 insertions(+), 11 deletions(-) diff --git a/examples/src/MultiChartFlatten.cpp b/examples/src/MultiChartFlatten.cpp index dd5b53d..09bc3fa 100644 --- a/examples/src/MultiChartFlatten.cpp +++ b/examples/src/MultiChartFlatten.cpp @@ -5,17 +5,20 @@ * * Builds a mesh with multiple connected components (here, a 3x3 grid torn * along two seams), extracts each component as an independent mesh, runs - * ABF++ + LSCM on each, and writes each flattened chart to its own .obj - * file. + * ABF++ + LSCM on each, packs the flattened charts into a shared coordinate + * frame with OpenABF::PackCharts, and writes the packed atlas to a single + * .obj file. * * The original mesh is never modified — the extracted sub-meshes own their - * own vertices and per-edge state, and each is parameterized in isolation. + * own vertices and per-edge state, and each is parameterized in isolation + * before being placed into the common frame. * * @see OpenABF::HalfEdgeMesh::split_path * @see OpenABF::HalfEdgeMesh::extract_connected_components + * @see OpenABF::PackCharts */ #include -#include +#include #include "OpenABF/OpenABF.hpp" @@ -23,9 +26,10 @@ int main() { using ABF = OpenABF::ABFPlusPlus; using LSCM = OpenABF::AngleBasedLSCM; + using Mesh = ABF::Mesh; // Build a 3x3 grid (9 vertices, 8 triangles) - auto mesh = ABF::Mesh::New(); + auto mesh = Mesh::New(); mesh->insert_vertices({ {0.f, 0.f, 0.f}, {1.f, 0.f, 0.f}, @@ -62,7 +66,8 @@ int main() auto charts = mesh->extract_connected_components(); std::cout << "Extracted " << charts.size() << " chart(s)\n"; - // Flatten each chart in isolation and write it out as its own .obj. + // Flatten each chart in isolation, collecting the parameterized meshes. + std::vector chartMeshes; for (std::size_t i = 0; i < charts.size(); ++i) { auto& cc = charts[i]; @@ -70,12 +75,10 @@ int main() float grad{OpenABF::INF}; ABF::Compute(cc.mesh, iters, grad); LSCM::Compute(cc.mesh); + chartMeshes.push_back(cc.mesh); - const auto out = "openabf_example_multi_chart_" + std::to_string(i) + ".obj"; - OpenABF::WriteMesh(out, cc.mesh); std::cout << "Chart " << i << ": " << cc.mesh->num_vertices() << " vertices, " - << cc.mesh->num_faces() << " faces, " << iters << " ABF++ iters -> " << out - << "\n"; + << cc.mesh->num_faces() << " faces, " << iters << " ABF++ iters\n"; // cc.vertex_map[chart_idx] -> original vertex idx // cc.face_map[chart_idx] -> original face idx @@ -83,8 +86,44 @@ int main() // keyed by source-mesh face corners. } + // Pack the flattened charts into a shared frame. `normalize` fits the whole + // atlas into [0,1]^2 via a single global uniform scale, which preserves the + // charts' relative sizes. Packing only edits each chart's 2D vertex + // positions in place; the per-chart vertex_map/face_map remain valid. + OpenABF::PackOptions opts; + opts.normalize = true; + auto extent = OpenABF::PackCharts(chartMeshes, opts); + std::cout << "Packed atlas extent: [" << extent.min[0] << ", " << extent.min[1] << "] -> [" + << extent.max[0] << ", " << extent.max[1] << "]\n"; + + // Merge the packed charts into a single mesh and write it as one atlas. + // The charts are disjoint in the packed frame, so concatenating their + // vertices and re-emitting their faces (with an index offset per chart) + // reconstructs a multi-component mesh in UV space. + auto packed = Mesh::New(); + std::size_t offset{0}; + for (const auto& chart : chartMeshes) { + for (const auto& v : chart->vertices()) { + packed->insert_vertex(v->pos[0], v->pos[1], v->pos[2]); + } + for (const auto& face : chart->faces()) { + std::vector idxs; + for (const auto& edge : *face) { + idxs.push_back(edge->vertex->idx + offset); + } + packed->insert_face(idxs); + } + offset += chart->num_vertices(); + } + + const std::string out = "openabf_example_multi_chart_packed.obj"; + OpenABF::WriteMesh(out, packed); + std::cout << "Wrote packed atlas: " << packed->num_vertices() << " vertices, " + << packed->num_faces() << " faces -> " << out << "\n"; + // The source mesh's 3D vertex positions are unchanged by the per-chart - // flattening — only the extracted sub-meshes hold the 2D UV result. + // flattening and packing — only the extracted sub-meshes hold the 2D UV + // result. std::cout << "Source mesh 3D positions intact: " << mesh->num_vertices() << " vertices, " << mesh->num_faces() << " faces\n"; } From 697b8975b9db89050be878f919e5a62198b090d8 Mon Sep 17 00:00:00 2001 From: Seth Parker Date: Sat, 20 Jun 2026 13:40:58 -0400 Subject: [PATCH 06/20] feat(merge): add MergeMeshes, inverse of extract_connected_components Add MergeMeshes -> MergedMesh{mesh, vertex_source, face_source}: concatenates meshes into one and returns provenance maps (merged idx -> {input mesh, source idx}) so the back-map chain survives a merge (compose with each component's vertex_map/face_map to reach the torn source mesh). Preserves vertex positions/traits; edge/face traits are default-constructed. Switch the MultiChartFlatten example to use MergeMeshes instead of an inline concatenate that discarded provenance. Wire the header into OpenABF.hpp and the multiheader install set; regenerate the single header. New test suite TestMeshMerge covers counts, provenance, throws, and an extract->merge round-trip that recovers original face identity. Co-Authored-By: Claude Opus 4.8 --- CMakeLists.txt | 1 + conductor/tracks/F2/plan.md | 15 ++- examples/src/MultiChartFlatten.cpp | 28 ++---- include/OpenABF/MeshMerge.hpp | 103 ++++++++++++++++++++ include/OpenABF/OpenABF.hpp | 1 + single_include/OpenABF/OpenABF.hpp | 105 ++++++++++++++++++++ tests/CMakeLists.txt | 1 + tests/src/TestMeshMerge.cpp | 151 +++++++++++++++++++++++++++++ 8 files changed, 383 insertions(+), 22 deletions(-) create mode 100644 include/OpenABF/MeshMerge.hpp create mode 100644 tests/src/TestMeshMerge.cpp diff --git a/CMakeLists.txt b/CMakeLists.txt index 67c547e..f3bd7bc 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -30,6 +30,7 @@ if(OPENABF_MULTIHEADER) include/OpenABF/HierarchicalLSCM.hpp include/OpenABF/AngleBasedLSCM.hpp include/OpenABF/ChartPacking.hpp + include/OpenABF/MeshMerge.hpp include/OpenABF/Math.hpp include/OpenABF/Vec.hpp include/OpenABF/MeshIO.hpp diff --git a/conductor/tracks/F2/plan.md b/conductor/tracks/F2/plan.md index f86fe03..e94e121 100644 --- a/conductor/tracks/F2/plan.md +++ b/conductor/tracks/F2/plan.md @@ -31,6 +31,19 @@ Design resolved 2026-06-20 (see spec.md → Design Decisions). it already tracks OpenABF.hpp transitively) ## Phase 4: Verify -- [x] 4.1 Run `ctest` — all 7 suites pass (incl. new OpenABF_TestChartPacking) +- [x] 4.1 Run `ctest` — all suites pass (incl. OpenABF_TestChartPacking, OpenABF_TestMeshMerge) - [x] 4.2 Run clang-format on changed files - [x] 4.3 Confirm single-header build compiles and runs + +## Phase 5: MergeMeshes helper (added during review) +Rationale: the inline atlas merge in the example severs the back-map chain. +MergeMeshes is the inverse of extract_connected_components — it returns +provenance maps so merged → chart → M' composition keeps working. +- [x] 5.1 Tests first: concatenation counts, vertex/face provenance, null/empty + throw, round-trip extract→merge recovers original (torn-mesh) identity +- [x] 5.2 Implement `MergeMeshes` → `MergedMesh{mesh, vertex_source, face_source}` + in `include/OpenABF/MeshMerge.hpp` (preserves vertex traits/positions; + edge/face traits default-constructed) +- [x] 5.3 Wire into OpenABF.hpp + multiheader install list; regenerate single header +- [x] 5.4 Switch MultiChartFlatten example to use MergeMeshes +- [x] 5.5 Verify: full ctest, single-header build, install-test all pass diff --git a/examples/src/MultiChartFlatten.cpp b/examples/src/MultiChartFlatten.cpp index 09bc3fa..03d049c 100644 --- a/examples/src/MultiChartFlatten.cpp +++ b/examples/src/MultiChartFlatten.cpp @@ -97,29 +97,15 @@ int main() << extent.max[0] << ", " << extent.max[1] << "]\n"; // Merge the packed charts into a single mesh and write it as one atlas. - // The charts are disjoint in the packed frame, so concatenating their - // vertices and re-emitting their faces (with an index offset per chart) - // reconstructs a multi-component mesh in UV space. - auto packed = Mesh::New(); - std::size_t offset{0}; - for (const auto& chart : chartMeshes) { - for (const auto& v : chart->vertices()) { - packed->insert_vertex(v->pos[0], v->pos[1], v->pos[2]); - } - for (const auto& face : chart->faces()) { - std::vector idxs; - for (const auto& edge : *face) { - idxs.push_back(edge->vertex->idx + offset); - } - packed->insert_face(idxs); - } - offset += chart->num_vertices(); - } + // MergeMeshes returns provenance maps (vertex_source/face_source) that, when + // composed with each component's vertex_map/face_map, trace any atlas + // element back to the torn source mesh. + auto merged = OpenABF::MergeMeshes(chartMeshes); const std::string out = "openabf_example_multi_chart_packed.obj"; - OpenABF::WriteMesh(out, packed); - std::cout << "Wrote packed atlas: " << packed->num_vertices() << " vertices, " - << packed->num_faces() << " faces -> " << out << "\n"; + OpenABF::WriteMesh(out, merged.mesh); + std::cout << "Wrote packed atlas: " << merged.mesh->num_vertices() << " vertices, " + << merged.mesh->num_faces() << " faces -> " << out << "\n"; // The source mesh's 3D vertex positions are unchanged by the per-chart // flattening and packing — only the extracted sub-meshes hold the 2D UV diff --git a/include/OpenABF/MeshMerge.hpp b/include/OpenABF/MeshMerge.hpp new file mode 100644 index 0000000..f1efd53 --- /dev/null +++ b/include/OpenABF/MeshMerge.hpp @@ -0,0 +1,103 @@ +/* +OpenABF +https://gitlab.com/educelab/OpenABF + +Copyright 2025 EduceLab + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ +#pragma once + +#include +#include +#include +#include + +namespace OpenABF +{ + +/** + * @brief Result of merging several meshes into one + * + * The inverse bookkeeping of `HalfEdgeMesh::extract_connected_components`: + * where extract splits one mesh into N with maps back to the source, merge + * combines N meshes into one with maps back to each source mesh. Compose + * `face_source`/`vertex_source` with the `face_map`/`vertex_map` of the + * components you merged to trace a merged element all the way back to the + * pre-extraction mesh. + * + * @tparam MeshType A HalfEdgeMesh specialization + */ +template +struct MergedMesh { + /** @brief The combined mesh */ + typename MeshType::Pointer mesh; + /** @brief `vertex_source[merged_idx] = {input mesh index, source vertex index}` */ + std::vector> vertex_source; + /** @brief `face_source[merged_idx] = {input mesh index, source face index}` */ + std::vector> face_source; +}; + +/** + * @brief Merge several meshes into a single mesh + * + * Concatenates the vertices and faces of each input mesh into one new mesh, + * offsetting face vertex indices per input so the inputs remain disjoint + * components. Returns the combined mesh alongside `vertex_source`/`face_source` + * maps that record, for every merged vertex and face, which input mesh and + * which source index it came from — the inverse of + * `extract_connected_components`, so the back-map chain survives the merge. + * + * Vertex positions and vertex traits are preserved (via the vertex copy + * constructor). Edge and face traits are default-constructed: merge rebuilds + * connectivity, so per-edge/per-face solver state is not carried over. + * + * @tparam MeshType A HalfEdgeMesh specialization + * @param meshes Meshes to merge + * @return The combined mesh and its provenance maps + * + * @throws std::invalid_argument If an input pointer is null or has no vertices. + */ +template +auto MergeMeshes(const std::vector& meshes) -> MergedMesh +{ + MergedMesh result{MeshType::New(), {}, {}}; + auto& out = result.mesh; + + for (std::size_t ci = 0; ci < meshes.size(); ++ci) { + const auto& src = meshes[ci]; + if (not src or src->num_vertices() == 0) { + throw std::invalid_argument("MergeMeshes: input mesh is null or has no vertices"); + } + // Vertices keep their relative order, so merged index == offset + sub + // index; record provenance alongside each insertion. + const auto offset = out->num_vertices(); + for (const auto& v : src->vertices()) { + out->insert_vertex(*v); + result.vertex_source.emplace_back(ci, v->idx); + } + // Re-emit each face against the offset vertex indices, preserving the + // source face's corner order. + for (const auto& face : src->faces()) { + std::vector idxs; + for (const auto& edge : *face) { + idxs.push_back(edge->vertex->idx + offset); + } + out->insert_face(idxs); + result.face_source.emplace_back(ci, face->idx); + } + } + return result; +} + +} // namespace OpenABF diff --git a/include/OpenABF/OpenABF.hpp b/include/OpenABF/OpenABF.hpp index 23754bb..c8f4d54 100644 --- a/include/OpenABF/OpenABF.hpp +++ b/include/OpenABF/OpenABF.hpp @@ -31,6 +31,7 @@ limitations under the License. #include "OpenABF/HierarchicalLSCM.hpp" #include "OpenABF/ChartPacking.hpp" +#include "OpenABF/MeshMerge.hpp" #include "OpenABF/MeshIO.hpp" // clang-format on diff --git a/single_include/OpenABF/OpenABF.hpp b/single_include/OpenABF/OpenABF.hpp index c8ca501..260b363 100644 --- a/single_include/OpenABF/OpenABF.hpp +++ b/single_include/OpenABF/OpenABF.hpp @@ -5144,6 +5144,111 @@ auto PackCharts(std::vector& charts, } // namespace OpenABF +// #include "OpenABF/MeshMerge.hpp" +/* +OpenABF +https://gitlab.com/educelab/OpenABF + +Copyright 2025 EduceLab + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + + +#include +#include +#include +#include + +namespace OpenABF +{ + +/** + * @brief Result of merging several meshes into one + * + * The inverse bookkeeping of `HalfEdgeMesh::extract_connected_components`: + * where extract splits one mesh into N with maps back to the source, merge + * combines N meshes into one with maps back to each source mesh. Compose + * `face_source`/`vertex_source` with the `face_map`/`vertex_map` of the + * components you merged to trace a merged element all the way back to the + * pre-extraction mesh. + * + * @tparam MeshType A HalfEdgeMesh specialization + */ +template +struct MergedMesh { + /** @brief The combined mesh */ + typename MeshType::Pointer mesh; + /** @brief `vertex_source[merged_idx] = {input mesh index, source vertex index}` */ + std::vector> vertex_source; + /** @brief `face_source[merged_idx] = {input mesh index, source face index}` */ + std::vector> face_source; +}; + +/** + * @brief Merge several meshes into a single mesh + * + * Concatenates the vertices and faces of each input mesh into one new mesh, + * offsetting face vertex indices per input so the inputs remain disjoint + * components. Returns the combined mesh alongside `vertex_source`/`face_source` + * maps that record, for every merged vertex and face, which input mesh and + * which source index it came from — the inverse of + * `extract_connected_components`, so the back-map chain survives the merge. + * + * Vertex positions and vertex traits are preserved (via the vertex copy + * constructor). Edge and face traits are default-constructed: merge rebuilds + * connectivity, so per-edge/per-face solver state is not carried over. + * + * @tparam MeshType A HalfEdgeMesh specialization + * @param meshes Meshes to merge + * @return The combined mesh and its provenance maps + * + * @throws std::invalid_argument If an input pointer is null or has no vertices. + */ +template +auto MergeMeshes(const std::vector& meshes) -> MergedMesh +{ + MergedMesh result{MeshType::New(), {}, {}}; + auto& out = result.mesh; + + for (std::size_t ci = 0; ci < meshes.size(); ++ci) { + const auto& src = meshes[ci]; + if (not src or src->num_vertices() == 0) { + throw std::invalid_argument("MergeMeshes: input mesh is null or has no vertices"); + } + // Vertices keep their relative order, so merged index == offset + sub + // index; record provenance alongside each insertion. + const auto offset = out->num_vertices(); + for (const auto& v : src->vertices()) { + out->insert_vertex(*v); + result.vertex_source.emplace_back(ci, v->idx); + } + // Re-emit each face against the offset vertex indices, preserving the + // source face's corner order. + for (const auto& face : src->faces()) { + std::vector idxs; + for (const auto& edge : *face) { + idxs.push_back(edge->vertex->idx + offset); + } + out->insert_face(idxs); + result.face_source.emplace_back(ci, face->idx); + } + } + return result; +} + +} // namespace OpenABF + // #include "OpenABF/MeshIO.hpp" diff --git a/tests/CMakeLists.txt b/tests/CMakeLists.txt index e381070..2cc81e8 100644 --- a/tests/CMakeLists.txt +++ b/tests/CMakeLists.txt @@ -17,6 +17,7 @@ set(tests src/TestMeshIOUtils.cpp src/TestMeshIO.cpp src/TestChartPacking.cpp + src/TestMeshMerge.cpp ) foreach(src ${tests}) diff --git a/tests/src/TestMeshMerge.cpp b/tests/src/TestMeshMerge.cpp new file mode 100644 index 0000000..2a9e3cd --- /dev/null +++ b/tests/src/TestMeshMerge.cpp @@ -0,0 +1,151 @@ +#include +#include +#include + +#include + +#include "OpenABF/OpenABF.hpp" + +using Mesh = OpenABF::HalfEdgeMesh; +using OpenABF::MergedMesh; +using OpenABF::MergeMeshes; + +namespace +{ + +/** @brief Build a w x h rectangle chart (two triangles) translated to (ox,oy) */ +auto MakeRectChart(float w, float h, float ox, float oy) -> Mesh::Pointer +{ + auto m = Mesh::New(); + m->insert_vertices( + {{ox, oy, 0.f}, {ox + w, oy, 0.f}, {ox + w, oy + h, 0.f}, {ox, oy + h, 0.f}}); + m->insert_faces({{0, 1, 2}, {0, 2, 3}}); + return m; +} + +} // namespace + +TEST(MeshMerge, EmptyListGivesEmptyMesh) +{ + std::vector charts; + auto merged = MergeMeshes(charts); + ASSERT_NE(merged.mesh, nullptr); + EXPECT_EQ(merged.mesh->num_vertices(), 0u); + EXPECT_EQ(merged.mesh->num_faces(), 0u); + EXPECT_TRUE(merged.vertex_source.empty()); + EXPECT_TRUE(merged.face_source.empty()); +} + +TEST(MeshMerge, NullChartThrows) +{ + std::vector charts{MakeRectChart(1.f, 1.f, 0.f, 0.f), nullptr}; + EXPECT_THROW(MergeMeshes(charts), std::invalid_argument); +} + +TEST(MeshMerge, EmptyChartThrows) +{ + std::vector charts{MakeRectChart(1.f, 1.f, 0.f, 0.f), Mesh::New()}; + EXPECT_THROW(MergeMeshes(charts), std::invalid_argument); +} + +TEST(MeshMerge, ConcatenatesVerticesAndFaces) +{ + std::vector charts{MakeRectChart(1.f, 1.f, 0.f, 0.f), + MakeRectChart(1.f, 1.f, 5.f, 0.f)}; + auto merged = MergeMeshes(charts); + EXPECT_EQ(merged.mesh->num_vertices(), 8u); + EXPECT_EQ(merged.mesh->num_faces(), 4u); + EXPECT_EQ(merged.vertex_source.size(), 8u); + EXPECT_EQ(merged.face_source.size(), 4u); +} + +TEST(MeshMerge, VertexSourcePreservesPositionsAndProvenance) +{ + std::vector charts{MakeRectChart(1.f, 1.f, 0.f, 0.f), + MakeRectChart(2.f, 3.f, 5.f, 0.f)}; + auto merged = MergeMeshes(charts); + + ASSERT_EQ(merged.vertex_source.size(), merged.mesh->num_vertices()); + for (std::size_t i = 0; i < merged.mesh->num_vertices(); ++i) { + auto [chart, sub] = merged.vertex_source[i]; + ASSERT_LT(chart, charts.size()); + ASSERT_LT(sub, charts[chart]->num_vertices()); + const auto& mergedV = merged.mesh->vertex(i); + const auto& srcV = charts[chart]->vertex(sub); + EXPECT_FLOAT_EQ(mergedV->pos[0], srcV->pos[0]) << "vertex " << i; + EXPECT_FLOAT_EQ(mergedV->pos[1], srcV->pos[1]) << "vertex " << i; + } +} + +TEST(MeshMerge, FaceSourceProvenanceIsValid) +{ + std::vector charts{MakeRectChart(1.f, 1.f, 0.f, 0.f), + MakeRectChart(1.f, 1.f, 5.f, 0.f)}; + auto merged = MergeMeshes(charts); + + ASSERT_EQ(merged.face_source.size(), merged.mesh->num_faces()); + // Each (chart, sub face) provenance is unique and in range. + std::set> seen; + for (const auto& [chart, sub] : merged.face_source) { + ASSERT_LT(chart, charts.size()); + ASSERT_LT(sub, charts[chart]->num_faces()); + EXPECT_TRUE(seen.emplace(chart, sub).second) << "duplicate face provenance"; + } +} + +// Composing the merge maps with extract's back-maps must recover original +// (torn-mesh) identity: this is the full chain merged -> chart -> M'. +TEST(MeshMerge, RoundTripExtractMergeRecoversOriginalIdentity) +{ + using ABF = OpenABF::ABFPlusPlus; + using LSCM = OpenABF::AngleBasedLSCM; + + auto mesh = ABF::Mesh::New(); + mesh->insert_vertices({ + {0.f, 0.f, 0.f}, + {1.f, 0.f, 0.f}, + {2.f, 0.f, 0.f}, + {0.f, 1.f, 0.f}, + {1.f, 1.f, 0.f}, + {2.f, 1.f, 0.f}, + {0.f, 2.f, 0.f}, + {1.f, 2.f, 0.f}, + {2.f, 2.f, 0.f}, + }); + mesh->insert_faces({ + {0, 3, 1}, + {1, 3, 4}, + {1, 4, 2}, + {2, 4, 5}, + {3, 6, 4}, + {4, 6, 7}, + {4, 7, 5}, + {5, 7, 8}, + }); + mesh->split_path({1, 4, 7}); + auto ccs = mesh->extract_connected_components(); + ASSERT_EQ(ccs.size(), 2u); + + std::vector chartMeshes; + std::size_t totalVerts = 0; + std::size_t totalFaces = 0; + for (auto& cc : ccs) { + LSCM::Compute(cc.mesh); + chartMeshes.push_back(cc.mesh); + totalVerts += cc.mesh->num_vertices(); + totalFaces += cc.mesh->num_faces(); + } + + auto merged = MergeMeshes(chartMeshes); + EXPECT_EQ(merged.mesh->num_vertices(), totalVerts); + EXPECT_EQ(merged.mesh->num_faces(), totalFaces); + + // merged face -> (chart, sub face) -> M' face via extract face_map. + // Every original (torn-mesh) face must be hit exactly once. + std::set origFaces; + for (const auto& [chart, sub] : merged.face_source) { + auto origFace = ccs[chart].face_map[sub]; + EXPECT_TRUE(origFaces.insert(origFace).second) << "duplicate original face " << origFace; + } + EXPECT_EQ(origFaces.size(), 8u); // all original faces recovered +} From 02e50305b0ca8e8a521a481c91c73aee1ebb24ee Mon Sep 17 00:00:00 2001 From: Seth Parker Date: Sat, 20 Jun 2026 13:45:55 -0400 Subject: [PATCH 07/20] docs(example): add UVMap construction reference (comment only) Add a non-compiled reference comment to MultiChartFlatten showing how to populate an educelab::core UVMap from the merged atlas: walk atlas faces back to the torn source mesh via face_source/face_map, take UVs from the packed chart vertices, and resolve corner positions by vertex identity (winding is not guaranteed stable). Demonstrates the optional WithChart trait for per-coordinate chart tagging. Co-Authored-By: Claude Opus 4.8 --- examples/src/MultiChartFlatten.cpp | 56 ++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) diff --git a/examples/src/MultiChartFlatten.cpp b/examples/src/MultiChartFlatten.cpp index 03d049c..b492a94 100644 --- a/examples/src/MultiChartFlatten.cpp +++ b/examples/src/MultiChartFlatten.cpp @@ -107,6 +107,62 @@ int main() std::cout << "Wrote packed atlas: " << merged.mesh->num_vertices() << " vertices, " << merged.mesh->num_faces() << " faces -> " << out << "\n"; + /* + * Reference: building a per-wedge UVMap from the merged result + * ----------------------------------------------------------------------- + * OpenABF does not own a UV-map type, but the merged atlas plus the + * components' back-maps carry everything needed to populate one. The + * snippet below (not compiled here) targets educelab::core's UVMap: + * + * educelab/core/types/UVMap.hpp + * + * The UVMap is keyed by (face, corner) against the *torn source mesh* + * `mesh` — which still holds the original 3D geometry, since only the + * extracted charts were flattened. UV coordinates come from the packed + * chart vertices. Corner positions are resolved by *vertex identity*, not + * by traversal order: a face's winding may be reversed at insertion time, + * so the chart/atlas corner order is not guaranteed to match the source + * face's corner order (see PackCharts / HalfEdgeMesh::insert_face). + * + * #include + * #include "educelab/core/types/UVMap.hpp" + * using educelab::UVMap; + * using educelab::traits::WithChart; // optional: tag each UV by chart + * + * UVMap uv; + * + * for (std::size_t mf = 0; mf < merged.mesh->num_faces(); ++mf) { + * // Atlas face -> source chart + chart-local face -> source (M') face. + * const auto [chart, subFace] = merged.face_source[mf]; + * const auto srcFace = charts[chart].face_map[subFace]; + * + * // Source face corner order, keyed by M' vertex index. + * std::vector srcCorners; + * for (const auto& e : *mesh->faces()[srcFace]) { + * srcCorners.push_back(e->vertex->idx); + * } + * + * // Each atlas-face corner carries its packed UV in pos. + * for (const auto& e : *merged.mesh->faces()[mf]) { + * const auto [vChart, vSub] = merged.vertex_source[e->vertex->idx]; + * const auto srcVert = charts[vChart].vertex_map[vSub]; // M' vertex + * + * // Place the UV at the matching corner of the source face. + * const auto corner = static_cast(std::distance( + * srcCorners.begin(), + * std::find(srcCorners.begin(), srcCorners.end(), srcVert))); + * + * UVMap::Coordinate c{e->vertex->pos[0], + * e->vertex->pos[1]}; + * c.chart = chart; // WithChart mixin + * uv.map(srcFace, corner, uv.insert(c)); + * } + * } + * + * // uv.get_coordinate(srcFace, corner) now yields the packed UV for + * // each wedge of `mesh`, ready for OBJ `vt` emission. + */ + // The source mesh's 3D vertex positions are unchanged by the per-chart // flattening and packing — only the extracted sub-meshes hold the 2D UV // result. From dead335acaee9423f0f422eb2bbe9f68328d4bd0 Mon Sep 17 00:00:00 2001 From: Seth Parker Date: Sat, 20 Jun 2026 13:55:31 -0400 Subject: [PATCH 08/20] docs(example): correct UVMap WithChart usage in reference comment WithChart's chart index denotes independent packing domains (separate [0,1]^2 atlases / usemtl texture pages), not the connected components of a single shared atlas. This example packs all CCs into one atlas via a single PackCharts call, so the snippet now uses the default UVMap (one domain) and documents that WithChart applies only when running multiple independent packings. Co-Authored-By: Claude Opus 4.8 --- examples/src/MultiChartFlatten.cpp | 23 +++++++++++++++++------ 1 file changed, 17 insertions(+), 6 deletions(-) diff --git a/examples/src/MultiChartFlatten.cpp b/examples/src/MultiChartFlatten.cpp index b492a94..d2815e5 100644 --- a/examples/src/MultiChartFlatten.cpp +++ b/examples/src/MultiChartFlatten.cpp @@ -124,12 +124,16 @@ int main() * so the chart/atlas corner order is not guaranteed to match the source * face's corner order (see PackCharts / HalfEdgeMesh::insert_face). * + * This single PackCharts call produced ONE shared [0,1]^2 atlas, i.e. one + * packing domain, so the default UVMap (no per-coordinate chart index) is + * the right type here — every wedge lives in the same domain. See the note + * after the snippet for when traits::WithChart applies. + * * #include * #include "educelab/core/types/UVMap.hpp" * using educelab::UVMap; - * using educelab::traits::WithChart; // optional: tag each UV by chart * - * UVMap uv; + * UVMap uv; * * for (std::size_t mf = 0; mf < merged.mesh->num_faces(); ++mf) { * // Atlas face -> source chart + chart-local face -> source (M') face. @@ -152,15 +156,22 @@ int main() * srcCorners.begin(), * std::find(srcCorners.begin(), srcCorners.end(), srcVert))); * - * UVMap::Coordinate c{e->vertex->pos[0], - * e->vertex->pos[1]}; - * c.chart = chart; // WithChart mixin - * uv.map(srcFace, corner, uv.insert(c)); + * uv.map(srcFace, corner, uv.insert(e->vertex->pos[0], + * e->vertex->pos[1])); * } * } * * // uv.get_coordinate(srcFace, corner) now yields the packed UV for * // each wedge of `mesh`, ready for OBJ `vt` emission. + * + * traits::WithChart is for MULTIPLE independent packing domains, not the + * connected components of a single atlas. Its `chart` index maps to a + * separate [0,1]^2 domain / texture page: educelab's write_obj groups + * faces into `usemtl materialN` by chart and emits one texture path per + * chart. You would use it if you partitioned the components into groups + * and ran PackCharts once per group (one atlas each), setting + * `coordinate.chart` to that group's domain index — not to tag the CCs + * that share this single packed frame. */ // The source mesh's 3D vertex positions are unchanged by the per-chart From fafe4d90ce2ca28e6fecdc21bbd139da0d2f0fcf Mon Sep 17 00:00:00 2001 From: Seth Parker Date: Sat, 20 Jun 2026 14:13:28 -0400 Subject: [PATCH 09/20] docs(example): note UVMap validity on torn and untorn mesh Document that the per-wedge UVMap is valid for both the torn and the untorn (pre-split) mesh because corners are resolved by vertex identity against the target mesh's own faces, which absorbs any insert_face winding reversal at build/extract/merge. Note the caveat that as-built corner order may differ from the raw input face list (tracked separately). Co-Authored-By: Claude Opus 4.8 --- examples/src/MultiChartFlatten.cpp | 24 +++++++++++------------- 1 file changed, 11 insertions(+), 13 deletions(-) diff --git a/examples/src/MultiChartFlatten.cpp b/examples/src/MultiChartFlatten.cpp index d2815e5..b9e6b52 100644 --- a/examples/src/MultiChartFlatten.cpp +++ b/examples/src/MultiChartFlatten.cpp @@ -124,11 +124,6 @@ int main() * so the chart/atlas corner order is not guaranteed to match the source * face's corner order (see PackCharts / HalfEdgeMesh::insert_face). * - * This single PackCharts call produced ONE shared [0,1]^2 atlas, i.e. one - * packing domain, so the default UVMap (no per-coordinate chart index) is - * the right type here — every wedge lives in the same domain. See the note - * after the snippet for when traits::WithChart applies. - * * #include * #include "educelab/core/types/UVMap.hpp" * using educelab::UVMap; @@ -164,14 +159,17 @@ int main() * // uv.get_coordinate(srcFace, corner) now yields the packed UV for * // each wedge of `mesh`, ready for OBJ `vt` emission. * - * traits::WithChart is for MULTIPLE independent packing domains, not the - * connected components of a single atlas. Its `chart` index maps to a - * separate [0,1]^2 domain / texture page: educelab's write_obj groups - * faces into `usemtl materialN` by chart and emits one texture path per - * chart. You would use it if you partitioned the components into groups - * and ran PackCharts once per group (one atlas each), setting - * `coordinate.chart` to that group's domain index — not to tag the CCs - * that share this single packed frame. + * This table is valid for BOTH the torn mesh and the untorn (pre-split) + * mesh. split_path preserves face indices and per-face winding (it never + * re-inserts faces), and the (face, corner) keys are resolved by vertex + * identity against `mesh`'s own faces — so any winding reversal insert_face + * applies (when M is built, when components are cloned by extract, and when + * charts are merged) is absorbed rather than baked into the keys. The rule + * that makes this work: consume the UVMap against the same HalfEdgeMesh (or + * one sharing its winding) and resolve corners by identity, never by a raw + * traversal index. Caveat: insert_face's auto-rewinding means a face's + * as-built corner order may differ from the raw input face list, and the + * mesh does not record that permutation (see issue tracker / bug track B9). */ // The source mesh's 3D vertex positions are unchanged by the per-chart From 9972af4f91f4cb24824dac0be77393a4925d96a8 Mon Sep 17 00:00:00 2001 From: Seth Parker Date: Sat, 20 Jun 2026 14:13:28 -0400 Subject: [PATCH 10/20] docs(conductor): file B9 track for insert_face rewinding discrepancy (#100) insert_face auto-reverses mis-wound faces (intended, makes almost-manifold input manifold) but silently changes a face's corner order vs the raw input and records no input->as-built permutation. Surfaced during F2; downstream per-corner round-trips can silently desync. Track B9 / issue #100 capture the problem and candidate fixes (reversal flag, permutation accessor, strict mode, or docs). Co-Authored-By: Claude Opus 4.8 --- conductor/tracks.md | 1 + conductor/tracks/B9/plan.md | 28 +++++++++++++++++++ conductor/tracks/B9/spec.md | 55 +++++++++++++++++++++++++++++++++++++ 3 files changed, 84 insertions(+) create mode 100644 conductor/tracks/B9/plan.md create mode 100644 conductor/tracks/B9/spec.md diff --git a/conductor/tracks.md b/conductor/tracks.md index aa328cb..a25b1cc 100644 --- a/conductor/tracks.md +++ b/conductor/tracks.md @@ -20,6 +20,7 @@ F2 or F5–F11 — based on priority at the time of starting. | open | F11 | Implement ACVD | [#62](https://github.com/educelab/OpenABF/issues/62) | 2026-03-20 | 2026-03-20 | | open | F2 | Multi-chart UV packing | [#18](https://github.com/educelab/OpenABF/issues/18) | 2026-03-13 | 2026-06-20 | | open | B8 | LSCM area-preserving rescale of flattening output | [#98](https://github.com/educelab/OpenABF/issues/98) | 2026-06-20 | 2026-06-20 | +| open | B9 | insert_face auto-rewinding silently changes face corner order vs input (unrecorded) | [#100](https://github.com/educelab/OpenABF/issues/100) | 2026-06-20 | 2026-06-20 | ## Archived Tracks diff --git a/conductor/tracks/B9/plan.md b/conductor/tracks/B9/plan.md new file mode 100644 index 0000000..2d6aece --- /dev/null +++ b/conductor/tracks/B9/plan.md @@ -0,0 +1,28 @@ +# B9 Implementation Plan + +Potential-bug track: insert_face auto-rewinding silently changes face corner +order vs the raw input, unrecorded. See spec.md and issue #100. + +## Phase 1: Investigation & decision +- [ ] 1.1 Reproduce: build a mesh with a deliberately mis-wound face; confirm + traversal corner order differs from input and nothing records it +- [ ] 1.2 Survey internal call sites that re-insert faces and could rewind + (insert_faces, clone_face_ used by extract_connected_components, any + merge/split helpers) and note where the discrepancy can leak +- [ ] 1.3 Choose an approach (flag / permutation accessor / strict mode / docs) + and record the decision in spec.md + +## Phase 2: Tests (write first) +- [ ] 2.1 Test asserting the chosen observable behavior on a mis-wound input face +- [ ] 2.2 If a recovery API is added: test that it maps corners back to input order +- [ ] 2.3 If a strict mode is added: test that mis-wound input throws under it + +## Phase 3: Implementation +- [ ] 3.1 Implement the chosen approach +- [ ] 3.2 Document auto-rewinding + corner-order effect on insert_face/insert_faces +- [ ] 3.3 Regenerate single header (and update install list if a header is added) + +## Phase 4: Verify +- [ ] 4.1 Run `ctest` — all suites pass +- [ ] 4.2 Run clang-format on changed files +- [ ] 4.3 Confirm single-header build compiles and runs diff --git a/conductor/tracks/B9/spec.md b/conductor/tracks/B9/spec.md new file mode 100644 index 0000000..2cb8e80 --- /dev/null +++ b/conductor/tracks/B9/spec.md @@ -0,0 +1,55 @@ +# B9 — insert_face auto-rewinding silently changes face corner order vs input + +## GitHub Issue +https://github.com/educelab/OpenABF/issues/100 + +## Summary +`HalfEdgeMesh::insert_face` auto-reverses a mis-wound face to keep the mesh's +winding globally consistent (`include/OpenABF/HalfEdgeMesh.hpp:1706-1748`). The +reversal is desirable for making almost-manifold input manifold, but it +**silently changes a face's stored corner/vertex traversal order relative to the +raw input face list**, and the mesh records neither that a reversal occurred nor +the input→as-built corner permutation. + +## Why this matters +Downstream consumers build a mesh from a known face list and later read back +per-face-corner data — applying a per-wedge UV map, transferring per-corner +attributes, emitting OBJ `vt`/`f` keyed by input corner order. For any reversed +face, the half-edge traversal order no longer matches the caller's input order, +with no signal. The discrepancy is silent. + +Discovered during F2 (multi-chart UV packing, PR #99). The robust workaround is +to key per-wedge data by **vertex identity**, never by raw traversal index — +correct but non-obvious, and the trap is currently unguarded. + +## Impact +- Silent mismatch: input corner order ≠ as-built corner order for reversed faces. +- Affects any round-trip of per-corner data through a `HalfEdgeMesh`. +- No API to detect a reversed face or recover the original corner order. + +## Out of scope +- Removing the auto-rewinding behavior itself (it is intentional and useful). + +## Acceptance Criteria +- [ ] Decision recorded on the chosen approach (see below). +- [ ] If a detection/recovery API is added: it reports, for each face, whether it + was reversed at insertion and/or maps a corner index back to input order, + with unit tests on a mesh containing at least one mis-wound input face. +- [ ] `insert_face` / `insert_faces` documentation prominently describes the + auto-rewinding behavior and its effect on corner order. +- [ ] A test constructs a mesh with a deliberately mis-wound face and asserts the + documented/observable behavior (and the recovery API if added). +- [ ] Single-header regenerated; multiheader install list updated if a new header + is introduced (none expected). + +## Candidate approaches (decide in Phase 1) +1. Record per-face reversal (flag) and/or store the input→as-built corner + permutation, exposed via an accessor. +2. Provide a query mapping a corner index back to input order. +3. Opt-in "strict" insertion mode that throws on mis-wound input instead of + silently reversing (caller fixes winding explicitly). +4. Minimum viable: document the behavior loudly + ship the identity-keying + guidance (already drafted in the F2 MultiChartFlatten reference comment). + +## Dependencies +- None. Independent of F2 (PR #99), though motivated by it. From 9abcb4a939299eb50d42dcdf11e83b38036cb651 Mon Sep 17 00:00:00 2001 From: Seth Parker Date: Sat, 20 Jun 2026 14:29:25 -0400 Subject: [PATCH 11/20] docs(conductor): broaden B9 to require mapping back to original input topology MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rescope B9 beyond insert_face rewinding to the full problem: there is no recoverable path from a torn/parameterized/packed result back to the caller's original input topology. Two unrecorded identity shifts compose — insert_face corner-order reversal and split_edge seam-vertex duplication (original -> duplicate). Require B9 to provide invertible mappings for both, composing with F2's vertex_map/face_map and vertex_source/face_source so a consumer can name the original input face, corner position, and vertex for any atlas corner. Update issue #100 to match. Co-Authored-By: Claude Opus 4.8 --- conductor/tracks.md | 2 +- conductor/tracks/B9/plan.md | 38 ++++++++----- conductor/tracks/B9/spec.md | 110 +++++++++++++++++++++++------------- 3 files changed, 96 insertions(+), 54 deletions(-) diff --git a/conductor/tracks.md b/conductor/tracks.md index a25b1cc..8fa30f5 100644 --- a/conductor/tracks.md +++ b/conductor/tracks.md @@ -20,7 +20,7 @@ F2 or F5–F11 — based on priority at the time of starting. | open | F11 | Implement ACVD | [#62](https://github.com/educelab/OpenABF/issues/62) | 2026-03-20 | 2026-03-20 | | open | F2 | Multi-chart UV packing | [#18](https://github.com/educelab/OpenABF/issues/18) | 2026-03-13 | 2026-06-20 | | open | B8 | LSCM area-preserving rescale of flattening output | [#98](https://github.com/educelab/OpenABF/issues/98) | 2026-06-20 | 2026-06-20 | -| open | B9 | insert_face auto-rewinding silently changes face corner order vs input (unrecorded) | [#100](https://github.com/educelab/OpenABF/issues/100) | 2026-06-20 | 2026-06-20 | +| open | B9 | No recoverable mapping from torn/parameterized mesh back to original input topology (insert_face rewind + split_path duplication) | [#100](https://github.com/educelab/OpenABF/issues/100) | 2026-06-20 | 2026-06-20 | ## Archived Tracks diff --git a/conductor/tracks/B9/plan.md b/conductor/tracks/B9/plan.md index 2d6aece..8da1add 100644 --- a/conductor/tracks/B9/plan.md +++ b/conductor/tracks/B9/plan.md @@ -1,26 +1,34 @@ # B9 Implementation Plan -Potential-bug track: insert_face auto-rewinding silently changes face corner -order vs the raw input, unrecorded. See spec.md and issue #100. +Provide a recoverable mapping from a torn/parameterized mesh back to the +original input topology. Two unrecorded identity shifts must be invertible: +insert_face corner-order reversal, and split_edge seam-vertex duplication. +See spec.md and issue #100. ## Phase 1: Investigation & decision -- [ ] 1.1 Reproduce: build a mesh with a deliberately mis-wound face; confirm - traversal corner order differs from input and nothing records it -- [ ] 1.2 Survey internal call sites that re-insert faces and could rewind - (insert_faces, clone_face_ used by extract_connected_components, any - merge/split helpers) and note where the discrepancy can leak -- [ ] 1.3 Choose an approach (flag / permutation accessor / strict mode / docs) - and record the decision in spec.md +- [ ] 1.1 Reproduce both shifts: (a) a mis-wound input face whose as-built corner + order differs from input; (b) a torn seam whose duplicate vertex has no + recorded link to its original — confirm neither is recoverable today +- [ ] 1.2 Survey re-insertion/duplication sites: insert_face/insert_faces (reversal), + split_edge/split_path (duplication), clone_face_ (extraction) — confirm + how each affects the original→result chain +- [ ] 1.3 Choose the approach (vertex origin tracking, returned remaps, per-face + corner permutation, or a combination) and record the decision in spec.md ## Phase 2: Tests (write first) -- [ ] 2.1 Test asserting the chosen observable behavior on a mis-wound input face -- [ ] 2.2 If a recovery API is added: test that it maps corners back to input order -- [ ] 2.3 If a strict mode is added: test that mis-wound input throws under it +- [ ] 2.1 Duplicate → original vertex recovery across split_path (incl. multi-seam) +- [ ] 2.2 Reversed-face corner-order recovery for a mis-wound input face +- [ ] 2.3 Full round-trip: packed/merged atlas corner → F2 maps → torn-mesh corner + → B9 maps → original input face, input corner position, input vertex +- [ ] 2.4 Identity/no-op case: an untorn, correctly-wound mesh round-trips to identity ## Phase 3: Implementation -- [ ] 3.1 Implement the chosen approach -- [ ] 3.2 Document auto-rewinding + corner-order effect on insert_face/insert_faces -- [ ] 3.3 Regenerate single header (and update install list if a header is added) +- [ ] 3.1 Implement duplicate → original vertex mapping in split_edge/split_path +- [ ] 3.2 Implement corner-order recovery for insert_face (reversed flag/permutation) +- [ ] 3.3 Ensure the mappings survive extract_connected_components (and compose with + F2's vertex_map/face_map and vertex_source/face_source) +- [ ] 3.4 Document behavior + recovery API on insert_face/insert_faces/split_* +- [ ] 3.5 Regenerate single header (and update install list if a header is added) ## Phase 4: Verify - [ ] 4.1 Run `ctest` — all suites pass diff --git a/conductor/tracks/B9/spec.md b/conductor/tracks/B9/spec.md index 2cb8e80..f9b99b1 100644 --- a/conductor/tracks/B9/spec.md +++ b/conductor/tracks/B9/spec.md @@ -1,55 +1,89 @@ -# B9 — insert_face auto-rewinding silently changes face corner order vs input +# B9 — No recoverable mapping from a torn/parameterized mesh back to the original input topology ## GitHub Issue https://github.com/educelab/OpenABF/issues/100 ## Summary -`HalfEdgeMesh::insert_face` auto-reverses a mis-wound face to keep the mesh's -winding globally consistent (`include/OpenABF/HalfEdgeMesh.hpp:1706-1748`). The -reversal is desirable for making almost-manifold input manifold, but it -**silently changes a face's stored corner/vertex traversal order relative to the -raw input face list**, and the mesh records neither that a reversal occurred nor -the input→as-built corner permutation. +A consumer who builds a `HalfEdgeMesh` from a known face list, tears it +(`split_path`), extracts components, parameterizes, and packs them has **no +recorded path back to their original input topology**. Two distinct, +*independently unrecorded* identity shifts sit between the input and the result: + +1. **insert_face winding reversal (corner order).** `insert_face` auto-reverses + a mis-wound face to keep the mesh globally manifold + (`include/OpenABF/HalfEdgeMesh.hpp:1706-1748`). This is intended and useful, + but it permutes a face's stored corner order relative to the raw input face + list, and the permutation is not recorded. +2. **split_edge vertex duplication (vertex identity).** Tearing duplicates seam + vertices via `insert_vertex(oldStart->pos)` + (`include/OpenABF/HalfEdgeMesh.hpp:1445,1465`), appending new indices. + `split_path`/`split_edge` return `void` and record no `duplicate → original` + vertex map; the only thing the duplicate carries is a copied position. + +These **compose**: rewinding happens at build time, then seam splitting changes +which vertex identity sits at a (still position-stable) corner. Working purely +in the post-split mesh's own namespace is fine and single-level (this is what +the F2 `PackCharts`/`MultiChartFlatten` per-wedge recipe does — key by position, +resolve identity within M′). But the moment a user needs to relate results back +to **their original input** — original vertex indices and original (input) face +corner order — both shifts must be inverted, and neither is currently +recoverable. ## Why this matters -Downstream consumers build a mesh from a known face list and later read back -per-face-corner data — applying a per-wedge UV map, transferring per-corner -attributes, emitting OBJ `vt`/`f` keyed by input corner order. For any reversed -face, the half-edge traversal order no longer matches the caller's input order, -with no signal. The discrepancy is silent. +Round-tripping per-corner / per-vertex data to the *caller's own topology* is a +normal need: applying an externally authored per-wedge UV map, transferring +per-vertex attributes captured before tearing, or emitting output indexed the +way the caller supplied geometry. Today that bridge is silently impossible for +any reversed face or any seam-duplicated vertex. Discovered during F2 (PR #99). -Discovered during F2 (multi-chart UV packing, PR #99). The robust workaround is -to key per-wedge data by **vertex identity**, never by raw traversal index — -correct but non-obvious, and the trap is currently unguarded. +## Goal +Provide a **recoverable mapping from the torn/extracted result back to the +original input mesh**, covering both shifts: +- duplicate seam vertex → original input vertex, and +- as-built face corner order → raw input face corner order. -## Impact -- Silent mismatch: input corner order ≠ as-built corner order for reversed faces. -- Affects any round-trip of per-corner data through a `HalfEdgeMesh`. -- No API to detect a reversed face or recover the original corner order. +Composed with the existing `ExtractedComponent::vertex_map`/`face_map` (and +`MergedMesh::vertex_source`/`face_source` from F2), a consumer should be able to +take any corner of a packed/merged atlas and name the original input mesh's +face, input corner position, and input vertex it came from. ## Out of scope -- Removing the auto-rewinding behavior itself (it is intentional and useful). +- Removing auto-rewinding or seam duplication (both are intentional). ## Acceptance Criteria -- [ ] Decision recorded on the chosen approach (see below). -- [ ] If a detection/recovery API is added: it reports, for each face, whether it - was reversed at insertion and/or maps a corner index back to input order, - with unit tests on a mesh containing at least one mis-wound input face. -- [ ] `insert_face` / `insert_faces` documentation prominently describes the - auto-rewinding behavior and its effect on corner order. -- [ ] A test constructs a mesh with a deliberately mis-wound face and asserts the - documented/observable behavior (and the recovery API if added). -- [ ] Single-header regenerated; multiheader install list updated if a new header - is introduced (none expected). +- [ ] `split_edge`/`split_path` expose a recoverable **duplicate → original** + vertex mapping (e.g. returned map, accumulator, or an origin index stored + on duplicated vertices that survives extraction). +- [ ] `insert_face`/`insert_faces` expose whether a face was reversed and/or a + way to recover the raw input corner order (reversed flag or + input→as-built corner permutation accessor). +- [ ] A worked path demonstrates full round-trip: packed/merged atlas corner → + (F2 maps) → torn-mesh corner → (B9 maps) → original input face, input + corner position, and input vertex index. +- [ ] Unit tests on a mesh with (a) at least one deliberately mis-wound input + face and (b) at least one torn seam, asserting both inverse mappings + recover the original identities. +- [ ] `insert_face`/`split_*` documentation describes the behavior and points to + the recovery API. +- [ ] Single-header regenerated; multiheader install list updated if a new + header is introduced. ## Candidate approaches (decide in Phase 1) -1. Record per-face reversal (flag) and/or store the input→as-built corner - permutation, exposed via an accessor. -2. Provide a query mapping a corner index back to input order. -3. Opt-in "strict" insertion mode that throws on mis-wound input instead of - silently reversing (caller fixes winding explicitly). -4. Minimum viable: document the behavior loudly + ship the identity-keying - guidance (already drafted in the F2 MultiChartFlatten reference comment). +1. **Vertex origin tracking.** Store an `origin` (original input vertex index) + that is set on construction and copied to duplicates by `split_edge`, so + every vertex — original or duplicate — names its input vertex. Survives + `clone_face_`/extraction via the vertex copy path. +2. **Returned remaps.** `split_edge`/`split_path` return/accumulate + `duplicate → original` pairs; a separate per-face reversal record handles + corner order. +3. **Per-face corner permutation.** Record, per face, the input→as-built corner + order (a reversed flag suffices for triangles; a rotation+reversal for + general polygons), with an accessor. +4. **Combination + docs.** Likely (1)+(3): identity via vertex origin, corner + order via per-face reversal record, plus prominent documentation and the + identity-keying guidance already drafted in the F2 reference comment. ## Dependencies -- None. Independent of F2 (PR #99), though motivated by it. +- Independent of F2 (PR #99), but motivated by it; the F2 maps + (`vertex_map`/`face_map`, `vertex_source`/`face_source`) are the downstream + half of the chain B9 completes back to the input. From 59aaf2092731346e0a8c6622fdae3a44f1c0154f Mon Sep 17 00:00:00 2001 From: Seth Parker Date: Sat, 20 Jun 2026 16:08:56 -0400 Subject: [PATCH 12/20] feat: surround packed charts with padding on all sides (F2) PackCharts applied `padding` only as a gutter between charts, so charts on the atlas perimeter still touched the boundary (left/bottom at the origin, rightmost/topmost at the extent). For a texture atlas that lets edge charts bleed across the boundary/seam under filtering, mipmapping, or wrap addressing. Inset the whole shelf layout: the cursor starts and wraps at `pad`, and `pad` is added to the far extents, so every chart has >= padding of empty space on all four sides, including against the atlas boundary. The atlas lower corner stays at the origin. normalize now fits the padded atlas into [0,1]^2. The library default stays padding = 0 (flush); the MultiChartFlatten example sets a visible padding to demonstrate the gutter. Co-Authored-By: Claude Opus 4.8 --- examples/src/MultiChartFlatten.cpp | 5 ++++ include/OpenABF/ChartPacking.hpp | 38 +++++++++++++++++++++--------- single_include/OpenABF/OpenABF.hpp | 38 +++++++++++++++++++++--------- tests/src/TestChartPacking.cpp | 30 ++++++++++++++++++++--- 4 files changed, 86 insertions(+), 25 deletions(-) diff --git a/examples/src/MultiChartFlatten.cpp b/examples/src/MultiChartFlatten.cpp index b9e6b52..7a8e805 100644 --- a/examples/src/MultiChartFlatten.cpp +++ b/examples/src/MultiChartFlatten.cpp @@ -92,6 +92,11 @@ int main() // positions in place; the per-chart vertex_map/face_map remain valid. OpenABF::PackOptions opts; opts.normalize = true; + // Add a gutter around every chart so neighbouring charts -- and the atlas + // boundary -- don't touch. Without this, charts pack flush and texture + // filtering can bleed one chart's texels into another. `padding` is in + // absolute chart units and is applied before the normalize scaling. + opts.padding = 0.1f; auto extent = OpenABF::PackCharts(chartMeshes, opts); std::cout << "Packed atlas extent: [" << extent.min[0] << ", " << extent.min[1] << "] -> [" << extent.max[0] << ", " << extent.max[1] << "]\n"; diff --git a/include/OpenABF/ChartPacking.hpp b/include/OpenABF/ChartPacking.hpp index e5c73db..da21eb0 100644 --- a/include/OpenABF/ChartPacking.hpp +++ b/include/OpenABF/ChartPacking.hpp @@ -72,7 +72,15 @@ struct PackOptions { */ std::optional target_width{}; - /** @brief Gutter added around each chart, in chart/absolute units */ + /** + * @brief Gutter added around every chart, in chart/absolute units + * + * Applied on all four sides of each chart, including against the atlas + * boundary, so perimeter charts are inset from the returned extent by + * `padding` as well -- not merely separated from their neighbors. + * Defaults to `0` (charts laid out flush). `padding` is in absolute chart + * units and is applied before any `normalize` scaling. + */ T padding{T(0)}; }; @@ -199,19 +207,20 @@ auto PackCharts(std::vector& charts, // Shelf layout. Charts are placed left-to-right; a row wraps to a new shelf // once it would exceed targetWidth (a chart wider than targetWidth still - // gets placed alone at the start of a shelf). Every chart's bbox-min is - // mapped to a cursor position >= 0, so the packed atlas's lower corner is - // the origin. + // gets placed alone at the start of a shelf). The cursor starts at `pad` + // and wraps back to `pad`, so every chart is inset by at least `pad` from + // the atlas's lower corner; the atlas's lower corner itself stays at the + // origin. std::vector offsetX(n); std::vector offsetY(n); - auto cursorX = T(0); - auto cursorY = T(0); + auto cursorX = pad; + auto cursorY = pad; auto shelfHeight = T(0); auto atlasMaxX = T(0); auto atlasMaxY = T(0); for (const auto i : order) { - if (cursorX > T(0) and cursorX + width[i] > targetWidth) { - cursorX = T(0); + if (cursorX > pad and cursorX + width[i] > targetWidth) { + cursorX = pad; cursorY += shelfHeight + pad; shelfHeight = T(0); } @@ -223,11 +232,18 @@ auto PackCharts(std::vector& charts, shelfHeight = std::max(shelfHeight, height[i]); } + // The atlas extent includes the perimeter gutter: charts are inset by + // `pad` from the lower corner (the cursor starts at `pad`), so add `pad` + // to the far edges too. Every chart then has >= `pad` of empty space on + // all four sides, including against the atlas boundary. + const T atlasW = atlasMaxX + pad; + const T atlasH = atlasMaxY + pad; + // Optional normalization: a single global uniform scale that fits the - // packed atlas into [0,1]^2, preserving relative chart sizes. + // padded atlas into [0,1]^2, preserving relative chart sizes. auto scale = T(1); if (opts.normalize) { - const auto extentMax = std::max(atlasMaxX, atlasMaxY); + const auto extentMax = std::max(atlasW, atlasH); if (extentMax > T(0)) { scale = T(1) / extentMax; } @@ -241,7 +257,7 @@ auto PackCharts(std::vector& charts, } } - result.max = Vec{atlasMaxX * scale, atlasMaxY * scale}; + result.max = Vec{atlasW * scale, atlasH * scale}; return result; } diff --git a/single_include/OpenABF/OpenABF.hpp b/single_include/OpenABF/OpenABF.hpp index 260b363..f3900ad 100644 --- a/single_include/OpenABF/OpenABF.hpp +++ b/single_include/OpenABF/OpenABF.hpp @@ -4969,7 +4969,15 @@ struct PackOptions { */ std::optional target_width{}; - /** @brief Gutter added around each chart, in chart/absolute units */ + /** + * @brief Gutter added around every chart, in chart/absolute units + * + * Applied on all four sides of each chart, including against the atlas + * boundary, so perimeter charts are inset from the returned extent by + * `padding` as well -- not merely separated from their neighbors. + * Defaults to `0` (charts laid out flush). `padding` is in absolute chart + * units and is applied before any `normalize` scaling. + */ T padding{T(0)}; }; @@ -5096,19 +5104,20 @@ auto PackCharts(std::vector& charts, // Shelf layout. Charts are placed left-to-right; a row wraps to a new shelf // once it would exceed targetWidth (a chart wider than targetWidth still - // gets placed alone at the start of a shelf). Every chart's bbox-min is - // mapped to a cursor position >= 0, so the packed atlas's lower corner is - // the origin. + // gets placed alone at the start of a shelf). The cursor starts at `pad` + // and wraps back to `pad`, so every chart is inset by at least `pad` from + // the atlas's lower corner; the atlas's lower corner itself stays at the + // origin. std::vector offsetX(n); std::vector offsetY(n); - auto cursorX = T(0); - auto cursorY = T(0); + auto cursorX = pad; + auto cursorY = pad; auto shelfHeight = T(0); auto atlasMaxX = T(0); auto atlasMaxY = T(0); for (const auto i : order) { - if (cursorX > T(0) and cursorX + width[i] > targetWidth) { - cursorX = T(0); + if (cursorX > pad and cursorX + width[i] > targetWidth) { + cursorX = pad; cursorY += shelfHeight + pad; shelfHeight = T(0); } @@ -5120,11 +5129,18 @@ auto PackCharts(std::vector& charts, shelfHeight = std::max(shelfHeight, height[i]); } + // The atlas extent includes the perimeter gutter: charts are inset by + // `pad` from the lower corner (the cursor starts at `pad`), so add `pad` + // to the far edges too. Every chart then has >= `pad` of empty space on + // all four sides, including against the atlas boundary. + const T atlasW = atlasMaxX + pad; + const T atlasH = atlasMaxY + pad; + // Optional normalization: a single global uniform scale that fits the - // packed atlas into [0,1]^2, preserving relative chart sizes. + // padded atlas into [0,1]^2, preserving relative chart sizes. auto scale = T(1); if (opts.normalize) { - const auto extentMax = std::max(atlasMaxX, atlasMaxY); + const auto extentMax = std::max(atlasW, atlasH); if (extentMax > T(0)) { scale = T(1) / extentMax; } @@ -5138,7 +5154,7 @@ auto PackCharts(std::vector& charts, } } - result.max = Vec{atlasMaxX * scale, atlasMaxY * scale}; + result.max = Vec{atlasW * scale, atlasH * scale}; return result; } diff --git a/tests/src/TestChartPacking.cpp b/tests/src/TestChartPacking.cpp index 68aabce..6a0374c 100644 --- a/tests/src/TestChartPacking.cpp +++ b/tests/src/TestChartPacking.cpp @@ -157,14 +157,15 @@ TEST(ChartPacking, ExtentBoundsAllCharts) TEST(ChartPacking, PaddingSeparatesChartsInSingleRow) { - // Force a single row with a large target width; padding should appear as a - // gap between the two charts, so the atlas width is w0 + padding + w1. + // Force a single row with a large target width. Padding surrounds every + // chart on all sides, so the atlas width is + // pad + w0 + pad + w1 + pad = 0.5 + 1 + 0.5 + 1 + 0.5 = 3.5. std::vector charts{MakeRectChart(1.f, 1.f), MakeRectChart(1.f, 1.f)}; PackOptions opts; opts.target_width = 1000.f; opts.padding = 0.5f; auto extent = PackCharts(charts, opts); - EXPECT_NEAR(extent.max[0] - extent.min[0], 2.5f, 1e-4f); + EXPECT_NEAR(extent.max[0] - extent.min[0], 3.5f, 1e-4f); auto b0 = ChartBBox(charts[0]); auto b1 = ChartBBox(charts[1]); @@ -172,6 +173,29 @@ TEST(ChartPacking, PaddingSeparatesChartsInSingleRow) EXPECT_GE(gap, 0.5f - 1e-4f); } +TEST(ChartPacking, PaddingSurroundsChartsAtPerimeter) +{ + // Padding is a gutter on all four sides of every chart, including against + // the atlas boundary -- not just between neighbours. Use several charts so + // the layout wraps to multiple shelves, exercising both the left/bottom + // margins and the right/top margins. + const float pad = 0.5f; + std::vector charts; + for (int i = 0; i < 5; ++i) { + charts.push_back(MakeRectChart(1.f, 1.f)); + } + PackOptions opts; + opts.padding = pad; + auto extent = PackCharts(charts, opts); + for (std::size_t i = 0; i < charts.size(); ++i) { + auto b = ChartBBox(charts[i]); + EXPECT_GE(b.minx, extent.min[0] + pad - 1e-4f) << "chart " << i; + EXPECT_GE(b.miny, extent.min[1] + pad - 1e-4f) << "chart " << i; + EXPECT_LE(b.maxx, extent.max[0] - pad + 1e-4f) << "chart " << i; + EXPECT_LE(b.maxy, extent.max[1] - pad + 1e-4f) << "chart " << i; + } +} + // --- Normalize mode -------------------------------------------------------- TEST(ChartPacking, NormalizeFitsUnitSquare) From 3a4dea5fa8fc5a5782892e2f4f3c4411975c2424 Mon Sep 17 00:00:00 2001 From: Seth Parker Date: Sat, 20 Jun 2026 16:09:03 -0400 Subject: [PATCH 13/20] docs(conductor): record F2 perimeter-padding refinement (Phase 6) Document that `padding` surrounds every chart on all four sides (incl. the atlas perimeter) in Design Decision 5 and the acceptance criteria, and add Phase 6 to the plan capturing the review question and resolution. Co-Authored-By: Claude Opus 4.8 --- conductor/tracks/F2/plan.md | 20 ++++++++++++++++++++ conductor/tracks/F2/spec.md | 6 +++++- 2 files changed, 25 insertions(+), 1 deletion(-) diff --git a/conductor/tracks/F2/plan.md b/conductor/tracks/F2/plan.md index e94e121..10ccdee 100644 --- a/conductor/tracks/F2/plan.md +++ b/conductor/tracks/F2/plan.md @@ -47,3 +47,23 @@ provenance maps so merged → chart → M' composition keeps working. - [x] 5.3 Wire into OpenABF.hpp + multiheader install list; regenerate single header - [x] 5.4 Switch MultiChartFlatten example to use MergeMeshes - [x] 5.5 Verify: full ctest, single-header build, install-test all pass + +## Phase 6: Perimeter padding (added during review) +Rationale: review question "shouldn't we add padding around the packed +charts?". The original layout applied `padding` only as a gutter *between* +charts — perimeter charts still touched the atlas boundary (left/bottom at the +origin, rightmost/topmost at the extent). For a texture atlas this lets edge +charts bleed across the boundary/seam under filtering, mipmapping, or wrap +addressing. Resolution (user-confirmed): inset the whole layout so `padding` +surrounds every chart on all four sides; keep the library default `padding = 0` +and instead set a visible padding in the example. +- [x] 6.1 Tests first: assert padding insets charts from the atlas perimeter + (new PaddingSurroundsChartsAtPerimeter); update single-row extent + expectation (pad + w0 + pad + w1 + pad) +- [x] 6.2 Implement perimeter inset: cursor starts/wraps at `pad`; add `pad` to + far extents; normalize fits the padded atlas into [0,1]² +- [x] 6.3 Update header docs (padding surrounds charts; atlas lower corner stays + at origin) + spec Decision 5 / acceptance criteria +- [x] 6.4 Set a visible `padding` in the MultiChartFlatten example +- [x] 6.5 Regenerate single header; verify full ctest, example run, single-header + build, clang-format all pass diff --git a/conductor/tracks/F2/spec.md b/conductor/tracks/F2/spec.md index 7508e1c..927ebf2 100644 --- a/conductor/tracks/F2/spec.md +++ b/conductor/tracks/F2/spec.md @@ -74,7 +74,8 @@ and emit per-corner `vt` entries, build atlases, etc., from there. struct PackOptions { bool normalize = false; // fit packed atlas into [0,1]^2 std::optional target_width{}; // overrides sqrt-area heuristic - T padding = T(0); // per-chart gutter, absolute units + T padding = T(0); // gutter on all sides of every chart + // (incl. atlas perimeter), abs units }; struct PackResult { Vec min, max; }; // packed atlas extent @@ -98,6 +99,9 @@ and emit per-corner `vt` entries, build atlases, etc., from there. - [ ] Shelf-packing with the ~square target-width heuristic, overridable. - [ ] No charts' bounding boxes overlap (padding respected); the packed set is contained in the returned extent (and in `[0,1]²` when normalized). +- [ ] `padding` surrounds every chart on all four sides, including against the + atlas boundary (perimeter charts are inset from the extent by `padding`, + not just separated from neighbors). - [ ] Edge cases handled per Design Decision 6. - [ ] Header documents the vertex-identity per-wedge recipe (Decision 2). - [ ] Tests: synthetic 2D charts for deterministic geometric assertions plus From 21a6ed105a1ea16ab846f5b28ffd4baabd307255 Mon Sep 17 00:00:00 2001 From: Seth Parker Date: Mon, 22 Jun 2026 07:08:22 +0200 Subject: [PATCH 14/20] Fix BFS bugs --- conductor/tracks/B9/plan.md | 55 ++++++----- conductor/tracks/B9/spec.md | 147 ++++++++++++++++------------- include/OpenABF/HalfEdgeMesh.hpp | 18 ++-- single_include/OpenABF/OpenABF.hpp | 18 ++-- 4 files changed, 139 insertions(+), 99 deletions(-) diff --git a/conductor/tracks/B9/plan.md b/conductor/tracks/B9/plan.md index 8da1add..48a9a3f 100644 --- a/conductor/tracks/B9/plan.md +++ b/conductor/tracks/B9/plan.md @@ -1,34 +1,43 @@ # B9 Implementation Plan -Provide a recoverable mapping from a torn/parameterized mesh back to the -original input topology. Two unrecorded identity shifts must be invertible: -insert_face corner-order reversal, and split_edge seam-vertex duplication. -See spec.md and issue #100. +Make every torn/extracted vertex name its pre-HEM (input) vertex, so a consumer +who wraps their own mesh in a HalfEdgeMesh to flatten can build a per-wedge UV +map that lands back on their pre-HEM mesh — seam-duplicated corners included. +The `insert_face` winding reversal is absorbed by identity-keyed corner +resolution and needs no separate record (see spec.md and issue #100). ## Phase 1: Investigation & decision -- [ ] 1.1 Reproduce both shifts: (a) a mis-wound input face whose as-built corner - order differs from input; (b) a torn seam whose duplicate vertex has no - recorded link to its original — confirm neither is recoverable today -- [ ] 1.2 Survey re-insertion/duplication sites: insert_face/insert_faces (reversal), - split_edge/split_path (duplication), clone_face_ (extraction) — confirm - how each affects the original→result chain -- [ ] 1.3 Choose the approach (vertex origin tracking, returned remaps, per-face - corner permutation, or a combination) and record the decision in spec.md +- [ ] 1.1 Reproduce the gap: tear a seam, confirm the duplicate vertex has no + recorded link to its pre-HEM origin and that an atlas seam corner + cannot be resolved to the consumer's `(face, corner)` today +- [ ] 1.2 Confirm the already-recoverable hops to keep scope tight: face index + (insert_faces order + split_path never re-inserts), non-seam vertex + index (insert_vertices order + tearing only appends), and that + identity-keyed corner resolution absorbs the winding reversal +- [ ] 1.3 Survey duplication/copy sites: split_edge/split_path (duplication) and + clone_face_ (extraction vertex copy) — confirm an `origin` field rides + the copy path; choose vertex-origin tracking vs returned remaps and + record the decision in spec.md ## Phase 2: Tests (write first) -- [ ] 2.1 Duplicate → original vertex recovery across split_path (incl. multi-seam) -- [ ] 2.2 Reversed-face corner-order recovery for a mis-wound input face -- [ ] 2.3 Full round-trip: packed/merged atlas corner → F2 maps → torn-mesh corner - → B9 maps → original input face, input corner position, input vertex -- [ ] 2.4 Identity/no-op case: an untorn, correctly-wound mesh round-trips to identity +- [ ] 2.1 Duplicate → pre-HEM vertex recovery across split_path (incl. multi-seam) +- [ ] 2.2 Full round-trip: packed/merged atlas corner → F2 maps → torn-HEM corner + → B9 mapping → pre-HEM face, corner, vertex; assert a seam corner lands + on the correct pre-HEM (face, corner) by identity +- [ ] 2.3 Mis-wound input face: confirm the winding reversal is absorbed — the + round-trip lands the correct corner with no corner-order record +- [ ] 2.4 Identity/no-op case: an untorn, correctly-wound mesh round-trips to the + identity mapping ## Phase 3: Implementation -- [ ] 3.1 Implement duplicate → original vertex mapping in split_edge/split_path -- [ ] 3.2 Implement corner-order recovery for insert_face (reversed flag/permutation) -- [ ] 3.3 Ensure the mappings survive extract_connected_components (and compose with - F2's vertex_map/face_map and vertex_source/face_source) -- [ ] 3.4 Document behavior + recovery API on insert_face/insert_faces/split_* -- [ ] 3.5 Regenerate single header (and update install list if a header is added) +- [ ] 3.1 Implement duplicate → pre-HEM vertex mapping in split_edge/split_path + (origin set at construction, copied to duplicates) +- [ ] 3.2 Ensure the mapping survives extract_connected_components (clone_face_ + vertex copy) and composes with F2's vertex_map/vertex_source +- [ ] 3.3 Document behavior + recovery API on split_*; update the + MultiChartFlatten.cpp reference comment to key corners against the + pre-HEM mesh using the vertex origin +- [ ] 3.4 Regenerate single header (and update install list if a header is added) ## Phase 4: Verify - [ ] 4.1 Run `ctest` — all suites pass diff --git a/conductor/tracks/B9/spec.md b/conductor/tracks/B9/spec.md index f9b99b1..4d16eec 100644 --- a/conductor/tracks/B9/spec.md +++ b/conductor/tracks/B9/spec.md @@ -1,89 +1,108 @@ -# B9 — No recoverable mapping from a torn/parameterized mesh back to the original input topology +# B9 — No recoverable mapping from torn seam-duplicate vertices back to the consumer's pre-HEM mesh ## GitHub Issue https://github.com/educelab/OpenABF/issues/100 +## Use case +Downstream consumers do **not** use `HalfEdgeMesh` as their primary mesh type. +They keep their own mesh (the "raw" / pre-HEM mesh — their own vertex array and +face list) and *wrap* it in a `HalfEdgeMesh` only to flatten: build the HEM with +`insert_vertices`/`insert_faces`, tear it (`split_path`), extract components, +parameterize, and pack/merge. They then build a **per-wedge UV map** from the +packed result and expect to apply it back onto their **pre-HEM mesh**, keyed by +their own `(face, corner)` identities. + ## Summary -A consumer who builds a `HalfEdgeMesh` from a known face list, tears it -(`split_path`), extracts components, parameterizes, and packs them has **no -recorded path back to their original input topology**. Two distinct, -*independently unrecorded* identity shifts sit between the input and the result: +That round-trip is `atlas corner → (F2 maps) → torn-HEM corner → pre-HEM mesh`. +Three of the four hops already round-trip cleanly: -1. **insert_face winding reversal (corner order).** `insert_face` auto-reverses - a mis-wound face to keep the mesh globally manifold - (`include/OpenABF/HalfEdgeMesh.hpp:1706-1748`). This is intended and useful, - but it permutes a face's stored corner order relative to the raw input face - list, and the permutation is not recorded. -2. **split_edge vertex duplication (vertex identity).** Tearing duplicates seam - vertices via `insert_vertex(oldStart->pos)` - (`include/OpenABF/HalfEdgeMesh.hpp:1445,1465`), appending new indices. - `split_path`/`split_edge` return `void` and record no `duplicate → original` - vertex map; the only thing the duplicate carries is a copied position. +- **Face index.** `insert_faces` inserts faces in order + (`include/OpenABF/HalfEdgeMesh.hpp:1040-1049`) and `split_path` only tears + edges (it never re-inserts faces), so pre-HEM face `i` == HEM face `i`, and + F2's `face_map`/`face_source` carry it back. +- **Corner order.** `insert_face` may auto-reverse a mis-wound face to keep the + mesh manifold (`include/OpenABF/HalfEdgeMesh.hpp:1706-1748`), permuting a + face's stored corner order relative to the raw input. This is **not** a + blocker: the documented per-wedge recipe resolves corners by *vertex + identity* against the consumer's own face, never by raw traversal index + (`include/OpenABF/ChartPacking.hpp:108-123`), so the reversal is **absorbed**, + not inverted. (See "Out of scope".) +- **Non-seam vertices.** `insert_vertices` preserves order and tearing only + *appends* new vertices, so a non-duplicated HEM vertex keeps its pre-HEM + index; F2's `vertex_map`/`vertex_source` carry it back. -These **compose**: rewinding happens at build time, then seam splitting changes -which vertex identity sits at a (still position-stable) corner. Working purely -in the post-split mesh's own namespace is fine and single-level (this is what -the F2 `PackCharts`/`MultiChartFlatten` per-wedge recipe does — key by position, -resolve identity within M′). But the moment a user needs to relate results back -to **their original input** — original vertex indices and original (input) face -corner order — both shifts must be inverted, and neither is currently -recoverable. +The one hop that breaks is **seam-vertex identity**. Tearing duplicates seam +vertices via `insert_vertex(oldStart->pos)` +(`include/OpenABF/HalfEdgeMesh.hpp:1445,1465`), appending a new index whose only +link to its origin is a copied position. `split_path`/`split_edge` return `void` +and record no `duplicate → original` vertex map. So a torn-HEM corner that lands +on a seam duplicate **cannot be expressed in the consumer's pre-HEM vertex +namespace** — and identity-based corner resolution against the consumer's own +face (the very mechanism that absorbs the winding reversal) fails for exactly +those corners. ## Why this matters -Round-tripping per-corner / per-vertex data to the *caller's own topology* is a -normal need: applying an externally authored per-wedge UV map, transferring -per-vertex attributes captured before tearing, or emitting output indexed the -way the caller supplied geometry. Today that bridge is silently impossible for -any reversed face or any seam-duplicated vertex. Discovered during F2 (PR #99). +The per-wedge UV recipe in `MultiChartFlatten.cpp` works entirely *within* the +torn HEM's own namespace, which is self-consistent. But the consumer's goal is +to land UVs on their **pre-HEM** mesh. For every non-seam corner that already +works; for a seam corner it silently cannot, because the duplicate has no +recoverable pre-HEM vertex identity. The same gap blocks scattering per-vertex +attributes captured on the pre-HEM mesh and emitting output indexed by the +consumer's original vertices. Discovered during F2 (PR #99). ## Goal -Provide a **recoverable mapping from the torn/extracted result back to the -original input mesh**, covering both shifts: -- duplicate seam vertex → original input vertex, and -- as-built face corner order → raw input face corner order. - -Composed with the existing `ExtractedComponent::vertex_map`/`face_map` (and -`MergedMesh::vertex_source`/`face_source` from F2), a consumer should be able to -take any corner of a packed/merged atlas and name the original input mesh's -face, input corner position, and input vertex it came from. +Make every torn/extracted vertex — original or seam duplicate — name its +**pre-HEM (input) vertex**, so the documented identity-keyed per-wedge recipe +resolves corners against the consumer's own faces for *all* corners, seams +included. Composed with F2's `vertex_map`/`face_map` and +`vertex_source`/`face_source`, a consumer can take any corner of a packed/merged +atlas and name the pre-HEM face, corner, and vertex it came from. ## Out of scope - Removing auto-rewinding or seam duplication (both are intentional). +- Recording the `insert_face` corner-order permutation / a reversed flag. + Identity-keyed corner resolution (`ChartPacking.hpp:108-123`) absorbs the + reversal, so it does **not** need to be inverted for this use case. The corner + order is recovered implicitly once seam-duplicate vertices carry their pre-HEM + identity (this AC), by locating each vertex within the consumer's own face. ## Acceptance Criteria -- [ ] `split_edge`/`split_path` expose a recoverable **duplicate → original** - vertex mapping (e.g. returned map, accumulator, or an origin index stored - on duplicated vertices that survives extraction). -- [ ] `insert_face`/`insert_faces` expose whether a face was reversed and/or a - way to recover the raw input corner order (reversed flag or - input→as-built corner permutation accessor). -- [ ] A worked path demonstrates full round-trip: packed/merged atlas corner → - (F2 maps) → torn-mesh corner → (B9 maps) → original input face, input - corner position, and input vertex index. -- [ ] Unit tests on a mesh with (a) at least one deliberately mis-wound input - face and (b) at least one torn seam, asserting both inverse mappings - recover the original identities. -- [ ] `insert_face`/`split_*` documentation describes the behavior and points to - the recovery API. +- [ ] `split_edge`/`split_path` expose a recoverable **duplicate → pre-HEM** + vertex mapping (e.g. an `origin` index stored on every vertex, set at + construction and copied to duplicates, that survives extraction; or a + returned/accumulated remap). +- [ ] The mapping survives `extract_connected_components` (`clone_face_` copies + vertices, so an `origin` field rides along) and composes with F2's + `vertex_map`/`vertex_source`. +- [ ] A worked path demonstrates the full round-trip: packed/merged atlas corner + → (F2 maps) → torn-HEM corner → (B9 mapping) → pre-HEM face, corner, and + vertex index — resolving the corner by vertex identity against the + consumer's own face, with seam-duplicate corners resolving correctly. +- [ ] Unit tests on a mesh with at least one torn seam assert the duplicate → + pre-HEM vertex mapping recovers the original identity, and that a seam + corner of the packed atlas lands on the correct pre-HEM `(face, corner)`. + Include a mis-wound input face to confirm the winding reversal is absorbed + (the round-trip still lands the right corner without a corner-order record). +- [ ] An untorn, correctly-wound mesh round-trips to the identity mapping. +- [ ] `split_*` documentation describes the behavior and points to the recovery + API; the `MultiChartFlatten.cpp` reference comment is updated to key + corners against the pre-HEM mesh using the new vertex origin. - [ ] Single-header regenerated; multiheader install list updated if a new header is introduced. ## Candidate approaches (decide in Phase 1) -1. **Vertex origin tracking.** Store an `origin` (original input vertex index) - that is set on construction and copied to duplicates by `split_edge`, so - every vertex — original or duplicate — names its input vertex. Survives - `clone_face_`/extraction via the vertex copy path. +1. **Vertex origin tracking (primary).** Store an `origin` (pre-HEM vertex index) + set on construction and copied to duplicates by `split_edge`, so every vertex + — original or duplicate — names its pre-HEM vertex. Survives + `clone_face_`/extraction via the vertex copy path. With this, corners are + located by identity in the consumer's own face and the winding reversal needs + no separate record. 2. **Returned remaps.** `split_edge`/`split_path` return/accumulate - `duplicate → original` pairs; a separate per-face reversal record handles - corner order. -3. **Per-face corner permutation.** Record, per face, the input→as-built corner - order (a reversed flag suffices for triangles; a rotation+reversal for - general polygons), with an accessor. -4. **Combination + docs.** Likely (1)+(3): identity via vertex origin, corner - order via per-face reversal record, plus prominent documentation and the - identity-keying guidance already drafted in the F2 reference comment. + `duplicate → original` pairs. Lighter-weight but does not survive extraction + without the caller threading it through, and does not give a uniform + "every vertex names its input vertex" accessor. ## Dependencies - Independent of F2 (PR #99), but motivated by it; the F2 maps (`vertex_map`/`face_map`, `vertex_source`/`face_source`) are the downstream - half of the chain B9 completes back to the input. + half of the chain B9 completes back to the pre-HEM mesh. diff --git a/include/OpenABF/HalfEdgeMesh.hpp b/include/OpenABF/HalfEdgeMesh.hpp index 5e58baa..10ae5e9 100644 --- a/include/OpenABF/HalfEdgeMesh.hpp +++ b/include/OpenABF/HalfEdgeMesh.hpp @@ -1218,19 +1218,22 @@ class HalfEdgeMesh continue; } - // Start a new connected component + // Start a new connected component. Mark faces visited as they are + // enqueued (not when dequeued) so each face is enqueued and + // expanded exactly once; otherwise a face is re-enqueued once per + // incident interior edge and the traversal blows up on large meshes. + visited[f->idx] = true; queue.push(f); while (not queue.empty()) { // Get the top of the queue auto p = queue.front(); queue.pop(); - // Mark as visited - visited[p->idx] = true; // Add the neighbor faces to the queue for (const auto& e : *p) { if (not e->pair->is_boundary()) { auto n = e->pair->face; if (not visited[n->idx]) { + visited[n->idx] = true; queue.push(n); } } @@ -1258,15 +1261,17 @@ class HalfEdgeMesh continue; } - // Start a new connected component + // Start a new connected component. Mark faces visited as they are + // enqueued (not when dequeued) so each face is enqueued and + // expanded exactly once; otherwise a face is re-enqueued once per + // incident interior edge and the traversal blows up on large meshes. current.clear(); + visited[f->idx] = true; queue.push(f); while (not queue.empty()) { // Get the top of the queue auto p = queue.front(); queue.pop(); - // Mark as visited - visited[p->idx] = true; // Add to this connected component current.emplace_back(p); // Add the neighbor faces to the queue @@ -1274,6 +1279,7 @@ class HalfEdgeMesh if (not e->pair->is_boundary()) { auto n = e->pair->face; if (not visited[n->idx]) { + visited[n->idx] = true; queue.push(n); } } diff --git a/single_include/OpenABF/OpenABF.hpp b/single_include/OpenABF/OpenABF.hpp index f3900ad..2895694 100644 --- a/single_include/OpenABF/OpenABF.hpp +++ b/single_include/OpenABF/OpenABF.hpp @@ -1662,19 +1662,22 @@ class HalfEdgeMesh continue; } - // Start a new connected component + // Start a new connected component. Mark faces visited as they are + // enqueued (not when dequeued) so each face is enqueued and + // expanded exactly once; otherwise a face is re-enqueued once per + // incident interior edge and the traversal blows up on large meshes. + visited[f->idx] = true; queue.push(f); while (not queue.empty()) { // Get the top of the queue auto p = queue.front(); queue.pop(); - // Mark as visited - visited[p->idx] = true; // Add the neighbor faces to the queue for (const auto& e : *p) { if (not e->pair->is_boundary()) { auto n = e->pair->face; if (not visited[n->idx]) { + visited[n->idx] = true; queue.push(n); } } @@ -1702,15 +1705,17 @@ class HalfEdgeMesh continue; } - // Start a new connected component + // Start a new connected component. Mark faces visited as they are + // enqueued (not when dequeued) so each face is enqueued and + // expanded exactly once; otherwise a face is re-enqueued once per + // incident interior edge and the traversal blows up on large meshes. current.clear(); + visited[f->idx] = true; queue.push(f); while (not queue.empty()) { // Get the top of the queue auto p = queue.front(); queue.pop(); - // Mark as visited - visited[p->idx] = true; // Add to this connected component current.emplace_back(p); // Add the neighbor faces to the queue @@ -1718,6 +1723,7 @@ class HalfEdgeMesh if (not e->pair->is_boundary()) { auto n = e->pair->face; if (not visited[n->idx]) { + visited[n->idx] = true; queue.push(n); } } From 10133282414c1922547fabe6418df1acf99795ee Mon Sep 17 00:00:00 2001 From: Seth Parker Date: Mon, 22 Jun 2026 07:34:51 +0200 Subject: [PATCH 15/20] refactor(packing): address PR #99 review feedback - Add static Vec::Dimensions; drop the detail::VecDimensions trait - Template PackResult on the mesh's vertex Vec type instead of Vec - Use std::minmax_element with structured bindings for chart bounds - MergeMeshes: batch faces through insert_faces so update_boundary runs - PackCharts: add minimize_bounding_box option (default on) that rotates each chart to its minimum-area orientation and stands its long axis vertical to match the tallest-first shelf strategy Co-Authored-By: Claude Opus 4.8 --- include/OpenABF/ChartPacking.hpp | 215 ++++++++++++++++++++++----- include/OpenABF/MeshMerge.hpp | 8 +- include/OpenABF/Vec.hpp | 3 + single_include/OpenABF/OpenABF.hpp | 227 +++++++++++++++++++++++------ tests/src/TestChartPacking.cpp | 70 ++++++++- 5 files changed, 440 insertions(+), 83 deletions(-) diff --git a/include/OpenABF/ChartPacking.hpp b/include/OpenABF/ChartPacking.hpp index da21eb0..64de550 100644 --- a/include/OpenABF/ChartPacking.hpp +++ b/include/OpenABF/ChartPacking.hpp @@ -19,30 +19,147 @@ limitations under the License. #pragma once #include +#include #include #include #include #include #include #include +#include #include #include -#include "OpenABF/Vec.hpp" - namespace OpenABF { namespace detail { -/** @brief Deduce the dimensionality of a Vec type */ -template -struct VecDimensions; +/** + * @brief Rotate a chart within its UV plane so its axis-aligned bounding box + * has minimum area, with the larger extent vertical + * + * The minimum-area enclosing rectangle of a planar point set always has one + * edge collinear with an edge of the set's convex hull, so it suffices to test + * the orientation induced by each hull edge. The chart is then stood on its + * long axis (larger extent vertical) so it aligns with PackCharts's + * tallest-first shelf strategy. Vertex positions are rotated in place about the + * origin; only the first two components are touched. Rotation preserves + * topology and vertex identity, so any back-maps remain valid. + * + * @tparam MeshType A HalfEdgeMesh specialization + */ +template +void MinimizeChartBoundingBox(const typename MeshType::Pointer& chart) +{ + using T = typename MeshType::type; + using Point = std::array; -template -struct VecDimensions> { - static constexpr std::size_t value = N; -}; + // Gather the 2D point set. + std::vector pts; + pts.reserve(chart->num_vertices()); + for (const auto& v : chart->vertices()) { + pts.push_back({v->pos[0], v->pos[1]}); + } + + // Convex hull via Andrew's monotone chain. Fewer than three unique points + // means a point or a segment, for which no rotation reduces the area. + std::sort(pts.begin(), pts.end()); + pts.erase(std::unique(pts.begin(), pts.end()), pts.end()); + const std::size_t m = pts.size(); + if (m < 3) { + return; + } + auto crossZ = [](const Point& o, const Point& a, const Point& b) -> T { + return (a[0] - o[0]) * (b[1] - o[1]) - (a[1] - o[1]) * (b[0] - o[0]); + }; + std::vector hull(2 * m); + std::size_t k = 0; + for (std::size_t i = 0; i < m; ++i) { + while (k >= 2 and crossZ(hull[k - 2], hull[k - 1], pts[i]) <= T(0)) { + --k; + } + hull[k++] = pts[i]; + } + for (std::size_t i = m - 1, t = k + 1; i > 0; --i) { + while (k >= t and crossZ(hull[k - 2], hull[k - 1], pts[i - 1]) <= T(0)) { + --k; + } + hull[k++] = pts[i - 1]; + } + hull.resize(k - 1); // drop the duplicated start point + const std::size_t h = hull.size(); + if (h < 3) { + return; + } + + // Bounding-box dimensions {width, height} of the hull after rotating every + // point by R(-theta), where (c, s) = (cos theta, sin theta). + auto boxFor = [&](T c, T s) -> Point { + auto mnX = std::numeric_limits::max(); + auto mnY = std::numeric_limits::max(); + auto mxX = std::numeric_limits::lowest(); + auto mxY = std::numeric_limits::lowest(); + for (const auto& p : hull) { + const auto rx = c * p[0] + s * p[1]; + const auto ry = -s * p[0] + c * p[1]; + mnX = std::min(mnX, rx); + mnY = std::min(mnY, ry); + mxX = std::max(mxX, rx); + mxY = std::max(mxY, ry); + } + return {mxX - mnX, mxY - mnY}; + }; + + // Seed with the current (unrotated) box so we only rotate on a strict + // area improvement. + auto bestCos = T(1); + auto bestSin = T(0); + auto bestBox = boxFor(T(1), T(0)); + auto bestArea = bestBox[0] * bestBox[1]; + for (std::size_t i = 0; i < h; ++i) { + const auto& p0 = hull[i]; + const auto& p1 = hull[(i + 1) % h]; + const auto ex = p1[0] - p0[0]; + const auto ey = p1[1] - p0[1]; + const auto len = std::sqrt(ex * ex + ey * ey); + if (len <= T(0)) { + continue; + } + // Align this hull edge with the x-axis (theta = atan2(ey, ex)). + const auto c = ex / len; + const auto s = ey / len; + const auto box = boxFor(c, s); + const auto area = box[0] * box[1]; + if (area < bestArea) { + bestArea = area; + bestCos = c; + bestSin = s; + bestBox = box; + } + } + + // Stand the chart on its long axis: the packer sorts tallest-first and + // fills horizontal shelves, so the larger extent should be vertical. If the + // min-area box is wider than tall, compose an extra 90-degree rotation + // (R90 * R(-theta), where R90 maps (x, y) -> (-y, x)). + if (bestBox[0] > bestBox[1]) { + const auto c = bestCos; + const auto s = bestSin; + bestCos = s; + bestSin = -c; + } + + // Apply the chosen rotation to every vertex in place (skip the identity). + if (bestCos != T(1) or bestSin != T(0)) { + for (const auto& v : chart->vertices()) { + const auto x = v->pos[0]; + const auto y = v->pos[1]; + v->pos[0] = bestCos * x + bestSin * y; + v->pos[1] = -bestSin * x + bestCos * y; + } + } +} } // namespace detail /** @@ -52,6 +169,19 @@ struct VecDimensions> { */ template struct PackOptions { + /** + * @brief Rotate each chart in-plane to minimize its bounding-box area + * + * When `true` (default), each chart is rotated within its UV plane before + * layout so its axis-aligned bounding box has minimum area, then stood on + * its long axis (larger extent vertical) to match the tallest-first shelf + * strategy. Shelf packing works on axis-aligned boxes, so tightening and + * consistently orienting each box lets charts nest more densely. The + * rotation is applied in place and preserves topology and vertex identity, + * so any back-maps a caller holds remain valid. + */ + bool minimize_bounding_box{true}; + /** * @brief Fit the packed atlas into the unit square `[0,1]^2` * @@ -87,14 +217,17 @@ struct PackOptions { /** * @brief The bounding box of the packed atlas * - * @tparam T Floating-point scalar type + * `min`/`max` use the same vector type as the input meshes' vertex positions; + * only the first two (`u`, `v`) components are meaningful. + * + * @tparam VecType The vertex position vector type of the packed meshes */ -template +template struct PackResult { /** @brief Lower corner of the packed atlas */ - Vec min; + VecType min; /** @brief Upper corner of the packed atlas */ - Vec max; + VecType max; }; /** @@ -103,10 +236,10 @@ struct PackResult { * Lays out a list of already-parameterized charts (2D meshes whose vertex * `pos` holds `{u, v, ...}`) into a single shared frame using shelf packing, * so that no two charts' bounding boxes overlap. Operates purely on geometry: - * each chart's vertex positions are translated (and, when `normalize` is set, - * uniformly scaled) **in place**. Topology, vertex indices, and face indices - * are untouched, so any `ExtractedComponent` back-maps a caller holds remain - * valid after packing. + * each chart's vertex positions are rotated (when `minimize_bounding_box` is + * set), translated, and (when `normalize` is set) uniformly scaled **in + * place**. Topology, vertex indices, and face indices are untouched, so any + * `ExtractedComponent` back-maps a caller holds remain valid after packing. * * @par Scaling * By default charts keep their absolute scale and are only translated; the @@ -133,7 +266,9 @@ struct PackResult { * @par Complexity * `O(n log n)` in the number of charts `n` (dominated by the height sort) plus * `O(V)` in the total vertex count `V` (two passes: one to measure bounding - * boxes, one to apply the transform). Memory overhead is `O(n)`. + * boxes, one to apply the transform). With `minimize_bounding_box`, each chart + * additionally costs an `O(v log v)` convex hull plus an `O(h v)` orientation + * search over its `h` hull edges. Memory overhead is `O(n)`. * * @tparam MeshType A HalfEdgeMesh specialization * @param charts Charts to pack; each chart's vertex positions are modified @@ -144,16 +279,17 @@ struct PackResult { * vertices. */ template -auto PackCharts(std::vector& charts, - const PackOptions& opts = - PackOptions{}) -> PackResult +auto PackCharts( + std::vector& charts, + const PackOptions& opts = PackOptions{}) + -> PackResult().pos)>> { using T = typename MeshType::type; - static_assert( - detail::VecDimensions().pos)>::value >= 2, - "PackCharts requires mesh vertices with at least 2 position dimensions"); + using VecType = std::decay_t().pos)>; + static_assert(VecType::Dimensions >= 2, + "PackCharts requires mesh vertices with at least 2 position dimensions"); - PackResult result{Vec{T(0), T(0)}, Vec{T(0), T(0)}}; + PackResult result{}; if (charts.empty()) { return result; } @@ -169,20 +305,19 @@ auto PackCharts(std::vector& charts, if (not chart or chart->num_vertices() == 0) { throw std::invalid_argument("PackCharts: chart is null or has no vertices"); } - auto mnX = std::numeric_limits::max(); - auto mnY = std::numeric_limits::max(); - auto mxX = std::numeric_limits::lowest(); - auto mxY = std::numeric_limits::lowest(); - for (const auto& v : chart->vertices()) { - mnX = std::min(mnX, v->pos[0]); - mnY = std::min(mnY, v->pos[1]); - mxX = std::max(mxX, v->pos[0]); - mxY = std::max(mxY, v->pos[1]); + // Tighten the chart's bounding box by rotating it in-plane first. + if (opts.minimize_bounding_box) { + detail::MinimizeChartBoundingBox(chart); } - minX[i] = mnX; - minY[i] = mnY; - width[i] = mxX - mnX; - height[i] = mxY - mnY; + const auto& verts = chart->vertices(); + const auto cmpX = [](const auto& a, const auto& b) { return a->pos[0] < b->pos[0]; }; + const auto cmpY = [](const auto& a, const auto& b) { return a->pos[1] < b->pos[1]; }; + const auto [xlo, xhi] = std::minmax_element(verts.begin(), verts.end(), cmpX); + const auto [ylo, yhi] = std::minmax_element(verts.begin(), verts.end(), cmpY); + minX[i] = (*xlo)->pos[0]; + minY[i] = (*ylo)->pos[1]; + width[i] = (*xhi)->pos[0] - (*xlo)->pos[0]; + height[i] = (*yhi)->pos[1] - (*ylo)->pos[1]; } // Place taller charts first so shelves pack tightly. @@ -257,7 +392,9 @@ auto PackCharts(std::vector& charts, } } - result.max = Vec{atlasW * scale, atlasH * scale}; + // result.min is value-initialized to the origin; only u/v of max are set. + result.max[0] = atlasW * scale; + result.max[1] = atlasH * scale; return result; } diff --git a/include/OpenABF/MeshMerge.hpp b/include/OpenABF/MeshMerge.hpp index f1efd53..7de0b30 100644 --- a/include/OpenABF/MeshMerge.hpp +++ b/include/OpenABF/MeshMerge.hpp @@ -74,6 +74,11 @@ auto MergeMeshes(const std::vector& meshes) -> Merge MergedMesh result{MeshType::New(), {}, {}}; auto& out = result.mesh; + // Gather every face's (offset) vertex indices so they can be inserted in a + // single insert_faces() call, which rebuilds the mesh boundary once at the + // end via update_boundary(). Inserting faces one at a time with + // insert_face() would leave the boundary stale. + std::vector> faces; for (std::size_t ci = 0; ci < meshes.size(); ++ci) { const auto& src = meshes[ci]; if (not src or src->num_vertices() == 0) { @@ -93,10 +98,11 @@ auto MergeMeshes(const std::vector& meshes) -> Merge for (const auto& edge : *face) { idxs.push_back(edge->vertex->idx + offset); } - out->insert_face(idxs); + faces.push_back(std::move(idxs)); result.face_source.emplace_back(ci, face->idx); } } + out->insert_faces(faces); return result; } diff --git a/include/OpenABF/Vec.hpp b/include/OpenABF/Vec.hpp index c40ac8d..e4db265 100644 --- a/include/OpenABF/Vec.hpp +++ b/include/OpenABF/Vec.hpp @@ -23,6 +23,9 @@ class Vec using Container = std::array; public: + /** @brief Number of dimensions (elements) in the vector */ + static constexpr std::size_t Dimensions = Dims; + /** Element type */ using value_type = T; /** Vector size type */ diff --git a/single_include/OpenABF/OpenABF.hpp b/single_include/OpenABF/OpenABF.hpp index 2895694..0a96fac 100644 --- a/single_include/OpenABF/OpenABF.hpp +++ b/single_include/OpenABF/OpenABF.hpp @@ -175,6 +175,9 @@ class Vec using Container = std::array; public: + /** @brief Number of dimensions (elements) in the vector */ + static constexpr std::size_t Dimensions = Dims; + /** Element type */ using value_type = T; /** Vector size type */ @@ -4921,31 +4924,147 @@ limitations under the License. #include +#include #include #include #include #include #include #include +#include #include #include -// #include "OpenABF/Vec.hpp" - - namespace OpenABF { namespace detail { -/** @brief Deduce the dimensionality of a Vec type */ -template -struct VecDimensions; +/** + * @brief Rotate a chart within its UV plane so its axis-aligned bounding box + * has minimum area, with the larger extent vertical + * + * The minimum-area enclosing rectangle of a planar point set always has one + * edge collinear with an edge of the set's convex hull, so it suffices to test + * the orientation induced by each hull edge. The chart is then stood on its + * long axis (larger extent vertical) so it aligns with PackCharts's + * tallest-first shelf strategy. Vertex positions are rotated in place about the + * origin; only the first two components are touched. Rotation preserves + * topology and vertex identity, so any back-maps remain valid. + * + * @tparam MeshType A HalfEdgeMesh specialization + */ +template +void MinimizeChartBoundingBox(const typename MeshType::Pointer& chart) +{ + using T = typename MeshType::type; + using Point = std::array; -template -struct VecDimensions> { - static constexpr std::size_t value = N; -}; + // Gather the 2D point set. + std::vector pts; + pts.reserve(chart->num_vertices()); + for (const auto& v : chart->vertices()) { + pts.push_back({v->pos[0], v->pos[1]}); + } + + // Convex hull via Andrew's monotone chain. Fewer than three unique points + // means a point or a segment, for which no rotation reduces the area. + std::sort(pts.begin(), pts.end()); + pts.erase(std::unique(pts.begin(), pts.end()), pts.end()); + const std::size_t m = pts.size(); + if (m < 3) { + return; + } + auto crossZ = [](const Point& o, const Point& a, const Point& b) -> T { + return (a[0] - o[0]) * (b[1] - o[1]) - (a[1] - o[1]) * (b[0] - o[0]); + }; + std::vector hull(2 * m); + std::size_t k = 0; + for (std::size_t i = 0; i < m; ++i) { + while (k >= 2 and crossZ(hull[k - 2], hull[k - 1], pts[i]) <= T(0)) { + --k; + } + hull[k++] = pts[i]; + } + for (std::size_t i = m - 1, t = k + 1; i > 0; --i) { + while (k >= t and crossZ(hull[k - 2], hull[k - 1], pts[i - 1]) <= T(0)) { + --k; + } + hull[k++] = pts[i - 1]; + } + hull.resize(k - 1); // drop the duplicated start point + const std::size_t h = hull.size(); + if (h < 3) { + return; + } + + // Bounding-box dimensions {width, height} of the hull after rotating every + // point by R(-theta), where (c, s) = (cos theta, sin theta). + auto boxFor = [&](T c, T s) -> Point { + auto mnX = std::numeric_limits::max(); + auto mnY = std::numeric_limits::max(); + auto mxX = std::numeric_limits::lowest(); + auto mxY = std::numeric_limits::lowest(); + for (const auto& p : hull) { + const auto rx = c * p[0] + s * p[1]; + const auto ry = -s * p[0] + c * p[1]; + mnX = std::min(mnX, rx); + mnY = std::min(mnY, ry); + mxX = std::max(mxX, rx); + mxY = std::max(mxY, ry); + } + return {mxX - mnX, mxY - mnY}; + }; + + // Seed with the current (unrotated) box so we only rotate on a strict + // area improvement. + auto bestCos = T(1); + auto bestSin = T(0); + auto bestBox = boxFor(T(1), T(0)); + auto bestArea = bestBox[0] * bestBox[1]; + for (std::size_t i = 0; i < h; ++i) { + const auto& p0 = hull[i]; + const auto& p1 = hull[(i + 1) % h]; + const auto ex = p1[0] - p0[0]; + const auto ey = p1[1] - p0[1]; + const auto len = std::sqrt(ex * ex + ey * ey); + if (len <= T(0)) { + continue; + } + // Align this hull edge with the x-axis (theta = atan2(ey, ex)). + const auto c = ex / len; + const auto s = ey / len; + const auto box = boxFor(c, s); + const auto area = box[0] * box[1]; + if (area < bestArea) { + bestArea = area; + bestCos = c; + bestSin = s; + bestBox = box; + } + } + + // Stand the chart on its long axis: the packer sorts tallest-first and + // fills horizontal shelves, so the larger extent should be vertical. If the + // min-area box is wider than tall, compose an extra 90-degree rotation + // (R90 * R(-theta), where R90 maps (x, y) -> (-y, x)). + if (bestBox[0] > bestBox[1]) { + const auto c = bestCos; + const auto s = bestSin; + bestCos = s; + bestSin = -c; + } + + // Apply the chosen rotation to every vertex in place (skip the identity). + if (bestCos != T(1) or bestSin != T(0)) { + for (const auto& v : chart->vertices()) { + const auto x = v->pos[0]; + const auto y = v->pos[1]; + v->pos[0] = bestCos * x + bestSin * y; + v->pos[1] = -bestSin * x + bestCos * y; + } + } +} } // namespace detail /** @@ -4955,6 +5074,19 @@ struct VecDimensions> { */ template struct PackOptions { + /** + * @brief Rotate each chart in-plane to minimize its bounding-box area + * + * When `true` (default), each chart is rotated within its UV plane before + * layout so its axis-aligned bounding box has minimum area, then stood on + * its long axis (larger extent vertical) to match the tallest-first shelf + * strategy. Shelf packing works on axis-aligned boxes, so tightening and + * consistently orienting each box lets charts nest more densely. The + * rotation is applied in place and preserves topology and vertex identity, + * so any back-maps a caller holds remain valid. + */ + bool minimize_bounding_box{true}; + /** * @brief Fit the packed atlas into the unit square `[0,1]^2` * @@ -4990,14 +5122,17 @@ struct PackOptions { /** * @brief The bounding box of the packed atlas * - * @tparam T Floating-point scalar type + * `min`/`max` use the same vector type as the input meshes' vertex positions; + * only the first two (`u`, `v`) components are meaningful. + * + * @tparam VecType The vertex position vector type of the packed meshes */ -template +template struct PackResult { /** @brief Lower corner of the packed atlas */ - Vec min; + VecType min; /** @brief Upper corner of the packed atlas */ - Vec max; + VecType max; }; /** @@ -5006,10 +5141,10 @@ struct PackResult { * Lays out a list of already-parameterized charts (2D meshes whose vertex * `pos` holds `{u, v, ...}`) into a single shared frame using shelf packing, * so that no two charts' bounding boxes overlap. Operates purely on geometry: - * each chart's vertex positions are translated (and, when `normalize` is set, - * uniformly scaled) **in place**. Topology, vertex indices, and face indices - * are untouched, so any `ExtractedComponent` back-maps a caller holds remain - * valid after packing. + * each chart's vertex positions are rotated (when `minimize_bounding_box` is + * set), translated, and (when `normalize` is set) uniformly scaled **in + * place**. Topology, vertex indices, and face indices are untouched, so any + * `ExtractedComponent` back-maps a caller holds remain valid after packing. * * @par Scaling * By default charts keep their absolute scale and are only translated; the @@ -5036,7 +5171,9 @@ struct PackResult { * @par Complexity * `O(n log n)` in the number of charts `n` (dominated by the height sort) plus * `O(V)` in the total vertex count `V` (two passes: one to measure bounding - * boxes, one to apply the transform). Memory overhead is `O(n)`. + * boxes, one to apply the transform). With `minimize_bounding_box`, each chart + * additionally costs an `O(v log v)` convex hull plus an `O(h v)` orientation + * search over its `h` hull edges. Memory overhead is `O(n)`. * * @tparam MeshType A HalfEdgeMesh specialization * @param charts Charts to pack; each chart's vertex positions are modified @@ -5047,16 +5184,17 @@ struct PackResult { * vertices. */ template -auto PackCharts(std::vector& charts, - const PackOptions& opts = - PackOptions{}) -> PackResult +auto PackCharts( + std::vector& charts, + const PackOptions& opts = PackOptions{}) + -> PackResult().pos)>> { using T = typename MeshType::type; - static_assert( - detail::VecDimensions().pos)>::value >= 2, - "PackCharts requires mesh vertices with at least 2 position dimensions"); + using VecType = std::decay_t().pos)>; + static_assert(VecType::Dimensions >= 2, + "PackCharts requires mesh vertices with at least 2 position dimensions"); - PackResult result{Vec{T(0), T(0)}, Vec{T(0), T(0)}}; + PackResult result{}; if (charts.empty()) { return result; } @@ -5072,20 +5210,19 @@ auto PackCharts(std::vector& charts, if (not chart or chart->num_vertices() == 0) { throw std::invalid_argument("PackCharts: chart is null or has no vertices"); } - auto mnX = std::numeric_limits::max(); - auto mnY = std::numeric_limits::max(); - auto mxX = std::numeric_limits::lowest(); - auto mxY = std::numeric_limits::lowest(); - for (const auto& v : chart->vertices()) { - mnX = std::min(mnX, v->pos[0]); - mnY = std::min(mnY, v->pos[1]); - mxX = std::max(mxX, v->pos[0]); - mxY = std::max(mxY, v->pos[1]); + // Tighten the chart's bounding box by rotating it in-plane first. + if (opts.minimize_bounding_box) { + detail::MinimizeChartBoundingBox(chart); } - minX[i] = mnX; - minY[i] = mnY; - width[i] = mxX - mnX; - height[i] = mxY - mnY; + const auto& verts = chart->vertices(); + const auto cmpX = [](const auto& a, const auto& b) { return a->pos[0] < b->pos[0]; }; + const auto cmpY = [](const auto& a, const auto& b) { return a->pos[1] < b->pos[1]; }; + const auto [xlo, xhi] = std::minmax_element(verts.begin(), verts.end(), cmpX); + const auto [ylo, yhi] = std::minmax_element(verts.begin(), verts.end(), cmpY); + minX[i] = (*xlo)->pos[0]; + minY[i] = (*ylo)->pos[1]; + width[i] = (*xhi)->pos[0] - (*xlo)->pos[0]; + height[i] = (*yhi)->pos[1] - (*ylo)->pos[1]; } // Place taller charts first so shelves pack tightly. @@ -5160,7 +5297,9 @@ auto PackCharts(std::vector& charts, } } - result.max = Vec{atlasW * scale, atlasH * scale}; + // result.min is value-initialized to the origin; only u/v of max are set. + result.max[0] = atlasW * scale; + result.max[1] = atlasH * scale; return result; } @@ -5243,6 +5382,11 @@ auto MergeMeshes(const std::vector& meshes) -> Merge MergedMesh result{MeshType::New(), {}, {}}; auto& out = result.mesh; + // Gather every face's (offset) vertex indices so they can be inserted in a + // single insert_faces() call, which rebuilds the mesh boundary once at the + // end via update_boundary(). Inserting faces one at a time with + // insert_face() would leave the boundary stale. + std::vector> faces; for (std::size_t ci = 0; ci < meshes.size(); ++ci) { const auto& src = meshes[ci]; if (not src or src->num_vertices() == 0) { @@ -5262,10 +5406,11 @@ auto MergeMeshes(const std::vector& meshes) -> Merge for (const auto& edge : *face) { idxs.push_back(edge->vertex->idx + offset); } - out->insert_face(idxs); + faces.push_back(std::move(idxs)); result.face_source.emplace_back(ci, face->idx); } } + out->insert_faces(faces); return result; } diff --git a/tests/src/TestChartPacking.cpp b/tests/src/TestChartPacking.cpp index 6a0374c..8ffc1af 100644 --- a/tests/src/TestChartPacking.cpp +++ b/tests/src/TestChartPacking.cpp @@ -1,4 +1,6 @@ #include +#include +#include #include #include #include @@ -25,6 +27,21 @@ auto MakeRectChart(float w, float h) -> Mesh::Pointer return m; } +/** @brief Build a w x h rectangle chart rotated by `theta` radians */ +auto MakeRotatedRectChart(float w, float h, float theta) -> Mesh::Pointer +{ + const float c = std::cos(theta); + const float s = std::sin(theta); + auto rot = [&](float x, float y) -> std::array { + return {c * x - s * y, s * x + c * y, 0.f}; + }; + std::vector> verts{rot(0.f, 0.f), rot(w, 0.f), rot(w, h), rot(0.f, h)}; + auto m = Mesh::New(); + m->insert_vertices(verts); + m->insert_faces({{0, 1, 2}, {0, 2, 3}}); + return m; +} + struct BBox { float minx{std::numeric_limits::max()}; float miny{std::numeric_limits::max()}; @@ -95,8 +112,11 @@ TEST(ChartPacking, ZeroAreaChartIsPlacedWithoutThrowing) TEST(ChartPacking, SingleChartMapsMinToOrigin) { + // Disable rotation so this test exercises translation/layout in isolation. std::vector charts{MakeRectChart(3.f, 2.f)}; - auto extent = PackCharts(charts); + PackOptions opts; + opts.minimize_bounding_box = false; + auto extent = PackCharts(charts, opts); auto b = ChartBBox(charts[0]); EXPECT_FLOAT_EQ(b.minx, 0.f); EXPECT_FLOAT_EQ(b.miny, 0.f); @@ -108,13 +128,17 @@ TEST(ChartPacking, SingleChartMapsMinToOrigin) TEST(ChartPacking, AbsoluteModePreservesChartSizes) { + // Disable rotation so the bounding boxes stay in their input orientation; + // this test is about absolute mode not rescaling charts. std::vector ws{1.f, 2.f, 0.5f, 3.f}; std::vector hs{1.f, 1.5f, 2.f, 0.7f}; std::vector charts; for (std::size_t i = 0; i < ws.size(); ++i) { charts.push_back(MakeRectChart(ws[i], hs[i])); } - PackCharts(charts); + PackOptions opts; + opts.minimize_bounding_box = false; + PackCharts(charts, opts); for (std::size_t i = 0; i < charts.size(); ++i) { auto b = ChartBBox(charts[i]); EXPECT_FLOAT_EQ(b.width(), ws[i]) << "chart " << i; @@ -196,6 +220,48 @@ TEST(ChartPacking, PaddingSurroundsChartsAtPerimeter) } } +// --- Bounding-box minimization (rotation) ---------------------------------- + +TEST(ChartPacking, MinimizeBoundingBoxTightensRotatedChart) +{ + // A 4x1 rectangle rotated off-axis has a loose axis-aligned bounding box. + // With minimize_bounding_box (the default), PackCharts rotates it back so + // the packed box collapses to the rectangle's true 4x1 area, standing the + // long axis vertical. + std::vector charts{MakeRotatedRectChart(4.f, 1.f, 0.6f)}; + PackCharts(charts); + auto b = ChartBBox(charts[0]); + EXPECT_NEAR(b.width() * b.height(), 4.f, 1e-2f); + // The long axis (~4) must be vertical; the short axis (~1) horizontal. + EXPECT_NEAR(b.height(), 4.f, 1e-2f); + EXPECT_NEAR(b.width(), 1.f, 1e-2f); +} + +TEST(ChartPacking, MinimizeBoundingBoxStandsWideChartUpright) +{ + // An axis-aligned chart that is already minimum-area but wider than tall is + // rotated 90 degrees so its long axis is vertical, matching the + // tallest-first shelf strategy. + std::vector charts{MakeRectChart(4.f, 1.f)}; + PackCharts(charts); + auto b = ChartBBox(charts[0]); + EXPECT_NEAR(b.width() * b.height(), 4.f, 1e-3f); + EXPECT_NEAR(b.height(), 4.f, 1e-3f); + EXPECT_NEAR(b.width(), 1.f, 1e-3f); +} + +TEST(ChartPacking, MinimizeBoundingBoxCanBeDisabled) +{ + // With minimization off, the off-axis rectangle keeps its loose bounding + // box, whose area is well above the rectangle's true 4x1 area. + std::vector charts{MakeRotatedRectChart(4.f, 1.f, 0.6f)}; + PackOptions opts; + opts.minimize_bounding_box = false; + PackCharts(charts, opts); + auto b = ChartBBox(charts[0]); + EXPECT_GT(b.width() * b.height(), 5.f); +} + // --- Normalize mode -------------------------------------------------------- TEST(ChartPacking, NormalizeFitsUnitSquare) From 073eb43bc2829c8fda1cf5a6567c257f2b4512ab Mon Sep 17 00:00:00 2001 From: Seth Parker Date: Fri, 24 Jul 2026 15:54:05 -0400 Subject: [PATCH 16/20] test(mesh): regression-test BFS visit-once; record F2 phases 7-8 Adds HalfEdgeMesh.ConnectedComponentsVisitEachFaceOnce, the missing guard for the BFS fix in 21a6ed1. Pre-fix, an 18-face grid reported 38 faces in its component list and extract_connected_components() threw "non-manifold face"; the test was confirmed Red against develop's traversal. Filed as #103. Also brings the F2 track docs back in line with the branch: - spec Decision 7 records in-plane bounding-box minimization (default on) and Decisions 1/5/6 are updated for the rotation step, the PackResult VecType, and Vec::Dimensions; acceptance criteria extended to match - plan gains Phase 7 (bbox minimization) and Phase 8 (the out-of-scope BFS fix this PR carries), so 33/33 no longer under-describes the work - MultiChartFlatten documents why chart flattening is wrapped in try/catch: unsolvable chart topologies are expected and must not abort the atlas - ChartPacking complexity note corrected to O(h^2) for the orientation search - gitignore openabf_example_*.obj/.ply so examples stop dirtying the tree Co-Authored-By: Claude Opus 5 --- .gitignore | 4 +++ conductor/tracks.md | 3 +- conductor/tracks/F2/plan.md | 37 ++++++++++++++++++++++++ conductor/tracks/F2/spec.md | 45 ++++++++++++++++++++++++------ examples/src/MultiChartFlatten.cpp | 27 +++++++++++++----- include/OpenABF/ChartPacking.hpp | 5 ++-- single_include/OpenABF/OpenABF.hpp | 5 ++-- tests/src/TestHalfEdgeMesh.cpp | 43 ++++++++++++++++++++++++++++ 8 files changed, 149 insertions(+), 20 deletions(-) diff --git a/.gitignore b/.gitignore index ac171e1..468f630 100644 --- a/.gitignore +++ b/.gitignore @@ -45,5 +45,9 @@ venv/* docs/latex/ docs/html/ +# Example programs write their output meshes to the working directory +openabf_example_*.obj +openabf_example_*.ply + # CLion cmake-build*/ diff --git a/conductor/tracks.md b/conductor/tracks.md index 8fa30f5..15210a0 100644 --- a/conductor/tracks.md +++ b/conductor/tracks.md @@ -18,7 +18,8 @@ F2 or F5–F11 — based on priority at the time of starting. | open | F9 | Implement OpenBLAS/LAPACK Backend | [#57](https://github.com/educelab/OpenABF/issues/57) | 2026-03-20 | 2026-03-20 | | open | F10 | Implement Ceres Solver Backend | [#58](https://github.com/educelab/OpenABF/issues/58) | 2026-03-20 | 2026-03-20 | | open | F11 | Implement ACVD | [#62](https://github.com/educelab/OpenABF/issues/62) | 2026-03-20 | 2026-03-20 | -| open | F2 | Multi-chart UV packing | [#18](https://github.com/educelab/OpenABF/issues/18) | 2026-03-13 | 2026-06-20 | +| open | F2 | Multi-chart UV packing | [#18](https://github.com/educelab/OpenABF/issues/18) | 2026-03-13 | 2026-07-24 | +| open | B10 | `extract_connected_components` throws on meshes with mutually-adjacent interior faces (BFS marked visited on dequeue) — fix + regression test carried by PR #99 (F2 Phase 8) | [#103](https://github.com/educelab/OpenABF/issues/103) | 2026-07-24 | 2026-07-24 | | open | B8 | LSCM area-preserving rescale of flattening output | [#98](https://github.com/educelab/OpenABF/issues/98) | 2026-06-20 | 2026-06-20 | | open | B9 | No recoverable mapping from torn/parameterized mesh back to original input topology (insert_face rewind + split_path duplication) | [#100](https://github.com/educelab/OpenABF/issues/100) | 2026-06-20 | 2026-06-20 | diff --git a/conductor/tracks/F2/plan.md b/conductor/tracks/F2/plan.md index 10ccdee..cbb790c 100644 --- a/conductor/tracks/F2/plan.md +++ b/conductor/tracks/F2/plan.md @@ -67,3 +67,40 @@ and instead set a visible padding in the example. - [x] 6.4 Set a visible `padding` in the MultiChartFlatten example - [x] 6.5 Regenerate single header; verify full ctest, example run, single-header build, clang-format all pass + +## Phase 7: In-plane bounding-box minimization (added during review) +Rationale: shelf packing works on axis-aligned boxes, so a chart that arrives +rotated wastes atlas area equal to the slack between its AABB and its true +footprint. Rotating each chart to its min-area box (and standing it on its long +axis) makes the tallest-first shelf strategy far more effective. Default is +`true` — see spec.md Decision 7 for why that default is safe. +- [x] 7.1 Tests first: MinimizeBoundingBoxTightensRotatedChart (off-axis 4x1 + rectangle collapses to its true 4x1 area, long axis vertical), + MinimizeBoundingBoxStandsWideChartUpright, MinimizeBoundingBoxCanBeDisabled +- [x] 7.2 Implement `detail::MinimizeChartBoundingBox` — monotone-chain convex + hull, orientation search over hull edges, 90-degree stand-up composition, + in-place rotation of the first two position components +- [x] 7.3 Add `PackOptions::minimize_bounding_box` (default true) and call it + during the bbox measurement pass +- [x] 7.4 Add `Vec::Dimensions` so PackCharts can static_assert on the position + type; document rotation + revised complexity in the header +- [x] 7.5 Record the feature in spec.md (Decision 1/5/6/7 + acceptance criteria) +- [x] 7.6 Update pre-existing translation/layout tests to disable rotation so + they still exercise layout in isolation +- [x] 7.7 Regenerate single header; full ctest green + +## Phase 8: HalfEdgeMesh BFS fix (out of scope, carried by this PR) +Rationale: found while running the packing pipeline on a real partitioned mesh. +`num_connected_components()` and `connected_components()` marked a face visited +when it was *dequeued*, so a face adjacent to two or more still-queued faces was +enqueued once per incident interior edge. This is a correctness bug, not just +wasted work: the duplicate entries reach `connected_components()`'s face lists, +and `extract_connected_components()` then clones the same face twice and throws +"Attempted to add non-manifold face". Measured on a 4x4 grid: 38 faces reported +for an 18-face mesh. Kept in this PR (user-confirmed) because F2's pipeline is +unusable without it; tracked separately as its own bug issue. +- [x] 8.1 Mark faces visited at enqueue time in both BFS traversals +- [x] 8.2 Regression test `ConnectedComponentsVisitEachFaceOnce`: component face + list has no duplicates and matches num_faces(); extracted face_map is a + permutation of [0, num_faces). Verified Red against develop's BFS. +- [x] 8.3 File the bug issue (#103, track B10) and link it from tracks.md diff --git a/conductor/tracks/F2/spec.md b/conductor/tracks/F2/spec.md index 927ebf2..4bfa3d0 100644 --- a/conductor/tracks/F2/spec.md +++ b/conductor/tracks/F2/spec.md @@ -33,9 +33,10 @@ and emit per-corner `vt` entries, build atlases, etc., from there. ## Design Decisions (resolved 2026-06-20) 1. **Geometry-only, in-place.** `PackCharts` mutates each chart's - `Vertex::pos` (translate + optional scale). Packing touches no topology - and no vertex/face indices, so any `ExtractedComponent` back-maps the - caller holds remain valid afterward. + `Vertex::pos` (optional in-plane rotation, then translate, then optional + scale — see Decision 7). Packing touches no topology and no vertex/face + indices, so any `ExtractedComponent` back-maps the caller holds remain + valid afterward. 2. **Per-wedge iteration is feasible and must key on vertex identity.** A `Face` stores no vertex list — corners come from walking the half-edge @@ -70,23 +71,45 @@ and emit per-corner `vt` entries, build atlases, etc., from there. 5. **API shape.** ```cpp - template + template struct PackOptions { + bool minimize_bounding_box = true; // in-plane min-area rotation (Decision 7) bool normalize = false; // fit packed atlas into [0,1]^2 std::optional target_width{}; // overrides sqrt-area heuristic T padding = T(0); // gutter on all sides of every chart // (incl. atlas perimeter), abs units }; - struct PackResult { Vec min, max; }; // packed atlas extent + template + struct PackResult { VecType min, max; }; // packed atlas extent PackResult PackCharts(std::vector& charts, - PackOptions opts = {}); + const PackOptions& opts = {}); ``` + `PackResult` carries the meshes' own position vector type rather than a + `Vec`, so the extent needs no conversion to compare against vertex + positions; only the `u`/`v` components are meaningful. 6. **Degenerate input.** Empty list → return empty extent (no-op). Zero-area / single-point charts are placed by their (flat) bbox. Null pointer or a - chart with zero vertices → `throw std::invalid_argument`. - `static_assert(MeshType::Dim >= 2)`. + chart with zero vertices → `throw std::invalid_argument`. The dimension + guard is `static_assert(VecType::Dimensions >= 2)` on the vertex position + type; `Vec` gained a public `static constexpr std::size_t Dimensions` to + support it (there is no `MeshType::Dim`). + +7. **In-plane bounding-box minimization (added during review, default on).** + Shelf packing reasons about axis-aligned boxes, so a chart that arrives at + an arbitrary orientation wastes atlas area proportional to how far its + AABB exceeds its true footprint. Before layout, each chart is rotated + within its UV plane so its AABB has minimum area (convex hull, then test + the orientation induced by each hull edge — the min-area enclosing + rectangle always has an edge collinear with a hull edge), then stood on its + long axis so the larger extent is vertical, matching the tallest-first + shelf strategy. Rotation is in place about the origin and touches only the + first two components; it preserves topology and vertex identity, so + back-maps stay valid and the wedge recipe in Decision 2 is unaffected. + Default is `true`: denser atlases are the reason a caller packs at all, and + UV orientation carries no meaning that packing must preserve. Callers who + need the charts' input orientation set `minimize_bounding_box = false`. ## Acceptance Criteria - [ ] `PackCharts` free function accepts a list of parameterized @@ -102,6 +125,12 @@ and emit per-corner `vt` entries, build atlases, etc., from there. - [ ] `padding` surrounds every chart on all four sides, including against the atlas boundary (perimeter charts are inset from the extent by `padding`, not just separated from neighbors). +- [ ] `minimize_bounding_box` (default on) rotates each chart in-plane to its + minimum-area bounding box and stands its long axis vertical; it is + disableable, and rotation preserves topology and vertex identity so the + wedge recipe still holds (Decision 7). +- [ ] The example demonstrates handling a chart whose topology the solver + cannot flatten (`SolverException`) without aborting the whole atlas. - [ ] Edge cases handled per Design Decision 6. - [ ] Header documents the vertex-identity per-wedge recipe (Decision 2). - [ ] Tests: synthetic 2D charts for deterministic geometric assertions plus diff --git a/examples/src/MultiChartFlatten.cpp b/examples/src/MultiChartFlatten.cpp index 7a8e805..19809ce 100644 --- a/examples/src/MultiChartFlatten.cpp +++ b/examples/src/MultiChartFlatten.cpp @@ -4,11 +4,15 @@ * # Multi-chart flattening demo * * Builds a mesh with multiple connected components (here, a 3x3 grid torn - * along two seams), extracts each component as an independent mesh, runs + * down its center line), extracts each component as an independent mesh, runs * ABF++ + LSCM on each, packs the flattened charts into a shared coordinate * frame with OpenABF::PackCharts, and writes the packed atlas to a single * .obj file. * + * Charts are flattened defensively: not every chart topology is solvable, so a + * chart that throws OpenABF::SolverException is reported and skipped rather + * than aborting the atlas. + * * The original mesh is never modified — the extracted sub-meshes own their * own vertices and per-edge state, and each is parameterized in isolation * before being placed into the common frame. @@ -73,12 +77,21 @@ int main() std::size_t iters{0}; float grad{OpenABF::INF}; - ABF::Compute(cc.mesh, iters, grad); - LSCM::Compute(cc.mesh); - chartMeshes.push_back(cc.mesh); - - std::cout << "Chart " << i << ": " << cc.mesh->num_vertices() << " vertices, " - << cc.mesh->num_faces() << " faces, " << iters << " ABF++ iters\n"; + // Not every chart topology is solvable, and one unsolvable chart should + // not cost you the whole atlas. Flatten each chart inside its own + // try/catch and pack whatever succeeded: a chart that throws is simply + // left out of `chartMeshes`, so its faces get no UVs while the rest of + // the atlas is still produced. + try { + ABF::Compute(cc.mesh, iters, grad); + LSCM::Compute(cc.mesh); + std::cout << "Chart " << i << ": " << cc.mesh->num_vertices() << " vertices, " + << cc.mesh->num_faces() << " faces, " << iters << " ABF++ iters\n"; + chartMeshes.push_back(cc.mesh); + } catch (const OpenABF::SolverException& e) { + std::cout << "Chart " << i << ": skipped, could not be flattened (" << e.what() + << ")\n"; + } // cc.vertex_map[chart_idx] -> original vertex idx // cc.face_map[chart_idx] -> original face idx diff --git a/include/OpenABF/ChartPacking.hpp b/include/OpenABF/ChartPacking.hpp index 64de550..299d64f 100644 --- a/include/OpenABF/ChartPacking.hpp +++ b/include/OpenABF/ChartPacking.hpp @@ -267,8 +267,9 @@ struct PackResult { * `O(n log n)` in the number of charts `n` (dominated by the height sort) plus * `O(V)` in the total vertex count `V` (two passes: one to measure bounding * boxes, one to apply the transform). With `minimize_bounding_box`, each chart - * additionally costs an `O(v log v)` convex hull plus an `O(h v)` orientation - * search over its `h` hull edges. Memory overhead is `O(n)`. + * additionally costs an `O(v log v)` convex hull over its `v` vertices plus an + * `O(h^2)` orientation search, which measures the hull's `h` points once per + * hull edge. Memory overhead is `O(n)`. * * @tparam MeshType A HalfEdgeMesh specialization * @param charts Charts to pack; each chart's vertex positions are modified diff --git a/single_include/OpenABF/OpenABF.hpp b/single_include/OpenABF/OpenABF.hpp index 0a96fac..3c842e4 100644 --- a/single_include/OpenABF/OpenABF.hpp +++ b/single_include/OpenABF/OpenABF.hpp @@ -5172,8 +5172,9 @@ struct PackResult { * `O(n log n)` in the number of charts `n` (dominated by the height sort) plus * `O(V)` in the total vertex count `V` (two passes: one to measure bounding * boxes, one to apply the transform). With `minimize_bounding_box`, each chart - * additionally costs an `O(v log v)` convex hull plus an `O(h v)` orientation - * search over its `h` hull edges. Memory overhead is `O(n)`. + * additionally costs an `O(v log v)` convex hull over its `v` vertices plus an + * `O(h^2)` orientation search, which measures the hull's `h` points once per + * hull edge. Memory overhead is `O(n)`. * * @tparam MeshType A HalfEdgeMesh specialization * @param charts Charts to pack; each chart's vertex positions are modified diff --git a/tests/src/TestHalfEdgeMesh.cpp b/tests/src/TestHalfEdgeMesh.cpp index 8cd3fc2..deef8d7 100644 --- a/tests/src/TestHalfEdgeMesh.cpp +++ b/tests/src/TestHalfEdgeMesh.cpp @@ -1,5 +1,9 @@ #include +#include +#include +#include + #include "OpenABF/OpenABF.hpp" #include "Utils.hpp" @@ -67,6 +71,45 @@ TEST(HalfEdgeMesh, ConnectedComponents) EXPECT_EQ(ccs.size(), mesh->num_connected_components()); } +TEST(HalfEdgeMesh, ConnectedComponentsVisitEachFaceOnce) +{ + // Regression: the BFS used to mark a face visited when it was *dequeued* + // rather than when it was enqueued. A face adjacent to two or more faces + // that are themselves still in the queue was therefore pushed once per + // incident interior edge, so it appeared multiple times in the component's + // face list (and the traversal did redundant work that compounds on large + // meshes). Any mesh with mutually-adjacent neighbors exercises this; a + // grid's interior faces do. + const auto mesh = ConstructGrid(4, 4); + ASSERT_EQ(mesh->num_faces(), 18); + ASSERT_EQ(mesh->num_connected_components(), 1); + + const auto ccs = mesh->connected_components(); + ASSERT_EQ(ccs.size(), 1); + + std::vector faceIdxs; + for (const auto& f : ccs[0]) { + faceIdxs.push_back(f->idx); + } + std::sort(faceIdxs.begin(), faceIdxs.end()); + const auto last = std::unique(faceIdxs.begin(), faceIdxs.end()); + EXPECT_EQ(last, faceIdxs.end()) << "component face list contains duplicate faces"; + EXPECT_EQ(faceIdxs.size(), mesh->num_faces()); + + // The same duplication reached extract_connected_components, which clones + // every face in the list into the sub-mesh. + const auto components = mesh->extract_connected_components(); + ASSERT_EQ(components.size(), 1); + EXPECT_EQ(components[0].mesh->num_faces(), mesh->num_faces()); + EXPECT_EQ(components[0].face_map.size(), mesh->num_faces()); + + auto faceMap = components[0].face_map; + std::sort(faceMap.begin(), faceMap.end()); + std::vector expected(mesh->num_faces()); + std::iota(expected.begin(), expected.end(), std::size_t{0}); + EXPECT_EQ(faceMap, expected); +} + TEST(HalfEdgeMesh, CheckWindingOrder) { // Build basic mesh From 542d46806964473e9781e6c82cd901df69bc3087 Mon Sep 17 00:00:00 2001 From: Seth Parker Date: Fri, 24 Jul 2026 16:00:05 -0400 Subject: [PATCH 17/20] docs(conductor): register P7 for PackCharts shelf-wrap padding mismatch Files #104: the sqrt-area target width is computed from padded chart areas while the wrap test measures the unpadded right edge against a cursor already inset by `pad`, so a row of k charts is budgeted k gutters when it spends k+1. Two 1x2 charts with padding 0.1 wrap into a 1.2x4.3 column instead of an almost-square 2.3x2.2 atlas. Marked blocked_by #18 on GitHub. Co-Authored-By: Claude Opus 5 --- conductor/tracks.md | 1 + 1 file changed, 1 insertion(+) diff --git a/conductor/tracks.md b/conductor/tracks.md index 15210a0..79d375f 100644 --- a/conductor/tracks.md +++ b/conductor/tracks.md @@ -20,6 +20,7 @@ F2 or F5–F11 — based on priority at the time of starting. | open | F11 | Implement ACVD | [#62](https://github.com/educelab/OpenABF/issues/62) | 2026-03-20 | 2026-03-20 | | open | F2 | Multi-chart UV packing | [#18](https://github.com/educelab/OpenABF/issues/18) | 2026-03-13 | 2026-07-24 | | open | B10 | `extract_connected_components` throws on meshes with mutually-adjacent interior faces (BFS marked visited on dequeue) — fix + regression test carried by PR #99 (F2 Phase 8) | [#103](https://github.com/educelab/OpenABF/issues/103) | 2026-07-24 | 2026-07-24 | +| open | P7 | PackCharts shelf-wrap test and sqrt-area target width disagree on padding, causing premature wraps and narrow atlases (blocked on F2/PR #99 merging) | [#104](https://github.com/educelab/OpenABF/issues/104) | 2026-07-24 | 2026-07-24 | | open | B8 | LSCM area-preserving rescale of flattening output | [#98](https://github.com/educelab/OpenABF/issues/98) | 2026-06-20 | 2026-06-20 | | open | B9 | No recoverable mapping from torn/parameterized mesh back to original input topology (insert_face rewind + split_path duplication) | [#100](https://github.com/educelab/OpenABF/issues/100) | 2026-06-20 | 2026-06-20 | From faf22d770bdd88daea6c05747bc3bbe6ab1b4e48 Mon Sep 17 00:00:00 2001 From: Seth Parker Date: Fri, 24 Jul 2026 16:17:26 -0400 Subject: [PATCH 18/20] docs(conductor): add P7 spec and plan for PackCharts squareness-based wrap Adds the track artifacts for issue #104: PackCharts's sqrt-area shelf-width heuristic counts k padded chart areas while the wrap test measures the unpadded right edge against a cursor already inset by pad, so a row of k charts is budgeted k gutters instead of the k+1 it spends. Rows wrap early and waste atlas area once the extent is fit to a square texture. spec.md records the worked two-chart case, the chosen fix (greedy wrap on lexicographic (max(W,H), W*H) cost over the stay/wrap candidate extents, with target_width keeping its current override semantics), expected extents, and acceptance criteria. plan.md phases the work TDD-first behind a gate on PR #99, where the code under change lands. Co-Authored-By: Claude Opus 5 --- conductor/tracks/P7/plan.md | 112 +++++++++++++++++++++++++ conductor/tracks/P7/spec.md | 157 ++++++++++++++++++++++++++++++++++++ 2 files changed, 269 insertions(+) create mode 100644 conductor/tracks/P7/plan.md create mode 100644 conductor/tracks/P7/spec.md diff --git a/conductor/tracks/P7/plan.md b/conductor/tracks/P7/plan.md new file mode 100644 index 0000000..6e0204c --- /dev/null +++ b/conductor/tracks/P7/plan.md @@ -0,0 +1,112 @@ +# P7 Implementation Plan + +Strict TDD (per `conductor/workflow.md`): failing tests first, then the +implementation, then docs/amalgamation, with manual verification at each phase +checkpoint. + +**Blocked until PR #99 (F2) merges.** Phase 0 is the gate. + +## Phase 0: Gate on F2 +- [ ] 0.1 Confirm PR #99 is merged and `include/OpenABF/ChartPacking.hpp` plus + `tests/src/TestChartPacking.cpp` are on `develop`. +- [ ] 0.2 Branch `p7-packing-square-wrap` from updated `develop`; open a draft PR + linked to issue #104 via GitHub's Development field. + +### Verification +- [ ] `ctest` is green on the fresh branch before any P7 change (clean baseline). + +## Phase 1: Pin current behavior with failing tests +Write the tests that the current heuristic fails, and record what it actually +produces so the improvement is measurable. + +- [ ] 1.1 Add `DefaultWrapPacksTallChartPairSideBySide` to + `tests/src/TestChartPacking.cpp`: two 1 × 2 charts, + `minimize_bounding_box = false`, `padding = 0.1`, no `target_width`; expect + extent 2.3 × 2.2. (Currently 1.2 × 4.3.) +- [ ] 1.2 Add `DefaultWrapKeepsAtlasRoughlySquare`: chart sets where near-square is + achievable (4, 6, and 9 unit charts; `padding = 0` and `padding = 0.5`); + assert `max(W, H) / min(W, H)` is within a tight bound. Excludes counts + (e.g. 2 identical squares) where no layout beats aspect 2. +- [ ] 1.3 Add `DefaultWrapPaddingDoesNotForceEarlyWrap`: for a set of identical + charts, the charts-per-shelf count with `padding > 0` matches the + `padding = 0` count. +- [ ] 1.4 Add `ExplicitTargetWidthWrapsAtGivenWidth`: pins the override path's wrap + case (the existing `PaddingSeparatesChartsInSingleRow` only pins the + no-wrap case), and `OversizedChartPlacedAloneOnShelf`: a chart wider than + `target_width` is placed at a shelf start rather than wrapping an empty + shelf. +- [ ] 1.5 Run `ctest`; record which new tests fail and the extents the current + heuristic produces, in this plan. + +### Verification +- [ ] 1.1–1.3 fail against unmodified `PackCharts`; 1.4 passes (override path is + unchanged by P7 and must stay that way). +- [ ] Measured pre-change extents recorded below. + +### Phase 1 results +_(to be filled in: per-case current vs. expected extents)_ + +## Phase 2: Squareness-based wrap +- [ ] 2.1 In `PackCharts` (`include/OpenABF/ChartPacking.hpp`), add a local + lexicographic cost on `(max(W, H), W * H)` and compute the stay/wrap + candidate extents from `cursorX`, `cursorY`, `shelfHeight`, `atlasMaxX`, + `atlasMaxY` as in the spec. +- [ ] 2.2 Replace the default wrap test with `cursorX > pad and cost(stay) > + cost(wrap)`; keep the `cursorX > pad` empty-shelf guard so oversized charts + are still placed alone. Strict `>` so ties keep the chart on the shelf. +- [ ] 2.3 Keep `opts.target_width` as an outright override on its existing + `cursorX + width[i] > targetWidth` test; delete the now-dead + `sqrt(Σ (w+pad)(h+pad))` `areaSum` loop from the default path. +- [ ] 2.4 Run `ctest` — Phase 1 tests 1.1–1.3 now pass, 1.4 still passes, and all + pre-existing `ChartPacking` tests pass **unmodified**. +- [ ] 2.5 Confirm `PaddingSurroundsChartsAtPerimeter` still spans ≥ 2 shelves under + the new decision (its stated purpose); if its 5 charts now fit one row, + adjust the chart set — not the assertions — so the top/right perimeter + margins stay covered, and update its comment. + +### Verification +- [ ] Full `ctest` green; no existing test assertion weakened or deleted. +- [ ] Non-overlap and perimeter-inset invariants still hold (`ChartsDoNotOverlap`, + `ExtentBoundsAllCharts`, `PaddingSurroundsChartsAtPerimeter`). + +## Phase 3: Documentation and single header +- [ ] 3.1 Rewrite the `PackOptions::target_width` Doxygen: no `sqrt(total area)` + default; describe the squareness-based wrap when unset, and the override's + exact semantics (unpadded right edge vs. the given width). +- [ ] 3.2 Review the `PackCharts` shelf-layout comment block and `@par Complexity` + — describe the new decision and confirm `O(n log n)` still holds. +- [ ] 3.3 `git clang-format` on changed files, re-stage. +- [ ] 3.4 Regenerate the amalgamated header: + `python3 thirdparty/amalgamate/amalgamate.py -c single_include.json -s .` + (no new headers introduced, so no `single_include.json` edit expected). +- [ ] 3.5 Build docs and confirm no new Doxygen warnings. + +### Verification +- [ ] `ctest` green against the regenerated single header (tests include only + `OpenABF.hpp`). +- [ ] `git diff` on `single_include/` reflects exactly the `ChartPacking.hpp` + change. + +## Phase 4: Example verification and integration +- [ ] 4.1 Build and run `examples/src/MultiChartFlatten.cpp`; confirm the reported + normalized extent is near-square instead of `[0, 0] -> [0.27907, 1]`, and + record the before/after values in this plan. +- [ ] 4.2 Confirm all other examples still build and run. +- [ ] 4.3 Commit with Conventional Commits (`test:` for Phase 1, `fix:` or `perf:` + for Phase 2, `docs:`/`chore:` for Phase 3); push. +- [ ] 4.4 Mark the PR ready, update its title and description with the full diff + summary, and confirm it closes #104. +- [ ] 4.5 Update `conductor/tracks.md`: P7 → closed/archived with the PR number, + and refresh the `Updated` date. + +### Phase 4 results +_(to be filled in: MultiChartFlatten extent before/after)_ + +## Final Verification +- [ ] All spec acceptance criteria met. +- [ ] `ctest` fully green; new tests cover the default heuristic (the gap #104 + identified), the padding-independence of shelf occupancy, and both + `target_width` override paths. +- [ ] `git clang-format` clean; single header regenerated and committed. +- [ ] Docs updated with no new warnings; example output verified. +- [ ] PR reviewed and ready to merge. diff --git a/conductor/tracks/P7/spec.md b/conductor/tracks/P7/spec.md new file mode 100644 index 0000000..48bc4a7 --- /dev/null +++ b/conductor/tracks/P7/spec.md @@ -0,0 +1,157 @@ +# P7 — PackCharts wastes atlas area: shelf-wrap test and sqrt-area target width disagree on padding + +## GitHub Issue +https://github.com/educelab/OpenABF/issues/104 + +## Summary +`PackCharts`'s default shelf-width heuristic and its shelf-wrap test account for +`padding` differently, so charts wrap to a new shelf earlier than the heuristic +intends. The layout stays valid and non-overlapping — this is not a correctness +bug — but it wastes atlas area, and at low chart counts a single premature wrap +turns a would-be square atlas into a narrow column. Introduced with F2 (#18, +PR #99). + +Fix by removing the estimate/measurement mismatch entirely: wrap when wrapping +leaves the running atlas extent closer to square than staying on the current +shelf, rather than when a row would exceed an estimated width. An explicit +`opts.target_width` continues to override with today's semantics. + +## Problem description +The target width is computed from **padded** chart areas +(`include/OpenABF/ChartPacking.hpp:336-342`): + +```cpp +areaSum += (width[i] + pad) * (height[i] + pad); +targetWidth = std::sqrt(areaSum); +``` + +but the wrap test measures the **unpadded** right edge of the chart being placed +against a cursor that already starts at the perimeter inset `pad` +(`include/OpenABF/ChartPacking.hpp:358`): + +```cpp +if (cursorX > pad and cursorX + width[i] > targetWidth) { /* wrap */ } +``` + +`sqrt(Σ (w+pad)(h+pad))` under-counts padding: a row of *k* charts spends *k+1* +horizontal gutters (the perimeter inset at both ends plus the interior ones), not +*k*. The estimate lands just below the width the charts actually need and the row +wraps early. + +Worked for two 1 × 2 charts with `padding = 0.1`: + +| step | value | +| ---- | ----- | +| `areaSum` | `2 × (1.1 × 2.1)` = 4.62 | +| `targetWidth` | `sqrt(4.62)` ≈ 2.149 | +| chart A at `cursorX = 0.1` | `0.1 + 1 = 1.1 ≤ 2.149` ✔ | +| cursor advances | `0.1 + 1 + 0.1 = 1.2` | +| chart B | `1.2 + 1 = 2.2 > 2.149` → **wrap** (by 0.05) | + +Side by side the pair needs `pad + 1 + pad + 1 + pad` = 2.3 wide by +`pad + 2 + pad` = 2.2 tall (area 5.06, more than the estimated 4.62), giving an +almost exactly square atlas. Instead `examples/src/MultiChartFlatten.cpp` (two +charts ≈ 1 × 2 after bounding-box minimization, `padding = 0.1`, +`normalize = true`) reports a single stacked column: + +``` +Packed atlas extent: [0, 0] -> [0.27907, 1] // 1.2 x 4.3 before normalization +``` + +## Why this matters +The wasted area is charged directly to the consumer as soon as the packed extent +is fit to a square texture — which is exactly what `opts.normalize` does, scaling +by `1 / max(atlasW, atlasH)`. For the two-chart example above the atlas occupies +`1.2 × 4.3 = 5.16` units inside a `4.3 × 4.3 = 18.5` square, a 28% fill; the +side-by-side layout fills `5.06 / 5.29` = 96% of its square. The effect is worst +at 2–4 charts and shrinks as chart count grows and rows fill. + +## Approach — wrap on squareness +Replace the fixed-width wrap test in the default path with a greedy comparison of +the two candidate atlas extents at each placement, and drop the now-unused +sqrt-area estimate. At the decision point for chart `i` the loop already holds +`cursorX`, `cursorY`, `shelfHeight`, `atlasMaxX`, and `atlasMaxY`, so both +candidates are O(1): + +```cpp +// stay on this shelf +const T stayW = std::max(atlasMaxX, cursorX + width[i]) + pad; +const T stayH = std::max(atlasMaxY, cursorY + height[i]) + pad; +// wrap to a new shelf +const T wrapY = cursorY + shelfHeight + pad; +const T wrapW = std::max(atlasMaxX, pad + width[i]) + pad; +const T wrapH = std::max(atlasMaxY, wrapY + height[i]) + pad; +if (cursorX > pad and cost(stayW, stayH) > cost(wrapW, wrapH)) { /* wrap */ } +``` + +`cost` is lexicographic on `(max(W, H), W * H)`: minimize the side of the +enclosing square first — the quantity `normalize` divides by and the one a square +texture pays for — then break ties toward the smaller total extent. Strict `>` +means an exact tie keeps the chart on the current shelf, so the layout prefers +fewer shelves and stays deterministic for identical inputs. + +The existing `cursorX > pad` guard is retained, so an empty shelf never wraps and +a chart wider than any sensible row is still placed alone at a shelf start. +Charts are still visited in descending-height order, so `height[i] <= shelfHeight` +for every non-first chart on a shelf and `stayH` reduces to the current +`atlasMaxY + pad` in practice; the `std::max` is kept for clarity and safety. + +Expected extents (`minimize_bounding_box = false`, `normalize = false`): + +| charts | padding | current | after P7 | +| ------ | ------- | ------- | -------- | +| 2 × (1 × 2) | 0.1 | 1.2 × 4.3 | 2.3 × 2.2 | +| 3 × (1 × 2) | 0.1 | 2.3 × 4.3 | 3.4 × 2.2 | +| 4 × (1 × 1) | 0.5 | 3.5 × 3.5 | 3.5 × 3.5 (unchanged) | + +## Acceptance Criteria +- [ ] With no `opts.target_width`, the wrap decision is made on squareness of the + running atlas extent; the `sqrt(Σ (w+pad)(h+pad))` estimate is removed from + the default path, so no estimate/measurement padding mismatch remains. +- [ ] Two 1 × 2 charts with `padding = 0.1` pack side by side to a 2.3 × 2.2 + extent (regression test for the reported case), and + `examples/src/MultiChartFlatten.cpp` reports a near-square normalized + extent instead of `[0, 0] -> [0.27907, 1]`. +- [ ] A test asserts the **default** heuristic's atlas aspect ratio on chart sets + where near-square is achievable (e.g. 4, 6, and 9 unit charts, with and + without padding), closing the gap noted in #104 that the existing + pinned-extent tests set `target_width` and therefore do not constrain the + heuristic. +- [ ] Adding padding does not change how many charts land on a shelf for a set of + identical charts (relative to `padding = 0`) — the specific symptom of the + mismatch. +- [ ] An explicit `opts.target_width` still wraps exactly as documented today + (both the no-wrap and wrap cases are pinned by tests), and a chart wider + than the target is still placed alone at the start of a shelf rather than + wrapping an empty shelf. +- [ ] All existing `ChartPacking` tests pass unmodified, including + `PaddingSeparatesChartsInSingleRow` and `PaddingSurroundsChartsAtPerimeter`; + `PaddingSurroundsChartsAtPerimeter` is confirmed to still span ≥ 2 shelves + under the new decision (its comment states it exercises multiple shelves) — + if the new layout puts its 5 charts in one row, the chart set is adjusted so + the multi-shelf perimeter margins remain covered. +- [ ] `PackOptions::target_width` Doxygen no longer documents a `sqrt(total area)` + default and instead describes the squareness-based wrap and the override's + exact (unpadded right edge vs. `target_width`) semantics; `padding` docs + remain accurate. +- [ ] `PackCharts`'s documented `O(n log n)` complexity is unchanged (the new test + is O(1) per chart). +- [ ] `git clang-format` clean; single header regenerated via + `thirdparty/amalgamate/amalgamate.py`. + +## Out of Scope +- Changing the semantics of an explicit `opts.target_width` (it keeps measuring + the unpadded right edge against the given width; only its documentation is + clarified). +- Direction 1 from #104 (repairing the sqrt-area estimate's gutter budget) — the + squareness test removes the need for a width estimate at all. +- Replacing shelf packing with skyline/MaxRects-style placement, or changing the + descending-height chart ordering. +- Packing-efficiency or fill-ratio reporting in the examples or benchmarks. +- Per-chart scaling; `normalize` remains a single global uniform scale. + +## Dependencies +- **F2 (#18, PR #99) must merge first** — `include/OpenABF/ChartPacking.hpp` and + its tests land there. Work starts from `develop` after that merge, not from the + `f2-multi-chart-packing` branch. +- B10 (#103) also rides on PR #99 but is otherwise independent of P7. From 2ec18bf517bb2507b01faf8caa9b87c7a631329a Mon Sep 17 00:00:00 2001 From: Seth Parker Date: Fri, 24 Jul 2026 18:47:43 -0400 Subject: [PATCH 19/20] fix(packing): validate inputs before mutating charts; measure atlas extent Addresses PR #99 review findings in PackCharts and MergeMeshes. PackCharts mutates caller-owned meshes in place, but validation was interleaved with the bounding-box rotation, so a null or empty chart partway through the list threw only after earlier charts had already been rotated, with no way for the caller to tell which. Validate all options and chart pointers in a dedicated pass first. Negative `padding` and non-positive `target_width` are now rejected as well, rather than silently producing a degenerate layout. PackResult::min was hard-coded to the origin on the assumption that the shelf cursor never goes negative, which made the returned extent silently wrong for inputs that broke the assumption. Measure the atlas corner from where the charts actually landed and shift by it when applying the transform, so the lower-corner-at-origin contract holds by construction instead of by assumption and survives future changes to the layout strategy. With the current shelf layout the shift is zero, so placement is unchanged. MergeMeshes relied on Vertex's copy constructor happening to drop `edge`; null it explicitly as clone() and extract_connected_components() do, so a merged vertex can never reference a half-edge in a source mesh. Vertex::is_boundary(), wheel(), and is_unreferenced() all read `edge` directly, so a carried-over pointer would report the source chart's neighborhood. Also from review: - PackCharts/MergeMeshes take std::vector> so MeshType is deduced; the explicit spelling keeps working. - Add HalfEdgeMesh::PositionType, replacing the decltype/declval dance in PackCharts's return type. - MergeMeshes validates and counts up front, enabling reserve(). - MultiChartFlatten: add missing includes and exit with a message when no chart could be flattened, rather than writing an empty atlas. Co-Authored-By: Claude Opus 5 --- examples/src/MultiChartFlatten.cpp | 10 +++ include/OpenABF/ChartPacking.hpp | 78 ++++++++++++++------- include/OpenABF/HalfEdgeMesh.hpp | 3 + include/OpenABF/MeshMerge.hpp | 27 ++++++-- single_include/OpenABF/OpenABF.hpp | 108 ++++++++++++++++++++--------- 5 files changed, 164 insertions(+), 62 deletions(-) diff --git a/examples/src/MultiChartFlatten.cpp b/examples/src/MultiChartFlatten.cpp index 19809ce..9bf6279 100644 --- a/examples/src/MultiChartFlatten.cpp +++ b/examples/src/MultiChartFlatten.cpp @@ -21,7 +21,10 @@ * @see OpenABF::HalfEdgeMesh::extract_connected_components * @see OpenABF::PackCharts */ +#include +#include #include +#include #include #include "OpenABF/OpenABF.hpp" @@ -99,6 +102,13 @@ int main() // keyed by source-mesh face corners. } + // Nothing to pack if every chart failed to flatten. Bail out loudly rather + // than writing an empty atlas that looks like a successful run. + if (chartMeshes.empty()) { + std::cerr << "No chart could be flattened; no atlas written\n"; + return EXIT_FAILURE; + } + // Pack the flattened charts into a shared frame. `normalize` fits the whole // atlas into [0,1]^2 via a single global uniform scale, which preserves the // charts' relative sizes. Packing only edits each chart's 2D vertex diff --git a/include/OpenABF/ChartPacking.hpp b/include/OpenABF/ChartPacking.hpp index 299d64f..083cd96 100644 --- a/include/OpenABF/ChartPacking.hpp +++ b/include/OpenABF/ChartPacking.hpp @@ -23,10 +23,10 @@ limitations under the License. #include #include #include +#include #include #include #include -#include #include #include @@ -198,7 +198,7 @@ struct PackOptions { * * Charts wrap to a new shelf when a row would exceed this width. If unset, * the width defaults to `sqrt(sum of per-chart bounding-box areas)`, which - * yields a roughly square atlas. + * yields a roughly square atlas. Must be positive when set. */ std::optional target_width{}; @@ -209,7 +209,8 @@ struct PackOptions { * boundary, so perimeter charts are inset from the returned extent by * `padding` as well -- not merely separated from their neighbors. * Defaults to `0` (charts laid out flush). `padding` is in absolute chart - * units and is applied before any `normalize` scaling. + * units and is applied before any `normalize` scaling. Must be + * non-negative. */ T padding{T(0)}; }; @@ -276,17 +277,19 @@ struct PackResult { * @param opts Packing options * @return The bounding box of the packed atlas * - * @throws std::invalid_argument If a chart pointer is null or a chart has no - * vertices. + * @throws std::invalid_argument If a chart pointer is null, a chart has no + * vertices, `opts.padding` is negative, or `opts.target_width` is set + * to a non-positive value. All inputs are validated before any chart is + * modified, so a throw leaves every chart untouched. */ template auto PackCharts( - std::vector& charts, + std::vector>& charts, const PackOptions& opts = PackOptions{}) - -> PackResult().pos)>> + -> PackResult { using T = typename MeshType::type; - using VecType = std::decay_t().pos)>; + using VecType = typename MeshType::PositionType; static_assert(VecType::Dimensions >= 2, "PackCharts requires mesh vertices with at least 2 position dimensions"); @@ -296,16 +299,28 @@ auto PackCharts( } const std::size_t n = charts.size(); - // Validate inputs and compute each chart's 2D bounding box (origin + size). + // Validate everything up front. This function mutates caller-owned meshes in + // place, so throwing partway through would leave some charts rotated and + // others not, with no way for the caller to tell which. + if (opts.padding < T(0)) { + throw std::invalid_argument("PackCharts: padding must be non-negative"); + } + if (opts.target_width and *opts.target_width <= T(0)) { + throw std::invalid_argument("PackCharts: target_width must be positive"); + } + for (const auto& chart : charts) { + if (not chart or chart->num_vertices() == 0) { + throw std::invalid_argument("PackCharts: chart is null or has no vertices"); + } + } + + // Compute each chart's 2D bounding box (origin + size). std::vector minX(n); std::vector minY(n); std::vector width(n); std::vector height(n); for (std::size_t i = 0; i < n; ++i) { const auto& chart = charts[i]; - if (not chart or chart->num_vertices() == 0) { - throw std::invalid_argument("PackCharts: chart is null or has no vertices"); - } // Tighten the chart's bounding box by rotating it in-plane first. if (opts.minimize_bounding_box) { detail::MinimizeChartBoundingBox(chart); @@ -352,8 +367,10 @@ auto PackCharts( auto cursorX = pad; auto cursorY = pad; auto shelfHeight = T(0); - auto atlasMaxX = T(0); - auto atlasMaxY = T(0); + auto placedMinX = std::numeric_limits::max(); + auto placedMinY = std::numeric_limits::max(); + auto placedMaxX = std::numeric_limits::lowest(); + auto placedMaxY = std::numeric_limits::lowest(); for (const auto i : order) { if (cursorX > pad and cursorX + width[i] > targetWidth) { cursorX = pad; @@ -362,18 +379,23 @@ auto PackCharts( } offsetX[i] = cursorX - minX[i]; offsetY[i] = cursorY - minY[i]; - atlasMaxX = std::max(atlasMaxX, cursorX + width[i]); - atlasMaxY = std::max(atlasMaxY, cursorY + height[i]); + placedMinX = std::min(placedMinX, cursorX); + placedMinY = std::min(placedMinY, cursorY); + placedMaxX = std::max(placedMaxX, cursorX + width[i]); + placedMaxY = std::max(placedMaxY, cursorY + height[i]); cursorX += width[i] + pad; shelfHeight = std::max(shelfHeight, height[i]); } - // The atlas extent includes the perimeter gutter: charts are inset by - // `pad` from the lower corner (the cursor starts at `pad`), so add `pad` - // to the far edges too. Every chart then has >= `pad` of empty space on - // all four sides, including against the atlas boundary. - const T atlasW = atlasMaxX + pad; - const T atlasH = atlasMaxY + pad; + // The atlas extent is measured from where the charts actually landed, then + // grown by the perimeter gutter, so every chart has >= `pad` of empty space + // on all four sides including against the atlas boundary. Measuring rather + // than assuming the lower corner keeps the returned extent honest if the + // layout strategy above ever changes. + const T atlasMinX = placedMinX - pad; + const T atlasMinY = placedMinY - pad; + const T atlasW = (placedMaxX + pad) - atlasMinX; + const T atlasH = (placedMaxY + pad) - atlasMinY; // Optional normalization: a single global uniform scale that fits the // padded atlas into [0,1]^2, preserving relative chart sizes. @@ -385,15 +407,19 @@ auto PackCharts( } } - // Apply translation (+ optional scale about the origin) in place. + // Apply translation (+ optional scale about the origin) in place. Shifting + // by the measured atlas corner puts that corner on the origin by + // construction rather than by assumption; with the shelf layout above the + // shift is already zero. for (std::size_t i = 0; i < n; ++i) { for (const auto& v : charts[i]->vertices()) { - v->pos[0] = (v->pos[0] + offsetX[i]) * scale; - v->pos[1] = (v->pos[1] + offsetY[i]) * scale; + v->pos[0] = (v->pos[0] + offsetX[i] - atlasMinX) * scale; + v->pos[1] = (v->pos[1] + offsetY[i] - atlasMinY) * scale; } } - // result.min is value-initialized to the origin; only u/v of max are set. + // The atlas lower corner now sits on the origin, so result.min keeps its + // value-initialized zero; only u/v of max are set. result.max[0] = atlasW * scale; result.max[1] = atlasH * scale; return result; diff --git a/include/OpenABF/HalfEdgeMesh.hpp b/include/OpenABF/HalfEdgeMesh.hpp index 10ae5e9..a3e1b20 100644 --- a/include/OpenABF/HalfEdgeMesh.hpp +++ b/include/OpenABF/HalfEdgeMesh.hpp @@ -388,6 +388,9 @@ class HalfEdgeMesh /** Pointer type */ using Pointer = std::shared_ptr; + /** Vertex position type */ + using PositionType = Vec; + struct Vertex; struct Edge; struct Face; diff --git a/include/OpenABF/MeshMerge.hpp b/include/OpenABF/MeshMerge.hpp index 7de0b30..cc2be55 100644 --- a/include/OpenABF/MeshMerge.hpp +++ b/include/OpenABF/MeshMerge.hpp @@ -19,6 +19,7 @@ limitations under the License. #pragma once #include +#include #include #include #include @@ -69,26 +70,42 @@ struct MergedMesh { * @throws std::invalid_argument If an input pointer is null or has no vertices. */ template -auto MergeMeshes(const std::vector& meshes) -> MergedMesh +auto MergeMeshes(const std::vector>& meshes) -> MergedMesh { MergedMesh result{MeshType::New(), {}, {}}; auto& out = result.mesh; + // Validate and size up the inputs before building anything. + std::size_t totalVerts{0}; + std::size_t totalFaces{0}; + for (const auto& src : meshes) { + if (not src or src->num_vertices() == 0) { + throw std::invalid_argument("MergeMeshes: input mesh is null or has no vertices"); + } + totalVerts += src->num_vertices(); + totalFaces += src->num_faces(); + } + result.vertex_source.reserve(totalVerts); + result.face_source.reserve(totalFaces); + // Gather every face's (offset) vertex indices so they can be inserted in a // single insert_faces() call, which rebuilds the mesh boundary once at the // end via update_boundary(). Inserting faces one at a time with // insert_face() would leave the boundary stale. std::vector> faces; + faces.reserve(totalFaces); for (std::size_t ci = 0; ci < meshes.size(); ++ci) { const auto& src = meshes[ci]; - if (not src or src->num_vertices() == 0) { - throw std::invalid_argument("MergeMeshes: input mesh is null or has no vertices"); - } // Vertices keep their relative order, so merged index == offset + sub // index; record provenance alongside each insertion. const auto offset = out->num_vertices(); for (const auto& v : src->vertices()) { - out->insert_vertex(*v); + const auto newIdx = out->insert_vertex(*v); + // Vertex's copy constructor copies only traits and position, but + // null `edge` explicitly -- as clone() and + // extract_connected_components() do -- so a merged vertex can never + // reference a half-edge belonging to one of the source meshes. + out->vertex(newIdx)->edge = nullptr; result.vertex_source.emplace_back(ci, v->idx); } // Re-emit each face against the offset vertex indices, preserving the diff --git a/single_include/OpenABF/OpenABF.hpp b/single_include/OpenABF/OpenABF.hpp index 3c842e4..5d9bc6c 100644 --- a/single_include/OpenABF/OpenABF.hpp +++ b/single_include/OpenABF/OpenABF.hpp @@ -835,6 +835,9 @@ class HalfEdgeMesh /** Pointer type */ using Pointer = std::shared_ptr; + /** Vertex position type */ + using PositionType = Vec; + struct Vertex; struct Edge; struct Face; @@ -4928,10 +4931,10 @@ limitations under the License. #include #include #include +#include #include #include #include -#include #include #include @@ -5103,7 +5106,7 @@ struct PackOptions { * * Charts wrap to a new shelf when a row would exceed this width. If unset, * the width defaults to `sqrt(sum of per-chart bounding-box areas)`, which - * yields a roughly square atlas. + * yields a roughly square atlas. Must be positive when set. */ std::optional target_width{}; @@ -5114,7 +5117,8 @@ struct PackOptions { * boundary, so perimeter charts are inset from the returned extent by * `padding` as well -- not merely separated from their neighbors. * Defaults to `0` (charts laid out flush). `padding` is in absolute chart - * units and is applied before any `normalize` scaling. + * units and is applied before any `normalize` scaling. Must be + * non-negative. */ T padding{T(0)}; }; @@ -5181,17 +5185,19 @@ struct PackResult { * @param opts Packing options * @return The bounding box of the packed atlas * - * @throws std::invalid_argument If a chart pointer is null or a chart has no - * vertices. + * @throws std::invalid_argument If a chart pointer is null, a chart has no + * vertices, `opts.padding` is negative, or `opts.target_width` is set + * to a non-positive value. All inputs are validated before any chart is + * modified, so a throw leaves every chart untouched. */ template auto PackCharts( - std::vector& charts, + std::vector>& charts, const PackOptions& opts = PackOptions{}) - -> PackResult().pos)>> + -> PackResult { using T = typename MeshType::type; - using VecType = std::decay_t().pos)>; + using VecType = typename MeshType::PositionType; static_assert(VecType::Dimensions >= 2, "PackCharts requires mesh vertices with at least 2 position dimensions"); @@ -5201,16 +5207,28 @@ auto PackCharts( } const std::size_t n = charts.size(); - // Validate inputs and compute each chart's 2D bounding box (origin + size). + // Validate everything up front. This function mutates caller-owned meshes in + // place, so throwing partway through would leave some charts rotated and + // others not, with no way for the caller to tell which. + if (opts.padding < T(0)) { + throw std::invalid_argument("PackCharts: padding must be non-negative"); + } + if (opts.target_width and *opts.target_width <= T(0)) { + throw std::invalid_argument("PackCharts: target_width must be positive"); + } + for (const auto& chart : charts) { + if (not chart or chart->num_vertices() == 0) { + throw std::invalid_argument("PackCharts: chart is null or has no vertices"); + } + } + + // Compute each chart's 2D bounding box (origin + size). std::vector minX(n); std::vector minY(n); std::vector width(n); std::vector height(n); for (std::size_t i = 0; i < n; ++i) { const auto& chart = charts[i]; - if (not chart or chart->num_vertices() == 0) { - throw std::invalid_argument("PackCharts: chart is null or has no vertices"); - } // Tighten the chart's bounding box by rotating it in-plane first. if (opts.minimize_bounding_box) { detail::MinimizeChartBoundingBox(chart); @@ -5257,8 +5275,10 @@ auto PackCharts( auto cursorX = pad; auto cursorY = pad; auto shelfHeight = T(0); - auto atlasMaxX = T(0); - auto atlasMaxY = T(0); + auto placedMinX = std::numeric_limits::max(); + auto placedMinY = std::numeric_limits::max(); + auto placedMaxX = std::numeric_limits::lowest(); + auto placedMaxY = std::numeric_limits::lowest(); for (const auto i : order) { if (cursorX > pad and cursorX + width[i] > targetWidth) { cursorX = pad; @@ -5267,18 +5287,23 @@ auto PackCharts( } offsetX[i] = cursorX - minX[i]; offsetY[i] = cursorY - minY[i]; - atlasMaxX = std::max(atlasMaxX, cursorX + width[i]); - atlasMaxY = std::max(atlasMaxY, cursorY + height[i]); + placedMinX = std::min(placedMinX, cursorX); + placedMinY = std::min(placedMinY, cursorY); + placedMaxX = std::max(placedMaxX, cursorX + width[i]); + placedMaxY = std::max(placedMaxY, cursorY + height[i]); cursorX += width[i] + pad; shelfHeight = std::max(shelfHeight, height[i]); } - // The atlas extent includes the perimeter gutter: charts are inset by - // `pad` from the lower corner (the cursor starts at `pad`), so add `pad` - // to the far edges too. Every chart then has >= `pad` of empty space on - // all four sides, including against the atlas boundary. - const T atlasW = atlasMaxX + pad; - const T atlasH = atlasMaxY + pad; + // The atlas extent is measured from where the charts actually landed, then + // grown by the perimeter gutter, so every chart has >= `pad` of empty space + // on all four sides including against the atlas boundary. Measuring rather + // than assuming the lower corner keeps the returned extent honest if the + // layout strategy above ever changes. + const T atlasMinX = placedMinX - pad; + const T atlasMinY = placedMinY - pad; + const T atlasW = (placedMaxX + pad) - atlasMinX; + const T atlasH = (placedMaxY + pad) - atlasMinY; // Optional normalization: a single global uniform scale that fits the // padded atlas into [0,1]^2, preserving relative chart sizes. @@ -5290,15 +5315,19 @@ auto PackCharts( } } - // Apply translation (+ optional scale about the origin) in place. + // Apply translation (+ optional scale about the origin) in place. Shifting + // by the measured atlas corner puts that corner on the origin by + // construction rather than by assumption; with the shelf layout above the + // shift is already zero. for (std::size_t i = 0; i < n; ++i) { for (const auto& v : charts[i]->vertices()) { - v->pos[0] = (v->pos[0] + offsetX[i]) * scale; - v->pos[1] = (v->pos[1] + offsetY[i]) * scale; + v->pos[0] = (v->pos[0] + offsetX[i] - atlasMinX) * scale; + v->pos[1] = (v->pos[1] + offsetY[i] - atlasMinY) * scale; } } - // result.min is value-initialized to the origin; only u/v of max are set. + // The atlas lower corner now sits on the origin, so result.min keeps its + // value-initialized zero; only u/v of max are set. result.max[0] = atlasW * scale; result.max[1] = atlasH * scale; return result; @@ -5328,6 +5357,7 @@ limitations under the License. #include +#include #include #include #include @@ -5378,26 +5408,42 @@ struct MergedMesh { * @throws std::invalid_argument If an input pointer is null or has no vertices. */ template -auto MergeMeshes(const std::vector& meshes) -> MergedMesh +auto MergeMeshes(const std::vector>& meshes) -> MergedMesh { MergedMesh result{MeshType::New(), {}, {}}; auto& out = result.mesh; + // Validate and size up the inputs before building anything. + std::size_t totalVerts{0}; + std::size_t totalFaces{0}; + for (const auto& src : meshes) { + if (not src or src->num_vertices() == 0) { + throw std::invalid_argument("MergeMeshes: input mesh is null or has no vertices"); + } + totalVerts += src->num_vertices(); + totalFaces += src->num_faces(); + } + result.vertex_source.reserve(totalVerts); + result.face_source.reserve(totalFaces); + // Gather every face's (offset) vertex indices so they can be inserted in a // single insert_faces() call, which rebuilds the mesh boundary once at the // end via update_boundary(). Inserting faces one at a time with // insert_face() would leave the boundary stale. std::vector> faces; + faces.reserve(totalFaces); for (std::size_t ci = 0; ci < meshes.size(); ++ci) { const auto& src = meshes[ci]; - if (not src or src->num_vertices() == 0) { - throw std::invalid_argument("MergeMeshes: input mesh is null or has no vertices"); - } // Vertices keep their relative order, so merged index == offset + sub // index; record provenance alongside each insertion. const auto offset = out->num_vertices(); for (const auto& v : src->vertices()) { - out->insert_vertex(*v); + const auto newIdx = out->insert_vertex(*v); + // Vertex's copy constructor copies only traits and position, but + // null `edge` explicitly -- as clone() and + // extract_connected_components() do -- so a merged vertex can never + // reference a half-edge belonging to one of the source meshes. + out->vertex(newIdx)->edge = nullptr; result.vertex_source.emplace_back(ci, v->idx); } // Re-emit each face against the offset vertex indices, preserving the From 66dabe07542b5f86c216ff3b91a0f9d50d2b200c Mon Sep 17 00:00:00 2001 From: Seth Parker Date: Fri, 24 Jul 2026 18:48:39 -0400 Subject: [PATCH 20/20] test(packing): cover pack/merge invariants from PR #99 review ChartPacking (16 -> 22 cases): - NegativePaddingThrows, NonPositiveTargetWidthThrows and InvalidInputLeavesChartsUntouched cover the validation fixes; all three are Red against the previous implementation. - TopologyAndIndicesUnchanged asserts the guarantee every caller's back-maps depend on: element counts, per-face corner order, and dense vertex indices all survive a pack, including the rotation. - PackingPreservesFaceOrientation checks signed-area sign and magnitude per face. Bounding-box minimization must be a rigid rotation; a reflection would flip texture handedness while leaving every bounding-box assertion in the file happy. - TargetWidthWrapsCharts exercises target_width for its actual purpose rather than only as a way to force a single row. MeshMerge (7 -> 9 cases): - FaceSourceIndexAlignsWithMergedFace compares centroids to prove face_source[i] describes merged face i; the existing test only checked that provenance was unique and in range. - MergedVerticesReferenceMergedMeshEdges asserts merged vertices point at edges of the merged mesh and that is_boundary() traversal works. This guards the Vertex copy-constructor dependency that MergeMeshes relies on. Two call sites use the newly deducible form so the build covers it. Co-Authored-By: Claude Opus 5 --- tests/src/TestChartPacking.cpp | 177 ++++++++++++++++++++++++++++++++- tests/src/TestMeshMerge.cpp | 58 ++++++++++- 2 files changed, 233 insertions(+), 2 deletions(-) diff --git a/tests/src/TestChartPacking.cpp b/tests/src/TestChartPacking.cpp index 8ffc1af..3ae4f08 100644 --- a/tests/src/TestChartPacking.cpp +++ b/tests/src/TestChartPacking.cpp @@ -71,6 +71,48 @@ auto Overlaps(const BBox& a, const BBox& b, float eps) -> bool (b.miny + eps < a.maxy); } +/** + * @brief Snapshot of a mesh's topology: per-face corner vertex indices + * + * PackCharts documents that it touches only vertex positions, so this must be + * identical before and after a pack. + */ +template +auto TopologySnapshot(const MeshPtr& m) -> std::vector> +{ + std::vector> topo; + for (const auto& face : m->faces()) { + std::vector corners; + for (const auto& edge : *face) { + corners.push_back(edge->vertex->idx); + } + topo.push_back(std::move(corners)); + } + return topo; +} + +/** @brief Twice the signed area of a face, from its first three corners */ +template +auto SignedArea2(const FacePtr& face) -> float +{ + std::vector> p; + for (const auto& edge : *face) { + p.push_back({edge->vertex->pos[0], edge->vertex->pos[1]}); + } + return (p[1][0] - p[0][0]) * (p[2][1] - p[0][1]) - (p[1][1] - p[0][1]) * (p[2][0] - p[0][0]); +} + +/** @brief All vertex positions of a chart, in index order */ +template +auto Positions(const MeshPtr& m) -> std::vector> +{ + std::vector> pos; + for (const auto& v : m->vertices()) { + pos.push_back({v->pos[0], v->pos[1]}); + } + return pos; +} + } // namespace // --- Edge cases ------------------------------------------------------------ @@ -97,6 +139,48 @@ TEST(ChartPacking, EmptyChartThrows) EXPECT_THROW(PackCharts(charts), std::invalid_argument); } +TEST(ChartPacking, NegativePaddingThrows) +{ + std::vector charts{MakeRectChart(1.f, 1.f)}; + PackOptions opts; + opts.padding = -0.1f; + EXPECT_THROW(PackCharts(charts, opts), std::invalid_argument); +} + +TEST(ChartPacking, NonPositiveTargetWidthThrows) +{ + std::vector charts{MakeRectChart(1.f, 1.f)}; + PackOptions zero; + zero.target_width = 0.f; + EXPECT_THROW(PackCharts(charts, zero), std::invalid_argument); + PackOptions negative; + negative.target_width = -1.f; + EXPECT_THROW(PackCharts(charts, negative), std::invalid_argument); +} + +// PackCharts mutates caller-owned meshes in place, so a rejected input must +// leave every chart exactly as it was -- otherwise the caller cannot tell which +// charts were already rotated when the throw happened. +TEST(ChartPacking, InvalidInputLeavesChartsUntouched) +{ + auto valid = MakeRectChart(4.f, 1.f); // wide: would be stood upright + const auto before = Positions(valid); + + std::vector withNull{valid, nullptr}; + EXPECT_THROW(PackCharts(withNull), std::invalid_argument); + EXPECT_EQ(Positions(valid), before) << "chart mutated before the null chart was rejected"; + + std::vector withEmpty{valid, Mesh::New()}; + EXPECT_THROW(PackCharts(withEmpty), std::invalid_argument); + EXPECT_EQ(Positions(valid), before) << "chart mutated before the empty chart was rejected"; + + std::vector single{valid}; + PackOptions badPad; + badPad.padding = -1.f; + EXPECT_THROW(PackCharts(single, badPad), std::invalid_argument); + EXPECT_EQ(Positions(valid), before) << "chart mutated before bad options were rejected"; +} + TEST(ChartPacking, ZeroAreaChartIsPlacedWithoutThrowing) { // A flat (collinear) chart has zero area but is a valid mesh: distinct @@ -152,7 +236,9 @@ TEST(ChartPacking, ChartsDoNotOverlap) for (int i = 0; i < 6; ++i) { charts.push_back(MakeRectChart(1.f + 0.3f * static_cast(i), 1.f)); } - PackCharts(charts); + // MeshType is deduced from the chart vector here; the explicit spelling used + // elsewhere in this file must keep working too. + PackCharts(charts); std::vector boxes; for (const auto& c : charts) { boxes.push_back(ChartBBox(c)); @@ -179,6 +265,95 @@ TEST(ChartPacking, ExtentBoundsAllCharts) } } +// The guarantee every caller's back-maps depend on: packing edits vertex +// positions only. Element counts, vertex indices, and per-face corner order must +// all survive, including the rotation performed by minimize_bounding_box. +TEST(ChartPacking, TopologyAndIndicesUnchanged) +{ + std::vector charts{MakeRectChart(3.f, 1.f), MakeRotatedRectChart(2.f, 1.f, 0.4f), + MakeRectChart(1.f, 1.f)}; + std::vector vertCounts; + std::vector faceCounts; + std::vector edgeCounts; + std::vector>> topo; + for (const auto& c : charts) { + vertCounts.push_back(c->num_vertices()); + faceCounts.push_back(c->num_faces()); + edgeCounts.push_back(c->num_edges()); + topo.push_back(TopologySnapshot(c)); + } + + PackCharts(charts); + + for (std::size_t i = 0; i < charts.size(); ++i) { + EXPECT_EQ(charts[i]->num_vertices(), vertCounts[i]) << "chart " << i; + EXPECT_EQ(charts[i]->num_faces(), faceCounts[i]) << "chart " << i; + EXPECT_EQ(charts[i]->num_edges(), edgeCounts[i]) << "chart " << i; + EXPECT_EQ(TopologySnapshot(charts[i]), topo[i]) << "chart " << i; + // Vertex indices must stay dense and in order for back-maps to resolve. + std::size_t expected = 0; + for (const auto& v : charts[i]->vertices()) { + EXPECT_EQ(v->idx, expected++) << "chart " << i; + } + } +} + +// The bounding-box minimization must be a rigid rotation, never a reflection: a +// mirrored chart would flip texture handedness while leaving every bounding-box +// assertion in this file happy. +TEST(ChartPacking, PackingPreservesFaceOrientation) +{ + std::vector charts{MakeRotatedRectChart(4.f, 1.f, 0.6f), MakeRectChart(3.f, 2.f), + MakeRotatedRectChart(1.f, 2.f, -1.2f)}; + std::vector> before; + for (const auto& c : charts) { + std::vector areas; + for (const auto& f : c->faces()) { + areas.push_back(SignedArea2(f)); + } + before.push_back(std::move(areas)); + } + + PackCharts(charts); + + for (std::size_t i = 0; i < charts.size(); ++i) { + std::size_t fi = 0; + for (const auto& f : charts[i]->faces()) { + const auto after = SignedArea2(f); + EXPECT_GT(after * before[i][fi], 0.f) + << "chart " << i << " face " << fi << " changed orientation"; + // Rotation is rigid, so the magnitude is preserved as well. + EXPECT_NEAR(std::abs(after), std::abs(before[i][fi]), 1e-3f) + << "chart " << i << " face " << fi; + ++fi; + } + } +} + +TEST(ChartPacking, TargetWidthWrapsCharts) +{ + // Four 1x1 charts against a target width of 2.5: two fit per shelf, so the + // layout must wrap into two rows and stay within the requested width. + std::vector charts; + for (int i = 0; i < 4; ++i) { + charts.push_back(MakeRectChart(1.f, 1.f)); + } + PackOptions opts; + opts.minimize_bounding_box = false; + opts.target_width = 2.5f; + auto extent = PackCharts(charts, opts); + + EXPECT_LE(extent.max[0] - extent.min[0], 2.5f + 1e-4f) << "layout exceeded target_width"; + + // Two distinct shelf origins, two charts on each. + std::set shelves; + for (const auto& c : charts) { + shelves.insert(ChartBBox(c).miny); + } + EXPECT_EQ(shelves.size(), 2u) << "expected the charts to wrap onto two shelves"; + EXPECT_NEAR(extent.max[1] - extent.min[1], 2.f, 1e-4f); +} + TEST(ChartPacking, PaddingSeparatesChartsInSingleRow) { // Force a single row with a large target width. Padding surrounds every diff --git a/tests/src/TestMeshMerge.cpp b/tests/src/TestMeshMerge.cpp index 2a9e3cd..b5c746d 100644 --- a/tests/src/TestMeshMerge.cpp +++ b/tests/src/TestMeshMerge.cpp @@ -52,7 +52,9 @@ TEST(MeshMerge, ConcatenatesVerticesAndFaces) { std::vector charts{MakeRectChart(1.f, 1.f, 0.f, 0.f), MakeRectChart(1.f, 1.f, 5.f, 0.f)}; - auto merged = MergeMeshes(charts); + // MeshType is deduced from the mesh vector here; the explicit spelling used + // elsewhere in this file must keep working too. + auto merged = MergeMeshes(charts); EXPECT_EQ(merged.mesh->num_vertices(), 8u); EXPECT_EQ(merged.mesh->num_faces(), 4u); EXPECT_EQ(merged.vertex_source.size(), 8u); @@ -93,6 +95,60 @@ TEST(MeshMerge, FaceSourceProvenanceIsValid) } } +// Uniqueness is not enough: face_source[i] must describe merged face i. Charts +// are placed far apart so each source face has a distinct centroid. +TEST(MeshMerge, FaceSourceIndexAlignsWithMergedFace) +{ + std::vector charts{MakeRectChart(1.f, 1.f, 0.f, 0.f), + MakeRectChart(2.f, 3.f, 10.f, 20.f)}; + auto merged = MergeMeshes(charts); + ASSERT_EQ(merged.face_source.size(), merged.mesh->num_faces()); + + auto centroid = [](const auto& face) { + float cx{0.f}; + float cy{0.f}; + float n{0.f}; + for (const auto& edge : *face) { + cx += edge->vertex->pos[0]; + cy += edge->vertex->pos[1]; + n += 1.f; + } + return std::pair{cx / n, cy / n}; + }; + + for (std::size_t i = 0; i < merged.mesh->num_faces(); ++i) { + const auto [chart, sub] = merged.face_source[i]; + ASSERT_LT(chart, charts.size()); + ASSERT_LT(sub, charts[chart]->num_faces()); + const auto [mx, my] = centroid(merged.mesh->face(i)); + const auto [sx, sy] = centroid(charts[chart]->face(sub)); + EXPECT_FLOAT_EQ(mx, sx) << "merged face " << i; + EXPECT_FLOAT_EQ(my, sy) << "merged face " << i; + } +} + +// Merged vertices must reference half-edges of the merged mesh, never of a +// source mesh: Vertex::is_boundary(), wheel(), and is_unreferenced() all read +// `edge` directly, so a carried-over pointer would silently report the source +// chart's neighbourhood. +TEST(MeshMerge, MergedVerticesReferenceMergedMeshEdges) +{ + std::vector charts{MakeRectChart(1.f, 1.f, 0.f, 0.f), + MakeRectChart(1.f, 1.f, 5.f, 0.f)}; + auto merged = MergeMeshes(charts); + for (const auto& v : merged.mesh->vertices()) { + ASSERT_NE(v->edge, nullptr) << "vertex " << v->idx << " has no edge"; + EXPECT_EQ(v->edge->mesh, merged.mesh.get()) + << "vertex " << v->idx << " references an edge outside the merged mesh"; + EXPECT_EQ(v->mesh, merged.mesh.get()) << "vertex " << v->idx; + } + // Every vertex of these two disjoint quads is on a boundary; this traverses + // via vertex->edge, so it only holds if the pointers are rebound correctly. + for (const auto& v : merged.mesh->vertices()) { + EXPECT_TRUE(v->is_boundary()) << "vertex " << v->idx; + } +} + // Composing the merge maps with extract's back-maps must recover original // (torn-mesh) identity: this is the full chain merged -> chart -> M'. TEST(MeshMerge, RoundTripExtractMergeRecoversOriginalIdentity)