# TODO

## Internal consistency

Findings from a sweep of `src/` and `include/`. These are consistency and
convention problems, not new functional defects — where one has a functional
consequence it is called out. Ordered roughly by blast radius. Items that
overlap the existing **Defects** list are cross-referenced rather than repeated.

### 1. Public naming conventions

1. **Include guards use three different schemes.** `_AKGL_ACTOR_H_`,
   `_AKGL_CHARACTER_H_`, `_AKGL_GAME_H_`, `_AKGL_HEAP_H_`, `_AKGL_ITERATOR_H_`,
   `_AKGL_SPRITE_H_`, and `_AKGL_TYPES_H_` carry the project prefix; `_ASSETS_H_`,
   `_CONTROLLER_H_`, `_DRAW_H_`, `_ERROR_H_`, `_JSON_HELPERS_H_`, `_PHYSICS_H_`,
   `_REGISTRY_H_`, `_RENDERER_H_`, `_TEXT_H_`, `_TILEMAP_H_`, and `_UTIL_H_` do
   not. Pick `_AKGL_<FILE>_H_` everywhere.

   The worst case is `include/akgl/staticstring.h:6`, which guards with
   `_STRING_H_` — a name several libc implementations use for their own
   `<string.h>`. The same file then does `#include "string.h"` (line 9) with
   quotes, which is a relative-first lookup that only reaches the system header
   by accident. Rename the guard to `_AKGL_STATICSTRING_H_` and use
   `#include <string.h>`.

2. **Function names contradict the stated convention.** `AGENTS.md` says public
   symbols use `akgl_` and types use `akgl_TypeName`. Exceptions:
   - `akgl_Actor_cmhf_left_on` and its seven siblings (`include/akgl/actor.h:196-252`)
     embed the *type* name in a *function* name, unlike every other actor entry
     point (`akgl_actor_*`). Rename to `akgl_actor_cmhf_*`.
   - `akgl_game_updateFPS` (`include/akgl/game.h:104`) is camelCase; every other
     function is snake_case.
   - `akgl_render_init2d` (`include/akgl/renderer.h:83`) puts the `2d` at the end
     while the six functions it installs are `akgl_render_2d_*`. Make it
     `akgl_render_2d_init`.
   - `akgl_sprite_sheet_coords_for_frame` (`include/akgl/sprite.h:86`) spells it
     `sprite_sheet`; `akgl_spritesheet_initialize` and `akgl_heap_next_spritesheet`
     spell it `spritesheet`.

3. **Two public types have no prefix at all.** `point` and `RectanglePoints`
   (`include/akgl/util.h:13-25`) are unprefixed, and `RectanglePoints` is
   PascalCase with no namespace. Both are dumped into every translation unit
   that includes `util.h`. Rename to `akgl_Point` / `akgl_RectanglePoints`.

4. **Global variables use four different conventions.** `window`, `bgm`, `game`,
   `gamemap`, `renderer`, `physics`, and `camera` (`src/game.c:26-43`) are
   unprefixed single common words exported from a shared library — `renderer`
   and `camera` in particular are very likely to collide with a consuming game.
   Alongside them the same file exports `akgl_mixer` and `akgl_tracks` (prefixed),
   `_akgl_renderer` / `_akgl_camera` / `_akgl_physics` / `_akgl_gamemap`
   (underscore-prefixed, which is reserved at file scope), and elsewhere
   `HEAP_ACTOR`…`HEAP_STRING` (`src/heap.c:17-21`) and `GAME_ControlMaps`
   (`src/controller.c:13`) are SCREAMING_SNAKE, which the convention reserves for
   constants and macros. Settle on `akgl_` for all exported objects.

5. **`AKGL_SPRITE_MAX_CHARACTER_NAME_LENGTH`** (`include/akgl/character.h:13`) is a
   character constant carrying the `SPRITE` prefix, and lives in `character.h`.
   Rename to `AKGL_CHARACTER_MAX_NAME_LENGTH`.

6. **`AKGL_TIME_ONESEC_MS` is misnamed and the error is live.**
   `include/akgl/game.h:22` defines it as `1000000`. One second in milliseconds
   is `1000`; `1000000` is the number of nanoseconds in a *millisecond*. The name
   says "one second" but the value means "one millisecond", so:
   - `src/character.c:209` and `src/sprite.c:141` use it correctly as a
     milliseconds-to-nanoseconds scale factor.
   - `src/game.c:136` uses it as documented — a one-second budget for the state
     lock — in a loop that advances `totaltime += 100` alongside `SDL_Delay(100)`.
     The loop therefore runs 10,000 iterations of 100 ms, so `akgl_game_state_lock`
     blocks for roughly 16 minutes rather than 1 second before reporting failure.

   Rename to `AKGL_TIME_ONEMS_NS` to match `AKGL_TIME_ONESEC_NS`, and give
   `akgl_game_state_lock` a real one-second budget.

