Textui reform (warning: long and full of C)

[takkaria]

Sorry for taking a while to respond. I've looked at your work in progress, which looks really great, and cleared up my misunderstandings of your original post that I was originally going to post about. Anyway, here's my thoughts, feel free to ignore

Things that look good to me and I don't have anything to say: the term stack stuff, Term_fresh() changes, using the 'transparent' background attr for background in ASCII mode instead of modulo, splitting apart the graphics files into multiple files sounds pretty sensible to me too.

On colour reform, there's a general belief that the game should use a limited colour palette so that the colours are distinguishable, at least in ASCII mode, so I'm not sure using rgba gains much.

With tiles and transparency, I think it would be good to be able to draw more than two tiles on a grid - so you can draw, e.g. the floor, a trap/an object, and a monster, one on top of the other. And like you say, it would be good to be able to apply tints of various kinds to the tiles. This could maybe remove the need to have 4 different tiles per terrain feature.

With regard to input, I guess it makes sense for keypresses to be global but mouse clicks are on particular sections of the screen. Maybe that means that mouseclicks get passed along in the ui_event struct with a pointer to the term they were triggered on.

[t4nk]

I certainly can't think of a single reason to say no.

I think one of the usual responses is "As long as the GCU port still works", so I'll say that

I mean just bugfixes for textui as it as. I sent a pull request.

[Nick]

Nick, do you want patches for current textui?
Also, I'm afraid this will be a rather substantial rewrite I'm still hoping to keep menu code, it's a good code... but the stuff like ui-input.c really makes me RAGE and why does it display messages?

I certainly can't think of a single reason to say no.

I think one of the usual responses is "As long as the GCU port still works", so I'll say that

[Pete Mack]

The inkey stuff is indeed horrendous. It's the reason I haven't got the replay log package running again--the original took advantage of some features in the macro section that has since been removed.

[takkaria]

Nick, do you want patches for current textui?
Also, I'm afraid this will be a rather substantial rewrite I'm still hoping to keep menu code, it's a good code... but the stuff like ui-input.c really makes me RAGE and why does it display messages?

edit: well, to be fair, it's really inkey() stuff that is so outrageous. That might be the single worst thing in angband's codebase...

Yeah, the inkey() stuff is enough to make you run away and never come back. I hate it but it's not the kind of thing you can refactor slowly, I think you basically just have to nuke a load of code and try again.

The message stuff also shouldn't be there, I'm assuming it is just because of the msg_flush stuff. It should obviously go in a separate ui-msg file or something.

[debo]

That might be the single worst thing in angband's codebase...

You probably haven't looked hard enough

