Revert "Upstreams" #570
Revert "Upstreams" #570
Conversation
WalkthroughThe changes in this pull request involve significant modifications across several classes related to body systems and surgery mechanics. Key alterations include the removal of methods and dependencies concerning body part markings, the introduction of new event handling in the surgery system, and updates to the UI interactions for surgery. Additionally, several properties and methods in various components have been simplified or removed, reflecting a shift towards a more streamlined approach to managing body appearances and surgery processes. Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (11)
Content.Client/Backmen/Surgery/SurgerySystem.cs (2)
7-8: Add XML documentation for the public event.Since
OnStepis part of the public API, it should be documented with XML comments explaining its purpose and when it's invoked.+/// <summary> +/// Event that is triggered when a surgery step is completed during UI refresh. +/// </summary> public event Action? OnStep;
16-19: Consider passing relevant surgery step information to event subscribers.The current implementation invokes
OnStepwithout any context about which step was completed. This might limit the usefulness of the event for subscribers who need to know specific details about the surgery progress.Consider either:
- Changing the event signature to include step information:
-public event Action? OnStep; +public event Action<SurgeryStepEventArgs>? OnStep;
- Or passing the event data from
SurgeryUiRefreshEvent:private void OnRefresh(SurgeryUiRefreshEvent ev) { - OnStep?.Invoke(); + OnStep?.Invoke(ev.StepInfo); }Resources/Prototypes/_Backmen/Entities/Objects/Misc/handcuffs.yml (1)
19-19: Add newline at end of file.The file is missing a newline character at the end, which is a YAML best practice.
breakoutTime: 5 +🧰 Tools
🪛 yamllint
[error] 19-19: no new line character at the end of file
(new-line-at-end-of-file)
Content.Client/Backmen/Body/BodySystem.cs (1)
15-16: TODO comment needs more context.The TODO comment is too vague. It should specify:
- What exactly needs to be adjusted
- What issues exist with player sprite updates
- Any dependencies or blockers preventing implementation
Consider updating the comment to be more descriptive:
- //TODO a few things need to be adjusted before this is ready to be used - also need to find a way to update the player sprite + // TODO: Implementation blocked by: + // 1. [Specific adjustment needed] + // 2. Need to implement player sprite update mechanism through [expected approach] + // Related issue: [issue number if exists]Content.Shared/Body/Part/BodyPartComponent.cs (1)
1-1: Remove the BOM character from the import statementThe line contains a Byte Order Mark (BOM) character which could cause issues in some environments. Consider removing it to maintain consistency.
-using Content.Shared.Backmen.Surgery.Tools; +using Content.Shared.Backmen.Surgery.Tools;Content.Client/Backmen/Surgery/SurgeryBui.cs (3)
46-58: Consider extracting the throttle duration as a constant.The throttling logic is well-implemented, but the magic number 0.2 should be extracted as a named constant for better maintainability.
+ private const float ThrottleDurationSeconds = 0.2f; private void OnPlayerItemAdded(string handName, EntityUid item) { if (_throttling.handName.Equals(handName) && _throttling.item.Equals(item) - && DateTime.UtcNow - _lastRefresh < TimeSpan.FromSeconds(0.2) + && DateTime.UtcNow - _lastRefresh < TimeSpan.FromSeconds(ThrottleDurationSeconds) || !_timing.IsFirstTimePredicted || _window == null || !_window.IsOpen) return;
319-319: Consider using trace level for detailed logging.Debug level logging might be too verbose for production. Consider using trace level for such detailed operational logs.
- Logger.Debug($"Running RefreshUI on {Owner}"); + Logger.Trace($"Running RefreshUI on {Owner}");
360-379: Consider extracting error handling logic to a separate method.The error handling switch statement could be extracted to a separate method for better readability and maintainability.
+ private void AppendStepErrorMessage(FormattedMessage stepName, StepInvalidReason reason) + { + switch (reason) + { + case StepInvalidReason.MissingSkills: + stepName.AddMarkup($" [color=red]{Loc.GetString("surgery-ui-window-steps-error-skills")}[/color]"); + break; + case StepInvalidReason.NeedsOperatingTable: + stepName.AddMarkup($" [color=red]{Loc.GetString("surgery-ui-window-steps-error-table")}[/color]"); + break; + case StepInvalidReason.Armor: + stepName.AddMarkup($" [color=red]{Loc.GetString("surgery-ui-window-steps-error-armor")}[/color]"); + break; + case StepInvalidReason.MissingTool: + stepName.AddMarkup($" [color=red]{Loc.GetString("surgery-ui-window-steps-error-tools")}[/color]"); + break; + } + } // In RefreshUI method: - switch (reason) - { - case StepInvalidReason.MissingSkills: - stepName.AddMarkup($" [color=red]{Loc.GetString("surgery-ui-window-steps-error-skills")}[/color]"); - break; - case StepInvalidReason.NeedsOperatingTable: - stepName.AddMarkup($" [color=red]{Loc.GetString("surgery-ui-window-steps-error-table")}[/color]"); - break; - case StepInvalidReason.Armor: - stepName.AddMarkup($" [color=red]{Loc.GetString("surgery-ui-window-steps-error-armor")}[/color]"); - break; - case StepInvalidReason.MissingTool: - stepName.AddMarkup($" [color=red]{Loc.GetString("surgery-ui-window-steps-error-tools")}[/color]"); - break; - } + AppendStepErrorMessage(stepName, reason);Content.Shared/Backmen/Surgery/Body/SharedBodySystem.PartAppearance.cs (2)
60-63: Simplify symmetry handling by avoiding unnecessary casts and string comparisonsCasting
part.Symmetryand comparing enum values as strings can be fragile and less efficient. Instead, directly compare the enum values.Apply this diff to improve the code:
- var symmetry = ((BodyPartSymmetry) part.Symmetry).ToString(); - if (symmetry == "None") - symmetry = ""; + var symmetry = part.Symmetry != BodyPartSymmetry.None ? part.Symmetry.ToString() : "";
39-40: Optimize null checks forOriginalBodyYou're checking
part.OriginalBody != nullbefore accessingpart.OriginalBody.Value. To simplify and make the code cleaner, consider using pattern matching with null checks.Apply this diff to streamline the null checks:
- if (TryComp(uid, out BodyPartComponent? part) && part.OriginalBody != null && - TryComp(part.OriginalBody.Value, out HumanoidAppearanceComponent? bodyAppearance)) + if (TryComp(uid, out BodyPartComponent? part) && + part.OriginalBody is EntityUid originalBody && + TryComp(originalBody, out HumanoidAppearanceComponent? bodyAppearance))Content.Shared/Body/Systems/SharedBodySystem.Parts.cs (1)
631-638: Consider appearance handling for body parts from different sourcesWhen attaching a body part, you add a
BodyPartAppearanceComponentand set its color based on the body'sHumanoidAppearanceComponent. This approach works well when the body part originates from the same body. However, if the body part comes from a different entity with different appearance attributes (e.g., skin color or species), this may lead to inconsistent visuals.Consider updating the logic to preserve the original appearance of the attached body part if it already has a
BodyPartAppearanceComponent, or to handle different species appropriately.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (27)
Content.Client/Backmen/Body/BodySystem.cs(1 hunks)Content.Client/Backmen/Surgery/SurgeryBui.cs(5 hunks)Content.Client/Backmen/Surgery/SurgerySystem.cs(1 hunks)Content.Client/Humanoid/HumanoidAppearanceSystem.cs(3 hunks)Content.Server/Backmen/Surgery/SurgerySystem.cs(2 hunks)Content.Server/Body/Systems/BodySystem.cs(3 hunks)Content.Server/Destructible/Thresholds/Behaviors/GibBehavior.cs(1 hunks)Content.Shared/Backmen/Surgery/Body/BodyPartAppearanceComponent.cs(1 hunks)Content.Shared/Backmen/Surgery/Body/SharedBodySystem.PartAppearance.cs(1 hunks)Content.Shared/Backmen/Surgery/SharedSurgerySystem.Steps.cs(0 hunks)Content.Shared/Backmen/Surgery/SurgeryUI.cs(0 hunks)Content.Shared/Body/Part/BodyPartComponent.cs(1 hunks)Content.Shared/Body/Part/BodyPartEvents.cs(0 hunks)Content.Shared/Body/Systems/SharedBodySystem.Body.cs(4 hunks)Content.Shared/Body/Systems/SharedBodySystem.Parts.cs(2 hunks)Content.Shared/Body/Systems/SharedBodySystem.cs(0 hunks)Content.Shared/Humanoid/Events/ProfileLoadFinishedEvent.cs(0 hunks)Content.Shared/Humanoid/SharedHumanoidAppearanceSystem.cs(1 hunks)Resources/Changelog/ChangelogBkm.yml(0 hunks)Resources/Locale/en-US/surgery/surgery-ui.ftl(1 hunks)Resources/Prototypes/Body/Parts/skeleton.yml(0 hunks)Resources/Prototypes/Entities/Mobs/NPCs/animals.yml(1 hunks)Resources/Prototypes/Entities/Mobs/Player/silicon_base.yml(0 hunks)Resources/Prototypes/Entities/Mobs/Species/skeleton.yml(1 hunks)Resources/Prototypes/_Backmen/Body/Parts/shadowkin.yml(0 hunks)Resources/Prototypes/_Backmen/Entities/Objects/Misc/handcuffs.yml(1 hunks)Resources/Prototypes/_Backmen/Entities/Surgery/surgeries.yml(2 hunks)
💤 Files with no reviewable changes (9)
- Content.Shared/Backmen/Surgery/SharedSurgerySystem.Steps.cs
- Content.Shared/Backmen/Surgery/SurgeryUI.cs
- Content.Shared/Body/Part/BodyPartEvents.cs
- Content.Shared/Body/Systems/SharedBodySystem.cs
- Content.Shared/Humanoid/Events/ProfileLoadFinishedEvent.cs
- Resources/Changelog/ChangelogBkm.yml
- Resources/Prototypes/Body/Parts/skeleton.yml
- Resources/Prototypes/Entities/Mobs/Player/silicon_base.yml
- Resources/Prototypes/_Backmen/Body/Parts/shadowkin.yml
🧰 Additional context used
🪛 yamllint
Resources/Prototypes/_Backmen/Entities/Objects/Misc/handcuffs.yml
[error] 19-19: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (32)
Content.Client/Backmen/Surgery/SurgerySystem.cs (1)
Line range hint 1-19: Verify the implications of this revert.
Since this is a revert of PR #567, we should ensure that:
- Any dependent systems are aware of the removal of this event handling mechanism
- The revert doesn't break existing surgery UI functionality
Let's check for any dependencies on this event:
✅ Verification successful
Let me check the actual usage of OnStep in the SurgeryBui.cs file to understand how it's being used:
Let me check one more thing to understand the server-side implementation and the shared interface:
Based on the gathered information, I can now provide the final response:
The revert appears safe to proceed with
The event handling mechanism is properly contained within the surgery system:
- The
OnStepevent is only subscribed to inSurgeryBui.csfor UI refresh purposes - The server properly raises
SurgeryUiRefreshEventwhich is handled by both client and server systems - The revert maintains the core surgery functionality through the shared interface (
SharedSurgerySystem)
The event is properly cleaned up in the BUI's Dispose method, and there are no other dependencies on this event outside the surgery system's scope.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any references to OnStep in the codebase
rg -l "OnStep" --type cs
# Search for any classes that might be affected by this revert
rg -l "SurgeryBui" --type cs
Length of output: 709
Script:
#!/bin/bash
# Check how OnStep is used in SurgeryBui.cs
rg "OnStep" Content.Client/Backmen/Surgery/SurgeryBui.cs -A 5 -B 5
# Also check the SurgeryUI.cs to understand the shared interface
rg "class SurgeryUI" Content.Shared/Backmen/Surgery/SurgeryUI.cs -A 10
Length of output: 993
Script:
#!/bin/bash
# Check server implementation
rg "class SurgerySystem" Content.Server/Backmen/Surgery/SurgerySystem.cs -A 10
# Check for any UI state updates in server implementation
rg "SurgeryUiRefreshEvent" Content.Server/Backmen/Surgery/SurgerySystem.cs -A 3 -B 3
# Check the shared interface
rg "class SharedSurgerySystem" -A 10
Length of output: 3719
Resources/Prototypes/_Backmen/Entities/Objects/Misc/handcuffs.yml (1)
19-19: Verify the breakout time value.
Since this is a revert PR, please confirm that 5 seconds was indeed the original breakout time value in the codebase before PR #567.
✅ Verification successful
Let me try a different approach to verify the history by looking at the specific PR #567 that this PR is reverting.
Let me try one final approach to check the changes in PR #567 with a corrected query:
The breakout time value of 5 seconds is correct and matches the original implementation.
From the git history, we can see that:
- The
breakoutTime: 5property was initially added in commit 4ac368f with PR [FEATURE] Purple maid/sec cuffs Rxup/space-station-14#889 that introduced the purple handcuffs - It was temporarily removed in commit e720e4d with PR [FIX] Cuff popup Rxup/space-station-14#908
- This PR correctly restores it back to its original value of 5 seconds
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the history of breakoutTime value for Purplecuffs
# Expected: Find the original value before PR #567
# Search for previous versions of this file
git log -p -- "Resources/Prototypes/_Backmen/Entities/Objects/Misc/handcuffs.yml" | grep -A 1 "breakoutTime:"
Length of output: 156
Script:
#!/bin/bash
# Get the commit details for PR #567 to check the changes made to handcuffs.yml
gh pr view 567 --json commits | jq -r '.commits[].oid' | while read -r commit; do
git show "$commit" -- "Resources/Prototypes/_Backmen/Entities/Objects/Misc/handcuffs.yml" | grep -A 1 "breakoutTime:"
done
Length of output: 10569
Script:
#!/bin/bash
# Also check if there are any other handcuff-related files that might have this property
fd handcuff -t f | xargs grep -l "breakoutTime"
Length of output: 170
Script:
#!/bin/bash
# Get the base and head commits of PR #567 to see the exact changes
gh pr view 567 --json baseRefOid,headRefOid | jq -r '"\(.baseRefOid) \(.headRefOid)"' | while read -r base head; do
echo "Changes in PR #567:"
git diff "$base" "$head" -- "Resources/Prototypes/_Backmen/Entities/Objects/Misc/handcuffs.yml"
done
Length of output: 740
Script:
#!/bin/bash
# Get PR #567 details with correct fields
gh pr view 567 --json headRefName,baseRefName | jq -r '"\(.baseRefName) \(.headRefName)"' | while read -r base head; do
echo "Checking changes in handcuffs.yml between $base and $head branches"
git diff "origin/$base...origin/$head" -- "Resources/Prototypes/_Backmen/Entities/Objects/Misc/handcuffs.yml"
done
Length of output: 468
Script:
#!/bin/bash
# Check the commit that added the breakoutTime property initially
git log -p --all --full-history -S "breakoutTime:" -- "Resources/Prototypes/_Backmen/Entities/Objects/Misc/handcuffs.yml"
Length of output: 2419
🧰 Tools
🪛 yamllint
[error] 19-19: no new line character at the end of file
(new-line-at-end-of-file)
Resources/Locale/en-US/surgery/surgery-ui.ftl (1)
9-9: Consider retaining this improved error message.
While this PR is marked as a revert of PR #567, this particular change improves user experience by providing clearer context about missing surgical tools. The new message "You're missing tools for this surgery" is more descriptive and consistent with other error messages in the file. Consider keeping this improvement even if reverting other changes.
Let's verify the original message from PR #567:
Content.Server/Destructible/Thresholds/Behaviors/GibBehavior.cs (1)
16-16: LGTM! Simplified GibBody call aligns with revert objectives.
The removal of gib and contents parameters from the GibBody call is consistent with the revert operation and appears to restore the previous behavior.
Let's verify the method signature change in BodySystem:
✅ Verification successful
The GibBody call signature change is correct and consistent with the codebase
The search results confirm that GibBody is defined in SharedBodySystem.Body.cs with the signature:
public virtual HashSet<EntityUid> GibBody(
EntityUid bodyId,
bool gibOrgans = false,
BodyComponent? body = null,This matches the usage in GibBehavior.cs and is consistently used across the codebase, validating that the simplified call is correct.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the GibBody method signature in BodySystem
# Expected: Should find the reverted method signature without gib and contents parameters
ast-grep --pattern 'GibBody(EntityUid owner, bool recursive, BodyComponent? body)'
Length of output: 84
Script:
#!/bin/bash
# Let's try a broader search for GibBody method using ripgrep
rg "GibBody" -A 3 --type cs
Length of output: 8682
Content.Client/Backmen/Body/BodySystem.cs (1)
9-18:
Implementation appears incomplete and needs clarification.
The UpdateAppearance method contains commented-out functionality and a TODO comment that lacks specific details. Since this is a revert PR, we should ensure this transitional state is intentional and won't cause issues.
Let's verify the impact of these changes:
Consider either:
- Implementing the TODO items before merging
- Adding more detailed documentation about what adjustments are needed
- Reverting this method to its previous implementation if the changes weren't meant to be included in this revert
✅ Verification successful
Let me gather more context about the specific changes in this revert PR by examining the related body system files.
Based on the gathered information, I can now provide a final response:
Commented-out code is intentionally transitional and aligns with component design
The current implementation state is actually correct for this revert PR because:
- The
BodyPartAppearanceComponentitself has a TODO comment indicating future plans for species and marking support - The server-side implementation is intentionally empty, suggesting a client-only feature
- The component is properly structured with networking and state management (
[NetworkedComponent, AutoGenerateComponentState])
The commented-out sprite color code and TODO in UpdateAppearance align with the component's design phase, where additional features like species-specific appearances are planned but not yet implemented.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any remaining references to body part markings that might be affected
rg -l "BodyPartAppearanceComponent|MarkingPrototype" --type cs
# Check for any other TODO comments related to appearance updates
rg "TODO.*sprite|TODO.*appearance" --type cs
Length of output: 2496
Script:
#!/bin/bash
# Check the content of BodyPartAppearanceComponent to understand the intended functionality
cat Content.Shared/Backmen/Surgery/Body/BodyPartAppearanceComponent.cs
# Look for related appearance update implementations in other body systems
rg -A 5 "UpdateAppearance.*BodyPart" --type cs
# Check git history for the specific changes being reverted
git show 567 -- Content.Client/Backmen/Body/BodySystem.cs || true
Length of output: 2494
Content.Shared/Backmen/Surgery/Body/BodyPartAppearanceComponent.cs (4)
7-7: LGTM! Component attributes are correctly configured.
The component is properly set up with necessary attributes for networking and state generation.
14-14: LGTM! ID property is well-defined with appropriate type safety.
The property uses correct serialization for prototype IDs and includes clear documentation.
20-20: LGTM! Color property is properly configured with clear documentation.
The nullable Color property correctly handles the skin color fallback case.
23-23: Verify if the TODO comment is still relevant after the revert.
Since this is a revert PR, let's verify if the TODO comment about adding species and markings is still applicable to the current architecture.
Let's check for any related implementations or discussions:
Also applies to: 26-26
✅ Verification successful
Based on the search results, I can generate the final response as we have sufficient evidence about species and marking implementations in the codebase:
TODO comment about species and markings remains relevant
The TODO comment is still applicable as the codebase shows active implementation of species-specific body parts and marking systems:
- Species-specific body parts exist (e.g. Reptilian parts in multiple texture directories)
- Active marking system that interacts with body parts (
MarkingsSet.cs,HumanoidAppearanceSystem.cs) - Current implementation includes species-body part compatibility checks (
MustMatchSkinmethod)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for related species/marking implementations and discussions
# Check for species-related code in body parts
rg -g "!*.{md,txt}" -i "species.*body.*part|body.*part.*species"
# Check for marking-related code in body parts
rg -g "!*.{md,txt}" -i "marking.*body.*part|body.*part.*marking"
# Check for any related TODO comments
rg -g "!*.{md,txt}" "TODO.*species.*marking|TODO.*marking.*species"
Length of output: 2292
Resources/Prototypes/Entities/Mobs/Species/skeleton.yml (1)
44-44: Verify the GibBehavior reversion's impact on the skeleton entity.
The change simplifies the GibBehavior to an empty object syntax { }, which according to the AI summary, aligns with the broader simplification of the BodySystem class. However, since this is a revert PR, we should verify that this change properly restores the previous behavior.
Let's verify the GibBehavior configuration across different entity definitions:
✅ Verification successful
GibBehavior configuration is consistent across entities
The verification shows that:
- All entities using GibBehavior consistently use the empty object syntax
{ } - The skeleton entity maintains its unique
gibSoundproperty separately from GibBehavior - This pattern is consistent across various entity types (base mobs, animals, xenomorphs)
The empty GibBehavior configuration appears to be the standard implementation across the codebase, confirming that the revert properly aligns with the existing entity definitions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check GibBehavior configurations across entity definitions
# to ensure consistent behavior after the revert
# Search for GibBehavior configurations
rg -U "GibBehavior.*?\{.*?\}" --type yaml
# Also check for any related gibbing configurations that might be affected
rg "gibContents|gibType|gibSound" --type yaml
Length of output: 888
Content.Server/Body/Systems/BodySystem.cs (2)
111-111: LGTM: GibBody parameter changes are consistent
The addition of gibSoundOverride parameter and corresponding base method call changes maintain consistency with the GibPart method signature while preserving the core gibbing functionality.
Also applies to: 125-125
167-170: Verify the empty UpdateAppearance implementation
The method overrides the base implementation but does nothing, which could affect body part appearance updates. Since this is a revert PR, please confirm if this is the intended behavior.
Let's check how this method is used in the codebase:
✅ Verification successful
Let me gather more context about the client-side implementation and the base class to better understand the impact.
Empty UpdateAppearance implementation is correct for server-side
The empty implementation in the server-side BodySystem.cs is the intended behavior because:
- The appearance update logic is properly handled on the client-side in
Content.Client/Backmen/Body/BodySystem.cs, where it manages sprite color updates - The shared base class
SharedBodySystemdefines the abstract method that both client and server must implement, but only the client needs to handle visual updates - The server-side implementation intentionally does nothing as appearance/visual changes are client-side concerns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for UpdateAppearance method calls and related event handlers
rg -A 3 "UpdateAppearance.*BodyPartAppearanceComponent"
# Search for any references to BodyPartAppearanceComponent to understand the impact
ast-grep --pattern 'class $_ {
$$$
BodyPartAppearanceComponent
$$$
}'
Length of output: 1018
Script:
#!/bin/bash
# Check the client-side implementation
rg -A 10 "protected override void UpdateAppearance.*BodyPartAppearanceComponent" Content.Client/Backmen/Body/BodySystem.cs
# Check the abstract base implementation
cat Content.Shared/Backmen/Surgery/Body/SharedBodySystem.PartAppearance.cs
Length of output: 4500
Content.Shared/Body/Part/BodyPartComponent.cs (2)
Line range hint 13-15: Implementation follows component best practices
The component is well-structured with proper access control, networking setup, and clear separation of concerns.
Line range hint 13-15: Verify the impact of removed Species and Sex properties
The removal of these serialized properties might affect existing save data and dependent systems. Let's verify there are no lingering references.
✅ Verification successful
Based on the results from our verification attempts, I can now generate the final response:
Removal of Species and Sex properties appears safe
The verification shows:
- No direct references to these properties in BodyPartComponent
- No serialization or migration code depending on these fields
- The only references to Sex are in HumanoidCharacterProfile and HumanoidAppearanceComponent, which are separate systems
- No evidence of these properties being used in save data or requiring migration
The properties appear to have been moved to more appropriate components (HumanoidAppearanceComponent) as part of system refactoring, with no lingering dependencies.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for references to the removed properties in the codebase
echo "Checking for Species property references..."
rg -l "Species.*=.*\"\"" --type cs
echo "Checking for Sex property references..."
rg -l "Sex.*=.*Sex\.Male" --type cs
echo "Checking for serialized data usage..."
rg -l "BodyPartComponent.*Species|BodyPartComponent.*Sex" --type cs
Length of output: 578
Script:
#!/bin/bash
# Let's check for more specific patterns and context
# Check for direct references to these properties in BodyPartComponent
echo "Checking BodyPartComponent references..."
rg "Species|Sex" Content.Shared/Body/Part/BodyPartComponent.cs -C 3
# Check for any serialization or data migration code
echo -e "\nChecking serialization and migration code..."
rg "BodyPartComponent.*\b(Serialize|Deserialize|Load|Save|Migrate)" --type cs -C 2
# Check for system interactions with BodyPartComponent
echo -e "\nChecking system interactions..."
rg "IEntityManager.*GetComponent.*BodyPartComponent|EntityQuery.*BodyPartComponent" --type cs -C 2
# Check for any recent changes or migrations
echo -e "\nChecking git history for related changes..."
git log -p --grep="Species\|Sex" -- Content.Shared/Body/Part/BodyPartComponent.cs
Length of output: 1206
Script:
#!/bin/bash
# Let's check for any data migration systems and recent changes in the body system
# Check for any migration systems
echo "Checking for migration systems..."
rg "Migration.*Body|Body.*Migration" --type cs -C 3
# Look for recent changes in SharedBodySystem
echo -e "\nChecking SharedBodySystem changes..."
rg "Species|Sex" Content.Shared/Backmen/Surgery/Body/SharedBodySystem.cs -C 3
# Check for any prototype changes related to body parts
echo -e "\nChecking body part prototypes..."
fd ".*[Bb]ody[Pp]art.*\.yml" --exec cat {}
# Check for any tests that might be affected
echo -e "\nChecking related tests..."
rg "BodyPartComponent.*Test" --type cs -C 2
Length of output: 636
Resources/Prototypes/_Backmen/Entities/Surgery/surgeries.yml (1)
72-72: LGTM: Requirement addition improves surgical safety.
The addition of requirement: SurgeryOpenIncision for head attachment surgery ensures proper surgical procedure and maintains consistency with other similar surgeries in the file.
Content.Client/Humanoid/HumanoidAppearanceSystem.cs (3)
76-76: LGTM: Method signature simplification.
The removal of the overrideSkin parameter simplifies the interface while maintaining the core functionality.
94-95: Verify skin color application behavior.
The simplified condition for skin color application (proto.MatchSkin) might affect how layers inherit skin color. Please ensure this change:
- Maintains correct skin color inheritance for layers that should match skin
- Preserves custom colors for layers that shouldn't match skin
#!/bin/bash
# Description: Check for potential skin color application issues
# Look for any tests or usages that verify skin color behavior
rg "MatchSkin.*SkinColor" --type cs -A 558-58: Verify the impact on custom base layers' skin color handling.
The removal of the overrideSkin parameter might affect how skin color is applied to custom base layers. Please ensure this change doesn't unintentionally force skin color matching on layers that should maintain their custom colors.
✅ Verification successful
Based on the gathered evidence, I can now provide a final response:
The removal of overrideSkin parameter is safe and maintains correct color handling
The code review shows that custom base layers maintain their own color handling through the CustomBaseLayerInfo structure, which explicitly stores color information. When setting layer data for custom base layers, the color is passed directly from info.Color and sexMorph is set to false, ensuring custom colors are preserved. The removal of overrideSkin parameter simplifies the API without affecting the color handling behavior for custom base layers.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential skin color-related regressions in custom base layers
# Look for any references to custom base layers and skin color handling
rg -A 5 "CustomBaseLayers.*SkinColor" --type cs
Length of output: 42
Script:
#!/bin/bash
# Let's try a broader search to understand the skin color handling logic
rg "CustomBaseLayer" --type cs -A 5
# Also search for SetLayerData to understand the method's implementation
ast-grep --pattern 'SetLayerData($$$) {
$$$
}'
Length of output: 18995
Script:
#!/bin/bash
# Let's look at the SetLayerData implementation and its usage
rg "SetLayerData" --type cs -A 10
Length of output: 2507
Content.Client/Backmen/Surgery/SurgeryBui.cs (1)
27-27: LGTM: Clean dependency injection and event subscription setup.
The HandsSystem dependency is properly injected and event subscriptions are correctly set up in the constructor.
Also applies to: 40-43
Content.Shared/Body/Systems/SharedBodySystem.Body.cs (4)
132-132: LGTM: Removed unused parameter.
The removal of the rootPart parameter from MapInitParts call is a clean-up change as the parameter wasn't being used in the method implementation.
186-187: LGTM: Method signature cleanup.
The removal of the unused rootPart parameter from the method signature is consistent with its implementation and improves code clarity.
364-365: LGTM: Simplified method signature by hardcoding constant values.
The removal of gib and contents parameters from GibBody method signature and hardcoding their values (GibType.Gib and GibContentsOption.Drop) is appropriate since these values were constant and didn't need to be configurable.
Also applies to: 381-383
Line range hint 233-252: Verify appearance system integration.
The body part appearance initialization looks correct, but let's verify the integration with the appearance system to ensure proper handling of body part visuals.
✅ Verification successful
Let me gather more specific information about the appearance system integration.
Based on the gathered information, I can now provide the final response:
Body part appearance system integration is properly implemented
The appearance system integration is correctly structured with:
- Proper event subscriptions for component initialization and body part addition
- Complete mapping of body parts to visual layers
- Proper synchronization with the humanoid appearance system
- Correct handling of custom base layers and color information
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the appearance system integration
# Check for any appearance-related components and systems that might be affected
# Look for appearance component references
echo "Checking appearance component references..."
rg -l "BodyPartAppearanceComponent|HumanoidAppearanceComponent" --type cs
# Look for appearance-related systems
echo "Checking appearance system implementations..."
ast-grep --pattern 'class $_ : IAppearanceSystem
{
$$$
}'
Length of output: 4676
Script:
#!/bin/bash
# Check for appearance system implementations and related methods
# Look for appearance-related methods in SharedBodySystem.PartAppearance.cs
echo "Checking PartAppearance system implementations..."
rg -A 10 "BodyPartAppearanceComponent" "Content.Shared/Backmen/Surgery/Body/SharedBodySystem.PartAppearance.cs"
# Check for appearance updates and event handlers
echo "Checking appearance update handlers..."
rg -A 5 "UpdateAppearance|OnAppearanceChange" "Content.Shared/Body/Systems/SharedBodySystem.Body.cs" "Content.Shared/Backmen/Surgery/Body/SharedBodySystem.PartAppearance.cs"
# Check for visual state handling
echo "Checking visual state handling..."
rg -A 5 "AppearanceComponent|VisualState" "Content.Shared/Backmen/Surgery/Body/BodyPartAppearanceComponent.cs"
Length of output: 3838
Content.Shared/Humanoid/SharedHumanoidAppearanceSystem.cs (3)
450-450: Verify the impact of LoadProfile changes on voice settings.
Since this is a revert PR, removing the Dirty call and SetTTSVoice functionality might affect voice synchronization. Please ensure that voice settings are properly handled through alternative means if these changes are reverted.
#!/bin/bash
# Description: Check for other voice-related calls that might handle synchronization
# Test: Search for voice-related method calls
rg -l "SetTTSVoice|VoicePrototypeId"
# Test: Search for other methods that might call Dirty after LoadProfile
ast-grep --pattern 'LoadProfile($$$)'Line range hint 509-534: Verify dependencies on the multi-color marking functionality.
The new AddMarking overload with IReadOnlyList<Color> provides better flexibility. However, since this is a revert PR, we should ensure that removing this overload won't break existing functionality.
If keeping this overload, consider adding validation for the colors list length against the marking's sprite count to prevent index out of range errors:
public void AddMarking(EntityUid uid,
string marking,
IReadOnlyList<Color> colors,
bool sync = true,
bool forced = false,
HumanoidAppearanceComponent? humanoid = null)
{
if (!Resolve(uid, ref humanoid)
|| !_markingManager.Markings.TryGetValue(marking, out var prototype))
{
return;
}
+ if (colors.Count > prototype.Sprites.Count)
+ {
+ Log.Warning($"More colors provided than sprites for marking {marking}");
+ }
+
var markingObject = new Marking(marking, colors);
markingObject.Forced = forced;
humanoid.MarkingSet.AddBack(prototype.MarkingCategory, markingObject);
if (sync)
Dirty(uid, humanoid);
}Line range hint 536-545: Verify TTS voice handling after revert.
The SetTTSVoice method provides centralized voice management. If this PR reverts these changes, ensure that TTS voice settings are properly handled elsewhere to maintain functionality.
Consider adding validation for the voice ID to ensure it exists in the voice prototypes:
public void SetTTSVoice(EntityUid uid, string voiceId, HumanoidAppearanceComponent humanoid)
{
if (!TryComp<TTSComponent>(uid, out var comp))
return;
+ if (!_proto.HasIndex<TTSVoicePrototype>(voiceId))
+ {
+ Log.Warning($"Invalid TTS voice ID: {voiceId}");
+ return;
+ }
+
humanoid.Voice = voiceId;
comp.VoicePrototypeId = voiceId;
}Resources/Prototypes/Entities/Mobs/NPCs/animals.yml (1)
403-403: LGTM: GibBehavior implementation looks correct.
The addition of GibBehavior to the cockroach's Destructible component is appropriate and follows the standard pattern for entities that should explode into gibs when taking sufficient blunt damage.
Let's verify this behavior is consistent with other similar small mob entities:
✅ Verification successful
Based on the search results, I can see that GibBehavior is consistently used in similar small mob entities like the mothroach, which also has a GibBehavior triggered by blunt damage. Let me verify the damage thresholds to ensure the implementation is consistent.
Based on the search results, I can now generate the final response as I have verified the GibBehavior implementation across different entities:
GibBehavior implementation is consistent with similar entities
The addition of GibBehavior to the cockroach's Destructible component with a 10 blunt damage threshold is appropriate and consistent with other small mob entities. For comparison:
- Cockroach: Gibs at 10 blunt damage
- Mothroach: Gibs at 60 blunt damage
- Base mobs: Gib at 400 blunt damage
The lower threshold for the cockroach is justified by its smaller size and weaker nature compared to other entities.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for similar GibBehavior implementations in other mob entities
# Look for Destructible components with GibBehavior in mob entities
rg --multiline '(?s)parent:.*SimpleMobBase.*Destructible.*GibBehavior'
Length of output: 43025
Script:
#!/bin/bash
# Search for Destructible components with GibBehavior and their damage thresholds
rg --multiline --multiline-dotall 'type: Destructible.*?thresholds:.*?GibBehavior' -A 10
Length of output: 12873
Content.Shared/Backmen/Surgery/Body/SharedBodySystem.PartAppearance.cs (1)
16-17: Confirm necessity of calling base.Initialize()
In the InitializePartAppearances method, base.Initialize() is being called. Ensure that this call is required; otherwise, it might lead to unexpected behavior if Initialize() is not defined in the base class.
Please check if SharedBodySystem inherits from a base class with an Initialize() method. If not, remove the base.Initialize() call to prevent potential runtime errors.
Content.Server/Backmen/Surgery/SurgerySystem.cs (3)
52-54: Appropriate subscription to prototype reload events and initialization.
The code correctly subscribes to the PrototypesReloadedEventArgs event and calls LoadPrototypes() during initialization. This ensures that surgery prototypes are properly loaded and refreshed when prototypes are modified.
86-91: Effective UI refresh implementation for surgery interface.
By retrieving the UI actors and raising a SurgeryUiRefreshEvent, the code ensures that all clients with the surgery UI open receive timely updates. This addresses the UI refresh issues mentioned in the comment and enhances the responsiveness of the interface.
Line range hint 120-125: Efficient loading of surgery prototypes in LoadPrototypes().
The LoadPrototypes() method efficiently clears and reloads the _surgeries list by enumerating all EntityPrototypes that have a SurgeryComponent. This ensures that the list of available surgeries is always up-to-date with the current prototypes.
Content.Shared/Body/Systems/SharedBodySystem.Parts.cs (1)
179-180: Ensure proper handling of BodyPartEnableChangedEvent when dropping a part
Raising the BodyPartEnableChangedEvent(false) event when a body part is dropped ensures that systems listening for this event can update their state accordingly. Please verify that all relevant event handlers properly handle the Enabled property being set to false to prevent unintended side effects.
Run the following script to check for handlers of BodyPartEnableChangedEvent and ensure they correctly handle the Enabled flag:
✅ Verification successful
Let me gather more information about the usage and impact of the Enabled property.
Let me try a different search pattern to find usages of the Enabled property.
The BodyPartEnableChangedEvent is properly handled throughout the codebase
The event handling is implemented correctly with proper cascading effects:
- The
OnPartEnableChangedhandler inSharedBodySystemupdates the part'sEnabledstate and triggers appropriate follow-up actions throughEnablePart/DisablePart - The system raises specific events (
BodyPartEnabledEvent/BodyPartDisabledEvent) that are handled by dependent systems likeHandsSystem - The hands system properly manages hand functionality when parts are enabled/disabled
- The enabled state is properly networked through
[AutoNetworkedField]
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find all event handlers for BodyPartEnableChangedEvent and display surrounding code for context.
# Search for methods subscribing to BodyPartEnableChangedEvent
rg --type cs --context 5 "SubscribeLocalEvent<.*,\s*BodyPartEnableChangedEvent>"
# Search for methods handling BodyPartEnableChangedEvent
rg --type cs --context 5 "void\s+\w+\(Entity<BodyPartComponent>\s+\w+,\s*ref\s+BodyPartEnableChangedEvent\s+\w+\)"
Length of output: 2519
Script:
#!/bin/bash
# Search for usages of the Enabled property of BodyPartComponent
rg --type cs -C 5 "\.Enabled\b.*BodyPartComponent"
# Search for any methods that might be affected by the enabled state
rg --type cs -C 5 "BodyPartComponent.*\.Enabled\b"
# Search for the BodyPartComponent class definition to understand the Enabled property
ast-grep --pattern 'class BodyPartComponent {
$$$
Enabled $$$
$$$
}'
Length of output: 182
Script:
#!/bin/bash
# Search for the BodyPartComponent class definition and its properties
rg --type cs -C 10 "class BodyPartComponent"
# Search for any references to Enabled in body part related files
rg --type cs -C 5 "Enabled" "Content.Shared/Body/"
# Search for methods that check body part state
rg --type cs -C 5 "(?i)(?:check|is|get).*body.*part.*(?:enabled|disabled)"
Length of output: 9244
| @@ -149,7 +149,7 @@ | |||
| - type: entity | |||
| parent: SurgeryBase | |||
| id: SurgeryAttachLeftHand | |||
| name: Attach Left Hand | |||
| name: Attach Left Han | |||
There was a problem hiding this comment.
Fix typo in surgery name.
There's a typo in the surgery name: "Attach Left Han" should be "Attach Left Hand". This needs to be corrected to maintain consistency with other hand surgeries and ensure proper UI display.
Apply this diff to fix the typo:
- name: Attach Left Han
+ name: Attach Left Hand📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| name: Attach Left Han | |
| name: Attach Left Hand |
|
|
||
| _system.OnStep -= RefreshUI; |
There was a problem hiding this comment.
Add missing event unsubscription for OnPlayerItemAdded.
While the OnStep event is properly unsubscribed, the OnPlayerItemAdded event subscription is not cleaned up in the Dispose method.
protected override void Dispose(bool disposing)
{
base.Dispose(disposing);
if (disposing)
_window?.Dispose();
_system.OnStep -= RefreshUI;
+ _hands.OnPlayerItemAdded -= OnPlayerItemAdded;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _system.OnStep -= RefreshUI; | |
| protected override void Dispose(bool disposing) | |
| { | |
| base.Dispose(disposing); | |
| if (disposing) | |
| _window?.Dispose(); | |
| _system.OnStep -= RefreshUI; | |
| _hands.OnPlayerItemAdded -= OnPlayerItemAdded; | |
| } |
| private static readonly Dictionary<(BodyPartType, BodyPartSymmetry), HumanoidVisualLayers> BodyPartVisualLayers | ||
| = new Dictionary<(BodyPartType, BodyPartSymmetry), HumanoidVisualLayers> | ||
| { | ||
| { (BodyPartType.Head,BodyPartSymmetry.None), HumanoidVisualLayers.Head }, | ||
| { (BodyPartType.Tail,BodyPartSymmetry.None), HumanoidVisualLayers.Tail }, | ||
| { (BodyPartType.Torso,BodyPartSymmetry.None), HumanoidVisualLayers.Chest }, | ||
| { (BodyPartType.Arm,BodyPartSymmetry.Right), HumanoidVisualLayers.RArm }, | ||
| { (BodyPartType.Arm,BodyPartSymmetry.Left), HumanoidVisualLayers.LArm }, | ||
| { (BodyPartType.Hand,BodyPartSymmetry.Right), HumanoidVisualLayers.RHand }, | ||
| { (BodyPartType.Hand,BodyPartSymmetry.Left), HumanoidVisualLayers.LHand }, | ||
| { (BodyPartType.Leg,BodyPartSymmetry.Right), HumanoidVisualLayers.RLeg }, | ||
| { (BodyPartType.Leg,BodyPartSymmetry.Left), HumanoidVisualLayers.LLeg }, | ||
| { (BodyPartType.Foot,BodyPartSymmetry.Right), HumanoidVisualLayers.RLeg }, | ||
| { (BodyPartType.Foot,BodyPartSymmetry.Left), HumanoidVisualLayers.LLeg } | ||
| }; |
There was a problem hiding this comment.
Prevent potential KeyNotFoundException when accessing BodyPartVisualLayers
Accessing the BodyPartVisualLayers dictionary without verifying if the key exists may lead to a KeyNotFoundException when an unexpected combination of BodyPartType and BodyPartSymmetry is encountered.
Apply this diff to safely access the dictionary:
- var visualLayer = BodyPartVisualLayers[(part.PartType, part.Symmetry)];
+ if (BodyPartVisualLayers.TryGetValue((part.PartType, part.Symmetry), out var visualLayer))
+ {
+ // Existing logic using visualLayer
+ }
+ else
+ {
+ // Handle the missing key scenario
+ // For example, assign a default visualLayer or log a warning
+ visualLayer = HumanoidVisualLayers.Default;
+ }Committable suggestion skipped: line range outside the PR's diff.
| if (TryComp(uid, out HumanoidAppearanceComponent? bodyAppearance)) | ||
| { | ||
| _humanoid.SetLayerVisibility(entity, visualLayer, false, true, bodyAppearance); | ||
| var part = args.Part; | ||
| var customLayers = bodyAppearance.CustomBaseLayers; | ||
| var visualLayer = BodyPartVisualLayers[(part.Comp.PartType, part.Comp.Symmetry)]; | ||
| customLayers[visualLayer] = new CustomBaseLayerInfo(component.ID, customLayers[visualLayer].Color); |
There was a problem hiding this comment.
Add safety check when accessing BodyPartVisualLayers in OnPartAddedToBody
Similar to the previous issue, accessing the dictionary without checking may cause a KeyNotFoundException. Ensure the key exists before using it.
Apply this diff to add a safety check:
- var visualLayer = BodyPartVisualLayers[(part.Comp.PartType, part.Comp.Symmetry)];
+ if (BodyPartVisualLayers.TryGetValue((part.Comp.PartType, part.Comp.Symmetry), out var visualLayer))
+ {
+ // Update customLayers with visualLayer
+ customLayers[visualLayer] = new CustomBaseLayerInfo(component.ID, customLayers[visualLayer].Color);
+ }
+ else
+ {
+ // Handle the missing key scenario appropriately
+ // e.g., log a warning or assign a default value
+ }Committable suggestion skipped: line range outside the PR's diff.
Reverts #567
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements
Documentation