Skip to content

Feat: Function cubes#289

Merged
AngeloTadeucci merged 6 commits intomasterfrom
interact-cubes
Oct 29, 2024
Merged

Feat: Function cubes#289
AngeloTadeucci merged 6 commits intomasterfrom
interact-cubes

Conversation

@AngeloTadeucci
Copy link
Collaborator

@AngeloTadeucci AngeloTadeucci commented Oct 28, 2024

Summary by CodeRabbit

Release Notes

  • New Features

    • Introduced a DailyResetCommand for players to trigger daily resets.
    • Added a LogoutCommand for logging out from the game.
    • Enhanced cube management with new checks for Farming and Ranching categories in the housing system.
    • Added FieldFunctionInteract class to manage interactions with plot cubes.
    • Implemented a new ClearInventoryCommand for managing player inventory.
    • New method in InventoryManager to clear all items from a specified inventory tab.
  • Improvements

    • Enhanced error handling and state management for function cubes and interactions.
    • Updated gathering mechanics to improve resource collection processes.
    • Improved the clarity of inventory management with a new method to clear inventory tabs.
  • Bug Fixes

    • Refined logic for handling interactions and state updates of function cubes.

These changes aim to improve user experience and gameplay dynamics.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 28, 2024

Walkthrough

The pull request introduces several modifications across multiple files, primarily enhancing the functionality related to game interactions, particularly with function cubes and nurturing mechanics. Key changes include the addition of new properties and methods for managing interactions, updates to SQL commands for resetting data, and the introduction of new commands for player actions. These alterations aim to improve the handling of game elements and player commands, ensuring better state management and interaction capabilities.

Changes

File Path Change Summary
Maple2.Database/Storage/Game/GameStorage.ServerInfo.cs Added SQL update command in DailyReset method to reset PlayedBy field in nurturing table.
Maple2.File.Ingest/Mapper/FunctionCubeMapper.cs Added RecipeId parameter to FunctionCubeMetadata in Map method; updated ParseNurturing for null/empty checks.
Maple2.Model/Game/Cube/PlotCube.cs Added InteractingCharacterId property to InteractCube; modified WriteTo method to refine serialization.
Maple2.Model/Metadata/FunctionCubeMetadata.cs Added RecipeId property to FunctionCubeMetadata record.
Maple2.Server.Game/Commands/DebugCommand.cs Added LogoutCommand class for logging out; changed visibility of DebugNpcAiCommand to private.
Maple2.Server.Game/Commands/PlayerCommand.cs Introduced DailyResetCommand for forcing daily resets; updated PlayerCommand constructor to include new command.
Maple2.Server.Game/Manager/Field/FieldManager.State.cs Added methods for managing FieldFunctionInteract objects; modified OnAddPlayer for sending interact cubes.
Maple2.Server.Game/Manager/Field/FieldManager.cs Added FunctionCubeMetadata property; updated Update method to include FieldFunctionInteract updates.
Maple2.Server.Game/Manager/HousingManager.cs Updated TryPlaceCube and TryRemoveCube methods to handle housing categories; improved error handling.
Maple2.Server.Game/Manager/MasteryManager.cs Added Gather(FieldFunctionInteract) method; refined success rate calculation and condition updates.
Maple2.Server.Game/Model/Field/Entity/FieldFunctionInteract.cs Introduced FieldFunctionInteract class for managing plot cube interactions.
Maple2.Server.Game/PacketHandlers/FunctionCubeHandler.cs Updated HandleUseCube and HandleNurturing methods for improved interaction handling and state management.
Maple2.Server.Game/Packets/FunctionCubePacket.cs Modified SendCubes method; added UpdateFunctionCube method and updated AddFunctionCube method for distinct operations.
Maple2.Server.Game/Manager/Items/InventoryManager.cs Added Clear(InventoryType tab) method for clearing items from a specified inventory tab.

Assessment against linked issues

