Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,63 @@
# SFX-Excluded Lettering Reading Order Implementation Plan

> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers/subagent-driven-development (recommended) or superpowers/executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking.

**Goal:** Ensure SFX never consumes deterministic lettering placement order.

**Architecture:** Keep the existing validated and sorted authored sequence for counts and provenance. Build one renderable-only sequence in `letter_panel` and enumerate only that sequence when placing dialogue and captions.

**Tech Stack:** Python 3.11, Pillow 12.3.0, `unittest`.

## Global Constraints

- Do not add fields or change the storyboard schema.
- Keep SFX validation and summary counts unchanged.
- Keep panel pixels byte-identical when an otherwise equivalent authored SFX is added.
- Modify only the lettering implementation, its regression test, and these design records.

---

### Task 1: Lock the failure with a public-behavior regression

**Files:**
- Modify: `tests/test_lettering.py`

**Interfaces:**
- Consumes: `letter_panel(output_path, panel_width, panel_height, text_items, character_bible) -> dict`
- Produces: a regression asserting placement IDs and contiguous `reading_order` values.

- [ ] **Step 1: Write the failing test**

Add a test that letters SFX priority 1, dialogue priority 2, and caption priority 3, then asserts placement IDs are dialogue/caption and reading orders are `[1, 2]`.

- [ ] **Step 2: Verify RED**

Run:

```bash
python -m unittest tests.test_lettering.LetteringTests.test_sfx_does_not_consume_rendered_reading_order -v
```

Expected: FAIL because the current values are `[2, 3]`.

### Task 2: Enumerate renderable items only

**Files:**
- Modify: `scripts/letter_panels.py:922-942`
- Test: `tests/test_lettering.py`

**Interfaces:**
- Consumes: validated `ordered: list[dict]`
- Produces: `renderable: list[dict]` containing only non-SFX items and contiguous placement order.

- [ ] **Step 1: Implement the minimal fix**

Create `renderable = [item for item in ordered if item.get("kind") != "sfx"]`, derive `rendered_text_count` from its length, and enumerate `renderable` directly.

- [ ] **Step 2: Verify GREEN and compatibility**

Run the focused regression and all lettering tests. Confirm the mixed-SFX byte-identity test still passes.

- [ ] **Step 3: Run full acceptance**

Run the complete unit suite, compile checks, package build/distribution validation, and diff hygiene. Deliver through a protected-branch pull request and verify the squash-merged `main` commit plus cross-platform CI.
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
# SFX-Excluded Lettering Reading Order Design

## Problem

Comic Sol sorts every authored text item by `priority` and `id`, then enumerates that complete list before skipping SFX. Because SFX is generated inside the artwork and is not placed by deterministic lettering, an SFX with priority 1 incorrectly causes the first rendered dialogue or caption to receive `reading_order: 2`.

## Decision

Preserve the authored ordering used for validation, counts, and deterministic sorting. Derive a separate `renderable` sequence containing only dialogue and caption items, then enumerate that sequence from 1 when producing placements.

SFX remains validated, included in `text_count`, `sfx_count`, and `word_count`, and excluded from placement and rendering. No storyboard field, schema version, cache version, or migration is added.

## Required behavior

Given these authored items:

1. SFX, priority 1
2. Dialogue, priority 2
3. Caption, priority 3

lettering must produce exactly two placements:

- Dialogue with `reading_order: 1`
- Caption with `reading_order: 2`

The same renderable text must produce byte-identical panel output whether an earlier SFX item is present or absent.

## Validation

A regression test exercises the public `letter_panel` behavior and must fail on the existing implementation because its placement orders are `[2, 3]`. After the minimal engine fix, focused lettering tests, the complete suite, package/distribution checks, and cross-platform CI must pass.
7 changes: 3 additions & 4 deletions scripts/letter_panels.py
Original file line number Diff line number Diff line change
Expand Up @@ -919,7 +919,8 @@ def letter_panel(
raise ValueError(f"text item {item.get('id', 'unknown')} has unknown anchor")
item["content"] = content

rendered_text_count = sum(item.get("kind") != "sfx" for item in ordered)
renderable = [item for item in ordered if item.get("kind") != "sfx"]
rendered_text_count = len(renderable)
sfx_count = len(ordered) - rendered_text_count
word_count = sum(normalized_word_count(item["content"]) for item in ordered)
summary = {
Expand All @@ -937,9 +938,7 @@ def letter_panel(
canvas = base.copy()
draw = ImageDraw.Draw(canvas, "RGBA")
occupied: list[dict[str, int]] = []
for reading_order, item in enumerate(ordered, 1):
if item.get("kind") == "sfx":
continue
for reading_order, item in enumerate(renderable, 1):
requested = item.get("anchor", "top-left")
start = ANCHORS.index(requested)
rect = None
Expand Down
22 changes: 22 additions & 0 deletions tests/test_lettering.py
Original file line number Diff line number Diff line change
Expand Up @@ -699,6 +699,28 @@ def test_sfx_never_reaches_placement_or_render(self):
render.assert_not_called()
self.assertEqual(0, result["rendered_text_count"])

def test_sfx_does_not_consume_rendered_reading_order(self):
result = letter_panel(
str(self.panel),
800,
1000,
[
sfx("KRAK!", priority=1),
dialogue("Too late.", priority=2),
caption("The gate closes.", priority=3),
],
self.characters,
)

self.assertEqual(
["dialogue-2", "caption-3"],
[placement["id"] for placement in result["placements"]],
)
self.assertEqual(
[1, 2],
[placement["reading_order"] for placement in result["placements"]],
)

def test_sfx_does_not_change_or_reserve_mixed_lettering(self):
without_sfx = self.root / "without-sfx.png"
with_sfx = self.root / "with-sfx.png"
Expand Down
Loading