### 2. Header/implementation surface drift

7. **Nineteen non-static functions are defined in `src/` but declared in no
   header.** They have external linkage and public-looking names, so they are
   part of the ABI whether intended or not, and no consumer can call them:

   `akgl_game_load_objectnamemap`, `akgl_game_load_versioncmp`,
   `akgl_game_save_actors`, `akgl_game_save_actorname_iterator`,
   `akgl_game_save_charactername_iterator`, `akgl_game_save_spritename_iterator`,
   `akgl_game_save_spritesheetname_iterator` (`src/game.c`);
   `akgl_get_json_properties_double`, `akgl_get_json_properties_float`,
   `akgl_get_json_properties_number`, `akgl_tilemap_load_layer_image`,
   `akgl_tilemap_load_layer_object_actor`, `akgl_tilemap_load_physics`
   (`src/tilemap.c`); `akgl_path_relative_from`, `akgl_path_relative_root`
   (`src/util.c`); `gamepad_handle_added`, `gamepad_handle_button_down`,
   `gamepad_handle_button_up`, `gamepad_handle_removed` (`src/controller.c`).

   Each should be either declared in its header or made `static`. Note that
   `tilemap.h` already has a "part of the internal API, exposed here for unit
   testing" block — the tilemap entries belong there.

8. **`akgl_game_init_screen` is declared but never defined**
   (`include/akgl/game.h:100`). Same failure mode as **Defects → Known and still
   open #10**, which covers the four `akgl_controller_handle_*` declarations;
   fold this one into that item.

9. **Static helpers use three different naming styles.** `actor_visible`
   (`src/actor.c:185`) is bare; `akgl_character_load_json_inner` and
   `akgl_character_load_json_state_int_from_strings` (`src/character.c:101,132`)
   and `akgl_sprite_load_json_spritesheet` (`src/sprite.c:51`) carry the full
   public prefix; `gamepad_handle_*` (`src/controller.c:121+`) uses a third
   subsystem word that appears nowhere else. Adopt one rule — the clearest is
   that `static` helpers drop the `akgl_` prefix, since it exists to avoid
   external collisions.

10. **Parameter names disagree between declaration and definition.** Doxygen
    documents the header spelling, so the generated docs describe names the
    implementation does not use:

    | Header | Implementation |
    |---|---|
    | `character.h:41` `basechar` | `character.c:21` `obj` |
    | `character.h:69` `props` | `character.c:75` `registry` |
    | `heap.h:121` `ptr` | `heap.c:143` `basechar` |
    | `registry.h:97` `value` | `registry.c:163` `src` |
    | `json_helpers.h:134` `e` | `json_helpers.c:149` `err` |
    | `tilemap.h:134,143` `dest` | `tilemap.c:646,767` `map` |

    The `tilemap.h` pair is the most misleading: the parameter is the map being
    *read* and drawn, but it is named `dest` and documented as "Output
    destination populated by the function".

11. **Object-pool size macros are defined twice, and the override hook is
    dead.** `heap.h:15-29` wraps `AKGL_MAX_HEAP_ACTOR`, `_SPRITE`, `_SPRITESHEET`,
    `_CHARACTER`, and `_STRING` in `#ifndef` guards so a consumer can override
    them, but `actor.h:65`, `sprite.h:19-20`, and `character.h:14` define the same
    four unconditionally and are included from `heap.h:9-11`. Whichever header
    lands first wins and the `#ifndef` never fires, so the override mechanism
    cannot work. Define each pool size once — `heap.h` is the natural home.

12. **Headers rely on their includers for types.** `iterator.h` uses `uint32_t`
    without `<stdint.h>`; `json_helpers.h` uses `json_t` without `<jansson.h>`;
    `util.h` uses `SDL_FRect` and `bool` without any SDL include; `text.h` uses
    `SDL_Color` and `akerr_ErrorContext` without including SDL or `akerror.h`;
    `controller.h` uses `akgl_Actor` without including `actor.h`. Each compiles
    only because of `.c`-file include ordering. Headers should be self-contained.

13. **Include spelling is split between quoted and angled forms for the same
    directory.** `actor.h:10-11`, `character.h:10-11`, `controller.h:11`,
    `game.h:11-14`, `json_helpers.h:10`, and `staticstring.h:9` use
    `#include "sibling.h"`; `physics.h:11-13`, `registry.h:9-10`, `renderer.h:13`,
    `tilemap.h:10-12`, and `util.h:10` use `#include <akgl/sibling.h>`. The
    angled form is correct for an installed library.