Objective Addressed Explanation
Implement Function Cubes (261)
Command to go back to login screen (262) No logout functionality implemented.

Possibly related PRs

  • Feat: House Decor Planner #229: Enhancements to metadata handling and item installations, which may relate to the SQL update command added in the main PR for resetting the PlayedBy field in the nurturing table.
  • Meret Market Rework #238: This PR focuses on modifications to the database context and the introduction of new SQL commands, which could be relevant to the SQL update command added in the main PR.
  • Feat: UGC Blueprints #239: The introduction of user-generated content blueprints may relate to the nurturing functionality enhanced in the main PR, as both involve managing game elements that players interact with.
  • Handle Blueprints in Meret Market #259: This PR enhances the Meret Market by incorporating blueprints, which could be related to the nurturing functionality in the main PR, as both involve item management and player interactions.
  • Feat: Pet Nurturing #281: The addition of nurturing data to the FunctionCubeMetadata entity directly relates to the changes in the main PR, which also focuses on nurturing functionality.

Suggested reviewers

  • Zintixx

Poem

🐰 In the fields where cubes do play,
New commands and functions come to stay.
With nurturing and resets, all in line,
The game grows richer, oh so fine!
Hopping through code, with joy we cheer,
For every change brings us near! 🌼


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between e10bbe9 and fd14a22.

📒 Files selected for processing (1)
  • Maple2.Server.Game/Model/Field/Entity/FieldFunctionInteract.cs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • Maple2.Server.Game/Model/Field/Entity/FieldFunctionInteract.cs

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.

@AngeloTadeucci AngeloTadeucci linked an issue Oct 28, 2024 that may be closed by this pull request
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: 18

🧹 Outside diff range and nitpick comments (16)
Maple2.Server.Game/Model/Field/Entity/FieldFunctionInteract.cs (3)

9-12: Consider adding null-safety annotations for the Cube property.

The Cube property should be marked as non-nullable to enforce compile-time null checks, preventing potential null reference exceptions.

-    public PlotCube Cube { get; }
+    public PlotCube Cube { get; } = null!;

14-17: Add parameter validation and XML documentation.

The constructor should validate its parameters and include XML documentation explaining the purpose of AutoStateChangeTime.

+    /// <summary>
+    /// Initializes a new instance of the FieldFunctionInteract class.
+    /// </summary>
+    /// <param name="field">The field manager instance.</param>
+    /// <param name="objectId">The unique identifier for this entity.</param>
+    /// <param name="value">The metadata containing configuration like AutoStateChangeTime.</param>
+    /// <param name="cube">The plot cube instance to manage.</param>
+    /// <exception cref="ArgumentNullException">Thrown when field, value, or cube is null.</exception>
     public FieldFunctionInteract(FieldManager field, int objectId, FunctionCubeMetadata value, PlotCube cube) : base(field, objectId, value) {
+        ArgumentNullException.ThrowIfNull(cube);
         Cube = cube;
         NextUpdateTick = Field.FieldTick + Value.AutoStateChangeTime;
     }

1-36: Consider improving state management architecture.

The current implementation has several architectural concerns:

  1. Timing mechanism relies on field ticks, which might not be reliable during server lag or after restarts.
  2. State changes and broadcasting are tightly coupled.
  3. No event-based system for state changes.

Consider these improvements:

  1. Use absolute timestamps instead of tick-based timing
  2. Implement an event system for state changes
  3. Separate state management from broadcasting logic

Example event-based approach:

public class FunctionCubeStateChangedEventArgs : EventArgs
{
    public InteractCubeState OldState { get; }
    public InteractCubeState NewState { get; }
    public PlotCube Cube { get; }
}

public event EventHandler<FunctionCubeStateChangedEventArgs>? StateChanged;
Maple2.Database/Storage/Game/GameStorage.ServerInfo.cs (2)

35-35: LGTM with suggestions for improvement.

