Record what writing the manual found: 18 defects and 27 false claims
Twenty-one chapters and two games were written against src/ rather than against the header comments, and the exercise turned up two classes of problem. Both are recorded here because publishing a problem you cannot fix yet is a contribution, and because the second class is the more dangerous one: every item in it was documented, in a header, incorrectly. The one to fix first is defect 1. akgl_game_update calls akgl_physics->simulate() with no NULL check and akgl_default_physics is zeroed BSS, so a program that never calls an initializer segfaults on its first frame -- measured, exit 139, not an AKERR_NULLPOINTER. physics.h tells the reader akgl_game_init selects a backend from a `physics.engine` property, which is false in both halves, so a caller who believes the header writes exactly the program that crashes. That is the worst first-contact experience in the library and the fix is a NULL check. Defect 2 is the subtlest. A child actor's offset is counted twice: physics.c writes x as parent->x + vx, an absolute coordinate, and actor.c then draws at parent->x + obj->x while actor_visible three lines above treats obj->x as absolute. Two readings of one field inside one function, and actor.h documents both of them without noticing. Confirmed by rendering the frame with and without the guard and hashing the readback. The rest run from silent invisibility (an actor on layer >= 16 is updated and simulated and never drawn; a map-spawned actor has no facing bit and so no sprite) through dead API (speedtime is loaded, mis-cast, and read by nothing; there is no way to play a sound effect from a file at all) to asset provenance: tests/assets/World_A1.png and util/assets/Actor1.png carry RPG Maker's default filenames with no licence file, and util/assets/littleguy.json -- the sample data for the one demo the library ships -- does not load. The second section lists the 27 header comments that describe code that has since changed. Nothing catches these: WARN_IF_UNDOCUMENTED proves a symbol has a comment, not that the comment is true, and check_api_surface.sh strips comments precisely because prose is not a declaration. Doxygen publishes them. A third section marks entries in this file that are themselves stale, including item 15, which describes a leak that src/util.c already fixes with exactly the technique the item proposes. AGENTS.md warns that this file "carried eleven entries describing code that had already changed"; that is still accumulating, and a premise nobody has re-checked is where the next defect is hiding. plan.md is the plan the work was executed from, kept because it records the decisions and, in three places, corrections to itself -- including getting the error-code count wrong in the document whose purpose was fixing wrong documentation. Co-Authored-By: Claude Code <noreply@anthropic.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
177
TODO.md
177
TODO.md
@@ -2184,3 +2184,180 @@ a live defect -- but nothing rejects it, and `max_timestep` is caller-settable.
|
||||
non-square sprite draws at its own proportions through the library's own
|
||||
`renderfunc`. This item is the half that remains: one uniform `scale`, with no way
|
||||
to expand a single axis.
|
||||
|
||||
## Found while writing the manual
|
||||
|
||||
Twenty-one chapters and two tutorial games were written against `src/` rather than
|
||||
against the header comments, and the exercise turned up two distinct classes of problem.
|
||||
Both are recorded here because publishing a problem you cannot fix yet is still a
|
||||
contribution, and because the second class is the more dangerous one: **every item in it
|
||||
was documented, in a header, incorrectly.**
|
||||
|
||||
The manual documents each of these where a reader would hit it, and points here.
|
||||
|
||||
### Defects with no prior entry
|
||||
|
||||
1. **`akgl_game_update` calls `simulate` through a NULL pointer.** `akgl_game_update`
|
||||
invokes `akgl_physics->simulate(akgl_physics, NULL)` with no NULL check, and
|
||||
`akgl_default_physics` is zeroed BSS — all four method pointers are NULL. A program
|
||||
that does not call `akgl_physics_init_arcade`/`_null` itself therefore **segfaults on
|
||||
its first frame**, measured as exit 139, rather than raising `AKERR_NULLPOINTER`.
|
||||
`physics.h:8-9,195` tells the reader `akgl_game_init` selects a backend from a
|
||||
`physics.engine` property, so a caller who believes the header writes exactly the
|
||||
program that crashes. This is the worst first-contact experience in the library and
|
||||
the fix is a NULL check. Closing it touches `src/game.c` only.
|
||||
|
||||
2. **Parent/child offset is double-counted at draw time.** `src/physics.c:192-197` writes
|
||||
a child's `x` as `parent->x + vx` — an absolute world coordinate. `src/actor.c:273-279`
|
||||
then draws it at `parent->x + obj->x`, while `actor_visible` three lines above tests
|
||||
the camera against the raw `obj->x` as absolute. Two readings of one field inside one
|
||||
function. Confirmed by measurement: with the player at (280,146) and a (-14,+10)
|
||||
offset, the guarded draw lands at (130,114) and the unguarded one at (410,260), off a
|
||||
320x240 screen; rendering both and hashing `SDL_RenderReadPixels` gives different
|
||||
images. Invisible only while the parent sits at the origin. `actor.h` documents **both**
|
||||
readings, in two places. `examples/jrpg` works around it with a `renderfunc` that nulls
|
||||
`obj->parent` for the duration of the draw.
|
||||
|
||||
3. **Actors and characters unregister under a different key than they register.**
|
||||
`akgl_actor_initialize` (`src/actor.c:46`) and `akgl_character_initialize`
|
||||
(`src/character.c:39`) register under the caller's untruncated `name`, while
|
||||
`akgl_heap_release_actor` (`src/heap.c:132`) and `_release_character`
|
||||
(`src/heap.c:165`) clear using the object's truncated 128-byte field. Past 127 bytes
|
||||
those are different keys, so releasing leaves a live registry entry pointing at a
|
||||
zeroed pool slot. Distinct from "Truncated registry keys can collide" above, which
|
||||
describes sprites and spritesheets, where the truncated name genuinely *is* the key.
|
||||
|
||||
4. **Every actor spawned from a map is invisible on frame one.**
|
||||
`akgl_actor_initialize` sets `movement_controls_face`, and the default `facefunc`
|
||||
clears every facing bit and re-sets one only from a *movement* bit. An NPC has no
|
||||
movement bit, so state 17 falls to 16, which maps to no sprite, and the actor is
|
||||
silently skipped by the draw. The same thing makes a player character vanish the
|
||||
moment they stop walking. Both tutorials clear the field on every live actor after
|
||||
loading a map; that workaround should not be necessary.
|
||||
|
||||
5. **`use_own_physics` is set and never read.** `akgl_tilemap_load_physics` builds a
|
||||
complete `akgl_PhysicsBackend` on the map when it declares a `physics.model` property,
|
||||
and sets `map->use_own_physics` — and nothing in the library ever consults it. A map's
|
||||
declared physics is silently ignored unless the application writes the switch itself.
|
||||
`README.md` used to show that `if`, which made a caller-side workaround read like a
|
||||
library feature.
|
||||
|
||||
6. **`akgl_TilemapLayer` does not record a layer's name.** `akgl_tilemap_load_layers`
|
||||
reads `id`, `opacity`, `visible`, `x`, `y` and `type` and drops the `name` Tiled
|
||||
wrote; the only `name` retained anywhere is per-object. A game therefore cannot ask
|
||||
for "the terrain layer" and must hard-code a numeric layer id, which changes whenever
|
||||
somebody reorders layers in the editor. Both tutorials hard-code one.
|
||||
|
||||
7. **`akgl_character_sprite_add` leaks a reference when the same sprite is re-added.**
|
||||
`ref->refcount += 1` is unconditional but the matching release is guarded by
|
||||
`displaced != ref`, so re-adding a sprite to a state it already occupies leaks a pool
|
||||
slot per call.
|
||||
|
||||
8. **`akgl_tilemap_compute_tileset_offsets` silently requires `spacing == 0` and
|
||||
`margin == 0`.** It adds `spacing` to the tile pitch but sets row 0's y offset to
|
||||
`spacing` rather than 0, and ignores `margin` entirely. A tileset with a gutter — which
|
||||
is most published tileset packs — renders misaligned with no diagnostic. This
|
||||
materially constrains what art the library can consume; it ruled out several otherwise
|
||||
suitable CC0 packs while sourcing the tutorial assets.
|
||||
|
||||
9. **`akgl_actor_render` hard-codes `SDL_FLIP_NONE`** (`src/actor.c:283`). There is no
|
||||
mirrored blit, so a side-on character needs both facings drawn in the sheet. That is
|
||||
why `docs/tutorials/assets/sidescroller/player.png` carries six frames rather than
|
||||
three, and it doubles the art cost of every such character.
|
||||
|
||||
10. **`akgl_Actor::layer` is unbounded but `draw_world` stops at `AKGL_TILEMAP_MAX_LAYERS`.**
|
||||
An actor assigned layer 16 or above is accepted, updated, simulated — and never drawn.
|
||||
|
||||
11. **`akgl_registry_load_properties` leaks one string-pool slot per failed property.**
|
||||
The per-property `CLEANUP` block is empty. The string pool is 256 slots, so a
|
||||
sufficiently malformed properties file drains it.
|
||||
|
||||
12. **`speedtime` is dead.** It is loaded from character JSON (`src/character.c:263`),
|
||||
written through `(int *)&obj->speedtime` on a `uint64_t` field — correct only because
|
||||
the struct was zeroed and the host is little-endian — and then read by nothing. Frame
|
||||
timing comes from `sprite->speed`.
|
||||
|
||||
13. **`AKGL_SPRITE_MAX_REGISTRY_SIZE` is dead.** Defined in `sprite.h`, referenced
|
||||
nowhere in `src/`, `include/`, `tests/` or `util/`.
|
||||
|
||||
14. **`AKGL_TILEMAP_MAX_TILES_PER_IMAGE` is checked nowhere**, and costs 512 KiB per
|
||||
tileset regardless of the image's real tile count.
|
||||
|
||||
15. **There is no sound-effect API.** `akgl_audio_*` is a synthesizer that reads no files;
|
||||
`akgl_load_start_bgm` is the only file-audio entry point, and its infinite-loop
|
||||
request is set on property set 0, so background music plays once. A game cannot load
|
||||
and play a sound effect through this library at all.
|
||||
|
||||
16. **A literal 512x512 tilemap is rejected.** Both bounds are `>=`, so the documented
|
||||
`AKGL_TILEMAP_MAX_WIDTH`/`_HEIGHT` of 512 is off by one and 512x511 is the largest
|
||||
map that loads.
|
||||
|
||||
17. **Unverified asset provenance.** `tests/assets/World_A1.png` and
|
||||
`util/assets/Actor1.png` carry the default filenames of RPG Maker's bundled art and
|
||||
ship with no license file, while `tests/assets/akgl_test_mono.ttf` sits beside
|
||||
`akgl_test_mono.LICENSE.txt`. RPG Maker's bundled assets are licensed to users of that
|
||||
product; redistributing them inside a C library is not something that license covers.
|
||||
The tutorial assets under `docs/tutorials/assets/` deliberately do not depend on
|
||||
either file, and `tests/docs_setups/tilemap.sh` says why it stages a different image.
|
||||
Closing this means replacing two fixtures and the maps that reference them.
|
||||
|
||||
18. **`util/assets/littleguy.json` does not load.** The sample data for `charviewer`, the
|
||||
one demo program the library ships, uses the pre-0.5.0 unprefixed state names
|
||||
(`ACTOR_STATE_ALIVE`) and `velocity_x`/`velocity_y` instead of `speed_x`/`speed_y`.
|
||||
The current loader accepts neither.
|
||||
|
||||
### Header comments that describe code that has changed
|
||||
|
||||
Twenty-seven claims across the public headers were false when checked against `src/`.
|
||||
`AGENTS.md` already warns that this file "carried eleven entries describing code that had
|
||||
already changed"; this is the same failure in the headers, and Doxygen publishes it.
|
||||
|
||||
Nothing catches these. `WARN_IF_UNDOCUMENTED` proves a symbol *has* a comment, not that
|
||||
the comment is true, and `api_surface` strips comments precisely because prose is not a
|
||||
declaration. The full list is in the manual, each noted in the chapter that covers the
|
||||
subsystem, but the ones that would actively mislead a caller are:
|
||||
|
||||
- `physics.h:8-9,195` — the `physics.engine` property and `akgl_game_init` calling the
|
||||
factory. Neither exists. See defect 1 above for what this costs.
|
||||
- `README.md` (now corrected) — "ONLY supports TilED TMJ tilemaps with tileset **external**
|
||||
references". Backwards: `"source"` appears nowhere in `src/tilemap.c`, and
|
||||
`akgl_tilemap_load_tilesets_each` reads `columns`/`firstgid`/`tilecount`/`image` inline.
|
||||
Only **embedded** tilesets load.
|
||||
- `renderer.h` — `frame_start`/`frame_end`/`draw_texture` "dereference `self` before it is
|
||||
checked". Each function's first statement is `FAIL_ZERO_RETURN(errctx, self, ...)`.
|
||||
- `sprite.h` — `speed` is "seconds, scaled to milliseconds". It is milliseconds scaled to
|
||||
nanoseconds. Also claims `frames` is unbounded (bounded at `src/sprite.c:207`) and that
|
||||
`akgl_sprite_initialize` overreads via `memcpy` (it uses `aksl_strncpy`).
|
||||
- `character.h` — `speedtime` "in seconds" (milliseconds), and `sprite_add` never releasing
|
||||
a displaced sprite (`src/character.c:60,77-79` releases it).
|
||||
- `actor.h` — the `cmhf` block comment says the `_off` handlers zero acceleration, thrust,
|
||||
environmental *and* velocity. They zero only `ax`/`tx` or `ay`/`ty`; zeroing `ey` was the
|
||||
gravity-cancel defect fixed in 0.6.0, and two `@note`s still describe it.
|
||||
- `json_helpers.h` — the conventions block says `dest` is not NULL-checked and that only
|
||||
`akgl_get_json_string_value` checks its key. All eleven accessors check `dest`, and all
|
||||
seven key-taking accessors check `key`.
|
||||
- `assets.h:17-18` — "`akgl_game_init` (or a bare `akgl_audio_init`) has to have run
|
||||
first". `akgl_mixer` is created only in `akgl_game_init`; `akgl_audio_init` opens the
|
||||
synthesizer's stream and never touches it, so `akgl_load_start_bgm` after only that
|
||||
hands NULL to `MIX_LoadAudio`.
|
||||
- `registry.h:54` — `akgl_registry_init` creating seven registries and not properties. It
|
||||
creates eight including properties; the genuinely false part is that `akgl_game_init`
|
||||
never calls it at all, calling the eight individually in a different order.
|
||||
- `tilemap.h:59-60,354-357,447-450,462-468` — object and tileset counts unbounded, and
|
||||
`akgl_tilemap_release` double-freeing. All fixed; the surviving half of the last one is
|
||||
that `release` does not release the map's actors.
|
||||
- `controller.h:232-234,262-263` — a negative `controlmapid` not rejected. Both call sites
|
||||
check.
|
||||
|
||||
### Entries in this file that are themselves stale
|
||||
|
||||
- **"Known and still open" item 15** — `akgl_path_relative` leaking an error-context slot
|
||||
per ENOENT call — is fixed. `src/util.c:115-129` sets a flag in the `HANDLE` block and
|
||||
calls `path_relative_root` after `FINISH`, which is exactly the fix the item proposes.
|
||||
- **`TODO.md:2027`**, under "akgl.pc names no dependencies", says "`akglConfig.cmake`
|
||||
re-finds the targets". No such file is generated or installed — `install()` ships the
|
||||
library, the headers and `akgl.pc` only, so `find_package(akgl)` cannot work against an
|
||||
install tree at all. That makes the section's problem worse than it states.
|
||||
- **`AGENTS.md:636`** says `sim_step()` sets `gravity_time` to `now - dt`.
|
||||
`tests/physics_sim.c` sets it to `0` and drives the step through `max_timestep`; the
|
||||
`now - dt` form was replaced because it went red under parallel `ctest`.
|
||||
|
||||
Reference in New Issue
Block a user