Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Port] Crossbow / Арбалет #76

Merged
merged 14 commits into from
Oct 26, 2024
Merged

[Port] Crossbow / Арбалет #76

merged 14 commits into from
Oct 26, 2024

Conversation

Spatison
Copy link
Member

@Spatison Spatison commented Oct 5, 2024

Описание PR

Порт и рефактор арбалета.


Медиа

Видео


Изменения

🆑 Spatison

  • add: Added crossbow / Добавлен арбалет

@Spatison Spatison requested a review from Remuchi October 5, 2024 18:33
@Spatison Spatison self-assigned this Oct 5, 2024
Copy link
Contributor

coderabbitai bot commented Oct 5, 2024

Walkthrough

This pull request introduces significant updates to the projectile and weapon systems within the game. The ProjectileSystem class has been enhanced with new dependencies and methods to better manage embedded projectiles, including event handling for embedding and removal. A new PoweredComponent class has been added to manage energy consumption and projectile speed. Additionally, two new entities for powered crossbows have been defined in the YAML configuration, and new tags have been introduced to categorize projectile types.

Changes

File Path Change Summary
Content.Server/Projectiles/ProjectileSystem.cs Enhanced with new dependencies and methods for managing embedded projectiles, including OnEmbed, OnEmbedActivate, and OnEmbedRemove.
Content.Server/_White/Guns/PoweredComponent.cs New class PoweredComponent added to manage energy consumption and projectile speed with fields EnergyPerUse and ProjectileSpeedModified.
Content.Shared/Projectiles/SharedProjectileSystem.cs Modified Embed method visibility and logic for embedding projectiles, added event raising capabilities.
Resources/Prototypes/_White/Entities/Objects/Weapons/Guns/crossbow.yml Introduced two new entities: WeaponPoweredCrossbow and WeaponPoweredCrossbowUnfinished, each with various components defining their characteristics and functionality.
Resources/Prototypes/_White/tags.yml Added new tags CrossbowBolt and Telecrystal.

Possibly related PRs

Suggested reviewers

  • Remuchi

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 3023dc7 and 705415e.

