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) }