Skip to content
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 codeword highlighting #30092

Merged
merged 8 commits into from
Aug 23, 2024

Conversation

SlamBamActionman
Copy link
Member

@SlamBamActionman SlamBamActionman commented Jul 16, 2024

About the PR

Makes any role that has codewords (Traitor and midround Traitor) show assigned codewords as highlighted in chat/voice bubbles.

Also removes "whisper" string from verbs.yml as it would cause unnecessary highlighting.

Also removes "Taste/Touch" string from adjectives.yml because it shouldn't be there.

Why / Balance

Highly requested feature that helps Syndicates collaborate more by better conveying when a codeword is used, and reduces the number of things an agent needs to keep in their head at all times.

Technical details

This works by injecting a [color] tag around any instance of the codeword in chat messages.

As codewords were previously not sent to the client, they now are. A network event is raised when the traitor role is given directed only at the role owner's client; this event provides a clientside-only component that gets filled with the codewords. This should keep the system from being exploited by hacked clients reading the entity components to learn codewords/who is a traitor.

Known issues:
Since the chat code doesn't make any distinction for what is the actual spoken message and what is chat metadata (like channel or character names), any instance of codewords in a chat message gets highlighted. If the codeword is "Lint", someone named "Clint" will have their name highlighted every time they speak. Because of this, certain words (like "whisper") have been removed from the codeword list. There has been a check made to avoid replacing words inside of brackets, which includes formatting tags and channel names.
PR author believes this issue will be uncommon enough that the feature is worth implementing with it, and can be fixed in a future chat refactor.

It also highlights emoted words, but that can be seen as a benefit as an emoteable codeword like "knock" can be used to communicate in an interesting manner (e.g. "John Syndicate knocks on the table").

Media

image
image

  • I have added screenshots/videos to this PR showcasing its changes ingame, or this PR does not require an ingame showcase

Breaking changes

Changelog

🆑

  • add: Codewords are now highlighted for traitors.

@github-actions github-actions bot added the S: Needs Review Status: Requires additional reviews before being fully accepted label Jul 16, 2024
@Cojoke-dot
Copy link
Contributor

Does this highlight words spoken over the announcement console or emotes? I could see a traitor sneaking to the announcement console grab everyone who could be a syndies attention. And of course mime should not be code-wording in emote chat.

@he1acdvv
Copy link
Contributor

honestly pretty dope wtf

@whateverusername0
Copy link

so real. merge now

@SlamBamActionman
Copy link
Member Author

Does this highlight words spoken over the announcement console or emotes? I could see a traitor sneaking to the announcement console grab everyone who could be a syndies attention. And of course mime should not be code-wording in emote chat.

Yes.
image

I think highlighting emotes is a fun addition, even if it's unintended behavior. If the codeword is "glance", having a mime walk up to you and go "Mime McUrists glances at you nervously" sounds like an interesting way to express a codeword (and nothing is stopping a mime from doing that without the word being highlighted anyways).

@SlamBamActionman
Copy link
Member Author

Also, to clarify; there is no check for whether the person speaking the line is a traitor. This highlights any instance of the codeword to make it easier for you to notice and that's all. You will still have to make a judgement call on whether the tider talking about "drip" is using it as a codeword, or if they're just coincidentally saying it.

@Fwutters
Copy link

I'm glad it works on emotes, what with keywords like bow which can be interpreted as the gift decoration OR the gesture itself.

@luizwritescode
Copy link
Contributor

luizwritescode commented Jul 16, 2024

This is great. Now traitors won't need to keep a mental note of different codewords every round. The less players are relying on memory the smoother the gameplay will be.

If the codeword is "Lint", someone named "Clint" will have their name highlighted

To prevent this my first thought would be to use the first "," in a chat message as a delimiter and keep everything before it unhighlighted, but then you would have to do that just for chat because it would break announcements.

Copy link
Contributor

@Piras314 Piras314 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code looks good to me

@SlamBamActionman
Copy link
Member Author

If the codeword is "Lint", someone named "Clint" will have their name highlighted

To prevent this my first thought would be to use the first "," in a chat message as a delimiter and keep everything before it unhighlighted, but then you would have to do that just for chat because it would break announcements.

It's not an impossible thing to fix but it ends up being checking edgecase after edgecase when the real answer is to just refactor chat. But that is a lot of work, so keeping this implementation lightweight will make it much easier to port it to a refactored system whenever that happens.