📒 Files selected for processing (1)
  • Resources/Prototypes/_White/tags.yml (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • Resources/Prototypes/_White/tags.yml

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

github-actions bot commented Oct 5, 2024

RSI Diff Bot; head commit 9d3c804 merging into 9131814
This PR makes changes to 1 or more RSIs. Here is a summary of all changes:

Resources/Textures/_White/Objects/Weapons/Guns/crossbow.rsi

State Old New Status
crossbow Added
inhand-left Added
inhand-right Added
loaded Added
stretched Added
unstrung Added

Edit: diff updated after 9d3c804

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 21

🧹 Outside diff range and nitpick comments (38)
Resources/Locale/ru-RU/_white/prototypes/entities/objects/weapons/guns/crossbow.ftl (2)

1-2: Consider expanding the description for more detail.

The translation for "арбалет" (crossbow) is accurate. However, the description "Опасная вещь" (Dangerous thing) is quite brief.

Consider expanding the description to provide more specific details about the crossbow, such as its power source or unique features. For example:

.desc = Мощный арбалет с электрическим механизмом. Очень опасное оружие дальнего боя.

(Translation: A powerful crossbow with an electric mechanism. A very dangerous long-range weapon.)


3-4: LGTM! Consider a minor rewording for clarity.

The translation for "недоделанный арбалет" (unfinished crossbow) is accurate and consistent with the previous entity.

The description "Будет опасной вещью" (Will be a dangerous thing) is good but could be slightly improved for clarity. Consider:

.desc = Незавершенный арбалет. После доработки станет опасным оружием.

(Translation: An incomplete crossbow. Once finished, it will become a dangerous weapon.)
This provides a bit more context about the item's current state and future potential.

Resources/Locale/ru-RU/prototypes/entities/objects/weapons/melee/stunprod.ftl (1)

1-7: Overall: Good localization updates supporting the crossbow feature.

The changes to this localization file are well-structured and support the PR objectives of porting and refactoring the crossbow feature. The modular approach used in defining entities (base and specific instances) is a good practice for maintainability.

However, there are a couple of points to consider:

  1. The file is named stunprod.ftl, but the PR objectives mention a crossbow. Verify if this file should be renamed or if additional files for the crossbow are missing from this review.
  2. Ensure that the use of "illegal" in the stunprod description aligns with the game's context.

Consider creating a separate localization file for the crossbow if it doesn't already exist, to maintain a clear separation of weapon types in the localization structure.

Content.Shared/_White/Penetrated/PenetratedComponent.cs (3)

1-1: Consider revising the namespace naming convention.

The current namespace Content.Shared._White.Penetrated uses an underscore in _White, which is unconventional in C# naming practices. Consider removing the underscore to align with standard C# naming conventions, e.g., Content.Shared.White.Penetrated.


6-10: Add XML documentation for properties.

While the property names are descriptive, adding XML documentation would greatly improve code readability and maintainability. Consider adding comments to explain the purpose and usage of each property.

Here's an example of how you could document these properties:

/// <summary>
/// The unique identifier of the projectile that penetrated this entity.
/// </summary>
[DataField]
public EntityUid? ProjectileUid;

/// <summary>
/// Indicates whether the entity is currently pinned in place by the penetrating projectile.
/// </summary>
[DataField]
public bool IsPinned;

1-11: Overall implementation aligns well with PR objectives.

The PenetratedComponent introduces a mechanism for handling projectile penetration, which aligns with the PR objective of adding a crossbow to the game. The component's design allows for tracking both the penetrating projectile and the pinned state of an entity, which could be useful for implementing crossbow bolt mechanics.

Some suggestions for improvement:

  1. Consider adding more detailed XML documentation to clarify the component's usage in the context of the crossbow feature.
  2. You might want to add methods for handling the penetration and unpinning logic, unless these are implemented elsewhere in the system.
Content.Server/_White/Guns/PoweredComponent.cs (1)

1-5: LGTM! Namespace and class declaration are well-structured.

The PoweredComponent class is correctly set up with the [RegisterComponent] attribute and appropriate modifiers. The namespace structure is clear, although the use of an underscore in _White is unconventional. If this follows a specific project convention, it's fine; otherwise, consider removing the underscore for consistency with C# naming conventions.

Content.Client/_White/Guns/Stretched/StretchedVisualsComponent.cs (3)

1-1: Consider revising the namespace naming convention.

The current namespace Content.Client._White.Guns.Stretched uses an underscore in _White, which is unconventional in C# naming practices. Consider removing the underscore to align with standard C# naming conventions, e.g., Content.Client.White.Guns.Stretched.


6-7: LGTM: LoadedState property is well-defined. Consider adding a comment.

The LoadedState property is appropriately defined:

  • Correct use of nullable string type.
  • [DataField] attribute for serialization.

To enhance code clarity, consider adding a brief comment explaining the purpose of this state, e.g.:

/// <summary>
/// Represents the visual state when the crossbow is loaded.
/// </summary>
[DataField]
public string? LoadedState;

9-10: LGTM: StretchedState property is well-defined. Consider adding a comment.

The StretchedState property is appropriately defined:

  • Correct use of nullable string type.
  • [DataField] attribute for serialization.

To enhance code clarity, consider adding a brief comment explaining the purpose of this state, e.g.:

/// <summary>
/// Represents the visual state when the crossbow is stretched/drawn.
/// </summary>
[DataField]
public string? StretchedState;
Content.Shared/_White/Projectile/PenetratedProjectileComponent.cs (4)

5-7: LGTM: Class declaration and attributes are correct.

The [RegisterComponent] attribute and class definition are appropriate for a game component. The class name clearly indicates its purpose.

Consider renaming to PenetratingProjectileComponent to imply an ongoing action rather than a completed state, which might be more accurate for a component that handles penetration logic.


8-9: Add XML documentation and consider making the field readonly.

While the MinimumSpeed field is correctly defined and attributed, consider the following improvements:

  1. Add XML documentation to explain the purpose and unit of this field.
  2. If this value shouldn't change during runtime, consider making it readonly.

Example:

/// <summary>
/// The minimum speed (in units per second) required for the projectile to penetrate.
/// </summary>
[DataField]
public readonly float MinimumSpeed = 40f;

Also, could you provide more context on how this value was determined? It might be helpful to add a comment explaining the reasoning behind the default value.


11-16: Add XML documentation and consider access modifiers for Damage and PenetratedUid fields.

The Damage and PenetratedUid fields are correctly defined, but could benefit from the following improvements:

  1. Add XML documentation to explain the purpose and usage of each field.
  2. Consider whether these fields need to be publicly settable. If not, make them public get; private set; properties.

Example:

/// <summary>
/// Specifies the damage characteristics of the penetrating projectile.
/// </summary>
[DataField]
public DamageSpecifier Damage { get; private set; } = new();

/// <summary>
/// Stores the EntityUid of the last entity penetrated by this projectile, if any.
/// </summary>
[DataField]
public EntityUid? PenetratedUid { get; private set; }

Also, consider adding validation logic for the Damage field to ensure it's always initialized with a valid DamageSpecifier.


1-16: Overall assessment: Good foundation, but consider broader implementation details

The PenetratedProjectileComponent provides a good foundation for implementing penetrating projectile behavior, which aligns well with the PR objective of adding a crossbow. The component structure is correct and includes necessary fields for speed threshold, damage specification, and penetrated entity tracking.

However, to ensure a robust implementation, consider the following:

  1. Add comprehensive XML documentation to all fields and the class itself.
  2. Implement or reference the actual penetration logic, possibly in a separate system class.
  3. Consider adding methods for resetting or updating the component's state after penetration.
  4. Ensure proper integration with other relevant systems (e.g., physics, damage application).
  5. Add unit tests to verify the component's behavior in various scenarios.

Lastly, it might be beneficial to provide a brief comment or documentation explaining how this component fits into the broader crossbow implementation and any specific gameplay mechanics it enables.

Content.Shared/_White/Guns/Stretched/StretchedComponent.cs (2)

9-10: Consider adding [DataField] attribute to the Stretched property.

The Stretched property might need to be serialized if it's part of the component's persistent state. If this is the case, consider adding the [DataField] attribute:

 [ViewVariables]
+[DataField]
 public bool Stretched;

This ensures the property can be properly serialized and deserialized when needed.


6-8: Add a class-level comment explaining the component's purpose.

To improve code documentation and make it easier for other developers to understand the role of this component, consider adding a class-level comment that explains its purpose in the context of the crossbow feature:

 [RegisterComponent]
+/// <summary>
+/// Represents the stretched state of a crossbow, managing its draw sound and ammo provider.
+/// This component is part of the crossbow weapon system introduced in the "[Port] Crossbow / Арбалет" feature.
+/// </summary>
 public sealed partial class StretchedComponent : Component

This comment provides context for the component and links it to the specific feature being implemented.

Resources/Textures/_White/Objects/Weapons/Guns/crossbow.rsi/meta.json (1)

1-4: LGTM! Consider formatting the URL for easier access.

The metadata section is well-structured with appropriate version and license information. The detailed copyright information is commendable.

Consider formatting the URL in the copyright information for easier access:

-  "copyright": "Taken from paradise at https://github.com/ParadiseSS13/Paradise at 76d0428022d17f3249585d96ac9b69076206efd4. Edit by Spatison",
+  "copyright": "Taken from paradise at https://github.com/ParadiseSS13/Paradise/tree/76d0428022d17f3249585d96ac9b69076206efd4. Edit by Spatison",
Content.Shared/Projectiles/EmbedEvent.cs (3)

17-28: LGTM! Consider adding a class-level summary comment.

The addition of AttemptEmbedEvent is a good practice, likely used to handle pre-embed logic. Its structure mirrors EmbedEvent, which is appropriate given they represent the same data at different stages of the embedding process.

Consider adding a class-level summary comment to AttemptEmbedEvent, similar to the one for EmbedEvent. This would improve code documentation and clarify the purpose of this new event. For example:

/// <summary>
/// Raised directed on an entity when an attempt is made to embed it in another entity.
/// </summary>
[ByRefEvent]
public readonly record struct AttemptEmbedEvent(EntityUid? Shooter, EntityUid Embedded)
{
    // ... existing code ...
}

Line range hint 1-28: Consider refactoring to reduce code duplication.

The AttemptEmbedEvent and EmbedEvent structs have identical structures. To improve maintainability and reduce code duplication, consider refactoring these into a base struct or interface.

Here's a suggested refactor:

namespace Content.Shared.Projectiles;

public interface IEmbedEvent
{
    EntityUid? Shooter { get; }
    EntityUid Embedded { get; }
}

/// <summary>
/// Raised directed on an entity when it embeds in another entity.
/// </summary>
[ByRefEvent]
public readonly record struct EmbedEvent : IEmbedEvent
{
    public EntityUid? Shooter { get; }
    public EntityUid Embedded { get; }

    public EmbedEvent(EntityUid? shooter, EntityUid embedded)
    {
        Shooter = shooter;
        Embedded = embedded;
    }
}

/// <summary>
/// Raised directed on an entity when an attempt is made to embed it in another entity.
/// </summary>
[ByRefEvent]
public readonly record struct AttemptEmbedEvent : IEmbedEvent
{
    public EntityUid? Shooter { get; }
    public EntityUid Embedded { get; }

    public AttemptEmbedEvent(EntityUid? shooter, EntityUid embedded)
    {
        Shooter = shooter;
        Embedded = embedded;
    }
}

This refactoring centralizes the common properties in the IEmbedEvent interface, reducing duplication and making future changes easier to manage.


Line range hint 1-28: Enhance property documentation with XML comments.

While the code structure is good, adding XML documentation comments to the properties would improve code clarity and IDE support.

Consider adding XML documentation to the properties in both EmbedEvent and AttemptEmbedEvent. For example:

public readonly record struct EmbedEvent(EntityUid? Shooter, EntityUid Embedded)
{
    /// <summary>
    /// The entity that initiated the embedding, if any.
    /// </summary>
    public readonly EntityUid? Shooter = Shooter;

    /// <summary>
    /// The entity that is embedded in another entity.
    /// </summary>
    public readonly EntityUid Embedded = Embedded;
}

Apply similar documentation to AttemptEmbedEvent. This will provide more context when using these properties throughout the codebase.

Content.Client/_White/Guns/Stretched/StretchedVisualizerSystem.cs (1)

9-9: LGTM: Proper use of dependency injection.

The dependency on AppearanceSystem is correctly injected using the [Dependency] attribute. This is a good practice for managing system dependencies.

Consider adding a comment explaining the purpose of the AppearanceSystem in this context, to improve code readability:

[Dependency] private readonly AppearanceSystem _appearance = default!; // Used for managing visual states based on entity appearance
Resources/Prototypes/Entities/Objects/Misc/improvised_gun_parts.yml (2)

30-35: LGTM! Consider adding a comment explaining the null deconstructionTarget.

The changes to the RifleStock entity align well with the PR objective of implementing a crossbow. The modification of the graph to WeaponPoweredCrossbowGraph and the node to stock suggests a good refactoring to accommodate the new weapon type.

Consider adding a brief comment explaining why deconstructionTarget is set to null. This would improve code maintainability and clarify the design decision. For example:

- type: Construction
  # Prevent deconstruction to maintain game balance
  deconstructionTarget: null
  graph: WeaponPoweredCrossbowGraph
  node: stock

Line range hint 2-2: Follow up on the TODO for asset management.

There's a TODO comment about assimilating these entities into the same RSI (Rsi = Robust Station Image, the sprite system used in SS14). This suggestion for improvement should be tracked and addressed in future updates.

Consider creating a GitHub issue to track this TODO item for future development. This will ensure that the asset management improvement isn't forgotten and can be prioritized appropriately.

Content.Server/Weapons/Ranged/Systems/GunSystem.Ballistic.cs (1)

40-45: LGTM: New method implemented correctly. Consider removing edit comments.

The GetStackEntity method is well-implemented, correctly using the StackSystem to split stacks and handling potential failures. The override is properly declared, and the logic is concise and clear.

Consider removing the "WD EDIT START" and "WD EDIT END" comments, as they are typically not needed in the final code and can clutter the file.

Resources/Prototypes/_White/Entities/Objects/Weapons/Guns/crossbow.yml (2)

27-29: Fix indentation for better readability.

The tags under whitelist are indented with 8 spaces, while the YAML standard typically uses 2 spaces for each indentation level. Adjust the indentation to maintain consistency with the rest of the file.

Apply this change:

  - type: BallisticAmmoProvider
    whitelist:
      tags:
-        - CrossbowBolt
+      - CrossbowBolt
🧰 Tools
🪛 yamllint

[warning] 29-29: wrong indentation: expected 6 but found 8

(indentation)


45-45: Remove trailing space.

There's a trailing space at the end of this line. While it doesn't affect functionality, it's good practice to remove trailing spaces to maintain clean code.

Apply this change:

-  - type: Stretched 
+  - type: Stretched
🧰 Tools
🪛 yamllint

[error] 45-45: trailing spaces

(trailing-spaces)

Resources/Prototypes/Entities/Objects/Specific/syndicate.yml (2)

24-26: LGTM! Consider standardizing edit comments.

The addition of the Tag component with the "Telecrystal" tag is a good practice for entity identification and filtering. This change looks good and aligns well with the entity's purpose.

Consider standardizing the edit comment format. Instead of "# WD EDIT", you might want to use a more descriptive comment that explains the reason for the change, such as "# Added tag for easier identification in game logic".


Line range hint 114-116: LGTM! Consider consistent formatting for tags.

The addition of the Tag component with the "NukeOpsUplink" tag is appropriate and aligns well with the entity's intended use for Nuclear Operatives. This change looks good and will help in filtering this specific uplink type in the game logic.

For consistency with the telecrystal entity, consider moving the Tag component higher up in the component list, right after the Store component. This would maintain a consistent structure across similar entities in the file.

Content.Server/Stunnable/Systems/StunbatonSystem.cs (3)

54-54: Consistent use of BatterySystem in OnExamined

The change to use _battery.TryGetBatteryComponent is consistent with the overall refactoring. For even better consistency, consider using the same pattern as in OnStaminaHitAttempt where both the battery component and entity are retrieved.

You could update the line to:

if (_battery.TryGetBatteryComponent(entity, out var battery, out var batteryUid))

This would make it consistent with other usage in the file and potentially useful if you need the battery entity in the future.


Line range hint 69-80: Improved battery check in TryTurnOn

The refactoring to use _battery.TryGetBatteryComponent is a good improvement, consistent with the changes in other methods. The logic for checking battery presence and charge is correct.

For consistency with other parts of the code, consider updating the battery check to:

if (!_battery.TryGetBatteryComponent(entity, out var battery, out var batteryUid)
    || battery.CurrentCharge < entity.Comp.EnergyPerUse)

This retrieves the battery entity as well, which might be useful in the future and maintains consistency with other method calls.


125-128: Improved CheckCharge method

The refactoring of CheckCharge to use _battery.TryGetBatteryComponent is consistent with the changes in other methods. The logic for checking battery presence and charge is correct, and the baton is appropriately deactivated when the charge is insufficient.

For consistency with other parts of the code, consider updating the battery check to:

if (!_battery.TryGetBatteryComponent(entity, out var battery, out var batteryUid)
    || battery.CurrentCharge < entity.Comp.EnergyPerUse)
    _itemToggle.TryDeactivate(entity.Owner, predicted: false);

This retrieves the battery entity as well, maintaining consistency with other method calls.

Content.Shared/Weapons/Ranged/Components/GunComponent.cs (1)

250-254: Approved addition. Consider adding documentation and clarifying its relation to the crossbow feature.

The addition of the ThrowAngle property is a good enhancement to the GunComponent class. It allows for optional specification of a throwing angle for guns, which could add interesting gameplay mechanics.

A few suggestions to improve this addition:

  1. Consider adding XML documentation to explain the purpose and usage of this property. For example:

    /// <summary>
    /// The angle at which the gun can be thrown, if applicable.
    /// </summary>
    [DataField]
    public Angle? ThrowAngle;
  2. Could you clarify how this property relates to the crossbow feature mentioned in the PR objectives? It would be helpful to understand if this is specific to crossbows or if it applies to all guns.

Content.Server/Power/EntitySystems/BatterySystem.cs (2)

9-9: LGTM! Consider removing the comment.

The addition of the SharedContainerSystem dependency is correct and necessary for the new functionality. However, the comment "// WD EDIT" might not be necessary in the long term and could be removed for cleaner code.

Consider removing the "// WD EDIT" comment:

-        [Dependency] private readonly SharedContainerSystem _containers = default!; // WD EDIT
+        [Dependency] private readonly SharedContainerSystem _containers = default!;

Also applies to: 17-17


205-231: LGTM! Consider some minor improvements.

The TryGetBatteryComponent method is well-implemented and provides a useful abstraction for getting a BatteryComponent from an entity or its container. Here are some suggestions for minor improvements:

  1. Consider removing the "WD EDIT START" and "WD EDIT END" comments for cleaner code.
  2. The method could benefit from a XML documentation comment explaining its purpose and parameters.
  3. Consider using pattern matching for a more concise null check on line 225.

Here's a suggested implementation with these improvements:

/// <summary>
/// Attempts to get a BatteryComponent from an entity or its "cell_slot" container.
/// </summary>
/// <param name="uid">The entity UID to check.</param>
/// <param name="battery">The found BatteryComponent, if any.</param>
/// <param name="batteryUid">The UID of the entity with the BatteryComponent, if found.</param>
/// <returns>True if a BatteryComponent was found, false otherwise.</returns>
public bool TryGetBatteryComponent(EntityUid uid, [NotNullWhen(true)] out BatteryComponent? battery,
    [NotNullWhen(true)] out EntityUid? batteryUid)
{
    if (TryComp(uid, out battery))
    {
        batteryUid = uid;
        return true;
    }

    if (!_containers.TryGetContainer(uid, "cell_slot", out var container)
        || container is not ContainerSlot slot)
    {
        battery = null;
        batteryUid = null;
        return false;
    }

    batteryUid = slot.ContainedEntity;

    return batteryUid is { } entity && TryComp(entity, out battery);
}
Content.Shared/_White/Penetrated/PenetratedSystem.cs (2)

13-13: Consider adding XML documentation comments for 'FreePenetrated' method

Adding XML documentation for public methods enhances code readability and assists other developers in understanding the method's functionality and usage.


24-25: Consistently use braces for single-line 'if' statements to improve readability

Including braces even for single-line if statements enhances code readability and maintainability.

Apply this diff to improve consistency:

 if (!Resolve(uid, ref penetrated, false))
-    return;
+{
+    return;
+}
Content.Server/_White/Projectiles/PenetratedProjectileSystem.cs (1)

37-37: Rename physicEmbedded to embeddedPhysics for clarity

The variable name physicEmbedded may cause confusion. Renaming it to embeddedPhysics improves readability and aligns with naming conventions.

Apply this change:

- || !TryComp(args.Embedded, out PhysicsComponent? physicEmbedded)
+ || !TryComp(args.Embedded, out PhysicsComponent? embeddedPhysics)

Ensure all references to physicEmbedded are updated accordingly.

Content.Server/Weapons/Ranged/Systems/GunSystem.cs (1)

Base class Shoot method signature does not include the new parameter bool throwItems = false.
Altering the overridden method without corresponding changes in the base class can lead to compile-time errors or unexpected behavior.
Ensure that the base class method in SharedGunSystem is updated accordingly, or consider overloading the method instead of modifying it.

🔗 Analysis chain

Line range hint 75-78: Method signature modification may cause override inconsistencies

You've added a new parameter bool throwItems = false to the overridden Shoot method. Altering the signature of an overridden method without corresponding changes in the base class can lead to compile-time errors or unexpected behavior. Ensure that the base class method in SharedGunSystem is updated accordingly, or consider overloading the method instead of modifying it.

Run the following script to verify that the base class method signature matches and update all method calls accordingly:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that the base class method signature includes the new parameter and find all calls to `Shoot`.

# Check the base class method definition.
rg 'public virtual void Shoot\(' -A 5 -B 2

# Expected result: The base class `Shoot` method should include `bool throwItems = false` as a parameter.

# Find all calls to the `Shoot` method in the codebase.
rg '\.Shoot\(' -A 2 -B 2

# Expected result: All calls to `Shoot` should be examined to ensure they pass the new `throwItems` parameter if necessary.

Length of output: 3905

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 4c2f775 and 7cefc75.

⛔ Files ignored due to path filters (6)
  • Resources/Textures/_White/Objects/Weapons/Guns/crossbow.rsi/crossbow.png is excluded by !**/*.png
  • Resources/Textures/_White/Objects/Weapons/Guns/crossbow.rsi/inhand-left.png is excluded by !**/*.png
  • Resources/Textures/_White/Objects/Weapons/Guns/crossbow.rsi/inhand-right.png is excluded by !**/*.png
  • Resources/Textures/_White/Objects/Weapons/Guns/crossbow.rsi/loaded.png is excluded by !**/*.png
  • Resources/Textures/_White/Objects/Weapons/Guns/crossbow.rsi/stretched.png is excluded by !**/*.png
  • Resources/Textures/_White/Objects/Weapons/Guns/crossbow.rsi/unstrung.png is excluded by !**/*.png
📒 Files selected for processing (32)
  • Content.Client/_White/Guns/Stretched/StretchedVisualizerSystem.cs (1 hunks)
  • Content.Client/_White/Guns/Stretched/StretchedVisualsComponent.cs (1 hunks)
  • Content.Server/Power/EntitySystems/BatterySystem.cs (3 hunks)
  • Content.Server/Projectiles/ProjectileSystem.cs (3 hunks)
  • Content.Server/Stunnable/Systems/StunbatonSystem.cs (5 hunks)
  • Content.Server/UserInterface/ActivatableUISystem.cs (3 hunks)
  • Content.Server/Weapons/Ranged/Systems/GunSystem.Ballistic.cs (3 hunks)
  • Content.Server/Weapons/Ranged/Systems/GunSystem.cs (2 hunks)
  • Content.Server/_White/Guns/PoweredComponent.cs (1 hunks)
  • Content.Server/_White/Guns/PoweredSystem.cs (1 hunks)
  • Content.Server/_White/Projectiles/PenetratedProjectileSystem.cs (1 hunks)
  • Content.Shared/Projectiles/EmbedEvent.cs (1 hunks)
  • Content.Shared/Projectiles/EmbeddableProjectileComponent.cs (2 hunks)
  • Content.Shared/Projectiles/SharedProjectileSystem.cs (3 hunks)
  • Content.Shared/Weapons/Ranged/Components/GunComponent.cs (1 hunks)
  • Content.Shared/Weapons/Ranged/Systems/SharedGunSystem.Ballistic.cs (3 hunks)
  • Content.Shared/_White/Guns/Stretched/StretchedComponent.cs (1 hunks)
  • Content.Shared/_White/Guns/Stretched/StretchedSystem.cs (1 hunks)
  • Content.Shared/_White/Penetrated/PenetratedComponent.cs (1 hunks)
  • Content.Shared/_White/Penetrated/PenetratedSystem.cs (1 hunks)
  • Content.Shared/_White/Projectile/PenetratedProjectileComponent.cs (1 hunks)
  • Resources/Locale/ru-RU/_white/prototypes/entities/objects/weapons/guns/crossbow.ftl (1 hunks)
  • Resources/Locale/ru-RU/_white/prototypes/entities/objects/weapons/melee/snatcherprod.ftl (1 hunks)
  • Resources/Locale/ru-RU/prototypes/entities/objects/weapons/melee/stunprod.ftl (1 hunks)
  • Resources/Prototypes/Entities/Mobs/base.yml (1 hunks)
  • Resources/Prototypes/Entities/Objects/Materials/parts.yml (3 hunks)
  • Resources/Prototypes/Entities/Objects/Misc/improvised_gun_parts.yml (1 hunks)
  • Resources/Prototypes/Entities/Objects/Specific/syndicate.yml (1 hunks)
  • Resources/Prototypes/_White/Entities/Objects/Weapons/Guns/crossbow.yml (1 hunks)
  • Resources/Prototypes/_White/Recipes/hidden_crafts.yml (1 hunks)
  • Resources/Prototypes/_White/tags.yml (1 hunks)
  • Resources/Textures/_White/Objects/Weapons/Guns/crossbow.rsi/meta.json (1 hunks)
🧰 Additional context used
🪛 yamllint
Resources/Prototypes/_White/Entities/Objects/Weapons/Guns/crossbow.yml

[warning] 29-29: wrong indentation: expected 6 but found 8

(indentation)


[error] 45-45: trailing spaces

(trailing-spaces)

Resources/Prototypes/_White/Recipes/hidden_crafts.yml

[warning] 89-89: wrong indentation: expected 8 but found 0

(indentation)


[warning] 95-95: wrong indentation: expected 4 but found 6

(indentation)


[warning] 97-97: wrong indentation: expected 8 but found 10

(indentation)


[warning] 103-103: wrong indentation: expected 4 but found 6

(indentation)


[warning] 105-105: wrong indentation: expected 8 but found 10

(indentation)


[error] 119-119: no new line character at the end of file

(new-line-at-end-of-file)

Resources/Prototypes/_White/tags.yml

[error] 8-8: no new line character at the end of file

(new-line-at-end-of-file)

🔇 Additional comments (60)
Resources/Locale/ru-RU/_white/prototypes/entities/objects/weapons/melee/snatcherprod.ftl (1)

1-2: LGTM: Localization quality is good.

The Russian translations for the Snatcherprod weapon name and description are well-crafted:

  1. "хваталка" is an appropriate and catchy name for the weapon.
  2. The description "Искрится жаждой воровства и коварства." (Sparks with a thirst for theft and cunning) is creative and fits the theme of the weapon.

The translations effectively convey the nature and purpose of the weapon to Russian-speaking players.

Resources/Prototypes/_White/tags.yml (3)

1-2: LGTM: Existing tag definition is correct.

The EnergySword tag definition remains unchanged and is consistent with the structure of other tags in the file.


4-5: LGTM: New CrossbowBolt tag added correctly.

The CrossbowBolt tag has been added with the correct structure, consistent with other tags in the file. This addition aligns with the PR objective of implementing a crossbow feature in the game.


7-8: LGTM: New Telecrystal tag added, but clarification needed.

The Telecrystal tag has been added with the correct structure, consistent with other tags in the file. However, the purpose of this tag in relation to the crossbow feature is not immediately clear from the PR objectives.

Could you please clarify the intended use of the Telecrystal tag in the context of the crossbow feature? This will help ensure that all additions are relevant to the PR's main objective.

🧰 Tools
🪛 yamllint

[error] 8-8: no new line character at the end of file

(new-line-at-end-of-file)

Resources/Locale/ru-RU/prototypes/entities/objects/weapons/melee/stunprod.ftl (2)

1-1: LGTM: Base entry for stunprod added.

The addition of ent-StunprodBase as a base entry for the stunprod is a good practice. It allows for better modularity and potential reuse in other related entities.


6-7: LGTM: Unfinished prod entry added. Verify crafting system integration.

The addition of ent-ProdUnfinished is good, as it suggests a crafting or construction system for the stunprod. The name and description accurately represent an incomplete state of the weapon.

To ensure this new entry is properly integrated with the crafting system, consider running the following script:

✅ Verification successful

LGTM: Unfinished prod entry references identified. Ensure integration in hidden_crafts.yml and stunprod.yml.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for references to the unfinished prod in crafting-related files

# Test: Search for "ProdUnfinished" in crafting-related files
rg --type yaml "ProdUnfinished" Resources/Prototypes/Recipes
rg --type yaml "ProdUnfinished" Resources/Prototypes/Entities/Objects/Misc/materials.yml

Length of output: 246


Script:

#!/bin/bash
# Description: Search for references to "ProdUnfinished" in all YAML files within Resources/Prototypes/

rg --type yaml "ProdUnfinished" Resources/Prototypes/

Length of output: 220

Content.Shared/_White/Penetrated/PenetratedComponent.cs (1)

3-4: LGTM! Component registration and class declaration are well-structured.

The [RegisterComponent] attribute and class declaration follow best practices for component-based architecture in game development. The use of sealed and partial keywords is appropriate for performance and potential extensibility.

Content.Server/_White/Guns/PoweredComponent.cs (2)

1-11: Consider expanding component functionality and documenting interactions.

The PoweredComponent aligns well with the PR objective of adding a crossbow to the game. It provides configurable properties for energy consumption and projectile speed modification, which is great for game balance and weapon variety.

However, consider the following improvements:

  1. Add XML documentation to the class explaining its purpose and how it's intended to be used.
  2. Consider adding methods to calculate energy consumption or modified projectile speed, which could be called by other systems.
  3. Document how this component is expected to interact with other components or systems (e.g., an energy system, projectile system, etc.).
  4. If applicable, add validation logic to ensure the field values are within acceptable ranges.

Example class documentation:

/// <summary>
/// Represents a powered weapon component, such as a crossbow, with customizable energy consumption and projectile speed modification.
/// </summary>
/// <remarks>
/// This component should be attached to entities representing powered weapons in the game.
/// It interacts with [list relevant systems or components here] to modify weapon behavior.
/// </remarks>

To better understand how this component fits into the larger system, run:

#!/bin/bash
# Search for other components or systems that might interact with PoweredComponent
rg "PoweredComponent" --type cs

6-7: Add XML documentation and consider the default value.

The EnergyPerUse field is well-named, but it lacks XML documentation to explain its purpose and unit (e.g., joules, watt-hours). Additionally, the default value of 180f seems arbitrary. Consider adding documentation and reviewing if this default value is appropriate for all powered weapons.

Example documentation:

/// <summary>
/// The amount of energy consumed per use of the powered weapon.
/// </summary>
/// <remarks>
/// Unit: [specify unit here, e.g., joules]
/// Default value: 180
/// </remarks>

To verify the usage of this field across the codebase, run:

Content.Client/_White/Guns/Stretched/StretchedVisualsComponent.cs (2)

3-5: LGTM: Class declaration and attributes are well-structured.

The class declaration for StretchedVisualsComponent is well-defined:

  • Appropriate use of [RegisterComponent] and [Access] attributes.
  • sealed keyword prevents unintended inheritance.
  • partial keyword allows for potential code generation or class splitting.
  • Inherits from Component, which is consistent with component-based architecture.

This structure provides a good foundation for the crossbow visuals component.


1-14: Overall assessment: Good foundation for crossbow visuals, with minor improvements needed.

The StretchedVisualsComponent provides a solid foundation for managing the visual states of the crossbow mentioned in the PR objectives. The use of nullable string properties with [DataField] attributes allows for flexible state management and serialization.

Key points:

  1. The component structure and attribute usage are appropriate.
  2. Property names are generally clear, with UnstrungVisible being an exception that needs clarification.
  3. Adding comments to explain the purpose of each state would enhance code readability.
  4. Consider revising the namespace to remove the underscore in _White.

These minor improvements will make the code more maintainable and align it better with C# best practices while fulfilling the PR objective of adding a crossbow to the game.

To ensure this component is properly integrated, let's check for its usage:

✅ Verification successful

Component integration verified: Proper usage in StretchedVisualizerSystem.

The StretchedVisualsComponent is correctly utilized within the StretchedVisualizerSystem, ensuring proper management of the crossbow's visual states. No issues were found regarding its integration.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for usage of StretchedVisualsComponent
rg --type csharp -i "StretchedVisualsComponent"

Length of output: 534

Content.Shared/_White/Projectile/PenetratedProjectileComponent.cs (1)

1-3: LGTM: Appropriate namespace and using statement.

The namespace and using statement are correctly structured and relevant to the component being defined.

Content.Shared/_White/Guns/Stretched/StretchedComponent.cs (2)

1-8: LGTM: Namespace and class declaration are well-structured.

The namespace hierarchy and class declaration follow best practices for component-based architecture in the game engine. The use of the [RegisterComponent] attribute ensures proper integration with the entity-component system.


15-15: Consider adding [ViewVariables] attribute and clarify initialization strategy.

For consistency with other properties and to aid in debugging, consider adding the [ViewVariables] attribute to the Provider property. Additionally, the use of default! suggests that this property will be initialized after construction. It would be helpful to clarify the initialization strategy in a comment:

+[ViewVariables]
+// Initialized in Start() method or via dependency injection
 public BallisticAmmoProviderComponent Provider = default!;

Could you please provide more information on how and when this property is expected to be initialized?

Resources/Textures/_White/Objects/Weapons/Guns/crossbow.rsi/meta.json (3)

5-8: LGTM! Size definition is appropriate.

The size definition of 32x32 pixels is clear and follows the expected format for sprite metadata. This is a common and appropriate size for game sprites.


1-31: LGTM! Well-structured and properly formatted JSON.

The overall structure and formatting of the JSON file adhere to best practices:

  • Valid JSON structure
  • Consistent indentation (2 spaces)
  • Correct use of commas (no trailing commas)
  • Clear and logical organization of information

This makes the file easy to read and maintain.


9-30: LGTM! Comprehensive state definitions. Verify completeness.

The states section is well-structured and covers various conditions of the crossbow, including different gameplay situations and holding positions. The inclusion of 4 directions for inhand states is particularly good for proper representation.

To ensure all necessary states are included, please verify:

  1. Are there any additional states needed for the crossbow's functionality?
  2. Are there any animation states missing (e.g., firing, reloading)?

Run the following script to check for any references to crossbow states in the codebase:

This will help identify if there are any states referenced in the code that are not defined in this metadata file.

Content.Client/_White/Guns/Stretched/StretchedVisualizerSystem.cs (3)

1-8: LGTM: Appropriate namespace and class declaration.

The namespace Content.Client._White.Guns.Stretched and the class name StretchedVisualizerSystem are well-chosen, clearly indicating the purpose and location of this system within the project structure. The inheritance from VisualizerSystem<StretchedVisualsComponent> is appropriate for handling the visualization of entities with a StretchedVisualsComponent.


11-12: LGTM: Correct method signature for OnAppearanceChange.

The OnAppearanceChange method correctly overrides the base class method with the appropriate parameters. The use of ref for the AppearanceChangeEvent parameter is a good practice for performance optimization.


1-21: Summary: Well-implemented visualizer system for crossbow states.

This new StretchedVisualizerSystem aligns well with the PR objectives of porting and refactoring the crossbow feature. It provides a solid foundation for managing the visual states of the crossbow (stretched, loaded, unstrung), which will enhance the gameplay experience as mentioned in the PR description.

The implementation is generally good, with proper use of dependency injection, null checks, and state management. The suggested improvements in the previous comments will further enhance its clarity and maintainability.

This system will play a crucial role in providing visual feedback to players about the state of their crossbow, contributing to the overall enhancement of the game's weaponry as intended by this PR.

Resources/Prototypes/Entities/Objects/Misc/improvised_gun_parts.yml (1)

Line range hint 1-9: Investigate and resolve the Item component issue.

There's a concerning comment about the game crashing when the Item size is uncommented. This issue should be investigated and resolved rather than leaving it as a mysterious problem.

To help investigate this issue, let's search for other occurrences of Item components with size attributes:

Please review the results and compare them with this file to identify any differences or patterns that might explain the issue.

Content.Server/Weapons/Ranged/Systems/GunSystem.Ballistic.cs (3)

1-1: LGTM: Import statement added correctly.

The new import statement for Content.Server.Stack is correctly placed and necessary for the StackSystem dependency.


11-11: LGTM: Dependency injection added correctly.

The StackSystem dependency is properly injected as a private readonly field with the correct attribute and initialization.


Line range hint 1-45: Summary: Changes improve stack handling in the gun system.

The modifications to this file enhance the GunSystem class by introducing better stack handling capabilities. This is achieved through the addition of a StackSystem dependency and a new GetStackEntity method. These changes appear to be part of the larger goal of porting and refactoring the crossbow feature, as mentioned in the PR objectives.

To ensure these changes align with the overall PR objectives:

  1. Verify that this improved stack handling is specifically beneficial for the crossbow implementation.
  2. Check if similar changes are needed in other related files for consistency.
  3. Consider adding a brief comment explaining the purpose of the GetStackEntity method in the context of the crossbow feature.

To confirm the relevance of these changes to the crossbow feature, you can run the following script:

This will help verify the connection between these changes and the crossbow implementation.

Content.Shared/Projectiles/EmbeddableProjectileComponent.cs (1)

51-54: LGTM! Consider documenting the Damage field.

The addition of the Damage field is appropriate for the EmbeddableProjectileComponent and aligns well with the PR objectives for porting and refactoring the crossbow feature. It allows for more specific damage definition for projectiles.

Consider adding an XML documentation comment to describe the purpose and usage of the Damage field, similar to other fields in this class. For example:

/// <summary>
/// Specifies the damage dealt when the projectile embeds into a target.
/// </summary>
[DataField]
public DamageSpecifier Damage = new();

To ensure this change is properly integrated, let's verify its usage:

Resources/Prototypes/_White/Entities/Objects/Weapons/Guns/crossbow.yml (3)

57-77: Verify construction graph for crossbow states.

The WeaponPoweredCrossbowUnfinished entity seems to be an intermediate state in the construction of the crossbow. Ensure that the WeaponPoweredCrossbowGraph correctly defines the progression from the unfinished to the finished state.

#!/bin/bash
# Description: Check the construction graph for crossbow states

# Test: Search for WeaponPoweredCrossbowGraph in configuration files
rg --type yaml 'WeaponPoweredCrossbowGraph' -C 10

22-25: Verify the throwAngle value for the crossbow.

The throwAngle is set to 225 degrees, which seems unusually high for a crossbow. This might affect the weapon's behavior in-game. Consider adjusting this value to a more realistic angle, typically between 0 and 45 degrees for most weapons.

#!/bin/bash
# Description: Check for other weapons' throwAngle values for comparison

# Test: Search for throwAngle in weapon configurations
rg --type yaml 'throwAngle:' -C 5

1-77: Verify complete game integration for the crossbow.

The crossbow entities seem well-defined, but it's crucial to ensure all necessary components for proper game integration are present. Please verify:

  1. The crossbow is properly balanced in terms of damage, fire rate, and ammo capacity.
  2. Appropriate sound effects are defined for all actions (firing, reloading, etc.).
  3. The crossbow interacts correctly with other game systems (inventory, crafting, etc.).
  4. Localization strings are defined for the crossbow's name and description.
🧰 Tools
🪛 yamllint

[warning] 29-29: wrong indentation: expected 6 but found 8

(indentation)


[error] 45-45: trailing spaces

(trailing-spaces)

Resources/Prototypes/Entities/Objects/Specific/syndicate.yml (2)

Line range hint 1-116: Overall, the changes improve entity identification and game mechanics.

The modifications to this file enhance the tagging system for syndicate-related entities, which should improve filtering and identification in the game logic. The changes are generally well-implemented and align with the apparent goals of the PR.

Key points:

  1. Telecrystal entities now have a specific tag for easier identification.
  2. BaseUplinkRadio now includes a GiftIgnore component, potentially affecting gift mechanics.
  3. The NukeOps-specific uplink is now properly tagged for filtering.

These changes seem to support the PR's objective of porting and refactoring the crossbow feature, although the direct connection to the crossbow isn't evident in this file. Ensure that these changes are properly documented and that they align with the overall design goals of the project.


Line range hint 82-82: LGTM! Please clarify the purpose of GiftIgnore.

The addition of the GiftIgnore component to the BaseUplinkRadio entity seems appropriate, as syndicate uplinks are not typically items that would be gifted in-game.

Could you provide more information about the GiftIgnore component and its specific purpose in the game mechanics? This would help ensure that the change aligns with the intended game design.

Also, consider adding a comment explaining the reason for this addition to improve code documentation.

To verify the usage of GiftIgnore, let's run the following script:

✅ Verification successful

Verification Successful: GiftIgnore Component Usage Confirmed

The GiftIgnore component is implemented in GiftIgnoreComponent.cs and is consistently used across multiple entities, as shown in your script output. This confirms that the addition to syndicate.yml aligns with existing usage patterns.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for other occurrences of GiftIgnore and its implementation

# Search for other uses of GiftIgnore
echo "Other entities using GiftIgnore:"
rg --type yaml "type: GiftIgnore" Resources/Prototypes/Entities

# Search for the implementation of GiftIgnore
echo "\nGiftIgnore implementation:"
rg --type cs "class GiftIgnore"

Length of output: 1684

Resources/Prototypes/Entities/Objects/Materials/parts.yml (4)

102-102: LGTM! Consistent tagging for metal rod stacks.

The addition of the CrossbowBolt tag to PartRodMetal10 is consistent with the changes made to the base PartRodMetal entity. This allows stacks of 10 metal rods to be recognized as potential crossbow ammunition, maintaining functionality across different stack sizes.


Line range hint 80-117: Summary: Crossbow functionality successfully implemented for metal rods

The changes in this file consistently implement crossbow bolt functionality for metal rods across different entities (PartRodMetal, PartRodMetal10, and PartRodMetal1). This aligns well with the PR objective of adding a crossbow to the game.

Key points:

  1. Metal rods can now be used as crossbow bolts.
  2. Damage and speed properties have been defined for the projectiles.
  3. The implementation is consistent across different stack sizes.

These changes should significantly enhance the gameplay by introducing a new weapon type and repurposing existing items as ammunition. Great job on maintaining consistency throughout the implementation!


117-117: LGTM! Consistent tagging for single metal rods.

The addition of the CrossbowBolt tag to PartRodMetal1 completes the set of changes, ensuring that single metal rods can also be used as crossbow ammunition. This maintains consistency across all metal rod entities.

To ensure proper functionality, please verify that the crossbow behaves correctly with different stack sizes of metal rods. Consider running the following script to check for any stack-related components or systems that might interact with the crossbow:

#!/bin/bash
# Description: Find stack-related components and systems that might interact with the crossbow

echo "Stack-related components and systems:"
rg --type yaml 'type: Stack|Stack[A-Z]' -A 5

echo "\nCrossbow-related systems and components:"
rg --type yaml 'Crossbow' -A 5

80-92: LGTM! Crossbow functionality added to metal rods.

The changes align well with the PR objective of adding a crossbow to the game. The new components (Tag, EmbeddableProjectile, and PenetratedProjectile) effectively transform metal rods into potential crossbow bolts.

Please verify that the damage values (15 for both Piercing damage types) and minimum speeds (15 for embedding, 30 for penetration) are balanced within the game's combat system. Consider running the following script to check for other projectile damage values for comparison:

Resources/Prototypes/_White/Recipes/hidden_crafts.yml (4)

87-87: LGTM: Minor formatting improvement.

The indentation correction for the DeleteEntity action improves the YAML structure consistency.


89-94: LGTM: Well-structured construction graph for the powered crossbow.

The graph structure is clear and logical, with three distinct nodes (stock, unfinished, and crossbow) representing the crafting stages. The entity definitions for the unfinished and completed crossbow are appropriately placed.

Also applies to: 100-102, 118-119

🧰 Tools
🪛 yamllint

[warning] 89-89: wrong indentation: expected 8 but found 0

(indentation)


95-99: Verify material requirements and crafting steps.

The crafting process seems well-defined, but please confirm:

  1. Are 3 metal rods sufficient for the stock?
  2. Is the total of 10 cables (5 in two steps) intentional?
  3. Are the doAfter times (3s, 5s, 0.5s, etc.) balanced with other weapons?

Also, consider adding comments to explain the purpose of each material and step for better maintainability.

Also applies to: 103-117

🧰 Tools
🪛 yamllint

[warning] 95-95: wrong indentation: expected 4 but found 6

(indentation)


[warning] 97-97: wrong indentation: expected 8 but found 10

(indentation)


Line range hint 1-119: Summary: Crossbow implementation aligns with PR objectives.

The addition of the WeaponPoweredCrossbowGraph successfully implements the crafting process for the new crossbow weapon, aligning with the PR objectives. The construction graph is well-structured and provides a clear progression from stock to finished crossbow.

A few minor improvements have been suggested:

  1. Verifying material requirements and crafting step durations.
  2. Fixing indentation issues for consistency with the rest of the file.
  3. Adding a newline character at the end of the file.

Once these suggestions are addressed, the implementation will be robust and consistent with the existing codebase structure.

🧰 Tools
🪛 yamllint

[warning] 89-89: wrong indentation: expected 8 but found 0

(indentation)


[warning] 95-95: wrong indentation: expected 4 but found 6

(indentation)


[warning] 97-97: wrong indentation: expected 8 but found 10

(indentation)


[warning] 103-103: wrong indentation: expected 4 but found 6

(indentation)


[warning] 105-105: wrong indentation: expected 8 but found 10

(indentation)


[error] 119-119: no new line character at the end of file

(new-line-at-end-of-file)

Content.Server/Stunnable/Systems/StunbatonSystem.cs (3)

41-43: Improved battery management in OnStaminaHitAttempt

The refactoring to use _battery.TryGetBatteryComponent instead of a local method is a good improvement. It centralizes battery management logic and makes the code more consistent with the rest of the system. The checks for battery presence and charge usage are correctly implemented.


Line range hint 119-122: Good addition of OnPowerCellChanged method

The new OnPowerCellChanged method is a valuable addition. It ensures that the baton's charge state is immediately checked and updated when the power cell is changed. This improves the overall responsiveness and accuracy of the system.


Line range hint 1-132: Overall improvement in battery management and code consistency

The changes in this file represent a significant improvement in the StunbatonSystem class:

  1. Consistent use of BatterySystem: All battery-related operations now use _battery.TryGetBatteryComponent, improving code consistency and centralizing battery management.
  2. Removal of redundant code: The local TryGetBatteryComponent method has been removed, reducing code duplication.
  3. Improved responsiveness: The new OnPowerCellChanged method ensures the baton's state is updated immediately when the power cell changes.
  4. Maintainability: The code is now more maintainable due to the consistent use of the BatterySystem.

These changes align well with the system's architecture and improve the overall quality of the code. The minor suggestions for further consistency in retrieving the battery entity are worth considering, but they don't detract from the overall positive impact of these changes.

Content.Server/UserInterface/ActivatableUISystem.cs (1)

28-28: LGTM. Verify the event processing order.

The modification to process the ActivateInWorldEvent after the ProjectileSystem looks good. This change could be related to the crossbow feature mentioned in the PR objectives.

Please confirm that this change achieves the intended behavior, especially in scenarios involving projectiles and UI activation.

Content.Server/Power/EntitySystems/BatterySystem.cs (1)

120-120: LGTM! Good performance optimization.

The addition of the ref keyword for the ChargerUpdateStatusEvent is a good performance optimization. It avoids unnecessary copying of the event struct when raising the local event.

Content.Server/_White/Guns/PoweredSystem.cs (4)

10-11: Dependencies are correctly injected.

The BatterySystem and GunSystem are properly injected using the [Dependency] attribute with default initializers.


13-17: Event subscriptions are appropriately set up in Initialize().

The system subscribes to the necessary events for handling shooting attempts and refreshing gun modifiers associated with the PoweredComponent.


19-22: Efficient handling in OnShoot method.

Upon an attempt to shoot, refreshing the gun modifiers ensures that any changes to the powered components are accurately reflected.


24-31: Proper battery usage and projectile speed modification in OnGunRefreshModifiers().

The method correctly checks for battery availability and consumes energy before modifying the projectile speed based on the powered component's properties.

Content.Shared/_White/Penetrated/PenetratedSystem.cs (1)

18-22: Verify if 'BodyType.KinematicController' is appropriate when freeing the entity

Ensure that setting the body type to BodyType.KinematicController is intended when freeing a penetrated entity. If the entity is expected to interact with physics normally, consider using BodyType.Dynamic instead.

Run the following script to check usages of _physics.SetBodyType in similar contexts:

Content.Shared/_White/Guns/Stretched/StretchedSystem.cs (1)

52-52: Ensure SoundDraw Is Assigned Before Playing Sound

When calling _audio.PlayPredicted(component.SoundDraw, uid, args.User);, it's important to ensure that component.SoundDraw is not null or unassigned to prevent runtime errors.

Run the following script to verify that all instances of StretchedComponent have SoundDraw properly assigned:

Content.Server/_White/Projectiles/PenetratedProjectileSystem.cs (3)

48-48: Assess the impact of setting BodyType.Static on the embedded entity

Setting the embedded entity's body type to Static may prevent it from interacting physically in the game world. Ensure that this change aligns with the intended game mechanics and won't cause issues if the entity needs to move or be interacted with later.

Consider scenarios where the embedded entity might need to regain mobility and whether additional code is required to reset its BodyType.


51-52: Confirm correct parent and position assignment in transformations

The transformation methods _transform.SetLocalPosition and _transform.SetParent are used to position the embedded entity relative to the projectile. Verify that the parent-child relationship and position settings are correct to prevent any unexpected behavior in the entity hierarchy.

Ensure that args.Embedded should have uid as its parent and that the local positions are calculated as intended.


45-45: Verify the parameters in _buckle.TryUnbuckle method call

In the OnEmbed method, _buckle.TryUnbuckle(args.Embedded, args.Embedded, true); uses the same entity for both parameters. Confirm that this is intentional and that the method accepts the same entity for unbuckling.

To verify the correct usage of TryUnbuckle, review its definition and typical invocation:

This script searches for TryUnbuckle method calls to compare parameter usage in the codebase.

Content.Shared/Projectiles/SharedProjectileSystem.cs (1)

60-60: Verify the necessity of changing the Embed method to public

The Embed method's access modifier has been changed from private to public. Please ensure that exposing this method is necessary and does not inadvertently allow unintended access or misuse from other parts of the codebase.

Content.Server/Projectiles/ProjectileSystem.cs (2)

100-105: Correct Application of Damage in OnEmbed Method

The OnEmbed method correctly applies damage to the embedded entity using _damageable.TryChangeDamage and raises a red color effect when damage is dealt. This ensures that the embedded projectile interacts appropriately with the target entity.


109-111: Proper Handling in OnEmbedActivate Method

The OnEmbedActivate method checks if the event has already been handled or if AttemptEmbedRemove fails before proceeding. This prevents redundant processing and ensures that the removal attempt is only initiated when appropriate.

Content.Shared/Weapons/Ranged/Systems/SharedGunSystem.Ballistic.cs (3)

5-5: Approved: Added necessary import for StackComponent

The addition of using Content.Shared.Stacks; is appropriate for the use of StackComponent in the code.


51-63: Approved: Correct handling of stackable ammunition in insertion logic

The updated logic in the OnBallisticInteractUsing method properly handles stackable ammunition by checking for the StackComponent and adjusting the insertion behavior accordingly. This ensures that when dealing with stacks, only the appropriate entity is inserted into the container.


296-301: Approved: Updated appearance data reflects ammunition count accurately

The changes in the UpdateBallisticAppearance method correctly update the visual state of the ballistic weapon based on the current ammunition count. Setting AmmoVisuals.HasAmmo to count != 0 ensures the appearance accurately represents whether the weapon has ammo.

Content.Server/Weapons/Ranged/Systems/GunSystem.cs (2)

15-15: Addition of required namespace for throwing mechanics

The inclusion of using Content.Shared.Throwing; is appropriate to access the throwing components and systems needed for the new functionality.


297-302: ⚠️ Potential issue

Ensure safe removal of ThrowingAngleComponent after throw

You've added a ThrowingAngleComponent to the entity before throwing and remove it immediately after calling ThrowingSystem.TryThrow. If TryThrow or any asynchronous processes rely on ThrowingAngleComponent beyond the scope of the method call, removing it immediately could cause unintended behavior. Verify that removing ThrowingAngleComponent right after TryThrow does not interfere with the throwing mechanics.

Run the following script to check if ThrowingAngleComponent is used after TryThrow:

#!/bin/bash
# Description: Verify that `ThrowingAngleComponent` is not required after `TryThrow` completes.

# Search for usages of `ThrowingAngleComponent` in `ThrowingSystem` methods called within `TryThrow`.
rg 'ThrowingAngleComponent' -A 5 -B 5

# Expected result: No code should depend on `ThrowingAngleComponent` after `TryThrow` returns.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Outside diff range and nitpick comments (1)
Resources/Prototypes/Entities/Objects/Materials/parts.yml (1)

119-119: LGTM! Consider refactoring to reduce duplication.

The addition of the "CrossbowBolt" tag to PartRodMetal1 is consistent with the changes made to the parent PartRodMetal entity and PartRodMetal10. This ensures that single metal rods can also be used as crossbow ammunition, maintaining consistency in the game mechanics.

To reduce duplication and improve maintainability, consider refactoring the "CrossbowBolt" tag addition. You could add it to the parent PartRodMetal entity and remove it from the child entities, as they should inherit this tag. This would centralize the change and make future modifications easier. Here's a suggested refactor:

  1. In the PartRodMetal entity, modify the Tag component:
  - type: Tag
    tags:
    - CrossbowBolt
    - RodMetal1
  1. Remove the "CrossbowBolt" tag from PartRodMetal10 and PartRodMetal1.

This refactor would ensure all metal rod entities have the CrossbowBolt tag while reducing code duplication.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 7cefc75 and 0717f11.

📒 Files selected for processing (5)
  • Content.Server/Projectiles/ProjectileSystem.cs (3 hunks)
  • Content.Server/Weapons/Ranged/Systems/GunSystem.Ballistic.cs (3 hunks)
  • Content.Shared/Projectiles/SharedProjectileSystem.cs (3 hunks)
  • Content.Shared/Weapons/Ranged/Systems/SharedGunSystem.Ballistic.cs (3 hunks)
  • Resources/Prototypes/Entities/Objects/Materials/parts.yml (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • Content.Server/Projectiles/ProjectileSystem.cs
  • Content.Server/Weapons/Ranged/Systems/GunSystem.Ballistic.cs
  • Content.Shared/Weapons/Ranged/Systems/SharedGunSystem.Ballistic.cs
🔇 Additional comments (3)
Resources/Prototypes/Entities/Objects/Materials/parts.yml (2)

104-104: LGTM! Consistent with parent entity changes.

The addition of the "CrossbowBolt" tag to PartRodMetal10 is consistent with the changes made to the parent PartRodMetal entity. This ensures that stacks of 10 metal rods can also be used as crossbow ammunition, maintaining consistency in the game mechanics.


80-94: LGTM! Verify combat values with game design team.

The addition of Tag, EmbeddableProjectile, and PenetratedProjectile components to the PartRodMetal entity aligns well with the PR objective of implementing crossbow functionality. These changes effectively turn metal rods into potential crossbow ammunition.

Please confirm with the game design team that the following values are appropriate for crossbow bolts:

  • Minimum speed for embedding: 15
  • Minimum speed for penetration: 30
  • Piercing damage: 15 for both embedding and penetration

To assist in verifying these values across the codebase, you can run the following script:

Content.Shared/Projectiles/SharedProjectileSystem.cs (1)

60-93: Confirm the necessity of changing Embed method visibility

The Embed method has been changed from private to public, and a new parameter bool raiseEvent has been added. Exposing this method publicly could have implications for how it is used elsewhere in the codebase. Please ensure that making it public is necessary and that it doesn't introduce unintended side effects or security concerns.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (5)
Resources/Prototypes/Entities/Objects/Materials/parts.yml (3)

104-104: LGTM! Consistent tagging for crossbow ammunition.

The addition of the "CrossbowBolt" tag to PartRodMetal10 is consistent with the changes made to the parent entity and supports the new crossbow functionality.

For improved readability, consider grouping related tags together. You could move the "CrossbowBolt" tag next to "RodMetal1":

  - type: Tag
    tags:
    - RodMetal1
    - CrossbowBolt

This minor change would make it easier to identify all crossbow-related tags at a glance.


119-119: LGTM! Consistent tagging for single crossbow bolts.

The addition of the "CrossbowBolt" tag to PartRodMetal1 is consistent with the changes made to the parent entity and PartRodMetal10, supporting the new crossbow functionality for single metal rods.

As suggested for PartRodMetal10, consider grouping related tags together for improved readability:

  - type: Tag
    tags:
    - RodMetal1
    - CrossbowBolt

This minor change would maintain consistency across all metal rod entities and improve code readability.


Line range hint 80-119: Overall implementation of crossbow functionality looks good.

The changes made to PartRodMetal, PartRodMetal10, and PartRodMetal1 entities consistently implement the crossbow functionality as intended in the PR objectives. The new components and tags added to these entities allow metal rods to serve as crossbow ammunition with appropriate combat mechanics.

A few minor suggestions for code organization have been made to improve readability and maintain consistency. These changes are not critical but would enhance the overall code quality.

Consider the following architectural advice for future improvements:

  1. If crossbow bolts become more complex in the future, you might want to create a separate CrossbowBolt component that encapsulates all bolt-specific behavior. This would allow for easier management and extension of crossbow ammunition types.

  2. As the game evolves, you may want to create a configuration file for weapon balance, including damage values and speed thresholds for various projectile types. This would make it easier to fine-tune game balance without modifying entity definitions.

  3. If you plan to add more varied types of crossbow ammunition in the future, consider creating a CrossbowAmmunition abstract entity that other ammunition types can inherit from. This would promote code reuse and consistency across different types of crossbow bolts.

Content.Shared/Projectiles/SharedProjectileSystem.cs (2)

62-93: LGTM: Enhanced embedding logic with improved checks and penetration handling.

The changes to the Embed method body significantly improve the embedding logic by:

  1. Adding checks for physics component and minimum speed.
  2. Introducing logic for handling penetrated projectiles.
  3. Raising an AttemptEmbedEvent for potential interception.

These enhancements align well with the PR objectives. However, to improve maintainability:

Consider adding comments to explain the purpose of the new checks and the penetrated projectile logic, especially around lines 73-81.


Server-side event subscriptions still present.

Event handlers OnEmbedActivate and OnEmbedRemove are still referenced in Content.Server/Projectiles/ProjectileSystem.cs. Please ensure these subscriptions are appropriately removed or handled to complete the refactoring.

  • OnEmbedActivate in ProjectileSystem.cs
  • OnEmbedRemove in ProjectileSystem.cs
🔗 Analysis chain

Line range hint 1-166: Verify the impact of removed event subscriptions.

The AI summary mentions that several local event subscriptions related to activating and removing embedded projectiles have been removed. While this aligns with the refactoring objectives:

Please confirm that the functionality previously handled by these removed subscriptions is either no longer needed or has been implemented elsewhere in the codebase.


Further verification needed for event subscriptions.

Additional references to OnEmbedActivate and OnEmbedRemove were found in the server-side ProjectileSystem.cs. Please verify whether these event subscriptions are appropriately handled or if they require removal to maintain consistency with the refactoring.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for any remaining references to the removed event handlers
rg --type csharp "OnEmbedActivate|OnEmbedRemove" .

Length of output: 737


Script:

# 
#!/bin/bash
# Search for any remaining references to the event handlers in the shared project
rg --type csharp "OnEmbedActivate|OnEmbedRemove" ./Content.Shared

Length of output: 67

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 7cefc75 and 0717f11.

📒 Files selected for processing (5)
  • Content.Server/Projectiles/ProjectileSystem.cs (3 hunks)
  • Content.Server/Weapons/Ranged/Systems/GunSystem.Ballistic.cs (3 hunks)
  • Content.Shared/Projectiles/SharedProjectileSystem.cs (3 hunks)
  • Content.Shared/Weapons/Ranged/Systems/SharedGunSystem.Ballistic.cs (3 hunks)
  • Resources/Prototypes/Entities/Objects/Materials/parts.yml (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • Content.Server/Projectiles/ProjectileSystem.cs
  • Content.Server/Weapons/Ranged/Systems/GunSystem.Ballistic.cs
🔇 Additional comments (6)
Resources/Prototypes/Entities/Objects/Materials/parts.yml (1)

80-94: LGTM! New components enhance crossbow functionality.

The addition of Tag, EmbeddableProjectile, and PenetratedProjectile components to the PartRodMetal entity aligns well with the PR objective of implementing crossbow functionality. These changes allow metal rods to serve as crossbow bolts with appropriate combat mechanics.

Consider running the following script to verify the uniqueness of the CrossbowBolt tag:

This will help ensure that the CrossbowBolt tag is not unintentionally applied to other entities, which could affect game balance.

✅ Verification successful

Verification Successful!
The CrossbowBolt tag is appropriately used in both parts.yml and crossbow.yml, ensuring consistent identification of crossbow-related entities without unintended overlaps.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for other entities with the CrossbowBolt tag

# Test: Search for other entities with the CrossbowBolt tag
rg --type yaml 'tags:(\n|\s)*- CrossbowBolt' --glob '!Resources/Prototypes/Entities/Objects/Materials/parts.yml'

Length of output: 306


Script:

#!/bin/bash
# Description: Check for other entities with the CrossbowBolt tag

# Test: Search for other entities with the CrossbowBolt tag
rg --type yaml --multiline 'tags:(\n|\s)*- CrossbowBolt' --glob '!Resources/Prototypes/Entities/Objects/Materials/parts.yml'

Length of output: 303

Content.Shared/Projectiles/SharedProjectileSystem.cs (3)

2-3: LGTM: New dependencies for penetrated projectiles.

The addition of these dependencies aligns with the new functionality for handling penetrated projectiles in the Embed method.


60-60: LGTM: Improved flexibility of the Embed method.

The method is now public, allowing for more flexible usage. The new raiseEvent parameter provides control over event raising, which can be beneficial for testing or specific scenarios.


Line range hint 1-166: Overall, the changes look good and align well with the PR objectives.

The modifications to the SharedProjectileSystem class, particularly the enhancements to the Embed method and the introduction of penetrated projectile handling, significantly improve the crossbow feature. The code is generally well-structured and the new functionality seems to be implemented thoughtfully.

A few minor suggestions have been made:

  1. Adding comments to explain the new logic in the Embed method.
  2. Verifying the impact of removed event subscriptions.

Once these small items are addressed, the changes will be ready for merging.

Content.Shared/Weapons/Ranged/Systems/SharedGunSystem.Ballistic.cs (2)

5-5: Addition of StackComponent namespace

Including using Content.Shared.Stacks; allows the use of StackComponent in the code, which is necessary for handling stackable items.


296-301: Efficient use of count variable for appearance updates

Storing the result of GetBallisticShots(component) in the count variable and reusing it improves performance by avoiding multiple method calls. This enhances the efficiency of updating the ballistic appearance.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 3

🧹 Outside diff range and nitpick comments (2)
Content.Shared/Projectiles/SharedProjectileSystem.cs (2)

60-93: Approve changes to Embed method with suggestions for documentation.

The modifications to the Embed method significantly enhance its functionality and flexibility. The new raiseEvent parameter and the additional checks for physics and penetration are valuable improvements.

Consider adding XML documentation to explain:

  1. The purpose of the raiseEvent parameter.
  2. The new behavior regarding penetrated projectiles.
  3. The minimum speed requirement for embedding.

This documentation will help other developers understand the method's behavior and use it correctly.

Here's a suggested documentation template:

/// <summary>
/// Embeds a projectile into a target entity.
/// </summary>
/// <param name="uid">The unique identifier of the projectile entity.</param>
/// <param name="target">The target entity to embed the projectile into.</param>
/// <param name="user">The user entity that initiated the embedding, if any.</param>
/// <param name="component">The EmbeddableProjectileComponent of the projectile.</param>
/// <param name="raiseEvent">Whether to raise the EmbedEvent after embedding. Defaults to true.</param>
/// <remarks>
/// The method checks for minimum speed and handles penetrated projectiles differently.
/// If the projectile has penetrated the target or another entity, it may not become static.
/// </remarks>
public void Embed(EntityUid uid, EntityUid target, EntityUid? user, EmbeddableProjectileComponent component, bool raiseEvent = true)

Broader Review Recommended for Embed Method Changes

The modification of the Embed method to public in SharedProjectileSystem.cs has widespread usage across multiple components and systems, as identified in the recent codebase search. This change could have significant implications for game mechanics, security, and system encapsulation.

Key Points to Consider:

  • Widespread Usage: The Embed method is invoked in various parts of the codebase, including shared and server-side systems.
  • Potential Implications: Making the method public may expose it to unintended interactions, which could affect game balance and introduce security vulnerabilities.
  • Consistency with Design: Ensure that the public exposure of Embed aligns with the overall game design and does not disrupt existing behaviors.

A comprehensive review is essential to verify that these changes integrate seamlessly with the rest of the system and maintain the integrity of the game's mechanics.

🔗 Analysis chain

Line range hint 1-160: Suggest a broader review of the projectile system integration.

The changes introduced in this file significantly enhance the projectile system's capabilities, particularly in handling penetrated projectiles and the embedding process. However, these modifications may have broader implications for the game mechanics and other parts of the codebase.

Consider the following points:

  1. The Embed method is now public. Verify if this change is intentional and review any potential security or encapsulation concerns.
  2. The new functionality for penetrated projectiles should be reviewed for consistency with the game's design and balance.
  3. Consider updating or creating documentation for the projectile system to reflect these new features and behaviors.
  4. It may be beneficial to conduct a broader review of how these changes integrate with other systems in the game, such as combat mechanics or physics simulations.

To assist with this review, you may want to run the following script to identify other parts of the codebase that might be affected by these changes:

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for usages of the Embed method and EmbeddableProjectileComponent
rg --type csharp -e "Embed\s*\(" -e "EmbeddableProjectileComponent"

# Search for other references to penetrated projectiles
rg --type csharp -e "PenetratedProjectileComponent" -e "PenetratedComponent"

Length of output: 8072

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 0717f11 and f434a2c.

📒 Files selected for processing (5)
  • Content.Client/_White/Guns/Stretched/StretchedVisualizerSystem.cs (1 hunks)
  • Content.Client/_White/Guns/Stretched/StretchedVisualsComponent.cs (1 hunks)
  • Content.Server/Projectiles/ProjectileSystem.cs (3 hunks)
  • Content.Server/_White/Guns/PoweredComponent.cs (1 hunks)
  • Content.Shared/Projectiles/SharedProjectileSystem.cs (3 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • Content.Client/_White/Guns/Stretched/StretchedVisualizerSystem.cs
  • Content.Client/_White/Guns/Stretched/StretchedVisualsComponent.cs
  • Content.Server/_White/Guns/PoweredComponent.cs
🔇 Additional comments (5)
Content.Shared/Projectiles/SharedProjectileSystem.cs (3)

2-3: Clarify the purpose of new dependencies and naming convention.

New dependencies have been added:

using Content.Shared._White.Penetrated;
using Content.Shared._White.Projectile;

Could you please explain the purpose of these new dependencies and the reasoning behind the _White naming convention? This information would help in understanding the context of the changes and ensure consistency with the project's architecture.


151-160: ⚠️ Potential issue

Fix the Clone method in RemoveEmbeddedProjectileEvent.

The Clone method in the RemoveEmbeddedProjectileEvent class is incorrectly implemented. It currently returns this, which does not create a new instance as expected from a clone operation.

This issue was previously identified in a past review. Please apply the suggested fix to ensure proper cloning behavior:

 public override DoAfterEvent Clone()
 {
-    return this;
+    return new RemoveEmbeddedProjectileEvent();
 }

151-160: ⚠️ Potential issue

Remove duplicate definition of RemoveEmbeddedProjectileEvent class.

The RemoveEmbeddedProjectileEvent class is defined twice in this file. This duplicate definition can cause compilation errors and unexpected behavior.

This issue was previously identified in a past review. Please remove the duplicate definition as suggested:

- [Serializable, NetSerializable]
- public sealed partial class RemoveEmbeddedProjectileEvent : DoAfterEvent
- {
-     public override DoAfterEvent Clone()
-     {
-         return this;
-     }
- }
Content.Server/Projectiles/ProjectileSystem.cs (2)

180-184: Conditional resolution of PenetratedProjectileComponent still requires attention

The past review comment regarding the conditional resolution of penetratedProjectile in the FreePenetrated method is still applicable. Ensure that you check if penetratedProjectile is null before resolving it to prevent overwriting any provided component and to handle the absence of the component gracefully.


134-138: Ensure FreePenetrated handles absence of PhysicsComponent correctly

In OnEmbedRemove, if the PhysicsComponent is not present on the entity, FreePenetrated(uid); is called, and the method returns. Verify that FreePenetrated can handle cases where the projectile lacks penetration data and that the projectile's state is appropriately managed when the PhysicsComponent is missing.

To confirm that FreePenetrated handles this scenario gracefully, you can run the following script:

✅ Verification successful

FreePenetrated correctly handles the absence of PhysicsComponent

The implementation of FreePenetrated utilizes Resolve to obtain the PenetratedProjectileComponent and checks penetratedProjectile.PenetratedUid.HasValue. If the component is missing or PenetratedUid is not set, the method returns early. This ensures that the projectile's state is appropriately managed when the PhysicsComponent is absent.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify that FreePenetrated handles missing PenetratedProjectileComponent without errors.

# Test: Search for FreePenetrated method implementations and usages to ensure it checks for null components.
# Expect: The method should safely handle null PenetratedProjectileComponent.

# Check the implementation of FreePenetrated
rg --type cs "void FreePenetrated\(EntityUid uid, PenetratedProjectileComponent\? penetratedProjectile = null\)" -A 10

# Verify that PenetratedProjectileComponent is checked within the method
rg --type cs "if.*penetratedProjectile == null" -A 5

Length of output: 997


Script:

#!/bin/bash
# Description: Further verify that FreePenetrated handles missing PenetratedProjectileComponent correctly.

# Search for any conditional checks or usage of the Resolve method within FreePenetrated
rg --type cs "void FreePenetrated\(EntityUid uid, PenetratedProjectileComponent\? penetratedProjectile = null\)" -A 20

# Search for usage of 'Resolve' within FreePenetrated to ensure it properly handles null components
rg --type cs "Resolve\(uid, ref penetratedProjectile\)" -A 5

# Search for any null checks or alternative handling of penetratedProjectile within FreePenetrated
rg --type cs "penetratedProjectile\.(?!PenetratedUid)" -A 5

Length of output: 1832

Content.Server/Projectiles/ProjectileSystem.cs Outdated Show resolved Hide resolved
Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Outside diff range and nitpick comments (2)
Resources/Prototypes/_White/Entities/Objects/Weapons/Guns/crossbow.yml (2)

39-54: Consider adding documentation for power cell usage and construction

The crossbow's power cell integration and construction options add complexity to the weapon. To improve player understanding and maintainability:

  1. Consider adding comments or documentation explaining how the power cell affects the crossbow's functionality.
  2. It might be helpful to include a brief description of the construction process or link to relevant documentation.

Would you like assistance in drafting this documentation?

🧰 Tools
🪛 yamllint

[error] 45-45: trailing spaces

(trailing-spaces)


57-77: Add clarification for the unfinished crossbow entity

The WeaponPoweredCrossbowUnfinished entity seems to be an intermediate stage in the crossbow's construction. To improve clarity:

  1. Consider adding a comment explaining the purpose of this unfinished version and how it relates to the crafting or upgrade system.
  2. You might want to include information on how players can progress from the unfinished to the finished version.

Would you like assistance in drafting this clarification?

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 8c68cea and 49bcc60.

📒 Files selected for processing (1)
  • Resources/Prototypes/_White/Entities/Objects/Weapons/Guns/crossbow.yml (1 hunks)
🧰 Additional context used
🪛 yamllint
Resources/Prototypes/_White/Entities/Objects/Weapons/Guns/crossbow.yml

[warning] 29-29: wrong indentation: expected 6 but found 8

(indentation)


[error] 45-45: trailing spaces

(trailing-spaces)

Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Copy link
Contributor

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@Remuchi Remuchi merged commit 266ae60 into master Oct 26, 2024
12 checks passed
@Remuchi Remuchi deleted the сrossbow branch October 26, 2024 09:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants