Inventory/equipment window: tile to ASCII mode change

[backwardsEric]

I'm adding tile support to the Mac interface and encountered a problem in the following situation:

1. While in the dungeon with the inventory window open, switch from MicroChasm's tiles (with use_bigtile on) to ASCII.
2. The Mac interface responds to that by calling do_cmd_redraw().
3. In response to the Term_react() call in do_cmd_redraw(), the Mac interface sets use_graphics to GRAPHICS_NONE, use_transparency to FALSE, use_bigtile to FALSE, sets higher_pict on all the terminals to zero, calls reset_visuals(TRUE), and calls verify_panel().
4. When Term_redraw() is called from do_cmd_redraw() for the inventory window that leads to text being pushed to the Mac interface which has, for x = 4 and y = 0, the color set to 255 and the character code to -1, i.e. the attributes of padding for a big tile. Downstream from that, the Mac interface has an assertion checking the color value and crashes.

It looks like modifying display_inven() and display_equip() to always redraw the column (column 4 with use_bigtile off; column 5 with use_bigtile on) between the equippy character/tile and the name of the item would fix the issue. However, there are at least two other options: have the Mac interface not allow switching the graphics mode while a game is in progress (does the Windows interface allow that?) or call Term_clear() for terminals other than the current one in do_cmd_redraw() (Angband 4.2.1 doesn't do that, it only calls Term_clear() for the current terminal; Hengband's version doesn't call Term_clear() at all).

[sil-quirk]

Windows does permit changing the graphics mode for a game in progress, Linux does not. Despite having squashed a number of bugs on Windows while switching graphics mode while playing I can't guarantee it's perfect, but it doesn't crash even with the inventory window open.

If I understand the issue right it appears that column 5 retains the invalid bigtile value on redraw - and I can see how this could happen as the Term_putstr function doesn't touch the column between the tile and the text on the redraw. Is this correct?

Does changing format("%c", object_char(o_ptr))); to format("%c ", object_char(o_ptr)));, adding a space to overwrite the next column, in line 1901 of object1.c fix this? If so we probably need to do the same in display_equip().

[sil-quirk]

Thanks again by the way for the work you're putting in to make the Mac port happen.

[backwardsEric]

The crash is because the Mac interface is checking the color value with the equivalent of assert(color >= 0 && color / MAX_COLOR <= BG_DARK). With the other front ends, I'd expect there could be visual glitches in the x=4 column after a tile to ASCII switch (as you say the current equipment/inventory redraw in ASCII mode doesn't update that column to overwrite the big tile padding) or in the x=5 column after a ASCII to tile switch (the leading character from the object name is left in place rather than being overwritten with a space).

Changing Term_putstr(3, i, 1, object_attr(o_ptr), format("%c", object_char(o_ptr))) to Term_putstr(3, i, 2, object_attr(o_ptr), format("%c ", object_char(o_ptr))) does handle the tile to ASCII transition problem for the inventory window. It doesn't for the equipment window if there's empty equipment slots. object_char() for an empty slot in ASCII graphics mode is giving a null character (at least when compiled on the Mac) so the string passed to Term_putstr() is equivalent to an empty string.

I also tried this code for drawing the symbol in display_equip(),

/* Display the symbol */
Term_putch(3, i - INVEN_WIELD, object_attr(o_ptr), object_char(o_ptr));
if (use_bigtile)
{
    Term_putch(4, i - INVEN_WIELD, 255, -1);
}

...
/* Display the entry itself */
Term_putch(offset - 1, i - INVEN_WIELD, attr, '  ');
Term_pustr(offset, i - INVEN_WIELD, n, attr, o_name);

, to handle the clearing for both transitions. That also doesn't work. Term_putch() rejects the null character from object_attr() in ASCII graphics mode for an empty equipment slot, and the x=3 column isn't updated. Another version of that, having a special case for o_ptr->tval == 0 when use_graphics is 0, is up as a pull request. That works on the Mac, but having to test for use_graphics seems like an ugly hack.

[sil-quirk]

An alternative would be to modify the object_attr macro to always return space while graphics are in ASCII mode - this would protect against any other usages elsewhere. Unfortunately the macro is already indecently complicated; the codebase in general is a bit of a horror story on more fronts than I can count, and I've elected not to attempt to substantially rewrite it on the basis that a proper cleanup would stall any feature additions for months or more. It might be worth promoting it to a function.

However, given that we have both use_bigtile and object_attr (which returns different things depending on whether graphics are on or not) in the function, I'm not sure use_graphics worsens things materially. I'm inclined to accept unless you have second thoughts?

[backwardsEric]

If it's acceptable for you, I don't have anything better since I didn't try to understand what object_char() and object_attr() are doing. Having object_attr() and object_char() as functions could make it easier when stepping through with a debugger. Also, I flubbed the comment in that pull request: both object_attr() and object_char() return '\0' for an empty equipment slot in ASCII mode, but it's the character value that's being changed to a ' '.

[sil-quirk]

Have merged and fixed the comment.

[sil-quirk]

Going to close this as fixed, but if there are any other problems related to this please feel free to reopen this or open a new issue.