diff --git a/TODO.md b/TODO.md index 0bb7ae1..c157c3c 100644 --- a/TODO.md +++ b/TODO.md @@ -672,6 +672,38 @@ Each was found by a test written to assert correct behavior. 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. + ## Build notes The vendored SDL satellite libraries are built into per-project subdirectories @@ -685,10 +717,18 @@ 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. Its interpreter -core is complete and passes its whole acceptance corpus against a stdio text sink; the -`libakgl`-backed sink and the graphics, sound and console verbs of Commodore BASIC 7.0 are -blocked on the four gaps below. +being built to link into `libakgl` as a scripting engine for game authors. + +**Items 1 through 4 are all resolved, 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**, filed below. Four are things `akbasic` had to work +around to build at all; each workaround is commented at its site over there with the words +"filed upstream", so they can be found and deleted when these land. These are 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 @@ -832,6 +872,80 @@ a consumer is the wanted outcome. Each entry says what the BASIC verb needs, wha 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. + +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. + +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. + +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. + +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. + ## Carried over 1. **Make character-to-sprite state bindings release their references symmetrically.**