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

LogSystem/SharpSystem replaced by SliceableSystem/ButcherableSystem, etc. #34851

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

Conversation

Plykiya
Copy link

@Plykiya Plykiya commented Feb 3, 2025

About the PR

I wanted to work towards this issue #28141 when I realized how deep I had to go to fix this, so I haven't actually touched SliceableFoods yet.

  • LogComponent and LogSystem in Content.Server replaced by more generic SliceableComponent and SliceableSystem in Content.Shared
  • SharpComponent replaced by ToolComponent and the "Slicing" ToolQualityPrototype and all instances of it replaced as such
  • SharpSystem in Content.Server replaced by ButcherableSystem in Content.Shared

LogComponent and LogSystem's entire purpose was just "if sharp object used, spawn these items". Sliceable does the same but in shared. I wanted to merge Sliceablefoods into it, but considering the current amount of changed files, I chose not to do that yet.

SharpComponent had a TODO saying to just make it a tool type. I deleted the SharpComponent and replaced every instance of it by adding the ToolComponent with the "Slicing" quality. When code checked for the presence of the SharpComponent, I checked for the ToolComponent and the "Slicing" ToolQualityPrototype instead.

SharpSystem, despite the name being quite generic, was used solely for the do-after and verbs related to butchering. I moved this all to the ButcherableSystem in Content.Shared instead, along with any changes required to make it not use the SharpComponent.

Why / Balance

LogComponent isn't generic enough, and SliceableFood should be merged into the new Sliceable stuff after this PR to address #28141
SharpComponent had a TODO comment saying it didn't need to be a component, could just be a tool quality. So I did that.

Technical details

  • SharpComponent deleted, replaced by ToolComponent with the "Slicing" ToolQualityPrototype. This is most of the yml changes.
  • Any checks for SharpComponent replaced by a check for ToolComponent and using the SharedToolSystem to check if the ToolComponent has the "Slicing" ToolQualityPrototype.
  • SharpSystem in Content.Server replaced by ButcherableSystem in Content.Shared. This makes the butcher verb predicted. Spawning and deletion of entities are only run on the server.
  • SharpSystem AfterInteractEvent subscription changed to ButcherableSystem InteractUsingEvent subscription. Previously, the knife checked if it interacted on a butcherable target. Now, the butcherable target checks if it was interacted with by a slicing tool.
  • LogComponent and LogSystem in Content.Server replaced by SliceableComponent and SliceableSystem in Content.Shared. Spawning and deletion of entities are only run on the server.
  • All do-afters args are gone and handled by UseTool() from the SharedToolSystem.
  • Integration test for Slicing system checks that using a cleaver on a sliceable object spawns the desired products and that you can't slice non-sliceable objects.
  • Integration test for Butcherable system checks that using a cleaver on a dead Ian spawns the desired products and that you can't butcher Ian while he's still alive.

Media

ButcherableSystem function + verb prediction and ToolQuality description:

2025-02-03.00-48-08.mp4

SliceableSystem function + Botany planter function:

2025-02-03.00-51-20.mp4

Requirements

Breaking changes

LogComponent and LogSystem have been removed and replaced by SliceableComponent and SliceableSystem.
SharpComponent has been removed and replaced by the usage of ToolComponent with a "Slicing" quality.
SharpSystem has been removed and replaced by the ButcherableSystem.

Changelog

🆑

  • tweak: There is now a do-after and sound effect when splitting log types and cotton boll types.
  • tweak: The butchering verb is now predicted, making it more responsive.
  • tweak: The cargo knife bounty now requires knives and shivs specifically, rather than anything sharp such as swords. That means plastic knives can now be used for the bounty.
  • tweak: The cargo knife bounty now asks for 18 knives instead of 5 knives.

Plykiya added 2 commits February 3, 2025 00:10
…d a SharpComponent.cs changed to use ToolQuality Slicing...
…tems

# Conflicts:
#	Content.Server/Botany/Systems/LogSystem.cs
@Plykiya Plykiya requested a review from Partmedia as a code owner February 3, 2025 09:17
@github-actions github-actions bot added S: Needs Review Status: Requires additional reviews before being fully accepted size/M Denotes a PR that changes 100-999 lines. S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. labels Feb 3, 2025
@Plykiya
Copy link
Author

Plykiya commented Feb 3, 2025

I'll deal with the Sliceable arbitrage test fail and write tests for the Sliceable and Butcherable systems