(For something that is a billion years old it is still pretty good though, IMO -- and I haven't really even looked closely post-refactor!)

[t4nk]

Nick, do you want patches for current textui?
Also, I'm afraid this will be a rather substantial rewrite I'm still hoping to keep menu code, it's a good code... but the stuff like ui-input.c really makes me RAGE and why does it display messages?

edit: well, to be fair, it's really inkey() stuff that is so outrageous. That might be the single worst thing in angband's codebase...

[t4nk]

Hey, I think this is smart work and mostly makes sense to me. I have some suggestions/ideas that I think would improve but don't have time to reply more fully right now. Will collect my thoughts over the next couple of days and write a longer reply

Looking forward to it! I'm not sure how to handle various prompts...

It's not a bug as such. Certain users like to fiddle with the game, adding things that the maintainers didn't plan for. Unless you're going to generate a tile for them, you're going to need a backup and that;s text.

We'll see. It's not actually difficult to do, I just don't feel the need to support all users and all uses. I do have a backup - THEY will generate a tile

[Carnivean]

Wow, don't agree. Bugs, such as missing tiles, should be fixed, not worked around.

It's not a bug as such. Certain users like to fiddle with the game, adding things that the maintainers didn't plan for. Unless you're going to generate a tile for them, you're going to need a backup and that;s text.

[takkaria]

Hey, I think this is smart work and mostly makes sense to me. I have some suggestions/ideas that I think would improve but don't have time to reply more fully right now. Will collect my thoughts over the next couple of days and write a longer reply

[t4nk]

@t4nk--
You have to be able to mix tiles and characters for any monster or object missing a tile. This happens more than you might think

Wow, don't agree. Bugs, such as missing tiles, should be fixed, not worked around. It's just too annoying to add tiles to existing huge monolithic tilesets! But otherwise you gave me an idea I don't know why separate arrays for attrs and chars should exist at all. This would be a more useful construct:

struct term_point {
    wchar_t ch;
    uint32_t attr;
    bitflag flags[TERM_FLAGS_SIZE];
};

Then term_win could be full of these things, and pict_hook() and text_hook() just eliminated and replaced with a single draw_hook(). The interpretation of ch, attr and flags would be left to the frontend.

Also, I have no idea if people do this with gfx enabled, but I have a pref file that turns the extra valuable scrolls pink. (Rune, Banishment, Mass Banishment, *Destruction*), and some lesser ones like *enchant weapon* yellow. It'd be impossible to do this without mixing tile and character.

It's called 'color modulation' and it's an easy thing to do actually (with tiles). Although that seems like an obscure feature that few players would miss.

My feeling is that the lower level code needed a refresh, but I wasn't prpared to go down that rabbit hole at the moment. If you are, that's great

I'll do what I can

[Nick]

This all sounds very promising.

The main problems that the ui has recently caused (or currently causes) me are

• bad tile drawing due to menus or failure to update
• flicker
• slowdown while resting or running due to main map and subwindow map updates
• failure to update correctly when changing panel, so the player running right appears on the left before disappearing on the right

These play off each other - for example, I think the last one appeared as a consequence of an attempt to fix slowdown or flicker.

My feeling is that the lower level code needed a refresh, but I wasn't prpared to go down that rabbit hole at the moment. If you are, that's great

[Pete Mack]

@t4nk--
You have to be able to mix tiles and characters for any monster or object missing a tile. This happens more than you might think

Also, I have no idea if people do this with gfx enabled, but I have a pref file that turns the extra valuable scrolls pink. (Rune, Banishment, Mass Banishment, *Destruction*), and some lesser ones like *enchant weapon* yellow. It'd be impossible to do this without mixing tile and character.

[t4nk]

Oops, posted in the wrong forum. Is it possible to move it to dev forum?

[t4nk]

First of all; I think that textui (the stuff in ui-*.[ch] files) is actually a pretty good UI. It performs its functions well, and, IMO, it looks nice (in an old school kind of way ). It's also a rather significant amount of tested, debugged code. Therefore, I think it's desirable to use as much of it as possible. Textui, however, does have significant technical problems. I assume that the readers of this post are well aware of what these problems are.
Here's my tentative plan for textui reform.

term_screen

Global Term will be replaced by global stack of terms

static struct {
    term *stack[TERM_STACK_MAX];
    size_t top;
} term_stack = {
    .stack[0] = term_screen
};

(or something like that; note that term_stack is NOT a replacement for angband_term[]; the latter will still exist). Some new functions will be added to ui-term.c (or, perhaps, to ui2-term.c); the main ones are Term_push(term *, struct term_hints *) and Term_pop(void). They will replace two constructions currently used in textui; the first one is
a) Term_activate

    term *old = Term;
    Term_activate(inv_term);
    ...
    Term_activate(old);

instead, it will become:

    Term_push(inv_term, NULL);
    ...
    Term_pop();

I think this is pretty self-explanatory. Another, much more important role for Term_push() and Term_pop() is
b) screen_save and screen_load

    screen_save();
    ...
    menu_select();
    ...
    screen_load();

saving and loading term_screen is used mainly (or exclusively?) by menu code, to draw menus directly on term_screen. If we want to eliminate tile_width/tile_height, menu code cannot use term_screen; the main term should display only one thing - the dungeon. That is, term_screen cannot have messages, sidebars, status lines or menus on it. It's easy to make sidebar or message line just use their own, separate terms (perhaps 'docked' to term_screen). But how to display menus? I propose they also use their own terms:

    /* that replaces screen_save() */
    struct term_hints hints = {
        .new = true,
        .width = 80,
        .height = 24,
        .position = TERM_POSITION_CENTERED_IN_TERM_SCREEN,
        .borders = true
    };
    Term_push(NULL, &hints); /* create new term for menu */
    ...
    menu_select(); /* do the usual thing */
    ...
    /* that replaces screen_load() */
    Term_pop(); /* destroy temporary term */

pushing a NULL term creates a new, termporary, empty term on top of the stack of terms (and a new subwindow appears on display). The menu code can use this term like it uses term_screen currently (Term_get_size(), Term_erase(), Term_gotoxy(), Term_putstr() should all work, and the menu still will be displayed in the new subwindow). Popping this temporary term frees it (and the subwindow disappears).
Why "term_hints"? Well, maybe the new term cannot be centered in term_screen; perhaps term_screen is too small and in the corner of display. Or maybe the frontend (main-something.c) doesn't know how to draw borders. In any case, the frontend should try to display menus in an attractive and useful way, based on information in hints.
I hope this will allow to use current menu code almost as it is; just screen_save() and screen_load() will have to be replaced by Term_push() and Term_pop(), and also stuff like region_erase() won't be necessary. menu_refresh() also does this thing:

    if (reset_screen) {
        screen_load();
        screen_save();
    }

