docs: address spec review feedback for pluggable fill strategies
Clarifies: strategy statefulness, cancellation handling, progress reporting, NestPhase.Custom for plugins, BinConverter visibility, LinearFillStrategy internal iteration, registry caching. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
@@ -8,6 +8,14 @@
|
||||
|
||||
Extract fill strategies into pluggable components behind a common interface. Engines compose strategies in a pipeline where each strategy receives the current best result from prior strategies and can decide whether to run. New strategies can be added by implementing the interface — including from plugin DLLs discovered via reflection.
|
||||
|
||||
## Scope
|
||||
|
||||
This refactoring targets only the **single-item fill path** (`DefaultNestEngine.FindBestFill`, called from the `Fill(NestItem, ...)` overload). The following are explicitly **out of scope** and remain unchanged:
|
||||
|
||||
- `Fill(List<Part> groupParts, ...)` — group-fill overload, has its own inline orchestration with different conditions (multi-phase block only runs when `groupParts.Count == 1`). May be refactored to use strategies in a future pass once the single-item pipeline is proven.
|
||||
- `PackArea` — packing is a different operation (bin-packing single-quantity items).
|
||||
- `Nest` — multi-item orchestration on `NestEngineBase`, uses `Fill` and `PackArea` as building blocks.
|
||||
|
||||
## Design
|
||||
|
||||
### `IFillStrategy` Interface
|
||||
@@ -22,6 +30,12 @@ public interface IFillStrategy
|
||||
}
|
||||
```
|
||||
|
||||
Strategies must be **stateless**. All mutable state lives in `FillContext`. This avoids leaking state between calls when strategies are shared across invocations.
|
||||
|
||||
Strategies **may** call `NestEngineBase.ReportProgress` for intermediate progress updates (e.g., `LinearFillStrategy` reports per-angle progress). The `FillContext` carries `Progress` and `PlateNumber` for this purpose. The pipeline orchestrator reports progress only when the overall best improves; strategies report their own internal progress as they work.
|
||||
|
||||
For plugin strategies that don't map to a built-in `NestPhase`, use `NestPhase.Custom` (a new enum value added as part of this work). The `Name` property provides the human-readable label.
|
||||
|
||||
### `FillContext`
|
||||
|
||||
Carries inputs and pipeline state through the strategy chain:
|
||||
@@ -49,11 +63,43 @@ public class FillContext
|
||||
}
|
||||
```
|
||||
|
||||
`SharedState` enables cross-strategy data sharing without direct coupling. For example, `PairsFillStrategy` stores the `BestFitCache` results and `ExtentsFillStrategy` retrieves them.
|
||||
`SharedState` enables cross-strategy data sharing without direct coupling. Well-known keys:
|
||||
|
||||
| Key | Type | Producer | Consumer |
|
||||
|-----|------|----------|----------|
|
||||
| `"BestFits"` | `List<BestFitResult>` | `PairsFillStrategy` | `ExtentsFillStrategy` |
|
||||
| `"BestRotation"` | `double` | Pipeline setup | `ExtentsFillStrategy`, `LinearFillStrategy` |
|
||||
| `"AngleCandidates"` | `List<double>` | Pipeline setup | `LinearFillStrategy` |
|
||||
|
||||
### Pipeline Setup
|
||||
|
||||
Before iterating strategies, `RunPipeline` performs shared pre-computation and stores results in `SharedState`:
|
||||
|
||||
```csharp
|
||||
private void RunPipeline(FillContext context)
|
||||
{
|
||||
// Pre-pipeline setup: shared across strategies
|
||||
var bestRotation = RotationAnalysis.FindBestRotation(context.Item);
|
||||
context.SharedState["BestRotation"] = bestRotation;
|
||||
|
||||
var angles = angleBuilder.Build(context.Item, bestRotation, context.WorkArea);
|
||||
context.SharedState["AngleCandidates"] = angles;
|
||||
|
||||
foreach (var strategy in FillStrategyRegistry.Strategies)
|
||||
{
|
||||
// ... strategy loop ...
|
||||
}
|
||||
|
||||
// Post-pipeline: record productive angles for cross-run learning
|
||||
angleBuilder.RecordProductive(context.AngleResults);
|
||||
}
|
||||
```
|
||||
|
||||
The `AngleCandidateBuilder` instance stays on `DefaultNestEngine` (not inside a strategy) because it accumulates cross-run learning state via `RecordProductive`. Strategies read the pre-built angle list from `SharedState["AngleCandidates"]`.
|
||||
|
||||
### `FillStrategyRegistry`
|
||||
|
||||
Discovers strategies via reflection, similar to `NestEngineRegistry.LoadPlugins`:
|
||||
Discovers strategies via reflection, similar to `NestEngineRegistry.LoadPlugins`. Stores strategy **instances** (not factories) because strategies are stateless:
|
||||
|
||||
```csharp
|
||||
public static class FillStrategyRegistry
|
||||
@@ -65,15 +111,27 @@ public static class FillStrategyRegistry
|
||||
LoadFrom(typeof(FillStrategyRegistry).Assembly);
|
||||
}
|
||||
|
||||
private static List<IFillStrategy> sorted;
|
||||
|
||||
public static IReadOnlyList<IFillStrategy> Strategies =>
|
||||
strategies.OrderBy(s => s.Order).ToList();
|
||||
sorted ??= strategies.OrderBy(s => s.Order).ToList();
|
||||
|
||||
public static void LoadFrom(Assembly assembly) { /* scan for IFillStrategy implementations */ }
|
||||
public static void LoadFrom(Assembly assembly)
|
||||
{
|
||||
/* scan for IFillStrategy implementations */
|
||||
sorted = null; // invalidate cache
|
||||
}
|
||||
|
||||
public static void LoadPlugins(string directory) { /* load DLLs and scan each */ }
|
||||
public static void LoadPlugins(string directory)
|
||||
{
|
||||
/* load DLLs and scan each */
|
||||
sorted = null; // invalidate cache
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
Strategy plugins use a `Strategies/` directory (separate from the `Engines/` directory used by `NestEngineRegistry`). Note: plugin strategies cannot use `internal` types like `BinConverter` from `OpenNest.Engine`. If a plugin needs rectangle packing, `BinConverter` would need to be made `public` — defer this until a plugin actually needs it.
|
||||
|
||||
### Built-in Strategy Order
|
||||
|
||||
| Strategy | Order | Notes |
|
||||
@@ -87,12 +145,40 @@ Gaps of 100 allow plugins to slot in between (e.g., Order 150 runs after Pairs,
|
||||
|
||||
### Strategy Implementations
|
||||
|
||||
Each strategy is a thin adapter around the existing filler class:
|
||||
Each strategy is a thin stateless adapter around the existing filler class. Strategies construct filler instances using `context.Plate` properties:
|
||||
|
||||
- **`PairsFillStrategy`** — wraps `PairFiller`, stores `BestFitCache` in `SharedState`
|
||||
- **`RectBestFitStrategy`** — wraps `FillBestFit` via `BinConverter`
|
||||
- **`ExtentsFillStrategy`** — wraps `FillExtents`, reads shared `BestFitCache`
|
||||
- **`LinearFillStrategy`** — wraps `FillLinear` + `AngleCandidateBuilder`
|
||||
```csharp
|
||||
public class PairsFillStrategy : IFillStrategy
|
||||
{
|
||||
public string Name => "Pairs";
|
||||
public NestPhase Phase => NestPhase.Pairs;
|
||||
public int Order => 100;
|
||||
|
||||
public List<Part> Fill(FillContext context)
|
||||
{
|
||||
var filler = new PairFiller(context.Plate.Size, context.Plate.PartSpacing);
|
||||
var result = filler.Fill(context.Item, context.WorkArea,
|
||||
context.PlateNumber, context.Token, context.Progress);
|
||||
|
||||
// Share the BestFitCache for Extents to use later.
|
||||
// This is a cache hit (PairFiller already called GetOrCompute internally),
|
||||
// so it's a dictionary lookup, not a recomputation.
|
||||
var bestFits = BestFitCache.GetOrCompute(
|
||||
context.Item.Drawing, context.Plate.Size.Length,
|
||||
context.Plate.Size.Width, context.Plate.PartSpacing);
|
||||
context.SharedState["BestFits"] = bestFits;
|
||||
|
||||
return result;
|
||||
}
|
||||
}
|
||||
```
|
||||
|
||||
Summary of all four:
|
||||
|
||||
- **`PairsFillStrategy`** — constructs `PairFiller(context.Plate.Size, context.Plate.PartSpacing)`, stores `BestFitCache` in `SharedState`
|
||||
- **`RectBestFitStrategy`** — uses `BinConverter.ToItem(item, partSpacing)` and `BinConverter.CreateBin(workArea, partSpacing)` to delegate to `FillBestFit`
|
||||
- **`ExtentsFillStrategy`** — constructs `FillExtents(context.WorkArea, context.Plate.PartSpacing)`, reads `SharedState["BestRotation"]` for angles, reads `SharedState["BestFits"]` from Pairs
|
||||
- **`LinearFillStrategy`** — constructs `FillLinear(context.WorkArea, context.Plate.PartSpacing)`, reads `SharedState["AngleCandidates"]` for angle list. Internally iterates all angle candidates, tracks its own best, writes per-angle `AngleResults` to context, and calls `ReportProgress` for per-angle updates (preserving the existing UX). Returns only its single best result.
|
||||
|
||||
The underlying classes (`PairFiller`, `FillLinear`, `FillExtents`, `FillBestFit`) are unchanged.
|
||||
|
||||
@@ -103,38 +189,63 @@ The underlying classes (`PairFiller`, `FillLinear`, `FillExtents`, `FillBestFit`
|
||||
```csharp
|
||||
private void RunPipeline(FillContext context)
|
||||
{
|
||||
foreach (var strategy in FillStrategyRegistry.Strategies)
|
||||
var bestRotation = RotationAnalysis.FindBestRotation(context.Item);
|
||||
context.SharedState["BestRotation"] = bestRotation;
|
||||
|
||||
var angles = angleBuilder.Build(context.Item, bestRotation, context.WorkArea);
|
||||
context.SharedState["AngleCandidates"] = angles;
|
||||
|
||||
try
|
||||
{
|
||||
context.Token.ThrowIfCancellationRequested();
|
||||
|
||||
var sw = Stopwatch.StartNew();
|
||||
var result = strategy.Fill(context);
|
||||
sw.Stop();
|
||||
|
||||
context.PhaseResults.Add(new PhaseResult(
|
||||
strategy.Phase, result?.Count ?? 0, sw.ElapsedMilliseconds));
|
||||
|
||||
if (IsBetterFill(result, context.CurrentBest, context.WorkArea))
|
||||
foreach (var strategy in FillStrategyRegistry.Strategies)
|
||||
{
|
||||
context.CurrentBest = result;
|
||||
context.CurrentBestScore = FillScore.Compute(result, context.WorkArea);
|
||||
context.WinnerPhase = strategy.Phase;
|
||||
ReportProgress(context.Progress, strategy.Phase, PlateNumber,
|
||||
result, context.WorkArea, BuildProgressSummary());
|
||||
context.Token.ThrowIfCancellationRequested();
|
||||
|
||||
var sw = Stopwatch.StartNew();
|
||||
var result = strategy.Fill(context);
|
||||
sw.Stop();
|
||||
|
||||
context.PhaseResults.Add(new PhaseResult(
|
||||
strategy.Phase, result?.Count ?? 0, sw.ElapsedMilliseconds));
|
||||
|
||||
if (IsBetterFill(result, context.CurrentBest, context.WorkArea))
|
||||
{
|
||||
context.CurrentBest = result;
|
||||
context.CurrentBestScore = FillScore.Compute(result, context.WorkArea);
|
||||
context.WinnerPhase = strategy.Phase;
|
||||
ReportProgress(context.Progress, strategy.Phase, PlateNumber,
|
||||
result, context.WorkArea, BuildProgressSummary());
|
||||
}
|
||||
}
|
||||
}
|
||||
catch (OperationCanceledException)
|
||||
{
|
||||
// Graceful degradation: return whatever best has been accumulated so far.
|
||||
Debug.WriteLine("[RunPipeline] Cancelled, returning current best");
|
||||
}
|
||||
|
||||
angleBuilder.RecordProductive(context.AngleResults);
|
||||
}
|
||||
```
|
||||
|
||||
After `RunPipeline`, the engine copies `context.PhaseResults` and `context.AngleResults` back to the `NestEngineBase` properties so existing UI and test consumers continue to work:
|
||||
|
||||
```csharp
|
||||
PhaseResults.AddRange(context.PhaseResults);
|
||||
AngleResults.AddRange(context.AngleResults);
|
||||
WinnerPhase = context.WinnerPhase;
|
||||
```
|
||||
|
||||
**Removed from `DefaultNestEngine`:**
|
||||
- `FindBestFill` method (replaced by `RunPipeline`)
|
||||
- `FillRectangleBestFit` method (moves into `RectBestFitStrategy`)
|
||||
- `QuickFillCount` method (moves into relevant strategy)
|
||||
- `AngleCandidateBuilder` field and `ForceFullAngleSweep` property (move into `LinearFillStrategy`)
|
||||
- `QuickFillCount` method (dead code — has zero callers, delete it)
|
||||
|
||||
**Stays in `DefaultNestEngine`:**
|
||||
- `Fill(List<Part> groupParts, ...)` overload — separate group-fill concern
|
||||
- `PackArea` — packing is not part of the fill pipeline
|
||||
**Stays on `DefaultNestEngine`:**
|
||||
- `AngleCandidateBuilder` field — owns cross-run learning state, used in pipeline setup/teardown
|
||||
- `ForceFullAngleSweep` property — forwards to `angleBuilder.ForceFullSweep`, keeps existing public API for `BruteForceRunner` and tests
|
||||
- `Fill(List<Part> groupParts, ...)` overload — out of scope (see Scope section)
|
||||
- `PackArea` — out of scope
|
||||
|
||||
**Static helpers `BuildRotatedPattern` and `FillPattern`** move to `Strategies/FillHelpers.cs`.
|
||||
|
||||
@@ -160,3 +271,25 @@ OpenNest.Engine/
|
||||
- `NestEngineBase.cs` — base class
|
||||
- `NestEngineRegistry.cs` — engine-level registry (separate concern)
|
||||
- `StripNestEngine.cs` — delegates to `DefaultNestEngine` internally
|
||||
|
||||
### Minor Changes to `NestPhase`
|
||||
|
||||
Add `Custom` to the `NestPhase` enum for plugin strategies that don't map to a built-in phase:
|
||||
|
||||
```csharp
|
||||
public enum NestPhase
|
||||
{
|
||||
Linear,
|
||||
RectBestFit,
|
||||
Pairs,
|
||||
Nfp,
|
||||
Extents,
|
||||
Custom
|
||||
}
|
||||
```
|
||||
|
||||
### Testing
|
||||
|
||||
- Existing `EngineRefactorSmokeTests` serve as the regression gate — they must pass unchanged after refactoring.
|
||||
- `BruteForceRunner` continues to access `ForceFullAngleSweep` via the forwarding property on `DefaultNestEngine`.
|
||||
- Individual strategy adapters do not need their own unit tests initially — the existing smoke tests cover the end-to-end pipeline. Strategy-level tests can be added as the strategy count grows.
|
||||
|
||||
Reference in New Issue
Block a user