The SQL command is correctly placed within the daily reset operation and is protected by the thread-safe lock. However, consider these improvements:

  1. Use a constant for the JSON array literal
  2. Add a comment explaining the purpose of resetting the PlayedBy field

Consider refactoring like this:

+ private const string EMPTY_JSON_ARRAY = "[]";
+ // Reset the list of players who have interacted with nurturing mechanics today
- Context.Database.ExecuteSqlRaw("UPDATE `nurturing` SET `PlayedBy` = '[]'");
+ Context.Database.ExecuteSqlRaw($"UPDATE `nurturing` SET `PlayedBy` = '{EMPTY_JSON_ARRAY}'");

36-36: Consider addressing the TODO comment.

The TODO comment about the death counter appears to be unrelated to this change. If you'd like, I can help implement the death counter reset logic.

Would you like me to help implement the death counter reset or create a GitHub issue to track this task?

Maple2.File.Ingest/Mapper/FunctionCubeMapper.cs (1)

Line range hint 47-63: Add array bounds validation in ParseRequiredGrowth

The method processes arrays in triplets but lacks validation to ensure array lengths are valid and aligned. This could lead to IndexOutOfRangeException if the input data is malformed.

Consider adding these validations:

 FunctionCubeMetadata.NurturingData.Growth[] ParseRequiredGrowth(Nurturing nurturing) {
+    if (nurturing.rewardItemByGrowth.Length % 3 != 0) {
+        throw new ArgumentException("rewardItemByGrowth length must be divisible by 3");
+    }
+    if (nurturing.requiredGrowth.Length * 3 != nurturing.rewardItemByGrowth.Length) {
+        throw new ArgumentException("requiredGrowth length must match growth rewards count");
+    }
     List<FunctionCubeMetadata.NurturingData.Growth> result = [];
     for (int i = 0; i < nurturing.rewardItemByGrowth.Length; i += 3) {
Maple2.Server.Game/Packets/FunctionCubePacket.cs (1)

Line range hint 1-44: Consider documenting the packet protocol structure.

The packet structure and protocol for function cube operations would benefit from clear documentation. This would help maintain compatibility with clients and make future modifications easier to implement correctly.

Consider adding a documentation block at the class level describing:

  1. The packet structure for each command
  2. The meaning and valid values for each field
  3. The expected client behavior for each operation
  4. Any version-specific changes or compatibility notes
Maple2.Server.Game/Commands/DebugCommand.cs (1)

31-31: Consider moving logout command to core game commands.

Logout is a core game functionality that should be available to all players, not just those with debug access. Consider moving this to a more appropriate command category (e.g., PlayerCommand or SystemCommand).

Maple2.Server.Game/Manager/HousingManager.cs (1)

453-456: LGTM: Clean implementation of function cube interactions.

The implementation maintains a clean symmetry between placement and removal operations, properly integrating with the existing field function interact system. The code follows good separation of concerns by delegating the actual interaction handling to the field system.

Consider adding logging for these operations to help with debugging and monitoring of farming/ranching cube interactions in production.

Also applies to: 472-475

Maple2.Server.Game/Manager/Field/FieldManager.State.cs (3)

516-538: Consider improving error handling and simplifying rotation.

  1. Add logging for metadata retrieval failures to help with debugging.
  2. The rotation conversion can be simplified.

Consider this refactor:

-            Rotation = new Vector3(0, 0, plotCube.Rotation),
+            Rotation = Vector3.UnitZ * plotCube.Rotation,

Also, consider adding logging:

 if (!ItemMetadata.TryGet(plotCube.ItemId, out ItemMetadata? item) || item.Install is null) {
+    logger.Warning("Invalid item or install metadata for PlotCube: {PlotCube}", plotCube);
     return null;
 }

 if (!FunctionCubeMetadata.TryGet(item.Install.InteractId, out FunctionCubeMetadata? metadata)) {
+    logger.Warning("Invalid function cube metadata for item: {ItemId}", plotCube.ItemId);
     return null;
 }

544-546: Simplify the TryGetFieldFunctionInteract method.

The conditional return can be simplified.

-    public FieldFunctionInteract? TryGetFieldFunctionInteract(string entityId) {
-        return fieldFunctionInteracts.TryGetValue(entityId, out FieldFunctionInteract? fieldFunctionInteract) ? fieldFunctionInteract : null;
-    }
+    public FieldFunctionInteract? TryGetFieldFunctionInteract(string entityId) => 
+        fieldFunctionInteracts.TryGetValue(entityId, out FieldFunctionInteract? fieldFunctionInteract) ? fieldFunctionInteract : null;

32-32: Consider extracting function cube management to a separate service.

The current implementation tightly couples function cube management with the field manager. Consider creating a dedicated FunctionCubeService to:

  1. Encapsulate function cube-specific logic
  2. Improve testability
  3. Reduce the responsibilities of the FieldManager class

This would align better with the Single Responsibility Principle.

Also applies to: 516-656

Maple2.Server.Game/PacketHandlers/FunctionCubeHandler.cs (4)

Line range hint 146-146: Replace hardcoded message with localized string ID

The hardcoded message should be replaced with a localized string ID to support internationalization and maintain consistency.

I can assist in finding or defining the correct string ID for this message. Would you like me to help with this?


Line range hint 170-170: Replace hardcoded message with localized string ID

The hardcoded message should be replaced with the appropriate localized string ID.

Let me know if you need assistance in identifying the correct string ID.


Line range hint 175-175: Replace hardcoded message with localized string ID

The hardcoded message should be replaced with a localized string ID to maintain consistency and support localization.

I can help find the correct string ID for this message. Would you like me to assist with this?


Line range hint 196-220: Consider removing or properly managing large commented-out code

There is a large block of commented-out code related to mailing functionality. Large commented sections can clutter the codebase and reduce readability. Consider the following options:

  • Remove the commented code if it's no longer needed, trusting version control for historical changes.
  • If the code is pending the resolution of an issue (as indicated), consider adding a clearer TODO comment with a reference to the issue.
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 453c7e0 and b7bf6f6.

📒 Files selected for processing (13)
  • Maple2.Database/Storage/Game/GameStorage.ServerInfo.cs (1 hunks)
  • Maple2.File.Ingest/Mapper/FunctionCubeMapper.cs (1 hunks)
  • Maple2.Model/Game/Cube/PlotCube.cs (1 hunks)
  • Maple2.Model/Metadata/FunctionCubeMetadata.cs (1 hunks)
  • Maple2.Server.Game/Commands/DebugCommand.cs (2 hunks)
  • Maple2.Server.Game/Commands/PlayerCommand.cs (3 hunks)
  • Maple2.Server.Game/Manager/Field/FieldManager.State.cs (3 hunks)
  • Maple2.Server.Game/Manager/Field/FieldManager.cs (3 hunks)
  • Maple2.Server.Game/Manager/HousingManager.cs (2 hunks)
  • Maple2.Server.Game/Manager/MasteryManager.cs (4 hunks)
  • Maple2.Server.Game/Model/Field/Entity/FieldFunctionInteract.cs (1 hunks)
  • Maple2.Server.Game/PacketHandlers/FunctionCubeHandler.cs (4 hunks)
  • Maple2.Server.Game/Packets/FunctionCubePacket.cs (2 hunks)
🔇 Additional comments (21)
Maple2.Model/Metadata/FunctionCubeMetadata.cs (1)

8-8: LGTM! Clean addition of RecipeId property.

The property is well-positioned and follows C# naming conventions.

Maple2.File.Ingest/Mapper/FunctionCubeMapper.cs (2)

Line range hint 31-33: LGTM! Robust null handling implementation.

The null and empty array checks provide comprehensive validation before processing the nurturing data, preventing potential runtime exceptions.


22-22: Verify the property name spelling: "receipeID"

There appears to be a typo in the property name "receipeID" (should be "recipeId"). However, since this might be intentional due to the underlying data model or XML schema, please verify:

  1. If this spelling matches the XML schema definition
  2. If this is a known naming convention in the codebase
✅ Verification successful

Property spelling "receipeID" is consistent with codebase patterns

The spelling "receipe" appears to be a consistent pattern across the codebase:

  • Used in XML file names: "masteryreceipe.xml"
  • Used in property names: "receipeID", "receipe.id", "receipe.rank", "receipe.count"
  • Used across multiple mapper and storage classes

This indicates the spelling is intentional and matches the underlying data model. The current implementation maintains consistency with this established pattern.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for other occurrences of this property name to verify consistency
rg -i "receipe" --type cs

# Search for the XML schema or definition files
fd -e xsd -e xml | xargs rg -i "receipe"

Length of output: 1784

Maple2.Model/Game/Cube/PlotCube.cs (1)

44-44: Consider adding thread safety mechanisms.

The InteractingCharacterId property could be accessed by multiple threads simultaneously in a multiplayer context. Consider using appropriate synchronization mechanisms or atomic operations to prevent race conditions.

Here's a script to verify concurrent access patterns:

Maple2.Server.Game/Commands/PlayerCommand.cs (1)

25-25: LGTM! Command registration follows established pattern.

The new command is properly registered in the constructor, maintaining consistency with other command registrations.

Maple2.Server.Game/Commands/DebugCommand.cs (1)

34-34: LGTM! Good encapsulation practice.

Making DebugNpcAiCommand private is appropriate since it's only used within DebugCommand. This change improves encapsulation and reduces the public API surface.

Maple2.Server.Game/Manager/Field/FieldManager.cs (3)

284-284: LGTM: Field function interact update follows established pattern

The update call for field function interacts is properly integrated into the update cycle, following the same pattern as other field objects.


45-45: LGTM: FunctionCubeMetadata property follows established pattern

The property is correctly declared with init-only setter and follows the same pattern as other metadata storage properties in the class.


141-148: ⚠️ Potential issue

Verify initialization of function cubes

The initialization of function cubes is correctly scoped to the default home map and properly filters for relevant housing categories. However, consider adding a null check for the first plot since FirstOrDefault() could return null.

Consider adding a null check:

-            List<PlotCube> lifeSkillCubes = Plots.FirstOrDefault().Value.Cubes.Values
+            Plot? firstPlot = Plots.FirstOrDefault();
+            if (firstPlot is null) {
+                return;
+            }
+            List<PlotCube> lifeSkillCubes = firstPlot.Value.Cubes.Values
                 .Where(x => x.HousingCategory is HousingCategory.Ranching or HousingCategory.Farming)
                 .ToList();
Maple2.Server.Game/Manager/Field/FieldManager.State.cs (1)

32-32: LGTM! Thread-safe collection choice.

The use of ConcurrentDictionary is appropriate for managing concurrent access to field function interactions.

Maple2.Server.Game/Manager/MasteryManager.cs (8)

5-5: LGTM!

The addition of using Maple2.Server.Core.Packets; is appropriate and necessary for packet handling in this file.


113-114: Verification of myHome calculation and success rate adjustment

The calculation of myHome as session.Field.OwnerId == session.AccountId ? 1 : 0 correctly determines if the player is in their own home. Passing myHome to lua.CalcGatheringObjectSuccessRate adjusts the success rate appropriately based on the player's location.


117-117: LGTM!

Invoking BeforeConditionUpdate(recipeMetadata); before attempting to gather ensures that all necessary pre-gathering conditions are correctly updated.


129-129: LGTM!

The spawning of the item using session.Field.SpawnItem and broadcasting it to the field is correctly implemented.


134-135: LGTM!

Calling AfterConditionUpdate(recipeMetadata, gatheringAmount); after successful gathering appropriately updates post-gathering conditions.


136-141: LGTM!

Updating the mastery experience, adding player experience, and incrementing the gathering count are correctly handled. The interaction packet is sent with the correct decrease amount.


Line range hint 194-211: LGTM!

The AfterConditionUpdate method correctly updates post-gathering conditions based on the MasteryType. Conditional checks and updates align with the intended game mechanics.


213-223: LGTM!

The BeforeConditionUpdate method appropriately handles condition updates prior to gathering, ensuring that all necessary conditions are set.

Maple2.Server.Game/PacketHandlers/FunctionCubeHandler.cs (3)

66-68: Good practice: Added null check for cube.Interact

The null check for cube.Interact ensures that the code safely handles cases where cube.Interact may be null, preventing potential NullReferenceException. Well done.


77-86: Proper handling of Farming and Ranching categories

The addition of handling for HousingCategory.Farming and HousingCategory.Ranching is appropriately implemented. Null checks are in place, and the interactions are correctly managed.


Line range hint 127-129: Verify the condition for fully grown nurturing

The condition checking if ClaimedGiftForStage is greater than RequiredGrowth.Length may need verification. Since array indices are zero-based, consider whether the condition should be >= instead of > to accurately determine if the nurturing is fully grown.

✅ Verification successful

The > comparison is correct for fully grown nurturing check

Based on the code analysis:

  1. ClaimedGiftForStage starts at 1 (initialized in both CreateNurturing and the constructor)
  2. Stages in RequiredGrowth array are 1-based (set in ParseRequiredGrowth where Stage: (short) (i / 3 + 1))
  3. When claiming a reward, ClaimedGiftForStage is incremented after verifying exp requirements
  4. The > check correctly identifies when all stages are completed because:
    • If array length is N, the last valid stage is N
    • When ClaimedGiftForStage becomes N+1 (greater than length), it means all stages are claimed

The current implementation is consistent with the 1-based stage system used throughout the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Verify if the condition should be '>=' instead of '>' for fully grown nurturing.

# Test: Search for similar comparisons in the codebase. Expect: Consistent use of comparison operators.
rg -A 2 'ClaimedGiftForStage\s*[>=]+\s*NurturingMetadata\.RequiredGrowth\.Length'

Length of output: 83


Script:

#!/bin/bash
# Let's try a broader search to find any references to ClaimedGiftForStage
rg -A 5 "ClaimedGiftForStage"

# Also search for RequiredGrowth to understand the context better
rg -A 5 "RequiredGrowth"

# Use ast-grep to find any array length comparisons that might be similar
ast-grep --pattern '$_.$_.Length > $_'
ast-grep --pattern '$_.$_.Length >= $_'

Length of output: 12809

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: 5

🧹 Outside diff range and nitpick comments (6)
Maple2.Server.Game/Packets/FunctionCubePacket.cs (1)

32-33: Follow C# naming conventions for constants.

Consider using PascalCase for public/private constants in C#:

-    private const byte StateAdd = 0;
-    private const byte StateUpdate = 1;
+    private const byte STATE_ADD = 0;
+    private const byte STATE_UPDATE = 1;
Maple2.Server.Game/Manager/MasteryManager.cs (3)

134-138: Consider making home gathering check more explicit.

The home gathering logic could be more self-documenting:

-int myHome = 1; // For outside gathering, it should be always 1
-if (session.Field.MapId is Constant.DefaultHomeMapId) {
-    myHome = session.Field.OwnerId == session.AccountId ? 1 : 0;
-}
+bool isHomeMap = session.Field.MapId is Constant.DefaultHomeMapId;
+bool isOwnHome = isHomeMap && session.Field.OwnerId == session.AccountId;
+int myHome = isHomeMap && !isOwnHome ? 0 : 1;

Line range hint 119-165: Consider breaking down GatherCommom into smaller methods.

The method handles multiple responsibilities (validation, gathering mechanics, rewards). Consider extracting these into separate private methods for better maintainability:

+private bool ValidateGatheringRequirements(MasteryRecipeTable.Entry recipe) {
+    if (recipe.RequiredMastery > this[recipe.Type]) {
+        session.Send(MasteryPacket.Error(MasteryError.s_mastery_error_lack_mastery));
+        return false;
+    }
+    return true;
+}

+private void DistributeRewards(MasteryRecipeTable.Entry recipe, Vector3 position, Vector3 rotation) {
+    foreach (ItemComponent itemReward in recipe.RewardItems) {
+        Item? item = session.Field.ItemDrop.CreateItem(itemReward.ItemId, itemReward.Rarity, itemReward.Amount);
+        if (item == null) continue;
+        
+        FieldItem fieldItem = session.Field.SpawnItem(position, rotation, item, session.CharacterId);
+        session.Field.Broadcast(FieldPacket.DropItem(fieldItem));
+    }
+}

Line range hint 167-197: Well-structured condition update methods.

The methods effectively handle different mastery types. Consider using C# 8.0's switch expressions for more concise code:

-private void BeforeConditionUpdate(MasteryRecipeTable.Entry recipeMetadata) {
-    switch (recipeMetadata.Type) {
-        case MasteryType.Farming:
-        case MasteryType.Breeding:
-            session.ConditionUpdate(ConditionType.mastery_harvest_try, codeLong: recipeMetadata.Id);
-            session.ConditionUpdate(ConditionType.mastery_farming_try, codeLong: recipeMetadata.Id);
-            break;
-        case MasteryType.Gathering:
-        case MasteryType.Mining:
-            session.ConditionUpdate(ConditionType.mastery_gathering_try, codeLong: recipeMetadata.Id);
-            break;
-    }
-}
+private void BeforeConditionUpdate(MasteryRecipeTable.Entry recipeMetadata) => 
+    _ = recipeMetadata.Type switch {
+        MasteryType.Farming or MasteryType.Breeding => UpdateFarmingConditions(recipeMetadata.Id),
+        MasteryType.Gathering or MasteryType.Mining => session.ConditionUpdate(ConditionType.mastery_gathering_try, codeLong: recipeMetadata.Id),
+        _ => null
+    };
+
+private object? UpdateFarmingConditions(int recipeId) {
+    session.ConditionUpdate(ConditionType.mastery_harvest_try, codeLong: recipeId);
+    session.ConditionUpdate(ConditionType.mastery_farming_try, codeLong: recipeId);
+    return null;
+}
Maple2.Server.Game/Manager/Items/InventoryManager.cs (2)

561-562: Consider batching the remove operations.

The current implementation calls Remove and Discard for each item individually, which could be inefficient for large inventories. Consider batching these operations to reduce the number of packets sent to the client.


553-565: Add logging for inventory clear operations.

For auditing and debugging purposes, consider adding logging when an inventory tab is cleared. This would help track when and why inventory clear operations occur.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between b7bf6f6 and e10bbe9.

📒 Files selected for processing (6)
  • Maple2.Server.Game/Commands/PlayerCommand.cs (3 hunks)
  • Maple2.Server.Game/Manager/Items/InventoryManager.cs (1 hunks)
  • Maple2.Server.Game/Manager/MasteryManager.cs (5 hunks)
  • Maple2.Server.Game/Model/Field/Entity/FieldFunctionInteract.cs (1 hunks)
  • Maple2.Server.Game/PacketHandlers/FunctionCubeHandler.cs (4 hunks)
  • Maple2.Server.Game/Packets/FunctionCubePacket.cs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • Maple2.Server.Game/PacketHandlers/FunctionCubeHandler.cs
🔇 Additional comments (7)
Maple2.Server.Game/Packets/FunctionCubePacket.cs (2)

27-27: The previous review comment about documenting WriteByte() is still valid.

The WriteByte() call without a value remains unclear and potentially problematic.


35-39: Clean and well-structured implementation!

The use of expression-bodied members and delegation to a shared helper method follows good C# practices.

Maple2.Server.Game/Manager/MasteryManager.cs (2)

101-105: LGTM! Good refactoring of the Gather method.

The method has been simplified by extracting common logic into GatherCommom, making it more maintainable.


107-117: Well-structured new Gather method for function cubes.

Good implementation with proper null checking and error handling. The method maintains consistency with the existing gather pattern.

Maple2.Server.Game/Commands/PlayerCommand.cs (2)

25-26: LGTM: Command registration follows established pattern.

The new commands are properly registered in the PlayerCommand constructor, maintaining consistency with other command registrations.


272-283: ⚠️ Potential issue

Critical: Add error handling, permissions, and logging.

The daily reset command implementation is missing several critical components:

  1. Error handling with try-catch block
  2. Permission checks to restrict access
  3. Audit logging for tracking usage
  4. User confirmation for this impactful operation

Apply this diff to improve the implementation:

 private void Handle(InvocationContext ctx) {
-    session.DailyReset();
+    try {
+        // Add permission check
+        if (!session.Player.Value.Character.IsAdmin) {
+            ctx.Console.Error.WriteLine("This command requires administrative privileges.");
+            ctx.ExitCode = 1;
+            return;
+        }
+
+        // Add confirmation
+        ctx.Console.Out.WriteLine("WARNING: This will reset all daily activities for the player.");
+        ctx.Console.Out.WriteLine("Are you sure? (y/N)");
+        string? input = ctx.Console.In.ReadLine()?.ToLower();
+        if (input != "y") {
+            ctx.Console.Out.WriteLine("Operation cancelled.");
+            ctx.ExitCode = 0;
+            return;
+        }
+
+        // Add logging
+        Console.WriteLine($"[{DateTime.UtcNow:u}] Daily reset executed for player {session.PlayerName} by admin");
+
+        session.DailyReset();
+        ctx.ExitCode = 0;
+    } catch (Exception ex) {
+        ctx.Console.Error.WriteLine($"Failed to execute daily reset: {ex.Message}");
+        ctx.ExitCode = 1;
+    }
 }
Maple2.Server.Game/Manager/Items/InventoryManager.cs (1)

553-565: 🛠️ Refactor suggestion

Consider adding validation for protected items.

The Clear method should validate if items can be safely removed before clearing them. Some items might be quest-related, equipped, or otherwise protected from bulk removal operations.

Consider adding validation and batching the operations:

 public void Clear(InventoryType tab) {
     lock (session.Item) {
         if (!tabs.TryGetValue(tab, out ItemCollection? items)) {
             session.Send(ItemInventoryPacket.Error(s_item_err_not_active_tab));
             return;
         }
 
+        // Batch items for removal
+        var itemsToRemove = new List<Item>();
+        foreach (Item item in items) {
+            // Skip protected items
+            if (IsProtectedItem(item)) {
+                continue;
+            }
+            itemsToRemove.Add(item);
+        }
+
+        // Log the clear operation
+        Log.Information("Clearing {Count} items from inventory tab {Tab}", itemsToRemove.Count, tab);
+
+        // Batch remove items
+        foreach (var batch in itemsToRemove.Chunk(BATCH_SIZE)) {
+            foreach (Item item in batch) {
+                Remove(item.Uid, out _, item.Amount);
+                Discard(item);
+            }
+            // Notify clients about the batch removal
+            session.Send(ItemInventoryPacket.BatchRemove(batch.Select(i => i.Uid).ToArray()));
+        }
+
+        // Notify about inventory clear completion
+        session.ConditionUpdate(ConditionType.inventory_clear, counter: itemsToRemove.Count);
-        foreach (Item item in items) {
-            Remove(item.Uid, out _, item.Amount);
-            Discard(item);
-        }
     }
 }

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Command to go back to login screen Implement Function Cubes

2 participants

Comments