Fix crossingsRemoved failures on self-intersecting and multi-component paths - #143
Open
hfutrell wants to merge 11 commits into
Open
Fix crossingsRemoved failures on self-intersecting and multi-component paths#143hfutrell wants to merge 11 commits into
hfutrell wants to merge 11 commits into
Conversation
…ng iteration budget - testCrossingsRemovedTwoOverlappingCircles: two near-circular overlapping components; checks that crossingsRemoved preserves the union bounding box. - testCrossingsRemovedTwoOverlappingCircles2: similar test with an irregular 5-segment second component. - testCrossingsRemovedFourthRealWorldCase: 27-element self-intersecting path that previously caused crossingsRemoved to return an empty/degenerate result. - testTempE10E12Direct: diagnostic test capturing two known-hard intersection pairs (E10∩E12, E13∩E16) in the fourth real-world path. - Raise maximumIterations 64→512 in bezierClipping to give the overlapping- circle tests enough budget to converge. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Add Arc.swift: Arc struct, arc detection (asArc), arcParameter, arc-arc, arc-line, arc-quadratic, and arc-cubic intersection routines using Bernstein polynomial root finding - Add BernsteinPolynomial6 with BezierClippingPolynomial conformance to support degree-6 polynomial roots in arcCubicIntersections - Add ArcTests.swift: unit tests for arcParameter including CCW/CW arcs where endAngle crosses the atan2 branch cut (> π and < -π) - Document angle range conventions in Arc.swift: startAngle ∈ (-π,π] from atan2; endAngle can exceed 2π (CCW crossing branch cut) or be negative (CW crossing branch cut) - Add regression tests for crossingsRemoved on overlapping near-circles and a self-intersecting 27-element path - Add testTempE10E12Direct to catch false-positive intersection between a degenerate near-point cubic and a large-radius arc cubic Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Three changes work together to correctly handle closed components with self-intersections in removeCrossings: 1. Wrap-around edges: closed components with intersections now use a single wrap-around edge from the last intersection node back to the first, bypassing the seam. This prevents inconsistent classification of arcs that straddle the component seam. 2. Gap-nodes-first ordering: in performOperation, intersection nodes whose own forward arc is not in solution are processed before other intersection nodes, ensuring they find their cycles before those arcs are consumed by other traversals. 3. preferNeighbors DFS: for removeCrossings, findUnvisitedPath tries neighbor (cross-component) forward arcs before the node's own forward arc, so cross-component edges are explored first. Neighbor arcs leading directly to the goal are deferred last to avoid trivial one-arc cycles that would split correct multi-arc cycles. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- CoincidenceInterval struct: replace [Intersection]? return type with a plain 4-CGFloat value type; callers materialize [Intersection] only when needed. Confirmed via assembly: 0 swift_allocObject in all four coincidenceCheck specializations. - coincidenceCheck bounding-box guard: skip the expensive project() root-find when an endpoint clearly lies outside the other curve's bounding box. - tangent-parallelness pre-filter: only call coincidenceCheck from the clipping result path when result.count==1 and sin²(angle) ≤ 0.1 (angle ≤ ~18°). Genuine transverse crossings are never coincidences. - asArc closed-form midpoint: compute t=0.5 algebraically instead of allocating a [CGPoint] array; replace the loop over 5 sampled points with explicit checks at t=0.25 and t=0.75 only (p0/pMid/p3 lie on the circumcircle by construction). - asArc R<5×chord filter: reject nearly-linear cubics that pass the distance checks but gain nothing from the arc intersection path. Performance (release build, Apple M-series): testCubicIntersectionsPerformance: 0.571 s → 0.486 s (−15 %) testQuadraticCubicIntersectionsPerformance: unchanged at 0.039 s Accuracy (exhaustive t-grid sweep): Cubic×Cubic (1884 pairs): max error 6.87e-16, avg 1.36e-16 Quad×Cubic (1934 pairs): max error 6.21e-16 Quad×Quad (1980 pairs): max error 6.87e-16 Line×Cubic (1960 pairs): max error 0 (exact) All errors at or below float64 machine epsilon (2.22e-16 ulp). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…duce mutual neighbor nodes When selfIntersections returns the same intersection twice (possible with degenerate near-coincident curves), sortAndMergeDuplicates creates two node pairs at the same IndexedPathLocation. Merging the second pair caused addNeighbor to encounter a neighbor already added during the first merge, triggering the assert. Fix replaceNeighbor to remove rather than duplicate when the replacement is already present, and fix mergeNeighbors to skip self-referential connections, guard against adding duplicates, and clean up the stale reference to the merged-away node. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
testCrossingsRemovedCircleWithTube covers the case where a circle (4-arc component) intersects a tube-like shape (6-element component); previously crossingsRemoved returned an empty path for this input. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
In findUnvisitedPath, neighborsContain(goal) was allowed as a base case unconditionally, which let the DFS close a wrong inner cycle when using preferNeighbors=true (the removeCrossings traversal). The fix gates that shortcut on !preferNeighbors so it still works for union/intersect but forces removeCrossings to traverse all the way back to the exact goal node. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Three related fixes to the augmented graph traversal for crossingsRemoved: 1. visitCoincidentEdges: skip edges that start (or end, for backward arcs) at self's ending node — they travel in the opposite direction and are not truly coincident, fixing false-positive coincident marking on loop arcs whose two endpoints share the same geometric location. 2. findUnvisitedPath: extend the goal-deferral check from "next node IS goal" to "next node IS goal OR is a neighbor of goal", and apply it to the own arc as well as neighbor arcs, preventing premature closure on short inner-face cycles when the correct outer boundary still needs traversal. 3. findUnvisitedPath: allow neighborsContain(goal) as a base-case termination for preferNeighbors (removeCrossings) mode, enabling correct closure when the outer boundary crosses between path components whose start/end nodes are geometrically coincident. Adds regression test for the circle+tube case. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…oveCrossings only The progressive-distance edge classification (trying offsets up to 5e-2) introduced in 61e1b83 for removeCrossings caused incorrect union results when two paths run close together. At large offsets, a test point can cross the other path's boundary, making an interior edge (which should be excluded) appear boundary-straddling and incorrectly including it in the solution. With the evenOdd fill rule, spurious interior components cause enclosed regions to be counted twice, appearing as outside the result. Fix: use only the small distance (1e-6) for union/subtract/intersect. The multi-distance approach is needed only for removeCrossings, where seam-wrapping arcs can land too close to adjacent component boundaries for the small offset to distinguish inside from outside. Also removes XCTExpectFailure from testUnionRealWorldCase3, which now passes. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <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
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
crossingsRemovedfailures on self-intersecting and multi-component paths that were producing wrong output or crashingChanges
Bug fixes in the augmented graph traversal (
AugmentedGraph.swift):visitCoincidentEdgesincorrectly marking an arc as coincident with a reverse-direction arc when both arcs share the same two geometric endpoints (as occurs with loop arcs). Also fixed premature goal-closure cutting off the correct outer boundary cycle.Three specific algorithmic fixes in the final commit:
visitCoincidentEdges: skip candidate edges that start (or end, for backward arcs) at self's ending node — they travel in the opposite direction and are not truly coincident.findUnvisitedPath: extend goal-deferral from "next node is exactly goal" to "next node is goal or a neighbor of goal", applied to own arc as well as neighbor arcs.findUnvisitedPath: allowneighborsContain(goal)as a cycle-termination condition inremoveCrossingsmode, enabling correct closure when the outer boundary crosses between components whose endpoints are geometrically coincident.Performance:
coincidenceCheckandasArchot paths in intersection detection.New intersection capability:
arcParametersupport.Test plan
xcodebuild test -scheme BezierKit -destination 'platform=macOS')testCrossingsRemovedTwoOverlappingCircles,testCrossingsRemovedTwoOverlappingCircles2testCrossingsRemovedAugmentedGraphDuplicateNeighborAsserttestCrossingsRemovedTwoOverlappingIrregularComponentstestCrossingsRemovedCircleWithTube,testCrossingsRemovedIrregularComponentWithLargeNeighbortestCrossingsRemovedEdgeCase(figure-8 path, previously passing, now verified still correct)🤖 Generated with Claude Code