Misc Additions - #478
Conversation
|
""" WalkthroughThis update introduces new features and refactors across multiple components. It adds language configuration support for ingestion tools, implements new metadata tables (SmartPush and AutoAction), introduces quest-specific portals with ownership logic, expands channel update mechanisms, and refines item, event, and mastery management. Several enums, records, and packet structures are extended to support these features. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant IngestTool
participant Mapper
participant Parser
User->>IngestTool: Set LANGUAGE env variable
IngestTool->>Mapper: Instantiate with language param
Mapper->>Parser: Pass language to parser
Parser-->>Mapper: Parse data in specified language
Mapper-->>IngestTool: Return mapped data
sequenceDiagram
participant GameSession
participant SmartPushHandler
participant Metadata
participant Inventory
participant Player
GameSession->>SmartPushHandler: Receive SmartPush packet
SmartPushHandler->>Metadata: Lookup SmartPushMetadata
alt AutoInteraction
SmartPushHandler->>Inventory: Check and consume required items
SmartPushHandler->>Player: Attempt gather actions
SmartPushHandler->>GameSession: Send gather activation packet
else AdditionalEffect
SmartPushHandler->>Inventory: Check and consume required items
SmartPushHandler->>Player: Deduct currency, apply buff/effect
SmartPushHandler->>GameSession: Send effect activation packet
end
sequenceDiagram
participant QuestManager
participant FieldManager
participant FieldQuestPortal
participant Player
QuestManager->>FieldManager: SpawnPortal with QuestSummonPortal
FieldManager->>FieldQuestPortal: Create portal instance (owner = Player)
FieldManager->>Player: Send portal add/summon packets
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (3)
✨ 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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 13
🔭 Outside diff range comments (1)
Maple2.File.Ingest/Mapper/AchievementMapper.cs (1)
14-16: 🛠️ Refactor suggestionConsider adding language parameter for consistency.
Based on the AI summary, other mapper classes like
ItemMapper,NpcMapper,MapMapper,QuestMapper,ScriptMapper,SkillMapper, andTableMapperwere updated to accept alanguageparameter. TheAchievementMapperconstructor should likely be updated for consistency with this pattern.Apply this diff to align with the language support pattern:
- public AchievementMapper(M2dReader xmlReader) { - parser = new AchieveParser(xmlReader); + public AchievementMapper(M2dReader xmlReader, string language) { + parser = new AchieveParser(xmlReader, language);
🧹 Nitpick comments (12)
Maple2.Model/Enum/CurrencyType.cs (1)
16-20: Maintain enum consistency with underlying type and file organization
- Other currency enums explicitly use
bytefor memory alignment—add: bytetoSmartPushCurrencyType.- For clarity and maintainability, consider moving
SmartPushCurrencyTypeinto its ownSmartPushCurrencyType.csfile under the same namespace.Suggested diff:
-public enum SmartPushCurrencyType { +public enum SmartPushCurrencyType : byte { None = 0, Meso = 1, Meret = 2, }Maple2.Server.Game/Manager/Items/InventoryManager.cs (1)
420-422: Good interface abstraction, but inconsistent material collection approaches.The change to
IList<Item>interface and using theFiltermethod with proper expiration, ID, and rarity filtering is an improvement. However, the same method uses two different approaches for collecting materials:
- New approach (lines 420-422): Uses
Filtermethod formaterialsById- Old approach (lines 427, 431): Uses
session.Item.Inventory.FindformaterialsByTagConsider refactoring for consistency:
foreach (ItemComponent ingredient in components) { if (materialsByTag.TryGetValue(ingredient.Tag, out IList<Item>? value)) { - foreach (Item item in session.Item.Inventory.Find(ingredient.ItemId, ingredient.Rarity)) { + foreach (Item item in Filter(item => !item.IsExpired() && item.Id == ingredient.ItemId && item.Rarity == ingredient.Rarity)) { value.Add(item); } } else { - materialsByTag.Add(ingredient.Tag, session.Item.Inventory.Find(ingredient.ItemId, ingredient.Rarity).ToList()); + materialsByTag.Add(ingredient.Tag, Filter(item => !item.IsExpired() && item.Id == ingredient.ItemId && item.Rarity == ingredient.Rarity)); } }Maple2.Server.Game/Service/ChannelService.Sync.cs (1)
28-32: Consider adding input validation and error handling.The implementation is functionally correct, but could be more robust:
- Data type conversion: Converting
int32toshortcould cause truncation if channel IDs exceed the short range (-32,768 to 32,767).- No input validation: The method doesn't validate that the channels list is non-empty or contains valid channel IDs.
- Missing error handling: Broadcasting exceptions are not caught.
Consider this enhanced implementation:
public override Task<ChannelsUpdateResponse> UpdateChannels(ChannelsUpdateRequest request, ServerCallContext context) { + if (request.Channels.Count == 0) { + return Task.FromResult(new ChannelsUpdateResponse { Error = 1 }); + } + + try { List<short> channels = request.Channels.Select(channel => (short) channel).ToList(); server.Broadcast(ChannelPacket.Update(channels)); return Task.FromResult(new ChannelsUpdateResponse()); + } catch (Exception ex) { + Logger.Error(ex, "Failed to broadcast channel updates"); + return Task.FromResult(new ChannelsUpdateResponse { Error = 1 }); + } }Maple2.Server.World/Containers/ChannelClientLookup.cs (2)
274-285: Fix the typo in parameter name.There's a typo in the parameter name
exclueChannelBroadcast- it should beexcludeChannelBroadcast.- private void UpdateChannels(int exclueChannelBroadcast = -1) { + private void UpdateChannels(int excludeChannelBroadcast = -1) { foreach ((int id, ChannelClient channelClient) in this) { - if (id == exclueChannelBroadcast) { + if (id == excludeChannelBroadcast) { continue; }
274-285: Fix parameter name typoThere's a typo in the parameter name.
-private void UpdateChannels(int exclueChannelBroadcast = -1) { +private void UpdateChannels(int excludeChannelBroadcast = -1) { foreach ((int id, ChannelClient channelClient) in this) { - if (id == exclueChannelBroadcast) { + if (id == excludeChannelBroadcast) { continue; }Maple2.Model/Game/Event/GameEvent.cs (1)
238-247: LGTM! Proper implementation of new event serialization cases.The new cases correctly serialize the event data following the established pattern. The
WriteInt()call without arguments on line 245 intentionally writes 0 as the default value.Consider adding a comment or explicit parameter for clarity:
- writer.WriteInt(); + writer.WriteInt(0); // No discount for instrument eventsMaple2.File.Ingest/Mapper/ItemMapper.cs (1)
16-16: Consider removing unused field.The
newXmlfield is stored but not used anywhere in the visible code. If this field isn't utilized elsewhere in the class, consider removing it to avoid dead code.Maple2.Server.Game/Manager/QuestManager.cs (1)
590-602: Consider removing redundant null check and add error handling.The implementation is solid but has a redundant null check since the caller already verifies
quest.Metadata.SummonPortal != null.Consider this refactor to remove redundancy and add error handling:
private void SummonPortal(Quest quest) { - if (quest.Metadata.SummonPortal == null) { - return; - } - if (session.NpcScript?.Npc == null) { return; } - FieldPortal portal = session.Field.SpawnPortal(quest.Metadata.SummonPortal, session.NpcScript.Npc, session.Player); - session.Send(PortalPacket.Add(portal)); - session.Send(QuestPacket.SummonPortal(session.NpcScript.Npc.ObjectId, portal.Value.Id, portal.StartTick)); + try { + FieldPortal portal = session.Field.SpawnPortal(quest.Metadata.SummonPortal, session.NpcScript.Npc, session.Player); + session.Send(PortalPacket.Add(portal)); + session.Send(QuestPacket.SummonPortal(session.NpcScript.Npc.ObjectId, portal.Value.Id, portal.StartTick)); + } catch (Exception ex) { + logger.Error(ex, "Failed to summon quest portal for quest {QuestId}", quest.Id); + } }Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs (2)
863-873: Improve code style by removing unnecessary continue statements.The logic correctly handles quest portal visibility but can be simplified for better readability.
foreach (FieldPortal fieldPortal in fieldPortals.Values) { - switch (fieldPortal) { - case FieldQuestPortal questPortal: - if (questPortal.Owner.ObjectId == added.ObjectId) { - added.Session.Send(PortalPacket.Add(questPortal)); - } - continue; - default: - added.Session.Send(PortalPacket.Add(fieldPortal)); - continue; - } + switch (fieldPortal) { + case FieldQuestPortal questPortal: + if (questPortal.Owner.ObjectId == added.ObjectId) { + added.Session.Send(PortalPacket.Add(questPortal)); + } + break; + default: + added.Session.Send(PortalPacket.Add(fieldPortal)); + break; + } }
863-872: Portal visibility logic correctly implements ownership model.The switch statement properly restricts quest portal visibility to owners while maintaining broadcast behavior for regular portals.
Consider removing the redundant
continuestatements since there's no code after the switch:switch (fieldPortal) { case FieldQuestPortal questPortal: if (questPortal.Owner.ObjectId == added.ObjectId) { added.Session.Send(PortalPacket.Add(questPortal)); } - continue; + break; default: added.Session.Send(PortalPacket.Add(fieldPortal)); - continue; + break; }Maple2.Server.Game/PacketHandlers/SmartPushHandler.cs (1)
70-70: Consider using constants for content type strings.The hardcoded strings "AutoFishing" and "AutoPlayInstrument" should be defined as constants.
+ private const string AutoFishingContent = "AutoFishing"; + private const string AutoPlayInstrumentContent = "AutoPlayInstrument"; + private void HandleAdditionalEffect(GameSession session, IByteReader packet, SmartPushMetadata metadata) { int packageId = packet.ReadInt(); if (metadata.RequiredItem.Tag != ItemTag.None && !session.Item.Inventory.Consume([metadata.RequiredItem])) { return; } - if (metadata.Content is "AutoFishing" or "AutoPlayInstrument") { + if (metadata.Content is AutoFishingContent or AutoPlayInstrumentContent) { if (!AutoActionPackage(session, metadata, packageId)) { return; }And update the switch statement accordingly:
string content = smartPushMetadata.Content; switch (content) { - case "AutoFishing": + case AutoFishingContent: if (session.FindEvent(GameEventType.SaleAutoFishing).FirstOrDefault()?.Metadata.Data is SaleAutoFishing saleAutoFishing) { content = saleAutoFishing.ContentType; } break; - case "AutoPlayInstrument": + case AutoPlayInstrumentContent:Also applies to: 87-87, 92-92
Maple2.Server.Game/Manager/MasteryManager.cs (1)
192-204: Consider making BeforeGather private.Since
BeforeGatheris only called internally fromGatherCommon, it should be private to maintain proper encapsulation.- public void BeforeGather(MasteryRecipeTable.Entry recipeMetadata) { + private void BeforeGather(MasteryRecipeTable.Entry recipeMetadata) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (46)
.env.example(1 hunks)Maple2.Database/Context/MetadataContext.cs(1 hunks)Maple2.Database/Storage/Metadata/TableMetadataStorage.cs(3 hunks)Maple2.File.Ingest/Maple2.File.Ingest.csproj(1 hunks)Maple2.File.Ingest/Mapper/AchievementMapper.cs(1 hunks)Maple2.File.Ingest/Mapper/AdditionalEffectMapper.cs(1 hunks)Maple2.File.Ingest/Mapper/ItemMapper.cs(2 hunks)Maple2.File.Ingest/Mapper/MapMapper.cs(1 hunks)Maple2.File.Ingest/Mapper/NpcMapper.cs(1 hunks)Maple2.File.Ingest/Mapper/QuestMapper.cs(2 hunks)Maple2.File.Ingest/Mapper/ScriptMapper.cs(1 hunks)Maple2.File.Ingest/Mapper/ServerTableMapper.cs(2 hunks)Maple2.File.Ingest/Mapper/SkillMapper.cs(1 hunks)Maple2.File.Ingest/Mapper/TableMapper.cs(3 hunks)Maple2.File.Ingest/Program.cs(3 hunks)Maple2.Model/Common/TableNames.cs(1 hunks)Maple2.Model/Enum/CurrencyType.cs(1 hunks)Maple2.Model/Enum/Portal.cs(1 hunks)Maple2.Model/Enum/SmartPushType.cs(1 hunks)Maple2.Model/Game/Event/GameEvent.cs(1 hunks)Maple2.Model/Metadata/AdditionalEffectMetadata.cs(1 hunks)Maple2.Model/Metadata/QuestMetadata.cs(2 hunks)Maple2.Model/Metadata/ServerTable/GameEventTable.cs(2 hunks)Maple2.Model/Metadata/Table/AutoActionTable.cs(1 hunks)Maple2.Model/Metadata/Table/ItemSocketTable.cs(1 hunks)Maple2.Model/Metadata/Table/SmartPushTable.cs(1 hunks)Maple2.Model/Metadata/TableMetadata.cs(1 hunks)Maple2.Server.Core/proto/channel/channel.proto(2 hunks)Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs(4 hunks)Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs(1 hunks)Maple2.Server.Game/Manager/Items/InventoryManager.cs(1 hunks)Maple2.Server.Game/Manager/MasteryManager.cs(3 hunks)Maple2.Server.Game/Manager/QuestManager.cs(4 hunks)Maple2.Server.Game/Model/Field/Entity/FieldPortal.cs(1 hunks)Maple2.Server.Game/Model/Field/Entity/FieldQuestPortal.cs(1 hunks)Maple2.Server.Game/PacketHandlers/ChannelHandler.cs(1 hunks)Maple2.Server.Game/PacketHandlers/SmartPushHandler.cs(1 hunks)Maple2.Server.Game/Packets/ChannelPacket.cs(1 hunks)Maple2.Server.Game/Packets/PortalPacket.cs(1 hunks)Maple2.Server.Game/Packets/QuestPacket.cs(2 hunks)Maple2.Server.Game/Packets/SkillMacroPacket.cs(2 hunks)Maple2.Server.Game/Packets/SmartPushPacket.cs(1 hunks)Maple2.Server.Game/Service/ChannelService.Sync.cs(2 hunks)Maple2.Server.Game/Session/GameSession.cs(2 hunks)Maple2.Server.World/Containers/ChannelClientLookup.cs(4 hunks)Maple2.Server.World/Containers/PlayerConfigLookUp.cs(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (9)
Maple2.Server.Game/Packets/PortalPacket.cs (1)
Maple2.Server.Core/Helpers/DebugByteWriter.cs (1)
WriteInt(70-73)
Maple2.Database/Context/MetadataContext.cs (1)
Maple2.Server.Game/Manager/QuestManager.cs (1)
SummonPortal(590-602)
Maple2.Model/Metadata/QuestMetadata.cs (1)
Maple2.Server.Game/Manager/QuestManager.cs (1)
SummonPortal(590-602)
Maple2.Model/Metadata/TableMetadata.cs (1)
Maple2.File.Ingest/Mapper/TableMapper.cs (2)
SmartPushTable(1781-1805)AutoActionTable(1807-1825)
Maple2.Model/Game/Event/GameEvent.cs (1)
Maple2.Server.Core/Helpers/DebugByteWriter.cs (1)
WriteInt(70-73)
Maple2.File.Ingest/Mapper/QuestMapper.cs (1)
Maple2.Server.Game/Manager/QuestManager.cs (1)
SummonPortal(590-602)
Maple2.Model/Metadata/Table/AutoActionTable.cs (1)
Maple2.File.Ingest/Mapper/TableMapper.cs (1)
AutoActionTable(1807-1825)
Maple2.Database/Storage/Metadata/TableMetadataStorage.cs (1)
Maple2.File.Ingest/Mapper/TableMapper.cs (2)
SmartPushTable(1781-1805)AutoActionTable(1807-1825)
Maple2.Server.Game/Manager/MasteryManager.cs (7)
Maple2.Server.Core/Network/Session.cs (2)
Send(129-129)Send(131-131)Maple2.Server.Game/Packets/InteractObjectPacket.cs (1)
InteractObjectPacket(11-129)Maple2.Server.Game/Model/Field/Entity/FieldFunctionInteract.cs (2)
FieldFunctionInteract(9-47)FieldFunctionInteract(15-19)Maple2.Server.Game/Packets/FunctionCubePacket.cs (1)
FunctionCubePacket(12-88)Maple2.Server.Game/Packets/MasteryPacket.cs (1)
MasteryPacket(10-61)Maple2.Server.Game/Manager/Field/FieldManager/HomeFieldManager.cs (2)
HomeFieldManager(9-42)HomeFieldManager(15-18)Maple2.Server.Game/LuaFunctions/Lua.cs (2)
Lua(11-3173)CalcGatheringObjectSuccessRate(621-679)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: format
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (73)
Maple2.Model/Metadata/Table/ItemSocketTable.cs (3)
1-2: EnsureIReadOnlyDictionarynamespace is availableThe
using System.Collections.Generic;directive was removed, butIReadOnlyDictionary<>lives in that namespace. Confirm that your project/global usings include it; otherwise re-add theusingstatement so the file compiles.
3-3: ApproveItemSocketTabledeclarationThe file-scoped namespace and the concise record for
ItemSocketTableare correctly defined and match project style.
5-5: ApproveItemSocketMetadatadeclarationThe
ItemSocketMetadatarecord cleanly encapsulates its data fields with minimal boilerplate.Maple2.Server.Game/Session/GameSession.cs (1)
286-286: LGTM! Consistent refactoring from Dynamic to Load method.The replacement of
ChannelPacket.DynamicwithChannelPacket.Loadis consistent with the broader channel packet refactoring and maintains the same functionality for initial channel loading scenarios.Also applies to: 482-482
Maple2.Server.Game/PacketHandlers/ChannelHandler.cs (1)
49-49: LGTM! Consistent with channel packet refactoring.The change from
ChannelPacket.DynamictoChannelPacket.Loadmaintains consistency with the refactoring and is appropriate for error recovery scenarios where the client needs the full channel list.Maple2.Server.Core/proto/channel/channel.proto (2)
10-11: LGTM! Well-designed RPC method for channel updates.The new
UpdateChannelsRPC method provides a clean interface for broadcasting channel availability changes to connected clients.
53-59: LGTM! Simple and appropriate message structure.The
ChannelsUpdateRequestandChannelsUpdateResponsemessages are well-designed with the request containing the channel list and response providing error status.Maple2.Server.Game/Service/ChannelService.Sync.cs (1)
5-5: LGTM! Required import for packet creation.The new using directive is necessary for accessing
ChannelPacket.Updatemethod.Maple2.Server.Game/Packets/SkillMacroPacket.cs (3)
15-35: LGTM! Method implementations are correctly swapped.The method names and their corresponding command enum values are now properly aligned. Both methods maintain identical logic patterns which is appropriate for packet serialization.
10-13:Details
❓ Verification inconclusive
Verify the enum value changes align with client protocol expectations.
The command values for
UpdateandLoadwere swapped (Updatechanged from 2 to 0,Loadchanged from 0 to 2). This appears to be intentional but could break client compatibility if not coordinated with client-side changes.
🏁 Script executed:
#!/bin/bash # Search for any client-side references or documentation about these command values rg -A 3 -B 3 "SkillMacro.*Command|Command.*SkillMacro"Length of output: 56
🏁 Script executed:
#!/bin/bash # Compare the Command enums in SkillMacroPacket and ChannelPacket echo "=== SkillMacroPacket Command enum ===" rg -n "private enum Command" -A3 -B3 Maple2.Server.Game/Packets/SkillMacroPacket.cs echo echo "=== ChannelPacket Command enum ===" rg -n "private enum Command" -A3 -B3 Maple2.Server.Game/Packets/ChannelPacket.csLength of output: 683
Confirm SkillMacroPacket enum values with protocol spec
I didn’t find any client‐side references for these command bytes. SkillMacroPacket now defines:
- Update = 0
- Load = 2
By comparison, ChannelPacket uses Load = 0 and Update = 1, indicating each packet has its own numbering. Without protocol docs or client implementation verification, this change could break compatibility. Please verify these numeric codes against the official Maple2 client protocol or test against client logs.
• File: Maple2.Server.Game/Packets/SkillMacroPacket.cs
• Lines: 10–13
10-35: LGTM! Command standardization looks correctThe enum value swap and corresponding method name changes appear to be part of a systematic refactor to standardize command values across packet types. The implementation logic remains consistent.
Maple2.Server.World/Containers/ChannelClientLookup.cs (4)
236-240: LGTM! Clean extraction of deactivation logic.The
Inactivemethod properly encapsulates the channel deactivation workflow: setting status, updating players to offline, and broadcasting channel updates.
242-254: LGTM! Good separation of activation concerns.The
Activemethod properly handles channel activation by setting status, broadcasting updates, and loading custom string boards. The logic is well-organized and clear.
187-187:Details
✅ Verification successful
Verify integration with new packet structure.
The call to
Active(channel)method uses the newUpdateChannelsmethod which should integrate with the newChannelPacket.Updatestructure. Ensure this integration works as expected.
🏁 Script executed:
#!/bin/bash # Verify the UpdateChannels RPC method implementation rg -A 10 "UpdateChannels.*ChannelsUpdateRequest"Length of output: 2763
🏁 Script executed:
#!/bin/bash rg -n "Active\(" Maple2.Server.World/Containers/ChannelClientLookup.cs -A 10 -B 10Length of output: 2140
#!/bin/bash rg -n "UpdateChannels" Maple2.Server.World/Containers/ChannelClientLookup.cs -A 5 -B 5
Integration Verified:
UpdateChannelsUses NewChannelPacket.UpdateStructureI’ve confirmed that:
ChannelClientLookup.UpdateChannels(int channelId)builds aChannelsUpdateRequestand callschannelClient.UpdateChannels(...).- The server-side handler in
ChannelService.Sync.csaccepts this request, converts the channel IDs to shorts, and broadcasts usingChannelPacket.Update(channels).No further changes are needed—the new packet structure is correctly wired through the RPC.
236-254: LGTM! Good refactoring to extract channel state managementThe extraction of activation and deactivation logic into separate methods improves code readability and reduces duplication. The logic correctly handles status updates and associated side effects.
Maple2.Server.World/Containers/PlayerConfigLookUp.cs (1)
140-143: LGTM! Excellent defensive programming.The additional check after decrementing
MsRemainingproperly handles the edge case where the remaining time becomes zero or negative after the calculation, ensuring immediate cleanup of expired death entries.Maple2.Model/Enum/SmartPushType.cs (1)
3-7: LGTM! Clean enum implementation.The enum values are descriptive and follow proper C# naming conventions. The implementation is straightforward and correct.
Maple2.Model/Metadata/TableMetadata.cs (1)
86-87: LGTM! Proper extension of polymorphic JSON serialization.The new JsonDerivedType attributes follow the established pattern and correctly register the new table types for JSON deserialization.
Maple2.Model/Common/TableNames.cs (1)
43-44: LGTM! Constants follow established naming conventions.The new table name constants are consistent with existing patterns and properly placed in the common table names section.
Maple2.Model/Metadata/Table/SmartPushTable.cs (2)
6-6: Well-designed table structure.The
SmartPushTablefollows the established pattern of inheriting fromTablewith a read-only dictionary for efficient metadata lookup.
8-14: Comprehensive metadata structure with appropriate types.The
SmartPushMetadatarecord has well-chosen property types:
longfor currency values handles large amountsIngredientInfointegrates with the existing item system- Enum type provides type safety for smart push categories
Maple2.Server.Game/Packets/SmartPushPacket.cs (3)
8-11: Clean packet structure following established patterns.The static class design and internal Command enum follow the codebase's packet creation conventions.
13-20: Well-structured effect activation packet.The
ActivateEffectmethod appropriately handles currency type and buff ID parameters for effect-based smart push actions.
22-29: Appropriate gather action packet structure.The
ActivateGathermethod handles item ID and amount parameters suitable for gathering-type smart push actions.Maple2.Model/Metadata/Table/AutoActionTable.cs (2)
3-3: Appropriate nested dictionary structure.The nested dictionary design (
string -> int -> AutoActionMetaData) aligns well with the parsing logic that groups auto actions by content type and then maps by ID.
5-10: Comprehensive auto action metadata with proper types.The
AutoActionMetaDatarecord includes all necessary properties:
Durationasintis appropriate for time values in game contextslongtypes for currency costs handle large monetary values- Clean separation of meret and meso costs
Maple2.Database/Storage/Metadata/TableMetadataStorage.cs (1)
49-51: LGTM! Consistent implementation following established patterns.The new metadata table additions follow the existing lazy-loading pattern correctly:
- Private readonly fields use consistent naming conventions
- Public properties expose the lazy-loaded values properly
- Constructor initialization uses the established
Retrieve<T>method with appropriate table name constantsAlso applies to: 115-117, 200-202
Maple2.File.Ingest/Maple2.File.Ingest.csproj (1)
22-22:Details
✅ Verification successful
Verify package version compatibility and availability.
The version update from 2.2.9 to 2.3.2 should be verified to ensure it's available and compatible with the new language-specific parsing features mentioned in the PR.
🏁 Script executed:
#!/bin/bash # Description: Verify the availability and recent changes of the Maple2.File.Parser.Tadeucci package # Check if version 2.3.2 exists on NuGet curl -s "https://api.nuget.org/v3-flatcontainer/maple2.file.parser.tadeucci/index.json" | jq '.versions | map(select(. == "2.3.2"))' # Get package information for version comparison curl -s "https://api.nuget.org/v3-flatcontainer/maple2.file.parser.tadeucci/2.3.2/maple2.file.parser.tadeucci.nuspec" | head -20Length of output: 1077
🏁 Script executed:
#!/bin/bash grep -R "<TargetFramework" -n Maple2.File.Ingest/Maple2.File.Ingest.csproj || grep -R "<TargetFrameworks" -n Maple2.File.Ingest/Maple2.File.Ingest.csprojLength of output: 128
Package reference updated to v2.3.2 – availability and compatibility confirmed
- File: Maple2.File.Ingest/Maple2.File.Ingest.csproj (Line 22)
<PackageReference Include="Maple2.File.Parser.Tadeucci" Version="2.3.2" />- Verified that v2.3.2 exists on NuGet and its .nuspec targets net8.0 (matching the project)
- No new dependencies or framework mismatches introduced; as a minor version bump under SemVer, no breaking changes are expected
Maple2.Server.Game/Model/Field/Entity/FieldPortal.cs (1)
11-11: LGTM! Consistent field addition following existing patterns.The
StartTickfield follows the same pattern as the existingEndTickfield and is appropriately placed for timing-related portal properties.Maple2.Model/Enum/Portal.cs (1)
8-8: LGTM! Appropriate enum member addition.The
Quest = 6enum member fills a logical gap in the sequence and follows the existing naming convention. The explicit byte value assignment is consistent with the enum's pattern.Maple2.Database/Context/MetadataContext.cs (1)
177-177: LGTM! Consistent with established JSON conversion pattern.The addition of JSON conversion for the
SummonPortalproperty follows the same pattern as other complex properties in theQuestMetadataconfiguration. This aligns with the quest portal functionality described in the PR objectives.Maple2.File.Ingest/Mapper/AdditionalEffectMapper.cs (1)
225-225: Good defensive programming with explicit type cast.The explicit cast to
intensures type safety and prevents potential compilation issues. This is a proper defensive programming practice that makes the type conversion intent explicit.Maple2.Server.Game/Packets/PortalPacket.cs (1)
39-39: Correct implementation for sending portal timing information.Writing
fieldPortal.StartTickprovides meaningful timing data to the client, which aligns with the quest portal functionality. This is an improvement over any hardcoded value that might have been used previously.Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs (1)
519-521: Appropriate quest portal consumption logic.The implementation correctly handles quest portals by removing them upon use, which is the expected behavior for consumable quest-related portals. The logic flows properly by breaking after removal to continue with the normal portal usage flow.
Maple2.File.Ingest/Mapper/AchievementMapper.cs (1)
29-33: LGTM! Good defensive programming practice.The addition of the null check for
grade.rewardprevents potential null reference exceptions and aligns with robust error handling practices..env.example (1)
6-8: LGTM! Clear documentation and sensible default.The addition of the
LANGUAGEenvironment variable with comprehensive documentation is well-implemented. The default value of "EN" is appropriate for most deployments.Maple2.File.Ingest/Mapper/SkillMapper.cs (1)
15-16: LGTM! Consistent with language support pattern.The constructor update correctly accepts the
languageparameter and passes it to theSkillParser, following the established pattern for multi-language support across mapper classes.Maple2.File.Ingest/Mapper/ScriptMapper.cs (1)
16-17: LGTM! Proper implementation of language support.The constructor update correctly implements the language parameter pattern, maintaining consistency with other mapper classes in the ingestion system.
Maple2.File.Ingest/Mapper/NpcMapper.cs (1)
15-16: LGTM! Language parameter addition is consistent and well-implemented.The constructor changes align with the systematic internationalization support being added across mapper classes. The parameter is properly passed through to the NpcParser constructor.
Maple2.File.Ingest/Mapper/MapMapper.cs (1)
14-16: LGTM! Consistent language parameter implementation across parsers.The language parameter is correctly passed to both
TableParserandMapParserconstructors, maintaining consistency with the internationalization pattern.Maple2.Model/Metadata/QuestMetadata.cs (2)
19-19: LGTM! Well-designed nullable property for quest portal functionality.The nullable
SummonPortalproperty appropriately handles the fact that not all quests require portal summoning, and the relevant code snippet shows proper null checking inQuestManager.SummonPortal.
94-96: LGTM! Clean and focused record design.The
QuestSummonPortalrecord has a clear, minimal interface with appropriately typed properties for the portal system integration.Maple2.Model/Metadata/ServerTable/GameEventTable.cs (2)
258-264: LGTM! Consistent design for new sale event types.Both
SaleAutoPlayInstrumentandSaleAutoFishingrecords follow a consistent pattern with appropriate property types for sale/discount functionality. The design aligns well with the existing GameEventData architecture.
299-300: LGTM! Proper JSON polymorphic type registration.The JSON derived type attributes are correctly added for both new sale event types, ensuring proper serialization/deserialization support.
Maple2.Server.Game/Packets/QuestPacket.cs (2)
24-24: LGTM: Improved enum clarity.Renaming
Unknown18toSummonPortalsignificantly improves code readability and makes the command's purpose clear.
131-139: LGTM: Well-structured packet method with meaningful parameters.The method signature clearly defines the required parameters for portal summoning, and the packet structure correctly writes the command followed by the three integer parameters. This implementation aligns well with the quest portal functionality.
Maple2.File.Ingest/Mapper/ItemMapper.cs (2)
18-22: LGTM: Language configuration support added.The constructor changes properly integrate language configuration into the item mapping process, aligning with the broader multi-language support mentioned in the PR objectives.
36-36:Details
❓ Verification inconclusive
Verify the generic type parameter removal.
The call changed from
parser.Parse<ItemDataRoot>()toparser.Parse(). Ensure this change is intentional and that the parser still returns the expected type structure.
🏁 Script executed:
#!/bin/bash # Description: Verify ItemParser.Parse() method signature and return type # Expected: The method should return the correct type without explicit generic parameter ast-grep --pattern $'class ItemParser { $$$ Parse($$$) { $$$ } $$$ }'Length of output: 82
Let’s locate and inspect the
ItemParserdefinition and itsParsemethod signature to confirm whether the generic parameter was removed or moved:
🏁 Script executed:
#!/bin/bash # 1. Find the file declaring ItemParser parser_file=$(rg -l --fixed-strings "class ItemParser") echo "Found ItemParser in: $parser_file" # 2. Show the class declaration rg -n "class\s\+ItemParser" -C2 "$parser_file" # 3. Show all Parse() overloads or definitions rg -n "Parse" -C3 "$parser_file"Length of output: 251
Let’s locate all parser classes and inspect their
Parsedefinitions to confirm the signature change:
🏁 Script executed:
#!/bin/bash # 1. List all classes ending with “Parser” echo "Parser class files:" rg -lE "class\s+\w+Parser" | sed 's/^/ - /' # 2. For each, show generic- and non-generic Parse methods for file in $(rg -lE "class\s+\w+Parser"); do echo echo "=== $file ===" rg -n "Parse<" -C2 "$file" || true rg -n "Parse\s*\(" -C2 "$file" || true doneLength of output: 918
Let’s correct the search and inspect all
*Parserclasses for both generic and non-genericParsesignatures:
🏁 Script executed:
#!/bin/bash echo "Searching for Parser classes…" # 1. Find all files declaring a class ending with “Parser” parser_files=$(rg -l -E 'class\s+\w+Parser' .) if [ -z "$parser_files" ]; then echo "No Parser classes found." exit 0 fi echo "Found Parser class files:" echo "$parser_files" | sed 's/^/ - /' # 2. In each file, show class declaration and both Parse overloads for file in $parser_files; do echo echo "=== $file ===" # Class declaration rg -n -E 'class\s+\w+Parser' -C2 "$file" # Generic Parse<T> rg -n -E 'Parse<[^>]+>' -C2 "$file" || echo " (no generic Parse<T>)" # Non-generic Parse( rg -n -E 'Parse\s*\(' -C2 "$file" || echo " (no non-generic Parse())" doneLength of output: 257
Let’s inspect
ItemMapper.csto identify theparservariable’s type and see itsParsesignature:
🏁 Script executed:
#!/bin/bash # 1. Locate ItemMapper.cs file=$(find . -type f -path "*Maple2.File.Ingest*/Mapper/ItemMapper.cs") if [[ ! -f "$file" ]]; then echo "Error: ItemMapper.cs not found" exit 1 fi echo "Inspecting: $file" # 2. Show parser declarations and usages echo echo ">> parser field/variable and constructor injection:" grep -n -E "(parser|Parser)" -C2 "$file" # 3. Show the Parse invocation around line 36 echo echo ">> Lines 30–50 (Parse call context):" sed -n '30,50p' "$file"Length of output: 3473
Let’s locate the actual
ItemParser.csfile and inspect itsParsemethod signatures:
🏁 Script executed:
#!/bin/bash # 1. Find the ItemParser.cs file echo "Searching for ItemParser.cs…" parser_files=$(find . -type f -name "ItemParser.cs") if [ -z "$parser_files" ]; then echo "❌ ItemParser.cs not found" exit 1 fi echo "Found:" echo "$parser_files" | sed 's/^/ - /' # 2. For each, show generic- and non-generic Parse signatures for file in $parser_files; do echo echo "=== $file ===" echo "-- Generic Parse<T> definitions --" rg -nE "public\s+.*Parse<[^>]+>" -C2 "$file" || echo " (none)" echo "-- Non-generic Parse definitions --" rg -nE "public\s+.*Parse\s*\(" -C2 "$file" || echo " (none)" doneLength of output: 206
Let’s directly locate the
ItemParserclass definition and inspect itsParseoverloads:
🏁 Script executed:
#!/bin/bash # 1. Find where ItemParser is defined echo "Searching for class ItemParser..." matches=$(grep -R "class ItemParser" -n .) if [ -z "$matches" ]; then echo "❌ No class ItemParser found in repository." exit 1 fi echo "$matches" # 2. Extract the first file path file=$(echo "$matches" | head -n1 | cut -d: -f1) echo echo "Inspecting Parse methods in $file:" # 3. Show generic Parse<T> signatures grep -n "Parse<" -n "$file" || echo " (no generic Parse<T> found)" # 4. Show non-generic Parse() signatures grep -nE "Parse\s*\(" -n "$file" | grep -v "Parse<" || echo " (no non-generic Parse() found)"Length of output: 242
Confirm parser.Parse() generic parameter removal
Please ensure that the change from
parser.Parse<ItemDataRoot>()toparser.Parse()was intentional and that the returned sequence still matches the expected(int id, string name, ItemData data)tuples:
- File: Maple2.File.Ingest/Mapper/ItemMapper.cs (line 36) –
parser.Parse()invocation- Verify in the
Maple2.File.Parserimplementation (external project/assembly) that the non-genericParse()overload still returns the correct type structure.Maple2.Server.Game/Model/Field/Entity/FieldQuestPortal.cs (1)
6-12: LGTM: Clean and focused quest portal implementation.The class design is excellent - it extends
FieldPortalwith minimal additional complexity while adding the essentialOwnerfield for tracking portal ownership. The constructor properly delegates to the base class and the implementation is straightforward and maintainable.Maple2.File.Ingest/Mapper/QuestMapper.cs (3)
17-18: LGTM: Language configuration support added.The constructor changes properly integrate language configuration into the quest mapping process, maintaining consistency with other mapper classes.
83-86: LGTM: Improved null safety for Mentoring property.The additional null check on
mentoringIconenhances robustness and prevents potential issues with empty or null icon references.
87-90: LGTM: Well-implemented SummonPortal property logic.The conditional logic correctly handles the case where both
fieldIDandportalIDare 0 by returning null, and properly instantiatesQuestSummonPortalotherwise. This implementation aligns well with the quest portal summoning functionality described in the PR objectives.Maple2.Server.Game/Manager/QuestManager.cs (5)
11-11: LGTM - Import addition for new functionality.The new import supports the quest portal functionality being added.
171-173: LGTM - Well-placed portal summoning logic.The portal summoning is correctly triggered after the quest start packet is sent, ensuring proper sequencing of operations.
11-11: Using directive addition looks good.The new import is necessary for the portal functionality being introduced.
649-649: Minor formatting improvement.The blank line addition improves code readability.
171-173:Details
✅ Verification successful
Portal summoning integration is well-placed.
The conditional portal summoning after quest start notification ensures portals are only created for successfully started quests.
Verify that all dependencies for portal spawning are available at this point in the quest start flow:
🏁 Script executed:
#!/bin/bash # Description: Check if portal spawning dependencies are properly initialized when Start method is called # Search for session.Field and session.NpcScript usage patterns rg -A 3 -B 3 "session\.Field.*SpawnPortal" # Check NpcScript initialization patterns rg -A 5 "NpcScript.*=" --type csLength of output: 10758
🏁 Script executed:
#!/bin/bash # Retrieve the SummonPortal method implementation for verification rg -C10 "SummonPortal" Maple2.Server.Game/Manager/QuestManager.csLength of output: 2095
Portal summoning dependencies validated
The
SummonPortalmethod in Maple2.Server.Game/Manager/QuestManager.cs already guards against missing data—returning early ifquest.Metadata.SummonPortalorsession.NpcScript?.Npcis null—before invokingsession.Field.SpawnPortal. All required dependencies are initialized by the time this code runs. Approving as-is.Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs (4)
173-187: LGTM - Quest portal spawning implementation is well-structured.The method correctly creates quest-specific portals with appropriate timing and positioning logic. The use of constants for portal parameters promotes maintainability.
688-688: LGTM - Minor formatting improvement.Clean formatting change that improves code consistency.
785-789: Portal removal logic correctly implements ownership model.The type checking and conditional packet sending ensures quest portals are only removed from their owner's view while maintaining broadcast behavior for regular portals.
688-688: Minor formatting improvement.The blank line addition enhances code readability.
Maple2.File.Ingest/Mapper/ServerTableMapper.cs (1)
851-853: LGTM! Correct logic for expired event handling.The modification properly allows
SaleAutoFishingandSaleAutoPlayInstrumentevents to be processed even when expired, which aligns with the PR requirement that these features need manual enabling.Maple2.File.Ingest/Mapper/TableMapper.cs (3)
41-46: LGTM!The language parameter addition to the constructor follows the established pattern for other mappers.
86-87: LGTM!The new table entries follow the established pattern for yielding table metadata.
1807-1825: LGTM!The implementation correctly groups entries by content and creates the appropriate nested dictionary structure.
Maple2.File.Ingest/Program.cs (2)
44-49: LGTM!Proper error handling for the environment variable with clear error message.
203-213: LGTM!All mapper constructors consistently receive the language parameter.
Maple2.Server.Game/PacketHandlers/SmartPushHandler.cs (1)
47-49: Correction: Maintain original control flow behavior.My previous suggestion to use
breakinstead ofreturnwould change the behavior. The original code exits the method entirely when item consumption fails.Maple2.Server.Game/Manager/MasteryManager.cs (5)
105-111: Refactoring looks good!The change to use a boolean return value from
GatherCommonand handle packet sending based on the result improves code clarity and maintainability.
114-119: Consistent refactoring pattern applied.Good job maintaining consistency with the FieldInteract version by using the same boolean return pattern.
159-169: Good extraction of success rate calculation.The success rate calculation is properly extracted with clear parameter handling. The home ownership check is correctly implemented.
189-189: Verify gathering count update timing.The gathering count is updated after success, which means the first gather attempt always uses
currentCount = 0for success rate calculation. Please confirm this is the intended behavior.
134-136:⚠️ Potential issueCritical: Inverted success check logic!
The condition
Random.Shared.NextDouble() > (successRate / 100)is inverted. WhensuccessRateis 100%, this condition will always be false (sinceNextDouble()returns [0, 1)), causing gathering to fail when it should succeed.Apply this fix:
- if (Random.Shared.NextDouble() > (successRate / 100)) { + if (Random.Shared.NextDouble() * 100 > successRate) { return false; }Or alternatively:
- if (Random.Shared.NextDouble() > (successRate / 100)) { + if (Random.Shared.NextDouble() <= (successRate / 100)) { + Gather(recipeMetadata, position, rotation); + this[recipeMetadata.Type] += recipeMetadata.RewardMastery; + if (!recipeMetadata.NoRewardExp) { + session.Exp.AddExp(ExpType.gathering); + } + return true; + } return false; - } - - Gather(recipeMetadata, position, rotation); - this[recipeMetadata.Type] += recipeMetadata.RewardMastery; - if (!recipeMetadata.NoRewardExp) { - session.Exp.AddExp(ExpType.gathering); - } - return true;Likely an incorrect or invalid review comment.
|
After merging #478, some reward equipment boxes can no longer be opened. Attempting to open them causes the server to crash. |
3 participants
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
Refactor
Chores