[Port] Blood Dagger / Кровавый Кинжал #86
Conversation
WalkthroughThis pull request introduces significant changes to the blood management and melee combat systems within the game. Key modifications include the addition of new components and systems for blood absorption and blood lust, as well as updates to existing systems to enhance gameplay dynamics related to blood loss and movement speed. Localization files have been updated to reflect changes in item names and descriptions, particularly for daggers, while new metadata for assets has been added. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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: 10
🧹 Outside diff range and nitpick comments (9)
Resources/Textures/_White/Objects/Weapons/Melee/Daggers/blood_dagger.rsi/meta.json (4)
1-8: LGTM with a minor formatting suggestion.The metadata information is correctly defined, including version, license, copyright, and size. The attribution to tgstation is appropriate.
Consider aligning the indentation of the "license" and "copyright" fields with the "version" field for consistency:
{ "version": 1, - "license": "CC-BY-SA-3.0", - "copyright": "Taken from tgstation at https://github.com/tgstation/", + "license": "CC-BY-SA-3.0", + "copyright": "Taken from tgstation at https://github.com/tgstation/", "size": { "x": 32, "y": 32 },
9-22: LGTM. Consider adding a comment to explain the animation.The "icon" state definition looks good. The delay sequence suggests an animation with 5 quick frames followed by a longer pause, which is suitable for a looping animation.
Consider adding a comment to explain the purpose of this animation, e.g.:
{ "name": "icon", "delays": [ [ 0.1, 0.1, 0.1, 0.1, 0.1, 0.5 ] ], "_comment": "Animates the blood dagger icon with a pulsing effect, pausing briefly at the end of each cycle." }
23-52: LGTM. Consider optimizing the delay sequence definition.The "inhand-right" state definition is correct, with appropriate delays for a multi-directional in-hand animation.
To reduce redundancy, consider consolidating the repeated delay sequences:
{ "name": "inhand-right", "directions": 4, "delays": [ - [ - 0.8, - 0.1, - 0.1, - 0.1 - ], - [ - 0.8, - 0.1, - 0.1, - 0.1 - ], - [ - 0.8, - 0.1, - 0.1, - 0.1 - ], - [ - 0.8, - 0.1, - 0.1, - 0.1 - ] + [0.8, 0.1, 0.1, 0.1] ] }This change maintains the same behavior while making the file more concise and easier to maintain.
53-84: LGTM. Consider optimizing the delay sequence definition.The "inhand-left" state definition is correct and consistent with the "inhand-right" state, which is appropriate for maintaining symmetry in the game.
As with the "inhand-right" state, consider consolidating the repeated delay sequences:
{ "name": "inhand-left", "directions": 4, "delays": [ - [ - 0.8, - 0.1, - 0.1, - 0.1 - ], - [ - 0.8, - 0.1, - 0.1, - 0.1 - ], - [ - 0.8, - 0.1, - 0.1, - 0.1 - ], - [ - 0.8, - 0.1, - 0.1, - 0.1 - ] + [0.8, 0.1, 0.1, 0.1] ] }This change maintains the same behavior while making the file more concise and easier to maintain.
Resources/Locale/en-US/_white/store/uplink-catalog.ftl (2)
22-23: LGTM! Consider minor grammatical improvement.The addition of the blood dagger is consistent with the PR objectives, and the description accurately reflects its unique mechanics. However, there's a minor grammatical issue in the last sentence of the description.
Consider applying this diff to improve the grammar:
uplink-blood-dagger-name = Blood dagger -uplink-blood-dagger-desc = A dagger of pain and blood. It has deadly accuracy, allowing you to deal critical damage and extract blood from opponents, treating the owner in proportion to the sucked blood. When absorbing the owner's blood, it briefly enhances his valuable profuse bleeding. +uplink-blood-dagger-desc = A dagger of pain and blood. It has deadly accuracy, allowing you to deal critical damage and extract blood from opponents, healing the owner in proportion to the absorbed blood. When absorbing the owner's blood, it briefly enhances their profuse bleeding.
Line range hint
1-23: Consider reorganizing entries for better readability.While the changes are consistent with the PR objectives and the file structure is intact, the entries for different types of items (weapons, implants, etc.) are mixed. Consider reorganizing the entries to group similar items together for better readability and maintainability.
Here's a suggested reorganization:
- Weapons (daggers, crossbow, etc.)
- Tools (flashlight, implanter)
- Implants
This organization will make it easier for future contributors to locate and update specific entries.
Resources/Locale/ru-RU/_white/store/uplink-catalog.ftl (1)
4-5: LGTM! Consider updating the description for consistency.The change from "knife" to "dagger" aligns with the PR objectives. However, for full consistency, consider updating the description to use "dagger" instead of "knife" if it's mentioned there as well.
Content.Server/_White/Melee/Crit/CritSystem.cs (1)
54-65: LGTM! Consider minor improvements for consistency and clarity.The new
CritHitEventclass is well-structured and uses modern C# features. The use ofIReadOnlyList<EntityUid>forTargetsis a good practice for immutability. To further improve the code:
Consider making the properties read-only to ensure immutability:
public EntityUid User { get; } public IReadOnlyList<EntityUid> Targets { get; }For consistency with C# naming conventions, consider renaming the parameter in the constructor from
targettotargets:public sealed class CritHitEvent(EntityUid user, IReadOnlyList<EntityUid> targets)Update the XML comment for
Targetsto use plural form:/// <summary> /// Entities that were attacked with a critical attack /// </summary>Resources/Prototypes/_White/Catalog/uplink_catalog.yml (1)
Residual References Detected for Removed Uplink Items
Several references to
UplinkEmpFlashlight,UplinkExperimentalSyndicateTeleporter, andUplinkMiniEbowstill exist in the codebase:
Content.Server/_White/EmpFlashlight/EmpOnHitComponent.csContent.Server/_White/EmpFlashlight/EmpOnHitSystem.csResources/Prototypes/_White/Entities/Objects/Devices/Syndicate_Gadgets/experimental_teleporter.ymlResources/Locale/ru-RU/experimentalsyndicateteleporter/experimentalsyndicateteleporter.ftlPlease ensure these references are intentionally retained or update/remove them to maintain consistency with the catalog changes.
🔗 Analysis chain
Line range hint
1-21: Please clarify the removal of several uplink items.The listings for UplinkEmpFlashlight, UplinkExperimentalSyndicateTeleporter, and UplinkMiniEbow have been removed. While these changes are noted in the AI summary, they are not mentioned in the PR objectives.
Could you please clarify:
- Are these removals intentional?
- How do these removals align with the overall game design and balance?
- What is the expected impact on gameplay?
To assess the impact of these removals, please run the following script to check for any remaining references to these items:
If any results are found, consider updating or removing these references to maintain consistency with the catalog changes.
Also applies to: 26-83
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for references to removed uplink items # Search for removed item names rg -i "EmpFlashlight|ExperimentalSyndicateTeleporter|MiniEbow"Length of output: 2181
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (6)
Resources/Textures/_White/Objects/Weapons/Melee/Daggers/betrayal_dagger.rsi/icon.pngis excluded by!**/*.pngResources/Textures/_White/Objects/Weapons/Melee/Daggers/betrayal_dagger.rsi/inhand-left.pngis excluded by!**/*.pngResources/Textures/_White/Objects/Weapons/Melee/Daggers/betrayal_dagger.rsi/inhand-right.pngis excluded by!**/*.pngResources/Textures/_White/Objects/Weapons/Melee/Daggers/blood_dagger.rsi/icon.pngis excluded by!**/*.pngResources/Textures/_White/Objects/Weapons/Melee/Daggers/blood_dagger.rsi/inhand-left.pngis excluded by!**/*.pngResources/Textures/_White/Objects/Weapons/Melee/Daggers/blood_dagger.rsi/inhand-right.pngis excluded by!**/*.png
📒 Files selected for processing (13)
- Content.Server/Body/Systems/BloodstreamSystem.cs (6 hunks)
- Content.Server/_White/Melee/BloodAbsorb/BloodAbsorbComponent.cs (1 hunks)
- Content.Server/_White/Melee/BloodAbsorb/BloodAbsorbSystem.cs (1 hunks)
- Content.Server/_White/Melee/Crit/CritSystem.cs (3 hunks)
- Content.Server/_White/Other/BloodLust/BloodLustComponent.cs (1 hunks)
- Content.Server/_White/Other/BloodLust/BloodLustSystem.cs (1 hunks)
- Resources/Locale/en-US/_white/store/uplink-catalog.ftl (2 hunks)
- Resources/Locale/ru-RU/_white/prototypes/entities/objects/weapons/melee/daggers.ftl (1 hunks)
- Resources/Locale/ru-RU/_white/store/uplink-catalog.ftl (2 hunks)
- Resources/Locale/ru-RU/store/uplink-catalog.ftl (1 hunks)
- Resources/Prototypes/_White/Catalog/uplink_catalog.yml (2 hunks)
- Resources/Prototypes/_White/Entities/Objects/Weapons/Melee/daggers.yml (2 hunks)
- Resources/Textures/_White/Objects/Weapons/Melee/Daggers/blood_dagger.rsi/meta.json (1 hunks)
🧰 Additional context used
🪛 yamllint
Resources/Prototypes/_White/Catalog/uplink_catalog.yml
[warning] 93-93: wrong indentation: expected 4 but found 2
(indentation)
[warning] 95-95: wrong indentation: expected 4 but found 2
(indentation)
[warning] 98-98: wrong indentation: expected 8 but found 6
(indentation)
🔇 Additional comments (26)
Resources/Locale/ru-RU/_white/prototypes/entities/objects/weapons/melee/daggers.ftl (3)
1-2: LGTM! Renaming from "BetrayalKnife" to "BetrayalDagger" is consistent.The renaming from "BetrayalKnife" to "BetrayalDagger" is consistent with the file name (daggers.ftl). The Russian translation and description are appropriate.
4-5: LGTM! New "BloodDagger" entry is consistent with PR objectives.The addition of the "BloodDagger" entry is consistent with the PR objectives mentioning the addition of a blood dagger. The Russian translation and description are appropriate and vivid, aligning well with the concept of a blood dagger.
1-5: Summary: Localization changes are consistent with PR objectives.The changes in this localization file accurately reflect the addition of the blood dagger as mentioned in the PR objectives. The renaming of "BetrayalKnife" to "BetrayalDagger" and the addition of the "BloodDagger" entry are both appropriate and well-translated. The descriptions are vivid and fitting for the weapons they describe.
Content.Server/_White/Other/BloodLust/BloodLustComponent.cs (4)
3-5: LGTM: Component structure is well-defined.The
BloodLustComponentis correctly registered and follows the expected patterns for components in this codebase. The use ofsealed partialis appropriate for performance and potential code generation.
12-13: Add documentation and consider balance implications.The
AttackRateModifierof 1.5 represents a significant 50% increase in attack rate. While this aligns with the "blood lust" theme, it could potentially lead to balance issues. Consider the following:
- Add XML documentation to explain the field's purpose and expected impact.
- Evaluate if this large increase in attack rate could lead to exploits or unbalanced gameplay.
- Consider implementing a cooldown or diminishing returns mechanism to prevent potential abuse.
To verify the usage and impact of this modifier, you can run the following script:
#!/bin/bash # Search for usages of AttackRateModifier to understand its impact rg --type csharp -g '!**/BloodLustComponent.cs' "AttackRateModifier" -C 5
6-7: Consider adding documentation and reviewing the modifier value.While the
SprintModifierfield is correctly defined, consider adding XML documentation to explain its purpose and impact on gameplay. Additionally, the value of 1.3 (30% increase) seems significant. Please ensure this aligns with the intended game balance.To verify the usage and impact of this modifier, you can run the following script:
#!/bin/bash # Search for usages of SprintModifier to understand its impact rg --type csharp -g '!**/BloodLustComponent.cs' "SprintModifier" -C 5
9-10: Reconsider modifier values and add documentation.The
WalkModifieris currently set to the same value asSprintModifier(1.3). This might not create a noticeable difference between sprinting and walking under the blood lust effect. Consider adjusting these values to create a more distinct difference. Also, add XML documentation to explain the field's purpose and impact.To verify the usage and impact of this modifier, you can run the following script:
✅ Verification successful
Verification Completed: WalkModifier is Unused.
The
WalkModifierfield inBloodLustComponent.csis currently not utilized anywhere in the codebase. If it's intended for future use, consider implementing its functionality. Otherwise, removing unused fields can help maintain a clean and efficient codebase.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for usages of WalkModifier to understand its impact rg --type csharp -g '!**/BloodLustComponent.cs' "WalkModifier" -C 5Length of output: 1221
Content.Server/_White/Melee/BloodAbsorb/BloodAbsorbComponent.cs (1)
1-5: LGTM: Namespace and component declaration are well-structured.The namespace
Content.Server._White.Melee.BloodAbsorbis appropriately organized, and the component is correctly registered using the[RegisterComponent]attribute. The class being sealed and partial is a good practice for performance and potential code organization.Resources/Textures/_White/Objects/Weapons/Melee/Daggers/blood_dagger.rsi/meta.json (1)
1-84: Overall, the meta.json file for the blood dagger asset is well-structured and complete.The file successfully defines all necessary components for the blood dagger asset, including metadata, icon state, and in-hand states for both left and right hands. The animations are well-defined with appropriate delays, ensuring smooth visualization in the game.
A few minor optimizations and documentation improvements have been suggested in the previous comments, which would enhance maintainability and clarity. These include:
- Consistent indentation in the metadata section.
- Adding a comment to explain the purpose of the icon animation.
- Consolidating repeated delay sequences in both "inhand-right" and "inhand-left" states.
Implementing these suggestions would further improve the quality of this well-crafted asset definition.
Resources/Prototypes/_White/Entities/Objects/Weapons/Melee/daggers.yml (3)
35-66: LGTM! New "blood dagger" entity added successfully.The new "blood dagger" entity has been implemented with appropriate components for a melee weapon, including unique features like blood absorption.
63-64: Please clarify the functionality of the BloodAbsorb component.The BloodAbsorb component with bloodLust set to true suggests a unique gameplay mechanic. Could you provide more details on how this component works and its impact on gameplay?
Run the following script to find the implementation of the BloodAbsorb component:
#!/bin/bash # Description: Find the implementation of the BloodAbsorb component # Test: Search for BloodAbsorb class definition. Expect: C# file containing the BloodAbsorb component implementation rg --type cs "class BloodAbsorb"Would you like me to review the BloodAbsorb component implementation once located?
5-8: LGTM! Verify entity references in the codebase.The renaming of "BetrayalKnife" to "BetrayalDagger" and the corresponding sprite path update improve consistency. However, ensure that all references to this entity in the codebase have been updated accordingly.
Run the following script to check for any remaining references to the old entity name:
Resources/Locale/ru-RU/_white/store/uplink-catalog.ftl (2)
21-21: LGTM! Clarify style guide for spacing.Adding a blank line can improve readability. However, to ensure consistency across the project, could you clarify if there's a specific style guide for spacing between entries in localization files?
#!/bin/bash # Check for consistent use of blank lines between entries rg -U --type ftl '(\n\n.+-.+(name|desc).+\n.+-.+(name|desc).+\n\n|\n\n.+-.+(name|desc).+\n.+-.+(name|desc).+$)' Resources/Locale/ru-RU/
22-23: LGTM! Verify gameplay mechanics with design team.The addition of the blood dagger aligns well with the PR objectives. The description provides a detailed explanation of its functionality, including critical damage, blood extraction, healing, and temporary empowerment through self-bleeding.
Given the complexity of these mechanics and their potential impact on gameplay balance, it would be prudent to verify that these align with the intended design.
Could you confirm that the design team has reviewed and approved these mechanics? Additionally, it might be helpful to run the following script to check for any related gameplay logic changes:
Content.Server/_White/Melee/Crit/CritSystem.cs (1)
Line range hint
23-36: Verify the impact of therefkeyword and consider documenting the new event.The change in the method signature to include
reffor theMeleeHitEvent argsparameter allows for modifications to the event arguments. This could potentially affect other systems that rely on this event.
- Ensure that all callers of this method are aware of the potential modifications to the
argsparameter.- Consider adding a comment explaining why the
refkeyword was added and what modifications, if any, are made to theargsparameter within this method.The addition of the
CritHitEventat the end of the method is a good way to allow other systems to react to critical hits. To improve maintainability:
- Consider adding XML documentation for the
HandleHitmethod, explaining its purpose and the significance of the new event.- Ensure that systems listening for
CritHitEventare properly documented and tested.To verify the usage of
MeleeHitEventand potential impact of therefkeyword, run the following script:✅ Verification successful
Verification Complete: No Conflicts Found
The introduction of the
refkeyword in theHandleHitmethod and the addition ofCritHitEventhave been reviewed. Therefmodification is confined, andCritHitEventhas only one subscriber (BloodAbsorbSystem.cs), ensuring limited impact on the codebase.Recommendations:
- Documentation: Add comments or XML documentation explaining the purpose of the
refkeyword and the newCritHitEvent.- Awareness: Ensure all method callers are informed about the potential modifications to the
argsparameter.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other occurrences of MeleeHitEvent to ensure compatibility with the ref keyword # Search for MeleeHitEvent usage echo "Searching for MeleeHitEvent usage:" rg --type csharp "MeleeHitEvent" -C 3 # Search for potential subscribers to CritHitEvent echo "\nSearching for potential subscribers to CritHitEvent:" rg --type csharp "SubscribeLocalEvent.*CritHitEvent" -C 3Length of output: 40549
Resources/Prototypes/_White/Catalog/uplink_catalog.yml (1)
22-25: LGTM! Verify related files for consistency.The renaming from "UplinkBetrayalKnife" to "UplinkBetrayalDagger" is consistent and aligns with the PR objectives. The change has been applied correctly to all relevant fields (id, name, description, and productEntity).
To ensure consistency across the codebase, please run the following script to check for any remaining references to "BetrayalKnife":
If any results are found, consider updating them to "BetrayalDagger" for consistency.
✅ Verification successful
All references to "BetrayalKnife" have been successfully updated. No remaining instances found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining references to "BetrayalKnife" # Search for "BetrayalKnife" in all files rg "BetrayalKnife"Length of output: 2181
Resources/Locale/ru-RU/store/uplink-catalog.ftl (4)
Line range hint
1-103: LGTM: New weapon and explosive entriesThe new entries for weapons and explosives are well-written and consistent in style. They provide clear descriptions of item functionality and usage, which aligns with the PR objective of adding new items to the game.
Line range hint
104-220: LGTM: New utility items and implants, including blood daggerThe new entries for utility items and implants are well-written and consistent in style. They provide clear descriptions of item functionality and usage, aligning with the PR objective of adding new items to the game.
Notably, the addition of the "blood dagger" (Кинжал жажды) on lines 108-109 directly fulfills the main objective of this PR as mentioned in the description.
Line range hint
221-359: LGTM: New bundles, tools, and job-specific itemsThe new entries for bundles, tools, and job-specific items are well-written and consistent in style. They provide clear descriptions of item contents and usage, which aligns with the PR objective of adding new items to the game.
Line range hint
360-443: LGTM: New armor, misc, and pointless items, including Special Forces HelmetThe new entries for armor, misc items, and pointless items are well-written and consistent in style. They provide clear descriptions of item functionality and usage, aligning with the PR objective of adding new items to the game.
Notably, a new item "Шлем спецназа" (Special Forces Helmet) has been added on lines 442-443. While not specifically mentioned in the PR objectives, it fits well with the overall theme of adding new equipment to the game.
Content.Server/_White/Other/BloodLust/BloodLustSystem.cs (1)
17-21: Verify thatargs.Useris the correct entity for retrievingBloodLustComponentIn
OnGetMeleeAttackRate, you are retrievingBloodLustComponentfromargs.User. Since this event is subscribed for entities withBloodLustComponent, consider whetheruid(the entity with the component) should be used instead ofargs.User. Confirm thatargs.Useris indeed the intended entity.To verify the correct entity to use, you can search the codebase for how
GetMeleeAttackRateEventis handled elsewhere:✅ Verification successful
Verified
args.Usercorrectly references the entity withBloodLustComponentNo issues found with using
args.Userto retrieveBloodLustComponentinOnGetMeleeAttackRate.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Search for usages of `OnGetMeleeAttackRate` handlers to verify entity usage. rg --type cs -A 5 'void\s+OnGetMeleeAttackRate\(.*GetMeleeAttackRateEvent args\)'Length of output: 748
Script:
#!/bin/bash # Description: Find the definition and usages of GetMeleeAttackRateEvent to understand the context of args.User. rg --type cs 'class\s+GetMeleeAttackRateEvent' -A 10Length of output: 54
Script:
#!/bin/bash # Description: Search for all occurrences of GetMeleeAttackRateEvent to find its definition and usages. rg --type cs 'GetMeleeAttackRateEvent'Length of output: 1020
Script:
#!/bin/bash # Description: Find all occurrences where args.User is used to retrieve BloodLustComponent. rg --type cs 'TryComp\(args\.User,\s*out\s+BloodLustComponent\?'Length of output: 191
Content.Server/_White/Melee/BloodAbsorb/BloodAbsorbSystem.cs (1)
62-66:⚠️ Potential issueCorrect the condition to apply effect when hitting other entities
The current condition proceeds only if the user has hit themselves (
args.HitEntities[0] != args.Userreturns false when the first hit entity is the user). If the intended behavior is to apply the effect when hitting others, you should return when the user hits themselves.Apply this diff to fix the condition:
-if (args.HitEntities.Count == 0 - || args.HitEntities[0] != args.User +if (args.HitEntities.Count == 0 + || args.HitEntities[0] == args.User || !component.BloodLust || !TryComp(args.User, out BloodstreamComponent? bloodstream)) return;Please confirm that this change aligns with the intended behavior of applying the BloodLust effect when hitting other entities.
Content.Server/Body/Systems/BloodstreamSystem.cs (4)
1-1: Import statement for BloodLust component is necessary.The addition of
using Content.Server._White.Other.BloodLust;correctly includes theBloodLustComponentneeded for the new logic.
18-18: Import statement for Movement systems is appropriate.Including
using Content.Shared.Movement.Systems;is required for accessingMovementSpeedModifierSystem.
44-44: Dependency injection of MovementSpeedModifierSystem is correctly added.The dependency on
_movementSpeedModifierallows for refreshing movement speed modifiers when necessary.
379-379: Conditional check updated appropriately to include 'createPuddle'.The condition now respects the
createPuddleparameter, allowing control over whether a puddle is created based on the caller's intent.
Описание PR
Порт и рефактор кровавого кинжала.
Изменения
🆑 Spatison