New term can just be cleared (Term_clear()) instead.

Actually, even stuff like Term->wid can be used pretty much as it is:

#define Term \
    Term_top()

term *Term_top(void)
{
    return term_stack.stack[term_stack.top];
}

term_stack.stack[0] is always term_screen; trying to pop it (making the stack completely empty) is a fatal error. OTOH, pushing term_screen on top of term_screen in the stack is ok (just like currently with term *old = Term; Term_activate(term_screen); Term_activate(old); old can already be term_screen).
That doesn't have to be a big stack. With current textui the max depth of the stack would be what, 3? (term_screen, store interface and item description, for instance)

Term_fresh() reform

Currently, Term_fresh() performs two functions; it flushes term's contents (via Term_fresh_row_*()) and it demands that (as main-xxx.c says) "This action should make sure that all 'output' to the window will actually appear on the window". The latter is impossible. Therefore, Term_fresh() should be split into two functions: Term_fresh() (much like it is now, but without calling Term_xtra()) and Term_redraw_screen():

void Term_redraw_screen(void)
{
    Term_xtra(TERM_XTRA_REDRAW, 0);
}

By far the most useful moment to update the screen is immediately before trying to get user input. That is, before pressing a key the user should know the latest information about the state of the game. In simpler words: Term_redraw_screen() should be called before calling inkey_ex().
Another useful moment to refresh the screen is various animations. Splashscreen ("Initializing monsters" etc) is also an animation in this sense.
In most other situations redrawing the screen is not so useful; in particular, it's not a useful thing to do after updating a single tile of the map.

Colors reform

In text mode, angband's terrain has only 3 different colors, and two of those are used for hybrid_walls/solid_walls. Why doesn't text hook make any use of term_win->vta? Current situation:

struct term {
    errr (*text_hook)(int x, int y,
            int n,
            int a,
            const wchar_t *s);

    errr (*pict_hook)(int x, int y,
            int n,
            const int *ap, const wchar_t *cp,
            const int *tap, const wchar_t *tcp);
}

The hackery with "a / MAX_COLORS", "a % MAX_COLORS" is just bizarre. Term->text_hook() should get tap and tcp just like pict_hook. Even if it's not very useful now, it may become useful over time. attrs could be something better than ints: for instance, uint32_t (one byte for each of red, green, blue, alpha channels). More colors will allow to achieve some interesting cosmetic effects; slightly transparent clear icky things, lantern light that gradually becomes more dim with the distance from the player, etc. Or how about a DCSS-style minimap?
uint32_t could still be used for the "a / MAX_COLORS" thing in some kind of "low color mode" (perhaps a new grafmode).

Tilesets

Note that if term_screen can only ever have either tiles or text, the job of ui2-term.c is significantly simplified:

errr Term_fresh(void)
{
    ...
    if (use_graphics) {
        Term_fresh_row_pict();
    } else {
        Term_fresh_row_text();
    }
    ...
}

Those massive tilesets look like a pain to update or modify. How about splitting them into separate files? The .prf file could look like this:

# <darkness>
feat:0:*:0:Something.png

# open floor
feat:1:torch:1:Open floor torch.png
feat:1:los:2:Open floor los.png
feat:1:lit:3:Open floor lit.png
feat:1:dark:4:Open floor dark.png

something:...:5:filename.png
something:...:6:filename.png
something:...:7:filename.png

The frontend will have to read all .png files, create an array of pixels, fill it with contents of files (in this particular order) and remember offsets into it. ui-prefs.c will ignore the file name part (*.png), and use the file number part (that precedes the file name) as char. attr then could be used for color moding the tile, or just be unused.
Actually, the file number part is just for illustration; it's not even necessary.
Well, tilesets currently work ok so I guess that's not that urgent.

Input

Why do terms have key_queue at all? Only one term ever does something with it. term_screen's key_queue may just as well be separated from term_screen and become its own thing.

So that't my current plan for textui reform. The main change is that any frontend is assumed to be capable of creating and destroying subwindows at will; I know that at least one official frontend can do it (main-sdl.c), and of course main-sdl2.c doesn't have any problems with that either. I don't see why any graphical frontend would find it too difficult. Ncurses, OTOH, needs the functionality of Term_load() and Term_save(). It's probably possible to implement fallback to term_win_copy() for backwards compatibility, but right now that doesn't concern me.