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

Borg Update: Borg Law UI rework, Borg radio rework #30655

Open
wants to merge 21 commits into
base: master
Choose a base branch
from

Conversation

valquaint
Copy link

@valquaint valquaint commented Aug 4, 2024

About the PR

This is the start of what will become hopefully a lot of Quality of Life changes to borgs down the line. This has 2 large changes, one is player-facing, the other is more mechanical. Player-facing, we have the Law UI. It now uses a dropdown at the top to select the channel for stating the laws, and a single button for stating the borg's laws. Mechanically, borgs now only intrinsically have access to the Binary channel, and any additional channels are controlled by a headset. This additionally means that borgs now have a headset slot. Placing a headset on a borg can be done by anyone, however removing it requires the borg to be unlocked and the panel to be open. The adding of encryption keys to Cyborgs. (See #30655 (comment))

Why / Balance

Borgs are currently limited on their uses in many departments. You either need to have a predefined chassis, or you make due with sprinting to the nearest intercom. If there is no science team available to make you a chassis, then you are stuck in a default chassis with absolutely no use to anyone aside from basic tools and minimal radio usage. By giving borgs headset capabilities, any chassis can now be applied to any department to enable communications so long as any member of command (or science team) is on station. This also allows new combinations of borgs, various RP opportunities, and utility of the role.
However, this also means that the Law UI may have more buttons than it may have been designed for, so a small rework of it was necessary. This was also fixed to accommodate the plausible maximum channels a borg could have.

The balancing of this feature/change is that a headset can only be placed onto a borg by someone else, and a borg cannot remove their own headset (unless they are left unlocked). This prevents law abuse of having a borg ordered to remove their own headset.

After much discussion in the discord over the best approach, including attempting to add encryption keys to borgs directly, this was decided as the best approach for the following reasons:

- It makes use of existing systems
- It will use future refactors also being discussed (ie: chatfactor)
- Adding encryption keys to the intrinsic radio was starting to cause breaking changes to other parts of the codebase
- It would expose the binary keys on cyborgs, which is currently only available in syndicate uplinks

This made adding headsets to cyborgs and keeping the Binary key as the only intrinsic radio channel the most logical option to create. See #30655 (comment) for updates to this, as new method has been applied to this PR after discussion and further research.

Technical details

Much of the code has comments where changes were made. Here is a short breakdown of the changes:

Content.Client/Silicons/Laws/Ui/ChannelDisplay.xaml and the respective code behind it was created to house the new dropdown list, to be shown in the Silicon Law Bound UI. It handles the state being passed from bound UI, and what channel the borg is trying to state their laws in.

Content.Client/Silicons/Laws/Ui/LawDisplay.xaml was updated to contain the newly made ChannelDisplay component, and resized to still show all 3 default laws without issue.

Content.Client/Silicons/Laws/Ui/SiliconLawMenu.xaml and the respective code behind was modified to handle the new method of displaying laws, which required passing the entity, and the state, to the law control itself going forward.

Content.Server/Silicons/Borgs/BorgSystem.cs had event listeners added to handle the unequipping of the radios, to detect if the panel was open or not.

Content.Server/Silicons/Laws/SiliconLawSystem.cs was modified for the BoundUI to handle a union of the IntrinsicRadio and a headset to be passed as a single HashSet to the UI.

Content.Shared/Silicons/Laws/Components/SiliconLawBoundComponent.cs was modified to support having a selected channel property that could be accessed in the BuiState

Various yml changes were done to support the above features, including:

  • base_borg_chassis.yml prototypes only have binary intrinsic radios
  • borg_chassis.yml prototypes only have binary intrinsic radios
  • silicon.yml enabled ear slots for borgs and positioned them accordingly

Update August 5th 2024 - While much of this technical rework remains, the system now uses Encryption keys, as explained in the previously linked comment. A new technical explanation is provided for the update:

Content.Client/Silicons/Laws/UI/LawDisplay.xaml was modified to remove a now redundant label

Content.Client/Silicons/Laws/UI/LawDisplay.xaml.cs was modified to change the button color to be more visually appealing and not seem like it was disabled

Content.Client/Silicons/Laws/UI/SiliconLawBoundUserInterface.cs was updated to remove the debugging _sawmill, as it had a significant amount of log spam under specific conditions

Content.Server/Radio/EntitySystems/HeadsetSystem.cs was modified to have radios account for the new possibility that IntrinsicRadios may also have EncryptionKeyHolder components, and that if an ActiveRadio component is found alongside an IntrinsicRadioReceiver component, it should no longer remove the ActiveRadio component if the number of channels falls to 0

Content.Server/Radio/EntitySystems/RadioSystem.cs was modified to accommodate a union of channels between IntrinsicRadio channels, and EncryptionKeyHolder components

