Conversation
…docs - Fix noexcept correctness bugs: Vec3T::operator[], unit(), lessLX() and MeshT::incrementWarning() all called throwing operations inside noexcept bodies; replaced map::at() with operator[] and .at() with EBGEOMETRY_EXPECT. - Remove noexcept from ~54 allocating functions across Parser, Transform, CSG, BVH, DCEL (Face, Vertex, Mesh), MeshDistanceFunctions, and ImplicitFunction. Declaring noexcept on functions that call make_shared/emplace_back silently converts std::bad_alloc into std::terminate. - Add noexcept where it was missing: Vec2T constructors, EdgeT constructors (x3), EdgeIteratorT constructors (x2), and five Transform virtual destructors that were inconsistent with their siblings. - Add [[nodiscard]] to all 18 Parser read* functions; discarding the returned shared_ptr is always a programming error. - Add EBGEOMETRY_EXPECT guards in Vec3T::minDir and Vec3T::maxDir. - Add reserve() before emplace_back loops in FaceT gather functions and MeshT::getAllVertexCoordinates. - Fix sign-compare warning in MollifyIF (int -> size_t loop variables). - Fix wrong Doxygen @return on both MeshT::signedDistance overloads (said "negative inside"; DCEL inward-normal convention is positive inside). - Add CMakePresets.json with debug/debug-san/release/release-test presets; update CI to use cmake --preset debug; add sanitizer and examples support to CMakeLists.txt and Tests/CMakeLists.txt. - Add Examples/CMakeLists.txt so examples are buildable and testable via CTest with label filtering (unit vs example). - Rewrite README quickstart; expand Contributing.rst with preset reference table and manual-options fallback for CMake < 3.22. - Add TODO.md tracking all findings; one open item remains (sanityCheck return type, deferred as API-breaking). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UJSdT1HUkhxMw4DFYPZE3y
All warnings were caused by stale or mis-scoped literalinclude line ranges: - ImplemBVH.rst: bottomUpSortAndPartition literalinclude pointed to the TreeBVH class declaration (lines 298-309, which started mid-Doxygen-comment) instead of the function declaration (lines 363-372). - ImplemCSG.rst: Transform literalinclude started at line 20 (inside the first /** comment, missing the opening /**) and ended at line 111 (mid-comment before Elongate); fixed to lines 19-130 (/** through Reflect). CSG literalinclude started at line 23 (same issue) and ended at line 153 (mid-SmoothIntersection comment); fixed to lines 22-214 (through FiniteRepetition). Handwritten approximateBoundingVolumeOctree code-block had stale `noexcept` and a typo (`const T>&`); replaced with a literalinclude of the actual declaration (ImplicitFunction.hpp lines 57-75). - ImplemOctree.rst: Both non-contiguous line selections pulled orphaned @param lines from the middles of different Doxygen comment blocks, producing garbled output. Replaced with coherent contiguous ranges that include full comment blocks: lines 75-100,180-200 for Construction; 75-76,102-119,202-214 for Traversal. - Parsers.rst: All five literalinclude blocks had stale line numbers that pointed to the wrong functions (each block was shifted by roughly 14-20 lines from a prior Parser refactor). Updated to show the correct functions: readIntoDCEL (137-157), readIntoMesh (159-179), readIntoFullBVH (181-211), readIntoCompactBVH (213-235), readIntoTriangleBVH (237-267). - index.rst: Move Contributing.rst out of the Introduction toctree into its own top-level "Contributing and testing" section placed after Examples and before References. Add CSS rule to suppress the section heading in HTML view. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UJSdT1HUkhxMw4DFYPZE3y
…h std::clamp Sign convention: flip DCEL face normals from inward to outward ((x2-x0)×(x2-x1) instead of (x2-x1)×(x2-x0)), making signedDistance negative inside / positive outside — matching the analytic SDF convention. Same flip applied to Triangle and all four TriangleSoA SIMD paths (SSE4.1 float, AVX float, AVX double W=4/8) plus the scalar fallback; the point-in-triangle test now uses abs(s0+s1+s2)>=2 so it is correct for both inward and outward normals. Updated Doxygen @return strings in DCEL_Mesh.hpp and Contributing.rst note. Tests flipped accordingly; two new FastTriMeshSDF tests added (52 tests, all passing). std::clamp: removed the custom scalar clamp() helper from EBGeometry_AnalyticDistanceFunctions.hpp; its Vec3T overload now delegates to std::clamp per component. All scalar call sites updated to std::clamp: Capsule and Cone SDFs in AnalyticDistanceFunctions, FiniteRepetitionIF in CSGImplem, std::min(std::max()) patterns in DCEL_EdgeImplem and DCEL_VertexImplem, and RandomCity examples. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UJSdT1HUkhxMw4DFYPZE3y
- Rename defaultK -> DefaultBranchingRatio - Rename LinearUpdater/LinearSorter -> PackedUpdater/PackedSorter - Rename equalCounts -> EqualCounts - Rename sah2WaySplit/sahKWaySplitImpl -> SAH2WaySplit/SAHKWaySplit - Remove virtual from TreeBVH destructor (no subclasses exist) - Remove spurious BV2 template parameter from PackedBVH identity constructor - Rename P2 -> PTree in PackedBVH converter constructor; expand @details - Apply m_ prefix to PackedBVH::Node members (bv, primOff, numPrims, childOff) - Apply m_ prefix to ChildAABBSoA members (lo, hi) - Add static_assert(BV==AABBT<T>) to SAH2WaySplit and SAHKWaySplit - Add EBGEOMETRY_EXPECT guards to all inline helper lambdas and Node::setChildOffset - Add {} braces to all bare if-branches Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UJSdT1HUkhxMw4DFYPZE3y
bvhSDF and linSDF both called readIntoPackedBVH after the parser rename. Drop the redundant bvhSDF slot; rename linSDF->meshSDF and update labels to FlatMeshSDF / MeshSDF (PackedBVH) / TriMeshSDF (PackedBVH). Also enable the FlatMeshSDF timing loop (was commented out) and add relative-speedup lines for both accelerated representations. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UJSdT1HUkhxMw4DFYPZE3y
Mirrors the existing operator-() so that +Vec3T<T>::max() compiles. The unary plus is an identity operation that returns *this. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UJSdT1HUkhxMw4DFYPZE3y
The Quickstart section only shows building one example directly with g++; there was no single place showing how to build and run the full test suite plus every example at once. Adds the CMake preset workflow (configure, build, ctest --preset debug, ctest --preset examples), with a pointer to the full preset table and Scripts/run-all-checks.sh in the Sphinx docs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
EBGeometry_Transform.hpp had zero behavioral test coverage -- Translate, Scale, Mollify, and Rotate were exercised only by the OctreeBoundingVolume example (which asserts nothing about the transformed value, only prints a bounding box), and Complement, Offset, Annular, Blur, Elongate, and Reflect were never exercised outside InstantiateAll.cpp's compile-only coverage. Covers every transform, mostly by cross-checking against an independently built analytic shape with the same expected geometry (translated/scaled/ offset/mirrored sphere; an elongated sphere against sphere-at-clamped-center); Rotate is checked via 360-degree and inverse-composition identities plus a hand-verified 90-degree axis swap on an asymmetric box; Blur and Mollify are checked via alpha=1/a_maxValue=0 exact-identity cases and, for the general smoothing case, a constant-valued implicit function fixture that verifies their sample weights are a true partition of unity (a weighted average of a constant must return that same constant) without needing to hand-derive the individual sample weights. Passes under debug-san (ASan+UBSan) and clang-tidy with zero new findings. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…arning The mismatch checks between FlatMeshSDF, MeshSDF, and TriMeshSDF printed a warning to stderr but always returned 0, so ctest reported this example as passed even if the accelerated representations silently disagreed with the brute-force one -- exactly the kind of precision/SIMD correctness regression this example is best positioned to catch. Now returns 1 on any mismatch, matching the pattern PackedSpheres and RandomCity already use. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…meOctree gap Neither EBGeometry_Octree.hpp's Node class nor ImplicitFunction::approximateBoundingVolumeOctree (built on top of it) had any Tests/ coverage -- the latter was only exercised by the OctreeBoundingVolume example, which prints a bounding box but never asserts it's correct. Octree::Node is tested directly with a minimal plain-unsigned-int-level tree: buildBreadthFirst/buildDepthFirst producing the expected 8^depth leaf count and agreeing with each other, meta-data propagating correctly per generation, traverse() correctly pruning a subtree via its Visiter and reordering visitation via a custom Sorter. approximateBoundingVolumeOctree is tested on an analytic sphere: the returned bounding volume always contains the true bounds and tightens as tree depth increases, a larger safety factor never shrinks the sphere out of the result, and both documented error fallbacks (an inverted initial box, and an initial box that never touches the surface) correctly return the maximal bounding volume. Passes under debug-san (ASan+UBSan) -- meaningful here given Node's recursive shared_ptr/enable_shared_from_this structure -- and clang-tidy with zero new findings. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
readIntoPackedBVH (used by the MeshSDF example but never in Tests/) and every readInto*(std::vector<std::string>) overload -- readIntoDCEL, readIntoMesh, readIntoPackedBVH, readIntoTriangleBVH -- had no coverage. Adds a direct cross-check for readIntoPackedBVH against MeshSDF built from the same parsed mesh, and verifies each multi-file overload is a faithful per-file loop (one result per input file, each identical to what the corresponding single-file call produces) using the dodecahedron fixture in two different formats. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Polygon2D was only exercised indirectly through DCEL::FaceT/MeshT's point-in-face containment logic. Adds direct coverage for all three inside/outside algorithms (winding number, crossing number, subtended angle): a convex square, a concave L-shaped polygon (specifically checking a point inside the vertices' convex hull but in the notched-out region), a triangle embedded in a non-axis-aligned plane (exercising the largest-normal-component projection-axis selection), and a grid sweep checking all three algorithms agree with each other away from polygon boundaries (where disagreement is expected and not meaningful to test). Passes under debug-san (ASan+UBSan) and clang-tidy with zero new findings. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
TriangleSoAT was only exercised indirectly through TriMeshSDF's internal packing. Adds direct coverage: signedDistance() against the minimum-|value| individual Triangle::signedDistance() for a fully-packed group (count == W), a padded group (count < W, verifying the repeated-last-triangle padding doesn't change the result), and a single-triangle group; computeBoundingVolume against an AABB built directly from the same triangles' vertices. Verified passing both under the debug preset (SIMD=none, scalar fallback path) and release-test (SIMD=avx, exercising the actual AVX packed-double intrinsics path for W=4/double) -- and under debug-san (ASan+UBSan), which matters given the class's over-alignment requirements. Zero new clang-tidy findings. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
SimpleTimer was completely untested. Adds basic coverage: default construction leaves it in a valid near-zero state, start()/stop() measures at least the requested sleep duration, calling start() again resets accumulated time, and a longer sleep yields a proportionally longer measurement. Uses generous one-sided bounds throughout to avoid flakiness on loaded CI runners. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…angle for float+double First batch of converting the existing double-only Catch2 suite to TEMPLATE_TEST_CASE(..., float, double), so behavioral correctness -- not just compilation via InstantiateAll.cpp -- is actually verified under float. WithinRel's type-specific overloads (100*epsilon<T> default) handle most relative comparisons automatically once the argument is T rather than a hardcoded double. WithinAbs has no such overload (only accepts double), so absolute-zero comparisons now scale their own margin by numeric_limits<T>::epsilon() -- tight for cases that are exactly representable in either precision (e.g. a normal computed from axis-aligned vectors), looser where a genuine sqrt/division makes the result only approximately zero even in double. Passes under debug-san (ASan+UBSan) and clang-tidy with zero new findings. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Introduces EBGEOMETRY_TEST_PRECISIONS (Tests/TestFloatingPointUtils.hpp), a macro that expands to just `double` by default and to `float, double` when Tests/CMakeLists.txt's new EBGEOMETRY_TEST_BOTH_PRECISIONS option is ON. Every TEMPLATE_TEST_CASE in the suite is written as TEMPLATE_TEST_CASE(..., EBGEOMETRY_TEST_PRECISIONS) instead of a hardcoded `float, double`, so local iteration stays fast and matches whatever the CMake preset otherwise builds, while CI (Unit-Tests, Sanitizers, Release-Test) now passes -DEBGEOMETRY_TEST_BOTH_PRECISIONS=ON to verify float behaviorally on every push. Retrofits the first batch of templatized files (TestVec, TestBoundingVolumes, TestAnalyticSDF, TestTriangle, TestPolygon2D, TestTriangleSoA, TestOBJ) onto this shared header, replacing their per-file margin() duplicate helpers with TestFloatingPointUtils.hpp's tightMargin<T>()/looseMargin<T>()/withinAbsT<T>() (WithinAbs, unlike WithinRel, has no type-specific default margin and only accepts double, so comparisons need an explicit, type-scaled margin either way). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Continues the EBGEOMETRY_TEST_PRECISIONS conversion. TestSTL additionally templatizes its two Parser::readSTL(...) file-parsing cases, verified against the on-disk tetrahedron.stl fixture in both precisions. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ug it found approximateBoundingVolumeOctree's intersection test used a bare `1.0` literal against a T-typed safety factor, silently double-promoting for T=float. This went undetected because it's a member function *template* (BV is independent of the class's own T) -- InstantiateAll.cpp only instantiates the class itself, never actually calls approximateBoundingVolumeOctree, so its body was never compiled for float before. Only caught once TestOctree's TEMPLATE_TEST_CASE actually invoked it with T=float under EBGEOMETRY_TEST_BOTH_PRECISIONS=ON. Fixed by writing the literal as T(1.0) in both call sites (EBGeometry_ImplicitFunctionImplem.hpp). The plain Octree::Node<unsigned int> tests are left as ordinary TEST_CASE -- they have no floating-point dependency at all. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Cross-representation signed-distance comparisons (across file formats, BVH build strategies, and BVH/FlatMeshSDF/MeshSDF/TriMeshSDF wrappers) needed type-scaled tolerances rather than the shared tightMargin/looseMargin helpers: formatMargin() for near-identical parses of the same geometry (1e-9 double / 1e-4 float) and traversalMargin() for comparisons that chain many more dot/cross products across the dodecahedron fixture's 36 faces (1e-6 double / 1e-2 float). Verified against the default, AVX, and debug-san builds, in both single- and both-precision configurations. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…tion bugs BVHUnionIF::value and BVHSmoothUnionIF::value's BVH pruning visiters compared a T-typed BV distance against a bare 0.0 literal, silently double-promoting for T=float. Same root cause as the Octree fix: these are member function bodies only ever instantiated when actually called, and nothing previously called them with T=float. Fixed by writing T(0.0) in both visiters (EBGeometry_CSGImplem.hpp). Introduces three domain-specific margin helpers (exactMargin/formulaMargin/ asymptoticMargin) for the wide range of tolerance needs in this file: two code paths computing the same closed-form expression, a hand-computed formula result, and a smooth blend's asymptotic convergence to its sharp counterpart, respectively -- each scaled up for float relative to the existing double tolerances rather than reusing the generic shared-header margins, since this file's comparisons span many different error accumulation profiles. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Same exactMargin/formulaMargin pattern as TestCSG, tailored to this file's comparisons: two code paths computing the same closed-form expression (exactMargin) vs. hand-computed formulas / partition-of-unity sums involving a few more floating-point operations (formulaMargin). No source bugs found this time -- verified in the default, both-precision, and debug-san builds. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Largest and last file in the float+double templatization pass. VertexT/ FaceT/EdgeT/MeshT/EdgeIteratorT/MeshSDF aliases and the loadTetrahedron()/ buildTetrahedron() fixture builders all become template aliases/functions parameterized on T; every hardcoded `double` in the 55 test bodies becomes T. Three margin tiers (exactMargin/formulaMargin/traversalMargin), matching the naming already established in TestBVH.cpp, cover unit-normal/symmetric checks, search-algorithm-agreement/near-zero-surface checks, and the FastTriMeshSDF-vs-MeshSDF brute-force comparison respectively. No source bugs found this time -- verified in the default, both-precision, and debug-san builds. This completes the EBGEOMETRY_TEST_PRECISIONS conversion of every Test*.cpp file with a floating-point dependency (TestSFC.cpp and TestSimpleTimer.cpp were already confirmed precision-independent and left untouched). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Catch2's WithinAbs/WithinRel matchers always operate on `double` internally (MatcherBase<double>), regardless of which type-specific overload constructed them. REQUIRE_THAT(<float value>, WithinRel(...)/WithinAbs(...)) therefore triggers an implicit float->double promotion *inside Catch2's own header* (catch_matchers_impl.hpp's MatchExpr::MatchExpr), which -Wdouble-promotion flags under Clang (confirmed with clang++-18 locally, matching the clang++-14 CI failure) once EBGEOMETRY_TEST_BOTH_PRECISIONS actually instantiates a TEMPLATE_TEST_CASE with T=float. GCC does not warn on the same code, which is why this was missed locally. This isn't a bug in EBGeometry's own code, so mark Catch2/Catch2WithMain's include directories as INTERFACE_SYSTEM_INCLUDE_DIRECTORIES instead of casting every REQUIRE_THAT argument to double at every call site. Verified with clang++-18 (previously reproduced the CI failure exactly, now builds and passes all 422 tests) and confirmed no regression in the default GCC build or clang-tidy. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…tolerance The mismatch check compared meshSum/triSum against dcelSum with a fixed, unscaled std::numeric_limits<T>::epsilon() -- effectively requiring bit-for-bit identical sums. Summing 1000 signed distances via a brute-force scan (FlatMeshSDF) and via BVH traversal (MeshSDF/TriMeshSDF) visits faces in a different order, and floating-point addition isn't associative, so a few-ULP discrepancy is expected and not a correctness bug. Confirmed by reproducing the reported failure locally (a ~2^-36-scale diff) and finding it recurs intermittently across runs with the same tolerance. Switches to a relative check (no more than 1e-7 relative difference between the sums, floored at magnitude 1) instead of a per-point comparison, which would degrade the timing loops this example also serves as a benchmark for. Verified with 20+ consecutive runs (double and float) against the armadillo mesh with no false positives. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…shSDF Both compared accumulated sums across representations (naive scan vs. BVH union, and for PackedSpheres also a finite-repetition tiling) with exact equality (sumSlow != sumFast) -- even more fragile than MeshSDF's original epsilon()-based check, since it tolerates zero floating-point discrepancy at all. Summing the same values in a different order is not bit-for-bit reproducible, so this was liable to intermittent, environment-dependent false failures for the same reason as the MeshSDF issue. Applies the identical fix: a relative tolerance (1e-7, floored at magnitude 1) instead of exact/absolute comparison. Verified with multiple runs in both double and float, via direct g++ invocation and the CMake build path. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
…ble) CI (Examples-FloatPrecision) failed on MeshSDF: TriMeshSDF vs FlatMeshSDF diff = 0.015625 against a sum of ~1.3e5, a relative gap of ~1.2e-7 -- right at float's own epsilon (~1.19e-7), just above the flat 1e-7 relative tolerance introduced for the sum-comparison fix. float genuinely has less headroom than double for the same "sum Nsamp terms in a different order" argument, so a single tolerance value for both precisions was always going to be borderline for float. Fixes MeshSDF, PackedSpheres, and RandomCity (all three share this sum-comparison pattern) to use 1e-4 relative tolerance for float and keep 1e-7 for double. Verified by reproducing the exact CI configuration (CMake debug preset, EBGEOMETRY_PRECISION=float) locally: 10 consecutive MeshSDF runs plus a full `ctest -L example` pass, all clean; confirmed no regression in the default double-precision build. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
rmrsk
changed the title
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Background
EBGeometry's
floatprecision path was untested, SIMD support didn't exist, and BVH/CSG naming had accumulated inconsistencies from organic growth. The Sphinx docs had also drifted: some pages were replicas of the README, implementation detail leaked into conceptual pages, andliteralincludedirectives silently broke whenever line numbers shifted upstream. This PR is a broad hardening, performance, and documentation pass across the whole library rather than a single feature.Solution
float/double); CI now exercises both, not justdouble.PackedBVH) and triangle evaluation (TriangleSoAT), with per-ISA compile-time defaults.TreeBVH/PackedBVH,BVH*CSG combinators), removed dead code, fixed a DCEL mesh reference-cycle leak, and reversed the DCEL sign convention to the standard one.noexcept/assertion audit, a cleanclang-tidypass, west-const, and include-what-you-use.Examples/(per-example CMake/GNUmake,ThirdParty/split for AMReX/Chombo) and rewrote the Sphinx docs end-to-end: a Concepts/Implementation split,literalincludebanned in favor of verified Doxygen links, and every page audited for accuracy and grammar.Side-effects
5.0.0to5.3.0to fix a real internal crash in Sphinx's cross-reference resolver; no other doc-build behavior changes.F18example was dropped (its backing data was removed).Alternative solutions
This could have been split into many smaller PRs, one per component. Given how many changes ripple together (a rename touches tests, docs, and examples in the same commit to stay buildable), a single pass kept every intermediate commit consistent; the 120+ commit history preserves the step-by-step reasoning for anyone who wants to review it incrementally instead.
PR-review checklist
@claude review.