Fill: honour the half-spacing edge rule when packing plops (#75)

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) <[email protected]> Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ
2026-07-21 10:11:08 -04:00
parent c1c873d3f0
commit 45da4b15e2
4 changed files with 182 additions and 31 deletions
@@ -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
@@ -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.
@@ -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 {
@@ -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)
 		}
 	}