Address fill-mode review: grid edge inset + drop dead coverage append
Build image / build-and-push (push) Successful in 18s
Build image / build-and-push (push) Successful in 18s
Gadfly findings on #95: - Correctness (3 models): the shared inset formula radius-spacing/2 collapses to 0 in grid mode (radius = spacing/2), so grid's outer row planted flush on / overhanging the bed edge instead of the half-spacing in that the rule wants. The inset genuinely differs by layout — a grid plant sits AT the plop centre (inset spacing/2), a clump's plants reach its rim (inset radius-spacing/2, overhanging by a half). Split it into a new edgeInset(radius, spacing, layout); hexCenters now takes a precomputed inset and is pure geometry (no spacing/layout knowledge). Regression guard: grid plants land at ±25 on a 60cm bed, not ±30. - Performance (2 findings): the in-loop `existing = append(existing, *p)` was dead — every plop in one fill shares a radius and sits on a distinct lattice point, and a plop is "covered" only when wholly inside another, impossible between equal-radius circles at different centres. Removing it stops the coveredByExisting scan growing during the fill (an empty-bed grid fill's check was needlessly quadratic in the plop count). - Docs: FillRegion/fillLoaded/hexCenters comments and the DESIGN.md bullets updated for plopRadiusFor/edgeInset (were still citing defaultPlopRadius and "written out in hexCenters"). - Test hygiene: split the grid + bad-layout cases out of TestFillAndClearAPI into TestFillLayoutAPI (one concern per test). The enum-tag finding is a non-issue: majordomo's DefineTool derives its arg schema from the same struct-tag reflection as Generate (proven by the vision SeedPacket enum), and the service validates mode regardless. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]> Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
This commit is contained in:
@@ -96,7 +96,7 @@ func TestHexCentersEdgeInset(t *testing.T) {
|
||||
} {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
r := rect(-tc.w/2, -tc.h/2, tc.w/2, tc.h/2)
|
||||
pts, total := hexCenters(r, tc.radius, tc.spacing, maxFillPlops)
|
||||
pts, total := hexCenters(r, tc.radius, edgeInset(tc.radius, tc.spacing, FillClump), maxFillPlops)
|
||||
if len(pts) == 0 {
|
||||
t.Fatal("no centers")
|
||||
}
|
||||
@@ -173,7 +173,7 @@ func TestHexCentersTinyRegion(t *testing.T) {
|
||||
{"off in a corner", rect(20, -40, 30, -30), 25, -35},
|
||||
} {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
pts, _ := hexCenters(tc.r, 15, 10, maxFillPlops)
|
||||
pts, _ := hexCenters(tc.r, 15, edgeInset(15, 10, FillClump), maxFillPlops)
|
||||
if len(pts) != 1 || pts[0].x != tc.wantX || pts[0].y != tc.wantY {
|
||||
t.Errorf("got %+v, want one plop at (%v,%v)", pts, tc.wantX, tc.wantY)
|
||||
}
|
||||
@@ -323,6 +323,7 @@ func TestFillGridLaysOutIndividualPlants(t *testing.T) {
|
||||
t.Errorf("grid produced %d plops, clump %d — grid should be denser", len(grid), len(clump))
|
||||
}
|
||||
// Each grid plop is one plant at radius spacing/2 = 5.
|
||||
maxAbs := 0.0
|
||||
for _, p := range grid {
|
||||
if p.RadiusCM != 5 {
|
||||
t.Errorf("grid plop radius = %v, want 5 (spacing/2)", p.RadiusCM)
|
||||
@@ -330,6 +331,14 @@ func TestFillGridLaysOutIndividualPlants(t *testing.T) {
|
||||
if p.DerivedCount != 1 {
|
||||
t.Errorf("grid plop derived count = %d, want 1 (one plant per plop)", p.DerivedCount)
|
||||
}
|
||||
maxAbs = math.Max(maxAbs, math.Max(math.Abs(p.XCM), math.Abs(p.YCM)))
|
||||
}
|
||||
// The half-spacing edge rule: a grid plant sits AT the plop centre, so the
|
||||
// outer row is inset a half-spacing (spacing/2 = 5) — at ±25 on this 60cm bed,
|
||||
// not flush on ±30. Regression guard: the clump inset formula (radius − half)
|
||||
// collapses to 0 for grid and would plant one on the very edge.
|
||||
if edge := 30.0; maxAbs > edge-5+1e-6 {
|
||||
t.Errorf("outermost grid plop at |coord|=%.2f — only %.2fcm from the edge; want a half-spacing (5cm) in", maxAbs, edge-maxAbs)
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user