From 72eb040185a2c7668a6d19d66683454e20556718 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rog=C3=A9rio=20Senna=20=5Bmmm1=5D?= Date: Sat, 1 Aug 2026 03:25:46 +0200 Subject: [PATCH 1/2] feat: extract guiltty-sprite crate, region-scoped footprint staleness Implements PR 1/2 of docs/design/sprite-crate-extraction.md: move Sprite/Bitmap out of guiltty-core into a new guiltty-sprite crate, replacing the inherent Canvas::draw_sprite with sprite.draw_on(&mut canvas) -- a breaking public-API change, no compatibility shim (pre-1.0). guiltty-core gains Canvas::id() and Canvas::region_version(Rect), backed by a per-tile version grid bumped by every pixel-mutating call (set_pixel, draw_shape, draw_text). guiltty-sprite's Sprite exposes draw_on/clear_footprint/place/discard_footprint: clear_footprint returns Err(StaleFootprint) (canvas left untouched) if anything wrote into its footprint's own region since capture, rather than silently restoring stale pixels -- the design's "Footprint staleness" fix. discard_footprint recovers a footprint that's gone permanently stale (the version counter only increases, so a bare retry can never succeed) without attempting a restore. Sprite/Bitmap/draw_sprite's existing tests moved to guiltty-sprite as draw_on tests; added coverage for the two bugs the design doc's first pass got wrong (stamp-after-blit self-invalidation, canvas-wide invalidation breaking disjoint sprites) plus the wrong-canvas no-op and discard_footprint recovery paths. Co-Authored-By: WOZCODE --- Cargo.lock | 12 +- Cargo.toml | 1 + crates/guiltty-core/Cargo.toml | 5 +- crates/guiltty-core/src/lib.rs | 657 +++++------------- crates/guiltty-sprite/Cargo.toml | 17 + crates/guiltty-sprite/src/lib.rs | 644 +++++++++++++++++ .../tests/fixtures/grayscale_2x2.png | Bin .../tests/fixtures/malformed.png | 0 .../tests/fixtures/rgb_2x2.png | Bin .../tests/fixtures/rgba_2x2.png | Bin crates/guiltty/Cargo.toml | 1 + crates/guiltty/src/lib.rs | 5 +- examples/src/bin/demo.rs | 8 +- 13 files changed, 870 insertions(+), 480 deletions(-) create mode 100644 crates/guiltty-sprite/Cargo.toml create mode 100644 crates/guiltty-sprite/src/lib.rs rename crates/{guiltty-core => guiltty-sprite}/tests/fixtures/grayscale_2x2.png (100%) rename crates/{guiltty-core => guiltty-sprite}/tests/fixtures/malformed.png (100%) rename crates/{guiltty-core => guiltty-sprite}/tests/fixtures/rgb_2x2.png (100%) rename crates/{guiltty-core => guiltty-sprite}/tests/fixtures/rgba_2x2.png (100%) diff --git a/Cargo.lock b/Cargo.lock index 8baab22..afe0a3c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -136,14 +136,12 @@ version = "0.0.0" dependencies = [ "guiltty-core", "guiltty-kitty", + "guiltty-sprite", ] [[package]] name = "guiltty-core" version = "0.0.0" -dependencies = [ - "image", -] [[package]] name = "guiltty-examples" @@ -161,6 +159,14 @@ dependencies = [ "kittage", ] +[[package]] +name = "guiltty-sprite" +version = "0.0.0" +dependencies = [ + "guiltty-core", + "image", +] + [[package]] name = "image" version = "0.25.10" diff --git a/Cargo.toml b/Cargo.toml index c857854..0623df8 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -2,6 +2,7 @@ resolver = "2" members = [ "crates/guiltty-core", + "crates/guiltty-sprite", "crates/guiltty-kitty", "crates/guiltty", "examples", diff --git a/crates/guiltty-core/Cargo.toml b/crates/guiltty-core/Cargo.toml index dfea427..2e15996 100644 --- a/crates/guiltty-core/Cargo.toml +++ b/crates/guiltty-core/Cargo.toml @@ -7,10 +7,7 @@ rust-version.workspace = true license.workspace = true authors.workspace = true repository.workspace = true -description = "Backend-agnostic canvas, shapes, text, sprites, regions, and zoom/scroll logic for guiltty" +description = "Backend-agnostic canvas, shapes, text, and regions/zoom/scroll logic for guiltty" [lints] workspace = true - -[dependencies] -image = { version = "0.25.10", default-features = false, features = ["png", "jpeg", "gif", "bmp"] } diff --git a/crates/guiltty-core/src/lib.rs b/crates/guiltty-core/src/lib.rs index 51b6ea0..1ab9865 100644 --- a/crates/guiltty-core/src/lib.rs +++ b/crates/guiltty-core/src/lib.rs @@ -1,7 +1,7 @@ //! Backend-agnostic core primitives (`Color`, `Point`, `Rect`) and the [`Backend`] trait that -//! rendering backends (e.g. `guiltty-kitty`) implement. `Canvas` supports shape drawing, text, -//! and movable sprites (see [`Canvas::draw_sprite`]); region/zoom/scroll logic described in the -//! spec is not implemented yet. +//! rendering backends (e.g. `guiltty-kitty`) implement. `Canvas` supports shape drawing and +//! text; movable sprites live in the separate `guiltty-sprite` crate, built entirely on this +//! one's public API. Region/zoom/scroll logic described in the spec is not implemented yet. /// RGBA8 color, used throughout for canvas pixels, shape fills, and sprite bitmaps. #[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] @@ -99,20 +99,37 @@ pub trait Backend { /// instance (including ones produced by `Canvas::clone()`) — see `Canvas::id`. static NEXT_CANVAS_ID: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0); +/// Side length, in pixels, of one tile in `Canvas`'s region-version grid (see +/// `Canvas::region_version`). An implementation detail, not public API. +const TILE_SIZE: u32 = 32; + /// A pixel buffer that can be drawn into. RGBA8, origin top-left, row-major. #[derive(Debug)] pub struct Canvas { /// Distinguishes this canvas from every other one, including clones of itself, so a - /// `Sprite`'s saved-under footprint (see [`DrawnFootprint`]) is never restored onto - /// the wrong canvas. + /// sprite's saved-under footprint (see the `guiltty-sprite` crate) is never restored + /// onto the wrong canvas. id: u64, width: u32, height: u32, pixels: Vec, + /// Tile grid backing `region_version`: one counter per `TILE_SIZE`x`TILE_SIZE` tile, + /// stamped with `next_version` whenever a pixel-mutating call touches that tile. + tiles_x: u32, + tiles_y: u32, + tile_versions: Vec, + /// Bumped once per pixel-mutating call, then stamped onto every tile that call's + /// bounding region overlaps. Never reset except by `Canvas::new`/`Canvas::clone`, so + /// `region_version` results are only ever comparable within one `Canvas` instance's + /// lifetime (matching `id`'s per-instance scoping). + next_version: u64, } /// Manually implemented (rather than `#[derive(Clone)]`) so a cloned canvas gets its own -/// fresh `id` instead of inheriting the original's — see the `id` field's doc comment. +/// fresh `id` instead of inheriting the original's — see the `id` field's doc comment. The +/// version-tracking fields reset fresh too: a clone's `id` won't match any footprint +/// captured from the original, so the original's version history has nothing left to +/// stay comparable with. impl Clone for Canvas { fn clone(&self) -> Self { Self { @@ -120,6 +137,10 @@ impl Clone for Canvas { width: self.width, height: self.height, pixels: self.pixels.clone(), + tiles_x: self.tiles_x, + tiles_y: self.tiles_y, + tile_versions: vec![0; self.tile_versions.len()], + next_version: 0, } } } @@ -139,14 +160,27 @@ impl Canvas { let len = (width as usize) .checked_mul(height as usize) .expect("Canvas dimensions too large: width * height overflows usize"); + let tiles_x = width.div_ceil(TILE_SIZE).max(1); + let tiles_y = height.div_ceil(TILE_SIZE).max(1); Self { id: NEXT_CANVAS_ID.fetch_add(1, std::sync::atomic::Ordering::Relaxed), width, height, pixels: vec![Color::default(); len], + tiles_x, + tiles_y, + tile_versions: vec![0; (tiles_x as usize) * (tiles_y as usize)], + next_version: 0, } } + /// Uniquely identifies this `Canvas` instance (including across clones -- see + /// [`Canvas::clone`]). Used by `guiltty-sprite` to guard against restoring a + /// sprite's saved footprint onto the wrong canvas. + pub fn id(&self) -> u64 { + self.id + } + /// Canvas width in pixels. pub fn width(&self) -> u32 { self.width @@ -177,6 +211,7 @@ impl Canvas { pub fn set_pixel(&mut self, x: u32, y: u32, color: Color) { if let Some(i) = self.index(x, y) { self.pixels[i] = color; + self.touch_region(Rect::new(x as i32, y as i32, 1, 1)); } } @@ -190,6 +225,142 @@ impl Canvas { bytes } + /// Returns the most recent write-version among the tiles `region` overlaps -- i.e. + /// "the most recent pixel-mutating call that could have touched any pixel in here." + /// `0` if `region` touches no tile that's ever been written to (including a region + /// entirely off-canvas). Used by `guiltty-sprite` to detect a stale footprint: a + /// captured `region_version` that no longer matches a fresh call means something + /// wrote into that region since capture. See `docs/design/sprite-crate-extraction.md`'s + /// "Footprint staleness". + pub fn region_version(&self, region: Rect) -> u64 { + let Some((tx_lo, tx_hi, ty_lo, ty_hi)) = self.tile_range(region) else { + return 0; + }; + let mut max_version = 0u64; + for ty in ty_lo..=ty_hi { + for tx in tx_lo..=tx_hi { + max_version = + max_version.max(self.tile_versions[(ty * self.tiles_x + tx) as usize]); + } + } + max_version + } + + /// Bumps every tile `region` overlaps to a fresh version. Called from every + /// pixel-mutating method (`set_pixel`, `draw_shape`, `draw_text`) with that call's own + /// bounding region, so `region_version` can later tell whether anything wrote into a + /// given area since some earlier point in time. A no-op if `region` is entirely + /// off-canvas. + fn touch_region(&mut self, region: Rect) { + let Some((tx_lo, tx_hi, ty_lo, ty_hi)) = self.tile_range(region) else { + return; + }; + self.next_version += 1; + let version = self.next_version; + for ty in ty_lo..=ty_hi { + for tx in tx_lo..=tx_hi { + self.tile_versions[(ty * self.tiles_x + tx) as usize] = version; + } + } + } + + /// Clips `region` to this canvas's bounds (in `i64`, mirroring the rest of this + /// file's overflow-safe clipping idiom) and converts the result to an inclusive tile + /// index range `(tx_lo, tx_hi, ty_lo, ty_hi)`. `None` if the clipped region is empty + /// (entirely off-canvas, or zero-sized). + fn tile_range(&self, region: Rect) -> Option<(u32, u32, u32, u32)> { + let canvas_w = self.width as i64; + let canvas_h = self.height as i64; + let x_lo = (region.x as i64).max(0); + let x_hi = (region.x as i64 + region.width as i64).min(canvas_w); + let y_lo = (region.y as i64).max(0); + let y_hi = (region.y as i64 + region.height as i64).min(canvas_h); + if x_hi <= x_lo || y_hi <= y_lo { + return None; + } + Some(( + (x_lo as u32) / TILE_SIZE, + ((x_hi - 1) as u32) / TILE_SIZE, + (y_lo as u32) / TILE_SIZE, + ((y_hi - 1) as u32) / TILE_SIZE, + )) + } + + /// Bounding region of `shape` in canvas coordinate space, for `touch_region` only -- + /// not used for rendering, so it doesn't need pixel-perfect precision, just to + /// contain every pixel the shape's own drawing logic below could touch. Computed in + /// `i64` so the extreme coordinates/radii this crate already guards against + /// elsewhere (see the `draw_shape_*_does_not_overflow_or_panic` tests) can't overflow + /// here either. + fn shape_bbox(shape: &Shape) -> Rect { + let (x_lo, y_lo, x_hi, y_hi) = match shape { + Shape::Line { from, to } => ( + (from.x as i64).min(to.x as i64), + (from.y as i64).min(to.y as i64), + (from.x as i64).max(to.x as i64) + 1, + (from.y as i64).max(to.y as i64) + 1, + ), + Shape::Rect { + origin, + width, + height, + } => ( + origin.x as i64, + origin.y as i64, + origin.x as i64 + *width as i64, + origin.y as i64 + *height as i64, + ), + Shape::Circle { center, radius } => Self::radial_bbox(*center, *radius, *radius), + Shape::Ellipse { center, rx, ry } => Self::radial_bbox(*center, *rx, *ry), + Shape::Triangle { a, b, c } => { + let xs = [a.x as i64, b.x as i64, c.x as i64]; + let ys = [a.y as i64, b.y as i64, c.y as i64]; + ( + xs.into_iter().min().unwrap(), + ys.into_iter().min().unwrap(), + xs.into_iter().max().unwrap() + 1, + ys.into_iter().max().unwrap() + 1, + ) + } + Shape::Path { points, .. } => { + if points.is_empty() { + (0, 0, 0, 0) + } else { + ( + points.iter().map(|p| p.x as i64).min().unwrap(), + points.iter().map(|p| p.y as i64).min().unwrap(), + points.iter().map(|p| p.x as i64).max().unwrap() + 1, + points.iter().map(|p| p.y as i64).max().unwrap() + 1, + ) + } + } + }; + Self::rect_from_i64_bounds(x_lo, y_lo, x_hi, y_hi) + } + + /// Bounding region of a circle/ellipse (`center` +/- `rx`/`ry`), for `shape_bbox`. + fn radial_bbox(center: Point, rx: u32, ry: u32) -> (i64, i64, i64, i64) { + ( + center.x as i64 - rx as i64, + center.y as i64 - ry as i64, + center.x as i64 + rx as i64 + 1, + center.y as i64 + ry as i64 + 1, + ) + } + + /// Builds a `Rect` from `i64` bounds, clamping to `i32`/`u32`-representable ranges + /// rather than propagating extreme values into `Rect`'s fields. Only used as an + /// approximate `touch_region` input, never for rendering, so this clamping can't + /// affect what's actually drawn -- only (conservatively) which tiles get marked + /// touched. + fn rect_from_i64_bounds(x_lo: i64, y_lo: i64, x_hi: i64, y_hi: i64) -> Rect { + let x = x_lo.clamp(i32::MIN as i64, i32::MAX as i64) as i32; + let y = y_lo.clamp(i32::MIN as i64, i32::MAX as i64) as i32; + let width = (x_hi - x_lo).clamp(0, u32::MAX as i64) as u32; + let height = (y_hi - y_lo).clamp(0, u32::MAX as i64) as u32; + Rect::new(x, y, width, height) + } + /// Draws `text` starting at `origin` (top-left of the first glyph) using `style`. /// /// v0's built-in font covers only space, digits, and uppercase `A`-`Z` (see the @@ -209,6 +380,15 @@ impl Canvas { let origin_y = origin.y as i64; let mut cursor_x = origin.x as i64; + let text_width = advance * text.chars().count() as i64; + let text_height = font::GLYPH_HEIGHT as i64 * scale; + self.touch_region(Self::rect_from_i64_bounds( + origin.x as i64, + origin_y, + origin.x as i64 + text_width, + origin_y + text_height, + )); + for ch in text.chars() { if cursor_x >= canvas_w { break; // everything further right is off-canvas; nothing more to draw @@ -286,6 +466,8 @@ impl Default for TextStyle { /// non-ASCII) — full font coverage is a follow-up task once real text needs demand it. mod font { pub const GLYPH_WIDTH: u32 = 3; + /// Every glyph is 5 rows tall (see the glyph table below). + pub const GLYPH_HEIGHT: u32 = 5; /// Returns the glyph for `ch` as 5 rows of `GLYPH_WIDTH` characters (`'#'` = lit /// pixel, anything else = unlit), or `None` for unsupported characters. @@ -432,6 +614,7 @@ impl Canvas { /// filled solid vs. stroked. pub fn draw_shape(&mut self, shape: &Shape, fill: Fill) { let color = fill.color(); + self.touch_region(Self::shape_bbox(shape)); match shape { Shape::Line { from, to } => self.stroke_line(*from, *to, color), Shape::Rect { @@ -654,273 +837,6 @@ impl Canvas { } } -/// A small RGBA8 image used as sprite content. Structurally similar to `Canvas`'s pixel -/// buffer, but represents drawable material rather than a render target. Can be built -/// in-memory ([`Bitmap::new`]/[`Bitmap::solid`]) or loaded from a file on disk -/// ([`Bitmap::from_file`]). -#[derive(Debug, Clone)] -pub struct Bitmap { - width: u32, - height: u32, - pixels: Vec, -} - -impl Bitmap { - /// Creates a bitmap from an explicit pixel buffer (row-major, RGBA8). - /// - /// # Panics - /// Panics if `pixels.len() != width * height`, or via [`Bitmap::checked_len`] if - /// `width`/`height` don't fit in `usize` on the current target, or if - /// `width * height` overflows `usize`. - pub fn new(width: u32, height: u32, pixels: Vec) -> Self { - let expected = Self::checked_len(width, height); - assert_eq!( - pixels.len(), - expected, - "Bitmap::new: pixels.len() ({}) must equal width*height ({})", - pixels.len(), - expected - ); - Self { - width, - height, - pixels, - } - } - - /// Creates a bitmap of the given size, every pixel set to `color`. - pub fn solid(width: u32, height: u32, color: Color) -> Self { - let len = Self::checked_len(width, height); - Self { - width, - height, - pixels: vec![color; len], - } - } - - /// Loads an image file (PNG, JPEG, GIF, or BMP) from `path` and converts it to a - /// bitmap. Every source pixel format (RGB, grayscale, indexed palette, etc.) is - /// converted to this crate's RGBA8 color model: sources without an alpha channel get - /// a fully-opaque (255) default alpha. - /// - /// Returns `Err(Error::ImageLoad(_))` rather than panicking for a missing file, an - /// unsupported format, or malformed/corrupt image data -- per this crate's - /// recoverable-error convention (see the crate-level Code Style notes), a bad file on - /// disk is exactly the kind of caller-facing condition that shouldn't panic. - pub fn from_file>(path: P) -> Result { - let img = image::open(path.as_ref()) - .map_err(|e| Error::ImageLoad(format!("{}: {e}", path.as_ref().display())))?; - let rgba = img.into_rgba8(); - let (width, height) = rgba.dimensions(); - let pixels = rgba - .pixels() - .map(|p| Color::rgba(p[0], p[1], p[2], p[3])) - .collect(); - Ok(Self { - width, - height, - pixels, - }) - } - - /// `width * height` as a `usize`, converting each dimension with a checked cast first - /// (rather than a truncating `as usize`) so this can't silently disagree with the - /// `u32` dimensions on a target where `usize` is narrower than `u32`. - fn checked_len(width: u32, height: u32) -> usize { - let w: usize = width.try_into().expect("Bitmap width too large for usize"); - let h: usize = height - .try_into() - .expect("Bitmap height too large for usize"); - w.checked_mul(h) - .expect("Bitmap dimensions too large: width * height overflows usize") - } - - /// Bitmap width in pixels. - pub fn width(&self) -> u32 { - self.width - } - - /// Bitmap height in pixels. - pub fn height(&self) -> u32 { - self.height - } - - /// Row-major pixel index for `(x, y)`, or `None` if out of bounds — mirrors - /// `Canvas::index`. - fn index(&self, x: u32, y: u32) -> Option { - if x >= self.width || y >= self.height { - return None; - } - Some(y as usize * self.width as usize + x as usize) - } - - /// Returns the color at `(x, y)`, or `None` if out of bounds. - pub fn pixel(&self, x: u32, y: u32) -> Option { - self.index(x, y).map(|i| self.pixels[i]) - } -} - -/// The rectangle of canvas pixels a [`Sprite`] last drew over, saved so -/// [`Canvas::draw_sprite`] can restore them before drawing the sprite again elsewhere. -/// Tagged with the id of the `Canvas` it was captured from, so it's never mistakenly -/// restored onto a different canvas instance (see `Canvas::draw_sprite`). -#[derive(Debug)] -struct DrawnFootprint { - canvas_id: u64, - x: i64, - y: i64, - width: u32, - height: u32, - pixels: Vec, -} - -/// A movable 2D bitmap positioned over a canvas. See [`Canvas::draw_sprite`] for how -/// moving and redrawing a sprite avoids leaving a trail of its previous position. -#[derive(Debug)] -pub struct Sprite { - bitmap: Bitmap, - position: Point, - last_draw: Option, -} - -/// Manually implemented (rather than `#[derive(Clone)]`) so a cloned sprite starts with -/// no drawing history of its own: it copies the bitmap and position, but not -/// `last_draw`, since the clone has never actually been drawn anywhere. Without this, a -/// clone of an already-drawn sprite would restore the *original* sprite's footprint the -/// first time it's drawn, corrupting whatever the original still shows on the canvas. -impl Clone for Sprite { - fn clone(&self) -> Self { - Self { - bitmap: self.bitmap.clone(), - position: self.position, - last_draw: None, - } - } -} - -impl Sprite { - /// Creates a sprite from `bitmap`, placed at `position` (top-left of the bitmap). - pub fn new(bitmap: Bitmap, position: Point) -> Self { - Self { - bitmap, - position, - last_draw: None, - } - } - - /// The sprite's current position. - pub fn position(&self) -> Point { - self.position - } - - /// Moves the sprite to a new position. Takes effect the next time - /// [`Canvas::draw_sprite`] is called with this sprite — that call restores whatever - /// the sprite covered at its previous position before drawing it at the new one. - pub fn move_to(&mut self, position: Point) { - self.position = position; - } - - /// The sprite's bitmap content. - pub fn bitmap(&self) -> &Bitmap { - &self.bitmap - } -} - -impl Canvas { - /// Draws `sprite`'s bitmap onto this canvas at its current position, clipped to - /// canvas bounds. - /// - /// Uses save-under/restore-under: if `sprite` was drawn by an earlier call to this - /// method, whatever the canvas showed at that previous footprint is restored first, - /// then the canvas content at the new footprint is captured before the sprite is - /// drawn over it. This is what lets a sprite move and be redrawn repeatedly without - /// leaving a trail of its old position — **as long as nothing else draws into its - /// footprint in between two `draw_sprite` calls for it**; if something does, this - /// restore overwrites that intervening content. A real per-frame redraw loop (a - /// future `Terminal`/`Frame` abstraction redrawing the whole scene from scratch each - /// frame) wouldn't have that limitation; this is the simpler, self-contained - /// mechanism available today. Each `Sprite` only remembers its own last footprint — - /// drawing a different sprite, or drawing this one onto a different `Canvas`, doesn't - /// know about or restore anything another sprite painted over the same area. - /// - /// Within the new footprint, fully transparent bitmap pixels (`alpha == 0`) are - /// skipped rather than overwriting the canvas; non-transparent pixels replace - /// outright (no alpha blending, matching the no-anti-aliasing precedent set by - /// `draw_shape`). - pub fn draw_sprite(&mut self, sprite: &mut Sprite) { - let canvas_w = self.width as i64; - let canvas_h = self.height as i64; - - if let Some(prev) = sprite.last_draw.take() { - // A footprint captured from a *different* canvas has nothing to do with this - // one's current pixels; restoring it here would corrupt this canvas with - // stale content from elsewhere, so just drop it instead. - if prev.canvas_id == self.id { - self.restore_footprint(&prev); - } - } - - let (px, py) = (sprite.position.x as i64, sprite.position.y as i64); - let bmp_w = sprite.bitmap.width as i64; - let bmp_h = sprite.bitmap.height as i64; - let x_lo = px.max(0); - let x_hi = (px + bmp_w).min(canvas_w); - let y_lo = py.max(0); - let y_hi = (py + bmp_h).min(canvas_h); - let cap_w = (x_hi - x_lo).max(0) as usize; - let cap_h = (y_hi - y_lo).max(0) as usize; - let mut saved = Vec::with_capacity(cap_w * cap_h); - - let canvas_row_len = self.width as usize; - let bmp_row_len = sprite.bitmap.width as usize; - for y in y_lo..y_hi { - let canvas_row = y as usize * canvas_row_len; - let bmp_row = (y - py) as usize * bmp_row_len; - for x in x_lo..x_hi { - // In-bounds by construction: x_lo/x_hi/y_lo/y_hi are already clipped to - // both canvas and bitmap dimensions, so no bounds check is needed here. - let idx = canvas_row + x as usize; - saved.push(self.pixels[idx]); - let color = sprite.bitmap.pixels[bmp_row + (x - px) as usize]; - if color.a != 0 { - self.pixels[idx] = color; - } - } - } - - sprite.last_draw = Some(DrawnFootprint { - canvas_id: self.id, - x: x_lo, - y: y_lo, - width: cap_w as u32, - height: cap_h as u32, - pixels: saved, - }); - } - - /// Restores the canvas pixels a sprite's previous footprint covered, clipped to - /// whatever part of that footprint still falls within current canvas bounds. - fn restore_footprint(&mut self, prev: &DrawnFootprint) { - let canvas_w = self.width as i64; - let canvas_h = self.height as i64; - let row_len = prev.width as usize; - for dy in 0..prev.height as i64 { - let y = prev.y + dy; - if y < 0 || y >= canvas_h { - continue; - } - let row = dy as usize * row_len; - for dx in 0..prev.width as i64 { - let x = prev.x + dx; - if x < 0 || x >= canvas_w { - continue; - } - self.set_pixel(x as u32, y as u32, prev.pixels[row + dx as usize]); - } - } - } -} - /// Clips the segment `(x0,y0)-(x1,y1)` to `[0,w) x [0,h)` via Liang-Barsky, returning the /// clipped integer endpoints, or `None` if the segment doesn't intersect the canvas at /// all. Used by `stroke_line` so an extreme-but-valid `Point` pair (e.g. one endpoint at @@ -1349,199 +1265,4 @@ mod tests { } } } - - #[test] - fn bitmap_new_and_pixel_roundtrip() { - let b = Bitmap::new( - 2, - 2, - vec![ - Color::rgb(1, 0, 0), - Color::rgb(2, 0, 0), - Color::rgb(3, 0, 0), - Color::rgb(4, 0, 0), - ], - ); - assert_eq!(b.width(), 2); - assert_eq!(b.height(), 2); - assert_eq!(b.pixel(0, 0), Some(Color::rgb(1, 0, 0))); - assert_eq!(b.pixel(1, 1), Some(Color::rgb(4, 0, 0))); - assert_eq!(b.pixel(2, 0), None); - } - - #[test] - #[should_panic(expected = "must equal width*height")] - fn bitmap_new_panics_on_mismatched_pixel_count() { - Bitmap::new(2, 2, vec![Color::default(); 3]); - } - - #[test] - fn bitmap_solid_fills_every_pixel() { - let b = Bitmap::solid(3, 2, Color::rgb(9, 9, 9)); - for y in 0..2 { - for x in 0..3 { - assert_eq!(b.pixel(x, y), Some(Color::rgb(9, 9, 9))); - } - } - } - - #[test] - fn bitmap_from_file_loads_rgba_png() { - let b = Bitmap::from_file("tests/fixtures/rgba_2x2.png").expect("fixture should load"); - assert_eq!((b.width(), b.height()), (2, 2)); - assert_eq!(b.pixel(0, 0), Some(Color::rgba(255, 0, 0, 255))); - assert_eq!(b.pixel(1, 0), Some(Color::rgba(0, 255, 0, 128))); - assert_eq!(b.pixel(0, 1), Some(Color::rgba(0, 0, 255, 255))); - assert_eq!(b.pixel(1, 1), Some(Color::rgba(255, 255, 0, 0))); - } - - #[test] - fn bitmap_from_file_converts_rgb_to_rgba8_with_opaque_default_alpha() { - let b = Bitmap::from_file("tests/fixtures/rgb_2x2.png").expect("fixture should load"); - assert_eq!((b.width(), b.height()), (2, 2)); - // Source has no alpha channel -- every pixel must come out fully opaque (255). - assert_eq!(b.pixel(0, 0), Some(Color::rgba(10, 20, 30, 255))); - assert_eq!(b.pixel(1, 0), Some(Color::rgba(40, 50, 60, 255))); - assert_eq!(b.pixel(0, 1), Some(Color::rgba(70, 80, 90, 255))); - assert_eq!(b.pixel(1, 1), Some(Color::rgba(100, 110, 120, 255))); - } - - #[test] - fn bitmap_from_file_converts_grayscale_to_rgba8() { - let b = Bitmap::from_file("tests/fixtures/grayscale_2x2.png").expect("fixture should load"); - assert_eq!((b.width(), b.height()), (2, 2)); - // Grayscale -> RGB replicates the luminance value across r/g/b, opaque alpha. - assert_eq!(b.pixel(0, 0), Some(Color::rgba(0, 0, 0, 255))); - assert_eq!(b.pixel(1, 0), Some(Color::rgba(85, 85, 85, 255))); - assert_eq!(b.pixel(0, 1), Some(Color::rgba(170, 170, 170, 255))); - assert_eq!(b.pixel(1, 1), Some(Color::rgba(255, 255, 255, 255))); - } - - #[test] - fn bitmap_from_file_missing_file_returns_err_not_panic() { - let result = Bitmap::from_file("tests/fixtures/does_not_exist.png"); - assert!(matches!(result, Err(Error::ImageLoad(_)))); - } - - #[test] - fn bitmap_from_file_malformed_image_returns_err_not_panic() { - let result = Bitmap::from_file("tests/fixtures/malformed.png"); - assert!(matches!(result, Err(Error::ImageLoad(_)))); - } - - #[test] - fn sprite_move_to_updates_position() { - let mut s = Sprite::new(Bitmap::solid(1, 1, Color::rgb(1, 1, 1)), Point::new(0, 0)); - assert_eq!(s.position(), Point::new(0, 0)); - s.move_to(Point::new(5, 7)); - assert_eq!(s.position(), Point::new(5, 7)); - } - - #[test] - fn draw_sprite_opaque_pixels_overwrite_background() { - let mut c = Canvas::new(4, 4); - c.set_pixel(1, 1, Color::rgb(50, 50, 50)); // pre-existing background content - let mut sprite = Sprite::new(Bitmap::solid(2, 2, Color::rgb(9, 9, 9)), Point::new(1, 1)); - c.draw_sprite(&mut sprite); - for y in 1..3 { - for x in 1..3 { - assert_eq!(c.pixel(x, y), Some(Color::rgb(9, 9, 9)), "at ({x},{y})"); - } - } - } - - #[test] - fn draw_sprite_transparent_pixels_preserve_background() { - let mut c = Canvas::new(3, 3); - c.set_pixel(1, 1, Color::rgb(50, 50, 50)); // background under the transparent sprite pixel - let bitmap = Bitmap::new( - 1, - 1, - vec![Color::rgba(9, 9, 9, 0)], // fully transparent - ); - let mut sprite = Sprite::new(bitmap, Point::new(1, 1)); - c.draw_sprite(&mut sprite); - // The transparent sprite pixel must not have overwritten the background beneath it. - assert_eq!(c.pixel(1, 1), Some(Color::rgb(50, 50, 50))); - } - - #[test] - fn draw_sprite_clips_to_canvas_bounds_without_panic() { - let mut c = Canvas::new(2, 2); - // Sprite mostly off-canvas to the bottom-right; only its top-left pixel is visible. - let mut sprite = Sprite::new(Bitmap::solid(4, 4, Color::rgb(1, 2, 3)), Point::new(1, 1)); - c.draw_sprite(&mut sprite); - assert_eq!(c.pixel(1, 1), Some(Color::rgb(1, 2, 3))); - assert_eq!(c.pixel(0, 0), Some(Color::default())); - } - - #[test] - fn draw_sprite_negative_position_does_not_panic() { - let mut c = Canvas::new(2, 2); - // Sprite anchored off-canvas to the top-left; only its bottom-right pixel is visible. - let mut sprite = Sprite::new(Bitmap::solid(2, 2, Color::rgb(4, 5, 6)), Point::new(-1, -1)); - c.draw_sprite(&mut sprite); - assert_eq!(c.pixel(0, 0), Some(Color::rgb(4, 5, 6))); - assert_eq!(c.pixel(1, 1), Some(Color::default())); - } - - #[test] - fn draw_sprite_move_and_redraw_restores_old_footprint() { - let mut c = Canvas::new(5, 1); - c.set_pixel(0, 0, Color::rgb(50, 50, 50)); // pre-existing background at the sprite's start - let mut sprite = Sprite::new(Bitmap::solid(1, 1, Color::rgb(9, 9, 9)), Point::new(0, 0)); - c.draw_sprite(&mut sprite); - assert_eq!(c.pixel(0, 0), Some(Color::rgb(9, 9, 9))); - - sprite.move_to(Point::new(4, 0)); - c.draw_sprite(&mut sprite); - // Old position must be restored to what it was before the sprite was ever drawn - // there — not left painted with the sprite's color. - assert_eq!(c.pixel(0, 0), Some(Color::rgb(50, 50, 50))); - // New position now shows the sprite. - assert_eq!(c.pixel(4, 0), Some(Color::rgb(9, 9, 9))); - } - - #[test] - fn draw_sprite_redraw_at_same_position_is_a_noop_change() { - let mut c = Canvas::new(3, 1); - let mut sprite = Sprite::new(Bitmap::solid(1, 1, Color::rgb(7, 7, 7)), Point::new(1, 0)); - c.draw_sprite(&mut sprite); - c.draw_sprite(&mut sprite); // redraw without moving - assert_eq!(c.pixel(1, 0), Some(Color::rgb(7, 7, 7))); - } - - #[test] - fn draw_sprite_clone_has_no_drawing_history() { - let mut c = Canvas::new(3, 1); - let mut original = Sprite::new(Bitmap::solid(1, 1, Color::rgb(1, 1, 1)), Point::new(0, 0)); - c.draw_sprite(&mut original); - - // Cloning after drawing must not carry over last_draw -- otherwise drawing the - // clone elsewhere would "restore" the original's footprint out from under it. - let mut clone = original.clone(); - clone.move_to(Point::new(2, 0)); - c.draw_sprite(&mut clone); - - // The original sprite's pixel must be untouched by the clone's draw. - assert_eq!(c.pixel(0, 0), Some(Color::rgb(1, 1, 1))); - assert_eq!(c.pixel(2, 0), Some(Color::rgb(1, 1, 1))); - } - - #[test] - fn draw_sprite_on_a_different_canvas_does_not_leak_the_first_canvas_pixels() { - let mut canvas_a = Canvas::new(2, 1); - let mut sprite = Sprite::new(Bitmap::solid(1, 1, Color::rgb(9, 9, 9)), Point::new(0, 0)); - canvas_a.draw_sprite(&mut sprite); // captures canvas_a's background into last_draw - - let mut canvas_b = Canvas::new(2, 1); - canvas_b.set_pixel(0, 0, Color::rgb(2, 2, 2)); // canvas_b's own distinct background - sprite.move_to(Point::new(1, 0)); - canvas_b.draw_sprite(&mut sprite); - - // The stale footprint captured from canvas_a must not have been restored onto - // canvas_b's position (0,0); canvas_b's own background must be untouched. - assert_eq!(canvas_b.pixel(0, 0), Some(Color::rgb(2, 2, 2))); - assert_eq!(canvas_b.pixel(1, 0), Some(Color::rgb(9, 9, 9))); - } } diff --git a/crates/guiltty-sprite/Cargo.toml b/crates/guiltty-sprite/Cargo.toml new file mode 100644 index 0000000..dbf3de4 --- /dev/null +++ b/crates/guiltty-sprite/Cargo.toml @@ -0,0 +1,17 @@ +[package] +name = "guiltty-sprite" +version = "0.0.0" +publish = false +edition.workspace = true +rust-version.workspace = true +license.workspace = true +authors.workspace = true +repository.workspace = true +description = "Movable sprites (Bitmap/Sprite) drawn onto a guiltty-core Canvas" + +[lints] +workspace = true + +[dependencies] +guiltty-core = { path = "../guiltty-core", version = "0.0.0" } +image = { version = "0.25.10", default-features = false, features = ["png", "jpeg", "gif", "bmp"] } diff --git a/crates/guiltty-sprite/src/lib.rs b/crates/guiltty-sprite/src/lib.rs new file mode 100644 index 0000000..c6cc31b --- /dev/null +++ b/crates/guiltty-sprite/src/lib.rs @@ -0,0 +1,644 @@ +//! Movable sprites: a [`Bitmap`] positioned over a `guiltty-core` [`Canvas`] via +//! [`Sprite`], using save/restore-under so moving and redrawing a sprite doesn't leave a +//! trail of its previous position. Extracted out of `guiltty-core` to keep that crate +//! scoped to the absolute-coordinate drawing surface; see +//! `docs/design/sprite-crate-extraction.md`. + +use guiltty_core::{Canvas, Color, Error, Point, Rect}; + +/// A small RGBA8 image used as sprite content. Structurally similar to `Canvas`'s pixel +/// buffer, but represents drawable material rather than a render target. Can be built +/// in-memory ([`Bitmap::new`]/[`Bitmap::solid`]) or loaded from a file on disk +/// ([`Bitmap::from_file`]). +#[derive(Debug, Clone)] +pub struct Bitmap { + width: u32, + height: u32, + pixels: Vec, +} + +impl Bitmap { + /// Creates a bitmap from an explicit pixel buffer (row-major, RGBA8). + /// + /// # Panics + /// Panics if `pixels.len() != width * height`, or via [`Bitmap::checked_len`] if + /// `width`/`height` don't fit in `usize` on the current target, or if + /// `width * height` overflows `usize`. + pub fn new(width: u32, height: u32, pixels: Vec) -> Self { + let expected = Self::checked_len(width, height); + assert_eq!( + pixels.len(), + expected, + "Bitmap::new: pixels.len() ({}) must equal width*height ({})", + pixels.len(), + expected + ); + Self { + width, + height, + pixels, + } + } + + /// Creates a bitmap of the given size, every pixel set to `color`. + pub fn solid(width: u32, height: u32, color: Color) -> Self { + let len = Self::checked_len(width, height); + Self { + width, + height, + pixels: vec![color; len], + } + } + + /// Loads an image file (PNG, JPEG, GIF, or BMP) from `path` and converts it to a + /// bitmap. Every source pixel format (RGB, grayscale, indexed palette, etc.) is + /// converted to this crate's RGBA8 color model: sources without an alpha channel get + /// a fully-opaque (255) default alpha. + /// + /// Returns `Err(Error::ImageLoad(_))` rather than panicking for a missing file, an + /// unsupported format, or malformed/corrupt image data -- per this crate's + /// recoverable-error convention, a bad file on disk is exactly the kind of + /// caller-facing condition that shouldn't panic. + pub fn from_file>(path: P) -> Result { + let img = image::open(path.as_ref()) + .map_err(|e| Error::ImageLoad(format!("{}: {e}", path.as_ref().display())))?; + let rgba = img.into_rgba8(); + let (width, height) = rgba.dimensions(); + let pixels = rgba + .pixels() + .map(|p| Color::rgba(p[0], p[1], p[2], p[3])) + .collect(); + Ok(Self { + width, + height, + pixels, + }) + } + + /// `width * height` as a `usize`, converting each dimension with a checked cast first + /// (rather than a truncating `as usize`) so this can't silently disagree with the + /// `u32` dimensions on a target where `usize` is narrower than `u32`. + fn checked_len(width: u32, height: u32) -> usize { + let w: usize = width.try_into().expect("Bitmap width too large for usize"); + let h: usize = height + .try_into() + .expect("Bitmap height too large for usize"); + w.checked_mul(h) + .expect("Bitmap dimensions too large: width * height overflows usize") + } + + /// Bitmap width in pixels. + pub fn width(&self) -> u32 { + self.width + } + + /// Bitmap height in pixels. + pub fn height(&self) -> u32 { + self.height + } + + /// Row-major pixel index for `(x, y)`, or `None` if out of bounds -- mirrors + /// `Canvas::index`. + fn index(&self, x: u32, y: u32) -> Option { + if x >= self.width || y >= self.height { + return None; + } + Some(y as usize * self.width as usize + x as usize) + } + + /// Returns the color at `(x, y)`, or `None` if out of bounds. + pub fn pixel(&self, x: u32, y: u32) -> Option { + self.index(x, y).map(|i| self.pixels[i]) + } +} + +/// The rectangle of canvas pixels a [`Sprite`] last drew over, saved so +/// [`Sprite::clear_footprint`] can restore them. Tagged with the id of the `Canvas` it +/// was captured from (never restored onto a different canvas instance) and the +/// `region_version` of its own `rect` at capture time (see "Footprint staleness" in +/// `docs/design/sprite-crate-extraction.md`). +#[derive(Debug)] +struct DrawnFootprint { + canvas_id: u64, + rect: Rect, + version: u64, + pixels: Vec, +} + +/// Returned by [`Sprite::clear_footprint`] (and propagated by [`Sprite::draw_on`]) when +/// the canvas has changed, within this footprint's own region, since it was captured. +/// The canvas is left untouched -- no partial restore happens. Recover via +/// [`Sprite::discard_footprint`], which drops the stale footprint (abandoning the +/// sprite's old on-canvas pixels rather than risking restoring stale ones) so the sprite +/// can be placed again. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub struct StaleFootprint; + +impl std::fmt::Display for StaleFootprint { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + write!( + f, + "sprite footprint is stale: canvas changed since it was captured" + ) + } +} + +impl std::error::Error for StaleFootprint {} + +/// A movable 2D bitmap positioned over a canvas. See [`Sprite::draw_on`] for how moving +/// and redrawing a sprite avoids leaving a trail of its previous position. +#[derive(Debug)] +pub struct Sprite { + bitmap: Bitmap, + position: Point, + last_draw: Option, +} + +/// Manually implemented (rather than `#[derive(Clone)]`) so a cloned sprite starts with +/// no drawing history of its own: it copies the bitmap and position, but not +/// `last_draw`, since the clone has never actually been drawn anywhere. Without this, a +/// clone of an already-drawn sprite would restore the *original* sprite's footprint the +/// first time it's drawn, corrupting whatever the original still shows on the canvas. +impl Clone for Sprite { + fn clone(&self) -> Self { + Self { + bitmap: self.bitmap.clone(), + position: self.position, + last_draw: None, + } + } +} + +impl Sprite { + /// Creates a sprite from `bitmap`, placed at `position` (top-left of the bitmap). + pub fn new(bitmap: Bitmap, position: Point) -> Self { + Self { + bitmap, + position, + last_draw: None, + } + } + + /// The sprite's current position. + pub fn position(&self) -> Point { + self.position + } + + /// Moves the sprite to a new position. Takes effect the next time [`Sprite::draw_on`] + /// (or [`Sprite::clear_footprint`]/[`Sprite::place`]) is called -- that restores + /// whatever the sprite covered at its previous position before drawing it at the new + /// one. + pub fn move_to(&mut self, position: Point) { + self.position = position; + } + + /// The sprite's bitmap content. + pub fn bitmap(&self) -> &Bitmap { + &self.bitmap + } + + /// Draws this sprite's bitmap onto `canvas` at its current position, clipped to + /// canvas bounds: `clear_footprint` followed by `place` (see both for what each + /// step does). This is what most callers want -- anything that doesn't need to draw + /// something else in between clearing the old footprint and placing the new one + /// (`guiltty-turtle`'s trail-drawing is the exception; see that crate). + /// + /// Propagates [`StaleFootprint`] from `clear_footprint` without calling `place` -- + /// i.e. on error, the canvas is left completely untouched (no restore, no new draw). + pub fn draw_on(&mut self, canvas: &mut Canvas) -> Result<(), StaleFootprint> { + self.clear_footprint(canvas)?; + self.place(canvas); + Ok(()) + } + + /// Restores the canvas pixels this sprite's last [`Sprite::place`] call covered, + /// clearing `last_draw` on success. + /// + /// - `Ok(())`, a no-op, if the sprite was never drawn, or if its last footprint was + /// captured from a *different* `Canvas` (irrelevant here -- dropped, not restored). + /// - `Err(StaleFootprint)`, canvas left untouched, if this footprint's region has + /// changed (per `canvas.region_version`) since it was captured -- something else + /// wrote into it in the meantime, so restoring would overwrite that write with + /// stale pixels. See "Footprint staleness" in + /// `docs/design/sprite-crate-extraction.md`. + /// - `Ok(())`, restored, otherwise. + pub fn clear_footprint(&mut self, canvas: &mut Canvas) -> Result<(), StaleFootprint> { + let Some(footprint) = self.last_draw.as_ref() else { + return Ok(()); + }; + if footprint.canvas_id != canvas.id() { + self.last_draw = None; + return Ok(()); + } + if canvas.region_version(footprint.rect) != footprint.version { + return Err(StaleFootprint); + } + let footprint = self + .last_draw + .take() + .expect("checked Some above, and canvas_id/region_version both matched"); + Self::restore_footprint(canvas, &footprint); + Ok(()) + } + + /// Captures the canvas pixels currently under this sprite's position, then blits the + /// sprite's bitmap over them (fully transparent bitmap pixels, `alpha == 0`, are + /// skipped rather than overwriting the canvas; non-transparent pixels replace + /// outright -- no alpha blending, no anti-aliasing). Does **not** restore any + /// previous footprint first -- see [`Sprite::clear_footprint`] for that, or + /// [`Sprite::draw_on`] for both together. + /// + /// The captured footprint's `region_version` is read *after* this blit completes, + /// not before -- the blit is itself a canvas write to that same region, so capturing + /// beforehand would make every sprite self-invalidate on its very next + /// `clear_footprint` call. + pub fn place(&mut self, canvas: &mut Canvas) { + let (px, py) = (self.position.x as i64, self.position.y as i64); + let bmp_w = self.bitmap.width as i64; + let bmp_h = self.bitmap.height as i64; + let canvas_w = canvas.width() as i64; + let canvas_h = canvas.height() as i64; + let x_lo = px.max(0); + let x_hi = (px + bmp_w).min(canvas_w); + let y_lo = py.max(0); + let y_hi = (py + bmp_h).min(canvas_h); + let cap_w = (x_hi - x_lo).max(0) as u32; + let cap_h = (y_hi - y_lo).max(0) as u32; + let mut saved = Vec::with_capacity(cap_w as usize * cap_h as usize); + + for y in y_lo..y_hi { + for x in x_lo..x_hi { + let (cx, cy) = (x as u32, y as u32); + saved.push(canvas.pixel(cx, cy).unwrap_or_default()); + let (bx, by) = ((x - px) as u32, (y - py) as u32); + if let Some(color) = self.bitmap.pixel(bx, by) { + if color.a != 0 { + canvas.set_pixel(cx, cy, color); + } + } + } + } + + let rect = Rect::new(x_lo as i32, y_lo as i32, cap_w, cap_h); + let version = canvas.region_version(rect); // after the blit above -- see doc comment + self.last_draw = Some(DrawnFootprint { + canvas_id: canvas.id(), + rect, + version, + pixels: saved, + }); + } + + /// Drops this sprite's saved footprint, if any, without attempting to restore it -- + /// the escape hatch out of a permanently-stale footprint (`Canvas`'s underlying + /// version only increases, so a `clear_footprint` that once returned + /// `Err(StaleFootprint)` never stops doing so on retry). The sprite's previous + /// on-canvas pixels are abandoned as-is -- a visible artifact, not cleaned up -- but + /// the sprite becomes drawable again via [`Sprite::place`]/[`Sprite::draw_on`]. + pub fn discard_footprint(&mut self) { + self.last_draw = None; + } + + /// Restores the canvas pixels a footprint covered, clipped to whatever part of it + /// still falls within current canvas bounds. Goes through `Canvas`'s public + /// `set_pixel` (no direct pixel-buffer access available from this crate). + fn restore_footprint(canvas: &mut Canvas, footprint: &DrawnFootprint) { + let canvas_w = canvas.width() as i64; + let canvas_h = canvas.height() as i64; + let row_len = footprint.rect.width as usize; + for dy in 0..footprint.rect.height as i64 { + let y = footprint.rect.y as i64 + dy; + if y < 0 || y >= canvas_h { + continue; + } + let row = dy as usize * row_len; + for dx in 0..footprint.rect.width as i64 { + let x = footprint.rect.x as i64 + dx; + if x < 0 || x >= canvas_w { + continue; + } + canvas.set_pixel(x as u32, y as u32, footprint.pixels[row + dx as usize]); + } + } + } +} + +#[cfg(test)] +mod tests { + use super::*; + use guiltty_core::{Fill, Shape}; + + #[test] + fn bitmap_new_and_pixel_roundtrip() { + let b = Bitmap::new( + 2, + 2, + vec![ + Color::rgb(1, 0, 0), + Color::rgb(2, 0, 0), + Color::rgb(3, 0, 0), + Color::rgb(4, 0, 0), + ], + ); + assert_eq!(b.width(), 2); + assert_eq!(b.height(), 2); + assert_eq!(b.pixel(0, 0), Some(Color::rgb(1, 0, 0))); + assert_eq!(b.pixel(1, 1), Some(Color::rgb(4, 0, 0))); + assert_eq!(b.pixel(2, 0), None); + } + + #[test] + #[should_panic(expected = "must equal width*height")] + fn bitmap_new_panics_on_mismatched_pixel_count() { + Bitmap::new(2, 2, vec![Color::default(); 3]); + } + + #[test] + fn bitmap_solid_fills_every_pixel() { + let b = Bitmap::solid(3, 2, Color::rgb(9, 9, 9)); + for y in 0..2 { + for x in 0..3 { + assert_eq!(b.pixel(x, y), Some(Color::rgb(9, 9, 9))); + } + } + } + + #[test] + fn bitmap_from_file_loads_rgba_png() { + let b = Bitmap::from_file("tests/fixtures/rgba_2x2.png").expect("fixture should load"); + assert_eq!((b.width(), b.height()), (2, 2)); + assert_eq!(b.pixel(0, 0), Some(Color::rgba(255, 0, 0, 255))); + assert_eq!(b.pixel(1, 0), Some(Color::rgba(0, 255, 0, 128))); + assert_eq!(b.pixel(0, 1), Some(Color::rgba(0, 0, 255, 255))); + assert_eq!(b.pixel(1, 1), Some(Color::rgba(255, 255, 0, 0))); + } + + #[test] + fn bitmap_from_file_converts_rgb_to_rgba8_with_opaque_default_alpha() { + let b = Bitmap::from_file("tests/fixtures/rgb_2x2.png").expect("fixture should load"); + assert_eq!((b.width(), b.height()), (2, 2)); + assert_eq!(b.pixel(0, 0), Some(Color::rgba(10, 20, 30, 255))); + assert_eq!(b.pixel(1, 0), Some(Color::rgba(40, 50, 60, 255))); + assert_eq!(b.pixel(0, 1), Some(Color::rgba(70, 80, 90, 255))); + assert_eq!(b.pixel(1, 1), Some(Color::rgba(100, 110, 120, 255))); + } + + #[test] + fn bitmap_from_file_converts_grayscale_to_rgba8() { + let b = Bitmap::from_file("tests/fixtures/grayscale_2x2.png").expect("fixture should load"); + assert_eq!((b.width(), b.height()), (2, 2)); + assert_eq!(b.pixel(0, 0), Some(Color::rgba(0, 0, 0, 255))); + assert_eq!(b.pixel(1, 0), Some(Color::rgba(85, 85, 85, 255))); + assert_eq!(b.pixel(0, 1), Some(Color::rgba(170, 170, 170, 255))); + assert_eq!(b.pixel(1, 1), Some(Color::rgba(255, 255, 255, 255))); + } + + #[test] + fn bitmap_from_file_missing_file_returns_err_not_panic() { + let result = Bitmap::from_file("tests/fixtures/does_not_exist.png"); + assert!(matches!(result, Err(Error::ImageLoad(_)))); + } + + #[test] + fn bitmap_from_file_malformed_image_returns_err_not_panic() { + let result = Bitmap::from_file("tests/fixtures/malformed.png"); + assert!(matches!(result, Err(Error::ImageLoad(_)))); + } + + #[test] + fn sprite_move_to_updates_position() { + let mut s = Sprite::new(Bitmap::solid(1, 1, Color::rgb(1, 1, 1)), Point::new(0, 0)); + assert_eq!(s.position(), Point::new(0, 0)); + s.move_to(Point::new(5, 7)); + assert_eq!(s.position(), Point::new(5, 7)); + } + + #[test] + fn draw_on_opaque_pixels_overwrite_background() { + let mut c = Canvas::new(4, 4); + c.set_pixel(1, 1, Color::rgb(50, 50, 50)); // pre-existing background content + let mut sprite = Sprite::new(Bitmap::solid(2, 2, Color::rgb(9, 9, 9)), Point::new(1, 1)); + sprite.draw_on(&mut c).expect("first draw always succeeds"); + for y in 1..3 { + for x in 1..3 { + assert_eq!(c.pixel(x, y), Some(Color::rgb(9, 9, 9)), "at ({x},{y})"); + } + } + } + + #[test] + fn draw_on_transparent_pixels_preserve_background() { + let mut c = Canvas::new(3, 3); + c.set_pixel(1, 1, Color::rgb(50, 50, 50)); // background under the transparent sprite pixel + let bitmap = Bitmap::new( + 1, + 1, + vec![Color::rgba(9, 9, 9, 0)], // fully transparent + ); + let mut sprite = Sprite::new(bitmap, Point::new(1, 1)); + sprite.draw_on(&mut c).expect("first draw always succeeds"); + // The transparent sprite pixel must not have overwritten the background beneath it. + assert_eq!(c.pixel(1, 1), Some(Color::rgb(50, 50, 50))); + } + + #[test] + fn draw_on_clips_to_canvas_bounds_without_panic() { + let mut c = Canvas::new(2, 2); + // Sprite mostly off-canvas to the bottom-right; only its top-left pixel is visible. + let mut sprite = Sprite::new(Bitmap::solid(4, 4, Color::rgb(1, 2, 3)), Point::new(1, 1)); + sprite.draw_on(&mut c).expect("first draw always succeeds"); + assert_eq!(c.pixel(1, 1), Some(Color::rgb(1, 2, 3))); + assert_eq!(c.pixel(0, 0), Some(Color::default())); + } + + #[test] + fn draw_on_negative_position_does_not_panic() { + let mut c = Canvas::new(2, 2); + // Sprite anchored off-canvas to the top-left; only its bottom-right pixel is visible. + let mut sprite = Sprite::new(Bitmap::solid(2, 2, Color::rgb(4, 5, 6)), Point::new(-1, -1)); + sprite.draw_on(&mut c).expect("first draw always succeeds"); + assert_eq!(c.pixel(0, 0), Some(Color::rgb(4, 5, 6))); + assert_eq!(c.pixel(1, 1), Some(Color::default())); + } + + #[test] + fn draw_on_move_and_redraw_restores_old_footprint() { + let mut c = Canvas::new(5, 1); + c.set_pixel(0, 0, Color::rgb(50, 50, 50)); // pre-existing background at the sprite's start + let mut sprite = Sprite::new(Bitmap::solid(1, 1, Color::rgb(9, 9, 9)), Point::new(0, 0)); + sprite.draw_on(&mut c).expect("first draw always succeeds"); + assert_eq!(c.pixel(0, 0), Some(Color::rgb(9, 9, 9))); + + sprite.move_to(Point::new(4, 0)); + sprite + .draw_on(&mut c) + .expect("nothing else wrote into the footprint in between"); + // Old position must be restored to what it was before the sprite was ever drawn + // there -- not left painted with the sprite's color. + assert_eq!(c.pixel(0, 0), Some(Color::rgb(50, 50, 50))); + // New position now shows the sprite. + assert_eq!(c.pixel(4, 0), Some(Color::rgb(9, 9, 9))); + } + + #[test] + fn draw_on_redraw_at_same_position_is_a_noop_change() { + let mut c = Canvas::new(3, 1); + let mut sprite = Sprite::new(Bitmap::solid(1, 1, Color::rgb(7, 7, 7)), Point::new(1, 0)); + sprite.draw_on(&mut c).expect("first draw always succeeds"); + sprite.draw_on(&mut c).expect("redraw without moving"); // redraw without moving + assert_eq!(c.pixel(1, 0), Some(Color::rgb(7, 7, 7))); + } + + #[test] + fn draw_on_clone_has_no_drawing_history() { + let mut c = Canvas::new(3, 1); + let mut original = Sprite::new(Bitmap::solid(1, 1, Color::rgb(1, 1, 1)), Point::new(0, 0)); + original + .draw_on(&mut c) + .expect("first draw always succeeds"); + + // Cloning after drawing must not carry over last_draw -- otherwise drawing the + // clone elsewhere would "restore" the original's footprint out from under it. + let mut clone = original.clone(); + clone.move_to(Point::new(2, 0)); + clone + .draw_on(&mut c) + .expect("clone starts with no footprint to restore"); + + // The original sprite's pixel must be untouched by the clone's draw. + assert_eq!(c.pixel(0, 0), Some(Color::rgb(1, 1, 1))); + assert_eq!(c.pixel(2, 0), Some(Color::rgb(1, 1, 1))); + } + + #[test] + fn draw_on_wrong_canvas_is_a_noop_not_a_panic() { + let mut canvas_a = Canvas::new(2, 1); + let mut sprite = Sprite::new(Bitmap::solid(1, 1, Color::rgb(9, 9, 9)), Point::new(0, 0)); + sprite + .draw_on(&mut canvas_a) + .expect("first draw always succeeds"); // captures canvas_a's background into last_draw + + let mut canvas_b = Canvas::new(2, 1); + canvas_b.set_pixel(0, 0, Color::rgb(2, 2, 2)); // canvas_b's own distinct background + sprite.move_to(Point::new(1, 0)); + sprite + .draw_on(&mut canvas_b) + .expect("a footprint from a different canvas is dropped, not an error"); + + // The stale footprint captured from canvas_a must not have been restored onto + // canvas_b's position (0,0); canvas_b's own background must be untouched. + assert_eq!(canvas_b.pixel(0, 0), Some(Color::rgb(2, 2, 2))); + assert_eq!(canvas_b.pixel(1, 0), Some(Color::rgb(9, 9, 9))); + } + + #[test] + fn clear_footprint_immediately_after_place_succeeds() { + // Guards against stamping the footprint's version before place's own blit -- + // that would make the blit self-invalidate the footprint it just created. + let mut c = Canvas::new(3, 3); + c.set_pixel(1, 1, Color::rgb(50, 50, 50)); + let mut sprite = Sprite::new(Bitmap::solid(1, 1, Color::rgb(9, 9, 9)), Point::new(1, 1)); + sprite.place(&mut c); + assert_eq!(c.pixel(1, 1), Some(Color::rgb(9, 9, 9))); + + sprite + .clear_footprint(&mut c) + .expect("no intervening write since place"); + assert_eq!(c.pixel(1, 1), Some(Color::rgb(50, 50, 50))); + } + + #[test] + fn clear_footprint_after_successful_restore_is_a_safe_noop() { + let mut c = Canvas::new(3, 1); + let mut sprite = Sprite::new(Bitmap::solid(1, 1, Color::rgb(7, 7, 7)), Point::new(1, 0)); + sprite.place(&mut c); + sprite + .clear_footprint(&mut c) + .expect("first clear restores successfully"); + // last_draw is now None: a second call has nothing to restore, so it's a safe + // no-op -- not an error, and it must not touch the canvas again. + sprite + .clear_footprint(&mut c) + .expect("clearing an already-cleared sprite is a no-op, not an error"); + } + + #[test] + fn clear_footprint_returns_stale_after_intervening_write() { + let mut c = Canvas::new(4, 4); + let mut sprite = Sprite::new(Bitmap::solid(1, 1, Color::rgb(9, 9, 9)), Point::new(1, 1)); + sprite.place(&mut c); + + // Something else writes into the same region before this sprite clears -- + // standing in for another sprite's trail crossing this one's footprint. + c.set_pixel(1, 1, Color::rgb(3, 3, 3)); + + let result = sprite.clear_footprint(&mut c); + assert_eq!(result, Err(StaleFootprint)); + // The canvas must be left exactly as the intervening write left it -- no partial + // restore of the stale background. + assert_eq!(c.pixel(1, 1), Some(Color::rgb(3, 3, 3))); + } + + #[test] + fn draw_on_propagates_stale_error_without_drawing() { + let mut c = Canvas::new(4, 4); + let mut sprite = Sprite::new(Bitmap::solid(1, 1, Color::rgb(9, 9, 9)), Point::new(1, 1)); + sprite.place(&mut c); + c.set_pixel(1, 1, Color::rgb(3, 3, 3)); // intervening write + + sprite.move_to(Point::new(2, 2)); + let result = sprite.draw_on(&mut c); + assert_eq!(result, Err(StaleFootprint)); + // draw_on must not have called place() after clear_footprint failed: the new + // position must show no sprite pixels, and the old position's intervening write + // must be untouched. + assert_eq!(c.pixel(1, 1), Some(Color::rgb(3, 3, 3))); + assert_eq!(c.pixel(2, 2), Some(Color::default())); + } + + #[test] + fn disjoint_write_does_not_invalidate_footprint() { + let mut c = Canvas::new(200, 200); + let mut sprite = Sprite::new(Bitmap::solid(1, 1, Color::rgb(9, 9, 9)), Point::new(1, 1)); + sprite.place(&mut c); + + // Write far away -- a different tile in the region-version grid -- standing in + // for a second, independent sprite/turtle moving elsewhere on a shared canvas. + c.set_pixel(190, 190, Color::rgb(3, 3, 3)); + + sprite + .clear_footprint(&mut c) + .expect("a disjoint write must not invalidate this footprint"); + } + + #[test] + fn discard_footprint_recovers_after_stale() { + let mut c = Canvas::new(4, 4); + let mut sprite = Sprite::new(Bitmap::solid(1, 1, Color::rgb(9, 9, 9)), Point::new(1, 1)); + sprite.place(&mut c); + c.set_pixel(1, 1, Color::rgb(3, 3, 3)); // makes the footprint permanently stale + + assert_eq!(sprite.clear_footprint(&mut c), Err(StaleFootprint)); + sprite.discard_footprint(); + // The sprite is drawable again -- place() no longer has a footprint to consult. + sprite.move_to(Point::new(2, 2)); + sprite.place(&mut c); + assert_eq!(c.pixel(2, 2), Some(Color::rgb(9, 9, 9))); + } + + #[test] + fn draw_shape_touching_a_sprites_footprint_makes_it_stale() { + // Not just set_pixel: any pixel-mutating Canvas call (draw_shape here) must + // participate in the same region-version tracking. + let mut c = Canvas::new(4, 4); + let mut sprite = Sprite::new(Bitmap::solid(2, 2, Color::rgb(9, 9, 9)), Point::new(0, 0)); + sprite.place(&mut c); + + c.draw_shape( + &Shape::line(Point::new(0, 0), Point::new(3, 0)), + Fill::solid(Color::rgb(3, 3, 3)), + ); + + assert_eq!(sprite.clear_footprint(&mut c), Err(StaleFootprint)); + } +} diff --git a/crates/guiltty-core/tests/fixtures/grayscale_2x2.png b/crates/guiltty-sprite/tests/fixtures/grayscale_2x2.png similarity index 100% rename from crates/guiltty-core/tests/fixtures/grayscale_2x2.png rename to crates/guiltty-sprite/tests/fixtures/grayscale_2x2.png diff --git a/crates/guiltty-core/tests/fixtures/malformed.png b/crates/guiltty-sprite/tests/fixtures/malformed.png similarity index 100% rename from crates/guiltty-core/tests/fixtures/malformed.png rename to crates/guiltty-sprite/tests/fixtures/malformed.png diff --git a/crates/guiltty-core/tests/fixtures/rgb_2x2.png b/crates/guiltty-sprite/tests/fixtures/rgb_2x2.png similarity index 100% rename from crates/guiltty-core/tests/fixtures/rgb_2x2.png rename to crates/guiltty-sprite/tests/fixtures/rgb_2x2.png diff --git a/crates/guiltty-core/tests/fixtures/rgba_2x2.png b/crates/guiltty-sprite/tests/fixtures/rgba_2x2.png similarity index 100% rename from crates/guiltty-core/tests/fixtures/rgba_2x2.png rename to crates/guiltty-sprite/tests/fixtures/rgba_2x2.png diff --git a/crates/guiltty/Cargo.toml b/crates/guiltty/Cargo.toml index e1a537d..1bc13f1 100644 --- a/crates/guiltty/Cargo.toml +++ b/crates/guiltty/Cargo.toml @@ -14,4 +14,5 @@ workspace = true [dependencies] guiltty-core = { path = "../guiltty-core", version = "0.0.0" } +guiltty-sprite = { path = "../guiltty-sprite", version = "0.0.0" } guiltty-kitty = { path = "../guiltty-kitty", version = "0.0.0" } diff --git a/crates/guiltty/src/lib.rs b/crates/guiltty/src/lib.rs index 2cead8d..6b778d9 100644 --- a/crates/guiltty/src/lib.rs +++ b/crates/guiltty/src/lib.rs @@ -1,7 +1,6 @@ //! Facade crate: re-exports the core API and the default (kitty) backend //! for consumers of guiltty. -pub use guiltty_core::{ - Backend, Bitmap, Canvas, Color, Error, Fill, Point, Rect, Shape, Sprite, TextStyle, -}; +pub use guiltty_core::{Backend, Canvas, Color, Error, Fill, Point, Rect, Shape, TextStyle}; pub use guiltty_kitty::KittyBackend; +pub use guiltty_sprite::{Bitmap, Sprite, StaleFootprint}; diff --git a/examples/src/bin/demo.rs b/examples/src/bin/demo.rs index b012946..8cef470 100644 --- a/examples/src/bin/demo.rs +++ b/examples/src/bin/demo.rs @@ -67,7 +67,9 @@ fn main() { // now that Bitmap::from_file exists. let bitmap = Bitmap::from_file(SPRITE_PNG).expect("bundled sprite asset should load"); let mut sprite = Sprite::new(bitmap, Point::new(10, 140)); - canvas.draw_sprite(&mut sprite); + sprite + .draw_on(&mut canvas) + .expect("first draw always succeeds"); let mut backend = KittyBackend::new(); backend @@ -78,7 +80,9 @@ fn main() { // in-memory move wouldn't actually prove movement or background preservation, so // both frames are required (see this task's own acceptance criteria). sprite.move_to(Point::new(370, 140)); - canvas.draw_sprite(&mut sprite); + sprite + .draw_on(&mut canvas) + .expect("nothing else wrote into the footprint in between"); backend .present(&canvas) .expect("second present should succeed"); From 6dae95a5e31d2ca5a4742b81cc453a09da162ad8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Rog=C3=A9rio=20Senna=20=5Bmmm1=5D?= Date: Sat, 1 Aug 2026 04:17:42 +0200 Subject: [PATCH 2/2] fix: region-scoped tile grid bugs from PR #38 bot review Four real bugs found by review bots in the initial version: - Canvas::new forced tile counts to >=1 per axis, so a canvas with one zero dimension (e.g. Canvas::new(0, u32::MAX)) still allocated a huge tile_versions buffer despite having zero actual pixels -- potential OOM. Tile count is now 0 when the corresponding dimension is 0. - set_pixel touched the region-version grid via a Rect built from u32 coordinates cast to i32 -- silently wraps negative for a canvas wide enough to have valid coordinates past i32::MAX, touching the wrong tile (or none) and letting a footprint appear falsely fresh. Added touch_pixel(x: u32, y: u32), computed directly in u32/i64, no Rect. - draw_shape/draw_text already touch their whole bounding region once up front, but their internal per-pixel helpers all called the public (touching) set_pixel too -- redundant tile-index recomputation and version bumps once per pixel instead of once per call. Split set_pixel into the public touching version and a private set_pixel_raw (write-only), used by stroke_line/fill_*. - draw_text's bbox arithmetic (advance * char count, origin + width) used unchecked i64 math that could overflow for extreme scale/length combinations; switched to saturating_mul/saturating_add since this bbox only feeds touch_region, never pixel addressing. Also: Sprite::place now expects (not unwrap_or_default) its background capture, since an out-of-bounds read there would mean a real Canvas bug, not a state to paper over silently. clear_footprint refactored to take last_draw up front and restore it only on the error path, instead of borrowing then taking late with an unreachable expect(). Path bbox combined from four O(n) passes into one. Added a regression test for the zero-dimension allocation fix. Independently re-verified via pr-review-toolkit:review-pr before pushing -- no further issues found. Co-Authored-By: WOZCODE --- crates/guiltty-core/src/lib.rs | 102 ++++++++++++++++++++++++------- crates/guiltty-sprite/src/lib.rs | 16 ++--- 2 files changed, 90 insertions(+), 28 deletions(-) diff --git a/crates/guiltty-core/src/lib.rs b/crates/guiltty-core/src/lib.rs index 1ab9865..c882813 100644 --- a/crates/guiltty-core/src/lib.rs +++ b/crates/guiltty-core/src/lib.rs @@ -160,8 +160,20 @@ impl Canvas { let len = (width as usize) .checked_mul(height as usize) .expect("Canvas dimensions too large: width * height overflows usize"); - let tiles_x = width.div_ceil(TILE_SIZE).max(1); - let tiles_y = height.div_ceil(TILE_SIZE).max(1); + // A zero-width or zero-height canvas has no pixels and needs no tiles either -- + // forcing at least one tile per axis (e.g. via `.max(1)`) would allocate a huge + // `tile_versions` buffer for a canvas like `Canvas::new(0, u32::MAX)` despite it + // holding zero actual pixels. + let tiles_x = if width == 0 { + 0 + } else { + width.div_ceil(TILE_SIZE) + }; + let tiles_y = if height == 0 { + 0 + } else { + height.div_ceil(TILE_SIZE) + }; Self { id: NEXT_CANVAS_ID.fetch_add(1, std::sync::atomic::Ordering::Relaxed), width, @@ -209,9 +221,21 @@ impl Canvas { /// Sets the color at `(x, y)`. Silently ignores out-of-bounds coordinates — there's /// nothing a caller needs recover from, so this isn't a `Result`. pub fn set_pixel(&mut self, x: u32, y: u32, color: Color) { + self.touch_pixel(x, y); + self.set_pixel_raw(x, y, color); + } + + /// Writes `color` at `(x, y)` without touching the region-version grid. Used + /// internally by this crate's own per-pixel shape/text-drawing helpers + /// (`stroke_line`, `fill_*`, `draw_glyph`), which are always invoked from a + /// `draw_shape`/`draw_text` call that already touched its whole bounding region up + /// front (see `shape_bbox`) -- touching per-pixel on top of that would redundantly + /// recompute tile indices and bump `next_version` once per pixel instead of once + /// per draw call. External callers needing the region-version side effect (e.g. + /// `guiltty-sprite`'s blit/restore) go through the public [`Canvas::set_pixel`]. + fn set_pixel_raw(&mut self, x: u32, y: u32, color: Color) { if let Some(i) = self.index(x, y) { self.pixels[i] = color; - self.touch_region(Rect::new(x as i32, y as i32, 1, 1)); } } @@ -264,6 +288,22 @@ impl Canvas { } } + /// Bumps the tile containing pixel `(x, y)` to a fresh version -- the single-pixel + /// equivalent of `touch_region`, computed directly from `u32` coordinates rather + /// than by building a `Rect` from them: `Rect`'s fields are `i32`, so a coordinate + /// past `i32::MAX` (which a large enough `Canvas` can have even though `Rect` can't + /// represent it) would silently wrap negative and touch the wrong tile, or none. A + /// no-op if `(x, y)` is out of bounds. + fn touch_pixel(&mut self, x: u32, y: u32) { + if x >= self.width || y >= self.height { + return; + } + self.next_version += 1; + let version = self.next_version; + let (tx, ty) = (x / TILE_SIZE, y / TILE_SIZE); + self.tile_versions[(ty * self.tiles_x + tx) as usize] = version; + } + /// Clips `region` to this canvas's bounds (in `i64`, mirroring the rest of this /// file's overflow-safe clipping idiom) and converts the result to an inclusive tile /// index range `(tx_lo, tx_hi, ty_lo, ty_hi)`. `None` if the clipped region is empty @@ -323,16 +363,19 @@ impl Canvas { ) } Shape::Path { points, .. } => { - if points.is_empty() { - (0, 0, 0, 0) - } else { - ( - points.iter().map(|p| p.x as i64).min().unwrap(), - points.iter().map(|p| p.y as i64).min().unwrap(), - points.iter().map(|p| p.x as i64).max().unwrap() + 1, - points.iter().map(|p| p.y as i64).max().unwrap() + 1, - ) + let Some(first) = points.first() else { + return Self::rect_from_i64_bounds(0, 0, 0, 0); + }; + let (mut x_lo, mut y_lo) = (first.x as i64, first.y as i64); + let (mut x_hi, mut y_hi) = (x_lo, y_lo); + for p in &points[1..] { + let (x, y) = (p.x as i64, p.y as i64); + x_lo = x_lo.min(x); + y_lo = y_lo.min(y); + x_hi = x_hi.max(x); + y_hi = y_hi.max(y); } + (x_lo, y_lo, x_hi + 1, y_hi + 1) } }; Self::rect_from_i64_bounds(x_lo, y_lo, x_hi, y_hi) @@ -380,13 +423,19 @@ impl Canvas { let origin_y = origin.y as i64; let mut cursor_x = origin.x as i64; - let text_width = advance * text.chars().count() as i64; - let text_height = font::GLYPH_HEIGHT as i64 * scale; + // Saturating, not plain, arithmetic: this bounding rect is only ever fed to + // touch_region (an approximate, conservative tile-touch input -- see + // rect_from_i64_bounds), never used to actually address pixels, so it's fine + // (and preferable) for a pathological scale/text-length combination to saturate + // to i64::MAX rather than overflow. + let char_count = i64::try_from(text.chars().count()).unwrap_or(i64::MAX); + let text_width = advance.saturating_mul(char_count); + let text_height = (font::GLYPH_HEIGHT as i64).saturating_mul(scale); self.touch_region(Self::rect_from_i64_bounds( origin.x as i64, origin_y, - origin.x as i64 + text_width, - origin_y + text_height, + (origin.x as i64).saturating_add(text_width), + origin_y.saturating_add(text_height), )); for ch in text.chars() { @@ -660,7 +709,7 @@ impl Canvas { let mut err = dx + dy; loop { if x0 >= 0 && y0 >= 0 { - self.set_pixel(x0 as u32, y0 as u32, color); + self.set_pixel_raw(x0 as u32, y0 as u32, color); } if x0 == x1 && y0 == y1 { break; @@ -706,7 +755,7 @@ impl Canvas { fn fill_clipped_rect(&mut self, x_lo: i64, x_hi: i64, y_lo: i64, y_hi: i64, color: Color) { for y in y_lo..y_hi { for x in x_lo..x_hi { - self.set_pixel(x as u32, y as u32, color); + self.set_pixel_raw(x as u32, y as u32, color); } } } @@ -738,7 +787,7 @@ impl Canvas { for x in x_lo..=x_hi { let dxr = (x - cx) as f64 / rxf; if dxr * dxr + dyr * dyr <= 1.0 { - self.set_pixel(x as u32, y as u32, color); + self.set_pixel_raw(x as u32, y as u32, color); } } } @@ -759,7 +808,7 @@ impl Canvas { for y in min_y..=max_y { for x in min_x..=max_x { if point_in_triangle(x, y, a, b, c) { - self.set_pixel(x as u32, y as u32, color); + self.set_pixel_raw(x as u32, y as u32, color); } } } @@ -830,7 +879,7 @@ impl Canvas { let x_lo = ((pair[0] - 0.5).ceil() as i64).max(0); let x_hi = ((pair[1] - 0.5).ceil() as i64).min(canvas_w); for x in x_lo..x_hi { - self.set_pixel(x as u32, y as u32, color); + self.set_pixel_raw(x as u32, y as u32, color); } } } @@ -953,6 +1002,17 @@ mod tests { ); } + #[test] + fn canvas_new_zero_width_does_not_allocate_a_huge_tile_grid() { + // Regression: forcing tile counts to at least 1 per axis (`.max(1)`) made a + // zero-width canvas with a huge height allocate ~134M tile-version counters + // despite having zero actual pixels. This must return promptly with no + // allocation anywhere near that size. + let c = Canvas::new(0, u32::MAX); + assert_eq!((c.width(), c.height()), (0, u32::MAX)); + assert_eq!(c.pixel(0, 0), None); // no pixels exist at all + } + #[test] fn canvas_new_is_fully_transparent() { let c = Canvas::new(4, 3); diff --git a/crates/guiltty-sprite/src/lib.rs b/crates/guiltty-sprite/src/lib.rs index c6cc31b..f5c78a4 100644 --- a/crates/guiltty-sprite/src/lib.rs +++ b/crates/guiltty-sprite/src/lib.rs @@ -223,20 +223,18 @@ impl Sprite { /// `docs/design/sprite-crate-extraction.md`. /// - `Ok(())`, restored, otherwise. pub fn clear_footprint(&mut self, canvas: &mut Canvas) -> Result<(), StaleFootprint> { - let Some(footprint) = self.last_draw.as_ref() else { + let Some(footprint) = self.last_draw.take() else { return Ok(()); }; if footprint.canvas_id != canvas.id() { - self.last_draw = None; + // last_draw is already None (taken above) -- correct final state for a + // footprint that has nothing to do with this canvas; nothing to restore. return Ok(()); } if canvas.region_version(footprint.rect) != footprint.version { + self.last_draw = Some(footprint); // error path must leave last_draw untouched return Err(StaleFootprint); } - let footprint = self - .last_draw - .take() - .expect("checked Some above, and canvas_id/region_version both matched"); Self::restore_footprint(canvas, &footprint); Ok(()) } @@ -269,7 +267,11 @@ impl Sprite { for y in y_lo..y_hi { for x in x_lo..x_hi { let (cx, cy) = (x as u32, y as u32); - saved.push(canvas.pixel(cx, cy).unwrap_or_default()); + saved.push( + canvas + .pixel(cx, cy) + .expect("canvas pixel out of bounds in Sprite::place"), + ); let (bx, by) = ((x - px) as u32, (y - py) as u32); if let Some(color) = self.bitmap.pixel(bx, by) { if color.a != 0 {