-
Notifications
You must be signed in to change notification settings - Fork 69
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add support for generic player chat commands (trigger words) #427
Add support for generic player chat commands (trigger words) #427
Conversation
add recent_actions
Add dataclass "MostRecentsEvents"
Add recent_actions.py
in progress...
Seems quite good for me, but I'd prefer FlorianSW to approve this one. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Didn't look that much into the other parts from what I commented. But first let us sort out my comments, please :)
rconweb/api/user_settings.py
Outdated
@@ -2474,6 +2474,85 @@ def set_watchlist_discord_webhooks_config(request): | |||
) | |||
|
|||
|
|||
@csrf_exempt | |||
@login_required() | |||
# TODO permission |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this going to be added in this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, yeah I need to add that, good catch.
), | ||
"1234", | ||
MostRecentEvents( | ||
last_victim_name="some guy", last_victim_weapon="some weapon" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
last_victim_name does not exist on MostRecentEvents? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of date from when I had them merged, then split them for reduced cache size, but we decided to put them back for ease of use.
tests/test_message_variables.py
Outdated
monkeypatch.setattr( | ||
rcon.hooks, | ||
"get_recent_actions", | ||
lambda x: {steam_id_64: recent_event}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be:
lambda: {steam_id_64: recent_event},
? (without the x argument)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah whoops, this was out of date from before I split the cache into two calls, one to set and one to get.
@@ -76,6 +89,97 @@ def initialise_vote_map(rcon: Rcon, struct_log): | |||
logger.exception("Something went wrong in vote map init") | |||
|
|||
|
|||
@on_chat |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's really hard for me to get through this code, tbh :D First of all: Searching for all these cache entries for the victim and nemesis seems unneeded and actually wrong, as all required information is already available through the player_1_cache entry, if I'm not mistaken.
I walked through the code locally and running the tests revealed them being red as well. My suggested code seems to work (at least the tests are green and I'm pretty sure it is doing what it is supposed to do based on my understanding of the code):
@on_chat | |
@on_chat | |
def trigger_words(rcon: Rcon, struct_log: StructuredLogLineType): | |
config = TriggerWordsUserConfig.load_from_db() | |
if not config.enabled: | |
logger.debug("Trigger words are disabled") | |
return | |
chat_message = struct_log["sub_content"] | |
if chat_message is None: | |
return | |
event_cache = get_recent_actions() | |
steam_id_64: str = struct_log["steam_id_64_1"] | |
if steam_id_64 is None: | |
return | |
player_1_cache = event_cache.get(steam_id_64, MostRecentEvents()) | |
chat_words = set(re.split(CHAT_WORDS_RE, chat_message)) | |
for trigger in config.trigger_words: | |
if not ( | |
triggered := contains_triggering_word(chat_words, trigger.words) | |
) and not contains_triggering_word(chat_words, config.describe_words): | |
continue | |
if triggered: | |
message_vars: list[str] = re.findall(MESSAGE_VAR_RE, trigger.message) | |
populated_variables = populate_message_variables( | |
vars=message_vars, steam_id_64=steam_id_64 | |
) | |
formatted_message = format_message_string( | |
trigger.message, | |
populated_variables=populated_variables, | |
context={ | |
MessageVariableContext.player_name.value: struct_log["player"], | |
MessageVariableContext.player_steam_id_64.value: steam_id_64, | |
MessageVariableContext.last_victim_steam_id_64.value: player_1_cache.last_victim_steam_id_64, | |
MessageVariableContext.last_victim_name.value: player_1_cache.last_victim_name, | |
MessageVariableContext.last_victim_weapon.value: player_1_cache.last_victim_weapon, | |
MessageVariableContext.last_nemesis_steam_id_64.value: player_1_cache.last_nemesis_steam_id_64, | |
MessageVariableContext.last_nemesis_name.value: player_1_cache.last_nemesis_name, | |
MessageVariableContext.last_nemesis_weapon.value: player_1_cache.last_nemesis_weapon, | |
MessageVariableContext.last_tk_nemesis_steam_id_64.value: player_1_cache.last_tk_nemesis_steam_id_64, | |
MessageVariableContext.last_tk_nemesis_name.value: player_1_cache.last_tk_nemesis_name, | |
MessageVariableContext.last_tk_nemesis_weapon.value: player_1_cache.last_tk_nemesis_weapon, | |
MessageVariableContext.last_tk_victim_steam_id_64.value: player_1_cache.last_tk_victim_steam_id_64, | |
MessageVariableContext.last_tk_victim_name.value: player_1_cache.last_tk_nemesis_name, | |
MessageVariableContext.last_tk_victim_weapon.value: player_1_cache.last_tk_victim_weapon, | |
}, | |
) | |
rcon.do_message_player( | |
steam_id_64=struct_log["steam_id_64_1"], | |
message=formatted_message, | |
save_message=False, | |
) | |
else: | |
description = config.describe_trigger_words() | |
if description: | |
rcon.do_message_player( | |
steam_id_64=struct_log["steam_id_64_1"], | |
message="\n".join(description), | |
save_message=False, | |
) | |
else: | |
logger.warning( | |
"No descriptions set for trigger words, %s", | |
", ".join(config.describe_words), | |
) |
|
||
|
||
@dataclass | ||
class MostRecentEvents: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The following fields seem to be missing? 🤔
last_victim_name
last_nemesis_name
last_tk_victim_name
last_tk_nemesis_name
?
rcon/hooks.py
Outdated
else MostRecentEvents() | ||
) | ||
|
||
chat_words = set(re.split(CHAT_WORDS_RE, chat_message)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The second thing I have trouble to understand, tbh 😅
I think the only reason of this code is to separate words based to be able to look for trigger words, right? What I don't get is: Why would we separate !who-killed-me
to ["!who", "killed", "me"]
? Shouldn't it be one entry in the array as the whole group of words? Like only splitting by spaces? It would be as simple as:
chat_words = chat_message.split(' ')
After all: why would you need to split the chat words at all? Wouldn't a simple "contains" for the trigger.words be sufficient? 🤔
Okay I:
|
|
||
set_recent_actions(cached_actions) | ||
# Refresh the redis cache so it's available for interprocess communication | ||
get_recent_actions() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds like premature optimization to me :D But it's ok.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's actually required if any other process wants to be able to pull it out of the redis cache, it clears on setting, and any other process that was to call it would just get back a totally empty one.
I don't know if we even want it available in any other process, but if another service wants those values for any reason it has to be done like this (or persisted to the database)
Looks good :) Feel free to merge after the conflicts are resolved 🤗 |
MostRecentEvents
) that is updated every kill or team killdiscord_invite_url
option toRconServerSettingsType
SafeStringFormat
a subclass ofdict
that will accept any formatted string and replace missing keys with the key itself to avoid exceptions when using"".format_map()"
This supports two types of information, info we can get from anywhere in the RCON without any extra details (server name, etc.) and info that is only available depending on the context. This is represented by two types (
MessageVariable
andMessageVariableContext
).It does pass in the
steam_id_64
when used inrcon.hooks.trigger_words
since the chatting players steam ID is always available and allows us to look up VIP status, etc.Users can add as many commands/command formats as they want, but they can only add
{variable}
style variables if they're either in theMessageVariable
orMessageVariableContext
enums to avoid typos, etc.This does require some more work to support the rest of the scoreboard types, and if this is an acceptable way to do this, we could use this across the board in other places in RCON (automods, etc.) to allow people more options when automatically messaging players.
We can also use this to add end of round stuff for #392