Content.Server/Silicons/Laws/SiliconLawSystem.cs was rolled back to no longer check for an ear slot, and updated to have a cleaner check for encryption keys

Content.Shared/Radio/EntitySystems/EncryptionKeySystem.cs was modified to support examining entities with panels better. Specifically, if an entity has a panel, it checks the new component bool variable ExamineWhenOpenOnly to confirm if the keys should be displayed first. Also, if the entity has a panel, it no longer includes the default channel in the examine text.

Content.Shared/Wires/WiresPanelComponent.cs was updated to have the new field ExamineWhenOpenOnly

Resources/Prototypes/Entities/Mobs/Cyborgs/base_borg_chassis.yml was updated to have the components WearingHeadset, Headset, and EncryptionKeyHolder added - all of which were required to make the previous changes function. The default key slots on a borg is set to 8, and the extraction method to Prying for crowbars.

Resources/Prototypes/InventoryTemplates/borg.yml was reverted back to remove ear slot from borgs entirely

Media

Demo of borg headset positions (Willing to reposision) Removed

Demo of UI elements

image
image
image

Short video demo:

2024-08-05.22-27-41.mp4

Requirements

  • I have read and I am following the Pull Request Guidelines. I understand that not doing so may get my pr closed at maintainer’s discretion
  • I have added screenshots/videos to this PR showcasing its changes ingame, or this PR does not require an ingame showcase

Breaking changes

No known breaking changes.

Changelog

🆑 Reisama

  • add: Cyborg radio systems have been updated to accept Encryption keys. Requires panel to be open, and uses a crowbar to remove. Examining a Cyborg with its panel open will show what keys are currently inserted into its chassis. Default channels are Common and Binary, and cannot be removed.
  • tweak: The cyborg law window has been updated to accommodate the new channels.

@github-actions github-actions bot added the Changes: UI Can be reviewed or fixed by people who are knowledgeable with UI design label Aug 4, 2024
@Blackern5000
Copy link
Contributor

Blackern5000 commented Aug 4, 2024

I really don't like the way the headset is offset by half a pixel on the borgs rather than being aligned. Also cyborgs wearing headsets just seems really strange in general. "Borg law 2 give me your headset with security and medical comms."

Assault borgs should probably just keep the keys built-in so people can't steal them.

@LankLTE
Copy link
Contributor

LankLTE commented Aug 5, 2024

Making Borg chassis take encryption keys is a much more elegant way of doing this imho. Them wearing a headset seems weird.

@valquaint
Copy link
Author

I dont mind adjusting the positioning - it was not a top priority getting things aligned. I can work on fixing it.

As for Law 2 "Give me your headset" - As stated above, borgs cannot remove their own headset. It requires someone to have the correct access level to do it, so even if the borg was ordered to, they cannot obey that command.

As for making the chassis take encryption keys, I addressed that above, but I'll also re-post it here for simplicity:

After much discussion in the discord over the best approach, including attempting to add encryption keys to borgs directly, this was decided as the best approach for the following reasons:

  • It makes use of existing systems
  • It will use future refactors also being discussed (ie: chatfactor)
  • Adding encryption keys to the intrinsic radio was starting to cause breaking changes to other parts of the codebase
  • It would expose the binary keys on cyborgs, which is currently only available in syndicate uplinks

This made adding headsets to cyborgs and keeping the Binary key as the only intrinsic radio channel the most logical option to create.

Yes, it was something I also considered, attempted multiple times, and ultimately opted out of for multiple reasons.

@Cojoke-dot
Copy link
Contributor

This made adding headsets to cyborgs and keeping the Binary key as the only intrinsic radio channel the most logical option to create.

They should probably have common too. It would be quite annoying to head to the department you need coms for and not be able to ask anyone in that department to put a headset on you because they are all out.

The state laws button should also probably be a little brighter, it's kind of dim and looks like it's disabled to me.

@slarticodefast
Copy link
Member

Might be worth it to split the new laws UI into a separate PR, or is it required for the encryption keys to work?

@valquaint
Copy link
Author

valquaint commented Aug 5, 2024

They should probably have common too. It would be quite annoying to head to the department you need coms for and not be able to ask anyone in that department to put a headset on you because they are all out.

I considered this, and was kind of 50/50 on it myself. I have no problem adding it back as an intrinsic channel, alongside binary

The state laws button should also probably be a little brighter, it's kind of dim and looks like it's disabled to me.

Easy enough to adjust. I can see how it may seem disabled. I'll brighten it.

Might be worth it to split the new laws UI into a separate PR, or is it required for the encryption keys to work?

