Auto-detect and use character specific hotkeys via key file #905
Conversation
Ezro
changed the title
Ezro
force-pushed
the
feature/auto_detect_hotkeys
branch
from
f086892
to
59f7681
Compare
Ezro
added
work in progress
ready for review
src/template_finder.py
Outdated
| @@ -60,10 +60,37 @@ def stored_templates() -> dict[Template]: | |||
| ) | |||
| return templates | |||
|
|
|||
| @cache | |||
| def stored_templates_by_dir() -> dict[Template]: | |||
There was a problem hiding this comment.
I dont like that we bascially copy the stored_templates() functionality here. I'd rather go for adding the dir normally to TEMPLATE_PATHS and to get all the specific template names loop through folder and get names of the files in a list.
There was a problem hiding this comment.
Then we also can ditch the search_all_templates() function.
There was a problem hiding this comment.
stored_templates_by_dir and search_all_templates have been removed
There was a problem hiding this comment.
@Ezro would you be willing to create a new test case for the new functionality? Just add here https://github.com/aeon0/botty/blob/master/test/template_finder_test.py
There was a problem hiding this comment.
@mgleed I added some asserts but they're very basic; please validate
|
One thing that needs to be tested here is localization. Botty currently allows users to select characters that have names that aren't in English alphabet. Not sure if this will break things if people have a keyfile with kanji, for example. |
Edit: I've updated the PR to no longer print the key file name |
|
@Ezro left a few comments earlier |
|
From my side it looks good. I leave it to gleed since he seems to still have some topics. |
|
Also, why'd you choose to stick with precise keyfile name rather than use regexp with character name like we discussed? |
There was a problem hiding this comment.
Overall very well done! See comments. Thanks for doing this.
Last wish list item for this PR would be basic validation.
There are some hotkeys that simply cannot be unset. Like force move, etc. Do you think it'd be hard to implement some basic validation for required hotkeys?
src/template_finder.py
Outdated
| @@ -60,10 +60,37 @@ def stored_templates() -> dict[Template]: | |||
| ) | |||
| return templates | |||
|
|
|||
| @cache | |||
| def stored_templates_by_dir() -> dict[Template]: | |||
There was a problem hiding this comment.
@Ezro would you be willing to create a new test case for the new functionality? Just add here https://github.com/aeon0/botty/blob/master/test/template_finder_test.py
I think we can implement that very easily (at some point). Here's the current spot for parsing the key block into actual hotkeys |
I haven't tested this but I think it's better to be explicit; I have a feeling that the hash is there for multi-account handling; e.g., if account A has character 'Paladin' and account B has character 'Paladin' and both were played online on the same PC then we wouldn't actually be able to determine which Paladin file to use. This adds a bit of overhead for the users (to find and enter the explicit key file name) but I think the trade-off is that Botty wouldn't have to do decision making logic for choosing / erroring. |
That's a good point and I'd bet your theory is correct. |
Ezro
force-pushed
the
feature/auto_detect_hotkeys
branch
2 times, most recently
from
fa84dff
to
eef2a92
Compare
Ezro
force-pushed
the
feature/auto_detect_hotkeys
branch
from
813a6c1
to
df2e5ac
Compare
Ezro
force-pushed
the
feature/auto_detect_hotkeys
branch
from
d463ed2
to
c1d0ddf
Compare
|
Are you able to recover my test that I wrote in template_finder_test.py for this? Looks like that got overwritten with a force push. |
Ezro
force-pushed
the
feature/auto_detect_hotkeys
branch
from
007c463
to
4e18c84
Compare
This implementation will read the provided 'key_file' param (which should map to the exact name in the saved_games_folder) and parse the hotkeys for the provided character. Once Botty goes into game it will then run a "discovery" to try to determine what skills are bound to the discovered hotkeys.
The PR should auto-detect sorc, pally, barb, necro, sin, and druid skill. The existing char scripts should be updated to use the proposed hotkey solution.
An important thing to note is that the implementation currently assumes that Quick Casting is disabled; if we plan on shifting to using Quick Casting then I think this solution can be tweaked to hold the keybind prior to taking the screenshot.