Multiple fixes - #358
Conversation
WalkthroughThis pull request applies several improvements across the Maple2 server code. In the game manager and packet handler modules, loop conditions, command enum values, and conditional checks have been refined to prevent runtime errors and ensure proper control flow. The changes also add thread safety and immediate database persistence when spawning items, adjust channel lookup and migration logic in the world service, and introduce a command-line override for instanced content. Additional tests have been added to verify the sorting behavior of item collections without altering public interfaces. Changes
Sequence Diagram(s)sequenceDiagram
participant Session
participant QuestHandler
participant GameInstance
Session->>QuestHandler: Send MapleGuide command with metadata
QuestHandler->>QuestHandler: Check if GoToMapId == DefaultHomeMapId
alt Is DefaultHomeMapId
QuestHandler->>Session: MigrateToInstance(DefaultHomeMapId)
QuestHandler-->>Session: Return early
else Not DefaultHomeMapId
QuestHandler->>QuestHandler: Prepare field enter packet
QuestHandler->>Session: Send enter packet
end
sequenceDiagram
participant Request
participant WorldService
participant ChannelService
Request->>WorldService: Initiate MigrateOut(request)
alt request.InstancedContent is true
WorldService->>ChannelService: Retrieve instanced channel ID
alt Channel is active?
WorldService->>WorldService: Assign channel for migration
else
WorldService->>Request: Throw RpcException ("No available instanced game channel")
end
else
alt Specified channel is active?
WorldService->>WorldService: Use specified channel
else
WorldService->>ChannelService: Find first available channel
alt Channel found
WorldService->>WorldService: Assign channel
else
WorldService->>Request: Throw RpcException ("No available game channels")
end
end
end
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
⏰ Context from checks skipped due to timeout of 90000ms (2)
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
Maple2.Server.Game/PacketHandlers/MasteryHandler.cs (1)
84-89: Logic flow improved for quest requirement validationThe conditional logic has been simplified and improved. Instead of checking both the quest existence and completion state, the code now uses a more straightforward approach: if the quest exists in the player's quest log, continue to the next quest; otherwise, send an error.
This change enhances readability and reduces nesting. However, there's a potential logic change - the old code may have been verifying quest completion status as well, whereas now it's only verifying quest existence.
Consider adding a comment to clarify the intended behavior here:
foreach (int questId in entry.RequiredQuests) { + // Only check if player has the quest in their log, not its completion status if (session.Quest.TryGetQuest(questId, out _)) continue; session.Send(MasteryPacket.Error(MasteryError.s_mastery_error_lack_quest)); return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
Maple2.Server.Game/Manager/Items/InventoryManager.cs(1 hunks)Maple2.Server.Game/Manager/Items/ItemCollection.cs(1 hunks)Maple2.Server.Game/PacketHandlers/HomeBankHandler.cs(2 hunks)Maple2.Server.Game/PacketHandlers/ItemInventoryHandler.cs(1 hunks)Maple2.Server.Game/PacketHandlers/MasteryHandler.cs(1 hunks)Maple2.Server.Game/PacketHandlers/QuestHandler.cs(1 hunks)Maple2.Server.Game/Program.cs(1 hunks)Maple2.Server.Tests/Game/Manager/Item/ItemCollectionTest.cs(2 hunks)Maple2.Server.World/Containers/ChannelClientLookup.cs(1 hunks)Maple2.Server.World/Service/WorldService.Migrate.cs(2 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: format
- GitHub Check: build
🔇 Additional comments (19)
Maple2.Server.Game/Program.cs (2)
42-43: Command-line argument added for instanced content controlAdding a parameter to force instanced content mode is a good enhancement that improves flexibility when starting the server.
52-52: Correctly implements the instanced content overrideThe use of logical OR here is appropriate - it ensures that InstancedContent will be true either when the flag is provided or when it's configured in Target.InstancedContent.
Maple2.Server.World/Containers/ChannelClientLookup.cs (1)
70-70: Fixed channel selection logic for instanced contentThis change properly fixes the fallback mechanism by ensuring that channels are matched not only by IP address and inactive status but also by their instanced content property. This prevents mismatches between instanced and non-instanced channels during migration.
Maple2.Server.World/Service/WorldService.Migrate.cs (5)
6-6: Appropriate addition of Core.Helpers namespaceThis import is needed for the ChannelClientCollection extension methods used later in the file, notably
TryGetInstancedChannelIdandFirstChannel.
36-41: Improved handling for instanced content channelsThe new code correctly prioritizes instanced channels when
request.InstancedContentis true, and provides a clear error message when no instanced game channels are available. This enhances the robustness of the migration process.
41-43: Clear fallback logic for explicit channel requestsThis conditional block properly handles the case when a specific channel is requested and available, providing a logical fallback path for non-instanced content.
44-49: Enhanced fallback mechanism with proper error handlingThe fallback to the first available channel is now clearly documented with a comment, and includes proper error handling when no channels are available. The channel initialization is properly handled in all code paths.
52-52: Improved error message specificityThe error message now includes the channel number, which makes debugging easier by providing more specific information about which channel couldn't be found.
Maple2.Server.Tests/Game/Manager/Item/ItemCollectionTest.cs (4)
260-265: Code formatting improved for readabilityThe array initialization in the CollectionAssert is now properly formatted with each item on a separate line, improving readability.
267-298: Good addition of test for full inventory sortingThis test validates that sorting works correctly when the inventory is at maximum capacity. This is important since the related bug fix in ItemCollection.Sort() addresses boundary conditions.
300-323: Good test for sorting with null items (gaps)This test ensures that sorting works correctly when there are gaps (null slots) in the inventory, validating that null items don't affect the sorting behavior. The test effectively verifies that items are properly compacted to the beginning of the collection.
325-355: Good test for sorting after removing an itemThis test verifies that sorting correctly handles the case where an item has been removed from the collection, creating a gap. This helps validate the bug fix in the Sort method that prevents IndexOutOfRangeException.
Maple2.Server.Game/Manager/Items/InventoryManager.cs (1)
653-655: Good addition of utility method for marking items for deletionThis new method encapsulates the functionality of adding an item to the deletion list, improving code organization by providing a dedicated public API instead of directly accessing the delete list from outside classes.
This method aligns with the existing pattern in the class where delete.Add() is encapsulated by the Discard method, but provides an alternative when you want to mark an item for deletion without the additional logic in Discard.
Maple2.Server.Game/PacketHandlers/ItemInventoryHandler.cs (2)
111-111: Bound items now properly discardedThe condition has been expanded to also discard items that have the
TransferFlag.Bindflag set. This ensures bound items are handled correctly when dropped, preventing potential exploits that could bypass binding restrictions.
116-116: Item tracking improved with deletion markingThe new call to
AddItemToDeletemarks the item for deletion before it's spawned in the field, which helps with inventory management and ensures consistent state tracking. This likely addresses issue #353 mentioned in the PR objectives about bank safe functionality.Maple2.Server.Game/PacketHandlers/QuestHandler.cs (1)
204-207: Special handling for default home map migrationThis new condition provides specialized handling when the destination map is the default home map. By migrating directly to an instanced version of the home map, the code ensures that players are correctly placed in their personal home instance rather than a shared map.
This change aligns with the PR objective of fixing "the fallback mechanism to the first available channel when migrating instances."
Maple2.Server.Game/PacketHandlers/HomeBankHandler.cs (3)
14-15: Updated enum values for bank commandsThe command values for
HomeandPremiumhave been updated, likely to match the client's expected command values. This change helps ensure correct packet handling and prevents potential misinterpretation of commands.
31-31: Simplified premium bank accessThe call to
HomeBank()for the Premium command has been simplified by removing the time parameter, as premium bank access doesn't need the cooldown time tracking that the regular home bank requires.
36-36: Enhanced method flexibility with default parameterThe
HomeBankmethod now includes a default parameter value, making it more flexible and allowing for cleaner code when the time parameter isn't needed (as in the Premium case). This is a good refactoring that simplifies the code while maintaining backward compatibility.
| session.Send(MasteryPacket.Error(MasteryError.s_mastery_error_lack_quest)); | ||
| return; | ||
| } | ||
| if (session.Quest.TryGetQuest(questId, out _)) continue; |
There was a problem hiding this comment.
isnt this supposed to be the reverse?
There was a problem hiding this comment.
If it found a quest, then it's fine. If not send error
There was a problem hiding this comment.
then it should break, ya? instead of continue
There was a problem hiding this comment.
If we break we won't check the next quests. If it loops through all quests and they are found we can keep going, if any of them are not found then we send the error and exit early
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs (1)
199-202: Improved thread safety and database persistence when spawning items.The changes add two important improvements:
- Thread safety through locking the
itemobject during database operations- Immediate persistence of spawned items to the database
This fixes potential race conditions and ensures items aren't lost during server restarts or instance migrations, which appears to address the bank safe functionality issue mentioned in the PR objectives.
However, the hardcoded
0parameter indb.SaveItems(0, item)could benefit from clarification:- db.SaveItems(0, item); + // The first parameter (0) represents [explanation of what 0 means] + db.SaveItems(0, item);Or consider using a named constant instead of the magic number.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs(2 hunks)Maple2.Server.Game/PacketHandlers/ItemInventoryHandler.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- Maple2.Server.Game/PacketHandlers/ItemInventoryHandler.cs
🔇 Additional comments (1)
Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs (1)
6-6: Required import added for database operations.The addition of the
Maple2.Database.Storagenamespace import is necessary to support the new database persistence functionality in theSpawnItemmethod.
Co-authored-by: Zin <62830952+Zintixx@users.noreply.github.com>
2 participants
dotnet run --instancedSummary by CodeRabbit
Bug Fixes
New Features
Tests