14. **Empty parameter lists.** `akgl_heap_init()`, `akgl_heap_init_actor()`,
    `akgl_registry_init*()`, `akgl_game_init()`, `akgl_game_init_screen()`, and
    `akgl_game_updateFPS()` declare `()` rather than `(void)`, while
    `akgl_controller_list_keyboards(void)`, `akgl_controller_open_gamepads(void)`,
    `akgl_game_lowfps(void)`, and `akgl_game_state_lock(void)` use `(void)`.
    Before C23 the two are not equivalent — `()` suppresses argument checking.
    `akgl_heap_init_actor` is even declared `()` in `heap.h:57` and defined
    `(void)` in `heap.c:48`.

15. **`AKERR_NOIGNORE` is applied inconsistently at definition sites.** Headers
    use it uniformly (except `akgl_sprite_sheet_coords_for_frame`, `sprite.h:86`,
    which omits it). Definitions are split even within one file: `registry.c:55,120,163,172`
    repeat it, `registry.c:27,44,63,71,79,96,104,112` do not. Since the attribute
    is already on the declaration, drop it from all definitions.

### 3. Error-handling pattern

16. **`*_RETURN` macros are used inside `ATTEMPT` blocks, which skips `CLEANUP`.**
    The established pattern is `FAIL_*_BREAK` / `CATCH` inside `ATTEMPT` and
    `*_RETURN` outside it. Violations:
    - `src/tilemap.c:52` — `SUCCEED_RETURN` on the success path of
      `akgl_get_json_tilemap_property` returns directly from inside `ATTEMPT`,
      bypassing the `CLEANUP` at line 54 that releases `tmpstr` and `typestr`.
      Every successful property lookup leaks two heap strings, and the string
      pool is only 256 entries.
    - `src/tilemap.c:620` — `FAIL_RETURN` inside `ATTEMPT`.
    - `src/controller.c:286-289,306` — `FAIL_ZERO_RETURN` / `FAIL_NONZERO_RETURN`
      inside `ATTEMPT`; `src/controller.c:383` — `SUCCEED_RETURN` inside `ATTEMPT`,
      which also leaves `akgl_controller_default` with no return statement on the
      path that falls out of `FINISH`.
    - `src/game.c:457,462` — `FAIL_NONZERO_RETURN` inside `ATTEMPT`, skipping the
      `fclose` in `CLEANUP` at line 482.

17. **NULL-check discipline varies by function.** In `src/json_helpers.c` only
    `akgl_get_json_string_value` (line 71) and `akgl_get_json_array_index_string`
    (line 128) validate `key`/`dest`; the other eight accessors validate only the
    container and then dereference `dest` unconditionally. In `src/physics.c` the
    arcade backends check `actor` but `akgl_physics_null_gravity`,
    `_null_collide`, and `_null_move` (lines 15-34) check only `self`. In
    `src/renderer.c:65,74`, `akgl_render_2d_frame_start` and `_frame_end`
    dereference `self->sdl_renderer` with no check on `self`, while
    `akgl_render_2d_draw_texture` (line 82) checks `self` first.

18. **Error-context variable naming is split between `errctx` and `e`,
    sometimes within one file.** `src/util.c` uses `e` in the path helpers
    (lines 32-116) and `errctx` in the geometry helpers (lines 118-259);
    `src/heap.c` uses `errctx` everywhere except `akgl_heap_init_actor` (line 50).
    `src/actor.c`, `character.c`, `json_helpers.c`, `registry.c`, `sprite.c`, and
    `tilemap.c` favor `errctx`; `game.c`, `physics.c`, `renderer.c`, and
    `controller.c` favor `e`. Pick one.

### 4. Types and macros

19. **`float`/`double` are used raw where `types.h` defines aliases.** `types.h`
    exports `float32_t` and `float64_t`, and the actor and character structs use
    `float32_t` throughout — but `akgl_get_json_number_value` takes `float *`
    (`json_helpers.h:55`), `akgl_get_json_double_value` takes `double *` (line 66),
    `akgl_PhysicsBackend`'s six drag/gravity fields are `double`
    (`physics.h:22-27`), and `akgl_Tilemap`'s perspective fields are `float`
    (`tilemap.h:105-108`). Either use the aliases consistently or delete them.

20. **`AKGL_COLLIDE_RECTANGLES` (`include/akgl/util.h:27`) has unbalanced
    parentheses** — three opens, two closes — so any use is a syntax error. It has
    no callers and duplicates `akgl_collide_rectangles`. Delete it.

21. **Bitmask macros are unparenthesized.** `AKGL_BITMASK_HAS(x, y)` expands to
    `(x & y) == y` with no outer parens, so `!AKGL_BITMASK_HAS(a, b)` parses as
    `!(a & b) == b`. No in-tree caller negates it today (`AKGL_BITMASK_HASNOT`
    exists for that), so this is latent rather than live. `AKGL_BITMASK_CLEAR(x)`
    (`game.h:86`) also carries a trailing semicolon inside the macro body, so
    normal use produces an empty statement. Parenthesize all five and drop the
    semicolon.

