diff --git a/CLAUDE.md b/CLAUDE.md index 58cab74..c263429 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 @@ -123,6 +129,22 @@ 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 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/DESIGN.md b/DESIGN.md index f217298..052426e 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.** 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 90be1d4..cc00a13 100644 --- a/internal/service/ops.go +++ b/internal/service/ops.go @@ -28,13 +28,9 @@ 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. +// 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), @@ -42,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} @@ -94,9 +100,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, 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, 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 { @@ -106,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 @@ -129,9 +137,21 @@ 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) - 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 } @@ -169,32 +189,112 @@ 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. +// +// 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. +// +// # 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 + return nil, 0 + } + // 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 - 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 - } - for x := xStart; x <= r.MaxX+eps; x += pitch { - if r.contains(x, y) { - pts = append(pts, localPoint{x, y}) - } - } - row++ + + // 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) + + // 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 } - return pts + 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 + // 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 = staggered, r.MinX+x0+pitch/2 + } + for i := 0; i < n; i++ { + pts = append(pts, localPoint{x + float64(i)*pitch, y}) + } + } + return pts, total +} + +// 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. +// +// 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 + } + // 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 diff --git a/internal/service/ops_test.go b/internal/service/ops_test.go index b078188..1a003a1 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,165 @@ 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, 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. + 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 > budget+1e-6 { + t.Errorf("plop at (%.1f,%.1f) overhangs by %.2f, budget %.2f", p.x, p.y, over, budget) + } + } + + // 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. +// +// 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) { + 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, 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) + } + }) + } +} + +// 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) + } + } + } +} + +// 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() @@ -99,16 +260,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) } }