Skip to content

Trigger - SetSkill - #394

Merged
AngeloTadeucci merged 1 commit into
masterfrom
trigger-skill
Apr 1, 2025
Merged

Trigger - SetSkill#394
AngeloTadeucci merged 1 commit into
masterfrom
trigger-skill

Conversation

@AngeloTadeucci

@AngeloTadeucci AngeloTadeucci commented Apr 1, 2025

Copy link
Copy Markdown
Collaborator

Feat: Region Skill Vibrate Objects
Reworked trigger command to add the ability of manually running trigger functions

Summary by CodeRabbit

  • New Features

    • Introduced new commands for trigger management, allowing users to list, reset, and execute map triggers.
    • Enhanced game skills now produce a vibration effect on nearby objects with refined damage notifications.
    • Improved debug feedback now displays a count of vibration objects detected for better in-game diagnostics.
    • Added a method to vibrate objects within a specified range, enhancing interaction with skills.
  • Refactor

    • Streamlined underlying operations to boost performance and maintainability.
    • Updated the structure of FieldEntityId to include an additional identifier for improved entity management.

@coderabbitai

coderabbitai Bot commented Apr 1, 2025

Copy link
Copy Markdown
Contributor

Walkthrough

The pull request updates the FieldEntityId type across several modules by adding a new string parameter to its constructor. Related methods and parsing logic have been adjusted accordingly. The changes also enhance command functionality by refactoring trigger commands into separate subcommands and adding vibrate object counting and execution within the field. Additional modifications include improved logging in trigger contexts, refined branching in NPC movement, and simplified syntax in keyframe event handling.

Changes

File(s) Change Summary
Maple2.File.Ingest/.../MapDataMapper.cs, Maple2.Model/Game/Field/.../FieldAccelerationStructure.cs, Maple2.Model/Metadata/.../FieldEntity.cs Updated FieldEntityId to use a three-parameter constructor (adding a string identifier). Adjusted instantiation, parsing, and serialization logic accordingly.
Maple2.Server.Game/Commands/.../DebugCommand.cs, Maple2.Server.Game/Commands/.../TriggerCommand.cs Enhanced command handling: DebugCommand now counts vibrate objects, and TriggerCommand has been split into modular subcommands (ListCommand, ResetCommand, RunCommand) for trigger management.
Maple2.Server.Game/Manager/Field/.../FieldManager.cs, Maple2.Server.Game/Model/Field/.../FieldSkill.cs Introduced field vibration functionality. FieldManager now provides a VibrateObjects method which FieldSkill invokes before conditionally broadcasting damage.
Maple2.Server.Game/Trigger/.../TriggerContext.Field.cs, Maple2.Server.Game/Trigger/.../TriggerContext.Npc.cs Transitioned logging from error to debug in trigger handling. Updated the skill setting logic and NPC list filtering methodologies.
Maple2.Server.Game/Model/Field/.../FieldNpc.cs Simplified the KeyframeEvent method by removing unnecessary block braces in the switch-case for routines starting with "Walk_".

Sequence Diagram(s)

sequenceDiagram
    participant FS as FieldSkill
    participant FM as FieldManager
    participant AS as AccelerationStructure
    participant Client as Game Client

    FS->>FM: Update method invoked (with SkillRecord & Position)
    FM->>AS: Query nearby vibrate entities
    AS-->>FM: Return list of FieldVibrateEntity objects
    FM->>Client: Broadcast VibratePacket for each entity
Loading
sequenceDiagram
    participant User as User Input
    participant TC as TriggerCommand Dispatcher
    participant List as ListCommand
    participant Reset as ResetCommand
    participant Run as RunCommand

    User->>TC: Issue trigger command
    alt Command is "list"
        TC->>List: Execute trigger listing
        List-->>TC: Return trigger count and states
    else Command is "reset"
        TC->>Reset: Execute trigger reset
        Reset-->>TC: Confirm reset or report error
    else Command is "run"
        TC->>Run: Execute trigger run function
        Run-->>TC: Return execution result or error
    end
    TC-->>User: Output command result
Loading

Possibly related PRs

Suggested reviewers

  • Zintixx

Poem