22. **The state and iterator bit macros mix forms.** `AKGL_ITERATOR_OP_UPDATE`
    (`iterator.h:15`) is written `1` while its 31 siblings are `1 << n`, and none
    of the `1 << n` values in `iterator.h` or `actor.h:15-49` are parenthesized.
    The hand-maintained trailing bit-pattern comments in `actor.h:34-49` are also
    wrong for the high word — they restart at `0000 0000 0000 0001` for bit 16
    rather than showing the full 32-bit value.

23. **`akgl_Frame` (`game.h:27-31`) is defined and never used** anywhere in
    `src/`, `include/`, `tests/`, or `util/`.

### 5. `AKGL_ACTOR_STATE_STRING_NAMES` disagrees with `actor.h`

24. **The array bound differs between declaration and definition.**
    `include/akgl/actor.h:57` declares `[AKGL_ACTOR_MAX_STATES+1]` (33);
    `src/actor_state_string_names.c:6` defines `[32]`. Any consumer that trusts
    the declared bound and reads index 32 reads past the object.

25. **Two entries name the wrong bit.** `actor.h` assigns bit 11 to
    `AKGL_ACTOR_STATE_MOVING_IN` and bit 12 to `AKGL_ACTOR_STATE_MOVING_OUT`, but
    `actor_state_string_names.c:18-19` puts `"AKGL_ACTOR_STATE_UNDEFINED_11"` and
    `"AKGL_ACTOR_STATE_UNDEFINED_12"` at those indices — names for macros that do
    not exist. Since `akgl_registry_init_actor_state_strings`
    (`src/registry.c:79-94`) builds `AKGL_REGISTRY_ACTOR_STATE_STRINGS` from this
    array and `akgl_character_load_json_state_int_from_strings`
    (`src/character.c:101`) resolves character-JSON state names through it, a
    character JSON can never bind a sprite to `MOVING_IN` or `MOVING_OUT`.

26. **The generation comment is stale.** `actor.h:53-55` says the file "is built
    by a utility script and not kept in git, see the Makefile for
    lib_src/actor_state_string_names.c". There is no Makefile (the project is
    CMake), no `lib_src/`, no such script under `scripts/` or `util/`, and the
    file *is* tracked in git at `src/actor_state_string_names.c`. Either restore
    the generator — which would fix items 24 and 25 by construction — or delete
    the comment and maintain the file by hand.

### 6. Doxygen drift

27. **Three struct doc comments in `tilemap.h` are rotated by one.**
    `akgl_TilemapObject` (line 31) is documented as "Stores tileset metadata,
    texture, and frame offsets" (that is `akgl_Tileset`); `akgl_TilemapLayer`
    (line 46) as "Represents an object embedded in a tilemap layer" (that is
    `akgl_TilemapObject`); `akgl_Tileset` (line 61) as "Stores tile, image, or
    object data for one map layer" (that is `akgl_TilemapLayer`).

28. **`point` is documented as "Represents a two-dimensional point"**
    (`util.h:12`) but has `x`, `y`, and `z`.

29. **Doc comments live on the definition for public functions.** The convention
    is header-side documentation, but `akgl_path_relative_root` (`util.c:23`),
    `akgl_path_relative_from` (`util.c:97`), the four `gamepad_handle_*`
    (`controller.c:114+`), the `akgl_game_save_*` iterators and
    `akgl_game_load_*` helpers (`game.c:173+`), and the tilemap helpers
    (`tilemap.c:90+`) are documented only in the `.c`. This is the same set as
    item 7 — resolving that resolves this.

### 7. Formatting

30. **A minority of files use a 2-column offset instead of the canonical
    4-column one.** The mix of tabs and spaces across most of `src/` is *not*
    disorder: it is exactly what Emacs `cc-mode` emits for the `stroustrup` style
    with `indent-tabs-mode t` and `tab-width 8` — depth 1 is four spaces, depth 2
    is one tab, depth 3 is a tab plus four spaces, and so on. Twelve of the
    seventeen `.c` files follow that ladder cleanly.

    The genuine outliers indent at 2 columns: `src/json_helpers.c` (82 lines,
    except `akgl_get_json_with_default` at line 149 which is 4), `src/util.c`
    (37 lines, from `akgl_rectangle_points` at line 118 onward), `src/assets.c`
    (9), `src/staticstring.c` (7), `src/actor.c` (8, in `akgl_actor_add_child`
    at line 272), plus `include/akgl/util.h` (7) and
    `include/akgl/staticstring.h` (2).

    **Resolved.** `AGENTS.md` now specifies the canonical style and the exact
    `cc-mode` settings; `.dir-locals.el` applies them in Emacs; and
    `scripts/reindent.sh` applies them in batch. The whole tree (32 files across
    `src/`, `include/`, `tests/`, and `util/`) has been reindented and is a fixed
    point of `scripts/reindent.sh --check`. Verified whitespace-only: apart from
    three trailing blank lines removed at EOF in `src/heap.c`, `src/registry.c`,
    and `tests/tilemap.c`, `git diff -w` over the reindent is empty, and the test
    results are unchanged (13/14, `character` still the intentional failure).
    `scripts/hooks/pre-commit` keeps it that way — enable with
    `git config core.hooksPath scripts/hooks`.