@TheShuEd TheShuEd added P2: Raised Priority: Item has a raised priority, indicating it might get increased maintainer attention. T: Refactor Type: Refactor of notable amount of codebase T: Cleanup Type: Code clean-up, without being a full refactor or feature D2: Medium Difficulty: A good amount of codebase knowledge required. A: Core Tech Area: Underlying core tech for the game and the Github repository. A: General Interactions Area: General in-game interactions that don't relate to another area. S: Conceptual Approval Status: Discussed by maintainers and has conceptual approval, but needs code review and removed S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. labels Feb 3, 2025
@Plykiya
Copy link
Author

Plykiya commented Feb 3, 2025

so the new test fail is complaining that the large pizza box order comes with 15 pizzas, each with their own plastic knife, which can count towards the new knife bounty since it's only checking for a knife tag

what do I do about that

make the knife bounty way cheaper?

@Plykiya
Copy link
Author

Plykiya commented Feb 3, 2025

I just increased the amount of knives until the test stopped complaining. Good to review at this point.

Copy link
Contributor

@MilonPL MilonPL left a comment

Choose a reason for hiding this comment

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

Mostly good, minor style nitpicks. Keep in mind that EntitySystems in Shared also need their Client/Server parts, even if they don't have a body yet. So you should rename the ButherableSystem to SharedButcherableSystem, and then make a dummy EntitySystem in both Client and Server that will extend it. Same for SliceableSystem.

Content.Server/Botany/Systems/BotanySystem.Seed.cs Outdated Show resolved Hide resolved
Content.Server/Kitchen/EntitySystems/KitchenSpikeSystem.cs Outdated Show resolved Hide resolved

public sealed class ButcherableSystem : EntitySystem
{
[Dependency] private readonly INetManager _net = default!;
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove the System part from all these, it shouldn't be used for dependencies. Sort it alphabetically.

Copy link
Author

@Plykiya Plykiya Feb 5, 2025

Choose a reason for hiding this comment

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

Can this style convention be added to the conventions doc page?

https://docs.spacestation14.com/en/general-development/codebase-info/conventions.html

Copy link
Contributor

Choose a reason for hiding this comment

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

At some point in the near future yes, many parts of the the conventions are pending rewrite.

Content.Shared/Nutrition/ButcherDoafterEvent.cs Outdated Show resolved Hide resolved
Content.Shared/Tools/Components/SliceableComponent.cs Outdated Show resolved Hide resolved
Content.Shared/Tools/Systems/SliceableSystem.cs Outdated Show resolved Hide resolved
Content.Shared/Tools/SliceableEvents.cs Outdated Show resolved Hide resolved
@Plykiya
Copy link
Author

Plykiya commented Feb 3, 2025

Mostly good, minor style nitpicks. Keep in mind that EntitySystems in Shared also need their Client/Server parts, even if they don't have a body yet. So you should rename the ButherableSystem to SharedButcherableSystem, and then make a dummy EntitySystem in both Client and Server that will extend it. Same for SliceableSystem.

They only need the Client/Server parts if the Shared system is abstract, requiring the dummy implementation on both Client and System.

Similarly, they don't need the Shared prefix since they only exist in Shared.

image

Good catches on some of the other reviews, thanks for them.

@MilonPL
Copy link
Contributor

MilonPL commented Feb 5, 2025

Yup, I was told that all EntitySystems in Shared should be abstract a long time ago and didn't really question it, but after checking with other maintainers, you're right on those.

@MilonPL MilonPL self-assigned this Feb 5, 2025
@MilonPL MilonPL added S: Awaiting Changes Status: Changes are required before another review can happen and removed S: Needs Review Status: Requires additional reviews before being fully accepted labels Feb 5, 2025
@Plykiya Plykiya requested a review from MilonPL February 5, 2025 18:50
@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 Feb 5, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: Core Tech Area: Underlying core tech for the game and the Github repository. A: General Interactions Area: General in-game interactions that don't relate to another area. D2: Medium Difficulty: A good amount of codebase knowledge required. P2: Raised Priority: Item has a raised priority, indicating it might get increased maintainer attention. S: Conceptual Approval Status: Discussed by maintainers and has conceptual approval, but needs code review S: Needs Review Status: Requires additional reviews before being fully accepted size/M Denotes a PR that changes 100-999 lines. T: Cleanup Type: Code clean-up, without being a full refactor or feature T: Refactor Type: Refactor of notable amount of codebase
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants