From 068de63e83c6f20fffdd507cb9c464a48bbfac4a Mon Sep 17 00:00:00 2001 From: AJ Isaacs Date: Sun, 15 Mar 2026 20:32:35 -0400 Subject: [PATCH] docs: address spec review feedback for abstract nest engine Fix MainForm callsite descriptions, clarify default implementations return empty lists, make FillExact non-virtual, document PackArea signature refactor, add AutoNester scope note, specify error handling for plugin loading, document thread safety and instance lifetime. Co-Authored-By: Claude Opus 4.6 (1M context) --- .../2026-03-15-abstract-nest-engine-design.md | 33 ++++++++++++------- 1 file changed, 22 insertions(+), 11 deletions(-) diff --git a/docs/superpowers/specs/2026-03-15-abstract-nest-engine-design.md b/docs/superpowers/specs/2026-03-15-abstract-nest-engine-design.md index 5dae15b..4d8904f 100644 --- a/docs/superpowers/specs/2026-03-15-abstract-nest-engine-design.md +++ b/docs/superpowers/specs/2026-03-15-abstract-nest-engine-design.md @@ -24,18 +24,22 @@ NestEngineRegistry (static, OpenNest.Engine) └── Factory method: Create(Plate) → NestEngineBase ``` +**Note on AutoNester:** The existing `AutoNester` static class (NFP + simulated annealing for mixed parts) is a natural future candidate for the registry but is currently unused by any caller. It is out of scope for this refactor — it can be wrapped as an engine later when it's ready for use. + ## NestEngineBase Abstract base class in `OpenNest.Engine`. Provides the contract, shared state, and utility methods. +**Instance lifetime:** Engine instances are short-lived and plate-specific — created per operation via the registry factory. Some engines (like `DefaultNestEngine`) maintain internal state across multiple `Fill` calls on the same instance (e.g., `knownGoodAngles` for angle pruning). Plugin authors should be aware that a single engine instance may receive multiple `Fill` calls within one nesting session. + ### Properties | Property | Type | Notes | |----------|------|-------| | `Plate` | `Plate` | The plate being nested | | `PlateNumber` | `int` | For progress reporting | -| `NestDirection` | `NestDirection` | Fill direction preference | -| `WinnerPhase` | `NestPhase` | Which phase produced the best result (private set) | +| `NestDirection` | `NestDirection` | Fill direction preference, set by callers after creation | +| `WinnerPhase` | `NestPhase` | Which phase produced the best result (protected set) | | `PhaseResults` | `List` | Per-phase results for diagnostics | | `AngleResults` | `List` | Per-angle results for diagnostics | @@ -48,7 +52,7 @@ Abstract base class in `OpenNest.Engine`. Provides the contract, shared state, a ### Virtual Methods (return parts, no side effects) -These are the core methods subclasses override: +These are the core methods subclasses override. Base class default implementations return empty lists — subclasses override the ones they support. ```csharp virtual List Fill(NestItem item, Box workArea, @@ -57,14 +61,13 @@ virtual List Fill(NestItem item, Box workArea, virtual List Fill(List groupParts, Box workArea, IProgress progress, CancellationToken token) -virtual List FillExact(NestItem item, Box workArea, - IProgress progress, CancellationToken token) - virtual List PackArea(Box box, List items, IProgress progress, CancellationToken token) ``` -Default implementations in the base class delegate to `DefaultNestEngine`'s logic (or return empty lists — see DefaultNestEngine below). +**`FillExact` is non-virtual.** It is orchestration logic (binary search wrapper around `Fill`) that works regardless of the underlying fill algorithm. It lives in the base class and calls the virtual `Fill` method. Any engine that implements `Fill` gets `FillExact` for free. + +**`PackArea` signature change:** The current `PackArea(Box, List)` mutates the plate directly and returns `bool`. The new virtual method adds `IProgress` and `CancellationToken` parameters and returns `List` (side-effect-free). This is a deliberate refactor — the old mutating behavior moves to the convenience overload `Pack(List)`. ### Convenience Overloads (non-virtual, add parts to plate) @@ -95,7 +98,9 @@ Rename of the current `NestEngine`. Inherits `NestEngineBase` and overrides all - `Name` → `"Default"` - `Description` → `"Multi-phase nesting (Linear, Pairs, RectBestFit, Remainder)"` -- All current private methods (`FindBestFill`, `FillWithPairs`, `FillRectangleBestFit`, `FillPattern`, `TryRemainderImprovement`, `BuildCandidateAngles`, etc.) remain as private methods in this class +- All current private methods (`FindBestFill`, `FillWithPairs`, `FillRectangleBestFit`, `FillPattern`, `TryRemainderImprovement`, `BuildCandidateAngles`, `QuickFillCount`, etc.) remain as private methods in this class +- `ForceFullAngleSweep` property stays on `DefaultNestEngine` (not the base class) — only used by `BruteForceRunner` which references `DefaultNestEngine` directly +- `knownGoodAngles` HashSet stays as a private field — accumulates across multiple `Fill` calls for angle pruning - No behavioral change — purely structural refactor ## StripNestEngine @@ -109,7 +114,7 @@ The planned `StripNester` (from the strip nester spec) becomes a `NestEngineBase ## NestEngineRegistry -Static class in `OpenNest.Engine` managing engine discovery and selection. +Static class in `OpenNest.Engine` managing engine discovery and selection. Accessed only from the UI thread — not thread-safe. Engines are created per-operation and used on background threads, but the registry itself is only mutated/queried from the UI thread at startup and when the user changes the active engine. ### NestEngineInfo @@ -146,7 +151,13 @@ Follows the existing `IPostProcessor` pattern from `Posts/`: - Reflect over types, find concrete subclasses of `NestEngineBase` - Require a constructor taking `Plate` - Register each with its `Name` and `Description` properties -- Called at application startup alongside post-processor loading +- Called at application startup alongside post-processor loading (WinForms app only — Console and MCP use built-in engines only) + +**Error handling:** +- DLLs that fail to load (bad assembly, missing dependencies) are logged and skipped +- Types without a `Plate` constructor are skipped +- Duplicate engine names: first registration wins, duplicates are logged and skipped +- Exceptions from plugin constructors during `Create()` are caught and surfaced to the caller ## Callsite Migration @@ -154,7 +165,7 @@ All `new NestEngine(plate)` calls become `NestEngineRegistry.Create(plate)`: | Location | Count | Notes | |----------|-------|-------| -| `MainForm.cs` | 3 | Auto-nest, fill plate, fill area | +| `MainForm.cs` | 3 | Auto-nest fill, auto-nest pack, single-drawing fill plate | | `ActionFillArea.cs` | 2 | | | `PlateView.cs` | 1 | | | `NestingTools.cs` (MCP) | 6 | |