31. **Leftover debug code ships in the library.** `src/controller.c:91-97` logs
    four lines whenever `event->type == 768 && event->key.which == 11 &&
    event->key.key == 13` — hardcoded decimal values for a specific keyboard ID
    on a specific developer's machine, inside the per-event inner loop.

32. **Large commented-out blocks.** `src/sprite.c:115-120,157,185-198,210`;
    `src/character.c:192-199,215`; `src/assets.c:18-27,50`;
    `src/tilemap.c:583,596-598,640`. All are the same abandoned
    `SDL_GetBasePath()` path-prefixing approach, superseded by
    `akgl_path_relative`. Delete them.

33. **Unused locals.** `screenwidth`/`screenheight` (`game.c:53-54`), `curTime`
    (`game.c:499`), `curTime` and `j` (`renderer.c:114,116` — `j` is also shadowed
    by the inner loop at line 128), `target` (`character.c:59`), `result`
    (`util.c:37,73`), `opflags` (`heap.c:146`, declared and cleared but never
    read).

34. **`akgl_game_update`'s default flags OR the same bit twice.**
    `src/game.c:496` reads
    `(AKGL_ITERATOR_OP_LAYERMASK | AKGL_ITERATOR_OP_LAYERMASK)`. Given the loop
    that follows walks layers and calls `updatefunc`, the intent was almost
    certainly `AKGL_ITERATOR_OP_UPDATE | AKGL_ITERATOR_OP_LAYERMASK`.

35. **`akgl_draw_background` is the only public function outside the error
    protocol.** `include/akgl/draw.h:14` returns `void` and reports nothing;
    every other public entry point returns `akerr_ErrorContext *`. It also calls
    `SDL_SetRenderDrawColor` and `SDL_RenderFillRect` without checking `renderer`
    or `renderer->sdl_renderer`.

36. **`akgl_registry_init_actor` is the only registry initializer that destroys
    an existing registry** before recreating it (`src/registry.c:47-49`). The
    other seven leak the old `SDL_PropertiesID` on a second call. Either all of
    them should do it or none should.

37. **Redundant casts obscure the code.** `(json_t *)json` where `json` is
    already `json_t *`, `(akgl_Tilemap *)dest`, `(akgl_Actor *)actorobj`,
    `(char *)&obj->name` on an array that already decays — roughly 180 pointer
    casts across `src/`, a large majority of them no-ops, densest in
    `src/tilemap.c:150-624` and `src/character.c:144-172`.
    They suppress exactly the conversion warnings that would catch a real
    mismatch.

38. **`struct`-qualified parameters in two definitions.**
    `akgl_physics_null_move` (`src/physics.c:29`) and `akgl_physics_arcade_move`
    (`src/physics.c:80`) are defined with `struct akgl_PhysicsBackend *self`
    while the header and the other eight physics functions use the typedef.

39. **`text.c:19` validates the wrong argument.**
    `FAIL_ZERO_RETURN(errctx, name, AKERR_NULLPOINTER, "Null filepath")` checks
    `name` a second time; `filepath` is never checked and is passed straight to
    `TTF_OpenFont`. Copy-paste of line 18.

40. **`akgl_path_relative` and `akgl_path_relative_from` disagree on the output
    parameter.** `akgl_path_relative` and `akgl_path_relative_root` take
    `akgl_String *dst`; `akgl_path_relative_from` takes `akgl_String **dst`
    (`src/util.c:105`). The `**` form matches the rest of the library
    (`akgl_get_json_string_value`, `akgl_get_property`, `akgl_heap_next_string`),
    which allocate when `*dest` is NULL. Related: **Defects → Known and still
    open #4**, which covers the fact that `akgl_path_relative_from` never writes
    `*dst` at all.

41. **`dst` vs `dest` for output parameters.** `akgl_string_copy` and the
    `akgl_path_relative*` family use `dst`; everything else uses `dest`.

## Coverage status

Generated with:

```sh
cmake -S . -B build-coverage -DAKGL_COVERAGE=ON -DCMAKE_BUILD_TYPE=Debug
cmake --build build-coverage --parallel
ctest --test-dir build-coverage --output-on-failure
```

