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

Fix borgs being able to drink from buckets and spray bottles. #32964

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

Conversation

amatwiedle
Copy link

@amatwiedle amatwiedle commented Oct 23, 2024

About the PR

Added additional logic to the drink system to prevent borgs with the regular or advanced cleaning module being able to drink the liquid inside buckets and spray bottles in their hand slots.

Why / Balance

Fixes #30775.

Technical details

DrinkSystem

  • TryDrink now checks for if the entity attempting to drink is a borg.

Media

janiborg_drink_fix_compressed.mp4

Requirements

Breaking changes

Changelog

🆑

  • fix: Borgs can no longer drink from tools provided by modules.

Copy link
Contributor

@pheenty pheenty left a comment

Choose a reason for hiding this comment

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

No need to run multiple checks, just use OR operator

Comment on lines 166 to 170
if (!HasComp<BodyComponent>(target))
return false;

if (HasComp<BorgChassisComponent>(target))
return false;
Copy link
Contributor

Choose a reason for hiding this comment

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

 if (!HasComp<BodyComponent>(target) || (HasComp<BorgChassisComponent>(target))
            return false;

Copy link
Author

@amatwiedle amatwiedle Oct 23, 2024

Choose a reason for hiding this comment

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

I did it that way for readability reasons over code conciseness, but it's a simple change I can make.
.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is still readable. Take a look at Content.Client/Doors/AirlockSystem.cs, line 93 for example

Copy link
Member

Choose a reason for hiding this comment

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

I don't think cyborgs should be hardcoded into the DrinkSystem. How does the FoodSystem handle this?

Copy link
Author

Choose a reason for hiding this comment

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

I don't think cyborgs should be hardcoded into the DrinkSystem. How does the FoodSystem handle this?

The food system checks for an UnremoveableComponent and returns out of the TryFeed method if it is present. All items given to borgs via ItemBorgModuleComponent are given UnremoveableComponent, which prevents them from eating. DrinkSystem has no such check, should I use that approach instead?

Copy link
Author

@amatwiedle amatwiedle Oct 24, 2024

Choose a reason for hiding this comment

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

It is also worth mentioning that the FoodSystem checks for a StomachComponent in the TryFeed method, which borgs do not have, while the DrinkSystem does not. If an entity gets past all the checks in TryDrink, then it assumes it has a stomach to put the drink into, which is why the solution is deleted from existence in the case of borgs.

Copy link
Contributor

@pheenty pheenty left a comment

Choose a reason for hiding this comment

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

Seems fine now

@osjarw
Copy link
Contributor

osjarw commented Oct 23, 2024

I believe this should be done with an event, a while back I tried to do something similar.

#27066 (comment)

Artifacts shouldn't be hardcoded into ghost code as it just becomes a big mess for forks, probably put in a bool on the component instead or use an event.

@chromiumboy chromiumboy added the S: Awaiting Changes Status: Changes are required before another review can happen label Oct 24, 2024
@chromiumboy chromiumboy self-assigned this Oct 24, 2024
@SlamBamActionman SlamBamActionman added the S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. label Nov 14, 2024
@beck-thompson beck-thompson added T: Bugfix Type: Bugs and/or bugfixes P2: Raised Priority: Item has a raised priority, indicating it might get increased maintainer attention. D3: Low Difficulty: Some codebase knowledge required. A: Silicons Area: Relates to Silicon roles, including AI. size/XS Denotes a PR that changes 0-9 lines. and removed S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. labels Nov 18, 2024
@github-actions github-actions bot added S: Needs Review Status: Requires additional reviews before being fully accepted and removed S: Awaiting Changes Status: Changes are required before another review can happen labels Dec 20, 2024
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.

Looks good to me and works as intended.

@slarticodefast slarticodefast added S: Approved Status: Reviewed and approved by at least one maintainer; a PR may require another approval. and removed S: Needs Review Status: Requires additional reviews before being fully accepted labels Dec 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: Silicons Area: Relates to Silicon roles, including AI. D3: Low Difficulty: Some codebase knowledge required. P2: Raised Priority: Item has a raised priority, indicating it might get increased maintainer attention. S: Approved Status: Reviewed and approved by at least one maintainer; a PR may require another approval. size/XS Denotes a PR that changes 0-9 lines. T: Bugfix Type: Bugs and/or bugfixes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Janitorial borg module lets you drink out of the bucket
8 participants