Content.Client/Roles/RoleCodewordComponent.cs Outdated Show resolved Hide resolved
Content.Client/Roles/RoleCodewordSystem.cs Outdated Show resolved Hide resolved
Content.Server/GameTicking/Rules/TraitorRuleSystem.cs Outdated Show resolved Hide resolved
Content.Shared/Roles/RoleCodewordEvent.cs Outdated Show resolved Hide resolved
@themias
Copy link
Contributor

themias commented Jul 16, 2024

If merged, this will close #27502 and #17790.

@ElPidor

This comment was marked as spam.

Copy link
Member

@Tayrtahn Tayrtahn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me, but I'm gonna wait for a second maintainer opinion.

Copy link

@whateverusername0 whateverusername0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i approve of this message 👍

Content.Client/Roles/RoleCodewordComponent.cs Outdated Show resolved Hide resolved
Content.Client/Roles/RoleCodewordSystem.cs Outdated Show resolved Hide resolved
Content.Server/GameTicking/Rules/TraitorRuleSystem.cs Outdated Show resolved Hide resolved
@DEATHB4DEFEAT
Copy link
Contributor

Would be cool if players could add their own words to highlight.

Content.Server/GameTicking/Rules/TraitorRuleSystem.cs Outdated Show resolved Hide resolved
Content.Server/GameTicking/Rules/TraitorRuleSystem.cs Outdated Show resolved Hide resolved
Content.Client/Roles/RoleCodewordComponent.cs Outdated Show resolved Hide resolved
Content.Client/Roles/RoleCodewordComponent.cs Outdated Show resolved Hide resolved
Resources/Prototypes/Datasets/verbs.yml Outdated Show resolved Hide resolved
Content.Client/Roles/RoleCodewordComponent.cs Outdated Show resolved Hide resolved
@EmoGarbage404 EmoGarbage404 added the S: Awaiting Changes Status: Changes are required before another review can happen label Jul 19, 2024
@UncaughtEx
Copy link

Would be cool if players could add their own words to highlight.

I second this notion as I've found this type of feature useful in SS13. I believe it would be a marvelous addition to SS14 and allow players to better react to chat in cases such as people calling for your department.

@github-actions github-actions bot added the S: Merge Conflict Status: Needs to resolve merge conflicts before it can be accepted label Jul 20, 2024
Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@SlamBamActionman
Copy link
Member Author

image

New color which is a bit lighter but not eye-bleeding red.

# Conflicts:
#	Content.Server/GameTicking/Rules/TraitorRuleSystem.cs
@github-actions github-actions bot removed the S: Merge Conflict Status: Needs to resolve merge conflicts before it can be accepted label Jul 20, 2024
@github-actions github-actions bot removed the S: Awaiting Changes Status: Changes are required before another review can happen label Jul 20, 2024
@Reese1243
Copy link

this looks awesome

@TheGoudaMan
Copy link
Contributor

TheGoudaMan commented Jul 24, 2024

I'd like to mention that because of chat system issues (as author mentioned) words would most likely get highlighted where they clearly not supposed to, such as OOC, LOOC, etc.

I'm kinda lazy to test it out, but from brief look, code doesn't include any regex or something like that, so I suppose that's the case here since its not first time ppl tried to implement this feature.

And probably it can break entire message rendering if codeword matches message tags? For example we may have tag "red" and codeword "red" and it may mess up message rendering because we'll wrap one tag into another and end up with white message with random tags or colors in it - please check this one out if haven't already, because it has been an issue before and may be case in here as well.

If it is an issue, you may want to implement some temporary hacky processing, such as regex, or removing tags - matching codes - placing tags back, or remove codewords that matches tags, or doing something smarter. Not sure how well accepted it'll be in here tho...

But overall, I'm okay with it, it's better to have rare issues with UI than having codewords that no one uses

@SlamBamActionman
Copy link
Member Author

It shouldn't be an issue to have a regex check in the tag injection function to ensure no word between [brackets] are swapped out. It is ultimately a band-aid fix and as noted in my response to Emo's review the real answer would be an extensive chat metadata system refactor.

Now I just need to figure out the regex...

@TheGoudaMan
Copy link
Contributor

TheGoudaMan commented Jul 24, 2024