Six of one, half dozen of the other. On one hand, the UI could technically be worked into a PR by itself, however it would lose some functionality behind it. On the other, making the radio update into its own PR would horribly break the existing UI and require it to be rewritten anyway, as it would cause buttons to be stacked or cut off with how they are currently rendered on the UI.

Something else I would like to also expand upon are some of the methods I explored to add encryption keys to borgs directly before moving onto the headset method:

Sandboxing with VV

I tried adding the keyholder component to various things to see what would be the best one to actually hold the keys, including the positronic brain or MMI, the chassis, a module, etc. The best candidate for all of these if going this route, in my opinion, would have been adding it to the brain or a module, as adding them to the chassis added the following problems:

Examining the chassis revealed the encryption keys, which can create negative situations. Off the top of my head, the following come to mind:

  • Borgs who have been emagged and subsequently given antag channels (This should be kept metashielded)
  • Borgs with higher level comms that are now higher priority targets for antags
  • The Binary key is now a theft target for anyone, when previously it was only accessible by syndicate uplink

The chassis would now need two tools to work with, otherwise a screwdriver would both open the panel and remove the keys. While a minor yml change, this could get confusing to players.

yml Theorizing

The existing code does not support using IntrinsicRadio alongside EncryptionKeyHolder and would require a much larger and likely code-breaking change than enabling borgs to wear a headset, as more than just borgs make use of IntrinsicRadio.

This being said, I tried adding it anyway to the chassis to see what could happen. I did manage to get a decent amount of the way through this path, up to and including allowing keys to be added to the chassis, however it broke the radio entirely (see initial statement in this section).

Hybridization

Ultimately, the solution I came down to settles on both - Leaving IntrinsicRadio in-tact and allowing borgs to use radios, having the channels borgs should always have in their InstrinsicRadio and relying on the new update to handle the rest. This was the least breaking change I found, and also should support any future chat updates since it relies the most on the existing radio functions, as opposed to what is effectively an immutable list.

@valquaint
Copy link
Author

Ok, taking all of the feedback into account, I've significantly changed how the PR works. I'll outline it below:

Major Changes

  • Borgs no longer have ear slots, nor do they wear headsets. Instead, if their panel is open, you can insert Encryption Keys directly into them.
  • Inserted keys can be removed using a crowbar
  • You cannot view a Cyborg's keys if the panel is not open

Significant Changes

  • New component variable added to WiresPanelComponent, ExamineWhenOpenOnly, that can be used to determine if the examine text will show if the panel is open or closed. This ensures it has no impact on existing components, and is only true on borgs for now
  • Borgs still have intrinsic Binary channel, and also now have Common channel by default. Other channels will require their respective key to be added.

Minor Changes

  • Borgs will no longer have a headset sprite added to them
  • Examining a borg with their panel open will list the keys, but not tell you the default channel of the borg.

I can go into a more in-depth review of how this works in the code if necessary. Feedback is welcome.

Copy link
Member

@slarticodefast slarticodefast left a comment

Choose a reason for hiding this comment

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

Ok, partial review so far and only a little testing. A lot of this can be simplified. The dropdown menu can probably just be a part of LawDisplay.cs and the BUI can call an update function if needed.

Visually the margins around the laws need to be adjusted a little.
Screenshot (232)

Also when I tested it the selectable channels in the law menu only updates when the flashlight is on.

Please make your changelog a little shorter, points 1-3 can easily be combined into a single shorter one. Point 4 can simply be "tweak: Improved the cyborg law menu". You don't have to explain to people how to use buttons.

Content.Client/Silicons/Laws/Ui/ChannelDisplay.xaml.cs Outdated Show resolved Hide resolved
Content.Shared/Wires/WiresPanelComponent.cs Outdated Show resolved Hide resolved
Resources/Locale/en-US/chat/ui/chat-box.ftl Outdated Show resolved Hide resolved
Resources/Locale/en-US/station-laws/laws.ftl Outdated Show resolved Hide resolved
Content.Shared/Radio/Components/HeadsetComponent.cs Outdated Show resolved Hide resolved
Content.Server/Silicons/Borgs/BorgSystem.cs Outdated Show resolved Hide resolved
Content.Client/Silicons/Laws/Ui/LawDisplay.xaml.cs Outdated Show resolved Hide resolved
@slarticodefast slarticodefast added the Status: Awaiting Changes This PR needs its reviews addressed or changes to be made in order to be merged. label Aug 7, 2024
@valquaint
Copy link
Author

Ok, partial review so far and only a little testing. A lot of this can be simplified. The dropdown menu can probably just be a part of LawDisplay.cs and the BUI can call an update function if needed.

Also when I tested it the selectable channels in the law menu only updates when the flashlight is on.

Please make your changelog a little shorter, points 1-3 can easily be combined into a single shorter one. Point 4 can simply be "tweak: Improved the cyborg law menu". You don't have to explain to people how to use buttons.

