PROTO: Create text_objects and use them for rendering to improve text rendering performance #773

Vagabond · 2024-11-19T18:07:37Z

This is a cherry-pick/rework of the commit from the n64 branch. It builds and works but it needs more work/cleanups.

EDIT(Nopey): Closes #677

src/engine.c

Vagabond · 2024-11-24T20:47:14Z

src/game/gui/menu.c

@@ -411,6 +416,11 @@ void menu_set_padding(component *c, int padding) {
    m->padding = padding;
 }

+void menu_invalidate_help_text_cache(component *c) {


Does anything use this?

game_menu_return should be calling it.

Maybe I goofed the rebase?

Could also be I missed it in the original PROTO submit.

Vagabond · 2024-11-24T20:49:10Z

src/game/gui/textinput.c

@@ -212,6 +213,7 @@ static void textinput_free(component *c) {
    textinput *tb = widget_get_obj(c);
    surface_free(&tb->sur);
    str_free(&tb->text);
+    text_objects_free(tb->text_cache, (sizeof(tb->text_cache) / sizeof(tb->text_cache[0])));


Why isn't the second parameter an integer?

size_t is an unsigned int, but will be 64bit or 32bit based on your architecture. And it's not a signed int because I never expect to have a negative amount of objects to destroy.

I mean why not just pass the number of text objects

size_t is fine, but the arraysize magic should probably be replaced with a named constant (same as the [0]'s, [1]'s, [2]'s and [3]'s, as pointed out in a different conversation)

This should become a macro called ARRAY_SIZE(tb->text_cache).
That makes it forward compatible, when someone expands or reduces the array, they won't need to change this instance.

src/game/gui/textinput.c

Vagabond · 2024-11-24T20:50:35Z

src/game/gui/textinput.c

@@ -27,6 +27,7 @@ typedef struct {

    textinput_done_cb done_cb;
    void *userdata;
+    text_object text_cache[3];


Can be 2 not 3 here

These text_caches should seriously be named variables.

I don't disagree, or at least used named indices

Sounds good.

src/game/scenes/scoreboard.c

Vagabond · 2024-11-24T20:56:05Z

src/game/scenes/vs.c

@@ -312,73 +319,83 @@ void vs_render(scene *scene) {
        char text[256];
        char money[16];
        fight_stats *fight_stats = &scene->gs->fight_stats;
+        static int force_redraw = 1;
+        if(force_redraw) {
+            // TODO: Find out why this caching doesn't work. The cache is built before the stats are correct.


TODO: investigate this

This problem can be seen when bringing up the in-game menu the second time through, it will show an empty spot where the "explanation" is on the N64 branch indicating that the text object wasn't correctly dirtied.

There's a game menu on the 'vs' screen?

Uhhh, that's interesting. I didn't realize I left this comment in the VS, it probably needs to be in arena somewhere.

src/game/scenes/vs.c

Vagabond · 2024-11-24T20:59:01Z

src/game/utils/score.c

@@ -38,6 +39,8 @@ vec2i interpolate(vec2i start, vec2i end, float fraction) {
 }

 void chr_score_create(chr_score *score) {
+    score->text_cache[0].dynamic = true;


Unclear why this needs to be dynamic. Only the x/y position changes, maybe those should not be cached?

Dynamic means something special, it means that the frame-over-frame rendering of the text object cannot be guaranteed to be the same. As the object may get occluded and require a re-render. Unfortunately I couldn't figure out a way to track this, so I resorted to hand picking objects that were NOT-SAFE to copy from the previous frame.

This only really matters on the N64, where we can copy from the previous frame buffer, instead of allocating an intermediate texture.
The fallback would be to allocate the intermediate texture on the N64.
And can be completely ignored on the PC.

ah, ok, maybe we can have a better name than dynamic, or have a comment that explains it.

other names I've considered: not_copy_safe, needs_intermediate_texture, not_static_composition,

is uncachable or changes_frequently appropriate?

It's more of a hint to the renderer telling it not to 2nd level optimize.
0th lvl - render each letter.
1st lvl - allocate a new texture, render each letter to texture on dirty bit, use the allocated texture on all invocations without the dirty bit.
2nd lvl - render each letter to FB on dirty bit, copy from FB without dirty bit.

Vagabond · 2024-11-24T21:00:10Z

src/video/video.c

@@ -246,3 +248,10 @@ void video_draw_remap(const surface *src_surface, int x, int y, int remap_offset
    dst.y = y;
    draw_args(src_surface, &dst, remap_offset, remap_rounds, 0, 255, 255, 0, options);
 }
+
+void video_render_text_block(text_object *text) {


I'm not sure this belongs in video.c

The idea here was that the video plugin can decide what to do with a text object.
For instance the N64 side doesn't traverse the letters if the object is not marked dirty. The code in here for video.c (PC) is unchanged from the original call pattern. If there is a better place that would allow the renderer to decide how to handle the text objects, I'm open to suggestions. This really seemed like the correct place. For instance the NULL renderer will just do nothing here too.

This is why we discussed the possible pluggable text renderer backend earlier. The plugin backend would decide how to call the video renderer instead.

I agree, but maybe we can do something intermediate for now, before we go to the "solves all the problems" pluggable text renderer.

I WANT IT NOW! We should throw in kubernetes too somehow ;)

@katajakasa Would this be something you could rework later, after this change goes in?

Yes, it is. Let's get this part merged first.

Vagabond · 2024-11-25T01:50:36Z

src/game/gui/label.c

@@ -55,7 +57,7 @@ component *label_create_with_width(const text_settings *tconf, const char *text,
    widget_set_obj(c, local);
    widget_set_render_cb(c, label_render);
    widget_set_free_cb(c, label_free);
-
+    // local->text_cache[0].dynamic = true;


Vagabond · 2024-11-25T01:51:16Z

src/game/gui/menu.c

@@ -110,7 +110,11 @@ static void menu_render(component *c) {
            if(m->help_bg2) {
                video_draw(m->help_bg2, m->help_x - 8, m->help_y - 8);
            }
-            text_render(&m->help_text_conf, TEXT_DEFAULT, m->help_x, m->help_y, m->help_w, m->help_h, (*tmp)->help);
+            // TODO: The text_cache should move into inner component. In this case the one of the s->objs.


I believe this comment is wrong and it's not invalidated every frame

I think what I ran into was that the menu didn't animate, so I marked all these dynamic.

Nopey · 2024-11-25T02:56:35Z

src/game/gui/text_render.h

+    int offset;
+    int limit;


Suggested change

int offset;

int limit;

int palette_offset;

int palette_limit;

Might also be possible to make these a smaller (8-bit) integer type, but that sounds like work for another PR.

Nopey · 2024-11-25T02:59:46Z

src/game/gui/text_render.h

+    int y;
+    int offset;
+    int limit;
+    surface *sur;


Suggested change

surface *sur;

surface const *sur;

Since this is not an owned surface, and we do not plan to modify the surface via this pointer.

Nopey · 2024-11-25T03:03:39Z

src/game/gui/text_render.c

+        if(cached_text->max_letters < (text_len * 4)) {
+            size_t size = (text_len * sizeof(letter) * 4);


What is magic number 4?

It looks like 4 is the number of draw calls per letter.
At the very least, it should be a named constant.

Perhaps all four draw calls should be stored in one struct letter? This would increase the number of fields in struct letter/struct text_object, but should decrease overall memory usage.

It's the shadows, we support up to 3 shadows per letter, including the letter itself. Actually per the code we support 4 shadows per letter (+letter). But I've never seen more than 3 being used.

Nopey · 2024-11-25T03:04:18Z

src/game/gui/text_render.c

-    render_char_surface(settings, state, sur, x, y);
-    return sur->w;
+void letter_set_parameters(text_object *cached_text, surface *sur, int x, int y, int offset, int limit) {
+    cached_text->cur_letter->x = x;


Please add an assert that checks that cached_text->cur_letter < cached_text->letter + cached_text->max_letters

… rendering performance

Vagabond · 2024-11-25T18:40:36Z

Ok, I think I've gained some familiarity with this PR now, and I'd like to suggest some changes:

I'd like the invalidation of a text cache to be explicit, not automatic. We can know when we invalidate text strings and update them appropriately.
I'd like to separate flowing text into a text object and rendering a text object in the API.
I'd like not to hold a pointer to the 'text' inside the text cache, since it should no longer be needed as per point 1
It might be useful, in some cases, to render multiple strings to the same text_cache. A good example is the fight stats page in tournament mode, or the HAR/Player stats in the mechlab. If we aren't tracking strings anymore, this also seems doable. This would massively reduce the number of text cache objects we have to maintain.

cc @nopjne @katajakasa

It might be useful, in some cases, to render multiple strings to the same text_cache. A good example is the fight stats page in tournament mode, or the HAR/Player stats in the mechlab. If we aren't tracking strings anymore, this also seems doable. This would massively reduce the number of text cache objects we have to maintain.

As long as that doesn't unnecessarily extend the bounds of the text object.

Vagabond · 2024-11-25T18:52:17Z

It might be useful, in some cases, to render multiple strings to the same text_cache. A good example is the fight stats page in tournament mode, or the HAR/Player stats in the mechlab. If we aren't tracking strings anymore, this also seems doable. This would massively reduce the number of text cache objects we have to maintain.

As long as that doesn't unnecessarily extend the bounds of the text object.

My idea would be that if we were flowing several chunks of text into the same text_object, then we could 'finish' the flow that would allow any underlying texture allocation stuff to happen, with the final set of bounds?

Vagabond force-pushed the adt/text-objects branch from 3e6dc77 to ae81038 Compare November 23, 2024 19:15

Vagabond commented Nov 24, 2024

View reviewed changes

Vagabond commented Nov 25, 2024

View reviewed changes

Nopey reviewed Nov 25, 2024

View reviewed changes

nopjne and others added 5 commits November 25, 2024 10:09

PROTO: Create text_objects and use them for rendering to improve text…

c04bb35

… rendering performance

Fixes and cleanups to text_objects

e7f48cf

Fix console text rendering

d4fa6db

Fix some leaks and do some cleanup

bd1fd57

Fix unit tests

ead6221

Vagabond force-pushed the adt/text-objects branch from 24a2118 to ead6221 Compare November 25, 2024 18:09

Suggested cleanups

607dd4f

		if(cached_text->max_letters < (text_len * 4)) {
		size_t size = (text_len * sizeof(letter) * 4);

PROTO: Create text_objects and use them for rendering to improve text rendering performance #773

Are you sure you want to change the base?

PROTO: Create text_objects and use them for rendering to improve text rendering performance #773

Conversation

Vagabond commented Nov 19, 2024 • edited by Nopey Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nopey Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nopjne Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nopey Nov 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Vagabond commented Nov 25, 2024 • edited by nopjne Loading

Vagabond commented Nov 25, 2024

Vagabond commented Nov 19, 2024 •

edited by Nopey

Loading

Nopey Nov 25, 2024 •

edited

Loading

nopjne Nov 25, 2024 •

edited

Loading

Nopey Nov 25, 2024 •

edited

Loading

Vagabond commented Nov 25, 2024 •

edited by nopjne

Loading