You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
Moves Sprite/Bitmap out of guiltty-core into a new guiltty-sprite crate. Canvas::draw_sprite is gone; the replacement is sprite.draw_on(&mut canvas). This is a breaking public-API change (pre-1.0, no compatibility shim, matching the project's existing precedent for this kind of change) — guiltty's facade re-exports Bitmap/Sprite from the new location so guiltty::Sprite etc. keep working, and the one in-repo call site (examples/src/bin/demo.rs) is updated.
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).
Sprite exposes draw_on/clear_footprint/place/discard_footprint. clear_footprint now returns Err(StaleFootprint) (canvas left untouched) if anything wrote into its footprint's own region since it was captured, instead of silently restoring stale pixels over newer drawing — the bug the design doc's "Footprint staleness" section exists to fix. discard_footprint recovers a footprint that's gone permanently stale (the version counter only increases, so a bare retry can never succeed) by dropping it without attempting a restore.
Region-scoping (a per-tile grid, not one canvas-wide counter) is what lets two sprites drawing in disjoint areas avoid spuriously invalidating each other — a real bug caught in this design's own review process before implementation started.
Test plan
Canvas::draw_sprite's existing tests (transparency, clipping, same-canvas move, cross-canvas) ported to guiltty-sprite as draw_on_* tests
New coverage for the two bugs an earlier draft of this design got wrong: stamp-after-blit self-invalidation, and canvas-wide (vs. region-scoped) invalidation breaking disjoint sprites
Wrong-canvas-id is a no-op (not a panic, not an error) — the case an earlier draft of this design mistakenly specified as Err
discard_footprint recovery path after a permanently-stale footprint
cargo fmt --all -- --check, cargo clippy --workspace --all-targets -- -D warnings, cargo test --workspace all pass
Extract sprite and bitmap functionality from guiltty-core into a dedicated guiltty-sprite crate, introduce region-scoped canvas version tracking to detect and avoid restoring stale sprite footprints, and update the public facade and example to use the new Sprite::draw_on-based API.
New Features:
Introduce guiltty-sprite crate providing Bitmap/Sprite types and sprite drawing APIs built on guiltty-core Canvas
Add region-scoped footprint staleness handling via Sprite::clear_footprint/draw_on returning StaleFootprint when the canvas changes under a sprite
Enhancements:
Extend Canvas with id() and region_version(Rect) backed by a per-tile version grid updated by pixel-mutating operations to support footprint staleness detection
Update text and shape drawing to participate in region version tracking so all canvas writes are visible to sprites
Adjust the guiltty facade to re-export sprites/bitmaps from the new guiltty-sprite crate
Tests:
Add extensive tests for guiltty-sprite covering bitmap loading, sprite movement/drawing, footprint restore semantics, wrong-canvas handling, and staleness scenarios
Port existing Canvas::draw_sprite tests to the new Sprite::draw_on API and expand coverage for region-scoped invalidation behaviour
Summary by cubic
Extracted guiltty-sprite with Bitmap/Sprite, and added region-scoped versioning in guiltty-core to detect and prevent restoring stale pixels. Also fixed tile-grid correctness and performance issues found in review.
New Features
New guiltty-sprite crate with Sprite::draw_on/clear_footprint/place/discard_footprint and StaleFootprint.
guiltty-core: Canvas::id() and Canvas::region_version(Rect) using per-tile counters; shape/text drawing participate in versioning.
guiltty facade re-exports Bitmap, Sprite, StaleFootprint; tests moved and expanded.
Breaking: replace canvas.draw_sprite(&mut sprite) with sprite.draw_on(&mut canvas) and handle Result<(), StaleFootprint>.
Bug Fixes
Zero-dimension canvases no longer allocate huge tile grids.
Accurate versioning: added touch_pixel (no i32 wrap) and set_pixel_raw so draws bump the grid once per call, not per pixel.
Safer text bounds: draw_text uses saturating math; path bbox computed in one pass.
More robust sprite restore: clear_footprint leaves state untouched on staleness; place validates background capture.
Written for commit 6dae95a. Summary will update on new commits.
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 <contact@withwoz.com>
Extracts Bitmap/Sprite logic from guiltty-core into a new guiltty-sprite crate, introduces region-scoped canvas version tracking to detect stale sprite footprints, and updates the public API and demo to use Sprite::draw_on with staleness handling.
File-Level Changes
Change
Details
Files
Extract Bitmap and Sprite types and sprite drawing logic from guiltty-core into a new guiltty-sprite crate, with a new Sprite::draw_on API and footprint lifecycle.
Move Bitmap struct and its constructors/file-loading helpers into guiltty-sprite, keeping behavior and tests equivalent.
Move Sprite struct, manual Clone, and sprite movement semantics into guiltty-sprite, still backed by save-under footprints.
Replace Canvas::draw_sprite with Sprite::draw_on/clear_footprint/place/discard_footprint methods that operate via Canvas’s public API.
Handle wrong-canvas footprints by dropping them as a no-op instead of restoring onto a different Canvas.
Update example demo to call sprite.draw_on(&mut canvas) and handle the Result instead of Canvas::draw_sprite.
Wire guiltty-sprite into the workspace and facade crate, re-exporting Bitmap, Sprite, and StaleFootprint from guiltty.
Trigger a new review: Comment @sourcery-ai review on the pull request.
Continue discussions: Reply directly to Sourcery's review comments.
Generate a GitHub issue from a review comment: Ask Sourcery to create an
issue from a review comment by replying to it. You can also reply to a
review comment with @sourcery-ai issue to create an issue from it.
Generate a pull request title: Write @sourcery-ai anywhere in the pull
request title to generate a title at any time. You can also comment @sourcery-ai title on the pull request to (re-)generate the title at any time.
Generate a pull request summary: Write @sourcery-ai summary anywhere in
the pull request body to generate a PR summary at any time exactly where you
want it. You can also comment @sourcery-ai summary on the pull request to
(re-)generate the summary at any time.
Generate reviewer's guide: Comment @sourcery-ai guide on the pull
request to (re-)generate the reviewer's guide at any time.
Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
pull request to resolve all Sourcery comments. Useful if you've already
addressed all the comments and don't want to see them anymore.
Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
request to dismiss all existing Sourcery reviews. Especially useful if you
want to start fresh with a new review - don't forget to comment @sourcery-ai review to trigger a new review!
The change extracts bitmap and sprite APIs into guiltty-sprite, adds tile-based canvas version tracking for stale footprint detection, updates facade exports, and migrates the demo to Sprite::draw_on.
Canvas now exposes instance IDs and region versions. Pixel, text, and shape mutations update conservative tile regions.
Bitmap and sprite crate crates/guiltty-core/src/lib.rs, crates/guiltty-sprite/Cargo.toml, crates/guiltty-sprite/src/lib.rs
Bitmap and sprite APIs move to guiltty-sprite. The crate adds image loading, clipped rendering, footprint restoration, movement, and stale-footprint errors.
rsenna/guiltty#37: Directly covers the sprite-crate extraction and Canvas version tracking.
rsenna/guiltty#6: Introduces the sprite implementation that this change relocates and extends.
rsenna/guiltty#23: Adds bitmap loading that this change moves into guiltty-sprite.
Suggested reviewers:owkwo-bot
Sequence Diagram(s)
sequenceDiagram
participant Sprite
participant Bitmap
participant Canvas
Sprite->>Bitmap: Read pixel data
Sprite->>Canvas: Save footprint and draw opaque pixels
Canvas-->>Sprite: Return region version
Sprite->>Canvas: Validate footprint before clearing
Canvas-->>Sprite: Restore pixels or return StaleFootprint
Loading🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
Check name
Status
Explanation
Resolution
Docstring Coverage
⚠️ Warning
Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.
Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name
Status
Explanation
Linked Issues check
✅ Passed
Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check
✅ Passed
Check skipped because no linked issues were found for this pull request.
Description Check
✅ Passed
Check skipped - CodeRabbit’s high-level summary is enabled.
Title check
✅ Passed
The title clearly identifies the sprite crate extraction and region-scoped footprint staleness changes.
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
Create stacked PR
Commit on current branch
🧪 Generate unit tests (beta)
Create PR with unit tests
Commit unit tests in branch extract-guiltty-sprite
Comment @coderabbitai help to get the list of available commands.
The reason will be displayed to describe this comment to others. Learn more.
Hey - I've found 1 issue, and left some high level feedback:
In Canvas::draw_text, text.chars().count() is used solely for computing the bounding box; consider caching the iterator length or using a more efficient metric to avoid iterating the string twice (once for the bbox and again in the drawing loop).
In Sprite::place, you always iterate over coordinates that are already clipped to the canvas bounds, so canvas.pixel(...).unwrap_or_default() and Bitmap::pixel checks add unnecessary overhead; you could use direct indexing into the underlying buffers (or a dedicated fast path on Canvas) to reduce per-pixel cost for sprite blitting.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments- In `Canvas::draw_text`, `text.chars().count()` is used solely for computing the bounding box; consider caching the iterator length or using a more efficient metric to avoid iterating the string twice (once for the bbox and again in the drawing loop).
- In `Sprite::place`, you always iterate over coordinates that are already clipped to the canvas bounds, so `canvas.pixel(...).unwrap_or_default()` and `Bitmap::pixel` checks add unnecessary overhead; you could use direct indexing into the underlying buffers (or a dedicated fast path on `Canvas`) to reduce per-pixel cost for sprite blitting.
## Individual Comments### Comment 1
<locationpath="crates/guiltty-sprite/src/lib.rs"line_range="271-272" />
<code_context>
+ 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) {
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Capturing footprint pixels via `unwrap_or_default` may silently hide canvas bugs.
In `Sprite::place`, `(cx, cy)` is already clipped to canvas bounds, so `canvas.pixel(cx, cy)` should never be `None` if `Canvas` is correct. Using `unwrap_or_default` hides violations of this invariant by recording a default color instead of failing fast, making footprint issues harder to debug. Prefer `unwrap()` or `expect("canvas pixel out of bounds")` to catch such bugs early while keeping the happy-path behavior unchanged.
```suggestion let (cx, cy) = (x as u32, y as u32); saved.push( canvas .pixel(cx, cy) .expect("canvas pixel out of bounds in Sprite::place"), );```
</issue_to_address>
Sourcery is free for open source - if you like our reviews please consider sharing them ✨
• Extract Bitmap/Sprite into new guiltty-sprite crate; replace Canvas::draw_sprite with
sprite.draw_on().
• Add Canvas::id() and region-scoped Canvas::region_version(Rect) to detect stale saved-under
restores.
• Port and expand sprite tests to cover staleness, disjoint regions, and recovery via
discard_footprint.
Diagram
graph TD
A["examples/demo.rs"] --> B["guiltty facade"] --> C["guiltty-sprite crate"] --> D["guiltty-core Canvas"]
D --> F[("Tile version grid")]
C --> E{{"image crate"}}
subgraph Legend
direction LR
_crate["Crate/module"] ~~~ _db[("State store") ] ~~~ _ext{{"External dep"}}
end
Loading
High-Level Assessment
The following are alternative approaches to this PR:
1. Provide a temporary compatibility shim
➕ Eases downstream migration by keeping a draw_sprite-like entrypoint (e.g., free function or extension trait).
➕ Allows phased adoption while still extracting the crate.
➖ Adds API surface that must later be removed (extra churn).
➖ Can obscure the new ownership model (sprite-driven draw) and error handling (StaleFootprint).
2. Canvas-wide version counter instead of per-tile grid
➕ Much simpler implementation and bookkeeping.
➕ Lower memory footprint than a tile grid.
➖ Incorrectly invalidates footprints for disjoint sprites/regions (the bug this PR explicitly fixes).
➖ Encourages callers to work around false positives, reducing usefulness of staleness detection.
➕ More precise invalidation; fewer false positives than tiling.
➕ Could support future optimizations like partial restores.
➖ Significantly higher CPU/memory overhead and implementation complexity.
➖ Harder to reason about and test than tile-based conservative tracking.
Recommendation: The PR’s approach is the best trade-off for correctness and simplicity: extraction keeps guiltty-core focused, and the per-tile region_version grid delivers region-scoped staleness detection without the false invalidation of a canvas-wide counter. Skipping a compatibility shim is consistent with the repo’s pre-1.0 precedent, but reviewers should confirm the breaking change is clearly communicated and that downstream crates have a straightforward migration path (sprite.draw_on(&mut canvas)).
Add Canvas identity + region-version tracking; remove Sprite/Bitmap API
• Introduces 'Canvas::id()' and 'Canvas::region_version(Rect)' backed by a per-tile version grid bumped by pixel-mutating calls ('set_pixel', 'draw_shape', 'draw_text'). Removes the in-core 'Bitmap'/'Sprite' implementation and the inherent 'Canvas::draw_sprite', pushing sprite behavior into the new crate.
lib.rsImplement Sprite/Bitmap with stale-footprint detection and tests+644/-0
Implement Sprite/Bitmap with stale-footprint detection and tests
• Implements 'Bitmap' and 'Sprite' with 'draw_on', 'place', 'clear_footprint', and 'discard_footprint'. Adds 'StaleFootprint' error and enforces region-scoped staleness checks using 'Canvas::id()' + 'Canvas::region_version(Rect)', plus extensive unit tests covering clipping, wrong-canvas no-op, disjoint invalidation, and stale recovery.
lib.rsRe-export Sprite/Bitmap/StaleFootprint from guiltty-sprite+2/-3
Re-export Sprite/Bitmap/StaleFootprint from guiltty-sprite
• Stops re-exporting sprite types from 'guiltty-core' and instead re-exports 'Bitmap', 'Sprite', and 'StaleFootprint' from 'guiltty-sprite' to preserve 'guiltty::Sprite'-style imports.
Cargo.lockAdd guiltty-sprite package to lockfile+9/-3
Add guiltty-sprite package to lockfile
• Adds the new 'guiltty-sprite' workspace crate entry and updates dependencies so 'guiltty' depends on it. Removes 'image' as a direct dependency of 'guiltty-core' now that bitmap loading moved out.
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
The extraction of sprite and bitmap logic into the guiltty-sprite crate is architecturally sound and fulfills the defined acceptance criteria. The implementation of Canvas::region_version successfully enables detection of stale sprite footprints through a per-tile versioning grid. Codacy analysis indicates the project remains up to standards, and all required test scenarios were addressed.
However, a critical performance regression exists in the set_pixel implementation. Currently, every individual pixel write triggers a touch_region call, leading to redundant tile index calculations and version increments. This affects both internal shape drawing in guiltty-core and sprite blitting in guiltty-sprite. These issues should be addressed by introducing bulk-write or internal-write methods that bypass the versioning logic for individual pixels while updating the version once per operation.
Test suggestions
Verify bitmap creation, loading, and pixel retrieval (ported tests)
Sprite movement correctly restores old background and blits at new position
Sprite::clear_footprint returns StaleFootprint error after an intervening pixel write
Disjoint writes (in separate grid tiles) do not cause spurious footprint invalidation
Sprite avoids self-invalidation by capturing version after its own blit
Discarding a footprint allows a sprite to be redrawn after becoming stale
Clearing a footprint on a different canvas instance is a safe no-op
Optional: remove the expect by taking the footprint up front.
clear_footprint borrows last_draw, runs the checks, then re-takes the same value and asserts it is Some. You can take it once and put it back on the stale path. That removes the expect and the second lookup.
♻️ Proposed refactor
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;
return Ok(());
}
if canvas.region_version(footprint.rect) != footprint.version {
+ self.last_draw = Some(footprint); // keep it for `discard_footprint`
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(())
}
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/guiltty-sprite/src/lib.rs` around lines 225 - 242, Refactor
clear_footprint to take last_draw at the start instead of borrowing it and
calling take later. Restore the footprint to last_draw before returning
Err(StaleFootprint), while preserving the existing canvas-id mismatch behavior
and successful restoration flow, and remove the expect assertion.
Avoid per-pixel version stamping inside the internal fill loops.
set_pixel now calls touch_region for every single pixel. The internal rasterizers (fill_clipped_rect, fill_ellipse, fill_triangle, fill_polygon_even_odd, stroke_line, draw_glyph) all write through set_pixel, and draw_shape/draw_text already stamp the whole bounding region before rendering. Each inner-loop pixel therefore pays a tile_range computation, a next_version bump, and a tile-grid write that the bounding-region stamp already covers.
Correctness is unaffected, because versions only increase. The cost is a constant-factor overhead on every rasterizer inner loop.
Consider a private untracked writer used by the internal fills, keeping the public set_pixel tracked.
♻️ Sketch of an untracked internal writer
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));
}
}
++ /// Writes one pixel without stamping the version grid. Only for callers that+ /// already stamped their whole bounding region (`draw_shape`, `draw_text`).+ fn set_pixel_untracked(&mut self, x: u32, y: u32, color: Color) {+ if let Some(i) = self.index(x, y) {+ self.pixels[i] = color;+ }+ }
Then switch the internal fill/stroke/glyph loops to set_pixel_untracked.
Also applies to: 254-265
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/guiltty-core/src/lib.rs` at line 214, Introduce a private untracked
pixel writer alongside set_pixel that performs the same pixel update without
calling touch_region, while preserving set_pixel’s public version-tracking
behavior. Update fill_clipped_rect, fill_ellipse, fill_triangle,
fill_polygon_even_odd, stroke_line, and draw_glyph to use the untracked writer
in their inner loops; keep draw_shape and draw_text bounding-region stamping
unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/guiltty-core/src/lib.rs`:
- Line 214: Introduce a private untracked pixel writer alongside set_pixel that
performs the same pixel update without calling touch_region, while preserving
set_pixel’s public version-tracking behavior. Update fill_clipped_rect,
fill_ellipse, fill_triangle, fill_polygon_even_odd, stroke_line, and draw_glyph
to use the untracked writer in their inner loops; keep draw_shape and draw_text
bounding-region stamping unchanged.
In `@crates/guiltty-sprite/src/lib.rs`:
- Around line 225-242: Refactor clear_footprint to take last_draw at the start
instead of borrowing it and calling take later. Restore the footprint to
last_draw before returning Err(StaleFootprint), while preserving the existing
canvas-id mismatch behavior and successful restoration flow, and remove the
expect assertion.
ℹ️ Review info⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 99678332-7910-4e51-b739-f37f001135ba
📥 Commits
Reviewing files that changed from the base of the PR and between d187745 and 72eb040.
⛔ Files ignored due to path filters (5)
Cargo.lock is excluded by !**/*.lock
crates/guiltty-sprite/tests/fixtures/grayscale_2x2.png is excluded by !**/*.png
crates/guiltty-sprite/tests/fixtures/malformed.png is excluded by !**/*.png
crates/guiltty-sprite/tests/fixtures/rgb_2x2.png is excluded by !**/*.png
crates/guiltty-sprite/tests/fixtures/rgba_2x2.png is excluded by !**/*.png
draw_text now computes text_width = advance * text.chars().count() (and then `origin.x +
text_width) using unchecked i64 arithmetic for the touch_region bounding rect. Large scale`
and/or long text can overflow and panic in debug builds (or wrap in release), causing incorrect
region stamping.
ⓘ Recommendations generated based on similar findings in past PRs
Evidence
The PR added an unchecked i64 multiply for text width as part of the new version-stamp bbox logic,
which can overflow despite the earlier overflow-safety intent for draw_text math.
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution
### Issue description
`Canvas::draw_text` introduced unchecked `i64` multiplication/addition to compute a bounding box for `touch_region`. This can overflow for large inputs, leading to debug panics or wrapped bounds.
### Issue Context
This bbox is only for version tracking, so it’s fine (and preferable) to be conservative: clamp/saturate bounds rather than overflow.
### Fix Focus Areas
- crates/guiltty-core/src/lib.rs[383-390]
### Implementation direction
- Replace `advance * text.chars().count() as i64` with `advance.checked_mul(count_i64).unwrap_or(i64::MAX)` (or `saturating_mul`).
- Similarly, compute `x_hi = origin_x.checked_add(text_width).unwrap_or(i64::MAX)` (or saturating).
- Feed the saturated bounds into `rect_from_i64_bounds` as today, so the touched region remains conservative without panicking.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
2. Per-pixel version bumps✓ Resolved🐞 Bug➹ Performance
Description
Canvas::set_pixel now calls touch_region, but draw_shape/draw_text still render via many
internal set_pixel calls, so a single draw bumps next_version and rewrites tile versions once
per pixel (plus the explicit pre-touch in draw_shape/draw_text). This adds heavy per-pixel
overhead and contradicts the stated intent that versioning is bumped once per pixel-mutating call
(set_pixel, draw_shape, draw_text).
ⓘ Recommendations generated based on similar findings in past PRs
Evidence
The added touch_region call in set_pixel means every internal set_pixel in rendering loops
updates the version grid; meanwhile draw_shape/draw_text also explicitly call touch_region,
creating an additional redundant bump per call.
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution
### Issue description
`Canvas::set_pixel` now calls `touch_region`, but internal drawing helpers (`stroke_line`, `fill_clipped_rect`, polygon fill, etc.) call `set_pixel` for each pixel. This makes `draw_shape` and `draw_text` effectively update the region-version grid O(pixels) times, even though they already call `touch_region` once for their bounding region.
### Issue Context
The PR’s doc/comments state versions should bump once per pixel-mutating *API call* (`set_pixel`, `draw_shape`, `draw_text`), with each call stamping the affected tiles. Right now, internal rendering loops cause many extra bumps and tile stamps.
### Fix Focus Areas
- crates/guiltty-core/src/lib.rs[211-216]
- crates/guiltty-core/src/lib.rs[249-265]
- crates/guiltty-core/src/lib.rs[375-409]
- crates/guiltty-core/src/lib.rs[612-636]
- crates/guiltty-core/src/lib.rs[706-712]
### Implementation direction
- Introduce a private `set_pixel_untracked` (or similar) that only writes `self.pixels[...]`.
- Keep `Canvas::set_pixel` as the tracked public API (calls `touch_region` once for its 1x1 region).
- Update all internal drawing loops (`stroke_line`, `fill_clipped_rect`, ellipse/triangle/polygon fills, glyph rendering) to use the untracked setter.
- Keep `draw_shape`/`draw_text` calling `touch_region` once per call (or move stamping to the end) so each API call bumps versions once.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
3. Coordinate truncation in stamping✓ Resolved🐞 Bug≡ Correctness
Description
set_pixel constructs Rect::new(x as i32, y as i32, 1, 1) for touch_region, so valid u32
coordinates above i32::MAX wrap negative and may be treated as off-canvas by tile_range. This
can silently fail to update tile_versions, breaking region_version/stale-footprint detection on
very large canvases.
ⓘ Recommendations generated based on similar findings in past PRs
Evidence
Rect stores x/y as i32, but set_pixel accepts u32 coordinates and now casts them to i32
when stamping; tile_range then interprets those signed coordinates when clipping, so wrapped
negatives can cause stamping to be skipped/misdirected.
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution
### Issue description
`Canvas::set_pixel` uses `x as i32` / `y as i32` when building the `Rect` passed to `touch_region`. For canvases that allow coordinates beyond `i32::MAX`, this truncates/wraps, so the wrong tile (or no tile) gets stamped.
### Issue Context
This affects only region-version tracking; pixel writes are still correct because indexing is based on `u32`.
### Fix Focus Areas
- crates/guiltty-core/src/lib.rs[41-61]
- crates/guiltty-core/src/lib.rs[211-216]
- crates/guiltty-core/src/lib.rs[267-287]
### Implementation direction
Pick one:
- (Preferred) Add a `touch_pixel(x: u32, y: u32)` / `touch_u32_region(...)` path that computes `(tx, ty)` directly from `u32` coordinates and updates `tile_versions` without going through `Rect`.
- Or enforce/document an invariant in `Canvas::new` that `width`/`height` (and thus valid `x/y`) must be `<= i32::MAX` and panic otherwise, so the cast is safe by construction.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
The reason will be displayed to describe this comment to others. Learn more.
4 issues found across 13 files
Confidence score: 2/5
In crates/guiltty-core/src/lib.rs, version tracking drops valid writes when coordinates exceed i32::MAX, so sprite footprints can look up-to-date and later restore stale pixels over newer drawing — keep tracking in u32 space for canvas coordinates.
In crates/guiltty-core/src/lib.rs, zero-width/zero-height canvases can trigger huge tile_versions allocation because a zero dimension is coerced to one tile, creating an OOM risk from valid input — preserve zero tile counts for zero canvas dimensions.
In crates/guiltty-core/src/lib.rs, large text plus maximal scale can overflow region tracking before clipping, causing debug panics and release-bound wrapping that can corrupt dirty-region behavior — switch these calculations to checked/saturating arithmetic.
In crates/guiltty-core/src/lib.rs, no-op draw calls (blank text, zero-radius shapes, unsupported text) still invalidate sprite footprints due to unconditional pre-stamp version changes, which can cause unnecessary or incorrect restores — only bump versions when pixels are actually written.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="crates/guiltty-core/src/lib.rs">
<violation number="1" location="crates/guiltty-core/src/lib.rs:163">
P2: Constructing a valid zero-height or zero-width canvas can allocate an enormous `tile_versions` buffer and OOM because the zero dimension is forced to one tile. Keep each tile count at zero when its corresponding canvas dimension is zero.</violation>
<violation number="2" location="crates/guiltty-core/src/lib.rs:214">
P1: Version tracking silently misses valid canvas writes above `i32::MAX`, allowing a sprite footprint there to remain apparently fresh and restore stale pixels over intervening drawing. Track the `u32` coordinates directly (or add a u32-coordinate touch helper) instead of converting them to `Rect`'s signed coordinates.</violation>
<violation number="3" location="crates/guiltty-core/src/lib.rs:383">
P2: A sufficiently long string with a maximal scale can overflow this tracking calculation in debug builds (and wrap its region bounds in release) before clipping stops at the canvas edge. Use checked or saturating conversion/multiplication and bounds addition for the tracking rectangle.</violation>
<violation number="4" location="crates/guiltty-core/src/lib.rs:385">
P2: Blank/unsupported text, zero-radius shapes, and other no-op drawing calls can spuriously invalidate a sprite footprint because the unconditional pre-stamp runs without any pixel write. Base version changes on actual writes or guard no-op primitives so `clear_footprint` fails only after a canvas mutation.</violation>
</file>
Reply with feedback, questions, or to request a fix.
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 <contact@withwoz.com>
In Canvas::draw_text, text.chars().count() is used solely for computing the bounding box; consider caching the iterator length or using a more efficient metric to avoid iterating the string twice (once for the bbox and again in the drawing loop).
In Sprite::place, you always iterate over coordinates that are already clipped to the canvas bounds, so canvas.pixel(...).unwrap_or_default() and Bitmap::pixel checks add unnecessary overhead; you could use direct indexing into the underlying buffers (or a dedicated fast path on Canvas) to reduce per-pixel cost for sprite blitting.
Not changing either of these -- both are the same accepted tradeoff, just in two places:
draw_text's double traversal (count, then iterate) is real but the built-in font's strings are always short; not worth the extra complexity for a v0 text renderer.
Sprite::place going through Canvas's public pixel()/set_pixel() instead of direct buffer indexing is a deliberate, documented design decision (docs/design/sprite-crate-extraction.md): guiltty-sprite has no access to Canvas's private pixel buffer by construction (that's the whole point of the crate split), so it pays per-pixel bounds-checked calls instead. The doc calls this out explicitly as something to revisit only if profiling ever shows it matters, not preemptively.
(The unwrap_or_default itself is fixed in 6dae95a -- changed to .expect(...), per your inline comment on that line -- but the underlying per-pixel API-call overhead stays, for the reason above.)
In @crates/guiltty-sprite/src/lib.rs around lines 225 - 242, Refactor clear_footprint to take last_draw at the start instead of borrowing it and calling take later. Restore the footprint to last_draw before returning Err(StaleFootprint), while preserving the existing canvas-id mismatch behavior and successful restoration flow, and remove the expect assertion.
Fixed in 6dae95a, matching this shape closely: clear_footprint now takes last_draw up front, restores it into self.last_draw on the stale-error path (leaving it untouched from the caller's perspective, per the documented contract), and the .expect() is gone. Independently re-verified control-flow equivalence (canvas-id mismatch, stale, success, never-drawn cases) via a pr-review-toolkit:review-pr pass before pushing.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Summary
Implements task 1/2 of
docs/design/sprite-crate-extraction.md:Sprite/Bitmapout ofguiltty-coreinto a newguiltty-spritecrate.Canvas::draw_spriteis gone; the replacement issprite.draw_on(&mut canvas). This is a breaking public-API change (pre-1.0, no compatibility shim, matching the project's existing precedent for this kind of change) —guiltty's facade re-exportsBitmap/Spritefrom the new location soguiltty::Spriteetc. keep working, and the one in-repo call site (examples/src/bin/demo.rs) is updated.guiltty-coregainsCanvas::id()andCanvas::region_version(Rect), backed by a per-tile version grid bumped by every pixel-mutating call (set_pixel,draw_shape,draw_text).Spriteexposesdraw_on/clear_footprint/place/discard_footprint.clear_footprintnow returnsErr(StaleFootprint)(canvas left untouched) if anything wrote into its footprint's own region since it was captured, instead of silently restoring stale pixels over newer drawing — the bug the design doc's "Footprint staleness" section exists to fix.discard_footprintrecovers a footprint that's gone permanently stale (the version counter only increases, so a bare retry can never succeed) by dropping it without attempting a restore.Test plan
Canvas::draw_sprite's existing tests (transparency, clipping, same-canvas move, cross-canvas) ported toguiltty-spriteasdraw_on_*testsErrdiscard_footprintrecovery path after a permanently-stale footprintcargo fmt --all -- --check,cargo clippy --workspace --all-targets -- -D warnings,cargo test --workspaceall passcargo llvm-cov --workspace --fail-under-lines 90: 92.92% overall (guiltty-sprite: 97.32% lines)🧙 Built with WOZCODE
Summary by Sourcery
Extract sprite and bitmap functionality from guiltty-core into a dedicated guiltty-sprite crate, introduce region-scoped canvas version tracking to detect and avoid restoring stale sprite footprints, and update the public facade and example to use the new Sprite::draw_on-based API.
New Features:
Enhancements:
Tests:
Summary by cubic
Extracted
guiltty-spritewithBitmap/Sprite, and added region-scoped versioning inguiltty-coreto detect and prevent restoring stale pixels. Also fixed tile-grid correctness and performance issues found in review.New Features
guiltty-spritecrate withSprite::draw_on/clear_footprint/place/discard_footprintandStaleFootprint.guiltty-core:Canvas::id()andCanvas::region_version(Rect)using per-tile counters; shape/text drawing participate in versioning.guilttyfacade re-exportsBitmap,Sprite,StaleFootprint; tests moved and expanded.canvas.draw_sprite(&mut sprite)withsprite.draw_on(&mut canvas)and handleResult<(), StaleFootprint>.Bug Fixes
touch_pixel(noi32wrap) andset_pixel_rawso draws bump the grid once per call, not per pixel.draw_textuses saturating math; path bbox computed in one pass.clear_footprintleaves state untouched on staleness;placevalidates background capture.Written for commit 6dae95a. Summary will update on new commits.