The dropdown I tried making a part of LawDisplay.cs at first. I determined that it needed to be split out from that, as having it part of the main window when it was updating was causing an issue, probably related to the "flashlight constantly causes updates" bug I've mentioned before.

Similarly, the selectable channels update when the BUI gets an update call, which I was not able to fully tease out. There were a few conditions that I found could reliably trigger that state to update, ranging from closing and opening the window again, opening a different UI, and a few various other triggers. I haven't been able to fully trace that bug, but it did not seem to be directly linked to only the flashlight, though I did see toggling the flashlight to cause a spam of update calls when I added debugging information to that area.

@valquaint
Copy link
Author

Made the requested changes, and some suggested edits from the community in the discord, including:

  • Moved the drop down to the bottom of the UI
  • Touched up the margins to be uniform
  • Changed from BackgroundDark to AngleRect in the LawDisplay.xaml for aesthetic purposes

Every other suggested change was tried, and agreed with. Also updated the CL above.

Content.Client/Silicons/Laws/Ui/LawDisplay.xaml.cs Outdated Show resolved Hide resolved
Content.Client/Silicons/Laws/Ui/SiliconLawMenu.xaml.cs Outdated Show resolved Hide resolved
Content.Server/Radio/EntitySystems/RadioSystem.cs Outdated Show resolved Hide resolved
Content.Server/Silicons/Laws/SiliconLawSystem.cs Outdated Show resolved Hide resolved
@github-actions github-actions bot added the Status: Needs Review This PR requires new reviews before it can be merged. label Aug 10, 2024
@github-actions github-actions bot removed the Status: Awaiting Changes This PR needs its reviews addressed or changes to be made in order to be merged. label Aug 10, 2024
@valquaint
Copy link
Author

All changes to this point are completed.

@ScarKy0
Copy link
Contributor

ScarKy0 commented Aug 25, 2024

I have a singular issue being that at some point youd run out keys if you make 2-3 borgs for a department(?)
I have no idea how common encryption keys are and what are the ways to obtain them though, so there is a big chance my concern is just invalid

@valquaint
Copy link
Author

I have a singular issue being that at some point youd run out keys if you make 2-3 borgs for a department(?)
I have no idea how common encryption keys are and what are the ways to obtain them though, so there is a big chance my concern is just invalid

There are usually at least 2 ways to get encryption keys for all non-command roles. Either through a mapped locker, or the respective wardrobe vendor. Most official maps have at least 8 headsets per department floating around if you know where to look, and command roles generally have access to more for their departments as well.

Getting the keys for the borgs wont be an issue. Unless, of course, you want to give a borg a specialized key - but those are hard to get for a reason, and if you're doing that then you already probably have an edge case where you have the spare key to shove it the borg already.

@JIPDawg
Copy link
Contributor

JIPDawg commented Aug 26, 2024

I have a singular issue being that at some point youd run out keys if you make 2-3 borgs for a department(?) I have no idea how common encryption keys are and what are the ways to obtain them though, so there is a big chance my concern is just invalid

I'm like 90% positive that all command roles have a box of their departments keys in their locker which has like 5 in it. Then hop has some more. Should be more than enough.

@ScarKy0
Copy link
Contributor

ScarKy0 commented Aug 26, 2024

Yeah if there are so many keys around then I have no issues with this, looking forward to this if it gets merged

@jamilnielsen
Copy link

i noticed you removed Science default key, i think most borgs/robotics would rather this stays untouched.
i don't see a reason to add an extra step to that channel specifically, i can agree to the rest.

also as a borg main i will have to disagree big time to "stuck in a default chassis with absolutely no use to anyone..."
specialized are not perfect, 99% of the time i dont need it, and its good RP to let those who sign up for it do it.

PPS: while we are on it, this might be a stretch, but could i bother you to consider making a PR for round start borg (start name and chassis choice)?
those would be cosmetic and RP choices i really want.

@Chubbicous
Copy link
Contributor

Chubbicous commented Aug 26, 2024

Do different bodies still start with their department's channels? (e.g. Does engiborg still keep science/engineering by default)
If not, I could see it getting annoying fast requiring a scientist to get one or two keys every time just for a borg to be able to do its job.

PPS: while we are on it, this might be a stretch, but could i bother you to consider making a PR for round start borg (start name and chassis choice)?

See #30406 for loadouts

@github-actions github-actions bot added the Merge Conflict This PR currently has conflicts that need to be addressed. label Aug 28, 2024
Copy link
Contributor

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Changes: UI Can be reviewed or fixed by people who are knowledgeable with UI design Merge Conflict This PR currently has conflicts that need to be addressed. Status: Needs Review This PR requires new reviews before it can be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants