From 913834a3af6bd40b756ea59adb50142a7b91a2cb Mon Sep 17 00:00:00 2001 From: Andrew Kesterson Date: Sat, 1 Aug 2026 06:45:34 -0400 Subject: [PATCH] Draw actors at their sprite's height, and update each one once a frame Closes Defects item 26 and Performance item 32. akgl_actor_render set dest.h from curSprite->width, so every actor was drawn square and a non-square sprite was stretched or squashed. Invisible in the fixtures because they are all square, so tests/actor.c gets a 48x24 sprite and a render backend whose draw_texture records the rectangle it is handed instead of drawing it. That recording backend is the first coverage akgl_actor_render has had at all -- every other test in the file stubs renderfunc out. akgl_game_update looped over AKGL_TILEMAP_MAX_LAYERS with the actor sweep nested inside it and never compared an actor's layer to the layer it was on, so every live actor's updatefunc ran sixteen times a frame. The sweep is hoisted out: updating an actor is not a per-layer operation, and akgl_render_2d_draw_world already walks the layers for the half that is. AKGL_ITERATOR_OP_LAYERMASK is honoured rather than ignored now, so a caller who wants one layer can still ask, and gets each of those actors once. Counting is the assertion, deliberately. The defect was invisible in the frame total because the tilemap blits are three orders of magnitude larger, so a timing test would have measured the rasterizer. tests/game.c counts calls into a stub updatefunc and reports 16 against the old code. PERFORMANCE.md records the timing side as what it honestly is: the gap between the akgl_game_update and draw_world rows of the same run, 92 us before and noise in both directions after. The absolute table is not re-taken -- a later run on this machine read every row about 15% high, including rows nothing has touched. 25/25 pass, reindent --check clean. Co-Authored-By: Claude Opus 5 (1M context) --- PERFORMANCE.md | 38 +++++++++--- TODO.md | 43 ++++++++------ include/akgl/game.h | 27 +++++---- src/actor.c | 2 +- src/game.c | 44 ++++++++------ tests/actor.c | 134 ++++++++++++++++++++++++++++++++++++++++++ tests/game.c | 139 ++++++++++++++++++++++++++++++++++++++++++++ tests/perf_render.c | 12 ++-- 8 files changed, 379 insertions(+), 60 deletions(-) diff --git a/PERFORMANCE.md b/PERFORMANCE.md index 38c06a8..b281d91 100644 --- a/PERFORMANCE.md +++ b/PERFORMANCE.md @@ -167,6 +167,14 @@ rows are raw `SDL_RenderTexture` loops with no libakgl in the path. | `draw_world`, 1200 tiles + 64 actors | frame | 16,484,493.6 | 61 | | `akgl_game_update`, full frame | frame | 16,576,902.2 | 60 | +The `akgl_game_update` row is from before the sixteen-times-per-actor defect was +fixed in 0.5.0; see below. It sat about 92 µs above `draw_world` in the same +run, and now sits level with it. The absolute figures in this table have not +been re-taken -- a later run on this machine read every row about 15% high, +including rows nothing has touched, so the table is a record of one machine +state and the honest measurement of that fix is the *gap* between two rows of +the same run, not a new absolute. + ## The frame budget At 60 fps a frame is 16.67 ms. Here is where it goes for a 640x480 game with a @@ -247,16 +255,30 @@ The design conclusion is not "make errors cheaper". It is that a *routine* condition should not be reported as an error. A character that has no sprite for a state should answer that question with a boolean. -### `akgl_game_update` updates every actor sixteen times +### `akgl_game_update` updated every actor sixteen times — fixed in 0.5.0 -`src/game.c:617` loops over `AKGL_TILEMAP_MAX_LAYERS`, and the actor sweep -nested inside it does not filter by layer. Every live actor's `updatefunc` runs -**16 times per frame**. At 68.4 ns per update and 64 actors that is 70 µs of -work to do 4.4 µs of work. +`akgl_game_update` looped over `AKGL_TILEMAP_MAX_LAYERS` with the actor sweep +nested inside it, and never compared an actor's `layer` to the layer it was on. +Every live actor's `updatefunc` ran **16 times per frame**: at 68.4 ns per +update and 64 actors, 70 µs of work to do 4.4 µs of it — and, more to the +point, every piece of a game's own per-frame actor logic running sixteen times. -It is invisible in the frame total here because the tilemap blits are three -orders of magnitude larger. On a GPU backend, where the frame might be 2 ms, it -is 3.5% of the frame doing nothing. Filed in `TODO.md`. +The sweep is hoisted out of the layer loop. Updating an actor is not a per-layer +operation; drawing is, and `akgl_render_2d_draw_world` walks the layers itself. +`AKGL_ITERATOR_OP_LAYERMASK` is honoured now rather than ignored, so a caller +that genuinely wants one layer can still ask for it and gets each of those +actors once. + +**How it was measured.** `tests/game.c` counts calls into a stub `updatefunc` +and asserts exactly one per live actor per `akgl_game_update`; against the old +code it reports 16. The timing side is the gap between the `akgl_game_update` +and `draw_world` rows of the *same* benchmark run: 92 µs before, and across +three runs afterwards -414 µs, -376 µs and +19 µs — noise in both directions. + +It was invisible in the frame total because the tilemap blits are three orders +of magnitude larger, which is exactly why it needed a counting test rather than +a stopwatch. On a GPU backend, where a frame might be 2 ms, it was 3.5% of the +frame doing nothing. ### Text has no cache at all diff --git a/TODO.md b/TODO.md index d9c896e..3c9f57d 100644 --- a/TODO.md +++ b/TODO.md @@ -975,13 +975,15 @@ without coming here first. Ordered by blast radius. `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. + **Fixed in 0.5.0**: `dest.h` takes `curSprite->height`. Every actor was + drawn square, so a non-square sprite was stretched or squashed -- invisible + in the fixtures because they are all square. - Fix: one word. Touches `src/actor.c:276`. Worth a test that renders a - deliberately non-square sprite and asserts on the destination rectangle. + `tests/actor.c` renders a 48x24 sprite through a backend whose + `draw_texture` records the rectangle it is handed rather than drawing it, + and asserts the destination is 48x24 and 96x48 at scale 2. That recording + backend is also the first coverage `akgl_actor_render` has had at all -- + every other test in the file stubs `renderfunc` out. ### Found while closing the akbasic API gaps @@ -1095,18 +1097,25 @@ Ordered by blast radius. Numbering continues the **Defects** list above. workaround and no longer has to. 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. + **Fixed in 0.5.0.** The sweep is hoisted out of the layer loop -- updating + an actor is not a per-layer operation, and `akgl_render_2d_draw_world` + already walks the layers for the half that is. - 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`. + `AKGL_ITERATOR_OP_LAYERMASK` is honoured rather than ignored now, so a + caller that genuinely wants one layer can ask for it and gets each of those + actors once. It is no longer in the default flag set: every live actor once + is the job, and restricting the sweep is the caller asking for less. + + `tests/game.c` counts calls into a stub `updatefunc` and asserts exactly one + per live actor per frame, two over two frames, and the layer mask selecting + only its own layer. Against the old code it reports 16. + + Counting is the assertion on purpose. The defect was invisible in the frame + total because the tilemap blits are three orders of magnitude larger, so a + timing test would have measured the rasterizer. The timing evidence is the + gap between the `akgl_game_update` and `draw_world` rows of the same + benchmark run: 92 us before, and noise in both directions after. See + `PERFORMANCE.md`. 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 diff --git a/include/akgl/game.h b/include/akgl/game.h index c0aa210..ea199a2 100644 --- a/include/akgl/game.h +++ b/include/akgl/game.h @@ -282,17 +282,24 @@ akerr_ErrorContext AKERR_NOIGNORE *akgl_game_state_unlock(void); /** * @brief One frame: update every actor layer by layer, step the physics, draw the world. * - * Takes the state lock, counts the frame, then walks layers 0 through - * #AKGL_TILEMAP_MAX_LAYERS calling each live actor's `updatefunc`, optionally - * rescaling it to the tilemap first. Then it steps `akgl_physics` and draws through - * `akgl_renderer`, and releases the lock. + * Takes the state lock, counts the frame, then calls each live actor's + * `updatefunc` **exactly once**, optionally rescaling it to the tilemap first. + * Then it steps `akgl_physics` and draws through `akgl_renderer`, and releases + * the lock. * - * @param opflags Iterator flags. Optional -- `NULL` selects a default set that - * sweeps one layer at a time, in which case `layerid` is advanced - * by the loop. Only #AKGL_ITERATOR_OP_TILEMAPSCALE is read here; - * with it clear every actor's `scale` is forced to 1.0. Note that - * the flags are *not* forwarded to the physics or render calls, - * both of which are passed `NULL`. + * Until 0.5.0 the sweep sat inside a walk over #AKGL_TILEMAP_MAX_LAYERS that + * never compared an actor's `layer` to the layer it was on, so every actor + * updated sixteen times a frame. Updating an actor is not a per-layer + * operation; drawing is, and akgl_render_2d_draw_world walks the layers itself. + * + * @param opflags Iterator flags. Optional -- `NULL` selects + * #AKGL_ITERATOR_OP_UPDATE alone, which is every live actor + * once. Two bits are read here: + * #AKGL_ITERATOR_OP_TILEMAPSCALE, without which every actor's + * `scale` is forced to 1.0; and #AKGL_ITERATOR_OP_LAYERMASK, + * which restricts the sweep to the actors whose `layer` matches + * `layerid`. Note that the flags are *not* forwarded to the + * physics or render calls, both of which are passed `NULL`. * @return `NULL` on success, otherwise an error context owned by the caller. * @throws AKGL_ERR_SDL If the state lock cannot be taken. * @throws AKERR_* Whatever an actor's `updatefunc`, akgl_tilemap_scale_actor, diff --git a/src/actor.c b/src/actor.c index 09cdd95..4463800 100644 --- a/src/actor.c +++ b/src/actor.c @@ -273,7 +273,7 @@ akerr_ErrorContext *akgl_actor_render(akgl_Actor *obj) dest.y = (obj->y - akgl_camera->y); } dest.w = curSprite->width * obj->scale; - dest.h = curSprite->width * obj->scale; + dest.h = curSprite->height * obj->scale; PASS(errctx, akgl_renderer->draw_texture(akgl_renderer, curSprite->sheet->texture, &src, &dest, 0, NULL, SDL_FLIP_NONE)); SUCCEED_RETURN(errctx); diff --git a/src/game.c b/src/game.c index b47803d..1347154 100644 --- a/src/game.c +++ b/src/game.c @@ -643,12 +643,11 @@ akerr_ErrorContext *akgl_game_load(char *fpath) akerr_ErrorContext *akgl_game_update(akgl_Iterator *opflags) { PREPARE_ERROR(errctx); + // No AKGL_ITERATOR_OP_LAYERMASK by default: updating every actor once is + // the whole job, and restricting the sweep to one layer is the caller + // deliberately asking for less. akgl_Iterator defflags = { - // Was (LAYERMASK | LAYERMASK). Nothing in the loop below reads either - // bit today, so this is a statement of intent rather than a behaviour - // change -- and the reason the loop ignores LAYERMASK is TODO.md - // Performance item 32, which is still open. - .flags = (AKGL_ITERATOR_OP_UPDATE | AKGL_ITERATOR_OP_LAYERMASK), + .flags = AKGL_ITERATOR_OP_UPDATE, .layerid = 0 }; akgl_Actor *actor = NULL; @@ -661,22 +660,29 @@ akerr_ErrorContext *akgl_game_update(akgl_Iterator *opflags) akgl_game_update_fps(); - for ( int i = 0; i < AKGL_TILEMAP_MAX_LAYERS; i++ ) { - if ( opflags == &defflags ) { - opflags->layerid = i; + // One sweep, not one per layer. This loop used to sit inside a walk over + // AKGL_TILEMAP_MAX_LAYERS that never compared actor->layer to the layer it + // was on, so every live actor's updatefunc ran sixteen times a frame -- + // measured at 68.4 ns per update and 64 actors, 70 us of work to do 4.4 us + // of it. Updating an actor is not a per-layer operation. Drawing is, and + // akgl_render_2d_draw_world walks the layers itself. + for ( int j = 0; j < AKGL_MAX_HEAP_ACTOR; j++ ) { + actor = &akgl_heap_actors[j]; + if ( actor->refcount == 0 ) { + continue; } - for ( int j = 0; j < AKGL_MAX_HEAP_ACTOR; j++ ) { - actor = &akgl_heap_actors[j]; - if ( actor->refcount == 0 ) { - continue; - } - if ( AKGL_BITMASK_HAS(opflags->flags, AKGL_ITERATOR_OP_TILEMAPSCALE) ) { - PASS(errctx, akgl_tilemap_scale_actor(akgl_gamemap, actor)); - } else { - actor->scale = 1.0; - } - PASS(errctx, actor->updatefunc(actor)); + // The flag is honoured now rather than ignored. A caller that sets it + // gets exactly the actors on opflags->layerid, once each. + if ( AKGL_BITMASK_HAS(opflags->flags, AKGL_ITERATOR_OP_LAYERMASK) && + (actor->layer != opflags->layerid) ) { + continue; } + if ( AKGL_BITMASK_HAS(opflags->flags, AKGL_ITERATOR_OP_TILEMAPSCALE) ) { + PASS(errctx, akgl_tilemap_scale_actor(akgl_gamemap, actor)); + } else { + actor->scale = 1.0; + } + PASS(errctx, actor->updatefunc(actor)); } PASS(errctx, akgl_physics->simulate(akgl_physics, NULL)); PASS(errctx, akgl_renderer->draw_world(akgl_renderer, NULL)); diff --git a/tests/actor.c b/tests/actor.c index 51a0435..8df9b14 100644 --- a/tests/actor.c +++ b/tests/actor.c @@ -17,6 +17,7 @@ #include #include #include +#include #include "testutil.h" @@ -892,6 +893,122 @@ akerr_ErrorContext *test_actor_sprite_sheet_coords(void) SUCCEED_RETURN(e); } +/* + * A render backend whose draw_texture records what it was asked to draw instead + * of drawing it. akgl_actor_render's whole output is the rectangle it hands to + * draw_texture, so that rectangle is the only thing there is to assert on. + */ +static akgl_RenderBackend testbackend; +static SDL_FRect lastdrawdest; +static SDL_FRect lastdrawsrc; +static int drawcalls = 0; + +static akerr_ErrorContext *record_draw_texture( + akgl_RenderBackend *self, + SDL_Texture *texture, + SDL_FRect *src, + SDL_FRect *dest, + double angle, + SDL_FPoint *center, + SDL_FlipMode flip) +{ + PREPARE_ERROR(errctx); + + if ( src != NULL ) { + lastdrawsrc = *src; + } + if ( dest != NULL ) { + lastdrawdest = *dest; + } + drawcalls += 1; + SUCCEED_RETURN(errctx); +} + +/** + * @brief akgl_actor_render must take the drawn height from the sprite's height. + * + * It read `curSprite->width` for both, so every actor was drawn square and a + * non-square sprite was stretched or squashed. Invisible in the fixtures + * because they are square, which is exactly why this test uses a sprite that + * is not: 48 wide by 24 high, so a regression puts 48 in both. + * + * This is also the first test of akgl_actor_render at all -- everything else in + * this file stubs renderfunc out. + */ +akerr_ErrorContext *test_actor_render_uses_sprite_height(void) +{ + PREPARE_ERROR(e); + akgl_Actor *actor = NULL; + akgl_Character *basechar = NULL; + akgl_Sprite *sprite = NULL; + SDL_Texture *sheettexture = NULL; + + ATTEMPT { + CATCH(e, make_bound_actor(&actor, &basechar, &sprite, "renderactor", AKGL_ACTOR_STATE_ALIVE)); + + // akgl_spritesheet_coords_for_frame reads sheet->texture->w, so the + // sheet needs a real one even though nothing is rasterized. + sheettexture = SDL_CreateTexture( + testbackend.sdl_renderer, + SDL_PIXELFORMAT_RGBA8888, + SDL_TEXTUREACCESS_TARGET, + 96, 48); + FAIL_ZERO_BREAK(e, sheettexture, AKGL_ERR_SDL, "%s", SDL_GetError()); + sprite->sheet->texture = sheettexture; + + sprite->width = 48; + sprite->height = 24; + sprite->frames = 2; + actor->curSpriteFrameId = 0; + actor->visible = true; + actor->scale = 1.0f; + actor->x = 0; + actor->y = 0; + + akgl_camera->x = 0; + akgl_camera->y = 0; + akgl_camera->w = 640; + akgl_camera->h = 480; + + drawcalls = 0; + TEST_EXPECT_OK(e, akgl_actor_render(actor), "rendering a non-square actor"); + TEST_ASSERT(e, drawcalls == 1, "actor render made %d draw calls, expected 1", drawcalls); + + TEST_ASSERT_FEQ(e, lastdrawdest.w, 48.0f, + "drawn width is %f, expected the sprite's 48", lastdrawdest.w); + TEST_ASSERT_FEQ(e, lastdrawdest.h, 24.0f, + "drawn height is %f, expected the sprite's 24 -- it used to take the width", + lastdrawdest.h); + + // The source rectangle has always come from the sprite properly; assert + // it too, so a regression cannot move the bug one line up. + TEST_ASSERT_FEQ(e, lastdrawsrc.w, 48.0f, "source width is %f, expected 48", lastdrawsrc.w); + TEST_ASSERT_FEQ(e, lastdrawsrc.h, 24.0f, "source height is %f, expected 24", lastdrawsrc.h); + + // Scale multiplies both, and must not collapse them back together. + actor->scale = 2.0f; + drawcalls = 0; + TEST_EXPECT_OK(e, akgl_actor_render(actor), "rendering a scaled non-square actor"); + TEST_ASSERT_FEQ(e, lastdrawdest.w, 96.0f, "scaled width is %f, expected 96", lastdrawdest.w); + TEST_ASSERT_FEQ(e, lastdrawdest.h, 48.0f, "scaled height is %f, expected 48", lastdrawdest.h); + } CLEANUP { + if ( sprite != NULL && sprite->sheet != NULL ) { + sprite->sheet->texture = NULL; + } + if ( sheettexture != NULL ) { + SDL_DestroyTexture(sheettexture); + } + if ( actor != NULL ) { + IGNORE(akgl_heap_release_actor(actor)); + } + if ( basechar != NULL ) { + IGNORE(akgl_heap_release_character(basechar)); + } + } PROCESS(e) { + } FINISH(e, true); + SUCCEED_RETURN(e); +} + int main(void) { akgl_actor_updated = 0; @@ -910,6 +1027,22 @@ int main(void) CATCH(errctx, akgl_registry_init_spritesheet()); CATCH(errctx, akgl_registry_init_character()); + // A renderer, so akgl_actor_render can be driven at all. The backend is + // bound and then has its draw_texture replaced, which is the point: + // what the function computes is only visible in what it hands over. + if ( !SDL_Init(SDL_INIT_VIDEO) ) { + FAIL_BREAK(errctx, AKGL_ERR_SDL, "Couldn't initialize SDL: %s", SDL_GetError()); + } + memset(&testbackend, 0x00, sizeof(akgl_RenderBackend)); + if ( !SDL_CreateWindowAndRenderer("net/aklabs/libakgl/test_actor", 640, 480, + SDL_WINDOW_HIDDEN, &akgl_window, &testbackend.sdl_renderer) ) { + FAIL_BREAK(errctx, AKGL_ERR_SDL, "Couldn't create window/renderer: %s", SDL_GetError()); + } + CATCH(errctx, akgl_render_2d_bind(&testbackend)); + testbackend.draw_texture = &record_draw_texture; + akgl_renderer = &testbackend; + akgl_camera = &akgl_default_camera; + CATCH(errctx, test_registry_actor_iterator_nullpointers()); CATCH(errctx, test_registry_actor_iterator_missingactor()); CATCH(errctx, test_registry_actor_iterator_updaterender()); @@ -925,6 +1058,7 @@ int main(void) CATCH(errctx, test_actor_update()); CATCH(errctx, test_actor_character_sprite_binding()); CATCH(errctx, test_actor_sprite_sheet_coords()); + CATCH(errctx, test_actor_render_uses_sprite_height()); } CLEANUP { } PROCESS(errctx) { } FINISH_NORETURN(errctx); diff --git a/tests/game.c b/tests/game.c index 60a6003..fed01ab 100644 --- a/tests/game.c +++ b/tests/game.c @@ -21,6 +21,9 @@ #include #include #include +#include +#include +#include #include "testutil.h" @@ -510,6 +513,141 @@ akerr_ErrorContext *test_game_updateFPS(void) SUCCEED_RETURN(e); } +/** @brief Counts akgl_game_update calls into each actor's updatefunc, by actor index. */ +static int updatecounts[AKGL_MAX_HEAP_ACTOR]; + +/** @brief updatefunc stub: record that this actor was updated. */ +static akerr_ErrorContext *counting_updatefunc(akgl_Actor *obj) +{ + PREPARE_ERROR(e); + int i = 0; + + for ( i = 0; i < AKGL_MAX_HEAP_ACTOR; i++ ) { + if ( &akgl_heap_actors[i] == obj ) { + updatecounts[i] += 1; + break; + } + } + SUCCEED_RETURN(e); +} + +/** @brief Physics backend stub: akgl_game_update calls simulate, and it must not matter here. */ +static akerr_ErrorContext *stub_simulate(akgl_PhysicsBackend *self, akgl_Iterator *opflags) +{ + PREPARE_ERROR(e); + SUCCEED_RETURN(e); +} + +/** @brief Render backend stub: akgl_game_update calls draw_world; drawing is not under test. */ +static akerr_ErrorContext *stub_draw_world(akgl_RenderBackend *self, akgl_Iterator *opflags) +{ + PREPARE_ERROR(e); + SUCCEED_RETURN(e); +} + +/** + * @brief akgl_game_update must call each live actor's updatefunc exactly once. + * + * The sweep used to sit inside a walk over AKGL_TILEMAP_MAX_LAYERS and never + * compared actor->layer to the layer it was on, so every live actor updated + * sixteen times a frame -- 70 microseconds of work to do 4.4 of it, and every + * bit of per-frame actor logic running sixteen times over. It hid behind a + * software rasterizer and would not have hidden behind a GPU backend. + * + * Counting is the assertion. A timing test would measure the machine. + */ +akerr_ErrorContext *test_game_update_visits_each_actor_once(void) +{ + PREPARE_ERROR(e); + akgl_PhysicsBackend stubphysics; + akgl_RenderBackend stubrenderer; + akgl_Iterator opflags; + akgl_Actor *actors[3] = { NULL, NULL, NULL }; + int layers[3] = { 0, 1, 1 }; + int i = 0; + int live = 0; + + ATTEMPT { + set_game_identity(); + akgl_game.statelock = SDL_CreateMutex(); + FAIL_ZERO_BREAK(e, akgl_game.statelock, AKGL_ERR_SDL, "unable to create the state mutex"); + akgl_game.lowfpsfunc = &akgl_game_lowfps; + + CATCH(e, akgl_heap_init()); + CATCH(e, akgl_registry_init_actor()); + + memset(&stubphysics, 0x00, sizeof(akgl_PhysicsBackend)); + memset(&stubrenderer, 0x00, sizeof(akgl_RenderBackend)); + stubphysics.simulate = &stub_simulate; + stubrenderer.draw_world = &stub_draw_world; + akgl_physics = &stubphysics; + akgl_renderer = &stubrenderer; + akgl_gamemap = &akgl_default_gamemap; + + for ( i = 0; i < 3; i++ ) { + char name[32]; + snprintf((char *)&name, sizeof(name), "sweepactor%d", i); + CATCH(e, akgl_heap_next_actor(&actors[i])); + CATCH(e, akgl_actor_initialize(actors[i], (char *)&name)); + actors[i]->updatefunc = &counting_updatefunc; + actors[i]->layer = layers[i]; + } + + // The default sweep: every live actor, once. + memset(&updatecounts, 0x00, sizeof(updatecounts)); + TEST_EXPECT_OK(e, akgl_game_update(NULL), "one default game update"); + live = 0; + for ( i = 0; i < AKGL_MAX_HEAP_ACTOR; i++ ) { + if ( akgl_heap_actors[i].refcount == 0 ) { + TEST_ASSERT(e, updatecounts[i] == 0, + "a free actor slot %d was updated %d times", i, updatecounts[i]); + continue; + } + live += 1; + TEST_ASSERT(e, updatecounts[i] == 1, + "actor %d updated %d times in one frame, expected 1", + i, updatecounts[i]); + } + TEST_ASSERT(e, live == 3, "%d live actors, expected 3", live); + + // Two frames means two updates, not thirty-two. + memset(&updatecounts, 0x00, sizeof(updatecounts)); + TEST_EXPECT_OK(e, akgl_game_update(NULL), "the second game update"); + TEST_EXPECT_OK(e, akgl_game_update(NULL), "the third game update"); + for ( i = 0; i < 3; i++ ) { + TEST_ASSERT(e, updatecounts[i] == 2, + "actor %d updated %d times over two frames, expected 2", + i, updatecounts[i]); + } + + // AKGL_ITERATOR_OP_LAYERMASK now means what it says: only layer 1. + memset(&updatecounts, 0x00, sizeof(updatecounts)); + AKGL_BITMASK_CLEAR(opflags.flags); + AKGL_BITMASK_ADD(opflags.flags, AKGL_ITERATOR_OP_UPDATE); + AKGL_BITMASK_ADD(opflags.flags, AKGL_ITERATOR_OP_LAYERMASK); + opflags.layerid = 1; + TEST_EXPECT_OK(e, akgl_game_update(&opflags), "a layer-masked game update"); + TEST_ASSERT(e, updatecounts[0] == 0, + "the layer 0 actor updated %d times under a layer 1 mask", updatecounts[0]); + TEST_ASSERT(e, updatecounts[1] == 1, + "the first layer 1 actor updated %d times, expected 1", updatecounts[1]); + TEST_ASSERT(e, updatecounts[2] == 1, + "the second layer 1 actor updated %d times, expected 1", updatecounts[2]); + } CLEANUP { + for ( i = 0; i < 3; i++ ) { + if ( actors[i] != NULL ) { + IGNORE(akgl_heap_release_actor(actors[i])); + } + } + if ( akgl_game.statelock != NULL ) { + SDL_DestroyMutex(akgl_game.statelock); + akgl_game.statelock = NULL; + } + } PROCESS(e) { + } FINISH(e, true); + SUCCEED_RETURN(e); +} + int main(void) { PREPARE_ERROR(errctx); @@ -535,6 +673,7 @@ int main(void) CATCH(errctx, test_game_state_lock_budget()); CATCH(errctx, test_game_updateFPS()); CATCH(errctx, test_game_updateFPS_without_a_lowfps_handler()); + CATCH(errctx, test_game_update_visits_each_actor_once()); } CLEANUP { } PROCESS(errctx) { } FINISH_NORETURN(errctx); diff --git a/tests/perf_render.c b/tests/perf_render.c index cc2cb2a..6d6271c 100644 --- a/tests/perf_render.c +++ b/tests/perf_render.c @@ -712,11 +712,13 @@ static akerr_ErrorContext *bench_scene(void) * @brief Time a whole frame the way akgl_game_update runs one. * * This is the number a host actually gets, and it is not the sum of the parts. - * akgl_game_update's update sweep is nested inside a loop over - * #AKGL_TILEMAP_MAX_LAYERS and does not filter by layer, so every live actor is - * updated sixteen times per frame rather than once. The gap between this - * benchmark and the scene benchmark plus the logic frame in tests/perf.c is that - * multiplier, and TODO.md carries it as a defect. + * + * It used to be measurably worse than that: the update sweep was nested inside + * a loop over #AKGL_TILEMAP_MAX_LAYERS that never filtered by layer, so every + * live actor updated sixteen times a frame, and this benchmark sat about 92 us + * above the scene benchmark below it. That gap *was* the multiplier. Since + * 0.5.0 the two are equal within run-to-run noise, which is the shape of the + * fix rather than a number worth quoting on its own. */ static akerr_ErrorContext *bench_game_update(void) {