File five defects akbasic found consuming the new APIs
akbasic has now consumed all four of the API gaps this file listed as blocking it: its text sink, graphics backend, audio backend and input backend are written and tested, and the BASIC 7.0 graphics, sound and console verbs work against them. Doing that turned up six things worth fixing here -- five of which still are. Four are workarounds akbasic had to write to build at all, and each is commented at its site over there with the words "filed upstream": - An embedded libakgl requires SDL, SDL_image, SDL_mixer, SDL_ttf and jansson to be installed, while vendoring all five. The dependency block is gated on being top-level, so an add_subdirectory() consumer takes the find_package path and fails with the submodules sitting right there in deps/. - include/akgl/controller.h does not compile on its own: it declares handlers taking an akgl_Actor * and includes nothing that declares the type. src/ never notices because it includes game.h first. - akgl_render_init2d() fuses two separable jobs -- creating a window, and installing the backend vtable -- so a caller who already owns an SDL_Renderer has no way to get the second without the first. That caller is exactly the embedding host this section exists to serve. - akgl_text_rendertextat() dereferences renderer->draw_texture with no check, which is the state item 7 leaves a host in. Same class of defect the draw commit added a test for; the text path still has it. The fifth is a real API gap: SOUND's frequency sweep has no equivalent, and it cannot be faked in the interpreter without tying audible pitch to the host's frame rate, so it has to be advanced on the mixer's own frame counter. The sixth was that42b60f7shipped 22 public symbols under an unchanged VERSION and soname.1066ac7fixed that independently while this was being written -- the two crossed rather than one following the other -- so it is filed as resolved rather than dropped. The reason is the part worth keeping: akbasic could not feature-test for the new API and had to pin libakgl by submodule commit in its README until that landed, which is the concrete cost of an additive release under an unchanged soname. All five outstanding items were re-verified against this tree after the rebase, not just carried forward: controller.h still includes neither actor.h nor a forward declaration, render_init2d still calls SDL_CreateWindowAndRenderer before installing the vtable, text.c:62 still reaches through draw_texture unchecked, the vendored-dependency block is still gated on being top-level, and there is still no akgl_audio_sweep. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
122
TODO.md
122
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.**
|
||||
|
||||
Reference in New Issue
Block a user