BUG: Fix infinite loop in VoronoiDiagram2DGenerator

[hjmjohnson]

Summary

Fix an infinite loop in VoronoiDiagram2DGenerator::ConstructDiagram() that
occurs with certain degenerate seed configurations (ITK issue #4386).

Supersedes PR #5400 by @ILoveGoulash with an improved termination strategy
that preserves correctness for all existing tests.

Root Cause

ConstructDiagram() builds Voronoi cell boundaries by assembling raw edges
into a connected chain. It pops an edge from the deque, tries to attach it
to the front or back of the growing chain, and pushes it back if it cannot
attach. With certain seed configurations, some edges can never be attached
(due to the geometric structure of the Voronoi diagram near boundary
corners), causing the loop to cycle endlessly.

The loop exit condition (maxStop) was originally present but was never
wired into the while condition. Commit cd97879 (2023) removed it as an
"unused variable."

Why PR #5400's fix was insufficient

PR #5400 restored maxStop as a simple countdown (maxStop = rawEdges[i].size(),
decrement each iteration). This limits the loop to one pass through the deque,
but is too aggressive: when an edge successfully attaches, it changes the
chain's front/back endpoints, potentially making previously unattachable edges
now attachable. The simple countdown drops these edges prematurely.

Evidence: With PR #5400's approach, itkVoronoiPartitioningImageFilterTest2
fails (17 pixels differ, mean error 29.1) because edges that would have been
attachable in a subsequent pass were dropped.

This fix

This PR uses a stall-detection strategy instead of a simple countdown:

auto remainingBeforeStall = rawEdges[i].size();
while (!(rawEdges[i].empty()) && (remainingBeforeStall != 0))
{
  --remainingBeforeStall;
  // ... try to attach edge ...
  bool edgeAttached = /* true if any branch consumed the edge */;
  if (edgeAttached)
  {
    // Progress was made — previously unattachable edges may now fit.
    remainingBeforeStall = rawEdges[i].size();
  }
}

The counter resets whenever an edge attaches. The loop only terminates when
a full pass through the remaining edges makes zero progress — proving
that no further edges can ever attach. This is both correct (preserves the
algorithm's ability to attach edges in multiple passes) and safe (guaranteed
O(n^2) termination in the worst case).

Test Results

Test	PR #5400	This PR
itkVoronoiDiagram2DInfiniteLoopTest	Pass	Pass
itkVoronoiSegmentationImageFilterTest	Pass	Pass
itkVoronoiSegmentationRGBImageFilterTest	Pass	Pass
itkVoronoiDiagram2DTest	Pass	Pass
itkVoronoiPartitioningImageFilterTest1	Pass	Pass
itkVoronoiPartitioningImageFilterTest2	Fail (17 px)	Pass

8/8 Voronoi tests pass with this fix.

AI Assistance

Claude Code (Opus) was used to:

• Analyze the algorithm and identify the root cause of the infinite loop
• Identify the flaw in PR BUG: fix an infinite loop in itk::VoronoiDiagram2DGenerator #5400's simple-countdown approach
• Implement the stall-detection fix and verify it against all existing tests
• Write the regression test and this PR description

All analysis and changes were reviewed and validated by the commit author.

Testing

cmake --build cmake-build-Release --target ITKVoronoiTestDriver -j$(nproc)
cd cmake-build-Release && ctest -R Voronoi --output-on-failure
# 100% tests passed, 0 tests failed out of 8

Closes #4386.

[dzenanz]

Looks good on a glance. It would be good if somebody else reviewed too.

[hjmjohnson]

Investigation Report: Unattachable Edges in VoronoiDiagram2DGenerator

Experiment Setup

Added diagnostic instrumentation to ConstructDiagram() to log:

• Which cells drop edges, how many, and their coordinates
• The chain state (front/back vertex) at time of stall
• Boundary classification (Pointonbnd) of all involved vertices
• Full cell boundary polygons for all 6 cells

Ran all 8 Voronoi tests with instrumentation.

Key Finding: Only 1 Edge Dropped, Only in the Degenerate Case

