angband
angband copied to clipboard
angband
Specific key codes shouldn't be hard-coded
Because Angband is using two separate command sets, we end up with problems like #4977 or the recent one where magic inscriptions to put thrown weapons into the quiver only worked right for the non-roguelike command set.
It also means that anyone who wants to change the keybindings will need to go rooting through the code to fix everywhere keys are checked for.
There ought to be a function to look up the keybinding for a command by command name, so instead of:
keycode_t ignore_key = (OPT(player, rogue_like_commands)) ? KTRL('D') : 'k';
One could write:
keycode_t ignore_key = get_key_by_command_name("ignore");
There is a function, cmd_lookup_key(), declared in ui-game.h. Perhaps that is what should be used in the patch to fix #4977 .
For inscription checking, I wasn't sure if cmd_lookup_key() was appropriate. Most are checked in the layer implementing the UI and already use cmd_lookup_key(). However, some inscriptions are checked below the layer implementing the UI, and it wasn't clear if calling back into the UI layer to lookup the key would be violating the intended layering (all current uses of cmd_lookup_key() are from the ui-*.c files). Also, in the special case of inscription for ignoring, there'd either have to be a way to enter ctrl-D in the directly in the inscription or use some alternate form, say "D" or "^D".
I think if the layers are leaky enough that you have to think about actual keystrokes at all in the game engine, as opposed to higher-level abstractions of player commands, then you might as well use cmd_lookup_key().
To match up with how the UI layer is handled for other things, one way to do it would be to keep cmd_lookup_key() but move its declaration and implementation to cmd-core.h/cmd-core.c. The implementation would use a function pointer to call what's appropriate for the configured UI. The default textui would have a textui_cmd_lookup_key(), with a declaration and implementation in ui-game.h/ui-game.c and could use the current cmd_lookup_key() implementation.
The current lookup procedure is inefficient (O(N) where N is the number of commands) so if doing the key lookup in more places turns out to be a bottleneck, that would have to be optimized.
The current lookup procedure is inefficient (O(N) where N is the number of commands) so if doing the key lookup in more places turns out to be a bottleneck, that would have to be optimized.
It'd have to be hitting the lookup code an awful lot for linear time over such a small set to be a problem.
This seems like a fairly niche issue. The main thing I care about is not using UI commands in the game core.
When I added ignore keys from both key sets to look, I wanted just such a function. I was a little surprised I couldn't find one, but I guess it doesn't come up that often. There's another hard-coded ternary for the look help text that prints out k or ^D, and that would be another place that could benefit from something like this.
Personally, if I ran into something like this again, I would definitely consider addressing it at that time.
The current cases where a command letter is assumed rather than looked up (if #4977 goes through in its current state) are:
- Checking for a '!t' inscription when an item will be replaced by wielding another (do_cmd_wield(); cmd-obj.c:316)
- Checking for '!g' and '=g' inscriptions during autopickup (auto_pickup_okay(); cmd-pickup.c:177-197)
- Checking for a '!k' inscription when deciding whether to ignore an item (object_is_ignored(); obj-ignore.c:585)
- Checking for a '!d' inscription when deciding whether to drop an ignored item (ignore_drop(); obj-ignore.c:657-670)
- Checking for an inscription that places an object in the quiver (preferred_quiver_slot(); obj-gear.c:1204-1208)
With the current keymaps, 1, 3, and 5 have keys that are different between the keymaps. Those differences aren't accounted for in the hardwired assumptions in 1 and 3. All of those instances are not in the layer handling the UI.
I can't help but notice that all of those are magic inscriptions; I wasn't even thinking of this when I wrote #4860
I'm going to set this to 4.2 for now, because 4.2.4 will be a while away and it is clearly less niche than I first thought...
Any thoughts about whether "!t" and "!k" should use the key appropriate for the user's selected keymap? The "!k" is a bit more problematic since "ctrl-D" isn't directly enterable into an inscription.
Not yet; it's a bit of a pain. I don't know if we should try to do something about #4860 at the same time, but I'm cautious about that because everyone will complain about their muscle memory (again). Setting it to 4.2 was fairly optimistic, we may decide to do some or none of it now and set it to Future.
However, some inscriptions are checked below the layer implementing the UI, and it wasn't clear if calling back into the UI layer to lookup the key would be violating the intended layering (all current uses of cmd_lookup_key() are from the ui-*.c files).
For the record, this is a wart I knew about when I was writing the command system, it's a layering failure on my part that I couldn't work out how to fix at the time. You're clearly right that the game code shouldn't call into the ui-* bits though. I remember starting to wonder whether the game core should know about command keysets but now I've seen #4860 I think remove magic inscriptions is a better solution.
