mushroom launcher2.00.1 - #612
Conversation
Co-authored-by: AngeloTadeucci <15664821+AngeloTadeucci@users.noreply.github.com>
Co-authored-by: AngeloTadeucci <15664821+AngeloTadeucci@users.noreply.github.com>
This reverts commit d684c38.
- Reject names with leading or trailing spaces instead of trimming them - Reject names with multiple consecutive spaces - Add comprehensive test coverage for space validation scenarios - Maintain backward compatibility with existing validation rules Co-authored-by: AngeloTadeucci <15664821+AngeloTadeucci@users.noreply.github.com>
Co-authored-by: AngeloTadeucci <15664821+AngeloTadeucci@users.noreply.github.com>
…orean, and Chinese names Co-authored-by: AngeloTadeucci <15664821+AngeloTadeucci@users.noreply.github.com>
WalkthroughBroad protocol, serialization, and model layout updates across client/server: opcode remaps, session version/IV changes, multiple packet payload realignments, enum and struct shape changes, new character-name validator and tests, handler logic tweaks (guild early-return, enum-based equip slot reads), added Admin packet, logging verbosity adjustments, and mapper/date parsing refinement. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant L as Login/Game Handler
participant V as CharacterNameValidator
participant R as Response
C->>L: Submit character name
L->>V: ValidateName(name)
V-->>L: CharacterCreateError? (null or code)
alt Invalid (error != null)
L-->>C: Error packet (s_char_err_*)
else Valid
L->>R: Proceed with existing checks
R-->>C: Success/next steps
end
sequenceDiagram
autonumber
participant C as Client
participant S as Login Server
participant G as Game Session
C->>S: ResponseKey (migration)
S-->>C: MoveResult(ok)
S-->>C: ServerList
S-->>C: CharacterList
Note over S,C: KMS does not send ServerEnter here
sequenceDiagram
autonumber
participant Client
participant Game as GameSession
participant P as Packets
participant A as AdminPacket
Client->>Game: EnterField
Game-->>Client: Stats, Update
Game->>A: AdminPacket.Enable()
A-->>Game: ByteWriter(Admin)
Game-->>Client: Admin Enable
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Maple2.Model/Game/Emote.cs (1)
14-17: Constructor doesn't initialize new fields.The constructor doesn't initialize
Unknown1,UnknownTime, orUnknown2, so they default tofalse/0. When theLoad()method inEmotePacket.csserializes the entire struct usingpWriter.Write<Emote>(emote), these uninitialized fields will be written to the network stream, which may not match protocol expectations if the client expects specific values.Consider explicitly initializing the new fields in the constructor or adding parameter overloads if they need non-default values:
- public Emote(int id, long expiryTime = 0) { + public Emote(int id, long expiryTime = 0, bool unknown1 = false, long unknownTime = 0, bool unknown2 = false) { Id = id; ExpiryTime = expiryTime; + Unknown1 = unknown1; + UnknownTime = unknownTime; + Unknown2 = unknown2; }
🧹 Nitpick comments (11)
Maple2.Server.Login/appsettings.json (1)
24-24: Re-check Verbose console level before shipping.Lowering the console sink to
Verbosewill surface every log event and can overwhelm stdout in production, increasing noise and I/O cost. Please confirm this is gated to environments where that extra verbosity is acceptable or add environment-specific overrides.Maple2.Server.Game/appsettings.json (1)
29-29: Double-check Verbose console logging for the game server.This change pushes every
Verboseevent to the console. On a busy game shard that can become noisy and costly. Please confirm the deployment profile really needs this level, or consider scoping it to diagnostics builds only.Maple2.Server.Game/PacketHandlers/SkillHandler.cs (1)
108-108: LGTM! Consider using.TrimEnd('\0')for precision.The addition of
.Trim('\0')correctly handles null-padded strings from the updated protocol. This is a common pattern when deserializing fixed-length string fields.Note:
.Trim('\0')removes null characters from both the start and end of the string. If you only need to remove trailing nulls (as the AI summary suggests), consider using.TrimEnd('\0')instead for clarity and precision.Optional refactor:
- record.HoldString = packet.ReadUnicodeString().Trim('\0'); + record.HoldString = packet.ReadUnicodeString().TrimEnd('\0');Would you like me to search the codebase for other Unicode string reads that might benefit from similar null-trimming?
Maple2.Server.Game/Packets/AdminPacket.cs (1)
9-15: Add documentation and consider using named constants for protocol values.The packet structure (writing a zero byte followed by 255) lacks explanation. Consider:
- Adding XML documentation explaining what this admin packet enables
- Using named constants instead of magic numbers for better maintainability
Example improvement:
+/// <summary> +/// Creates an admin packet to enable admin mode for the session. +/// </summary> +/// <returns>A ByteWriter containing the admin enable packet.</returns> public static ByteWriter Enable() { + const byte AdminEnableFlag = 255; ByteWriter pWriter = Packet.Of(SendOp.Admin); pWriter.WriteByte(); - pWriter.WriteByte(255); + pWriter.WriteByte(AdminEnableFlag); return pWriter; }Maple2.Server.Game/Manager/Config/SkillInfo.cs (2)
196-196: Document the magic number.The hardcoded
1appears to be a protocol header or version indicator. Add a constant or comment explaining its purpose.- writer.WriteInt(1); + writer.WriteInt(1); // Protocol version/header
255-255: Document the hardcoded boolean value.The fixed
falsevalue appears to be a protocol requirement or placeholder. Add a comment explaining its purpose.- writer.WriteBool(false); + writer.WriteBool(false); // Reserved/placeholder flagMaple2.Model/Validators/CharacterNameValidator.cs (2)
9-13: Consider word-boundary or pattern-based filtering to prevent bypasses.The substring-based forbidden word check can be bypassed with simple character substitution (e.g., "adm1n", "G_M", "maple_2"). Additionally, substring matching can produce false positives (e.g., "class" contains "ass", "classic" contains "ass").
Consider implementing more sophisticated filtering:
- Use word boundaries with regex to match whole words
- Add pattern matching for common substitutions (e.g., l33tspeak)
- Maintain a separate list for substring-only patterns vs. whole-word patterns
Do you want me to generate an improved implementation that addresses these bypass scenarios while reducing false positives?
15-19: Eliminate duplication between BannedNames and ForbiddenWords.Several entries appear in both
BannedNamesandForbiddenWords(e.g., "admin", "moderator", "gm", "gamemaster", "staff", "system", "server", "maple", "nexon", "maplestory", "maple2", "support", "helper", "bot"). This creates maintenance overhead and potential inconsistencies.Consider consolidating to a single data structure with metadata indicating whether each term is banned entirely or only as a substring.
Maple2.Server.Game/Model/Field/Entity/FieldNpcSpawnPoint.cs (1)
44-47: Clarify why the warning log was disabled in FieldNpcSpawnPoint.cs:45. Commenting out this warning suppresses failures to load NPC metadata and can hide configuration or data‐integrity issues. If certain NPCs are expected to be missing, add a conditional filter or document those cases; otherwise, restore the warning (or lower its level) so missing metadata isn’t silently ignored.Maple2.Server.Game/Packets/ServerEnterPacket.cs (1)
6-6: Remove unused import
Theusing Maple2.Tools.Extensions;directive in ServerEnterPacket.cs is unused and can be removed.Maple2.Server.World/Containers/BlackMarketLookup.cs (1)
104-131: Drop unused locals in option checks
ratein the basic loop andvaluein the special loop are now dead stores. Please remove them to avoid confusion when we revisit this logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (43)
Maple2.File.Ingest/Mapper/ServerTableMapper.cs(1 hunks)Maple2.Model/Game/Emote.cs(1 hunks)Maple2.Model/Game/Item/Item.cs(1 hunks)Maple2.Model/Game/Item/ItemEnchant.cs(1 hunks)Maple2.Model/Game/Item/ItemOption.cs(1 hunks)Maple2.Model/Game/Item/ItemStats.cs(3 hunks)Maple2.Model/Game/Item/ItemTransfer.cs(2 hunks)Maple2.Model/Game/Sync/StateSync.cs(5 hunks)Maple2.Model/Game/User/SkillPoint.cs(1 hunks)Maple2.Model/Validators/CharacterNameValidator.cs(1 hunks)Maple2.Server.Core/Constants/RecvOp.cs(2 hunks)Maple2.Server.Core/Constants/SendOp.cs(1 hunks)Maple2.Server.Core/Network/Session.cs(1 hunks)Maple2.Server.Core/PacketHandlers/ResponseVersionHandler.cs(1 hunks)Maple2.Server.Core/Packets/CharacterListPacket.cs(0 hunks)Maple2.Server.Core/Packets/Helper/EquipPacketHelper.cs(1 hunks)Maple2.Server.Core/Packets/ServerListPacket.cs(1 hunks)Maple2.Server.Game/Manager/Config/SkillInfo.cs(2 hunks)Maple2.Server.Game/Manager/ItemMergeManager.cs(1 hunks)Maple2.Server.Game/Model/Field/Actor/FieldPlayer.cs(1 hunks)Maple2.Server.Game/Model/Field/Entity/FieldNpcSpawnPoint.cs(1 hunks)Maple2.Server.Game/Model/Stats.cs(1 hunks)Maple2.Server.Game/PacketHandlers/CheckCharacterNameHandler.cs(2 hunks)Maple2.Server.Game/PacketHandlers/GuildHandler.cs(1 hunks)Maple2.Server.Game/PacketHandlers/ItemEquipHandler.cs(2 hunks)Maple2.Server.Game/PacketHandlers/SkillHandler.cs(1 hunks)Maple2.Server.Game/PacketHandlers/UserSyncHandler.cs(1 hunks)Maple2.Server.Game/Packets/AdminPacket.cs(1 hunks)Maple2.Server.Game/Packets/DungeonRoomPacket.cs(1 hunks)Maple2.Server.Game/Packets/EmotePacket.cs(1 hunks)Maple2.Server.Game/Packets/EnchantScrollPacket.cs(0 hunks)Maple2.Server.Game/Packets/EquipPacket.cs(1 hunks)Maple2.Server.Game/Packets/FieldPacket.cs(0 hunks)Maple2.Server.Game/Packets/ItemInventoryPacket.cs(3 hunks)Maple2.Server.Game/Packets/ServerEnterPacket.cs(2 hunks)Maple2.Server.Game/Session/GameSession.cs(1 hunks)Maple2.Server.Game/Util/ItemStatsCalculator.cs(3 hunks)Maple2.Server.Game/appsettings.json(1 hunks)Maple2.Server.Login/PacketHandlers/CharacterManagementHandler.cs(3 hunks)Maple2.Server.Login/PacketHandlers/ResponseKeyHandler.cs(1 hunks)Maple2.Server.Login/appsettings.json(1 hunks)Maple2.Server.Tests/Validators/CharacterNameValidatorTests.cs(1 hunks)Maple2.Server.World/Containers/BlackMarketLookup.cs(2 hunks)
💤 Files with no reviewable changes (3)
- Maple2.Server.Core/Packets/CharacterListPacket.cs
- Maple2.Server.Game/Packets/FieldPacket.cs
- Maple2.Server.Game/Packets/EnchantScrollPacket.cs
🧰 Additional context used
🧬 Code graph analysis (26)
Maple2.Server.Game/Manager/ItemMergeManager.cs (1)
Maple2.Model/Game/Item/ItemStats.cs (2)
Option(92-118)Option(100-104)
Maple2.Server.Core/Packets/ServerListPacket.cs (1)
Maple2.Server.Core/Helpers/DebugByteWriter.cs (1)
WriteString(84-87)
Maple2.Server.Core/Packets/Helper/EquipPacketHelper.cs (1)
Maple2.Model/ModelExtensions.cs (1)
EquipSlot(82-83)
Maple2.Server.Login/PacketHandlers/CharacterManagementHandler.cs (3)
Maple2.Model/Validators/CharacterNameValidator.cs (2)
CharacterCreateError(29-67)CharacterNameValidator(7-88)Maple2.Server.Core/Network/Session.cs (2)
Send(129-129)Send(131-131)Maple2.Server.Core/Packets/CharacterListPacket.cs (1)
CharacterListPacket(17-190)
Maple2.Server.Game/Packets/EquipPacket.cs (1)
Maple2.Model/ModelExtensions.cs (1)
EquipSlot(82-83)
Maple2.Model/Validators/CharacterNameValidator.cs (1)
Maple2.Model/Metadata/Constants.cs (1)
Constant(10-960)
Maple2.Server.Game/PacketHandlers/CheckCharacterNameHandler.cs (2)
Maple2.Model/Validators/CharacterNameValidator.cs (1)
CharacterNameValidator(7-88)Maple2.Server.Core/Packets/CharacterListPacket.cs (1)
CharacterListPacket(17-190)
Maple2.File.Ingest/Mapper/ServerTableMapper.cs (1)
Maple2.Database/Extensions/DateTimeExtensions.cs (1)
ToEpochSeconds(4-10)
Maple2.Server.Game/Session/GameSession.cs (2)
Maple2.Server.Core/Network/Session.cs (2)
Send(129-129)Send(131-131)Maple2.Server.Game/Packets/AdminPacket.cs (1)
AdminPacket(7-16)
Maple2.Server.Game/Packets/ItemInventoryPacket.cs (1)
Maple2.Server.Core/Helpers/DebugByteWriter.cs (1)
WriteUnicodeString(89-92)
Maple2.Server.Game/Packets/AdminPacket.cs (1)
Maple2.Server.Core/Packets/Packet.cs (1)
Packet(8-24)
Maple2.Server.Login/PacketHandlers/ResponseKeyHandler.cs (1)
Maple2.Server.Login/Session/LoginSession.cs (2)
ListServers(88-92)ListCharacters(94-121)
Maple2.Server.Game/Packets/DungeonRoomPacket.cs (1)
Maple2.Server.Core/Helpers/DebugByteWriter.cs (2)
WriteInt(69-72)WriteLong(79-82)
Maple2.Model/Game/User/SkillPoint.cs (1)
Maple2.Server.Core/Helpers/DebugByteWriter.cs (1)
WriteInt(69-72)
Maple2.Server.Tests/Validators/CharacterNameValidatorTests.cs (1)
Maple2.Model/Validators/CharacterNameValidator.cs (3)
CharacterNameValidator(7-88)CharacterCreateError(29-67)GetForbiddenWord(84-87)
Maple2.Server.Game/PacketHandlers/UserSyncHandler.cs (1)
Maple2.Model/Game/Sync/StateSync.cs (1)
StateSync(10-171)
Maple2.Server.Game/Manager/Config/SkillInfo.cs (1)
Maple2.Server.Core/Helpers/DebugByteWriter.cs (3)
WriteInt(69-72)WriteByte(59-62)WriteBool(54-57)
Maple2.Server.Core/PacketHandlers/ResponseVersionHandler.cs (1)
Maple2.Server.Core/Network/Session.cs (3)
Session(21-307)Session(53-76)Session(78-78)
Maple2.Server.Game/Packets/EmotePacket.cs (1)
Maple2.Server.Core/Helpers/DebugByteWriter.cs (2)
WriteInt(69-72)WriteLong(79-82)
Maple2.Model/Game/Item/ItemEnchant.cs (1)
Maple2.Server.Core/Helpers/DebugByteWriter.cs (2)
WriteLong(79-82)WriteInt(69-72)
Maple2.Server.Game/PacketHandlers/ItemEquipHandler.cs (1)
Maple2.Model/ModelExtensions.cs (1)
EquipSlot(82-83)
Maple2.Model/Game/Item/ItemTransfer.cs (1)
Maple2.Server.Core/Helpers/DebugByteWriter.cs (1)
WriteBool(54-57)
Maple2.Model/Game/Sync/StateSync.cs (3)
Maple2.Model/Game/Sync/StateSyncCoupleDance.cs (2)
WriteTo(11-16)ReadFrom(18-23)Maple2.Model/Game/Sync/StateSyncRps.cs (2)
WriteTo(12-18)ReadFrom(20-26)Maple2.Model/Game/Sync/StateSyncWeddingEmotion.cs (2)
WriteTo(13-24)ReadFrom(26-37)
Maple2.Server.Core/Constants/SendOp.cs (1)
Maple2.Server.Game/Packets/GuideObjectPacket.cs (1)
GuideObject(34-42)
Maple2.Model/Game/Item/Item.cs (2)
Maple2.Model/Game/Item/ItemTransfer.cs (3)
ItemTransfer(8-80)ItemTransfer(17-22)ItemTransfer(24-26)Maple2.Model/Game/Item/ItemBinding.cs (3)
ItemBinding(6-40)ItemBinding(12-15)ItemBinding(17-19)
Maple2.Server.Core/Constants/RecvOp.cs (1)
Maple2.Server.Game/PacketHandlers/ItemExchangeScrollHandler.cs (1)
ItemExchangeScroll(13-93)
🔇 Additional comments (18)
Maple2.Server.Game/Packets/DungeonRoomPacket.cs (1)
82-83: Verify the default values and add inline documentation.The AI summary incorrectly states these writes occur "per entry," but they actually execute once after the loop. Additionally, the purpose of these two fields (int and long, both defaulting to 0) is undocumented.
Please confirm:
- Are the default values (0, 0) correct for the protocol specification?
- What do these fields represent in the packet payload?
Consider adding inline comments to document the purpose of these fields for future maintainability:
+ // TODO: Document purpose of these protocol fields pWriter.WriteInt(); pWriter.WriteLong();Maple2.Server.Core/Constants/RecvOp.cs (2)
3-4: Formatting change: enum brace placement.The opening brace was moved to a new line, aligning with typical C# formatting conventions.
60-191: Packet handlers aligned with updated RecvOp enum; no hardcoded opcodes or duplicate values detected.Maple2.File.Ingest/Mapper/ServerTableMapper.cs (1)
1650-1651: Ensure source dates include seconds or use TryParseExact
- StartTime now uses
"yyyy-MM-dd-HH-mm-ss", matching EndTime and other shop parsing.- Verify
item.startDateactually contains seconds; otherwiseDateTime.ParseExactwill throw.- Consider using
DateTime.TryParseExactwith a fallback for more defensive parsing.Maple2.Server.Game/Manager/Config/SkillInfo.cs (1)
255-257: Ensure SkillInfo.ReadFrom matches serialization boolean order. The deserializer must call ReadBool() three times—in the same sequence as WriteBool(false), WriteBool(Level > 0), WriteBool(Notify)—to avoid protocol mismatches.Maple2.Model/Validators/CharacterNameValidator.cs (1)
62-64: LGTM!The check for names consisting only of non-alphanumeric characters correctly catches edge cases like "---" or "___" that would pass the regex but shouldn't be valid names.
Maple2.Server.Login/PacketHandlers/ResponseKeyHandler.cs (1)
40-43: LGTM!The addition of
ListServers()andListCharacters()after successful migration aligns the flow with KMS client expectations. The comment clearly documents why this differs from the original implementation.Maple2.Server.Game/Packets/EquipPacket.cs (1)
18-18: Verify client-server protocol compatibility for the serialization format change.Changing from
WriteUnicodeString(string) toWrite<EquipSlot>(enum) is a breaking change in the network protocol. The client must be updated to deserialize the slot as an enum rather than parsing a string.Ensure that:
- The client has been updated to match this new serialization format
- There are no mixed-version scenarios where old clients connect to new servers
- Similar changes in
ItemEquipHandler.csandEquipPacketHelper.csare alignedThe change improves type safety and reduces payload size, but requires careful version coordination.
Maple2.Server.Game/PacketHandlers/CheckCharacterNameHandler.cs (1)
28-33: LGTM!Replacing inline length checks with the centralized
CharacterNameValidator.ValidateNameimproves maintainability and ensures consistent validation across the codebase. The error handling flow remains clear and correct.Maple2.Server.Game/PacketHandlers/ItemEquipHandler.cs (2)
34-34: LGTM - Verify protocol alignment with EquipPacket.cs changes.The change from string parsing to direct enum deserialization (
Read<EquipSlot>()) improves performance and type safety. This aligns with the corresponding serialization change inEquipPacket.cs(line 18).Note that the previous parse-failure guard is removed, so the code now assumes the enum value is always valid. Ensure the
Read<EquipSlot>()method handles invalid values appropriately (e.g., throws or returns a default value) to prevent bad state.The protocol change must be coordinated with client updates to ensure both sides serialize/deserialize the slot as an enum.
56-56: LGTM - Consistent with HandleEquip changes.Same deserialization pattern as
HandleEquipat line 34.Maple2.Server.Game/PacketHandlers/UserSyncHandler.cs (1)
35-35: Ensure default StateSync handles all other states
Only MicroGameRps, MicroGameCoupleDance, and WeddingEmotion use custom subclasses; confirm the base StateSync serializes/deserializes correctly for all other ActorState values and that no specialized handling is required.Maple2.Server.Login/PacketHandlers/CharacterManagementHandler.cs (3)
10-10: LGTM!The new imports are necessary for the centralized name validation logic and are used appropriately in the code.
Also applies to: 13-13
118-123: Excellent refactor to centralize validation.Replacing manual length checks with the comprehensive
CharacterNameValidatorimproves maintainability and consistency. The validator enforces length constraints, forbidden words, banned names, and character patterns—all properly tested in the new test suite.
139-141: Protocol parsing updated consistently.The change from string-based to byte-based
EquipSlotparsing aligns with the serialization updates inEquipPacketHelper.cs. The validation logic for invalid slots (SK, OH, Unknown) is correctly preserved.Maple2.Server.Tests/Validators/CharacterNameValidatorTests.cs (1)
1-184: Excellent comprehensive test coverage.The test suite thoroughly validates the
CharacterNameValidatorbehavior across all scenarios:
- Valid names (including minimum/maximum lengths)
- Error cases (too short/long, banned, forbidden words, invalid characters)
- Edge cases (null, whitespace, special characters only, spaces)
- Case-insensitive validation
- Multilingual support (Japanese, Korean, Chinese characters)
The multilingual tests are particularly valuable for ensuring the validator's Unicode pattern correctly handles international character sets. This gives high confidence in the centralized validation logic now used in
CharacterManagementHandler.Maple2.Server.Core/Packets/Helper/EquipPacketHelper.cs (1)
13-13: Protocol change verifiedNetwork serialization now uniformly uses Write/Read; no string-based writes or reads remain. Breaking change—deploy client and server in lockstep.
Maple2.Server.Game/Packets/ServerEnterPacket.cs (1)
10-56: Verify client-server protocol alignment for payload changes.According to the AI summary, this packet's payload structure has been modified (two data entries removed). Protocol changes require strict version coordination between client and server to prevent deserialization errors or data corruption.
Please confirm:
- The client has been updated to match the new ServerEnter packet structure
- Version checks are in place to prevent protocol mismatches
- This change aligns with the "mushroom launcher" update mentioned in the PR description
| [StructLayout(LayoutKind.Sequential, Pack = 4, Size = 26)] | ||
| public readonly struct Emote { | ||
| public readonly int Id; | ||
| public readonly int Level = 1; | ||
| public readonly bool Unknown1; | ||
| public readonly long ExpiryTime; | ||
| public readonly long UnknownTime; | ||
| public readonly bool Unknown2; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify struct layout aligns with protocol serialization.
The explicit Size = 26 with three new fields (Unknown1, UnknownTime, Unknown2) may not match the natural memory layout with Pack = 4. With typical alignment:
- Id (4) + Level (4) + Unknown1 (1) + padding (3) + ExpiryTime (8) + UnknownTime (8) + Unknown2 (1) = 29 bytes
Additionally, the Learn() method in EmotePacket.cs serializes only 24 bytes (Id, Level, ExpiryTime, trailing long), omitting Unknown1 and Unknown2, while Load() writes the entire 26-byte struct. This creates a serialization mismatch between the two code paths.
Run the following script to verify how these fields are used in serialization:
🏁 Script executed:
#!/bin/bash
# Description: Check Emote struct serialization patterns across the codebase
# Search for Write<Emote> calls to see where the full struct is serialized
rg -n 'Write<Emote>' --type=cs -C 3
# Search for reads of Emote fields to understand protocol expectations
rg -n 'emote\.(Unknown1|UnknownTime|Unknown2)' --type=cs -C 2Length of output: 518
[scratchpad]
[task review_file_1/2 review_emotepacket_1/1]
[observations]
rgfoundpWriter.Write<Emote>(emote)at Maple2.Server.Game/Packets/EmotePacket.cs:21.- No matches for
rg -n 'emote\.(Unknown1|UnknownTime|Unknown2)'.
[pending] - Inspect EmotePacket.cs to see how
Emoteis serialized/deserialized.
[actions] - Extract relevant sections around Write and corresponding Read uses.
[done]
[/scratchpad]
Fix StructLayout alignment
Pack=4 with explicit Size=26 introduces 3 bytes of padding before the 8-byte fields (natural size 29). Change to Pack=1 to eliminate padding (so total size 26) or update Size to 29 to match Pack=4.
🤖 Prompt for AI Agents
In Maple2.Model/Game/Emote.cs around lines 5 to 12 the StructLayout attribute
uses Pack=4 but explicitly Size=26 which causes 3 bytes of padding before the
8-byte fields; update the layout so the declared size matches the packing:
either change Pack=4, Size=29 (to reflect the natural 29-byte layout) or change
Pack=1, Size=26 (to eliminate padding) — pick one and apply that change
consistently in the attribute.
| public BasicOption(float percent) : this(0, percent) { } | ||
| [StructLayout(LayoutKind.Sequential, Pack = 4, Size = 4)] | ||
| public readonly record struct BasicOption(int Value) { | ||
| public BasicOption(float percent) : this((int) percent * 100) { } |
There was a problem hiding this comment.
Fix percent ctor truncation
Casting to int before multiplying forces any percentage below 1 to 0 (e.g., the RandomizeValues path in ItemStatsCalculator now feeds 0.15f, which becomes 0). That wipes out rate-based options after this refactor. Multiply first, then cast (or otherwise preserve precision) so fractional rates survive serialization.
- public BasicOption(float percent) : this((int) percent * 100) { }
+ public BasicOption(float percent) : this((int) (percent * 100)) { }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public BasicOption(float percent) : this((int) percent * 100) { } | |
| public BasicOption(float percent) : this((int)(percent * 100)) { } |
🤖 Prompt for AI Agents
In Maple2.Model/Game/Item/ItemOption.cs around line 8, the BasicOption(float
percent) constructor casts percent to int before multiplying which truncates
values <1 (e.g., 0.15f -> 0); change the expression to multiply first then cast
so fractional percentages are preserved (for example use (int)(percent * 100) or
(int)Math.Round(percent * 100) as appropriate) and update the constructor call
accordingly to avoid losing precision during serialization.
| writer.Write<SkillPointSource>(source); | ||
| writer.WriteClass<PointRank>(point); | ||
| } | ||
| writer.WriteInt(); |
There was a problem hiding this comment.
Fix misplaced padding write
The new writer.WriteInt() is outside the foreach, so we now emit the padding value only once instead of once per (source, point) entry. That truncates the payload relative to the new layout and will desynchronize every reader expecting the extra 4 bytes for each entry. Move the write inside the loop (or drop it) so serialization stays aligned.
foreach ((SkillPointSource source, PointRank point) in Points) {
writer.Write<SkillPointSource>(source);
writer.WriteClass<PointRank>(point);
+ writer.WriteInt();
}
- writer.WriteInt();🤖 Prompt for AI Agents
In Maple2.Model/Game/User/SkillPoint.cs around line 37 the call to
writer.WriteInt() was placed after the foreach, causing the 4-byte padding to be
written only once instead of once per (source, point) entry; move the
writer.WriteInt() call into the body of the foreach so each iteration emits its
padding (or remove it entirely if the per-entry padding is not required by the
new layout).
|
|
||
| public static ByteWriter Load(string serverName, IList<IPEndPoint> serverIps, ICollection<int> channels) { | ||
| var pWriter = Packet.Of(SendOp.ServerList); | ||
| pWriter.WriteString("dev"); // env (Live, Staging, qa, dev) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Hardcoded environment string prevents deployment to other environments.
The "dev" string is hardcoded, making it impossible to deploy this code to production, staging, or QA environments without code changes. The comment indicates multiple valid values (Live, Staging, qa, dev), confirming this should be environment-specific.
Refactor to use a configuration value:
- pWriter.WriteString("dev"); // env (Live, Staging, qa, dev)
+ pWriter.WriteString(configuration.Environment); // env (Live, Staging, qa, dev)Additionally, ensure that clients have been updated to expect this new field in the protocol, as adding a field at the start of the payload is a breaking change.
🏁 Script executed:
#!/bin/bash
sed -n '1,50p' Maple2.Server.Core/Packets/ServerListPacket.cs
rg -nP "class\s+\w*Config" -t cs
rg -nP "IConfiguration" -t cs -C3
rg -n "configuration" -t cs -C3Length of output: 5920
🏁 Script executed:
#!/bin/bash
# Show ConfigManager implementation
sed -n '1,200p' Maple2.Server.Game/Manager/ConfigManager.cs
# Look for any environment-related properties or fields
rg -nP "Environment" -C3 Maple2.Server.Game/Manager/ConfigManager.csLength of output: 8810
Extract environment from configuration/env instead of hardcoding
- Replace
pWriter.WriteString("dev")inServerListPacket.Loadwith a runtime value (e.g., add anenvironmentparameter or read fromIConfiguration["Environment"]/Environment.GetEnvironmentVariable). - Coordinate client updates for this added field at the start of the payload (breaking change).
🤖 Prompt for AI Agents
In Maple2.Server.Core/Packets/ServerListPacket.cs around line 22, the code
currently hardcodes the environment string with pWriter.WriteString("dev");
replace this with a runtime value by either adding an environment parameter to
ServerListPacket.Load (and propagate it from caller) or by reading
IConfiguration["Environment"] or
Environment.GetEnvironmentVariable("ENVIRONMENT") at runtime and passing that to
pWriter.WriteString; update the packet construction so the environment field is
written at the start of the payload as required and document/coordinate the
client-side change because this is a breaking change to the packet layout.
| int count = 0; | ||
| for (int i = 0; i < SKILL_TYPES; i++) { | ||
| int count = 0; | ||
| for (int j = 0; j < SKILL_RANKS; j++) { | ||
| count += Skills[i, j].Count + SubSkills[i, j].Count; | ||
| } | ||
| writer.WriteByte((byte) count); | ||
|
|
||
| } | ||
| writer.WriteByte((byte) count); |
There was a problem hiding this comment.
Byte overflow risk when casting skill count.
If the total number of skills and subskills exceeds 255, the cast to byte at Line 204 will silently truncate the value, causing deserialization errors and potential data corruption.
Consider using WriteShort or WriteInt instead, or add a validation check:
int count = 0;
for (int i = 0; i < SKILL_TYPES; i++) {
for (int j = 0; j < SKILL_RANKS; j++) {
count += Skills[i, j].Count + SubSkills[i, j].Count;
}
}
- writer.WriteByte((byte) count);
+ if (count > byte.MaxValue) {
+ throw new InvalidOperationException($"Skill count {count} exceeds byte.MaxValue");
+ }
+ writer.WriteByte((byte) count);Or use a larger data type:
- writer.WriteByte((byte) count);
+ writer.WriteShort((short) count);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| int count = 0; | |
| for (int i = 0; i < SKILL_TYPES; i++) { | |
| int count = 0; | |
| for (int j = 0; j < SKILL_RANKS; j++) { | |
| count += Skills[i, j].Count + SubSkills[i, j].Count; | |
| } | |
| writer.WriteByte((byte) count); | |
| } | |
| writer.WriteByte((byte) count); | |
| int count = 0; | |
| for (int i = 0; i < SKILL_TYPES; i++) { | |
| for (int j = 0; j < SKILL_RANKS; j++) { | |
| count += Skills[i, j].Count + SubSkills[i, j].Count; | |
| } | |
| } | |
| if (count > byte.MaxValue) { | |
| throw new InvalidOperationException($"Skill count {count} exceeds byte.MaxValue"); | |
| } | |
| writer.WriteByte((byte) count); |
🤖 Prompt for AI Agents
In Maple2.Server.Game/Manager/Config/SkillInfo.cs around lines 198-204 the total
skill count is summed into an int and then cast to byte via
writer.WriteByte((byte) count), which will truncate values >255; update the
serialization to use a larger integer write method (e.g.,
writer.WriteShort((short)count) or writer.WriteInt(count)) or, if the protocol
must remain a single byte, add an explicit validation that throws/handles when
count > 255 before casting so the overflow cannot silently occur; ensure any
corresponding reader/deserializer is updated to match the new size if you change
the field width.
| case ActorState.EmotionIdle: | ||
| if (UpdateStateSyncTimeTracking()) { | ||
| Field.SkillMetadata.TryGet(stateSync.EmotionId, 1, out SkillMetadata? emote); | ||
| if (emote == null) { | ||
| break; | ||
| } | ||
| Session.ConditionUpdate(ConditionType.emotiontime, codeString: emote.Property.Emotion); | ||
| // Field.SkillMetadata.TryGet(stateSync.EmotionId, 1, out var emote); | ||
| // if (emote == null) { | ||
| // break; | ||
| // } | ||
| // Session.ConditionUpdate(ConditionType.emotiontime, codeString: emote.Property.Emotion); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
Clarify the intent of the commented-out emotion tracking code.
The emotion timing condition update for EmotionIdle state has been commented out rather than removed. This means:
- The
emotiontimecondition will no longer be tracked during emote animations - Gameplay features or quests that depend on tracking emote duration may break
- The
stateSync.EmotionIdfield may be unused, but the time tracking (UpdateStateSyncTimeTracking()) still runs
If this is temporary or incomplete refactoring related to the broader StateSync changes mentioned in the AI summary, consider adding a TODO comment explaining the plan. If this functionality is permanently removed, delete the commented code and the now-redundant UpdateStateSyncTimeTracking() call.
Run the following script to verify if EmotionId is still referenced elsewhere and if any tests depend on emotiontime condition:
🏁 Script executed:
#!/bin/bash
# Description: Check for usage of EmotionId and emotiontime condition
# Search for EmotionId references in StateSync and related code
echo "=== Searching for EmotionId references ==="
rg -n -C3 'EmotionId' --type=cs
echo -e "\n=== Searching for emotiontime condition usage ==="
rg -n -C3 'ConditionType\.emotiontime|emotiontime' --type=csLength of output: 11320
Clarify or remove commented-out emotiontime update
The code in EmotionIdle no longer updates ConditionType.emotiontime, breaking emotion-duration logic in ConditionUtil and QuestManager. Either restore the StateSync.EmotionId lookup and Session.ConditionUpdate call or delete the dead code (and the redundant UpdateStateSyncTimeTracking() call) with a clear TODO if this behavior is permanently removed.
🤖 Prompt for AI Agents
In Maple2.Server.Game/Model/Field/Actor/FieldPlayer.cs around lines 336-343, the
EmotionIdle case contains commented-out code that previously updated
ConditionType.emotiontime from StateSync.EmotionId, which breaks
emotion-duration logic used by ConditionUtil and QuestManager; either restore
the lookup and Session.ConditionUpdate call (use
Field.SkillMetadata.TryGet(stateSync.EmotionId, 1, out var emote) and if emote
!= null call Session.ConditionUpdate(ConditionType.emotiontime, codeString:
emote.Property.Emotion) after verifying UpdateStateSyncTimeTracking()
semantics), or remove the dead commented block and the redundant
UpdateStateSyncTimeTracking() call and add a TODO comment indicating intentional
removal so future reviewers know emotiontime behavior was consciously dropped.
| public void AddTotal(SpecialOption option) { | ||
| AddTotal((int) option.Value); | ||
| Rate += option.Rate; | ||
| AddTotal((int) option.Rate); | ||
| // Rate += option.Rate; | ||
| } |
There was a problem hiding this comment.
Keep SpecialOption feeding Stat.Rate
Total() still multiplies by stat.Rate, but this change stops feeding that field—casting the float rate to an int (often zero) and skipping AddRate entirely. Special options that should give percentage bonuses now have no effect. Restore the rate accumulation (e.g., call AddRate(option.Rate) and drop the integer cast) so Total() continues to work.
- public void AddTotal(SpecialOption option) {
- AddTotal((int) option.Rate);
- // Rate += option.Rate;
- }
+ public void AddTotal(SpecialOption option) {
+ AddRate(option.Rate);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public void AddTotal(SpecialOption option) { | |
| AddTotal((int) option.Value); | |
| Rate += option.Rate; | |
| AddTotal((int) option.Rate); | |
| // Rate += option.Rate; | |
| } | |
| public void AddTotal(SpecialOption option) { | |
| AddRate(option.Rate); | |
| } |
🤖 Prompt for AI Agents
In Maple2.Server.Game/Model/Stats.cs around lines 199-202, the
AddTotal(SpecialOption option) method currently casts option.Rate to int and
calls AddTotal((int) option.Rate), which discards fractional percentage bonuses
and prevents Stat.Rate from being updated; change this to feed the float rate
into the stat by removing the integer cast and calling AddRate(option.Rate)
(and/or AddTotal overloads that accept the proper type) so the special option's
percentage bonus accumulates into Rate and Total() continues to multiply
correctly.
|
|
||
| return; |
There was a problem hiding this comment.
🧩 Analysis chain
Early return completely disables all guild functionality.
The unconditional return statement after reading the command prevents any guild-related operations from executing. This effectively breaks all guild features including creation, invites, management, etc.
If this is intentional (e.g., temporarily disabling guild features during migration), please:
- Add a clear comment explaining why and when this will be re-enabled
- Consider throwing a "not implemented" error instead of silently ignoring commands
- Track this with a TODO or issue reference
If this was added accidentally during debugging, it should be removed before merging.
Please confirm whether this is intentional and, if so, document the rationale.
#!/bin/bash
FILE=$(fd --hidden --absolute-path GuildHandler.cs | head -n1)
sed -n '60,90p' "$FILE"
#!/bin/bash
FILE=$(fd --absolute-path GuildHandler.cs | head -n1)
sed -n '60,90p' "$FILE"
🏁 Script executed:
#!/bin/bash
FILE=$(fd GuildHandler.cs | head -n1)
sed -n '60,90p' "$FILE"Length of output: 987
Remove or document the early return disabling guild functionality
The return; immediately after reading command in GuildHandler.cs’s Handle method (around line 72) bypasses the switch and breaks all guild operations. Remove it (if accidental) or replace it with a clear exception and add a comment/TODO explaining why guild commands are intentionally disabled.
🤖 Prompt for AI Agents
In Maple2.Server.Game/PacketHandlers/GuildHandler.cs around lines 72-73, there
is an early "return;" immediately after reading the command which short-circuits
the switch and disables all guild operations; remove this stray return if guild
handling should run, or replace it with a thrown NotSupportedException (or
similar) and a clear comment/TODO explaining guild commands are intentionally
disabled and under what conditions. Update unit tests/logging if present to
reflect the change and ensure the switch that dispatches guild commands executes
as intended when the return is removed, or that callers handle the explicit
exception when intentionally disabled.
| public static ByteWriter Learn(Emote emote) { | ||
| var pWriter = Packet.Of(SendOp.Emote); | ||
| pWriter.Write<Command>(Command.Learn); | ||
| pWriter.Write<Emote>(emote); | ||
| pWriter.WriteInt(emote.Id); | ||
| pWriter.WriteInt(emote.Level); | ||
| pWriter.WriteLong(emote.ExpiryTime); | ||
| pWriter.WriteLong(); | ||
|
|
||
| return pWriter; | ||
| } |
There was a problem hiding this comment.
Serialization inconsistency between Learn and Load methods.
The Learn() method serializes individual fields (Id, Level, ExpiryTime, trailing long) totaling 24 bytes, while Load() at line 21 serializes the entire Emote struct (26 bytes including Unknown1, Unknown2). This creates an inconsistency where:
Learn()omitsUnknown1(bool after Level) andUnknown2(bool at the end)- The trailing
WriteLong()at line 33 might correspond to theUnknownTimefield, but this is unclear - Clients receiving Learn vs. Load packets would parse different data layouts
Apply this diff to align Learn() serialization with the struct layout:
public static ByteWriter Learn(Emote emote) {
var pWriter = Packet.Of(SendOp.Emote);
pWriter.Write<Command>(Command.Learn);
pWriter.WriteInt(emote.Id);
pWriter.WriteInt(emote.Level);
+ pWriter.WriteBool(emote.Unknown1);
pWriter.WriteLong(emote.ExpiryTime);
- pWriter.WriteLong();
+ pWriter.WriteLong(emote.UnknownTime);
+ pWriter.WriteBool(emote.Unknown2);
return pWriter;
}Alternatively, if the protocol genuinely differs between Learn and Load, consider using Write<Emote> for consistency and documenting the intentional difference.
🤖 Prompt for AI Agents
In Maple2.Server.Game/Packets/EmotePacket.cs around lines 27 to 36, the Learn
method serializes Id, Level, ExpiryTime and a trailing long but omits the Emote
struct's Unknown1 and Unknown2 fields, causing a different packet layout than
Load; update Learn to serialize the full Emote struct (e.g.
pWriter.Write<Emote>(emote)) or, if the protocol really differs, explicitly
write Unknown1 (bool), Unknown2 (bool) and UnknownTime in the correct order and
types to match Load, and add a short comment documenting the intentional
difference if you choose the divergent layout.
| Send(AdminPacket.Enable()); | ||
|
|
There was a problem hiding this comment.
Do not broadcast GM enable packet to every session.
AdminPacket.Enable() writes SendOp.Admin with a grade byte of 255 (see Maple2.Server.Game/Packets/AdminPacket.cs). Pushing that unconditionally in EnterField elevates every connecting player to maximum GM privileges, which is a critical security break. Gate this send behind the existing GM/account checks (or drop it entirely) so normal players never receive the admin enable packet.
|
What is this @rhkdfuf28-beep ?? |
mushroom launcher 맞고 다운로드.
Summary by CodeRabbit
New Features
Changes
Known Limitations
Tests