diff --git a/docs/design/sprite-crate-extraction.md b/docs/design/sprite-crate-extraction.md new file mode 100644 index 0000000..a01aa92 --- /dev/null +++ b/docs/design/sprite-crate-extraction.md @@ -0,0 +1,292 @@ +# Design: extract `guiltty-sprite`, add relative movement to `Sprite` + +Source: not a `docs/spec.md` v0 success criterion — forward-looking +groundwork for `iklo` (games, turtle graphics), prompted by wanting +`guiltty-core` to stay scoped to exactly "draw into a kitty-like terminal" +and nothing else. Companion doc: +[`docs/design/turtle-geometry.md`](turtle-geometry.md), which builds on top +of what this doc adds. + +## Objective + +Two changes, sequenced as one design since the second only makes sense once +the first has landed: + +1. Move `Sprite`/`Bitmap` out of `guiltty-core` into a new `guiltty-sprite` + crate — `guiltty-core` keeps only the absolute-coordinate drawing surface + (`Canvas`, `Shape`, text, the `Backend` trait). +2. Give `Sprite` a **relative, actor-centric** movement API (`heading`, + `forward`/`backward`, `turn`) alongside its **existing absolute** one + (`move_to`) — both stay first-class, neither replaces the other. + +## Two coordinate paradigms, both grounded in one absolute truth + +Two ways of thinking about movement are both genuinely useful, and this +design keeps both available rather than picking one: + +- **Absolute (canvas-coordinate).** The canvas is the source of truth: a + `(0, 0)` origin, everything else addressed as absolute coordinates — + `Shape::line(Point::new(x0, y0), Point::new(x1, y1))`, `Sprite::move_to(Point)`. + This is what `guiltty-core` already is today and stays exactly that. +- **Relative (actor-centric).** Movement described from the mover's own + point of view — "go forward 10, turn right 90" — with no absolute + coordinate mentioned at all. This is turtle geometry's defining trait, but + it's useful independent of turtle *graphics* (drawing a trail): a + game sprite that has a heading and moves forward along it needs the same + primitive, with no pen involved. + +The relative paradigm is not a competing coordinate system requiring its own +storage — a `Sprite`'s `forward(distance)` computes one absolute +`(x + distance * heading_deg.to_radians().cos(), y + distance * +heading_deg.to_radians().sin())` from its current position and heading +(heading is stored and specified in **degrees**; radians only exist inside +the formula itself), then is drawn exactly like any other absolute move. On +this canvas's top-left origin (positive Y downward), 0° faces +x (east) and +positive degrees turn **clockwise** — e.g. 90° faces +y (south), not north. +Relative motion is a stateful convenience layer that always resolves to an +absolute position before anything is drawn; `Canvas` never needs to know a +caller was "thinking in relative terms" at all. This is why the paradigm +split maps directly onto the crate split: `guiltty-core` only ever deals in +absolutes, and the relative layer lives entirely in `guiltty-sprite` on top +of it. + +**Both movement APIs stay on `Sprite` itself** — this isn't relative-only: +`move_to(Point)` (already implemented today) remains for absolute +placement, and `forward`/`turn` are additive. A caller can freely mix both +on the same sprite (e.g. `sprite.move_to(spawn_point); sprite.forward(5.0);`). + +## The extraction's one real wrinkle: `Canvas::draw_sprite` touches private fields + +`Canvas::draw_sprite` (`crates/guiltty-core/src/lib.rs`) isn't a simple +consumer of `Canvas`'s public API today — its save/restore-under logic +reads `self.pixels` directly (not through the public, bounds-checked +`pixel()`) and tags each `DrawnFootprint` with `self.id`, a private field +that exists solely so a sprite's saved footprint is never restored onto the +wrong `Canvas` instance. Both are private to `guiltty-core`; once `Sprite` +lives in a different crate, an inherent `Canvas::draw_sprite` can't exist +there anymore (Rust's orphan rule), and the new crate has no access to +`Canvas`'s private fields either way. + +Resolution, in two parts: + +- `Canvas` gains two new public accessors: `pub fn id(&self) -> u64` + (or an opaque `CanvasId` newtype if we'd rather not expose the raw + `u64`) — enough for `guiltty-sprite` to replicate the + wrong-canvas-guard without needing direct field access — and `pub fn + region_version(&self, region: Rect) -> u64`, used below to detect a + stale footprint scoped to the region it actually overlaps, not the + whole canvas. Both are additive and non-breaking: `guiltty-core`'s + existing public API is unchanged, only extended. +- The draw method itself moves to `guiltty-sprite` as `sprite.draw_on(&mut + canvas)` (a method on `Sprite`, since `Canvas` can no longer host an + inherent method for a foreign type), reimplemented entirely against + `Canvas`'s existing public `pixel`/`set_pixel` — trading direct slice + indexing for per-pixel bounds-checked accessor calls. This is slightly + more overhead per pixel, not a behavior change, and consistent with how + every other cross-boundary drawing operation in this codebase already + works; revisit only if profiling ever shows it matters. + +**This second part *is* a breaking change**, and the extraction as a whole +should ship as one: `Sprite`, `Bitmap`, and `Canvas::draw_sprite` disappear +from `guiltty-core`'s public API, and `canvas.draw_sprite(&mut sprite)` +call sites become `sprite.draw_on(&mut canvas)`. `guiltty`'s facade crate +can re-export `Sprite`/`Bitmap`'s new location under the same path (so +`guiltty::Sprite` keeps working), but it **cannot** preserve +`Canvas::draw_sprite` as an inherent method — a re-export doesn't grant a +downstream crate the right to add inherent methods to `Canvas`. Given the +project is pre-1.0 with every crate at `0.0.0` (`docs/spec.md`'s existing +precedent for T1's `Backend::present` signature change), no compatibility +shim is planned: this ships as a documented breaking change in the PR +description, with call sites in this repo's own examples/tests updated in +the same PR, not a deprecation cycle. + +No other part of `Canvas`'s public API needs to change. `Bitmap` moves +alongside `Sprite` (it's `Sprite`'s only real dependency) — including its +`from_file` error path: `Bitmap::from_file` keeps returning +`Result` (the `Error::ImageLoad` variant already +defined in `guiltty-core`), rather than inventing a new crate-local error +type. `guiltty-sprite` already depends on `guiltty-core` directly (for +`Canvas`, `Color`, `Point`, and now `Canvas::id()`), so depending on its +`Error` type too is not a new coupling — just reusing what's already +required. + +## API sketch (`guiltty-sprite`) + +```rust +pub struct Sprite { + bitmap: Bitmap, + exact_position: (f32, f32), // canonical position — sub-pixel precision + heading_deg: f32, // NEW — relative-movement state; 0.0 = facing +x (east) + last_draw: Option, +} + +impl Sprite { + pub fn new(bitmap: Bitmap, position: Point) -> Self; // heading defaults to 0.0 + + // --- absolute (unchanged from today) --- + pub fn position(&self) -> Point; // exact_position, rounded to i32 + pub fn move_to(&mut self, position: Point); // resets exact_position to (x as f32, y as f32) -- no fractional carry-over across an absolute jump + + // --- relative (new) --- + pub fn heading(&self) -> f32; + pub fn set_heading(&mut self, degrees: f32); + pub fn forward(&mut self, distance: f32); // moves exact_position along current heading + pub fn backward(&mut self, distance: f32); // forward(-distance) + pub fn turn(&mut self, degrees: f32); // positive = clockwise + pub fn left(&mut self, degrees: f32); // sugar for turn(-degrees) + pub fn right(&mut self, degrees: f32); // sugar for turn(degrees) + + pub fn bitmap(&self) -> &Bitmap; + + // `draw_on` is `clear_footprint` followed by `place` -- see below. Most callers + // (anything not interleaving other drawing between a sprite's redraws, e.g. + // `guiltty-turtle`) just want this one call. + pub fn draw_on(&mut self, canvas: &mut Canvas); // was Canvas::draw_sprite + + // The two steps `draw_on` composes, exposed separately for callers (like + // `guiltty-turtle`) that need to draw something else *in between* clearing the + // sprite's old footprint and placing it at the new one -- seeing/using only + // `draw_on` can't do this, since it bundles restore+capture+blit as one atomic + // step with nothing else able to run in the middle. + pub fn clear_footprint(&mut self, canvas: &mut Canvas) -> Result<(), StaleFootprint>; // restore-only; Ok(()) no-op if never drawn; Err(StaleFootprint) — canvas left untouched — if drawn on a different Canvas or if this footprint's region has changed since it was captured (see "Footprint staleness" below) + pub fn place(&mut self, canvas: &mut Canvas); // capture-new-footprint-then-blit only, no restore + + // Recovery from a permanently-stale footprint (see "Footprint staleness" + // below): drops last_draw without attempting to restore. The sprite's old + // on-canvas pixels are abandoned as-is -- a visible artifact, not cleaned + // up -- but the sprite becomes drawable again via place()/draw_on(). + pub fn discard_footprint(&mut self); +} +``` + +`exact_position` — not `Point` — is the struct's one canonical position +field; `Point` is only ever a rounded *view* of it, produced by `position()` +and consumed by `draw_on`. This resolves the rounding-drift problem +directly: many small `forward()` calls each accumulate into +`exact_position` at full `f32` precision, and only get rounded to `i32` at +the moment something (`position()`, `draw_on`) actually needs a pixel +coordinate — so fractional displacement from repeated sub-pixel moves is +never silently discarded call-by-call. `move_to` is the one place that +*resets* `exact_position` outright (from the supplied integer `Point`, +losing any prior fractional part) rather than accumulating into it, since an +absolute jump has no meaningful "fractional carry-over" from wherever the +sprite was before. + +## Footprint staleness: version-stamped, fail-fast, region-scoped + +`clear_footprint` restores a snapshot captured at `place` time. If anything +else draws into that same region between the capture and the restore — a +second `clear_footprint` call replaying an already-consumed snapshot, or +(in `guiltty-turtle`) a *different* sprite's trail drawn through this +sprite's footprint before it's cleared — a naive restore silently blits the +old snapshot back, discarding whatever drew there in the meantime. This is +a pixel-level hazard, not a "whose trail is it" one: the canvas has no +notion of ownership, only of what was written and when. + +The fix is version-stamping, checked fail-fast rather than avoided by +restricting when callers are allowed to draw — and scoped to the +footprint's own region, not the whole canvas, so two sprites drawing in +disjoint areas never spuriously invalidate each other: + +```rust +struct DrawnFootprint { + canvas_id: u64, + rect: Rect, // where this footprint was captured -- region_version's input + version: u64, // canvas.region_version(rect), taken *after* place's blit completes + // .. existing footprint pixel data .. +} + +pub struct StaleFootprint; // this footprint's region_version has advanced since capture +``` + +`Canvas` internally divides itself into a coarse fixed-size tile grid (an +implementation detail, not public API) and keeps one version counter per +tile. Every pixel-mutating call (`set_pixel`, `draw_shape`, a sprite's +`place`) computes the `Rect` it touched and stamps a fresh, canvas-wide +monotonic value onto every tile that `Rect` overlaps. +`Canvas::region_version(region: Rect)` returns the *maximum* tile version +across the tiles `region` overlaps — i.e. "the most recent write that could +have touched any pixel in here." `place` captures `region_version(rect)` +**after** its own blit completes, not before — capturing pre-blit would +make the blit itself immediately invalidate the footprint it just created, +since the blit is itself a pixel-mutating write to that same rect, and +every sprite would self-invalidate on the first `clear_footprint` call. +`clear_footprint` recomputes `region_version` over the same stored `rect` +at call time and compares; on a mismatch it returns `Err(StaleFootprint)` +and leaves the canvas untouched, instead of restoring pixels that no +longer reflect what's actually been drawn. + +Scoping to tiles (rather than one canvas-wide counter) is what makes this +safe for independent multi-sprite use: a write to tiles outside a +footprint's own tiles never bumps that footprint's `region_version`, so two +turtles moving in disjoint parts of the canvas never see spurious +staleness from each other — only a write that actually overlaps a +footprint's tiles does. Tile granularity is a tunable trade-off, not a +correctness one: coarser tiles mean fewer tiles to touch per write +(cheaper) but a slightly larger "blast radius" per write (a write in one +corner of a tile can still false-positive a footprint elsewhere in the +same tile); revisit the tile size only if that proves too coarse in +practice. + +**Recovery.** The underlying counter only increases, so once a footprint +goes stale, it stays stale forever — a *retry* of the same `clear_footprint` +call can never succeed. `Sprite::discard_footprint` exists for exactly +this: it drops `last_draw` unconditionally, without attempting a restore, +so the sprite can be `place`d again. The trade-off is explicit and +caller-visible: the sprite's previous on-canvas pixels are never cleaned +up (a duplicate/ghost image can remain), rather than being silently +overwritten with stale data. `guiltty-turtle`'s `Turtle::resync` (see +companion doc) is the caller-facing wrapper around this for the common +turtle case. + +## Non-goals + +- **No collision detection.** Mentioned as a motivating future use case for + relative sprite movement (games), but out of scope for this design — + revisit once there's a concrete need. +- **No change to the existing save/restore-under trail-avoidance + behavior** — `draw_on` preserves `draw_sprite`'s exact semantics, just + relocated and reimplemented against public `Canvas` accessors. + `clear_footprint`/`place` are additive decompositions of that same + behavior (see companion doc's Turtle for why they're needed), not a + new drawing model. +- **No pen/drawing behavior on `Sprite` itself** — that's + `guiltty-turtle`'s job, on top of this crate; see the companion doc. + +## Follow-up + +Two PRs, in order: + +1. **Extract `guiltty-sprite`**: new workspace member, move `Sprite`/`Bitmap` + verbatim (including `Bitmap::from_file`'s `Result` + signature, unchanged), add `Canvas::id()` and `Canvas::region_version()`, + reimplement `draw_on`/`clear_footprint`/`place`/`discard_footprint` against `Canvas`'s + public API, update `guiltty`'s facade re-exports and any existing + sprite-related tests/examples to the new crate and call-site + (`sprite.draw_on(&mut canvas)` instead of `canvas.draw_sprite(&mut + sprite)`). No drawing-behavior change, but a breaking public-API change + as described above — call this out explicitly in the PR description, + don't call it "non-breaking." "Any existing sprite-related tests" is + concretely: `Canvas::draw_sprite`'s current transparency, clipping, + same-canvas-movement, and cross-canvas-drawing tests move over to + `Sprite::draw_on` with their assertions preserved, plus a new test that + `draw_on`/`clear_footprint` is a no-op — not a panic, not a draw onto + the wrong pixels — when called with a `Canvas` whose `id()` doesn't + match the footprint's captured one. Also cover `clear_footprint`'s + stale-detection: calling it twice in a row returns `Err(StaleFootprint)` + on the second call, and a write to the canvas between `place` and + `clear_footprint` (standing in for another sprite's trail crossing this + one's footprint) does too — both leaving the canvas' pixels unchanged. + Also cover the two bugs this design previously got wrong: a + `clear_footprint` called immediately after `place`, with no intervening + writes, must succeed (guards against stamping the footprint's version + before `place`'s own blit); and a write to a *disjoint* region of the + canvas must not cause a subsequent `clear_footprint` to fail (the + region-scoping this design relies on). Finally, a recovery test: + `discard_footprint` after `Err(StaleFootprint)`, followed by `place`, + succeeds. +2. **Add relative movement**: `heading`/`forward`/`backward`/`turn`/`left`/ + `right` on `Sprite`, with unit tests covering heading after known turn + sequences, position after known forward/turn sequences (including the + sub-pixel rounding case), and that `move_to` and `forward` compose + correctly when mixed. diff --git a/docs/design/turtle-geometry.md b/docs/design/turtle-geometry.md new file mode 100644 index 0000000..b8733eb --- /dev/null +++ b/docs/design/turtle-geometry.md @@ -0,0 +1,156 @@ +# Design: turtle geometry (`guiltty-turtle`) + +Source: not a `docs/spec.md` v0 success criterion — forward-looking work for +`iklo`, which will need turtle-style drawing on top of `guiltty` later. +Depends on [`docs/design/sprite-crate-extraction.md`](sprite-crate-extraction.md), +which must land first: this design assumes `guiltty-sprite`'s `Sprite` +already has absolute (`move_to`) and relative (`heading`/`forward`/`turn`) +movement — turtle graphics adds nothing to *movement* itself, only pen +state and drawing. + +## Objective + +Give callers a Logo-style turtle: a `guiltty-sprite`-backed actor that can +trace a line as it moves, with pen up/down toggling whether movement draws. +Multiple turtles (a feature of some Logo dialects) then falls out for free +from having multiple `Sprite`s, exactly as multiple sprites already do. + +## Decision: `Turtle` wraps a `Sprite`, adds only pen state + +Movement (absolute and relative) is entirely `guiltty-sprite`'s concern +already — see that doc's rationale for why. What's specifically "turtle +graphics" and not more generally useful is narrow: **does moving leave a +visible trail**. That's the only thing this crate adds: + +```rust +pub struct Turtle { + sprite: Sprite, + pen_down: bool, + pen_color: Color, +} + +impl Turtle { + pub fn new(bitmap: Bitmap, position: Point) -> Self; // pen down, black, wraps Sprite::new + + // --- pen state (new — this crate's entire reason to exist) --- + pub fn pen_up(&mut self) -> &mut Self; + pub fn pen_down(&mut self) -> &mut Self; + pub fn set_pen_color(&mut self, color: Color) -> &mut Self; + + // --- movement (delegates straight to the wrapped Sprite, then draws if pen is down) --- + // Drawing moves return Result, not `&mut Self`: `clear_footprint` can now + // fail (see companion doc's "Footprint staleness"), and a failed move must + // not silently continue as if it had drawn. `?`-chaining replaces + // method-chaining for these three. + pub fn forward(&mut self, canvas: &mut Canvas, distance: f32) -> Result<&mut Self, StaleFootprint>; + pub fn backward(&mut self, canvas: &mut Canvas, distance: f32) -> Result<&mut Self, StaleFootprint>; + pub fn turn(&mut self, degrees: f32) -> &mut Self; // no line to draw — heading-only, no canvas needed + pub fn left(&mut self, degrees: f32) -> &mut Self; + pub fn right(&mut self, degrees: f32) -> &mut Self; + pub fn goto(&mut self, canvas: &mut Canvas, position: Point) -> Result<&mut Self, StaleFootprint>; // absolute move + draw + + pub fn sprite(&self) -> &Sprite; // escape hatch to the underlying Sprite + pub fn sprite_mut(&mut self) -> &mut Sprite; + + // Recovery from Err(StaleFootprint) (see below): discards the sprite's + // stale footprint and re-places it at its current position -- no trail + // segment is drawn (there's no known-good `from` pixel state to draw + // over). After `resync`, `forward`/`backward`/`goto` work normally again. + pub fn resync(&mut self, canvas: &mut Canvas) -> &mut Self; +} +``` + +Each drawing move (`forward`/`backward`/`goto`) uses `guiltty-sprite`'s +`clear_footprint`/`place` split (not the bundled `draw_on`) specifically to +get a trail line drawn *between* the two — using `draw_on` here would +restore the sprite's *old* footprint **after** the trail line already +drew into it, erasing exactly the pixels where the line started, on every +single move. In order: (1) `sprite.clear_footprint(canvas)` — reveals +whatever the canvas actually showed before the icon was last placed there +(which includes any trail segment drawn on a prior move, since that segment +was drawn *before* that prior move's own `place` call captured it), (2) +record the current position as `from`, (3) delegate to the wrapped +`Sprite`'s own `forward`/`backward`/`move_to` to update its position, (4) if +`pen_down`, draw `canvas.draw_shape(&Shape::line(from, sprite.position()), +Fill::solid(pen_color))` onto the now-cleared canvas, (5) +`sprite.place(canvas)` — captures the footprint *after* the trail segment is +already there, then blits the icon on top. The icon ends up sitting over the +trail's endpoint each move (cosmetic — trail-under-icon, not the other way +around — acceptable and easy to flip later if wanted). + +Step (1) can now fail: if `clear_footprint` returns `Err(StaleFootprint)` +— *another* sprite's write actually overlapped this sprite's footprint +since its last `place`, e.g. a *different* `Turtle`'s pen-down move drew +through this one's current footprint (disjoint turtles never trigger this +— see the companion doc's region-scoping) — `forward`/`backward`/`goto` +propagate the error instead of drawing, leaving this turtle's position, the +sprite's `last_draw`, and the canvas all untouched (see +[`sprite-crate-extraction.md`](sprite-crate-extraction.md)'s "Footprint +staleness"). This is what makes it safe for two turtles' trails to cross: +whichever one next tries to redraw over the intersection gets a caught +error on that one move instead of silently erasing the other turtle's +trail. It does not automatically preserve both trails through the overlap; +it only guarantees the conflict can't pass silently. + +Retrying the same move after `Err(StaleFootprint)` cannot succeed on its +own — the underlying version only increases, so the mismatch is permanent +for that footprint. The caller must call `resync(canvas)` first (discards +the stale footprint, re-places the icon with no trail segment for that +recovery step), then retry the move normally. A turtle that never calls +`resync` after a stale error stays stuck: every subsequent move on it +repeats the same failed `clear_footprint` check. + +`turn`/`left`/`right` +only change heading — no line to draw, so unlike the movement methods they +don't need a `&mut Canvas` argument at all. + +This is a strict subset of what the original standalone sketch proposed: +no separate position/heading fields (the wrapped `Sprite` already has them), +no separate rounding-drift handling (already solved in `guiltty-sprite`). +It does, however, depend on `guiltty-sprite` exposing the `clear_footprint`/ +`place` split alongside `draw_on` — see that doc's API sketch. + +## Non-goals + +(Unchanged from the original sketch.) + +- **No automatic closed-shape fill.** A turtle traces one `Shape::Line` per + move; it does not detect when a path closes and switch to `Shape::Path`'s + fill behavior. Out of scope for this first version. +- **No angle/arc/circle turtle commands** — only straight segments and + in-place turns. Can be added later without breaking this API. +- **No serialization/replay of a turtle's move history.** +- **No collision/game-oriented features** — those belong to plain + `guiltty-sprite` usage (relative movement without a pen), not this crate. + +## Open questions + +- Whether `Turtle::new` should take an already-constructed `Sprite` instead + of a `Bitmap`+`Point` (so a caller who already built one via + `guiltty-sprite` doesn't have to unpack/repack it) — leaning toward + accepting `Sprite` directly once `guiltty-sprite` lands, revisit at + implementation time. +- Default heading-0° convention (east vs. Logo's traditional north) — + inherited from `guiltty-sprite`, not re-decided here. + +## Follow-up + +Once [`sprite-crate-extraction.md`](sprite-crate-extraction.md)'s two PRs +land: scaffold `crates/guiltty-turtle` (new workspace member, depending on +both `guiltty-sprite`, for `Sprite`/`Bitmap`, **and directly on +`guiltty-core`**, for `Canvas`/`Color`/`Point`/`Shape`/`Fill` — the sketch +above uses all of these directly, so this isn't a `guiltty-sprite`-only +dependency), implement `Turtle` per the sketch above with unit tests +(pen-down drawing a line segment on the move that triggered it, pen-up not +drawing, pen-color changes affecting only subsequent segments, +`turn` not requiring a canvas, a regression test asserting a single +turtle's multi-move trail has **no gap** at any previous position — the +exact bug this design's `clear_footprint`/`place` ordering exists to +prevent — a two-turtle test where B's footprint overlaps a segment A +draws afterward: B's next `forward` returns `Err(StaleFootprint)` instead +of erasing A's trail, a test that two turtles moving in genuinely disjoint +areas never see `Err(StaleFootprint)` from each other, and a recovery test +that `resync` followed by a normal move succeeds after a prior +`StaleFootprint`), and a small example under `examples/` +tracing a recognizable shape (e.g. a star or spiral) to double as the +manual visual check.