I'm a bunny coding in the glen,
Hopping through changes again and again.
Fields vibrate and triggers now sing,
With new parameters, my code bells ring.
Carrots and code, what a lovely blend!
🐰💻

✨ Finishing Touches
  • 📝 Generate Docstrings

🪧 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 generate docstrings to generate docstrings for this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai plan to trigger planning for file edits and PR creation.
  • @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.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🔭 Outside diff range comments (1)
Maple2.Server.Game/Commands/TriggerCommand.cs (1)

22-46: 💡 Verification agent

🧩 Analysis chain

Verify session.Field usage.
In ListCommand, you're using session.Field! with the null-forgiving operator. Confirm that session.Field is never null in all contexts where this command can be invoked, or consider adding a safeguard.

Please run the following script to check how session.Field is assigned or validated throughout the codebase:


🏁 Script executed:

#!/bin/bash
fd -e cs --exec rg 'session\.Field'

Length of output: 37411


Action Required: Safeguard against null session.Field in ListCommand
Multiple parts of the codebase explicitly check if session.Field is null. Given these null-checks, using the null-forgiving operator (!) in the ListCommand—specifically on the line initializing fieldTriggers—may lead to unexpected runtime errors if session.Field isn’t guaranteed to be non-null when the command executes. Consider adding a preliminary null-check in the Handle method, for example:

  • Verify that session.Field is non-null before enumerating triggers.
    if (session.Field is null) {
        ctx.Console.Out.WriteLine("Error: Field is not available.");
        return;
    }
  • Remove the null-forgiving operator once a safeguard is in place.
🧹 Nitpick comments (4)
Maple2.Server.Game/Trigger/TriggerContext.Field.cs (1)

325-340: Implementation improved for SetSkill method.

The method has been enhanced to actually set skills instead of just logging. It now iterates through triggerIds, retrieves corresponding skills, validates their metadata, and adds them to the field with proper positioning.

Consider using a Dictionary lookup instead of FirstOrDefault for retrieving skills by triggerId, as this would be more efficient for large collections:

-            Ms2TriggerSkill? skill = Field.Entities.Trigger.Skills.FirstOrDefault(x => x.TriggerId == triggerId);
+            if (!Field.Entities.Trigger.Skills.TryGetValue(triggerId, out Ms2TriggerSkill? skill) || skill == null) {
+                continue;
+            }

This assumes you could refactor Field.Entities.Trigger.Skills to be a Dictionary<int, Ms2TriggerSkill> rather than a collection that requires linear search.

Maple2.Server.Game/Trigger/TriggerContext.Npc.cs (1)

63-64: Performance improvement: Materialized collection for better efficiency

Converting the LINQ query result to a List improves performance by:

  1. Materializing the results immediately instead of deferred execution
  2. Using Count == 0 on a List which is more efficient than Any() on an IEnumerable
  3. Avoiding re-evaluating the query when iterating in the foreach loop later
Maple2.Server.Game/Commands/TriggerCommand.cs (2)

48-91: Clarify usage for negative state index.
Using -1 to reset a trigger is functional but not explicitly documented in the usage help. Consider adding an example or note in the command description to inform users that passing a negative state index performs a reset.