Reports land in `build-coverage/coverage/` (`index.html`, `coverage.xml`).

**Line coverage 72.2%, function coverage 78.6%** (1534/2125 lines), up from a
39.6% / 44.3% baseline. `character` is the one intentionally failing suite;
everything else passes.

| File | Lines | Functions |
|---|---|---|
| `src/actor.c` | 205/258 (80%) | 16/18 |
| `src/assets.c` | 0/21 (0%) | 0/1 |
| `src/character.c` | 104/118 (88%) | 6/7 |
| `src/controller.c` | 220/243 (90%) | 9/9 |
| `src/draw.c` | 0/13 (0%) | 0/1 |
| `src/game.c` | 124/230 (54%) | 10/15 |
| `src/heap.c` | 116/116 (100%) | 12/12 |
| `src/json_helpers.c` | 111/111 (100%) | 11/11 |
| `src/physics.c` | 140/140 (100%) | 10/10 |
| `src/registry.c` | 76/102 (74%) | 11/12 |
| `src/renderer.c` | 7/70 (10%) | 1/7 |
| `src/sprite.c` | 93/101 (92%) | 5/5 |
| `src/staticstring.c` | 16/17 (94%) | 2/2 |
| `src/text.c` | 0/28 (0%) | 0/2 |
| `src/tilemap.c` | 201/426 (47%) | 10/20 |
| `src/util.c` | 121/131 (92%) | 7/8 |

Branch coverage reads 18.6% and should not be used as a target. The akerror
control-flow macros (`ATTEMPT`/`CATCH`/`PROCESS`/`FINISH`, `FAIL_*_RETURN`)
expand into large branch trees per call site, most of them unreachable in normal
operation — `src/game.c` reports over 1700 branches across 230 lines. Track line
and function coverage; treat branch coverage as a relative signal within a file.

### Suites

Every suite is registered through the `AKGL_TEST_SUITES` list in
`CMakeLists.txt`, which drives target creation, CTest registration, the
`WORKING_DIRECTORY`/`TIMEOUT` properties, the link line, and the
`FIXTURES_REQUIRED akgl_coverage` list together. Adding `tests/<name>.c` and the
name to that list is all a new suite needs; it can no longer be accidentally
left out of the coverage fixture.

Shared assertion helpers are in `tests/testutil.h`: `TEST_ASSERT`,
`TEST_ASSERT_FEQ`, `TEST_EXPECT_STATUS`, `TEST_EXPECT_OK`, `TEST_EXPECT_ANY_ERROR`,
and `TEST_ASSERT_FLAG`. All except the last expand to a `break` on failure, so
they belong directly inside an `ATTEMPT` block, not inside a loop nested in one.

Done:

- `tests/physics.c` — both backends, the factory, and the full simulation loop
  including thrust clamping, drag, layer masking, parent/child positioning, and
  logic-interrupt handling. 100%.
- `tests/heap.c` — pool exhaustion for all five pools, refcount clamping,
  recursive child release, registry cleanup on release. 100%.
- `tests/json_helpers.c` — every typed accessor, both string accessors'
  allocate-vs-reuse paths, array bounds, and `akgl_get_json_with_default`. 100%.
- `tests/controller.c` — control map push and capacity, the default binding set,
  keyboard and gamepad dispatch including cross-device rejection, the dpad
  handlers, and device add/remove against the dummy drivers. 90%.
- `tests/game.c` — version gating, the save/load roundtrip, foreign-save
  rejection, truncated-table detection, the state lock, and FPS accounting. 54%.
- `tests/actor.c` — extended with the eight control-map handlers, automatic
  facing, movement logic, the animation frame state machine, `akgl_actor_update`,
  and character/sprite binding lookups. 80%.

## Remaining work

### Needs the offscreen renderer harness

`src/renderer.c` (63 lines), `src/text.c` (28), `src/draw.c` (13),
`src/assets.c` (21), `akgl_actor_render`/`actor_visible` in `src/actor.c` (53),
and the drawing half of `src/tilemap.c` all need a live `renderer` global.

Build `tests/harness.c` / `tests/harness.h` with `akgl_test_init_headless()` and
`akgl_test_shutdown_headless()`: set the dummy video and audio drivers,
`SDL_Init()`, `akgl_heap_init()`, `akgl_registry_init()`, create a software
`SDL_CreateWindowAndRenderer`, and point the global `renderer` at it. Four
existing tests hand-roll this today (`tests/sprite.c:194`, `tests/character.c:200`,
`tests/tilemap.c:421`, `tests/charviewer.c:42`); collapse them onto the shared
harness in the same change. The new suites set the driver hints inline and need
no window, so they are unaffected.

Then:

