naev
naev copied to clipboard
naev
Refactoring input.c to use enums
Will rewrite SDL_getKeybind and SDL_setKeybind to use enums for now. Later, based on whether we pass enums to Lua or not, will either leave as is, or have a string -> enum mapping
As mentioned above, the array no longer needs the order to be exact; I could write a macro to do the same, but I doubt it would make a difference. Will now rewrite the internal calls to use enums.
With 0.11.0 out, it would be interesting to look into redoing input.c, do you still have the intention to finish this PR? Thanks!
I do, and I'm nearly done. But I'm not sure how to turn the string into a number in less than O(n), because otherwise strcmp() is just as useful, right?
The only time you need to turn a string into a number is when you do a lookup of a keybind from name, no? In that sense, it doesn't really matter, but if you keep the table ordered by names, you can use bsearch with strcmp to have O(logn) lookups.
I think I found a better solution, since these are strings. I'll use a trie and compare letter by letter. I'll also split setKeybind into a c internal one and an external one to be called from lua. A similar approach for the others should work.
Which are the functions called by the lua interface? I'll wrap them with the string -> enum function.
Which are the functions called by the lua interface? I'll wrap them with the string -> enum function.
In src/nlua_naev.c all the functions that start with key, e.g., naevL_keyGet.
What is hparam (from input.c) and toolkit_getListPos (from option.c)? I need to rewrite these to use the enum instead of strings.
What is hparam (from input.c) and toolkit_getListPos (from option.c)? I need to rewrite these to use the enum instead of strings.
"hparam" are hook parameters that are passed to hooks. In this case, when any input is pressed, it runs a hook. You should pass a string representing the key name (usable by the naev.key* API).
toolkit_getListPos gets the current selected index in the list. You probably won't have to touch this, since this is relative to the toolkit list. However, you have to make sure the indices there match the ones in your enum or you'll have to do some translation.
Can I get a resource for what the order is in toolkit_* and what the names are in hparam?
For the latter, if it is just the old strings, just let me know. I have those strings with me. However, if possible or sensible, perhaps they ought to be shifted to be the same as the brief description?
w.r.t the trie, do we already have an implementation? Also, is there a standard file/set of files where such data structures are usually put, or can I put it in input.h?
Can I get a resource for what the order is in toolkit_* and what the names are in hparam?
For the latter, if it is just the old strings, just let me know. I have those strings with me. However, if possible or sensible, perhaps they ought to be shifted to be the same as the brief description?
The names in hparam are just the old strings. It should be the strings you use with functions like naev.keyGet so names. Not sure if it is used in the code, but would probably make sense to check for input hooks and see what names they are using.
The order of toolkit_* is whatever is used when creating it. IIRC, we just pass the list of keys, so it would be in the native order of the array in input.c
w.r.t the trie, do we already have an implementation? Also, is there a standard file/set of files where such data structures are usually put, or can I put it in input.h?
AFAIK no. I would probably toss it into trie.[ch], but honestly, I don't think it may be necessary to use a trie. Just using a sorted array, and then bsearch (like we do in most places) should be fast enough.
Having a conflict or two in conf.c, not sure what that's trying to do, o/w good to go once I sort that list lexicographically. Sorry about the radio silence, it's been a very busy semester. Please lmk if there's a way to sort it other than by hand, been putting it off cos of that :(
I've fixed almost everything in the input branch. One major issue is that, since the old strings have been removed, it won't be able to load old config files. Would be a good idea to have a translation table and code path to load old configs and move to the new format, at least until 0.13.0 or so. I've added a codepath to do that based on the brief field of input_keybinds. However, this field is currently not populated and set to NULL so it won't work. Could you populate the field again? Thanks.
OK, so the old way of doing it was we had brief and name field. So brief="switchtab0", name="Switch Tab 0". You got rid of the brief field, so now it's just the name field. However, if we save that, it has spaces in it, so Lua gives errors because you get something like Switch Tab 0=foo instead of switchtab0=foo. I've changed it to use a table and string keys instead so we don't need the brief field anymore. However, old configs will be saving using the old system so we, at least temporarily, need to add the brief field so it can load the old configs and save to the new format. Of course, we can just keep the brief field and use that like before.
My bad, found the problem. I have to set the brief field in the keybind_info, right?
First of all, please merge the input branch into your branch. It should merge cleanly except for the last commits you did.
Second, that does not fix it, you're setting the brief to what was the name, and the description to name. The old brief fields have been all removed completely so they have to be added.
The old code has definitions like:
{ "accel", N_("Accelerate"), N_("Makes your ship accelerate forward.") },
{ "left", N_("Turn Left"), N_("Makes your ship turn left.") },
{ "right", N_("Turn Right"), N_("Makes your ship turn right.") },
{ "reverse", N_("Reverse"), N_("Makes your ship face the direction you're moving from. Useful for braking.") },
{ "stealth", N_("Stealth"), N_("Tries to enter stealth mode.") },
Where the first field is the brief. You replaced that with the enums so they don't exist now.
To the first, very sorry, partway through that now.
To the second, ahh, I see, it's the deleted field which only shows up in find_keys().
Couldn't we just read in the configs in the old format and read them out in the new one? find_keys() can convert the string to a KeySemanticType
To the first, very sorry, partway through that now.
Are you merging the input branch not the main branch? I had fixed all the merge conflicts for you (you can reset to the input branch and force-push if you want).
To the second, ahh, I see, it's the deleted field which only shows up in find_keys().
Ah, I see. I must have missed that function. Yes, that would work.
Couldn't we just read in the configs in the old format and read them out in the new one? find_keys() can convert the string to a KeySemanticType
We need the reverse of the find_keys function. That is, take a KeySemanticType and output the brief string, then it should be trivial to solve all. We can even just go back to the old config format, which could be a good idea.
Honestly, since find_key is barely used (only in nlua_naev, and probably conf in the near future), it would be easier to just add it to keybind_info and do linear search with find_key and the reverse function (you can't binary search both directions anyway). That way everything is more compact and simple without additional giant arrays. To add or remove a keybinding all you have to do is mess with the one array at the top.
As of now, I've tried to just handle things by putting the brief back in.
Honestly, since
find_keyis barely used (only in nlua_naev, and probably conf in the near future), it would be easier to just add it tokeybind_infoand do linear search withfind_keyand the reverse function (you can't binary search both directions anyway). That way everything is more compact and simple without additional giant arrays. To add or remove a keybinding all you have to do is mess with the one array at the top.
From the standpoint of reliability, I agree. All it takes to mess with the current sorted array is one key in the wrong place.
Are you merging the input branch not the main branch? I had fixed all the merge conflicts for you (you can reset to the input branch and force-push if you want).
I was merging input when I wrote the message, but I had already merged main.
We need the reverse of the find_keys function. That is, take a KeySemanticType and output the brief string, then it should be trivial to solve all. We can even just go back to the old config format, which could be a good idea.
Correct me if I'm wrong, but our current loading is: {"accel":
OK, cool.
Correct me if I'm wrong, but our current loading is: {"accel": , ...} or something like that, right? Couldn't we load "accel" as 0 and store it as {0: ,...} (The dictionary bit is just representative)
Right now the format is like:
-- Targets the nearest non-disabled ship.
target_nearest = { type = "keyboard", mod = "none", key = "N" } > setting non-standard global variable 'target_nearest'
-- Cycles through hostile ship targets.
target_nextHostile = { type = "keyboard", mod = "ctrl", key = "R" } >
I had missed your find_keys stuff (and it was breaking the conf on load because of invalid Lua), so I swapped it to something like:
keybinds = {}
-- Targets the nearest non-disabled ship.
keybinds["Target Nearest"] = { type = "keyboard", mod = "none", key = "N" } > setting non-standard global variable 'target_nearest'
-- Cycles through hostile ship targets.
keybinds["Target Hostile"] = { type = "keyboard", mod = "ctrl", key = "R" } >
Would make sense to revert it back. I can do that easy with the brief fields.
BTW, it still seems like it's not compiling. I would recommend resetting to the input branch, force-pushing, and working from there.
It was an IDE issue on my end locally, it's hopefully not an issue now.
EDIT: seems to be compiling now
Where is the
-- Targets the nearest non-disabled ship. target_nearest = { type = "keyboard", mod = "none", key = "N" } > setting non-standard global variable 'target_nearest' -- Cycles through hostile ship targets. target_nextHostile = { type = "keyboard", mod = "ctrl", key = "R" } >
code? I might be able to make it work with find_keys and no brief. It would just be an if condition on the load, right? If you see a string then find_keys, if it's a passable int then use KST lookup directly, and save just with KST. This would make the config not readable absent the list of KST->number mappings, though we could just print the mappings at the bottom of the config or something using preprocessor directives
I've reverted the config stuff to input branch. However, I've noticed that the keybindings don't seem to work well. In particular, I use the arrow keys, but they seem to not work as expected. I may not have time to debug right now though.