ENH: Add Bounding Box Skin option to the surface meshing filters - #1708
Open
imikejackson wants to merge 24 commits into
Open
ENH: Add Bounding Box Skin option to the surface meshing filters#1708imikejackson wants to merge 24 commits into
imikejackson wants to merge 24 commits into
Conversation
imikejackson
force-pushed
the
topic/omit_bounding_box_skin
branch
from
August 13, 2026 18:04
359d4e0 to
a73f1f1
Compare
…kground helpers Add two SECTION blocks to exercise CreateFullyIndexedPolycrystal() and CreateAllBackground() to ensure they work correctly before being used by Task 8 to validate no-op behavior. - CreateFullyIndexedPolycrystal verifies 8 distinct features (1-8), no background - CreateAllBackground verifies all 1728 voxels are feature ID 0
…xterior Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
REQUIRE aborts the test case on first failure, so the reproduction only ever demonstrated one of the two symptoms it documents. CHECK records each failure and continues, so a single run reports all four label-pair outcomes. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…Label
Padding was remapped to 0 and then every 0 was rewritten to -1, so real
Feature 0 interfaces became {-1, k} and box skin over background became
{-1, -1}. Map Padding directly to -1 instead.
Winding is unchanged: isQuadFrontFacing is computed before the remap.
The SurfaceNets exemplar tests are unaffected because the Small IN100 test
dataset has no Feature Id 0 voxels (min=1, max=219), so {Padding, k} faces
are byte-identical either way.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
numJunctions() counts junction face-crossings (0-6), not features meeting at a node, so a normal SurfaceNets node was 0 -- colliding with NodeType::Unused. Compute min(distinct corner labels, 4) + 10 if padding instead, which is the same quantity QuickSurfaceMesh derives from its owner lists over the same 8 voxels. Also removes the non-idempotent CalculatePadding (which turned a twice-visited node from 2 into 12 then 13) and the dead, incorrect getCellLabels. Vertex positions and smoothing are unchanged. Also fixes SurfaceMeshingTest's ImageGeom builders, which created a "CellData" AttributeMatrix but never called setCellData() to register it with the geometry. This left IGridGeometry::getCellData() returning nullptr, which QuickSurfaceMeshFilter::preflightImpl dereferences -- a latent crash that only surfaced once this task's new test exercised QuickSurfaceMesh against the shared cylinder-in-box helper. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Skips wall faces whose Face Labels would be {-1, 0} in both the counting and
emit passes. Wall faces capping a real Feature are still generated, so a
Feature flush with the box stays closed. Node registration sits inside the
same guard, so orphan vertices are never created.
Also factors a shared, templated RunMesher() test helper (plus MeshResult and
CollectLabelPairs) into SurfaceMeshingTestUtils.hpp so the SurfaceNets and
M3C omit-skin tasks can reuse it instead of duplicating a per-filter runner.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adding k_OmitBoundingBoxSkin_Key without bumping parametersVersion() would cause every saved pipeline predating this change to emit a "Parameter key not found in JSON" warning on load, per the existing 1->2 precedent in this same file. Bump 2->3 and document the migration inline. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The predicate runs on raw quadLabels in both the counting and emit passes, where MMSurfaceNet::Padding is still distinct from Feature Id 0. Because SurfaceNets vertices come from the cell map before triangles exist, add an explicit vertex compaction pass for vertices orphaned by dropped faces.
M3C's counting pass is topology-only and never derives per-triangle labels, so the skip cannot happen there without duplicating case-generator logic across three byte-identical sweep paths. Prune the scratch triangle/mCubeID vectors at the top of finalizeMesh instead -- before the +10 promotion and before any output array is sized, so the output geometry is still exactly sized. Orphaned candidate nodes are cleared from nodeType so the existing nodeType > 0 prefix-scan compaction drops them from the vertex list. Node Types after omitting the skin were verified to match the full mesh exactly (divergentCount == 0 on the standard test cylinder), unlike SurfaceNets/QuickSurfaceMesh which make no such claim for M3C but happen to satisfy it here. Generalized SurfaceMeshingTestUtils' RunMesher to work with M3C's differently named Cell Feature Ids parameter key via a FeatureIdsKeyTrait specialization, without forking the helper or touching QuickSurfaceMesh/SurfaceNets call sites. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Adds no-op tests on a fully-indexed volume and an empty-mesh warning (-56340) for all three meshers. Also fixes a bug in 8875a1b5d: M3C's orphan-node cleanup ran whenever the option was merely enabled, even if pruning dropped zero faces, so it stripped pre-existing orphan candidates that vanilla M3C legitimately emits and changed the vertex count on a no-op input. Gate the same cleanup on whether pruning actually changed anything (survivingCount < nTriangle) instead of on the option flag: skip entirely when nothing was dropped (bit-for-bit no-op), otherwise clear every node not referenced by a surviving triangle exactly as before (needed so a partial prune still leaves no orphan vertices, and a full prune still zeroes the mesh).
The existing Node Type Range criterion is per-vertex, so it also erodes a
one-triangle-wide rim wherever an internal surface meets the bounding box.
The new Face Labels mode drops only faces labeled {-1, 0}, which keeps
Features flush with the box closed and fixes meshes already on disk without
re-meshing. Default mode is unchanged.
The SurfaceNets Node Type table listed values (0, 4-8, 13, 14, 15-18) that were artifacts of two now-fixed bugs. All three meshers emit the same 2/3/4 + 10 convention, so the two tables collapse into one. Also documents the Face Labels criterion on ExtractInternalSurfaces and M3C's orphan-vertex behavior when the option prunes at least one face.
…rate case
The Node Type formula ("...capped at 4, plus 10...") was ambiguous about
whether the exterior counts as an owner, inviting an off-by-one Node Type
Range. Made explicit in SurfaceNetsFilter.md, M3CSurfaceMeshingFilter.md,
and ExtractInternalSurfacesFromTriangleGeometryFilter.md that the exterior
is one of the owners, so a plain wall vertex is 12, not 11.
Also documented the all-background degenerate case (warning -56340, zero
vertices/faces, treated as success) in all three mesher docs.
ChoicesParameter keys must end with _index, enforced by nx::core::FilterValidation::Filter Parameter Keys. The key ships as internal_surface_criterion_index; the parametersVersion migration comment is corrected to match.
… Skin Portable env-var shim for MSVC in M3CSurfaceMeshingTest (was POSIX-only, blocked Windows CI); exercise the shipped default (Repair Triangle Winding on) in the all-background degenerate case for all three meshers; make the SurfaceNets/QuickSurfaceMesh Node Type equivalence test exact instead of histogram-based; document the two unconditional SurfaceNets output changes (Face Labels, Node Types); compare Face Labels arrays (not just counts) in the ExtractInternalSurfaces oracle test; add Node Types checks to the Omit-Bounding-Box-Skin no-op sections; restore five pre-existing RunM3COnToy call sites to their original formatting. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…and Node Types
Face Labels no longer conflate Feature Id 0 with the exterior, and Node Types
now use the shared 2/3/4 + 10 convention. Vertices and connectivity are
unchanged from v3.
Regenerated by re-running the exact pipeline stored inside the v3 archive
(SurfaceNetsTest_v3.d3dpipeline) against the current build via nxrunner,
pointed at a new output file. Diffing v3 vs v4 with h5py confirms the only
datasets that changed are the four "Vertex Data/Node Type" arrays; the input
DataContainer, Shared Vertex List, Shared Faces List, and Face Labels are
byte-identical. New Node Type values fall entirely within {2,3,4,12,13,14}.
…vertex bugs * SurfaceNets used voxelSize[1] (Y spacing) instead of voxelSize[2] (Z spacing) when relocating vertices by a half-voxel offset, producing wrong Z coordinates on anisotropic grids. Isotropic exemplar data hid this, so add a dedicated anisotropic-spacing regression test that fails without the fix and passes with it. * QuickSurfaceMesh's createNodesAndTriangles wrote grid corner (i, j+1, k+1) for nodeId4 on the i==0 wall but queried coordinates at (i+1, j+1, k+1), a one-voxel X offset. The same corner is written correctly by later iterations, so this has no observed effect on any existing test or exemplar; fixed as a correctness cleanup. Audited the other five wall branches in both determineActiveNodes and createNodesAndTriangles: no other nodeId/coordinate mismatches found. Signed-off-by: Michael Jackson <mike.jackson@bluequartz.net>
…arameter
The bool "Omit Bounding Box Skin" parameter added to QuickSurfaceMeshFilter,
SurfaceNetsFilter, and M3CSurfaceMeshingFilter on this branch has not shipped
yet. Since this codebase has no parameter migration machinery to safely widen
a bool into an enum later, convert it now to a two-value ChoicesParameter
("bounding_box_skin_mode_index") before it ships, so a plausible future third
mode does not require a silent-default-substituting key change. Only two
choices exist today (Off / Background-Backed Walls Only); no new behavior is
added.
Also fixes a related performance regression: QuickSurfaceMesh's SkipWallFace
predicate took an already-read Feature Id, so the counting pass evaluated a
DataStore read for every boundary voxel even with the option off. It now
takes the store and index so the mode check short-circuits before any read,
while remaining the single shared function called from both the counting and
emit passes (the count/emit agreement is a correctness requirement, not just
style).
Renamed M3C's local `isBackgroundSkin` lambda to `SkipBackgroundSkinFace` to
match the Skip* naming used by the other two meshers' shared predicates, and
renamed the test-only `FeatureIdsKeyTrait<FilterT>::Key` to `::k_Key` for
convention consistency.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…3C orphan-clear, validate Feature Id sentinels Four related changes to the Omit Bounding Box Skin warning/error handling, all touching QuickSurfaceMesh, SurfaceNets, and M3CSurfaceMeshing: 1. De-duplicate the copy-pasted empty-mesh warning (-56340) into MeshingUtilities::MakeEmptyMeshWarning() in TriangleUtilities.hpp/.cpp, named alongside a new k_EmptyMeshAfterSkinRemovalWarning constant. Tests now assert on the named constant. 2. Add a second warning (k_NoFacesPrunedWarning, -56342) emitted when the option is on but suppressed zero faces, since a fully-indexed volume is the most common dataset shape and previously gave no feedback that the option had no effect. QuickSurfaceMesh and SurfaceNets count suppressions in their existing counting passes; M3C compares survivingCount to nTriangle. Never fires alongside the empty-mesh warning. 3. Narrow M3CSurfaceMeshing's orphan nodeType clearing to nodes orphaned BY the prune (referenced by a dropped triangle, referenced by no surviving triangle), leaving pre-existing orphan candidates untouched. Dropped triangles' node ids are recorded in a small vector during the prune loop instead of allocating a second full 7*numSites mask. 4. Validate Feature Ids against each mesher's internal sentinel space (negative values, and INT32_MAX for SurfaceNets/M3C) at the top of each algorithm's execute entry point, rejecting with a clear error naming the value and array path. Mitigation for simplnx#1705, not a fix. Test changes: relaxed M3C's "no orphan vertices" assertion to "no vertex newly orphaned by the prune"; the all-background degenerate test's M3C vertex-count expectation is now the empirically measured pre-existing orphan count (144 for the test fixture) instead of 0; added tests for the no-op-prune warning and the Feature Id sentinel rejections. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…er/corner coverage
Wave 3: repo-wide sweep to rename every "Omit Bounding Box Skin" reference (the
parameter's old bool-era label) to the current "Bounding Box Skin" ChoicesParameter
and its "Background-Backed Walls Only" mode, across warning text, code comments,
test names, and filter docs (24 hits, 11 files).
Test hygiene:
- SurfaceNetsFaceLabelsTest.cpp and SurfaceNetsNodeTypesTest.cpp now build Arguments
via the shared RunMesher helper instead of duplicating the field-by-field block.
- Three TEST_CASEs that never called UnitTest::CheckArraysInheritTupleDims now do,
by having their helpers return the DataStructure instead of discarding it.
- SurfaceMeshingTestUtilsTest.cpp's throwaway CheckArraysInheritTupleDims(DataStructure{})
moved into each SECTION against that section's real dataStructure.
- Added missing REQUIRE_NOTHROW around getDataRefAs<T>() in CollectLabelPairs and in
the SurfaceNets/QuickSurfaceMesh Node Types comparison.
- Fused SurfaceNets' two-pass vertex compaction (assign then copy) into one pass,
preserving the ascending-old-index invariant that makes the in-place copy safe.
New coverage:
- Cross-mesher conformance test: QuickSurfaceMesh, SurfaceNets, and M3C agree on the
Face Label pair set (not counts/arrays, which legitimately differ) with the mode
both off and on; watertightness is asserted only for the "on" mode, since "off"
has a pre-existing T-junction where an internal boundary meets the box wall.
- Corner-Feature watertightness test: a Feature occupying a box corner so three
suppressed wall planes meet at a right angle. Passes on all three meshers with no
edge-use defects found.
ctest -R "SimplnxCore::" 974/974 (was 972/972); ctest -R "FilterValidation" 5/5.
Final grep for "Omit Bounding Box Skin" under src: 0 hits.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…tertightness, refresh stale example pipelines * M3CSurfaceMeshingFilter.md: the "clears all pre-existing orphan vertices" claim no longer matches finalizeMesh() -- the option now narrows its clear to only the vertices its own prune orphaned. Rewrote the note to describe the current behavior and why -56340 reports a remaining vertex count instead of assuming zero. * SurfaceNetsFilter.md: added the previously-undocumented Z-offset fix (Z component used Y spacing) to "Changed Behavior for Existing Pipelines". * All three mesher docs: documented the new -56342 no-op warning, the -56343 Feature Id sentinel validation error (with the accepted range per mesher), and the manifold-vs-non-manifold T-junction benefit asserted by OmitBoundingBoxSkinTest.cpp's cross-mesher conformance test. * QuickSurfaceMeshFilter.md: added the "exterior counts as an owner" Node Type explanation (parity with the other two mesher docs) and a note on Node Type stability across the prune. * LaplacianSmoothingFilter.md / HierarchicalSmoothFilter.md: noted that smoothing a Surface Nets mesh now behaves differently because Surface Nets' Node Types were corrected to the shared convention -- a correction, not a regression. * Round-tripped 5 stale example pipelines (Small IN100 Quick Mesh, Compute_Feature_Face_Curvature_Small_IN100, SurfaceNets_Demo, M3C_Demo, Porosity Mesh Export) through Pipeline::FromFile/toJson via the simplnx Python bindings, restoring the pinnedParams/ workflowParams GUI fields the core toJson() omits, to clear the -5432 "Parameter key not found" warnings this branch reintroduced. Signed-off-by: Michael Jackson <mike.jackson@bluequartz.net>
…scription LaplacianSmoothingFilter.md had a stale DREAM3D 6.x NodeType table (SurfaceMesh namespace, Unused = -1) and a sentence reconciled to it instead of to the actual code, where nx::core::NodeType::Unused is 0. Corrected the table and sentence to match INodeGeometry2D.hpp and LaplacianSmoothing.cpp's switch statement. HierarchicalSmoothFilter.md described the interval-bisection loop's halt condition backwards relative to HierarchicalSmooth.cpp's smooth(): the loop continues while the objective's slope is significant and halts once it goes flat (converged), not the reverse. Kept the M3CSurfaceMeshingFilter.md edit from a prior interrupted sweep, which correctly distinguishes M3C's zero-faces-but-not-necessarily-zero-vertices degenerate case from QuickSurfaceMesh/SurfaceNets. Also reviewed QuickSurfaceMeshFilter.md, SurfaceNetsFilter.md, and ExtractInternalSurfacesFromTriangleGeometryFilter.md end to end against source and tests; no further discrepancies found. Docs build remains at the 7 pre-existing warnings baseline; ctest SimplnxCore:: remains 974/974. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…, address remaining review nits Final review-gate fixes for Omit Bounding Box Skin: - Restate the -56342 warning (code, doc comment, and all three mesher docs) to say only what is measured -- no bounding-box wall face is background-backed -- not that the volume has no background voxels. Add CreateEnclosedPorosity() and a covering test for the interior-porosity counterexample this distinction depends on. - Fix the remaining "terminates early" self-contradiction in HierarchicalSmoothFilter.md. - Make ValidateFeatureIdsAgainstSentinels report progress and poll cancellation every 1M tuples instead of running an uninterruptible, silent full-volume scan; update all three call sites. - Correct the corner-case test attribution in the mesher docs, add absolute label-pair assertions to the cross-mesher agreement test, use the named M3CNodeType::k_Unused instead of a literal 0, pass the named BoundingBoxSkinMode constants instead of bool literals at every RunMesher call site, and finish the REQUIRE_NOTHROW/CheckArraysInheritTupleDims hygiene sweep. - Reserve droppedNodeIds before M3C's orphan-node compaction loop, and drop TriangleUtilities.hpp's ChoicesParameter.hpp include now that the mode constants are plain uint64. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
imikejackson
force-pushed
the
topic/omit_bounding_box_skin
branch
from
August 13, 2026 18:15
a73f1f1 to
d754c27
Compare
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
Adds a Bounding Box Skin
ChoicesParameterto Create Surface Mesh (QuickMesh), Create Surface Mesh (Surface Nets) and M3C Surface Meshing, with modes Off (default) and Background-Backed Walls Only.Mode 1 suppresses a bounding box wall face only where the voxel behind it is background (Feature Id 0). Wall faces that cap a real Feature are still generated — that cut plane is the only possible closure for a Feature flush with the box — so such Features stay closed and watertight. Because the test is per face rather than per vertex, no triangles are lost along the rim where an internal boundary meets the wall, which is the erosion the existing Node Type Range criterion produces.
INT32_MAX.Changed behavior for existing pipelines (not gated by the new option)
{-1, 0}(was{-1, -1}), and Feature-0-vs-Feature-k faces are{0, k}(was{-1, k}).Unused), pinning their smoothing weight to 0.0. Neither filter's tests use a Surface Nets mesh, so neither could detect this.Surface Nets exemplars are regenerated as
SurfaceNetsTest_v4.tar.gz(uploaded to the Data_Archive release); only the four Node Type arrays differ from v3. Note the exemplar dataset contains no Feature Id 0 voxels, so exemplars cannot exercise the new option — the synthetic tests added here are its only coverage.Related follow-up issues: #1704, #1705, #1706, #1707.
Test Plan
ctest -R "SimplnxCore::"fully green, including new synthetic tests: no-op on fully-indexed volumes (bit-identical output per mesher), enclosed-background no-op, all-background degenerate case, flush-cylinder and box-corner watertightness, cross-mesher Face-Label conformance, QuickMesh↔Surface Nets Node Type corner-for-corner equivalence, anisotropic-spacing vertex Z, Feature Id sentinel rejectionnx::core::FilterValidationsuite green (parameter key conventions)-5432warnings