Andrew Kesterson
3a262bee54
Closes internal-consistency items 7 through 15. Nineteen non-static functions were in the ABI with no declaration anywhere, so no consumer could call them and any consumer could collide with them. The four gamepad_handle_* functions are the ones that mattered: controller.h declared akgl_controller_handle_button_down and three siblings that did not exist, so anything compiled against the header alone failed to link. The definitions carry the declared names now, which also closes Defects -> Known and still open item 10, and their documentation moved to the header. The rest are either declared under a "part of the internal API" block -- akgl_game_save_actors and akgl_game_load_versioncmp, which tests/game.c had to declare for itself, plus six tilemap loader helpers the untested-loader work wants to reach -- or static, which is what the four save iterators and load_objectnamemap should always have been. akgl_path_relative_from is deleted: declared nowhere, called from nowhere, never wrote its output, and leaked a pooled string on every call, so it closes Known and still open item 4 and item 40 by ceasing to exist. scripts/check_api_surface.sh keeps it closed. It reads the built library's dynamic symbol table and every public header with comments stripped, and fails on an exported akgl_* symbol that is declared nowhere. Stripping comments is the whole point -- four of these were mentioned in controller.h prose, which is how they went unnoticed. The pool-size ceilings are defined once, in heap.h, so the #ifndef override hook fires for the first time; actor.h, sprite.h and character.h were defining the same four unconditionally from headers heap.h includes above its own guard. tests/header_pool_override.c fails the compile if that regresses. Also here: (void) rather than () on the twelve no-argument entry points, AKERR_NOIGNORE only on declarations, static helpers with the akgl_ prefix dropped, and the six parameter-name mismatches. akgl_get_json_with_default had its two contexts swapped rather than merely misspelled -- the incoming one was `err` and its own was `e`, which is the name reserved for an incoming one. 24/24 pass, reindent --check clean. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
1838 lines
111 KiB
Markdown
1838 lines
111 KiB
Markdown
# TODO
|
|
|
|
## Internal consistency
|
|
|
|
Findings from a sweep of `src/` and `include/`. These are consistency and
|
|
convention problems, not new functional defects — where one has a functional
|
|
consequence it is called out. Ordered roughly by blast radius. Items that
|
|
overlap the existing **Defects** list are cross-referenced rather than repeated.
|
|
|
|
### 1. Public naming conventions
|
|
|
|
**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:
|
|
|
|
| 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>`.
|
|
|
|
`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
|
|
`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
|
|
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.
|
|
|
|
**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
|
|
`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
|
|
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 `<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 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 <akgl/sibling.h>`, and `staticstring.h`'s `#include "string.h"` --
|
|
a relative-first lookup that reached the system header by accident -- is
|
|
`#include <string.h>`. `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`.**
|
|
The established pattern is `FAIL_*_BREAK` / `CATCH` inside `ATTEMPT` and
|
|
`*_RETURN` outside it. Violations:
|
|
- `src/tilemap.c:52` — `SUCCEED_RETURN` on the success path of
|
|
`akgl_get_json_tilemap_property` returns directly from inside `ATTEMPT`,
|
|
bypassing the `CLEANUP` at line 54 that releases `tmpstr` and `typestr`.
|
|
Every successful property lookup leaks two heap strings, and the string
|
|
pool is only 256 entries.
|
|
- `src/tilemap.c:620` — `FAIL_RETURN` inside `ATTEMPT`.
|
|
- `src/controller.c:286-289,306` — `FAIL_ZERO_RETURN` / `FAIL_NONZERO_RETURN`
|
|
inside `ATTEMPT`; `src/controller.c:383` — `SUCCEED_RETURN` inside `ATTEMPT`,
|
|
which also leaves `akgl_controller_default` with no return statement on the
|
|
path that falls out of `FINISH`.
|
|
- `src/game.c:457,462` — `FAIL_NONZERO_RETURN` inside `ATTEMPT`, skipping the
|
|
`fclose` in `CLEANUP` at line 482.
|
|
|
|
17. **NULL-check discipline varies by function.** In `src/json_helpers.c` only
|
|
`akgl_get_json_string_value` (line 71) and `akgl_get_json_array_index_string`
|
|
(line 128) validate `key`/`dest`; the other eight accessors validate only the
|
|
container and then dereference `dest` unconditionally. In `src/physics.c` the
|
|
arcade backends check `actor` but `akgl_physics_null_gravity`,
|
|
`_null_collide`, and `_null_move` (lines 15-34) check only `self`. In
|
|
`src/renderer.c:65,74`, `akgl_render_2d_frame_start` and `_frame_end`
|
|
dereference `self->sdl_renderer` with no check on `self`, while
|
|
`akgl_render_2d_draw_texture` (line 82) checks `self` first.
|
|
|
|
18. **Error-context variable naming is split between `errctx` and `e`,
|
|
sometimes within one file.** `src/util.c` uses `e` in the path helpers
|
|
(lines 32-116) and `errctx` in the geometry helpers (lines 118-259);
|
|
`src/heap.c` uses `errctx` everywhere except `akgl_heap_init_actor` (line 50).
|
|
`src/actor.c`, `character.c`, `json_helpers.c`, `registry.c`, `sprite.c`, and
|
|
`tilemap.c` favor `errctx`; `game.c`, `physics.c`, `renderer.c`, and
|
|
`controller.c` favor `e`. Pick one.
|
|
|
|
### 4. Types and macros
|
|
|
|
19. **`float`/`double` are used raw where `types.h` defines aliases.** `types.h`
|
|
exports `float32_t` and `float64_t`, and the actor and character structs use
|
|
`float32_t` throughout — but `akgl_get_json_number_value` takes `float *`
|
|
(`json_helpers.h:55`), `akgl_get_json_double_value` takes `double *` (line 66),
|
|
`akgl_PhysicsBackend`'s six drag/gravity fields are `double`
|
|
(`physics.h:22-27`), and `akgl_Tilemap`'s perspective fields are `float`
|
|
(`tilemap.h:105-108`). Either use the aliases consistently or delete them.
|
|
|
|
20. **`AKGL_COLLIDE_RECTANGLES` (`include/akgl/util.h:27`) has unbalanced
|
|
parentheses** — three opens, two closes — so any use is a syntax error. It has
|
|
no callers and duplicates `akgl_collide_rectangles`. Delete it.
|
|
|
|
21. **Bitmask macros are unparenthesized.** `AKGL_BITMASK_HAS(x, y)` expands to
|
|
`(x & y) == y` with no outer parens, so `!AKGL_BITMASK_HAS(a, b)` parses as
|
|
`!(a & b) == b`. No in-tree caller negates it today (`AKGL_BITMASK_HASNOT`
|
|
exists for that), so this is latent rather than live. `AKGL_BITMASK_CLEAR(x)`
|
|
(`game.h:86`) also carries a trailing semicolon inside the macro body, so
|
|
normal use produces an empty statement. Parenthesize all five and drop the
|
|
semicolon.
|
|
|
|
22. **The state and iterator bit macros mix forms.** `AKGL_ITERATOR_OP_UPDATE`
|
|
(`iterator.h:15`) is written `1` while its 31 siblings are `1 << n`, and none
|
|
of the `1 << n` values in `iterator.h` or `actor.h:15-49` are parenthesized.
|
|
The hand-maintained trailing bit-pattern comments in `actor.h:34-49` are also
|
|
wrong for the high word — they restart at `0000 0000 0000 0001` for bit 16
|
|
rather than showing the full 32-bit value.
|
|
|
|
23. **`akgl_Frame` (`game.h:27-31`) is defined and never used** anywhere in
|
|
`src/`, `include/`, `tests/`, or `util/`.
|
|
|
|
### 5. `AKGL_ACTOR_STATE_STRING_NAMES` disagrees with `actor.h`
|
|
|
|
24. **The array bound differs between declaration and definition.**
|
|
`include/akgl/actor.h:57` declares `[AKGL_ACTOR_MAX_STATES+1]` (33);
|
|
`src/actor_state_string_names.c:6` defines `[32]`. Any consumer that trusts
|
|
the declared bound and reads index 32 reads past the object.
|
|
|
|
25. **Two entries name the wrong bit.** `actor.h` assigns bit 11 to
|
|
`AKGL_ACTOR_STATE_MOVING_IN` and bit 12 to `AKGL_ACTOR_STATE_MOVING_OUT`, but
|
|
`actor_state_string_names.c:18-19` puts `"AKGL_ACTOR_STATE_UNDEFINED_11"` and
|
|
`"AKGL_ACTOR_STATE_UNDEFINED_12"` at those indices — names for macros that do
|
|
not exist. Since `akgl_registry_init_actor_state_strings`
|
|
(`src/registry.c:79-94`) builds `AKGL_REGISTRY_ACTOR_STATE_STRINGS` from this
|
|
array and `akgl_character_load_json_state_int_from_strings`
|
|
(`src/character.c:101`) resolves character-JSON state names through it, a
|
|
character JSON can never bind a sprite to `MOVING_IN` or `MOVING_OUT`.
|
|
|
|
26. **The generation comment is stale.** `actor.h:53-55` says the file "is built
|
|
by a utility script and not kept in git, see the Makefile for
|
|
lib_src/actor_state_string_names.c". There is no Makefile (the project is
|
|
CMake), no `lib_src/`, no such script under `scripts/` or `util/`, and the
|
|
file *is* tracked in git at `src/actor_state_string_names.c`. Either restore
|
|
the generator — which would fix items 24 and 25 by construction — or delete
|
|
the comment and maintain the file by hand.
|
|
|
|
### 6. Doxygen drift
|
|
|
|
27. **Three struct doc comments in `tilemap.h` are rotated by one.**
|
|
`akgl_TilemapObject` (line 31) is documented as "Stores tileset metadata,
|
|
texture, and frame offsets" (that is `akgl_Tileset`); `akgl_TilemapLayer`
|
|
(line 46) as "Represents an object embedded in a tilemap layer" (that is
|
|
`akgl_TilemapObject`); `akgl_Tileset` (line 61) as "Stores tile, image, or
|
|
object data for one map layer" (that is `akgl_TilemapLayer`).
|
|
|
|
28. **`point` is documented as "Represents a two-dimensional point"**
|
|
(`util.h:12`) but has `x`, `y`, and `z`.
|
|
|
|
29. **Doc comments live on the definition for public functions.** The convention
|
|
is header-side documentation, but `akgl_path_relative_root` (`util.c:23`),
|
|
`akgl_path_relative_from` (`util.c:97`), the four `gamepad_handle_*`
|
|
(`controller.c:114+`), the `akgl_game_save_*` iterators and
|
|
`akgl_game_load_*` helpers (`game.c:173+`), and the tilemap helpers
|
|
(`tilemap.c:90+`) are documented only in the `.c`. This is the same set as
|
|
item 7 — resolving that resolves this.
|
|
|
|
### 7. Formatting
|
|
|
|
30. **A minority of files use a 2-column offset instead of the canonical
|
|
4-column one.** The mix of tabs and spaces across most of `src/` is *not*
|
|
disorder: it is exactly what Emacs `cc-mode` emits for the `stroustrup` style
|
|
with `indent-tabs-mode t` and `tab-width 8` — depth 1 is four spaces, depth 2
|
|
is one tab, depth 3 is a tab plus four spaces, and so on. Twelve of the
|
|
seventeen `.c` files follow that ladder cleanly.
|
|
|
|
The genuine outliers indent at 2 columns: `src/json_helpers.c` (82 lines,
|
|
except `akgl_get_json_with_default` at line 149 which is 4), `src/util.c`
|
|
(37 lines, from `akgl_rectangle_points` at line 118 onward), `src/assets.c`
|
|
(9), `src/staticstring.c` (7), `src/actor.c` (8, in `akgl_actor_add_child`
|
|
at line 272), plus `include/akgl/util.h` (7) and
|
|
`include/akgl/staticstring.h` (2).
|
|
|
|
**Resolved.** `AGENTS.md` now specifies the canonical style and the exact
|
|
`cc-mode` settings; `.dir-locals.el` applies them in Emacs; and
|
|
`scripts/reindent.sh` applies them in batch. The whole tree (32 files across
|
|
`src/`, `include/`, `tests/`, and `util/`) has been reindented and is a fixed
|
|
point of `scripts/reindent.sh --check`. Verified whitespace-only: apart from
|
|
three trailing blank lines removed at EOF in `src/heap.c`, `src/registry.c`,
|
|
and `tests/tilemap.c`, `git diff -w` over the reindent is empty, and the test
|
|
results are unchanged (13/14, `character` still the intentional failure).
|
|
`scripts/hooks/pre-commit` keeps it that way — enable with
|
|
`git config core.hooksPath scripts/hooks`.
|
|
|
|
31. **Leftover debug code ships in the library.** `src/controller.c:91-97` logs
|
|
four lines whenever `event->type == 768 && event->key.which == 11 &&
|
|
event->key.key == 13` — hardcoded decimal values for a specific keyboard ID
|
|
on a specific developer's machine, inside the per-event inner loop.
|
|
|
|
32. **Large commented-out blocks.** `src/sprite.c:115-120,157,185-198,210`;
|
|
`src/character.c:192-199,215`; `src/assets.c:18-27,50`;
|
|
`src/tilemap.c:583,596-598,640`. All are the same abandoned
|
|
`SDL_GetBasePath()` path-prefixing approach, superseded by
|
|
`akgl_path_relative`. Delete them.
|
|
|
|
33. **Unused locals.** `screenwidth`/`screenheight` (`game.c:53-54`), `curTime`
|
|
(`game.c:499`), `curTime` and `j` (`renderer.c:114,116` — `j` is also shadowed
|
|
by the inner loop at line 128), `target` (`character.c:59`), `result`
|
|
(`util.c:37,73`), `opflags` (`heap.c:146`, declared and cleared but never
|
|
read).
|
|
|
|
34. **`akgl_game_update`'s default flags OR the same bit twice.**
|
|
`src/game.c:496` reads
|
|
`(AKGL_ITERATOR_OP_LAYERMASK | AKGL_ITERATOR_OP_LAYERMASK)`. Given the loop
|
|
that follows walks layers and calls `updatefunc`, the intent was almost
|
|
certainly `AKGL_ITERATOR_OP_UPDATE | AKGL_ITERATOR_OP_LAYERMASK`.
|
|
|
|
35. **`akgl_draw_background` is the only public function outside the error
|
|
protocol.** `include/akgl/draw.h:14` returns `void` and reports nothing;
|
|
every other public entry point returns `akerr_ErrorContext *`. It also calls
|
|
`SDL_SetRenderDrawColor` and `SDL_RenderFillRect` without checking `renderer`
|
|
or `renderer->sdl_renderer`.
|
|
|
|
36. **`akgl_registry_init_actor` is the only registry initializer that destroys
|
|
an existing registry** before recreating it (`src/registry.c:47-49`). The
|
|
other seven leak the old `SDL_PropertiesID` on a second call. Either all of
|
|
them should do it or none should.
|
|
|
|
37. **Redundant casts obscure the code.** `(json_t *)json` where `json` is
|
|
already `json_t *`, `(akgl_Tilemap *)dest`, `(akgl_Actor *)actorobj`,
|
|
`(char *)&obj->name` on an array that already decays — roughly 180 pointer
|
|
casts across `src/`, a large majority of them no-ops, densest in
|
|
`src/tilemap.c:150-624` and `src/character.c:144-172`.
|
|
They suppress exactly the conversion warnings that would catch a real
|
|
mismatch.
|
|
|
|
38. **`struct`-qualified parameters in two definitions.**
|
|
`akgl_physics_null_move` (`src/physics.c:29`) and `akgl_physics_arcade_move`
|
|
(`src/physics.c:80`) are defined with `struct akgl_PhysicsBackend *self`
|
|
while the header and the other eight physics functions use the typedef.
|
|
|
|
39. **`text.c:19` validates the wrong argument.**
|
|
`FAIL_ZERO_RETURN(errctx, name, AKERR_NULLPOINTER, "Null filepath")` checks
|
|
`name` a second time; `filepath` is never checked and is passed straight to
|
|
`TTF_OpenFont`. Copy-paste of line 18.
|
|
|
|
**Resolved** alongside the text measurement work; `tests/text.c` asserts a
|
|
NULL filepath reports `AKERR_NULLPOINTER`.
|
|
|
|
40. **`akgl_path_relative` and `akgl_path_relative_from` disagree on the output
|
|
parameter.** **Moot in 0.5.0**: `akgl_path_relative_from` is deleted (see
|
|
**Defects → Known and still open** item 4) and `akgl_path_relative_root` is
|
|
`static`, so `akgl_path_relative` is the only member of the family left and
|
|
there is nothing left to disagree with. It still takes `akgl_String *dst`
|
|
rather than the `**` form the rest of the library uses, which stays as
|
|
**item 41**'s question of naming and this one's question of shape -- but
|
|
with one function it is a decision to make when something needs it, not a
|
|
drift between siblings.
|
|
|
|
41. **`dst` vs `dest` for output parameters.** `akgl_string_copy` and the
|
|
`akgl_path_relative*` family use `dst`; everything else uses `dest`.
|
|
|
|
## 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** in `tests/testutil.h`: `TEST_TRAP_UNHANDLED_ERRORS()` installs a
|
|
handler that collapses any status a byte cannot carry onto 1, and every suite's
|
|
`main()` calls it immediately after `akgl_error_init()`. `tests/error.c` calls
|
|
it after its first test instead, because that suite has no `akgl_error_init()`
|
|
in `main()` and libakerror's lazy `akerr_init()` would overwrite the handler.
|
|
|
|
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.
|
|
|
|
Worth knowing: the fix belongs here rather than in libakerror only because
|
|
`exit(status)` is a reasonable thing for a library to do when statuses fit in a
|
|
byte. libakerror's own band does. Consumers' bands start at 256 by construction,
|
|
so any consumer's test suites have this problem. That is worth raising upstream.
|
|
|
|
## Coverage status
|
|
|
|
Generated with:
|
|
|
|
```sh
|
|
cmake -S . -B build-coverage -DAKGL_COVERAGE=ON -DCMAKE_BUILD_TYPE=Debug
|
|
cmake --build build-coverage --parallel
|
|
ctest --test-dir build-coverage --output-on-failure
|
|
```
|
|
|
|
Reports land in `build-coverage/coverage/` (`index.html`, `coverage.xml`).
|
|
|
|
**Line coverage 79.6%, function coverage 87.2%** (2189/2749 lines, 164/188
|
|
functions), up from 77.2% / 83.1% and from a 39.6% / 44.3% baseline. All 21
|
|
suites pass. (An earlier revision of this file recorded `character` as an
|
|
intentionally failing suite; it passes now, and nothing in `tests/character.c`
|
|
or `src/character.c` has changed since — the fix came from elsewhere in the tree
|
|
and this note was never updated.)
|
|
|
|
Note the trap described in "Known and still open" item 13 while regenerating
|
|
these numbers: a coverage tree rebuilt after a source edit fails `coverage_reset`
|
|
with `GCOV returncode was 5`. `find build-coverage -name '*.gcda' -delete` clears
|
|
it.
|
|
|
|
| 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` | 104/118 (88%) | 6/7 |
|
|
| `src/controller.c` | 300/329 (91%) | 16/16 |
|
|
| `src/draw.c` | 253/267 (95%) | 11/12 |
|
|
| `src/error.c` | 9/9 (100%) | 1/1 |
|
|
| `src/game.c` | 128/235 (54%) | 12/17 |
|
|
| `src/heap.c` | 111/111 (100%) | 12/12 |
|
|
| `src/json_helpers.c` | 111/111 (100%) | 11/11 |
|
|
| `src/physics.c` | 140/140 (100%) | 10/10 |
|
|
| `src/registry.c` | 76/102 (74%) | 11/12 |
|
|
| `src/renderer.c` | 42/75 (56%) | 7/8 |
|
|
| `src/sprite.c` | 93/101 (92%) | 5/5 |
|
|
| `src/staticstring.c` | 16/17 (94%) | 2/2 |
|
|
| `src/text.c` | 48/48 (100%) | 4/4 |
|
|
| `src/tilemap.c` | 201/426 (47%) | 10/20 |
|
|
| `src/util.c` | 121/131 (92%) | 7/8 |
|
|
| `src/version.c` | 2/2 (100%) | 1/1 |
|
|
|
|
Branch coverage reads 21.2% 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.
|
|
|
|
### Mutation testing
|
|
|
|
`scripts/mutation_test.py` was run over the three new files as a smoke check
|
|
(`--max-mutants 8` to 10 each, so these are samples rather than exhaustive
|
|
scores):
|
|
|
|
| File | Score | Surviving mutants |
|
|
|---|---|---|
|
|
| `src/draw.c` | 90% | Deleting the `FAIL_ZERO_BREAK` on `SDL_CreateTextureFromSurface` in `akgl_draw_flood_fill` |
|
|
| `src/audio.c` | 75% | Deleting `SUCCEED_RETURN` from the static `check_voice`; deleting `spec.freq` before opening a device |
|
|
| `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 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.
|
|
|
|
### Suites
|
|
|
|
Every suite is registered through the `AKGL_TEST_SUITES` list in
|
|
`CMakeLists.txt`, which drives target creation, CTest registration, the
|
|
`WORKING_DIRECTORY`/`TIMEOUT` properties, the link line, and the
|
|
`FIXTURES_REQUIRED akgl_coverage` list together. Adding `tests/<name>.c` and the
|
|
name to that list is all a new suite needs; it can no longer be accidentally
|
|
left out of the coverage fixture.
|
|
|
|
Shared assertion helpers are in `tests/testutil.h`: `TEST_ASSERT`,
|
|
`TEST_ASSERT_FEQ`, `TEST_EXPECT_STATUS`, `TEST_EXPECT_OK`, `TEST_EXPECT_ANY_ERROR`,
|
|
and `TEST_ASSERT_FLAG`. All except the last expand to a `break` on failure, so
|
|
they belong directly inside an `ATTEMPT` block, not inside a loop nested in one.
|
|
|
|
Done:
|
|
|
|
- `tests/physics.c` — both backends, the factory, and the full simulation loop
|
|
including thrust clamping, drag, layer masking, parent/child positioning, and
|
|
logic-interrupt handling. 100%.
|
|
- `tests/heap.c` — pool exhaustion for all five pools, refcount clamping,
|
|
recursive child release, registry cleanup on release. 100%.
|
|
- `tests/json_helpers.c` — every typed accessor, both string accessors'
|
|
allocate-vs-reuse paths, array bounds, and `akgl_get_json_with_default`. 100%.
|
|
- `tests/controller.c` — control map push and capacity, the default binding set,
|
|
keyboard and gamepad dispatch including cross-device rejection, the dpad
|
|
handlers, and device add/remove against the dummy drivers. 90%.
|
|
- `tests/game.c` — version gating, the save/load roundtrip, foreign-save
|
|
rejection, truncated-table detection, the state lock, and FPS accounting. 54%.
|
|
- `tests/actor.c` — extended with the eight control-map handlers, automatic
|
|
facing, movement logic, the animation frame state machine, `akgl_actor_update`,
|
|
and character/sprite binding lookups. 80%.
|
|
- `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.
|
|
|
|
## Defects
|
|
|
|
### Fixed while building the suites
|
|
|
|
Each was found by a test written to assert correct behavior.
|
|
|
|
1. **`akgl_physics_simulate` dereferenced `self` before its NULL check.**
|
|
`src/physics.c:132` read `self->gravity_time` at declaration time, three
|
|
lines above `FAIL_ZERO_RETURN(e, self, ...)`. A NULL backend segfaulted.
|
|
2. **`akgl_game_save` never flushed or closed its stream.** `CLEANUP` and
|
|
`PROCESS` were transposed, which put the `fclose` inside the `PROCESS`
|
|
switch, where it only ran if an error context existed and reported success.
|
|
An ordinary save produced an empty file.
|
|
3. **`akgl_game_save_actors` wrote name-table terminators from a single char.**
|
|
`aksl_fwrite((void *)&nullval, 1, AKGL_ACTOR_MAX_NAME_LENGTH, fp)` emitted
|
|
127 bytes of adjacent stack memory into the save file and produced a sentinel
|
|
the loader could not recognize.
|
|
4. **`akgl_game_load_objectnamemap` swallowed read failures.** `CATCH` used
|
|
directly inside `while (1)` breaks the loop, not the function, so a truncated
|
|
or corrupt name table loaded as a successful game.
|
|
5. **`akgl_Actor_cmhf_up_on` and `_down_on` dereferenced `basechar` unguarded**,
|
|
unlike their left and right counterparts.
|
|
6. **`akgl_actor_logic_movement` checked `actor` twice** instead of checking
|
|
`actor->basechar` before dereferencing it.
|
|
7. **The gamepad handlers checked `appstate` three times each**, so a NULL event
|
|
or a missing player actor was never caught and `player->state` was
|
|
dereferenced regardless.
|
|
|
|
### Known and still open
|
|
|
|
1. **`akgl_render_and_compare` compares a texture against itself.**
|
|
`src/util.c:228` and `src/util.c:245` both draw `t1`; `t2` is never rendered,
|
|
so the function always passes and the image assertions in `tests/sprite.c`
|
|
assert nothing.
|
|
2. **`akgl_tilemap_release` double-frees tileset textures.**
|
|
`src/tilemap.c:849` destroys `dest->tilesets[i].texture` inside the *layers*
|
|
loop; it should be `dest->layers[i].texture`. It also never NULLs the
|
|
pointers, so a second release is a use-after-free.
|
|
3. **`akgl_registry_init` never initializes the properties registry.**
|
|
`akgl_registry_init_properties()` is not called from `akgl_registry_init()`
|
|
(`src/registry.c:27`), so `AKGL_REGISTRY_PROPERTIES` stays 0 unless the
|
|
caller initializes it separately. `akgl_set_property` is then a silent no-op
|
|
and `akgl_get_property` always returns the caller's default, which means
|
|
`akgl_physics_init_arcade` and `akgl_render_init2d` silently ignore
|
|
configuration. `akgl_game_init` does call it; a caller that does not use
|
|
`akgl_game_init` does not get it.
|
|
4. **`akgl_path_relative_from` is a stub that leaks.** **Deleted in 0.5.0.**
|
|
It claimed a heap string, never wrote `*dst`, 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.**
|
|
`src/util.c:208` compares `s1->pitch * s1->h` bytes of `s2` without verifying
|
|
the surfaces share dimensions, pitch, or format.
|
|
6. **`akgl_string_initialize` overflows by four bytes when `init` is NULL.**
|
|
`src/staticstring.c:17` does `memset(&obj->data, 0x00, sizeof(akgl_String))`,
|
|
but `data` starts four bytes into the struct (after `refcount`), so the memset
|
|
runs four bytes past the end.
|
|
7. **Savegame name lengths disagree between writer and reader.**
|
|
`akgl_game_save_actors` writes spritesheet names at
|
|
`AKGL_SPRITE_SHEET_MAX_FILENAME_LENGTH` (512) and character names at
|
|
`AKGL_SPRITE_MAX_CHARACTER_NAME_LENGTH` (128), but `akgl_game_load` reads
|
|
every table at `AKGL_ACTOR_MAX_NAME_LENGTH` (128). A save with any registered
|
|
spritesheet cannot be read back. The current roundtrip test passes only
|
|
because empty registries write nothing but zeroed sentinels.
|
|
8. **Heap acquire functions are asymmetric.** `akgl_heap_next_string` increments
|
|
`refcount`; `next_actor`, `next_sprite`, `next_spritesheet`, and
|
|
`next_character` do not. `tests/heap.c` pins the current behavior and says so;
|
|
decide whether to make them symmetric or document the split.
|
|
9. **`tests/util.c` defines `test_akgl_collide_point_rectangle_logic` but
|
|
`main()` never calls it.**
|
|
10. **`controller.h` declares functions that do not exist.** **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.** Both check `controlmapid >= AKGL_MAX_CONTROL_MAPS` but not
|
|
`controlmapid < 0`, so a negative id indexes before `GAME_ControlMaps`.
|
|
12. **A failed controller-DB fetch silently destroys the tracked fallback.**
|
|
`include/akgl/SDL_GameControllerDB.h` is committed deliberately so the
|
|
library still builds if upstream disappears. But `mkcontrollermappings.sh`
|
|
has no `set -e` and never checks `curl`'s exit status: on a failed fetch it
|
|
writes `mappings.txt` empty, then overwrites the good tracked header with
|
|
`AKGL_SDL_GAMECONTROLLER_DB_LEN 0` and an empty initializer — and exits 0.
|
|
Verified by pointing the script at an unresolvable host.
|
|
|
|
Two compounding problems:
|
|
|
|
- `add_custom_command` at `CMakeLists.txt:89-93` declares
|
|
`OUTPUT include/akgl/SDL_GameControllerDB.h` as a *relative* path, which
|
|
CMake resolves against the binary directory, while the script writes to
|
|
the source directory. The declared output never appears, so the command is
|
|
permanently out of date and re-runs on every build — making every build
|
|
depend on network access and leaving the file dirty in the working tree
|
|
each time.
|
|
- `const char *SDL_GAMECONTROLLER_DB[] = {\n};` is an empty initializer,
|
|
which is a constraint violation in ISO C and compiles only as a GCC
|
|
extension.
|
|
|
|
Fix: have the script fetch to a temporary file, check `curl`'s status and a
|
|
plausible minimum line count, and leave the existing header untouched on
|
|
failure (exiting non-zero). Separately, make regeneration explicit — a
|
|
dedicated `controllerdb` target, or an `OUTPUT` that matches where the
|
|
script actually writes — so an ordinary build neither needs the network nor
|
|
dirties the tree.
|
|
13. **A stale build tree in the source directory breaks the coverage run.**
|
|
`CMakeLists.txt:208` and `CMakeLists.txt:223` pass
|
|
`--root "${CMAKE_CURRENT_SOURCE_DIR}"` to gcovr, and gcovr searches for
|
|
`.gcda`/`.gcno` files under the root. The `--object-directory` argument on
|
|
the line below each does *not* narrow that search: per `gcovr --help` it
|
|
only identifies "the path between gcda files and the directory where the
|
|
compiler was originally run". So every instrumented build tree left inside
|
|
the source directory is folded into the report alongside the one actually
|
|
being measured.
|
|
|
|
With a `build-coverage/` from an earlier session still present, a freshly
|
|
configured `-DAKGL_COVERAGE=ON` tree fails in `coverage_reset`, before any
|
|
test runs:
|
|
|
|
AssertionError: Got function akgl_game_lowfps on multiple lines: 45, 46.
|
|
|
|
45 and 46 are that function's line numbers before and after an unrelated
|
|
`#include` was added to `src/game.c`: gcovr found 18 stale `.gcno` files
|
|
describing the old layout, merged them with the current ones, and could not
|
|
reconcile the two. Moving `build-coverage/` aside makes the same tree pass
|
|
18/18. Verified with gcovr 7.0.
|
|
|
|
The `build*/` entry in `.gitignore` hides these trees from `git status`,
|
|
which makes the state easier to get into and no easier to notice.
|
|
|
|
A second, smaller version of the same thing: rebuilding an existing
|
|
coverage tree after editing a test leaves `.gcda` files describing the old
|
|
object layout, and `coverage_reset` -- whose whole job is to delete them --
|
|
fails with `GCOV returncode was 5` before it gets the chance. `find
|
|
build-coverage -name '*.gcda' -delete` clears it. Observed with gcovr 7.0.
|
|
|
|
Fix: pass the build tree to gcovr as an explicit search path instead of
|
|
letting it default to `--root`, so only the tree under measurement is
|
|
considered. Touches the two `add_test` blocks at `CMakeLists.txt:204-211`
|
|
and `CMakeLists.txt:220-229`; nothing outside the `AKGL_COVERAGE` branch.
|
|
|
|
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.
|
|
|
|
### Found while rewriting the Doxygen comments
|
|
|
|
Each of these came out of reading an implementation against the contract its
|
|
header claimed. They are recorded inline as `@note` or `@warning` on the
|
|
function concerned, so a reader of the generated documentation finds them
|
|
without coming here first. Ordered by blast radius.
|
|
|
|
15. **`akgl_path_relative` leaks one error-context slot per call on its fallback
|
|
path.** `src/util.c:120` returns `akgl_path_relative_root(...)` from inside
|
|
the `HANDLE(e, ENOENT)` block. `FINISH` is what carries `RELEASE_ERROR`, so
|
|
returning before it never gives the context back. libakerror hands these out
|
|
of a fixed `AKERR_ARRAY_ERROR[128]`, and this is not a rare path — it is the
|
|
*ordinary* one, taken every time an asset names a neighbour relative to its
|
|
own file rather than to the working directory. Every tileset image, layer
|
|
image and spritesheet in a map costs a slot, permanently. Once the array is
|
|
exhausted every subsequent failure anywhere in the process has nowhere to
|
|
report from.
|
|
|
|
Fix: assign the result to a local, `break`, and return after `FINISH`; or
|
|
hoist the fallback out of the handler entirely and let the `HANDLE` block
|
|
only record that a retry is wanted. Touches `src/util.c:117-121` only.
|
|
|
|
16. **`akgl_sprite_load_json` does not bound the `frames` array.**
|
|
`src/sprite.c:165` sets `obj->frames` from `json_array_size()` and
|
|
`src/sprite.c:167` then writes that many entries into
|
|
`frameids[AKGL_SPRITE_MAX_FRAMES]`, which is 16. A sprite definition with 17
|
|
or more frames writes past the array into the rest of `akgl_Sprite`, and
|
|
past the struct into the neighbouring pool slot. It is also written through
|
|
a `uint32_t *` cast of a `uint8_t *`, so each element write touches four
|
|
bytes; that happens to work on a little-endian machine because the following
|
|
iterations overwrite the spill and the last write lands in the struct's
|
|
alignment padding, but it is not portable and it is what makes the overflow
|
|
reach four bytes past `frames` rather than one.
|
|
|
|
Fix: refuse a `frames` array longer than `AKGL_SPRITE_MAX_FRAMES` with
|
|
`AKERR_OUTOFBOUNDS`, and read into an `int` local before narrowing to
|
|
`uint8_t`. Touches `src/sprite.c:164-169`.
|
|
|
|
17. **Two more unbounded array loads in the tilemap loader.**
|
|
`src/tilemap.c:387-389` walks a Tiled object layer straight into
|
|
`curlayer->objects[j]` with no check against
|
|
`AKGL_TILEMAP_MAX_OBJECTS_PER_LAYER` (128), and `src/tilemap.c:278-280`
|
|
fills `dest->tilesets[i]` with no check against `AKGL_TILEMAP_MAX_TILESETS`
|
|
(16).
|
|
Both are the same shape as item 16 and both are reachable from an ordinary
|
|
map file: 128 objects is not a large object layer. Note that
|
|
`akgl_tilemap_load_layers` *does* bound its loop and raises
|
|
`AKERR_OUTOFBOUNDS`, so the pattern to copy is already in the same file.
|
|
|
|
Fix: add the bound check at the top of each loop body, matching
|
|
`akgl_tilemap_load_layers`. Touches `src/tilemap.c:387` and
|
|
`src/tilemap.c:278`.
|
|
|
|
18. **`akgl_get_json_with_default` defaults on a status the array accessors never
|
|
raise.** `src/json_helpers.c:164-165` handles `AKERR_KEY` and `AKERR_INDEX`,
|
|
but `akgl_get_json_array_index_object`, `_integer` and `_string` all report
|
|
a short array as `AKERR_OUTOFBOUNDS`. So the "this element is optional" form
|
|
silently does not work for an array — the error propagates instead of being
|
|
replaced by the default. Only the object accessors, which do raise
|
|
`AKERR_KEY`, are actually served by this function today. Nothing in tree
|
|
passes an array accessor's error here, which is why it has not been noticed.
|
|
|
|
Fix: add a third `HANDLE_GROUP(err, AKERR_OUTOFBOUNDS)`. Note that the
|
|
existing two rely on `HANDLE_GROUP` not emitting a `break`, so the `KEY`
|
|
case falls through into the `INDEX` body — a third arm has to go *above* the
|
|
one holding the `memcpy`, not below it. Touches `src/json_helpers.c:164-167`.
|
|
|
|
19. **The background music never loops.** `src/assets.c:20` initialises
|
|
`bgmprops` to 0, `src/assets.c:44` sets `MIX_PROP_PLAY_LOOPS_NUMBER` on it,
|
|
and `src/assets.c:46` plays the track with it. 0 is SDL's "no property set"
|
|
sentinel, not a set this function owns, so the write is rejected —
|
|
unchecked — and the play call is given no options. The music plays once and
|
|
stops. `akgl_load_start_bgm` reports success either way.
|
|
|
|
Fix: `SDL_CreateProperties()` into `bgmprops`, check it, and destroy it in
|
|
`CLEANUP`. Touches `src/assets.c:20-47`.
|
|
|
|
20. **`akgl_character_sprite_add` leaks a sprite reference when a state is
|
|
remapped.** `src/character.c:51` writes the new sprite over any existing
|
|
entry for that state without releasing the one it displaces, whose refcount
|
|
was incremented when it was added. The displaced sprite's pool slot is never
|
|
reclaimed. The write itself is also unchecked, so a failure to record the
|
|
mapping is reported as success.
|
|
|
|
Fix: read the existing entry first and `akgl_heap_release_sprite` it, and
|
|
check `SDL_SetPointerProperty`'s return. Touches `src/character.c:43-55`.
|
|
|
|
21. **`akgl_heap_release_character` abandons the whole state-to-sprite map.**
|
|
`src/heap.c:150` 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.**
|
|
`src/util.c:75` returns past `CLEANUP` on the over-long-path branch, so the
|
|
two scratch strings claimed at `src/util.c:67-68` are never released. This is
|
|
the exact hazard AGENTS.md documents under the error-handling protocol, and
|
|
the same class as the heap-string leak already fixed in
|
|
`akgl_get_json_tilemap_property`. Smaller blast radius than item 15 because
|
|
the branch is rare, but it is a mechanical fix.
|
|
|
|
Fix: `FAIL_BREAK`. Touches `src/util.c:75`.
|
|
|
|
23. **Three smaller leaks on failure paths.** All the same shape: a resource
|
|
acquired before an `ATTEMPT`/`CLEANUP` pair, or released only on the success
|
|
path.
|
|
|
|
- `src/renderer.c:26-32` — `akgl_render_init2d` releases the two pooled
|
|
strings holding the screen dimensions only after both `aksl_atoi` calls
|
|
succeed, so a non-numeric `game.screenwidth` leaks two string slots.
|
|
- `src/controller.c:80` — `akgl_controller_open_gamepads` calls
|
|
`SDL_free(gamepads)` only after the loop completes, so a gamepad that
|
|
fails to open leaks the enumeration array.
|
|
- `src/text.c:57-64` — `akgl_text_rendertextat` destroys the surface and
|
|
texture only on the success path, so a failed texture upload or a failed
|
|
draw leaks both. On a HUD line redrawn every frame that is a leak per
|
|
frame.
|
|
|
|
24. **`akgl_get_property` reads past the end of the property value.**
|
|
`src/registry.c:182` copies a fixed `AKGL_MAX_STRING_LENGTH` bytes out of
|
|
whatever `SDL_GetStringProperty` returned, rather than that string's length.
|
|
The destination is a full-sized pool string so nothing is corrupted, but the
|
|
source is an ordinary NUL-terminated string owned by SDL and the read runs
|
|
up to PATH_MAX bytes past its end. Benign in practice and immediately fatal
|
|
under ASAN, which is the reason to fix it rather than leave it.
|
|
|
|
Fix: `aksl_strlcpy` or an explicit length. Touches `src/registry.c:181-186`.
|
|
|
|
25. **`akgl_character_load_json_state_int_from_strings` checks the same argument
|
|
twice.** `src/character.c:123` guards `states` and `src/character.c:124`
|
|
guards `states` again under the message "NULL destination integer" — the
|
|
guard for `dest` was never written. A `NULL` `dest` is dereferenced at
|
|
`src/character.c:132`. Only reachable from inside the character loader, which
|
|
always passes a real pointer, so it is a latent hole rather than a live bug.
|
|
|
|
Fix: change the second guard's subject to `dest`. Touches
|
|
`src/character.c:124`.
|
|
|
|
26. **`akgl_actor_render` computes a sprite's drawn height from its width.**
|
|
`src/actor.c:276` sets `dest.h = curSprite->width * obj->scale` where it
|
|
should read `curSprite->height`. Every actor is therefore drawn square, and
|
|
a non-square sprite is stretched or squashed. Invisible in the current
|
|
assets because they are square; it will not stay that way.
|
|
|
|
Fix: one word. Touches `src/actor.c:276`. Worth a test that renders a
|
|
deliberately non-square sprite and asserts on the destination rectangle.
|
|
|
|
### Found while closing the akbasic API gaps
|
|
|
|
27. **`akgl_text_rendertextat` refuses the empty string, and `akgl_text_measure` accepts it.**
|
|
SDL_ttf returns `NULL` with "Text has zero width" from both `TTF_RenderText_Blended` and
|
|
`TTF_RenderText_Blended_Wrapped` for `""`, so `src/text.c:56`'s `FAIL_ZERO_RETURN` on the
|
|
surface reports `AKERR_NULLPOINTER` for what a caller means as "draw nothing". Verified
|
|
directly against SDL_ttf 3.x with the fixture font.
|
|
|
|
`akgl_text_measure("")` is documented as legal and returns 0 wide by one line high, which
|
|
is what a cursor sitting on an empty line needs, so the two halves of the same header
|
|
disagree about the same string. The functional consequence is a caller that draws a line of
|
|
text which may be empty — a line editor, a `PRINT` of an empty string, a HUD field that has
|
|
not been filled in yet — has to test for it before every call or handle a failure that
|
|
means nothing went wrong.
|
|
|
|
**Fix:** return success without rasterizing when `text[0] == '\0'`, matching the measure
|
|
side, and say so in the header. Touches `akgl_text_rendertextat` only. `tests/text.c` has
|
|
the case written and deliberately not asserted; it would become a `TEST_EXPECT_OK`. Until
|
|
then the header carries the wart as a `@note` pointing here.
|
|
|
|
## 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.
|
|
|
|
### Defects the perf suites found
|
|
|
|
Ordered by blast radius. Numbering continues the **Defects** list above.
|
|
|
|
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.** Measured by
|
|
counting non-zero `HEAP_STRING` refcounts across load/release cycles: five
|
|
per cycle, exactly, and `akgl_tilemap_release` gives none of them back. The
|
|
52nd map load in a process finds the pool empty.
|
|
|
|
Blast radius: every level transition. A game with fifty levels, or one that
|
|
reloads a level on death, hits it. The fix is a sweep of the loader's
|
|
`CLEANUP` blocks — `akgl_tilemap_load`, `akgl_tilemap_load_layers`,
|
|
`akgl_tilemap_load_layer_tile`, `akgl_tilemap_load_layer_objects`, and
|
|
`akgl_tilemap_load_tilesets_each` all claim scratch strings — and it wants a
|
|
test that asserts the pool is unchanged across a load/release cycle. That is
|
|
its own commit, not a footnote to a benchmark.
|
|
|
|
Until it is fixed, the tilemap-load benchmark in `tests/perf_render.c`
|
|
reclaims the pool by hand between iterations, and says so.
|
|
|
|
30. **Two JSON accessors turn string-pool exhaustion into a segfault.**
|
|
`akgl_get_json_string_value` ends its `ATTEMPT` with
|
|
`FINISH(errctx, false)` at `src/json_helpers.c:89`, so a failed
|
|
`akgl_heap_next_string` is swallowed rather than passed up, and
|
|
`src/json_helpers.c:91` then `strncpy`s `AKGL_MAX_STRING_LENGTH` bytes
|
|
through the pointer it never set. `akgl_get_json_array_index_string` has the
|
|
same pair at `src/json_helpers.c:143` and `src/json_helpers.c:145`. Item 29
|
|
is what makes this reachable, and this is what item 29 looks like from the
|
|
outside: not `AKGL_ERR_HEAP`, a crash inside `strncpy` with a NULL
|
|
destination.
|
|
|
|
Fix: `FINISH(errctx, true)` in both. One word each, but it changes what
|
|
callers see on a path they currently cannot survive, so it wants a test that
|
|
exhausts the pool deliberately and asserts `AKGL_ERR_HEAP` comes back out of
|
|
both functions.
|
|
|
|
31. **`akgl_game_update` segfaults if `akgl_game_init` did not run.**
|
|
`src/game.c:182` calls `game.lowfpsfunc()` through the pointer with no NULL
|
|
check, on every frame where `game.fps` is under 30 — which includes the
|
|
first frame, before there is a frame rate to compare against. `akgl_game_init`
|
|
installs the default; nothing else does.
|
|
|
|
That matters because `include/akgl/renderer.h` documents the other path on
|
|
purpose: a host that owns its own window and calls `akgl_render_bind2d`
|
|
instead of `akgl_render_init2d`. An embedder following that documentation
|
|
and then calling `akgl_game_update` crashes on frame one. `akbasic` is
|
|
exactly that embedder.
|
|
|
|
Fix: guard the call, or have `akgl_game_updateFPS` install the default when
|
|
it finds a NULL. `tests/perf_render.c` installs it by hand as a workaround
|
|
and points here.
|
|
|
|
32. **`akgl_game_update` runs the actor update sweep once per tilemap layer.**
|
|
`src/game.c:617` loops `i` over `AKGL_TILEMAP_MAX_LAYERS` and the actor
|
|
sweep nested inside it never compares `actor->layer` to `i`, so every live
|
|
actor's `updatefunc` runs **16 times per frame**. Measured: 68.4 ns per
|
|
update, 64 actors, so 70 µs of work to do 4.4 µs of work.
|
|
|
|
It hides behind the rasterizer today (a 640x480 software frame is 16 ms) and
|
|
it will not hide behind a GPU backend, where a frame is nearer 2 ms and this
|
|
is 3.5% of it. Fix: either filter by layer like `akgl_render_2d_draw_world`
|
|
does, or hoist the update sweep out of the layer loop entirely — updating an
|
|
actor is not a per-layer operation. The second is almost certainly right,
|
|
and it is the one that needs a test asserting `updatefunc` runs exactly once
|
|
per actor per `akgl_game_update`.
|
|
|
|
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.
|
|
|
|
### 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.
|
|
|
|
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.
|
|
|
|
| # | 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** |
|
|
| 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 | no broad phase exists; the naive loop is 1.9 ms at 256 actors | **missed** |
|
|
| 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 leaks 5 strings per cycle (item 29) | **missed** |
|
|
| 17 | Pool exhaustion reports `AKGL_ERR_HEAP` and never crashes | item 30 crashes | **missed** |
|
|
| 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.** 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.
|
|
|
|
- **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.
|
|
|
|
- **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.
|
|
|
|
## 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; "still
|
|
reachable" is what SDL and FreeType keep for the process lifetime and says
|
|
nothing about this library.
|
|
|
|
### 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.
|
|
|
|
Ordered by blast radius as they were found. Numbering continues the lists above.
|
|
Every size below is measured, not estimated.
|
|
|
|
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 |
|
|
|
|
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.
|
|
|
|
### 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.
|
|
|
|
### 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.
|
|
|
|
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
|
|
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.
|
|
|
|
**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.
|
|
|
|
## 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(<target> <dir>)` macro adds each of the seven submodules when
|
|
nothing has already declared that target *and* the submodule is actually checked out; the
|
|
`find_package` lookups for anything left over run afterwards, unchanged, so a checkout
|
|
without submodules still resolves against the system. A macro rather than a function
|
|
because `add_subdirectory()` inside a function runs in that function's variable scope.
|
|
|
|
The CTest suppression moved with them, and is lifted again before this project registers
|
|
its own suites, so an embedding consumer's `add_test()` still reaches CTest. Two smaller
|
|
consequences of the move: `find_package(PkgConfig)` now runs only when something has to
|
|
come from the system (a fully vendored build needs no pkg-config of its own, though
|
|
`deps/SDL_mixer` asks for it separately), and the build-tree RPATH block keys on whether
|
|
anything was vendored rather than on being the top-level project, so an embedded build's
|
|
tests can also find the satellite libraries.
|
|
|
|
Verified by configuring and building a scratch consumer that embeds this repository with
|
|
`CMAKE_DISABLE_FIND_PACKAGE_SDL3`, `_SDL3_image`, `_SDL3_mixer`, `_SDL3_ttf`, `_akerror`,
|
|
`_akstdlib` and `_jansson` all set, so any reliance on an installed copy would have failed
|
|
the configure. It configures, builds and links. **The five `add_subdirectory` lines in
|
|
`akbasic`'s `CMakeLists.txt` can be deleted.**
|
|
|
|
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 `<akgl/actor.h>`. Not a forward declaration:
|
|
`akgl_Actor` is a typedef of a named struct, and repeating a typedef is C11, not C99. The
|
|
include closes no cycle — `actor.h` reaches only `types.h` and `character.h`, neither of
|
|
which knows about the controller.
|
|
|
|
`tests/headers.c` is the test, and it is a whole suite for one `#include` on purpose: a
|
|
second `#include` after the first proves nothing about the second, because by then the
|
|
first has dragged its dependencies in. Covering another header means another file shaped
|
|
like that one. It also stopped being true that `tests/controller.c` has to include
|
|
`akgl/actor.h` first, and the comment there saying so is gone.
|
|
|
|
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%.
|
|
|
|
## 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.
|
|
|
|
**Still open: removal and replacement.** There is no API to unbind one state,
|
|
and `akgl_character_sprite_add` still overwrites an existing entry without
|
|
releasing the sprite it displaces (**Defects** item 20). A character that
|
|
rebinds a state while alive still leaks one reference per rebind; teardown
|
|
only gives back whatever the map happens to hold at the end. Wants
|
|
`akgl_character_sprite_del`, a release of the displaced entry inside
|
|
`akgl_character_sprite_add`, and tests for removal, replacement, and
|
|
duplicate sprite bindings across multiple states.
|
|
|
|
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.
|