P5: the per-tile tileset scan, and the four bugs hiding in it #25

New Issue

tachikoma · 2026-08-02T18:33:06-04:00

tachikoma commented

2026-08-02 18:33:06 -04:00

Source: TODO.md, "Performance" -> "The plan", item 5 (at bbb7b8f)

The per-tile scan in akgl_tilemap_draw (src/tilemap.c:756-798) is the known
O(tiles x tilesets) FIXME, worth 0.8% of the frame at eight tilesets. It is also
wrong four ways, and three of the four are correctness rather than speed:

The range test uses >= on the upper bound --
map->tilesets[i].firstgid + tilecount >= tilenum -- so the tile one past the
end of set A also matches set B's firstgid.
There is no break after a match, so a tile matching two ranges is
blitted twice.
start_x/start_y are never clamped to zero (src/tilemap.c:719-728), so
a viewport at a negative coordinate indexes backwards.
The first-column src.x += accumulates across rows (:766). dest.x is
reset at the top of each row; src.x is not.

Fix: correct the test to tilenum < firstgid + tilecount, break on match,
clamp the start indices, reset src.x/src.y per row, and skip tilenum == 0
before scanning at all.

Verify with a counting test -- a stub draw_texture backend asserting exactly
one blit per non-empty visible cell, the same pattern tests/game.c uses for the
update counter -- because a boundary double-blit is invisible to a stopwatch and
mostly invisible on screen.

Mutation testing is mandatory here; this is control flow.

Do not reorganize the loader for this -- 0.8% does not justify it.

Files: src/tilemap.c:719-728, src/tilemap.c:756-798

Filed by Tachikoma (Claude Code, Opus 5, 1M context)

**Source:** TODO.md, "Performance" -> "The plan", item 5 (at bbb7b8f) The per-tile scan in `akgl_tilemap_draw` (`src/tilemap.c:756-798`) is the known O(tiles x tilesets) FIXME, worth 0.8% of the frame at eight tilesets. **It is also wrong four ways**, and three of the four are correctness rather than speed: 1. **The range test uses `>=` on the upper bound** -- `map->tilesets[i].firstgid + tilecount >= tilenum` -- so the tile one past the end of set A also matches set B's `firstgid`. 2. **There is no `break` after a match**, so a tile matching two ranges is blitted twice. 3. **`start_x`/`start_y` are never clamped to zero** (`src/tilemap.c:719-728`), so a viewport at a negative coordinate indexes backwards. 4. **The first-column `src.x +=` accumulates across rows** (`:766`). `dest.x` is reset at the top of each row; `src.x` is not. **Fix:** correct the test to `tilenum < firstgid + tilecount`, `break` on match, clamp the start indices, reset `src.x`/`src.y` per row, and skip `tilenum == 0` before scanning at all. **Verify with a counting test** -- a stub `draw_texture` backend asserting exactly one blit per non-empty visible cell, the same pattern `tests/game.c` uses for the update counter -- because a boundary double-blit is invisible to a stopwatch and mostly invisible on screen. **Mutation testing is mandatory here; this is control flow.** Do not reorganize the loader for this -- 0.8% does not justify it. **Files:** `src/tilemap.c:719-728`, `src/tilemap.c:756-798` --- Filed by Tachikoma (Claude Code, Opus 5, 1M context)

tachikoma added this to the 0.9.x milestone 2026-08-02 18:33:06 -04:00

tachikoma added the defect performance blast-radius:high labels 2026-08-02 18:33:06 -04:00

tachikoma referenced this issue

2026-08-02 18:34:50 -04:00

The performance plan: nine changes in blast-radius order #60

tachikoma added the status::grooming label 2026-08-02 18:49:16 -04:00

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: andrew/libakgl#25