Add PICO-8 custom font compilation target#6
Open
The-Nice-One wants to merge 4 commits intomainfrom
Open
Conversation
📝 WalkthroughWalkthroughThis PR introduces a complete Pico-8 backend for SPFC. Changes include workspace setup, a 256-entry character lookup table, pixmap-to-bitmap glyph conversion, Lua program generation with embedded font data, compile entry points, and development container configuration. The TTF backend is updated to use shared workspace dependencies. ChangesPico-8 Backend Implementation
Possibly Related Issues
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Comment |
![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)
.devcontainer/devcontainer.json (1)
8-10: ⚡ Quick winPrefer Dev Container Feature for Git LFS instead of apt in
postCreateCommand.Lines 8–10 show an unused features section. Moving Git LFS provisioning from line 27's
postCreateCommandto the properghcr.io/devcontainers/features/git-lfs:1feature is cleaner, faster, and more reproducible.Proposed change
- // Features to add to the dev container. More info: https://containers.dev/features. - // "features": {}, + // Features to add to the dev container. More info: https://containers.dev/features. + "features": { + "ghcr.io/devcontainers/features/git-lfs:1": {} + },- "postCreateCommand": "sudo apt update && sudo apt install -y git-lfs && git lfs install" + "postCreateCommand": "git lfs install --skip-repo"🤖 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 @.devcontainer/devcontainer.json around lines 8 - 10, The devcontainer.json currently leaves "features" unused and installs Git LFS in postCreateCommand; replace that by adding the Git LFS devcontainer feature instead of apt install in postCreateCommand: populate the "features" object with "ghcr.io/devcontainers/features/git-lfs:1": {} and remove the Git LFS install steps from postCreateCommand (references: the "features" key and the "postCreateCommand" entry) so provisioning is handled by the Dev Container feature.
🤖 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 `@backends/spfc-target-p8/src/builders/writer.rs`:
- Around line 9-12: Sanitize process.family_name and process.manufacturer before
splicing into program_string and when building font_function: create a safe
comment string by stripping/normalizing newlines and removing any "--" sequences
and control chars (replace with spaces), create a safe identifier for
font_function by to_ascii_lowercase(), replacing any non [a-z0-9_] with "_" and
prefixing (e.g., "f_") if it would start with a digit, and create a safe quoted
string by escaping backslashes and quotes and replacing newlines with "\n"; then
use those sanitized values instead of raw
process.family_name/process.manufacturer when constructing program_string and
font_function (references: program_string, font_function, load_... function).
- Around line 15-30: The loop currently skips glyphs whose keys are not in
P8SCII (crate::utilities::get_character_by_symbol), leading to silent missing
characters; before allocating the buffer or writing (i.e., before the call that
uses last_character_index(&process.pixmap_pairs) and before filling buffer),
iterate process.pixmap_pairs and validate every key via get_character_by_symbol,
and if any validation returns None immediately return an Err (or propagate an
error) naming the unsupported symbol so the caller fails fast; only proceed to
buffer allocation and the existing write loop after all keys are confirmed
valid.
In `@backends/spfc-target-p8/src/lib.rs`:
- Around line 27-28: The code currently picks layout.font_tables.first() and
font_table.fonts.first() without validation; update the selection logic in
lib.rs to validate cardinality or accept an explicit selection option: check
layout.font_tables.len() and font_table.fonts.len(), and if >1 either return a
clear error (including counts and available identifiers) or use a provided
plugin/config option to choose the desired table/font (by index or name);
reference the symbols layout.font_tables, font_table.fonts, font_table, and font
when adding these checks and error messages so ambiguous inputs fail loudly
instead of silently selecting the first item.
- Around line 25-37: The current compile entrypoint unwraps I/O and parse
operations (std::fs::read, layout_from_data, font_table/font lookup,
create_program_string, std::fs::write) which will panic; replace each unwrap
with proper error handling that converts failures into a controlled
CompileResult (or the crate's error type) and returns early. Specifically,
validate the file read (options.input), layout_from_data(error), presence checks
on layout.font_tables.first() and font_table.fonts.first(), the
create_program_string result, and the write to options.output; map each failure
to a clear CompileResult error variant or message and propagate (use ? or
explicit map_err) instead of panicking so Process, create_pixmap_pairs, and
downstream callers never cause a panic.
In `@backends/spfc-target-p8/src/utilities/pixmap.rs`:
- Around line 28-42: The code currently unwraps and uses todo!() which panics
for malformed or unsupported pixmap inputs; replace the unwrap on bits_per_pixel
(from pixmap_table.constant_bits_per_pixel.or(pixmap.custom_bits_per_pixel))
with graceful handling that returns a clear diagnostic error or uses a safe
fallback (e.g., treat missing bits_per_pixel as 1 or return Err with a helpful
message), and replace the todo!() checks for character.advance_x vs width,
width/height > 8, and bits_per_pixel != 1 with non-panicking logic that emits
diagnostics (or logs) and applies the intended fallbacks: set advance_x = width
when unsupported, clamp or treat width and height > 8 as 8, and coerce
bits_per_pixel to 1 (opaque) when non-1; update the functions/structs
referencing pixmap_table, pixmap.custom_bits_per_pixel, character.advance_x,
width, height, and bits_per_pixel to propagate errors or diagnostics instead of
calling todo!()/unwrap().
---
Nitpick comments:
In @.devcontainer/devcontainer.json:
- Around line 8-10: The devcontainer.json currently leaves "features" unused and
installs Git LFS in postCreateCommand; replace that by adding the Git LFS
devcontainer feature instead of apt install in postCreateCommand: populate the
"features" object with "ghcr.io/devcontainers/features/git-lfs:1": {} and remove
the Git LFS install steps from postCreateCommand (references: the "features" key
and the "postCreateCommand" entry) so provisioning is handled by the Dev
Container feature.
🪄 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: cffec29b-e34a-462d-8503-81ef360c037c
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
.devcontainer/devcontainer.jsonCargo.tomlbackends/spfc-target-p8/Cargo.tomlbackends/spfc-target-p8/src/builders/mod.rsbackends/spfc-target-p8/src/builders/writer.rsbackends/spfc-target-p8/src/lib.rsbackends/spfc-target-p8/src/utilities/characters.rsbackends/spfc-target-p8/src/utilities/mod.rsbackends/spfc-target-p8/src/utilities/pixmap.rsbackends/spfc-target-ttf/Cargo.toml
Comment on lines
+9
to
+12
| program_string += format!("-- {} v{}\n-- {}\n-- Generated by spfc\n-- https://github.com/SimplePixelFont/spfc\n\n", process.family_name, process.family_version, process.manufacturer).as_str(); | ||
|
|
||
| let font_function = format!("load_{}", process.family_name.to_ascii_lowercase().replace(" ", "_")); | ||
| program_string += format!("-- Copy the function below\n-- into your own cart\nfunction {}()\n\t-- enable custom fonts\n\tpoke(0x5f58,0x81)\n\n\t-- add font to memory\n", font_function).as_str(); |
There was a problem hiding this comment.
Sanitize font metadata before embedding it into Lua.
family_name and manufacturer are spliced into a comment, a function name, and a quoted string verbatim. Quotes, newlines, punctuation, or a leading digit can generate invalid Lua, and crafted metadata can inject extra code into the emitted cart.
Also applies to: 34-35
🤖 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 `@backends/spfc-target-p8/src/builders/writer.rs` around lines 9 - 12, Sanitize
process.family_name and process.manufacturer before splicing into program_string
and when building font_function: create a safe comment string by
stripping/normalizing newlines and removing any "--" sequences and control chars
(replace with spaces), create a safe identifier for font_function by
to_ascii_lowercase(), replacing any non [a-z0-9_] with "_" and prefixing (e.g.,
"f_") if it would start with a digit, and create a safe quoted string by
escaping backslashes and quotes and replacing newlines with "\n"; then use those
sanitized values instead of raw process.family_name/process.manufacturer when
constructing program_string and font_function (references: program_string,
font_function, load_... function).
Comment on lines
+15
to
+30
| let mut buffer = vec![0; (last_character_index(&process.pixmap_pairs) as usize + 1) * 8]; | ||
| buffer[0] = max_width(&process.pixmap_pairs) + 1; //1 added for letter spacing | ||
| buffer[1] = max_width(&process.pixmap_pairs); // This will be added as a custom flag | ||
| buffer[2] = max_height(&process.pixmap_pairs) + 1; //1 added for line spacing | ||
| buffer[3] = 0; // This will be added as a custom flag offset_x | ||
| buffer[4] = 0; // This will be added as a custom flag offset_y | ||
|
|
||
| for (key, glyph) in &process.pixmap_pairs { | ||
| if let Some(p8_char) = crate::utilities::get_character_by_symbol(key) { | ||
| let char_index = p8_char.id as usize; | ||
| let bitmap_data = &glyph.bitmap; | ||
| for (i, byte) in bitmap_data.iter().enumerate() { | ||
| buffer[char_index * 8 + i] = *byte; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Fail fast on unsupported glyph symbols instead of silently dropping them.
If a glyph key is not present in P8SCII_CHARSET, last_character_index(...) effectively ignores it and this loop just skips writing it. The result is a “successful” cart with missing characters and no signal to the caller. Please reject unsupported symbols before allocating/writing the buffer.
Suggested direction
+ let unsupported: Vec<_> = process
+ .pixmap_pairs
+ .keys()
+ .filter(|key| crate::utilities::get_character_by_symbol(key).is_none())
+ .cloned()
+ .collect();
+
+ if !unsupported.is_empty() {
+ anyhow::bail!(
+ "unsupported Pico-8 glyphs: {}",
+ unsupported.join(", ")
+ );
+ }
+
let mut buffer = vec![0; (last_character_index(&process.pixmap_pairs) as usize + 1) * 8];🤖 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 `@backends/spfc-target-p8/src/builders/writer.rs` around lines 15 - 30, The
loop currently skips glyphs whose keys are not in P8SCII
(crate::utilities::get_character_by_symbol), leading to silent missing
characters; before allocating the buffer or writing (i.e., before the call that
uses last_character_index(&process.pixmap_pairs) and before filling buffer),
iterate process.pixmap_pairs and validate every key via get_character_by_symbol,
and if any validation returns None immediately return an Err (or propagate an
error) naming the unsupported symbol so the caller fails fast; only proceed to
buffer allocation and the existing write loop after all keys are confirmed
valid.
Comment on lines
+25
to
+37
| let data = std::fs::read(&options.input).unwrap(); | ||
| let layout = layout_from_data(&data).unwrap(); | ||
| let font_table = layout.font_tables.first().unwrap(); | ||
| let font = font_table.fonts.first().unwrap(); | ||
|
|
||
| let mut process = Process::default(); | ||
| process.family_name = font.name.clone(); | ||
| process.family_version = font.version as f64; | ||
| process.manufacturer = font.author.clone(); | ||
| process.pixmap_pairs = create_pixmap_pairs(&layout); | ||
|
|
||
| let font_data = create_program_string(&process).unwrap(); | ||
| std::fs::write(&options.output, &font_data).unwrap(); |
There was a problem hiding this comment.
Remove panic paths from the exported compile entrypoint.
Line 25–Line 37 use unwrap() repeatedly on I/O, parse, lookup, generation, and write. Any bad input or FS failure will panic instead of returning a controlled CompileResult, which makes this backend brittle in host environments.
🤖 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 `@backends/spfc-target-p8/src/lib.rs` around lines 25 - 37, The current compile
entrypoint unwraps I/O and parse operations (std::fs::read, layout_from_data,
font_table/font lookup, create_program_string, std::fs::write) which will panic;
replace each unwrap with proper error handling that converts failures into a
controlled CompileResult (or the crate's error type) and returns early.
Specifically, validate the file read (options.input), layout_from_data(error),
presence checks on layout.font_tables.first() and font_table.fonts.first(), the
create_program_string result, and the write to options.output; map each failure
to a clear CompileResult error variant or message and propagate (use ? or
explicit map_err) instead of panicking so Process, create_pixmap_pairs, and
downstream callers never cause a panic.
Comment on lines
+27
to
+28
| let font_table = layout.font_tables.first().unwrap(); | ||
| let font = font_table.fonts.first().unwrap(); |
There was a problem hiding this comment.
Avoid implicit “first font” selection.
Line 27 and Line 28 silently pick the first table/font. For inputs containing multiple fonts, this can compile the wrong asset with no signal. Please validate the expected cardinality (or add explicit selection via plugin options) and fail clearly when ambiguous.
🤖 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 `@backends/spfc-target-p8/src/lib.rs` around lines 27 - 28, The code currently
picks layout.font_tables.first() and font_table.fonts.first() without
validation; update the selection logic in lib.rs to validate cardinality or
accept an explicit selection option: check layout.font_tables.len() and
font_table.fonts.len(), and if >1 either return a clear error (including counts
and available identifiers) or use a provided plugin/config option to choose the
desired table/font (by index or name); reference the symbols layout.font_tables,
font_table.fonts, font_table, and font when adding these checks and error
messages so ambiguous inputs fail loudly instead of silently selecting the first
item.
Comment on lines
+28
to
+42
| let bits_per_pixel = pixmap_table | ||
| .constant_bits_per_pixel | ||
| .or(pixmap.custom_bits_per_pixel) | ||
| .unwrap(); | ||
|
|
||
| let advance_x = character.advance_x.unwrap_or(width); | ||
| if advance_x != width { | ||
| todo!("ABI 0.2.0: will add logging; On p8 target the advance_x is ignored because pico-8 doesn't support it.") | ||
| } | ||
| if width > 8 || height > 8 { | ||
| todo!("ABI 0.2.0: will add logging; On p8 target only 8x8 pixels are supported, Anything larger than that will be treated as 8x8.") | ||
| } | ||
| if bits_per_pixel != 1 { | ||
| todo!("ABI 0.2.0: will add logging; On p8 target only 1 bit per pixel is supported, Anything other than 0 will be treated as opaque.") | ||
| } |
There was a problem hiding this comment.
Replace the panicking placeholder paths before release.
This builder currently aborts on reachable input: Line 31 panics if bits-per-pixel is missing, and Lines 35, 38, and 41 panic on non-default metrics or unsupported glyph sizes. That makes unsupported or malformed fonts crash compilation instead of returning a diagnostic or applying the fallback the messages describe.
🤖 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 `@backends/spfc-target-p8/src/utilities/pixmap.rs` around lines 28 - 42, The
code currently unwraps and uses todo!() which panics for malformed or
unsupported pixmap inputs; replace the unwrap on bits_per_pixel (from
pixmap_table.constant_bits_per_pixel.or(pixmap.custom_bits_per_pixel)) with
graceful handling that returns a clear diagnostic error or uses a safe fallback
(e.g., treat missing bits_per_pixel as 1 or return Err with a helpful message),
and replace the todo!() checks for character.advance_x vs width, width/height >
8, and bits_per_pixel != 1 with non-panicking logic that emits diagnostics (or
logs) and applies the intended fallbacks: set advance_x = width when
unsupported, clamp or treat width and height > 8 as 8, and coerce bits_per_pixel
to 1 (opaque) when non-1; update the functions/structs referencing pixmap_table,
pixmap.custom_bits_per_pixel, character.advance_x, width, height, and
bits_per_pixel to propagate errors or diagnostics instead of calling
todo!()/unwrap().
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
1 participant
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 by CodeRabbit
New Features
Chores