Improve task management - #414
Conversation
WalkthroughThis update introduces several improvements across the codebase. A new static method centralizes NPC dummy selection based on gender, and its usage replaces redundant checks. Multiple methods handling NPC tasks and patrol cleanups are refactored to accept a player parameter, while task management logic is refined with enhanced encapsulation and a new helper method. Additionally, the NPC task priority enum is updated to better distinguish emotive actions, and vector transformation logic gains added normalization. Minor syntactical adjustments further streamline the implementation. Changes
Sequence Diagram(s)sequenceDiagram
participant P as Player
participant FM as FieldManager
participant C as Constants
participant MS as MovementState
participant CT as CleanupTask
participant TS as TaskState
P->>FM: Initiates player movement
FM->>C: Call DummyNpc(gender)
C-->>FM: Return dummy NPC ID
FM->>MS: Call CleanupPatrolData(player)
MS->>CT: Create NpcCleanupPatrolDataTask(player)
CT->>TS: Update task state and execute cleanup
Poem
Tip ⚡💬 Agentic Chat (Pro Plan, General Availability)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Maple2.Server.Game/Model/Field/Actor/FieldNpc.cs (1)
173-173: Improved C# style consistency.Changed from
String.Jointostring.Jointo follow C# naming conventions, where the keywordstring(lowercase) is preferred over the class nameString.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
Maple2.Model/Metadata/Constants.cs(1 hunks)Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs(2 hunks)Maple2.Server.Game/Model/Enum/NpcTaskPriority.cs(1 hunks)Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/MovementState.cs(2 hunks)Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/MovementStateTasks/MovementState.CleanupTask.cs(1 hunks)Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/MovementStateTasks/MovementState.EmoteTask.cs(2 hunks)Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/TaskState.cs(6 hunks)Maple2.Server.Game/Model/Field/Actor/FieldNpc.cs(1 hunks)Maple2.Tools/VectorMath/Transform.cs(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
Maple2.Server.Game/Model/Field/Actor/FieldNpc.cs (3)
Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs (1)
BroadcastAiMessage(585-591)Maple2.Server.Game/Manager/Field/FieldManager/IField.cs (1)
BroadcastAiMessage(69-69)Maple2.Server.Game/Packets/CinematicPacket.cs (1)
CinematicPacket(8-195)
Maple2.Tools/VectorMath/Transform.cs (1)
Maple2.Server.Game/Manager/Field/AgentNavigation.cs (5)
Vector3(184-186)Vector3(188-222)Vector3(224-241)Vector3(243-256)Vector3(258-260)
Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/MovementStateTasks/MovementState.CleanupTask.cs (2)
Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/MovementState.cs (10)
MovementState(11-317)MovementState(37-48)NpcTask(50-60)NpcTask(62-73)NpcTask(75-85)NpcTask(87-97)NpcTask(99-102)NpcTask(104-106)NpcTask(121-126)NpcTask(128-130)Maple2.Server.Game/Model/Field/Actor/FieldPlayer.cs (1)
MoveToPosition(514-520)
Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs (3)
Maple2.Model/Metadata/Constants.cs (2)
Constant(10-940)DummyNpc(924-924)Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/MovementState.cs (2)
MovementState(11-317)MovementState(37-48)Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/MovementStateTasks/MovementState.CleanupTask.cs (1)
MovementState(8-52)
Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/MovementStateTasks/MovementState.EmoteTask.cs (5)
Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/TaskState.cs (2)
TaskFinished(146-146)ToString(156-158)Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/MovementStateTasks/MovementState.SkillCastTask.cs (1)
TaskFinished(34-39)Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/MovementStateTasks/MovementState.TalkTask.cs (1)
TaskFinished(19-22)Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/MovementStateTasks/MovementState.WalkTask.cs (3)
TaskFinished(29-32)TaskFinished(72-75)TaskFinished(131-134)Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/MovementState.cs (1)
Idle(140-157)
Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/TaskState.cs (1)
Maple2.Server.Game/Model/Field/Actor/FieldNpc.cs (3)
FieldNpc(22-474)FieldNpc(97-128)Update(138-188)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (23)
Maple2.Tools/VectorMath/Transform.cs (2)
141-142: Vector normalization ensures mathematical correctness.The addition of vector normalization for both the
directionandupparameters is a good practice that ensures these vectors have unit length before being used in calculations. This improves the robustness of the transformation math.
153-153: Recalculating the up vector improves axis orthogonality.Replacing the direct use of the input
upvector with a cross product calculation ensures that all three axes (right, up, and direction) form a proper orthogonal basis. This is mathematically correct for 3D transformations and prevents potential issues that could arise from non-orthogonal axes.Maple2.Server.Game/Model/Enum/NpcTaskPriority.cs (1)
6-7: Well-structured task priority separation for emotes.The separation of
Emoteinto a distinct priority fromIdleActionimproves the task management system by providing better control over when NPCs perform emotive actions versus movement actions like wandering or patrolling.Maple2.Model/Metadata/Constants.cs (1)
924-924: Good abstraction for gender-based NPC selection.This helper method centralizes the dummy NPC selection logic based on gender, reducing code duplication and improving maintainability throughout the codebase. Future gender-related changes will only need to be modified in one place.
Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs (2)
555-555: Good use of the new helper method for dummy NPC selection.Replaced explicit gender check with the new
Constant.DummyNpc()method, improving code readability and maintainability.
567-567:Details
✅ Verification successful
Enhanced patrol cleanup with player context.
Modified
CleanupPatrolDatato accept the player parameter, allowing the method to use player-specific information during the cleanup process. This provides better encapsulation of task management logic.Run the following script to examine how the player parameter is used in the
CleanupPatrolDatamethod:
🏁 Script executed:
#!/bin/bash # Find the definition and usages of CleanupPatrolData to understand how the player parameter is utilized echo "Finding CleanupPatrolData method definition:" rg -A 15 "CleanupPatrolData.*FieldPlayer" --type csharp echo -e "\nFinding all usages of CleanupPatrolData:" rg "\.CleanupPatrolData\(" --type csharpLength of output: 4323
Enhanced Patrol Cleanup Approved
After verification, the changes to theCleanupPatrolDatamethod have been confirmed to work as intended:
- In
Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/MovementState.cs, the method now accepts aFieldPlayerparameter and correctly instantiates theNpcCleanupPatrolDataTaskwith player context.- In
Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs, the call todummyNpc.MovementState.CleanupPatrolData(player)properly passes the player parameter.The modifications enhance encapsulation by ensuring that player-specific data is utilized during patrol cleanup, aligning with the intended task management logic.
Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/MovementState.cs (3)
90-94: Improved task management by checking for existing standby tasksThis addition efficiently prevents duplicate standby tasks by checking if a task with the same priority is already running or pending. This avoids unnecessary task creation and improves the overall task management system.
100-100: Enhanced task prioritization with dedicated Emote priorityChanging from
NpcTaskPriority.IdleActiontoNpcTaskPriority.Emotefor idle emotes creates a clearer distinction between different types of NPC behaviors. This change makes the system more maintainable by better categorizing task types.
128-129: Improved cleanup process with player contextAdding the
playerparameter to theCleanupPatrolDatamethod allows the cleanup task to have player context, which enables more contextual behavior in the actual cleanup process.Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/MovementStateTasks/MovementState.EmoteTask.cs (3)
10-11: Improved encapsulation of propertiesRemoving the default value from
Sequenceand changingIsIdlefrom public to private improves encapsulation. Since these properties are only used within the class and its base class, restricting visibility reduces unnecessary exposure.
13-13: Improved encapsulation of Duration propertyChanging
Durationfrom public to private is a good practice since this property is only used internally by the class.
33-35: Added useful ToString() overrideAdding a ToString() override that includes the sequence name provides helpful debugging information. This is especially useful for identifying tasks in complex scenarios or debugging.
Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/MovementStateTasks/MovementState.CleanupTask.cs (4)
1-3: Added required imports for new functionalityThe addition of
System.NumericsandMaple2.Model.Metadataimports supports the new Vector3 functionality and MS2WayPoint usage in the updated task.
11-12: Enhanced task with player context and position trackingAdding fields for player reference and last position storage enables the improved functionality of the cleanup task, allowing it to work with player positioning and NPC locations.
15-23: Improved constructor to capture last patrol positionThe updated constructor now accepts a player parameter and captures the last waypoint position from the patrol if available. This stored position is later used to determine where to move the player.
26-50: Enhanced cleanup behavior with player interactionThe revised TaskResumed method significantly improves the cleanup behavior by:
- Only removing the NPC if the patrol is null
- Finding the nearest NPC to the player within talking distance
- Making the player look at and move to a contextually relevant position
This creates a more immersive experience by providing continuity for the player after an NPC is removed.
Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/TaskState.cs (7)
7-12: Improved encapsulation and immutabilityMaking the Actor property private and marking the collections as readonly improves encapsulation and ensures these critical components can't be inadvertently changed after initialization.
26-37: Refactored task queue management logicThe condition for overriding a task has been simplified and clarified. The changes make the code more readable and maintain the same functionality.
61-72: Optimized task finishing logicThe task finishing logic has been improved with better conditional checks and clearer flow. This makes the code more maintainable and fixes potential edge cases.
77-84: Enhanced update logic with explicit task cleanupThe update method now explicitly cleans up cancelled tasks before resuming the next task, which ensures there are no lingering cancelled tasks in the queue.
91-94: Added useful HasTask methodThe new HasTask method provides a type-safe way to check for specific tasks in the queue, which enables the improved standby task management in the MovementState class.
97-101: Improved NpcTask property encapsulation and pattern matchingChanging Queue from protected to private and using pattern matching for the IsDone property improves encapsulation and readability.
156-158: Added helpful ToString() overrideThe ToString() override provides useful debugging information, making it easier to understand the state of tasks during development and troubleshooting.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/MovementStateTasks/MovementState.CleanupTask.cs (1)
48-49: Ensure vector normalization is safe from zero-length vectors.The code correctly normalizes the direction vector for the player to look at, but consider adding a safety check for the edge case where positions might be identical.
- player.Transform.LookTo(Vector3.Normalize(closestNpc.Position - lastPosition.Value)); + Vector3 direction = closestNpc.Position - lastPosition.Value; + if (direction != Vector3.Zero) { + player.Transform.LookTo(Vector3.Normalize(direction)); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/MovementStateTasks/MovementState.CleanupTask.cs(1 hunks)Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/TaskState.cs(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/TaskState.cs
🧰 Additional context used
🧬 Code Graph Analysis (1)
Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/MovementStateTasks/MovementState.CleanupTask.cs (2)
Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/MovementState.cs (10)
MovementState(11-317)MovementState(37-48)NpcTask(50-60)NpcTask(62-73)NpcTask(75-85)NpcTask(87-97)NpcTask(99-102)NpcTask(104-106)NpcTask(121-126)NpcTask(128-130)Maple2.Server.Game/Model/Field/Actor/FieldPlayer.cs (1)
MoveToPosition(514-520)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (5)
Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/MovementStateTasks/MovementState.CleanupTask.cs (5)
11-12: Good addition of necessary fields for player tracking and position memory.These fields provide crucial state for the enhanced functionality. The nullable Vector3 type for lastPosition is appropriate since it may not always be available.
15-23: Constructor changes properly incorporate player reference and position tracking.The new parameter and initialization logic properly store the player reference and extract the last waypoint position from the patrol data. The early return when there's no last waypoint is a good defensive programming approach.
26-28: Improved control flow with inverted condition.Inverting the check to return early when patrol exists improves readability and follows the early-return pattern more consistently.
30-34: Clean separation of NPC removal and position-dependent logic.Good practice to first remove the NPC and then perform an early return if the last position isn't available, ensuring clear control flow.
36-46: Well-implemented nearest NPC search logic.The search for the closest NPC is efficiently implemented:
- Uses squared distance calculations to avoid costly square root operations
- Excludes the current actor from the search
- Properly filters based on a maximum distance constant
- Includes an early return if no NPC is found
3 participants
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
These enhancements work together to provide a more engaging and immersive game experience.