From f7940efe93a45af49b7f266b2bf553f4e4e81dc9 Mon Sep 17 00:00:00 2001 From: AJ Isaacs Date: Thu, 12 Mar 2026 23:45:21 -0400 Subject: [PATCH] docs: fix contour re-indexing spec from review feedback - Drop Circle.ToArcFrom (zero-sweep problem), keep Circle in shape and handle in ConvertShapeToMoves with full-circle ArcMove - Use point-distance tolerance for Arc.SplitAt instead of angle comparison to avoid wrap-around issues at 0/2pi - Simplify SplitAt return types to non-nullable tuple - Add ArgumentException guard in ReindexAt - Add throw for unexpected entity types in ConvertShapeToMoves - Document absolute coordinate convention and shared references - Clarify variable names for both replacement sites --- .../2026-03-12-contour-reindexing-design.md | 59 ++++++++++--------- 1 file changed, 31 insertions(+), 28 deletions(-) diff --git a/docs/superpowers/specs/2026-03-12-contour-reindexing-design.md b/docs/superpowers/specs/2026-03-12-contour-reindexing-design.md index 768758f..d5ecce8 100644 --- a/docs/superpowers/specs/2026-03-12-contour-reindexing-design.md +++ b/docs/superpowers/specs/2026-03-12-contour-reindexing-design.md @@ -11,37 +11,30 @@ All geometry additions live on existing classes in `OpenNest.Geometry`. The stra ### Line.SplitAt(Vector point) ```csharp -public (Line first, Line second)? SplitAt(Vector point) +public (Line first, Line second) SplitAt(Vector point) ``` - Returns two lines: `StartPoint → point` and `point → EndPoint`. -- If the point is at `StartPoint` (within `Tolerance.Epsilon`), `first` is null. -- If the point is at `EndPoint` (within `Tolerance.Epsilon`), `second` is null. -- If both halves would be degenerate, returns null. +- If the point is at `StartPoint` (within `Tolerance.Epsilon` distance), `first` is null. +- If the point is at `EndPoint` (within `Tolerance.Epsilon` distance), `second` is null. - The point is assumed to lie on the line (caller is responsible — it comes from `ClosestPointTo`). ### Arc.SplitAt(Vector point) ```csharp -public (Arc first, Arc second)? SplitAt(Vector point) +public (Arc first, Arc second) SplitAt(Vector point) ``` - Computes `splitAngle = Center.AngleTo(point)`, normalized via `Angle.NormalizeRad`. - First arc: same center, radius, direction — `StartAngle → splitAngle`. - Second arc: same center, radius, direction — `splitAngle → EndAngle`. -- If `splitAngle` equals `StartAngle` (within tolerance), `first` is null. -- If `splitAngle` equals `EndAngle` (within tolerance), `second` is null. -- If both halves would be degenerate, returns null. +- **Endpoint tolerance**: compare `point.DistanceTo(arc.StartPoint())` and `point.DistanceTo(arc.EndPoint())` rather than comparing angles directly. This avoids wrap-around issues at the 0/2π boundary. +- If the point is at `StartPoint()` (within `Tolerance.Epsilon` distance), `first` is null. +- If the point is at `EndPoint()` (within `Tolerance.Epsilon` distance), `second` is null. -### Circle.ToArcFrom(Vector point) +### Circle — no conversion needed -```csharp -public Arc ToArcFrom(Vector point) -``` - -- Computes angle: `Center.AngleTo(point)`. -- Returns an `Arc` with same center, radius, `StartAngle = EndAngle = angle`, and `IsReversed` matching the circle's `Rotation` (CW → reversed, CCW → not reversed). -- This produces a full-sweep arc that starts and ends at `point`. +Circles are kept as-is in `ReindexAt`. The `ConvertShapeToMoves` method handles circles directly by emitting an `ArcMove` from the start point back to itself (a full circle), matching the existing `ConvertGeometry.AddCircle` pattern. This avoids the problem of constructing a "full-sweep arc" where `StartAngle == EndAngle` would produce zero sweep. ## Shape.ReindexAt @@ -51,20 +44,21 @@ public Shape ReindexAt(Vector point, Entity entity) - `point`: the start/end point for the reindexed contour (from `ClosestPointTo`). - `entity`: the entity containing `point` (from `ClosestPointTo`'s `out` parameter). -- Returns a **new** Shape (does not modify the original). +- Returns a **new** Shape (does not modify the original). The new shape shares entity references with the original for unsplit entities — callers must not mutate either. +- Throws `ArgumentException` if `entity` is not found in `Entities`. ### Algorithm 1. If `entity` is a `Circle`: - - Return a new Shape with a single entity: `circle.ToArcFrom(point)`. + - Return a new Shape with that single `Circle` entity and `point` stored for `ConvertShapeToMoves` to use as the start point. -2. Find the index `i` of `entity` in `Entities`. +2. Find the index `i` of `entity` in `Entities`. Throw `ArgumentException` if not found. 3. Split the entity at `point`: - `Line` → `line.SplitAt(point)` → `(firstHalf, secondHalf)` - `Arc` → `arc.SplitAt(point)` → `(firstHalf, secondHalf)` -4. Build the new entity list (all non-null): +4. Build the new entity list (skip null entries): - `secondHalf` (if not null) - `Entities[i+1]`, `Entities[i+2]`, ..., `Entities[count-1]` (after the split) - `Entities[0]`, `Entities[1]`, ..., `Entities[i-1]` (before the split, wrapping around) @@ -76,7 +70,7 @@ public Shape ReindexAt(Vector point, Entity entity) - **Point lands on entity boundary** (start/end of an entity): one half of the split is null. The reordering still works — it just starts from the next full entity. - **Single-entity shape that is an Arc**: split produces two arcs, reorder is just `[secondHalf, firstHalf]`. -- **Single-entity Circle**: handled by step 1 (converts to full-sweep arc). +- **Single-entity Circle**: handled by step 1 — kept as Circle, converted to a full-circle ArcMove in `ConvertShapeToMoves`. ## Wiring into ContourCuttingStrategy @@ -85,23 +79,33 @@ public Shape ReindexAt(Vector point, Entity entity) Add a private method to `ContourCuttingStrategy`: ```csharp -private List ConvertShapeToMoves(Shape shape) +private List ConvertShapeToMoves(Shape shape, Vector startPoint) ``` -Iterates `shape.Entities` and converts each to cutting moves: +The `startPoint` parameter is needed for the Circle case (to know where the full-circle ArcMove starts). + +Iterates `shape.Entities` and converts each to cutting moves using **absolute coordinates** (consistent with `ConvertGeometry`): - `Line` → `LinearMove(line.EndPoint)` - `Arc` → `ArcMove(arc.EndPoint(), arc.Center, arc.IsReversed ? RotationType.CW : RotationType.CCW)` -- `Circle` → should not appear (circles are converted to arcs by `ReindexAt`) +- `Circle` → `ArcMove(startPoint, circle.Center, circle.Rotation)` — full circle from start point back to itself, matching `ConvertGeometry.AddCircle` +- Any other entity type → throw `InvalidOperationException` No `RapidMove` between entities — they are contiguous in a reindexed shape. The lead-in already positions the head at the shape's start point. ### Replace NotImplementedException -In `ContourCuttingStrategy.Apply()`, replace both `throw new NotImplementedException(...)` blocks with: +In `ContourCuttingStrategy.Apply()`, replace the two `throw new NotImplementedException(...)` blocks: +**Cutout loop** (uses `cutout` shape variable): +```csharp +var reindexed = cutout.ReindexAt(closestPt, entity); +result.Codes.AddRange(ConvertShapeToMoves(reindexed, closestPt)); ``` -var reindexed = shape.ReindexAt(closestPt, entity); -result.Codes.AddRange(ConvertShapeToMoves(reindexed)); + +**Perimeter block** (uses `profile.Perimeter`): +```csharp +var reindexed = profile.Perimeter.ReindexAt(perimeterPt, perimeterEntity); +result.Codes.AddRange(ConvertShapeToMoves(reindexed, perimeterPt)); ``` The full sequence for each contour becomes: @@ -119,7 +123,6 @@ When the lead-out is `MicrotabLeadOut`, the last cutting move must be trimmed by |------|--------| | `OpenNest.Core/Geometry/Line.cs` | Add `SplitAt(Vector)` method | | `OpenNest.Core/Geometry/Arc.cs` | Add `SplitAt(Vector)` method | -| `OpenNest.Core/Geometry/Circle.cs` | Add `ToArcFrom(Vector)` method | | `OpenNest.Core/Geometry/Shape.cs` | Add `ReindexAt(Vector, Entity)` method | | `OpenNest.Core/CNC/CuttingStrategy/ContourCuttingStrategy.cs` | Add `ConvertShapeToMoves`, replace `NotImplementedException` blocks |