Here is a small diff to clarify usage:

 public ResetCommand(GameSession session) : base("reset", "Reset a specific trigger") {
     this.session = session;

     var triggerName = new Argument<string>("triggerName", "Name of the trigger to reset");
-    var stateOption = new Option<int>(["--state", "-s"], () => -1, "State index to set");
+    var stateOption = new Option<int>(["--state", "-s"], () => -1, "State index to set (use -1 to reset trigger)');

93-181: Enhance parameter parsing and trigger selection.

  1. Parsing booleans and integers (bool.Parse, int.Parse) can throw exceptions if invalid input is passed. Consider graceful error handling to prevent unhandled exceptions.
  2. Currently, the command retrieves only the first trigger in the field (FirstOrDefault()), which might be insufficient if multiple triggers exist. Consider adding an argument to let users specify the trigger name they want to run.

Below is a potential refactor to demonstrate specifying a trigger name:

- var functionName = new Argument<string>("functionName", "Name of the function to run");
- var parameters = new Argument<string[]>("parameters", "Parameters to pass to the function");
+ var triggerArg = new Argument<string>("triggerName", "Name of the trigger to use");
+ var functionName = new Argument<string>("functionName", "Name of the function to run");
+ var parameters = new Argument<string[]>("parameters", () => Array.Empty<string>(), 
+     "Parameters to pass to the function");

- AddArgument(functionName);
+ AddArgument(triggerArg);
  AddArgument(functionName);
  AddArgument(parameters);

- this.SetHandler<InvocationContext, string, string[]>(Handle, functionName, parameters);
+ this.SetHandler<InvocationContext, string, string, string[]>(Handle, triggerArg, functionName, parameters);

// Then, inside `Handle`:
- FieldTrigger? trigger = session.Field.EnumerateTrigger().FirstOrDefault();
+ FieldTrigger? trigger = session.Field.EnumerateTrigger()
+     .FirstOrDefault(t => t.Value.Name.Equals(triggerName, StringComparison.OrdinalIgnoreCase));
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 5c51ac6 and 02ee9fe.

📒 Files selected for processing (10)
  • Maple2.File.Ingest/Mapper/MapDataMapper.cs (1 hunks)
  • Maple2.Model/Game/Field/FieldAccelerationStructure.cs (6 hunks)
  • Maple2.Model/Metadata/FieldEntity/FieldEntity.cs (1 hunks)
  • Maple2.Server.Game/Commands/DebugCommand.cs (1 hunks)
  • Maple2.Server.Game/Commands/TriggerCommand.cs (2 hunks)
  • Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs (2 hunks)
  • Maple2.Server.Game/Model/Field/Actor/FieldNpc.cs (1 hunks)
  • Maple2.Server.Game/Model/Field/Entity/FieldSkill.cs (2 hunks)
  • Maple2.Server.Game/Trigger/TriggerContext.Field.cs (3 hunks)
  • Maple2.Server.Game/Trigger/TriggerContext.Npc.cs (1 hunks)
🧰 Additional context used
🧬 Code Definitions (5)
Maple2.Server.Game/Trigger/TriggerContext.Npc.cs (4)
Maple2.Server.Game/Model/Field/Entity/FieldMobSpawn.cs (1)
  • List (69-97)
Maple2.Server.Game/Manager/Field/AgentNavigation.cs (3)
  • List (32-34)
  • List (36-38)
  • List (40-158)
Maple2.Server.Game/Model/Field/Actor/FieldNpc.cs (2)
  • FieldNpc (22-474)
  • FieldNpc (97-128)
Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs (2)
  • FieldNpc (106-126)
  • FieldNpc (128-130)
Maple2.File.Ingest/Mapper/MapDataMapper.cs (1)
Maple2.Model/Metadata/FieldEntity/FieldEntity.cs (1)
  • FieldEntityId (24-30)
Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs (5)
Maple2.Server.Game/Model/Field/Entity/FieldEntity.cs (2)
  • FieldEntity (7-26)
  • FieldEntity (16-21)
Maple2.Server.Game/Model/Field/Actor/Actor.cs (1)
  • SkillRecord (214-232)
Maple2.Server.Game/Model/Field/Actor/IActor.cs (1)
  • SkillRecord (29-29)
Maple2.Server.Game/Manager/Field/FieldManager/IField.cs (1)
  • Broadcast (68-68)
Maple2.Server.Game/GameServer.cs (1)
  • Broadcast (185-189)
Maple2.Server.Game/Model/Field/Actor/FieldNpc.cs (2)
Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/MovementState.cs (2)
  • MovementState (11-320)
  • MovementState (37-48)
Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/MovementStateTasks/MovementState.WalkTask.cs (1)
  • MovementState (7-182)
Maple2.Server.Game/Commands/TriggerCommand.cs (1)
Maple2.Server.Game/Trigger/TriggerContext.Npc.cs (5)
  • SpawnMonster (22-27)
  • SpawnMonster (47-50)
  • MoveNpc (60-77)
  • SetNpcEmotionSequence (154-159)
  • DestroyMonster (29-45)
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: build
🔇 Additional comments (14)
Maple2.Server.Game/Trigger/TriggerContext.Field.cs (1)

386-387: Improved logging level in RemoveBuff.

Changed from error logging to debug logging, which is more appropriate for this routine operation.

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

103-103:

Details

✅ Verification successful

Updated constructor to match revised FieldEntityId signature

Added the new string.Empty parameter to align with the modified FieldEntityId constructor that now accepts a string ID parameter in addition to the numeric identifiers.

Run the following script to verify that all other instantiations of FieldEntityId have been properly updated:


🏁 Script executed:

#!/bin/bash
# Find all instances of FieldEntityId constructor usage
rg -A 1 "new FieldEntityId\(" --type cs

Length of output: 1653


FieldEntityId Constructor Update Verified Across the Codebase

The update in MapDataMapper.cs—adding the string.Empty parameter to match the revised FieldEntityId constructor—has been confirmed. The verification script output shows that all other usages (in both model and game modules) consistently pass three parameters, ensuring uniformity in instantiation.

  • Maple2.Model/Metadata/FieldEntity/FieldEntity.cs uses new FieldEntityId(high, low, id).
  • Maple2.Model/Game/Field/FieldAccelerationStructure.cs contains multiple updates using either string.Empty or correctly reading a string from a reader.
  • The targeted change in Maple2.File.Ingest/Mapper/MapDataMapper.cs aligns with these adjustments.
Maple2.Server.Game/Model/Field/Actor/FieldNpc.cs (1)

241-242: Simplified case syntax for consistency

Removed unnecessary braces around the return statement, making the code more consistent with other case statements in the switch block and improving readability.

Maple2.Model/Metadata/FieldEntity/FieldEntity.cs (2)

18-21: Enhanced FieldEntityId with string representation

Added a new string property to store the original ID representation, which is beneficial for debugging, serialization, or maintaining the original format without needing to reconstruct it.


24-30: Improved string parsing performance

Enhances the FromString method with more efficient string handling:

  1. Uses ReadOnlySpan<char> and AsSpan() for more efficient string slicing without allocations
  2. Uses ulong.Parse with explicit NumberStyles.HexNumber instead of Convert.ToUInt64
  3. Properly passes the original string ID to the constructor
Maple2.Server.Game/Commands/DebugCommand.cs (1)

304-314: Great enhancement for vibrate object debugging!

Adding the counter and displaying the total vibrate object count is a useful improvement for debugging. This will help track the number of vibrate objects in a specified area, making it easier to understand the visual effects distribution in the game.

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

14-14: LGTM - Required imports added

The new imports support the vibrate functionality implementation.

Also applies to: 21-21


567-577:

Details

❓ Verification inconclusive

Well-implemented vibrate objects interaction method

The new VibrateObjects method effectively implements the functionality to trigger vibration effects on field objects when skills are used. The implementation correctly:

  1. Calculates the range based on the skill's attack range
  2. Guards against null AccelerationStructure
  3. Efficiently queries and broadcasts vibration packets to affected objects

The query uses twice the range distance (line 573), which expands the area of effect. This is likely intentional but worth confirming.


🏁 Script executed:

#!/bin/bash
# Check if other range calculations in the codebase also double the distance
rg "2 \* new Vector3\(.*range" --type cs

Length of output: 279


VibrateObjects Method Approved – Please Confirm the Expanded Range Behavior

The implementation is solid, with proper null-checks and broadcasting of vibration effects to entities. The method doubles the range—as seen in the call to QueryVibrateObjectsCenterList (i.e., 2 * new Vector3(rangeDistance, rangeDistance, rangeDistance))—which appears to be intentional since no other parts of the codebase contradict this approach. Please confirm that the increased area of effect is by design.

  • Null check for AccelerationStructure is correctly applied.
  • The doubled range calculation is uniquely used in this function.
  • If the expanded range is expected, no further changes are needed.
Maple2.Server.Game/Model/Field/Entity/FieldSkill.cs (2)

175-175: Good integration of vibrate functionality with skills

Adding the vibration effect before damage application ensures visual feedback is synchronized with gameplay. This enhances player experience by providing visual cues for skill impacts.


186-188: Performance optimization for target packet broadcasting

The conditional check before broadcasting target packets prevents unnecessary network traffic when no targets are hit. This is a good performance optimization, especially for AoE skills that might not always hit targets.

Maple2.Model/Game/Field/FieldAccelerationStructure.cs (2)

367-367: Consistent FieldEntityId structure update

The changes to FieldEntityId initialization properly integrate the new string parameter across the codebase. Using empty strings as defaults is appropriate for these initialization contexts.

Also applies to: 522-523, 568-569, 809-810, 864-865, 872-873


737-738: Serialization update for FieldEntityId

The added line ensures the string ID is properly written during serialization, maintaining data integrity when storing and retrieving field entities.

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

14-14: Corrected command description.
This updated description better reflects the broadened scope of trigger management in this class.


17-19: Good use of subcommands.
Splitting trigger commands into separate subcommands (ListCommand, ResetCommand, RunCommand) improves clarity, maintainability, and adheres to the single responsibility principle.

Feat: Region Skill Vibrate Objects
Reworked trigger command to add the ability of manually running trigger functions

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 0

🧹 Nitpick comments (7)
Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs (1)

573-576: Consider performance optimization for large object counts

While the current implementation works well, in scenarios with many vibrate objects, consider adding a maximum limit to the number of objects that can vibrate simultaneously to prevent potential performance issues.

-List<FieldVibrateEntity> vibrateObjects = AccelerationStructure.QueryVibrateObjectsCenterList(position, 2 * new Vector3(rangeDistance, rangeDistance, rangeDistance));
-foreach (FieldVibrateEntity vibrate in vibrateObjects) {
-    Broadcast(VibratePacket.Attack(vibrate.Id.Id, record));
-}
+const int MaxVibrateObjectsCount = 50; // Adjust based on performance testing
+List<FieldVibrateEntity> vibrateObjects = AccelerationStructure.QueryVibrateObjectsCenterList(position, 2 * new Vector3(rangeDistance, rangeDistance, rangeDistance));
+
+// Take only a subset if there are too many objects to prevent performance issues
+foreach (FieldVibrateEntity vibrate in vibrateObjects.Take(MaxVibrateObjectsCount)) {
+    Broadcast(VibratePacket.Attack(vibrate.Id.Id, record));
+}
Maple2.Server.Game/Commands/TriggerCommand.cs (6)

107-122: Consider using constants for function names and enhancing trigger selection

The function names are currently hardcoded as string literals. Using constants would improve maintainability.

Also, the code always selects the first trigger found, which may be confusing if multiple triggers exist in the map.

-            functionName = functionName.ToLower();
-            List<string> functionNames = ["set_skill", "spawn_monster", "move_npc", "set_npc_emotion_sequence", "destroy_monster"];
+            functionName = functionName.ToLower();
+            // Define constants for function names to improve maintainability
+            const string FN_SET_SKILL = "set_skill";
+            const string FN_SPAWN_MONSTER = "spawn_monster";
+            const string FN_MOVE_NPC = "move_npc";
+            const string FN_SET_NPC_EMOTION = "set_npc_emotion_sequence";
+            const string FN_DESTROY_MONSTER = "destroy_monster";
+            
+            List<string> functionNames = [FN_SET_SKILL, FN_SPAWN_MONSTER, FN_MOVE_NPC, FN_SET_NPC_EMOTION, FN_DESTROY_MONSTER];

-            FieldTrigger? trigger = session.Field.EnumerateTrigger().FirstOrDefault();
+            // Consider adding an optional trigger name parameter to select a specific trigger
+            var triggerOption = new Option<string?>(["--trigger", "-t"], "Name of the trigger to use");
+            AddOption(triggerOption);
+            
+            FieldTrigger? trigger;
+            if (!string.IsNullOrEmpty(triggerName))
+            {
+                trigger = session.Field.EnumerateTrigger().FirstOrDefault(t => t.Value.Name == triggerName);
+                if (trigger is null)
+                {
+                    ctx.Console.Error.WriteLine($"Trigger {triggerName} not found.");
+                    return;
+                }
+            }
+            else
+            {
+                trigger = session.Field.EnumerateTrigger().FirstOrDefault();
+            }

123-135: Add error handling for parse failures in set_skill

The current implementation might throw exceptions if the skill IDs aren't properly formatted numbers. Consider adding try-catch blocks to handle parse errors.

-                    var skillIds = parameters[0].Split(',').Select(int.Parse).ToArray();
-                    bool enabled = parameters.Length > 1 && bool.Parse(parameters[1]);
+                    try {
+                        var skillIds = parameters[0].Split(',').Select(int.Parse).ToArray();
+                        bool enabled = parameters.Length > 1 && bool.TryParse(parameters[1], out bool result) ? result : false;
+                        
+                        trigger.Context.SetSkill(skillIds, enabled);
+                    } catch (FormatException) {
+                        ctx.Console.Error.WriteLine("Error: Skill IDs must be comma-separated integers");
+                        return;
+                    }
-                    trigger.Context.SetSkill(skillIds, enabled);

135-146: Add error handling for parse failures in spawn_monster

Similar to the set_skill command, this could benefit from error handling for parse failures.

-                    var spawnIds = parameters[0].Split(',').Select(int.Parse).ToArray();
-                    bool spawnAnimation = parameters.Length > 1 && bool.Parse(parameters[1]);
+                    try {
+                        var spawnIds = parameters[0].Split(',').Select(int.Parse).ToArray();
+                        bool spawnAnimation = parameters.Length > 1 && bool.TryParse(parameters[1], out bool result) ? result : false;
+                        
+                        trigger.Context.SpawnMonster(spawnIds, spawnAnimation, 0);
+                    } catch (FormatException) {
+                        ctx.Console.Error.WriteLine("Error: Spawn IDs must be comma-separated integers");
+                        return;
+                    }
-                    trigger.Context.SpawnMonster(spawnIds, spawnAnimation, 0);

146-157: Add error handling for parse failures in move_npc

Add try-catch blocks to handle potential parse errors for the spawn ID.

-                    int spawnId = int.Parse(parameters[0]);
-                    string patrolName = parameters[1];
+                    try {
+                        int spawnId = int.Parse(parameters[0]);
+                        string patrolName = parameters[1];
+                        
+                        trigger.Context.MoveNpc(spawnId, patrolName);
+                    } catch (FormatException) {
+                        ctx.Console.Error.WriteLine("Error: Spawn ID must be an integer");
+                        return;
+                    }
-                    trigger.Context.MoveNpc(spawnId, patrolName);

158-169: Add error handling for parse failures in set_npc_emotion_sequence

Consider adding try-catch blocks to handle potential parse errors for the spawn ID and duration.

-                    int npcSpawnId = int.Parse(parameters[0]);
-                    string sequenceName = parameters[1];
-                    int duration = parameters.Length > 2 ? int.Parse(parameters[2]) : 0;
+                    try {
+                        int npcSpawnId = int.Parse(parameters[0]);
+                        string sequenceName = parameters[1];
+                        int duration = parameters.Length > 2 ? int.Parse(parameters[2]) : 0;
+                        
+                        trigger.Context.SetNpcEmotionSequence(npcSpawnId, sequenceName, duration);
+                    } catch (FormatException) {
+                        ctx.Console.Error.WriteLine("Error: Spawn ID and duration must be integers");
+                        return;
+                    }
-                    trigger.Context.SetNpcEmotionSequence(npcSpawnId, sequenceName, duration);

169-179: Add error handling for parse failures in destroy_monster

Consider adding try-catch blocks to handle potential parse errors for the monster spawn IDs.

-                    int[] monsterSpawnIds = parameters[0].Split(',').Select(int.Parse).ToArray();
+                    try {
+                        int[] monsterSpawnIds = parameters[0].Split(',').Select(int.Parse).ToArray();
+                        
+                        trigger.Context.DestroyMonster(monsterSpawnIds, false);
+                    } catch (FormatException) {
+                        ctx.Console.Error.WriteLine("Error: Monster spawn IDs must be comma-separated integers");
+                        return;
+                    }
-                    trigger.Context.DestroyMonster(monsterSpawnIds, false);
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 02ee9fe and 986d79f.

📒 Files selected for processing (10)
  • Maple2.File.Ingest/Mapper/MapDataMapper.cs (1 hunks)
  • Maple2.Model/Game/Field/FieldAccelerationStructure.cs (6 hunks)
  • Maple2.Model/Metadata/FieldEntity/FieldEntity.cs (1 hunks)
  • Maple2.Server.Game/Commands/DebugCommand.cs (1 hunks)
  • Maple2.Server.Game/Commands/TriggerCommand.cs (2 hunks)
  • Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs (2 hunks)
  • Maple2.Server.Game/Model/Field/Actor/FieldNpc.cs (1 hunks)
  • Maple2.Server.Game/Model/Field/Entity/FieldSkill.cs (2 hunks)
  • Maple2.Server.Game/Trigger/TriggerContext.Field.cs (3 hunks)
  • Maple2.Server.Game/Trigger/TriggerContext.Npc.cs (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
  • Maple2.Server.Game/Trigger/TriggerContext.Npc.cs
  • Maple2.Server.Game/Commands/DebugCommand.cs
  • Maple2.File.Ingest/Mapper/MapDataMapper.cs
  • Maple2.Server.Game/Trigger/TriggerContext.Field.cs
  • Maple2.Server.Game/Model/Field/Entity/FieldSkill.cs
  • Maple2.Model/Metadata/FieldEntity/FieldEntity.cs
  • Maple2.Model/Game/Field/FieldAccelerationStructure.cs
  • Maple2.Server.Game/Model/Field/Actor/FieldNpc.cs
🧰 Additional context used
🧬 Code Definitions (2)
Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs (2)
Maple2.Server.Game/Model/Field/Actor/FieldNpc.cs (1)
  • SkillRecord (355-371)
Maple2.Server.Game/Manager/Field/FieldManager/IField.cs (1)
  • Broadcast (68-68)
Maple2.Server.Game/Commands/TriggerCommand.cs (1)
Maple2.Server.Game/Trigger/TriggerContext.Npc.cs (5)
  • SpawnMonster (22-27)
  • SpawnMonster (47-50)
  • MoveNpc (60-77)
  • SetNpcEmotionSequence (154-159)
  • DestroyMonster (29-45)
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: build
🔇 Additional comments (8)
Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs (2)

14-14: Appropriate imports for the new functionality

The added imports properly support the new vibration feature, with FieldEntity namespace for vibration entities and the Skill namespace for skill records.

Also applies to: 21-21


567-577: Well-implemented vibration effect functionality

The VibrateObjects method effectively implements the vibration feature:

  1. Properly calculates range based on the skill's attack range
  2. Has a good null check for the acceleration structure
  3. Efficiently queries for vibration objects in range
  4. Correctly broadcasts the vibration effect to all players

The implementation aligns well with the PR objectives for enhancing region skills with vibration effects.

Maple2.Server.Game/Commands/TriggerCommand.cs (6)

14-14: Description update reflects expanded functionality

The description now accurately reflects that this command manages triggers rather than just resetting them, which aligns with the new subcommands being introduced.


17-20: Good command structure with clear separation of concerns

The refactoring of functionality into separate subcommands (list, reset, run) improves code organization and follows command-line interface best practices.


22-46: ListCommand implementation is clean and effective

The ListCommand provides a useful way to enumerate and inspect triggers in the current map. The output formatting is clear and includes both trigger names and their associated states.


48-91: ResetCommand provides flexible trigger management

The ResetCommand implementation correctly handles both full trigger resets and setting specific states. Error handling is properly implemented for cases like invalid trigger names or state indices.


93-106: RunCommand setup looks good

The RunCommand class is well structured with appropriate arguments for function name and parameters.


180-182: Good success message upon function execution

The feedback to the user about successful function execution is clear and helpful.

return;
}

List<FieldVibrateEntity> vibrateObjects = AccelerationStructure.QueryVibrateObjectsCenterList(position, 2 * new Vector3(rangeDistance, rangeDistance, rangeDistance));

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Does this take into consideration the shape of the skill?

@AngeloTadeucci
AngeloTadeucci merged commit 9cced40 into master Apr 1, 2025
@AngeloTadeucci
AngeloTadeucci deleted the trigger-skill branch April 1, 2025 23:14
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.

2 participants