All existing Voronoi tests (normal seed configurations) produce zero dropped edges. Only the degenerate infinite-loop test (issue #4386) drops a single edge in cell 2.

The Dropped Edge — Detailed Analysis

Cell 2: 1 unattachable edge dropped
  Chain: front=back=vertex 0 (-1.58976, -0.0852448)  [single-vertex closed loop]
  Dropped: vertex 5 (-1.61569, -0.0039021) → vertex 2 (-1.61566, -0.00388532)
           Both endpoints on boundary 1 (left wall: x = -1.61569)
           Chain endpoints NOT on boundary (frontbnd=0, backbnd=0)

What happened: The chain for cell 2 assembled into a closed loop (front == back == vertex 0) before this edge was processed. The edge connects two points on the left boundary wall that are extremely close together (Δx = 0.00003, Δy = 0.00002). Neither endpoint matches the chain's single vertex, and the boundary-matching logic doesn't apply because the chain endpoints are not on a boundary.

Why This Edge is Unattachable

The edge 5→2 is unattachable because:

1. The chain is already closed — front[0] == back[1] == vertex 0. There is no open end to attach to.
2. No endpoint match — Neither vertex 5 nor vertex 2 equals vertex 0 (direct attach fails).
3. No boundary bridging — The chain endpoints are interior (not on domain boundary), so the boundary-matching branches (frontbnd != 0 || backbnd != 0) don't fire.

Is the Edge Redundant?

Looking at the final cell boundaries:

Cell 2: [0]→[4]→[2]→[1]→[6]  (5 vertices, includes vertex 2)
Cell 5: [7]→[12]→[5]→[2]      (4 vertices, includes both vertex 5 and 2)

Vertex 2 is already in cell 2's boundary (attached via a different path). Vertex 5 is in cell 5's boundary. The dropped edge 5→2 would represent a shared boundary segment between cells 5 and 2, which is already represented by the vertex 2 being shared between both cells. The edge is topologically redundant — it describes a boundary segment that both cells already incorporate through their other edges.

Is the Result Geometrically Correct?

The 6 seeds near-collinear along x ≈ -1.2 to -1.4 create Voronoi cells that are extremely elongated. The two vertices 5 and 2 on the left boundary are only 0.00003 apart — this is a near-degenerate edge from Fortune's algorithm. In exact arithmetic, this edge likely has zero length (the two boundary intersection points are the same point). The floating-point discrepancy creates an infinitesimally short edge that cannot be assembled because the chain has already closed through a different topological path.

Conclusion: The dropped edge is a floating-point artifact. Dropping it produces the correct Voronoi diagram. The stall-detection fix is the right approach — it terminates when no progress can be made, and the only edges dropped are these degenerate near-zero-length artifacts.

Verification

• All 8 existing Voronoi tests pass ✓
• No edges dropped in any non-degenerate test ✓
• Cell polygons are closed and correctly partition the domain ✓
• itkVoronoiPartitioningImageFilterTest2 passes (which the simpler PR BUG: fix an infinite loop in itk::VoronoiDiagram2DGenerator #5400 countdown broke) ✓

[greptile-apps]

Greptile Summary

This PR correctly fixes the infinite loop in VoronoiDiagram2DGenerator::ConstructDiagram() (ITK issue #4386) using stall-detection combined with explicit degenerate-edge dropping — the algorithm fix is sound and all 8 Voronoi tests pass. However, Modules/Segmentation/Voronoi/test/CMakeLists.txt has two build-breaking P1 bugs that must be fixed before merging:

• itkVoronoiDiagram2DInfiniteLoopTest.cxx is listed twice in the ITKVoronoiTests source set, causing a duplicate-symbol linker error.
• itk_add_test(NAME itkVoronoiDiagram2DInfiniteLoopTest ...) is registered twice, causing CMake to fail at configure time.

Confidence Score: 4/5

Not safe to merge until the two duplicate CMakeLists.txt entries are removed — they will break the build on all platforms.

The algorithm fix in itkVoronoiDiagram2DGenerator.hxx is correct and well-tested, but two P1 build-breaking bugs in CMakeLists.txt (duplicate source file entry and duplicate itk_add_test registration) prevent CI from passing. Confidence would be 5 once those two lines are removed.

Modules/Segmentation/Voronoi/test/CMakeLists.txt requires removal of one duplicate source entry (line 9) and one duplicate itk_add_test block (lines 74–79).

Important Files Changed

Filename	Overview
Modules/Segmentation/Voronoi/test/CMakeLists.txt	Two P1 build-breaking bugs: test source listed twice in ITKVoronoiTests, and itk_add_test registered twice with the same test name
Modules/Segmentation/Voronoi/include/itkVoronoiDiagram2DGenerator.hxx	Stall-detection fix and degenerate-edge drop are algorithmically correct; post-loop itkExceptionMacro is a minor concern for unusual valid configurations
Modules/Segmentation/Voronoi/test/itkVoronoiDiagram2DInfiniteLoopTest.cxx	Well-written regression test with clear root-cause comments and a concrete seed reproducer for issue #4386

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["Start cell i: remainingBeforeStall = deque.size()"] --> B{"deque empty OR\nremainingBeforeStall == 0?"}
    B -- Yes --> G["Post-loop: deque empty?"]
    B -- No --> C["--remainingBeforeStall\npop front edge"]
    C --> D{"differentPoint check:\nedge is degenerate?"}
    D -- "Yes: drop" --> E["remainingBeforeStall = deque.size()\ncontinue"]
    E --> B
    D -- No --> F{"Try to attach edge\nto front or back of chain"}
    F -- Attached --> H["remainingBeforeStall = deque.size()"]
    F -- "Not attached" --> I["push_back edge to deque"]
    H --> B
    I --> B
    G -- "Not empty" --> J["itkExceptionMacro:\nunexpected failure"]
    G -- Empty --> K["Finalize cell boundary"]

Loading

Comments Outside Diff (1)

1. Modules/Segmentation/Voronoi/test/CMakeLists.txt, line 74-79 (link)

   Duplicate itk_add_test registration

   itkVoronoiDiagram2DInfiniteLoopTest is already registered at lines 14–19. CMake will fail at configure time with "Cannot add test with already existing name," preventing the project from configuring on any platform. Remove this second block entirely; the first registration is sufficient.

_{Reviews (1): Last reviewed commit: "BUG: Fix infinite loop in VoronoiDiagram..." | Re-trigger Greptile}