ENH: Ingest ITKSubdivisionQuadEdgeMeshFilter into Modules/Filtering (v4, stacks on #6204)#6229
Merged
hjmjohnson merged 61 commits intoInsightSoftwareConsortium:mainfrom May 7, 2026
Conversation
github-actions
Bot
added
type:Infrastructure
hjmjohnson
force-pushed
the
ingest-SubdivisionQuadEdgeMeshFilter
branch
from
c1d39ce to
1ffc1f2
Compare
hjmjohnson
changed the title
Splits the v3 single-driver flow into two independent phases so the
upstream-archival publish step never runs on a working tree that
filter-repo has rewritten:
- Phase A (ingest-module-v4.sh): local, throwaway clone; rewrites
history into Modules/<Group>/<Module>; never touches the upstream
remote.
- Phase B (archive-remote-module.sh): fresh shallow clone; pushes
one removal commit + promotes MIGRATION_README.md to README.md;
flips archived=true. Gated on the Phase A PR being merged.
The shared sanitize-history.py walks the rewritten branch and applies
the same hooks ITK's .pre-commit-config.yaml enforces:
- clang-format on every C/C++ blob (content sniff)
- black on every Python blob (content sniff)
- gersemi on every CMake blob using ITK's .gersemi.config
- trailing-whitespace, end-of-file-fixer, mixed-line-ending on
every text-classified blob
- VTK / SVG / hash-content sidecar files are skipped intact
- Every commit subject without an ITK prefix gets one heuristically
auto-prepended; decisions are logged for operator audit
filter-repo's blob_callback is filename-blind by design, so excludes
are approximated via content sniffing. IOMeshSTL whitelist is the
first concrete example; smoke-tested via --dry-run successfully
(143->89 commits, 44->24 merges preserved, 108/149 blobs reformatted).
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The whitelist admits whole directories (test/, wrapping/, ...) but
some upstream remote modules place CI / packaging scaffolding inside
those (e.g. test/Docker/, .github/, azure-pipelines/). Without a
deny-pass after the whitelist filter, those leak into ITK history.
Mirrors v3's --invert-paths --path-glob pass; discovered missing on
the first Cuberille v4 ingest (test/Docker/{Dockerfile,build,run,test}.sh
leaked).
Fixes the four classes of ghostflow errors that surfaced on the first Cuberille v4 ingest (PR InsightSoftwareConsortium#6205) — the only allowable remaining error should be the unavoidable upstream root commit. - Strip *.orig / *.rej / *.BACKUP.* / *.LOCAL.* / *.REMOTE.* / *.BASE.* merge-conflict-artifact files from history (catches leftover-conflict-marker errors like the .h.orig on Cuberille). - sanitize-history.py: cap every commit subject at 78 chars (ghostflow / kw-commit-msg rule); excess words move into the body. - sanitize-history.py: insert a blank line between subject and body when the original commit message lacked one (two-line subject error). - sanitize-history.py: clear the executable bit (mode 100755 -> 100644) on text-file extensions so root-commit "executable permissions but file does not look executable" stops firing.
Many ITK remote modules' itk-module.cmake reads
file(READ "${MY_CURRENT_DIR}/README.rst" DOCUMENTATION)
to populate the CMake DESCRIPTION variable from the upstream README.
The v4 whitelist intentionally excludes the upstream README (the
operator ships a new README.md as part of the ingest PR), so without
this rewrite every commit's CMakeLists fails to configure post-ingest
until the operator manually patches itk-module.cmake.
sanitize-history.py now rewrites the file(READ ... README.rst ...)
reference to README.md as part of the cmake-blob transform, so every
historical commit on the rewritten branch is independently buildable.
Surfaced on the IOMeshSTL ingest (PR following PR InsightSoftwareConsortium#6204).
…terns
Add a 'Known artifacts at PR-review time' section to capture two
things future ingest operators need to expect but that v3/v4 do not
fix automatically:
- The single ghostflow-check-main 'root commit not allowed' error
that every Mode-A merge produces. Maintainers override at merge.
Calling it out keeps operators from chasing a non-fix.
- A short table of code-level patterns Greptile has flagged
post-ingest on multiple modules (IOMeshSTL InsightSoftwareConsortium#6206, RLEImage InsightSoftwareConsortium#6208):
signed-vs-unsigned size types, missing override, dead return after
itkExceptionMacro, stray test args, external-friend declarations,
GetBuffer-const correctness, and allocator-init invariants.
The table is the durable channel for cross-ingest review knowledge —
extend it as new recurring patterns surface.
ExternalData stores resolved fetched content as .ExternalData_<algo>_<hash> files alongside the .cid / .md5 sidecars they correspond to. These are local fetch cache, not source — ExternalData regenerates them on demand at consumer-time. Upstream remote modules sometimes commit them inadvertently from a CTest run inside their working tree (e.g. ITKIOMeshSTL had four such files in test/Baseline/, flagged by @dzenanz on PR InsightSoftwareConsortium#6206). Add **/.ExternalData_* to the scaffolding deny-pass so they never enter ITK history.
Upstream remote modules wrap their top-level CMakeLists.txt in
if(NOT ITK_SOURCE_DIR)
find_package(ITK REQUIRED)
list(APPEND CMAKE_MODULE_PATH ${ITK_CMAKE_DIR})
include(ITKModuleExternal)
else()
itk_module_impl()
endif()
so the same file works both as a fetched remote module and as a
stand-alone CMake project. In an in-tree ingested module
ITK_SOURCE_DIR is always defined, so the if-branch is dead code;
@dzenanz flagged it on PR InsightSoftwareConsortium#6206 (IOMeshSTL).
Add patch_standalone_build_guard() that detects the idiom (allowing
2- or 4-space indent variants) and replaces it with a bare
itk_module_impl() before gersemi formatting runs. Track count via
sanitizer.standalone_guard_patches and print it in the run summary.
The 'Greptile post-ingest patterns' table mixed mechanical artifacts (strippable in Phase A) with semantic concerns (require human judgment). Split into two sections so the operator's checklist shrinks as the sanitizer learns more rules. Promote .ExternalData_* and the standalone-build CMake guard to the auto-sanitized table now that the previous two commits handle them.
Upstream remote modules typically begin their top-level CMakeLists.txt with their own cmake_minimum_required(VERSION X.Y.Z) declaration, often 3.10.2 or earlier. ITK's top-level CMakeLists pins a higher minimum, so the per-module line is redundant and frequently *lower* than ITK's floor. @dzenanz flagged it on every ingest with the comment 'Version behind, not needed.' — most recently on PR InsightSoftwareConsortium#6215 (IOFDF). Add patch_drop_cmake_minimum_required() that detects the idiom and removes the matching line via a multiline regex. Track count via sanitizer.cmake_min_required_drops and print it in the run summary. Apply alongside patch_standalone_build_guard() in the cmake-blob branch of blob_callback.
…cmake
Archival README.md files contain semicolons and bracket characters.
CMake's foreach(arg \${ARGN}) splits on semicolons, so reading them
into a DESCRIPTION argument produces CMake (dev) configure warnings
for every module ingested via v4 (observed: RLEImage, SplitComponents,
IOFDF, IOMeshMZ3, IOMeshSTL).
Add patch_dynamic_description() to sanitize-history.py to strip the
get_filename_component/file(READ README.md) preamble and replace
DESCRIPTION "\${DOCUMENTATION}" with a static one-liner during history
rewrite. Document the pattern in INGESTION_STRATEGY_v4.md.
Fixes link errors.
This check replaces default bodies of special member functions with = default;. The explicitly defaulted function declarations enable more opportunities in optimization, because the compiler might treat explicitly defaulted functions as trivial. Additionally, the C++11 use of = default more clearly expreses the intent for the special member functions.
The mission of NumFOCUS is to promote open practices in research, data, and scientific computing. https://numfocus.org
The iterative subdivision, conditional subdivision, and criteria classes provided by this module are templated over some subset of the provided subdivision filters (rather than simply being templated over the QuadEdgeMesh type). The current wrappers provide incorrect template arguments to these classes, causing a failure of compilation when the wrapping is enabled. This patch corrects these errors, allowing the build to compile.
The Delaunay Conforming filter in the testing suite is causing a number of test failures. This patch disables the Delaunay Conforming filter temporarily while the bug is investigated, and adds a FIXME comment explaining the change.
This patch expands test coverage, modernizes test syntax, and prefers the use of ITK testing macros to manual testing. Line coverage is improved from 83.9% to 88.6%. Function coverage is improved from 62.7% to 80.8%.
Currently, any CellData in the input mesh is not passed to the subdivided output mesh. This patch checks if any CellData exists, and if so, passes that data into the child cells of the subdivided output mesh. This patch creates a new method, PassCellData, which is called immediately following each call to AddFaceTriangle. Moreover, the interfaces to SplitTriangleFromOneEdge, SplitTriangleFromTwoEdges, and SplitTriangleFromThreeEdges have all been extended to take the cell identifier of the input cell, so that it will be available to pass to the PassCellData method. This has also required that the class for iterative subdivision (IterativeCellSubdivisionQuadEdgeMeshFilter) be updated.
1. Test an untested branch of itkTriangleCellSubdivisionQuadEdgeMeshFilterTest
so as to exercise AddSubdividedCellId().
2. Add assertions to check the functionality of SetCellsToBeSubdivided()
and AddSubdividedCellId().
3. Reduce redundancy in the CMakeLists.txt file for the testing suite by
using foreach loops as appropriate.
4. Minimal improvement in testing coverage.
Line coverage: 88.9% => 89.6%
Function coverage: 81.2% => 84.0%
This patch addresses warnings due to: 1. Unused local typedefs. 2. Comparison of strings with string literals. Warnings were emitted on g++ 9.3.0 when compiling with Wall, Wextra, Wmissing-braces.
Fixes changes made in InsightSoftwareConsortium#2053. ITK_DISALLOW_COPY_AND_ASSIGN will be used if ITK_FUTURE_LEGACY_REMOVE=OFF.
The ability to include either .h or .hxx files as header files required recursively reading the .h files twice. The added complexity is unnecessary, costly, and can confuse static analysis tools that monitor header guardes (due to reaching the maximum depth of recursion limits for nested #ifdefs in checking).
When preparing for the future with ITK by setting ITK_FUTURE_LEGACY_REMOVE:BOOL=ON ITK_LEGACY_REMOVEBOOL=ON The future preferred macro should be used │ - itkTypeMacro │ + itkOverrideGetNameOfClassMacro
For the sake of code readability, a new 'CoordinateType' alias is added for each nested 'CoordRepType' alias. The old 'CoordRepType' aliases will still be available with ITK 6.0, but it is recommended to use 'CoordinateType' instead. The 'CoordRepType' aliases will be removed when 'ITK_FUTURE_LEGACY_REMOVE' is enabled. Similarly, 'InputCoordinateType', 'OutputCoordinateType', and 'ImagePointCoordinateType' replace 'InputCoordRepType', 'OutputCoordRepType', and 'ImagePointCoordRepType', respectively.
hjmjohnson
force-pushed
the
ingest-SubdivisionQuadEdgeMeshFilter
branch
from
1ffc1f2 to
58da70a
Compare
dzenanz
approved these changes
May 7, 2026
Contributor
|
| Filename | Overview |
|---|---|
| Modules/Filtering/SubdivisionQuadEdgeMeshFilter/include/itkLoopTriangleCellSubdivisionQuadEdgeMeshFilter.hxx | Two defects: wrong ID type inserted into smoothedPointSet (cell ID instead of point ID), and a large dead commented-out block from a prior refactor. |
| Modules/Filtering/SubdivisionQuadEdgeMeshFilter/include/itkTriangleCellSubdivisionQuadEdgeMeshFilter.hxx | GenerateOutputCells() uses continue without advancing cellIt; any non-triangle cell in the input mesh causes an infinite loop. |
| Modules/Filtering/SubdivisionQuadEdgeMeshFilter/include/itkSquareThreeTriangleCellSubdivisionQuadEdgeMeshFilter.hxx | GenerateOutputCells() has the same continue-without-advance infinite loop as itkTriangleCellSubdivisionQuadEdgeMeshFilter.hxx. |
| Modules/Filtering/SubdivisionQuadEdgeMeshFilter/itk-module.cmake | New module declaration with correct DEPENDS on ITKQuadEdgeMesh/ITKQuadEdgeMeshFiltering, EXCLUDE_FROM_DEFAULT, and test dependencies; no issues. |
| Utilities/Maintenance/RemoteModuleIngest/sanitize-history.py | New v4 ingestion script; CID file detection regex matches only hex hashes (not base32 CIDv1), so .cid files receive text-only whitespace normalization rather than being skipped entirely — harmless in practice. |
| Utilities/Maintenance/RemoteModuleIngest/ingest-module-v4.sh | New Phase A driver shell script; uses set -euo pipefail, proper argument parsing, and preflight checks for required tools. |
| Modules/Remote/SubdivisionQuadEdgeMeshFilter.remote.cmake | Correctly removed — the in-tree module replaces the remote fetch entry. |
| Modules/Filtering/SubdivisionQuadEdgeMeshFilter/test/CMakeLists.txt | Test driver and parametric test entries look correct; covers cell- and edge-based methods plus criterion-driven tests. |
| pyproject.toml | Adds -DModule_SubdivisionQuadEdgeMeshFilter:BOOL=ON to the CI configure command; correct and expected for an in-tree module. |
Class Diagram
%%{init: {'theme': 'neutral'}}%%
classDiagram
class SubdivisionQuadEdgeMeshFilter {
+GenerateOutputPoints()
+GenerateOutputCells()
+GenerateData()
#m_EdgesPointIdentifier
}
class TriangleCellSubdivisionQuadEdgeMeshFilter {
+GenerateOutputPoints()
+GenerateOutputCells()
+SetCellsToBeSubdivided()
+AddSubdividedCellId()
#m_CellsToBeSubdivided
#m_Uniform
}
class TriangleEdgeCellSubdivisionQuadEdgeMeshFilter {
+GenerateOutputPoints()
+GenerateOutputCells()
}
class ConditionalSubdivisionQuadEdgeMeshFilter {
+SetCriterion()
}
class LoopTriangleCellSubdivisionQuadEdgeMeshFilter {
+AddNewCellPoints()
+CopyInputMeshToOutputMeshPoints()
+SmoothingPoint()
}
class SquareThreeTriangleCellSubdivisionQuadEdgeMeshFilter {
+AddNewCellPoints()
+GenerateOutputCells()
}
SubdivisionQuadEdgeMeshFilter <|-- TriangleCellSubdivisionQuadEdgeMeshFilter
SubdivisionQuadEdgeMeshFilter <|-- TriangleEdgeCellSubdivisionQuadEdgeMeshFilter
TriangleCellSubdivisionQuadEdgeMeshFilter <|-- ConditionalSubdivisionQuadEdgeMeshFilter
TriangleCellSubdivisionQuadEdgeMeshFilter <|-- LoopTriangleCellSubdivisionQuadEdgeMeshFilter
TriangleCellSubdivisionQuadEdgeMeshFilter <|-- SquareThreeTriangleCellSubdivisionQuadEdgeMeshFilter
Reviews (1): Last reviewed commit: "ENH: Enable Module_SubdivisionQuadEdgeMe..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
In the non-uniform adaptive path of LoopTriangleCellSubdivisionQuadEdgeMeshFilter, the inner loop was inserting *it (the OutputCellIdentifier from m_CellsToBeSubdivided) instead of *pter (the InputPointIdentifier from the cell's PointIds iterator) into smoothedPointSet. The set is later checked by point index via smoothedPointSet.count(ptIt->Index()), so cell IDs and point IDs ended up compared across types — the smoothing step was silently skipped for nearly every vertex it should have moved. Caught by Greptile review on PR InsightSoftwareConsortium#6229 ingest of ITKSubdivisionQuadEdgeMeshFilter. A regression test that constructs a small in-memory mesh and exercises the non-uniform adaptive Loop path follows in a subsequent commit.
In TriangleCellSubdivisionQuadEdgeMeshFilter::GenerateOutputCells() and SquareThreeTriangleCellSubdivisionQuadEdgeMeshFilter::GenerateOutputCells(), the guard branch that skips non-triangle / null cells used `continue` to restart the while-loop body, but the `++cellIt` lived at the bottom of the loop. The first non-triangle (or null) cell encountered caused an infinite loop on the same iterator. Existing tests pass because the venus.vtk regression mesh is purely triangular, but any input with a mixed cell topology hangs both filters. Caught by Greptile review on PR InsightSoftwareConsortium#6229. Regression test follows in a subsequent commit.
…t helper The non-uniform adaptive Loop path retained a 44-line commented-out block of the original inline neighborhood-smoothing computation. The live code beneath it now delegates to the SmoothingPoint() helper that encodes the identical algorithm; the comment was a refactor artifact, not an alternative implementation. Caught by Greptile review on PR InsightSoftwareConsortium#6229.
Adds itkSubdivisionQuadEdgeMeshFilterRegressionTest covering the three regressions caught by Greptile review on PR InsightSoftwareConsortium#6229's ingest: 1. Loop non-uniform adaptive smoothing — constructs a 4-point, 2-cell mesh, sets CellsToBeSubdivided = {0}, asserts that all three vertices of cell 0 (point IDs 0, 1, 2) are smoothed. Before the fix the smoothedPointSet contained {0} (the cell ID), so only the coincidentally-indexed point ever moved. 2. TriangleCellSubdivision termination — injects a 4-point polygon cell into the cells container alongside the triangles and runs LinearTriangleCellSubdivisionQuadEdgeMeshFilter. The CTest TIMEOUT of 30 seconds catches the infinite-loop regression. 3. SquareThreeTriangleCellSubdivision termination — same shape, runs SquareThreeTriangleCellSubdivisionQuadEdgeMeshFilter on the same mixed-cell mesh. The existing venus.vtk-driven tests cannot trigger any of these because that mesh is purely triangular and is exercised only through the uniform-subdivide-all path.
dzenanz
approved these changes
May 7, 2026
hjmjohnson
merged commit e18a952
into
InsightSoftwareConsortium:main
19 of 20 checks passed
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
9 participants
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.
Ingest the standalone remote module
InsightSoftwareConsortium/itkSubdivisionQuadEdgeMeshFilterinto
Modules/Filtering/SubdivisionQuadEdgeMeshFilter/via the v4ingestion pipeline introduced in #6204. Triangle/edge-based
QuadEdgeMesh subdivision filters (Loop, Modified Butterfly, Square
Three, Linear) are now in-tree under
EXCLUDE_FROM_DEFAULT.Stacked on #6204 (do not merge until #6204 lands). Closes the
consolidation work of issue #6060 for this module.
Ingest results (v4)
Utilities/Maintenance/RemoteModuleIngest/whitelists/SubdivisionQuadEdgeMeshFilter.listinclude/,test/,CMakeLists.txt,itk-module.cmake).cid, 0.md5, 0.shaNNNtest/examples/directorygit blameSanitize-history.py counters
Every commit subject in the rewritten history carries a valid ITK
prefix; ghostflow's only unfixable error remains the upstream root
commit (intrinsic to every remote module).
Local validation
pre-commit run --all-files: cleancmake -DModule_SubdivisionQuadEdgeMeshFilter:BOOL=ON .: configures cleanninja SubdivisionQuadEdgeMeshFilter-all: builds cleanctest -R SubdivisionQuadEdgeMeshFilter: 37/37 tests passNo manual
itk-module.cmakefix was needed — v4's sanitize stepauto-converts the upstream
file(READ README.rst DESCRIPTION)patternto a static
set(DOCUMENTATION ...)block.Commits at PR tip (top → bottom, above #6204's tip `c7768a5377`)
ENH:Enable Module_SubdivisionQuadEdgeMeshFilter in configure-ciCOMP:Remove SubdivisionQuadEdgeMeshFilter .remote.cmake (in-tree)DOC:Add SubdivisionQuadEdgeMeshFilter README pointing at upstreamENH:Ingest ITKSubdivisionQuadEdgeMeshFilter into Modules/Filtering(merge commit
--no-ff --allow-unrelated-historiesto whitelisted +sanitized filter-repo'd upstream history; preserves 12 upstream
merges and
git blameto original authors)ENH:Add v4 whitelist for SubdivisionQuadEdgeMeshFilter ingestPhase B (post-merge) — upstream archival
After this PR merges, run Phase B from a fresh worktree:
Utilities/Maintenance/RemoteModuleIngest/archive-remote-module.sh \ SubdivisionQuadEdgeMeshFilterPhase B publishes the upstream removal commit, promotes
MIGRATION_README.mdtoREADME.mdso the GitHub landing pagerenders the migration notice (per the
ingest-archive-readmerule), and flipsarchived=true. No upstream side-effects happen until that step.Refs: #6060, #6204