From af34ced208952b615f45b7abfb0472a34dbe14ba Mon Sep 17 00:00:00 2001 From: Steve Dudenhoeffer Date: Sat, 18 Jul 2026 20:16:53 -0400 Subject: [PATCH] Address Gadfly review on #9: memoize shapes, fades, unique ids, robustness MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes from the PR #29 adversarial review (considered; not graded). Performance / correctness - ObjectShape is memo'd, so a pan/zoom (which only mutates the world transform) no longer re-renders every object. - 'circle' objects render as an (rx/ry), honoring both dims — a circle when width == height — instead of ignoring heightCm. - The 1 m grid now actually fades in (opacity ramps as cells grow past the visibility threshold), matching its description. - Unique SVG pattern id (useId) so multiple canvases can't collide. - The canvas re-fits when the garden id changes (not just once), so #11 switching gardens reframes. Robustness - geometry.zoomToFitRect takes rect size by magnitude; wheel/pinch ignore non-finite deltas; ObjectShape clamps dimensions to >= 0 (no invalid SVG). Maintainability - Renamed the Size params from `viewport` to `canvasSize` (4 models); moved easeInOutCubic/lerp into geometry.ts; renamed toLocal → clientToCanvas; named constants for the label/corner factors; DEFAULT_FILL const; EditorGarden.unitPref imports UnitPref; the Fit control uses the shared Button. focusedObjectId/plantable are intentionally reserved for #11/#15. tsc + 17 vitest tests green; re-verified in a browser (ellipses render, unique pattern id, wheel zoom + Fit). Co-Authored-By: Claude Opus 4.8 (1M context) Claude-Session: https://claude.ai/code/session_01JdQpdYYsTgtkJBxbcpAszi --- web/src/editor/GardenCanvas.tsx | 54 +++++++++++++++------------- web/src/editor/ObjectShape.tsx | 62 +++++++++++++++++++++------------ web/src/editor/types.ts | 4 ++- web/src/editor/useViewport.ts | 26 +++++++++----- web/src/lib/geometry.ts | 29 ++++++++++----- 5 files changed, 109 insertions(+), 66 deletions(-) diff --git a/web/src/editor/GardenCanvas.tsx b/web/src/editor/GardenCanvas.tsx index 83fa8bd..204e160 100644 --- a/web/src/editor/GardenCanvas.tsx +++ b/web/src/editor/GardenCanvas.tsx @@ -1,4 +1,5 @@ -import { useEffect, useMemo, useRef, useState } from 'react' +import { useEffect, useId, useMemo, useRef, useState } from 'react' +import { Button } from '@/components/ui/Button' import type { Size } from '@/lib/geometry' import { ObjectShape } from './ObjectShape' import { useEditorStore } from './store' @@ -6,19 +7,21 @@ import { useViewport } from './useViewport' import type { EditorGarden, EditorObject } from './types' const GRID_CM = 100 // 1 m grid -const GRID_MIN_CELL_PX = 6 // hide the grid when cells get this small (zoomed out) +const GRID_MIN_CELL_PX = 6 // grid is invisible below this cell size (zoomed out) +const GRID_MAX_OPACITY = 0.18 /** * The editor canvas: a full-size SVG with a single world that pan/zoom/pinch - * transforms. Renders a fading 1 m grid, the garden boundary, and its objects. - * Interactions beyond viewport + basic select (move/resize/rotate, palette) land - * in #11; this is the foundation, driven by mock data until then. + * transforms. Renders a 1 m grid that fades in as you zoom, the garden boundary, + * and its objects. Interactions beyond viewport + basic select (move/resize/ + * rotate, palette) land in #11; this is the foundation, driven by mock data. */ export function GardenCanvas({ garden, objects }: { garden: EditorGarden; objects: EditorObject[] }) { const svgRef = useRef(null) const containerRef = useRef(null) const [size, setSize] = useState({ w: 0, h: 0 }) - const fittedRef = useRef(false) + const fittedForRef = useRef(null) + const gridId = 'garden-grid-' + useId().replace(/:/g, '') const viewport = useEditorStore((s) => s.viewport) const selectedId = useEditorStore((s) => s.selectedId) @@ -41,15 +44,20 @@ export function GardenCanvas({ garden, objects }: { garden: EditorGarden; object return () => ro.disconnect() }, []) - // Frame the garden the first time we know the canvas size. + // Frame the garden once the canvas size is known, and again if we switch to a + // different garden. useEffect(() => { - if (!fittedRef.current && size.w > 0 && size.h > 0) { - fittedRef.current = true + const usable = size.w > 0 && size.h > 0 && garden.widthCm > 0 && garden.heightCm > 0 + if (usable && fittedForRef.current !== garden.id) { + fittedForRef.current = garden.id fitToRect(gardenRect, size) } - }, [size, gardenRect, fitToRect]) + }, [size, garden.id, garden.widthCm, garden.heightCm, gardenRect, fitToRect]) + + // Grid fades in as cells grow from GRID_MIN_CELL_PX to 2× that. + const cellPx = GRID_CM * viewport.scale + const gridOpacity = Math.max(0, Math.min(GRID_MAX_OPACITY, ((cellPx - GRID_MIN_CELL_PX) / GRID_MIN_CELL_PX) * GRID_MAX_OPACITY)) - const showGrid = GRID_CM * viewport.scale >= GRID_MIN_CELL_PX const ordered = useMemo(() => [...objects].sort((a, b) => a.zIndex - b.zIndex), [objects]) return ( @@ -63,21 +71,21 @@ export function GardenCanvas({ garden, objects }: { garden: EditorGarden; object onPointerDown={() => select(null)} > - {showGrid && ( + {gridOpacity > 0.005 && ( <> - + - + )} @@ -104,15 +112,13 @@ export function GardenCanvas({ garden, objects }: { garden: EditorGarden; object
{viewport.scale.toFixed(2)} px/cm
-
- -
+ ) } diff --git a/web/src/editor/ObjectShape.tsx b/web/src/editor/ObjectShape.tsx index 0f2a6e3..6014772 100644 --- a/web/src/editor/ObjectShape.tsx +++ b/web/src/editor/ObjectShape.tsx @@ -1,6 +1,8 @@ -import type { PointerEvent } from 'react' +import { memo, type PointerEvent } from 'react' import type { EditorObject } from './types' +const DEFAULT_FILL = '#8a8a8a' + // Default fills by kind (overridable per object via object.color). Muted, earthy // tones so plops (added in #15) read clearly on top. const kindColors: Record = { @@ -10,21 +12,29 @@ const kindColors: Record = { in_ground: '#7a6a4a', tree: '#4f7a4f', path: '#b8b0a0', - structure: '#8a8a8a', + structure: DEFAULT_FILL, } +// Label font size = this fraction of the object's smaller side, clamped to a +// readable cm range. Corner radius = this fraction of the smaller half-side. +const LABEL_FONT_FACTOR = 0.28 +const LABEL_FONT_MIN_CM = 8 +const LABEL_FONT_MAX_CM = 40 +const CORNER_RADIUS_FACTOR = 0.06 + function fillFor(o: EditorObject): string { - return o.color ?? kindColors[o.kind] ?? '#8a8a8a' + return o.color ?? kindColors[o.kind] ?? DEFAULT_FILL } /** - * One garden object in world (cm) space: a centered rect or circle, rotated - * about its center, with the object's name. The parent world applies the - * viewport scale, so geometry is authored in cm; strokes use - * vector-effect=non-scaling-stroke to stay a constant pixel width at any zoom. - * Full move/resize/rotate interactions come in #11. + * One garden object in world (cm) space: a centered rect or ellipse (a circle + * when width == height), rotated about its center, with the object's name. The + * parent world applies the viewport scale, so geometry is authored in cm and + * strokes use vector-effect=non-scaling-stroke to stay a constant pixel width at + * any zoom. memo'd so a pan/zoom (which only changes the world transform) + * doesn't re-render every object. Full move/resize/rotate come in #11. */ -export function ObjectShape({ +export const ObjectShape = memo(function ObjectShape({ object, selected, onSelect, @@ -34,16 +44,21 @@ export function ObjectShape({ onSelect: (id: number) => void }) { const fill = fillFor(object) - const halfW = object.widthCm / 2 - const halfH = object.heightCm / 2 - // Label size scales with the object but is clamped to a sensible cm range. - const fontCm = Math.max(8, Math.min(40, Math.min(object.widthCm, object.heightCm) * 0.28)) + const halfW = Math.max(0, object.widthCm / 2) + const halfH = Math.max(0, object.heightCm / 2) + const fontCm = Math.max( + LABEL_FONT_MIN_CM, + Math.min(LABEL_FONT_MAX_CM, Math.min(object.widthCm, object.heightCm) * LABEL_FONT_FACTOR), + ) function handleDown(e: PointerEvent) { e.stopPropagation() // don't let the canvas treat this as an empty-space pan/deselect onSelect(object.id) } + const stroke = selected ? '#2f7a3e' : '#00000033' + const strokeWidth = selected ? 2 : 1 + return ( {object.shape === 'circle' ? ( - ) : ( )} @@ -91,4 +107,4 @@ export function ObjectShape({ )} ) -} +}) diff --git a/web/src/editor/types.ts b/web/src/editor/types.ts index 4c02471..e711f8d 100644 --- a/web/src/editor/types.ts +++ b/web/src/editor/types.ts @@ -1,6 +1,8 @@ // The object shape the canvas renders. A subset of the server's garden object // (#10); #11 maps /full's objects onto this. Positioned by center in garden cm. +import type { UnitPref } from '@/lib/units' + export type ObjectShapeKind = 'rect' | 'circle' export interface EditorObject { @@ -24,5 +26,5 @@ export interface EditorGarden { name: string widthCm: number heightCm: number - unitPref: 'metric' | 'imperial' + unitPref: UnitPref } diff --git a/web/src/editor/useViewport.ts b/web/src/editor/useViewport.ts index 31234d5..0d30ca5 100644 --- a/web/src/editor/useViewport.ts +++ b/web/src/editor/useViewport.ts @@ -1,15 +1,20 @@ import { useCallback, useEffect, useRef, type RefObject } from 'react' import { useGesture } from '@use-gesture/react' -import { zoomToFitRect, zoomViewportAt, type Point, type Rect, type Size } from '@/lib/geometry' +import { + easeInOutCubic, + lerp, + zoomToFitRect, + zoomViewportAt, + type Point, + type Rect, + type Size, +} from '@/lib/geometry' import { MAX_SCALE, MIN_SCALE, useEditorStore } from './store' const WHEEL_SENSITIVITY = 0.0015 // exponential zoom per wheel delta unit const FIT_PADDING = 32 // px margin around a fitted rect const FIT_DURATION_MS = 350 -const easeInOutCubic = (t: number) => (t < 0.5 ? 4 * t * t * t : 1 - Math.pow(-2 * t + 2, 3) / 2) -const lerp = (a: number, b: number, t: number) => a + (b - a) * t - /** * Wires pan/zoom/pinch gestures onto the svg (via @use-gesture) and returns an * animated `fitToRect`. Wheel and pinch zoom toward the pointer; drag on empty @@ -28,7 +33,8 @@ export function useViewport(svgRef: RefObject) { } }, []) - const toLocal = useCallback( + // Client (page) coords → coords within the svg, for anchoring zoom. + const clientToCanvas = useCallback( (clientX: number, clientY: number): Point => { const rect = svgRef.current?.getBoundingClientRect() return rect ? { x: clientX - rect.left, y: clientY - rect.top } : { x: clientX, y: clientY } @@ -49,12 +55,14 @@ export function useViewport(svgRef: RefObject) { onWheelStart: cancelAnim, onWheel: ({ event, delta: [, dy] }) => { event.preventDefault() - const p = toLocal(event.clientX, event.clientY) + if (!Number.isFinite(dy)) return // never let a bad delta corrupt the viewport + const p = clientToCanvas(event.clientX, event.clientY) setViewport((vp) => zoomViewportAt(vp, p, vp.scale * Math.exp(-dy * WHEEL_SENSITIVITY), MIN_SCALE, MAX_SCALE)) }, onPinchStart: cancelAnim, onPinch: ({ origin: [ox, oy], offset: [scale] }) => { - const p = toLocal(ox, oy) + if (!Number.isFinite(scale)) return + const p = clientToCanvas(ox, oy) setViewport((vp) => zoomViewportAt(vp, p, scale, MIN_SCALE, MAX_SCALE)) }, }, @@ -71,10 +79,10 @@ export function useViewport(svgRef: RefObject) { ) const fitToRect = useCallback( - (rect: Rect, viewport: Size) => { + (rect: Rect, canvasSize: Size) => { cancelAnim() const from = useEditorStore.getState().viewport - const to = zoomToFitRect(rect, viewport, FIT_PADDING, MIN_SCALE, MAX_SCALE) + const to = zoomToFitRect(rect, canvasSize, FIT_PADDING, MIN_SCALE, MAX_SCALE) const start = performance.now() const step = (now: number) => { const t = Math.min(1, (now - start) / FIT_DURATION_MS) diff --git a/web/src/lib/geometry.ts b/web/src/lib/geometry.ts index 5454698..0dfddda 100644 --- a/web/src/lib/geometry.ts +++ b/web/src/lib/geometry.ts @@ -68,6 +68,16 @@ export function clampScale(scale: number, min: number, max: number): number { return Math.min(max, Math.max(min, scale)) } +/** Linear interpolation. */ +export function lerp(a: number, b: number, t: number): number { + return a + (b - a) * t +} + +/** Cubic ease-in-out for viewport tweens. */ +export function easeInOutCubic(t: number): number { + return t < 0.5 ? 4 * t * t * t : 1 - Math.pow(-2 * t + 2, 3) / 2 +} + /** * Re-scale the viewport while keeping the world point currently under * `screenPoint` stationary — the math behind wheel-zoom-to-cursor and pinch. The @@ -90,28 +100,29 @@ export function zoomViewportAt( } /** - * The viewport that frames `rect` centered in a viewport of `viewport` pixels, + * The viewport that frames `rect` centered in a canvas of `canvasSize` pixels, * with `padding` px of margin on every side. Scale is clamped to [minScale, - * maxScale] so a tiny/huge rect still lands in a sane zoom. + * maxScale] so a tiny/huge rect still lands in a sane zoom. Rect dimensions are + * taken by magnitude, so a degenerate (0/negative) size can't corrupt the math. */ export function zoomToFitRect( rect: Rect, - viewport: Size, + canvasSize: Size, padding: number, minScale: number, maxScale: number, ): Viewport { - const availW = Math.max(1, viewport.w - padding * 2) - const availH = Math.max(1, viewport.h - padding * 2) - const rectW = Math.max(rect.w, 1e-6) - const rectH = Math.max(rect.h, 1e-6) + const availW = Math.max(1, canvasSize.w - padding * 2) + const availH = Math.max(1, canvasSize.h - padding * 2) + const rectW = Math.max(Math.abs(rect.w), 1e-6) + const rectH = Math.max(Math.abs(rect.h), 1e-6) const scale = clampScale(Math.min(availW / rectW, availH / rectH), minScale, maxScale) const centerX = rect.x + rect.w / 2 const centerY = rect.y + rect.h / 2 return { scale, - tx: viewport.w / 2 - centerX * scale, - ty: viewport.h / 2 - centerY * scale, + tx: canvasSize.w / 2 - centerX * scale, + ty: canvasSize.h / 2 - centerY * scale, } }