2026-07-29 18:01:05 -04:00
# TODO
Record internal consistency findings and the controller DB defect
Add an "Internal consistency" section to TODO.md: 41 numbered findings
from a sweep of src/ and include/, each cited by file:line and grouped by
blast radius. These are convention problems, not a re-audit of behavior,
but several have live functional consequences:
- AKGL_TIME_ONESEC_MS is misnamed. Its value is nanoseconds-per-
millisecond, but akgl_game_state_lock uses it as a one-second budget
against a 100ms delay loop, so the lock blocks ~16 minutes.
- SUCCEED_RETURN inside an ATTEMPT block at tilemap.c:52 bypasses the
CLEANUP two lines below, leaking two heap strings on every successful
tilemap property lookup, against a 256-entry pool.
- AKGL_ACTOR_STATE_STRING_NAMES is declared [33] and defined [32], and
names bits 11 and 12 as UNDEFINED where actor.h defines MOVING_IN and
MOVING_OUT, so no character JSON can bind a sprite to those states.
- 19 non-static functions are defined in src/ but declared in no header,
and akgl_game_init_screen is declared but never defined.
- AKGL_COLLIDE_RECTANGLES has unbalanced parentheses and cannot compile.
- text.c:19 checks `name` twice and never validates `filepath`.
Item 30 is marked resolved by the reindent in the preceding commit.
Also add Defects #12: a failed controller-DB fetch silently destroys the
tracked fallback. include/akgl/SDL_GameControllerDB.h is committed on
purpose so the library still builds if upstream disappears, but
mkcontrollermappings.sh has no `set -e` and never checks curl's exit
status -- on failure it overwrites the good header with
AKGL_SDL_GAMECONTROLLER_DB_LEN 0 and an empty initializer, and exits 0.
Verified against an unresolvable host. CMakeLists.txt:89-93 compounds
this: the custom command's OUTPUT is relative, so CMake resolves it
against the binary dir while the script writes to the source dir, leaving
the command permanently out of date and re-running on every build.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-07-30 18:17:58 -04:00
## 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.
2026-07-31 06:55:25 -04:00
**Resolved ** alongside the text measurement work; `tests/text.c` asserts a
NULL filepath reports `AKERR_NULLPOINTER` .
Record internal consistency findings and the controller DB defect
Add an "Internal consistency" section to TODO.md: 41 numbered findings
from a sweep of src/ and include/, each cited by file:line and grouped by
blast radius. These are convention problems, not a re-audit of behavior,
but several have live functional consequences:
- AKGL_TIME_ONESEC_MS is misnamed. Its value is nanoseconds-per-
millisecond, but akgl_game_state_lock uses it as a one-second budget
against a 100ms delay loop, so the lock blocks ~16 minutes.
- SUCCEED_RETURN inside an ATTEMPT block at tilemap.c:52 bypasses the
CLEANUP two lines below, leaking two heap strings on every successful
tilemap property lookup, against a 256-entry pool.
- AKGL_ACTOR_STATE_STRING_NAMES is declared [33] and defined [32], and
names bits 11 and 12 as UNDEFINED where actor.h defines MOVING_IN and
MOVING_OUT, so no character JSON can bind a sprite to those states.
- 19 non-static functions are defined in src/ but declared in no header,
and akgl_game_init_screen is declared but never defined.
- AKGL_COLLIDE_RECTANGLES has unbalanced parentheses and cannot compile.
- text.c:19 checks `name` twice and never validates `filepath`.
Item 30 is marked resolved by the reindent in the preceding commit.
Also add Defects #12: a failed controller-DB fetch silently destroys the
tracked fallback. include/akgl/SDL_GameControllerDB.h is committed on
purpose so the library still builds if upstream disappears, but
mkcontrollermappings.sh has no `set -e` and never checks curl's exit
status -- on failure it overwrites the good header with
AKGL_SDL_GAMECONTROLLER_DB_LEN 0 and an empty initializer, and exits 0.
Verified against an unresolvable host. CMakeLists.txt:89-93 compounds
this: the custom command's OUTPUT is relative, so CMake resolves it
against the binary dir while the script writes to the source dir, leaving
the command permanently out of date and re-running on every build.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-07-30 18:17:58 -04:00
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` .
2026-07-30 02:08:38 -04:00
## Coverage status
2026-07-30 01:40:06 -04:00
2026-07-30 02:08:38 -04:00
Generated with:
2026-07-30 01:40:06 -04:00
```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` ).
2026-07-31 12:54:04 -04:00
**Line coverage 79.6%, function coverage 87.2%** (2189/2749 lines, 164/188
functions), up from 77.2% / 83.1% and from a 39.6% / 44.3% baseline. All 21
suites pass. (An earlier revision of this file recorded `character` as an
intentionally failing suite; it passes now, and nothing in `tests/character.c`
or `src/character.c` has changed since — the fix came from elsewhere in the tree
and this note was never updated.)
Note the trap described in "Known and still open" item 13 while regenerating
these numbers: a coverage tree rebuilt after a source edit fails `coverage_reset`
with `GCOV returncode was 5` . `find build-coverage -name '*.gcda' -delete` clears
it.
2026-07-30 02:08:38 -04:00
| File | Lines | Functions |
|---|---|---|
2026-07-31 12:54:04 -04:00
| `src/actor.c` | 205/258 (80%) | 16/18 |
2026-07-30 02:08:38 -04:00
| `src/assets.c` | 0/21 (0%) | 0/1 |
2026-07-31 12:54:04 -04:00
| `src/audio.c` | 229/248 (92%) | 22/23 |
2026-07-30 02:08:38 -04:00
| `src/character.c` | 104/118 (88%) | 6/7 |
2026-07-31 12:54:04 -04:00
| `src/controller.c` | 300/329 (91%) | 16/16 |
2026-07-31 07:17:18 -04:00
| `src/draw.c` | 253/267 (95%) | 11/12 |
| `src/error.c` | 9/9 (100%) | 1/1 |
2026-07-31 12:54:04 -04:00
| `src/game.c` | 128/235 (54%) | 12/17 |
2026-07-31 07:17:18 -04:00
| `src/heap.c` | 111/111 (100%) | 12/12 |
2026-07-30 02:08:38 -04:00
| `src/json_helpers.c` | 111/111 (100%) | 11/11 |
| `src/physics.c` | 140/140 (100%) | 10/10 |
2026-07-31 12:54:04 -04:00
| `src/registry.c` | 76/102 (74%) | 11/12 |
| `src/renderer.c` | 42/75 (56%) | 7/8 |
2026-07-30 02:08:38 -04:00
| `src/sprite.c` | 93/101 (92%) | 5/5 |
| `src/staticstring.c` | 16/17 (94%) | 2/2 |
2026-07-31 12:54:04 -04:00
| `src/text.c` | 48/48 (100%) | 4/4 |
2026-07-30 02:08:38 -04:00
| `src/tilemap.c` | 201/426 (47%) | 10/20 |
| `src/util.c` | 121/131 (92%) | 7/8 |
2026-07-31 07:17:18 -04:00
| `src/version.c` | 2/2 (100%) | 1/1 |
2026-07-30 02:08:38 -04:00
2026-07-31 07:30:18 -04:00
Branch coverage reads 21.2% and should not be used as a target. The akerror
2026-07-30 02:08:38 -04:00
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.
2026-07-31 07:30:18 -04:00
### Mutation testing
`scripts/mutation_test.py` was run over the three new files as a smoke check
(`--max-mutants 8` to 10 each, so these are samples rather than exhaustive
scores):
| File | Score | Surviving mutants |
|---|---|---|
| `src/draw.c` | 90% | Deleting the `FAIL_ZERO_BREAK` on `SDL_CreateTextureFromSurface` in `akgl_draw_flood_fill` |
| `src/audio.c` | 75% | Deleting `SUCCEED_RETURN` from the static `check_voice` ; deleting `spec.freq` before opening a device |
2026-07-31 12:54:04 -04:00
| `src/text.c` | 50% | Three in `akgl_text_rendertextat` , which had no test yet, plus one `SUCCEED_RETURN` deletion |
2026-07-31 07:30:18 -04:00
The first pass over `src/audio.c` scored 50% and named two real gaps, both of
which are now tested: nothing asserted that an * unconfigured * voice is audible
(the whole reason the table defaults to a square wave at full level rather than
a zeroed struct), and no envelope test used a non-zero attack * and * decay
together, so the decay measuring from the wrong origin survived. `src/draw.c`
scored 70% first and named one: the circle was only checked at its four axis
points, which a mis-signed octant reflection survives, so it now checks that
every plotted pixel has a mirror in the other three quadrants.
2026-07-31 12:54:04 -04:00
Re-run as a smoke check after the akbasic API work, again at 10 sampled
mutants each:
| File | Score | Surviving mutants |
|---|---|---|
| `src/controller.c` | 40% | Three `SDL_Log` deletions; `keybuffer_head` 's initialiser; `count > 0` in `keybuffer_attach_text` ; the `mod` a text-only entry is given |
| `src/audio.c` | 60% | The `break` on the last `switch` case; `errctx->handled` in the device callback; `ensure_voices()` in the mixer; the mixer's voice loop bound |
Only one of those was a real gap and it is now asserted: nothing checked that
a text-only ring entry reports no modifiers. Of the rest, three are equivalent
mutants rather than misses — a ring buffer whose head starts at 1 behaves
identically, `count > 0` cannot be false where it is checked, and deleting the
`break` on a `switch` 's last case changes nothing — and the mixer's `v <=`
bound reads one voice past a zeroed table, which is undefined rather than
observable. `SDL_Log` deletions are unobservable by construction.
2026-07-31 07:30:18 -04:00
What is left is honestly untestable from here. Deleting a `SUCCEED_RETURN`
leaves a non-void function falling off its end, which is undefined rather than
observably wrong, and the surviving SDL branches are allocation failures the
2026-07-31 12:54:04 -04:00
suite has no way to provoke. The three `src/text.c` survivors in
`akgl_text_rendertextat` are gone: it is tested now, against a software
renderer, and deleting any of its three backend checks fails the suite. That
was not free — the first draft of the test used a made-up `SDL_Renderer`
pointer, which SDL refuses on its own, so the deleted-check mutant survived it.
A live renderer was what made the check observable.
2026-07-31 07:30:18 -04:00
2026-07-30 02:08:38 -04:00
### 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%.
2026-07-31 07:17:18 -04:00
- `tests/audio.c` — every waveform, the ADSR envelope stage by stage, gate
2026-07-31 12:54:04 -04:00
expiry and release, the frequency sweep frame by frame, voice summing and
clamping, the master level, and device open/shutdown under the dummy driver.
92%.
2026-07-31 07:17:18 -04:00
- `tests/draw.c` — every primitive against a 64x64 software renderer with the
pixels read back, including flood-fill containment, save/paste roundtrip, and
that drawing restores the renderer's draw color. 95%.
2026-07-31 12:54:04 -04:00
- `tests/text.c` — font loading into the registry, both measurement entry points
against a monospaced fixture font, and drawing through a bound backend over a
software renderer, including every way the backend can be unusable. 100%.
- `tests/renderer.c` — the 2D vtable binding, frame start and end, both
`draw_texture` paths, the refusals a bound-but-unrendered backend gives, and
the `draw_mesh` stub. 56%; `akgl_render_init2d` and `akgl_render_2d_draw_world`
are what is left, and both want the harness.
- `tests/headers.c` — that `akgl/controller.h` compiles as the first include in
a translation unit. The assertion is the compile; `main()` only has to return
zero.
2026-07-30 02:08:38 -04:00
## Remaining work
### Needs the offscreen renderer harness
2026-07-31 12:54:04 -04:00
`akgl_render_init2d` and `akgl_render_2d_draw_world` in `src/renderer.c` (33
lines), `akgl_draw_background` in `src/draw.c` (13), `src/assets.c` (21),
2026-07-31 07:17:18 -04:00
`akgl_actor_render` /`actor_visible` in `src/actor.c` (53), and the drawing half
2026-07-31 12:54:04 -04:00
of `src/tilemap.c` all need a live `renderer` global, a window, or the world
globals.
`akgl_text_rendertextat` was on this list and is not any more: `tests/text.c`
builds a software renderer and binds a backend to it with `akgl_render_bind2d`
in nine lines, which is enough for anything that only needs * a * renderer rather
than the whole world. `tests/renderer.c` and `tests/draw.c` do the same. The
harness is still wanted, but for what is left it is a convenience rather than
the blocker it was.
2026-07-30 02:08:38 -04:00
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
2026-07-31 12:54:04 -04:00
`SDL_CreateWindowAndRenderer` , and point the global `renderer` at it. Seven
2026-07-30 02:08:38 -04:00
existing tests hand-roll this today (`tests/sprite.c:194` , `tests/character.c:200` ,
2026-07-31 12:54:04 -04:00
`tests/tilemap.c:421` , `tests/charviewer.c:42` , plus `tests/draw.c` ,
`tests/renderer.c` and `tests/text.c` ); collapse them onto the shared harness in
the same change.
2026-07-30 02:08:38 -04:00
Then:
2026-07-31 12:54:04 -04:00
- **`tests/renderer.c` extensions** — `akgl_render_init2d` populating the camera
from the property registry, and `draw_world` layer ordering. `tests/renderer.c`
exists and covers everything that does not need a world or the registry: the
vtable binding, frame start/end against a NULL `sdl_renderer` , both
`draw_texture` paths including `angle != 0` with a NULL center, and the
`draw_mesh` stub. Note `defflags` at `src/renderer.c:113` is uninitialized
until the `if` body runs.
2026-07-31 07:17:18 -04:00
- **`tests/assets.c` ** — BGM loading into `AKGL_REGISTRY_MUSIC` under the dummy
audio driver.
- **`tests/draw.c` extensions** — `akgl_draw_background` at zero, negative and
oversized dimensions. `tests/draw.c` exists and covers every other primitive
against a software renderer; `akgl_draw_background` is the one function in the
file that still reads the global `renderer` rather than taking a backend.
2026-07-30 02:08:38 -04:00
- **`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.**
2026-07-30 01:40:06 -04:00
`src/tilemap.c:849` destroys `dest->tilesets[i].texture` inside the * layers *
2026-07-30 02:08:38 -04:00
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.**
2026-07-30 01:40:06 -04:00
`akgl_registry_init_properties()` is not called from `akgl_registry_init()`
2026-07-30 02:08:38 -04:00
(`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.**
2026-07-30 01:40:06 -04:00
`src/util.c:208` compares `s1->pitch * s1->h` bytes of `s2` without verifying
2026-07-30 02:08:38 -04:00
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` .
Record internal consistency findings and the controller DB defect
Add an "Internal consistency" section to TODO.md: 41 numbered findings
from a sweep of src/ and include/, each cited by file:line and grouped by
blast radius. These are convention problems, not a re-audit of behavior,
but several have live functional consequences:
- AKGL_TIME_ONESEC_MS is misnamed. Its value is nanoseconds-per-
millisecond, but akgl_game_state_lock uses it as a one-second budget
against a 100ms delay loop, so the lock blocks ~16 minutes.
- SUCCEED_RETURN inside an ATTEMPT block at tilemap.c:52 bypasses the
CLEANUP two lines below, leaking two heap strings on every successful
tilemap property lookup, against a 256-entry pool.
- AKGL_ACTOR_STATE_STRING_NAMES is declared [33] and defined [32], and
names bits 11 and 12 as UNDEFINED where actor.h defines MOVING_IN and
MOVING_OUT, so no character JSON can bind a sprite to those states.
- 19 non-static functions are defined in src/ but declared in no header,
and akgl_game_init_screen is declared but never defined.
- AKGL_COLLIDE_RECTANGLES has unbalanced parentheses and cannot compile.
- text.c:19 checks `name` twice and never validates `filepath`.
Item 30 is marked resolved by the reindent in the preceding commit.
Also add Defects #12: a failed controller-DB fetch silently destroys the
tracked fallback. include/akgl/SDL_GameControllerDB.h is committed on
purpose so the library still builds if upstream disappears, but
mkcontrollermappings.sh has no `set -e` and never checks curl's exit
status -- on failure it overwrites the good header with
AKGL_SDL_GAMECONTROLLER_DB_LEN 0 and an empty initializer, and exits 0.
Verified against an unresolvable host. CMakeLists.txt:89-93 compounds
this: the custom command's OUTPUT is relative, so CMake resolves it
against the binary dir while the script writes to the source dir, leaving
the command permanently out of date and re-running on every build.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-07-30 18:17:58 -04:00
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.
2026-07-30 22:58:55 -04:00
13. **A stale build tree in the source directory breaks the coverage run. **
`CMakeLists.txt:208` and `CMakeLists.txt:223` pass
`--root "${CMAKE_CURRENT_SOURCE_DIR}"` to gcovr, and gcovr searches for
`.gcda` /`.gcno` files under the root. The `--object-directory` argument on
the line below each does * not * narrow that search: per `gcovr --help` it
only identifies "the path between gcda files and the directory where the
compiler was originally run". So every instrumented build tree left inside
the source directory is folded into the report alongside the one actually
being measured.
With a `build-coverage/` from an earlier session still present, a freshly
configured `-DAKGL_COVERAGE=ON` tree fails in `coverage_reset` , before any
test runs:
AssertionError: Got function akgl_game_lowfps on multiple lines: 45, 46.
45 and 46 are that function's line numbers before and after an unrelated
`#include` was added to `src/game.c` : gcovr found 18 stale `.gcno` files
describing the old layout, merged them with the current ones, and could not
reconcile the two. Moving `build-coverage/` aside makes the same tree pass
18/18. Verified with gcovr 7.0.
The `build*/` entry in `.gitignore` hides these trees from `git status` ,
which makes the state easier to get into and no easier to notice.
2026-07-31 07:30:18 -04:00
A second, smaller version of the same thing: rebuilding an existing
coverage tree after editing a test leaves `.gcda` files describing the old
object layout, and `coverage_reset` -- whose whole job is to delete them --
fails with `GCOV returncode was 5` before it gets the chance. `find
build-coverage -name '*.gcda' -delete` clears it. Observed with gcovr 7.0.
2026-07-30 22:58:55 -04:00
Fix: pass the build tree to gcovr as an explicit search path instead of
letting it default to `--root` , so only the tree under measurement is
considered. Touches the two `add_test` blocks at `CMakeLists.txt:204-211`
and `CMakeLists.txt:220-229` ; nothing outside the `AKGL_COVERAGE` branch.
2026-07-30 02:08:38 -04:00
File five defects akbasic found consuming the new APIs
akbasic has now consumed all four of the API gaps this file listed as blocking
it: its text sink, graphics backend, audio backend and input backend are written
and tested, and the BASIC 7.0 graphics, sound and console verbs work against
them. Doing that turned up six things worth fixing here -- five of which still
are.
Four are workarounds akbasic had to write to build at all, and each is commented
at its site over there with the words "filed upstream":
- An embedded libakgl requires SDL, SDL_image, SDL_mixer, SDL_ttf and jansson to
be installed, while vendoring all five. The dependency block is gated on being
top-level, so an add_subdirectory() consumer takes the find_package path and
fails with the submodules sitting right there in deps/.
- include/akgl/controller.h does not compile on its own: it declares handlers
taking an akgl_Actor * and includes nothing that declares the type. src/
never notices because it includes game.h first.
- akgl_render_init2d() fuses two separable jobs -- creating a window, and
installing the backend vtable -- so a caller who already owns an SDL_Renderer
has no way to get the second without the first. That caller is exactly the
embedding host this section exists to serve.
- akgl_text_rendertextat() dereferences renderer->draw_texture with no check,
which is the state item 7 leaves a host in. Same class of defect the draw
commit added a test for; the text path still has it.
The fifth is a real API gap: SOUND's frequency sweep has no equivalent, and it
cannot be faked in the interpreter without tying audible pitch to the host's
frame rate, so it has to be advanced on the mixer's own frame counter.
The sixth was that 42b60f7 shipped 22 public symbols under an unchanged VERSION
and soname. 1066ac7 fixed that independently while this was being written -- the
two crossed rather than one following the other -- so it is filed as resolved
rather than dropped. The reason is the part worth keeping: akbasic could not
feature-test for the new API and had to pin libakgl by submodule commit in its
README until that landed, which is the concrete cost of an additive release
under an unchanged soname.
All five outstanding items were re-verified against this tree after the rebase,
not just carried forward: controller.h still includes neither actor.h nor a
forward declaration, render_init2d still calls SDL_CreateWindowAndRenderer
before installing the vtable, text.c:62 still reaches through draw_texture
unchecked, the vendored-dependency block is still gated on being top-level, and
there is still no akgl_audio_sweep.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-07-31 08:48:40 -04:00
14. **22 public symbols shipped without a version or soname bump. ** 42b60f7 added
`akgl_draw_point` , `_line` , `_rect` , `_filled_rect` , `_circle` , `_flood_fill` ,
`_copy_region` and `_paste_region` ; `akgl_audio_init` , `_shutdown` , `_tone` , `_stop` ,
`_waveform` , `_envelope` , `_volume` , `_voice_active` and `_mix` ;
`akgl_controller_poll_key` and `_flush_keys` ; and `akgl_text_measure` and
`_measure_wrapped` . `project(akgl VERSION 0.1.0)` and the `libakgl.so.0.1` soname were
both left alone.
That contradicts this repository's own stated rule, which `akbasic` 's `CLAUDE.md` quotes
back: *"for both 0.x libraries the soname carries `MAJOR.MINOR` deliberately: 0.1 and 0.2
are different ABIs."* Adding exported symbols under an unchanged soname has two
consequences, and the first is the one that bites:
- A binary compiled against the new headers links happily against a `libakgl.so.0.1`
built from the old tree, because the soname says they are the same ABI. It fails at
symbol resolution rather than at configure time.
- `AKGL_VERSION_AT_LEAST(0, 1, 0)` is true for both trees, so a consumer cannot
feature-test for the new API at all. `akbasic` pins the requirement by submodule commit
instead, and says so in its README, which is not a thing a released library should make
anybody do.
Fix: bump `project()` to 0.2.0, which carries the soname to `libakgl.so.0.2` through the
existing logic at `CMakeLists.txt:138` . `akstdlibConfigVersion.cmake` 's `SameMinorVersion`
equivalent then refuses the mismatch at configure time as well.
**Resolved by 1066ac7 ** , which did exactly that while this was being written — the two
crossed, rather than one following the other. Kept rather than deleted because the * reason *
is still the useful part: `akbasic` could not feature-test for the new API and had to pin
`libakgl` by submodule commit in its README until this landed, which is the concrete cost of
an additive release under an unchanged soname. Worth remembering the next time a handful of
symbols looks too small to bump for.
Record the twelve defects reading the code for documentation turned up
Writing an honest @throws list means reading the implementation against the
contract its header claimed, and the two disagreed in more places than the
status codes. Recorded as items 14-25 under Defects, ordered by blast radius,
each with file, line, consequence and what closing it would touch. They are
also noted inline as @note or @warning on the function concerned, so a reader
of the generated documentation finds them without coming here first.
The three that matter most:
- akgl_path_relative returns from inside its ENOENT handler, skipping the
RELEASE_ERROR that FINISH carries, so it leaks one of libakerror's 128
error-context slots per call. That is not a rare path - it is the ordinary
one, taken whenever an asset names a neighbour relative to its own file.
- akgl_sprite_load_json writes json_array_size() entries into a 16-element
frameids array with no bound check, and does it through a uint32_t * cast of
a uint8_t *. The same unbounded shape appears twice more in the tilemap
loader, for objects per layer and tilesets per map.
- akgl_heap_release_character zeroes the character without walking its
state-to-sprite map, so every sprite reference it took is lost and the SDL
property set holding the map is leaked. Releasing characters between levels
exhausts the sprite pool. akgl_character_state_sprites_iterate exists to do
that walk and is not called from there.
Also: the background music never loops, because MIX_PROP_PLAY_LOOPS_NUMBER is
set on property set 0 rather than one akgl_load_start_bgm owns; and
akgl_get_json_with_default defaults on AKERR_INDEX while the array accessors
report a short array as AKERR_OUTOFBOUNDS, so the optional-element form
silently does not work for arrays.
No code changes. Every src/*.c:NNN citation in the new section was checked
against the file after the preceding commit shifted the line numbers.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-07-31 11:02:40 -04:00
### Found while rewriting the Doxygen comments
Each of these came out of reading an implementation against the contract its
header claimed. They are recorded inline as `@note` or `@warning` on the
function concerned, so a reader of the generated documentation finds them
without coming here first. Ordered by blast radius.
15. * * `akgl_path_relative` leaks one error-context slot per call on its fallback
path.** `src/util.c:120` returns `akgl_path_relative_root(...)` from inside
the `HANDLE(e, ENOENT)` block. `FINISH` is what carries `RELEASE_ERROR` , so
returning before it never gives the context back. libakerror hands these out
of a fixed `AKERR_ARRAY_ERROR[128]` , and this is not a rare path — it is the
* ordinary * one, taken every time an asset names a neighbour relative to its
own file rather than to the working directory. Every tileset image, layer
image and spritesheet in a map costs a slot, permanently. Once the array is
exhausted every subsequent failure anywhere in the process has nowhere to
report from.
Fix: assign the result to a local, `break` , and return after `FINISH` ; or
hoist the fallback out of the handler entirely and let the `HANDLE` block
only record that a retry is wanted. Touches `src/util.c:117-121` only.
16. * * `akgl_sprite_load_json` does not bound the `frames` array.**
`src/sprite.c:165` sets `obj->frames` from `json_array_size()` and
`src/sprite.c:167` then writes that many entries into
`frameids[AKGL_SPRITE_MAX_FRAMES]` , which is 16. A sprite definition with 17
or more frames writes past the array into the rest of `akgl_Sprite` , and
past the struct into the neighbouring pool slot. It is also written through
a `uint32_t *` cast of a `uint8_t *` , so each element write touches four
bytes; that happens to work on a little-endian machine because the following
iterations overwrite the spill and the last write lands in the struct's
alignment padding, but it is not portable and it is what makes the overflow
reach four bytes past `frames` rather than one.
Fix: refuse a `frames` array longer than `AKGL_SPRITE_MAX_FRAMES` with
`AKERR_OUTOFBOUNDS` , and read into an `int` local before narrowing to
`uint8_t` . Touches `src/sprite.c:164-169` .
17. **Two more unbounded array loads in the tilemap loader. **
`src/tilemap.c:387-389` walks a Tiled object layer straight into
`curlayer->objects[j]` with no check against
`AKGL_TILEMAP_MAX_OBJECTS_PER_LAYER` (128), and `src/tilemap.c:278-280`
fills `dest->tilesets[i]` with no check against `AKGL_TILEMAP_MAX_TILESETS`
(16).
Both are the same shape as item 16 and both are reachable from an ordinary
map file: 128 objects is not a large object layer. Note that
`akgl_tilemap_load_layers` * does * bound its loop and raises
`AKERR_OUTOFBOUNDS` , so the pattern to copy is already in the same file.
Fix: add the bound check at the top of each loop body, matching
`akgl_tilemap_load_layers` . Touches `src/tilemap.c:387` and
`src/tilemap.c:278` .
18. * * `akgl_get_json_with_default` defaults on a status the array accessors never
raise.** `src/json_helpers.c:164-165` handles `AKERR_KEY` and `AKERR_INDEX` ,
but `akgl_get_json_array_index_object` , `_integer` and `_string` all report
a short array as `AKERR_OUTOFBOUNDS` . So the "this element is optional" form
silently does not work for an array — the error propagates instead of being
replaced by the default. Only the object accessors, which do raise
`AKERR_KEY` , are actually served by this function today. Nothing in tree
passes an array accessor's error here, which is why it has not been noticed.
Fix: add a third `HANDLE_GROUP(err, AKERR_OUTOFBOUNDS)` . Note that the
existing two rely on `HANDLE_GROUP` not emitting a `break` , so the `KEY`
case falls through into the `INDEX` body — a third arm has to go * above * the
one holding the `memcpy` , not below it. Touches `src/json_helpers.c:164-167` .
19. **The background music never loops. ** `src/assets.c:20` initialises
`bgmprops` to 0, `src/assets.c:44` sets `MIX_PROP_PLAY_LOOPS_NUMBER` on it,
and `src/assets.c:46` plays the track with it. 0 is SDL's "no property set"
sentinel, not a set this function owns, so the write is rejected —
unchecked — and the play call is given no options. The music plays once and
stops. `akgl_load_start_bgm` reports success either way.
Fix: `SDL_CreateProperties()` into `bgmprops` , check it, and destroy it in
`CLEANUP` . Touches `src/assets.c:20-47` .
20. * * `akgl_character_sprite_add` leaks a sprite reference when a state is
remapped.** `src/character.c:51` writes the new sprite over any existing
entry for that state without releasing the one it displaces, whose refcount
was incremented when it was added. The displaced sprite's pool slot is never
reclaimed. The write itself is also unchecked, so a failure to record the
mapping is reported as success.
Fix: read the existing entry first and `akgl_heap_release_sprite` it, and
check `SDL_SetPointerProperty` 's return. Touches `src/character.c:43-55` .
21. * * `akgl_heap_release_character` abandons the whole state-to-sprite map.**
`src/heap.c:150` zeroes the character without walking `state_sprites` , so
every sprite reference the character took in `akgl_character_sprite_add` is
lost and the `SDL_PropertiesID` holding the map is never destroyed. Loading
and releasing characters in a loop — level to level — exhausts the sprite
pool and leaks an SDL property set each time.
`akgl_character_state_sprites_iterate` with `AKGL_ITERATOR_OP_RELEASE` exists
precisely to do this walk and is not called from here.
Fix: enumerate `state_sprites` with that iterator, then
`SDL_DestroyProperties` , before the `memset` . Touches `src/heap.c:145-151` .
22. * * `akgl_path_relative_root` uses `FAIL_RETURN` inside its `ATTEMPT` block.**
`src/util.c:75` returns past `CLEANUP` on the over-long-path branch, so the
two scratch strings claimed at `src/util.c:67-68` are never released. This is
the exact hazard AGENTS.md documents under the error-handling protocol, and
the same class as the heap-string leak already fixed in
`akgl_get_json_tilemap_property` . Smaller blast radius than item 15 because
the branch is rare, but it is a mechanical fix.
Fix: `FAIL_BREAK` . Touches `src/util.c:75` .
23. **Three smaller leaks on failure paths. ** All the same shape: a resource
acquired before an `ATTEMPT` /`CLEANUP` pair, or released only on the success
path.
- `src/renderer.c:26-32` — `akgl_render_init2d` releases the two pooled
strings holding the screen dimensions only after both `aksl_atoi` calls
succeed, so a non-numeric `game.screenwidth` leaks two string slots.
- `src/controller.c:80` — `akgl_controller_open_gamepads` calls
`SDL_free(gamepads)` only after the loop completes, so a gamepad that
fails to open leaks the enumeration array.
- `src/text.c:57-64` — `akgl_text_rendertextat` destroys the surface and
texture only on the success path, so a failed texture upload or a failed
draw leaks both. On a HUD line redrawn every frame that is a leak per
frame.
24. * * `akgl_get_property` reads past the end of the property value.**
`src/registry.c:182` copies a fixed `AKGL_MAX_STRING_LENGTH` bytes out of
whatever `SDL_GetStringProperty` returned, rather than that string's length.
The destination is a full-sized pool string so nothing is corrupted, but the
source is an ordinary NUL-terminated string owned by SDL and the read runs
up to PATH_MAX bytes past its end. Benign in practice and immediately fatal
under ASAN, which is the reason to fix it rather than leave it.
Fix: `aksl_strlcpy` or an explicit length. Touches `src/registry.c:181-186` .
25. * * `akgl_character_load_json_state_int_from_strings` checks the same argument
twice.** `src/character.c:123` guards `states` and `src/character.c:124`
guards `states` again under the message "NULL destination integer" — the
guard for `dest` was never written. A `NULL` `dest` is dereferenced at
`src/character.c:132` . Only reachable from inside the character loader, which
always passes a real pointer, so it is a latent hole rather than a live bug.
Fix: change the second guard's subject to `dest` . Touches
`src/character.c:124` .
26. * * `akgl_actor_render` computes a sprite's drawn height from its width.**
`src/actor.c:276` sets `dest.h = curSprite->width * obj->scale` where it
should read `curSprite->height` . Every actor is therefore drawn square, and
a non-square sprite is stretched or squashed. Invisible in the current
assets because they are square; it will not stay that way.
Fix: one word. Touches `src/actor.c:276` . Worth a test that renders a
deliberately non-square sprite and asserts on the destination rectangle.
2026-07-31 12:54:04 -04:00
### Found while closing the akbasic API gaps
27. * * `akgl_text_rendertextat` refuses the empty string, and `akgl_text_measure` accepts it.**
SDL_ttf returns `NULL` with "Text has zero width" from both `TTF_RenderText_Blended` and
`TTF_RenderText_Blended_Wrapped` for `""` , so `src/text.c:56` 's `FAIL_ZERO_RETURN` on the
surface reports `AKERR_NULLPOINTER` for what a caller means as "draw nothing". Verified
directly against SDL_ttf 3.x with the fixture font.
`akgl_text_measure("")` is documented as legal and returns 0 wide by one line high, which
is what a cursor sitting on an empty line needs, so the two halves of the same header
disagree about the same string. The functional consequence is a caller that draws a line of
text which may be empty — a line editor, a `PRINT` of an empty string, a HUD field that has
not been filled in yet — has to test for it before every call or handle a failure that
means nothing went wrong.
**Fix: ** return success without rasterizing when `text[0] == '\0'` , matching the measure
side, and say so in the header. Touches `akgl_text_rendertextat` only. `tests/text.c` has
the case written and deliberately not asserted; it would become a `TEST_EXPECT_OK` . Until
then the header carries the wart as a `@note` pointing here.
2026-07-30 02:08:38 -04:00
## 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.
2026-07-30 01:40:06 -04:00
2026-07-31 06:29:17 -04:00
## API gaps blocking akbasic
`akbasic` (the C port of the BASIC interpreter, `source.starfort.tech/andrew/akbasic` ) is
File five defects akbasic found consuming the new APIs
akbasic has now consumed all four of the API gaps this file listed as blocking
it: its text sink, graphics backend, audio backend and input backend are written
and tested, and the BASIC 7.0 graphics, sound and console verbs work against
them. Doing that turned up six things worth fixing here -- five of which still
are.
Four are workarounds akbasic had to write to build at all, and each is commented
at its site over there with the words "filed upstream":
- An embedded libakgl requires SDL, SDL_image, SDL_mixer, SDL_ttf and jansson to
be installed, while vendoring all five. The dependency block is gated on being
top-level, so an add_subdirectory() consumer takes the find_package path and
fails with the submodules sitting right there in deps/.
- include/akgl/controller.h does not compile on its own: it declares handlers
taking an akgl_Actor * and includes nothing that declares the type. src/
never notices because it includes game.h first.
- akgl_render_init2d() fuses two separable jobs -- creating a window, and
installing the backend vtable -- so a caller who already owns an SDL_Renderer
has no way to get the second without the first. That caller is exactly the
embedding host this section exists to serve.
- akgl_text_rendertextat() dereferences renderer->draw_texture with no check,
which is the state item 7 leaves a host in. Same class of defect the draw
commit added a test for; the text path still has it.
The fifth is a real API gap: SOUND's frequency sweep has no equivalent, and it
cannot be faked in the interpreter without tying audible pitch to the host's
frame rate, so it has to be advanced on the mixer's own frame counter.
The sixth was that 42b60f7 shipped 22 public symbols under an unchanged VERSION
and soname. 1066ac7 fixed that independently while this was being written -- the
two crossed rather than one following the other -- so it is filed as resolved
rather than dropped. The reason is the part worth keeping: akbasic could not
feature-test for the new API and had to pin libakgl by submodule commit in its
README until that landed, which is the concrete cost of an additive release
under an unchanged soname.
All five outstanding items were re-verified against this tree after the rebase,
not just carried forward: controller.h still includes neither actor.h nor a
forward declaration, render_init2d still calls SDL_CreateWindowAndRenderer
before installing the vtable, text.c:62 still reaches through draw_texture
unchecked, the vendored-dependency block is still gated on being top-level, and
there is still no akgl_audio_sweep.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-07-31 08:48:40 -04:00
being built to link into `libakgl` as a scripting engine for game authors.
2026-07-31 12:54:04 -04:00
**All ten items are resolved.** Items 1 through 4 landed first and `akbasic` has since
consumed every one of them: its `libakgl` -backed text sink, its graphics backend, its sound
backend and its input backend are written and tested, and the BASIC 7.0 graphics verbs
(`GRAPHIC` , `COLOR` , `DRAW` , `BOX` , `CIRCLE` , `PAINT` , `SCALE` , `SSHAPE` , `GSHAPE` , `LOCATE` ),
the sound verbs (`SOUND` , `ENVELOPE` , `VOL` , `PLAY` , `TEMPO` ) and the console input verbs
(`GET` , `GETKEY` , `SCNCLR` ) all work against them.
File five defects akbasic found consuming the new APIs
akbasic has now consumed all four of the API gaps this file listed as blocking
it: its text sink, graphics backend, audio backend and input backend are written
and tested, and the BASIC 7.0 graphics, sound and console verbs work against
them. Doing that turned up six things worth fixing here -- five of which still
are.
Four are workarounds akbasic had to write to build at all, and each is commented
at its site over there with the words "filed upstream":
- An embedded libakgl requires SDL, SDL_image, SDL_mixer, SDL_ttf and jansson to
be installed, while vendoring all five. The dependency block is gated on being
top-level, so an add_subdirectory() consumer takes the find_package path and
fails with the submodules sitting right there in deps/.
- include/akgl/controller.h does not compile on its own: it declares handlers
taking an akgl_Actor * and includes nothing that declares the type. src/
never notices because it includes game.h first.
- akgl_render_init2d() fuses two separable jobs -- creating a window, and
installing the backend vtable -- so a caller who already owns an SDL_Renderer
has no way to get the second without the first. That caller is exactly the
embedding host this section exists to serve.
- akgl_text_rendertextat() dereferences renderer->draw_texture with no check,
which is the state item 7 leaves a host in. Same class of defect the draw
commit added a test for; the text path still has it.
The fifth is a real API gap: SOUND's frequency sweep has no equivalent, and it
cannot be faked in the interpreter without tying audible pitch to the host's
frame rate, so it has to be advanced on the mixer's own frame counter.
The sixth was that 42b60f7 shipped 22 public symbols under an unchanged VERSION
and soname. 1066ac7 fixed that independently while this was being written -- the
two crossed rather than one following the other -- so it is filed as resolved
rather than dropped. The reason is the part worth keeping: akbasic could not
feature-test for the new API and had to pin libakgl by submodule commit in its
README until that landed, which is the concrete cost of an additive release
under an unchanged soname.
All five outstanding items were re-verified against this tree after the rebase,
not just carried forward: controller.h still includes neither actor.h nor a
forward declaration, render_init2d still calls SDL_CreateWindowAndRenderer
before installing the vtable, text.c:62 still reaches through draw_texture
unchecked, the vendored-dependency block is still gated on being top-level, and
there is still no akgl_audio_sweep.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-07-31 08:48:40 -04:00
2026-07-31 12:54:04 -04:00
Doing that turned up **items 5 through 9 ** . Four were things `akbasic` had to work around to
build at all; each workaround is commented at its site over there with the words "filed
upstream". **Those workarounds can now be deleted ** — including the five `add_subdirectory`
lines for the vendored SDL projects and the hand-assigned render vtable in
`tests/akgl_backends.c` .
2026-07-31 06:29:17 -04:00
2026-07-31 11:19:56 -04:00
Building the * standalone * SDL frontend on top of all that — a real window, a real event pump
and a line editor — turned up **item 10 ** , and re-confirmed items 6 and 7: both workarounds
had to be written a second time, in `src/frontend_akgl.c` , because a host that owns a window is
exactly the caller they inconvenience.
2026-07-31 12:54:04 -04:00
These were filed here rather than worked around in `akbasic` because growing `libakgl` to serve
2026-07-31 06:29:17 -04:00
a consumer is the wanted outcome. Each entry says what the BASIC verb needs, what the
`akgl_*` entry point should look like, and what would cover it.
2026-07-31 12:54:04 -04:00
Items 7, 9 and 10 add public symbols and item 9 adds three fields to `akgl_AudioVoice` , so the
project version goes to **0.3.0 ** and the soname with it — an 0.2 consumer cannot be handed
this library and told it is compatible.
2026-07-31 06:29:17 -04:00
1. **No way to measure rendered text. ** `include/akgl/text.h` exposes `akgl_text_loadfont()`
and `akgl_text_rendertextat()` , and nothing that reports how large a string will be in a
given font. A terminal-style text surface cannot be built on that: a cursor needs the
advance width of one cell, and wrapping needs to know where a string crosses the right
margin. The reference interpreter got this from SDL2_ttf's `font.SizeUTF8("A")` and derived
its whole character grid from it.
Wants `akerr_ErrorContext AKERR_NOIGNORE *akgl_text_measure(TTF_Font *font, char *text, int *w, int *h);`
over `TTF_GetStringSize` , and probably a companion
`akgl_text_measure_wrapped(font, text, wraplength, w, h)` matching the `wraplength`
argument `akgl_text_rendertextat` already takes. Tests: a known string in a known font at a
known size, the empty string, and a wrapped string wide enough to force two lines.
This is the only one of the four that blocks work already designed and waiting. * * `akbasic`
cannot render any output through `libakgl` until it lands.**
2026-07-31 06:55:25 -04:00
**Resolved. ** `akgl_text_measure(font, text, w, h)` and
`akgl_text_measure_wrapped(font, text, wraplength, w, h)` are in
`include/akgl/text.h` , over `TTF_GetStringSize` and `TTF_GetStringSizeWrapped` .
Neither needs a renderer. A negative `wraplength` is refused with
`AKERR_OUTOFBOUNDS` rather than passed through, because SDL_ttf reads it as a
very large unsigned width and silently stops wrapping. `tests/text.c` covers
both against `tests/assets/akgl_test_mono.ttf` , a 10 KB monospaced ASCII
subset added for the purpose — being monospaced, it lets the suite assert
`width("AAAA") == 4 * width("A")` instead of hardcoding glyph metrics that
FreeType is free to round differently. Same change fixed item 39 below:
`akgl_text_loadfont` checked `name` twice and never checked `filepath` .
2026-07-31 06:29:17 -04:00
2. **No immediate-mode drawing. ** `include/akgl/draw.h` declares exactly one function,
`akgl_draw_background(int w, int h)` , and `src/draw.c` is at 0% coverage. BASIC 7.0's
graphics verbs are all immediate-mode plotting against the current screen: `DRAW` (line and
point), `BOX` , `CIRCLE` , `PAINT` (flood fill), `LOCATE` (set the pixel cursor), `COLOR` ,
and `SSHAPE` /`GSHAPE` (save and restore a rectangle of pixels).
Wants an `akgl_draw_*` family taking the renderer the host already initialized --
`akgl_draw_line` , `_rect` , `_filled_rect` , `_circle` , `_point` , `_flood_fill` ,
`_copy_region` -- in the shape of the existing `akgl_render_2d_draw_texture` . SDL3's
`SDL_RenderLine` /`SDL_RenderRect` /`SDL_RenderFillRect` cover most of it; the circle and the
flood fill do not exist in SDL3 and need writing. Tests belong with the offscreen renderer
harness described under "Remaining work": render a known shape, read the target back, and
compare against a reference surface with the existing `akgl_compare_sdl_surfaces` .
Draw shapes immediately against a renderer
BASIC 7.0's graphics verbs -- DRAW, BOX, CIRCLE, PAINT, SSHAPE and GSHAPE -- all
plot against the current screen right now rather than adding to a scene, and
draw.h had exactly one function in it. This adds akgl_draw_point, _line, _rect,
_filled_rect, _circle, _flood_fill, _copy_region and _paste_region, each taking
the akgl_RenderBackend the host already initialized.
Color is an argument rather than a current-color global, so there is no second
copy of state to disagree with the caller's own, and each call restores the
renderer's draw color when it is done. SDL3 has no circle and no flood fill: the
circle is a midpoint circle, and the fill reads the target back, walks the
region with a fixed 4096-entry span stack, and blits back only the bounding box
of what changed. Exhausting that stack reports AKERR_OUTOFBOUNDS and says in the
header that the region is left partially filled.
tests/draw.c draws into a 64x64 software renderer under the dummy video driver
and reads the pixels back, so it needs no display and no offscreen harness.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-07-31 07:05:36 -04:00
**Resolved. ** `include/akgl/draw.h` now declares `akgl_draw_point` , `_line` ,
`_rect` , `_filled_rect` , `_circle` , `_flood_fill` , `_copy_region` and
`_paste_region` , all taking the `akgl_RenderBackend *` the host initialized,
in the shape of `akgl_render_2d_draw_texture` . Decisions worth knowing:
- **Color is an argument, not state.** There is no current-color global to
get out of step with the caller's own. Each call saves and restores the
renderer's draw color, so drawing a line does not change what the host's
next `SDL_RenderClear` paints. `tests/draw.c` asserts that.
- **The circle is a midpoint circle**, integer arithmetic with eight-way
symmetry, plotted eight points per step through `SDL_RenderPoints` .
- **The flood fill reads the target back, fills on the CPU, and blits only
the bounding box of what changed.** It keeps a fixed
`AKGL_DRAW_MAX_FLOOD_SPANS` (4096) stack of horizontal runs at file scope
rather than recursing per pixel; running out reports `AKERR_OUTOFBOUNDS`
and leaves the region partially filled, which is stated in the header. It
is therefore not reentrant — neither is anything else that draws to a
single `SDL_Renderer` .
- `_copy_region` allocates when `*dest` is `NULL` and otherwise copies into
the caller's surface, matching `akgl_get_json_string_value` and friends. A
region that would be clipped by the target edge is refused rather than
silently returning a smaller surface.
`tests/draw.c` draws into a 64x64 software renderer under the dummy video
driver and reads pixels back, so it did not need the offscreen harness. That
harness is still wanted for `src/renderer.c` , `src/text.c` and `src/assets.c` .
`akgl_draw_background` is untouched and still outside the error protocol
(item 35).
2026-07-31 06:29:17 -04:00
3. **No audio API at all. ** `SDL3_mixer` is a vendored dependency and `registry.h` declares
`AKGL_REGISTRY_MUSIC` , but there is no `src/audio.c` , no `include/akgl/audio.h` , and no
`akgl_*` symbol that opens a mixer, loads a chunk, or plays a note. BASIC 7.0's sound verbs
are `SOUND` (a tone on a voice, with a duration), `PLAY` (a string of notes in a
Commodore-specific notation), `ENVELOPE` (ADSR per voice), `FILTER` , `VOL` and `TEMPO` .
`PLAY` and `ENVELOPE` want a synthesised voice rather than a sample, which SDL3_mixer does
not provide directly -- the honest first step is a small tone generator feeding
`SDL_AudioStream` , with `akgl_audio_init` , `akgl_audio_tone(voice, hz, ms)` ,
`akgl_audio_envelope(voice, a, d, s, r)` and `akgl_audio_volume(level)` . This is the largest
of the four and the one most worth designing before writing. Tests can run under the dummy
audio driver and assert state transitions rather than sound.
Synthesise tones on three voices
There was no audio API at all: SDL3_mixer was vendored and AKGL_REGISTRY_MUSIC
was declared, but nothing opened a mixer, and a mixer plays recordings anyway.
BASIC 7.0's SOUND, PLAY, ENVELOPE and VOL describe notes, so this adds a tone
generator -- three voices, five waveforms, an ADSR envelope each, mixed to one
float stream and fed to an SDL_AudioStream.
The voice table works whether or not a device is open. akgl_audio_init connects
it to one; a host that owns its own audio pipeline can call akgl_audio_mix
instead. That is also what makes the tests deterministic: a device pulls samples
on SDL's audio thread whenever it likes, so tests/audio.c mixes by hand and only
opens a device in its last test.
Phase is computed from the frame counter rather than accumulated, because a
float increment of hz/44100 added 44100 times a second walks a held note off
pitch. A voice that has never been configured defaults to a square wave at full
level, since a zeroed voice would have a sustain of 0.0 and make no sound with
no error to explain why. Voices summing past full scale are clamped rather than
scaled, so one voice plays at the level it was asked for.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-07-31 07:13:18 -04:00
**Resolved. ** `include/akgl/audio.h` and `src/audio.c` add a three-voice tone
generator over `SDL_AudioStream` : `akgl_audio_init` , `_shutdown` , `_tone` ,
`_stop` , `_waveform` , `_envelope` , `_volume` , `_voice_active` and `_mix` .
It is deliberately separate from the SDL3_mixer side of the library, which
plays audio * assets * ; nothing in `audio.c` reads a file. Decisions worth
knowing:
- **The voice table works with no device open.** `akgl_audio_init` connects
it to one; without that a host can still pull samples itself through
`akgl_audio_mix` . That is not only for embedding — it is what makes the
suite deterministic. A device pulls samples on SDL's audio thread whenever
it likes, so a test that opened one and then asserted on voice state would
be racing the callback. `tests/audio.c` mixes by hand and opens a device
only in its last test.
- **Phase is derived from the frame counter, not accumulated.** A float
increment of `hz / 44100` is not exact, and adding it 44100 times a second
walks a held note off pitch.
- **A voice that was never configured is audible.** A zeroed voice has a
sustain of 0.0, which is silence with no error to explain it, so the table
defaults to a square wave held at full level.
- **Three voices summing past full scale are clamped, not scaled**, so one
voice plays at the level it was asked for rather than a third of it.
- Everything that touches the table takes the stream lock when a device is
open, since the callback reads it on another thread.
Still missing for a complete BASIC sound vocabulary: `FILTER` (SDL3 has no
filter primitive; this would need writing), `TEMPO` and the `PLAY` note-string
parser, both of which belong in the interpreter rather than here.
2026-07-31 06:29:17 -04:00
4. **No non-blocking keystroke read. ** `include/akgl/controller.h` is built around SDL event
handlers the host pumps (`akgl_controller_handle_event` and friends), which suits a game
loop and does not suit `GET` and `GETKEY` -- those ask "is there a keystroke waiting, yes or
no" and must not require the interpreter to own the event loop. Goal 3 of `akbasic` forbids
it owning one.
Wants `akerr_ErrorContext AKERR_NOIGNORE *akgl_controller_poll_key(int *keycode, bool *available);`
reading a small ring buffer that `akgl_controller_handle_event` already fills, so the host
keeps pumping events and the interpreter drains characters at its own pace. Tests: push
synthetic `SDL_EVENT_KEY_DOWN` events through the existing handler and drain them, plus the
empty-buffer and overflow cases.
2026-07-31 06:57:50 -04:00
**Resolved. ** `akgl_controller_poll_key(int *keycode, bool *available)` and
`akgl_controller_flush_keys(void)` are in `include/akgl/controller.h` , over a
fixed `AKGL_CONTROLLER_KEY_BUFFER` (32) ring in `src/controller.c` .
`akgl_controller_handle_event` records every `SDL_EVENT_KEY_DOWN` * before * it
scans the control maps, so a key bound to an actor still reaches a polling
caller — the scan returns as soon as a binding claims the event, and doing it
afterwards would have lost exactly the keys a game also acts on. An empty
buffer is success with `available` false, not an error. A full buffer drops
the newest key rather than overwriting the oldest, matching the Commodore
keyboard buffer and keeping what was typed first. `tests/controller.c` covers
drain order, the release-is-not-a-keystroke case, a key shared with a control
map, flush, both NULL arguments, and overflow plus reuse afterwards.
File five defects akbasic found consuming the new APIs
akbasic has now consumed all four of the API gaps this file listed as blocking
it: its text sink, graphics backend, audio backend and input backend are written
and tested, and the BASIC 7.0 graphics, sound and console verbs work against
them. Doing that turned up six things worth fixing here -- five of which still
are.
Four are workarounds akbasic had to write to build at all, and each is commented
at its site over there with the words "filed upstream":
- An embedded libakgl requires SDL, SDL_image, SDL_mixer, SDL_ttf and jansson to
be installed, while vendoring all five. The dependency block is gated on being
top-level, so an add_subdirectory() consumer takes the find_package path and
fails with the submodules sitting right there in deps/.
- include/akgl/controller.h does not compile on its own: it declares handlers
taking an akgl_Actor * and includes nothing that declares the type. src/
never notices because it includes game.h first.
- akgl_render_init2d() fuses two separable jobs -- creating a window, and
installing the backend vtable -- so a caller who already owns an SDL_Renderer
has no way to get the second without the first. That caller is exactly the
embedding host this section exists to serve.
- akgl_text_rendertextat() dereferences renderer->draw_texture with no check,
which is the state item 7 leaves a host in. Same class of defect the draw
commit added a test for; the text path still has it.
The fifth is a real API gap: SOUND's frequency sweep has no equivalent, and it
cannot be faked in the interpreter without tying audible pitch to the host's
frame rate, so it has to be advanced on the mixer's own frame counter.
The sixth was that 42b60f7 shipped 22 public symbols under an unchanged VERSION
and soname. 1066ac7 fixed that independently while this was being written -- the
two crossed rather than one following the other -- so it is filed as resolved
rather than dropped. The reason is the part worth keeping: akbasic could not
feature-test for the new API and had to pin libakgl by submodule commit in its
README until that landed, which is the concrete cost of an additive release
under an unchanged soname.
All five outstanding items were re-verified against this tree after the rebase,
not just carried forward: controller.h still includes neither actor.h nor a
forward declaration, render_init2d still calls SDL_CreateWindowAndRenderer
before installing the vtable, text.c:62 still reaches through draw_texture
unchecked, the vendored-dependency block is still gated on being top-level, and
there is still no akgl_audio_sweep.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-07-31 08:48:40 -04:00
5. **An embedded `libakgl` demands its dependencies be installed, while vendoring them. **
`CMakeLists.txt:17` gates the whole vendored-dependency block on
`CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR` , so `deps/SDL` , `deps/SDL_image` ,
`deps/SDL_mixer` , `deps/SDL_ttf` and `deps/jansson` are added only when this repository is
the top-level project. Embedded with `add_subdirectory()` , the `else()` branch at `:51`
runs `find_package(SDL3 REQUIRED)` and friends instead, and configure fails on a machine
that has none of them installed — with the submodules sitting right there in `deps/` ,
already checked out by the recursive clone the consumer just did.
`akbasic` works around it by adding those five subdirectories itself, immediately before
`add_subdirectory(deps/libakgl)` . That works only because every lookup in the `else()`
branch is guarded with `if(NOT TARGET ...)` — which is the same escape hatch
`akerror::akerror` and `akstdlib::akstdlib` already rely on, and it should not be the
documented answer. **Fix: ** add the vendored dependencies on both paths, still guarded by
`if(NOT TARGET ...)` so a consumer that has already declared them wins. The suppression of
their CTest registration should move with them.
2026-07-31 12:54:04 -04:00
**Resolved. ** The vendored block no longer sits behind
`CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR` . An
`akgl_add_vendored_dependency(<target> <dir>)` macro adds each of the seven submodules when
nothing has already declared that target * and * the submodule is actually checked out; the
`find_package` lookups for anything left over run afterwards, unchanged, so a checkout
without submodules still resolves against the system. A macro rather than a function
because `add_subdirectory()` inside a function runs in that function's variable scope.
The CTest suppression moved with them, and is lifted again before this project registers
its own suites, so an embedding consumer's `add_test()` still reaches CTest. Two smaller
consequences of the move: `find_package(PkgConfig)` now runs only when something has to
come from the system (a fully vendored build needs no pkg-config of its own, though
`deps/SDL_mixer` asks for it separately), and the build-tree RPATH block keys on whether
anything was vendored rather than on being the top-level project, so an embedded build's
tests can also find the satellite libraries.
Verified by configuring and building a scratch consumer that embeds this repository with
`CMAKE_DISABLE_FIND_PACKAGE_SDL3` , `_SDL3_image` , `_SDL3_mixer` , `_SDL3_ttf` , `_akerror` ,
`_akstdlib` and `_jansson` all set, so any reliance on an installed copy would have failed
the configure. It configures, builds and links. **The five `add_subdirectory` lines in
`akbasic` 's `CMakeLists.txt` can be deleted.**
File five defects akbasic found consuming the new APIs
akbasic has now consumed all four of the API gaps this file listed as blocking
it: its text sink, graphics backend, audio backend and input backend are written
and tested, and the BASIC 7.0 graphics, sound and console verbs work against
them. Doing that turned up six things worth fixing here -- five of which still
are.
Four are workarounds akbasic had to write to build at all, and each is commented
at its site over there with the words "filed upstream":
- An embedded libakgl requires SDL, SDL_image, SDL_mixer, SDL_ttf and jansson to
be installed, while vendoring all five. The dependency block is gated on being
top-level, so an add_subdirectory() consumer takes the find_package path and
fails with the submodules sitting right there in deps/.
- include/akgl/controller.h does not compile on its own: it declares handlers
taking an akgl_Actor * and includes nothing that declares the type. src/
never notices because it includes game.h first.
- akgl_render_init2d() fuses two separable jobs -- creating a window, and
installing the backend vtable -- so a caller who already owns an SDL_Renderer
has no way to get the second without the first. That caller is exactly the
embedding host this section exists to serve.
- akgl_text_rendertextat() dereferences renderer->draw_texture with no check,
which is the state item 7 leaves a host in. Same class of defect the draw
commit added a test for; the text path still has it.
The fifth is a real API gap: SOUND's frequency sweep has no equivalent, and it
cannot be faked in the interpreter without tying audible pitch to the host's
frame rate, so it has to be advanced on the mixer's own frame counter.
The sixth was that 42b60f7 shipped 22 public symbols under an unchanged VERSION
and soname. 1066ac7 fixed that independently while this was being written -- the
two crossed rather than one following the other -- so it is filed as resolved
rather than dropped. The reason is the part worth keeping: akbasic could not
feature-test for the new API and had to pin libakgl by submodule commit in its
README until that landed, which is the concrete cost of an additive release
under an unchanged soname.
All five outstanding items were re-verified against this tree after the rebase,
not just carried forward: controller.h still includes neither actor.h nor a
forward declaration, render_init2d still calls SDL_CreateWindowAndRenderer
before installing the vtable, text.c:62 still reaches through draw_texture
unchecked, the vendored-dependency block is still gated on being top-level, and
there is still no akgl_audio_sweep.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-07-31 08:48:40 -04:00
6. * * `include/akgl/controller.h` does not compile on its own.** Lines 35, 36 and 41 declare
handler function pointers taking an `akgl_Actor *` , and the header includes only
`SDL3/SDL.h` , `akerror.h` and `types.h` — none of which declares that type. Any translation
unit that includes `akgl/controller.h` before `akgl/actor.h` fails with *"unknown type name
'akgl_Actor'"*. `src/controller.c` never notices because it includes `akgl/game.h` first.
This is the house rule in `AGENTS.md` — keep headers self-contained, include what you use.
**Fix: ** `#include "actor.h"` in `controller.h` , or forward-declare
`struct akgl_Actor` if the include order makes that circular. A one-line test that includes
only `akgl/controller.h` would have caught it and would keep catching it.
2026-07-31 12:54:04 -04:00
**Resolved. ** `controller.h` includes `<akgl/actor.h>` . Not a forward declaration:
`akgl_Actor` is a typedef of a named struct, and repeating a typedef is C11, not C99. The
include closes no cycle — `actor.h` reaches only `types.h` and `character.h` , neither of
which knows about the controller.
`tests/headers.c` is the test, and it is a whole suite for one `#include` on purpose: a
second `#include` after the first proves nothing about the second, because by then the
first has dragged its dependencies in. Covering another header means another file shaped
like that one. It also stopped being true that `tests/controller.c` has to include
`akgl/actor.h` first, and the comment there saying so is gone.
File five defects akbasic found consuming the new APIs
akbasic has now consumed all four of the API gaps this file listed as blocking
it: its text sink, graphics backend, audio backend and input backend are written
and tested, and the BASIC 7.0 graphics, sound and console verbs work against
them. Doing that turned up six things worth fixing here -- five of which still
are.
Four are workarounds akbasic had to write to build at all, and each is commented
at its site over there with the words "filed upstream":
- An embedded libakgl requires SDL, SDL_image, SDL_mixer, SDL_ttf and jansson to
be installed, while vendoring all five. The dependency block is gated on being
top-level, so an add_subdirectory() consumer takes the find_package path and
fails with the submodules sitting right there in deps/.
- include/akgl/controller.h does not compile on its own: it declares handlers
taking an akgl_Actor * and includes nothing that declares the type. src/
never notices because it includes game.h first.
- akgl_render_init2d() fuses two separable jobs -- creating a window, and
installing the backend vtable -- so a caller who already owns an SDL_Renderer
has no way to get the second without the first. That caller is exactly the
embedding host this section exists to serve.
- akgl_text_rendertextat() dereferences renderer->draw_texture with no check,
which is the state item 7 leaves a host in. Same class of defect the draw
commit added a test for; the text path still has it.
The fifth is a real API gap: SOUND's frequency sweep has no equivalent, and it
cannot be faked in the interpreter without tying audible pitch to the host's
frame rate, so it has to be advanced on the mixer's own frame counter.
The sixth was that 42b60f7 shipped 22 public symbols under an unchanged VERSION
and soname. 1066ac7 fixed that independently while this was being written -- the
two crossed rather than one following the other -- so it is filed as resolved
rather than dropped. The reason is the part worth keeping: akbasic could not
feature-test for the new API and had to pin libakgl by submodule commit in its
README until that landed, which is the concrete cost of an additive release
under an unchanged soname.
All five outstanding items were re-verified against this tree after the rebase,
not just carried forward: controller.h still includes neither actor.h nor a
forward declaration, render_init2d still calls SDL_CreateWindowAndRenderer
before installing the vtable, text.c:62 still reaches through draw_texture
unchecked, the vendored-dependency block is still gated on being top-level, and
there is still no akgl_audio_sweep.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-07-31 08:48:40 -04:00
7. **There is no way to attach a 2D backend to a renderer the caller already has. **
`akgl_render_init2d()` (`src/renderer.c:17` ) does two separable things: it creates a window
and an `SDL_Renderer` from the `game.screenwidth` /`game.screenheight` properties and writes
to the `camera` global, and it installs the six function pointers that make an
`akgl_RenderBackend` usable. A caller who already owns an `SDL_Renderer` wants only the
second half — and that caller is not hypothetical, it is precisely the embedding host the
API-gap section above exists to serve, since an embedded interpreter must not create the
window.
Today the only options are to call `akgl_game_init()` and let libakgl own the window, or to
assign the six pointers by hand, which is what `akbasic` 's `tests/akgl_backends.c` does.
**Fix: ** split out `akgl_render_bind2d(akgl_RenderBackend *self)` that installs the vtable
and nothing else, and have `akgl_render_init2d()` call it after it has made its window.
`tests/renderer.c` could then cover the vtable half without a display at all.
2026-07-31 12:54:04 -04:00
**Resolved. ** `akgl_render_bind2d(akgl_RenderBackend *self)` installs the six pointers and
returns; `akgl_render_init2d()` calls it once the window and the camera exist. It
deliberately does * not * touch `self->sdl_renderer` , which is the whole point — a host that
already owns one keeps it, and a host that does not gets a backend whose entry points all
report `AKERR_NULLPOINTER` instead of crashing.
`tests/renderer.c` is new and needs no display: it covers the binding (including that the
caller's `SDL_Renderer` survives it, and that binding a backend without one is legal),
frame start and end and both `draw_texture` paths against a software renderer under the
dummy video driver, the refusals a bound-but-unrendered backend gives, and the `draw_mesh`
"not implemented" stub. `src/renderer.c` goes from 10% line coverage to 56%; what is left
is `akgl_render_init2d` itself and `akgl_render_2d_draw_world` , both of which want the
offscreen harness and the world globals. * * `akbasic` 's hand-assigned vtable in
`tests/akgl_backends.c` can be deleted.**
File five defects akbasic found consuming the new APIs
akbasic has now consumed all four of the API gaps this file listed as blocking
it: its text sink, graphics backend, audio backend and input backend are written
and tested, and the BASIC 7.0 graphics, sound and console verbs work against
them. Doing that turned up six things worth fixing here -- five of which still
are.
Four are workarounds akbasic had to write to build at all, and each is commented
at its site over there with the words "filed upstream":
- An embedded libakgl requires SDL, SDL_image, SDL_mixer, SDL_ttf and jansson to
be installed, while vendoring all five. The dependency block is gated on being
top-level, so an add_subdirectory() consumer takes the find_package path and
fails with the submodules sitting right there in deps/.
- include/akgl/controller.h does not compile on its own: it declares handlers
taking an akgl_Actor * and includes nothing that declares the type. src/
never notices because it includes game.h first.
- akgl_render_init2d() fuses two separable jobs -- creating a window, and
installing the backend vtable -- so a caller who already owns an SDL_Renderer
has no way to get the second without the first. That caller is exactly the
embedding host this section exists to serve.
- akgl_text_rendertextat() dereferences renderer->draw_texture with no check,
which is the state item 7 leaves a host in. Same class of defect the draw
commit added a test for; the text path still has it.
The fifth is a real API gap: SOUND's frequency sweep has no equivalent, and it
cannot be faked in the interpreter without tying audible pitch to the host's
frame rate, so it has to be advanced on the mixer's own frame counter.
The sixth was that 42b60f7 shipped 22 public symbols under an unchanged VERSION
and soname. 1066ac7 fixed that independently while this was being written -- the
two crossed rather than one following the other -- so it is filed as resolved
rather than dropped. The reason is the part worth keeping: akbasic could not
feature-test for the new API and had to pin libakgl by submodule commit in its
README until that landed, which is the concrete cost of an additive release
under an unchanged soname.
All five outstanding items were re-verified against this tree after the rebase,
not just carried forward: controller.h still includes neither actor.h nor a
forward declaration, render_init2d still calls SDL_CreateWindowAndRenderer
before installing the vtable, text.c:62 still reaches through draw_texture
unchecked, the vendored-dependency block is still gated on being top-level, and
there is still no akgl_audio_sweep.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-07-31 08:48:40 -04:00
8. * * `akgl_text_rendertextat()` dereferences an uninitialised backend vtable.**
`src/text.c` calls `renderer->draw_texture(renderer, ...)` with no NULL check on either
`renderer` or the function pointer, so a backend that has an `SDL_Renderer` but has not been
through `akgl_render_init2d()` segfaults on the first line of text. That is exactly the
state item 7 leaves a host in, and it is the same class of defect the draw commit at
42b60f7 added a test for — *"a backend that exists but was never given an SDL_Renderer,
which is the state a host is in between allocating one and initializing it; every draw
entry point has to report it rather than dereference it."* The text path still has it.
**Fix: ** `FAIL_ZERO_RETURN` on `renderer` , on `renderer->sdl_renderer` and on
`renderer->draw_texture` , reporting `AKGL_ERR_SDL` or `AKERR_NULLPOINTER` as the draw
entry points do. Test alongside the existing `tests/draw.c` backend-without-a-renderer case.
2026-07-31 12:54:04 -04:00
**Resolved. ** All three are checked, with `AKERR_NULLPOINTER` to match the draw entry
points, and they are checked * before * anything is rasterized rather than after: refusing
early costs nothing and leaks nothing, where refusing after the rasterize would hit the
leak the `@note` on this function already describes.
`tests/text.c` grew a software renderer under the dummy video driver — bound with the new
`akgl_render_bind2d` , which is what made this cheap — and covers all three refusals plus
the successful draw, wrapped and unwrapped. `src/text.c` goes from 58% to **100% ** line
coverage, and the three `akgl_text_rendertextat` mutants the mutation run left surviving
are dead. The third case is the one that matters: with a * live * `SDL_Renderer` behind a
backend that was never bound, the old code got all the way to a NULL function pointer.
Checking it against a made-up renderer pointer, as the first draft of the test did, is
worth nothing — SDL refuses the bogus handle and the call fails for the wrong reason, which
a deleted-check mutant survives.
Found while writing that test and filed below as item 27: SDL_ttf refuses the empty string
on both rasterizing paths, so drawing an empty line is an error while measuring one is not.
File five defects akbasic found consuming the new APIs
akbasic has now consumed all four of the API gaps this file listed as blocking
it: its text sink, graphics backend, audio backend and input backend are written
and tested, and the BASIC 7.0 graphics, sound and console verbs work against
them. Doing that turned up six things worth fixing here -- five of which still
are.
Four are workarounds akbasic had to write to build at all, and each is commented
at its site over there with the words "filed upstream":
- An embedded libakgl requires SDL, SDL_image, SDL_mixer, SDL_ttf and jansson to
be installed, while vendoring all five. The dependency block is gated on being
top-level, so an add_subdirectory() consumer takes the find_package path and
fails with the submodules sitting right there in deps/.
- include/akgl/controller.h does not compile on its own: it declares handlers
taking an akgl_Actor * and includes nothing that declares the type. src/
never notices because it includes game.h first.
- akgl_render_init2d() fuses two separable jobs -- creating a window, and
installing the backend vtable -- so a caller who already owns an SDL_Renderer
has no way to get the second without the first. That caller is exactly the
embedding host this section exists to serve.
- akgl_text_rendertextat() dereferences renderer->draw_texture with no check,
which is the state item 7 leaves a host in. Same class of defect the draw
commit added a test for; the text path still has it.
The fifth is a real API gap: SOUND's frequency sweep has no equivalent, and it
cannot be faked in the interpreter without tying audible pitch to the host's
frame rate, so it has to be advanced on the mixer's own frame counter.
The sixth was that 42b60f7 shipped 22 public symbols under an unchanged VERSION
and soname. 1066ac7 fixed that independently while this was being written -- the
two crossed rather than one following the other -- so it is filed as resolved
rather than dropped. The reason is the part worth keeping: akbasic could not
feature-test for the new API and had to pin libakgl by submodule commit in its
README until that landed, which is the concrete cost of an additive release
under an unchanged soname.
All five outstanding items were re-verified against this tree after the rebase,
not just carried forward: controller.h still includes neither actor.h nor a
forward declaration, render_init2d still calls SDL_CreateWindowAndRenderer
before installing the vtable, text.c:62 still reaches through draw_texture
unchecked, the vendored-dependency block is still gated on being top-level, and
there is still no akgl_audio_sweep.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2026-07-31 08:48:40 -04:00
9. * * `SOUND` 's frequency sweep has no `akgl_audio_*` equivalent.** BASIC 7.0's
`SOUND voice, freq, dur, dir, min, step` ramps the pitch from `freq` toward `min` in `step`
increments per tick, in the direction `dir` selects — a siren, a laser, the whole reason
`SOUND` has six arguments. `akgl_audio_tone(voice, hz, ms)` holds one pitch for one
duration, and nothing changes pitch over the life of a note.
`akbasic` refuses those three arguments rather than faking them, and the reasoning is worth
recording because it is what makes this a `libakgl` gap rather than an interpreter one: the
only way to fake a sweep from the interpreter is to re-issue tones from its step loop, which
ties audible pitch to how often the host happens to call it — a tune that changes key with
the frame rate. It has to be advanced on the mixer's own frame counter, which is where the
phase is already derived from and which only this library can see.
**Fix: ** `akgl_audio_sweep(int voice, float32_t from_hz, float32_t to_hz, float32_t step_hz, uint32_t ms)` ,
advanced in `akgl_audio_mix()` beside the envelope. Tests would drive `akgl_audio_mix` by
hand and assert the frame at which the pitch has moved, exactly as `tests/audio.c` already
does for the envelope stages.
2026-07-31 12:54:04 -04:00
**Resolved. ** `akgl_audio_sweep(voice, from_hz, to_hz, step_hz, ms)` is in
`include/akgl/audio.h` , with the step taken in `akgl_audio_mix()` off the mixer's own frame
counter. Decisions worth knowing:
- **One step every 1/60 second** (`AKGL_AUDIO_SWEEP_TICK_HZ` ), because that is the rate the
machine this vocabulary comes from advanced its sweep at, and its tunes are written for
it. It divides 44100 exactly, so a step boundary always lands on a whole frame — which is
what lets the suite assert the exact frame the pitch moves on rather than a tolerance.
- **Direction comes from the two frequencies, not from the sign of the step.** `step_hz`
stays positive; `to_hz` below `from_hz` sweeps down. A negative step is refused rather
than silently meaning something.
- **It arrives and holds.** The last step is clamped to `to_hz` rather than overshooting,
and equal frequencies are a legal held tone rather than an error, so a caller computing
its own limits does not have to special-case them.
- **`akgl_audio_tone` is now the same start path with a step of 0**, which is also what
makes a voice reused for a plain tone stop sweeping. That is one function, `start_note` ,
rather than two copies of the same six assignments.
- **A swept voice accumulates its phase** instead of deriving it from the frame counter.
Deriving assumes a constant frequency; under a sweep it jumps the waveform at every step,
which is an audible click. The drift the derived form exists to avoid is not something a
note that is changing pitch anyway can be said to suffer from.
`tests/audio.c` drives the mixer by hand: one tick's frames do not move the pitch, the next
frame does, both directions clamp at their target, a gate shorter than the sweep cuts it off
part way, and a plain tone on a swept voice stays put. `src/audio.c` holds at 92%.
Still missing for a complete `SOUND` : the oscillating third direction (C128 `dir` 2), which
is a re-issue on arrival rather than a third kind of sweep, and `FILTER` , `TEMPO` and the
`PLAY` note-string parser as before.
2026-07-31 11:19:56 -04:00
10. **The keystroke ring carries a keycode and nothing else, so Shift is invisible. **
`akgl_controller_handle_event()` (`src/controller.c:104-105` ) pushes `event->key.key` into
the ring and drops the rest of the event, and `akgl_controller_poll_key(int *keycode, bool
*available)` hands back only that. Item 4 asked for a non-blocking keystroke read and got
one; what it did not ask for, and what a * line editor * turns out to need, is which
character the keystroke actually produced.
`akbasic` has now built one — an `INPUT` and a REPL prompt drawn in the window, in
`src/sink_akgl.c` — and the consequence is concrete: no shifted character is reachable, so
`"` , `!` , `(` , `)` , `:` and `;` cannot be typed at all, and lower case cannot be typed
either. A BASIC line editor that cannot type a double quote cannot enter a string literal.
The interpreter folds letters to upper case, which is what a C128 does anyway and is the
right resolution for letters; it is not a resolution for punctuation, and there is nothing
the caller can do about it because the modifier state was discarded two layers down.
Note this is not solvable by the caller polling `SDL_GetModState()` at read time: by then
the key is long released, and the whole point of a ring is that reads are decoupled from
events.
**Fix: ** widen the ring entry to carry the modifier state and the composed text SDL already
computes. Either
```c
typedef struct akgl_Keystroke {
SDL_Keycode key;
SDL_Keymod mod;
char text[8]; /* UTF-8, from SDL_EVENT_TEXT_INPUT; "" for a non-printing key */
} akgl_Keystroke;
akerr_ErrorContext AKERR_NOIGNORE *akgl_controller_poll_keystroke(akgl_Keystroke *dest, bool *available);
```
alongside the existing `akgl_controller_poll_key()` , which stays as it is so nothing
breaks — a game asking "was the up arrow pressed" wants exactly what it already gets.
Filling `text` means handling `SDL_EVENT_TEXT_INPUT` as well as `SDL_EVENT_KEY_DOWN` , which
is the only correct way to get a character out of SDL anyway: it is what makes a keyboard
layout, a compose key and a dead key work, none of which a keycode can express.
Tests would push a shifted key-down plus its text-input event through
`akgl_controller_handle_event()` and assert both the keycode and the composed character
come back, mirroring what `tests/controller.c` already does for the plain ring.
2026-07-31 12:54:04 -04:00
**Resolved. ** `akgl_Keystroke` (keycode, `SDL_Keymod` , and eight bytes of UTF-8) and
`akgl_controller_poll_keystroke(akgl_Keystroke *dest, bool *available)` are in
`include/akgl/controller.h` ; the ring now holds those instead of bare keycodes.
`akgl_controller_poll_key()` is untouched from its caller's side. Decisions worth knowing:
- **The text is attached to the press that is still waiting for it**, tracked by a flag
rather than by "whatever entry is newest". Without the flag, a press dropped by a full
buffer hands its text to some older key that happens to be sitting at the end of the
ring — which is wrong in exactly the case where things are already going badly.
- **Text with no press behind it is buffered on its own**, with a keycode of 0. An input
method commit and a character finished by a dead key have no key press of their own, and
a line editor wants the character regardless. `akgl_controller_poll_key()` discards those
on the way past rather than reporting a keystroke with no key.
- **Oversized text is cut on a code point boundary.** An IME can commit several characters
at once and an entry holds one; truncating on a byte boundary would leave a partial UTF-8
sequence that nothing downstream can render.
- **SDL only sends `SDL_EVENT_TEXT_INPUT` while text input is started**, so a host that
wants `text` populated calls `SDL_StartTextInput()` on its window. Without it `key` and
`mod` still arrive. That is documented on the function rather than left to be discovered.
`tests/controller.c` covers the shifted-key case end to end (the `"` that started this),
a non-printing key composing to nothing, a text-only entry through both pollers, text that
outlived its own press, the code-point-boundary truncation, and both NULL arguments.
`src/controller.c` holds at 91%.
2026-07-30 01:40:06 -04:00
## Carried over
2026-07-29 18:01:05 -04:00
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.
2026-07-30 01:40:06 -04:00
2026-07-30 02:08:38 -04:00
`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.
