[Port/Tweak] Eshield / Е-Щит #94
Conversation
WalkthroughThe pull request introduces several modifications across multiple systems, primarily focusing on battery and weapon management. Key changes include the addition of the 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 (
|
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (12)
Resources/Locale/en-US/_white/rechargeable-blocking/rechargeable-blocking.ftl (1)
2-2: LGTM with a minor suggestion for improvement.The localization entry for the remaining charge time is well-structured and uses appropriate color formatting. However, consider externalizing the word "seconds" to improve localization flexibility.
Consider updating the entry to use a placeholder for the time unit:
-rechargeable-blocking-remaining-time = Time left to charge: [color=green]{ $remainingTime }[/color] seconds. +rechargeable-blocking-remaining-time = Time left to charge: [color=green]{ $remainingTime }[/color] { $timeUnit }.This change would allow for easier localization and potential future support for different time units.
Resources/Locale/ru-RU/_white/rechargeable-blocking/rechargeable-blocking.ftl (1)
2-2: LGTM with a minor suggestion for clarity.The localization string for the remaining charging time is well-structured and effectively uses color formatting to highlight the important information. The translation appears accurate and consistent with the English version.
Consider adding the word "оставшихся" (remaining) before "секунд" for improved clarity:
-rechargeable-blocking-remaining-time = Осталось времени для зарядки: [color=green]{ $remainingTime }[/color] секунд. +rechargeable-blocking-remaining-time = Осталось времени для зарядки: [color=green]{ $remainingTime }[/color] оставшихся секунд.This change would make the sentence more grammatically correct and clearer in Russian.
Content.Shared/Weapons/Ranged/Events/HitScanReflectAttempt.cs (1)
12-13: LGTM: Added Damage parameter to HitScanReflectAttemptEventThe addition of the
DamageSpecifier? Damageparameter to theHitScanReflectAttemptEventrecord struct is a good improvement. It allows for including damage information in the event, which can be useful for more detailed hit scan reflection handling.Consider adding a brief XML documentation comment for the new
Damageparameter to improve code clarity:/// <param name="Damage">Optional damage specification for the hit scan.</param>Content.Shared/_White/CVars.cs (1)
Line range hint
43-48: LGTM! Consider adding a brief comment for clarity.The addition of the
LogInChatCVar is well-structured and follows the existing patterns in the file. The flags used (CLIENT, ARCHIVE, REPLICATED) are appropriate for a client-side setting that can be saved and updated by the server.Consider adding a brief comment explaining the purpose of this CVar, for example:
/// <summary> /// Determines whether chat-related actions should be logged. /// </summary> public static readonly CVarDef<bool> LogInChat = CVarDef.Create("white.log_in_chat", true, CVar.CLIENT | CVar.ARCHIVE | CVar.REPLICATED);This would improve code documentation and make it easier for other developers to understand the CVar's purpose.
Content.Server/PowerCell/PowerCellSystem.cs (1)
Line range hint
234-243: LGTM! Consider adding XML documentation.The changes to
OnBatteryExaminedlook good. Making it public allows for more flexibility, which aligns with the PR objectives. The use ofResolveis consistent with the codebase style.Consider adding XML documentation to the method, especially since it's now public:
/// <summary> /// Examines the battery component and adds charge information to the examination text. /// </summary> /// <param name="uid">The entity UID.</param> /// <param name="component">The battery component, if any.</param> /// <param name="args">The examination event arguments.</param> public void OnBatteryExamined(EntityUid uid, BatteryComponent? component, ExaminedEvent args)Content.Server/Power/EntitySystems/BatterySystem.cs (2)
9-9: LGTM! Consider removing the comment.The addition of the
SharedContainerSystemdependency is correct and necessary for the new functionality. The using statement is appropriately added.Consider removing the "// WD EDIT" comment as it's not necessary and may clutter the code.
Also applies to: 17-18
205-231: LGTM! Consider some minor improvements.The
TryGetBatteryComponentmethod is well-implemented and aligns with the PR objectives. It correctly handles both direct entity components and contained batteries.
- Consider removing the "// WD EDIT START" and "// WD EDIT END" comments.
- For better readability, you could simplify the null check on line 225:
return batteryUid != null && TryComp(batteryUid, out battery);- Consider adding XML documentation comments to describe the method's purpose and parameters.
Resources/Prototypes/Entities/Objects/Shields/shields.yml (3)
368-378: Excellent enhancements to the EnergyShield entity!The changes significantly improve the functionality and realism of the energy shield. The addition of a battery system with self-recharging capability, along with the rechargeable blocking mechanism, adds depth to the gameplay. The adjustments to light and reflection properties also enhance the shield's visual and functional aspects.
Consider adding a comment explaining the
predictable: falsesetting in theItemTogglecomponent, as it might affect client-side behavior in multiplayer scenarios.Also applies to: 414-415, 418-419, 443-452
Line range hint
1-1000: Balanced adjustments to various shield entitiesThe minor changes to several shield entities (
WoodenBuckler,MakeshiftShield,MirrorShield, andTelescopicShield) appear to be part of a broader balancing effort. These adjustments to damage thresholds, destruction behaviors, and blocking modifiers help maintain game balance while differentiating the shields' characteristics.Consider adding a comment in the file or updating the documentation to explain the reasoning behind these balancing changes. This would be helpful for future maintenance and understanding of the game's design decisions.
Line range hint
1-1000: Overall improvement to shield mechanics and game balanceThe changes in this file significantly enhance the shield mechanics, particularly with the introduction of the rechargeable energy shield system. The balancing adjustments to various shield types contribute to a more diverse and engaging gameplay experience. These modifications align well with the PR objectives of porting and refactoring the energy shield changes from the Aviu project.
Consider creating a separate configuration file for shield balance parameters. This would make it easier to fine-tune game balance in the future without modifying entity definitions directly.
Content.Server/_White/Blocking/RechargeableBlockingSystem.cs (1)
64-64: Update localization key for accurate user feedbackThe localization key
"stunbaton-component-low-charge"is used, which references a stun baton. Since this is theRechargeableBlockingComponent, it would be more appropriate to use a specific localization key to provide accurate and context-specific messages to the user.Consider changing the localization string to:
-_popup.PopupEntity(Loc.GetString("stunbaton-component-low-charge"), args.User ?? uid); +_popup.PopupEntity(Loc.GetString("rechargeable-blocking-component-low-charge"), args.User ?? uid);And ensure that
"rechargeable-blocking-component-low-charge"is defined in the localization files with the appropriate message.Content.Shared/Weapons/Reflect/ReflectSystem.cs (1)
171-171: Update method documentation to reflect new parameterdamage.The method
TryReflectHitscannow includes theDamageSpecifier? damageparameter. Ensure that any documentation or XML comments for this method are updated to include the new parameter and explain its purpose.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
- Content.Server/Power/EntitySystems/BatterySystem.cs (3 hunks)
- Content.Server/PowerCell/PowerCellSystem.cs (1 hunks)
- Content.Server/Weapons/Ranged/Systems/GunSystem.cs (1 hunks)
- Content.Server/_White/Blocking/RechargeableBlockingComponent.cs (1 hunks)
- Content.Server/_White/Blocking/RechargeableBlockingSystem.cs (1 hunks)
- Content.Shared/Weapons/Ranged/Events/HitScanReflectAttempt.cs (2 hunks)
- Content.Shared/Weapons/Reflect/ReflectComponent.cs (1 hunks)
- Content.Shared/Weapons/Reflect/ReflectSystem.cs (7 hunks)
- Content.Shared/_White/CVars.cs (1 hunks)
- Resources/Locale/en-US/_white/rechargeable-blocking/rechargeable-blocking.ftl (1 hunks)
- Resources/Locale/ru-RU/_white/rechargeable-blocking/rechargeable-blocking.ftl (1 hunks)
- Resources/Prototypes/Catalog/uplink_catalog.yml (1 hunks)
- Resources/Prototypes/Entities/Objects/Shields/shields.yml (3 hunks)
🧰 Additional context used
🔇 Additional comments (19)
Resources/Locale/en-US/_white/rechargeable-blocking/rechargeable-blocking.ftl (1)
1-1: LGTM: Clear and well-formatted localization entry.The localization entry for the discharged state is concise and uses appropriate color formatting for emphasis.
Resources/Locale/ru-RU/_white/rechargeable-blocking/rechargeable-blocking.ftl (1)
1-1: LGTM: Clear and well-formatted localization string.The localization string for the discharged state is concise, clear, and effectively uses color formatting to emphasize the important information. The translation appears accurate and consistent with the English version.
Content.Server/_White/Blocking/RechargeableBlockingComponent.cs (2)
1-3: LGTM: Namespace and component registration are correct.The namespace
Content.Server._White.Blockingis well-structured, and the[RegisterComponent]attribute is correctly used to register the component in the game's component system.
4-5: LGTM: Component class definition is well-structured.The
RechargeableBlockingComponentclass is correctly defined as apublic sealed partial classinheriting fromComponent. This structure provides good performance, encapsulation, and flexibility for code organization.Content.Shared/Weapons/Ranged/Events/HitScanReflectAttempt.cs (2)
2-2: LGTM: Import added for DamageSpecifierThe added import for
Content.Shared.Damageis necessary and consistent with the changes made to theHitScanReflectAttemptEventrecord struct.
Line range hint
1-13: Summary: Enhanced HitScanReflectAttemptEvent with damage informationThe changes to this file improve the
HitScanReflectAttemptEventby adding the capability to include damage information. This enhancement aligns well with the PR objectives of porting and refactoring energy shield changes. The modification maintains backward compatibility and provides more flexibility in handling hit scan reflections.These changes, combined with the updates to the
GunSystemmentioned in the AI summary, should allow for more sophisticated interactions between weapons, shields, and damage systems.Content.Shared/Weapons/Reflect/ReflectComponent.cs (1)
35-39: 🛠️ Refactor suggestionEnhance documentation and initialization for the new
DamageOnReflectModifierfield.While the addition of this field aligns with the PR objectives for tweaking the energy shield, there are a few suggestions to improve its implementation:
Add XML documentation to clarify the purpose, usage, and any constraints of this modifier. This will help other developers understand how to use it correctly.
Consider initializing the field with a default value (e.g., 1.0f for no modification) to prevent potential issues if it's not set explicitly.
The field name could be more specific. Consider a name like
ReflectedDamageModifierorDamageModifierOnReflectto more clearly indicate its purpose.Here's a suggested implementation incorporating these changes:
/// <summary> /// Modifies the damage of reflected projectiles or hitscan attacks. /// A value of 1.0 means no change, less than 1.0 reduces damage, greater than 1.0 increases damage. /// </summary> [DataField, AutoNetworkedField] public float ReflectedDamageModifier = 1.0f;To ensure this change doesn't conflict with existing logic, let's check for any current usage:
✅ Verification successful
Verified the usage of
DamageOnReflectModifierand recommend initializing it with a default value.The
DamageOnReflectModifieris actively used inReflectSystem.csto adjust damage values. Currently, it lacks a default initialization, which defaults to0.0fand may unintentionally negate damage if not explicitly set.
- Initialize with a Default Value: Set
DamageOnReflectModifierto1.0fto ensure no damage alteration by default.- Enhance Documentation: Provide XML comments to clarify its purpose and usage.
- Naming Consistency: Consider renaming to
ReflectedDamageModifierfor better clarity./// <summary> /// Modifies the damage of reflected projectiles or hitscan attacks. /// A value of 1.0 means no change, less than 1.0 reduces damage, greater than 1.0 increases damage. /// </summary> [DataField, AutoNetworkedField] public float ReflectedDamageModifier = 1.0f;🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any existing references to DamageOnReflectModifier rg "DamageOnReflectModifier"Length of output: 658
Content.Server/Power/EntitySystems/BatterySystem.cs (2)
120-120: LGTM! Consistent use ofreffor events.The addition of
refforChargerUpdateStatusEventis correct and consistent with the coding style used elsewhere in the file.
Line range hint
1-233: Overall, the changes toBatterySystem.csare well-implemented and align with the PR objectives.The new
TryGetBatteryComponentmethod enhances the system's functionality, allowing for more flexible battery component retrieval. The changes are consistent with the existing codebase and improve the energy shield functionality as intended.Consider addressing the minor suggestions in the previous comments to further improve code readability and maintainability.
Resources/Prototypes/Entities/Objects/Shields/shields.yml (2)
Line range hint
1-1000: Improved effectiveness of RiotLaserShield against heat damageThe changes to the
RiotLaserShieldentity enhance its performance against heat-based attacks. The reduced coefficients for both passive and active blocking, along with the addition of a flat reduction for heat damage, make this shield more specialized and effective in its intended role.
Line range hint
1-1000: Enhanced protection of RiotBulletShield against ballistic attacksThe modifications to the
RiotBulletShieldentity significantly improve its performance against blunt and piercing damage. The reduced coefficients for both passive and active blocking, coupled with the increased flat reduction, make this shield more effective in its specialized role of protecting against bullets and other ballistic threats.Resources/Prototypes/Catalog/uplink_catalog.yml (2)
Line range hint
1453-1463: Cost adjustment approved.The increase in cost for UplinkHristovAmmo from 5 to 6 Telecrystals is a minor change, likely for balance purposes.
Line range hint
1453-1463: LGTM! Consider verifying the balance impact.The changes to the UplinkEshield listing look good. The reduced cost, added sale limit, and purchase restriction for non-NukeOps users could impact gameplay balance.
To ensure these changes don't negatively impact game balance, please run the following script:
✅ Verification successful
All changes to UplinkEshield have been verified and are consistent with existing balance adjustments.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage and impact of UplinkEshield in the game # Test: Check for references to UplinkEshield in other game files echo "Checking for UplinkEshield references:" rg "UplinkEshield" --type yaml --type cs # Test: Look for balance-related comments or code near UplinkEshield mentions echo "Checking for balance-related code near UplinkEshield:" rg -C 5 "UplinkEshield.*balance" --type yaml --type cs # Test: Check for any NukeOpsUplink tag usage echo "Checking for NukeOpsUplink tag usage:" rg "NukeOpsUplink" --type yaml --type csLength of output: 2908
Script:
#!/bin/bash # Description: Further verify balance implications related to UplinkEshield # Test: Search for balance-related terms in the codebase echo "Searching for balance-related terms in the codebase:" rg "balance|balancing|adjust_balance|game_balance" --type yaml --type cs # Test: Check for Telecrystal cost modifications in other listings echo "Checking for Telecrystal cost modifications in other listings:" rg "Telecrystal" --type yaml --type cs | rg -i "cost" # Test: Verify if saleLimit is enforced programmatically echo "Verifying enforcement of saleLimit for UplinkEshield:" rg -C 5 "saleLimit: 1" --type yaml --type csLength of output: 35561
Content.Server/_White/Blocking/RechargeableBlockingSystem.cs (1)
82-90:⚠️ Potential issueClarify charge thresholds in
CheckChargemethodThe logic in
CheckChargesetscomponent.Dischargedtotruewhenbattery.CurrentCharge < 1, and sets it tofalsewhenbattery.CurrentCharge > component.RechargeDelay. This leaves a range wherebattery.CurrentChargeis between1andcomponent.RechargeDelay, during whichcomponent.Dischargedremainstrue.Consider adjusting the logic to handle this range appropriately. If the component should be active when there is sufficient charge, you might modify the code as follows:
-if (battery.CurrentCharge < 1) +if (battery.CurrentCharge <= 0) { component.Discharged = true; _itemToggle.TryDeactivate(uid, predicted: false); } -if (battery.CurrentCharge > component.RechargeDelay) +else if (battery.CurrentCharge >= component.RechargeDelay) { component.Discharged = false; }This ensures that
component.Dischargedis set tofalsewhen the charge level is equal to or exceeds the required delay, and only set totruewhen the charge is depleted.Content.Shared/Weapons/Reflect/ReflectSystem.cs (4)
128-134: Verify multiplication ofDamageSpecifierby a float.Ensure that
DamageSpecifiersupports multiplication by a float (reflect.DamageOnReflectModifier). If not, you may need to implement a method to scale the damage values appropriately.Consider defining a method to multiply each damage type within
DamageSpecifierby the modifier. For example:private DamageSpecifier ScaleDamage(DamageSpecifier damage, float modifier) { var scaledDamage = new DamageSpecifier(); foreach (var (type, amount) in damage.DamageDict) { scaledDamage.DamageDict[type] = amount * modifier; } return scaledDamage; }Then apply the scaled damage:
if (reflect.DamageOnReflectModifier != 0) { + if (projectileComp.Damage != null) + { + var scaledDamage = ScaleDamage(projectileComp.Damage, reflect.DamageOnReflectModifier); _damageable.TryChangeDamage(reflector, scaledDamage, projectileComp.IgnoreResistances, origin: projectileComp.Shooter); + } }
158-158: Confirm passinguidfor bothuserandreflectoris intended.In the
TryReflectHitscanmethod call,uidis passed for bothuserandreflector. Verify that this is the intended behavior and that it does not introduce any unintended side effects.
39-39: LGTM!The addition of the
DamageableSystemdependency is appropriate and necessary for applying damage within the reflect logic.
189-191:⚠️ Potential issueCheck for null
damagebefore applying damage to the reflector.While there is a null check for
damage, ensure thatdamageis not null when multiplying byreflect.DamageOnReflectModifierto prevent potential exceptions.Also, similar to the earlier comment, verify that multiplying a
DamageSpecifierby a float operates as intended.Content.Server/Weapons/Ranged/Systems/GunSystem.cs (1)
213-213:⚠️ Potential issueEnsure all event handlers for
HitScanReflectAttemptEventhandle the newDamageparameterThe addition of
hitscan.Damageto theHitScanReflectAttemptEventconstructor means that any existing event handlers listening for this event need to be updated to accept and properly process the newDamageparameter. This is crucial to prevent runtime exceptions and ensure that damage reflection behaves as intended.Run the following script to locate all event handlers for
HitScanReflectAttemptEventand verify they have been updated:
Описание PR
Порт и рефактор изменений энерго щита от авиу.
Изменения
🆑 Spatison