From 8450ceee401dd69f827bace4cd80e5e6a8c2fa4c Mon Sep 17 00:00:00 2001 From: AJ Isaacs Date: Wed, 30 Sep 2026 21:20:58 -0400 Subject: [PATCH] refactor(geometry): share signed-angle accumulation through ArcFit EllipseConverter, SplineConverter and GeometrySimplifier each carried an identical private SumSignedAngles. Move the unchanged body to ArcFit.SumSignedAngles and call it from all three, keeping the ordered accumulation, strict half-turn comparisons and empty/single-point result. ArcFitTests compares the shared method bit-for-bit with a separate test-local accumulator across half turns, the atan2 seam, multiple turns, translated centres and NaN inputs, and checks that inputs are not mutated. The converter winding characterization from 8664656 still passes unchanged. --- OpenNest.Core/Geometry/ArcFit.cs | 22 +++ OpenNest.Core/Geometry/EllipseConverter.cs | 19 +- OpenNest.Core/Geometry/GeometrySimplifier.cs | 29 +-- OpenNest.Core/Geometry/SplineConverter.cs | 23 +-- OpenNest.Tests/Geometry/ArcFitTests.cs | 175 +++++++++++++++++++ 5 files changed, 205 insertions(+), 63 deletions(-) create mode 100644 OpenNest.Tests/Geometry/ArcFitTests.cs diff --git a/OpenNest.Core/Geometry/ArcFit.cs b/OpenNest.Core/Geometry/ArcFit.cs index 72b1dab..1773cb0 100644 --- a/OpenNest.Core/Geometry/ArcFit.cs +++ b/OpenNest.Core/Geometry/ArcFit.cs @@ -1,4 +1,5 @@ using System.Collections.Generic; +using OpenNest.Math; namespace OpenNest.Geometry { @@ -118,6 +119,27 @@ namespace OpenNest.Geometry return System.Math.Atan2(ux * to.Y - uy * to.X, ux * to.X + uy * to.Y); } + /// + /// Sums signed angular change traversing consecutive points around a center. + /// Positive = CCW, negative = CW. + /// + public static double SumSignedAngles(Vector center, List points) + { + var total = 0.0; + for (var i = 0; i < points.Count - 1; i++) + { + var a1 = System.Math.Atan2(points[i].Y - center.Y, points[i].X - center.X); + var a2 = System.Math.Atan2(points[i + 1].Y - center.Y, points[i + 1].X - center.X); + var da = a2 - a1; + while (da > System.Math.PI) + da -= Angle.TwoPI; + while (da < -System.Math.PI) + da += Angle.TwoPI; + total += da; + } + return total; + } + /// /// Computes the maximum radial deviation of interior points from a circle. /// diff --git a/OpenNest.Core/Geometry/EllipseConverter.cs b/OpenNest.Core/Geometry/EllipseConverter.cs index 8f6d8a8..6d62106 100644 --- a/OpenNest.Core/Geometry/EllipseConverter.cs +++ b/OpenNest.Core/Geometry/EllipseConverter.cs @@ -330,7 +330,7 @@ namespace OpenNest.Geometry var endAngle = System.Math.Atan2(p1.Y - arcCenter.Y, p1.X - arcCenter.X); var points = new List { p0, pMid, p1 }; - var isReversed = SumSignedAngles(arcCenter, points) < 0; + var isReversed = ArcFit.SumSignedAngles(arcCenter, points) < 0; if (startAngle < 0) startAngle += Angle.TwoPI; @@ -339,22 +339,5 @@ namespace OpenNest.Geometry return new Arc(arcCenter, radius, startAngle, endAngle, isReversed); } - - private static double SumSignedAngles(Vector center, List points) - { - var total = 0.0; - for (var i = 0; i < points.Count - 1; i++) - { - var a1 = System.Math.Atan2(points[i].Y - center.Y, points[i].X - center.X); - var a2 = System.Math.Atan2(points[i + 1].Y - center.Y, points[i + 1].X - center.X); - var da = a2 - a1; - while (da > System.Math.PI) - da -= Angle.TwoPI; - while (da < -System.Math.PI) - da += Angle.TwoPI; - total += da; - } - return total; - } } } diff --git a/OpenNest.Core/Geometry/GeometrySimplifier.cs b/OpenNest.Core/Geometry/GeometrySimplifier.cs index 72cf371..18a7ce8 100644 --- a/OpenNest.Core/Geometry/GeometrySimplifier.cs +++ b/OpenNest.Core/Geometry/GeometrySimplifier.cs @@ -435,7 +435,7 @@ public class GeometrySimplifier // Reject arcs that subtend a tiny angle — these are nearly-straight lines // that happen to fit a huge circle. Applied after extension so that many small // segments can accumulate enough sweep to qualify. - var sweep = System.Math.Abs(SumSignedAngles(center, points)); + var sweep = System.Math.Abs(ArcFit.SumSignedAngles(center, points)); if (sweep < Angle.ToRadians(5)) return null; @@ -454,7 +454,7 @@ public class GeometrySimplifier continue; // Check that the arc doesn't bulge away from the original line segments - var isReversed = SumSignedAngles(center, points) < 0; + var isReversed = ArcFit.SumSignedAngles(center, points) < 0; var arcDev = MaxArcToSegmentDeviation(points, center, radius, isReversed); if (arcDev > Tolerance) continue; @@ -673,7 +673,7 @@ public class GeometrySimplifier var lastPt = points[^1]; var rx = lastPt.X - center.X; var ry = lastPt.Y - center.Y; - var sign = SumSignedAngles(center, points) >= 0 ? 1 : -1; + var sign = ArcFit.SumSignedAngles(center, points) >= 0 ? 1 : -1; return new Vector(-sign * ry, sign * rx); } @@ -792,7 +792,7 @@ public class GeometrySimplifier var endAngle = NormalizeAngle( System.Math.Atan2(lastPoint.Y - center.Y, lastPoint.X - center.X) ); - var isReversed = SumSignedAngles(center, points) < 0; + var isReversed = ArcFit.SumSignedAngles(center, points) < 0; var arc = new Arc(center, radius, startAngle, endAngle, isReversed); arc.Layer = sourceEntity.Layer; @@ -816,27 +816,6 @@ public class GeometrySimplifier _ => Vector.Invalid, }; - /// - /// Sums signed angular change traversing consecutive points around a center. - /// Positive = CCW, negative = CW. - /// - private static double SumSignedAngles(Vector center, List points) - { - var total = 0.0; - for (var i = 0; i < points.Count - 1; i++) - { - var a1 = System.Math.Atan2(points[i].Y - center.Y, points[i].X - center.X); - var a2 = System.Math.Atan2(points[i + 1].Y - center.Y, points[i + 1].X - center.X); - var da = a2 - a1; - while (da > System.Math.PI) - da -= Angle.TwoPI; - while (da < -System.Math.PI) - da += Angle.TwoPI; - total += da; - } - return total; - } - /// /// Measures the maximum distance from sampled points along the fitted arc /// back to the original line segments. This catches cases where points lie diff --git a/OpenNest.Core/Geometry/SplineConverter.cs b/OpenNest.Core/Geometry/SplineConverter.cs index 6ab4eff..fc28217 100644 --- a/OpenNest.Core/Geometry/SplineConverter.cs +++ b/OpenNest.Core/Geometry/SplineConverter.cs @@ -80,7 +80,7 @@ namespace OpenNest.Geometry } var finalPoints = points.GetRange(start, endIdx - start + 1); - var sweep = System.Math.Abs(SumSignedAngles(center, finalPoints)); + var sweep = System.Math.Abs(ArcFit.SumSignedAngles(center, finalPoints)); if (sweep < Angle.ToRadians(5)) return null; @@ -151,27 +151,10 @@ namespace OpenNest.Geometry double radius ) => ArcFit.MaxRadialDeviation(points, cx, cy, radius); - private static double SumSignedAngles(Vector center, List points) - { - var total = 0.0; - for (var i = 0; i < points.Count - 1; i++) - { - var a1 = System.Math.Atan2(points[i].Y - center.Y, points[i].X - center.X); - var a2 = System.Math.Atan2(points[i + 1].Y - center.Y, points[i + 1].X - center.X); - var da = a2 - a1; - while (da > System.Math.PI) - da -= Angle.TwoPI; - while (da < -System.Math.PI) - da += Angle.TwoPI; - total += da; - } - return total; - } - private static Vector ComputeEndTangent(Vector center, List points) { var lastPt = points[^1]; - var totalAngle = SumSignedAngles(center, points); + var totalAngle = ArcFit.SumSignedAngles(center, points); var rx = lastPt.X - center.X; var ry = lastPt.Y - center.Y; @@ -186,7 +169,7 @@ namespace OpenNest.Geometry var startAngle = System.Math.Atan2(firstPoint.Y - center.Y, firstPoint.X - center.X); var endAngle = System.Math.Atan2(lastPoint.Y - center.Y, lastPoint.X - center.X); - var isReversed = SumSignedAngles(center, points) < 0; + var isReversed = ArcFit.SumSignedAngles(center, points) < 0; if (startAngle < 0) startAngle += Angle.TwoPI; diff --git a/OpenNest.Tests/Geometry/ArcFitTests.cs b/OpenNest.Tests/Geometry/ArcFitTests.cs new file mode 100644 index 0000000..45182f3 --- /dev/null +++ b/OpenNest.Tests/Geometry/ArcFitTests.cs @@ -0,0 +1,175 @@ +using OpenNest.Geometry; + +namespace OpenNest.Tests.Geometry; + +public class ArcFitTests +{ + [Theory] + [InlineData("empty")] + [InlineData("single")] + [InlineData("positive-half-turn")] + [InlineData("negative-half-turn")] + [InlineData("repeated")] + [InlineData("translated")] + [InlineData("closed-ccw")] + [InlineData("closed-cw")] + [InlineData("multiple-turns-ccw")] + [InlineData("multiple-turns-cw")] + [InlineData("irregular-multiple-turns")] + [InlineData("two-points")] + [InlineData("atan-seam-ccw")] + [InlineData("atan-seam-cw")] + [InlineData("positive-x-seam")] + [InlineData("collinear-through-center")] + [InlineData("nan-point-x")] + [InlineData("nan-point-y")] + [InlineData("nan-center")] + [InlineData("nan-empty-center")] + [InlineData("nan-single-center")] + [InlineData("nan-single-point")] + public void SumSignedAngles_MatchesFrozenAccumulatorBitExactlyWithoutMutation(string scenario) + { + var (center, points) = Corpus(scenario); + var centerBefore = Snapshot(center); + var pointsBefore = points.Select(Snapshot).ToArray(); + var expected = FrozenSumSignedAngles(center, points); + + var actual = ArcFit.SumSignedAngles(center, points); + + Assert.Equal(BitConverter.DoubleToInt64Bits(expected), BitConverter.DoubleToInt64Bits(actual)); + Assert.Equal(centerBefore, Snapshot(center)); + Assert.Equal(pointsBefore, points.Select(Snapshot).ToArray()); + + switch (scenario) + { + // Strict > / < comparisons must leave exact +PI and -PI unreduced. + case "positive-half-turn": + Assert.Equal(BitConverter.DoubleToInt64Bits(System.Math.PI), BitConverter.DoubleToInt64Bits(actual)); + break; + case "negative-half-turn": + Assert.Equal(BitConverter.DoubleToInt64Bits(-System.Math.PI), BitConverter.DoubleToInt64Bits(actual)); + break; + case "empty": + case "single": + case "repeated": + case "collinear-through-center": + case "nan-empty-center": + case "nan-single-center": + case "nan-single-point": + Assert.Equal(BitConverter.DoubleToInt64Bits(0.0), BitConverter.DoubleToInt64Bits(actual)); + break; + case "multiple-turns-ccw": + case "irregular-multiple-turns": + Assert.True(actual > 2 * System.Math.PI); + break; + case "multiple-turns-cw": + Assert.True(actual < -2 * System.Math.PI); + break; + case "nan-point-x": + case "nan-point-y": + case "nan-center": + // Characterized before extraction: a visited NaN propagates into the + // total; neither normalization loop runs because comparisons are false. + // Empty/single lists never evaluate Atan2, even with a NaN center/point. + Assert.True(double.IsNaN(actual)); + break; + } + } + + private static (long x, long y) Snapshot(Vector point) => + (BitConverter.DoubleToInt64Bits(point.X), BitConverter.DoubleToInt64Bits(point.Y)); + + private static (Vector center, List points) Corpus(string scenario) + { + var center = new Vector(0, 0); + var points = scenario switch + { + "empty" => new List(), + "single" => new List { new Vector(2, -3) }, + "positive-half-turn" => new List { new Vector(1, 0), new Vector(-1, 0) }, + "negative-half-turn" => new List { new Vector(-1, 0), new Vector(1, 0) }, + "repeated" => new List { new Vector(1, 2), new Vector(1, 2), new Vector(1, 2) }, + "translated" => new List + { + new Vector(9, -11), new Vector(7, -9), new Vector(5, -11), + new Vector(7, -13), new Vector(9, -11), + }, + "closed-ccw" or "closed-cw" => CardinalLoop(1), + "multiple-turns-ccw" or "multiple-turns-cw" => CardinalLoop(3), + "irregular-multiple-turns" => IrregularTurns(), + "two-points" => new List { new Vector(1, 1), new Vector(-1, 1) }, + "atan-seam-ccw" => new List { new Vector(-1, 0.001), new Vector(-1, -0.001) }, + "atan-seam-cw" => new List { new Vector(-1, -0.001), new Vector(-1, 0.001) }, + "positive-x-seam" => new List { new Vector(1, -0.001), new Vector(1, 0.001) }, + "collinear-through-center" => new List + { + new Vector(1, 0), new Vector(0, 0), new Vector(-1, 0), + new Vector(0, 0), new Vector(1, 0), + }, + "nan-point-x" => new List + { + new Vector(1, 0), new Vector(double.NaN, 1), new Vector(0, 1), + }, + "nan-point-y" => new List + { + new Vector(1, 0), new Vector(1, double.NaN), new Vector(0, 1), + }, + "nan-center" => new List { new Vector(1, 0), new Vector(0, 1) }, + "nan-empty-center" => new List(), + "nan-single-center" => new List { new Vector(1, 0) }, + "nan-single-point" => new List { new Vector(double.NaN, double.NaN) }, + _ => throw new ArgumentOutOfRangeException(nameof(scenario)), + }; + if (scenario == "translated") + center = new Vector(7, -11); + else if (scenario is "nan-center" or "nan-empty-center" or "nan-single-center") + center = new Vector(double.NaN, 0); + if (scenario is "closed-cw" or "multiple-turns-cw") + points.Reverse(); + return (center, points); + } + + private static List CardinalLoop(int turns) + { + var cycle = new[] { new Vector(1, 0), new Vector(0, 1), new Vector(-1, 0), new Vector(0, -1) }; + var points = new List(); + for (var i = 0; i <= turns * 4; i++) + points.Add(cycle[i % 4]); + return points; + } + + private static List IrregularTurns() + { + var steps = new[] { 0.13, 0.81, -0.27, 1.01, 0.34, 0.0 }; + var points = new List(); + var angle = 2.9; + for (var i = 0; i < 80; i++) + { + points.Add(new Vector(System.Math.Cos(angle), System.Math.Sin(angle))); + angle += steps[i % steps.Length]; + } + return points; + } + + // Independent frozen test oracle, written before the production extraction. + // Do not route this through ArcFit or any other production angle helper. + private static double FrozenSumSignedAngles(Vector center, List points) + { + const double fullTurn = 2.0 * System.Math.PI; + var sum = 0.0; + for (var index = 1; index < points.Count; index++) + { + var previous = points[index - 1]; + var current = points[index]; + var previousAngle = System.Math.Atan2(previous.Y - center.Y, previous.X - center.X); + var currentAngle = System.Math.Atan2(current.Y - center.Y, current.X - center.X); + var change = currentAngle - previousAngle; + while (change > System.Math.PI) + change -= fullTurn; + while (change < -System.Math.PI) + change += fullTurn; + sum += change; + } + return sum; + } +}