- **`tests/renderer.c`** — `akgl_render_init2d` populating all six function
  pointers and the camera; frame start/end against a NULL `sdl_renderer`; the
  rotated `draw_texture` path including `angle != 0` with a NULL center; the
  `draw_mesh` "not implemented" stub; and `draw_world` layer ordering. Note
  `defflags` at `src/renderer.c:113` is uninitialized until the `if` body runs.
- **`tests/text.c`**, **`tests/draw.c`**, **`tests/assets.c`** — font loading
  into `AKGL_REGISTRY_FONT`, text rendering with wrap on and off, background
  drawing at zero/negative/oversized dimensions, and BGM loading into
  `AKGL_REGISTRY_MUSIC` under the dummy audio driver.
- **`tests/tilemap.c` extensions** — `akgl_tilemap_draw`, `_draw_tileset`, and
  `akgl_tilemap_load_layer_image`.

### Does not need a renderer

- **`src/tilemap.c`** — `akgl_tilemap_scale_actor` is pure math over three
  branches (`src/tilemap.c:824-830`); `akgl_get_json_properties_number`,
  `_float`, and `_double` need only a JSON snippet; `akgl_tilemap_load_physics`
  needs a fixture with the physics property block present, absent, and
  malformed. Together roughly 100 of the 225 uncovered lines.
- **`src/game.c`** — `akgl_game_init`, `akgl_game_update`, `akgl_game_lowfps`,
  and `akgl_game_updateFPS`'s frame loop need a window; revisit after the
  harness lands.
- **`src/registry.c`** — `akgl_registry_load_properties` needs a fixture with a
  `properties` object, plus the missing-file, missing-key, and wrong-value-type
  cases. Assert the loop at `src/registry.c:148-158` does not leak the string
  heap.

## Defects

### Fixed while building the suites

Each was found by a test written to assert correct behavior.

1. **`akgl_physics_simulate` dereferenced `self` before its NULL check.**
   `src/physics.c:132` read `self->gravity_time` at declaration time, three
   lines above `FAIL_ZERO_RETURN(e, self, ...)`. A NULL backend segfaulted.
2. **`akgl_game_save` never flushed or closed its stream.** `CLEANUP` and
   `PROCESS` were transposed, which put the `fclose` inside the `PROCESS`
   switch, where it only ran if an error context existed and reported success.
   An ordinary save produced an empty file.
3. **`akgl_game_save_actors` wrote name-table terminators from a single char.**
   `aksl_fwrite((void *)&nullval, 1, AKGL_ACTOR_MAX_NAME_LENGTH, fp)` emitted
   127 bytes of adjacent stack memory into the save file and produced a sentinel
   the loader could not recognize.
4. **`akgl_game_load_objectnamemap` swallowed read failures.** `CATCH` used
   directly inside `while (1)` breaks the loop, not the function, so a truncated
   or corrupt name table loaded as a successful game.
5. **`akgl_Actor_cmhf_up_on` and `_down_on` dereferenced `basechar` unguarded**,
   unlike their left and right counterparts.
6. **`akgl_actor_logic_movement` checked `actor` twice** instead of checking
   `actor->basechar` before dereferencing it.
7. **The gamepad handlers checked `appstate` three times each**, so a NULL event
   or a missing player actor was never caught and `player->state` was
   dereferenced regardless.

### Known and still open

1. **`akgl_render_and_compare` compares a texture against itself.**
   `src/util.c:228` and `src/util.c:245` both draw `t1`; `t2` is never rendered,
   so the function always passes and the image assertions in `tests/sprite.c`
   assert nothing.
2. **`akgl_tilemap_release` double-frees tileset textures.**
   `src/tilemap.c:849` destroys `dest->tilesets[i].texture` inside the *layers*
   loop; it should be `dest->layers[i].texture`. It also never NULLs the
   pointers, so a second release is a use-after-free.
3. **`akgl_registry_init` never initializes the properties registry.**
   `akgl_registry_init_properties()` is not called from `akgl_registry_init()`
   (`src/registry.c:27`), so `AKGL_REGISTRY_PROPERTIES` stays 0 unless the
   caller initializes it separately. `akgl_set_property` is then a silent no-op
   and `akgl_get_property` always returns the caller's default, which means
   `akgl_physics_init_arcade` and `akgl_render_init2d` silently ignore
   configuration. `akgl_game_init` does call it; a caller that does not use
   `akgl_game_init` does not get it.
4. **`akgl_path_relative_from` is a stub that leaks.** `src/util.c:105` claims a
   heap string, never writes `*dst`, and never releases it. 256 calls exhaust
   the string pool.
5. **`akgl_compare_sdl_surfaces` memcmps without checking geometry.**
   `src/util.c:208` compares `s1->pitch * s1->h` bytes of `s2` without verifying
   the surfaces share dimensions, pitch, or format.
