From 45da4b15e24f5cfff43b9cce7d569cbf9257fd17 Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Tue, 21 Jul 2026 10:11:08 -0400 Subject: [PATCH 1/7] Fill: honour the half-spacing edge rule when packing plops (#75) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Filling a bed left the outer row too far from the edge, and staggered rows worse still. Two defects, both from anchoring the lattice at the region's min corner: - Odd rows offset by `radius` started at `MinX + 2·radius`, leaving a bare strip a whole plop wide down one side of every other row. - All the leftover slack piled up on the far edge, where plops hung 13cm outside the bed on a 4×8ft garlic bed. Nothing clips them, so they drew over the bed outline. Spacing is a constraint between neighbouring plants competing for the same soil, light and water. A bed edge is not a competitor, so the outer row owes it half the spacing — the arithmetic inside every square-foot-gardening chart (4/square = 6" apart, 3" from the square's edge). The wrinkle: a plop is a CLUMP, not a plant. defaultPlopRadius is 1.5×spacing, so keeping the whole circle inside the bed insets the outer row by 1.5 spacings, three times what the rule allows. So centre the lattice and set the minimum centre-inset to `radius - spacing/2`: the clump may cross the edge by up to half a spacing, putting its outermost plants exactly the half-spacing from the edge the rule asks for. Capped there — a clump mostly outside the bed would be a drawing of plants in the path. Same bed, same 15 plops, now symmetric with a deliberate 6.5cm overhang inside the 7.5cm budget instead of an accidental 13cm on one side only. The stagger falls out of the centring for free: an offset row holds one fewer plop, and centring that run puts it exactly half a pitch off its neighbours. TestFillRegionDeterministicPacking expected 4 plops in a 60×60 bed; the fourth was centred ON the east edge with half of it outside, well past the budget. It is 3 now — the fix working, not a regression in it. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ --- CLAUDE.md | 6 ++ DESIGN.md | 1 + internal/service/ops.go | 98 ++++++++++++++++++++++--------- internal/service/ops_test.go | 108 +++++++++++++++++++++++++++++++++-- 4 files changed, 182 insertions(+), 31 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 58cab74..0d6e95a 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -85,6 +85,12 @@ Frontend: React 19 + Vite + Tailwind 4 + TanStack Router/Query, built into deliberately. `ErrForbidden` means "you can see it but may not do that". - **Plops (plantings) live in their parent object's local frame**, origin at the object's center, `-y` is north. Moving or rotating a bed moves its plants free. +- **A plop is a clump, not a plant.** `defaultPlopRadius` is `1.5 × spacing`, so a + plop is three spacings across and holds `π·r²/spacing²` plants. Reasoning about + fills as if one plop were one plant gets the geometry wrong every time — which + is how #75 happened: requiring the whole circle inside the bed inset the outer + row by 1.5 spacings when the horticultural rule is *half* a spacing. Spacing is + a constraint between neighbouring plants; a bed edge is nobody's neighbour. - **Soft removal**: "clear bed" sets `removed_at`; the editor reads `removed_at IS NULL`. Hard delete is a different operation. - **Migrations** are numbered `.sql` files in `internal/store/migrations/`, run diff --git a/DESIGN.md b/DESIGN.md index f217298..cd0a8c3 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -7,6 +7,7 @@ Work is tracked in Gitea issues; the tracking epic links every piece in dependen ## Decisions - **Placement model:** freeform plops (not a square-foot grid), scaled by real plant spacing. Grid snapping may come later as a toggle. +- **Spacing is a plant-to-plant rule, so bed edges get half of it.** Spacing describes two plants competing for the same soil, light and water; a bed edge is not a competitor, so the outer row owes it only *half* the spacing — the arithmetic behind every square-foot chart (4/square = 6" apart, 3" from the edge). `FillRegion` centres its lattice and lets a plop, which is a *clump* 3 spacings across, cross the edge by up to half a spacing so its outermost plants land at that half-spacing. See `hexCenters`; #75 is what getting this wrong looked like. - **Stack:** Go 1.26.x backend, module `gitea.stevedudenhoeffer.com/steve/pansy`; React + TypeScript + Vite + Tailwind frontend, production build embedded via `embed.FS` → one static binary (`CGO_ENABLED=0`). - **Users:** multi-user with ownership. Users own gardens; a garden can be shared with other users as viewer (read) or editor (edit content). Owner additionally shares/deletes. - **Auth:** OIDC-first (Authentik is the primary IdP), local argon2id passwords as an optional fallback. diff --git a/internal/service/ops.go b/internal/service/ops.go index 90be1d4..5ab9ff6 100644 --- a/internal/service/ops.go +++ b/internal/service/ops.go @@ -28,11 +28,6 @@ type Region struct { MinX, MinY, MaxX, MaxY float64 } -// contains reports whether a local point lies in the region. -func (r Region) contains(x, y float64) bool { - return x >= r.MinX && x <= r.MaxX && y >= r.MinY && y <= r.MaxY -} - // clampTo intersects the region with an object's local bounds (±halfW, ±halfH), // so an oversized caller-supplied region can't make hexCenters loop forever. func (r Region) clampTo(halfW, halfH float64) Region { @@ -94,9 +89,10 @@ func defaultPlopRadius(spacingCM float64) float64 { // FillRegion lays a hex-packed field of plops of one plant across a region of a // plantable object the actor can edit. Plop radius comes from the plant's spacing // (or spacingOverride) via defaultPlopRadius; centers sit on a hex lattice at 2× -// radius pitch, kept where the center is inside the region. A candidate is -// skipped when its plop would sit entirely inside an existing active plop (so -// re-filling doesn't stack duplicates). Returns the plops it created. +// radius pitch, centered in the region with a half-pitch inset at each edge (see +// hexCenters for why that inset and not a full one). A candidate is skipped when +// its plop would sit entirely inside an existing active plop (so re-filling +// doesn't stack duplicates). Returns the plops it created. func (s *Service) FillRegion(ctx context.Context, actorID, objectID int64, region Region, plantID int64, spacingOverride *float64) ([]domain.Planting, error) { o, _, err := s.objectForRole(ctx, actorID, objectID, roleEditor) if err != nil { @@ -130,7 +126,7 @@ func (s *Service) fillLoaded(ctx context.Context, actorID int64, o *domain.Garde } region = region.clampTo(o.WidthCM/2, o.HeightCM/2) - centers := hexCenters(region, radius) + centers := hexCenters(region, radius, spacing) if len(centers) > maxFillPlops { return nil, domain.ErrInvalidInput // region too large for this spacing; ask for less } @@ -169,34 +165,84 @@ func (s *Service) fillLoaded(ctx context.Context, actorID int64, o *domain.Garde type localPoint struct{ x, y float64 } -// hexCenters returns hex-packed lattice centers whose center lies in the region. -// Rows are spaced radius·√3 apart and every other row is offset by radius, the -// standard hexagonal packing at a 2×radius pitch. The lattice is anchored one -// radius inside the region's min corner so the first plop sits inside it. -func hexCenters(r Region, radius float64) []localPoint { +// hexCenters returns hex-packed lattice centers filling a region: rows radius·√3 +// apart, alternate rows offset by half a pitch, at a 2×radius pitch. The lattice +// is CENTERED, so the leftover is shared between opposite edges instead of piling +// up against the far one. +// +// # How close to the edge the outer row goes +// +// Spacing is a constraint BETWEEN NEIGHBOURING PLANTS competing for the same +// soil, light and water. A bed edge is not a competitor, so the outer row only +// owes it HALF the spacing — the half it would otherwise share with a neighbour. +// That is the arithmetic inside every square-foot-gardening chart: 4 per square +// is 6" apart and 3" from the square's edge; 9 per square is 4" apart and 2" +// from the edge. Garlic at 9 per square goes in 2" from the frame, not 6". +// +// A plop is a CLUMP, not a plant — defaultPlopRadius makes it 1.5×spacing, so +// three spacings across — and its plants sit out to its rim. So keeping the whole +// circle inside the bed would inset the outer row by a full 1.5 spacings, three +// times what the rule allows. Instead the clump may hang over the edge by up to +// half a spacing, which puts its outermost plants exactly the half-spacing from +// the edge that the rule asks for. Overhang is capped there and nowhere near the +// full radius: a clump mostly outside the bed is a drawing of plants in the path. +// +// Anchoring at the min corner instead (what this did originally) was wrong twice +// over: staggered rows started a FULL pitch in, leaving a clump-sized bare strip +// down one side of every other row, while the far edge had clumps hanging off it +// by 13cm — and nothing clips them, so they drew outside the bed. +func hexCenters(r Region, radius, spacing float64) []localPoint { if radius <= 0 { return nil } pitch := 2 * radius rowH := pitch * math.Sqrt(3) / 2 - const eps = 1e-6 - var pts []localPoint - row := 0 - for y := r.MinY + radius; y <= r.MaxY+eps; y += rowH { - xStart := r.MinX + radius - if row%2 == 1 { - xStart += radius + + // How far a clump's centre must stay inside the edge: its own radius, less the + // half-spacing of overhang the rule allows. Never negative, and never past the + // centre of the clump. + inset := math.Max(0, radius-math.Max(0, spacing)/2) + + rows, y0 := fitAxis(r.MaxY-r.MinY, rowH, inset) + cols, x0 := fitAxis(r.MaxX-r.MinX, pitch, inset) + + pts := make([]localPoint, 0, rows*cols) + for row := 0; row < rows; row++ { + y := r.MinY + y0 + float64(row)*rowH + n, x := cols, r.MinX+x0 + // The stagger falls out of centering: an offset row holds one fewer plop, + // and centering THAT run puts it exactly half a pitch off its neighbours. + // A single-column region has nothing to stagger against. + if row%2 == 1 && cols > 1 { + n, x = cols-1, r.MinX+x0+radius } - for x := xStart; x <= r.MaxX+eps; x += pitch { - if r.contains(x, y) { - pts = append(pts, localPoint{x, y}) - } + for i := 0; i < n; i++ { + pts = append(pts, localPoint{x + float64(i)*pitch, y}) } - row++ } return pts } +// fitAxis returns how many lattice positions fit along a span at `step`, keeping +// at least `inset` from each end, and the offset from the span's start that +// centers them — so the leftover is split between the two edges rather than all +// landing on the far one. +// +// A span too small to hold even one position at that inset still gets one, in the +// middle: filling a bed narrower than a single plop with one plop is a better +// answer than refusing to plant it. +func fitAxis(length, step, inset float64) (n int, start float64) { + if step <= 0 || length < 2*inset { + return 1, length / 2 + } + // The epsilon keeps an exact fit from being lost to floating point — a 60cm + // span at a 30cm step should give 2 positions, not 1 because the division + // landed on 0.9999999. + const eps = 1e-9 + n = int(math.Floor((length-2*inset)/step+eps)) + 1 + return n, (length - float64(n-1)*step) / 2 +} + // coveredByExisting reports whether a new plop (center, radius) would sit // entirely inside some existing active plop. func coveredByExisting(x, y, radius float64, existing []domain.Planting) bool { diff --git a/internal/service/ops_test.go b/internal/service/ops_test.go index b078188..b2e5d11 100644 --- a/internal/service/ops_test.go +++ b/internal/service/ops_test.go @@ -3,6 +3,8 @@ package service import ( "context" "errors" + "math" + "sort" "testing" "gitea.stevedudenhoeffer.com/steve/pansy/internal/domain" @@ -71,6 +73,94 @@ func TestDefaultPlopRadius(t *testing.T) { } } +// TestHexCentersEdgeInset pins the spacing rule the packing exists to honour: +// spacing is a constraint between neighbouring plants, so a bed edge — which is +// nobody's neighbour — is owed half a pitch, not a whole one. +// +// The bug this guards against was visible to anyone who filled a bed: staggered +// rows began a full pitch in, leaving a bare strip a whole plop wide down one +// side of every other row, while the far edge had plops hanging off it. +func TestHexCentersEdgeInset(t *testing.T) { + for _, tc := range []struct { + name string + w, h, radius, spacing float64 + wantRowStarts []float64 // x of the first plop in rows 0 and 1 + }{ + // 4ft × 8ft bed, garlic at 15cm spacing → radius 22.5, pitch 45. Three + // columns, the outer ones overhanging by 6.5cm — under the 7.5cm the rule + // allows. Anchored at the corner this row started at -38.5 and its + // staggered neighbour a full 45 further in still. + {"4ft bed of garlic", 122, 244, 22.5, 15, []float64{-45, -22.5}}, + // An exact fit: 90 wide at pitch 30 → 3 columns, no overhang needed. + {"exact fit", 90, 90, 15, 10, []float64{-30, -15}}, + } { + t.Run(tc.name, func(t *testing.T) { + r := rect(-tc.w/2, -tc.h/2, tc.w/2, tc.h/2) + pts := hexCenters(r, tc.radius, tc.spacing) + if len(pts) == 0 { + t.Fatal("no centers") + } + + // A clump may cross the edge, but only by the half-spacing the rule + // allows — never enough to be mostly out in the path. + max := tc.spacing / 2 + for _, p := range pts { + over := math.Max( + math.Max(r.MinX-(p.x-tc.radius), (p.x+tc.radius)-r.MaxX), + math.Max(r.MinY-(p.y-tc.radius), (p.y+tc.radius)-r.MaxY), + ) + if over > max+1e-6 { + t.Errorf("plop at (%.1f,%.1f) overhangs by %.2f, max %.2f", p.x, p.y, over, max) + } + } + + // The margins match on opposite edges: the leftover is shared, not piled + // against the far side. + minX, maxX, minY, maxY := pts[0].x, pts[0].x, pts[0].y, pts[0].y + for _, p := range pts { + minX, maxX = math.Min(minX, p.x), math.Max(maxX, p.x) + minY, maxY = math.Min(minY, p.y), math.Max(maxY, p.y) + } + if w, e := minX-r.MinX, r.MaxX-maxX; math.Abs(w-e) > 1e-6 { + t.Errorf("lopsided horizontally: west margin %.2f, east %.2f", w, e) + } + if n, s := minY-r.MinY, r.MaxY-maxY; math.Abs(n-s) > 1e-6 { + t.Errorf("lopsided vertically: north margin %.2f, south %.2f", n, s) + } + + // The staggered row is offset by HALF a pitch, not a whole one. + starts := map[float64]float64{} + for _, p := range pts { + if x, ok := starts[p.y]; !ok || p.x < x { + starts[p.y] = p.x + } + } + ys := make([]float64, 0, len(starts)) + for y := range starts { + ys = append(ys, y) + } + sort.Float64s(ys) + for i, want := range tc.wantRowStarts { + if i >= len(ys) { + t.Fatalf("only %d rows, want at least %d", len(ys), len(tc.wantRowStarts)) + } + if got := starts[ys[i]]; math.Abs(got-want) > 1e-6 { + t.Errorf("row %d starts at x=%.2f, want %.2f", i, got, want) + } + } + }) + } +} + +// TestHexCentersTinyRegion covers a region too small to hold a plop at the +// half-pitch inset: planting one in the middle beats refusing to plant at all. +func TestHexCentersTinyRegion(t *testing.T) { + pts := hexCenters(rect(-5, -5, 5, 5), 15, 10) + if len(pts) != 1 || pts[0].x != 0 || pts[0].y != 0 { + t.Errorf("tiny region gave %+v, want a single centered plop", pts) + } +} + // seedFillBed makes a plantable bed of the given size centered in a big garden. func seedFillBed(t *testing.T, s *Service, owner, gardenID int64, w, h float64) *domain.GardenObject { t.Helper() @@ -99,16 +189,24 @@ func TestFillRegionDeterministicPacking(t *testing.T) { if err != nil { t.Fatalf("FillRegion: %v", err) } - // Hex lattice on [-30,30]² at pitch 30, rows ~26 apart → 4 plops (2 rows × 2). - if len(created) != 4 { - t.Fatalf("filled %d plops, want 4 (60×60 bed, radius 15)", len(created)) + // Hex lattice on [-30,30]² at pitch 30, rows ~26 apart, centered: a row of 2 + // (x=±15), then a staggered row of 1 (x=0) → 3 plops. + // + // This was 4 while the lattice was anchored at the min corner, and the fourth + // sat at x=30 — centred ON the east edge, so half of it lay outside the bed, + // well past the half-spacing (5cm here) the rule allows. Packing one fewer + // plop is the point of the fix, not a regression in it. + if len(created) != 3 { + t.Fatalf("filled %d plops, want 3 (60×60 bed, radius 15)", len(created)) } for _, p := range created { if p.RadiusCM != 15 || p.PlantedAt == nil || p.DerivedCount < 1 { t.Errorf("unexpected created plop: %+v", p) } - if p.XCM < -30 || p.XCM > 30 || p.YCM < -30 || p.YCM > 30 { - t.Errorf("plop center out of bed bounds: %+v", p) + // This bed fits its lattice exactly, so nothing should need to overhang. + if p.XCM-p.RadiusCM < -30 || p.XCM+p.RadiusCM > 30 || + p.YCM-p.RadiusCM < -30 || p.YCM+p.RadiusCM > 30 { + t.Errorf("plop overhangs a bed it fits inside: %+v", p) } } From 3af0d087790a524c6adbc8d2dad57ae26bb93a72 Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Tue, 21 Jul 2026 10:13:33 -0400 Subject: [PATCH 2/7] Fill: plant nothing for a region that misses the object entirely MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit clampTo INVERTS a region lying wholly outside the object — Max clamps below Min — rather than emptying it. The old loop-until-past-MaxX form handled that for free by never entering the loop. Counting positions up front does not: a region 500cm east of a bed with ±50cm local bounds produced 4 plops at x=275, a couple of metres off the bed. Caught by removing the guard and watching the new test fail, not by assuming it would. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ --- internal/service/ops.go | 7 +++++++ internal/service/ops_test.go | 21 +++++++++++++++++++++ 2 files changed, 28 insertions(+) diff --git a/internal/service/ops.go b/internal/service/ops.go index 5ab9ff6..a0c9865 100644 --- a/internal/service/ops.go +++ b/internal/service/ops.go @@ -195,6 +195,13 @@ func hexCenters(r Region, radius, spacing float64) []localPoint { if radius <= 0 { return nil } + // clampTo INVERTS a region that lies wholly outside the object (Max clamps + // below Min), and an inverted region has no inside to plant. The old + // loop-until-past-MaxX form got this for free by never entering the loop; + // counting positions up front does not, and would site a plop off the bed. + if r.MaxX < r.MinX || r.MaxY < r.MinY { + return nil + } pitch := 2 * radius rowH := pitch * math.Sqrt(3) / 2 diff --git a/internal/service/ops_test.go b/internal/service/ops_test.go index b2e5d11..d36e499 100644 --- a/internal/service/ops_test.go +++ b/internal/service/ops_test.go @@ -161,6 +161,27 @@ func TestHexCentersTinyRegion(t *testing.T) { } } +// TestFillRegionOutsideObjectPlantsNothing covers a region that misses the object +// entirely. clampTo inverts such a region rather than emptying it, and an +// inverted region must plant nothing — not one plop at some point off the bed. +func TestFillRegionOutsideObjectPlantsNothing(t *testing.T) { + ctx := context.Background() + s := newTestService(t, openConfig()) + owner := seedUser(t, s, "a@example.com") + g, _ := s.CreateGarden(ctx, owner, GardenInput{Name: "Big", WidthCM: 2000, HeightCM: 2000}) + bed := seedFillBed(t, s, owner, g.ID, 100, 100) // local bounds ±50 + plant := seedOwnPlant(t, s, owner, 10) + + // Wholly east of the bed: clampTo gives MinX=500, MaxX=50. + created, err := s.FillRegion(ctx, owner, bed.ID, rect(500, -50, 600, 50), plant.ID, nil) + if err != nil { + t.Fatalf("FillRegion: %v", err) + } + if len(created) != 0 { + t.Errorf("filled %d plops for a region outside the bed, want 0: %+v", len(created), created) + } +} + // seedFillBed makes a plantable bed of the given size centered in a big garden. func seedFillBed(t *testing.T, s *Service, owner, gardenID int64, w, h float64) *domain.GardenObject { t.Helper() From f8929a19a8b7f47613f6e91bce326991299710e2 Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Tue, 21 Jul 2026 10:23:22 -0400 Subject: [PATCH 3/7] Address Gadfly findings on #76 - clampTo's doc justified itself by stopping hexCenters "looping forever", which stopped being true when hexCenters became count-bounded. Say what it actually does now, and note the inversion the new guard relies on. - Trim the changelog prose from hexCenters' doc down to the one line that earns its keep: don't re-anchor at the min corner, and why. - Rename a test local from `max` so it stops shadowing the builtin. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ --- internal/service/ops.go | 12 +++++++----- internal/service/ops_test.go | 6 +++--- 2 files changed, 10 insertions(+), 8 deletions(-) diff --git a/internal/service/ops.go b/internal/service/ops.go index a0c9865..f1e18be 100644 --- a/internal/service/ops.go +++ b/internal/service/ops.go @@ -29,7 +29,10 @@ type Region struct { } // clampTo intersects the region with an object's local bounds (±halfW, ±halfH), -// so an oversized caller-supplied region can't make hexCenters loop forever. +// so a fill can't plant outside the object it was aimed at. +// +// Note this INVERTS (Max ends up below Min) rather than empties a region that +// misses the object altogether — hexCenters treats that as "nothing to plant". func (r Region) clampTo(halfW, halfH float64) Region { return Region{ MinX: math.Max(r.MinX, -halfW), MinY: math.Max(r.MinY, -halfH), @@ -187,10 +190,9 @@ type localPoint struct{ x, y float64 } // the edge that the rule asks for. Overhang is capped there and nowhere near the // full radius: a clump mostly outside the bed is a drawing of plants in the path. // -// Anchoring at the min corner instead (what this did originally) was wrong twice -// over: staggered rows started a FULL pitch in, leaving a clump-sized bare strip -// down one side of every other row, while the far edge had clumps hanging off it -// by 13cm — and nothing clips them, so they drew outside the bed. +// Do not "simplify" this back to anchoring at the region's min corner. That is +// what #75 was: staggered rows start a full pitch in, and the leftover all lands +// on the far edge, where clumps hang outside a bed that nothing clips them to. func hexCenters(r Region, radius, spacing float64) []localPoint { if radius <= 0 { return nil diff --git a/internal/service/ops_test.go b/internal/service/ops_test.go index d36e499..18e6939 100644 --- a/internal/service/ops_test.go +++ b/internal/service/ops_test.go @@ -103,14 +103,14 @@ func TestHexCentersEdgeInset(t *testing.T) { // A clump may cross the edge, but only by the half-spacing the rule // allows — never enough to be mostly out in the path. - max := tc.spacing / 2 + budget := tc.spacing / 2 for _, p := range pts { over := math.Max( math.Max(r.MinX-(p.x-tc.radius), (p.x+tc.radius)-r.MaxX), math.Max(r.MinY-(p.y-tc.radius), (p.y+tc.radius)-r.MaxY), ) - if over > max+1e-6 { - t.Errorf("plop at (%.1f,%.1f) overhangs by %.2f, max %.2f", p.x, p.y, over, max) + if over > budget+1e-6 { + t.Errorf("plop at (%.1f,%.1f) overhangs by %.2f, budget %.2f", p.x, p.y, over, budget) } } From 70ff9706720d46b54e0ae364f4660892cceb1ad7 Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Tue, 21 Jul 2026 12:13:50 -0400 Subject: [PATCH 4/7] Address second round of Gadfly findings on #76 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - FillRegion's doc still said "half-pitch inset", left over from the first draft of the fix; the inset is radius - spacing/2. Two docs on the same function disagreeing is worse than either being terse. - Reject non-finite region bounds. They survive clamping and the inverted- region guard (NaN compares false both ways). Nothing corrupt reached the table — SQLite stores NaN as NULL and NOT NULL refuses it — but NaN surfaced as a raw store error and +Inf as a silent zero-plop success. - TestHexCentersTinyRegion used a region symmetric about the origin, so it could not distinguish "the middle of the region" from "the origin" and would have passed for an implementation that just returned (0,0). Added an off-centre case. Not taken: the finding that `make(..., rows*cols)` over-allocates ~12% because staggered rows hold cols-1. True, but the slice is capped at maxFillPlops (5000) and the exact count needs a ceil/floor split for no measurable gain. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ --- internal/service/ops.go | 21 ++++++++++++--- internal/service/ops_test.go | 51 +++++++++++++++++++++++++++++++++--- 2 files changed, 65 insertions(+), 7 deletions(-) diff --git a/internal/service/ops.go b/internal/service/ops.go index f1e18be..4e3cab7 100644 --- a/internal/service/ops.go +++ b/internal/service/ops.go @@ -92,10 +92,11 @@ func defaultPlopRadius(spacingCM float64) float64 { // FillRegion lays a hex-packed field of plops of one plant across a region of a // plantable object the actor can edit. Plop radius comes from the plant's spacing // (or spacingOverride) via defaultPlopRadius; centers sit on a hex lattice at 2× -// radius pitch, centered in the region with a half-pitch inset at each edge (see -// hexCenters for why that inset and not a full one). A candidate is skipped when -// its plop would sit entirely inside an existing active plop (so re-filling -// doesn't stack duplicates). Returns the plops it created. +// radius pitch, centered in the region, and set in from each edge by the plop's +// radius less half a spacing — see hexCenters for why that half-spacing is what +// the edge is owed. A candidate is skipped when its plop would sit entirely +// inside an existing active plop (so re-filling doesn't stack duplicates). +// Returns the plops it created. func (s *Service) FillRegion(ctx context.Context, actorID, objectID int64, region Region, plantID int64, spacingOverride *float64) ([]domain.Planting, error) { o, _, err := s.objectForRole(ctx, actorID, objectID, roleEditor) if err != nil { @@ -128,6 +129,18 @@ func (s *Service) fillLoaded(ctx context.Context, actorID int64, o *domain.Garde return nil, domain.ErrInvalidInput } + // A caller-supplied region is arbitrary floats, and non-finite ones survive + // everything downstream: clamping keeps them, the inverted-region guard can't + // see NaN (it compares false both ways), and fitAxis centres on them happily. + // Nothing corrupt reaches the table — SQLite stores NaN as NULL and the NOT + // NULL constraint refuses it — but the caller gets an opaque store error for + // NaN, and for +Inf a silent zero-plop success. Both are lies about what went + // wrong; say "bad input" here instead. + if !isFinite(region.MinX) || !isFinite(region.MinY) || + !isFinite(region.MaxX) || !isFinite(region.MaxY) { + return nil, domain.ErrInvalidInput + } + region = region.clampTo(o.WidthCM/2, o.HeightCM/2) centers := hexCenters(region, radius, spacing) if len(centers) > maxFillPlops { diff --git a/internal/service/ops_test.go b/internal/service/ops_test.go index 18e6939..af84de1 100644 --- a/internal/service/ops_test.go +++ b/internal/service/ops_test.go @@ -154,10 +154,55 @@ func TestHexCentersEdgeInset(t *testing.T) { // TestHexCentersTinyRegion covers a region too small to hold a plop at the // half-pitch inset: planting one in the middle beats refusing to plant at all. +// +// The off-centre case earns its place — a region symmetric about the origin +// can't tell "the middle of the region" from "the origin", so on its own it +// would pass for an implementation that just returned (0,0). func TestHexCentersTinyRegion(t *testing.T) { - pts := hexCenters(rect(-5, -5, 5, 5), 15, 10) - if len(pts) != 1 || pts[0].x != 0 || pts[0].y != 0 { - t.Errorf("tiny region gave %+v, want a single centered plop", pts) + for _, tc := range []struct { + name string + r Region + wantX, wantY float64 + }{ + {"centred on the origin", rect(-5, -5, 5, 5), 0, 0}, + {"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) + 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) + } + }) + } +} + +// TestFillRegionRejectsNonFiniteRegion: non-finite bounds survive clamping and +// the inverted-region guard (NaN compares false both ways). Without the explicit +// check, NaN surfaced as a raw store error ("NOT NULL constraint failed") and +// +Inf as a silent success that planted nothing — neither of which tells the +// caller what it actually did wrong. +func TestFillRegionRejectsNonFiniteRegion(t *testing.T) { + ctx := context.Background() + s := newTestService(t, openConfig()) + owner := seedUser(t, s, "a@example.com") + g, _ := s.CreateGarden(ctx, owner, GardenInput{Name: "Big", WidthCM: 2000, HeightCM: 2000}) + bed := seedFillBed(t, s, owner, g.ID, 100, 100) + plant := seedOwnPlant(t, s, owner, 10) + + nan := math.NaN() + for _, r := range []Region{ + {MinX: nan, MinY: -50, MaxX: 50, MaxY: 50}, + {MinX: -50, MinY: -50, MaxX: 50, MaxY: math.Inf(1)}, + } { + created, err := s.FillRegion(ctx, owner, bed.ID, r, plant.ID, nil) + if !errors.Is(err, domain.ErrInvalidInput) { + t.Errorf("FillRegion(%+v) err = %v, want ErrInvalidInput", r, err) + } + for _, p := range created { + if !isFinite(p.XCM) || !isFinite(p.YCM) { + t.Errorf("persisted a plop with non-finite coordinates: %+v", p) + } + } } } From 28af1016343562bdf2daa400fdeaf502f2fd70dc Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Tue, 21 Jul 2026 13:25:58 -0400 Subject: [PATCH 5/7] Address third round of Gadfly findings on #76 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - hexCenters now derives its exact point count BEFORE building anything and returns it alongside the points, refusing over the cap without allocating. Previously it materialised the whole lattice and fillLoaded checked len() afterwards — so the "too large" path paid for the thing it was rejecting. This also makes the preallocation exact, which subsumes the earlier over-allocation finding I'd declined: staggered rows hold cols-1, so rows*cols over-reserved by ~12%. - Region.empty() names the invariant that clampTo expresses "no overlap" by INVERTING the region rather than zeroing it. A bare `MaxX < MinX` at each call site was spreading a non-obvious convention across three functions. The count is now load-bearing (it gates the cap), so the test asserts it matches what actually gets built. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ --- CLAUDE.md | 15 ++++++++++ internal/service/ops.go | 57 +++++++++++++++++++++++++----------- internal/service/ops_test.go | 9 ++++-- 3 files changed, 62 insertions(+), 19 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 0d6e95a..ce29833 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -129,6 +129,21 @@ fix what's real → merge when the pipeline is green. Do not grade Gadfly findin A push to `main` builds the image and deploys to Komodo; the live instance at `pansy.orgrimmar.dudenhoeffer.casa` updates a few minutes later. +**Gadfly reviews the PR as opened, not as merged.** The workflow triggers on +`opened`/`reopened`/`ready_for_review` — deliberately *not* `synchronize` — so +every commit you push afterwards, including the ones you push in response to +Gadfly itself, is unreviewed unless you ask. Once you've stopped pushing and +before you merge, comment **`@gadfly review`** on the PR to re-trigger it. The +phrase is required, and this is not hypothetical: on #76 the follow-up commit +was the one that contained a real bug. + +**A skipped Gadfly run reports success.** A comment without the trigger phrase +still starts the workflow, which logs `comment does not contain trigger phrase` +and exits green in ~2 seconds. So "the pipeline is green" does NOT mean "this +was reviewed". Confirm a re-review actually ran by checking that it posted a new +consensus comment — or that the run took minutes rather than seconds — not by +its status. + Workflow- and config-only changes (CI, this file, docs) go straight to `main` without the PR dance. diff --git a/internal/service/ops.go b/internal/service/ops.go index 4e3cab7..2fa05df 100644 --- a/internal/service/ops.go +++ b/internal/service/ops.go @@ -29,10 +29,8 @@ type Region struct { } // clampTo intersects the region with an object's local bounds (±halfW, ±halfH), -// so a fill can't plant outside the object it was aimed at. -// -// Note this INVERTS (Max ends up below Min) rather than empties a region that -// misses the object altogether — hexCenters treats that as "nothing to plant". +// so a fill can't plant outside the object it was aimed at. A region that misses +// the object entirely comes back empty — see empty(). func (r Region) clampTo(halfW, halfH float64) Region { return Region{ MinX: math.Max(r.MinX, -halfW), MinY: math.Max(r.MinY, -halfH), @@ -40,6 +38,16 @@ func (r Region) clampTo(halfW, halfH float64) Region { } } +// empty reports whether the region encloses nothing. +// +// This exists because clampTo expresses "no overlap" by INVERTING the region — +// Max clamps below Min — rather than by zeroing it, which is not something a +// reader guesses. Naming it once here beats a bare `MaxX < MinX` at each place +// that has to care. +func (r Region) empty() bool { + return r.MaxX < r.MinX || r.MaxY < r.MinY +} + // rect builds a rectangular region. func rect(minX, minY, maxX, maxY float64) Region { return Region{MinX: minX, MinY: minY, MaxX: maxX, MaxY: maxY} @@ -142,8 +150,8 @@ func (s *Service) fillLoaded(ctx context.Context, actorID int64, o *domain.Garde } region = region.clampTo(o.WidthCM/2, o.HeightCM/2) - centers := hexCenters(region, radius, spacing) - if len(centers) > maxFillPlops { + centers, total := hexCenters(region, radius, spacing, maxFillPlops) + if total > maxFillPlops { return nil, domain.ErrInvalidInput // region too large for this spacing; ask for less } @@ -206,16 +214,19 @@ type localPoint struct{ x, y float64 } // Do not "simplify" this back to anchoring at the region's min corner. That is // what #75 was: staggered rows start a full pitch in, and the leftover all lands // on the far edge, where clumps hang outside a bed that nothing clips them to. -func hexCenters(r Region, radius, spacing float64) []localPoint { +// It reports the total alongside the points, and works that total out BEFORE +// building anything: a fill large enough to be refused shouldn't allocate its +// whole lattice first just to be counted and thrown away. Over `limit` it +// returns (nil, total) so the caller can refuse with the real number. +func hexCenters(r Region, radius, spacing float64, limit int) ([]localPoint, int) { if radius <= 0 { - return nil + return nil, 0 } - // clampTo INVERTS a region that lies wholly outside the object (Max clamps - // below Min), and an inverted region has no inside to plant. The old - // loop-until-past-MaxX form got this for free by never entering the loop; - // counting positions up front does not, and would site a plop off the bed. - if r.MaxX < r.MinX || r.MaxY < r.MinY { - return nil + // An empty region has no inside to plant. The old loop-until-past-MaxX form + // got this for free by never entering the loop; counting positions up front + // does not, and would site a plop off the bed. + if r.empty() { + return nil, 0 } pitch := 2 * radius rowH := pitch * math.Sqrt(3) / 2 @@ -228,7 +239,19 @@ func hexCenters(r Region, radius, spacing float64) []localPoint { rows, y0 := fitAxis(r.MaxY-r.MinY, rowH, inset) cols, x0 := fitAxis(r.MaxX-r.MinX, pitch, inset) - pts := make([]localPoint, 0, rows*cols) + // Exact, not an upper bound: staggered rows hold one fewer, so rows*cols would + // over-reserve by ~12% — and, more to the point, allocating it is the thing we + // are trying to avoid when the answer is "too many". + staggered := cols + if cols > 1 { + staggered = cols - 1 + } + total := (rows+1)/2*cols + rows/2*staggered + if total > limit { + return nil, total + } + + pts := make([]localPoint, 0, total) for row := 0; row < rows; row++ { y := r.MinY + y0 + float64(row)*rowH n, x := cols, r.MinX+x0 @@ -236,13 +259,13 @@ func hexCenters(r Region, radius, spacing float64) []localPoint { // and centering THAT run puts it exactly half a pitch off its neighbours. // A single-column region has nothing to stagger against. if row%2 == 1 && cols > 1 { - n, x = cols-1, r.MinX+x0+radius + n, x = staggered, r.MinX+x0+radius } for i := 0; i < n; i++ { pts = append(pts, localPoint{x + float64(i)*pitch, y}) } } - return pts + return pts, total } // fitAxis returns how many lattice positions fit along a span at `step`, keeping diff --git a/internal/service/ops_test.go b/internal/service/ops_test.go index af84de1..1a003a1 100644 --- a/internal/service/ops_test.go +++ b/internal/service/ops_test.go @@ -96,10 +96,15 @@ 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 := hexCenters(r, tc.radius, tc.spacing) + pts, total := hexCenters(r, tc.radius, tc.spacing, maxFillPlops) if len(pts) == 0 { t.Fatal("no centers") } + // The count is derived up front so an oversized fill is refused without + // building its lattice — which only works if it matches what gets built. + if total != len(pts) { + t.Errorf("reported total %d, built %d", total, len(pts)) + } // A clump may cross the edge, but only by the half-spacing the rule // allows — never enough to be mostly out in the path. @@ -168,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) + pts, _ := hexCenters(tc.r, 15, 10, 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) } From 958b90ebc655df5f312bbc7ed8a8fed0a6819524 Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Tue, 21 Jul 2026 14:03:41 -0400 Subject: [PATCH 6/7] Address fourth round of Gadfly findings on #76 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Split hexCenters' doc: the count/limit contract had run straight on from the #75 anti-regression paragraph with no separator, so its opening "It" read as referring to the wrong thing. - Write the stagger as pitch/2 rather than radius. Same value, but the intent is "half a pitch" and only incidentally "one radius". - fitAxis's step<=0 guard is unreachable from its only caller. Kept, and now says so: a helper this small shouldn't need its caller read to be shown safe, and the failure mode without it is ±Inf into an int conversion. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ --- CLAUDE.md | 7 ++++--- internal/service/ops.go | 19 ++++++++++++++----- 2 files changed, 18 insertions(+), 8 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index ce29833..c263429 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -140,9 +140,10 @@ was the one that contained a real bug. **A skipped Gadfly run reports success.** A comment without the trigger phrase still starts the workflow, which logs `comment does not contain trigger phrase` and exits green in ~2 seconds. So "the pipeline is green" does NOT mean "this -was reviewed". Confirm a re-review actually ran by checking that it posted a new -consensus comment — or that the run took minutes rather than seconds — not by -its status. +was reviewed". Confirm a re-review actually ran by its **duration** — a real +pass takes ~10 minutes, a skip takes 2 seconds. Don't look for a new consensus +comment: Gadfly EDITS its existing status-board and consensus comments in place, +so their `created_at` stays at the first review and only `updated_at` moves. Workflow- and config-only changes (CI, this file, docs) go straight to `main` without the PR dance. diff --git a/internal/service/ops.go b/internal/service/ops.go index 2fa05df..08e4b5f 100644 --- a/internal/service/ops.go +++ b/internal/service/ops.go @@ -214,10 +214,13 @@ type localPoint struct{ x, y float64 } // Do not "simplify" this back to anchoring at the region's min corner. That is // what #75 was: staggered rows start a full pitch in, and the leftover all lands // on the far edge, where clumps hang outside a bed that nothing clips them to. -// It reports the total alongside the points, and works that total out BEFORE -// building anything: a fill large enough to be refused shouldn't allocate its -// whole lattice first just to be counted and thrown away. Over `limit` it -// returns (nil, total) so the caller can refuse with the real number. +// +// # Counting before building +// +// hexCenters returns the total alongside the points, and works that total out +// BEFORE building anything: a fill large enough to be refused shouldn't allocate +// its whole lattice first just to be counted and thrown away. Over `limit` it +// returns (nil, total), so the caller can still refuse with the real number. func hexCenters(r Region, radius, spacing float64, limit int) ([]localPoint, int) { if radius <= 0 { return nil, 0 @@ -259,7 +262,7 @@ func hexCenters(r Region, radius, spacing float64, limit int) ([]localPoint, int // and centering THAT run puts it exactly half a pitch off its neighbours. // A single-column region has nothing to stagger against. if row%2 == 1 && cols > 1 { - n, x = staggered, r.MinX+x0+radius + n, x = staggered, r.MinX+x0+pitch/2 } for i := 0; i < n; i++ { pts = append(pts, localPoint{x + float64(i)*pitch, y}) @@ -276,6 +279,12 @@ func hexCenters(r Region, radius, spacing float64, limit int) ([]localPoint, int // A span too small to hold even one position at that inset still gets one, in the // middle: filling a bed narrower than a single plop with one plop is a better // answer than refusing to plant it. +// +// The step<=0 half of that guard is currently unreachable — hexCenters, the only +// caller, returns early unless radius > 0, which makes both steps it passes +// positive. It stays because dividing by a non-positive step yields ±Inf and then +// a garbage int conversion, and a helper this small should not require reading +// its caller to know it is safe. Deliberate, not an oversight. func fitAxis(length, step, inset float64) (n int, start float64) { if step <= 0 || length < 2*inset { return 1, length / 2 From 07d598cffd6c32eb44ca72f89260635708fdae5b Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Tue, 21 Jul 2026 14:27:58 -0400 Subject: [PATCH 7/7] Address fifth round of Gadfly findings on #76 - fillLoaded's doc listed what it does and omitted the non-finite-region rejection this PR added to it. - Trim the half-spacing rule's restatement in DESIGN.md to the decision and a pointer. The rule, the square-foot arithmetic and the failure mode are written out once, in hexCenters, rather than near-verbatim in four places. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ --- DESIGN.md | 2 +- internal/service/ops.go | 6 +++--- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index cd0a8c3..052426e 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -7,7 +7,7 @@ Work is tracked in Gitea issues; the tracking epic links every piece in dependen ## Decisions - **Placement model:** freeform plops (not a square-foot grid), scaled by real plant spacing. Grid snapping may come later as a toggle. -- **Spacing is a plant-to-plant rule, so bed edges get half of it.** Spacing describes two plants competing for the same soil, light and water; a bed edge is not a competitor, so the outer row owes it only *half* the spacing — the arithmetic behind every square-foot chart (4/square = 6" apart, 3" from the edge). `FillRegion` centres its lattice and lets a plop, which is a *clump* 3 spacings across, cross the edge by up to half a spacing so its outermost plants land at that half-spacing. See `hexCenters`; #75 is what getting this wrong looked like. +- **Spacing is a plant-to-plant rule, so bed edges get half of it.** A bed edge is not a competitor for soil, light or water, so the outer row owes it half the spacing rather than a full one. `FillRegion` centres its lattice accordingly, and lets a plop — a *clump* three spacings across — cross the edge by up to half a spacing so its outermost plants land at that half-spacing. The rule, the square-foot-chart arithmetic behind it, and the failure mode it prevents are written out once in `hexCenters`; #75 is what getting it wrong looked like. - **Stack:** Go 1.26.x backend, module `gitea.stevedudenhoeffer.com/steve/pansy`; React + TypeScript + Vite + Tailwind frontend, production build embedded via `embed.FS` → one static binary (`CGO_ENABLED=0`). - **Users:** multi-user with ownership. Users own gardens; a garden can be shared with other users as viewer (read) or editor (edit content). Owner additionally shares/deletes. - **Auth:** OIDC-first (Authentik is the primary IdP), local argon2id passwords as an optional fallback. diff --git a/internal/service/ops.go b/internal/service/ops.go index 08e4b5f..cc00a13 100644 --- a/internal/service/ops.go +++ b/internal/service/ops.go @@ -114,9 +114,9 @@ func (s *Service) FillRegion(ctx context.Context, actorID, objectID int64, regio } // fillLoaded is the shared body of FillRegion/FillNamedRegion given an object -// already loaded and authorized (roleEditor). It clamps the region to the -// object's bounds, refuses fills over maxFillPlops, and inserts the whole batch -// in one transaction rather than one round-trip per plop. +// already loaded and authorized (roleEditor). It rejects a non-finite region, +// clamps the region to the object's bounds, refuses fills over maxFillPlops, and +// inserts the whole batch in one transaction rather than one round-trip per plop. func (s *Service) fillLoaded(ctx context.Context, actorID int64, o *domain.GardenObject, region Region, plantID int64, spacingOverride *float64) ([]domain.Planting, error) { if !o.Plantable { return nil, domain.ErrInvalidInput