Files
libakgl/TODO.md
Tachikoma ff4af80693 Move outstanding work from TODO.md into the issue tracker
TODO.md carried two records in one file: what had been done, with the
measurements behind it, and what was left. The second half is what a tracker
is for, and keeping it here has already cost something -- AGENTS.md records a
round where eleven entries described code that had already changed, and this
file admitted to three more.

Every open item is now an issue on source.starfort.tech/andrew/libakgl,
labelled by kind and blast radius and milestoned by what it can land in: 0.9.x
for anything that breaks no ABI, 0.10.0 for new or changed public symbols,
1.0.0 for the design work. Four are epics: the performance plan (#60),
coverage (#61), actor rotation (#62), and the false header comments (#63).

Verified against the tree before filing rather than transcribed. Three entries
were already fixed and were not filed: the akgl_path_relative context leak, the
akgl_draw_background test extension, and the SDL enumeration audit -- keyboards,
gamepads and mappings are all freed in CLEANUP today. Two were reworded because
the code had moved: the fonts item is a missing teardown entry point rather than
a missing API, since akgl_text_unloadallfonts exists, and draw_world's tilemap
call is already bounded by numlayers, so only the per-layer actor rescan remains.

TODO.md keeps the part a tracker has no place for: why a decision went the way
it did, what the measurement was, and which arguments turned out to be wrong.

TODO.txt is deleted. Four of its eight entries had shipped -- actor-to-actor
collision, actor-to-world collision, automatic facing, image layers -- and the
four that had not are #74 through #77, with the GPU renderer's research links
kept because that is the part that took the time.

Every reference that named an item number or a moved section is repointed, in
the manual, the headers, the tests and the examples.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Andrew Kesterson <andrew@aklabs.net>
2026-08-02 18:47:34 -04:00

934 lines
56 KiB
Markdown

# Record
**Outstanding work is in the issue tracker, not in this file:**
<https://source.starfort.tech/andrew/libakgl/issues>
This file used to carry both halves of the record -- what had been done and what
was left -- and the second half is what a tracker is for. Every open item moved
there. What stays here is the part a tracker has no place for: **why a decision
went the way it did, what the measurement was, and which arguments turned out to
be wrong.**
The rule that produced this split is the one this file kept breaking: an entry
describing work still to do goes stale the moment somebody does it, and stale
entries are where the next defect hides. `AGENTS.md` records the last round of
that -- eleven entries here described code that had already changed -- and this
file admitted to three more when it was migrated. **A record of finished work
cannot go stale in that way.**
Issues are labelled by kind (`defect`, `performance`, `test-coverage`, `api-gap`,
`packaging`, `docs`, `design-decision`, `abi-break`, `hygiene`) and by blast
radius, and milestoned by what they can land in: `0.9.x` for anything that breaks
no ABI, `0.10.0` for new or changed public symbols, `1.0.0` for the design work,
`Unscheduled` for constraints waiting on a trigger.
Four issues are epics with the rest hanging off them: the performance plan
(#60), coverage (#61), actor rotation (#62), and the false header comments (#63).
---
## Test suites that could not fail
Found while renaming the exported globals. Both are fixed, and this is the entry
that argues hardest for not trusting a green suite.
**Every suite reported success on any status whose low byte is zero.**
libakerror's default unhandled-error handler ends in `exit(errctx->status)`.
`exit` keeps only the low byte of what it is given, and libakgl's status band
starts at `AKERR_FIRST_CONSUMER_STATUS`, which is 256. `AKGL_ERR_SDL` is
therefore exactly 256, `exit(256)` is a wait status of 0, and CTest recorded a
pass. **The most common failure status in a library built on SDL was the one
status that could not fail a test.**
**`tests/character.c` was green while running one of its four tests.** It aborted
in `test_character_sprite_mgmt` with
`Failed loading asset .../spritesheet.png : Parameter 'renderer' is invalid` --
the symbol collision described under "Public naming" below -- and exited 0
because that status was `AKGL_ERR_SDL`. Two independent defects had to line up,
and they did, for long enough that this file recorded the suite as passing and
wondered which change had fixed it. Nothing had; it had stopped running.
**Fixed twice.** 0.5.0 worked around it here, with a `TEST_TRAP_UNHANDLED_ERRORS()`
in `tests/testutil.h` that installed a handler collapsing any status a byte cannot
carry onto 1. Once installed, no other suite changed colour, so this was not
masking anything beyond `character` -- but it could have been at any time, and
nothing would have said so.
That entry ended "any consumer's test suites have this problem; that is worth
raising upstream", and it was. **libakerror 2.0.1 fixes it at the source**:
`akerr_exit()` owns the status-to-exit-code mapping and the default handler calls
it, so 0 exits 0, 1 through 255 exit themselves, and anything else exits
`AKERR_EXIT_STATUS_UNREPRESENTABLE` (125) rather than a low byte that is either a
lie or a claim of success. There is no wider `exit()` to reach for -- `_exit()`,
`_Exit()`, `quick_exit()` and the raw `exit_group` syscall all truncate the same
way, and even `waitid()` reports the truncated value.
The workaround is gone as of 0.6.0, along with its 21 call sites. Verified by
putting the original failure back: a `FAIL_BREAK(AKGL_ERR_SDL)` in
`tests/character.c`'s `main` now exits 125 and CTest records a failure, where it
exited 0 and passed before.
## Internal consistency
A sweep of `src/` and `include/` found 41 consistency and convention problems.
All are resolved except the two now filed as #4 and #5. What follows is the part
worth keeping: what changed, and which of them were not cosmetic after all.
### Public naming, resolved in 0.5.0
An ABI break, carrying the soname to `libakgl.so.0.5`. What a consumer had to
rename:
| Was | Is |
|---|---|
| `_ASSETS_H_`, `_CONTROLLER_H_`, `_DRAW_H_`, `_ERROR_H_`, `_JSON_HELPERS_H_`, `_PHYSICS_H_`, `_REGISTRY_H_`, `_RENDERER_H_`, `_TEXT_H_`, `_TILEMAP_H_`, `_UTIL_H_`, `_STRING_H_` | `_AKGL_<FILE>_H_` |
| `akgl_Actor_cmhf_*` (8 functions) | `akgl_actor_cmhf_*` |
| `akgl_game_updateFPS` | `akgl_game_update_fps` |
| `akgl_render_init2d`, `akgl_render_bind2d` | `akgl_render_2d_init`, `akgl_render_2d_bind` |
| `akgl_sprite_sheet_coords_for_frame` | `akgl_spritesheet_coords_for_frame` |
| `point`, `RectanglePoints` | `akgl_Point`, `akgl_RectanglePoints` |
| `window`, `bgm`, `game`, `gamemap`, `renderer`, `physics`, `camera` | the same, `akgl_`-prefixed |
| `_akgl_renderer`, `_akgl_physics`, `_akgl_camera`, `_akgl_gamemap` | `akgl_default_renderer`, `akgl_default_physics`, `akgl_default_camera`, `akgl_default_gamemap` |
| `HEAP_ACTOR`, `HEAP_SPRITE`, `HEAP_SPRITESHEET`, `HEAP_CHARACTER`, `HEAP_STRING` | `akgl_heap_actors`, `akgl_heap_sprites`, `akgl_heap_spritesheets`, `akgl_heap_characters`, `akgl_heap_strings` |
| `GAME_ControlMaps` | `akgl_controlmaps` |
| `AKGL_SPRITE_MAX_CHARACTER_NAME_LENGTH` | `AKGL_CHARACTER_MAX_NAME_LENGTH` |
| `AKGL_TIME_ONESEC_MS` | `AKGL_TIME_ONEMS_NS` |
`include/akgl/staticstring.h` also stopped guarding with `_STRING_H_` -- a name
several libc implementations use for their own `<string.h>` -- and its
`#include "string.h"` is now `#include <string.h>`.
**The renames were done by renaming each declaration and letting the compiler find
every use, rather than by pattern substitution.** That distinction matters for
`renderer`, `physics` and `camera`, which are also parameter and struct-member
names: a `sed` would have rewritten `map->physics` and every
`akgl_RenderBackend *renderer` parameter, and nothing would have complained.
**One rename had a live bug behind it.** `akgl_game_state_lock` counted its retry
loop against a constant named "one second in milliseconds" that held `1000000`, so
it retried 10,000 times at 100 ms and blocked for roughly sixteen minutes before
reporting failure. The budget is now `AKGL_GAME_STATE_LOCK_BUDGET_MS` (1000), with
the cadence named separately as `AKGL_GAME_STATE_LOCK_RETRY_MS`. `tests/game.c`
carries `test_game_state_lock_budget`, which holds the mutex from a second thread
and asserts the call gives up between half a second and five. The old code fails it
by timing the suite out; a build that gave up without waiting at all fails the
lower bound. **Nothing exercised the contended path before** -- the existing lock
test only ever took an uncontended mutex, which never reaches the retry loop.
**Renaming the exported globals was not cosmetic, and the proof was sitting in the
test suite.** `renderer` was exported from the shared library, and
`tests/character.c` defined an `SDL_Renderer *renderer` of its own. Both had
external linkage and the same spelling, so the executable's definition preempted
the library's: `akgl_sprite_load_json` read an `SDL_Renderer *` through an
`akgl_RenderBackend *`, every texture load in that suite failed with
`Parameter 'renderer' is invalid`, and the suite still reported success. **A
consuming game with a variable called `renderer` would have hit the same thing
with no test to notice.**
### Header/implementation surface drift, resolved in 0.5.0
**Nineteen non-static functions were defined in `src/` but declared in no header.**
Each is now declared or `static`. The controller handlers were the interesting
case: they were defined as `gamepad_handle_*` while `controller.h` declared the
`akgl_controller_handle_*` names it never defined -- four symbols *mentioned* in
prose, which is not the same as being declared.
**This is enforced now.** `scripts/check_api_surface.sh` reads the built library's
dynamic symbol table, strips comments out of every public header, and fails when an
exported `akgl_*` symbol is declared nowhere. It runs as the `api_surface` CTest
test. **Stripping comments is the point**, and is exactly how those four went
unnoticed.
**Headers relied on their includers for types.** `iterator.h` used `uint32_t`
without `<stdint.h>`; `json_helpers.h` used `json_t` without `<jansson.h>`;
`util.h` used `SDL_FRect` and `bool` without any SDL include. Each compiled only
because of `.c`-file include ordering.
Resolved, and enforced rather than merely fixed: `AKGL_PUBLIC_HEADERS` in
`CMakeLists.txt` is the single list behind both `install()` and a generated
translation unit per header -- each including exactly that header and nothing
before it -- linked into the `headers` suite. **A header that ships is a header
that is checked.** Writing that check found a case the hand-written list had
missed: `registry.h` uses `SDL_PropertiesID` in eight declarations and included no
SDL header at all. That is the argument for generating the check off the install
list rather than hand-listing the headers somebody thought were at risk.
**Object-pool size macros were defined twice and the override hook was dead.**
They live once, in `heap.h`, inside the `#ifndef` guards that were always supposed
to make them overridable. `tests/header_pool_override.c` is the regression test: it
defines its own ceilings, includes `heap.h`, and `#error`s if the guard did not
fire. **The assertion is the compile.**
**Empty parameter lists.** `akgl_game_init`, `akgl_game_update_fps`,
`akgl_heap_init`, `akgl_heap_init_actor` and the eight `akgl_registry_init*`
functions declare and define `(void)`. Before C23 `()` means "unspecified
arguments" and suppresses argument checking, **so these were the entry points a
caller could pass anything to.**
Also resolved: `akgl_game_init_screen` declared and never defined; static helpers
using three naming styles; six parameter-name disagreements between declaration and
definition (`akgl_get_json_with_default` was the interesting one -- it took the
incoming context as `err` and named its *own* context `e`, and renaming it was what
surfaced that the two had been swapped rather than merely misspelled); and
`AKERR_NOIGNORE` applied inconsistently at definition sites.
### Error-handling pattern, resolved in 0.5.0
**`*_RETURN` macros were used inside `ATTEMPT` blocks, which skips `CLEANUP`.** Ten
sites, found by scanning rather than from the list -- which named six and missed the
four in `akgl_collide_rectangles`.
The one that mattered was the success path of `akgl_get_json_tilemap_property`,
which leaked two of the string pool's 256 entries on every lookup that *found* what
it was asked for. A map load does that several times per layer.
**Two needed more than swapping the macro.** In `akgl_get_json_tilemap_property` a
plain `break` would have fallen through to the "property not found" `FAIL_RETURN`
after `FINISH`, reporting a miss for something found, so the success path sets a
flag. In `akgl_collide_rectangles` the eight early exits were followed by
`*collide = false;`, which would have overwritten the hit that broke out of the
block; each corner test writes the flag itself, so that line is gone rather than
moved.
**`scripts/check_error_protocol.py` keeps this closed**, as the `error_protocol`
test. It also enforces the other rule with a silent failure mode -- no `return` out
of a `HANDLE` block.
Also resolved: NULL-check discipline varying by function (the eight typed JSON
accessors that validated their container and then wrote through `dest`
unconditionally; the null physics backend; `akgl_render_2d_frame_start`, `_frame_end`
and `_shutdown`, which `tests/renderer.c` now calls with `NULL` and which segfaulted
before), and the `errctx`/`e` split -- all 45 remaining sites are `errctx`, in its
own commit because it is a rename and nothing else, with `e` keeping its meaning as
an incoming context being inspected.
### Types and macros, resolved in 0.5.0
**`AKGL_COLLIDE_RECTANGLES` had unbalanced parentheses.** Three opens against two
closes meant any expansion was a syntax error. It had no callers and duplicated
`akgl_collide_rectangles`, so it was deleted.
**Bitmask macros were unparenthesized.** All five are fully parenthesized, and
`AKGL_BITMASK_CLEAR` no longer carries a semicolon inside its body.
`tests/bitmasks.c` covers the composition cases, and writing them turned up
something worth recording: **the obvious test does not catch this.** For a bit that
is *set*, `!AKGL_BITMASK_HAS(mask, bit)` misparses to `!(mask & bit) == bit`, which
is `0 == bit` -- false, the same answer the correct parse gives. It only diverges
for a bit that is *not* set and whose value is not 1: `!(0)` is 1, and `1 == 64` is
false where the answer should be true. The suite uses that shape, and fails against
the old macros.
Also resolved: `float`/`double` spelled raw where `types.h` defines `float32_t` and
`float64_t`; the state and iterator bit macros mixing forms; and `akgl_Frame`,
defined and never used, deleted.
The bit-pattern comments in `actor.h` were not wrong so much as unlabelled: each
shows the pattern *within its own 16-bit half*, which is why bit 16 looks like it
restarts at bit 0. The section headings say so now. Rendering the full 32-bit value
instead would push those lines past the 100-column fill.
### `AKGL_ACTOR_STATE_STRING_NAMES`, resolved in 0.5.0
**The array bound differed between declaration and definition.** The header
declared `[AKGL_ACTOR_MAX_STATES+1]` (33) and the definition was a literal `[32]`,
so a consumer trusting the declared bound read past the object.
**Two entries named the wrong bit.** Indices 11 and 12 said
`AKGL_ACTOR_STATE_UNDEFINED_11` and `_12` where `actor.h` has `MOVING_IN` and
`MOVING_OUT`, so **no character JSON could ever bind a sprite to either state** --
the name it would have to write was not in the registry.
`tests/registry.c` now walks the whole table: every entry non-`NULL`, every entry
resolving to its own bit, no two entries sharing a name (a duplicate silently
overwrites and makes one bit unreachable), and `MOVING_IN`/`MOVING_OUT` named
explicitly so a regression reads as what it is.
The generation comment was stale -- there is no generator, no Makefile and no
`lib_src/`. The file's own header says it is maintained by hand and states the two
invariants that keep breaking.
### Formatting and hygiene, resolved in 0.5.0
**Leftover debug code.** Four `SDL_Log` lines in `src/controller.c` guarded by
`event->type == 768 && event->key.which == 11 && event->key.key == 13` -- decimal
literals for one keyboard on one developer's machine, inside the per-event inner
loop.
**`akgl_game_update`'s default flags OR-ed the same bit twice.** It reads
`AKGL_ITERATOR_OP_UPDATE | AKGL_ITERATOR_OP_LAYERMASK` now. This was a statement of
intent rather than a behaviour change, and the reason is worth knowing: **nothing in
that loop read either bit**, which was the per-layer update sweep defect fixed
later.
**`akgl_draw_background` was the only public function outside the error protocol.**
It takes an `akgl_RenderBackend *` like every other entry point in `draw.h`, returns
an error context, checks the backend and its `sdl_renderer`, and restores the draw
colour it found. It was recorded as needing the offscreen renderer harness. **It did
not, and never did** -- what it needed was to stop reading the global.
**`akgl_registry_init_actor` was the only initializer that destroyed the registry it
was replacing.** All eight go through one `registry_create()` helper now, so the
other seven stop leaking an `SDL_PropertiesID` per call after the first -- which a
game that resets between levels makes on every level. Fixed alongside it, because it
is the same function: `akgl_registry_init()` never called
`akgl_registry_init_properties()`, so `AKGL_REGISTRY_PROPERTIES` stayed 0 for any
caller that did not also go through `akgl_game_init`, making `akgl_set_property` a
silent no-op and `akgl_get_property` always return the caller's default -- so
`akgl_physics_init_arcade` and `akgl_render_2d_init` quietly ignored their
configuration.
Also resolved: five abandoned `SDL_GetBasePath()` blocks; a dozen unused locals;
`struct`-qualified parameters in two physics definitions; `text.c` validating the
wrong argument; and `dst` vs `dest` for output parameters.
## Coverage
**Line coverage 83.9%, function coverage 91.2%** (2433/2901 lines, 176/193
functions), up from 79.6% / 87.2% before the 0.5.0 defect work and from a
39.6% / 44.3% baseline. Snapshot at 0.9.0; the gaps behind it are #61.
Generated with:
```sh
cmake -S . -B build-coverage -DAKGL_COVERAGE=ON -DCMAKE_BUILD_TYPE=Debug
cmake --build build-coverage --parallel
ctest --test-dir build-coverage --output-on-failure
```
Reports land in `build-coverage/coverage/` (`index.html`, `coverage.xml`). A
coverage tree rebuilt after a source edit fails `coverage_reset` with
`GCOV returncode was 5`; `find build-coverage -name '*.gcda' -delete` clears it.
| File | Lines | Functions |
|---|---|---|
| `src/actor.c` | 205/258 (80%) | 16/18 |
| `src/assets.c` | 0/21 (0%) | 0/1 |
| `src/audio.c` | 229/248 (92%) | 22/23 |
| `src/character.c` | 122/126 (97%) | 7/7 |
| `src/controller.c` | 314/349 (90%) | 17/17 |
| `src/draw.c` | 281/286 (98%) | 12/12 |
| `src/error.c` | 9/9 (100%) | 1/1 |
| `src/game.c` | 142/243 (58%) | 13/18 |
| `src/heap.c` | 116/116 (100%) | 12/12 |
| `src/json_helpers.c` | 123/123 (100%) | 11/11 |
| `src/physics.c` | 144/144 (100%) | 10/10 |
| `src/registry.c` | 83/111 (75%) | 12/13 |
| `src/renderer.c` | 61/83 (74%) | 8/8 |
| `src/sprite.c` | 102/110 (93%) | 5/5 |
| `src/staticstring.c` | 17/18 (94%) | 2/2 |
| `src/text.c` | 79/80 (99%) | 7/7 |
| `src/tilemap.c` | 276/444 (62%) | 13/20 |
| `src/util.c` | 128/130 (98%) | 7/7 |
| `src/version.c` | 2/2 (100%) | 1/1 |
`src/tilemap.c` moved the most, 47% to 62%, because the bounds and leak work needed
the loaders driven rather than merely called.
**Branch coverage reads 24.7% and should not be used as a target.** The akerror
control-flow macros expand into large branch trees per call site, most of them
unreachable in normal operation -- `src/game.c` reports over 1700 branches across
230 lines. Track line and function coverage; treat branch coverage as a relative
signal within a file.
### Suites
Every suite is registered through the `AKGL_TEST_SUITES` list in `CMakeLists.txt`,
which drives target creation, CTest registration, the `WORKING_DIRECTORY`/`TIMEOUT`
properties, the link line, and the `FIXTURES_REQUIRED akgl_coverage` list together.
Adding `tests/<name>.c` and the name to that list is all a new suite needs; **it can
no longer be accidentally left out of the coverage fixture.**
Shared assertion helpers are in `tests/testutil.h`: `TEST_ASSERT`,
`TEST_ASSERT_FEQ`, `TEST_EXPECT_STATUS`, `TEST_EXPECT_OK`, `TEST_EXPECT_ANY_ERROR`
and `TEST_ASSERT_FLAG`. All except the last expand to a `break` on failure, so they
belong directly inside an `ATTEMPT` block, not inside a loop nested in one.
### Mutation testing
**Macros expand at call sites, so mutation testing is the only thing that checks
them.** `scripts/mutation_test.py`, run when touching control flow, refcounting,
stack traces or handlers.
Sampled runs (`--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 |
| `src/text.c` | 50% | Three in `akgl_text_rendertextat`, which had no test yet, plus one `SUCCEED_RETURN` deletion |
| `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 |
**What the runs found, and what they did not.** `src/audio.c` scored 50% first and
named two real gaps, both 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. In `src/controller.c`, one real gap
-- nothing checked that a text-only ring entry reports no modifiers.
The rest are **equivalent mutants rather than misses**, and knowing which is which
is the point: a ring buffer whose head starts at 1 behaves identically, `count > 0`
cannot be false where it is checked, 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. Deleting a `SUCCEED_RETURN` leaves a non-void function falling off its
end, which is undefined rather than observably wrong.
The three `src/text.c` survivors are gone: it is tested 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.
## Defects
### Fixed while building the suites
Each was found by a test written to assert correct behaviour.
1. **`akgl_physics_simulate` dereferenced `self` before its NULL check.**
`src/physics.c:132` read `self->gravity_time` at declaration time, three lines
above the `FAIL_ZERO_RETURN`. 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.
### Fixed in 0.5.0 and later
- **`akgl_render_and_compare` compared a texture against itself.** Both passes drew
`t1`, so it always reported a match and **every image assertion built on it
asserted nothing.**
- **`akgl_tilemap_release` double-freed tileset textures.** The layers loop tested
`layers[i].texture` and destroyed `tilesets[i].texture`, so every tileset texture
was freed twice on a single release and no image layer's texture was freed at all.
Each pointer is cleared as it goes now, which also makes a second release safe
rather than a use-after-free.
- **`akgl_path_relative_from` was a stub that leaked.** It claimed a heap string,
never wrote its output, and never released it, so 256 calls exhausted the string
pool. Declared in no header, called from nowhere, duplicating what
`akgl_path_relative` already does. **Fixing an unfinished function nobody can
reach is worse than deleting it.**
- **`akgl_compare_sdl_surfaces` memcmp'd without checking geometry.** `tests/util.c`
compares a 32x32 surface against an 8x8 one in both directions, which used to read
4 KiB past the end of the smaller.
- **`akgl_string_initialize` overflowed by four bytes when `init` is NULL.** The four
bytes it ran past the end of the object landed on the *next* pool slot's
`refcount`, which is the field the allocator reads to decide whether a slot is free
-- **so the overrun could hand a live string out twice.** Fixed alongside, same
class: `akgl_string_copy` accepted a `count` above `AKGL_MAX_STRING_LENGTH`, which
read past the end of one pool slot and wrote past the end of another. The header
documented that as behaviour.
- **Savegame name lengths disagreed between writer and reader.** The reader used
`AKGL_ACTOR_MAX_NAME_LENGTH` for all four tables; only the spritesheet table was
wrong, and that was enough. **The failure was worse than "cannot be read back",
which is what made the test interesting**: the tables carry no length prefix and
end at a zeroed sentinel, so a reader stepping the wrong width does not run off
anything -- it finds a run of zeros somewhere inside an entry, stops early, and
reports success with silently wrong maps. A test that only asserted the load
succeeded passed against the broken reader. So `akgl_game_load` checks the stream
is at EOF once the four tables are read, which is what turns a width disagreement
into `AKERR_IO` instead of a corruption. **That EOF check has to move when the
objects themselves start being written; there is a comment at the site saying so.**
A first pass introduced four `AKGL_GAME_SAVE_*_NAME_WIDTH` aliases, on the
reasoning that the on-disk format's widths are a separate concern from the object
model's. **They were removed**: each expanded to exactly one existing constant and
had exactly one use per side, so they were indirection with no second consumer --
and the divergence they anticipated cannot happen quietly anyway, since raising one
of those lengths is an ABI change and `akgl_game_load` refuses a save whose
`libversion` does not match before it reads a single table.
- **`akgl_controller_pushmap` and `akgl_controller_default` accepted negative map
ids.** Both check the lower bound now.
- **A failed controller-DB fetch silently destroyed the tracked fallback.**
`mkcontrollermappings.sh` runs under `set -euo pipefail`, fetches into a temporary
directory, and moves the result into place only after checking curl's exit status
(with `--fail`, so an HTTP error is a status rather than an error page in the
body), a plausible minimum mapping count, and that no mapping carries a quote or
backslash that would break the C string literal it becomes.
**Verified against all five failure modes**: an unresolvable host, a 404, a
truncated response, an empty-but-successful response -- the original failure
exactly -- and a response containing a quote. Each refuses, and the header's
checksum is unchanged after every one.
The build no longer runs it. The `add_custom_command` declared
`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, so the declared output never appeared, the command was permanently out
of date, and **every build re-ran it** -- needing network access and leaving the
tree dirty.
Regenerating against the real upstream produced byte-identical mappings -- 2255
entries, no content change -- so the rewrite is faithful.
- **A stale build tree in the source directory broke the coverage run.** gcovr
searches for `.gcda`/`.gcno` under its search paths, and with none given it
searches `--root` -- the source directory, which is where developers keep their
build trees. `--object-directory` does not narrow that; per gcovr's own help it
only identifies "the path between gcda files and the directory where the compiler
was originally run". **Verified by reproducing it**: two instrumented trees built
inside the source directory with a source edit between them fail the old
invocation with `Got function write_exact on multiple lines: 46, 48` and exit 64;
the new one exits 0.
- **22 public symbols shipped without a version or soname bump**, contradicting this
repository's own rule that for 0.x libraries the soname carries `MAJOR.MINOR`
deliberately. 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, and fails at
symbol resolution rather than at configure time. And
`AKGL_VERSION_AT_LEAST(0, 1, 0)` was true for both trees, **so a consumer could
not feature-test for the new API at all** -- `akbasic` pinned the requirement by
submodule commit instead, and said so in its README, which is not a thing a
released library should make anybody do. **Worth remembering the next time a
handful of symbols looks too small to bump for.**
### Found while rewriting the Doxygen comments
Each came out of reading an implementation against the contract its header claimed.
They are recorded inline as `@note` or `@warning` on the function concerned.
- **`akgl_sprite_load_json` did not bound the `frames` array.** The count is bounded
against `AKGL_SPRITE_MAX_FRAMES` before anything is written, and each element is
read into an `int` and narrowed deliberately rather than written through a
`uint32_t *` cast of a `uint8_t *`. A frame number that does not fit a `uint8_t`
is refused rather than truncated into an index that names a different tile.
`tests/sprite.c` covers exactly the maximum, one past it, and the wide frame
number; **against the old code the middle case loads happily.**
- **Two more unbounded array loads in the tilemap loader**, both fixed with the
bound at the top of the loop body. The object one is the reachable half -- 128
objects is not a large object layer and `akgl_TilemapObject` is not small.
- **`akgl_get_json_with_default` defaulted on a status the array accessors never
raise.** Worth knowing for the next test written against this function: **it
returns the context it was given** when it does not handle the status, so
`TEST_EXPECT_OK` -- which releases whatever the statement returns -- will
double-release it against a `CLEANUP` block that also releases it, and a
double-released context corrupts the failure instead of reporting it. The first
draft of that test did exactly that and passed against the unfixed library.
- **`akgl_character_sprite_add` leaked a sprite reference when a state is remapped.**
The new reference is taken *before* the write and given back if the write fails, so
there is no window where a sprite is bound with nothing behind it.
- **`akgl_heap_release_character` abandoned the whole state-to-sprite map.**
`src/heap.c:150` zeroed the character without walking `state_sprites`, so every
sprite reference the character took was lost and the `SDL_PropertiesID` holding the
map was never destroyed. **The test was already written**: `tests/character.c` had
asserted this contract all along -- "character did not reduce reference count of
its child sprites when released" -- and had never once run, for the two reasons
under "Test suites that could not fail".
- **`akgl_actor_render` computed a sprite's drawn height from its width.** Every
actor was drawn square, so a non-square sprite was stretched or squashed --
invisible in the fixtures because they are all square. `tests/actor.c` renders a
48x24 sprite through a backend whose `draw_texture` records the rectangle it is
handed rather than drawing it. **That recording backend is also the first coverage
`akgl_actor_render` has had at all.**
- **`akgl_text_rendertextat` refused the empty string while `akgl_text_measure`
accepted it.** The check sits after the font, text and backend guards rather than
before them, so drawing nothing still refuses the things drawing something refuses
-- a caller does not get a different contract for an empty string.
- **`character_load_json_state_int_from_strings` checked the same argument twice.**
**Not asserted, deliberately.** The function is `static` and has one call site,
which passes `&stateval`, so a `NULL` `dest` is unreachable from the public API and
the guard cannot fire. Testing it would mean giving the function external linkage
purely to reach a defensive check.
## Performance
The first measured baseline is in `PERFORMANCE.md`, produced by `tests/perf.c` and
`tests/perf_render.c` (`ctest --test-dir build -L perf`). Both suites hold every
measurement to a budget set at roughly ten times the recorded baseline, so an
algorithmic regression fails the suite rather than being discovered by a player.
**Read `PERFORMANCE.md` before arguing with anything below** -- every claim has a
number behind it, and several of the things expected to be slow are not.
The plan for the missed targets is #60.
### Defects the perf suites found
- **`akgl_path_relative` leaked an error context on every root-fallback
resolution.** It took its `ENOENT` branch by `return`ing from inside the `HANDLE`
block, which skips the `RELEASE_ERROR` that `FINISH` ends with. One entry of
`AKERR_ARRAY_ERROR` was lost per call, and the 129th call hit "Unable to pull an
error context from the array!" and **exited the process**. Every tilemap load
resolves several paths this way, so a game that loaded fifty levels died in the
loader. `tests/util.c` resolves `AKERR_MAX_ARRAY_ERROR * 2` paths through that
branch; **against the old code that test does not fail, it terminates the suite.**
This is the *only* `return` from inside a `HANDLE` block in `src/` -- the other
candidates use `SUCCEED_RETURN`, which releases correctly. Worth a grep before
anyone writes a new one, and it is now in `AGENTS.md`'s error-handling protocol,
which warned about `*_RETURN` inside `ATTEMPT` but not about returning out of
`HANDLE`.
- **`akgl_tilemap_load` leaked five pooled strings per load**, in two parts. Three
were `akgl_get_json_tilemap_property` leaking two scratch strings on every
*successful* property lookup. The remaining two were each a claim with no matching
release. **Finding the last two meant dumping the contents of every still-claimed
slot after a cycle rather than reading the code again**; `'tilelayer'` and an
assets directory named themselves immediately. Fixed alongside, same class:
`akgl_tilemap_load_layer_objects` released its scratch string after reading each
object's name and then kept using the slot -- and `akgl_get_json_string_value`
reuses a non-`NULL` destination *without* taking another reference, so the slot was
free while still live.
- **Two JSON accessors turned string-pool exhaustion into a segfault.**
`tests/json_helpers.c` claims every slot in the pool and asserts `AKGL_ERR_HEAP`
comes back out of both accessors. **Against the old code that test segfaults rather
than failing** -- and so did the new tilemap property-lookup test, which is how this
was confirmed rather than merely believed.
- **`akgl_game_update` segfaulted if `akgl_game_init` did not run.** `game.fps` is 0
for the first second of the process, which is under the threshold, so the unguarded
`akgl_game_lowfps` call fired on frame one. Only `akgl_game_init` installed the
default -- and `include/akgl/renderer.h` documents the other path on purpose: a
host that owns its own window calls `akgl_render_2d_bind` instead of
`akgl_render_2d_init`. **An embedder following that documentation crashed on its
first frame, and `akbasic` is exactly that embedder.**
- **`akgl_game_update` ran the actor update sweep once per tilemap layer.** The sweep
is hoisted out of the layer loop -- updating an actor is not a per-layer operation.
`tests/game.c` counts calls into a stub `updatefunc`; against the old code it
reports 16. **Counting is the assertion on purpose**: the defect was invisible in
the frame total because the tilemap blits are three orders of magnitude larger, so
a timing test would have measured the rasterizer.
### Targets
What a library like this *should* hit. The frame budget throughout is 16.67 ms
(60 fps); where a target is stated per-operation it is because that is the number
that survives a change of renderer.
The one that governs the rest: **libakgl's own bookkeeping should never be the
reason a frame is late.** Everything the library decides -- pool scans, registry
lookups, state-to-sprite mapping, physics, visibility, error contexts -- should fit
in 5% of a frame, leaving 95% for the pixels and the game's own logic. At 64 actors
and a screenful of tiles that is a ceiling of about 800 us.
| # | Target | Today | Verdict |
|---|---|---|---|
| 1 | Library bookkeeping under 5% of a 60 fps frame at 64 actors + 1200 tiles | ~0.3% (excluding pixel work) | **met** |
| 2 | One actor's logic update under 200 ns | 68.4 ns | **met** |
| 3 | One actor's render bookkeeping, excluding the blit, under 500 ns | ~250 ns, by subtraction | **met, weakly measured** |
| 4 | Physics sweep under 25 ns per *live* actor, proportional to live actors rather than pool size | 19 ns per live actor, but 63.9 ns for an *empty* pool | **partly** |
| 5 | Tilemap draw bookkeeping under 100 ns per tile, excluding the blit | ~25 ns | **met** |
| 6 | Pool acquire under 100 ns regardless of how full the pool is | 3.9 ns empty, 250.9 ns on the last free string slot | **missed** (#21) |
| 7 | Pool release proportional to the bytes actually used | 47.2 ns, a fixed 4 KiB wipe | **missed** (#21) |
| 8 | Re-drawing an unchanged line of text under 1 us | 12.6 us, every frame, no cache | **missed** (#22) |
| 9 | Zero texture creation or destruction per frame in steady state | one create + one destroy per line of text per frame | **missed** (#22) |
| 10 | A handled, routine condition costs no more than twice the path that succeeds | 616.5 ns vs 68.4 ns -- 9x | **missed** (#23) |
| 11 | 256 actors simulated, updated and made ready to draw in under 1 ms | ~22 us at 64 actors; linear, so ~90 us extrapolated | **met, untested at that size** |
| 12 | Collision for 256 actors under 2 ms without the caller writing a broad phase | 54.1 us at `-DAKGL_MAX_HEAP_ACTOR=256` | **met** |
| 13 | Level load under 100 ms for a map with 8 tilesets, 4 layers and 64 actors | 11.9 ms for a 2x2 map with one tileset | **unknown at that size** (#27) |
| 14 | Fixed per-load overhead under 1% of a level load | 11.5% -- zeroing 26 MB of `akgl_Tilemap` | **missed** (#28) |
| 15 | Static footprint under 4 MB in the default configuration | 28 MB, 94% of it one tilemap | **missed** (#28) |
| 16 | No pool leaks across a load/release cycle of any asset type | tilemap is clean, asserted over 64 cycles; sprite, spritesheet and character cycles are not asserted | **met where tested** (#13) |
| 17 | Pool exhaustion reports `AKGL_ERR_HEAP` and never crashes | all five pools report it | **met** |
| 18 | Every benchmark held to 10x its recorded baseline, enforced in CI | done, `ctest -L perf` | **met** |
Notes on the ones worth arguing about:
- **6 and 7 are the same fix**, and **8 and 9 are one cache**, and **14 and 15 are
the same refactor.** Each pair is one issue for that reason.
- **10 is a design target, not a speed target.** The answer is not a faster error
context; it is that "this character has no sprite for this state" is a question
with a boolean answer.
- **12 is a scope decision, not a defect, and it is done.** The structure is the
incremental uniform grid, not the tree -- cells keyed on tile size, static arrays,
insert/remove as actors move, wired into the `collide` slot of
`akgl_PhysicsBackend`. Construct measured that design at a 96% check reduction over
brute force and rejected quadtrees as costlier to maintain; Phaser's
rebuild-the-RTree-every-frame approach carries a documented ~5,000-body ceiling and
is the counterexample.
Measured here rather than cited: **the grid beats the BSP by 2.6x on the same
64-proxy population** (44.3 ns against 116.1 ns per query), before counting that the
tree rebuilds whenever a proxy moves and the grid's `move` is 11.9 ns when the cell
rectangle has not changed. Over a 4x range in actor count the step grew 3.5x and the
all-pairs control grew 15.6x, **which is the n2 the index exists to avoid.**
**The BSP ships anyway.** A vtable with one implementation behind it has never been
asked to be a vtable; `tests/partition.c` runs its whole contract against a table of
both, so the seam is proven rather than asserted. It would earn its place in a world
with wildly non-uniform object sizes or one larger than the grid's 128x128 cell
array covers.
## Memory checking
`cmake --build build --target memcheck` runs **the suites that already exist** under
valgrind -- `ctest -T memcheck` with the headless drivers forced, wrapped by
`scripts/memcheck.sh` so that a finding is an exit status rather than a line in a log
nobody reads. **There are no memory-check test programs, and there should never be
any**: `tests/benchutil.h` notices that it is running under valgrind and divides every
benchmark's iteration count by two thousand, which turns the perf suites into the
broadest path coverage in the tree at a cost valgrind can survive. The whole run is
about thirty seconds.
**The two halves fit together on purpose.** A benchmark is a program that walks one
path a hundred thousand times; a leak check wants every path walked once. Same
binaries, same registration, one flag apart.
Third-party findings are suppressed in `scripts/valgrind.supp`, and the run forces
`SDL_VIDEO_DRIVER=dummy` / `SDL_RENDER_DRIVER=software` / `SDL_AUDIO_DRIVER=dummy` so
the vendor GPU stack is never loaded -- that removes thousands of unfixable findings
inside `amdgpu_dri.so` **without suppressing anything at all.** Only *definite* losses
and invalid accesses are counted.
### Defects the memory checker found
**All six are fixed**, and the CI job that runs memcheck gates: the next one of these
fails the build on the push that introduces it. Every size below is measured.
1. **Every JSON loader leaked its parsed document.** Four `json_load_file` calls in
`src/` and not one `json_decref` anywhere in the library:
| Loader | Site | Leaked per call |
|---|---|---:|
| `akgl_sprite_load_json` | `src/sprite.c:140` | ~1,500 bytes |
| `akgl_character_load_json` | `src/character.c:232` | ~2,150 bytes |
| `akgl_tilemap_load` | `src/tilemap.c:693` | ~9,000 bytes (2x2 fixture map) |
| `akgl_registry_load_properties` | `src/registry.c:134` | not exercised by any test |
Blast radius: every asset load, forever. A 128x128 map leaks on the order of a
megabyte per load, and **this is the one item in this list that grew without
bound.** `akgl_registry_load_properties` needed its loop moved inside the `ATTEMPT`
block first: `props` is a borrowed reference into the document and was read after
the block ended. **The test is the memcheck run** -- nothing in the public API can
observe a jansson refcount, and inventing a hook to prove it would be testing the
test.
2. **`akgl_get_property` read up to 4 KiB past the end of the property value.** It
copied a fixed `AKGL_MAX_STRING_LENGTH` bytes out of whatever
`SDL_GetStringProperty` returns, and what it returns is an `SDL_strdup` of the
value -- four bytes for `"0.0"`. Valgrind reported an invalid read on **every
call**. This had been in `registry.h` as a `@note` about the copy being "a fixed
#AKGL_MAX_STRING_LENGTH bytes rather than the length" -- **filed as waste. It was
not waste, it was an out-of-bounds read**, and on a value that lands at the end of
a page it is a segfault in a getter.
3. **The savegame name tables read past the end of every registry key, and wrote what
they found into the file.** Two consequences, and the second is the interesting
one: the read can fault if the key sits at the end of a page, **and whatever it
reads goes into the save file, so a savegame contained up to 500 bytes of this
process's heap per registered object.** That is a file a player might send someone.
All four iterators write through `write_name_field` now, and a negative-array-size
typedef fails the build if any of the four widths ever outgrows the staging buffer.
4. **`akgl_controller_list_keyboards` leaked the array SDL gives it.** One `SDL_free`,
in a `CLEANUP` block so a failure inside the loop cannot take the array with it.
The same question was asked of every other SDL enumeration in `src/controller.c`;
`SDL_GetGamepads` and `SDL_GetGamepadMapping` are freed too.
5. **A font, once loaded, was never freed and could not be.** 10,523 bytes per font.
**It was a gap in the API rather than only a leak**: a game that switches fonts
between scenes, or a tool like `charviewer` that reloads one while the user picks a
size, had no way to give the old one back. `akgl_text_unloadfont` is a new public
symbol; `akgl_text_loadfont` calls it when it replaces a live name, **but only
after the new font has opened**, so a failed reload leaves the caller with the font
they already had. What is still missing is a teardown that calls it -- #15.
6. **Vendored `deps/semver`'s own unit test leaks 188 bytes.** Not libakgl's code and
not libakgl's to fix. **Suppressed**, which is what this list said the answer would
be if it ever became noise, and gating the job made it noise. The two entries in
`scripts/valgrind.supp` name the functions rather than the file, so a rewrite of
those cases stops being suppressed and comes back as a finding. **They are the only
entries there that hide a real leak in a program this build runs**; the alternative
was a fork of a vendored dependency.
### Not defects, and why
- **`tests/util.c` fixtures were reading uninitialised stack floats.** Three
null-pointer tests declared `SDL_FRect` and `point` fixtures without initializing
them, which is sixteen "conditional jump depends on uninitialised value" findings
for a test that is not about coordinates. The library was never at fault; **a memory
checker that reports noise gets ignored**, which is the only reason this was worth
touching.
- **"Still reachable" at exit is not counted.** SDL's global state, the hint table,
the property registry and FreeType's library instance are one per process and are
reclaimed by `SDL_Quit` / `TTF_Quit`. Counting them would bury the six findings
above under a hundred that mean nothing.
- **The GPU driver's findings are avoided rather than suppressed.** Running the suite
against the real driver produces thousands of findings inside `amdgpu_dri.so`;
running it headless produces none, and headless is what the suites are written for.
### Found while embedding libakgl in a consumer
**Shadowing `add_test()` unconditionally broke every embedding consumer.** Fixed in
the same release it was introduced in, and kept because the CMake behaviour behind it
is not obvious and will catch the next person.
**CMake chains command overrides exactly one level deep.** Overriding `add_test`
makes the builtin available as `_add_test`; overriding it a *second* time rebinds
`_add_test` to the *first* override and the builtin becomes unreachable to everybody,
permanently -- there is no `__add_test`. Verified directly rather than inferred from
the documentation:
```cmake
function(add_test) _add_test(${ARGV}) endfunction() # _add_test -> builtin
function(add_test) _add_test(${ARGV}) endfunction() # _add_test -> the first override
if(COMMAND __add_test) ... endif() # absent
```
So two projects in one tree cannot both shadow it. `akbasic` shadows it first -- it
has to, for `libakerror` and `libakstdlib`, which register their tests
unconditionally -- and its own registrations then recursed until CMake stopped at
"Maximum recursion depth of 1000 exceeded". **A consumer had no way to work around
it**: once the builtin is gone it is gone for that project too.
The override is guarded on being top-level again. An embedded `libakgl` leaves
`add_test` alone and lets its consumer suppress what it does not want -- machinery
the consumer has to have anyway.
## 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 had 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` 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.
## API gaps that blocked akbasic
`akbasic` (the C port of the BASIC interpreter,
`source.starfort.tech/andrew/akbasic`) links `libakgl` as a scripting engine for game
authors. **All ten items are resolved**, and its `libakgl`-backed text sink, graphics
backend, sound backend and input backend are written and tested.
Decisions worth keeping, because each was a fork in the road rather than an
implementation detail:
- **Text measurement.** `akgl_text_measure` and `_measure_wrapped` sit over
`TTF_GetStringSize`/`TTF_GetStringSizeWrapped`. 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.** The test font is
monospaced on purpose: it lets the suite assert `width("AAAA") == 4 * width("A")`
instead of hardcoding glyph metrics that FreeType is free to round differently.
- **Drawing: colour is an argument, not state.** There is no current-colour global to
get out of step with the caller's own. Each call saves and restores the renderer's
draw colour, so drawing a line does not change what the host's next
`SDL_RenderClear` paints. The flood fill reads the target back, fills on the CPU,
and blits only the bounding box of what changed, keeping a fixed
`AKGL_DRAW_MAX_FLOOD_SPANS` (4096) stack of horizontal runs at file scope rather
than recursing per pixel. **It is therefore not reentrant -- neither is anything
else that draws to a single `SDL_Renderer`.**
- **Audio: the voice table works with no device open.** 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. Phase is derived from the frame counter
rather than accumulated, because 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 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.
- **The sweep steps once every 1/60 second** (`AKGL_AUDIO_SWEEP_TICK_HZ`), because
that is the rate the machine this vocabulary comes from advanced its sweep at. 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.
**A swept voice accumulates its phase** instead of deriving it: deriving assumes a
constant frequency, and under a sweep it jumps the waveform at every step, which is
an audible click.
- **Keystrokes: the ring records every `SDL_EVENT_KEY_DOWN` *before* the control-map
scan**, 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. A full buffer drops the newest key rather than
overwriting the oldest, matching the Commodore keyboard buffer. **The composed 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 sitting at the end of the ring -- wrong in
exactly the case where things are already going badly.
- **`akgl_render_2d_bind` 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.
- **Vendored dependencies are added on both paths now**, guarded by
`if(NOT TARGET ...)`, through an `akgl_add_vendored_dependency(<target> <dir>)`
macro -- a macro rather than a function because `add_subdirectory()` inside a
function runs in that function's variable scope. Verified by configuring and
building a scratch consumer with `CMAKE_DISABLE_FIND_PACKAGE_*` set for all seven
dependencies, so any reliance on an installed copy would have failed the configure.
- **`tests/headers.c` 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.
Still missing for a complete BASIC vocabulary, and filed: the oscillating `SOUND`
direction and `FILTER` (#58). `TEMPO` and the `PLAY` note-string parser belong in the
interpreter rather than here.
## `akgl_collide_rectangles` stays; the corner helpers did not
The collision plan's last step was to **delete `akgl_rectangle_points`,
`akgl_collide_point_rectangle` and `akgl_collide_rectangles`** once the shape API
landed. Two of the three went, in 0.8.0, along with `akgl_Point` and
`akgl_RectanglePoints`. The third stays, and this is the reasoning rather than an
oversight.
**Why the two went.** They were the intermediate form of an implementation that
changed. `akgl_collide_rectangles` was eight corner-containment tests built on them;
it is four span comparisons now and does not go through either. Nothing outside
`tests/` called them, and a point-in-rectangle test is four comparisons a caller can
write without a struct conversion in front of it.
**Why the third stays.**
- **It has callers that are correct.** `examples/sidescroller/player.c` uses it twice,
for coins and for hazards. Those are overlap *questions* asked from an `updatefunc`
-- "am I touching this" -- and the right answer is a `bool`. Routing them through a
collision shape would mean a proxy, a broad-phase insert and a narrowphase call to
compute a normal and a depth that nothing reads.
- **It was fixed in this same series**, not left broken. It missed the cross case and
truncated through `akgl_Point`'s `int` members; it is exact in `float32_t` now.
**Deleting it two commits after fixing it is a strange thing to do to a caller.**
- **A game-level overlap question is not always a physics question.** A minimap
marker, a UI hit test, "is the cursor over this". None of those wants to be pushed,
and none of them belongs in a collision world.
## Why libakstdlib's collections are not used
`aksl_HashMap`, `aksl_List` and `aksl_TreeNode` exist and libakgl uses SDL property
sets for every registry instead. **That is not an oversight** -- the registries hand
`SDL_PropertiesID` values to callers and SDL owns the lifetime. Recorded so nobody
re-derives it.
The wrappers that were adopted, and why each was worth it: `aksl_fclose` (an unchecked
close in `akgl_game_save` lost buffered data silently), `aksl_fgetc` (`fgetc(3)` spells
EOF and a read error the same way), `aksl_snprintf` at two sites that were truncating
under a silenced `-Wformat-truncation`, and `aksl_strncpy` at the two `src/game.c`
sites the fixed-width copy sweep missed. Whether to adopt the pure-arithmetic
wrappers is #19.
## Why `deps/tg` is still in the tree
`deps/tg` is pinned at v0.7.9 and nothing uses it. It was evaluated alongside
`deps/libccd` for the collision work and lost, for two reasons that are properties of
the library rather than preferences: **it is 2D permanently** (its own documentation
says the z coordinate serves no purpose but export, and no predicate reads it), and
**its predicates return `bool`** -- resolving a collision needs a minimum translation
vector, which `tg_geom_intersects` cannot supply.
It is kept rather than dropped because there is one job it does that libccd
structurally cannot: **indexed point-in-polygon against concave polygons with holes.**
GJK and MPR are convex-only, always.
**That is a deadline, not a permanent exemption** -- #55, with #75 as the consumer
that would justify it.
## The ccd arena
`src/collision_arena.c` is one static arena of `AKGL_CCD_ARENA_BYTES` (64 KB) with a
bump pointer, reset at the top of every `akgl_collision_test`. **The size is a
measurement, not a guess**: one GJK/EPA box pair measures 7,264 bytes. Exhaustion is
reported, not crashed -- `ccdGJKPenetration` returns -2 and `akgl_collision_test`
raises `AKGL_ERR_COLLISION` with the high-water mark in the message, **so the failure
names its own fix.** MPR, the default on the frame path, allocates nothing at all.
`akgl_ccd_arena_highwater()` is public so a game can assert its own ceiling. The
threading constraint that follows from a single arena is #57.