Uh oh!
There was an error while loading. Please reload this page.
Shop Purchase & Field Instruments Fix - #307
Conversation
WalkthroughThis pull request introduces several modifications related to instrument and shop management in the game server. A new concurrent dictionary in the FieldManager class now tracks FieldInstrument objects. Methods are updated to spawn instruments, remove them correctly (while handling score events), and send start score packets to newly added players. Additionally, the FieldInstrument class gains a nullable Score property. The InstrumentHandler’s score management is revised, and the ShopManager’s purchase logic is improved to account for zero stock. Changes
Sequence Diagram(s)sequenceDiagram
participant P as Player
participant FM as FieldManager
participant FI as FieldInstrument
participant IH as InstrumentHandler
%% When a new instrument is spawned
FM->>FI: SpawnInstrument()
FI-->>FM: Instrument created and added to dictionary
%% When a player is added to the field
P->>FM: OnAddPlayer()
FM->>P: Send StartScore packet for each instrument with a score
%% Score handling events via InstrumentHandler
IH->>FI: HandleStartScore (assign Score)
IH->>FM: HandleStopScore (call RemoveInstrument with instrument ID)
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 ( |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
Maple2.Server.Game/Manager/ShopManager.cs (2)
565-572: Fix GameMeret currency handling.The code is deducting from
Meretinstead ofGameMeretwhen the cost type isGameMeret.Apply this fix:
case ShopCurrencyType.GameMeret: if (session.Currency.CanAddGameMeret(-price) != -price) { session.Send(ShopPacket.Error(ShopError.s_err_lack_merat)); return false; } - session.Currency.Meret -= price;+ session.Currency.GameMeret -= price; break;
600-600: Fix incorrect currency deduction amount.The code uses
cost.Amountinstead ofpricefor the currency deduction, which could lead to incorrect amounts being deducted.Apply this fix:
- session.Currency[currencyType] -= cost.Amount;+ session.Currency[currencyType] -= price;
🧹 Nitpick comments (4)
Maple2.Server.Game/Manager/Field/FieldManager.State.cs (1)
644-651: Consider adding XML documentation.The implementation is correct and follows the pattern of other remove methods. Consider adding XML documentation to describe the method's purpose, parameters, and return value.
+ /// <summary>+ /// Removes an instrument from the field and broadcasts a stop score packet.+ /// </summary>+ /// <param name="objectId">The object ID of the instrument to remove.</param>+ /// <returns>True if the instrument was successfully removed; otherwise, false.</returns> public bool RemoveInstrument(int objectId) {Maple2.Server.Game/Manager/ShopManager.cs (2)
439-442: LGTM! The fix correctly handles unlimited supply items.The condition now properly allows purchases when
StockCountis 0 (unlimited supply) while maintaining the stock check for limited items.Consider adding a comment to clarify the logic:
+ // StockCount of 0 indicates unlimited supply if (shopItem.StockCount != 0 && quantity > shopItem.StockCount - shopItem.StockPurchased) {
454-454: Consider implementing missing shop features.The TODO comment indicates missing implementations for Guild Merchant Type/Level, Championship, and Alliance features.
Would you like me to help implement these features or create issues to track them?
Maple2.Server.Game/PacketHandlers/InstrumentHandler.cs (1)
149-151: Consider adding debug logging for error cases.The method silently returns when
FieldorInstrumentis null. Consider adding debug logging to help track down potential issues:if (session.Field == null || session.Instrument == null) { + Logger.Debug("HandleStopScore failed: Field={Field}, Instrument={Instrument}",+ session.Field != null, session.Instrument != null); return; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Maple2.Server.World/Migrations/20250207213214_HomeDecoration.csis excluded by!Maple2.Server.World/Migrations/*
📒 Files selected for processing (4)
Maple2.Server.Game/Manager/Field/FieldManager.State.cs(4 hunks)Maple2.Server.Game/Manager/ShopManager.cs(1 hunks)Maple2.Server.Game/Model/Field/Entity/FieldInstrument.cs(2 hunks)Maple2.Server.Game/PacketHandlers/InstrumentHandler.cs(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: format
- GitHub Check: build
🔇 Additional comments (8)
Maple2.Server.Game/Manager/Field/FieldManager.State.cs (3)
34-34: LGTM!The use of
ConcurrentDictionaryis appropriate for thread-safe management of field instruments.
503-503: LGTM!The instrument storage implementation follows the established pattern used by other spawn methods in the class.
664-668: LGTM!The implementation correctly handles sending score packets to newly added players, fixing the issue where field instruments were not loading correctly when a player enters a map where a score is in progress.
Maple2.Server.Game/PacketHandlers/InstrumentHandler.cs (3)
141-141: LGTM! Important state management fix.Setting the Score property on the instrument is crucial for maintaining the correct state, which helps fix the issue where field instruments weren't loading correctly for players entering a map during an active score.
172-172: LGTM! Improved cleanup of instruments.Using
RemoveInstrumentinstead of just broadcasting a stop packet ensures proper cleanup of the instrument state from the field, preventing potential issues with lingering instruments.
337-337: LGTM! Removed redundant null check.The removal of the
!operator is correct sincesession.Fieldis already guaranteed to be non-null at this point due to the null checks in the calling methods.Maple2.Server.Game/Model/Field/Entity/FieldInstrument.cs (2)
1-2: LGTM!The addition of the
Maple2.Model.Gamenamespace and reordering of using directives looks good.
12-12: LGTM! The Score property addition aligns with the PR objectives.The nullable
Scoreproperty will help track the score being played when a player enters a map where a score is in progress, addressing the field instruments loading issue mentioned in the PR objectives.Let's verify the usage of this property:
✅ Verification successful
🏁 Script executed:
#!/bin/bash# Description: Verify the Score property usage in the codebase# Expected: Usage in InstrumentHandler for managing scores# Search for Score property usage rg -A 5 "Score\s*="# Search for any potential null reference issues ast-grep --pattern 'session.Instrument.Score.$$$'Length of output: 12433
Verified: Score Property Usage Confirmed
The search results confirm that the new nullable
Scoreproperty is utilized correctly in the codebase, notably inInstrumentHandler(specifically at the assignmentsession.Instrument.Score = score;). This usage aligns with the PR's objectives to track active scores without introducing null reference issues.
Zintixx
commented
Feb 10, 2025
yes |
Summary by CodeRabbit
New Features
Bug Fixes