From 7c1faa151555a77055ea120a8ac4f02d5bc184a0 Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Wed, 22 Jul 2026 00:01:27 -0400 Subject: [PATCH 1/2] Fill mode: a "grid" layout that plants individual plants in rows (#77) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Steve chose option 3: keep clumps as the default primitive, add a grid/rows fill mode, so sketching and planning are different operations with different outputs rather than one model forced to be both. A plop is a CLUMP, not a plant — great for "a few plops of garlic in a corner", useless for drawing a plantable 8-rows-of-garlic bed (that came out as ~15 blobs, #77). FillLayout selects what a fill packs: - clump (default, unchanged): radius 1.5×spacing, ~7 plants per plop. - grid: radius spacing/2, pitch = spacing, ONE plant per plop — rows you could actually plant from. The geometry is the SAME hexCenters lattice and the SAME #75 half-spacing edge rule; only the radius→spacing relationship differs (plopRadiusFor). Grid keeps no 15cm floor — its whole point is true spacing — while clump keeps it so a tiny-spacing plant doesn't make invisible clumps. Threaded through FillRegion/FillNamedRegion (empty layout = clump, so existing callers are unchanged; unknown layout = ErrInvalidInput), the REST /fill endpoint (`layout`), and the agent's fill_region tool (`mode`, enum clump|grid), so "plant the bed in rows" works. Tests: grid produces many more, single-plant plops than clump on the same bed (radius spacing/2, derived count 1); unknown layout is refused at both the service and the API. maxFillPlops still caps a grid fill of a huge bed. No frontend fill affordance exists yet (fill is agent-only in the UI; the fill UI was deferred in #82), so the mode toggle rides along when that's built — noted. Docs: DESIGN placement-model decision. Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ --- DESIGN.md | 1 + internal/agent/runtime_test.go | 2 +- internal/agent/tools.go | 3 +- internal/agent/tools_test.go | 2 +- internal/api/ops.go | 8 +++- internal/api/ops_test.go | 21 +++++++++ internal/service/ops.go | 59 +++++++++++++++++++++--- internal/service/ops_test.go | 73 ++++++++++++++++++++++++++---- internal/service/revisions_test.go | 10 ++-- 9 files changed, 154 insertions(+), 25 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index fec4cd1..51363d3 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. +- **A fill is one of two operations (#77).** A plop is a *clump*, not a plant, which is the right primitive for SKETCHING ("a few plops of garlic in a corner") but can't draw a real planting — a filled bed comes out as ~15 blobs, not 8 rows of garlic. So `FillRegion`/`FillNamedRegion` take a `FillLayout`: `clump` (default; plop radius 1.5×spacing, ~7 plants each — quick coverage) or `grid` (radius spacing/2, pitch = spacing, ONE plant per plop — a layout you could plant from). Same `hexCenters` lattice and #75 edge rule for both; only the radius→spacing relationship differs. Surfaced on `POST /objects/:id/fill` (`layout`) and the agent's `fill_region` (`mode`). A grid-filled bed approaches the low-hundreds-of-plops the SVG budget was sized for, which the semantic-zoom tiers already anticipate. - **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. The first registered user is `is_admin` (set race-free inside the INSERT); admin gates instance-wide Settings (`requireAdmin`), the only thing that reads that flag. diff --git a/internal/agent/runtime_test.go b/internal/agent/runtime_test.go index 16791bd..a25945d 100644 --- a/internal/agent/runtime_test.go +++ b/internal/agent/runtime_test.go @@ -60,7 +60,7 @@ func TestTurnIsOneChangeSet(t *testing.T) { if err != nil { t.Fatalf("bed: %v", err) } - if _, err := svc.FillNamedRegion(ctx, owner, bed.ID, "all", garlic.ID, nil); err != nil { + if _, err := svc.FillNamedRegion(ctx, owner, bed.ID, "all", garlic.ID, nil, service.FillClump); err != nil { t.Fatalf("seed garlic: %v", err) } diff --git a/internal/agent/tools.go b/internal/agent/tools.go index 9d59c7a..021a6d9 100644 --- a/internal/agent/tools.go +++ b/internal/agent/tools.go @@ -127,8 +127,9 @@ func (a *adapter) fillRegion(ctx context.Context, args struct { Region string `json:"region" description:"nw|ne|sw|se corner, north|south|east|west (or top|bottom|left|right) half, or all"` PlantID int64 `json:"plantId" description:"plant to fill with"` SpacingOverride *float64 `json:"spacingOverrideCm" description:"optional in-row spacing override in cm; omit to use the plant's spacing"` + Mode string `json:"mode" enum:"clump,grid" description:"clump (default) drops a few fat clumps for a quick sketch; grid lays out individual plants in rows at true spacing, a layout you could plant from"` }) (any, error) { - return a.svc.FillNamedRegion(ctx, a.actor, args.ObjectID, args.Region, args.PlantID, args.SpacingOverride) + return a.svc.FillNamedRegion(ctx, a.actor, args.ObjectID, args.Region, args.PlantID, args.SpacingOverride, service.FillLayout(args.Mode)) } func (a *adapter) findPlant(ctx context.Context, args struct { diff --git a/internal/agent/tools_test.go b/internal/agent/tools_test.go index d599540..6e06ec8 100644 --- a/internal/agent/tools_test.go +++ b/internal/agent/tools_test.go @@ -169,7 +169,7 @@ func TestGarlicBedToCucumbers(t *testing.T) { if err != nil { t.Fatalf("bed: %v", err) } - if _, err := svc.FillNamedRegion(ctx, owner, bed.ID, "all", garlic.ID, nil); err != nil { + if _, err := svc.FillNamedRegion(ctx, owner, bed.ID, "all", garlic.ID, nil, service.FillClump); err != nil { t.Fatalf("seed the garlic: %v", err) } diff --git a/internal/api/ops.go b/internal/api/ops.go index f212055..e152109 100644 --- a/internal/api/ops.go +++ b/internal/api/ops.go @@ -51,6 +51,10 @@ type objectFillRequest struct { // SpacingOverrideCM plants tighter or looser than the plant's mature spacing // without editing the catalog entry. SpacingOverrideCM *float64 `json:"spacingOverrideCm"` + // Layout is "clump" (default; fat clumps for a quick sketch) or "grid" + // (individual plants in rows at true spacing). Empty = clump. An unknown value + // is refused by the service (#77). + Layout string `json:"layout"` } func (h *handlers) fillObject(c *gin.Context) { @@ -84,9 +88,9 @@ func (h *handlers) fillObject(c *gin.Context) { return } region := service.Region{MinX: rect.MinX, MinY: rect.MinY, MaxX: rect.MaxX, MaxY: rect.MaxY} - created, err = h.svc.FillRegion(c.Request.Context(), actor, id, region, req.PlantID, req.SpacingOverrideCM) + created, err = h.svc.FillRegion(c.Request.Context(), actor, id, region, req.PlantID, req.SpacingOverrideCM, service.FillLayout(req.Layout)) } else { - created, err = h.svc.FillNamedRegion(c.Request.Context(), actor, id, req.Region, req.PlantID, req.SpacingOverrideCM) + created, err = h.svc.FillNamedRegion(c.Request.Context(), actor, id, req.Region, req.PlantID, req.SpacingOverrideCM, service.FillLayout(req.Layout)) } if err != nil { writeServiceError(c, err) diff --git a/internal/api/ops_test.go b/internal/api/ops_test.go index bf317de..013b41a 100644 --- a/internal/api/ops_test.go +++ b/internal/api/ops_test.go @@ -95,6 +95,27 @@ func TestFillAndClearAPI(t *testing.T) { if n := int(decodeMap(t, w.Body.Bytes())["cleared"].(float64)); n != 0 { t.Errorf("second clear removed %d, want 0", n) } + + // Grid layout (#77) packs individual plants at true spacing — many more, small + // plops than the clump default on the same bed. + w = doJSON(t, r, http.MethodPost, fillPath(objID), map[string]any{ + "plantId": plantID, "region": "all", "layout": "grid", + }, cookie) + if w.Code != http.StatusOK { + t.Fatalf("grid fill: status %d, body %s", w.Code, w.Body.String()) + } + if gridN := int(decodeMap(t, w.Body.Bytes())["created"].(float64)); gridN <= created { + t.Errorf("grid fill created %d, want more than the clump fill's %d", gridN, created) + } + // An unknown layout is a 400, not a silent clump fill. + if w := doJSON(t, r, http.MethodPost, clearPath(objID), nil, cookie); w.Code != http.StatusOK { + t.Fatalf("clear before bad-layout: %d", w.Code) + } + if w := doJSON(t, r, http.MethodPost, fillPath(objID), map[string]any{ + "plantId": plantID, "region": "all", "layout": "spiral", + }, cookie); w.Code != http.StatusBadRequest { + t.Errorf("unknown layout = %d, want 400", w.Code) + } } // TestFillRegionSelectionAPI: exactly one of region/rect, and a rect fills only diff --git a/internal/service/ops.go b/internal/service/ops.go index cc00a13..085371d 100644 --- a/internal/service/ops.go +++ b/internal/service/ops.go @@ -97,6 +97,49 @@ func defaultPlopRadius(spacingCM float64) float64 { return math.Max(1.5*spacingCM, 15) } +// FillLayout selects what a fill packs (#77). +// +// A plop is a CLUMP, not a plant, and that abstraction is the right primitive for +// SKETCHING — "a few plops of garlic in one corner" — but it can't draw a real +// planting: a filled 4×8ft bed comes out as ~15 blobs, not 8 rows of garlic. So +// filling is now two operations. FillClump (the default, unchanged) drops fat +// clumps for quick coverage; FillGrid lays out individual plants at true spacing, +// producing a layout you could actually plant from. +type FillLayout string + +const ( + // FillClump packs fat clumps (radius 1.5×spacing). Each plop is ~7 plants. + FillClump FillLayout = "clump" + // FillGrid packs one plant per plop at true spacing (radius spacing/2, pitch + // = spacing). A bed becomes rows of individual plants. + FillGrid FillLayout = "grid" +) + +// plopRadiusFor is the plop radius a fill uses, given the plant's spacing and the +// layout. Grid mode is a plain spacing/2 (so the pitch is one spacing and each +// plop's derived count is 1); clump mode keeps the 15cm floor that stops a +// tiny-spacing plant from making invisibly small clumps — a floor grid mode +// doesn't want, since its whole point is true spacing. +func plopRadiusFor(spacingCM float64, layout FillLayout) float64 { + if layout == FillGrid { + return spacingCM / 2 + } + return defaultPlopRadius(spacingCM) +} + +// validFillLayout normalizes a layout: empty defaults to clump (so existing +// callers are unchanged), a known value passes, anything else is rejected. +func validFillLayout(l FillLayout) (FillLayout, bool) { + switch l { + case "", FillClump: + return FillClump, true + case FillGrid: + return FillGrid, true + default: + return "", false + } +} + // 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× @@ -105,22 +148,26 @@ func defaultPlopRadius(spacingCM float64) float64 { // 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) { +func (s *Service) FillRegion(ctx context.Context, actorID, objectID int64, region Region, plantID int64, spacingOverride *float64, layout FillLayout) ([]domain.Planting, error) { o, _, err := s.objectForRole(ctx, actorID, objectID, roleEditor) if err != nil { return nil, err } - return s.fillLoaded(ctx, actorID, o, region, plantID, spacingOverride) + return s.fillLoaded(ctx, actorID, o, region, plantID, spacingOverride, layout) } // fillLoaded is the shared body of FillRegion/FillNamedRegion given an object // 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) { +func (s *Service) fillLoaded(ctx context.Context, actorID int64, o *domain.GardenObject, region Region, plantID int64, spacingOverride *float64, layout FillLayout) ([]domain.Planting, error) { if !o.Plantable { return nil, domain.ErrInvalidInput } + layout, ok := validFillLayout(layout) + if !ok { + return nil, domain.ErrInvalidInput + } plant, err := s.visiblePlant(ctx, actorID, plantID) if err != nil { return nil, err @@ -132,7 +179,7 @@ func (s *Service) fillLoaded(ctx context.Context, actorID int64, o *domain.Garde } spacing = *spacingOverride } - radius := defaultPlopRadius(spacing) + radius := plopRadiusFor(spacing, layout) if !isFinite(radius) || radius <= 0 { return nil, domain.ErrInvalidInput } @@ -311,7 +358,7 @@ func coveredByExisting(x, y, radius float64, existing []domain.Planting) bool { // FillNamedRegion is FillRegion addressed by a compass name ("ne", "south half") // instead of a resolved Region — the ergonomic form for agent tools, which don't // hold the object's geometry. It resolves the name against the object, then fills. -func (s *Service) FillNamedRegion(ctx context.Context, actorID, objectID int64, regionName string, plantID int64, spacingOverride *float64) ([]domain.Planting, error) { +func (s *Service) FillNamedRegion(ctx context.Context, actorID, objectID int64, regionName string, plantID int64, spacingOverride *float64, layout FillLayout) ([]domain.Planting, error) { o, _, err := s.objectForRole(ctx, actorID, objectID, roleEditor) if err != nil { return nil, err @@ -320,7 +367,7 @@ func (s *Service) FillNamedRegion(ctx context.Context, actorID, objectID int64, if err != nil { return nil, err } - return s.fillLoaded(ctx, actorID, o, region, plantID, spacingOverride) + return s.fillLoaded(ctx, actorID, o, region, plantID, spacingOverride, layout) } // ClearObject soft-removes every active plop in an object the actor can edit (one diff --git a/internal/service/ops_test.go b/internal/service/ops_test.go index 1a003a1..f18cc36 100644 --- a/internal/service/ops_test.go +++ b/internal/service/ops_test.go @@ -59,7 +59,7 @@ func TestFillRegionCappedForHugeArea(t *testing.T) { bed := seedFillBed(t, s, owner, g.ID, 6000, 6000) // ~46k lattice points at radius 15 → over the cap plant := seedOwnPlant(t, s, owner, 10) region, _ := NamedRegion(bed, "all") - if _, err := s.FillRegion(ctx, owner, bed.ID, region, plant.ID, nil); !errors.Is(err, domain.ErrInvalidInput) { + if _, err := s.FillRegion(ctx, owner, bed.ID, region, plant.ID, nil, FillClump); !errors.Is(err, domain.ErrInvalidInput) { t.Errorf("oversized fill err = %v, want ErrInvalidInput (over maxFillPlops)", err) } } @@ -199,7 +199,7 @@ func TestFillRegionRejectsNonFiniteRegion(t *testing.T) { {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) + created, err := s.FillRegion(ctx, owner, bed.ID, r, plant.ID, nil, FillClump) if !errors.Is(err, domain.ErrInvalidInput) { t.Errorf("FillRegion(%+v) err = %v, want ErrInvalidInput", r, err) } @@ -223,7 +223,7 @@ func TestFillRegionOutsideObjectPlantsNothing(t *testing.T) { 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) + created, err := s.FillRegion(ctx, owner, bed.ID, rect(500, -50, 600, 50), plant.ID, nil, FillClump) if err != nil { t.Fatalf("FillRegion: %v", err) } @@ -256,7 +256,7 @@ func TestFillRegionDeterministicPacking(t *testing.T) { plant := seedOwnPlant(t, s, owner, 10) // radius = max(15,15) = 15 region, _ := NamedRegion(bed, "all") - created, err := s.FillRegion(ctx, owner, bed.ID, region, plant.ID, nil) + created, err := s.FillRegion(ctx, owner, bed.ID, region, plant.ID, nil, FillClump) if err != nil { t.Fatalf("FillRegion: %v", err) } @@ -283,7 +283,7 @@ func TestFillRegionDeterministicPacking(t *testing.T) { // Re-filling the same region skips everything (each candidate sits exactly on // an existing plop → entirely inside it). - again, err := s.FillRegion(ctx, owner, bed.ID, region, plant.ID, nil) + again, err := s.FillRegion(ctx, owner, bed.ID, region, plant.ID, nil, FillClump) if err != nil { t.Fatalf("second FillRegion: %v", err) } @@ -292,6 +292,61 @@ func TestFillRegionDeterministicPacking(t *testing.T) { } } +// TestFillGridLaysOutIndividualPlants is the #77 grid mode: a grid fill packs one +// plant per plop at true spacing, so a bed becomes rows of plants rather than a +// few fat clumps. On the same bed it produces many more, smaller plops, each a +// single plant. +func TestFillGridLaysOutIndividualPlants(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, 60, 60) + plant := seedOwnPlant(t, s, owner, 10) // spacing 10 + + region, _ := NamedRegion(bed, "all") + clump, err := s.FillRegion(ctx, owner, bed.ID, region, plant.ID, nil, FillClump) + if err != nil { + t.Fatalf("clump: %v", err) + } + if _, err := s.ClearObject(ctx, owner, bed.ID); err != nil { + t.Fatalf("clear: %v", err) + } + grid, err := s.FillRegion(ctx, owner, bed.ID, region, plant.ID, nil, FillGrid) + if err != nil { + t.Fatalf("grid: %v", err) + } + + // Grid packs at spacing 10 (radius 5, pitch 10); clump at radius 15 (pitch 30). + // Grid must produce many more plops. + if len(grid) <= len(clump) { + 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. + for _, p := range grid { + if p.RadiusCM != 5 { + t.Errorf("grid plop radius = %v, want 5 (spacing/2)", p.RadiusCM) + } + if p.DerivedCount != 1 { + t.Errorf("grid plop derived count = %d, want 1 (one plant per plop)", p.DerivedCount) + } + } +} + +// TestFillRejectsUnknownLayout: a layout that isn't clump/grid is ErrInvalidInput. +func TestFillRejectsUnknownLayout(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, 60, 60) + plant := seedOwnPlant(t, s, owner, 10) + region, _ := NamedRegion(bed, "all") + if _, err := s.FillRegion(ctx, owner, bed.ID, region, plant.ID, nil, FillLayout("spiral")); !errors.Is(err, domain.ErrInvalidInput) { + t.Errorf("unknown layout err = %v, want ErrInvalidInput", err) + } +} + func TestFillRegionRotatedBedUsesLocalFrame(t *testing.T) { ctx := context.Background() s := newTestService(t, openConfig()) @@ -304,7 +359,7 @@ func TestFillRegionRotatedBedUsesLocalFrame(t *testing.T) { plant := seedOwnPlant(t, s, owner, 20) region, _ := NamedRegion(bed, "ne") - created, err := s.FillRegion(ctx, owner, bed.ID, region, plant.ID, nil) + created, err := s.FillRegion(ctx, owner, bed.ID, region, plant.ID, nil, FillClump) if err != nil { t.Fatalf("FillRegion: %v", err) } @@ -327,7 +382,7 @@ func TestClearObject(t *testing.T) { bed := seedBed(t, s, owner, g.ID) plant := seedOwnPlant(t, s, owner, 10) region, _ := NamedRegion(bed, "all") - if _, err := s.FillRegion(ctx, owner, bed.ID, region, plant.ID, nil); err != nil { + if _, err := s.FillRegion(ctx, owner, bed.ID, region, plant.ID, nil, FillClump); err != nil { t.Fatalf("fill: %v", err) } @@ -361,7 +416,7 @@ func TestOpsForbiddenForViewer(t *testing.T) { } region, _ := NamedRegion(bed, "all") - if _, err := s.FillRegion(ctx, viewer, bed.ID, region, plant.ID, nil); !errors.Is(err, domain.ErrForbidden) { + if _, err := s.FillRegion(ctx, viewer, bed.ID, region, plant.ID, nil, FillClump); !errors.Is(err, domain.ErrForbidden) { t.Errorf("viewer fill = %v, want ErrForbidden", err) } if _, err := s.ClearObject(ctx, viewer, bed.ID); !errors.Is(err, domain.ErrForbidden) { @@ -391,7 +446,7 @@ func TestFillScenario(t *testing.T) { if err != nil { t.Fatalf("region %q: %v", name, err) } - if _, err := s.FillRegion(ctx, owner, bed.ID, region, plantID, nil); err != nil { + if _, err := s.FillRegion(ctx, owner, bed.ID, region, plantID, nil, FillClump); err != nil { t.Fatalf("fill %q: %v", name, err) } } diff --git a/internal/service/revisions_test.go b/internal/service/revisions_test.go index f83c008..ecc500c 100644 --- a/internal/service/revisions_test.go +++ b/internal/service/revisions_test.go @@ -73,7 +73,7 @@ func TestFillRegionIsOneChangeSet(t *testing.T) { plant := seedOwnPlant(t, s, owner, 15) ctx := context.Background() - created, err := s.FillNamedRegion(ctx, owner, bed.ID, "all", plant.ID, nil) + created, err := s.FillNamedRegion(ctx, owner, bed.ID, "all", plant.ID, nil, FillClump) if err != nil { t.Fatalf("FillNamedRegion: %v", err) } @@ -320,7 +320,7 @@ func TestRevertClearObject(t *testing.T) { plant := seedOwnPlant(t, s, owner, 15) ctx := context.Background() - if _, err := s.FillNamedRegion(ctx, owner, bed.ID, "all", plant.ID, nil); err != nil { + if _, err := s.FillNamedRegion(ctx, owner, bed.ID, "all", plant.ID, nil, FillClump); err != nil { t.Fatalf("fill: %v", err) } before, _ := s.store.ListActivePlantingsForObject(ctx, bed.ID) @@ -688,7 +688,7 @@ func TestClearObjectOnlyClearsWhatItSnapshotted(t *testing.T) { plant := seedOwnPlant(t, s, owner, 15) ctx := context.Background() - if _, err := s.FillNamedRegion(ctx, owner, bed.ID, "all", plant.ID, nil); err != nil { + if _, err := s.FillNamedRegion(ctx, owner, bed.ID, "all", plant.ID, nil, FillClump); err != nil { t.Fatalf("fill: %v", err) } before, _ := s.store.ListActivePlantingsForObject(ctx, bed.ID) @@ -725,7 +725,7 @@ func TestRevertResultCarriesItsCounts(t *testing.T) { plant := seedOwnPlant(t, s, owner, 15) ctx := context.Background() - if _, err := s.FillNamedRegion(ctx, owner, bed.ID, "all", plant.ID, nil); err != nil { + if _, err := s.FillNamedRegion(ctx, owner, bed.ID, "all", plant.ID, nil, FillClump); err != nil { t.Fatalf("fill: %v", err) } // A second, different kind of change, so the breakdown has more than one row @@ -830,7 +830,7 @@ func TestSucceededTurnRecordsEvenIfTheCallerWentAway(t *testing.T) { cs, err := s.WithChangeSet(ctx, owner, g.ID, ChangeSetOptions{ Source: domain.SourceAgent, Summary: "plant beans in the second bed", }, func(ctx context.Context) error { - if _, err := s.FillNamedRegion(ctx, owner, bed.ID, "all", plant.ID, nil); err != nil { + if _, err := s.FillNamedRegion(ctx, owner, bed.ID, "all", plant.ID, nil, FillClump); err != nil { return err } cancel() // the client disconnects, mid-turn, after the work landed From c80cf15bf18a57dc5102c3c535a752dd36079e9b Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Wed, 22 Jul 2026 00:21:47 -0400 Subject: [PATCH 2/2] Address fill-mode review: grid edge inset + drop dead coverage append MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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) Claude-Session: https://claude.ai/code/session_01H3zbym8Doka2d7D48maSgZ --- DESIGN.md | 4 +- internal/api/ops_test.go | 31 +++++++++--- internal/service/ops.go | 94 +++++++++++++++++++++--------------- internal/service/ops_test.go | 13 ++++- 4 files changed, 93 insertions(+), 49 deletions(-) diff --git a/DESIGN.md b/DESIGN.md index 51363d3..4043566 100644 --- a/DESIGN.md +++ b/DESIGN.md @@ -7,8 +7,8 @@ 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. -- **A fill is one of two operations (#77).** A plop is a *clump*, not a plant, which is the right primitive for SKETCHING ("a few plops of garlic in a corner") but can't draw a real planting — a filled bed comes out as ~15 blobs, not 8 rows of garlic. So `FillRegion`/`FillNamedRegion` take a `FillLayout`: `clump` (default; plop radius 1.5×spacing, ~7 plants each — quick coverage) or `grid` (radius spacing/2, pitch = spacing, ONE plant per plop — a layout you could plant from). Same `hexCenters` lattice and #75 edge rule for both; only the radius→spacing relationship differs. Surfaced on `POST /objects/:id/fill` (`layout`) and the agent's `fill_region` (`mode`). A grid-filled bed approaches the low-hundreds-of-plops the SVG budget was sized for, which the semantic-zoom tiers already anticipate. -- **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. +- **A fill is one of two operations (#77).** A plop is a *clump*, not a plant, which is the right primitive for SKETCHING ("a few plops of garlic in a corner") but can't draw a real planting — a filled bed comes out as ~15 blobs, not 8 rows of garlic. So `FillRegion`/`FillNamedRegion` take a `FillLayout`: `clump` (default; plop radius 1.5×spacing, ~7 plants each — quick coverage) or `grid` (radius spacing/2, pitch = spacing, ONE plant per plop — a layout you could plant from). Same `hexCenters` lattice and #75 edge rule for both; only the radius→spacing relationship differs. Surfaced on `POST /objects/:id/fill` (`layout`) and the agent's `fill_region` (`mode`). Same centered `hexCenters` lattice for both, but BOTH the plop radius (`plopRadiusFor`) and the edge inset (`edgeInset`) differ by layout: a grid plant sits at the plop's centre, so it insets a half-spacing; a clump's plants reach its rim, so it insets radius-less-a-half and overhangs the edge by that half — reusing the clump formula for grid would inset by zero and plant flush on the edge. A grid-filled bed approaches the low-hundreds-of-plops the SVG budget was sized for, which the semantic-zoom tiers already anticipate. +- **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 how it differs by layout are written out once in `edgeInset` (which `hexCenters` then honours); #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. The first registered user is `is_admin` (set race-free inside the INSERT); admin gates instance-wide Settings (`requireAdmin`), the only thing that reads that flag. - **Auth:** OIDC-first (Authentik is the primary IdP), local argon2id passwords as an optional fallback. diff --git a/internal/api/ops_test.go b/internal/api/ops_test.go index 013b41a..7a091ed 100644 --- a/internal/api/ops_test.go +++ b/internal/api/ops_test.go @@ -95,22 +95,39 @@ func TestFillAndClearAPI(t *testing.T) { if n := int(decodeMap(t, w.Body.Bytes())["cleared"].(float64)); n != 0 { t.Errorf("second clear removed %d, want 0", n) } +} - // Grid layout (#77) packs individual plants at true spacing — many more, small - // plops than the clump default on the same bed. +// TestFillLayoutAPI covers the layout selector (#77): grid packs denser than the +// clump default, and an unknown layout is a 400 rather than a silent clump fill. +func TestFillLayoutAPI(t *testing.T) { + r := authEngine(t, localCfg()) + cookie := registerAndCookie(t, r, "layout@example.com") + _, objID, plantID := seedFillableBed(t, r, cookie, 200, 200, 20) + + // Clump (default) for the baseline count. + w := doJSON(t, r, http.MethodPost, fillPath(objID), map[string]any{ + "plantId": plantID, "region": "all", + }, cookie) + if w.Code != http.StatusOK { + t.Fatalf("clump fill: status %d, body %s", w.Code, w.Body.String()) + } + clumpN := int(decodeMap(t, w.Body.Bytes())["created"].(float64)) + if w := doJSON(t, r, http.MethodPost, clearPath(objID), nil, cookie); w.Code != http.StatusOK { + t.Fatalf("clear between fills: %d", w.Code) + } + + // Grid packs individual plants at true spacing — many more, small plops. w = doJSON(t, r, http.MethodPost, fillPath(objID), map[string]any{ "plantId": plantID, "region": "all", "layout": "grid", }, cookie) if w.Code != http.StatusOK { t.Fatalf("grid fill: status %d, body %s", w.Code, w.Body.String()) } - if gridN := int(decodeMap(t, w.Body.Bytes())["created"].(float64)); gridN <= created { - t.Errorf("grid fill created %d, want more than the clump fill's %d", gridN, created) + if gridN := int(decodeMap(t, w.Body.Bytes())["created"].(float64)); gridN <= clumpN { + t.Errorf("grid fill created %d, want more than the clump fill's %d", gridN, clumpN) } + // An unknown layout is a 400, not a silent clump fill. - if w := doJSON(t, r, http.MethodPost, clearPath(objID), nil, cookie); w.Code != http.StatusOK { - t.Fatalf("clear before bad-layout: %d", w.Code) - } if w := doJSON(t, r, http.MethodPost, fillPath(objID), map[string]any{ "plantId": plantID, "region": "all", "layout": "spiral", }, cookie); w.Code != http.StatusBadRequest { diff --git a/internal/service/ops.go b/internal/service/ops.go index 085371d..ec6e764 100644 --- a/internal/service/ops.go +++ b/internal/service/ops.go @@ -127,6 +127,33 @@ func plopRadiusFor(spacingCM float64, layout FillLayout) float64 { return defaultPlopRadius(spacingCM) } +// edgeInset is how far a plop's CENTRE must stay inside the region edge. It +// differs by layout because the half-spacing rule is about where the PLANT lands, +// and the plant sits in a different place within the plop. +// +// Spacing is a constraint between neighbouring plants competing for the same soil, +// light and water; a bed edge is nobody's neighbour, so the outer plant owes it +// only HALF the spacing — the half it would otherwise share. That is the +// square-foot-chart arithmetic: garlic at 9-per-square sits 2" from the frame, not +// 6". +// +// - Grid: one plant, at the plop's centre. Put that centre a half-spacing in and +// the outer row lands exactly where the rule wants it — inset = spacing/2. +// - Clump: a fat plop (radius 1.5×spacing) whose plants fill out to its RIM. +// Insetting the whole circle would push the outer row a full 1.5 spacings in, +// three times the rule. Instead the clump may hang over by a half-spacing (rim +// at spacing/2 past the edge), landing its outermost plants that same +// half-spacing in — inset = radius − spacing/2. A grid plop reusing THAT +// formula would inset by radius − spacing/2 = 0 and plant flush on the edge, +// which is the bug this split fixes. +func edgeInset(radius, spacing float64, layout FillLayout) float64 { + half := math.Max(0, spacing) / 2 + if layout == FillGrid { + return half + } + return math.Max(0, radius-half) +} + // validFillLayout normalizes a layout: empty defaults to clump (so existing // callers are unchanged), a known value passes, anything else is rejected. func validFillLayout(l FillLayout) (FillLayout, bool) { @@ -140,14 +167,15 @@ func validFillLayout(l FillLayout) (FillLayout, bool) { } } -// 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, 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. +// FillRegion lays a field of plops of one plant across a region of a plantable +// object the actor can edit. The layout picks the primitive: FillClump drops fat +// clumps for quick sketching, FillGrid lays out individual plants at true spacing +// (see FillLayout). Plop radius comes from the plant's spacing (or spacingOverride) +// via plopRadiusFor; centers sit on a centered hex lattice at 2×radius pitch, set +// in from each edge by edgeInset — a half-spacing for grid, radius-less-a-half- +// spacing for a clump (see edgeInset for the why). 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, layout FillLayout) ([]domain.Planting, error) { o, _, err := s.objectForRole(ctx, actorID, objectID, roleEditor) if err != nil { @@ -157,9 +185,10 @@ 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 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. +// already loaded and authorized (roleEditor). It validates the layout, 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, layout FillLayout) ([]domain.Planting, error) { if !o.Plantable { return nil, domain.ErrInvalidInput @@ -197,7 +226,7 @@ func (s *Service) fillLoaded(ctx context.Context, actorID int64, o *domain.Garde } region = region.clampTo(o.WidthCM/2, o.HeightCM/2) - centers, total := hexCenters(region, radius, spacing, maxFillPlops) + centers, total := hexCenters(region, radius, edgeInset(radius, spacing, layout), maxFillPlops) if total > maxFillPlops { return nil, domain.ErrInvalidInput // region too large for this spacing; ask for less } @@ -208,13 +237,17 @@ func (s *Service) fillLoaded(ctx context.Context, actorID int64, o *domain.Garde } today := s.now().UTC().Format(dateLayout) batch := make([]*domain.Planting, 0, len(centers)) + // Only the plops that were ALREADY here can cover a candidate: every plop this + // fill makes shares one radius and sits on a distinct lattice point, and a plop + // is "covered" only when it lies entirely inside another — impossible between + // two equal-radius circles at different centres. So skip against `existing` as + // loaded and don't grow it per plop, which made an empty-bed grid fill's check + // needlessly quadratic. for _, c := range centers { if coveredByExisting(c.x, c.y, radius, existing) { continue } - p := &domain.Planting{ObjectID: o.ID, PlantID: plantID, XCM: c.x, YCM: c.y, RadiusCM: radius, PlantedAt: &today} - batch = append(batch, p) - existing = append(existing, *p) // so later candidates in THIS fill don't stack on it + batch = append(batch, &domain.Planting{ObjectID: o.ID, PlantID: plantID, XCM: c.x, YCM: c.y, RadiusCM: radius, PlantedAt: &today}) } created, err := s.store.CreatePlantings(ctx, batch) if err != nil { @@ -243,24 +276,14 @@ type localPoint struct{ x, y float64 } // // # 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". +// The caller passes `inset`: the margin the outer row keeps from every edge. It +// encodes the half-spacing rule (spacing is owed between neighbouring plants, and +// a bed edge is nobody's neighbour) and differs by layout — see edgeInset, which +// derives it. hexCenters just honours it on all four sides. // -// 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. +// Do not "simplify" the centering 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 // @@ -268,7 +291,7 @@ type localPoint struct{ x, y float64 } // 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) { +func hexCenters(r Region, radius, inset float64, limit int) ([]localPoint, int) { if radius <= 0 { return nil, 0 } @@ -281,11 +304,6 @@ func hexCenters(r Region, radius, spacing float64, limit int) ([]localPoint, int pitch := 2 * radius rowH := pitch * math.Sqrt(3) / 2 - // 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) diff --git a/internal/service/ops_test.go b/internal/service/ops_test.go index f18cc36..ba2810a 100644 --- a/internal/service/ops_test.go +++ b/internal/service/ops_test.go @@ -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) } }