FieldManager rework & Initial DungeonField implementation - #341
Conversation
WalkthroughThis PR introduces extensive updates across the codebase. Many methods now use a default value and have renamed the identifier from “instanceId” to “roomId” to better reflect room-based logic. New conversion helper methods in game storage and database models streamline the conversion of home and map layouts. Several new types and enums have been added to support dungeon gameplay, and dungeon as well as party management features have been enhanced with new classes, methods, and packet structures. Overall, field management, networking contracts, and UI/debug outputs have been refactored for consistency. Changes
Sequence Diagram(s)sequenceDiagram
participant Session
participant PartyManager
participant DungeonManager
participant FieldManager
Session->>PartyManager: Request SetDungeon(dungeonId, dungeonRoomId)
PartyManager-->>Session: Validate party & leader
PartyManager->>DungeonManager: Invoke SetDungeon(dungeonId, dungeonRoomId)
DungeonManager->>FieldManager: Create/Update dungeon field
FieldManager-->>DungeonManager: Return updated field info
DungeonManager-->>PartyManager: Confirm dungeon setup
PartyManager-->>Session: Broadcast updated dungeon info
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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: 5
🔭 Outside diff range comments (2)
Maple2.Server.DebugGame/Graphics/Ui/Windows/FieldListWindow.cs (1)
81-81: 🛠️ Refactor suggestionUpdate column header to match field property name.
Line 101 has been updated to use
renderer.Field.RoomIdinstead of the previousInstanceId, but the column header on line 81 still displays "Instance Id". This creates an inconsistency between the header and the displayed data.- ImGui.Text("Instance Id"); + ImGui.Text("Room Id");Also applies to: 101-101
Maple2.Server.DebugGame/Graphics/Ui/Windows/WindowListWindow.cs (1)
97-97: 🛠️ Refactor suggestionColumn header inconsistency detected
While the displayed value has been updated to show RoomId, the column header still says "Instance Id", which is inconsistent with the terminology change.
- ImGui.Text("Instance Id"); + ImGui.Text("Room Id");
🧹 Nitpick comments (15)
Maple2.Server.Game/Manager/Field/FieldManager/IField.cs (3)
22-22: Consider removing redundant access modifiers on interface members.
In C#, declaring interface members aspublicis unnecessary because they are implicitly public. Removing thepublickeyword improves readability and aligns with common C# conventions.
60-60: Review the usage ofpublic virtual void Init()in an interface.
Interface default implementations are allowed in recent C# versions, but thevirtualkeyword is often omitted, as a default interface method is distinct from a class-based virtual method. If you need a default interface implementation, consider simply using:public void Init() { /* default implementation here */ }
71-71: Address the pendingTODOcomment regarding room timer logic.
You've left aTODOto “MOVE THIS TO RANDOM ONLY.” If this step is required for correctness or a design requirement, consider fulfilling it now or tracking it promptly to prevent technical debt.Do you want me to open a new issue and draft a revised implementation for
SetRoomTimer?Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.Ugc.cs (1)
63-67: Consider using polymorphism instead of runtime type checking.The introduction of this type check suggests the
CommitPlotmethod has behavior that should be specialized forHomeFieldManager. This pattern can lead to maintenance issues if additional field manager types are added in the future.Consider refactoring to use polymorphism:
- if (this is HomeFieldManager homeField) { - if (session.AccountId == homeField.OwnerId && home.Indoor.MapId == MapId && Plots.TryGetValue(home.Indoor.Number, out Plot? indoorPlot) && !indoorPlot.IsPlanner) { - SavePlot(indoorPlot); - } - }Replace with a virtual method in
FieldManagerthatHomeFieldManagercan override:protected virtual void CommitIndoorPlot(GameSession session, Home home) { // Default implementation is empty } // In HomeFieldManager: protected override void CommitIndoorPlot(GameSession session, Home home) { if (session.AccountId == OwnerId && home.Indoor.MapId == MapId && Plots.TryGetValue(home.Indoor.Number, out Plot? indoorPlot) && !indoorPlot.IsPlanner) { SavePlot(indoorPlot); } } // Then in CommitPlot: private void CommitPlot(GameSession session) { Home home = session.Player.Value.Home; using GameStorage.Request db = GameStorage.Context(); CommitIndoorPlot(session, home); if (home.Outdoor != null && home.Outdoor.MapId == MapId && Plots.TryGetValue(home.Outdoor.Number, out Plot? outdoorPlot)) { SavePlot(outdoorPlot); } // Rest of the method... }Maple2.Server.Game/Manager/Field/PerformanceStageManager.cs (2)
19-21: Pattern matching used to safely access FieldManager-specific properties.The pattern matching check ensures type safety by only proceeding if the
Fieldis actually aFieldManager. However, consider adding a log message or throwing an appropriate exception when the type check fails rather than silently returning.if (Field is not FieldManager fieldManager) { + logger.Warning("EnterExitStage requires a FieldManager implementation but received {FieldType}", Field.GetType().Name); return; }
19-28: Consider extracting magic numbers to constants for better readability.The code contains several magic numbers (101, 802, 803) that would benefit from being extracted into named constants to improve code readability and maintainability.
public class PerformanceStageManager { private readonly ILogger logger = Log.Logger.ForContext<PerformanceStageManager>(); + + // Constants for trigger boxes and portals + private const int STAGE_TRIGGER_BOX_ID = 101; + private const int STAGE_ENTRY_PORTAL_ID = 802; + private const int STAGE_EXIT_PORTAL_ID = 803; private IField Field { get; } // ... public void EnterExitStage(GameSession session) { if (Field is not FieldManager fieldManager) { return; } - fieldManager.TriggerObjects.Boxes.TryGetValue(101, out TriggerBox? triggerBox); + fieldManager.TriggerObjects.Boxes.TryGetValue(STAGE_TRIGGER_BOX_ID, out TriggerBox? triggerBox); if (triggerBox is null) { return; } bool insideStage = triggerBox.Contains(session.Player.Position); - Field.MoveToPortal(session, insideStage ? 802 : 803); + Field.MoveToPortal(session, insideStage ? STAGE_ENTRY_PORTAL_ID : STAGE_EXIT_PORTAL_ID); } }Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.Factory.cs (3)
29-39: Consider if thedungeonscollection is actually used or remove it to declutter.
At the moment,dungeonsis declared but never referenced or populated throughout this file, unlikefieldsorhomes. If it is part of a future plan, consider adding a TODO so it doesn’t appear like dead code. Otherwise, removing it might help keep the class concise.
37-37: Consider a single dictionary for all FieldManager instances.
Currently, the code uses a nestedConcurrentDictionary<int, ConcurrentDictionary<int, FieldManager>> fields. This may be slightly cumbersome when enumerating or disposing fields. Merging into a single dictionary keyed by(mapId, roomId)tuples (or similar) could simplify concurrency and iteration.
241-261: Release resources if the factory is disposed before the loop fully starts.
If the disposal occurs quickly, the thread has a chance of being still inStart(). The current approach forcibly joins the thread (line 260), which is fine, but consider a more robust synchronization approach if concurrency issues arise for partially uninitialized fields.Maple2.Database/Storage/Game/GameStorage.Map.cs (1)
19-19: Added optional parameter for improved API flexibility.Making the
ownerIdparameter optional with a default value of-1improves the API flexibility without breaking existing functionality. The method body already has logic to handle the case whereownerId < 0, making this a safe change.Consider adding a comment or XML documentation to explain the significance of the
-1value and how it affects the behavior of the method. This would improve code readability and help future developers understand the API's behavior.Maple2.Model/Metadata/Table/DungeomRoomTable.cs (1)
1-60: Validate file naming convention.
All records in this file consistently refer to “DungeonRoom”, but the file name contains “DungeomRoomTable.cs”. Consider renaming the file to “DungeonRoomTable.cs” for clarity and consistency.- // Current file name: DungeomRoomTable.cs + // Suggested file name: DungeonRoomTable.csMaple2.File.Ingest/Mapper/TableMapper.cs (2)
16-20: Confirm alias usage clarity.
Using these aliases for enums is valid, but be cautious of potential confusion when referencing the built-inSystem.DayOfWeek. Ensure developers understand that “DayOfWeek” maps specifically to the system enum in this file.
1543-1610: Robust parsing of new DungeonRoom metadata.
TheParseDungeonRoom()implementation is clean, mapping all relevant properties into strongly-typed records. The usage of a localParseDayOfWeekfunction is straightforward. Consider adding unit tests for corner cases (e.g., empty or invalid arrays, extremely large values) to avoid subtle bugs.Maple2.Server.Game/Manager/Field/FieldManager/HomeFieldManager.cs (1)
32-35: Straightforward survey accessors.The
SetHomeSurveyandRemoveHomeSurveymethods are concise and clear. Consider adding logging or validations if the survey data needs cross-verification with other systems.Maple2.Server.Game/Session/GameSession.cs (1)
344-349: Commented-out instance field logic.Since the
InstanceFieldTablelogic is commented out, ensure you keep or remove it intentionally. If no longer needed, removing commented code can reduce clutter.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (54)
Maple2.Database/Storage/Game/GameStorage.Map.cs(1 hunks)Maple2.Database/Storage/Metadata/TableMetadataStorage.cs(3 hunks)Maple2.File.Ingest/Mapper/TableMapper.cs(3 hunks)Maple2.Model/Enum/Dungeon.cs(1 hunks)Maple2.Model/Game/User/Character.cs(1 hunks)Maple2.Model/Game/User/Home.cs(1 hunks)Maple2.Model/Metadata/Table/DungeomRoomTable.cs(1 hunks)Maple2.Model/Metadata/TableMetadata.cs(1 hunks)Maple2.Server.Core/Packets/CharacterListPacket.cs(1 hunks)Maple2.Server.Core/proto/channel/channel.proto(2 hunks)Maple2.Server.Core/proto/world/world.proto(3 hunks)Maple2.Server.DebugGame/Graphics/DebugFieldWindow.cs(1 hunks)Maple2.Server.DebugGame/Graphics/DebugGraphicsContext.cs(1 hunks)Maple2.Server.DebugGame/Graphics/Ui/FieldPropertiesWindow.cs(1 hunks)Maple2.Server.DebugGame/Graphics/Ui/Windows/FieldListWindow.cs(1 hunks)Maple2.Server.DebugGame/Graphics/Ui/Windows/WindowListWindow.cs(1 hunks)Maple2.Server.Game/Commands/FieldCommand.cs(1 hunks)Maple2.Server.Game/Commands/HomeCommands/SurveyCommand.cs(5 hunks)Maple2.Server.Game/GameServer.cs(1 hunks)Maple2.Server.Game/Manager/Field/FieldManager.Factory.cs(0 hunks)Maple2.Server.Game/Manager/Field/FieldManager.Home.cs(0 hunks)Maple2.Server.Game/Manager/Field/FieldManager/DungeonFieldManager.cs(1 hunks)Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.Factory.cs(1 hunks)Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.Home.cs(1 hunks)Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs(8 hunks)Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.Trigger.cs(1 hunks)Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.Ugc.cs(2 hunks)Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs(7 hunks)Maple2.Server.Game/Manager/Field/FieldManager/HomeFieldManager.cs(1 hunks)Maple2.Server.Game/Manager/Field/FieldManager/IField.cs(1 hunks)Maple2.Server.Game/Manager/Field/PerformanceStageManager.cs(2 hunks)Maple2.Server.Game/Manager/HousingManager.cs(2 hunks)Maple2.Server.Game/Manager/MasteryManager.cs(3 hunks)Maple2.Server.Game/Model/Field/Actor/Actor.cs(1 hunks)Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/AiState.cs(1 hunks)Maple2.Server.Game/Model/Field/Actor/FieldNpc.cs(1 hunks)Maple2.Server.Game/Model/Field/Actor/FieldPet.cs(1 hunks)Maple2.Server.Game/Model/Field/Actor/FieldPlayer.cs(0 hunks)Maple2.Server.Game/Model/Field/Entity/FieldEntity.cs(1 hunks)Maple2.Server.Game/Model/Field/Entity/FieldPortal.cs(1 hunks)Maple2.Server.Game/Model/Field/Entity/FieldSkill.cs(1 hunks)Maple2.Server.Game/Model/Field/Entity/IFieldEntity.cs(1 hunks)Maple2.Server.Game/PacketHandlers/GlobalPortalHandler.cs(1 hunks)Maple2.Server.Game/PacketHandlers/HomeActionHandler.cs(2 hunks)Maple2.Server.Game/PacketHandlers/InstrumentHandler.cs(1 hunks)Maple2.Server.Game/PacketHandlers/LoadUgcMapHandler.cs(2 hunks)Maple2.Server.Game/PacketHandlers/MoveFieldHandler.cs(2 hunks)Maple2.Server.Game/PacketHandlers/RequestCubeHandler.cs(0 hunks)Maple2.Server.Game/Packets/FieldPacket.cs(1 hunks)Maple2.Server.Game/Service/ChannelService.TimeEvent.cs(2 hunks)Maple2.Server.Game/Session/GameSession.cs(3 hunks)Maple2.Server.World/Containers/GlobalPortalManager.cs(4 hunks)Maple2.Server.World/Service/WorldService.Migrate.cs(3 hunks)Maple2.Server.World/Service/WorldService.TimeEvent.cs(1 hunks)
💤 Files with no reviewable changes (4)
- Maple2.Server.Game/PacketHandlers/RequestCubeHandler.cs
- Maple2.Server.Game/Model/Field/Actor/FieldPlayer.cs
- Maple2.Server.Game/Manager/Field/FieldManager.Home.cs
- Maple2.Server.Game/Manager/Field/FieldManager.Factory.cs
✅ Files skipped from review due to trivial changes (10)
- Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.Trigger.cs
- Maple2.Server.Game/Model/Field/Entity/FieldSkill.cs
- Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.Home.cs
- Maple2.Server.Game/Model/Field/Actor/Actor.cs
- Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/AiState.cs
- Maple2.Server.Game/Model/Field/Entity/IFieldEntity.cs
- Maple2.Server.Game/Model/Field/Actor/FieldPet.cs
- Maple2.Server.Game/Model/Field/Actor/FieldNpc.cs
- Maple2.Model/Enum/Dungeon.cs
- Maple2.Server.Game/Model/Field/Entity/FieldEntity.cs
🧰 Additional context used
🧠 Learnings (1)
Maple2.Server.Game/PacketHandlers/LoadUgcMapHandler.cs (1)
Learnt from: AngeloTadeucci
PR: AngeloTadeucci/Maple2#229
File: Maple2.Server.Game/PacketHandlers/LoadUgcMapHandler.cs:58-58
Timestamp: 2024-11-12T15:13:52.918Z
Learning: When requested to create a GitHub issue, use the GitHub CLI (gh) to create an issue with a descriptive title and body, including relevant context from the code and pull request.
🔇 Additional comments (95)
Maple2.Server.DebugGame/Graphics/DebugGraphicsContext.cs (1)
319-325: Property rename looks goodThis change correctly updates the property used for comparison from
InstanceIdtoRoomId, consistent with the broader PR objective of standardizing room-based identification throughout the codebase.Maple2.Model/Game/User/Home.cs (1)
75-75: Default passcode change looks goodChanging the default passcode from a placeholder value to an empty string is a good improvement. This makes it clearer that no passcode is set initially rather than having a non-functional placeholder that might confuse users.
Just a quick note: Make sure any code that validates passcodes properly handles empty passcodes, especially if this changes existing behavior.
Maple2.Server.Game/Manager/MasteryManager.cs (3)
9-9: Updated import aligns with the FieldManager refactoringThis change adds an import for the new
Maple2.Server.Game.Model.Roomnamespace, which appears to be part of the broader FieldManager refactoring effort mentioned in the PR title.
136-138: Good refactoring using type checking instead of ID comparisonThis change improves type safety by replacing what was likely a MapId comparison with an explicit type check for
HomeFieldManager. This is more robust as it directly ties the behavior to the specific field type rather than relying on constant values.The ownership check to determine
myHomeis logical - setting it to 1 if the player owns the home field, and 0 otherwise.
172-172: Improved field ownership verificationSimilar to the previous change, this replaces a property-based check with a type check, making the code more maintainable and less prone to errors during future refactoring. The condition now explicitly checks if the field is a
HomeFieldManagerand if the player is not the owner before updating the mastery harvest condition.Maple2.Server.Game/Manager/Field/FieldManager/IField.cs (3)
22-22: Evaluate disposal responsibilities explicitly.
Your interface extendsIDisposablebut does not define how it should be invoked or integrated with its members. Consider documenting expectations (e.g., which resources to release) or providing a default interface method forDispose()if you want a standardized disposal pattern.
23-24: Confirm thread-safety ofDisposedproperty.
Although marking the property asbool Disposed { get; }helps track object lifecycle, ensure multi-threaded scenarios won’t cause race conditions. If multiple threads might check or set this flag, consider adding synchronization or concurrency notes to the interface's documentation.
49-52: ClarifyConcurrentDictionaryusage and concurrency approach.
While the use ofConcurrentDictionarycan help avoid data races, confirm that any operations on Players, Npcs, Mobs, and Pets collections are all accounted for (e.g., iteration vs. mutation) since some interactions can still lead to race conditions if not carefully orchestrated.Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.Ugc.cs (2)
7-7: New import reflects the architectural change from field-based to room-based naming.This change supports the broader refactoring effort to standardize on room-based identification throughout the codebase.
68-68: Ensure consistent formatting with a single blank line.This blank line provides good separation between the indoor and outdoor plot logic blocks.
Maple2.Server.Game/Manager/Field/PerformanceStageManager.cs (4)
2-2: Added the necessary Room namespace for IField.The added import is appropriate given the changes to use the
IFieldinterface, which appears to be defined in the Room namespace.
12-12: Good refactoring to use the IField interface instead of concrete FieldManager.The change from concrete
FieldManagerto the more abstractIFieldinterface follows good design principles by depending on abstractions rather than implementations. This makes the code more flexible and easier to test.
14-14: Constructor signature updated to match the Field property type.The constructor properly accepts an
IFieldparameter, maintaining consistency with the property type change.
22-22: Properly using the cast fieldManager variable.Now correctly uses the pattern-matched
fieldManagervariable to accessTriggerObjects.Boxes.Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs (8)
10-10: Added Room namespace import to support RoomId changes.The addition of this namespace import supports the transition from InstanceId to RoomId, which is part of the broader refactoring in this PR.
60-60: Renamed InstanceId to RoomId for consistency.Changed the property from InstanceId to RoomId, aligning with the broader refactoring in this PR.
160-169: Updated SpawnPortal method to use RoomId instead of InstanceId.The method signature and implementation now use RoomId instead of InstanceId, maintaining consistency with the naming changes throughout the codebase.
462-463: Field creation now uses the FieldFactory.The field creation logic now uses a factory pattern through the FieldFactory, which is a good design improvement for better separation of concerns.
471-472: Updated portal spawning to use RoomId.Changed the SpawnPortal call to use the RoomId property instead of InstanceId, maintaining consistency with the naming changes.
484-486: Made SetRoomTimer method public.Changed the SetRoomTimer method from private to public, allowing it to be called from other classes. This is necessary for the new field management architecture.
24-27:Details
❓ Verification inconclusive
Improved collection accessibility and modernized initialization syntax.
The collections have been changed from internal to public, allowing better access from other components. Additionally, the initialization syntax has been updated to use the more concise C# collection expressions (
[]).While making these collections public improves accessibility, ensure that this doesn't break encapsulation. Consider whether these collections should be exposed as read-only interfaces instead:
🏁 Script executed:
#!/bin/bash # Check for any usage of these collections that modifies them outside the class rg -A 2 -B 2 "\.Players\[(.*?)\]\s*=" --glob "*.cs" | grep -v "FieldManager.cs" rg -A 2 -B 2 "\.Npcs\[(.*?)\]\s*=" --glob "*.cs" | grep -v "FieldManager.cs" rg -A 2 -B 2 "\.Mobs\[(.*?)\]\s*=" --glob "*.cs" | grep -v "FieldManager.cs" rg -A 2 -B 2 "\.Pets\[(.*?)\]\s*=" --glob "*.cs" | grep -v "FieldManager.cs"Length of output: 1264
Review Update: Verify Encapsulation Impact on Public Collections
The updated code now uses modern initialization syntax for public collections in FieldManager. However, the grep results indicate that the
Petscollection is modified externally (e.g., inMaple2.Database/Model/CharacterUnlock.csandMaple2.Server.Game/Manager/Config/PetManager.cs). While this change improves accessibility, it does expose mutable state that is actively used outside the class.
- Action Items:
- Confirm whether the external mutation of the
Petscollection is an intentional design choice.- If encapsulation is a concern, consider refactoring the API—possibly by exposing these collections via read-only interfaces and providing dedicated modification methods—to avoid unintended side effects in external components.
- Review external usages of the other collections (
Players,Npcs,Mobs) to ensure they remain consistent with the intended design, even though no modifications were detected for these in the current grep search.
560-560:Details
❓ Verification inconclusive
Made RemovePlayer virtual to support inheritance.
Added the virtual keyword to the RemovePlayer method, which allows derived classes like HomeFieldManager and DungeonFieldManager to override this behavior with specialized implementations.
It's good practice to check if there are already implementations of this method in derived classes:
🏁 Script executed:
#!/bin/bash # Check for implementations of RemovePlayer in derived classes rg -A 5 "override\s+bool\s+RemovePlayer" --glob "*.cs"Length of output: 56
Action Required: Confirm Derived Class Overrides of RemovePlayer
The change to add the
virtualkeyword to theRemovePlayermethod inMaple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.csis correctly positioned to support inheritance. Automated checks usingrgdid not reveal any overrides ofRemovePlayerin derived classes. However, since the search output was empty and may not capture all variations, please manually verify that none of the derived classes (such asHomeFieldManagerorDungeonFieldManager) override this method unexpectedly.• File:
Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.csat line 560
• Snippet:public virtual bool RemovePlayer(int objectId, [NotNullWhen(true)] out FieldPlayer? fieldPlayer) {Once you confirm that no overriding implementations exist (or that any overrides are intentional), this change can be safely approved.
Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.Factory.cs (5)
46-49: Ensure the disposal thread is cleaned up upon unexpected exceptions.
While this is unlikely, if an exception is thrown before the disposal loop starts, the current design might not terminate the thread properly. Consider a try/catch or thread lifecycle check to mitigate orphan threads.
54-70: Potential logic confusion when returningnullfor non-instance fields.
In lines 55–59, the code callsCreate(mapId)only if it's an instance map butownerIdandroomIdare both zero. If the map is not an instance field or if parameters differ, the method relies onGetInternal. IfGetInternalfails to create a field for some reason, the method returnsnull. Ensure that is the intended behavior for your game’s logic.
117-129: Ensure home creation handles invalid states more robustly.
When no validHomeis found (line 120) and a placeholder is constructed, consider clarifying whether this is truly an error path or normal for first-time housing setups. If it’s an error, raising an exception or logging more details may help debugging.
177-239: Check concurrency for enumerating and disposing fields.
WhileConcurrentDictionaryallows safe concurrent enumeration, modifications during enumeration might lead to partial snapshots of the data. The logic here appears acceptable provided you are comfortable with partial snapshots (some fields might not be processed if added during enumeration). If consistent disposal is critical, consider a locking mechanism or a short-living copy of the dictionary for iteration.
267-282: Consider verifying owners and rooms inGetInternal.
This method callsCreateif no existing map or home field is found. Since it’s a core retrieval path, ensure the parameters (mapId, ownerId, and roomId) are valid. If invalid parameters reach here, the system might create unexpected fields or overwrite others.Maple2.Server.Game/PacketHandlers/GlobalPortalHandler.cs (1)
64-64: Field renaming looks good.The change from
InstanceIdtoRoomIdin the MigrateOutRequest is consistent with the standardization of room-based identification in the codebase.Maple2.Server.Core/Packets/CharacterListPacket.cs (1)
150-150: Consistent field renaming.The change from
InstanceIdtoRoomIdin the character serialization is aligned with the global refactoring approach.Maple2.Server.Game/Commands/FieldCommand.cs (1)
32-32: Console output updated appropriately.The console output now correctly displays the
RoomIdinstead of the previous identifiers, maintaining consistency with the refactoring.Maple2.Model/Metadata/TableMetadata.cs (1)
80-80: Good addition of the new DungeonRoomTable type.The new JsonDerivedType registration for DungeonRoomTable with "dungeonroom" as the type discriminator supports the new dungeon functionality being added in this refactoring.
Make sure there is corresponding implementation of the DungeonRoomTable class and related functionality to support this new table type.
Maple2.Server.DebugGame/Graphics/DebugFieldWindow.cs (1)
154-154: Terminology update confirmed: InstanceId → RoomIdThe window title has been updated to display RoomId instead of InstanceId, which aligns with the broader refactoring effort across the codebase.
Maple2.Server.World/Service/WorldService.TimeEvent.cs (1)
31-31: Consistent terminology change: RoomId replaces InstanceIdThe variable name and object property have been correctly updated to match the new terminology. This change aligns with the broader renaming pattern throughout the codebase.
Also applies to: 35-35
Maple2.Server.DebugGame/Graphics/Ui/Windows/WindowListWindow.cs (1)
113-113: Terminology updated for field displayUpdated to display RoomId instead of InstanceId to align with the codebase-wide refactoring.
Maple2.Server.Game/PacketHandlers/InstrumentHandler.cs (1)
205-205: Updated check to use RoomId instead of InstanceIdCorrectly updated the condition to check if members are in the same room using RoomId instead of InstanceId. This change maintains the logical behavior while using the new identifier system.
Maple2.Server.Game/GameServer.cs (1)
103-104: Parameter renamed for consistencyThe method parameter has been renamed from
instanceIdtoroomId, which aligns with the broader codebase refactoring to standardize on room-based terminology. This is a consistent change that helps unify the conceptual model across the system.Maple2.Server.Game/PacketHandlers/HomeActionHandler.cs (2)
51-53: Improved type safety with pattern matchingAdding the type check with pattern matching for
HomeFieldManagerstrengthens type safety and clarifies method expectations. Early returning when the field isn't of the correct type prevents potential runtime errors.
61-61: Enhanced variable naming clarityUsing the properly typed
homeFieldvariable to accessHomeSurveyimproves code readability by making it explicit that this property comes from aHomeFieldManagerinstance.Maple2.Server.Game/Packets/FieldPacket.cs (1)
296-296: Field reference updated for consistencyUpdated the serialization to use
character.RoomIdinstead of the previousInstanceIdfield, maintaining consistency with the renamed field in the Character class.Maple2.Server.Game/PacketHandlers/MoveFieldHandler.cs (2)
10-10: Added necessary import for HomeFieldManager.The added import for the
Roomnamespace is necessary for the type casting toHomeFieldManagerin the modified condition below.
87-91: Improved type safety with HomeFieldManager check.The conditional check has been refactored to use type checking instead of property access. This change:
- Improves type safety by checking if the field is specifically a
HomeFieldManager- Makes the code more explicit about the field type requirements
- Removes the dependency on specific field properties
This refactoring aligns with the larger architectural changes being made to the field management system.
Maple2.Server.Game/Model/Field/Entity/FieldPortal.cs (1)
15-15:Details
❓ Verification inconclusive
Renamed InstanceId to RoomId for consistency.
This change is part of a broader refactoring effort across the codebase to standardize the naming convention from
InstanceIdtoRoomId. Ensuring consistent naming across the system improves readability and maintainability.Run the following script to confirm that all references to
InstanceIdhave been updated toRoomIdthroughout the codebase:
🏁 Script executed:
#!/bin/bash # Search for any remaining references to InstanceId that might have been missed echo "Checking for remaining references to InstanceId:" rg "InstanceId" --type csharpLength of output: 935
Renaming in FieldPortal.cs Verified – Further Action Needed for Consistency
The change in
Maple2.Server.Game/Model/Field/Entity/FieldPortal.cs(renamingInstanceIdtoRoomId) is correctly applied. However, a repository-wide search still shows multiple remaining references toInstanceIdin several files. If the goal is a complete renaming for consistency across the codebase, please review and update the following files:
- Maple2.Model/Game/FieldInstance.cs (occurrences in the declaration, assignment, and writer calls)
- Maple2.Model/Metadata/ServerTable/RoomRandomTable.cs
- Maple2.Model/Metadata/InstanceFieldTable.cs
- Maple2.Model/Metadata/QuestMetadata.cs
- Maple2.File.Ingest/Mapper/QuestMapper.cs
- Maple2.File.Ingest/Mapper/ServerTableMapper.cs
- Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs
If these changes are planned for a later stage or intentionally excluded, please confirm that this scope is acceptable. Otherwise, updating these references will ensure full consistency across the system.
Maple2.Server.World/Containers/GlobalPortalManager.cs (8)
17-17: Renamed InstanceIds to RoomIds for consistency.This change is part of the broader refactoring to standardize terminology across the codebase.
24-24: Updated constructor to initialize RoomIds array.Correctly updated the constructor to initialize the renamed field.
34-34: Updated for loop to use RoomIds array.Loop iteration variable correctly references the renamed array.
38-38: Updated field name in TimeEventRequest to RoomId.Field name in the request object has been updated to follow the new naming convention.
45-45: Updated assignment to use RoomIds array.Array assignment correctly uses the renamed array.
68-68: Updated TimeEventRequest to use RoomId field.Request parameter correctly uses the renamed field.
80-80: Updated TimeEventRequest GetField to use RoomId.This GetField request also correctly uses the renamed field.
87-87: Updated assignment to use RoomIds array.Final array assignment also correctly uses the renamed array.
Maple2.Server.Game/PacketHandlers/LoadUgcMapHandler.cs (3)
10-10: New import adds clarity to type usage.The addition of the import for
Maple2.Server.Game.Model.Roomis now properly included, clearly indicating the code will work with specialized room types.
39-44: Improved type safety with pattern matching.The refactoring from property checks to type-based checking using pattern matching is a significant improvement. Using
is not HomeFieldManager homeFieldManagerallows for both type checking and variable declaration in a single statement, making the code more robust.
47-47: Property access from the correct specialized type.Correctly accessing
homeFieldManager.OwnerIdinstead of the previoussession.Field.OwnerIdensures type safety and makes the owner ID access explicit to the HomeFieldManager, consistent with the broader refactoring effort.Maple2.Server.Game/Manager/HousingManager.cs (3)
16-16: Added appropriate namespace import.The addition of the import for
Maple2.Server.Game.Model.Roomproperly supports the specialized room type references used in the updated code.
131-133: Enhanced type safety with explicit field type checking.The refactoring from a null check to a specific type check improves code safety and clarity. The pattern matching approach with
is not HomeFieldManager homeFieldelegantly combines type checking and variable declaration.
135-135: Properly scoped property access.Using
homeField.OwnerIdinstead of accessing the property through the base class ensures type safety and aligns with the field management refactoring, making the ownership validation more explicit.Maple2.Database/Storage/Metadata/TableMetadataStorage.cs (3)
65-65: Added new dungeon room table field following established pattern.The new lazy-loaded dungeon room table field is properly defined, consistent with the class's existing pattern for metadata storage.
124-124: Added corresponding public accessor property.The new property properly exposes the dungeon room table using the same pattern as other table properties in the class.
181-181: Properly initialized the new table in constructor.The dungeonRoomTable is correctly initialized using the established Retrieve pattern with the appropriate XML file key, maintaining consistency with how other tables are loaded.
Maple2.Server.Game/Service/ChannelService.TimeEvent.cs (3)
6-6: Added namespace import for room models.The addition of the import for room-specific models ensures proper type references throughout the code.
46-46: Updated field retrieval to use roomId instead of instanceId.The method now correctly uses field.RoomId for field retrieval, consistent with the broader refactoring to standardize room-based identification across the codebase.
54-55: Improved field info with proper type checking.The response now correctly uses RoomId and conditionally sets OwnerId based on the field manager type. This pattern matching approach properly handles the specialized HomeFieldManager case while providing a sensible default for other field types.
Maple2.Server.Core/proto/channel/channel.proto (2)
277-280: Consistent renaming frominstance_idtoroom_idThe field renaming from
instance_idtoroom_idin theGetFieldmessage is part of the broader terminology standardization across the codebase. This change improves naming consistency.
296-301: Consistent renaming frominstance_idtoroom_idThe field renaming from
instance_idtoroom_idin theFieldInfomessage maintains consistency with the changes in other parts of the codebase, ensuring a unified terminology for room-based identification.Maple2.Server.World/Service/WorldService.Migrate.cs (3)
10-11: Consistent renaming fromInstanceIdtoRoomIdin record structThe field renaming in the
TokenEntryrecord struct aligns with the codebase-wide terminology standardization from "instance" to "room".
41-41: Updated parameter name fromInstanceIdtoRoomIdThe parameter name update in the
TokenEntryconstructor call maintains consistency with the renamed field in the record struct.
72-74: Updated property name fromInstanceIdtoRoomIdThe property name update in the
MigrateInResponseensures consistency with the renamed field in theTokenEntrystruct.Maple2.Server.Game/Commands/HomeCommands/SurveyCommand.cs (6)
6-6: Added namespace import for room-related functionalityThe addition of the
Maple2.Server.Game.Model.Roomnamespace import is necessary to support the refactored field management approach.
60-60: Updated field reference with local variableThe code now uses the local variable
field(of typeHomeFieldManager) instead of directly accessingsession.Field, improving type safety and readability.
65-65: Updated field reference with local variableUsing the typed
fieldvariable instead ofsession.Fieldensures type safety and follows the pattern established by the refactoring.
88-88: Updated field reference with local variableUsing the typed
fieldvariable for accessingHomeSurveymaintains consistency with the refactoring pattern.
99-99: Updated field reference with local variableUsing the typed
fieldvariable for accessingHomeSurveymaintains consistency with the refactoring pattern.
107-107: Updated method call with local variableThe code now calls
RemoveHomeSurvey()on the typedfieldvariable instead ofsession.Field, maintaining consistency with the refactoring pattern.Maple2.Server.Game/Manager/Field/FieldManager/DungeonFieldManager.cs (1)
1-18: Well-structured new DungeonFieldManager classThe new
DungeonFieldManagerclass is well-designed, inheriting from the baseFieldManagerand providing specific functionality for dungeon fields. The class includes appropriate properties for metadata access and a concurrent dictionary for managing room fields.A few observations:
- The class properly initializes the
Metadataproperty in the constructor- The
DungeonIdproperty provides a convenient accessor for the metadata ID- The
RoomFieldsdictionary uses the new C# collection initialization syntax[]This implementation properly supports the field management rework by adding specialized functionality for dungeons.
Maple2.Server.Core/proto/world/world.proto (3)
102-102: Confirm usage alignment across all references.
Changinginstance_idtoroom_idis appropriate; confirm that related code (e.g., method parameters, database columns, etc.) is likewise updated.
485-485: Check for complete integration of the renamed field.
The renaming toroom_idapplies well here. Make sure packet serialization and deserialization logic handles the new field name properly.
70-70:Details
❓ Verification inconclusive
Ensure consistency with renamed field.
Renaminginstance_idtoroom_idis clear for standardizing room-based identification, but please verify that all references to the old field name have been updated accordingly throughout the codebase.
🏁 Script executed:
#!/bin/bash # Description: Search for any lingering usage of 'instance_id' in the codebase. rg 'instance_id'Length of output: 16
Manual Verification Needed: Confirm Field Renaming Consistency
It appears that the
instance_idfield was successfully renamed toroom_idin the proto file as intended. However, the automated grep (rg 'instance_id') returned no visible output, which might indicate that there are no lingering references, but the result isn’t wholly conclusive.Please manually verify that no other parts of the codebase still reference
instance_idbefore finalizing this change.Maple2.File.Ingest/Mapper/TableMapper.cs (1)
110-111: Seamless addition of new table metadata.
The addition of"dungeonroom.xml"and its correspondingParseDungeonRoom()method aligns well with the rest of the ingest process. No issues observed here.Maple2.Server.Game/Manager/Field/FieldManager/HomeFieldManager.cs (3)
11-14: Class design looks clean and consistent.The
HomeFieldManagerinheritance fromFieldManageris clear, and the properties (OwnerId,Home,HomeSurvey) are well-defined. No immediate issues with usinginitand private setters to control mutability.
17-20: Validate parameter usage and ownership details.Ensure that the
homeargument passed to the constructor is always valid (e.g., non-null) and that theOwnerIdis indeed meant to behome.AccountId. Consider logging a warning ifhomemight be null or invalid.
22-29: Check concurrency for Plots dictionary assignment.In
Init(), the code loads plots from the database and assigns them toPlots[plot.Number]. Confirm thatPlotsis not accessed by any other threads until initialization completes, or consider making it thread-safe if concurrent access is possible.Maple2.Server.Game/Session/GameSession.cs (6)
126-126: Consistent rename to roomId.Replacing
instanceIdwithroomIdaligns with the new naming convention. This parameter name change looks correct.
183-183: Double-check override logic for roomId.Overwriting
roomIdwithFieldManager.NextGlobalId()whenplotModeis notNormalmay be intentional, but confirm that discardingmigrateResponse.RoomIdwon't cause conflicts if the caller expected to use that specific ID.
186-186: Proper usage of PrepareField with roomId.Invoking
PrepareFieldwithroomIdensures the new field references the correct identifier. This aligns well with the new naming scheme.
330-331: Overload relies on PrepareFieldInternal.This overload properly defers logic down to
PrepareFieldInternal. Verify that default arguments (-1,0) match typical usage patterns, especially forownerIdandroomId.
334-335: Out parameter overload aligns with new signature.The second
PrepareFieldoverload is consistent with the new naming and parameter list, returning the constructedFieldManagerinnewField.
338-339: Method signature updated to support roomId.
PrepareFieldInternaluses the renamed parameterroomIdinstead ofinstanceId. Implementation looks correct.Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs (8)
19-19: New using directive.
using Maple2.Server.Game.Model.Room;introduces references for home or room-based classes. Ensure the namespace structure is consistent with your broader architecture.
31-31: Partial class implementing IField.Declaring
FieldManageras partial and implementingIFieldsuggests a broader design approach. Ensure the partial definitions remain cohesive across files.
56-57: Init-only properties for Entities & Navigation.Using init-only properties enhances immutability. Looks good for ensuring they are set only during construction/initialization.
75-77: New properties: MapId, RoomId, FieldInstance.Declaring them with
initor private setters is a clean approach to preserving read-only usage. TheFieldInstanceproperty’s private setter is useful for internal state management.
93-93: Unique RoomId assignment.Generating
RoomIdviaNextGlobalId()ensures uniqueness. Confirm that theInterlocked.Incrementlogic used byNextGlobalId()does not overlap with other ID counters to avoid collisions.
95-95: Default FieldInstance initialization.Setting
FieldInstancetoFieldInstance.Defaultduring construction is straightforward. Verify if further customization is needed for special field types.
107-107: Public virtual Init method.Exposing
Initallows derived classes (likeHomeFieldManager) to override base initialization. This expansion of scope seems intentional and consistent with usage.
126-126: Loading all plots ignoring ownerId.If different owners or specialized field managers need user-specific plots, ensure
HomeFieldManageroverrides this correctly, as the base code here loads all plots forMapId.
Zintixx
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.Factory.cs (1)
112-118:⚠️ Potential issueDispose the overwritten field to avoid resource leaks.
This code overwrites any existing(mapId, roomId)entries without disposing them first, potentially causing leaks or concurrency issues.** from previous review
To fix, dispose the old field when overwriting:
fields.AddOrUpdate( mapId, _ => new ConcurrentDictionary<int, FieldManager>([new KeyValuePair<int, FieldManager>(roomId, field)]), (_, existingOwnerFields) => { existingOwnerFields.AddOrUpdate(roomId, field, (_, oldField) => { + oldField.Dispose(); return field; }); return existingOwnerFields; });
🧹 Nitpick comments (20)
Maple2.Server.Game/Trigger/TriggerContext.Dungeon.cs (3)
109-120: Consider adding a null check for Field property.The method has been refactored to properly check if the field is a dungeon field and handles party presence correctly. The logic is sound, but missing a defensive check.
public bool CheckDungeonLobbyUserCount() { + if (Field is null) { + DebugLog("[CheckDungeonLobbyUserCount] Field is null"); + return false; + } + if (Field is not DungeonFieldManager dungeonField) { return false; } if (dungeonField.Party is null) { return Field.Players.Values.Count >= 1; } DebugLog("[CheckDungeonLobbyUserCount]"); return dungeonField.Party.Members.Count == Field.Players.Values.Count; }
118-119: Enhance debug log with relevant party and player counts.The current debug log provides minimal information. Adding the actual counts would make debugging easier when reviewing logs.
-DebugLog("[CheckDungeonLobbyUserCount]"); +DebugLog("[CheckDungeonLobbyUserCount] PartyMemberCount: {0}, FieldPlayerCount: {1}", + dungeonField.Party.Members.Count, Field.Players.Values.Count);
127-137: Consider adding a null check for Field property and enhance debug log.Similar to the previous method, this implementation needs a defensive null check for the Field property and could benefit from more detailed logging.
public bool IsDungeonRoom() { + if (Field is null) { + DebugLog("[IsDungeonRoom] Field is null"); + return false; + } + if (Field is not DungeonFieldManager dungeonField) { return false; } if (dungeonField.Party is not null) { return true; } - DebugLog("[IsDungeonRoom]"); + DebugLog("[IsDungeonRoom] No party found in dungeon field"); return false; }Maple2.Server.Game/Manager/Field/FieldManager/DungeonFieldManager.cs (4)
2-2: Consider removing unused import.The
System.Diagnostics.CodeAnalysisimport doesn't appear to be used within this class.-using System.Diagnostics.CodeAnalysis;
10-17: Add XML documentation comments to improve code clarity.Adding XML documentation comments for the class and its public properties would enhance code readability and maintainability, particularly since this is a new component in the system.
+/// <summary> +/// Manages dungeon field instances and their associated properties. +/// </summary> public class DungeonFieldManager : FieldManager { + /// <summary> + /// The metadata containing information about this dungeon room. + /// </summary> public DungeonRoomTable.DungeonRoomMetadata Metadata { get; private set; } + /// <summary> + /// The unique identifier for this dungeon. + /// </summary> public int DungeonId => Metadata.Id; + /// <summary> + /// Reference to the lobby field of this dungeon, or this instance if it is the lobby. + /// </summary> public DungeonFieldManager? Lobby { get; init; } + /// <summary> + /// Collection of all room fields associated with this dungeon. + /// </summary> public readonly ConcurrentDictionary<int, DungeonFieldManager> RoomFields = []; + /// <summary> + /// The party associated with this dungeon instance, if any. + /// </summary> public Party? Party { get; init; }
15-15: Consider initializing with capacity when expected size is known.For better performance, you might want to initialize the ConcurrentDictionary with an estimated capacity if you have a reasonable expectation of how many rooms might be created.
- public readonly ConcurrentDictionary<int, DungeonFieldManager> RoomFields = []; + public readonly ConcurrentDictionary<int, DungeonFieldManager> RoomFields = new(initialCapacity: 4);
19-26: Consider adding a check for null metadata parameters.While C# has nullable reference types, it's still a good practice to validate critical constructor parameters.
public DungeonFieldManager(DungeonRoomTable.DungeonRoomMetadata metadata, MapMetadata mapMetadata, UgcMapMetadata ugcMetadata, MapEntityMetadata entities, NpcMetadataStorage npcMetadata, long ownerId = 0, Party? party = null) : base(mapMetadata, ugcMetadata, entities, npcMetadata, ownerId) { + ArgumentNullException.ThrowIfNull(metadata, nameof(metadata)); + ArgumentNullException.ThrowIfNull(mapMetadata, nameof(mapMetadata)); Party = party; Metadata = metadata; if (metadata.LobbyFieldId == mapMetadata.Id) { Lobby = this; } }Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs (1)
414-465: Handle empty Plots, address TODO, and consider fallback logic.
- Multiple lines rely on
Plots.First(), which could raise exceptions ifPlotsis empty. Confirm thatPlotscan never be empty for these portal operations.- There's a
//TODO: Migrate back to original channelplaceholder underPortalType.LeaveDungeon.Would you like help implementing the migration logic or adding guard checks for empty
Plots?Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.Factory.cs (2)
192-236: Review concurrency approach in the DisposeLoop.
WhileConcurrentDictionarymethods are atomic, there’s no locking around removal. Concurrently creating or retrieving a field while one is being removed may cause subtle races or partial reads. Verify that this approach is acceptable or consider using the samemapLockhere.
350-351: TODO indicates an unhandled case.
Line 350–351 has a “TODO: this should never happen.” If this code path is truly invalid, consider throwing an exception here for better detection, or implement the necessary logic/function call forDungeonLobby.Maple2.Model/Error/DungeonRoomError.cs (4)
24-24: Typo in enum value name.The enum value name contains a typo - "commingSoon" should be "comingSoon".
- s_room_dungeon_commingSoon = 9, + s_room_dungeon_comingSoon = 9,
47-48: Duplicate error descriptions for different error codes.Error codes 28 and 29 have identical descriptions but different error codes. Consider adding more specific information to differentiate between these error conditions or consolidate them if they represent the same logical error.
- [Description("This dungeon is not available yet. \\nCheck the requirements in the Dungeon Info menu.")] - s_room_dungeon_is_not_open_period_date = 28, - [Description("This dungeon is not available yet. \\nCheck the requirements in the Dungeon Info menu.")] - s_room_dungeon_is_not_open_period = 29, + [Description("This dungeon is not available yet due to date restrictions. \\nCheck the requirements in the Dungeon Info menu.")] + s_room_dungeon_is_not_open_period_date = 28, + [Description("This dungeon is not available yet due to general period restrictions. \\nCheck the requirements in the Dungeon Info menu.")] + s_room_dungeon_is_not_open_period = 29,Also applies to: 61-64
67-72: Typos in enum value names.The enum value names for items 32 and 33 contain typos - "reawrd" should be "reward".
- s_room_dungeon_error_still_have_united_reawrd = 32, - s_room_dungeon_error_shutdown_united_reawrd_reset = 33, + s_room_dungeon_error_still_have_united_reward = 32, + s_room_dungeon_error_shutdown_united_reward_reset = 33,
7-73: Consider adding documentation comments for the enum and/or grouping related errors.This enum contains many entries covering various aspects of dungeon room errors. Consider adding summary documentation for the enum itself and potentially organizing related errors into regions or separate enums for better maintainability.
Here's an example of how you might add documentation:
+/// <summary> +/// Defines error codes related to dungeon room entry and management. +/// These codes are used in the DungeonRoomPacket to communicate errors to clients. +/// </summary> public enum DungeonRoomError {And potentially group related errors with comments:
public enum DungeonRoomError { none = 0, + // Party-related errors [Description("The party leader must enter first.")] s_room_party_err_not_chief = 1, // ... + // Time and availability errors [Description("This dungeon has expired. Entry is no longer possible.")] s_room_dungeon_expired = 8, // ...Maple2.Server.Game/Service/ChannelService.Party.cs (1)
216-230: Consider adding error response handling for improved robustnessThe implementation follows the pattern of other methods in this file, but lacks error handling beyond session and party verification. Consider enhancing the error handling to return appropriate error responses in case of issues.
private PartyResponse SetDungeon(IEnumerable<long> receiverIds, PartyRequest.Types.SetDungeon setDungeon) { + bool anySuccessful = false; foreach (long characterId in receiverIds) { if (!server.GetSession(characterId, out GameSession? session)) { continue; } if (session.Party.Party?.Id != setDungeon.PartyId) { continue; } session.Party.SetDungeon(setDungeon.DungeonId, setDungeon.DungeonRoomId, setDungeon.Set); + anySuccessful = true; } + if (!anySuccessful) { + return new PartyResponse { Error = (int)PartyError.s_party_err_not_found }; + } return new PartyResponse(); }Maple2.Server.World/Service/WorldService.Party.cs (1)
229-237: Consider handling the return value from SetDungeon.The method doesn't check the error returned by
manager.SetDungeon(), even though it could return aPartyErrorvalue. Consider propagating this error to the caller by updating the code to include the error in the response.private PartyResponse SetDungeon(long requestorId, PartyRequest.Types.SetDungeon setDungeon) { if (!partyLookup.TryGet(setDungeon.PartyId, out PartyManager? manager)) { return new PartyResponse { Error = (int) PartyError.s_party_err_not_found }; } - manager.SetDungeon(requestorId, setDungeon.DungeonId, setDungeon.DungeonRoomId); + PartyError error = manager.SetDungeon(requestorId, setDungeon.DungeonId, setDungeon.DungeonRoomId); + return new PartyResponse { Error = (int) error }; - return new PartyResponse(); }Maple2.Server.Game/PacketHandlers/DungeonRoomHandler.cs (2)
45-47: Implement or remove the emptyHandleResetmethod.
This method is currently empty, indicating that either the feature is not yet implemented or no longer required. If you plan to add reset logic in the future, consider adding a TODO or placeholder comment explaining the intention. Otherwise, removing unused methods can help keep the codebase clean.
48-53: Clarify the usage ofunknownandunknown2.
The variablesunknownandunknown2are read from the packet but are never used, which can be confusing. Remove them if not necessary, or document their purpose if they are reserved for future implementation.Maple2.Server.Game/Manager/DungeonManager.cs (2)
27-34: UnusedCancellationTokenSource.
ThetokenSourceis created but not consumed, suggesting incomplete or abandoned cancellation logic. To improve maintainability, consider implementing proper cancellation flows or removing this token source if it’s irrelevant.
83-84: Revisit auto-migration design.
MigrateToDungeon()is invoked immediately after creating the dungeon. If the user expects to be prompted, or if additional setup is needed prior to migration, consider introducing an explicit user flow rather than auto-migrating in every scenario.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
Maple2.Model/Error/DungeonRoomError.cs(1 hunks)Maple2.Model/Game/Party/Party.cs(1 hunks)Maple2.Server.Core/proto/channel/channel.proto(4 hunks)Maple2.Server.Core/proto/world/world.proto(6 hunks)Maple2.Server.DebugGame/Graphics/Ui/FieldPropertiesWindow.cs(1 hunks)Maple2.Server.Game/Manager/DungeonManager.cs(1 hunks)Maple2.Server.Game/Manager/Field/AiManager.cs(1 hunks)Maple2.Server.Game/Manager/Field/FieldManager/DungeonFieldManager.cs(1 hunks)Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.Factory.cs(1 hunks)Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs(9 hunks)Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs(9 hunks)Maple2.Server.Game/Manager/FishingManager.cs(1 hunks)Maple2.Server.Game/Manager/PartyManager.cs(2 hunks)Maple2.Server.Game/PacketHandlers/DungeonRoomHandler.cs(1 hunks)Maple2.Server.Game/PacketHandlers/MoveFieldHandler.cs(2 hunks)Maple2.Server.Game/Packets/DungeonRoomPacket.cs(1 hunks)Maple2.Server.Game/Packets/PartyPacket.cs(5 hunks)Maple2.Server.Game/Service/ChannelService.Party.cs(2 hunks)Maple2.Server.Game/Session/GameSession.cs(7 hunks)Maple2.Server.Game/Trigger/TriggerContext.Dungeon.cs(3 hunks)Maple2.Server.World/Containers/PartyManager.cs(2 hunks)Maple2.Server.World/Service/WorldService.Party.cs(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- Maple2.Server.Game/Manager/Field/AiManager.cs
🚧 Files skipped from review as they are similar to previous changes (4)
- Maple2.Server.Game/PacketHandlers/MoveFieldHandler.cs
- Maple2.Server.DebugGame/Graphics/Ui/FieldPropertiesWindow.cs
- Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs
- Maple2.Server.Game/Session/GameSession.cs
🔇 Additional comments (40)
Maple2.Server.Game/Trigger/TriggerContext.Dungeon.cs (2)
1-1: Appropriate using directive added.The addition of the correct namespace import for the
DungeonFieldManagertype used in the refactored methods below.
109-137: Good implementation of type-safe field type checking.The refactored methods correctly use pattern matching with the
isoperator to check if the field is of the expected type while also capturing it in a variable for further use. This is a modern and idiomatic approach in C#.Maple2.Model/Game/Party/Party.cs (2)
26-26:Details
✅ Verification successful
Property aligns with the PR's purpose, but verify its usage across the codebase.
The addition of this property supports the PR's objective of reworking field management, specifically for dungeon functionality. It complements the existing
DungeonIdproperty and provides a way to track the lobby room for dungeons.
🏁 Script executed:
#!/bin/bash # Check for places where DungeonLobbyRoomId is being set or used # to ensure proper integration with the rest of the codebase echo "Files setting DungeonLobbyRoomId:" rg "DungeonLobbyRoomId\s*=" --type cs -A 2 -B 2 echo -e "\nFiles accessing DungeonLobbyRoomId:" rg "\.DungeonLobbyRoomId" --type cs -A 2 -B 2 echo -e "\nLooking for PartyManager.SetDungeon method mentioned in the summary:" rg "SetDungeon" --type cs -A 10 -B 2Length of output: 19183
DungeonLobbyRoomId usage verified across the codebase
The property addition aligns perfectly with the PR’s objectives. Verification confirms that:
- Setting in PartyManager: The property is correctly set in files like
Maple2.Server.Game/Manager/PartyManager.csandMaple2.Server.World/Containers/PartyManager.cs.- Access in DungeonManager: Its value is consistently used to retrieve the proper lobby room (e.g., via
FieldFactory.GetinDungeonManager.cs).- Integration with SetDungeon: The usage in
SetDungeonmethods across the codebase demonstrates that the new property meshes well with the existing field-management logic.No further changes are needed.
26-26:Details
❓ Verification inconclusive
Verify if any related UI components need updating.
Since this property represents a new state for parties in relation to dungeons, ensure any UI components or client notifications relating to party state are updated to reflect this new information if needed.
🏁 Script executed:
#!/bin/bash # Look for UI components or packet handlers related to party dungeons # that might need to be updated echo "Checking for party-related UI components:" rg "Party.*UI|UI.*Party" --type cs -A 2 -B 2 echo -e "\nChecking for party-related packet handlers:" rg "Party.*Handler|Handle.*Party" --type cs -A 2 -B 2 echo -e "\nChecking for dungeon lobby related code:" rg "DungeonLobby|LobbyRoom" --type cs -A 2 -B 2Length of output: 12334
Verify Client-Side Handling of the New Dungeon Lobby State
The updated
DungeonLobbyRoomIdproperty is now integrated on the server side (e.g., inPartyManager,DungeonManager, andWorldService.Party.cs), ensuring that party information broadcasts include the new dungeon lobby state. However, our searches did not reveal any explicit UI components referencing this property. Please double-check any client notifications or UI layers that render party state to confirm they correctly interpret and display the new dungeon lobby information if applicable.Maple2.Server.Game/Manager/Field/FieldManager/DungeonFieldManager.cs (1)
23-25:Details
❓ Verification inconclusive
Clarify lobby initialization logic.
The code sets
Lobby = thiswhen the current field is a lobby, but doesn't address howLobbyis set when this isn't the case. Consider adding a comment explaining this behavior or implementing additional initialization logic.Is there a separate process that sets the Lobby property for non-lobby instances? If not, you might need to handle this scenario:
🏁 Script executed:
#!/bin/bash # Search for any code that sets the Lobby property on DungeonFieldManager rg "DungeonFieldManager.*Lobby\s*=" --type csLength of output: 47
Verify Lobby Property Initialization Logic
It appears that the only assignment in
DungeonFieldManager.csis the one at lines 23–25 whereLobbyis set tothiswhen the current field qualifies as a lobby. No additional assignments for non-lobby instances were found in our repository search. Could you please manually verify that this behavior is intentional? If non-lobby instances require explicit initialization ofLobby, consider adding either the missing logic or an explanatory comment clarifying why no further assignment is needed.Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs (7)
19-19: No concerns with the new using statement.
31-31: Clarify disposal and interface usage.
The class now implementsIFieldyet still has its ownDispose()method. If you intend to keep explicit disposal functionality, consider havingIFieldextendIDisposableor explicitly re-implementIDisposablewithinFieldManagerto make the ownership of disposal logic clear.
56-57: Immutability enhancements look great.
Switching from readonly fields toinitproperties forEntitiesandNavigationis a solid move towards safer, more flexible initialization.
75-77: Good use of init-only properties.
UsinginitensuresMapId,RoomId, andFieldInstance(private setter) remain controlled post-construction. This improves encapsulation.
107-107: Making Init() public virtual is beneficial for extensibility.
This change allows derived classes (e.g. dungeon-specific managers) to override initialization logic for specialized behavior.
121-121: Verify usage of instance-based IDs.
Despite the shift fromInstanceIdtoRoomId, this line still referencesinstanceField.InstanceId. Confirm if this remains necessary or if it should be replaced with room-based logic to avoid confusion.
126-127: Ensure Plots dictionary and subsequent lookups are safe.
Loading plots for a map unconditionally is fine, but your code later callsPlots.First(). Verify thatPlotsis never empty or null in scenario edges, or handle an empty dictionary gracefully.Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.Factory.cs (4)
60-71: Optionally switch to asynchronous locking or confirm synchronous usage.
This method uses synchronousmapLock.Wait(). If other code paths are asynchronous, switching toawait mapLock.WaitAsync()will help prevent blocking. If the entire approach is synchronous by design, it’s fine, but be consistent throughout your codebase.
128-128: Check for potential PII logging.
LoggingOwnerIdcan reveal identifying information. If player IDs are considered sensitive, evaluate whether to obfuscate or remove them from logs to comply with privacy requirements.
178-180: Good job clarifying sub-dungeon disposal.
Storing subfields insubDungeonFieldsand removing them in the disposal loop addresses the earlier concern about leftover resources. This approach ensures a clear disposal path and avoids potential memory leaks.
248-253: Confirm home fields are also locked or concurrency-safe during disposal.
Home fields are disposed similarly to standard fields, but there’s no direct locking aroundhomes. If other threads manipulate home fields, ensure it remains thread-safe or adopt a consistent locking pattern.Maple2.Model/Error/DungeonRoomError.cs (6)
1-6: ReSharper suppression is appropriate for the naming conventions used.The directive suppresses warnings for inconsistent naming and identifier typos, which is necessary given the snake_case naming convention used for the enum values that follow.
7-9: Enum definition follows typical C# practices.The enum starts with a
none = 0value, which is good practice for representing the absence of an error condition.
39-42:Details
❓ Verification inconclusive
Ensure placeholders in descriptions are correctly handled.
Several error messages contain format placeholders like
{0}. Verify that the code responsible for displaying these error messages correctly provides values for these placeholders.
🏁 Script executed:
#!/bin/bash # Search for code that uses these error enum values with descriptions containing placeholders rg "OverMaxUserCount|UnderMinUserCount" --type cs -A 5 -B 5 # Look for the pattern of handling Description attributes in the codebase rg "GetDescription|Description.*Attribute" --type csLength of output: 1961
Action Required: Verify Error Message Placeholder Substitution
- Error enums in Maple2.Model/Error/DungeonRoomError.cs (lines 39–42) continue to use format placeholders (e.g.,
{0}) as seen ins_room_dungeon_OverMaxUserCountands_room_dungeon_UnderMinUserCount.- The grep results confirm that similar placeholder usage appears consistently in other error definitions (e.g., in ChangeAttributesError.cs).
- Please ensure that all call sites (likely using methods like
GetDescriptionor equivalent formatting utilities) correctly supply runtime values to replace these placeholders.
33-34:Details
❓ Verification inconclusive
Verify the map ID reference in the error message.
The error message contains a reference to a map ID (
$map:63000063$). Ensure this ID is correct and that the message formatting system correctly replaces this token with the appropriate map name.
🏁 Script executed:
#!/bin/bash # Check for references to this map ID in the codebase rg "63000063" --type cs # Check for other similar map reference patterns rg "\$map:[0-9]+\$" --type csLength of output: 145
Map ID Reference Review: Manual Verification Required
The token
$map:63000063$inMaple2.Model/Error/DungeonRoomError.csat lines 33–34 is confirmed to exist. However, the automatic search did not reveal additional context regarding token formatting or cross-references. Please manually verify that:
- The map ID
63000063is indeed the correct identifier intended for use.- The message formatting system correctly interprets and replaces the token with the appropriate map name at runtime.
Review any related configuration files or mapping logic to ensure consistency across the codebase.
15-19:Details
❓ Verification inconclusive
Missing sequential value in enum ordering.
I notice that value 4 is skipped in the sequence (goes from 3 to 5). This might be intentional, but it's worth verifying if there should be an error code with value 4.
🏁 Script executed:
#!/bin/bash # Check if there are any references to a DungeonRoomError with value 4 in the codebase rg -i "DungeonRoomError.*4" --type cs # Or check for any other enum in the codebase with the same pattern to see if this is intentional rg -i "enum.*Error.*=.*4" --type csLength of output: 77
Attention: Confirm Sequential Enum Values
The review comment noted that the enum ordering skips from value 3 to 5, leaving value 4 unassigned. Our initial search for references to an error code with value 4 produced no output, which makes it unclear whether this omission was intentional or if an error code is missing.
Please manually verify that:
- The sequential ordering in
Maple2.Model/Error/DungeonRoomError.csis intentional.- No internal dependencies expect an error code with the value 4.
🏁 Script executed:
[run_scripts] #!/bin/bash # List all enum values with their numerical assignments from DungeonRoomError.cs rg -n "s_[A-Za-z0-9_]+\s*=\s*[0-9]+" Maple2.Model/Error/DungeonRoomError.csLength of output: 1734
Enum Sequential Mismatch – Verify if Intentional
The enum inMaple2.Model/Error/DungeonRoomError.csjumps from value 3 to 5 (i.e.s_room_dungeon_error_invalidPartyOID = 3ands_room_dungeon_reward_CantUseExtraReward = 5), leaving no entry for value 4. Please confirm whether this gap is intentional. If an error code corresponding to value 4 was mistakenly removed, consider restoring it or revising the numbering to avoid potential confusion.
- Location:
Maple2.Model/Error/DungeonRoomError.cs- Details:
- Value 3:
s_room_dungeon_error_invalidPartyOID- Value 5:
s_room_dungeon_reward_CantUseExtraReward- Missing: Value 4
29-30:Details
❓ Verification inconclusive
Ensure proper handling of escaped newlines in descriptions.
The description contains a double backslash before newline (
\\n). If you want an actual newline in the displayed message, a single backslash should be used (\n). If you want to display the literal\nto the user, then the double backslash is correct.
🏁 Script executed:
#!/bin/bash # Check how these descriptions are used in the codebase rg "\\\\n" --type cs --include "*.cs" -A 2 -B 2Length of output: 489
🏁 Script executed:
#!/bin/bash # Corrected command using --type flag to restrict search to C# files. rg --type cs '\\\\n' -A 2 -B 2Length of output: 4767
Attention: Verify newline escapes in error descriptions!
The error description inMaple2.Model/Error/DungeonRoomError.cs(lines 29–30) currently uses a double backslash (\\n):[Description("No entry allowed today.\\nPlease check the entry conditions in the dungeon information.")] s_room_dungeon_canEnterDayOfWeeks = 12,A similar pattern appears in several other error files (e.g.,
WeddingError.cs). This setup means that the string literal will contain the two characters\andn, rather than an actual newline character.
If the intention is to display a newline in the output, the escape should be changed to a single backslash (
\n):[Description("No entry allowed today.\nPlease check the entry conditions in the dungeon information.")] s_room_dungeon_canEnterDayOfWeeks = 12,If the literal
\nis meant to be shown, then no change is necessary.Please review the intended display behavior and update accordingly.
Maple2.Server.Game/Manager/FishingManager.cs (1)
311-313: Good addition of prize fish counter tracking!This change properly increments the
TotalPrizeFishcounter when a player catches a fish that qualifies as a prize fish (when the fish size exceeds the maximum threshold). This is a valuable enhancement to the fishing statistics tracking system.Maple2.Server.Game/Packets/DungeonRoomPacket.cs (1)
8-21: LGTM - Well-structured packet handler for dungeon room errorsThe implementation is clean and follows the packet structure patterns seen elsewhere in the codebase. The Command enum is appropriately defined, and the Error method provides a clear way to create error packets with the necessary information.
Maple2.Server.Game/Service/ChannelService.Party.cs (1)
36-37: LGTM - Good integration of the new SetDungeon handlerThe switch case addition properly integrates the new dungeon functionality into the party request handling system.
Maple2.Server.Game/Packets/PartyPacket.cs (3)
9-10: Appropriate imports for the new functionality.These new imports allow access to the field management components needed for the updated dungeons functionality.
129-129: Good improvement for initialized dungeon ID.The change now writes the actual party's DungeonId rather than an uninitialized integer value, ensuring that the correct dungeon information is included in party load packets.
224-228:Details
❓ Verification inconclusive
Verify the logical condition in the DungeonReset method.
The method now checks
field is not DungeonFieldManagerrather than writing a static boolean value. This inverted condition should be verified to ensure it matches the intended behavior.If you're writing
truewhen the field is NOT a dungeon field, andfalsewhen it IS a dungeon field, please make sure this matches the expected client behavior.
🏁 Script executed:
#!/bin/bash # Find where DungeonReset is called to verify intended usage rg -A 3 "DungeonReset" --type csharpLength of output: 2319
Confirm Inverted Boolean Logic in DungeonReset
The static analysis shows that calls to
DungeonResetfrom bothPartyManager.csandGameSession.cspass aFieldobject, and the resulting boolean is written astruewhen the field is not aDungeonFieldManager, andfalseotherwise. Please ensure that this mapping (i.e.,truefor non-dungeon fields,falsefor dungeon fields) precisely matches the expected client behavior before merging.Maple2.Server.World/Containers/PartyManager.cs (2)
81-81: Good improvement for leader selection.The change ensures that only online members are considered as candidates for leadership, which is a logical enhancement.
406-423: Well-implemented dungeon setting functionality.This new method is properly structured:
- It verifies that only the party leader can set dungeon information
- It updates both the DungeonId and DungeonLobbyRoomId properties
- It broadcasts the update to all party members with the correct message format
The implementation follows the established pattern in the rest of the class.
Maple2.Server.World/Service/WorldService.Party.cs (2)
44-45: Appropriate case handling for the new SetDungeon functionality.The addition of this case in the switch statement properly routes the SetDungeon requests to the new handler method.
247-247: Properly maps the DungeonLobbyRoomId to the protocol buffer.The update to include the DungeonLobbyRoomId in the PartyInfo response is consistent with the changes made in the protocol buffers.
Maple2.Server.Core/proto/channel/channel.proto (3)
160-165: Well-structured SetDungeon message definition.The SetDungeon message is properly defined with all necessary fields for the dungeon configuration. The field types and ordering follow the protobuf conventions.
182-182: Proper addition to the Party oneof.The SetDungeon field is correctly added to the Party oneof with an appropriate tag number.
286-286:Details
✅ Verification successful
Verify that all instance_id references are renamed to room_id.
These changes rename fields from
instance_idtoroom_id, which appears to be part of a broader terminology change in the codebase. Ensure that all places that reference these fields are updated consistently.Also applies to: 305-305
🏁 Script executed:
#!/bin/bash # Search for any remaining references to instance_id that might need updating rg "instance_id" --type cs --type protoLength of output: 69
Action: Confirm complete renaming of
instance_idtoroom_idacross proto and C# filesIt appears that the initial search failed due to the use of
--type proto. Please re-run the search using file extension filters to properly verify that all instances ofinstance_idhave been updated. For example, try the following shell script:Review the output from both searches. If no results are found, it confirms that the renaming to
room_idis complete.
🏁 Script executed:
#!/bin/bash # Search for any remaining references to instance_id in .proto files echo "Searching .proto files for 'instance_id' references..." rg "instance_id" -g "*.proto" # Search for references in C# files echo "Searching C# files for 'instance_id' references..." rg "instance_id" --type csLength of output: 298
Renaming verification complete: All
instance_idreferences have been successfully replaced withroom_id.
- Verified via targeted searches in both *.proto and C# files.
- No stray
instance_idoccurrences were found in the repository, including the affected lines (286 and 305) in Maple2.Server.Core/proto/channel/channel.proto.Maple2.Server.Game/PacketHandlers/DungeonRoomHandler.cs (1)
62-65: Check for null reference inHandleEnterField.
Before callingsession.Dungeon.EnterInitField(), confirm thatsession.Dungeonis not null. A defensive check can prevent possible NullReferenceExceptions if theDungeonManagerhas not been initialized.Maple2.Server.Game/Manager/DungeonManager.cs (2)
58-63: Confirm fallback behavior for creation failures.
When dungeon creation fails, the code sends an error packet, but there is no retry logic or explicit user feedback mechanism beyond that. Confirm whether this one-time termination is appropriate or if a fallback path or re-attempt is desired.
124-149: Forced disconnection inMigrateToDungeon().
Thefinallyblock callssession.Disconnect(), meaning the session is always closed whether an exception occurs or not. Ensure this aligns with the desired design: if you need a contingency plan where the user remains in the current session upon failure, revise the disconnection logic.Maple2.Server.Core/proto/world/world.proto (2)
70-70: Confirm backward compatibility with field renames.
Renaminginstance_idtoroom_idmay break older clients or existing protobuf consumers that still expect the original field name. Validate that these changes won’t introduce serialization mismatches or require version checks.Also applies to: 102-102, 273-273
331-336: Validate newSetDungeonmessage field numbering.
The new fieldsdungeon_idanddungeon_room_idinSetDungeonshould not overlap with existing field numbers. Also, confirm that the new message structure is recognized by all relevant consumers of the service.
There was a problem hiding this comment.
Actionable comments posted: 5
🔭 Outside diff range comments (2)
Maple2.Model/Game/Cube/InteractCube.cs (1)
48-54: 💡 Verification agent🧩 Analysis chain
Consider updating WriteTo to include new properties
The
WriteTomethod doesn't serialize the newly addedObjectCodeandMetadataproperties. If these properties are needed when deserializing the object, you should consider updating the serialization logic.Are there corresponding deserialization methods that need to be updated? Does the receiving end expect these new properties?
🏁 Script executed:
#!/bin/bash # Find deserialization code related to InteractCube rg "ReadClass<InteractCube>|InteractCube\(" --type cs -A 5 -B 5Length of output: 9555
Action: Update serialization logic in WriteTo for new properties
The
WriteTomethod inMaple2.Model/Game/Cube/InteractCube.cscurrently writes out theId,State, and, when applicable, theNurturingobject but omits the newly addedObjectCodeandMetadataproperties. Our investigation shows that deserialization routines—such as the conversion methods and record constructor inMaple2.Database/Model/Map/InteractCube.cs—rely on these properties (withObjectCodebeing derived fromMetadata.Id). To maintain consistency between serialization and deserialization, please update theWriteTomethod to also serialize theObjectCodeandMetadataproperties.
- Ensure the new properties are written (e.g.,
writer.Write(ObjectCode)andwriter.WriteClass<FunctionCubeMetadata>(Metadata)or an equivalent based on howMetadatashould be serialized).- Verify that the corresponding deserialization paths correctly read these values.
Maple2.Server.Game/PacketHandlers/RequestCubeHandler.cs (1)
543-546:⚠️ Potential issueMissing ownership validation for reward claiming.
The ownership checks have been removed from this method. Previously, there would likely have been validation to ensure the user owns the plot before allowing them to claim rewards, similar to how
HandleInteriorDesignCheckInon line 535 verifies ownership. Without these checks, any user could potentially claim rewards for plots they don't own.Add plot ownership validation similar to what's in
HandleInteriorDesignCheckIn:private void HandleInteriorDesignReward(GameSession session, IByteReader packet) { + Plot? plot = session.Housing.GetFieldPlot(); + if (plot == null) { + return; + } + + if (plot.OwnerId != session.AccountId) { + session.Send(CubePacket.Error(UgcMapError.s_ugcmap_dont_have_ownership)); + return; + } + byte rewardId = packet.ReadByte(); session.Housing.InteriorReward(rewardId); }
🧹 Nitpick comments (14)
Maple2.Database/Model/Map/UgcMapCube.cs (2)
28-41: Consider using symmetric conversion patterns.I notice that while the implicit conversion operator from
UgcMapCubetoPlotCubehas been removed (as mentioned in the summary), the implicit conversion fromPlotCubetoUgcMapCuberemains. For a more consistent API, consider either:
- Keeping both conversions implicit, or
- Making both conversions explicit via named methods
The latter approach would be more maintainable and explicit, matching the
ToPlotCubemethod that was apparently added elsewhere.
1-54: Consider adding a comment about the conversion design change.Since you've removed the implicit conversion from
UgcMapCubetoPlotCubein favor of an explicit method elsewhere, it would be helpful to add a comment explaining this design decision. This would provide context for future developers.// ReSharper disable ReplaceConditionalExpressionWithNullCoalescing namespace Maple2.Database.Model; +// Note: Implicit conversion from UgcMapCube to PlotCube was removed in favor of +// using an explicit ToPlotCube method to make conversions more explicit and safer. internal class UgcMapCube {Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.Factory.cs (4)
107-110: Remove or clarify commented-out code
Leaving large code blocks commented out can lead to confusion and technical debt. If the commented-out dungeon logic is no longer needed or replaced by another approach, consider removing it to keep the codebase lean. Otherwise, include a clear comment explaining why it is temporarily commented out.
264-294: Confirm disposal flow for dungeon fields
While the loop correctly disposes idle or unneeded dungeon fields, verifying partial usage scenarios (e.g., sub-fields in active use while the lobby remains empty) can help ensure no unintended disposal occurs. For clarity, consider logging subtleties where only certain sub-fields are active versus the main dungeon.
299-301: Avoid swallowing exceptions
Silently catching exceptions via the emptycatchblock can mask potential errors or debugging clues. If you expect cancellations, consider catchingOperationCanceledExceptionexplicitly or adding at least minimal logging in the catch block.
361-363: Clarify the “never happen” scenario or re-check the logic
The comment indicates a scenario that “should never happen,” but it is still being handled. This might mean there’s a latent edge case in the system. Revisit the assumption or provide more context on why it’s guarded.Would you like help confirming if there’s a path that leads here or opening a follow-up issue to track this?
Maple2.Model/Enum/MigrationType.cs (1)
3-8: Enum definition looks good, but consider documenting the purpose of each value.The enum is well-structured with named values and appropriate numeric assignments. The gap between values 1 and 3 suggests room for future expansion or potentially replaced/removed values.
Consider adding XML documentation comments to describe the purpose of each migration type, especially since this enum appears to be used in critical path migration functionality.
namespace Maple2.Model.Enum; +/// <summary> +/// Defines the types of migration between different game areas or modes. +/// </summary> public enum MigrationType { + /// <summary>Normal migration between map areas</summary> Normal = 0, + /// <summary>Migration to decoration planning mode</summary> DecorPlanner = 1, + /// <summary>Migration to blueprint designer mode</summary> BlueprintDesigner = 3, + /// <summary>Migration to dungeon instance</summary> Dungeon = 4, }Maple2.Server.Core/proto/world/world.proto (1)
74-79: Consider using imports instead of duplicating enum definitions.The
MigrationTypeenum is defined both here and inMaple2.Model/Enum/MigrationType.cs. This duplication can lead to maintenance issues if the values get out of sync.Consider importing the enum from a shared proto file instead, if Protocol Buffers allows for cross-file enum imports in your environment.
Maple2.Server.Game/Packets/PartyPacket.cs (1)
247-258: Comment block formatting improved for clarity.The comment formatting change improves readability while preserving the content.
Maple2.Database/Storage/Game/GameStorage.User.cs (1)
175-182: Duplicate error handling pattern - consider extracting to a helper methodThis code block is nearly identical to the one for
home.Layouts. Consider extracting this pattern into a helper method to avoid duplication.- Maple2.Model.Game.HomeLayout? homeLayoutModel = ToHomeLayout(layout); - if (homeLayoutModel == null) { - Logger.LogError("Failed to convert HomeLayout: {LayoutUid}", layoutUid); - continue; - } - - home.Blueprints.Add(homeLayoutModel); + AddHomeLayoutIfValid(layout, layoutUid, home.Blueprints);And add this helper method:
private void AddHomeLayoutIfValid(HomeLayout layout, long layoutUid, ICollection<Maple2.Model.Game.HomeLayout> collection) { Maple2.Model.Game.HomeLayout? homeLayoutModel = ToHomeLayout(layout); if (homeLayoutModel == null) { Logger.LogError("Failed to convert HomeLayout: {LayoutUid}", layoutUid); return; } collection.Add(homeLayoutModel); }Maple2.Server.World/Containers/PartyManager.cs (1)
414-420: Consider adding parameter validation.The method accepts dungeon ID and room ID parameters without validating them for valid ranges or existence, which could lead to setting invalid values.
Consider adding basic validation before setting these values:
Party.DungeonId = dungeonId; Party.DungeonLobbyRoomId = dungeonRoomId; +// Log the dungeon information for debugging purposes +logger.Debug("Party {PartyId} dungeon set to {DungeonId} with room {RoomId}", Party.Id, dungeonId, dungeonRoomId); Broadcast(new PartyRequest { SetDungeon = new PartyRequest.Types.SetDungeon { PartyId = Party.Id, DungeonId = dungeonId, DungeonRoomId = dungeonRoomId, }, });Maple2.Server.Game/Manager/DungeonManager.cs (3)
26-26: Property naming could be improved to avoid confusion.The property
Party => session.Party.Partycreates a potentially confusing naming scenario with nestedPartyreferences. This could lead to maintenance challenges.Consider renaming the property to better indicate its purpose:
-private Party? Party => session.Party.Party; +private Party? PlayerParty => session.Party.Party;Then update all references to this property throughout the class.
42-85: Add error logging and fix empty line.The
CreateDungeonRoommethod has good validation logic but lacks error logging for key failure cases. There's also an unnecessary empty line at line 59.Add error logging and remove the empty line:
public void CreateDungeonRoom(int dungeonId, bool withParty) { if (!session.TableMetadata.DungeonRoomTable.Entries.TryGetValue(dungeonId, out DungeonRoomTable.DungeonRoomMetadata? metadata)) { + logger.Error("Invalid dungeon ID: {DungeonId}", dungeonId); session.Send(DungeonRoomPacket.Error(DungeonRoomError.s_room_dungeon_notOpenTimeDungeon)); return; } if (withParty) { if (Party == null) { + logger.Error("Cannot create dungeon with party: party is null for character {CharacterId}", session.CharacterId); session.Send(DungeonRoomPacket.Error(DungeonRoomError.s_room_dungeon_error_invalidPartyOID)); return; } if (Party.LeaderCharacterId != session.CharacterId) { + logger.Error("Character {CharacterId} is not party leader (leader: {LeaderId})", session.CharacterId, Party.LeaderCharacterId); session.Send(DungeonRoomPacket.Error(DungeonRoomError.s_room_party_err_not_chief)); return; } } - DungeonFieldManager? dungeonField = session.FieldFactory.CreateDungeon(metadata, session.CharacterId, Party); if (dungeonField == null) { + logger.Error("Failed to create dungeon field for dungeon {DungeonId}", dungeonId); session.Send(DungeonRoomPacket.Error(DungeonRoomError.s_room_dungeon_NotAllowTime)); return; } // Rest of the method... }
115-123: Consider validating the room ID parameter.The
SetDungeonmethod validates the dungeon ID but not the room ID, which could lead to issues if an invalid room ID is provided.Add validation for the room ID:
public void SetDungeon(int dungeonId, int roomId) { if (!session.TableMetadata.DungeonRoomTable.Entries.TryGetValue(dungeonId, out DungeonRoomTable.DungeonRoomMetadata? metadata)) { logger.Error("Dungeon metadata not found for dungeonId {dungeonId}", dungeonId); return; } + // Validate room ID if possible + if (roomId <= 0) { + logger.Warning("Invalid room ID: {RoomId} for dungeon {DungeonId}", roomId, dungeonId); + // Still continue since this might be a default value scenario + } Metadata = metadata; LobbyRoomId = roomId; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Maple2.Server.World/Migrations/20250304061437_InteractCubeFix.Designer.csis excluded by!Maple2.Server.World/Migrations/*Maple2.Server.World/Migrations/20250304061437_InteractCubeFix.csis excluded by!Maple2.Server.World/Migrations/*
📒 Files selected for processing (34)
Maple2.Database/Model/Map/HomeLayout.cs(0 hunks)Maple2.Database/Model/Map/HomeLayoutCube.cs(0 hunks)Maple2.Database/Model/Map/InteractCube.cs(1 hunks)Maple2.Database/Model/Map/UgcMapCube.cs(1 hunks)Maple2.Database/Storage/Game/GameStorage.HomeLayout.cs(2 hunks)Maple2.Database/Storage/Game/GameStorage.Map.cs(6 hunks)Maple2.Database/Storage/Game/GameStorage.Nurturing.cs(1 hunks)Maple2.Database/Storage/Game/GameStorage.Quest.cs(1 hunks)Maple2.Database/Storage/Game/GameStorage.User.cs(2 hunks)Maple2.File.Ingest/Mapper/ItemMapper.cs(1 hunks)Maple2.Model/Enum/MigrationType.cs(1 hunks)Maple2.Model/Game/Cube/InteractCube.cs(3 hunks)Maple2.Model/Game/Item/ItemType.cs(0 hunks)Maple2.Model/Metadata/ItemMetadata.cs(1 hunks)Maple2.Server.Core/proto/channel/channel.proto(4 hunks)Maple2.Server.Core/proto/world/world.proto(6 hunks)Maple2.Server.Game/Commands/FieldCommand.cs(1 hunks)Maple2.Server.Game/Manager/DungeonManager.cs(1 hunks)Maple2.Server.Game/Manager/Field/FieldManager/DungeonFieldManager.cs(1 hunks)Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.Factory.cs(1 hunks)Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs(11 hunks)Maple2.Server.Game/Manager/Field/FieldManager/HomeFieldManager.cs(1 hunks)Maple2.Server.Game/Manager/HousingManager.cs(7 hunks)Maple2.Server.Game/Manager/PartyManager.cs(3 hunks)Maple2.Server.Game/PacketHandlers/FunctionCubeHandler.cs(2 hunks)Maple2.Server.Game/PacketHandlers/PartyHandler.cs(4 hunks)Maple2.Server.Game/PacketHandlers/PartySearchHandler.cs(2 hunks)Maple2.Server.Game/PacketHandlers/RequestCubeHandler.cs(1 hunks)Maple2.Server.Game/Packets/PartyPacket.cs(6 hunks)Maple2.Server.Game/Service/ChannelService.Party.cs(2 hunks)Maple2.Server.Game/Session/GameSession.cs(9 hunks)Maple2.Server.World/Containers/PartyLookup.cs(1 hunks)Maple2.Server.World/Containers/PartyManager.cs(4 hunks)Maple2.Server.World/Service/WorldService.Migrate.cs(4 hunks)
💤 Files with no reviewable changes (3)
- Maple2.Model/Game/Item/ItemType.cs
- Maple2.Database/Model/Map/HomeLayout.cs
- Maple2.Database/Model/Map/HomeLayoutCube.cs
✅ Files skipped from review due to trivial changes (1)
- Maple2.Database/Storage/Game/GameStorage.Quest.cs
🚧 Files skipped from review as they are similar to previous changes (10)
- Maple2.Server.Game/Commands/FieldCommand.cs
- Maple2.Server.Game/Service/ChannelService.Party.cs
- Maple2.Server.Core/proto/channel/channel.proto
- Maple2.Server.Game/Manager/HousingManager.cs
- Maple2.Server.Game/Manager/Field/FieldManager/DungeonFieldManager.cs
- Maple2.Server.Game/Manager/PartyManager.cs
- Maple2.Server.Game/Manager/Field/FieldManager/HomeFieldManager.cs
- Maple2.Server.World/Service/WorldService.Migrate.cs
- Maple2.Server.Game/Session/GameSession.cs
- Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs
🔇 Additional comments (53)
Maple2.Model/Game/Cube/InteractCube.cs (3)
12-13: Good addition of metadata object and object codeThe addition of
MetadataandObjectCodeproperties improves the design by providing access to the complete metadata object rather than just individual properties. Theinitaccessor onObjectCodeensures immutability after initialization, which is a good practice for object identifiers.
22-26: LGTM: Constructor updated to use the new metadata-based approachThe constructor has been properly refactored to utilize the metadata object for initializing the cube. Setting
Metadata,ObjectCode, andStatefrom the metadata parameter consolidates the initialization logic and ensures consistency with the metadata source.
39-44:Details
✅ Verification successful
Constructor signature updated correctly for metadata-based approach
The constructor signature has been updated to replace
InteractCubeState defaultStatewithFunctionCubeMetadata metadata, which aligns with the architectural changes. Property assignments have been correctly updated to use the metadata object.However, you should verify that all call sites have been updated to pass a
FunctionCubeMetadataobject instead of anInteractCubeState.
🏁 Script executed:
#!/bin/bash # Find all places where InteractCube constructor is called to ensure they've been updated rg "new InteractCube\([^,]+,[^,]+" --type cs -A 2 -B 2Length of output: 1480
Verified: Constructor and Call Sites Updated Correctly
The constructor now correctly receives a
FunctionCubeMetadataobject, and all instantiation sites (in particular, those inMaple2.Server.Game/Manager/HousingManager.cs) have been updated accordingly to pass a metadata object instead of anInteractCubeState.
- Verified call sites use the updated signature (e.g.,
new InteractCube(plotCube.Position, functionCubeMetadata)).Maple2.Database/Model/Map/UgcMapCube.cs (2)
6-6: Appropriate namespace addition.Adding the Metadata namespace import improves code clarity by making the dependencies explicit. This change aligns with good coding practices.
1-54:Details
✅ Verification successful
Verify usages of the removed implicit conversion operator.
The removal of the implicit conversion operator from
UgcMapCubetoPlotCubeis a significant change that might impact existing code. Ensure all callers now explicitly use the newToPlotCubemethod instead.
🏁 Script executed:
#!/bin/bash # Description: Search for places that might still expect implicit conversion from UgcMapCube to PlotCube # Find usages of UgcMapCube where a PlotCube might be expected echo "Searching for potential conversion issues..." rg -A 3 -B 3 --type csharp "UgcMapCube\s+\w+\s*[=;]" --glob "!**/UgcMapCube.cs" # Find places where PlotCube is expected but UgcMapCube is provided echo "Searching for method calls that might expect PlotCube but receive UgcMapCube..." rg -A 2 -B 2 --type csharp "(\w+\s*\(\s*.*UgcMapCube.*\)|\w+\s*=\s*.*UgcMapCube.*)" --glob "!**/UgcMapCube.cs" # Find direct casts that might no longer work echo "Searching for direct casts..." rg -A 2 -B 2 --type csharp "\(\s*PlotCube\s*\)\s*\w+\s*" --glob "!**/UgcMapCube.cs" # Look for usages of ToPlotCube echo "Checking for ToPlotCube method usage..." rg -A 2 -B 2 --type csharp "ToPlotCube\s*\(" --glob "!**/UgcMapCube.cs"Length of output: 5014
Conversion Verification Complete – No Remaining Issues
After verifying the codebase, we found that all conversions from UgcMapCube to PlotCube are now handled explicitly using the
ToPlotCubemethod. The implicit conversion operator that converted from UgcMapCube to PlotCube has been removed, and no residual usages of that removed operator were identified. Note that the remaining implicit conversion from PlotCube to UgcMapCube remains in place and is unaffected by this change.
- Location Verified:
Maple2.Database/Storage/Game/GameStorage.Map.csshows explicit calls toToPlotCubewhere a conversion toPlotCubeis needed.- No Direct Casts: Searches for direct casts to
PlotCubeinvolvingUgcMapCubereturned no results.Maple2.Database/Model/Map/InteractCube.cs (4)
3-3: Import requirement for metadata handling is appropriate.Adding the
Maple2.Model.Metadatanamespace is necessary to support the new conversion method that usesFunctionCubeMetadata.
17-17: Conversion operator updated to use ObjectCode instead of DefaultState.The implicit conversion operator has been appropriately updated to match the property rename.
22-22: Comment provides clear explanation for design choice.The comment clearly explains why an explicit conversion method is used instead of an implicit operator, which helps maintainers understand the design decision.
22-25:Details
❓ Verification inconclusive
New explicit conversion method replaces implicit conversion.
Moving from implicit to explicit conversion is a good design choice since it requires metadata that wasn't previously needed. This makes the dependency requirements clearer and prevents potential errors.
However, I notice the method allows for overriding
noticeSettingsandportalSettingswith external values, which could potentially cause inconsistencies with the database model's own properties.Could this lead to unexpected behavior if the provided settings differ from the object's original settings? Consider documenting the intended precedence or adding validation if the parameters are expected to match the object's own properties.
🏁 Script executed:
#!/bin/bash # Check for usages of this Convert method to understand how it's being used rg "Convert\(.*FunctionCubeMetadata" --type csLength of output: 245
Action Required: Review the handling of external settings in Convert()
The explicit conversion method now requires metadata and allows external values for both
noticeSettingsandportalSettings. While the change clarifies the dependency requirements, it also means that if these parameters differ from the database model’s inherent settings, it could lead to unexpected behavior. There’s no evidence from the codebase showing additional usage that mitigates this risk. Please verify whether these external parameters should be allowed to override internal defaults—if so, document the intended precedence; otherwise, consider adding validation to ensure consistency.
- File: Maple2.Database/Model/Map/InteractCube.cs (lines 22–25)
- Suggestion: Document parameter precedence or enforce validation, as the external settings might conflict with the object’s original property values.
Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.Factory.cs (2)
42-45: Use standard object initializers rather than bracket-based syntax
Currently, thefields,homes,dungeons, andsubDungeonFieldsdictionaries are initialized with= [];, which is non-standard C# syntax and can lead to compilation issues in many environments.
123-129: Dispose previously created fields upon overwriting
UsingAddOrUpdatewithout disposing an existing field can lead to resource leaks or concurrency issues. Confirm that overwriting the old field is acceptable, or explicitly dispose the old field before overwriting it.Maple2.Model/Enum/MigrationType.cs (1)
3-8:Details
✅ Verification successful
Verify consistency with the proto definition.
The enum defined here must match exactly with the MigrationType enum defined in world.proto to ensure proper serialization/deserialization. Currently, they appear to match in terms of values, but naming style differs (PascalCase vs UPPERCASE).
🏁 Script executed:
#!/bin/bash # Check consistency between C# enum and Proto enum definition echo "Comparing MigrationType values between C# and Proto definitions:" echo "C# enum (Maple2.Model/Enum/MigrationType.cs):" cat Maple2.Model/Enum/MigrationType.cs | grep -E '^\s+\w+ = \d+,' -B 1 -A 3 echo -e "\nProto enum (Maple2.Server.Core/proto/world/world.proto):" cat Maple2.Server.Core/proto/world/world.proto | grep -A 5 "enum MigrationType"Length of output: 627
Verified: Enum values are consistent between C# and proto definitions.
The numeric values for each
MigrationType(0, 1, 3, 4) match exactly, even though the naming conventions differ (PascalCase in C# vs. UPPERCASE in proto). This difference is expected given the language-specific style guidelines and will not affect serialization/deserialization.Maple2.Server.Core/proto/world/world.proto (7)
70-71: Good renaming from instance_id to room_id for better semantics.The renaming from
instance_idtoroom_idimproves code clarity by better reflecting the meaning based on the PR objectives. The change ofplot_modetotypewith theMigrationTypeenum aligns with the new type system.
103-104: Consistent renaming applied correctly in MigrateInResponse.The renaming from
instance_idtoroom_idand using theMigrationTypeenum is consistently applied in theMigrateInResponsemessage, which is good for maintaining code coherence.
332-336: New SetDungeon message to support dungeon functionality.The
SetDungeonmessage provides the structure needed for setting up dungeon instances, which directly supports the PR's goal of implementing dungeon functionality. The fields look appropriate for tracking party, dungeon, and room relationships.
351-351: Properly added SetDungeon to the party oneof.The
set_dungeonfield has been correctly added to thepartyoneof in thePartyRequestmessage, which allows clients to make requests related to dungeon setup.
493-493: Consistent renaming in GlobalPortalInfo.The renaming from
instance_idtoroom_idhas been consistently applied in theGlobalPortalInfomessage, which maintains code coherence across the codebase.
70-71:Details
✅ Verification successful
Verify all references to the renamed fields are updated elsewhere in the codebase.
The renaming from
instance_idtoroom_idand the addition of new fields and messages need to be reflected in all code that references these fields. Make sure to check:
- C# code that deserializes these messages
- Handler functions for these message types
- Client code that sends/receives these messages
- Any utility functions that process these fields
This is especially important for the
SetDungeonmessage and the expandedPartyInfofields.Also applies to: 103-104, 274-281, 332-336, 493-493
🏁 Script executed:
#!/bin/bash # Search for potential references to the old field names echo "Searching for potential references to old 'instance_id' that might need updating:" rg "instance_id" --type cs --glob "!**/obj/**" --glob "!**/bin/**" echo -e "\nChecking for handler implementations for PartyRequest:" rg -A 5 "PartyRequest|set_dungeon" --type cs --glob "!**/obj/**" --glob "!**/bin/**"Length of output: 45588
Field renaming verified
The search forinstance_idreturned no results in the C# files, and our review of the PartyRequest handlers (including those forSetDungeon) confirms that all references now useroom_id. All the key areas—deserialization, handler functions, client communications, and utility processing—appear to be in sync with the updated proto definitions. Please give a final manual pass on client-side integrations if needed, but as far as the codebase goes, the renaming has been successfully reflected.
274-281:Details
❓ Verification inconclusive
Expanded PartyInfo message with dungeon and matching-related fields.
The
PartyInfomessage has been expanded with several fields related to dungeon functionality and party matching, which aligns with the PR objective of implementing dungeon functionality. The added fields provide useful context for party management within dungeons.Just make sure that these new fields are properly initialized and handled throughout the codebase, especially in code that previously only dealt with a subset of these fields.
🏁 Script executed:
#!/bin/bash # Check for code that handles PartyInfo to ensure new fields are properly handled echo "Searching for code that processes PartyInfo fields:" rg -A 3 -B 3 "PartyInfo" --type cs --glob "!**/obj/**" --glob "!**/bin/**"Length of output: 6426
Attention: Ensure Complete Initialization for Matching Fields in PartyInfo
The updated
PartyInfoinMaple2.Server.Core/proto/world/world.protonow includes fields for dungeon functionality as well as party matching (i.e.,match_party_name,match_party_id,is_matching,require_approval, andparty_search). While the conversion logic (e.g., inWorldService.Party.cs’sToPartyInfofunction) correctly maps the dungeon-related fields (such asdungeon_room_idviaparty.DungeonLobbyRoomId), the matching-specific fields are not yet initialized.Please ensure that:
- Matching Fields Initialization: The new matching-related fields are set to appropriate default values or mapped from the corresponding properties in the
Partyentity wherever thePartyInfomessage is constructed.- Consistent Handling: All modules that process or utilize
PartyInfo(such as inPartyManager.csandFieldManager.Factory.cs) are updated to handle the expanded message schema.Review and update the conversion and initialization logic as needed so that all new fields are properly integrated throughout the codebase.
Maple2.File.Ingest/Mapper/ItemMapper.cs (1)
152-154:Details
✅ Verification successful
Property updates for ItemMetadataInstall properly align with object model changes.
The changes to use
FunctionIdandObjectCubeIdinstead of the previousInteractIdreflect a more precise modeling of item installation metadata. This separation allows for more specific functionality and object identification.Let's check if these new properties are used consistently across the codebase:
🏁 Script executed:
#!/bin/bash # Check for references to the new properties in the codebase rg "FunctionId|ObjectCubeId" --type csLength of output: 1671
Verified: Consistent Property Usage
The update replacing the legacy
InteractIdwith distinctFunctionIdandObjectCubeIdproperties is consistently applied across the codebase. The search confirms that these new properties are referenced in various modules (e.g., in NpcScriptManager, HousingManager, and FieldManager), which supports a more granular and accurate handling of item installation metadata.
- The change in
Maple2.File.Ingest/Mapper/ItemMapper.csaligns well with object model requirements.- References in other files (e.g.,
Maple2.Server.Game/Manager/NpcScriptManager.csandMaple2.Server.Game/Manager/HousingManager.cs) confirm consistency and proper propagation of these new properties.Maple2.Server.Game/Packets/PartyPacket.cs (5)
9-10: Appropriate imports added for dungeon functionality.The new imports for field management and room model classes support the dungeon implementation work being introduced in this PR.
115-115: Method signature enhancement forLoadadds dungeon quick entry support.The addition of the
quickEnterparameter with a default value offalsemaintains backward compatibility while adding functionality for the new dungeon implementation.
128-129: Updated packet structure for dungeon-related data.The packet now correctly writes the
quickEnterflag and the party'sDungeonId, which provides necessary information for dungeon management on the client side.
224-224: Method signature update forDungeonResetenhances field management integration.The method now accepts a
FieldManagerparameter and an optionaldungeonIdparameter, aligning with the new field management architecture being implemented.
227-228: Improved dungeon reset packet with proper field type checking.The change to check if the field is not a
DungeonFieldManageris more precise than the previous static boolean value. Writing the actualdungeonIdalso provides better context for clients.Maple2.Database/Storage/Game/GameStorage.Map.cs (5)
17-17: Default parameter value improves flexibility ofLoadPlotsForMap.Adding a default value of
-1for theownerIdparameter makes the method more versatile, allowing it to be called without explicitly specifying an owner ID when not needed.
37-39: Improved robustness with null checking inLoadCubesForOwner.The changes add proper null checks using the new
ToPlotCubehelper method, preventing potential null reference exceptions when processing cube data.
171-173: Enhanced null safety inSaveCubesusing helper methods.Using the
ToPlotCubehelper method with null filtering ensures that only valid cubes are processed, reducing the risk of runtime errors.
226-237: RefactoredToPlotmethod with improved null checking.The method now uses the
ToPlotCubehelper and explicitly checks for null values before accessing nested properties, making the code more robust against null reference exceptions.
262-307: Well-structured conversion helper methods improve code maintainability.The new helper methods (
ToHomeLayout,ToInteractCube, andToPlotCubeoverloads) encapsulate conversion logic with proper null checking, making the code more modular and easier to maintain.These methods follow the single responsibility principle by isolating conversion logic and ensuring consistent null handling across different model transformations.
Maple2.Server.World/Containers/PartyLookup.cs (1)
62-63:Details
❓ Verification inconclusive
Established bidirectional relationship between PartyManager and PartyLookup.
Setting the
PartyLookupproperty in thePartyManagerinstance enables the manager to access lookup functionality, which is likely necessary for the dungeon implementation to allow parties to interact with dungeon systems.Let's check if the
PartyLookupproperty is used elsewhere in the codebase:
🏁 Script executed:
#!/bin/bash # Check for usages of the PartyLookup property in PartyManager rg "PartyLookup\s*=" --type cs rg "\.PartyLookup\." --type csLength of output: 143
Action Required: Verify PartyLookup Usage Across the Codebase
The current changes correctly set up the bidirectional relationship by assigning
PartyLookup = thisinMaple2.Server.World/Containers/PartyLookup.cs(Lines 62-63). However, our search only revealed this assignment and did not find additional references (e.g., using.PartyLookup.) elsewhere in the code. As this might be intentional (for future use or indirect invocation), please manually verify that thePartyLookupproperty is indeed being leveraged as expected—especially in the context of the dungeon implementation.
- Location:
Maple2.Server.World/Containers/PartyLookup.cs(Lines 62-63)- Action: Confirm that the lookup functionality exposed by the
PartyLookupproperty is accessible or invoked appropriately in other parts of the codebase.Maple2.Server.Game/PacketHandlers/PartySearchHandler.cs (1)
60-60:Details
✅ Verification successful
Verify the quickEnter parameter behavior across field types
The new condition
quickEnter: session.Field is not DungeonFieldManagerdisables quick entry when the player is in a dungeon field. This suggests that players in dungeons should not be able to quick-enter parties.
🏁 Script executed:
#!/bin/bash # Check for other usages of quickEnter parameter to understand its behavior rg "quickEnter" --type cs # Look for the PartyPacket.Load method to understand parameter usage rg "PartyPacket\.Load\(" --type cs -A 5 # Check for DungeonFieldManager class to understand related restrictions rg "class DungeonFieldManager" --type cs -A 10Length of output: 5456
QuickEnter Validation: Dungeon Field Behavior Confirmed
The condition
quickEnter: session.Field is not DungeonFieldManageris applied consistently across all relevant modules (i.e.,PartyManager.cs,PartyHandler.cs, andPartySearchHandler.cs). This setup correctly disables quick entry for players in dungeon fields, aligning with the game design. No changes are necessary.Maple2.Model/Metadata/ItemMetadata.cs (1)
119-121:Details
✅ Verification successful
Ensure consumers of ItemMetadataInstall are updated for new property names
The
InteractIdproperty has been replaced with two more specific properties:FunctionIdandObjectCubeId. This change provides better separation of concerns between an item's functionality and its physical representation.
🏁 Script executed:
#!/bin/bash # Find code that previously used InteractId rg "\.InteractId" --type cs # Check the usage of the new properties to ensure proper migration rg "\.FunctionId|\.ObjectCubeId" --type cs # Look for any places that might be initializing ItemMetadataInstall rg "new ItemMetadataInstall" --type cs -A 5Length of output: 2136
Review: Confirm Updated Usage of ItemMetadataInstall Properties
The verification confirms that the instantiation and consumption of
ItemMetadataInstallnow properly use the new properties. In particular:
- Item Creation: In
Maple2.File.Ingest/Mapper/ItemMapper.cs, the constructor now correctly passesFunctionId(usingdata.install.funcCode) andObjectCubeId(usingdata.install.objCode), alongsideMapAttribute.- Consumer Updates: Consumers in
Maple2.Server.Game/Manager/HousingManager.cs,Maple2.Server.Game/Manager/NpcScriptManager.cs, and withinFieldManager.State.csare updated to reference the new properties (e.g.,itemMetadata.Install.ObjectCubeIdandState.Contents.ElementAt(Index).FunctionId).Note: There are still some references to
InteractIdin files likeMaple2.Model/Game/InteractObject.csand parts ofFieldManager.State.cs. These appear to belong to legacy code (likely related toInteractObjectMetadata) rather thanItemMetadataInstalland do not conflict with the migration.Overall, the changes for
ItemMetadataInstallhave been successfully propagated. Please double-check that the legacyInteractIdusages are intentional for their respective contexts.Maple2.Database/Storage/Game/GameStorage.User.cs (1)
159-166: Good error handling for layout conversionThe added null checks and error logging improve the robustness of the code by ensuring that only valid layouts are added to the home object. This prevents potential null reference exceptions later in the code.
Maple2.Database/Storage/Game/GameStorage.Nurturing.cs (2)
12-12: Simplified return statement improves readabilityThe return statement has been simplified to directly construct the
Nurturingobject using the provided metadata. This makes the code more concise and easier to follow.
10-10:Details
✅ Verification successful
Validate all callers of GetNurturing have been updated for the new parameter
The
GetNurturingmethod now requires ametadataparameter of typeFunctionCubeMetadata.NurturingData. All callers of this method must be updated to provide this parameter.
🏁 Script executed:
#!/bin/bash # Find all calls to GetNurturing rg "GetNurturing\(" --type cs -A 2 # Look for the FunctionCubeMetadata.NurturingData type to understand its structure rg "FunctionCubeMetadata\.NurturingData" --type csLength of output: 4385
All GetNurturing Callers Are Updated Correctly
The verification confirms that every call to
GetNurturinghas been updated to provide the requiredmetadataparameter. In particular:
- In
Maple2.Database/Storage/Game/GameStorage.Map.cs, all calls passcube.Interact.Metadata.Nurturing.- In both
Maple2.Server.Game/PacketHandlers/RequestCubeHandler.csandMaple2.Server.Game/PacketHandlers/FunctionCubeHandler.cs, the updated call signature with themetadataparameter is correctly in place.- The usage of
FunctionCubeMetadata.NurturingDatais consistent across the codebase.No further modifications are needed regarding this change.
Maple2.Server.Game/PacketHandlers/RequestCubeHandler.cs (1)
235-238: Good improvement with additional null checks.The updated code now properly checks both
plotCube.Interact.NurturingandplotCube.Interact.Metadata.Nurturingfor null values before proceeding, providing more robust null-safety.Maple2.Database/Storage/Game/GameStorage.HomeLayout.cs (2)
18-18: Consistent model transformation implementation.The change ensures the HomeLayout data is consistently processed through the
ToHomeLayouttransformation method, which likely ensures proper null handling and mapping between database and domain models.
31-32:Details
❓ Verification inconclusive
Query optimization using projection.
The code now uses a projection with
Select(ToHomeLayout)to transform data as part of the query pipeline, which is a good practice for ensuring consistent transformations.However, verify that the
ToHomeLayoutmethod handles all edge cases properly when used in a LINQ projection.
🏁 Script executed:
#!/bin/bash # Search for the ToHomeLayout method implementation to verify its implementation echo "Searching for ToHomeLayout method implementation..." rg -A 15 'private\\s+HomeLayout\\?\\s+ToHomeLayout' --type csLength of output: 176
Attention: Manual Verification of
ToHomeLayoutImplementation RequiredThe projection via
.Select(ToHomeLayout)is a good optimization. However, our initial automated search did not locate the implementation ofToHomeLayout. Please manually verify that theToHomeLayoutmethod exists and properly handles all edge cases (e.g., null values, unexpected input formats, and any potential exceptions) within its LINQ projection context.Maple2.Server.Game/PacketHandlers/FunctionCubeHandler.cs (2)
91-93: Updated property references for state management.The code now references state information from
cube.Interact.Metadata.DefaultStateinstead of directly fromcube.Interact.DefaultState, which suggests a refactoring to better organize the data structure.
101-105: Improved null check and updated parameter passing.The added null check for
cube.Interact?.Metadata.Nurturingprevents potential null reference exceptions. The updated call todb.GetNurturingnow includes the metadata parameter, which is likely needed after a method signature change.Maple2.Server.Game/PacketHandlers/PartyHandler.cs (4)
10-10: New import added for DungeonFieldManager.The code now imports from
Maple2.Server.Game.Model.Roomnamespace, which is necessary for theDungeonFieldManagertype check.
169-170: Consistent application of field-aware party loading.The same pattern for conditionally setting
quickEnteris consistently applied here, maintaining the same behavior across different party joining scenarios.
246-247: Consistent application of field-aware party loading.The same pattern for conditionally setting
quickEnteris consistently applied here for party search join responses, maintaining behavior consistency.
111-112:Details
✅ Verification successful
Added dungeon-aware party loading behavior.
The code now conditionally sets
quickEnterbased on whether the session is in a dungeon field. This helps differentiate party loading behavior between dungeon and non-dungeon contexts.
🏁 Script executed:
#!/bin/bash # Check how quickEnter parameter affects party loading behavior echo "Checking PartyPacket.Load implementation and usage of quickEnter..." rg -A 10 'Load\(.*quickEnter' --type csLength of output: 6227
Dungeon-aware party loading verified and approved.
The implementation consistently sets thequickEnterparameter based on whether the session’s field is a dungeon (i.e.,session.Field is not DungeonFieldManager), as seen in PartyHandler.cs, PartySearchHandler.cs, and PartyManager.cs. No further changes are needed.Maple2.Server.World/Containers/PartyManager.cs (3)
20-20: Good use of C# 11 required modifier for properties.The use of the
requiredmodifier for thePartyLookupproperty ensures that it must be initialized at object creation time, which helps prevent null reference exceptions.
81-83: LGTM - Improved leader selection logic.The LINQ expression provides a cleaner and more readable approach to finding a new party leader, prioritizing online members.
90-93: LGTM - Clean party disbanding check.The method correctly checks if there are 2 or fewer members left in the party before disbanding it using the
PartyLookupservice.Maple2.Server.Game/Manager/DungeonManager.cs (5)
19-35: Good class structure with proper initialization.The
DungeonManagerclass is well-structured with clear properties, constructor, and proper initialization of resources like the cancellation token source.
37-40: LGTM - Clean field setting method.The
SetDungeonmethod correctly sets both the field and metadata properties.
100-113: LGTM - Well-implemented field entry logic.The
EnterInitFieldmethod includes proper null checks, error logging, and sends appropriate responses to the client.
154-157: LGTM - Proper resource cleanup in Dispose method.The
Disposemethod correctly cleans up the session and cancellation token source, following theIDisposablepattern.
125-151:Details
❓ Verification inconclusive
Session disconnection timing issue in MigrateToDungeon.
The
MigrateToDungeonmethod disconnects the session in thefinallyblock, which happens regardless of success or failure. This might not be the intended behavior if there's an exception.Consider if this is the intended behavior:
try { var request = new MigrateOutRequest { // ...request parameters... }; MigrateOutResponse response = session.World.MigrateOut(request); var endpoint = new IPEndPoint(IPAddress.Parse(response.IpAddress), response.Port); session.Send(MigrationPacket.GameToGame(endpoint, response.Token, Metadata.LobbyFieldId)); session.State = SessionState.ChangeMap; } catch (RpcException ex) { session.Send(MigrationPacket.GameToGameError(MigrationError.s_move_err_default)); session.Send(NoticePacket.Disconnect(new InterfaceText(ex.Message))); -} finally { - session.Disconnect(); + // Disconnect only on error + session.Disconnect(); }
🏁 Script executed:
#!/bin/bash # Search for other migration patterns to verify the common approach echo "Searching for other migration methods that follow similar patterns..." rg -A 5 "MigrateOut" --type csLength of output: 12755
Clarification Required: Session Disconnection Behavior in MigrateToDungeon
It appears that the current implementation always disconnects the session via the
finallyblock, even after a successful migration. In several other migration methods (for example, in parts of the Guild and GameSession handlers), the disconnect occurs only under error conditions or is handled explicitly on a successful path. Please verify whether this unconditional disconnection is intentional. If a successful migration should preserve the session (letting the client continue its transition), consider moving the disconnect call into the catch block. For example:try { var request = new MigrateOutRequest { // ...request parameters... }; MigrateOutResponse response = session.World.MigrateOut(request); var endpoint = new IPEndPoint(IPAddress.Parse(response.IpAddress), response.Port); session.Send(MigrationPacket.GameToGame(endpoint, response.Token, Metadata.LobbyFieldId)); session.State = SessionState.ChangeMap; } catch (RpcException ex) { session.Send(MigrationPacket.GameToGameError(MigrationError.s_move_err_default)); session.Send(NoticePacket.Disconnect(new InterfaceText(ex.Message))); -} finally { - session.Disconnect(); + // Disconnect only on error + session.Disconnect(); }
- Confirm if the session should remain connected on a successful migration.
- Align the disconnection logic with similar migration flows elsewhere in the codebase.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
Maple2.Server.Game/Manager/PartyManager.cs (1)
298-308: 🛠️ Refactor suggestionImplement conditional handling for the
setparameter.The
setparameter is not being properly utilized in the implementation. Currently, the dungeon properties are always updated regardless of thesetvalue.Apply this diff to properly utilize the
setparameter:public void SetDungeon(int dungeonId, int dungeonRoomId, bool set) { if (Party == null) { return; } - Party.DungeonId = dungeonId; - Party.DungeonLobbyRoomId = dungeonRoomId; - Party.DungeonSet = set; + if (set) { + Party.DungeonId = dungeonId; + Party.DungeonLobbyRoomId = dungeonRoomId; + } else { + Party.DungeonId = 0; + Party.DungeonLobbyRoomId = 0; + } + Party.DungeonSet = set; session.Dungeon.SetDungeon(dungeonId, dungeonRoomId, set); session.Send(PartyPacket.DungeonReset(set, dungeonId)); }Maple2.Model/Game/Party/Party.cs (1)
59-60:⚠️ Potential issueInclude DungeonLobbyRoomId in serialization.
The
DungeonLobbyRoomIdproperty is missing from theWriteTomethod, which could lead to incomplete data being sent to clients.Apply this diff to include the DungeonLobbyRoomId in serialization:
writer.WriteBool(DungeonSet); writer.WriteInt(DungeonId); +writer.WriteInt(DungeonLobbyRoomId); writer.WriteBool(false); // unk boolThis ensures that the new dungeon room information is properly serialized and can be used by client code.
Maple2.Server.Game/Manager/DungeonManager.cs (2)
80-82:⚠️ Potential issueAdd better error handling for RPC communication.
The error handling for the RPC call just swallows the exception. It would be better to log the error for debugging purposes.
try { session.World.Party(request); -} catch (RpcException) { } +} catch (RpcException ex) { + logger.Error(ex, "Failed to send SetDungeon request to World server for party {PartyId}", Party!.Id); + // Consider notifying the player if this is a critical failure +}
88-99: 🛠️ Refactor suggestionAdd error notification for EnterLobby failures.
The
EnterLobbymethod silently fails without notifying the player when the party is null or the field is not a dungeon field.public void EnterLobby() { if (Party == null) { + logger.Warning("Cannot enter lobby: party is null for character {CharacterId}", session.CharacterId); + session.Send(DungeonRoomPacket.Error(DungeonRoomError.s_room_dungeon_error_invalidPartyOID)); return; } FieldManager? field = session.Field.FieldFactory.Get(roomId: Party.DungeonLobbyRoomId); if (field is not DungeonFieldManager dungeonField) { + logger.Error("Cannot enter lobby: field {RoomId} is not a dungeon field", Party.DungeonLobbyRoomId); + session.Send(DungeonRoomPacket.Error(DungeonRoomError.s_room_dungeon_NotAllowTime)); return; } SetDungeon(dungeonField); MigrateToDungeon(); }
🧹 Nitpick comments (16)
Maple2.Server.Game/Manager/Field/FieldManager/IField.cs (4)
22-24: Clarify the disposal pattern and naming for theDisposedproperty.
Since this interface derives fromIDisposable, consider renamingDisposedtoIsDisposedor removing it from the interface if it's strictly meant for internal usage. Additionally, ensure classes that implement this interface follow a consistent and correctIDisposablepattern for cleaning up unmanaged resources.
26-42: Revisit large metadata storage injection.
Injecting numerous metadata storages and calculators in one interface may make it too broad and violate SRP (Single Responsibility Principle). Consider splitting complex responsibilities into separate interfaces or providers to keep responsibilities clearly delineated.
60-60: Evaluate the defaultInit()method in an interface.
Providing a default implementation in an interface can obscure intended usage and couple implementers to shared logic. Consider moving this initialization logic to an abstract base class or ensuring each implementer is explicit about its own initialization.
71-71: Address the "TODO: MOVE THIS TO RANDOM ONLY" comment.
A TODO in production interfaces can lead to confusion. If this timer functionality belongs to a specific feature, consider relocating or refining it before merging.Maple2.Server.World/Service/WorldService.Party.cs (3)
229-237: Error handling inconsistency in SetDungeon method.Unlike other methods in this class, the
SetDungeonmethod doesn't check for or return any error codes from themanager.SetDungeoncall. This creates inconsistent error handling patterns across the service.private PartyResponse SetDungeon(long requestorId, PartyRequest.Types.SetDungeon setDungeon) { if (!partyLookup.TryGet(setDungeon.PartyId, out PartyManager? manager)) { return new PartyResponse { Error = (int) PartyError.s_party_err_not_found }; } - manager.SetDungeon(requestorId, setDungeon.DungeonId, setDungeon.Set, setDungeon.DungeonRoomId); - - return new PartyResponse(); + PartyError error = manager.SetDungeon(requestorId, setDungeon.DungeonId, setDungeon.Set, setDungeon.DungeonRoomId); + if (error != PartyError.none) { + return new PartyResponse { Error = (int) error }; + } + + return new PartyResponse { PartyId = setDungeon.PartyId }; }
229-237: Consider returning updated party information.Most methods that modify party state return some information about the updated party (either the full party info or at least the party ID). The
SetDungeonmethod returns an empty response, which is inconsistent with the rest of the API.private PartyResponse SetDungeon(long requestorId, PartyRequest.Types.SetDungeon setDungeon) { if (!partyLookup.TryGet(setDungeon.PartyId, out PartyManager? manager)) { return new PartyResponse { Error = (int) PartyError.s_party_err_not_found }; } manager.SetDungeon(requestorId, setDungeon.DungeonId, setDungeon.Set, setDungeon.DungeonRoomId); - return new PartyResponse(); + return new PartyResponse { Party = ToPartyInfo(manager.Party) }; // Or at minimum: return new PartyResponse { PartyId = setDungeon.PartyId }; }
247-248: Property naming inconsistency between internal and external models.There's a naming inconsistency between
party.DungeonLobbyRoomId(internal) andDungeonRoomId(external API). Consider aligning these names for better consistency across the codebase.return new PartyInfo { Id = party.Id, CreationTime = party.CreationTime, LeaderAccountId = party.LeaderAccountId, LeaderCharacterId = party.LeaderCharacterId, LeaderName = party.LeaderName, DungeonId = party.DungeonId, - DungeonRoomId = party.DungeonLobbyRoomId, + DungeonLobbyRoomId = party.DungeonLobbyRoomId, DungeonSet = party.DungeonSet, // Or update the property in Party class to match: // DungeonRoomId = party.DungeonRoomId, Members = { // ... }, };Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.Factory.cs (3)
35-35: Remove or explain the commented field declaration.
The commented-outprivate readonly Fields fields;seems to be a leftover and is never used. If it’s no longer needed, consider removing it to keep the code clean.
107-111: Remove or document commented-out Dungeon code.
There is a large commented block referencing a dungeon scenario that may be outdated or incomplete. If you intend to activate it later, add a clear “TODO” comment; otherwise, remove it for clarity.
60-71: Consider eliminating redundant synchronization.
You are acquiring a map-level lock and also using thread-safe ConcurrentDictionaries. In many cases, you only need the dictionary’s concurrency guarantees or the map-level semaphore, but not both. Review the concurrency model to simplify and optimize.Maple2.Server.Game/Manager/DungeonManager.cs (6)
23-24: Consider using a property for Metadata for consistency.The
Metadatafield is public but not following the property pattern used for other members likeFieldandLobbyRoomId.- public DungeonRoomTable.DungeonRoomMetadata? Metadata; + public DungeonRoomTable.DungeonRoomMetadata? Metadata { get; set; }
112-113: Improve readability by replacing complex ternary with if-else.The complex ternary operation makes the code harder to read. Consider using an if-else statement for clarity.
- session.Send(session.PrepareField(firstField.MapId, roomId: firstField.RoomId) ? FieldEnterPacket.Request(session.Player) : - FieldEnterPacket.Error(MigrationError.s_move_err_default)); + if (session.PrepareField(firstField.MapId, roomId: firstField.RoomId)) { + session.Send(FieldEnterPacket.Request(session.Player)); + } else { + session.Send(FieldEnterPacket.Error(MigrationError.s_move_err_default)); + }
58-59: Remove unnecessary empty line.There's an extra empty line in the
CreateDungeonRoommethod that can be removed for consistent formatting.} } - DungeonFieldManager? dungeonField = session.FieldFactory.CreateDungeon(metadata, session.CharacterId, Party);
42-86: Consider handling party validation in a separate method.The party validation logic in
CreateDungeonRoomcould be extracted to a separate method to improve readability and maintainability, especially if similar validation will be needed elsewhere.public void CreateDungeonRoom(int dungeonId, bool withParty) { if (!session.TableMetadata.DungeonRoomTable.Entries.TryGetValue(dungeonId, out DungeonRoomTable.DungeonRoomMetadata? metadata)) { session.Send(DungeonRoomPacket.Error(DungeonRoomError.s_room_dungeon_notOpenTimeDungeon)); return; } if (withParty) { - if (Party == null) { - session.Send(DungeonRoomPacket.Error(DungeonRoomError.s_room_dungeon_error_invalidPartyOID)); - return; - } - if (Party.LeaderCharacterId != session.CharacterId) { - session.Send(DungeonRoomPacket.Error(DungeonRoomError.s_room_party_err_not_chief)); - return; - } + if (!ValidatePartyForDungeon()) { + return; + } } DungeonFieldManager? dungeonField = session.FieldFactory.CreateDungeon(metadata, session.CharacterId, Party); // Rest of the method... } +private bool ValidatePartyForDungeon() { + if (Party == null) { + session.Send(DungeonRoomPacket.Error(DungeonRoomError.s_room_dungeon_error_invalidPartyOID)); + return false; + } + if (Party.LeaderCharacterId != session.CharacterId) { + session.Send(DungeonRoomPacket.Error(DungeonRoomError.s_room_party_err_not_chief)); + return false; + } + return true; +}
132-158: Enhance error handling in MigrateToDungeon.The method should notify the user about migration failures with more specific error messages rather than just disconnecting them. Also, consider setting an appropriate error state on the session.
private void MigrateToDungeon() { if (Metadata == null) { logger.Error("Dungeon metadata is null, cannot migrate to dungeon"); + session.Send(MigrationPacket.GameToGameError(MigrationError.s_move_err_default)); + session.Send(NoticePacket.MessageBox(new InterfaceText("Failed to prepare dungeon metadata."))); return; } try { // Existing migration code... } catch (RpcException ex) { session.Send(MigrationPacket.GameToGameError(MigrationError.s_move_err_default)); - session.Send(NoticePacket.Disconnect(new InterfaceText(ex.Message))); + session.Send(NoticePacket.MessageBox(new InterfaceText("Failed to connect to dungeon server. Please try again later."))); + logger.Error(ex, "RPC exception during dungeon migration for character {CharacterId}", session.CharacterId); } finally { session.Disconnect(); } }
1-165: Overall the implementation is well-structured but can be improved.The
DungeonManagerclass has a clear responsibility and provides a good foundation for dungeon functionality. The main areas for improvement are:
- Better error handling and user feedback
- Avoiding silent failures
- Reconsidering the ownership of resources (especially the session)
- Code organization and extraction of reusable validation logic
Once these issues are addressed, the implementation will be more robust and maintainable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Maple2.Server.World/Migrations/20250304061437_InteractCubeFix.csis excluded by!Maple2.Server.World/Migrations/*
📒 Files selected for processing (17)
Maple2.Model/Game/Party/Party.cs(2 hunks)Maple2.Server.Core/proto/channel/channel.proto(4 hunks)Maple2.Server.Core/proto/world/world.proto(6 hunks)Maple2.Server.Game/Manager/DungeonManager.cs(1 hunks)Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.Factory.cs(1 hunks)Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs(8 hunks)Maple2.Server.Game/Manager/Field/FieldManager/IField.cs(1 hunks)Maple2.Server.Game/Manager/FishingManager.cs(1 hunks)Maple2.Server.Game/Manager/PartyManager.cs(2 hunks)Maple2.Server.Game/Model/Field/Actor/FieldPlayer.cs(0 hunks)Maple2.Server.Game/PacketHandlers/PartyHandler.cs(1 hunks)Maple2.Server.Game/PacketHandlers/PartySearchHandler.cs(1 hunks)Maple2.Server.Game/Packets/PartyPacket.cs(5 hunks)Maple2.Server.Game/Service/ChannelService.Party.cs(2 hunks)Maple2.Server.Game/Session/GameSession.cs(8 hunks)Maple2.Server.World/Containers/PartyManager.cs(4 hunks)Maple2.Server.World/Service/WorldService.Party.cs(4 hunks)
💤 Files with no reviewable changes (1)
- Maple2.Server.Game/Model/Field/Actor/FieldPlayer.cs
✅ Files skipped from review due to trivial changes (2)
- Maple2.Server.Game/PacketHandlers/PartySearchHandler.cs
- Maple2.Server.Game/PacketHandlers/PartyHandler.cs
🚧 Files skipped from review as they are similar to previous changes (6)
- Maple2.Server.Game/Manager/FishingManager.cs
- Maple2.Server.Game/Packets/PartyPacket.cs
- Maple2.Server.Game/Service/ChannelService.Party.cs
- Maple2.Server.World/Containers/PartyManager.cs
- Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs
- Maple2.Server.Game/Session/GameSession.cs
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: format
🔇 Additional comments (16)
Maple2.Server.Game/Manager/Field/FieldManager/IField.cs (1)
74-77: Validate thread-safety for item-spawning methods.
These methods likely interact with the concurrent collections for entities. Verify that all spawn operations are safely handled in multi-threaded scenarios to avoid race conditions.Maple2.Server.Core/proto/channel/channel.proto (3)
160-165: New SetDungeon message structure correctly defined.The message structure includes all necessary fields to manage dungeon association for parties.
182-182: Integration of SetDungeon message into PartyRequest looks good.The field has been properly added to the Party oneof structure with an appropriate sequential number.
286-286: Consistent renaming from "instance_id" to "room_id" improves clarity.This renaming is well-applied across both the GetField and FieldInfo messages, maintaining consistency with other files in the PR.
Also applies to: 305-305
Maple2.Server.Core/proto/world/world.proto (6)
70-70: Renaming instance_id to room_id maintains consistent terminology.This renaming provides better semantic clarity about the purpose of the field and is consistently applied across multiple messages.
Also applies to: 103-103, 495-495
71-71: Improved MigrationType naming for better semantics.Changing from PlotMode to MigrationType provides clearer meaning for the purpose of this field.
Also applies to: 104-104
74-78: MigrationType enum appropriately structured.The enum includes the necessary migration types, and the addition of DUNGEON is well-integrated.
273-280: Party dungeon fields well-organized.The party dungeon-related fields are organized with sequential numbers, and the dungeon_set boolean precedes the dungeon_id which makes logical sense for field ordering.
333-338: SetDungeon message properly defined.The message definition for SetDungeon contains all necessary fields with appropriate types to handle dungeon party management.
353-353: SetDungeon integration into PartyRequest looks good.The field has been properly added to the party oneof with an appropriate sequential number.
Maple2.Model/Game/Party/Party.cs (1)
25-27: Adding dungeon related properties is appropriate.The addition of
DungeonSetandDungeonLobbyRoomIdproperties aligns with the overall changes for dungeon functionality.Maple2.Server.World/Service/WorldService.Party.cs (2)
44-45: New dungeon functionality properly integrated into the Party method switch cases.This change correctly extends the switch statement to handle the new
SetDungeoncase, following the established pattern of other case implementations.
229-237:Details
❌ Incorrect review comment
Authorization check missing in SetDungeon method.
The method doesn't verify if the requestor is authorized to set dungeon properties. Typically, this operation should be restricted to the party leader to prevent any party member from changing dungeon settings.
🏁 Script executed:
#!/bin/bash # Check if PartyManager.SetDungeon performs authorization checks # Look for leader validation in the SetDungeon method rg -A 10 "SetDungeon\(" --glob "**/*PartyManager*"Length of output: 2404
Authorization check is enforced in the lower layer: The service method passes the
requestorIdintoPartyManager.SetDungeon, and as verified inMaple2.Server.World/Containers/PartyManager.cs, theSetDungeonmethod performs the leader validation by comparingrequestorIdwithParty.LeaderCharacterId. Therefore, no additional authorization check is required in the service layer.Likely an incorrect or invalid review comment.
Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.Factory.cs (2)
42-45: Clarify or correct the bracket-based dictionary initialization.
The syntax= [];for ConcurrentDictionaries is non-standard in typical C# usage and may cause compilation issues.
123-129: Prevent potential overwriting of existing fields without disposal.
When usingAddOrUpdate, the old field gets overwritten with the new field and is never disposed. This can cause resource leaks or concurrency risks.Maple2.Server.Game/Manager/DungeonManager.cs (1)
155-157:Details
❓ Verification inconclusive
Verify if session disconnection is always intended in MigrateToDungeon.
The
session.Disconnect()is called in thefinallyblock, which means it happens even on successful migration. Verify if this is the intended behavior, as it might cause issues if the session should remain active after migration.
🏁 Script executed:
#!/bin/bash # Search for similar migration patterns in other managers rg "Migration.*finally.*Disconnect" --type csharp -A 5 -B 5Length of output: 61
ACTION REQUIRED: Confirm Session Disconnection Behavior in MigrateToDungeon
The current implementation in
Maple2.Server.Game/Manager/DungeonManager.cs(lines 155-157) unconditionally disconnects the session via:} finally { session.Disconnect(); }Because this
finallyblock ensures thatsession.Disconnect()is always invoked—even when the migration is successful—please verify that this behavior is intentional. In particular, consider the following:
- Should the session remain active after a successful migration?
- Is this pattern consistent with how other migration methods or managers handle session lifecycle management?
- Could disconnecting the session under normal conditions result in unintended side effects?
Given that our initial search for similar migration patterns did not yield conclusive references, a manual verification of the intended behavior in the context of MigrateToDungeon is advised.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Maple2.Server.Game/Manager/StatsManager.cs (1)
137-138: New debug logging for gear score calculations looks good.This addition enhances visibility into how gear scores are calculated for each equipped item, which is valuable for debugging and understanding the system. The log statement captures all relevant parameters (item ID, gear score, rarity, enchant level, and limit break level) that are used in the calculation on the following line.
Consider adding conditional logging or sampling if this generates too much output in busy servers. For example, you might want to conditionally log only for specific players during testing or limit logging frequency:
-Log.Logger.Debug("Calculating Gearscore. Item ID: {id} - Gearscore: {gearscore} - Rarity: {rarity}, Enchant Level: {enchantLevel}, Limit Break Level: {limitBreakLevel}", item.Metadata.Id, item.Metadata.Property.GearScore, item.Rarity, item.Enchant?.Enchants ?? 0, item.LimitBreak?.Level ?? 0); +if (ShouldLogDetailedGearScore()) // Define this method based on your logging needs +{ + Log.Logger.Debug("Calculating Gearscore. Item ID: {id} - Gearscore: {gearscore} - Rarity: {rarity}, Enchant Level: {enchantLevel}, Limit Break Level: {limitBreakLevel}", item.Metadata.Id, item.Metadata.Property.GearScore, item.Rarity, item.Enchant?.Enchants ?? 0, item.LimitBreak?.Level ?? 0); +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
Maple2.Server.Game/Manager/StatsManager.cs(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
3 participants
WIP? looking for feedbackSummary by CodeRabbit
New Features
DungeonRoomHandlerto manage dungeon-related packet handling.DungeonManagerclass for comprehensive dungeon functionalities, including room creation and migration handling.MigrationTypeandDungeonRoomErrorto streamline migration processes and error handling.Improvements
Stability