docs: design turtle geometry and a guiltty-sprite crate extraction - #37
Merged
Conversation
Forward-looking groundwork for iklo, not tied to a v0 success criterion. - sprite-crate-extraction.md: move Sprite/Bitmap out of guiltty-core into a new guiltty-sprite crate, keeping core scoped to absolute-coordinate drawing only. Adds relative (heading/forward/turn) movement to Sprite alongside its existing absolute move_to. - turtle-geometry.md: rewritten to wrap a guiltty-sprite Sprite, adding only pen state -- movement itself is guiltty-sprite's concern.
|
Caution The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased. |
Reviewer's GuideAdds two forward-looking design documents describing the extraction of Sprite/Bitmap into a new guiltty-sprite crate with relative movement APIs, and a guiltty-turtle crate that layers Logo-style turtle geometry and pen state on top of sprites. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
owkwo-bot has reached the 50-credit limit for trial accounts. To continue receiving code reviews, upgrade your plan.
📝 WalkthroughWalkthroughThe PR adds design documents for extracting ChangesSprite and Turtle Geometry
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="docs/design/sprite-crate-extraction.md" line_range="116-117" />
<code_context>
+}
+```
+
+`forward`/`backward` track position as `f32` internally (same
+rounding-drift reasoning as `guiltty-turtle`'s original sketch: many small
+moves compounding integer rounding error is a real problem for both games
+and turtle patterns) and round to `Point`'s `i32` only when the position is
</code_context>
<issue_to_address>
**issue (typo):** Plural subject in the rounding-drift sentence should take "are" instead of "is".
These lines read as a single sentence: "many small moves compounding integer rounding error is a real problem...". Because the subject "many small moves" is plural, the verb should be "are a real problem" rather than "is a real problem."
```suggestion
rounding-drift reasoning as `guiltty-turtle`'s original sketch: many small
moves compounding integer rounding error are a real problem for both games
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Up to standards ✅🟢 Issues
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
docs/design/turtle-geometry.md (1)
40-50: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDefine the contract for
sprite_mut.
Turtlemovement applies pen logic, butsprite_mut()exposesSprite::move_to,Sprite::forward, andSprite::draw_ondirectly. These operations can bypass pen trails. Either restrict mutable access or document that direct sprite operations do not create pen segments. AddTurtle::draw_onif callers need to render the initial sprite without using this escape hatch.Also applies to: 53-60
🤖 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 `@docs/design/turtle-geometry.md` around lines 40 - 50, Document the contract of Turtle::sprite_mut to state that direct Sprite operations, including move_to, forward, and draw_on, bypass Turtle pen-trail handling and do not create pen segments. Add a Turtle::draw_on method for rendering the initial sprite without exposing the escape hatch, and update the related API documentation consistently.
🤖 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.
Inline comments:
In `@docs/design/sprite-crate-extraction.md`:
- Around line 66-79: Update the extraction design around the public API changes
to explicitly classify them as a breaking semver release and document the
required migration, or define a supported compatibility layer that preserves
existing Sprite, Bitmap, and Canvas::draw_sprite usage. Address the affected
call sites and explain how re-exports and the new Sprite::draw_on API handle
compatibility, noting that re-exports alone cannot preserve the inherent Canvas
method.
- Around line 81-82: Clarify the extraction design around Bitmap::from_file by
specifying which error type it returns after moving to the new crate, how
image-loading failures are represented, and which image dependency supplies
them. Define whether callers retain guiltty-core::Error::ImageLoad or migrate to
a new crate-local error, including the required caller impact, before describing
the move as mechanical.
- Around line 87-92: The Sprite API sketch must include a separate exact
floating-point position alongside the integer position. Update Sprite and the
documented new, move_to, position, forward/backward, and drawing behavior to
keep these states synchronized, preserve fractional displacement across repeated
sub-pixel movement, and reset the exact position from the supplied integer point
in move_to.
- Around line 38-41: Update the Sprite forward movement formula to convert
heading_deg from degrees with to_radians() before passing it to sin or cos, and
document that the top-left origin makes positive Y downward and positive turns
clockwise. Add movement tests covering 0° and 90° with assertions for the
expected cardinal positions.
In `@docs/design/turtle-geometry.md`:
- Around line 25-50: Add guiltty-core as a direct dependency of guiltty-turtle
and import Canvas, Color, Point, Shape, and Fill from it, while continuing to
source Sprite and Bitmap from guiltty-sprite. If using re-exports instead,
define and apply those paths consistently throughout the Turtle API.
---
Nitpick comments:
In `@docs/design/turtle-geometry.md`:
- Around line 40-50: Document the contract of Turtle::sprite_mut to state that
direct Sprite operations, including move_to, forward, and draw_on, bypass Turtle
pen-trail handling and do not create pen segments. Add a Turtle::draw_on method
for rendering the initial sprite without exposing the escape hatch, and update
the related API documentation consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e884b3e2-055c-435b-b7a0-dab59a850186
📒 Files selected for processing (2)
docs/design/sprite-crate-extraction.mddocs/design/turtle-geometry.md
![codacy-production[bot]](https://avatars.githubusercontent.com/in/56611?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull Request Overview
The PR provides a solid conceptual foundation for the guiltty-sprite extraction and guiltty-turtle geometry layer. While Codacy results are up to standards, two critical design issues must be resolved before merging. First, the Sprite implementation details contradict the acceptance criteria regarding f32 coordinate tracking, which is essential to prevent rounding drift during accumulated relative movements. Second, the proposed drawing sequence for the Turtle will lead to visual bugs where the sprite's restoration logic overwrites the start of its own trail. These consistency and logic gaps should be addressed to ensure the design is technically sound.
Test suggestions
- Missing recommended test scenario: Verify
Spriterelative movement updates position correctly based on heading and distance using f32 precision. - Missing recommended test scenario: Verify
Turtledraws a line segment on theCanvaswhen moving while the pen is down. - Missing recommended test scenario: Verify
Turtledoes not draw a trail when moving while the pen is up. - Missing recommended test scenario: Verify
Sprite::draw_oncorrectly uses the proposedCanvas::id()to prevent drawing on incorrect canvas instances. - Missing recommended test scenario: Verify that
Spriteabsolute (move_to) and relative (forward) movement methods compose correctly.
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Missing recommended test scenario: Verify `Sprite` relative movement updates position correctly based on heading and distance using f32 precision.
2. Missing recommended test scenario: Verify `Turtle` draws a line segment on the `Canvas` when moving while the pen is down.
3. Missing recommended test scenario: Verify `Turtle` does not draw a trail when moving while the pen is up.
4. Missing recommended test scenario: Verify `Sprite::draw_on` correctly uses the proposed `Canvas::id()` to prevent drawing on incorrect canvas instances.
5. Missing recommended test scenario: Verify that `Sprite` absolute (`move_to`) and relative (`forward`) movement methods compose correctly.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 2 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
- Convert heading degrees to radians in the movement formula, and document the clockwise/top-left-origin convention explicitly. - Stop calling the guiltty-sprite extraction "non-breaking" -- moving Sprite/Bitmap out and replacing Canvas::draw_sprite with sprite.draw_on is a breaking public-API change; document it as such with no compatibility shim planned (pre-1.0, matches T1's precedent). - Define Bitmap::from_file's error ownership after extraction: it keeps returning guiltty_core::Error, not a new crate-local error type. - Fix the Sprite API sketch's position field: exact_position: (f32, f32) is now the one canonical field, with Point only ever a rounded view of it -- resolves the contradiction between the struct (Point) and prose (f32 tracking). - Split Sprite's draw_on into clear_footprint/place primitives, needed to fix a real bug in the turtle drawing sequence: drawing a trail line then calling draw_on restored the sprite's *old* footprint after the line already drew into it, erasing the trail's start on every move. The fix clears first, draws the trail, then places -- with a regression test requirement added to the follow-up. - Clarify guiltty-turtle depends directly on guiltty-core (Canvas, Color, Point, Shape, Fill) in addition to guiltty-sprite, not guiltty-sprite alone.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
docs/design/sprite-crate-extraction.md (1)
78-85: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winMove and extend the sprite drawing tests before removing
Canvas::draw_sprite.Adapt the existing tests for transparency, clipping, same-canvas movement, and cross-canvas drawing to
Sprite::draw_on. Add an interleaving test forclear_footprint/placethat checks underlay capture and restoration order.🤖 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 `@docs/design/sprite-crate-extraction.md` around lines 78 - 85, Move the existing transparency, clipping, same-canvas movement, and cross-canvas drawing tests from Canvas::draw_sprite to Sprite::draw_on, preserving their current assertions. Add an interleaving test covering clear_footprint and place that verifies underlays are captured and restored in the correct order before removing Canvas::draw_sprite.
🤖 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.
Inline comments:
In `@docs/design/sprite-crate-extraction.md`:
- Around line 139-150: Update the clear_footprint/place API description to
specify that clear_footprint restores and consumes last_draw only on the canvas
where it was captured, making repeated clears no-ops; calls with a different
canvas must not restore or consume the saved footprint. State that place
captures and blits the new footprint, replacing any existing last_draw with the
new canvas and footprint.
In `@docs/design/turtle-geometry.md`:
- Around line 53-77: The design must define a mechanism that preserves trails
when one Turtle draws through another Turtle’s footprint, such as a separate
persistent trail layer or deterministic full-scene redraw order. Update the
movement and rendering design around clear_footprint, place, and Turtle state
accordingly, and add a test covering two turtles whose trails cross without
either trail being erased.
---
Nitpick comments:
In `@docs/design/sprite-crate-extraction.md`:
- Around line 78-85: Move the existing transparency, clipping, same-canvas
movement, and cross-canvas drawing tests from Canvas::draw_sprite to
Sprite::draw_on, preserving their current assertions. Add an interleaving test
covering clear_footprint and place that verifies underlays are captured and
restored in the correct order before removing Canvas::draw_sprite.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 20251ef9-cfe9-4032-b035-3283349eb9bd
📒 Files selected for processing (2)
docs/design/sprite-crate-extraction.mddocs/design/turtle-geometry.md
clear_footprint restored a captured snapshot with no check that the canvas had changed since capture -- a second clear, or another sprite's trail drawn through this one's footprint, would silently blit stale pixels back over newer drawing. Add a monotonic Canvas::version() counter, stamp it onto DrawnFootprint at capture time, and have clear_footprint return Err(StaleFootprint) on a mismatch instead of restoring. Propagate that into Turtle's forward/backward/goto, which now return Result instead of &mut Self. Co-Authored-By: WOZCODE <contact@withwoz.com>
Bot review flagged two test-coverage gaps in the Follow-up sections: which of Canvas::draw_sprite's existing tests must survive the move to Sprite::draw_on (transparency, clipping, same/cross-canvas), plus a wrong-canvas-id no-op test; and Turtle's pen-down case was only implied by the no-gap regression test, never stated on its own. Co-Authored-By: WOZCODE <contact@withwoz.com>
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/design/sprite-crate-extraction.md (1)
142-153: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPropagate stale-footprint errors from
draw_on.
draw_oncomposesclear_footprint, butclear_footprintcan fail whiledraw_onreturns(). ReturnResult<(), StaleFootprint>and skipplacewhen clearing fails. Otherwise callers cannot handle stale footprints safely.🤖 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 `@docs/design/sprite-crate-extraction.md` around lines 142 - 153, Update the draw_on method contract to return Result<(), StaleFootprint>, propagate any error from clear_footprint, and only call place after clearing succeeds. Preserve the existing successful draw behavior by returning Ok(()) after place completes.
🤖 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.
Inline comments:
In `@docs/design/sprite-crate-extraction.md`:
- Around line 184-199: Update the sprite place flow so the footprint’s
under-pixel data is captured before blitting, but assign DrawnFootprint.version
from canvas.version() only after place completes its pixel writes. Ensure the
saved version represents the post-blit canvas state so the first clear_footprint
call succeeds.
- Around line 194-201: Define recovery for stale clears in the Canvas
clear_footprint flow: ensure a StaleFootprint cannot leave an indefinitely
retryable last_draw with a permanently mismatched version. Either add an
explicit operation to discard or recover the saved footprint, or consume
last_draw when returning StaleFootprint, and document that the previously drawn
pixels remain unchanged.
In `@docs/design/turtle-geometry.md`:
- Around line 75-85: Define a recovery path for StaleFootprint in the public
Turtle API: add an explicit resynchronization operation or document a safe
low-level sequence that rebases or discards sprite.last_draw before retrying
forward, backward, or goto. Update the turtle geometry documentation and add
tests covering stale failure followed by successful recovery, while preserving
the guarantee that the failed move leaves position and canvas unchanged.
- Around line 75-86: Update the stale-footprint handling described for Canvas
and clear_footprint so canvas writes invalidate only footprints whose regions
overlap the affected write, rather than comparing a global Canvas::version().
Preserve Err(StaleFootprint) for actual footprint conflicts and add coverage for
two non-overlapping turtles moving sequentially without errors.
---
Outside diff comments:
In `@docs/design/sprite-crate-extraction.md`:
- Around line 142-153: Update the draw_on method contract to return Result<(),
StaleFootprint>, propagate any error from clear_footprint, and only call place
after clearing succeeds. Preserve the existing successful draw behavior by
returning Ok(()) after place completes.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 5c994937-5b01-4c05-9cb1-cff27bd5b720
📒 Files selected for processing (2)
docs/design/sprite-crate-extraction.mddocs/design/turtle-geometry.md
Three bugs in the previous fix: - place stamped the footprint's version before its own blit, which is itself a canvas write -- every sprite self-invalidated on the very first clear_footprint call. - Canvas::version() was a single global counter, so any sprite's move invalidated every other sprite's footprint too, even in disjoint areas -- breaking the multi-turtle case the design's own Objective promises "falls out for free." - Once stale, a footprint's version can never match again (monotonic), but nothing let a caller escape that -- a stale Turtle was permanently stuck retrying the same failing clear_footprint forever. Replace the single counter with Canvas::region_version(Rect), backed by a per-tile version grid: only writes that overlap a footprint's own tiles can invalidate it, and place stamps the version after its blit completes. Add Sprite::discard_footprint and Turtle::resync as the explicit recovery path out of a permanently-stale footprint. Co-Authored-By: WOZCODE <contact@withwoz.com>
6 tasks
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Summary
Two design docs for forward-looking
iklowork (games + turtle graphics), not tied to a v0 success criterion:docs/design/sprite-crate-extraction.md— extractsSprite/Bitmapout ofguiltty-coreinto a newguiltty-spritecrate, keepingguiltty-corescoped to exactly "draw into a kitty-like terminal" (absolute-coordinateCanvas/Shape/text/Backend). Flags the one real technical wrinkle:Canvas::draw_sprite's save/restore-under logic currently readsCanvas's privatepixels/idfields directly, so the extraction needs one small additive public method (Canvas::id()) and reimplementing the draw againstCanvas's existing publicpixel/set_pixelaccessors. Also adds a relative, actor-centric movement API (heading/forward/backward/turn) toSprite, alongside its existing absolutemove_to— both stay available, useful independently (e.g. games) of turtle graphics specifically.docs/design/turtle-geometry.md— aTurtlethat wraps aguiltty-sprite-basedSprite, adding only pen up/down + pen color. Movement itself is entirely delegated to the wrapped sprite; multiple turtles fall out for free from multiple sprites.Sequencing (each its own follow-up PR): (1) extract
guiltty-spritemechanically, (2) add relative movement toSprite, (3) buildguiltty-turtleon top.Test plan
Summary by Sourcery
Add forward-looking design documents for extracting sprite functionality into a dedicated crate and building a turtle-geometry layer on top.
Documentation:
Summary by cubic
Adds two forward-looking design docs: extracting
Sprite/Bitmapfromguiltty-coreintoguiltty-spritewith a relative movement API, and aguiltty-turtlelayer that wraps a sprite for pen drawing. Documents a breaking API change (Canvas::draw_sprite→Sprite::draw_on), region-scoped footprint versioning, and recovery paths to avoid trail corruption.Refactors
Sprite/Bitmaptoguiltty-sprite; keepguiltty-corefocused on absolute drawing.Canvas::draw_spritewithSprite::draw_on(&mut Canvas); addCanvas::id()andCanvas::region_version(Rect); use publicpixel/set_pixel.Bitmap::from_filecontinues returningguiltty_core::Error.New Features
Sprite:headingin degrees (clockwise, top-left origin),forward/backward,turn/left/right, plusmove_to; trackexact_position: (f32, f32)withPointas a rounded view.clear_footprintandplace; footprints are version-stamped by canvas id andregion_version(Rect);clear_footprintreturnsResult<(), StaleFootprint>on wrong-canvas or region changes; addSprite::discard_footprintto recover.guiltty-turtle: wraps aSprite, addspen_up/pen_downandpen_color; drawing moves (forward/backward/goto) returnResultand use clear/draw/place so trails don’t lose their start; safe trail crossings via staleness errors;Turtle::resyncrecovers from stale footprints.Written for commit f86a9d1. Summary will update on new commits.
Summary by CodeRabbit