6. **`akgl_string_initialize` overflows by four bytes when `init` is NULL.**
   `src/staticstring.c:17` does `memset(&obj->data, 0x00, sizeof(akgl_String))`,
   but `data` starts four bytes into the struct (after `refcount`), so the memset
   runs four bytes past the end.
7. **Savegame name lengths disagree between writer and reader.**
   `akgl_game_save_actors` writes spritesheet names at
   `AKGL_SPRITE_SHEET_MAX_FILENAME_LENGTH` (512) and character names at
   `AKGL_SPRITE_MAX_CHARACTER_NAME_LENGTH` (128), but `akgl_game_load` reads
   every table at `AKGL_ACTOR_MAX_NAME_LENGTH` (128). A save with any registered
   spritesheet cannot be read back. The current roundtrip test passes only
   because empty registries write nothing but zeroed sentinels.
8. **Heap acquire functions are asymmetric.** `akgl_heap_next_string` increments
   `refcount`; `next_actor`, `next_sprite`, `next_spritesheet`, and
   `next_character` do not. `tests/heap.c` pins the current behavior and says so;
   decide whether to make them symmetric or document the split.
9. **`tests/util.c` defines `test_akgl_collide_point_rectangle_logic` but
   `main()` never calls it.**
10. **`controller.h` declares functions that do not exist.** It declares
    `akgl_controller_handle_button_down`, `_button_up`, `_added`, and `_removed`,
    but `src/controller.c` defines them as `gamepad_handle_*`. Anything compiled
    against the header alone fails to link.
11. **`akgl_controller_pushmap` and `akgl_controller_default` accept negative map
    ids.** Both check `controlmapid >= AKGL_MAX_CONTROL_MAPS` but not
    `controlmapid < 0`, so a negative id indexes before `GAME_ControlMaps`.
12. **A failed controller-DB fetch silently destroys the tracked fallback.**
    `include/akgl/SDL_GameControllerDB.h` is committed deliberately so the
    library still builds if upstream disappears. But `mkcontrollermappings.sh`
    has no `set -e` and never checks `curl`'s exit status: on a failed fetch it
    writes `mappings.txt` empty, then overwrites the good tracked header with
    `AKGL_SDL_GAMECONTROLLER_DB_LEN 0` and an empty initializer — and exits 0.
    Verified by pointing the script at an unresolvable host.

    Two compounding problems:

    - `add_custom_command` at `CMakeLists.txt:89-93` declares
      `OUTPUT include/akgl/SDL_GameControllerDB.h` as a *relative* path, which
      CMake resolves against the binary directory, while the script writes to
      the source directory. The declared output never appears, so the command is
      permanently out of date and re-runs on every build — making every build
      depend on network access and leaving the file dirty in the working tree
      each time.
    - `const char *SDL_GAMECONTROLLER_DB[] = {\n};` is an empty initializer,
      which is a constraint violation in ISO C and compiles only as a GCC
      extension.

    Fix: have the script fetch to a temporary file, check `curl`'s status and a
    plausible minimum line count, and leave the existing header untouched on
    failure (exiting non-zero). Separately, make regeneration explicit — a
    dedicated `controllerdb` target, or an `OUTPUT` that matches where the
    script actually writes — so an ordinary build neither needs the network nor
    dirties the tree.

## Build notes

The vendored SDL satellite libraries are built into per-project subdirectories
that are not on the loader's default search path, and `LD_LIBRARY_PATH` is
searched ahead of RPATH — so a developer who has previously run `rebuild.sh` had
their installed `libakgl.so` shadow the one under test, and a developer who had
not saw every test abort before `main()` with "cannot open shared object file".
`CMakeLists.txt` now sets `BUILD_RPATH` on the library, the utility, and every
test target, and prepends the build tree to `LD_LIBRARY_PATH` for the CTest run.

## Carried over

1. **Make character-to-sprite state bindings release their references symmetrically.**
   `akgl_character_sprite_add()` increments the sprite refcount for every state-map
   binding, but no corresponding removal API exists. Replacing a state binding
   also leaves the previous sprite's refcount incremented. When
   `akgl_heap_release_character()` drops the character refcount to zero, it clears
   the character registry entry and zeroes the structure without enumerating
   `state_sprites`, decrementing the bound sprites, or destroying the SDL property
   map. Implement binding removal/replacement so each removed binding releases
   exactly one sprite reference. On final character release, enumerate every
   remaining binding (the existing `akgl_character_state_sprites_iterate()` release
   path may be reusable), release each reference, destroy `state_sprites`, and then
   clear the character. Add tests for removal, replacement, duplicate sprite
   bindings across multiple states, and final character release.

   `akgl_character_state_sprites_iterate` is still uncovered and is the natural
   place to start; `tests/actor.c` now covers `akgl_character_sprite_get` and the
   composite-state binding behavior it depends on.