diff --git a/TODO.md b/TODO.md index 159dd11..8a33eff 100644 --- a/TODO.md +++ b/TODO.md @@ -844,38 +844,53 @@ deviations from the reference's *program*: `main.go` and the SDL half of exactly, and the whole golden corpus is driven through this loop to prove it. What it changes for a *user* is that the window closes when asked. -26. **The frame is not cleared, but the text layer erases its own rows.** The reference blits a - text surface onto the window surface and calls `UpdateSurface`. Here the graphics verbs draw - straight to the renderer rather than into a display list, so a `SDL_RenderClear` at the top - of each frame would erase every `DRAW` at the end of the frame it was issued in. The host - therefore does not clear. +26. **The text layer repaints every row it owns, every frame.** Not just the rows that changed. - **The sink erases row by row instead**, filling each row it is about to draw — and each row - that has emptied since it last drew — with its background colour first. That makes the text - layer authoritative over the rows it occupies and leaves every other pixel alone, which is - how a C128's text plane sits over its bitmap. + That was tried — a `drawn[]` array marking which rows had carried glyphs, so an untouched + row could be skipped — and it is **wrong on real hardware**. `SDL_RenderPresent` swaps + buffers, so a frame does not inherit the frame before it; it inherits the one *two* back, + and on some backends something undefined. A row erased once is clean in one buffer and still + dirty in the other, and presenting alternates between them. - **This was originally "the text layer is drawn over whatever is already there", and that was - a bug rather than a deviation.** The grid was always right and the screen kept the previous - frame, which surfaced as three separate-looking faults reported together: a backspaced - character that stayed on screen, a scroll that appeared to overwrite the display with - garbage, and the editing cursor smearing an underscore along every column it had passed - through. One cause, three symptoms. `tests/akgl_frontend.c` asserts the fix at pixel level — - grid assertions could never have caught it, because the grid was never wrong. + That is what a backspace across a line wrap looked like: the first character of the wrapped + tail flickering in and out forever after the rest had gone. It reproduces on X11 and never + under the dummy driver or a software renderer, which is exactly why the suite was green + through two rounds of fixing it. `tests/akgl_frontend.c` now paints stale pixels by hand — + which is what a swapped-in buffer hands back — so the case is covered without needing real + hardware. - **What it still costs:** a program that draws a picture and then prints loses the strip of - picture behind each text row it uses. That is the deliberate half of the trade — the - alternative was clearing the whole frame, which loses the picture entirely. + There is no cheaper correct answer available: a frame either owns every pixel it presents or + it inherits pixels it cannot reason about. -27. **There is no cursor.** The reference draws one as a literal underscore glyph (`drawCursor`, - `basicruntime_graphics.go:33`) and this did too. Removed on request: it sat under the text - being typed and made it hard to read. + **What this costs, and it is more than it used to.** The host still does not call + `SDL_RenderClear`, but the text area is now repainted in full every frame, so anything a + graphics verb drew underneath it is erased every frame rather than only where text lands. In + the standalone driver the text area is the whole window, so `DRAW` and `PRINT` no longer + coexist there. - Worth separating the two reasons it was unbearable, because only one of them is gone. It - *smeared* — an underscore left at every column the cursor passed through — and that was - deviation 26's missing erase, now fixed. It also simply sits under the text, which is what - an underscore cursor does in a font whose glyphs reach the baseline. A cursor could come back - and behave now; a block or a different colour would read better than an underscore. + **And the same buffer swap means the graphics verbs were never reliable across frames + anyway**, which is worth stating plainly rather than leaving as a surprise: a one-shot `DRAW` + lands in one buffer and the next present shows the other. It looked fine in a single + screenshot and would have flickered exactly as the text did. Making graphics survive needs a + persistent surface the frame is composited from, which is real work and its own commit — + filed here rather than half-done. + +27. **The cursor is a blinking block.** The reference draws an underscore glyph (`drawCursor`, + `basicruntime_graphics.go:33`), and so did this until the underscore turned out to sit + *under* the text being typed and make it hard to read. A block occupies the cell after the + text instead of the space beneath it. + + Half a second per blink (`AKBASIC_SINK_CURSOR_BLINK_MS`), which is about a Commodore's and + slow enough to read under. `akbasic_AkglSink.cursorperiodms` set to zero holds it solid, + which is what makes a frame deterministic for a test. + + Two details that are decisions rather than accidents. The clock is **SDL's**, not the + host's: everywhere else the host owns the clock because the library owns no loop and must + not block, but this is an adaptor that already links SDL and threading a timestamp through + the sink interface to animate a cursor would be a poor trade. And a line that exactly fills + a row leaves the cursor one column past the end, because `putchar_at` wraps only when the + next character arrives — the cursor is drawn at the start of the next row instead, which is + where the next character will actually land and where a C128 puts it. 28. **The window is closed by the host, and that is not `QUIT`.** Closing the window stops the frontend and leaves `obj->mode` alone; it does not set `AKBASIC_MODE_QUIT`. The distinction diff --git a/include/akbasic/akgl.h b/include/akbasic/akgl.h index 3da923c..f17b781 100644 --- a/include/akbasic/akgl.h +++ b/include/akbasic/akgl.h @@ -52,6 +52,14 @@ #include #include +/** + * @brief One full cursor blink in milliseconds, half on and half off. + * + * Half a second, which is about what a Commodore does and is slow enough to read + * under. + */ +#define AKBASIC_SINK_CURSOR_BLINK_MS 500 + /** @brief How many saved SSHAPE regions the graphics backend will hold at once. */ #define AKBASIC_AKGL_MAX_SHAPES 16 @@ -113,12 +121,10 @@ typedef struct char text[64][256]; /** - * Which rows carried glyphs when this sink last rendered. A row that has - * emptied since -- scrolled away, cleared, or shortened -- still has to be - * erased, and after it is erased there is no longer anything in the grid to - * say it once had something in it. + * Milliseconds for one full cursor blink, half on and half off. Zero holds + * the cursor solid, which is what a test wanting a deterministic frame sets. */ - bool drawn[64]; + uint64_t cursorperiodms; /* * The line editor. Nothing here is live except while readline is running, diff --git a/src/sink_akgl.c b/src/sink_akgl.c index 96b5f62..12b5b08 100644 --- a/src/sink_akgl.c +++ b/src/sink_akgl.c @@ -417,6 +417,7 @@ akerr_ErrorContext *akbasic_sink_init_akgl(akbasic_TextSink *obj, akbasic_AkglSi state->color.b = 0xff; state->color.a = 0xff; /* Opaque, not transparent: the text layer owns the rows it draws. */ + state->cursorperiodms = AKBASIC_SINK_CURSOR_BLINK_MS; state->background.r = 0x00; state->background.g = 0x00; state->background.b = 0x00; @@ -457,23 +458,26 @@ akerr_ErrorContext *akbasic_sink_akgl_render(akbasic_TextSink *obj) "akgl sink has no state"); /* - * **Erase each row before drawing it.** Without this the sink paints glyphs - * on top of whatever the previous frame left, and the host deliberately does - * not clear the frame either -- §5 deviation 26, so that a DRAW survives - * past the frame it was issued in. The grid was always right and the screen - * kept the last frame, which showed up as three separate-looking bugs: a - * backspaced character that would not go away, a scroll that appeared to - * overwrite the screen with garbage, and the editing cursor smearing an - * underscore along every column it passed through. + * **Repaint every row of the text area, every frame.** Not just the rows + * that changed -- that was tried, with a `drawn[]` array marking which rows + * had carried glyphs, and it is wrong on real hardware. * - * Row by row rather than one clear over the whole text area, and that is the - * deliberate part: the text layer is authoritative over the rows it occupies - * and leaves every other pixel alone. A program that draws a picture and - * prints one line loses one line's strip of it, the way a C128's text plane - * sits over its bitmap -- rather than losing the picture. + * SDL_RenderPresent swaps buffers. What a frame inherits is not the frame + * before it, it is the frame *two* before it, and on some backends it is + * undefined. So a row erased once is erased in one buffer and still dirty in + * the other, and presenting alternates between them: the row blinks in and + * out forever. That is exactly what a backspace across a line wrap looked + * like -- the first character of the wrapped tail flickering after the rest + * had gone. It reproduces on X11 and never under the dummy driver or a + * software renderer, which is why the suite was green. * - * `drawn` is what makes a row that has *emptied* get erased. Once it is - * cleared there is nothing left in the grid to say it ever held anything. + * There is no cheaper correct answer available here. A frame either owns + * every pixel it presents or it inherits pixels it cannot reason about. + * + * Row by row rather than one fill over the whole area, still: the two are + * equivalent in cost here and the row loop keeps the text layer's ownership + * of exactly `rows * cellh` pixels obvious, which matters when the area is + * smaller than the window. * * A loop, so no CATCH and no _BREAK macros: they expand to a C break, which * would escape the loop with an error still pending. PASS only. @@ -483,20 +487,15 @@ akerr_ErrorContext *akbasic_sink_akgl_render(akbasic_TextSink *obj) * put characters somewhere the cursor does not think they are. */ for ( row = 0; row < state->rows; row++ ) { - bool hastext = (state->text[row][0] != '\0'); SDL_FRect cells; - if ( !hastext && !state->drawn[row] ) { - continue; - } cells.x = (float)state->x; cells.y = (float)(state->y + (row * state->cellh)); cells.w = (float)state->width; cells.h = (float)state->cellh; PASS(errctx, akgl_draw_filled_rect(state->renderer, &cells, state->background)); - state->drawn[row] = hastext; - if ( !hastext ) { + if ( state->text[row][0] == '\0' ) { continue; } PASS(errctx, akgl_text_rendertextat(state->font, state->text[row], @@ -506,13 +505,52 @@ akerr_ErrorContext *akbasic_sink_akgl_render(akbasic_TextSink *obj) } /* - * No cursor glyph. The reference draws one as a literal underscore - * (drawCursor, basicruntime_graphics.go:33) and this did too, until it turned - * out to sit *under* the text being typed and make it hard to read. Removed - * on request; §5 records it, and the smearing that made it unbearable is - * fixed by the erase above rather than by the removal -- so a cursor could - * come back and behave, if one is wanted. + * The cursor: a filled block at the cursor cell, blinking. + * + * A block rather than the reference's underscore glyph (drawCursor, + * basicruntime_graphics.go:33). The underscore sat *under* the text being + * typed and made it hard to read, which is what got it removed; a block + * occupies the cell after the text instead of the space beneath it. + * + * The clock is SDL's rather than the host's. Everywhere else in this + * interpreter the host owns the clock, because the library owns no loop and + * must not block -- but this is an adaptor that already links SDL, the + * blink is cosmetic, and threading a timestamp through the sink interface to + * animate a cursor would be a poor trade. `cursorperiodms` of zero holds it + * solid, which is what makes a frame deterministic for a test. */ + if ( state->editing ) { + SDL_FRect cell; + bool visible = true; + + if ( state->cursorperiodms > 0 ) { + visible = ((SDL_GetTicks() % state->cursorperiodms) < + (state->cursorperiodms / 2)); + } + if ( visible ) { + int curcol = state->cursorcol; + int currow = state->cursorrow; + + /* + * A line that exactly fills a row leaves the cursor one past the last + * column, because putchar_at only wraps when the *next* character + * arrives. Drawn there it would be off the right edge and invisible. + * Show it where the next character will actually land instead, which + * is also where a C128 puts it. + */ + if ( curcol >= state->columns ) { + curcol = 0; + currow += 1; + } + if ( currow < state->rows ) { + cell.x = (float)(state->x + (curcol * state->cellw)); + cell.y = (float)(state->y + (currow * state->cellh)); + cell.w = (float)state->cellw; + cell.h = (float)state->cellh; + PASS(errctx, akgl_draw_filled_rect(state->renderer, &cell, state->color)); + } + } + } SUCCEED_RETURN(errctx); } diff --git a/tests/akgl_frontend.c b/tests/akgl_frontend.c index 6217d3d..7bcc457 100644 --- a/tests/akgl_frontend.c +++ b/tests/akgl_frontend.c @@ -29,6 +29,7 @@ #include #include +#include #include #include @@ -666,7 +667,7 @@ static akerr_ErrorContext AKERR_NOIGNORE *test_backspace_across_a_wrap(void) * cursor had passed through, which read as an underline beneath the whole line * and made typed text hard to read. Removed outright; §5 records it. */ -static akerr_ErrorContext AKERR_NOIGNORE *test_no_cursor_underline(void) +static akerr_ErrorContext AKERR_NOIGNORE *test_block_cursor_blinks(void) { PREPARE_ERROR(errctx); SDL_Surface *shot = NULL; @@ -678,32 +679,119 @@ static akerr_ErrorContext AKERR_NOIGNORE *test_no_cursor_underline(void) PASS(errctx, start_frontend(NULL)); sink = &FRONTEND.akglstate; - /* - * Rendered *while editing*, which is the only state the cursor was ever - * drawn in -- an earlier version of this test rendered after readline had - * returned, by which time `editing` is false and a re-added cursor would - * sail straight through it. - */ PASS(errctx, FRONTEND.akglsink.write(&FRONTEND.akglsink, "ab")); sink->editing = true; - PASS(errctx, akbasic_sink_akgl_render(&FRONTEND.akglsink)); - /* - * The bottom scanline of each cell the text occupies and the one after it. - * An underscore lands there; "a" and "b" in this font do not, so anything - * lit on that line is the cursor and nothing else. - */ + /* Period zero holds it solid, so a frame is deterministic. */ + sink->cursorperiodms = 0; + PASS(errctx, akbasic_sink_akgl_render(&FRONTEND.akglsink)); shot = SDL_RenderReadPixels(FRONTEND.renderer->sdl_renderer, NULL); TEST_REQUIRE(shot != NULL, "could not read the render target back"); + + /* + * A *block*: every scanline of the cursor cell is lit, including the top + * one. An underscore lights only the bottom, and it was the underscore + * sitting under the text that made typed text hard to read. + */ + TEST_REQUIRE(anything_drawn(shot, sink->x + (2 * sink->cellw), sink->y, sink->cellw, 1), + "the cursor cell's top scanline should be lit: a block, not an underscore"); + TEST_REQUIRE(anything_drawn(shot, sink->x + (2 * sink->cellw), + sink->y + sink->cellh - 1, sink->cellw, 1), + "the cursor cell's bottom scanline should be lit too"); + + /* And nothing under the text itself, which is what was reported. */ y = sink->y + sink->cellh - 1; - for ( col = 0; col < 4; col++ ) { + for ( col = 0; col < 2; col++ ) { if ( anything_drawn(shot, sink->x + (col * sink->cellw), y, sink->cellw, 1) ) { lit += 1; } } TEST_REQUIRE_INT(lit, 0); SDL_DestroySurface(shot); + + /* + * It blinks. A short period and a wait past half of it: the same cell must + * come out differently, which is the whole of what blinking is. + */ + sink->cursorperiodms = 200; + while ( (SDL_GetTicks() % 200) >= 100 ) { + SDL_Delay(5); + } + PASS(errctx, akbasic_sink_akgl_render(&FRONTEND.akglsink)); + shot = SDL_RenderReadPixels(FRONTEND.renderer->sdl_renderer, NULL); + TEST_REQUIRE(anything_drawn(shot, sink->x + (2 * sink->cellw), sink->y, + sink->cellw, sink->cellh), + "the cursor should be visible in the first half of its period"); + SDL_DestroySurface(shot); + + while ( (SDL_GetTicks() % 200) < 100 ) { + SDL_Delay(5); + } + PASS(errctx, akbasic_sink_akgl_render(&FRONTEND.akglsink)); + shot = SDL_RenderReadPixels(FRONTEND.renderer->sdl_renderer, NULL); + TEST_REQUIRE(!anything_drawn(shot, sink->x + (2 * sink->cellw), sink->y, + sink->cellw, sink->cellh), + "the cursor should be hidden in the second half of its period"); + SDL_DestroySurface(shot); + sink->editing = false; + sink->cursorperiodms = 0; + stop_frontend(); + SUCCEED_RETURN(errctx); +} + +/** + * @brief A frame owns every pixel it presents, including ones it did not write. + * + * **This is the regression test for the blinking backspace, and it is the only + * one that could have caught it without real hardware.** The sink used to skip + * rows it believed were already clear, tracked in a `drawn[]` array. That + * assumes a frame inherits the frame before it. SDL_RenderPresent swaps + * buffers, so a frame inherits the one *two* back -- a row erased once is clean + * in one buffer and dirty in the other, and presenting alternates between them. + * The first character of a backspaced wrap flickered forever. + * + * It reproduced on X11 and never under the dummy driver, because a software + * renderer with a single target does preserve. Rather than require real + * hardware, this paints the stale pixels by hand -- which is exactly what the + * other buffer hands back -- and asserts the sink erases them anyway. + */ +static akerr_ErrorContext AKERR_NOIGNORE *test_frame_owns_every_pixel(void) +{ + PREPARE_ERROR(errctx); + SDL_Surface *shot = NULL; + akbasic_AkglSink *sink = NULL; + SDL_FRect stale; + + PASS(errctx, start_frontend(NULL)); + sink = &FRONTEND.akglstate; + + /* One line of text, and the grid otherwise empty. */ + PASS(errctx, FRONTEND.akglsink.writeln(&FRONTEND.akglsink, "KEPT")); + PASS(errctx, akbasic_sink_akgl_render(&FRONTEND.akglsink)); + + /* + * Now dirty row 1 behind the sink's back, the way a swapped-in buffer + * arrives already dirty. The sink has never drawn there and its grid says + * the row is empty, so nothing in its own bookkeeping suggests any work. + */ + stale.x = (float)sink->x; + stale.y = (float)(sink->y + sink->cellh); + stale.w = (float)sink->cellw; + stale.h = (float)sink->cellh; + PASS(errctx, akgl_draw_filled_rect(FRONTEND.renderer, &stale, sink->color)); + + shot = SDL_RenderReadPixels(FRONTEND.renderer->sdl_renderer, NULL); + TEST_REQUIRE(!cell_is_blank(shot, sink, 0, 1), "the stale pixels should be there to start"); + SDL_DestroySurface(shot); + + PASS(errctx, akbasic_sink_akgl_render(&FRONTEND.akglsink)); + shot = SDL_RenderReadPixels(FRONTEND.renderer->sdl_renderer, NULL); + TEST_REQUIRE(shot != NULL, "could not read the render target back"); + TEST_REQUIRE(cell_is_blank(shot, sink, 0, 1), + "a row the grid says is empty must be erased even if this sink never drew it"); + TEST_REQUIRE(!cell_is_blank(shot, sink, 0, 0), "and the row that has text is still drawn"); + SDL_DestroySurface(shot); stop_frontend(); SUCCEED_RETURN(errctx); @@ -789,7 +877,8 @@ int main(void) CATCH(errctx, test_render_erases_what_it_drew()); CATCH(errctx, test_scroll_reaches_the_screen()); CATCH(errctx, test_backspace_across_a_wrap()); - CATCH(errctx, test_no_cursor_underline()); + CATCH(errctx, test_block_cursor_blinks()); + CATCH(errctx, test_frame_owns_every_pixel()); CATCH(errctx, test_repl_session()); } CLEANUP { stop_frontend();