For your information:

  • You may also want to consider filtering out OOC/LOOC/names/channels in that "regex" part of logic, should be doable, but most likely hacky and result in some shitcode - not my recommendation, just something that can be done
  • Also beware that depending on where/how regex processes, some trickery bugs may/may not happen on weird player input, which is something you maybe want to avoid (these even beyond edgecases, but its better to be aware of em if they do happen).
    Sadly, I can't recall exactly these cases, but it's something like player spelling out brackets and they get processed by regex.

Pointing it out just in case, cuz these are annoying and not too easy to spot...

Hopefully these won't occur, or at least not in a bad way. Good luck with PR!

@SlamBamActionman
Copy link
Member Author

  • You may also want to consider filtering out OOC/LOOC/names/channels in that "regex" part of logic, should be doable, but most likely hacky.
  • Also beware that depending on where/how regex processes, some trickery bugs may/may not happen on weird player messages, which is something you maybe want to avoid (these even beyond edgecases, but its better to be aware of em if they do happen).

I do not want to implement fixes for all edgecases where it isn't absolutely necessary; highlighting names and LOOC is ultimately just a minor visual bug and trying for control for them could either be smelly code or errorprone in its own right.

A faulty tag could cause some more breaking issues however, so that should be checked for.

Again, the real solution is to do a larger refactor of chat (the only reason this PR wasn't made half a year ago is because I held off due to a chat refactor that got cancelled), but pending that weeks-long project this PR should suffice.

@SlamBamActionman
Copy link
Member Author

SlamBamActionman commented Jul 24, 2024

And before I get told by some higher power "just do the [weeks long, complicated] chat refactor instead then": this PR would be reusable in that new system, you'd only have to transfer over 2 lines of code.

@SlamBamActionman
Copy link
Member Author

SlamBamActionman commented Jul 29, 2024

Added a regex thing to exclude anything in brackets. This was intended to solve injecting tags inside other tags (which would break them) but turns out that since channel names are also in brackets, those codewords can stay too:
image

Technically this also means that if you write "Oh hello [there] buddy", the "there" would not be highlighted. But that would be a very, very unusual situation in normal gameplay. Unfortunately "whisper" will still have to be removed from codewords since that is not inside brackets.

@VitusVeit
Copy link

Would it be possible to implement the highlighting of custom keywords too? As others already said, users could input in their department, name, or profession, which could greatly benefit keeping track of all messages on comms. I made a visual representation to explain myself better:
Words highlighting

@SlamBamActionman
Copy link
Member Author

Would it be possible to implement the highlighting of custom keywords too? As others already said, users could input in their department, name, or profession, which could greatly benefit keeping track of all messages on comms. I made a visual representation to explain myself better: [image]

The tag injection system this PR added could be used for this, but I don't have any intention to support it out of the gate with this PR due to the added complexity.

@Pumkin69
Copy link

this is my dream PR man the amount of times i say codewords on repeat to blatant syndies and they just don't get it is wild

@deathride58 deathride58 added the S: Undergoing Maintainer Discussion Status: Currently going through an extended discussion amongst maintainers, as per procedure. label Aug 19, 2024
@SlamBamActionman
Copy link
Member Author

Hello! The PR was put up to a maintainer vote which concluded with the following result: Merge.

Maintainers like the concept and feel it's beneficial, but there were reservations around the color injection. This would ideally be solved via a chat refactor, but pending that, maintainers feel this solution is good enough to merge as-is.

@SlamBamActionman SlamBamActionman merged commit 61a1e89 into space-wizards:master Aug 23, 2024
11 checks passed
@ElPidor

This comment was marked as spam.

@UbaserB UbaserB removed the S: Undergoing Maintainer Discussion Status: Currently going through an extended discussion amongst maintainers, as per procedure. label Aug 25, 2024
@VitusVeit VitusVeit mentioned this pull request Aug 25, 2024
2 tasks
Erisfiregamer1 pushed a commit to The-Arcadis-Team/arc-station-14 that referenced this pull request Jan 9, 2025
* Added codeword highlighting

* Updated to support more codeword roles, color is set serverside

* Review feedback

* Change to a Component-based system using SessionSpecific

* Tidied up CanGetState, set Access restrictions on component

* Clean-up

* Makes the injection ignore brackets, restore some codewords, remove "Taste/Touch" from adjectives
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S: Needs Review Status: Requires additional reviews before being fully accepted
Projects
None yet
Development

Successfully merging this pull request may close these issues.