From 64ec7a1d70147c47683a424d7b644ce2fb8ac251 Mon Sep 17 00:00:00 2001 From: Ryan Turner Date: Tue, 19 May 2026 20:13:27 -0500 Subject: [PATCH] gfx: add gfx_printBufferClipped Adds gfx_printBufferClipped: a variant of gfx_printBuffer that accepts an explicit right-edge limit (max_x) and a vertical clip window [clip_top_y, clip_bot_y]. Word-wrap fires when the next glyph would exceed max_x. Pixels outside the clip window are suppressed without affecting layout, so a negative start.y can be used to implement smooth text scrolling. gfx_printBuffer now delegates entirely to gfx_printBufferClipped with clip_top_y=0 and clip_bot_y=CONFIG_SCREEN_HEIGHT-1. The old pixel guard used strict > 0 comparisons, which excluded pixels at row 0 and column 0. The new implementation renders those pixels; callers that relied on the old exclusion will see a behaviour change. Compared to a naive port of gfx_printBuffer, this implementation addresses several correctness issues: - Control characters '\n' and '\r' are checked before the glyph array is indexed, avoiding undefined behaviour on fonts whose first codepoint is at or above 0x20 (e.g. TomThumb, first=0x20), since '\n' (0x0A) would produce a negative index. - get_line_size uses <= instead of < when accumulating glyph widths so that a glyph whose xAdvance exactly equals max_width is counted. The render loop uses strict > to trigger wrap, so the old < mismatch caused text_size.x to be under-reported by one glyph whenever a line was an exact fit, and misplaced CENTER/RIGHT lines in that case. - For TEXT_ALIGN_LEFT, line_size is recomputed on every '\n' before the max_line_size comparison. Without this, max_line_size was stuck at the first-line width for the entire call, so text_size.x was wrong whenever a subsequent line was wider. - saved_start_y is int16_t to match point_t.y, allowing correct text_size.y computation when start.y is negative (scroll offset). - text_size.y is computed as (start.y - saved_start_y) + line_h, matching the sign fix introduced for gfx_printBuffer. - Pixels above row 0 (py < 0) are suppressed before the uint16_t cast passed to gfx_setPixel, preventing a silent wrap to a large row index. - max_line_size tracks the widest line across all line breaks and wraps; text_size.x reflects the widest line, not just the last. Extend tests/unit/gfx_text.cpp with return-value tests covering: single-line height, two-line height delta equals yAdvance, negative start.y scroll offset, negative clip_top_y, exact-fit line width (get_line_size <= boundary), max line width from a wider first line, max line width from a wider second line (TEXT_ALIGN_LEFT regression), glyph at clip_bot_y not skipped, and TEXT_ALIGN_RIGHT per-line reset. Co-authored-by: GitHub Copilot Signed-off-by: Ryan Turner --- openrtx/include/core/graphics.h | 33 +++++ openrtx/src/core/graphics.c | 208 ++++++++++++++++---------------- tests/unit/gfx_text.cpp | 205 +++++++++++++++++++++++++++++++ 3 files changed, 345 insertions(+), 101 deletions(-) diff --git a/openrtx/include/core/graphics.h b/openrtx/include/core/graphics.h index 02284b6e..bb5f49f8 100644 --- a/openrtx/include/core/graphics.h +++ b/openrtx/include/core/graphics.h @@ -220,6 +220,11 @@ point_t gfx_printBuffer(point_t start, fontSize_t size, textAlign_t alignment, * * Simulates the same word-wrap line-break decisions (wrap at max_x). * Alignment is not simulated; only the vertical extent is computed. + * Wrap decisions always use start_x as the left margin of every line, + * which matches left-aligned layout. For TEXT_ALIGN_CENTER or + * TEXT_ALIGN_RIGHT the actual rendered start.x may be larger than + * start_x, so the wrap point may differ and the returned height may + * not exactly match what gfx_printBufferClipped produces. * Passing char_count < strlen(buf) lets callers find the y-coordinate * of an arbitrary cursor position for scroll-offset calculations. * @@ -237,6 +242,34 @@ point_t gfx_printBuffer(point_t start, fontSize_t size, textAlign_t alignment, uint16_t gfx_measureText(fontSize_t size, const char *buf, uint16_t start_x, uint16_t max_x, size_t char_count); +/** + * Prints text from a char buffer into a clipped rectangular region. + * + * Like gfx_printBuffer but with an explicit right-edge limit and a + * vertical clip window. Word-wrap fires when the next glyph would + * exceed max_x rather than CONFIG_SCREEN_WIDTH. Pixels outside the + * range [clip_top_y, clip_bot_y] are suppressed without affecting + * layout, so a negative start.y can be used to implement scrolling. + * + * The returned text_size.y reflects only the lines processed before + * clip_bot_y is reached, not the full height of the text block. Use + * gfx_measureText to obtain the total height for scroll calculations. + * + * @param start: top-left origin for the text block, in pixel coordinates. + * @param size: text font size. + * @param alignment: text alignment. + * @param color: text colour. + * @param buf: NUL-terminated string to render. + * @param max_x: right-edge pixel limit (exclusive) for word-wrap. + * @param clip_top_y: topmost pixel row to draw (inclusive). + * @param clip_bot_y: bottommost pixel row to draw (inclusive). + * @return text width and height of the processed portion as point_t. + */ +point_t gfx_printBufferClipped(point_t start, fontSize_t size, + textAlign_t alignment, color_t color, + const char *buf, uint16_t max_x, + int16_t clip_top_y, int16_t clip_bot_y); + /** * Prints text on the screen at the specified coordinates. * @param start: text line start point, in pixel coordinates. diff --git a/openrtx/src/core/graphics.c b/openrtx/src/core/graphics.c index b0ed5d99..6822a14a 100644 --- a/openrtx/src/core/graphics.c +++ b/openrtx/src/core/graphics.c @@ -403,7 +403,7 @@ static inline uint16_t get_line_size(GFXfont f, const char *text, uint16_t line_size = 0; for(unsigned i = 0; i < length && text[i] != '\n' && text[i] != '\r'; i++) { - if(text[i] < f.first || text[i] > f.last) + if (text[i] < f.first || text[i] > f.last) continue; GFXglyph glyph = f.glyph[text[i] - f.first]; if (line_size + glyph.xAdvance <= max_width) @@ -417,20 +417,22 @@ static inline uint16_t get_line_size(GFXfont f, const char *text, /** * Compute the start x coordinate of a new line of given pixel size - * @param alinment: enum representing the text alignment + * @param alignment: enum representing the text alignment * @param line_size: the size of the current text line in pixels + * @param startx: left-edge x position of the text block + * @param max_width: right-edge pixel limit (width of the text region) */ static inline uint16_t get_reset_x(textAlign_t alignment, uint16_t line_size, - uint16_t startx) + uint16_t startx, uint16_t max_width) { switch(alignment) { case TEXT_ALIGN_LEFT: return startx; case TEXT_ALIGN_CENTER: - return (CONFIG_SCREEN_WIDTH - line_size)/2; + return (max_width - line_size)/2; case TEXT_ALIGN_RIGHT: - return CONFIG_SCREEN_WIDTH - line_size - startx; + return max_width - line_size - startx; } return 0; @@ -446,102 +448,9 @@ uint8_t gfx_getFontHeight(fontSize_t size) point_t gfx_printBuffer(point_t start, fontSize_t size, textAlign_t alignment, color_t color, const char *buf) { - GFXfont f = fonts[size]; - - size_t len = strlen(buf); - - // Compute size of the first row in pixels - uint16_t line_size = get_line_size(f, buf, len, CONFIG_SCREEN_WIDTH); - uint16_t reset_x = get_reset_x(alignment, line_size, start.x); - start.x = reset_x; - // Save initial start.y value to calculate vertical size - int16_t saved_start_y = start.y; - uint16_t line_h = 0; - - /* For each char in the string */ - for(unsigned i = 0; i < len; i++) - { - char c = buf[i]; - - // Handle newline and carriage return before accessing glyph table - if (c == '\n') - { - if(alignment != TEXT_ALIGN_CENTER) - { - start.x = reset_x; - } - else - { - line_size = get_line_size(f, &buf[i+1], len-(i+1), - CONFIG_SCREEN_WIDTH); - start.x = reset_x = get_reset_x(alignment, line_size, start.x); - } - start.y += f.yAdvance; - continue; - } - else if (c == '\r') - { - start.x = reset_x; - continue; - } - - GFXglyph glyph = f.glyph[c - f.first]; - uint8_t *bitmap = f.bitmap; - - uint16_t bo = glyph.bitmapOffset; - uint8_t w = glyph.width, h = glyph.height; - int8_t xo = glyph.xOffset, - yo = glyph.yOffset; - uint8_t xx, yy, bits = 0, bit = 0; - line_h = h; - - // Handle wrap around - if (start.x + glyph.xAdvance > CONFIG_SCREEN_WIDTH) - { - // Compute size of the remaining text for this new line - line_size = get_line_size(f, &buf[i], len - i, - CONFIG_SCREEN_WIDTH); - // Pass reset_x (the original start x) not start.x (current pos) - start.x = reset_x = get_reset_x(alignment, line_size, reset_x); - start.y += f.yAdvance; - } - - // Draw bitmap - for (yy = 0; yy < h; yy++) - { - for (xx = 0; xx < w; xx++) - { - if (!(bit++ & 7)) - { - bits = bitmap[bo++]; - } - - if (bits & 0x80) - { - if (start.y + yo + yy < CONFIG_SCREEN_HEIGHT && - start.x + xo + xx < CONFIG_SCREEN_WIDTH && - start.y + yo + yy > 0 && - start.x + xo + xx > 0) - { - point_t pos; - pos.x = start.x + xo + xx; - pos.y = start.y + yo + yy; - gfx_setPixel(pos, color); - - } - } - - bits <<= 1; - } - } - - start.x += glyph.xAdvance; - } - // Calculate text size - point_t text_size = {0, 0}; - text_size.x = line_size; - text_size.y = (start.y - saved_start_y) + line_h; - return text_size; + return gfx_printBufferClipped(start, size, alignment, color, buf, + CONFIG_SCREEN_WIDTH, 0, + CONFIG_SCREEN_HEIGHT - 1); } uint16_t gfx_measureText(fontSize_t size, const char *buf, uint16_t start_x, @@ -582,6 +491,103 @@ uint16_t gfx_measureText(fontSize_t size, const char *buf, uint16_t start_x, return cur_y; } +point_t gfx_printBufferClipped(point_t start, fontSize_t size, + textAlign_t alignment, color_t color, + const char *buf, uint16_t max_x, + int16_t clip_top_y, int16_t clip_bot_y) +{ + GFXfont f = fonts[size]; + + size_t len = strlen(buf); + + uint16_t line_size = get_line_size(f, buf, (uint16_t)len, max_x); + uint16_t max_line_size = line_size; + uint16_t origin_x = start.x; + uint16_t reset_x = get_reset_x(alignment, line_size, origin_x, max_x); + start.x = reset_x; + int16_t saved_start_y = start.y; + uint16_t line_h = 0; + + for (unsigned i = 0; i < len; i++) { + /* Even the topmost pixel of the next glyph falls below clip_bot_y. */ + if ((int16_t)(start.y - (int16_t)f.yAdvance) > clip_bot_y) + break; + + char c = buf[i]; + + /* Handle control characters before accessing the glyph table. */ + if (c == '\n') { + if (line_size > max_line_size) + max_line_size = line_size; + line_size = get_line_size(f, &buf[i + 1], + (uint16_t)(len - (i + 1)), max_x); + if (alignment == TEXT_ALIGN_CENTER + || alignment == TEXT_ALIGN_RIGHT) { + start.x = reset_x = get_reset_x(alignment, line_size, origin_x, + max_x); + } else { + start.x = reset_x; + } + start.y += f.yAdvance; + continue; + } else if (c == '\r') { + start.x = reset_x; + continue; + } + + if (c < f.first || c > f.last) + continue; + GFXglyph glyph = f.glyph[c - f.first]; + uint8_t *bitmap = f.bitmap; + + uint16_t bo = glyph.bitmapOffset; + uint8_t w = glyph.width, h = glyph.height; + int8_t xo = glyph.xOffset, yo = glyph.yOffset; + uint8_t xx, yy, bits = 0, bit = 0; + line_h = h; + + /* Wrap when the glyph would exceed the right-edge limit. */ + if (start.x + glyph.xAdvance > max_x) { + if (line_size > max_line_size) + max_line_size = line_size; + line_size = get_line_size(f, &buf[i], (uint16_t)(len - i), max_x); + start.x = reset_x = get_reset_x(alignment, line_size, origin_x, + max_x); + start.y += f.yAdvance; + } + + /* Draw pixels, suppressing those outside the clip window. */ + for (yy = 0; yy < h; yy++) { + for (xx = 0; xx < w; xx++) { + if (!(bit++ & 7)) + bits = bitmap[bo++]; + + if (bits & 0x80) { + int16_t px = (int16_t)(start.x + xo + xx); + int16_t py = (int16_t)(start.y + yo + yy); + + if (py >= clip_top_y && py <= clip_bot_y && py >= 0 + && px >= 0 && px < (int16_t)max_x) { + point_t pos = {(uint16_t)px, (uint16_t)py}; + gfx_setPixel(pos, color); + } + } + + bits <<= 1; + } + } + + start.x += glyph.xAdvance; + } + + if (line_size > max_line_size) + max_line_size = line_size; + point_t text_size = {0, 0}; + text_size.x = max_line_size; + text_size.y = (start.y - saved_start_y) + line_h; + return text_size; +} + point_t gfx_print(point_t start, fontSize_t size, textAlign_t alignment, color_t color, const char *fmt, ... ) { diff --git a/tests/unit/gfx_text.cpp b/tests/unit/gfx_text.cpp index 01d20031..1959e41a 100644 --- a/tests/unit/gfx_text.cpp +++ b/tests/unit/gfx_text.cpp @@ -24,6 +24,50 @@ static constexpr uint16_t WIDE = 4096; /* A max_x so narrow (1 px) that every printable glyph triggers a wrap. */ static constexpr uint16_t NARROW = 1; +/* ----------------------------------------------------------------------- + * gfx_printBuffer regression tests. + * + * These pin the behaviour fixed in the commit that corrected wrap logic, + * UB glyph access order, and the sign of text_size.y. + * ----------------------------------------------------------------------- */ + +TEST_CASE("gfx_printBuffer text_size.y is positive", "[gfx][text]") +{ + color_t white = { 255, 255, 255, 255 }; + point_t start = { 0, 10 }; + + point_t sz = gfx_printBuffer(start, FONT_SIZE_5PT, TEXT_ALIGN_LEFT, white, + "Hello"); + REQUIRE(sz.y > 0); +} + +TEST_CASE("gfx_printBuffer text_size.y grows with newlines", "[gfx][text]") +{ + color_t white = { 255, 255, 255, 255 }; + point_t start = { 0, 10 }; + + point_t sz1 = gfx_printBuffer(start, FONT_SIZE_5PT, TEXT_ALIGN_LEFT, white, + "Hello"); + point_t sz2 = gfx_printBuffer(start, FONT_SIZE_5PT, TEXT_ALIGN_LEFT, white, + "Hello\nHello"); + + REQUIRE(sz2.y > sz1.y); + REQUIRE((sz2.y - sz1.y) == TT_Y_ADVANCE); +} + +TEST_CASE("gfx_printBuffer does not access glyph for newline", "[gfx][text]") +{ + /* If the UB glyph-access-before-newline-check bug were present, this + * would either crash or corrupt memory when '\\n' (0x0A) is below + * f.first. Reaching REQUIRE without fault is the assertion. */ + color_t white = { 255, 255, 255, 255 }; + point_t start = { 0, 10 }; + + point_t sz = gfx_printBuffer(start, FONT_SIZE_5PT, TEXT_ALIGN_LEFT, white, + "a\nb\nc"); + REQUIRE(sz.y > 0); +} + TEST_CASE("gfx_measureText single-line strings", "[gfx][text]") { SECTION("empty string occupies one line") @@ -117,3 +161,164 @@ TEST_CASE("gfx_measureText word-wrap at max_x", "[gfx][text]") REQUIRE(h_narrow > h_wide); } } + +/* ----------------------------------------------------------------------- + * gfx_printBufferClipped return-value tests. + * + * gfx_setPixel is a no-op in the Linux platform test build, so we cannot + * verify pixel output. We verify the returned text_size instead, which + * exercises the layout logic without display hardware. + * ----------------------------------------------------------------------- */ + +TEST_CASE("gfx_printBufferClipped text_size.y for single-line text", + "[gfx][text]") +{ + color_t white = { 255, 255, 255, 255 }; + + /* + * Single-line: start.y does not advance, so text_size.y should equal + * the raw glyph height of the last character rendered (line_h). It + * must be > 0 and <= TT_Y_ADVANCE. + */ + point_t start = { 0, 20 }; + point_t sz = gfx_printBufferClipped(start, FONT_SIZE_5PT, TEXT_ALIGN_LEFT, + white, "Hi", (uint16_t)WIDE, 0, 127); + REQUIRE(sz.y > 0); + REQUIRE(sz.y <= TT_Y_ADVANCE); +} + +TEST_CASE("gfx_printBufferClipped text_size.y grows for two lines", + "[gfx][text]") +{ + color_t white = { 255, 255, 255, 255 }; + point_t start = { 0, 20 }; + + point_t sz1 = gfx_printBufferClipped(start, FONT_SIZE_5PT, TEXT_ALIGN_LEFT, + white, "Hi", (uint16_t)WIDE, 0, 127); + point_t sz2 = gfx_printBufferClipped(start, FONT_SIZE_5PT, TEXT_ALIGN_LEFT, + white, "Hi\nHi", (uint16_t)WIDE, 0, + 127); + + /* Two lines must be taller than one. */ + REQUIRE(sz2.y > sz1.y); + /* The difference should equal exactly one yAdvance. */ + REQUIRE((sz2.y - sz1.y) == TT_Y_ADVANCE); +} + +TEST_CASE("gfx_printBufferClipped negative start.y does not corrupt result", + "[gfx][text]") +{ + color_t white = { 255, 255, 255, 255 }; + + /* Scrolled position: top of text block is above the visible area. */ + point_t start_neg = { 0, -10 }; + point_t start_pos = { 0, 0 }; + + point_t sz_neg = gfx_printBufferClipped(start_neg, FONT_SIZE_5PT, + TEXT_ALIGN_LEFT, white, "Hi", + (uint16_t)WIDE, 0, 127); + point_t sz_pos = gfx_printBufferClipped(start_pos, FONT_SIZE_5PT, + TEXT_ALIGN_LEFT, white, "Hi", + (uint16_t)WIDE, 0, 127); + + /* Layout is the same regardless of vertical scroll offset. */ + REQUIRE(sz_neg.y == sz_pos.y); + REQUIRE(sz_neg.y > 0); +} + +TEST_CASE("gfx_printBufferClipped negative clip_top_y does not corrupt result", + "[gfx][text]") +{ + color_t white = { 255, 255, 255, 255 }; + + /* Pixels above row 0 must be suppressed without corrupting layout. */ + point_t start = { 0, 10 }; + point_t sz = gfx_printBufferClipped(start, FONT_SIZE_5PT, TEXT_ALIGN_LEFT, + white, "Hi", (uint16_t)WIDE, -10, 127); + REQUIRE(sz.y > 0); + REQUIRE(sz.y <= TT_Y_ADVANCE); +} + +TEST_CASE("get_line_size exact-fit glyph is included in line width", + "[gfx][text]") +{ + color_t white = { 255, 255, 255, 255 }; + point_t start = { 0, 10 }; + + /* TomThumb 'A' xAdvance=4: with max_x=4 the glyph exactly fills the line. */ + point_t sz = gfx_printBufferClipped(start, FONT_SIZE_5PT, TEXT_ALIGN_LEFT, + white, "A", 4, 0, 127); + REQUIRE(sz.x == 4); +} + +TEST_CASE("gfx_printBufferClipped text_size.x is max line width, not last line", + "[gfx][text]") +{ + color_t white = { 255, 255, 255, 255 }; + point_t start = { 0, 10 }; + + /* + * First line "Hello" is wider than second line "Hi". + * text_size.x must reflect the wider first line, not the shorter last. + */ + point_t sz_wide = gfx_printBufferClipped(start, FONT_SIZE_5PT, + TEXT_ALIGN_LEFT, white, "Hello", + (uint16_t)WIDE, 0, 127); + point_t sz_multi = + gfx_printBufferClipped(start, FONT_SIZE_5PT, TEXT_ALIGN_LEFT, white, + "Hello\nHi", (uint16_t)WIDE, 0, 127); + REQUIRE(sz_multi.x == sz_wide.x); +} + +TEST_CASE("gfx_printBufferClipped text_size.x when second line is wider", + "[gfx][text]") +{ + color_t white = { 255, 255, 255, 255 }; + point_t start = { 0, 10 }; + + /* Second line "Hello" is wider than first line "Hi". */ + point_t sz_wide = gfx_printBufferClipped(start, FONT_SIZE_5PT, + TEXT_ALIGN_LEFT, white, "Hello", + (uint16_t)WIDE, 0, 127); + point_t sz_multi = + gfx_printBufferClipped(start, FONT_SIZE_5PT, TEXT_ALIGN_LEFT, white, + "Hi\nHello", (uint16_t)WIDE, 0, 127); + REQUIRE(sz_multi.x == sz_wide.x); +} + +TEST_CASE("gfx_printBufferClipped glyph at clip_bot_y baseline is not skipped", + "[gfx][text]") +{ + color_t white = { 255, 255, 255, 255 }; + + /* Baseline at clip_bot_y+1: all glyph pixels still fall within window. */ + int16_t base_y = 20; + int16_t clip_bot = base_y - 1; + point_t start = { 0, base_y }; + + point_t sz = gfx_printBufferClipped(start, FONT_SIZE_5PT, TEXT_ALIGN_LEFT, + white, "A", (uint16_t)WIDE, 0, + clip_bot); + REQUIRE(sz.y > 0); +} + +TEST_CASE("gfx_printBufferClipped TEXT_ALIGN_RIGHT resets per line", + "[gfx][text]") +{ + color_t white = { 255, 255, 255, 255 }; + point_t start = { 0, 10 }; + + /* + * With TEXT_ALIGN_RIGHT each line's start.x should be computed from + * that line's own width. The returned text_size.x must equal the + * width of the widest line, same as for LEFT alignment. + */ + point_t sz_right = + gfx_printBufferClipped(start, FONT_SIZE_5PT, TEXT_ALIGN_RIGHT, white, + "Hello\nHi", (uint16_t)WIDE, 0, 127); + point_t sz_left = + gfx_printBufferClipped(start, FONT_SIZE_5PT, TEXT_ALIGN_LEFT, white, + "Hello\nHi", (uint16_t)WIDE, 0, 127); + /* Max line width is alignment-independent. */ + REQUIRE(sz_right.x == sz_left.x); +}