From be5fef687570d03490d61a524c406d025654e7fc Mon Sep 17 00:00:00 2001 From: AJ Isaacs Date: Tue, 3 Nov 2026 18:03:24 -0500 Subject: [PATCH] fix(release): enforce one finish per commit and keep real unplaced reasons Delta review fixes: - ReleaseRenest throws when a commit does not call finish, or calls it twice: either would leave ledger plates uncoded or count a sheet twice for later nests of the run. The ledger sheet count is the codes given. - A stage whose layout is not kept now lists the copies its layout missed with their own reasons (a part no sheet fits stays "does not fit"); only the copies it placed are "the proposal cannot be kept". - A valid ledger layout that places nothing says "Auto Nest placed nothing on it" rather than claiming no part fits. - ReleaseLedgerOffer rejects null rows; docs list every ledger note. --- .../MaterialLedger/ReleaseLedgerStock.cs | 14 +++--- OpenNest.Desktop.Tests/ReleaseRenestTests.cs | 47 +++++++++++++++++++ OpenNest.Desktop/ReleaseRenest.cs | 38 ++++++++++----- OpenNest.Engine/Jobs/NestRenest.cs | 27 ++++++++--- OpenNest.Tests/Engine/NestRenestTests.cs | 16 +++++++ docs/nest-storage.md | 8 ++-- 6 files changed, 121 insertions(+), 29 deletions(-) diff --git a/OpenNest.Data/MaterialLedger/ReleaseLedgerStock.cs b/OpenNest.Data/MaterialLedger/ReleaseLedgerStock.cs index 8179d2e3..27e816e2 100644 --- a/OpenNest.Data/MaterialLedger/ReleaseLedgerStock.cs +++ b/OpenNest.Data/MaterialLedger/ReleaseLedgerStock.cs @@ -19,7 +19,8 @@ public sealed record ReleaseLedgerOffer(IReadOnlyList Rows, stri /// /// The rows as engine stock: finite, under their ledger:{n} ids, no costs. /// - public IReadOnlyList Options { get; } = Rows + public IReadOnlyList Options { get; } = + (Rows ?? throw new ArgumentNullException(nameof(Rows))) .Select(row => new PlateOption { Width = row.Size.Width, @@ -132,24 +133,23 @@ public sealed class ReleaseLedgerStock } /// - /// The release note for nested on : how - /// many sheets came from the ledger, or why it was not used. + /// The release note for a nest nested on : how many sheets came + /// from the ledger, or why it was not used. /// + /// The ledger codes gave the nest. /// /// True when Auto Nest could not keep a layout on the offer. /// - public static string Note(ReleaseLedgerOffer offer, Nest nest, bool layoutNotKept) + public static string Note(ReleaseLedgerOffer offer, int sheets, bool layoutNotKept) { ArgumentNullException.ThrowIfNull(offer); - ArgumentNullException.ThrowIfNull(nest); if (offer.NotUsed != null) return $"MaterialLedger not used: {offer.NotUsed}."; - var sheets = nest.Plates.Sum(p => p.StockItemCodes.Count); var note = layoutNotKept ? "MaterialLedger not used: Auto Nest could not keep a layout on its stock." : sheets == 0 - ? "No MaterialLedger stock used: no part fits it." + ? "No MaterialLedger stock used: Auto Nest placed nothing on it." : string.Format(CultureInfo.CurrentCulture, "{0} sheet{1} from MaterialLedger stock.", sheets, sheets == 1 ? "" : "s"); return offer.OtherNestsUnchecked == null diff --git a/OpenNest.Desktop.Tests/ReleaseRenestTests.cs b/OpenNest.Desktop.Tests/ReleaseRenestTests.cs index a80583f6..05c9aaad 100644 --- a/OpenNest.Desktop.Tests/ReleaseRenestTests.cs +++ b/OpenNest.Desktop.Tests/ReleaseRenestTests.cs @@ -296,4 +296,51 @@ public class ReleaseRenestTests Assert.Equal(["S-1"], codesAtClose); } + + /// Rectangles, except that a ledger sheet gets an empty layout. + private sealed class NothingOnLedger : INestingEngine + { + private readonly INestingEngine _inner = NestingEngineRegistry.Create("Rectangles"); + + public NestJobResult Solve(NestJob job, IProgress progress = null, + CancellationToken token = default) => + LedgerStockMapper.IsLedgerId(job.Plates[0].Id) + ? new NestJobResultBuilder(job).Build(NestJobStopReason.Completed) + : _inner.Solve(job, progress, token); + } + + [Fact] + public async Task AValidLedgerLayoutThatPlacesNothing_SaysSo() + { + var ledger = new FakeLedger { Stock = Stock(Sheet("S-1", 60, 120)) }; + var nest = NestOf(20, 2); + + var result = await Release(ledger.Create(), nest, new NothingOnLedger()); + + Assert.True(result.Nested, result.Note); + Assert.EndsWith(" No MaterialLedger stock used: Auto Nest placed nothing on it.", + result.Note); + } + + [Theory] + [InlineData(0)] + [InlineData(2)] + public async Task ACommitThatDoesNotFinishItsPlatesOnce_Throws(int finishes) + { + var ledger = new FakeLedger { Stock = Stock(Sheet("S-1", 60, 120)) }.Create(); + var nest = NestOf(20, 2); + + await Assert.ThrowsAsync(() => ReleaseRenest.RunAsync(nest, + ledger, "Rectangles", 0, + request => Task.FromResult(NestPipeline.Run(NestingEngineRegistry.Create("Rectangles"), + "Rectangles", request, null, CancellationToken.None)), + (result, finish) => + { + var plates = NestPipelineCommit.ApplyToPlates(result, nest.CreatePlate); + for (var i = 0; i < finishes; i++) + finish(plates); + return plates; + }, + CancellationToken.None)); + } } diff --git a/OpenNest.Desktop/ReleaseRenest.cs b/OpenNest.Desktop/ReleaseRenest.cs index b42980c5..f2583020 100644 --- a/OpenNest.Desktop/ReleaseRenest.cs +++ b/OpenNest.Desktop/ReleaseRenest.cs @@ -25,8 +25,8 @@ namespace OpenNest.Desktop { /// /// Attaches a kept result's sheets to the nest's plates and returns them. It must call - /// finish with those plates before its undo step closes; that gives them their - /// ledger codes. + /// finish once with those plates before its undo step closes; that gives them + /// their ledger codes. Not calling it, or calling it twice, throws. /// public delegate IReadOnlyList Commit(NestPipelineResult result, Action> finish); @@ -47,24 +47,38 @@ namespace OpenNest.Desktop var wanted = nest.Drawings.Any(d => !d.IsCutOff && d.Quantity.Remaining > 0); var offer = ledger != null && wanted ? await ledger.OfferAsync(nest, token) : null; var locations = new Dictionary(StringComparer.Ordinal); + var sheets = 0; var renested = await NestRenest.RunStagedAsync(nest, engineName, offer?.Options, minimumSalvageDimension, solve, - result => commit(result, plates => + result => { - if (offer == null) - return; - var stock = NestPipelineCommit.PairWithStock(result, plates) - .Select(p => (p.Plate, p.Stock.Id)) - .ToList(); - foreach (var (code, location) in ledger.AssignCodes(nest, offer, stock)) - locations[code] = location; - }), + var finished = false; + var plates = commit(result, applied => + { + if (finished) + throw new InvalidOperationException( + "A release commit finished its plates twice."); + finished = true; + if (offer == null) + return; + var stock = NestPipelineCommit.PairWithStock(result, applied) + .Select(p => (p.Plate, p.Stock.Id)) + .ToList(); + foreach (var (code, location) in ledger.AssignCodes(nest, offer, stock)) + locations[code] = location; + sheets += stock.Sum(p => p.Plate.StockItemCodes.Count); + }); + if (!finished) + throw new InvalidOperationException( + "A release commit did not finish its plates, so they have no codes."); + return plates; + }, token); var ledgerNote = offer == null ? null - : ReleaseLedgerStock.Note(offer, nest, renested.FirstStockNotKept); + : ReleaseLedgerStock.Note(offer, sheets, renested.FirstStockNotKept); return new ReleaseRenestResult(renested.Nested, Join(renested.Note, ledgerNote), locations); } diff --git a/OpenNest.Engine/Jobs/NestRenest.cs b/OpenNest.Engine/Jobs/NestRenest.cs index cee769a4..5e5bb9fe 100644 --- a/OpenNest.Engine/Jobs/NestRenest.cs +++ b/OpenNest.Engine/Jobs/NestRenest.cs @@ -9,7 +9,10 @@ namespace OpenNest.Engine.Jobs; /// /// What kept: whether a layout was committed, its physical sheets -/// and utilization, the copies it left unplaced, and one operator-facing note. +/// and utilization, the copies it left unplaced, and one operator-facing note. A staged run +/// () lists every copy of a layout it did not keep, +/// the ones that layout placed as ; +/// lists the result's own unplaced copies. /// public sealed record NestRenestResult( bool Nested, @@ -126,7 +129,7 @@ public static class NestRenest if (!result.IsValid || !result.CanKeep) { // Every copy this stage asked for is left, not just what its layout missed. - unplaced = NotKept(request); + unplaced = NotKept(request, result); firstStockNotKept |= stock != null; } continue; @@ -212,11 +215,21 @@ public static class NestRenest .Select(p => (p.Plate, p.Stock.Id)) .ToList(); - private static IReadOnlyList NotKept(NestPipelineRequest request) => - request.Items - .Select(item => new NestUnplacedPart(item.Drawing.Name, item.Drawing, item.Quantity, - NestUnplacedReason.ProposalNotKeepable)) - .ToList(); + // The copies the layout missed keep their own reasons; the ones it placed were not kept. + private static IReadOnlyList NotKept(NestPipelineRequest request, + NestPipelineResult result) + { + var unplaced = result.Unplaced.ToList(); + foreach (var item in request.Items) + { + var listed = unplaced.Where(u => ReferenceEquals(u.Drawing, item.Drawing)) + .Sum(u => u.Count); + if (item.Quantity > listed) + unplaced.Add(new NestUnplacedPart(item.Drawing.Name, item.Drawing, + item.Quantity - listed, NestUnplacedReason.ProposalNotKeepable)); + } + return unplaced; + } private static bool IsKeepable(NestPipelineResult result) => result.IsValid && result.CanKeep && result.Plates.Any(p => p.Parts.Count > 0); diff --git a/OpenNest.Tests/Engine/NestRenestTests.cs b/OpenNest.Tests/Engine/NestRenestTests.cs index 2d9a5243..e82e7d71 100644 --- a/OpenNest.Tests/Engine/NestRenestTests.cs +++ b/OpenNest.Tests/Engine/NestRenestTests.cs @@ -122,6 +122,22 @@ public class NestRenestTests Assert.StartsWith("Nested on 1 sheet", kept.Note); } + [Fact] + public void ANotKeptLayout_KeepsTheRealReasonForCopiesNoSheetFits() + { + var nest = NestOf(("SYN-A", 4, 2, 2), ("SYN-HUGE", 200, 200, 1)); + + var result = NestRenest.Run(nest, new OverlappingEngine(), "overlapping"); + + Assert.False(result.Nested); + Assert.Equal( + [ + ("SYN-A", 2, NestUnplacedReason.ProposalNotKeepable), + ("SYN-HUGE", 1, NestUnplacedReason.NoStockFits), + ], + result.Unplaced.Select(u => (u.Drawing.Name, u.Count, u.Reason)).OrderBy(u => u.Name)); + } + /// Puts every copy at the same spot, so the layout never validates. private sealed class OverlappingEngine : INestingEngine { diff --git a/docs/nest-storage.md b/docs/nest-storage.md index 2e400382..d9d189f8 100644 --- a/docs/nest-storage.md +++ b/docs/nest-storage.md @@ -729,10 +729,12 @@ every drawing's required quantity with the selected engine, in two steps: When the nest has no material or thickness, or the ledger is not set up on the PC, cannot be reached, rejects the API key, has no mapping for the material or has no usable stock of it (the note counts what was left out: on other nests, -taken by earlier nests of this release, too small for any part), the group goes +taken by earlier nests of this release, too small for any part, of unknown +shape; "no free stock" when the ledger had none at all), the group goes straight to step 2 and its note says "MaterialLedger not used: " and why. So -does a step 1 whose layout Auto Nest cannot keep. Otherwise the note says how -many sheets came from MaterialLedger stock, and adds "Codes on other nests were +does a step 1 whose layout Auto Nest cannot keep. A step 1 that places nothing +says "No MaterialLedger stock used: Auto Nest placed nothing on it." Otherwise +the note says how many sheets came from MaterialLedger stock, and adds "Codes on other nests were not checked" with the reason when the nest server could not list them. The ledger is not read for a group with nothing to nest.