KillCommand, Improve Warp and Buff command, Level Up Potion - #407
Conversation
WalkthroughThe changes standardize identifier naming in spawn point classes by replacing uses of Changes
Sequence Diagram(s)sequenceDiagram
participant P as Player
participant BC as BuffCommand
participant BM as BuffManager
participant B as Buff
P->>BC: Issue buff command (buffId, level, stack, duration, all, target, remove)
BC->>BM: Call AddBuff with the provided parameters
BM->>B: Instantiate Buff with duration parameter
B-->>BM: Buff applied and configured
BM-->>BC: Return confirmation
BC-->>P: Acknowledge buff operation completed
sequenceDiagram
participant P as Player
participant KC as KillCommand
participant Field as Game Field
participant NPC as NPC/Mob
P->>KC: Execute kill command (all or near NPCs)
KC->>Field: Validate skill & retrieve target NPCs
Field->>NPC: Apply damage via Kill method (create DamageRecord)
NPC->>Field: Update health and broadcast damage event
KC-->>P: Confirm command execution
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
🪧 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: 2
🧹 Nitpick comments (10)
Maple2.Server.Game/Model/Field/Entity/FieldNpcSpawnPoint.cs (1)
40-40: Consider performance optimization for actor filteringThe current implementation gets all actors by spawn ID and then filters to only include NPCs, which might be inefficient if there are many entities in the field.
- IEnumerable<FieldNpc> npcs = Field.GetActorsBySpawnId(SpawnId).OfType<FieldNpc>().ToList(); // Only get npcs + // Get only NPCs from Npcs and Mobs collections directly + IEnumerable<FieldNpc> npcs = Field.Npcs.Values + .Concat(Field.Mobs.Values) + .Where(npc => npc.SpawnPointId == SpawnId) + .ToList();Maple2.Server.Game/Commands/KillCommand.cs (5)
41-45: Consider extracting the hardcoded skill ID as a constant.The skill ID
10000001is hardcoded directly in the method. This makes it difficult to maintain and understand the purpose of this specific skill.private class KillAllNpcCommand : Command { private readonly GameSession session; + private const int KILL_SKILL_ID = 10000001; + private const int KILL_SKILL_LEVEL = 1; // ... private void Handle(InvocationContext ctx, bool npcOnly, bool mobOnly) { - if (!session.SkillMetadata.TryGet(10000001, 1, out SkillMetadata? skill)) { + if (!session.SkillMetadata.TryGet(KILL_SKILL_ID, KILL_SKILL_LEVEL, out SkillMetadata? skill)) {
47-58: Clarify the condition logic for NPC and mob selection.The current condition
npcOnly == mobOnlymight be confusing. It would be clearer to use explicit conditions for each case.- if (npcOnly == mobOnly) { - // Consider both to be true + if ((npcOnly && mobOnly) || (!npcOnly && !mobOnly)) { + // Either both options are set or neither is set, so kill everything
90-93: Duplicate skill retrieval logic across commands.This method retrieves the same skill using the same hardcoded ID as the
KillAllNpcCommand. Consider refactoring to avoid duplication.Extract the skill retrieval logic into a shared method or use the same constants across both command classes:
private class KillNearNpcCommand : Command { private readonly GameSession session; + private const int KILL_SKILL_ID = 10000001; + private const int KILL_SKILL_LEVEL = 1; // ... private void Handle(InvocationContext ctx, int distance) { // ... - if (!session.SkillMetadata.TryGet(10000001, 1, out SkillMetadata? skill)) { + if (!session.SkillMetadata.TryGet(KILL_SKILL_ID, KILL_SKILL_LEVEL, out SkillMetadata? skill)) {
106-106: Remove unnecessary semicolon.There's an extra semicolon at the end of the if block which serves no purpose.
- }; + }
111-136: Consider adding a check for already dead NPCs.The
Killmethod doesn't check if the NPC is already dead before attempting to kill it, which could lead to unnecessary processing or potential errors.public static void Kill(GameSession session, FieldNpc npc, SkillMetadata skill) { + // Skip if the NPC is already dead + if (npc.IsDead) { + return; + } + var damageRecord = new DamageRecord(skill, skill.Data.Motions[0].Attacks[0]) {Also, the method for generating the
TargetUidcould be improved for better uniqueness:- TargetUid = ((long) Random.Shared.Next(int.MinValue, int.MaxValue) << 32) | (uint) Random.Shared.Next(int.MinValue, int.MaxValue), + TargetUid = ((long) Random.Shared.NextInt64()) & long.MaxValue, // Ensure positive valueMaple2.Server.Game/Manager/QuestManager.cs (2)
514-580: Well-designed new LevelPotion method, but includes a console write statement for debuggingThe new
LevelPotionmethod is well-structured with clear organization for handling epic quests based on player level. The implementation correctly manages quest state transitions and chapter completion without duplicating rewards.However, there's a debugging console write statement at line 524 that should be replaced with proper logging.
- Console.WriteLine($"Chapter {chapterQuests.Key}"); + logger.Debug("Processing Chapter {ChapterId}", chapterQuests.Key);
576-579: TODO comment needs implementationThere's an unimplemented TODO comment about starting and loading fame quests. Consider creating a separate ticket to track this implementation if it's not going to be addressed in this PR.
Would you like me to create a more detailed implementation suggestion for handling fame quests here?
Maple2.Server.Game/PacketHandlers/ItemUseHandler.cs (1)
523-526: Consider better error handling for parameter parsingThe error handling for the
lastClearEpicQuestIdparameter parsing is structured in a confusing way. The nested conditionals make it harder to follow than necessary.- if (xmlParameters.TryGetValue("lastClearEpic", out string? lastEpicQuestId) && !int.TryParse(lastEpicQuestId, out lastClearEpicQuestId)) { - lastClearEpicQuestId = 0; - } + if (xmlParameters.TryGetValue("lastClearEpic", out string? lastEpicQuestId)) { + _ = int.TryParse(lastEpicQuestId, out lastClearEpicQuestId); + }Maple2.Server.Game/Commands/BuffCommand.cs (1)
59-60: Repetitive code pattern for stacking buffsThe code for handling buff stacking is duplicated in three places with very similar logic. Consider extracting this into a helper method to reduce duplication.
+ private void StackBuffAndBroadcast(Buff buff, int stack) { + if (stack > 1) { + buff.Stack(stack); + session.Field?.Broadcast(BuffPacket.Update(buff)); + } + } // Then replace each instance of the stacking logic with a call to this methodAlso applies to: 80-81, 91-92
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
Maple2.Database/Storage/Metadata/MapEntityStorage.cs(1 hunks)Maple2.Database/Storage/Metadata/QuestMetadataStorage.cs(2 hunks)Maple2.File.Ingest/Mapper/MapEntityMapper.cs(1 hunks)Maple2.Model/Enum/Admin.cs(1 hunks)Maple2.Model/Metadata/MapEntity/SpawnPoint.cs(3 hunks)Maple2.Server.Game/Commands/BuffCommand.cs(2 hunks)Maple2.Server.Game/Commands/KillCommand.cs(1 hunks)Maple2.Server.Game/Commands/WarpCommand.cs(1 hunks)Maple2.Server.Game/Manager/Config/BuffManager.cs(2 hunks)Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs(3 hunks)Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs(1 hunks)Maple2.Server.Game/Manager/QuestManager.cs(3 hunks)Maple2.Server.Game/Model/Field/Actor/Actor.cs(1 hunks)Maple2.Server.Game/Model/Field/Buff.cs(2 hunks)Maple2.Server.Game/Model/Field/Entity/FieldNpcSpawnPoint.cs(2 hunks)Maple2.Server.Game/PacketHandlers/ItemUseHandler.cs(4 hunks)
🧰 Additional context used
🧬 Code Definitions (9)
Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs (2)
Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs (1)
FieldSpawnPointNpc(361-370)Maple2.Server.Game/Model/Field/Entity/FieldEntity.cs (1)
Update(25-25)
Maple2.Server.Game/Commands/WarpCommand.cs (4)
Maple2.Server.Game/Commands/ItemCommand.cs (1)
Handle(41-73)Maple2.Server.Game/Commands/NpcCommand.cs (2)
Handle(30-53)Handle(78-118)Maple2.Server.Game/Commands/FindCommand.cs (1)
Handle(49-71)Maple2.Server.Game/Commands/PlayerCommand.cs (12)
Handle(43-68)Handle(83-92)Handle(106-121)Handle(138-185)Handle(197-201)Handle(218-227)Handle(244-271)Handle(282-284)Handle(310-320)Handle(334-356)Handle(379-394)Handle(418-436)
Maple2.Server.Game/Manager/QuestManager.cs (5)
Maple2.Server.Game/Manager/Items/InventoryManager.cs (3)
Item(560-568)Add(149-254)Load(89-100)Maple2.Server.Game/Manager/Items/ItemDropManager.cs (1)
Item(295-317)Maple2.Database/Storage/Game/GameStorage.Quest.cs (3)
GameStorage(10-60)Quest(12-18)Quest(52-58)Maple2.Model/Game/Quest/Quest.cs (2)
Quest(9-47)Quest(20-23)Maple2.Server.Game/Packets/QuestPacket.cs (1)
QuestPacket(13-245)
Maple2.Server.Game/Model/Field/Actor/Actor.cs (1)
Maple2.Server.Game/Manager/Config/BuffManager.cs (1)
AddBuff(61-138)
Maple2.Server.Game/Commands/KillCommand.cs (6)
Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs (2)
FieldNpc(106-126)FieldNpc(128-130)Maple2.Server.Game/Manager/QuestManager.cs (2)
Add(111-117)Update(181-210)Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs (1)
Update(263-290)Maple2.Server.Game/Model/Field/Actor/Actor.cs (1)
Update(199-210)Maple2.Server.Game/Model/Field/Entity/FieldNpcSpawnPoint.cs (1)
Update(27-37)Maple2.Server.Game/Model/Field/Buff.cs (1)
Update(80-109)
Maple2.Server.Game/Model/Field/Entity/FieldNpcSpawnPoint.cs (3)
Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs (3)
Update(263-290)IEnumerable(357-375)IEnumerable(417-421)Maple2.Server.Game/Model/Field/Actor/Actor.cs (1)
Update(199-210)Maple2.Server.Game/Model/Field/Entity/FieldSpawnGroup.cs (1)
Update(123-134)
Maple2.Server.Game/Commands/BuffCommand.cs (4)
Maple2.Database/Storage/Metadata/SkillMetadataStorage.cs (1)
TryGetEffect(51-72)Maple2.Server.Game/Model/Field/Actor/FieldPlayer.cs (3)
FieldPlayer(13-401)FieldPlayer(45-58)Update(99-138)Maple2.Server.Game/Model/Field/Buff.cs (2)
Stack(59-71)Update(80-109)Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs (2)
Broadcast(600-609)Update(263-290)
Maple2.Server.Game/PacketHandlers/ItemUseHandler.cs (3)
Maple2.Server.Game/Manager/QuestManager.cs (1)
LevelPotion(514-579)Maple2.Server.Game/Session/GameSession.cs (4)
GameSession(37-746)GameSession(105-115)GameSession(683-683)ConditionUpdate(511-514)Maple2.Server.Game/Util/XmlParseUtil.cs (2)
Dictionary(6-28)XmlParseUtil(5-29)
Maple2.Server.Game/Model/Field/Buff.cs (2)
Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs (2)
FieldManager(34-647)FieldManager(85-110)Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs (1)
FieldManager(24-822)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (34)
Maple2.Server.Game/Commands/WarpCommand.cs (1)
82-82: The parameter mapping is now correctly implemented.This change properly wires up the
mapIndexoption to theHandlemethod, enabling users to directly warp to a map by its index in the search results. This completes the implementation of the feature that was already defined in the option declaration and handle method signature.Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs (1)
284-284: LGTM: Good enhancement to ensure NPC spawn points are updated correctly.This line adds NPC spawn point updates to the field's update cycle, ensuring that spawn points can trigger their regeneration logic each tick. This aligns with the pattern used for other field entities and properly uses the
Updatemethod implemented inFieldSpawnPointNpc.Maple2.Model/Enum/Admin.cs (1)
68-78: Good improvement on AdminPermissions enum valuesExplicitly defining bitwise values for each permission in the
AdminPermissionsenum is a great improvement. This change:
- Makes the bitwise flag values explicit and clear
- Properly uses powers of 2 for each permission (1, 2, 4, 8, etc.)
- Adds new permissions (StringBoard, EventManagement) with appropriate values
- Maintains compatibility with existing composite permissions
This approach is exactly right for a
[Flags]enum and will make permission checks more maintainable and less error-prone.Maple2.Server.Game/Model/Field/Entity/FieldNpcSpawnPoint.cs (5)
9-9: Consistent identifier naming change approvedChanging from
Value.IdtoValue.SpawnPointIdimproves code clarity and maintains consistency with the standardized identifier naming convention implemented across the codebase.
13-14: LGTM: Added timing mechanism for NPC respawningThe new
checkTickfield properly supports the periodic spawning functionality implemented in theUpdatemethod.
27-37: Well-implemented Update method following the entity update patternThis implementation follows the established pattern used throughout the codebase for entity updates, appropriately checking the active state and timing constraints before triggering respawns. The conversion from seconds to milliseconds is handled correctly.
47-48: Improved spawn count calculation logicThe calculation now correctly determines how many additional NPCs need to be spawned by considering only the NPCs with matching NPC IDs from this specific spawn point.
53-53: Proper entity tracking with SpawnPointIdSetting the
SpawnPointIdproperty on the spawned NPC enables proper tracking and management of NPCs associated with this spawn point.Maple2.File.Ingest/Mapper/MapEntityMapper.cs (2)
105-105: LGTM: Consistent identifier usage for EventSpawnPointNPC constructor.The change adds
npcSpawn.EntityIdas the first parameter to the EventSpawnPointNPC constructor, while keepingnpcSpawn.SpawnPointIDas the second parameter. This aligns with the standardization of identifier naming across spawn point classes.
111-111: LGTM: Consistent identifier usage for SpawnPointNPC constructor.Similar to the EventSpawnPointNPC change, this adds
npcSpawn.EntityIdas the first parameter to the SpawnPointNPC constructor. This maintains consistency with the entity model changes and ensures proper identifier usage throughout the codebase.Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs (3)
41-41: Key change in fieldSpawnPointNpcs dictionary typeThe dictionary type has been changed from using integers to strings as keys. This aligns with the standardization of identifiers across spawn point classes, using
EntityId(string) instead ofId(int).
367-367: Updated dictionary key to use EntityIdCorrectly updated to use
metadata.EntityIdas the key when adding to the dictionary, which is consistent with the dictionary type change in line 41.
382-385: Updated property reference in ToggleNpcSpawnPoint methodThe method now correctly filters spawn points using
SpawnPointIdinstead ofId, which is consistent with the standardization of identifier naming across spawn point classes in this PR.Maple2.Server.Game/Commands/KillCommand.cs (1)
15-26: Good command structure with modular design.The command is well-organized with a clear hierarchy and separation of concerns. The main
KillCommandclass serves as a container for subcommands, which follows good practices for command-line tools.Maple2.Database/Storage/Metadata/MapEntityStorage.cs (1)
78-78: Consistent identifier naming improves code clarityThe changes to use
SpawnPointIdinstead ofIdwhen referencing spawn points in dictionaries align with the updated parameter names in the SpawnPoint class hierarchy. This makes the code more explicit about what type of identifier is being used and maintains consistency across the codebase.Also applies to: 82-82, 88-88
Maple2.Model/Metadata/MapEntity/SpawnPoint.cs (4)
7-7: Improved identifier naming in abstract recordRenaming the parameter from
IdtoSpawnPointIdin the abstract SpawnPoint record improves clarity by making it explicit that this identifier represents a spawn point rather than being a generic ID.
10-10: Consistent parameter naming in SpawnPointPCThe SpawnPointPC record correctly adopts the renamed parameter and updates the base constructor call to match. This maintains consistency with the base class changes.
Also applies to: 15-15
20-21: Enhanced type safety with separate EntityId and SpawnPointId parametersThe changes to SpawnPointNPC and EventSpawnPointNPC make an important distinction between two concepts:
EntityId(string): Identifies the NPC entity itselfSpawnPointId(int): Identifies the spawn point locationThis separation enhances type safety and clarity by explicitly differentiating between these two identifiers, which likely serve different purposes in the game logic. The base constructor calls have been appropriately updated to match these parameter changes.
Also applies to: 30-30, 33-34, 44-44
47-47: Consistent parameter naming in EventSpawnPointItemThe EventSpawnPointItem record correctly adopts the renamed parameter and updates the base constructor call to match. This maintains consistency with the base class changes.
Also applies to: 55-55
Maple2.Server.Game/Model/Field/Buff.cs (3)
41-41: Added parameter for custom buff duration control.The constructor now accepts an
int durationparameter, allowing for specifying a custom duration when creating a buff. This enhances flexibility in buff management.
52-52: Properly passing duration parameter to Stack method.The constructor now passes the custom duration to the Stack method, ensuring the buff's duration is set correctly during initialization.
59-69: Enhanced Stack method with custom duration support.The Stack method has been improved to handle a custom duration parameter. The implementation correctly:
- Accepts a duration parameter with default value of 0
- Only modifies EndTick when duration > 0
- Places this logic after the existing EndTick setting code to ensure explicit duration overrides the default
This change complements the constructor modification and increases the flexibility of the buff system.
Maple2.Server.Game/Model/Field/Actor/Actor.cs (1)
84-84: Improved code readability with named parameters.The method call to
Buffs.AddBuffnow uses a named parameternotifyField: notifyFieldwhich makes the code more readable and less prone to errors when the method signature changes. This is particularly important now that theAddBuffmethod has an additional parameter.Maple2.Database/Storage/Metadata/QuestMetadataStorage.cs (1)
51-63: Added useful method to filter quests by type.The new
GetQuestsByTypemethod enhances the API by allowing clients to retrieve quests filtered by their type. The implementation properly:
- Uses a lock on
Contextto ensure thread safety, consistent with other methods- Handles
QuestType.EpicQuestspecially since it's represented as NULL in the database- Uses appropriate SQL queries with
FromSqlRawto fetch the dataThis is a valuable addition that improves the flexibility of quest retrieval operations.
Maple2.Server.Game/Manager/Config/BuffManager.cs (3)
61-61: Enhanced AddBuff method with duration parameter.The method signature now includes a
durationSecparameter with a default value of -1, allowing clients to specify a custom duration for buffs. This improves the flexibility of the buff system.
67-69: Added defensive duration handling.The added code properly checks if
durationSecis less than 0 and, if so, sets it to the default duration from the buff's metadata. This ensures a valid duration is always used, even when not explicitly provided by the caller.
100-100: Updated Buff instantiation with duration parameter.The Buff constructor call now includes the durationSec parameter, completing the implementation of custom buff durations throughout the system. This change correctly ties together the modifications in both classes.
Maple2.Server.Game/Manager/QuestManager.cs (1)
451-473: Good enhancement to make item rewards optionalThe modification to support skipping item rewards in the
CompleteChaptermethod is a good addition. This makes the method more flexible and reusable, especially when completing chapters in bulk operations.Maple2.Server.Game/PacketHandlers/ItemUseHandler.cs (4)
104-109: Good extension of the item function handlingThe addition of the LevelPotion case in the function handler is well-integrated with the existing structure.
491-491: Method signature improvement - removed unused parameterGood cleanup by removing the unused packet parameter from the HandleDefenseGuard method.
264-264: Using modern C# syntax for empty arraysThe change from
Array.Empty<int>()to[]uses the newer C# collection expression syntax for empty arrays, which is more concise.
517-547: Well-implemented LevelPotion handler with all necessary operationsThe new HandleLevelPotion method properly:
- Parses parameters from the item metadata
- Updates the player's level
- Broadcasts the level-up event
- Calls the Quest.LevelPotion method
- Updates all level-related conditions
- Updates player info across the system
- Consumes the item
This comprehensive approach ensures all systems are updated correctly when a player uses a level potion.
Maple2.Server.Game/Commands/BuffCommand.cs (2)
26-38: Good command enhancements with descriptive optionsThe addition of new command options (duration, all, target, and remove) provides much more flexibility for buff management. The options have clear names and helpful descriptions.
41-95: Enhanced buff handling for multiple targets and removal scenariosThe expanded
Handlemethod implementation now properly manages buff application and removal for multiple scenarios:
- Application to all players in a field
- Application to a specific player by name
- Removal of buffs
- Setting custom duration
The code is well-structured with proper error handling and field broadcasts when needed.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
Maple2.Server.Game/Manager/QuestManager.cs (2)
462-472: Conditional block for item rewards.Wrapping item creation in
if (!ignoreItemRewards)is logically sound. You might consider adding a log or comment clarifying why the item rewards are skipped to avoid confusion during maintenance.
514-579: Review of the newLevelPotionmethod.
- Forcibly completing quests by setting conditions and skipping the standard
Completeflow might omit normal quest rewards (experience, currency, etc.). Verify that this is desired.- Consider replacing the
Console.WriteLinecall with a logger to maintain consistent server logging practices.- Adding dedicated unit tests would help validate that quests are correctly flagged completed and that item rewards are intentionally skipped.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Maple2.Server.Game/Commands/BuffCommand.cs(2 hunks)Maple2.Server.Game/Manager/QuestManager.cs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Maple2.Server.Game/Commands/BuffCommand.cs
🧰 Additional context used
🧬 Code Definitions (1)
Maple2.Server.Game/Manager/QuestManager.cs (2)
Maple2.Server.Game/Manager/Items/InventoryManager.cs (2)
Item(560-568)Add(149-254)Maple2.Server.Game/Manager/Items/ItemDropManager.cs (1)
Item(295-317)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (1)
Maple2.Server.Game/Manager/QuestManager.cs (1)
451-451: Optional parameter added for skipping item rewards.The new
ignoreItemRewards = falseparameter is a sensible approach to allow conditional reward distribution. Ensure all call sites properly handle this parameter, especially when skipping rewards is intentional.
| using GameStorage.Request db = session.GameStorage.Context(); | ||
| bool lastQuestFound = false; | ||
| foreach (IGrouping<int, QuestMetadata> chapterQuests in chapterQuestsGroups.OrderBy(q => q.Key)) { | ||
| Console.WriteLine($"Chapter {chapterQuests.Key}"); |
There was a problem hiding this comment.
| Console.WriteLine($"Chapter {chapterQuests.Key}"); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Maple2.Server.Game/Commands/KillCommand.cs (1)
71-105: Remove extraneous semicolon in KillNearNpcCommand.
Line 102 ends with a semicolon after the closing brace, which is unusual in C# and may be a minor style issue.Apply this diff to remove the trailing semicolon:
- }; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Maple2.Server.Game/Commands/KillCommand.cs(1 hunks)Maple2.Server.Game/Manager/QuestManager.cs(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Maple2.Server.Game/Manager/QuestManager.cs
🧰 Additional context used
🧬 Code Definitions (1)
Maple2.Server.Game/Commands/KillCommand.cs (3)
Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs (2)
FieldNpc(106-126)FieldNpc(128-130)Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs (1)
Update(263-290)Maple2.Server.Game/Model/Field/Actor/Actor.cs (1)
Update(199-210)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: format
- GitHub Check: build
🔇 Additional comments (3)
Maple2.Server.Game/Commands/KillCommand.cs (3)
15-23: Looks great for the primary KillCommand setup.
The constructor properly references subcommands. No issues found here.
24-68: Logic for killing all NPCs or mobs is clear and straightforward.
The code checks for skill availability, handles both NPCs and mobs in various scenarios, and provides user feedback. All looks fine.
107-132:Details
✅ Verification successful
Confirm approach for triggering death logic.
Directly subtracting the full health from NPCs will set them to zero, letting the next update cycle callOnDeath()(per the snippet inActor.cs). This is acceptable, but double-check that any immediate effects or notifications tied to an instant kill are handled correctly. If you need them immediately, consider invoking the normal death processing more explicitly.Would you like me to search other references to confirm that post-kill events (e.g., drop rewards) rely on the update cycle?
🏁 Script executed:
#!/bin/bash # Searching for usage of OnDeath() across the codebase to confirm additional post-kill effects rg "OnDeath" -A 5 -B 5 --type csLength of output: 7951
Confirm NPC death processing via update cycle
After verifying the codebase, subtracting NPC health to zero inKillCommand.cscorrectly defers the death handling to the next update cycle (as seen inActor.cs). This is consistent with how death events are processed for players, NPCs, and pets across the codebase.
- The update cycle in
Actor.cscallsOnDeath()when an actor’s health is zero, matching the design seen inFieldNpc.csand others.- Ensure that any immediate effects (e.g., drop rewards, buff removals) that might rely on an instant kill aren’t adversely affected by this deferred processing. If immediate processing is needed, consider invoking the death handling more explicitly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Maple2.Server.Game/Commands/KillCommand.cs (1)
71-105: Naming nitpick: "KillNearNpcCommand" also affects mobs.For consistency, rename or otherwise clarify that this command also kills mobs within the specified distance.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Maple2.Server.Game/Commands/KillCommand.cs(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
Maple2.Server.Game/Commands/KillCommand.cs (2)
Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs (1)
Update(263-290)Maple2.Server.Game/Model/Field/Actor/Actor.cs (1)
Update(199-210)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (3)
Maple2.Server.Game/Commands/KillCommand.cs (3)
15-23: Looks great!The top-level command initialization is straightforward, and the subcommand structure follows best practices for
System.CommandLine.
107-132: Direct health manipulation duplicates older concerns.As previously noted in older reviews, subtracting health directly (line 128) and relying on the next
Actor.Update()to trigger death can cause partial bypass of immediate death logic. If your design demands immediate cleanup or synchronous death events, explicitly invoking the normal death processing (e.g.,OnDeath()) may be preferable.
24-69:Details
❓ Verification inconclusive
Rename to better reflect its functionality & verify concurrency safety.
- Naming: The class is named
KillAllNpcCommand, but it can also kill mobs when both flags are set or if both flags happen to be the same (line 44). Consider renaming it for clarity:- private class KillAllNpcCommand : Command { + private class KillAllCommand : Command {
- Concurrency: Re-check if
session.Field.Npcs.Valuesandsession.Field.Mobs.Valuescan be safely enumerated without a lock or concurrency-aware collections.Run the following script to see if these dictionaries are thread-safe or protected by locks:
🏁 Script executed:
#!/bin/bash # Checking whether these collections are declared as concurrency-safe or locked rg --context 5 "Dictionary<|ConcurrentDictionary<" --type csLength of output: 71862
Action Required: Rename Command & Verify Thread Safety
Naming: The class is currently named
KillAllNpcCommand, but its functionality includes killing both NPCs and mobs when both flags are identical. Please rename it to something likeKillAllCommandfor better clarity. For example:- private class KillAllNpcCommand : Command { + private class KillAllCommand : Command {Concurrency: Ensure that enumerating over
session.Field.Npcs.Valuesandsession.Field.Mobs.Valuesis safe in a concurrent setting. If these collections are not implemented as thread-safe (e.g., usingConcurrentDictionaryor another concurrency-aware mechanism) or protected by locks, there is a potential risk during enumeration. Please double-check their declarations and, if needed, add the appropriate locking or switch to a thread-safe collection.
2 participants
Summary by CodeRabbit
New Features
Improvements