From 7a5c06bc99b14bf526e5e2e6e6b90f8432778d2d Mon Sep 17 00:00:00 2001 From: AJ Isaacs Date: Fri, 16 Oct 2026 15:05:43 -0400 Subject: [PATCH] fix(bestfit): bind loaded pair candidates to a canonical copy Saved pairs are in the drawing's canonical frame, but loaded candidates held the source drawing, so BuildCanonicalParts/BuildSourceParts rebuilt a different pair for any drawing with a non-zero canonical angle (a 30 degree T-shape came back 16.57 x 10.39 instead of its saved 16.92 x 8). That reached the Irregular engine's pair catalog and the best-fit viewer after opening a saved nest. Import now binds every candidate to one canonical copy, as computed candidates are; fills already passed the canonical drawing explicitly. Co-Authored-By: Claude Opus 5.5 --- OpenNest.Engine/BestFit/BestFitCacheStore.cs | 10 +++++- .../BestFit/BestFitCacheStoreTests.cs | 7 ++-- OpenNest.Tests/IO/BestFitPersistenceTests.cs | 32 +++++++++---------- 3 files changed, 30 insertions(+), 19 deletions(-) diff --git a/OpenNest.Engine/BestFit/BestFitCacheStore.cs b/OpenNest.Engine/BestFit/BestFitCacheStore.cs index d860e425..2a555809 100644 --- a/OpenNest.Engine/BestFit/BestFitCacheStore.cs +++ b/OpenNest.Engine/BestFit/BestFitCacheStore.cs @@ -58,14 +58,22 @@ namespace OpenNest.Engine.BestFit public void Import(Drawing drawing, IReadOnlyList sets) { + // Saved pairs are in the canonical frame, so their candidates hold a canonical copy, + // as computed ones do; the source drawing would rebuild a different pair. + Drawing canonical = null; + foreach (var set in sets) { + if (set.Results.Count == 0) + continue; + + canonical ??= CanonicalFrame.AsCanonicalCopy(drawing); var results = set .Results.Select(r => new BestFitResult { Candidate = new PairCandidate { - Drawing = drawing, + Drawing = canonical, Part1Rotation = r.Part1Rotation, Part2Rotation = r.Part2Rotation, Part2Offset = new Vector(r.Part2OffsetX, r.Part2OffsetY), diff --git a/OpenNest.Tests/BestFit/BestFitCacheStoreTests.cs b/OpenNest.Tests/BestFit/BestFitCacheStoreTests.cs index 6675a493..44bcd55e 100644 --- a/OpenNest.Tests/BestFit/BestFitCacheStoreTests.cs +++ b/OpenNest.Tests/BestFit/BestFitCacheStoreTests.cs @@ -1,4 +1,5 @@ using OpenNest.BestFit; +using OpenNest.Engine; using OpenNest.Engine.BestFit; using OpenNest.Geometry; using OpenNest.Shapes; @@ -71,7 +72,7 @@ public class BestFitCacheStoreTests } [Fact] - public void Import_PopulatesCacheWithCandidatesBoundToTheDrawing() + public void Import_PopulatesCacheWithCandidatesOnACanonicalCopyOfTheDrawing() { BestFitCache.Clear(); var drawing = MakeDrawing(); @@ -82,7 +83,9 @@ public class BestFitCacheStoreTests var restored = Assert.Single(BestFitCache.GetAllForDrawing(drawing)); Assert.Equal((60.0, 40.0, Spacing), restored.Key); var result = Assert.Single(restored.Value); - Assert.Same(drawing, result.Candidate.Drawing); + Assert.NotSame(drawing, result.Candidate.Drawing); + Assert.Same(drawing, CanonicalFrame.SourceOf(result.Candidate.Drawing)); + Assert.Equal(0.0, result.Candidate.Drawing.Source.Angle); Assert.Equal(0.5, result.Candidate.Part1Rotation); Assert.Equal(System.Math.PI, result.Candidate.Part2Rotation); Assert.Equal(new Vector(3.25, -1.5), result.Candidate.Part2Offset); diff --git a/OpenNest.Tests/IO/BestFitPersistenceTests.cs b/OpenNest.Tests/IO/BestFitPersistenceTests.cs index c4ee4d30..f060029c 100644 --- a/OpenNest.Tests/IO/BestFitPersistenceTests.cs +++ b/OpenNest.Tests/IO/BestFitPersistenceTests.cs @@ -137,11 +137,10 @@ public class BestFitPersistenceTests } [Fact] - public void RoundTrip_RotatedSource_LoadedCandidatesHoldTheSourceDrawingNotACanonicalCopy() + public void RoundTrip_RotatedSource_LoadedCandidatesRebuildTheSavedPair() { - // Pins current behaviour: computed candidates hold a canonical copy (angle 0), while - // loaded candidates hold the loaded source drawing itself, so BuildCanonicalParts on a - // loaded result builds from the source frame. + // Saved pairs are in the canonical frame. Loaded candidates must hold a canonical copy, + // as computed ones do, or the source drawing rebuilds a different pair. BestFitCache.Clear(); var nest = CreateNest(MakeRotatedTShape()); var source = nest.Drawings.First(); @@ -156,18 +155,19 @@ public class BestFitPersistenceTests Assert.False(Tolerance.IsEqualTo(drawing.Source.Angle, 0)); Assert.Equal(source.Source.Angle, drawing.Source.Angle, 9); - Assert.Same(drawing, loaded.Candidate.Drawing); + Assert.NotSame(drawing, loaded.Candidate.Drawing); + Assert.Same(drawing, CanonicalFrame.SourceOf(loaded.Candidate.Drawing)); + Assert.Equal(0.0, loaded.Candidate.Drawing.Source.Angle); - Assert.Equal(0.0, computed.BuildCanonicalParts()[0].BaseDrawing.Source.Angle); - Assert.Same(drawing, loaded.BuildCanonicalParts()[0].BaseDrawing); - - // The computed pair rebuilds to its saved bounds; the loaded one, built from the - // source frame with canonical-frame offsets, does not. - var computedBounds = computed.GetCutBounds(computed.BuildSourceParts(source)); - Assert.Equal(computed.BoundingWidth, computedBounds.Length, 2); - Assert.Equal(computed.BoundingHeight, computedBounds.Width, 2); - var loadedBounds = loaded.GetCutBounds(loaded.BuildSourceParts(drawing)); - Assert.NotEqual(loaded.BoundingWidth * loaded.BoundingHeight, loadedBounds.Length * loadedBounds.Width, 2); + foreach (var (result, owner) in new[] { (computed, source), (loaded, drawing) }) + { + var parts = result.BuildSourceParts(owner); + var bounds = result.GetCutBounds(parts); + Assert.All(parts, p => Assert.Same(owner, p.BaseDrawing)); + Assert.Equal(result.BoundingWidth, bounds.Length, 2); + Assert.Equal(result.BoundingHeight, bounds.Width, 2); + Assert.False(parts[0].Intersects(parts[1], out _)); + } } [Fact] @@ -263,7 +263,7 @@ public class BestFitPersistenceTests { var e = expected[i]; var a = actual[i]; - Assert.Same(loadedDrawing, a.Candidate.Drawing); + Assert.Same(loadedDrawing, CanonicalFrame.SourceOf(a.Candidate.Drawing)); AssertBits(e.Candidate.Part1Rotation, a.Candidate.Part1Rotation); AssertBits(e.Candidate.Part2Rotation, a.Candidate.Part2Rotation); AssertBits(e.Candidate.Part2Offset.X, a.Candidate.Part2Offset.X);