Repository navigation
feat!: mesh review follow-ups — constraint preservation, corefine, implicit-cut snapping - #24
Merged
Merged
Conversation
This was referenced Sep 28, 2026
…it-cut snapping - Track fixed-edge constraints by geometry (endpoint coords) instead of by Edge_index so they survive clip()/collect_garbage() and the exact-kernel round trip, via snapshot_constraint_coords()/rebuild_fixed_edges_from_coords(). Re-resolve constraints after cutWithSurface and clipWithPlane. - Add TriMesh::corefine: insert the shared intersection polyline into both meshes so patches from each side stitch together watertight. - cut_with_implicit_function: add snap_tol to reuse an existing endpoint when a crossing lands within that fraction of an edge, avoiding sliver triangles; nudge exactly-on-value properties to the positive side so every vertex is unambiguously classified. - export_mesh: guard non-positive duplicate_vertex_threshold (1/0 quantisation was scaling coords to non-finite and silently dropping the whole mesh). - Add regression tests for user constraints surviving cut + protected remesh. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The numpy array constructor looped over the triangle array twice: the first loop validated indices and skipped degenerate/out-of-range faces, the second re-added every face with no validation. The second loop was dead work for valid meshes (re-adding an existing face returns null_face in CGAL) and, worse, re-introduced the out-of-bounds vector access the first loop was written to guard against — a triangle with an out-of-range index skipped by loop 1 would hit undefined behavior in loop 2. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
vertex_properties and the caller's property array are indexed by vertex.idx(). When the mesh carries tombstoned vertices from a prior clip, idx() values can exceed number_of_vertices(), so indexing either array reads/writes out of bounds. collect_garbage() at entry renumbers live vertices 0..n-1, keeping all idx() values in range and making the property-size check meaningful. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The three status prints at the top of the function wrote to stdout unconditionally, unlike every other diagnostic in the file. Wrap them in the LoopCGAL::verbose guard. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Face indices read from disk were used as verts[i0] with no bounds check, and stream read failures (truncated file) went unchecked — a corrupt or truncated LCMESH file caused out-of-bounds access instead of a clean error. Check the stream after each read and validate every face index against the vertex count, throwing runtime_error on failure. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…imitation - corefine's header comment claimed it returns "intersection polyline segments inserted", but it returns the number of vertices added to this mesh (matching the pybind docstring). Fix the header to match the implementation. - Note in clipWithPlane that a constraint the plane cuts through is not recovered, since the plane overload takes no edge_is_constrained_map and the split breaks endpoint adjacency. - Fix the include-guard end comment (MESH_HANDLER_H -> MESH_H). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The clip was wrapped in two nested try blocks both catching std::exception; the outer only guarded non-throwing flag checks and logging, so its catch was unreachable. Flatten to a single try around the clip, with the success/failure reporting after it. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
v0/v1 were bound to columns 1/0 for the validity check while the halfedge lookup used columns 0/1 directly, so the error messages printed mismatched vertex pairs. Bind v0=col0, v1=col1 once and reuse for the lookup; the resolved halfedge (columns 0->1) is unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…fixes Phase 1 of the improvement plan — raise the assertion floor and cover the newest features. - Replace the smoke test for cut_with_implicit_function (which only asserted n_verts/n_faces >= 0 — true for any mesh) with tests/test_implicit_cut.py that pin real geometry: seam inserted exactly on the isovalue, no degenerate faces, PRESERVE_INTERSECTION keeps full area, NaN triangles kept untouched, snap_tol reuses vertices. Three tests assert the *tight* cut and are marked xfail (strict) to document a discovered bug: KEEP_POSITIVE/NEGATIVE_SIDE retain a one-triangle fringe of wrong-side sub-triangles at the seam. Fixed in Phase 3. - Add tests/test_corefine.py: the exact-kernel path inserts a coincident shared polyline into both meshes (watertight) and preserves area. The use_exact_kernel=False path hard-crashes (SIGBUS) on open surfaces, so it is documented and skipped rather than executed (a native crash cannot be xfailed). - Add a truncated-file regression test for read_from_file and a raw-constructor test proving out-of-range/degenerate triangles are skipped without crashing and no face is double-inserted. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Phase 3 of the improvement plan. Refactor: the six hand-written split cases in cut_with_implicit_function were sign-paired duplicates — CASE 1≡5, 2≡6, 3≡7 produce identical oriented sub-triangles because the split topology depends only on *which* vertex is the "lone" one (the vertex whose side differs), not on its sign. Collapse them into one table keyed on the lone vertex (~130 lines -> ~30), and drop the dead all-positive skip (already covered by the `active` mask) and per-case verbose prints. Fix: KEEP_POSITIVE_SIDE / KEEP_NEGATIVE_SIDE kept a one-triangle fringe past the isovalue. When a straddling triangle is split, the wrong-side sub-triangle has two crossing vertices whose value is exactly `value`; the assembly filter's strict < / > comparison spared it. Use inclusive <= / >= so it is dropped and the cut stops exactly at the seam. Verified: for property=x on a 10x10 grid, POS+NEG areas now sum to the original and each matches the analytic half-area. Removes the strict-xfail markers from the three tightness tests added in Phase 1, which now pass. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Phase 2 of the improvement plan — adopt a clear error-handling contract. A return of 0 is now reserved for the one legitimate no-op (a cut whose meshes do not intersect); every genuine failure raises instead of printing to stderr and returning a sentinel. pybind11 maps invalid_argument -> ValueError and runtime_error -> RuntimeError. - cutWithSurface: raise ValueError on empty/invalid source or clipper; raise RuntimeError if the clip of intersecting meshes throws or reports failure. Non-intersecting meshes still return 0. - clipWithPlane: raise ValueError on empty mesh; raise RuntimeError if the clip throws. - cut_with_implicit_function: raise ValueError on property/vertex size mismatch. - corefine: refuse use_exact_kernel=False up front with ValueError. That path ran CGAL corefinement on the inexact predicate kernel (Simple_cartesian), which hard-crashes (SIGBUS) even on trivial valid inputs — uncatchable, so it must be prevented rather than trapped. corefine failures now raise RuntimeError. Tests: add tests/test_error_contract.py; convert the previously-skipped corefine inexact test into one asserting it raises ValueError. BREAKING CHANGE: operations that previously returned 0 on invalid/empty input or logged-and-continued on failure now raise ValueError/RuntimeError. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
lachlangrose
force-pushed
the
fix/mesh-review-followups
branch
from
September 28, 2026 11:59
9c8dd97 to
5d2eea3
Compare
The ubuntu-latest runner is still 24.04, whose libcgal-dev is 5.6. Passing an edge_is_constrained_map to the mesh-mesh PMP::clip does not compile there: 5.6's put() for Boolean_property_map takes a non-const reference while CGAL's own corefinement internals pass it as const, so the build dies inside Corefinement/face_graph_utils.h. Fixed in CGAL 6. Verified by compiling src/mesh.cpp against each: 5.6 (ubuntu 24.04) fails, 6.0.1 (debian trixie), 6.1.1 (ubuntu 26.04) and 6.2 (homebrew) all succeed. That is why only the ubuntu jobs were red while every macOS job passed. CGAL is header-only, so fetch the 6.0.1 release rather than wait for the runner image to catch up, and pass it through CMAKE_ARGS the same way the Windows job passes its toolchain. Pinned, not floating, and it matches the version the project Dockerfile already builds. Also state the floor in CMakeLists: find_package(CGAL 6.0 REQUIRED) turns a 200-line template error into one clear line for anyone still on 5.6. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
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.
What & why
Eleven commits of follow-up work on the mesh operations, batched here so they land
before the clip fix in #22 rather than being tangled up with it.
feat: preserve edge constraints through cuts, add corefine and implicit-cut snappingfeat!: raise on genuine failure instead of returning 0 / crashing— breakingrefactor: data-driven marching-triangles table + fix wrong-side fringerefactor: collapse redundant nested try/catch in cutWithSurfacefix: compact mesh before cut_with_implicit_function to avoid OOBfix: validate stream state and face indices in read_from_filefix: remove duplicate face-insertion loop in array constructorfix: gate cut_with_implicit_function debug prints on verbosecleanup: consistent vertex naming in add_fixed_edgesdocs: correct corefine return-value comment and document clip-split limitationtest: harden implicit-cut coverage, add corefine tests, cover recent fixesHow to verify
Risk
feat!:above changes failure behaviour from returning 0 / crashing toraising. Callers checking for a 0 return need updating.
output is expected to change where that bug fired.
Checklist
pytestpasses locally🤖 Generated with Claude Code