From eabb9ad376991b3027940a631bed7ead4eff57b6 Mon Sep 17 00:00:00 2001 From: Tachikoma Date: Sun, 2 Aug 2026 18:47:34 -0400 Subject: [PATCH 1/2] 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) --- AGENTS.md | 6 +- PERFORMANCE.md | 2 +- README.md | 4 +- TODO.md | 3181 +++++++-------------------- TODO.txt | 27 - docs/01-introduction.md | 2 +- docs/03-getting-started.md | 2 +- docs/04-errors.md | 2 +- docs/05-the-heap.md | 2 +- docs/06-the-registry.md | 12 +- docs/07-the-game-and-the-frame.md | 2 +- docs/08-rendering.md | 2 +- docs/10-spritesheets-and-sprites.md | 6 +- docs/11-characters.md | 2 +- docs/12-actors.md | 8 +- docs/13-tilemaps.md | 6 +- docs/14-physics.md | 4 +- docs/15-collision.md | 2 +- docs/16-input.md | 2 +- docs/17-text-and-fonts.md | 6 +- docs/18-audio.md | 2 +- docs/19-utilities.md | 6 +- docs/20-tutorial-sidescroller.md | 13 +- docs/21-tutorial-jrpg.md | 10 +- docs/23-appendix-limits.md | 7 +- docs/README.md | 2 +- examples/jrpg/jrpg.c | 3 +- examples/jrpg/textbox.c | 4 +- examples/sidescroller/player.c | 4 +- include/akgl/collision.h | 6 +- include/akgl/controller.h | 2 +- include/akgl/error.h | 2 +- include/akgl/game.h | 2 +- include/akgl/heap.h | 4 +- include/akgl/registry.h | 4 +- include/akgl/text.h | 5 +- include/akgl/tilemap.h | 2 +- src/collision.c | 2 +- src/game.c | 2 +- tests/json_helpers.c | 2 +- tests/perf_render.c | 2 +- tests/physics_sim.c | 2 +- tests/renderer.c | 2 +- 43 files changed, 850 insertions(+), 2520 deletions(-) delete mode 100644 TODO.txt diff --git a/AGENTS.md b/AGENTS.md index 0b51199..8006936 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -309,7 +309,7 @@ and are unaffected either way. **Never silence a warning with a cast.** Three `-Wpointer-sign` findings in `src/sprite.c` were real signedness mismatches -- `uint32_t *` passed where an `int *` was expected -- and a cast would have hidden every one. That is the -whole argument of `TODO.md` item 37. Read into a correctly typed local, check +whole argument of issue #5. Read into a correctly typed local, check the range, and assign. ### No repository-wide formatter @@ -496,8 +496,8 @@ to that device are accepted and the `ENOSPC` surfaces at the flush. **Do not convert the pure-arithmetic calls.** `memset`, `memcpy`, `strlen`, `strcmp` and friends can only fail on a NULL argument, and the tree is -inconsistent about them already; see `TODO.md`, "libakstdlib wrappers not yet -adopted", before starting a sweep. +inconsistent about them already; see issue #19, which is the decision this +waits on, before starting a sweep. **Never silence `-Wformat-truncation`.** `include/akgl/error.h` still exports `DISABLE_GCC_WARNING_FORMAT_TRUNCATION` and nothing uses it. Both sites that diff --git a/PERFORMANCE.md b/PERFORMANCE.md index f287815..3229b5a 100644 --- a/PERFORMANCE.md +++ b/PERFORMANCE.md @@ -563,7 +563,7 @@ day a GPU backend makes the pixels cheap. ## Defects these tests found -Six, and they are filed where defects live: `TODO.md`, under **Performance -> +Six, and they are recorded where defects live: `TODO.md`, under **Performance -> Defects the perf suites found**, items 28-33, each with its file, line, functional consequence, and what fixing it would touch. They are not repeated here. diff --git a/README.md b/README.md index 1a20453..e2e0d8e 100644 --- a/README.md +++ b/README.md @@ -184,7 +184,7 @@ AKGL_BENCH_SCALE=0.1 ctest --test-dir build -L perf # a tenth of the iterations Each measurement is the best of five runs and is held to a budget set at roughly ten times the recorded baseline, so a suite that turns red means an algorithmic regression rather than a busy machine. Budgets are enforced only in an optimized build at full scale; below `AKGL_BENCH_SCALE=1.0`, and in a coverage build, they are reported without failing. -`PERFORMANCE.md` carries the recorded baseline, the frame budget it adds up to, and what the numbers say — including the raw-SDL control rows that separate what libakgl costs from what the rasterizer costs. The six defects the stress tests turned up, and the targets the numbers are measured against, are in `TODO.md` under **Performance**. +`PERFORMANCE.md` carries the recorded baseline, the frame budget it adds up to, and what the numbers say — including the raw-SDL control rows that separate what libakgl costs from what the rasterizer costs. The six defects the stress tests turned up, and the targets the numbers are measured against, are in `TODO.md` under **Performance**; the plan for the missed targets is issue #60. ## Memory checking @@ -211,7 +211,7 @@ this tree, that notice has to travel with it. It is the only dependency in that position. SDL3, SDL3_image, SDL3_mixer, SDL3_ttf, jansson, libakerror and libakstdlib are all linked as separate shared objects and carry their own notices; `deps/semver` is a single header, MIT, installed alongside ours; and `deps/tg` is -vendored but **compiled into nothing** -- see `TODO.md`, "tg is vendored and has no +vendored but **compiled into nothing** -- see `TODO.md`, "Why deps/tg is still in the consumer". libccd is given a static arena rather than the allocator it ships with, so "libakgl does not diff --git a/TODO.md b/TODO.md index aeb03ef..a00c267 100644 --- a/TODO.md +++ b/TODO.md @@ -1,16 +1,83 @@ -# TODO +# Record + +**Outstanding work is in the issue tracker, not in this file:** + + +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 -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. +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. -### 1. Public naming conventions +### Public naming, resolved in 0.5.0 -**Items 1 through 6 are resolved in 0.5.0**, which is an ABI break and carries -the soname to `libakgl.so.0.5`. What changed, and what a consumer has to rename: +An ABI break, carrying the soname to `libakgl.so.0.5`. What a consumer had to +rename: | Was | Is | |---|---| @@ -27,412 +94,198 @@ the soname to `libakgl.so.0.5`. What changed, and what a consumer has to rename: | `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 `` — and its +`include/akgl/staticstring.h` also stopped guarding with `_STRING_H_` -- a name +several libc implementations use for their own `` -- and its `#include "string.h"` is now `#include `. -`akgl_render_bind2d` was not on the original list. It is the same defect as -`akgl_render_init2d`, `2d` trailing where the six functions it installs carry it -in the middle, and renaming one without the other would have been worse than -renaming neither. - -The renames were done by renaming each declaration and letting the compiler find -every use, rather than by pattern substitution. That distinction matters for +**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. -**Item 6 had a live bug behind the name**, now fixed. `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 +**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 +lower bound. **Nothing exercised the contended path before** -- the existing lock test only ever took an uncontended mutex, which never reaches the retry loop. -**Item 4 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 a `SDL_Renderer *` through an +**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 — see -"Test suites that could not fail" below. It now binds a real backend with -`akgl_render_2d_bind`, the way `tests/sprite.c` and `tests/text.c` do. A +`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. - -### 2. Header/implementation surface drift - -**Items 7, 8, 9, 11, 12, 13, 14 and 15 are resolved in 0.5.0.** Item 10 is -resolved for every pair it listed. - -7. **Nineteen non-static functions were defined in `src/` but declared in no - header.** Each is now declared or `static`: - - - `akgl_controller_handle_button_down`, `_button_up`, `_added`, `_removed` - were defined as `gamepad_handle_*` while `controller.h` declared the - `akgl_controller_handle_*` names it never defined. The definitions now - carry the declared names, which closes this and **Defects → Known and - still open item 10** in one change, and their documentation moved from the - definitions to the header where the convention puts it. `tests/controller.c` - no longer needs its local re-declarations. - - `akgl_game_save_actors` and `akgl_game_load_versioncmp` are declared in - `game.h` under a new "part of the internal API" block; `tests/game.c` - reaches them through the header now instead of declaring them itself. - - The four save-table iterators and `akgl_game_load_objectnamemap` are - `static` and have dropped the prefix -- `save_actorname_iterator`, - `load_objectnamemap` and so on. They are SDL enumeration callbacks and a - file-local reader; nothing outside `game.c` has any business calling them. - - `akgl_get_json_properties_number`, `_float`, `_double`, - `akgl_tilemap_load_layer_image`, `akgl_tilemap_load_layer_object_actor` and - `akgl_tilemap_load_physics` are declared in `tilemap.h`'s internal-API - block, again with their documentation moved to the header. That is where - the "does not need a renderer" work under **Remaining work** wanted them. - - `akgl_path_relative_root` is `static path_relative_root`. - `akgl_path_relative_from` is **deleted**; see below. - - **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: four of these - symbols were *mentioned* in `controller.h` prose, which is not the same as - being declared there and is exactly how they went unnoticed. - -8. **`akgl_game_init_screen` was declared and never defined.** The declaration - is gone. Screen setup is `akgl_render_2d_init`. - -9. **Static helpers used three naming styles.** They drop the `akgl_` prefix - now, which is what it is for: `character_load_json_inner`, - `character_load_json_state_int_from_strings`, `sprite_load_json_spritesheet`, - alongside the `actor_visible`, `write_exact` and `write_name_field` that - already did. - -10. **Parameter names disagreed between declaration and definition.** All six - pairs agree now. `akgl_character_initialize` takes `obj`, - `akgl_character_state_sprites_iterate` takes `props`, `akgl_heap_release_character` - takes `ptr`, `akgl_set_property` takes `value`, and the two `akgl_tilemap_draw*` - functions take `map` -- the header called them `dest` and documented them as - "Output destination populated by the function", which they are not. - - `akgl_get_json_with_default` was the interesting one: it took the incoming - context as `err` and named its *own* context `e`, which is the name the - convention reserves for an incoming one. It is `e` and `errctx` now, and - renaming it was what surfaced that the two had been swapped rather than - merely misspelled. - -11. **Object-pool size macros were defined twice and the override hook was - dead.** `AKGL_MAX_HEAP_ACTOR`, `_SPRITE`, `_SPRITESHEET` and `_CHARACTER` are - defined once, in `heap.h`, inside the `#ifndef` guards that were always - supposed to make them overridable. `actor.h`, `sprite.h` and `character.h` - no longer define them. - - `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 or if - `AKGL_MAX_HEAP_SPRITE` stopped deriving from `AKGL_MAX_HEAP_ACTOR`. The - assertion is the compile -- it deliberately disagrees with the built - library's ceilings and never touches the pools, which is fine because - overriding a ceiling means the library and everything linking it have to be - rebuilt together anyway. - - -12. **Headers relied on their includers for types.** `iterator.h` used - `uint32_t` without ``; `json_helpers.h` used `json_t` without - ``; `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 now 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 this item 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. - -13. **Include spelling was split between quoted and angled forms.** - **Resolved.** Every in-project include in a public header uses - `#include `, and `staticstring.h`'s `#include "string.h"` -- - a relative-first lookup that reached the system header by accident -- is - `#include `. `tests/*.c` still use `#include "testutil.h"`, - correctly: that one is test-local rather than installed. - -14. **Empty parameter lists.** **Resolved.** `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. - -15. **`AKERR_NOIGNORE` was applied inconsistently at definition sites.** - **Resolved.** It is on the declarations, where it does its work, and on no - definition in `src/`. - -### 3. Error-handling pattern - -16. **`*_RETURN` macros are used inside `ATTEMPT` blocks, which skips `CLEANUP`.** - **Fixed in 0.5.0.** Ten sites, found by scanning rather than from this list - -- it 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. - - `akgl_controller_default` was the other behavioural one: its - `SUCCEED_RETURN` was the last statement in the `ATTEMPT` block, so the path - that falls out of `FINISH` reached the closing brace of a non-void function. - - **`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. - -17. **NULL-check discipline varies by function.** **Fixed in 0.5.0.** The eight - typed JSON accessors that validated their container and then wrote through - `dest` unconditionally now check `key` and `dest` as the two string - accessors always did; the null physics backend checks its actors like the - arcade one; and `akgl_render_2d_frame_start`, `_frame_end` and `_shutdown` - check `self`, which the first two read straight through. - - `tests/renderer.c` calls all three with `NULL`, which segfaulted before. - -18. **Error-context variable naming is split between `errctx` and `e`.** - **Fixed in 0.5.0**, in its own commit because it is a rename and nothing - else. All 45 remaining sites are `errctx`; applying `\be\b -> errctx` to - each removed line reproduces the added line exactly, and the counts match at - 333 either way. - - `e` keeps its meaning where the convention wants it -- an incoming context - being inspected, as in `akgl_get_json_with_default(e, ...)`. - -### 4. Types and macros - -**Items 19 through 23 are resolved in 0.5.0.** - -19. **`float`/`double` used raw where `types.h` defines aliases.** The four - signatures and twelve struct fields that spelled them out now use - `float32_t` and `float64_t` like the actor and character structs. They are - plain typedefs, so this is a spelling change and not an ABI one. - `float64_t`'s doc no longer says "unused so far". - -20. **`AKGL_COLLIDE_RECTANGLES` had unbalanced parentheses.** Deleted. Three - opens against two closes meant any expansion was a syntax error, it had no - callers, and it duplicated `akgl_collide_rectangles`. - -21. **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 now uses that shape, and fails against the old macros. - -22. **The state and iterator bit macros mixed forms.** - `AKGL_ITERATOR_OP_UPDATE` is `(1 << 0)` like its 31 siblings, and every - `1 << n` in `iterator.h` and `actor.h` is parenthesized, with the - hand-aligned value columns preserved. - - The bit-pattern comments in `actor.h` are 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. - - **Newly recorded, and still open:** `1 << 31` is undefined behaviour on a - signed `int`. It is `(1 << 31)` in both tables and wants to be an unsigned - shift, but `akgl_Actor::state` is `int32_t` and `akgl_Iterator::flags` is - `uint32_t`, so the two tables do not want the same answer. Worth deciding - deliberately rather than sneaking a `u` in. - -23. **`akgl_Frame` was defined and never used.** Deleted. - -### 5. `AKGL_ACTOR_STATE_STRING_NAMES` disagrees with `actor.h` - -**All three resolved in 0.5.0.** - -24. **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. Both are `[AKGL_ACTOR_MAX_STATES]` now, and the definition is sized - by the macro rather than by a literal. - -25. **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 through - `AKGL_REGISTRY_ACTOR_STATE_STRINGS`, 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. - -26. **The generation comment was stale.** There is no generator, no Makefile - and no `lib_src/`. The comment is gone and the file's own header now says it - is maintained by hand and states the two invariants that keep breaking. - -### 6. Doxygen drift - -27. **Three struct doc comments in `tilemap.h` were rotated by one.** Already - correct -- the doxygen rewrite fixed this before it was checked here. - `akgl_TilemapObject`, `akgl_TilemapLayer` and `akgl_Tileset` each describe - themselves. - -28. **`point` documented as two-dimensional with an `x`, `y` and `z`.** Already - correct, and the type is `akgl_Point` now. - -29. **Doc comments on the definition rather than the header.** Resolved with - item 7: the four controller handlers and the six tilemap loader helpers had - their documentation moved to the header when they were declared there. - -### 7. Formatting and hygiene - -**Items 31 through 36, 38 and 41 are resolved in 0.5.0. Item 37 is not; see -below.** - -31. **Leftover debug code.** The 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 -- are gone. - -32. **Large commented-out blocks.** The five abandoned `SDL_GetBasePath()` - path-prefixing lines across `assets.c`, `sprite.c`, `character.c` and - `tilemap.c` are gone. They were superseded by `akgl_path_relative`. - -33. **Unused locals.** `curTime` in `akgl_game_update` and in - `akgl_render_2d_draw_world`, `j` in `akgl_render_2d_draw_world` (shadowed - by its own inner loop), `target` in `akgl_character_sprite_get`, both - `result` declarations in `util.c`, and `screenwidth`/`screenheight` in - `akgl_game_init`. `opflags` in `akgl_heap_release_character` is no longer - unused -- it is what drives the state-sprite walk added for **Defects** - item 21. - -34. **`akgl_game_update`'s default flags OR-ed the same bit twice.** It reads - `AKGL_ITERATOR_OP_UPDATE | AKGL_ITERATOR_OP_LAYERMASK` now. This is a - statement of intent rather than a behaviour change, and the reason is worth - knowing: **nothing in that loop reads either bit**. It never compares - `actor->layer` to the layer it is sweeping, which is exactly **Performance** - item 32 -- every actor updated sixteen times a frame -- and that is still - open. - -35. **`akgl_draw_background` was the only public function outside the error - protocol.** It now 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 instead of leaving it - changed. - - It was listed under **Remaining work** as needing the offscreen renderer - harness. It does not, and never did -- what it needed was to stop reading - the global. `tests/draw.c` covers the checkerboard pattern, the restored - draw colour, zero and negative sizes, and a `NULL` backend. - -36. **`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()`, which is **Defects → Known - and still open** item 3. `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. `tests/registry.c` sets a property and reads it back. - -37. **Redundant casts obscure the code.** **Still open -- but the precondition - it named is now met.** Roughly 180 pointer casts across `src/`, a large - majority no-ops, densest in `src/tilemap.c` and `src/character.c`. - - This item used to say the benefit only arrives once the build turns on the - warnings those casts suppress, and that doing it first buys nothing but - churn. `-Wall` is on now (see `AGENTS.md`, "Compiler warnings"), so the - sweep can proceed as its own commit. - - **The argument turned out to be right, with evidence.** Turning `-Wall` on - found three genuine signedness mismatches in `src/sprite.c`: `obj->width`, - `obj->height` and `obj->speed` are `uint32_t` and were being passed straight - to `akgl_get_json_integer_value(..., int *)`. A cast would have silenced all - three. They are fixed by reading into an `int`, range-checking, and - assigning -- which also turned up that `speed` is scaled by 1,000,000 into a - 32-bit field, so anything past 4294 ms overflowed rather than being held. - - When doing the sweep: remove a cast, rebuild, and read what the compiler - says. A cast that was load-bearing will say so. - -38. **`struct`-qualified parameters in two definitions.** `akgl_physics_null_move` - and `akgl_physics_arcade_move` use the typedef like the other eight. - -39. **`text.c` validated the wrong argument.** Resolved earlier, alongside the - text measurement work. - -40. **`akgl_path_relative_from` disagreed on the output parameter.** Moot: the - function is deleted. See **Defects → Known and still open** item 4. - -41. **`dst` vs `dest` for output parameters.** `akgl_string_copy` and - `akgl_path_relative` take `dest` now, like everything else. - - -## Test suites that could not fail - -Found while renaming the exported globals, and the reason item 4 above is filed -as a real defect rather than a style complaint. Both are fixed. - -**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 in item 4 — and exited 0 because that status was -`AKGL_ERR_SDL`. Two independent defects had to line up, and they did, for long -enough that `TODO.md` 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. - -## Coverage status +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 ``; `json_helpers.h` used `json_t` without ``; +`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: @@ -442,21 +295,9 @@ cmake --build build-coverage --parallel ctest --test-dir build-coverage --output-on-failure ``` -Reports land in `build-coverage/coverage/` (`index.html`, `coverage.xml`). - -**Line coverage 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. All 25 registered tests pass, and that count now -includes three non-C ones: `api_surface`, `error_protocol` and the generated -per-header compile checks inside `headers`. - -The `character` suite deserves its own line. It was recorded here as passing; -it was not running. See "Test suites that could not fail" below. - -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. +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 | |---|---|---| @@ -480,2095 +321,613 @@ it. | `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. `src/assets.c` is still the -one file with no coverage at all. +`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 (`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. +**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/.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 -`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): +**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 | - -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. - -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. +**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. -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 -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. +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. -### 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/.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%. -- `tests/audio.c` — every waveform, the ADSR envelope stage by stage, gate - 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%. -- `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%. -- `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. - -## Remaining work - -### Needs the offscreen renderer harness - -`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), -`akgl_actor_render`/`actor_visible` in `src/actor.c` (53), and the drawing half -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. - -Build `tests/harness.c` / `tests/harness.h` with `akgl_test_init_headless()` and -`akgl_test_shutdown_headless()`: set the dummy video and audio drivers, -`SDL_Init()`, `akgl_heap_init()`, `akgl_registry_init()`, create a software -`SDL_CreateWindowAndRenderer`, and point the global `renderer` at it. Seven -existing tests hand-roll this today (`tests/sprite.c:194`, `tests/character.c:200`, -`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. - -Then: - -- **`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. -- **`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. -- **`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. +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 behavior. +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 `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. + `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. + `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. +7. **The gamepad handlers checked `appstate` three times each**, so a NULL event or + a missing player actor was never caught. -### Known and still open +### Fixed in 0.5.0 and later -1. **`akgl_render_and_compare` compares a texture against itself.** - **Fixed in 0.5.0**: the second pass draws `t2`. Both passes drew `t1`, so it - always reported a match and every image assertion built on it -- including - the ones in `tests/sprite.c` -- asserted nothing. +- **`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.** -2. **`akgl_tilemap_release` double-frees tileset textures.** - **Fixed in 0.5.0.** 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. + 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. - `tests/tilemap.c` loads the fixture map, releases it three times, and - asserts every texture pointer is `NULL`. + **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. -3. **`akgl_registry_init` never initializes the properties registry.** - **Fixed in 0.5.0**, alongside internal-consistency item 36, which is the same - function. `akgl_registry_init()` calls `akgl_registry_init_properties()` now, - so `AKGL_REGISTRY_PROPERTIES` is live for callers that do not also go through - `akgl_game_init` -- which is what made `akgl_set_property` a silent no-op and - `akgl_get_property` always hand back the caller's default, and so what made - `akgl_physics_init_arcade` and `akgl_render_2d_init` quietly ignore their - configuration. `tests/registry.c` sets a property and reads it back. -4. **`akgl_path_relative_from` is a stub that leaks.** **Deleted in 0.5.0.** - It claimed a heap string, never wrote its output, and never released it, so 256 - calls exhausted the string pool. It was declared in no header, called from - nowhere, and duplicated what `akgl_path_relative` already does. Fixing an - unfinished function nobody can reach is worse than deleting it; this also - closes item 40, which was about the output-parameter shape it disagreed with - the rest of the family on. -5. **`akgl_compare_sdl_surfaces` memcmps without checking geometry.** - **Fixed in 0.5.0**: dimensions, pitch and pixel format are compared first, - and any difference is a mismatch. `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. + 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. -6. **`akgl_string_initialize` overflows by four bytes when `init` is NULL.** - **Fixed in 0.5.0**: it zeroes `sizeof(obj->data)`. The four bytes it used to - run 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. - - `tests/staticstring.c` claims two adjacent slots, stamps a sentinel into the - second's refcount, and initializes the first. - - Fixed alongside it, because it is the same file and the 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. It is `AKERR_OUTOFBOUNDS` now, and a - negative count is refused too. -7. **Savegame name lengths disagree between writer and reader.** **Fixed in - 0.5.0.** The reader names the same constant the writer does, per table -- - `AKGL_SPRITE_SHEET_MAX_FILENAME_LENGTH` for spritesheets and so on. It used - `AKGL_ACTOR_MAX_NAME_LENGTH` for all four; the other three are 128 as well, - so 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. That is what turns a width disagreement into `AKERR_IO` instead of a - corruption, and it is the assertion `tests/game.c` hangs on. The new - roundtrip test registers a name in each of the four registries, with a - full-length one in the spritesheet registry, and fails with `AKERR_IO` - against a mismatched reader. - - 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 at this introduced four `AKGL_GAME_SAVE_*_NAME_WIDTH` aliases, - one per table, 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: raising one of those lengths is an ABI change, - which bumps the version, and `akgl_game_load` refuses a save whose - `libversion` does not match before it reads a single table. What actually - guards the widths is the EOF check, which works however they are spelled. - -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.** **Fixed in 0.5.0**; it is called, and passes. - -10. **`controller.h` declares functions that do not exist.** **Fixed in 0.5.0**; - the definitions carry the declared `akgl_controller_handle_*` names now. See - internal-consistency item 7, and `scripts/check_api_surface.sh`, which is - what stops this class of drift coming back. -11. **`akgl_controller_pushmap` and `akgl_controller_default` accept negative map - ids.** **Fixed in 0.5.0**: both check the lower bound as well. - `tests/controller.c` passes -1 and -4096 to each. - -12. **A failed controller-DB fetch silently destroys the tracked fallback.** - **Fixed in 0.5.0**, both halves. - - `mkcontrollermappings.sh` now runs under `set -euo pipefail`, fetches into a - temporary directory, and moves the result into place only after checking - three things: 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. Any of those failing leaves the tracked header - exactly as it was and exits non-zero. `AKGL_CONTROLLERDB_URL` and - `AKGL_CONTROLLERDB_MIN_LINES` are environment-overridable, which is how the - failure paths were exercised. - - Verified against all five: 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. It is an explicit `controllerdb` target now, and - the header is no longer listed as a library source, which is all it was - there for. - - The empty-initializer problem goes with it: `const char *SDL_GAMECONTROLLER_DB[] = {};` - is a constraint violation in ISO C that compiles only as a GCC extension, - and the minimum-count check is what guarantees at least one entry. - - Regenerating against the real upstream produced byte-identical mappings -- - 2255 entries, no content change -- so the rewrite is faithful. The only diff - is the `$(date)` stamp and the include guard, which is - `_AKGL_SDL_GAMECONTROLLERDB_H_` now to match every other header. That change - was made in the generator rather than by hand, and the tracked copy carries - it so a future regeneration shows no spurious diff. - -13. **A stale build tree in the source directory breaks the coverage run.** - **Fixed in 0.5.0.** Both `gcovr` invocations take the build tree as an - explicit positional search path and neither passes `--object-directory`. - - 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 were built inside the - source directory with a source edit between them, so they described - different line numbers for the same functions. The old invocation fails with - `Got function write_exact on multiple lines: 46, 48` and exits 64; the new - one exits 0 and the whole 25-test coverage run passes with the stale tree - still sitting there. - - The second, smaller version of the same thing -- rebuilding a coverage tree - after editing a test leaves `.gcda` files describing the old object layout, - and `coverage_reset` fails with `GCOV returncode was 5` before it can delete - them -- is unchanged. `find build-coverage -name '*.gcda' -delete` clears - it. That one is gcov's, not gcovr's search path. - -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. + 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 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. +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. -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.** - **Fixed in 0.5.0.** 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 too, - rather than truncated into an index that names a different tile. - - `tests/sprite.c` covers exactly the maximum (must load, and every id must - arrive), one past it, and the wide frame number, against three new fixtures. - Against the old code the middle case loads happily. - -17. **Two more unbounded array loads in the tilemap loader.** - **Fixed in 0.5.0**, both with the bound at the top of the loop body, the - shape `akgl_tilemap_load_layers` in the same file already used. - - `tests/tilemap.c` covers both against generated fixtures: an object layer - of exactly 128 (must load) and of 129, and a map with 17 tilesets. The - object one is the reachable half -- 128 objects is not a large object layer - and `akgl_TilemapObject` is not small. - -18. **`akgl_get_json_with_default` defaults on a status the array accessors never - raise.** **Fixed in 0.5.0**: a third `HANDLE_GROUP(e, AKERR_OUTOFBOUNDS)`, - placed *above* the arm holding the `memcpy`, since `HANDLE_GROUP` emits no - `break` and every arm falls into that body. - - `tests/json_helpers.c` covers it through the integer and object index - accessors, so the fix is pinned to the status rather than to one call site. - - 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 this test did exactly that and passed against the unfixed - library. It takes the result into a local and hands ownership over - explicitly now. - -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.** **Fixed in 0.5.0**: it reads the existing entry first and - releases it once the new binding is recorded, and the write is checked. - - 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. - Rebinding a state to the sprite already there is not treated as a - displacement. - - `tests/character.c` binds, rebinds, and then runs 200 alternating rebinds, - asserting the displaced sprite's count comes back each time. This is the - half **Carried over** item 1 calls replacement; there is still no API to - *remove* a binding without replacing it. - -21. **`akgl_heap_release_character` abandons the whole state-to-sprite map.** - `src/heap.c:150` zeroed the character without walking `state_sprites`, so - every sprite reference the character took in `akgl_character_sprite_add` was - lost and the `SDL_PropertiesID` holding the map was never destroyed. Loading - and releasing characters in a loop — level to level — exhausted the sprite - pool and leaked an SDL property set each time. - - **Fixed in 0.5.0.** At a refcount of zero it enumerates `state_sprites` with - `akgl_character_state_sprites_iterate` and `AKGL_ITERATOR_OP_RELEASE`, then - `SDL_DestroyProperties`, then zeroes the slot. The iterator existed for - exactly this walk and simply was never called from here. - - The test was already written. `tests/character.c` has 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". It runs now, and it fails against the old code. - - Still open and separate: **item 20**, `akgl_character_sprite_add` leaking the - displaced sprite when a state is *remapped*. Releasing at character teardown - does not cover a binding replaced while the character is alive. - -22. **`akgl_path_relative_root` uses `FAIL_RETURN` inside its `ATTEMPT` block.** - **Fixed in 0.5.0** with internal-consistency item 16, which swept every - such site. It is `FAIL_BREAK`, and `path_relative_root` is `static` now. - `scripts/check_error_protocol.py` fails the build if one comes back. - -23. **Three smaller leaks on failure paths.** **All three fixed in 0.5.0**, each - by moving the release into a `CLEANUP` block. - - - `akgl_render_2d_init` released its two pooled strings only after both - `aksl_atoi` calls succeeded, so a non-numeric `game.screenwidth` leaked - two of the pool's 256 entries. `tests/renderer.c` runs 512 failing - initializations against each of the two properties and asserts the pool is - unchanged; against the old code the first loop claims every slot. - - `akgl_controller_open_gamepads` freed the enumeration array only after the - loop completed, so a gamepad that failed to open took the array with it. - The open failure is recorded and reported after the loop, because a - `FAIL_ZERO_BREAK` inside it would have broken the loop rather than the - block. It also frees the array on the "no gamepads enumerated" path, which - SDL still allocates for. - - `akgl_text_rendertextat` destroyed the surface and texture only on the - success path, so a failed upload leaked the surface and a failed draw - leaked both -- once per frame on a HUD line. - -24. **`akgl_get_property` reads past the end of the property value.** - **Already fixed**, as **Defects the memory checker found** item 35 -- the - two entries are the same defect found twice, from reading the code and from - running valgrind. Recorded here only so the duplicate does not read as - outstanding. - -25. **`character_load_json_state_int_from_strings` checks the same argument - twice.** **Fixed in 0.5.0**: the second guard's subject is `dest`, which is - what it was always meant to be. - - **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, which undoes - internal-consistency item 9. The fix is a one-word correction to a guard - that is there for the next call site, not for this one. - -26. **`akgl_actor_render` computes a sprite's drawn height from its width.** - **Fixed in 0.5.0**: `dest.h` takes `curSprite->height`. 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, - and asserts the destination is 48x24 and 96x48 at scale 2. That recording - backend is also the first coverage `akgl_actor_render` has had at all -- - every other test in the file stubs `renderfunc` out. - -### Found while closing the akbasic API gaps - -27. **`akgl_text_rendertextat` refuses the empty string, and `akgl_text_measure` accepts it.** - **Fixed in 0.5.0**: it returns success without rasterizing when - `text[0] == '\0'`, matching the measure side. - - 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. - - `tests/text.c` had the case written and deliberately unasserted, waiting for - the two halves of the header to agree. It is a `TEST_EXPECT_OK` now, on both - the wrapped and unwrapped paths, and against the old code it reports - `AKERR_NULLPOINTER`. +- **`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 -here has a number behind it, and several of the things I expected to be slow are -not. +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 -Ordered by blast radius. Numbering continues the **Defects** list above. +- **`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.** -28. **`akgl_path_relative` leaked an error context on every root-fallback - resolution.** `src/util.c:118` 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. - - **Fixed.** The branch now records a flag and calls - `akgl_path_relative_root` after `FINISH`. `tests/util.c` carries - `test_akgl_path_relative_releases_contexts`, which resolves - `AKERR_MAX_ARRAY_ERROR * 2` paths through that branch and asserts the pool - is where it started; 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 worth a line in AGENTS.md's - error-handling protocol, which warns about `*_RETURN` inside `ATTEMPT` but - not about returning out of `HANDLE`. - -29. **`akgl_tilemap_load` leaks five pooled strings per load.** **Fixed in - 0.5.0**, in two steps, and the split changes what the number means. - - Three of the five were `akgl_get_json_tilemap_property` leaking two scratch - strings on every *successful* property lookup, through a `SUCCEED_RETURN` - inside its `ATTEMPT` block -- internal-consistency item 16. That took the - measured leak from five per load to two. - - The remaining two were each a claim with no matching release: - `akgl_tilemap_load_layers` never gave back the string it read every layer's - `type` into, and `akgl_tilemap_load` never gave back the `dirname` its - relative paths resolve against. Both release in `CLEANUP` now. - - `tests/tilemap.c` asserts the pool is exactly where it started after one - load/release cycle and after 64 -- enough that a leak of one string per load - could not finish. 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 file and 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 and any - other claim could have been handed it. - - The tilemap-load benchmark in `tests/perf_render.c` no longer needs to - reclaim the pool by hand between iterations. - -30. **Two JSON accessors turn string-pool exhaustion into a segfault.** - **Fixed in 0.5.0**: `FINISH(errctx, true)` in both, so a failed - `akgl_heap_next_string` reaches the caller instead of being swallowed and - then `strncpy`d through. - - `tests/json_helpers.c` claims every slot in the pool and asserts - `AKGL_ERR_HEAP` comes back out of both accessors with the destination left - untouched. 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. - -31. **`akgl_game_update` segfaults if `akgl_game_init` did not run.** - **Fixed in 0.5.0**: `akgl_game_update_fps` installs `akgl_game_lowfps` when - it finds the hook `NULL`. - - Worth keeping the reasoning. `game.fps` is 0 for the first second of the - process, which is under the threshold, so the unguarded 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. - - `tests/game.c` clears the hook and calls the function, which is precisely - that state. `tests/perf_render.c` installed the default by hand as a - workaround and no longer has to. - -32. **`akgl_game_update` runs the actor update sweep once per tilemap layer.** - **Fixed in 0.5.0.** The sweep is hoisted out of the layer loop -- updating - an actor is not a per-layer operation, and `akgl_render_2d_draw_world` - already walks the layers for the half that is. - - `AKGL_ITERATOR_OP_LAYERMASK` is honoured rather than ignored now, so a - caller that genuinely wants one layer can ask for it and gets each of those - actors once. It is no longer in the default flag set: every live actor once - is the job, and restricting the sweep is the caller asking for less. - - `tests/game.c` counts calls into a stub `updatefunc` and asserts exactly one - per live actor per frame, two over two frames, and the layer mask selecting - only its own layer. 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. The timing evidence is the - gap between the `akgl_game_update` and `draw_world` rows of the same - benchmark run: 92 us before, and noise in both directions after. See - `PERFORMANCE.md`. - -33. **`akgl_heap_release_character` leaks its `state_sprites` property set.** - **Fixed in 0.5.0**; see item 21, which is the same defect. The - character-load benchmark in `tests/perf_render.c` was written around it and - no longer has to be. + 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. These are not predictions of what the -current code does — several are missed today, and each says which. 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. +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 µs. +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 rather than direct measurement | **met, weakly measured** | -| 4 | Physics sweep under 25 ns per *live* actor, and proportional to live actors rather than pool size | 19 ns per live actor, but 63.9 ns for an *empty* pool | **partly** | +| 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** | -| 7 | Pool release proportional to the bytes actually used, not to the slot's capacity | 47.2 ns, a fixed 4 KiB wipe | **missed** | -| 8 | Re-drawing an unchanged line of text under 1 µs | 12.6 µs, every frame, no cache | **missed** | -| 9 | Zero texture creation or destruction per frame in steady state | one create + one destroy per line of text per frame | **missed** | -| 10 | A handled, routine condition costs no more than twice the path that succeeds | 616.5 ns vs 68.4 ns — 9x | **missed** | -| 11 | 256 actors simulated, updated and made ready to draw in under 1 ms | ~22 µs at 64 actors (5.8 µs measured logic + estimated render bookkeeping); linear, so ~90 µs extrapolated | **met, untested at that size** | -| 12 | Collision for 256 actors under 2 ms without the caller writing a broad phase | 54.1 µs for a whole physics step with collision 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** | -| 14 | Fixed per-load overhead under 1% of a level load | 11.5% — zeroing 26 MB of `akgl_Tilemap` | **missed** | -| 15 | Static footprint under 4 MB in the default configuration | 28 MB, 94% of it one tilemap | **missed** | -| 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** | -| 17 | Pool exhaustion reports `AKGL_ERR_HEAP` and never crashes | all five pools report it, and the two JSON accessors that used to `strncpy` through a NULL now report it too | **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** | -Targets 16 and 17 moved in 0.5.0, with **Defects** items 29 and 30. Target 16 is -qualified rather than met outright on purpose: the tilemap cycle is the one that -was leaking and the one that is now asserted, and nothing yet asserts the same -of a sprite, spritesheet or character load/release cycle. That is a gap in the -tests, not a known leak. - Notes on the ones worth arguing about: -- **6 and 7 are the same fix.** The acquire scan is 64x slower on a full string - pool than an empty one purely because the pool is a megabyte and the scan - touches one refcount per 4 KiB. A free-list index — one `int` per layer, - remembering where the last free slot was — takes both to constant time without - changing the "no `malloc`" rule at all. That is the change I would make first, - and it is worth doing *before* anyone raises `AKGL_MAX_HEAP_*`, because the - cost of the current design grows with the ceiling rather than with the usage. +- **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. -- **8 and 9 are one cache.** A single entry keyed on (font, string, colour, - wrap) would cover the common case — a HUD field that changes once a second — - and a four- or eight-entry ring would cover the rest. This is the clearest - optimisation in the library and it is maybe forty lines. + 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.** -- **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, and reporting it through `AKERR_KEY` costs - nine times the update it replaces. `akgl_character_sprite_get` wants a - companion that returns `NULL` without raising. - -- **12 is a scope decision, not a defect.** The library deliberately does not - own a broad phase, and at 64 actors the naive all-pairs loop is 0.7% of a - frame — genuinely fine. At 256 it is 11%. Either the ceiling stays where it is - and this target is dropped, or a uniform grid keyed on tile size goes in. I do - not think a spatial index belongs here yet; I think the target belongs on - record so that raising `AKGL_MAX_HEAP_ACTOR` is a decision made with the - number in front of it. - -- **14 and 15 are the same fix too.** `akgl_Tilemap` is 26 MB because every - layer carries a 512x512 `int` grid and every tileset a 65,536-entry offset - table, sized for the worst case at compile time. The pool rule does not - require *this*: a layer could carry an index into one shared cell arena sized - by `AKGL_TILEMAP_MAX_WIDTH * AKGL_TILEMAP_MAX_HEIGHT` once rather than - sixteen times, and the offset table could be sized by `tilecount` rather than - by the maximum. That is a real refactor with a real ABI break, so it belongs - to 0.4 rather than to a patch release — but 28 MB of BSS on a handheld or an - ESP32-class target is the difference between fitting and not. - -- **13 is untested and should not stay that way.** The only map fixture in the - tree is 2x2 with one tileset, which is why the load benchmark measures a PNG - decode and a `memset` rather than a map. A realistic fixture — 128x128, four - layers, several tilesets — would make target 13 measurable and would probably - find something. - -### The plan - -The order below is the order the work should happen in, set by blast radius -per frame and per byte, not by how interesting the change is. Items 1 through -6 are the next changes; 7 through 9 are on record so the decision that -unblocks each is made with the numbers in front of it. The engine comparison -that informs several of these is in `PERFORMANCE.md` under **How other -engines spend the same frame**. - -1. **String pool free-list index — targets 6 and 7, one change.** A next-free - hint per pool in `src/heap.c` (the five scans are `src/heap.c:52-116`) - takes the last-slot claim from 250.9 ns to constant time, and it lands - before anyone raises `AKGL_MAX_HEAP_*`, because today's cost grows with the - ceiling rather than the usage. Fold in item 8 under **Known and still - open** — `akgl_heap_next_string` is the only acquire that takes a - reference, and a free-list change touches every acquire anyway. Make the - release wipe (`src/heap.c:211`) proportional to bytes used: either `memset` - through `strnlen(data, AKGL_MAX_STRING_LENGTH) + 1` or a tracked length - field — and the length-field option must respect the layout history in - item 6, which put two real bugs on the bytes after `data`. Budgets to - move: `tests/perf.c:317`, `:333`, `:347`; re-record the three rows in - `PERFORMANCE.md` in the same commit. - -2. **Text texture ring cache — targets 8 and 9, one cache.** A static - four-to-eight entry ring in `src/text.c` keyed on (`TTF_Font *`, text - bytes, `SDL_Color`, wraplength) — `x`/`y` stay out of the key, position - only affects `dest`. Invalidate in `akgl_text_unloadfont` - (`src/text.c:40-55`) and `akgl_text_unloadallfonts` (`:76-87`): a cached - texture keyed on a closed font whose address was reused is a stale hit, so - unload must sweep the ring. Verification is a counting test, not a - stopwatch — target 9 is *zero* texture create/destroy per steady-state - frame, so count them through a stub; plus a new perf row ("rendertextat, - cached") with the raw-SDL control row the perf rules require. Budget to - move: `tests/perf_render.c:391`. - - **The UI subsystem is this cache's second consumer, and the bigger one.** - `akgl_ui_execute_commands` (`src/ui.c`) draws every TEXT render command - through `akgl_text_rendertextat`, one wrapped line per command per frame — - a menu of five rows re-rasterizes five lines at 60 Hz whether or not any - of them changed, where the original HUD case was one line changing once a - second. Abstract-on-the-second-consumer says this item's time has come; - the key already fits, since clay hands the executor stable text bytes - between frames. - -3. **Non-raising sprite lookup — target 10, a design change.** Give - `akgl_character_sprite_get` (`src/character.c:84-94`) the companion that - returns `NULL` without raising, then convert the three sites that raise - and handle `AKERR_KEY` for a routine condition: `akgl_actor_update` - (`src/actor.c:165`, handled `:172`), `actor_visible` (`:209`, handled - `:212-216`) and `akgl_actor_render` (`:241`, handled `:245-249`). While - there, stop `akgl_actor_render` looking the sprite up twice on the success - path (`:241` and again inside `actor_visible` at `:242`). Takes the - sprite-less update from 616.5 ns toward the twice-success ceiling. Budget - to move: `tests/perf.c:588`. - -4. **Demote the hot-path log lines.** `akgl_actor_initialize` logs every - spawn (`src/actor.c:51`) and `akgl_character_sprite_add` every binding - (`src/character.c:80`); 20% of a spawn is formatting a line nobody reads, - and that is with the output thrown away. Demote them (and - `src/character.c:285`, `src/renderer.c:34`, `src/tilemap.c:660`) from - `SDL_Log` to `SDL_LogDebug` — SDL checks priority before formatting, so - the cost disappears at default priority and the messages survive for - anyone who turns debug on. The spawn benchmark pair - (`tests/perf.c:410`, `:429`) already measures exactly this gap. - -5. **Fix the tileset scan, and the two bugs hiding in it.** The per-tile scan - in `akgl_tilemap_draw` (`src/tilemap.c:756-798`) is the known - O(tiles x tilesets) FIXME, worth 0.8% of the frame at eight tilesets — but - it is also wrong twice. The range test uses `>=` on the upper bound - (`firstgid + tilecount >= tilenum`, `src/tilemap.c:760-761`), so the tile - one past the end of set A also matches set B's `firstgid`; and there is no - `break` after a match, so a tile matching two ranges is blitted twice. - Correct the test to `tilenum < firstgid + tilecount`, `break` on match, - and skip `tilenum == 0` before scanning at all. Verify with a counting - test — a stub `draw_texture` backend asserting exactly one blit per - non-empty visible cell, same pattern as the `tests/game.c` update counter — - because a boundary double-blit is invisible to a stopwatch and mostly - invisible on screen. File alongside, same entry: `start_x`/`start_y` are - never clamped to zero (`src/tilemap.c:719-728`), and the first-column - `src.x +=` accumulates across rows (`:766`). Mutation testing is - mandatory here; this is control flow. Do not reorganize the loader for - this — 0.8% does not justify it. - -6. **Bound the draw_world layer loop.** `akgl_render_2d_draw_world` - (`src/renderer.c:150-167`) walks all `AKGL_TILEMAP_MAX_LAYERS` and rescans - all 64 actor slots per layer — 1024 refcount checks a frame for a - one-layer map. Bound the walk by `akgl_gamemap->numlayers` and build the - per-layer actor lists in one pool pass into static index arrays. - Invisible at 60 fps under the software renderer; measurable on a 2 ms GPU - frame. Counting test, same as item 5. - -7. **Target 12: done in 0.8.0, and the prediction held.** 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` that the simulation loop now calls. 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. - Citations stay in `PERFORMANCE.md`. - - 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. A whole physics step - with collision is 15.4 µs at 64 actors and 54.1 µs at 256, against a 2 ms - target. Over that 4x range the step grew 3.5x and the all-pairs control grew - 15.6x, which is the `n²` 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. - -8. **Target 13 needs the fixture before it needs anything else.** A - 128x128, four-layer, multi-tileset JSON map fixture, so - `akgl_tilemap_load` is measured on a map rather than on a PNG decode and a - `memset`. Today the loader benchmark uses the 2x2 fixture while - `perf_render` synthesizes its 128x128 map in-process - (`tests/perf_render.c:141-175`) — the draw path is measured at realistic - size and the load path is not. It would probably find something. - -9. **Targets 14 and 15: the footprint refactor, sketched for when it is - scheduled.** One shared cell arena sized - `AKGL_TILEMAP_MAX_WIDTH * AKGL_TILEMAP_MAX_HEIGHT` once instead of - per-layer, and tileset offset tables sized by `tilecount` instead of by - `AKGL_TILEMAP_MAX_TILES_PER_IMAGE`. One correction to the record: the - FIXME at `include/akgl/tilemap.h:113-128` worries about wasted *leading* - entries per tileset, but `akgl_tilemap_compute_tileset_offsets` - (`src/tilemap.c:179-231`) indexes by local id from zero — the table is - merely oversized, not sparse. The refactor kills most of the 1.37 ms - `memset` in `akgl_tilemap_load` (`src/tilemap.c:615`) and takes BSS from - 28 MB toward the 4 MB target. Real ABI break; belongs to a minor release, - as the note above already says. + **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. +`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 +**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; "still -reachable" is what SDL and FreeType keep for the process lifetime and says -nothing about this library. +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.** They are kept here rather than deleted because the sizes -are measured and the reasoning is worth having next time somebody asks why a -loader ends in a `CLEANUP` block, or why a name field is staged through a zeroed -buffer. `cmake --build build --target memcheck` is clean, and the CI job that -runs it gates: the next one of these fails the build on the push that -introduces it. +**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. -Ordered by blast radius as they were found. Numbering continues the lists above. -Every size below is measured, not estimated. +1. **Every JSON loader leaked its parsed document.** Four `json_load_file` calls in + `src/` and not one `json_decref` anywhere in the library: -34. **Every JSON loader leaks its parsed document.** There are four - `json_load_file` calls in `src/` and not one `json_decref` anywhere in the - library, so the whole parsed tree — objects, hashtables, strings — is - abandoned on both the success and failure paths: + | 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 | - | 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. The map figure is for the 2x2 - fixture; a real map's JSON is the size of its layer data, so a 128x128 map - leaks on the order of a megabyte per load. A game that reloads a level on - death leaks a level's worth of JSON each time, and this is the one item in - this list that grows without bound. - - **Fixed.** `json_decref` in the `CLEANUP` block of each, on the success - path as well as the failure one, with the handle nulled after so a second - pass cannot double-release. `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, so every exit from that - loop leaked the document. 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. - -35. **`akgl_get_property` reads up to 4 KiB past the end of the property - value.** `src/registry.c:181` copies 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 reports an - invalid read on **every call**, twelve of them in `tests/physics.c` alone, - because `akgl_physics_init_arcade` reads six properties and - `akgl_render_init2d` reads two more. - - This has 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 - is not waste, it is an out-of-bounds read: today it walks into the rest of - SDL's heap and returns garbage past the terminator, and on a value that - lands at the end of a page it is a segfault in a getter. - - **Fixed.** The copy is bounded by the value's own length plus its - terminator, and a value too long for an akgl_String is now refused with - AKERR_OUTOFBOUNDS instead of silently truncated. The documented - AKERR_NULLPOINTER for "unset with no default" is unchanged -- it is raised - deliberately now rather than arriving from inside `aksl_memcpy`. - `tests/registry.c` fills the destination with a sentinel, reads a - three-byte property back, and asserts every byte past the terminator still - holds the sentinel; that assertion fails against the old code. - -36. **The savegame name tables read past the end of every registry key, and - write what they find into the file.** `akgl_game_save_actorname_iterator` - (`src/game.c:219`) writes `AKGL_ACTOR_MAX_NAME_LENGTH` — 128 — bytes - starting at the key SDL handed it, and SDL allocated that key to fit the - name. Valgrind catches it on a 40-byte allocation. The three sibling - iterators do the same thing at `src/game.c:248`, `src/game.c:280` and - `src/game.c:308`, with 128, 512 and 128 byte fixed widths; only the actor - one is reached by the current tests, because the other registries are empty - in the save roundtrip. - - 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 contains up to 500 bytes of this process's heap per - registered object — anything that happened to be next to the key. That is a - file a player might send someone. - - **Fixed.** All four iterators now write through `write_name_field`, which - stages the key into a zeroed fixed-width buffer, so the padding is - deterministic and nothing but the name leaves the process. A - negative-array-size typedef fails the build if any of the four widths ever - outgrows the staging buffer. Still open and unrelated to the overread: - **Defects -> Known and still open** item 7, the same tables disagreeing - about their widths between writer and reader. - -37. **`akgl_controller_list_keyboards` leaks the array SDL gives it.** - `src/controller.c:188` calls `SDL_GetKeyboards`, which allocates, and never - calls `SDL_free` on the result. Four bytes per call in the test environment - — one keyboard id — but it is per call, and the function is shaped like - something a game calls when a device is hotplugged. - - **Fixed.** One `SDL_free`, in a `CLEANUP` block so a failure inside the - loop cannot take the array with it. Still worth asking the same question of - every SDL enumeration in `src/controller.c`: `SDL_GetGamepads` has the same - contract, and the dummy driver reports no gamepads, so no test reaches it. - -38. **A font, once loaded, is never freed and cannot be.** - `akgl_text_loadfont` (`src/text.c:20`) opens a `TTF_Font`, puts the pointer - in `AKGL_REGISTRY_FONT`, and that is the last anyone can do about it: the - header exposes no way to close a font, and `SDL_Quit` destroying the - property registry drops the last reference. 10,523 bytes per font — 736 of - them SDL_ttf's, the rest FreeType's. - - This is bounded by how many fonts a game loads, so it is not the runaway - that item 34 is. 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. - - **Fixed**, and this one is a new public symbol rather than a repair: - `akgl_text_unloadfont(char *name)` clears the registry entry and closes the - font. `akgl_text_loadfont` calls it when it replaces a live name, which - closes the second leak the header used to document as intended behaviour -- - but only after the new font has opened, so a failed reload leaves the caller - with the font they already had. The version goes to 0.4.0 with it: an 0.3 - consumer cannot be handed this library and told it is the same ABI. - - Still open: nothing closes the registry's remaining fonts at shutdown. A - game that exits without unloading leaks them exactly once, which valgrind - reports against the process rather than against a loop, and which - `akgl_game_shutdown` would be the natural home for if it existed. - -39. **Vendored `deps/semver`'s own unit test leaks 188 bytes** across 16 blocks, - from the `calloc`s in `test_strcut_first` and `test_strcut_second` - (`deps/semver/semver_unit.c:8` and `:21`). Not libakgl's code and not - libakgl's to fix; recorded so that nobody re-diagnoses it, and because - `semver_unit` is registered as one of our CTest tests and so shows up in our - memcheck run. - - **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. + 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 and then made one real call with them at the end, which is - sixteen "conditional jump depends on uninitialised value" findings for a test - that is not about coordinates. The fixtures are zeroed now. 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 anyway. + 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 -40. ~~**Shadowing `add_test()` unconditionally broke every embedding consumer.**~~ **Fixed in - the same release it was introduced in**, and worth keeping because the CMake behaviour - behind it is not obvious and will catch the next person. +**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. - 18399f2 moved the vendored-dependency block out from behind - `if(CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR)` — which was the point of that - commit — and took the `add_test()`/`set_tests_properties()` override out with it. The - override's own comment claimed "an embedding consumer's `add_test()` still reaches CTest". - It does not, and cannot. +**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 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 +``` - ```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. - 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. - - **Fix:** the override is guarded on being top-level again, exactly as it was before 18399f2 - and exactly as `libakstdlib` guards its own. The vendored-dependency block stays - unconditional, which is what that commit was actually for. An embedded `libakgl` leaves - `add_test` alone and lets its consumer suppress what it does not want — machinery the - consumer has to have anyway. +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 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. - -## API gaps blocking akbasic - -`akbasic` (the C port of the BASIC interpreter, `source.starfort.tech/andrew/akbasic`) is -being built to link into `libakgl` as a scripting engine for game authors. - -**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. - -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`. - -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. - -These were filed here rather than worked around in `akbasic` because growing `libakgl` to serve -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. - -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. - -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.** - - **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`. - -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`. - - **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). - -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. - - **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. - -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. - - **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. - -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. - - **Resolved.** The vendored block no longer sits behind - `CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR`. An - `akgl_add_vendored_dependency( )` 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.** - -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. - - **Resolved.** `controller.h` includes ``. 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. - -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. - - **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.** - -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. - - **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. - -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. - - **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. - -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. - - **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%. - -## Truncated registry keys can collide - -Found while turning `-Wall` on. `akgl_actor_initialize` and -`akgl_character_initialize` document their name fields as "Truncated, not -rejected, if the source name is longer", and that is what they do -- the copy is -`aksl_strncpy` bounded to the field, so it always terminates now. - -Termination was the overread half, and it is fixed. The other half is not: the -truncated name **is the registry key**. Two distinct 200-character names -truncate to the same 127-byte key, and the second `SDL_SetPointerProperty` -silently replaces the first. The objects are different; the registry cannot tell. - -The same applies to `akgl_Sprite::name` (128), `akgl_SpriteSheet::name` (512) -and the tilemap's object and tileset names. - -Whether that matters depends on whether long asset names are realistic, and 127 -bytes is generous for a hand-written name in a JSON file. Recording it rather -than fixing it, because the fix is a contract change -- refuse an over-long name -with `AKERR_OUTOFBOUNDS` instead of truncating -- and every one of those headers -currently promises the opposite. `aksl_strncpy` already reports exactly that -status when the bytes do not fit, so the change is to stop capping `n` at -`size - 1` and let it raise. - -## akgl.pc names no dependencies - -`akgl.pc.in` carries `Libs: -L${libdir} -lakgl` and no `Requires:` or -`Requires.private:` line, so a consumer that finds libakgl through pkg-config is -told nothing about libakerror, libakstdlib, SDL3, SDL3_image, SDL3_mixer, -SDL3_ttf or jansson. - -That has always been wrong, and libakerror 2.x makes it sharper: `akerror.h` is -part of libakgl's *public* interface -- `IGNORE` and the `FAIL_*` macros expand -in the consumer's own translation units and reference `__akerr_last_ignored`, -which is thread-local as of 2.0.0. A consumer that builds against whatever -`akerror.h` happens to be on its include path and links whatever `libakerror.so` -the loader finds can get a mismatch that pkg-config had every opportunity to -prevent and did not mention. - -The fix is a `Requires:` line for the libraries whose headers libakgl's headers -include (akerror, akstdlib, SDL3) and `Requires.private:` for the rest, plus a -version floor of `akerror >= 2.0.1`. Not done here because it changes what -`pkg-config --libs akgl` emits for every existing consumer, and that wants to be -its own commit with an install-tree test behind it. The CMake path is not -affected -- `akglConfig.cmake` re-finds the targets. - -## libakstdlib wrappers not yet adopted - -`libakgl` links `libakstdlib` and uses ten of its wrappers. It calls the raw -libc function at a good many more sites. Adopting the four that were carrying a -real defect is done (see the 0.6.0 commit); this is what is left, and the reason -each is left. - -**Adopted, for the record:** `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. - -### The pure-arithmetic wrappers are deliberately not adopted - -`memset` (38 sites), `strlen` (14), `strcmp` (9), `strncmp` (9), `memcpy` (6), -`memcmp` (2). Every one of these can only fail on a NULL argument, and at all -but a handful of those sites the argument is a stack object whose address cannot -be NULL. `aksl_memset` and `aksl_memcpy` are used twice each, which is the -inconsistency worth naming: there is no rule distinguishing those two sites from -the other 44. - -Converting them costs an `errctx` check per line and buys a NULL check the -compiler already knows is redundant. Not converting them leaves the file mixing -two spellings of the same operation. **Pick one and write it down** -- the -recommendation is to convert only where the argument is a parameter or a heap -pointer, and say so in `AGENTS.md`, rather than either extreme. - -### `DISABLE_GCC_WARNING_FORMAT_TRUNCATION` is now dead - -`include/akgl/error.h` exports it and `RESTORE_GCC_WARNINGS`, and after the -`aksl_snprintf` conversion nothing in the tree uses either. They are public -macros, so removing them is an API change and is not done here. Both suppressed -sites turned out to be real truncations, which is the argument for deleting -rather than keeping them: the two times this library silenced that warning, the -compiler was right. - -### No wrapper is used for the collections - -`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. - -## Arcade physics feel - -Found by building `tests/physics_sim.c`, which runs the arcade backend as a game -would -- a Mario-esque jump and fall, Zelda-style top-down movement, and a -fast run reversed at speed -- and prints what the actor actually did. Three -defects it found are fixed in 0.6.0 (an unbounded first step, release handlers -zeroing the gravity accumulator, and per-axis thrust caps composing to a 141% -diagonal). These are what it found and did **not** fix. - -### No terminal velocity - -`akgl_physics_arcade_gravity` adds `gravity_y * dt` to `ey` every step and -nothing bounds it. `sy` caps *thrust*, deliberately -- it is the character's own -top speed, not a speed limit on the world -- so a fall accelerates without limit -for as long as it lasts. The jump simulation reaches 560 px/s in 0.7 s and would -keep going. - -Drag is the mechanism that exists for this: `physics.drag.y` makes `ey` -approach `gravity_y / drag_y` instead of diverging. So the behaviour is -reachable, it just is not the default and is not documented as the way to get a -terminal velocity. Either document it that way or add an explicit -`physics.terminal_velocity`. Touches `src/physics.c` and -`include/akgl/physics.h`; no ABI change if it is a property rather than a field. - -### Releasing a direction stops the actor dead - -`akgl_actor_cmhf_*_off` sets `tx` (or `ty`) to zero, so a character running at -full speed stops within one frame of the key coming up -- the top-down -simulation measures 0.0 px of drift in the second after release. That is correct -for Zelda and wrong for Mario, who slides. There is no deceleration or ground -friction anywhere in the arcade backend; `ax` is the only rate, and it applies -only while a direction is held. - -The shape that fits the existing model is a per-character deceleration that -decays `tx` toward zero over several frames instead of assigning zero, with the -current behaviour as the value that means "instant". That is a new -`akgl_Character` field and an ABI break, so it wants to land with other -character changes rather than alone. - -### Integration is explicit Euler, so trajectories are frame-rate dependent - -Velocity is applied to position using the velocity computed *this* step, and -thrust is accumulated before the cap, so the same jump traces a slightly -different arc at 30 Hz than at 144 Hz. The reversal simulation measures 0.500 s -to turn around where the closed form predicts 0.489 s -- about 2% at 60 Hz, and -it grows with the step. - -`max_timestep` bounds how bad it gets (that is half of why it exists), and 2% is -not a bug a player can see. It is recorded because the fix -- a fixed-step -accumulator, integrating in whole `max_timestep` slices and carrying the -remainder -- would also remove the slow-motion trade that bounding the step -currently makes, and the two are the same piece of work. - -### A large drag coefficient can invert velocity - -`actor->ex -= actor->ex * self->drag_x * dt` is a first-order decay, so it only -decays for `drag * dt < 1`. Past that it overshoots zero, and past `2` it -diverges. With `dt` bounded to the default 0.05 s that needs a drag coefficient -above 20, which is not a plausible setting, so this is a sharp edge rather than -a live defect -- but nothing rejects it, and `max_timestep` is caller-settable. -`src/physics.c`, in the three drag blocks. The exact fix is -`expf(-drag * dt)` instead of `1 - drag * dt`, which is unconditionally stable. - -## Carried over - -1. **Make character-to-sprite state bindings release their references symmetrically.** - `akgl_character_sprite_add()` increments the sprite refcount for every state-map - binding, but no corresponding removal API exists. Replacing a state binding - also leaves the previous sprite's refcount incremented. When - `akgl_heap_release_character()` drops the character refcount to zero, it clears - the character registry entry and zeroes the structure without enumerating - `state_sprites`, decrementing the bound sprites, or destroying the SDL property - map. Implement binding removal/replacement so each removed binding releases - exactly one sprite reference. On final character release, enumerate every - remaining binding (the existing `akgl_character_state_sprites_iterate()` release - path may be reusable), release each reference, destroy `state_sprites`, and then - clear the character. Add tests for removal, replacement, duplicate sprite - bindings across multiple states, and final character release. - - **The final-release half is done in 0.5.0** — see **Defects** item 21. - `akgl_heap_release_character` now enumerates every remaining binding, releases - each reference, destroys `state_sprites`, and then clears the character, and - `akgl_character_state_sprites_iterate` is covered by that path. - - **Replacement is done too**, as **Defects** item 20: - `akgl_character_sprite_add` releases the sprite it displaces, and - `tests/character.c` covers binding, rebinding, and 200 alternating rebinds. - - **Still open: removal.** There is no API to unbind one state without binding - something else over it -- `akgl_character_sprite_del` -- and no test for - duplicate sprite bindings across several states. Neither leaks anything - today; they are a gap in the surface rather than a defect. - -2. **An actor cannot be scaled per axis.** `akgl_Actor::scale` is one `float32_t` applied - to both `dest.w` and `dest.h`, so there is no way to express "twice as wide, the same - height". The VIC-II has had separate x- and y-expand bits since 1982 and Commodore - BASIC 7.0's `SPRITE` verb exposes both, so a BASIC interpreter drawing through libakgl - cannot implement its own documented verb. - - Two shapes are plausible and the choice is a design decision rather than an obvious - fix: add `scale_x` / `scale_y` and keep `scale` as a convenience that writes both, or - replace `scale` outright and take the ABI break while the major version is still 0. - The second is cleaner and the soname already carries `MAJOR.MINOR`. - - Related to defect 26, and reached the same way: `akbasic`'s sprites are Commodore - sprites, 24 wide and 21 high, and `SPRITE n,,,,1` expands one axis. It works around - both by installing its own `renderfunc` on each actor rather than patching around the - library. - - **Half of that workaround is no longer needed.** Defect 26 -- `akgl_actor_render` - taking the drawn height from the sprite's width -- is fixed in 0.5.0, so a - non-square sprite draws at its own proportions through the library's own - `renderfunc`. This item is the half that remains: one uniform `scale`, with no way - to expand a single axis. - -3. **`akgl_ControlMap.mouseid` and `.penid` are still dead fields, deliberately.** - The UI subsystem (0.9.0) brought the mouse into the library, and it would have - been natural to wire these up on the way -- it did not, on purpose. Control maps - translate device events into *actor handler calls*, and the UI wants absolute - pointer state per event; those are different consumers, and building - gameplay-mouse machinery into `src/controller.c` with no game asking for it is - the abstract-before-the-second-consumer mistake. The mouse path lives in - `src/ui.c` (`akgl_ui_handle_event`); when a game wants mouse-driven actors, - *that* is the consumer these fields were declared for, and the work lands in - `controller.c` then. Same for `.axis`, `.axis_range_min` and `.axis_range_max`, - which `akgl_controller_handle_event` has never consulted. - -## Found while writing the manual - -Twenty-one chapters and two tutorial games were written against `src/` rather than -against the header comments, and the exercise turned up two distinct classes of problem. -Both are recorded here because publishing a problem you cannot fix yet is still a -contribution, and because the second class is the more dangerous one: **every item in it -was documented, in a header, incorrectly.** - -The manual documents each of these where a reader would hit it, and points here. - -### Defects with no prior entry - -1. **`akgl_game_update` calls `simulate` through a NULL pointer.** `akgl_game_update` - invokes `akgl_physics->simulate(akgl_physics, NULL)` with no NULL check, and - `akgl_default_physics` is zeroed BSS — all four method pointers are NULL. A program - that does not call `akgl_physics_init_arcade`/`_null` itself therefore **segfaults on - its first frame**, measured as exit 139, rather than raising `AKERR_NULLPOINTER`. - `physics.h:8-9,195` tells the reader `akgl_game_init` selects a backend from a - `physics.engine` property, so a caller who believes the header writes exactly the - program that crashes. This is the worst first-contact experience in the library and - the fix is a NULL check. Closing it touches `src/game.c` only. - -2. **Parent/child offset is double-counted at draw time.** `src/physics.c:192-197` writes - a child's `x` as `parent->x + vx` — an absolute world coordinate. `src/actor.c:273-279` - then draws it at `parent->x + obj->x`, while `actor_visible` three lines above tests - the camera against the raw `obj->x` as absolute. Two readings of one field inside one - function. Confirmed by measurement: with the player at (280,146) and a (-14,+10) - offset, the guarded draw lands at (130,114) and the unguarded one at (410,260), off a - 320x240 screen; rendering both and hashing `SDL_RenderReadPixels` gives different - images. Invisible only while the parent sits at the origin. `actor.h` documents **both** - readings, in two places. `examples/jrpg` works around it with a `renderfunc` that nulls - `obj->parent` for the duration of the draw. - -3. **Actors and characters unregister under a different key than they register.** - `akgl_actor_initialize` (`src/actor.c:46`) and `akgl_character_initialize` - (`src/character.c:39`) register under the caller's untruncated `name`, while - `akgl_heap_release_actor` (`src/heap.c:132`) and `_release_character` - (`src/heap.c:165`) clear using the object's truncated 128-byte field. Past 127 bytes - those are different keys, so releasing leaves a live registry entry pointing at a - zeroed pool slot. Distinct from "Truncated registry keys can collide" above, which - describes sprites and spritesheets, where the truncated name genuinely *is* the key. - -4. **Every actor spawned from a map is invisible on frame one.** - `akgl_actor_initialize` sets `movement_controls_face`, and the default `facefunc` - clears every facing bit and re-sets one only from a *movement* bit. An NPC has no - movement bit, so state 17 falls to 16, which maps to no sprite, and the actor is - silently skipped by the draw. The same thing makes a player character vanish the - moment they stop walking. Both tutorials clear the field on every live actor after - loading a map; that workaround should not be necessary. - -5. **`use_own_physics` is set and never read.** `akgl_tilemap_load_physics` builds a - complete `akgl_PhysicsBackend` on the map when it declares a `physics.model` property, - and sets `map->use_own_physics` — and nothing in the library ever consults it. A map's - declared physics is silently ignored unless the application writes the switch itself. - `README.md` used to show that `if`, which made a caller-side workaround read like a - library feature. - -6. **`akgl_TilemapLayer` does not record a layer's name.** `akgl_tilemap_load_layers` - reads `id`, `opacity`, `visible`, `x`, `y` and `type` and drops the `name` Tiled - wrote; the only `name` retained anywhere is per-object. A game therefore cannot ask - for "the terrain layer" and must hard-code a numeric layer id, which changes whenever - somebody reorders layers in the editor. Both tutorials hard-code one. - -7. **`akgl_character_sprite_add` leaks a reference when the same sprite is re-added.** - `ref->refcount += 1` is unconditional but the matching release is guarded by - `displaced != ref`, so re-adding a sprite to a state it already occupies leaks a pool - slot per call. - -8. **`akgl_tilemap_compute_tileset_offsets` silently requires `spacing == 0` and - `margin == 0`.** It adds `spacing` to the tile pitch but sets row 0's y offset to - `spacing` rather than 0, and ignores `margin` entirely. A tileset with a gutter — which - is most published tileset packs — renders misaligned with no diagnostic. This - materially constrains what art the library can consume; it ruled out several otherwise - suitable CC0 packs while sourcing the tutorial assets. - -9. **`akgl_actor_render` hard-codes `SDL_FLIP_NONE`** (`src/actor.c:283`). There is no - mirrored blit, so a side-on character needs both facings drawn in the sheet. That is - why `docs/tutorials/assets/sidescroller/player.png` carries six frames rather than - three, and it doubles the art cost of every such character. - -10. **`akgl_Actor::layer` is unbounded but `draw_world` stops at `AKGL_TILEMAP_MAX_LAYERS`.** - An actor assigned layer 16 or above is accepted, updated, simulated — and never drawn. - -11. **`akgl_registry_load_properties` leaks one string-pool slot per failed property.** - The per-property `CLEANUP` block is empty. The string pool is 256 slots, so a - sufficiently malformed properties file drains it. - -12. **`speedtime` is dead.** It is loaded from character JSON (`src/character.c:263`), - written through `(int *)&obj->speedtime` on a `uint64_t` field — correct only because - the struct was zeroed and the host is little-endian — and then read by nothing. Frame - timing comes from `sprite->speed`. - -13. **`AKGL_SPRITE_MAX_REGISTRY_SIZE` is dead.** Defined in `sprite.h`, referenced - nowhere in `src/`, `include/`, `tests/` or `util/`. - -14. **`AKGL_TILEMAP_MAX_TILES_PER_IMAGE` is checked nowhere**, and costs 512 KiB per - tileset regardless of the image's real tile count. - -15. **There is no sound-effect API.** `akgl_audio_*` is a synthesizer that reads no files; - `akgl_load_start_bgm` is the only file-audio entry point, and its infinite-loop - request is set on property set 0, so background music plays once. A game cannot load - and play a sound effect through this library at all. - -16. **A literal 512x512 tilemap is rejected.** Both bounds are `>=`, so the documented - `AKGL_TILEMAP_MAX_WIDTH`/`_HEIGHT` of 512 is off by one and 512x511 is the largest - map that loads. - -17. **Unverified asset provenance.** `tests/assets/World_A1.png` and - `util/assets/Actor1.png` carry the default filenames of RPG Maker's bundled art and - ship with no license file, while `tests/assets/akgl_test_mono.ttf` sits beside - `akgl_test_mono.LICENSE.txt`. RPG Maker's bundled assets are licensed to users of that - product; redistributing them inside a C library is not something that license covers. - The tutorial assets under `docs/tutorials/assets/` deliberately do not depend on - either file, and `tests/docs_setups/tilemap.sh` says why it stages a different image. - Closing this means replacing two fixtures and the maps that reference them. - -18. **`util/assets/littleguy.json` does not load.** The sample data for `charviewer`, the - one demo program the library ships, uses the pre-0.5.0 unprefixed state names - (`ACTOR_STATE_ALIVE`) and `velocity_x`/`velocity_y` instead of `speed_x`/`speed_y`. - The current loader accepts neither. - -19. **A control map could not say "any keyboard", so binding one was a coin flip.** - `akgl_controller_handle_event` matched `event->key.which == curmap->kbid` exactly, and - there was no way to express "whatever keyboard the player types on". That is not a - theoretical gap, because the id a key event *carries* is chosen by the video backend - and does not have to be an id `SDL_GetKeyboards()` reports: on X11 without XInput2 - every key event carries `SDL_GLOBAL_KEYBOARD_ID` (0) while SDL registers the keyboard - as `SDL_DEFAULT_KEYBOARD_ID` (1), and with XInput2 the events carry the physical slave - device's `sourceid` while `SDL_GetKeyboards()[0]` may be the master. A game that bound - `SDL_GetKeyboards()[0]` -- the obvious thing -- got a map that matched nothing and - controls that silently did nothing. `examples/sidescroller` shipped exactly that. - - **Fixed**: `kbid`/`jsid` of 0 now match any device of that kind. 0 is safe to spend - because SDL documents `which` as 0 when the source is unknown or virtual and joystick - ids start at 1, so no real device is 0. A non-zero id still matches only that device, - which is what keeps two local players apart. `util/charviewer.c` was already passing 0 - and depending on the old accidental behaviour, so it works under XInput2 now too. - - The reason this reached a release is worth keeping: **every test in - `tests/controller.c` dispatched an event whose id equalled the id the map was bound - with**, so none of them could see it, and the sidescroller's smoke test called the - handlers directly rather than dispatching events, so it passed against a control map - that matched nothing. Both are fixed -- `test_controller_wildcard_device_ids` asserts - the contract, and the smoke test now drives `akgl_controller_handle_event` from a - non-zero device id and fails if the press does not arrive. - -### Header comments that describe code that has changed - -Twenty-seven claims across the public headers were false when checked against `src/`. -`AGENTS.md` already warns that this file "carried eleven entries describing code that had -already changed"; this is the same failure in the headers, and Doxygen publishes it. - -Nothing catches these. `WARN_IF_UNDOCUMENTED` proves a symbol *has* a comment, not that -the comment is true, and `api_surface` strips comments precisely because prose is not a -declaration. The full list is in the manual, each noted in the chapter that covers the -subsystem, but the ones that would actively mislead a caller are: - -- `physics.h:8-9,195` — the `physics.engine` property and `akgl_game_init` calling the - factory. Neither exists. See defect 1 above for what this costs. -- `README.md` (now corrected) — "ONLY supports TilED TMJ tilemaps with tileset **external** - references". Backwards: `"source"` appears nowhere in `src/tilemap.c`, and - `akgl_tilemap_load_tilesets_each` reads `columns`/`firstgid`/`tilecount`/`image` inline. - Only **embedded** tilesets load. -- `renderer.h` — `frame_start`/`frame_end`/`draw_texture` "dereference `self` before it is - checked". Each function's first statement is `FAIL_ZERO_RETURN(errctx, self, ...)`. -- `sprite.h` — `speed` is "seconds, scaled to milliseconds". It is milliseconds scaled to - nanoseconds. Also claims `frames` is unbounded (bounded at `src/sprite.c:207`) and that - `akgl_sprite_initialize` overreads via `memcpy` (it uses `aksl_strncpy`). -- `character.h` — `speedtime` "in seconds" (milliseconds), and `sprite_add` never releasing - a displaced sprite (`src/character.c:60,77-79` releases it). -- `actor.h` — the `cmhf` block comment says the `_off` handlers zero acceleration, thrust, - environmental *and* velocity. They zero only `ax`/`tx` or `ay`/`ty`; zeroing `ey` was the - gravity-cancel defect fixed in 0.6.0, and two `@note`s still describe it. -- `json_helpers.h` — the conventions block says `dest` is not NULL-checked and that only - `akgl_get_json_string_value` checks its key. All eleven accessors check `dest`, and all - seven key-taking accessors check `key`. -- `assets.h:17-18` — "`akgl_game_init` (or a bare `akgl_audio_init`) has to have run - first". `akgl_mixer` is created only in `akgl_game_init`; `akgl_audio_init` opens the - synthesizer's stream and never touches it, so `akgl_load_start_bgm` after only that - hands NULL to `MIX_LoadAudio`. -- `registry.h:54` — `akgl_registry_init` creating seven registries and not properties. It - creates eight including properties; the genuinely false part is that `akgl_game_init` - never calls it at all, calling the eight individually in a different order. -- `tilemap.h:59-60,354-357,447-450,462-468` — object and tileset counts unbounded, and - `akgl_tilemap_release` double-freeing. All fixed; the surviving half of the last one is - that `release` does not release the map's actors. -- `controller.h:232-234,262-263` — a negative `controlmapid` not rejected. Both call sites - check. - -### Entries in this file that are themselves stale - -- **"Known and still open" item 15** — `akgl_path_relative` leaking an error-context slot - per ENOENT call — is fixed. `src/util.c:115-129` sets a flag in the `HANDLE` block and - calls `path_relative_root` after `FINISH`, which is exactly the fix the item proposes. -- **`TODO.md:2027`**, under "akgl.pc names no dependencies", says "`akglConfig.cmake` - re-finds the targets". No such file is generated or installed — `install()` ships the - library, the headers and `akgl.pc` only, so `find_package(akgl)` cannot work against an - install tree at all. That makes the section's problem worse than it states. -- **`AGENTS.md:636`** says `sim_step()` sets `gravity_time` to `now - dt`. - `tests/physics_sim.c` sets it to `0` and drives the step through `max_timestep`; the - `now - dt` form was replaced because it went red under parallel `ctest`. - -## tg is vendored and has no consumer - -`deps/tg` is pinned at v0.7.9 and **nothing in the tree 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 is - "only carried along and serve[s] no other purpose than to be available for - when it's desired to export to an output representation"; no predicate reads - it. libccd is natively 3D, so the 2D case is a restriction that lifts later. -- **Its predicates return `bool`.** Resolving a collision needs a minimum - translation vector, and `tg_geom_intersects` cannot supply one. - -Secondary: every coordinate in its API is `double` against libakgl's -`float32_t`; `tg.c` is a single 16,000-line amalgamation that embeds a second -JSON parser next to the jansson already vendored; and it ships no build system, -so `akgl_add_vendored_dependency` would silently do nothing for it. - -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. A trigger volume or walkable -region authored as a concave Tiled polygon is the case, and tg answers it at -10M+ ops/sec with its own segment index. - -**This entry is the deadline.** Either that consumer appears, or `deps/tg` comes -out. A vendored dependency with no caller is a question every future reader has -to answer, and 6 MB of it is cloned by every recursive checkout in CI. Closing -it means either writing the region-query API or `git rm` -- not leaving it here. - -## Actor rotation, and collision under it - -**No actor carries an angle.** `akgl_actor_render` hard-codes `SDL_FLIP_NONE`, -every collision shape is axis-aligned, and both example games depend on that -being true. Adding rotation is a small change in five named places and a large -change in one, and this entry exists so the large one is not a surprise. - -What it touches, in the order it would have to be done: - -1. **`akgl_Actor` grows an `angle`**, in radians, about z. Render consumes it: - `SDL_RenderTextureRotated` takes an angle in *degrees* and a centre, so - `akgl_actor_render` converts and picks a centre. The centre is a decision, - not a detail — a sprite rotating about its frame centre and one rotating - about its feet look nothing alike. -2. **`collision_support` rotates `dir` into local space** at the top and rotates - the answer back. That one function is written so this is *the whole - narrowphase change*: every arm of its switch already answers in the shape's - own frame, and the world centre is added by `collision_ccdobj` rather than - inside the switch. A `ccd_quat_t` on `collision_ccdobj`, one `ccdQuatRotVec` - in and one `ccdQuatRotVec` out, and MPR and GJK both work on rotated shapes - with nothing else touched. `collision_center` is unaffected — the centre of a - shape rotated about its own centre is the same point. -3. **The box fast path stops being valid for a rotated pair.** `collision_box_box` - is a closed form that assumes both boxes are axis-aligned; a rotated pair has - to fall through to MPR. That is a branch on `(a->angle != 0 || b->angle != 0)` - and a measurable cost — the fast path is 2-7x cheaper (`PERFORMANCE.md`, - "Collision, measured"), so a game that rotates everything pays for it. -4. **The broad-phase bound becomes the rotated bound.** `akgl_collision_shape_bounds` - returns the shape's own AABB; a rotated shape needs the AABB *of the rotated - shape*, which is larger. The function grows an angle parameter, and every - caller — `akgl_collision_settle`, the grid's `insert`/`move`, `ss_grounded` in - the sidescroller — passes one. A broad phase that keeps returning the - unrotated bound under-reports, which is the one failure mode the partitioner - contract forbids. -5. **`akgl_collision_proxy_sync` copies the angle** alongside the shape and the - position, since that is the one place the proxy's copy is refreshed. - -**The z-extrusion invariant is unaffected.** Rotation is about z, so a shape's -half-extent along z does not change and `AKGL_COLLISION_DEPTH_RATIO` still makes -z overlap exceed any achievable planar penetration. See -[Chapter 15](docs/15-collision.md), "Why a 2D shape has a depth". - -**What it does not touch:** the response. `akgl_actor_collide_block` works on a -normal and a velocity and does not care how the normal was found. Angular -velocity, torque and rotational response are a *different* piece of work and are -not implied by any of the above — a rotated shape that resolves linearly is a -perfectly coherent intermediate state, and it is the one a top-down shooter -actually wants. - -## Swept narrowphase for fast movers - -`AKGL_COLLISION_FLAG_BULLET` is defined, documented as reserved, and **does -nothing**. Sub-stepping bounds how far an actor travels between tests and it is -capped at `AKGL_COLLISION_MAX_SUBSTEPS` (8) sub-steps of at most -`AKGL_COLLISION_SUBSTEP_FRACTION` (0.5) of a cell, so the speed above which -tunnelling returns is: - -``` - v_max = substeps * fraction * cellsize / max_timestep - = 8 * 0.5 * 16 / 0.05 - = 1280 px/s on 16-pixel tiles at the default max_timestep -``` - -Check your own numbers against that: a game with 8-pixel tiles halves it, and a -game that raises `physics.max_timestep` lowers it proportionally. A walking -character is nowhere near it; a projectile is a projectile. - -The fix is a swept test rather than more sub-steps — sweeping the shape's AABB -over the sub-step's motion, testing that, and resolving at the time of first -contact rather than at the end position. It belongs behind the flag so it is -paid for only by the shapes that need it. - -## The ccd arena is single-threaded and sized by measurement - -`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`. Two things -follow that are true today and would not survive a change: - -- **It is not thread-safe.** libakgl is single-threaded behind - `akgl_game.statelock`, and two threads in the narrowphase at once would - interleave allocations in the same arena. Threading libakgl means an arena per - thread, or a lock, before anything else. -- **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, so the arena is only under pressure - from a caller who has explicitly asked for EPA's manifold. - -`akgl_ccd_arena_highwater()` is public so a game can assert its own ceiling. +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( )` + 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, on the grounds that collision now has a real narrowphase. 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. +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. +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. +- **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. + 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. -One thing found while doing this and fixed rather than recorded: -`akgl_RectanglePoints`' Doxygen comment still said `akgl_collide_rectangles` -"works corner by corner rather than by comparing edge spans", which had been -false since the fix. It went with the type. +## 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. diff --git a/TODO.txt b/TODO.txt deleted file mode 100644 index b0ee02b..0000000 --- a/TODO.txt +++ /dev/null @@ -1,27 +0,0 @@ -Rendering should move to the SDL GPU renderer so i can do lighting and particles etc - - Example suitable for my most primitive use case: https://github.com/TheSpydog/SDL_gpu_examples/blob/main/Examples/Blit2DArray.c - - Try vulkan and D3D tutorials to come up to speed on the moving pieces, then figure ou the details from the examples and API docs - - https://vulkan-tutorial.com/Drawing_a_triangle/Graphics_pipeline_basics/Introduction - - https://learn.microsoft.com/en-us/previous-versions/windows/desktop/bb153302(v=vs.85) - - http://www.directxtutorial.com/LessonList.aspx?listid=112 - - Shaders compiled with: https://github.com/libsdl-org/SDL_shadercross. There are no docs on this tool. - - Shader language is tricky. MS is abandoning HLSL and everyone unifying on Spir-V. But Spir-V is intermediate, HLSL is more high level so easier to pick up. - - https://flathub.org/apps/org.shadered.SHADERed - - Really good HLSL tutorials: https://www.youtube.com/playlist?list=PL78XDi0TS4lEMvytsE_MoWEpzBcukXv9b - - Spir-V tutorials: https://github.com/google/spirv-tutor - -Actors should collide with each other - - Box2D is going to be overkill for this for simpler platforms. We're going to benefit from a simpler approach. - - https://katyscode.wordpress.com/2013/01/18/2d-platform-games-collision-detection-for-dummies/ looks promising - -World collision geometry should be loaded from object layers on the map - -Actors should collide with the world - -Actors should be able to follow a path defined with a polyline on a layer on the map - -Actors should be able to calculate their facing state automatically depending on their movement state (for path following) - -Maps should be able to have image layers - -Map parallax should work diff --git a/docs/01-introduction.md b/docs/01-introduction.md index 728e0c4..7950fa0 100644 --- a/docs/01-introduction.md +++ b/docs/01-introduction.md @@ -68,7 +68,7 @@ library's. ## What is not implemented Named here rather than discovered later. Each gets a callout in the chapter where you would -hit it, and an entry in `TODO.md`. +hit it, and an issue in the tracker. | Gap | Behaviour today | Chapter | |---|---|---| diff --git a/docs/03-getting-started.md b/docs/03-getting-started.md index 3f1a687..c0b7894 100644 --- a/docs/03-getting-started.md +++ b/docs/03-getting-started.md @@ -117,7 +117,7 @@ pkg-config --cflags --libs akgl akerror akstdlib sdl3 sdl3-ttf sdl3-mixer jansso `sdl3-image` is needed at link time but appears in no public header, so it belongs in the private half. -Recorded in `TODO.md` under "akgl.pc names no dependencies", along with why the fix wants +Recorded as issue #17, along with why the fix wants its own commit: adding a `Requires:` line changes what `pkg-config --libs akgl` emits for every existing consumer. diff --git a/docs/04-errors.md b/docs/04-errors.md index 64d0277..7218895 100644 --- a/docs/04-errors.md +++ b/docs/04-errors.md @@ -173,7 +173,7 @@ So the context has exactly one owner at a time, and it is never both yours and i returned releases the same slot twice, and a double-released context corrupts the failure instead of reporting it. That is not hypothetical: it is how the first draft of the `AKERR_OUTOFBOUNDS` test in `tests/json_helpers.c` passed against the unfixed library. -`AGENTS.md` documents the shape under "Testing Guidelines"; `TODO.md` item 18 records the +`AGENTS.md` documents the shape under "Testing Guidelines"; `TODO.md` records the history. The correct shape: take the result into a local, `NULL` your own pointer immediately, and diff --git a/docs/05-the-heap.md b/docs/05-the-heap.md index 5db47c9..8cf3683 100644 --- a/docs/05-the-heap.md +++ b/docs/05-the-heap.md @@ -105,7 +105,7 @@ normal shape is acquire-then-initialize and never acquire-then-use. ### The refcount asymmetry This is the one thing about the heap that will surprise you, and it is a known defect -rather than a design (`TODO.md`, "Known and still open" item 8; also the `@warning` on +rather than a design (issue #14; also the `@warning` on `heap.h` itself). | Acquire | Takes the reference? | Who takes it | diff --git a/docs/06-the-registry.md b/docs/06-the-registry.md index 37758f8..ccbd473 100644 --- a/docs/06-the-registry.md +++ b/docs/06-the-registry.md @@ -35,8 +35,8 @@ same `SDL_Texture`, and the texture is loaded once. entry `i` to `1 << i`, which is what lets a character definition write `"AKGL_ACTOR_STATE_FACE_LEFT"` instead of `2`. Two entries in that name table disagree with `actor.h`: bits 11 and 12 are `UNDEFINED_11` and `UNDEFINED_12` rather than `MOVING_IN` and -`MOVING_OUT`, **so those two states cannot be named from JSON at all** (`TODO.md` items -24–26). The individual name registrations are not checked, either. +`MOVING_OUT`, **so those two states cannot be named from JSON at all** (`TODO.md`, +"`AKGL_ACTOR_STATE_STRING_NAMES`"). The individual name registrations are not checked, either. **The music registry is empty.** `akgl_registry_init_music` creates it and nothing in the library ever writes to it. It is there for you. @@ -51,8 +51,7 @@ around. Read against `src/registry.c` and `src/game.c`, this is what is true: - `akgl_registry_init` creates **eight**, including `AKGL_REGISTRY_PROPERTIES`. The - properties call was added in 0.5.0 (`TODO.md`, "Known and still open" item 3, marked - fixed). Its order is spritesheet, sprite, character, actor, actor-state-strings, font, + properties call was added in 0.5.0 (`TODO.md`, "Formatting and hygiene"). Its order is spritesheet, sprite, character, actor, actor-state-strings, font, music, properties. - **`akgl_game_init` never calls `akgl_registry_init`.** It calls the eight individual initializers itself, in a different order: actor, sprite, spritesheet, character, font, @@ -113,12 +112,11 @@ document. The fields: **Two distinct names that agree on their first 127 bytes truncate to the same key**, and the second `SDL_SetPointerProperty` silently replaces the first. The objects are different; -the registry cannot tell. That is recorded in `TODO.md`, "Truncated registry keys can -collide", and it is not being fixed because the fix is a contract change — refusing an +the registry cannot tell. That is issue #54, and it is not fixed yet because the fix is a contract change — refusing an over-long name with `AKERR_OUTOFBOUNDS` — and every one of those headers currently promises the opposite. -There is a second half that the `TODO.md` entry understates. It says the truncated name *is* +There is a second half that the issue understates. It says the truncated name *is* the registry key. That is true for sprites and spritesheets, which register under their own copied field. It is **not** true for actors and characters: diff --git a/docs/07-the-game-and-the-frame.md b/docs/07-the-game-and-the-frame.md index 3c6e13d..2e9a1f8 100644 --- a/docs/07-the-game-and-the-frame.md +++ b/docs/07-the-game-and-the-frame.md @@ -347,7 +347,7 @@ Three further things to know: - **A save containing any registered spritesheet cannot be read back.** The writer uses `AKGL_SPRITE_SHEET_MAX_FILENAME_LENGTH` (512) for that table and the reader used `AKGL_ACTOR_MAX_NAME_LENGTH` (128) for all four. That is fixed as of 0.5.0 (`TODO.md`, - "Known and still open" item 7) — and what catches it is the `require_at_eof` check, which + "Defects") — and what catches it is the `require_at_eof` check, which turns a field-width disagreement into `AKERR_IO` instead of a silent load with wrong maps. A reader whose widths disagree finds a run of zeros mid-entry, stops early, and would otherwise report success. diff --git a/docs/08-rendering.md b/docs/08-rendering.md index e88569d..8bae56b 100644 --- a/docs/08-rendering.md +++ b/docs/08-rendering.md @@ -187,7 +187,7 @@ land. The first failure aborts the frame and propagates unchanged; there is no draw-what-you-can behaviour. -> **Known defect (`TODO.md`, "Performance", item 6).** The layer loop always runs +> **Known defect (issue #26).** The layer loop always runs > all 16 iterations and rescans all 64 actor slots on each — 1024 refcount checks > per frame for a one-layer map. Invisible at 60 fps under the software renderer, > measurable on a 2 ms GPU frame. Bounding the walk by `numlayers` and building diff --git a/docs/10-spritesheets-and-sprites.md b/docs/10-spritesheets-and-sprites.md index ca82eef..209e611 100644 --- a/docs/10-spritesheets-and-sprites.md +++ b/docs/10-spritesheets-and-sprites.md @@ -295,14 +295,14 @@ game that wants one binds its own `changeframefunc`. - **Truncated names are registry keys.** `akgl_Sprite::name` is 128 bytes and `akgl_SpriteSheet::name` is 512. A longer name truncates *silently*, and two names that truncate the same collide — the second registration replaces the first with no - error. Recorded in `TODO.md`, "Truncated registry keys can collide"; the fix is a + error. Recorded as issue #54; the fix is a contract change, since the headers currently promise truncation. - **The heap acquire functions are asymmetric.** `akgl_heap_next_string` increments - `refcount`; `next_sprite` and `next_spritesheet` do not. `TODO.md` item 8. See + `refcount`; `next_sprite` and `next_spritesheet` do not. Issue #14. See [Chapter 05](05-the-heap.md). - **No test asserts a clean sprite or spritesheet load/release cycle.** The tilemap cycle is asserted over 64 iterations; the sprite, spritesheet and character ones - are not. `TODO.md`, "Targets", row 16. + are not. `TODO.md`, "Targets", row 16; issue #13. - **`akgl_spritesheet_coords_for_frame` bounds-checks nothing.** See above. ## Where to look next diff --git a/docs/11-characters.md b/docs/11-characters.md index c4f3127..0913f67 100644 --- a/docs/11-characters.md +++ b/docs/11-characters.md @@ -310,7 +310,7 @@ character out from under a live actor leaves a dangling `basechar`. pointer, while `akgl_heap_release_character` clears it under the character's own truncated 128-byte copy. For a name over 127 bytes those differ, and the registry entry survives the release — pointing at a zeroed slot. Latent for ordinary names; - related to `TODO.md`, "Truncated registry keys can collide", which covers the + related to issue #54, which covers the collision half. - **`speedtime` is written through an `int *` cast of a `uint64_t` field.** `src/character.c` passes `(int *)&obj->speedtime` to diff --git a/docs/12-actors.md b/docs/12-actors.md index 9ea717e..ecc15bc 100644 --- a/docs/12-actors.md +++ b/docs/12-actors.md @@ -331,7 +331,7 @@ backend accumulates gravity, so tapping down mid-jump stopped the character in t air. Velocity was never theirs to clear either — `simulate` recomputes `v` as `e + t` every step. Those notes predate the fix. -> **Known defect (`TODO.md`, "Arcade physics feel").** There is no friction and no +> **Known defect (issue #30).** There is no friction and no > deceleration: zeroing `tx` on release **stops the actor dead**. Correct for Zelda, > wrong for Mario. Bind your own `_off` handler that decays `tx` over time if you > want momentum. @@ -431,12 +431,12 @@ for `akgl_Actor`. `akgl_actor_initialize` writes the registry entry under the *caller's* `name` pointer while `akgl_heap_release_actor` clears it under the actor's truncated 128-byte copy. Identical for ordinary names; divergent past 127 bytes, and then - the registry keeps an entry pointing at a zeroed slot. Related to `TODO.md`, - "Truncated registry keys can collide". + the registry keeps an entry pointing at a zeroed slot. That is issue #38; + issue #54 covers the related truncation collision. - **`akgl_actor_initialize` silently replaces a same-named actor**, and the one it displaced becomes unreachable rather than being released. - **No friction on release**, and **no terminal velocity** under gravity — - `TODO.md`, "Arcade physics feel". [Chapter 14](14-physics.md) covers both. + issues #29 through #32. [Chapter 14](14-physics.md) covers both. ## Where to look next diff --git a/docs/13-tilemaps.md b/docs/13-tilemaps.md index 33afb3d..9f56523 100644 --- a/docs/13-tilemaps.md +++ b/docs/13-tilemaps.md @@ -362,7 +362,7 @@ pass per layer; see [Chapter 8](08-rendering.md). ## Known defects Documented here because this is where a reader hits them. Each is cross-referenced to -`TODO.md`. +its issue. **A failed load releases nothing.** `akgl_tilemap_load` zeroes the destination up front and then loads physics, geometry, layers and tilesets in order. If any of that fails, the @@ -375,8 +375,8 @@ leak them. **`akgl_tilemap_release` does not release the actors an object layer created.** It destroys tileset and image-layer textures and clears each pointer as it goes — that half is correct -as of 0.5.0, and a second release is safe rather than a use-after-free (`TODO.md`, "Known -and still open" item 2). The actors, and their references on characters and sprites, are +as of 0.5.0, and a second release is safe rather than a use-after-free (`TODO.md`, +"Defects"). The actors, and their references on characters and sprites, are yours to release. The header's `@warning` describing a tileset double-free is stale; the loop it describes was fixed and `tests/tilemap.c` releases the fixture three times and asserts every texture pointer is `NULL`. diff --git a/docs/14-physics.md b/docs/14-physics.md index 50c1045..b046977 100644 --- a/docs/14-physics.md +++ b/docs/14-physics.md @@ -177,7 +177,7 @@ cannot get in the way of that. The first version of that helper set `gravity_tim `now - dt` and let the real clock supply the step; a machine busy enough to deschedule the process between that store and the `SDL_GetTicksNS()` inside `simulate` produced a longer step and a red suite, exactly once, under a parallel `ctest`. **That the engine cannot be -stepped exactly is itself a finding**, recorded in `TODO.md` under "Arcade physics feel". +stepped exactly is itself a finding**, recorded as issue #31. Two more rules from that suite, both learned the hard way. `main` must call `SDL_Init` — without it SDL sets its clock epoch on first use, `SDL_GetTicksNS()` returns something @@ -332,7 +332,7 @@ this section used to carry. Found by `tests/physics_sim.c`, which runs the arcade backend the way a game does — a Mario-esque jump and fall, Zelda-style top-down walking, and a run reversed at full speed — and prints what the actor actually did. Three other defects that suite found *are* fixed in -0.6.0. These four are not. All are in `TODO.md`, "Arcade physics feel". +0.6.0. These four are not. All four are issues #29 through #32. | Gap | What you see | The workaround today | |---|---|---| diff --git a/docs/15-collision.md b/docs/15-collision.md index c30b6fb..5c7cbc4 100644 --- a/docs/15-collision.md +++ b/docs/15-collision.md @@ -225,7 +225,7 @@ akgl_collision_world_init(&world, "bsp", 16.0f, 16.0f); - **No rotation.** No actor carries an angle, `akgl_actor_render` hard-codes `SDL_FLIP_NONE`, and every shape is axis-aligned. The support functions are written so that - adding it touches one static function in the narrowphase; see `TODO.md`. + adding it touches one static function in the narrowphase; see issue #62. - **No restitution and no friction.** The default response blocks. Anything bouncier is your `collidefunc`. - **No continuous collision.** Sub-stepping bounds how far an actor travels between tests, and diff --git a/docs/16-input.md b/docs/16-input.md index b1b83a9..8bfa3b9 100644 --- a/docs/16-input.md +++ b/docs/16-input.md @@ -353,7 +353,7 @@ re-derives them from the header prose: `akgl_controller_default` check `controlmapid < 0` as well as the upper bound and raise `AKERR_OUTOFBOUNDS`. `tests/controller.c` passes `-1` and `-4096` to each. The `@warning` blocks in `include/akgl/controller.h` saying only the upper bound is checked - are stale; `TODO.md`, "Known and still open" item 11. + are stale; `TODO.md`, "Defects", records the fix. - **The `akgl_controller_handle_*` functions exist.** `controller.h` once declared four names that were defined under different spellings, so they linked nowhere. Fixed in 0.5.0, and `scripts/check_api_surface.sh` runs as the `api_surface` test to stop that diff --git a/docs/17-text-and-fonts.md b/docs/17-text-and-fonts.md index 5022c50..2eeeeee 100644 --- a/docs/17-text-and-fonts.md +++ b/docs/17-text-and-fonts.md @@ -127,8 +127,8 @@ Measuring is 340 times cheaper than drawing, which tells you the whole cost is t and the upload. **Six HUD readouts is 76 µs a frame. That is fine at 60 fps on a laptop and it is 4% of a 2 ms GPU frame** — and it is being paid every frame for a score that changes once a second. A one-line cache keyed on (font, string, colour) would take it to nothing; -`PERFORMANCE.md` calls it the single clearest optimisation in the library, and `TODO.md` -carries it as a target. +`PERFORMANCE.md` calls it the single clearest optimisation in the library, and it is +issue #22. So: **fine for a HUD line, wrong for a static body of text redrawn every frame.** If you are drawing a page of dialogue that does not change, rasterize it yourself once with SDL3_ttf, @@ -209,7 +209,7 @@ guards, so drawing nothing still refuses everything drawing something refuses. > refused, and `text.h:120-122` still warns that a failure after rasterizing leaks the > surface and the texture. Both describe pre-0.5.0 behaviour. `src/text.c:110-112` returns > success for `""`, and `src/text.c:140-146` destroys both objects in a `CLEANUP` block that -> runs on every path. `TODO.md` items 27 and 23 record both as fixed. The header comments +> runs on every path. `TODO.md` records both as fixed. The header comments > want correcting in their own commit. ## Fonts, the pools, and what is not shared diff --git a/docs/18-audio.md b/docs/18-audio.md index 7388254..61eebf6 100644 --- a/docs/18-audio.md +++ b/docs/18-audio.md @@ -302,7 +302,7 @@ touches `akgl_mixer`, so calling only that leaves `akgl_load_start_bgm` handing `src/assets.c:43` sets `MIX_PROP_PLAY_LOOPS_NUMBER` on it, and `src/assets.c:45` 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, and `akgl_load_start_bgm` reports success either way. `TODO.md` item 19; the fix +and stops, and `akgl_load_start_bgm` reports success either way. Issue #16; the fix is `SDL_CreateProperties()` into `bgmprops`, checked, and destroyed in `CLEANUP`. **`AKGL_REGISTRY_MUSIC` exists and nothing populates it.** `akgl_registry_init_music()` diff --git a/docs/19-utilities.md b/docs/19-utilities.md index a75df9e..4d4216e 100644 --- a/docs/19-utilities.md +++ b/docs/19-utilities.md @@ -161,8 +161,8 @@ Three things to know: > **Stale defect note.** `util.h:124-128` warns that the fallback path "returns straight out > of the ENOENT handler and so never releases the error context it was handling", costing one -> of libakerror's 128 context slots per call for the life of the process, and `TODO.md` item -> 15 records the same thing as open. **Both are out of date.** `src/util.c:115-129` sets a +> of libakerror's 128 context slots per call for the life of the process, and `TODO.md` +> recorded the same thing as open. **Both were out of date.** `src/util.c:115-129` sets a > flag in the `HANDLE(errctx, ENOENT)` block and calls `path_relative_root` *after* `FINISH`, > which is exactly the fix that entry proposes, and the code carries a comment explaining > why. The leak is gone; the two notes describing it are not. @@ -332,7 +332,7 @@ for an object member. > `HANDLE_GROUP(e, AKERR_OUTOFBOUNDS)` arm, placed *above* the arm holding the `memcpy` > because `HANDLE_GROUP` emits no `break` and every arm falls into that body, and > `tests/json_helpers.c` covers it through both the integer and object index accessors. -> `TODO.md` item 18 records it as fixed. The header comment wants correcting in its own +> `TODO.md` records it as fixed. The header comment wants correcting in its own > commit. `defsize` is trusted, not derived: it is a `memcpy` through `void *` with no type diff --git a/docs/20-tutorial-sidescroller.md b/docs/20-tutorial-sidescroller.md index 889864f..bfefe92 100644 --- a/docs/20-tutorial-sidescroller.md +++ b/docs/20-tutorial-sidescroller.md @@ -412,7 +412,7 @@ window: ``` Without this line the first frame calls through a null function pointer. Making the library -pick a default is tracked in `TODO.md` under "Known and still open"; until it does, this call +pick a default is issue #36; until it does, this call belongs in every libakgl program. ### The frame loop @@ -524,7 +524,7 @@ Assign it right after `akgl_game_init()`: ``` The hook exists so a game can shed work when it is running slowly. This one has nothing to -shed. The first-second false positive is recorded in `TODO.md`. +shed. The first-second false positive is recorded in `TODO.md`, "Performance". --- @@ -763,8 +763,7 @@ Add three custom properties to the **map itself** (Map → Map Properties): | `physics.drag.y` | float | `1.5` | Air resistance on the vertical axis | Gravity of 900 px/s² with a drag of 1.5 gives a terminal fall speed of 600 px/s. Drag is what -bounds a fall, so do not set it to zero. (A `terminal_velocity` setting is in `TODO.md` under -"Arcade physics feel".) +bounds a fall, so do not set it to zero. (A `terminal_velocity` setting is issue #29.) Putting physics in the map rather than in code is what lets a swimming level and a walking level differ by data. @@ -816,7 +815,7 @@ Then the map: **Load into `akgl_gamemap`, which already points at storage the library owns.** Do not declare an `akgl_Tilemap` on the stack: it is about 26 MB, several times a default thread -stack, and you get a segfault before the loader writes a byte. Shrinking it is `TODO.md` +stack, and you get a segfault before the loader writes a byte. Shrinking it is issue #28 targets 14 and 15. That one call creates an actor for every `actor` object in the object layer, binds each to @@ -1200,7 +1199,7 @@ Snap to zero below a pixel per second, because an exponential decay never actual The library's own `akgl_actor_cmhf_left_off` zeroes `tx` outright, which stops the actor dead in one frame — right for a top-down game, wrong for a sidescroller. Friction and deceleration -in the backend are tracked in `TODO.md` under "Arcade physics feel"; when they land, this +in the backend are issues #29 through #32; when they land, this whole section becomes a setting. ### The whole movement function, in order @@ -1569,7 +1568,7 @@ else. underneath the registry that still points at them. Do not call `akgl_tilemap_release` unless you are loading a second level — it has a -double-free, recorded in `TODO.md` under "Known and still open" item 2. A process that is +double-free, recorded in `TODO.md` under "Defects". A process that is exiting can leave the textures to `SDL_Quit`. ### Reporting a failure from `main` diff --git a/docs/21-tutorial-jrpg.md b/docs/21-tutorial-jrpg.md index a67d1a8..d75b0c4 100644 --- a/docs/21-tutorial-jrpg.md +++ b/docs/21-tutorial-jrpg.md @@ -536,7 +536,7 @@ state is skipped rather than reported, so every NPC in the town disappears on fr silently, and so does the player as soon as they stop walking. Clearing the field leaves the facing bits wherever they were last set, which is what a -top-down game wants. `TODO.md` tracks making the default behave; until then this loop belongs +top-down game wants. Issue #39 tracks making the default behave; until then this loop belongs in every game with a standing NPC in it. The loop covers the player as well as the NPCs, which is what you want — a player who stops @@ -748,7 +748,7 @@ The physics step writes a child's `x` and `y` as an absolute world position, and `akgl_actor_render` adds the parent's position to it a second time. Detaching takes the branch that does not add it, and `CLEANUP` puts the parent back on every path including the failing one — an actor left with a `NULL` parent would be simulated as a free agent on the -next step. This is `TODO.md`, "Known and still open"; when the two agree on one reading, this +next step. This is issue #37; when the two agree on one reading, this hook becomes `akgl_actor_render` again. Run now. A second character walks a fixed distance behind and below the player. @@ -892,7 +892,7 @@ Guard against the empty string: ``` `akgl_text_rendertextat` refuses a zero-width string while `akgl_text_measure` accepts one. -The disagreement is recorded in `TODO.md` under "Known and still open"; the check above is +The disagreement is issue #37; the check above is the guard until it is settled. ### Drawing it over the world @@ -1132,8 +1132,8 @@ than the `NULL` chapter 20 passes. `AKGL_ITERATOR_OP_UPDATE` asks for the update registry and `SDL_Quit` destroys it, taking the last reference to every font with no way left to close them. -`akgl_tilemap_release` is not called. It double-frees tileset textures, which is `TODO.md` -"Known and still open" item 2; `SDL_Quit` reclaims them correctly. A game that loads a second +`akgl_tilemap_release` is not called. It double-freed tileset textures, which is `TODO.md` +"Defects"; `SDL_Quit` reclaims them correctly. A game that loads a second level has to unwind properly, and that is what the fix will make possible. The pools are static storage and the process is exiting, so there is nothing else to free. diff --git a/docs/23-appendix-limits.md b/docs/23-appendix-limits.md index d0fbb03..b91d2ef 100644 --- a/docs/23-appendix-limits.md +++ b/docs/23-appendix-limits.md @@ -301,7 +301,8 @@ A ninth child is `AKERR_OUTOFBOUNDS`; an over-long name is silently truncated. `akgl_sprite_load_json` bounds the `frames` array against `AKGL_SPRITE_MAX_FRAMES` before writing anything, and refuses a frame number that will not fit a `uint8_t` rather than -truncating it into an index naming a different tile (`TODO.md` item 16). +truncating it into an index naming a different tile (`TODO.md`, "Found while rewriting +the Doxygen comments"). **`AKGL_SPRITE_MAX_REGISTRY_SIZE` is dead.** It is defined in `sprite.h` and referenced nowhere in `src/`, `include/`, `tests/` or `util/`. It bounds nothing. Do not size anything @@ -335,8 +336,8 @@ Three corrections to what those comments say, all verified against `src/tilemap. - **`AKGL_TILEMAP_MAX_OBJECTS_PER_LAYER` *is* enforced.** The comment above says it is not. `akgl_tilemap_load_layer_objects` bounds `j` at the top of the loop body and raises `AKERR_OUTOFBOUNDS`; `akgl_tilemap_load_tilesets` does the same for - `AKGL_TILEMAP_MAX_TILESETS`. Both landed in 0.5.0 (`TODO.md`, "Known and still open" - item 17), and the header comment was not updated with them. + `AKGL_TILEMAP_MAX_TILESETS`. Both landed in 0.5.0 (`TODO.md`, "Found while rewriting + the Doxygen comments"), and the header comment was not updated with them -- issue #63. - **Width and height are not bounded individually.** `akgl_tilemap_load` checks `width * height >= AKGL_TILEMAP_MAX_WIDTH * AKGL_TILEMAP_MAX_HEIGHT`, and the comparison is `>=`, so the true ceiling is **262143 tiles** in any shape. A 1024×256 map loads; a diff --git a/docs/README.md b/docs/README.md index a116205..cf978d2 100644 --- a/docs/README.md +++ b/docs/README.md @@ -58,7 +58,7 @@ that is not C in any dialect, and, in the first snippet a reader ever saw, the e chapters turned up **twenty-seven** header claims that were false against `src/`. Where a chapter documents behaviour that is a known defect rather than a design decision, -it says so and points at `TODO.md`. See `MAINTENANCE.md` if you are editing an example. +it says so and points at its issue. See `MAINTENANCE.md` if you are editing an example. ## Assets diff --git a/examples/jrpg/jrpg.c b/examples/jrpg/jrpg.c index 96154dd..4023f24 100644 --- a/examples/jrpg/jrpg.c +++ b/examples/jrpg/jrpg.c @@ -347,8 +347,7 @@ static akerr_ErrorContext *frame(long frameno) * - akgl_tilemap_release is **not** called. Its layer loop destroys * `tilesets[i].texture` rather than `layers[i].texture`, so it double-frees * every tileset texture and never frees an image layer's, and it NULLs - * nothing, so a second call is a use-after-free. TODO.md, "Known and still - * open" item 2. `SDL_Quit` reclaims the textures correctly; calling the + * nothing, so a second call is a use-after-free. TODO.md, "Defects". `SDL_Quit` reclaims the textures correctly; calling the * function that is supposed to would be worse than not. * - The pools are static storage. There is nothing to free and the process is * about to exit. diff --git a/examples/jrpg/textbox.c b/examples/jrpg/textbox.c index edac8de..b3fb6e3 100644 --- a/examples/jrpg/textbox.c +++ b/examples/jrpg/textbox.c @@ -83,8 +83,8 @@ akerr_ErrorContext *jrpg_textbox_draw(void) // akgl_text_rendertextat refuses the empty string -- SDL_ttf reports "Text // has zero width" and the library passes that on as AKERR_NULLPOINTER, // while akgl_text_measure accepts it. The two disagree, so anything drawing - // a line that might be empty checks for it. TODO.md, "Known and still - // open". + // a line that might be empty checks for it. TODO.md, "Found while + // rewriting the Doxygen comments". if ( textbox_text[0] == '\0' ) { SUCCEED_RETURN(errctx); } diff --git a/examples/sidescroller/player.c b/examples/sidescroller/player.c index b9caa98..582c248 100644 --- a/examples/sidescroller/player.c +++ b/examples/sidescroller/player.c @@ -10,7 +10,7 @@ * `movementlogicfunc`, which is the only hook the step calls. * * Two of the library's documented gaps are worked around here rather than - * papered over. Both are in `TODO.md` under "Arcade physics feel". + * papered over. Both are issues #29 through #32. */ #include @@ -324,7 +324,7 @@ akerr_ErrorContext *ss_player_controls(int controlmapid, char *actorname) FAIL_ZERO_RETURN(errctx, actorname, AKERR_NULLPOINTER, "actorname"); /* akgl_controller_pushmap checks the upper bound and not the lower one, so * a negative id indexes before the start of akgl_controlmaps. TODO.md, - * "Known and still open" item 11. */ + * "Defects". */ FAIL_NONZERO_RETURN( errctx, ((controlmapid < 0) || (controlmapid >= AKGL_MAX_CONTROL_MAPS)), diff --git a/include/akgl/collision.h b/include/akgl/collision.h index 0dc9621..e5a225d 100644 --- a/include/akgl/collision.h +++ b/include/akgl/collision.h @@ -74,7 +74,7 @@ struct akgl_CollisionWorld; * * Sub-stepping bounds how far an actor moves between collision tests, which * stops anything moving at ordinary speeds from passing through a wall. A - * projectile is not moving at ordinary speeds. See `TODO.md`. + * projectile is not moving at ordinary speeds. See issue #56. */ #define AKGL_COLLISION_FLAG_BULLET 0x00000008u @@ -557,8 +557,8 @@ akerr_ErrorContext AKERR_NOIGNORE *akgl_collision_sync_actors(akgl_CollisionWorl * Follows the convention every pool here uses: akgl_heap_next_collision_proxy * finds a free slot and does **not** claim it, and this takes the reference. * Write the two adjacent, because until the reference is taken the slot is still - * free and the next acquire hands out the same pointer. That is `TODO.md` "Known - * and still open" item 8, it applies to four of the five existing pools, and + * free and the next acquire hands out the same pointer. That asymmetry is issue + * #14, it applies to four of the five existing pools, and * this one does not depart from it -- a sixth convention would be worse than the * defect. * diff --git a/include/akgl/controller.h b/include/akgl/controller.h index 31ed9f5..113d8c3 100644 --- a/include/akgl/controller.h +++ b/include/akgl/controller.h @@ -231,7 +231,7 @@ akerr_ErrorContext AKERR_NOIGNORE *akgl_controller_handle_removed(void *appstate * * @warning A **negative** @p controlmapid is not rejected -- only the upper * bound is checked -- and indexes before the start of - * ::akgl_controlmaps. TODO.md, "Known and still open" item 11. + * ::akgl_controlmaps. TODO.md, "Defects". */ akerr_ErrorContext AKERR_NOIGNORE *akgl_controller_pushmap(int controlmapid, akgl_Control *control); diff --git a/include/akgl/error.h b/include/akgl/error.h index 7fa0bbf..1ddc7f0 100644 --- a/include/akgl/error.h +++ b/include/akgl/error.h @@ -51,7 +51,7 @@ // AKERR_OUTOFBOUNDS on truncation instead. That is the better answer: the // compiler was right both times, and silencing it left the truncation // happening and unreported. Kept because they are public and a consumer may -// have them; see TODO.md. +// have them; see issue #20. #define DISABLE_GCC_WARNING_FORMAT_TRUNCATION \ _Pragma("GCC diagnostic push") \ _Pragma("GCC diagnostic ignored \"-Wformat-truncation\"") diff --git a/include/akgl/game.h b/include/akgl/game.h index a0dbb58..1512657 100644 --- a/include/akgl/game.h +++ b/include/akgl/game.h @@ -217,7 +217,7 @@ void akgl_game_update_fps(void); * * @note This is a partial implementation: the name tables are written but the * objects themselves are not, so the file is not yet enough to restore a - * session. See also TODO.md, "Known and still open" item 7 -- the writer + * session. See also TODO.md, "Defects" -- the writer * and the reader disagree on the name-field widths, so a save with any * registered spritesheet cannot be read back. */ diff --git a/include/akgl/heap.h b/include/akgl/heap.h index 2f83152..b91632e 100644 --- a/include/akgl/heap.h +++ b/include/akgl/heap.h @@ -22,8 +22,8 @@ * reference for you, and the other four do not -- their caller is * expected to take it, which in practice the `*_initialize` function * does. Until one is taken the slot is still free and the next - * allocation hands out the same pointer. TODO.md, "Known and still - * open" item 8. + * allocation hands out the same pointer. Whether to make them symmetric + * or document the split is issue #14. */ #ifndef _AKGL_HEAP_H_ diff --git a/include/akgl/registry.h b/include/akgl/registry.h index c81ae92..aad86c6 100644 --- a/include/akgl/registry.h +++ b/include/akgl/registry.h @@ -23,7 +23,7 @@ * no-op akgl_set_property and an akgl_get_property that always hands * back the caller's default, which in turn means akgl_render_2d_init * and akgl_physics_init_arcade quietly ignore their configuration. - * TODO.md, "Known and still open" item 3. + * TODO.md, "Formatting and hygiene". */ #ifndef _AKGL_REGISTRY_H_ @@ -170,7 +170,7 @@ akerr_ErrorContext AKERR_NOIGNORE *akgl_registry_load_properties(char *fname); * @note The name table it reads disagrees with actor.h in two places: bits 11 * and 12 are named `UNDEFINED_11`/`UNDEFINED_12` rather than `MOVING_IN` * and `MOVING_OUT`, so those two states cannot be named from JSON at all. - * TODO.md items 24-26. + * TODO.md, "AKGL_ACTOR_STATE_STRING_NAMES". */ akerr_ErrorContext AKERR_NOIGNORE *akgl_registry_init_actor_state_strings(void); /** diff --git a/include/akgl/text.h b/include/akgl/text.h index a7b3e87..3df9d0d 100644 --- a/include/akgl/text.h +++ b/include/akgl/text.h @@ -123,8 +123,9 @@ akerr_ErrorContext AKERR_NOIGNORE *akgl_text_unloadallfonts(void); * @note The empty string is **refused**, not drawn as nothing: SDL_ttf reports * "Text has zero width" and this passes that on as `AKERR_NULLPOINTER`. * akgl_text_measure() accepts it, so the two disagree. A caller drawing a - * line of text that may be empty has to check for it. TODO.md, "Known and - * still open". + * line of text that may be empty has to check for it. Stale: the two agree + * as of 0.5.0 -- an empty string returns success without rasterizing. See + * TODO.md, "Found while rewriting the Doxygen comments", and issue #63. */ akerr_ErrorContext AKERR_NOIGNORE *akgl_text_rendertextat(TTF_Font *font, char *text, SDL_Color color, int wraplength, int x, int y); /** diff --git a/include/akgl/tilemap.h b/include/akgl/tilemap.h index c12c105..e3bfb6b 100644 --- a/include/akgl/tilemap.h +++ b/include/akgl/tilemap.h @@ -484,7 +484,7 @@ akerr_ErrorContext AKERR_NOIGNORE *akgl_tilemap_load_tilesets(akgl_Tilemap *dest * than `layers[i].texture`, so tileset textures are destroyed twice and * image-layer textures never. Nothing is set to `NULL` either, so a * second call is a use-after-free. It also does not release the actors - * the map's object layers created. TODO.md, "Known and still open" + * the map's object layers created. TODO.md, "Defects" * item 2. */ akerr_ErrorContext AKERR_NOIGNORE *akgl_tilemap_release(akgl_Tilemap *dest); diff --git a/src/collision.c b/src/collision.c index 26f88d2..f57899b 100644 --- a/src/collision.c +++ b/src/collision.c @@ -93,7 +93,7 @@ typedef struct { * There is no rotation, here or anywhere in this library yet -- `util.h` says so * and both example games depend on it. Adding it means rotating @p dir into the * shape's local frame at the top of this function and rotating the answer back, - * and nothing else in the narrowphase changes. See `TODO.md`, "Actor rotation". + * and nothing else in the narrowphase changes. See issue #62. */ static void collision_support(const void *obj, const ccd_vec3_t *dir, ccd_vec3_t *dest) { diff --git a/src/game.c b/src/game.c index 590830b..d7d9abb 100644 --- a/src/game.c +++ b/src/game.c @@ -411,7 +411,7 @@ static void save_spritename_iterator(void *userdata, SDL_PropertiesID props, con * save_actorname_iterator. * @note This is the table the loader disagrees with: it reads every table at * #AKGL_ACTOR_MAX_NAME_LENGTH, so a save containing any spritesheet cannot - * be read back. TODO.md, "Known and still open" item 7. + * be read back. TODO.md, "Defects". */ static void save_spritesheetname_iterator(void *userdata, SDL_PropertiesID props, const char *name) { diff --git a/tests/json_helpers.c b/tests/json_helpers.c index f34b755..37ff46a 100644 --- a/tests/json_helpers.c +++ b/tests/json_helpers.c @@ -66,7 +66,7 @@ static akerr_ErrorContext *load_fixture(void) * usually because something is not releasing -- arrived as a segfault inside * strncpy. * - * This is what TODO.md Performance item 29 looked like from the outside: not + * This is what the tilemap string-pool leak looked like from the outside: not * "the tilemap loader leaks five strings a load", but a crash somewhere else * entirely, fifty levels later. */ diff --git a/tests/perf_render.c b/tests/perf_render.c index 1915b68..397f63d 100644 --- a/tests/perf_render.c +++ b/tests/perf_render.c @@ -483,7 +483,7 @@ static akerr_ErrorContext *bench_asset_load(void) // The 52nd load would find the pool empty, and what happens then is // not an AKGL_ERR_HEAP: akgl_get_json_string_value finishes with // pass-up false, swallows the failed claim, and dereferences the - // pointer it never set. Both defects are filed in TODO.md; this loop + // pointer it never set. Both defects are recorded in TODO.md; this loop // works around the first so it never reaches the second. for ( j = 0; j < AKGL_MAX_HEAP_STRING; j++ ) { akgl_heap_strings[j].refcount = 0; diff --git a/tests/physics_sim.c b/tests/physics_sim.c index 1d59004..191e802 100644 --- a/tests/physics_sim.c +++ b/tests/physics_sim.c @@ -31,7 +31,7 @@ * inside simulate produced a longer step and a red suite. It failed * exactly once, under a parallel ctest, which is the worst way to find * out. That the engine cannot be stepped exactly is itself a finding; - * see TODO.md. + * see issues #29 through #32. */ #include diff --git a/tests/renderer.c b/tests/renderer.c index 6d2ad1b..d39077a 100644 --- a/tests/renderer.c +++ b/tests/renderer.c @@ -10,7 +10,7 @@ * * akgl_render_2d_init() is not covered here: it creates a window from the * property registry and writes the `camera` global, which is what the offscreen - * harness described in TODO.md exists to make testable. + * harness described in issue #6 exists to make testable. */ #include -- 2.43.0 From 963d5112241fc4e688abdfd498842005e6b1ae11 Mon Sep 17 00:00:00 2001 From: Tachikoma Date: Sun, 2 Aug 2026 19:24:20 -0400 Subject: [PATCH 2/2] Point AGENTS.md at the issue tracker for outstanding work The rules section now says where new work goes: an issue on the forge, with the file and line, the functional consequence, and what closing it would touch, carrying status::grooming until its scope is settled. TODO.md is the record of decisions and measurements, and adding outstanding items back to it is what the split exists to prevent -- a description of work still to do goes stale the moment somebody does it. Two rules go with it. Say what you learn about an open issue in the issue, because an issue nobody has re-checked is worth what an unverified comment is worth. And file a dependency's defect against that dependency rather than working around it here. The 'do not trust a comment, a TODO entry, or a CI exclusion' lesson keeps its evidence and gains the three entries found stale during the migration. Co-Authored-By: Claude Opus 5 (1M context) --- AGENTS.md | 24 +++++++++++++++++++----- 1 file changed, 19 insertions(+), 5 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 8006936..bdc8c4f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -124,7 +124,7 @@ Every suite runs in every job. The `character` suite used to be excluded from the coverage and memory-check jobs on the grounds that it failed deliberately. It did not: it was reporting success while running one of its four tests, for two independent reasons, and both are fixed. See TODO.md, "Test suites that -could not fail". +could not fail", which is the record of how that happened. ## Coding Style @@ -549,12 +549,13 @@ cheap to check and expensive to assume. a bit that is **not** set whose value is **not** 1. The obvious test passes either way. -- **Do not trust a comment, a TODO entry, or a CI exclusion that states a +- **Do not trust a comment, an issue, or a CI exclusion that states a premise.** Verify it. `tests/character.c` was excluded from two CI jobs and from the mutation harness because it "fails deliberately". It did not: it was - exiting 0 while running one of its four tests. `TODO.md` carried eleven - entries describing code that had already changed. A premise nobody has - re-checked is where the next defect is hiding. + exiting 0 while running one of its four tests. `TODO.md`, when it still held + outstanding work, carried eleven entries describing code that had already + changed, and three more were found stale when it was migrated to the tracker. + A premise nobody has re-checked is where the next defect is hiding. - **When a measurement moves, say what you measured.** Absolute benchmark numbers drift with the machine. The `akgl_game_update` fix is recorded as the @@ -564,6 +565,19 @@ cheap to check and expensive to assume. ## Rules +- **Outstanding work goes in the issue tracker, not in a file.** Open an issue at + — `tea issues create --repo andrew/libakgl` + — with the file and line it touches, the functional consequence, and what closing it would + touch. Label it by kind and blast radius, and leave `status::grooming` on it until its scope + and approach are settled. **Do not add outstanding items to `TODO.md`**; that file is the + record of decisions, measurements and defects already closed, and the reason for the split is + that a description of work still to do goes stale the moment somebody does it. +- **When you learn something about an open issue, say so in the issue** — including that it is + worse than recorded, that it is already fixed, or that its premise has expired. An issue + nobody has re-checked is worth what an unverified comment is worth. +- **A defect in a dependency is filed against that dependency**, not worked around here. + `libakerror`, `libakstdlib` and `akbasic` all have trackers on the same forge. Comment the + workaround at its site with the words "filed upstream" and delete it when the fix lands. - Add yourself (agent program name, model name and version) as a co-author on every commit message - Avoid dynamic memory allocation. Use the `akgl_heap_*` methods to retrieve necessary objects at runtime, and to release them when done. If the akgl heap facilities don't provide the kind of object needed, create a new heap layer to support that type of object. -- 2.43.0