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