Open
Conversation
…r constants that are processed during file ingest WIP.
…ring file ingest WIP
…ing file ingest MS2Community#2 WIP
…ing file ingest MS2Community#3 WIP
…ning and non-IConvertible data types handling during constant parsing.
After sorting through it, I have roughly fixed the following major bugs, while some minor bugs have not been included We have made certain fixes to address the existing abnormal issues and BUGs on the server. The fixes are as follows 1.Fixed the issue where monsters would walk through walls when using skills 2.Fixed the issue of monsters facing players when using normal attacks and skills. Now, monsters will not launch attacks or use skills with their backs facing players 3.Fixed the issue where monster skills were directed incorrectly and went through walls 4.Fixed the issue where player-range skills did not have damage numbers and did not interact with hit reactions 5.Fixed the issue where monsters using range skills could not correctly identify player positions, resulting in location errors 6.Fixed the issue where summoned NPCs or monsters would instantly die under certain circumstances, making it impossible to complete some dungeons 7.Fixed the issue where players may falsely go offline (unable to use chat, player status is offline, and the team channel displays 65535) when entering or exiting dungeons or houses 8.Fixed the issue where fishing did not earn character experience points 9.Fixed the issue where all item rewards in the group would be distributed at once when reading dungeon rewards (in cases where it was not due to an XML configuration error)
… nested foreach loop and indexing the property names to search them later when looping through the parsed XML constants data.
…w them to retrieve their values.
… for ConstantsTable.
…onfusion from two constructors and nethier defined as the JSON constructor.
… back GlobalCubeSkillIntervalTime and merged into XML parsing functionality, while also adding back NpcMetadataCorpse parameter in NpcMetadata record.
…and it's much cleaner than what was being used to pass constant values for lasting sight radius, height up, and height down.
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds multiple gameplay and server-side changes: guild applications and DB access, migration tracking, trigger/spawn tracking, navigation fallbacks, auto-save background service, combat/vector sanitization, telemetry for damage/healing, debug-mode balance tweaks, and several API surface and serialization adjustments. Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (5)
Maple2.Tools/VectorMath/Transform.cs (1)
141-156: Good defensive validation, but consider extracting the repeated check pattern.The NaN/Infinity/near-zero validation is a solid safeguard. However, the same check pattern appears four times in this method. Consider extracting it to a helper:
private static bool IsInvalidVector(Vector3 v) { return float.IsNaN(v.X) || float.IsNaN(v.Y) || float.IsNaN(v.Z) || float.IsInfinity(v.X) || float.IsInfinity(v.Y) || float.IsInfinity(v.Z) || v.LengthSquared() < 1e-6f; }This aligns with the
SanitizePositionpattern inFieldSkill.csand improves maintainability.Additionally, consider using the existing
EPSILONconstant (line 12) or a squared variant for consistency with the rest of the class.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Tools/VectorMath/Transform.cs` around lines 141 - 156, Extract the repeated NaN/Infinity/near-zero checks in Transform.cs into a private helper like IsInvalidVector(Vector3 v) and replace the four inline checks with calls to that helper (refer to the code paths that currently set direction = FrontAxis and up = new Vector3(0,0,1)), and when checking near-zero use the class EPSILON constant (squared if you keep LengthSquared) to replace the hardcoded 1e-6f so the threshold is consistent with the rest of the class.Maple2.Server.Game/GameServer.cs (1)
83-92: Good fix for the session migration race condition.The
ReferenceEqualscheck correctly prevents a stale disconnecting session from removing a newly registered session's mapping during migrations. This properly addresses the false-offline bug (PR objective#7).Minor: Consider translating the Chinese comments to English for codebase consistency.
,
♻️ Optional: Translate comments to English
long cid = session.CharacterId; if (cid == 0) return; - // 关键：只允许"当前登记在 sessions 里的那个会话"删除自己 - // 迁移/换图时会出现：新会话先注册，旧会话后断开 - // 如果这里无条件 Remove，会把新会话也删掉 => Heartbeat unknown => 假离线 + // Critical: only allow the session currently registered in 'sessions' to remove itself. + // During migration/map-change the new session registers first, then the old session disconnects. + // An unconditional Remove here would delete the new session => Heartbeat unknown => false offline. if (sessions.TryGetValue(cid, out GameSession? current) && ReferenceEquals(current, session)) { sessions.Remove(cid); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Server.Game/GameServer.cs` around lines 83 - 92, Translate the inline Chinese comments above the session removal logic into clear English while preserving the original meaning: explain that only the session currently registered in the sessions dictionary for the character id (cid) may remove itself to avoid a migration/scene-change race where a new session registers before the old one disconnects, and that without this check Remove could delete the new session causing heartbeat unknown/false-offline; keep the check using sessions.TryGetValue(cid, out GameSession? current) and ReferenceEquals(current, session) and the subsequent sessions.Remove(cid) intact.Maple2.Server.Game/Manager/FishingManager.cs (1)
339-353: Consider using English comments for codebase consistency.The implementation logic is correct and consistent with the existing mastery exp handling. The use of
modifier: 0fwithadditionalExp: charExpeffectively bypasses the table-based exp calculation to use the fish-specificExpvalue directly, which appears intentional.However, the Chinese comments should be translated to English for maintainability across the codebase.
💬 Suggested English comments
- // ====== 钓鱼给角色经验 ====== + // ====== Award character experience for fishing ====== long charExp = selectedFish.Exp; - // 可选倍率：首次 / 奖牌鱼 + // Apply bonus multiplier for first catch or prize fish if (caughtFishType == CaughtFishType.FirstKind) { charExp *= 2; } if (caughtFishType == CaughtFishType.Prize) { charExp *= 2; } if (charExp > 0) { session.Exp.AddExp(ExpType.fishing, modifier: 0f, additionalExp: charExp); } - // ====== 角色经验结束 ====== + // ====== End character experience ======🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Server.Game/Manager/FishingManager.cs` around lines 339 - 353, Replace the Chinese comments around the fishing XP allocation with English ones to match project style: update the block surrounding selectedFish.Exp, caughtFishType checks (CaughtFishType.FirstKind and CaughtFishType.Prize), and the session.Exp.AddExp(ExpType.fishing, modifier: 0f, additionalExp: charExp) call in FishingManager (the charExp calculation and calls should remain unchanged) so the comments explain “granting fish experience,” “apply optional multipliers for first-catch or prize fish,” and “award additional fishing exp via AddExp” in English.Maple2.Server.World/Service/WorldService.Migrate.cs (1)
24-24: Avoid hard-coded migration TTL drift.Line 24 uses
45directly while token validity is already modeled byAuthExpiry. Deriving the migration window from constants keeps these two paths in sync during future tuning.♻️ Suggested refactor
private static readonly TimeSpan AuthExpiry = TimeSpan.FromSeconds(30); +private static readonly TimeSpan MigrationGrace = TimeSpan.FromSeconds(15); @@ - worldServer.MarkMigrating(request.CharacterId, 45); + worldServer.MarkMigrating( + request.CharacterId, + checked((int) (AuthExpiry + MigrationGrace).TotalSeconds) + );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Server.World/Service/WorldService.Migrate.cs` at line 24, The call to worldServer.MarkMigrating(request.CharacterId, 45) uses a hard-coded TTL; replace the literal 45 by deriving the migration window from the existing AuthExpiry model (or a single shared MigrationTtl constant) so token validity and migration TTL stay in sync. Locate the call to worldServer.MarkMigrating and compute the TTL from AuthExpiry (or introduce a MigrationTtl constant computed from AuthExpiry) and pass that value instead of 45; adjust any related callers/tests that assume the literal.Maple2.Server.Game/Model/Field/Entity/FieldSkill.cs (1)
32-41: Consider sharing one vector sanitization utility.
SanitizePositionhere andWriteVector3SSafeinMaple2.Server.Game/Packets/SkillDamagePacket.csencode the same finite+clamp rules. A shared utility would reduce drift risk.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Server.Game/Model/Field/Entity/FieldSkill.cs` around lines 32 - 41, Extract the finite-check + clamp logic into a single helper (e.g., VectorUtils.SanitizeVector3 or VectorSanitizer.Sanitize) and replace the duplicate implementations in SanitizePosition (FieldSkill) and WriteVector3SSafe (SkillDamagePacket) to call that helper; ensure the helper performs the same checks (IsNaN/IsInfinity on X/Y/Z and clamp to short.MinValue/short.MaxValue then return Vector3.Zero for invalid input) and update any call sites to use the shared method so both code paths maintain identical behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Maple2.Model/Game/Market/SoldUgcMarketItem.cs`:
- Around line 20-28: SoldUgcMarketItem currently writes Count via
writer.WriteInt(Count) but Count is never populated (defaults to 1) and the
surrounding empty writer.Write*() calls follow the client packet schema; fix by
locating where SoldUgcMarketItem instances are constructed or hydrated from the
DB (e.g., the ctor, a FromData/Load method or repository mapping) and map the
correct DB column or source field into the Count property (use the real column
name used for stack/quantity in your market table), then add validation in that
same location to ensure Count > 0 (or clamped to 1 if the client expects minimum
1) before serialization in Write; also double-check that the sequence of
writer.WriteLong/WriteUnicodeString/WriteInt calls in SoldUgcMarketItem.Write
matches the client packet field order/schema to avoid misalignment.
- Line 13: SoldUgcMarketItem has a Count property with a default of 1 that is
never populated from or written to the database during conversion; update the
database model and the implicit conversion operators (the implicit operator
methods converting to/from the DB model) so Count is persisted and mapped (add a
Count field to the DB model and copy it in both implicit operators), or if UGC
are truly always single-quantity remove the Count property and all references to
it (including mapping code) to avoid misleading defaults—modify the conversions
in the implicit operators and the DB entity accordingly.
In `@Maple2.Model/Metadata/Constants.cs`:
- Line 126: Normalize and consistently compare/store Unicode names: update
ValidName() to call name = name?.Normalize(NormalizationForm.FormC) (and apply
when creating/validating guild/character names) and ensure the
AllowUnicodeInNames flow stores the normalized string in the database; replace
culture-sensitive comparisons (e.g., any name.ToLower() ==
character.Name.ToLower() checks in GameStorage.User methods and
StringComparison.CurrentCultureIgnoreCase uses in ClubManager and HomeHandler)
with normalized ordinal comparisons (use
normalizedName.Equals(otherNormalizedName, StringComparison.OrdinalIgnoreCase)
or compare OrdinalIgnoreCase after Normalize) so all lookups, validations, and
storage use NFC-normalized values and StringComparison.OrdinalIgnoreCase.
In `@Maple2.Server.Game/Manager/Field/AgentNavigation.cs`:
- Around line 336-338: The fallback path assignment sets currentPath to
[agent.npos, target] but never marks it as a fallback, so the Advance method's
navmesh re-snap logic (Advance) never runs; update both fallback locations (the
Logger.Warning + currentPath assignment around agent.npos/target and the similar
block at the later occurrence) to also set currentPathIsFallback = true
immediately after constructing currentPath so the Advance(...) navmesh re-snap
block can detect and handle fallback paths.
In `@Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs`:
- Around line 122-126: The SpawnNpc flow uses inconsistent SpawnPointId
assignment causing deadSpawnPoints keys to mismatch: update the SpawnNpc method
so that when a SpawnPointNPC argument is provided you set the spawned
FieldNpc.SpawnPointId to spawnPointNpc.SpawnPointId (rather than owner?.Value.Id
?? 0), and ensure any removal
(deadSpawnPoints.TryRemove(spawnPointNpc.SpawnPointId, out _)) and later
death-path additions use the same SpawnPointId source; adjust the assignment in
SpawnNpc (and any overload that calls it) to prefer spawnPointNpc.SpawnPointId
when spawnPointNpc is not null so IsSpawnPointDead(spawnId) remains consistent.
In `@Maple2.Server.Game/Packets/SkillDamagePacket.cs`:
- Around line 23-35: Replace the generic write call in WriteVector3SSafe:
instead of calling ByteWriter.Write<Vector3S>(v) use explicit field writes on
the ByteWriter to serialize the Vector3 components (e.g., WriteShort/WriteInt
for the X, Y, Z fields in the correct packet order and size expected by
Vector3S), preserving the clamped values and the same endianness/ordering;
locate the ByteWriter usage and replace the generic Write<Vector3S> call with
three explicit writes referencing the X, Y and Z components of v.
- Around line 68-69: The packet encodes Command.Damage's direction with
WriteVector3SSafe (Vector3S) but must use the full Vector3 (float3) like the
other direction fields; change the call that writes record.Direction in
SkillDamagePacket (the WriteVector3SSafe on the Command.Damage path) to the
Vector3 helper used elsewhere (e.g., WriteVector3Safe/WriteVector3) so the
direction is serialized as float3 and matches the writes at lines referenced
(the other direction writes at the top and later in SkillDamagePacket).
In `@Maple2.Server.Game/Trigger/TriggerContext.Field.cs`:
- Around line 278-281: GetOrAddMesh can create lightweight placeholder meshes
already marked visible, so the current check if (mesh.Visible == visible) can
incorrectly skip broadcasting the initial update; change the logic so
newly-created placeholders always trigger an update: modify Objects.GetOrAddMesh
to signal creation (either by returning a bool/tuple like (mesh, created) or
setting a TriggerObjectMesh property such as IsPlaceholder/WasCreated), then
only suppress the update when mesh.Visible == visible AND the mesh was not just
created (e.g., if (!created && mesh.Visible == visible) return;), ensuring the
first update for placeholders is always sent.
In `@Maple2.Server.Game/Trigger/TriggerContext.Npc.cs`:
- Around line 228-234: The current logic only checks spawnIds for death when no
matchingMobs exist at all, which lets mixed cases pass; change the block to loop
every spawnId and for each one return false if a live mob exists
(matchingMobs.Any(m => m.SpawnId == spawnId)) or if
Field.IsSpawnPointDead(spawnId) is false, ensuring all spawnIds are confirmed
dead (or absent) before considering the trigger satisfied; update the code that
uses matchingMobs, spawnIds and Field.IsSpawnPointDead accordingly.
In `@Maple2.Server.Game/Util/Sync/PlayerInfoStorage.cs`:
- Around line 85-102: The SendUpdate method’s retry loop currently swallows
failures after maxRetries and blocks the calling thread with Thread.Sleep;
change it to an async retry that uses await Task.Delay instead of Thread.Sleep
(e.g., rename to SendUpdateAsync and make it return Task), replace
Thread.Sleep(200*(i+1)) with await Task.Delay(200*(i+1)), and propagate/await
the new async method at callers; additionally, after the loop finishes without
success, emit a final logger.Error including context and the last exception (or
throw a specific exception) so failures are observable (reference SendUpdate,
world.UpdatePlayer, logger.Warning).
In `@Maple2.Server.World/WorldServer.cs`:
- Around line 127-151: The migration state methods (MarkMigrating,
ClearMigrating, IsMigrating) are only setting/clearing TTL but never consulted;
update the heartbeat/offline transition paths that mark players offline or
disconnect (the methods that currently set offline status or remove sessions) to
first call IsMigrating(characterId) and if true skip or defer the
offline/disconnect logic, and if IsMigrating returns false ensure you call
ClearMigrating(characterId) before proceeding so expired entries are cleaned up;
use the exact method names MarkMigrating, ClearMigrating and IsMigrating to
locate and gate the behavior.
---
Nitpick comments:
In `@Maple2.Server.Game/GameServer.cs`:
- Around line 83-92: Translate the inline Chinese comments above the session
removal logic into clear English while preserving the original meaning: explain
that only the session currently registered in the sessions dictionary for the
character id (cid) may remove itself to avoid a migration/scene-change race
where a new session registers before the old one disconnects, and that without
this check Remove could delete the new session causing heartbeat
unknown/false-offline; keep the check using sessions.TryGetValue(cid, out
GameSession? current) and ReferenceEquals(current, session) and the subsequent
sessions.Remove(cid) intact.
In `@Maple2.Server.Game/Manager/FishingManager.cs`:
- Around line 339-353: Replace the Chinese comments around the fishing XP
allocation with English ones to match project style: update the block
surrounding selectedFish.Exp, caughtFishType checks (CaughtFishType.FirstKind
and CaughtFishType.Prize), and the session.Exp.AddExp(ExpType.fishing, modifier:
0f, additionalExp: charExp) call in FishingManager (the charExp calculation and
calls should remain unchanged) so the comments explain “granting fish
experience,” “apply optional multipliers for first-catch or prize fish,” and
“award additional fishing exp via AddExp” in English.
In `@Maple2.Server.Game/Model/Field/Entity/FieldSkill.cs`:
- Around line 32-41: Extract the finite-check + clamp logic into a single helper
(e.g., VectorUtils.SanitizeVector3 or VectorSanitizer.Sanitize) and replace the
duplicate implementations in SanitizePosition (FieldSkill) and WriteVector3SSafe
(SkillDamagePacket) to call that helper; ensure the helper performs the same
checks (IsNaN/IsInfinity on X/Y/Z and clamp to short.MinValue/short.MaxValue
then return Vector3.Zero for invalid input) and update any call sites to use the
shared method so both code paths maintain identical behavior.
In `@Maple2.Server.World/Service/WorldService.Migrate.cs`:
- Line 24: The call to worldServer.MarkMigrating(request.CharacterId, 45) uses a
hard-coded TTL; replace the literal 45 by deriving the migration window from the
existing AuthExpiry model (or a single shared MigrationTtl constant) so token
validity and migration TTL stay in sync. Locate the call to
worldServer.MarkMigrating and compute the TTL from AuthExpiry (or introduce a
MigrationTtl constant computed from AuthExpiry) and pass that value instead of
45; adjust any related callers/tests that assume the literal.
In `@Maple2.Tools/VectorMath/Transform.cs`:
- Around line 141-156: Extract the repeated NaN/Infinity/near-zero checks in
Transform.cs into a private helper like IsInvalidVector(Vector3 v) and replace
the four inline checks with calls to that helper (refer to the code paths that
currently set direction = FrontAxis and up = new Vector3(0,0,1)), and when
checking near-zero use the class EPSILON constant (squared if you keep
LengthSquared) to replace the hardcoded 1e-6f so the threshold is consistent
with the rest of the class.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
Maple2.Database/Storage/Game/GameStorage.User.csMaple2.Model/Game/Market/SoldUgcMarketItem.csMaple2.Model/Metadata/Constants.csMaple2.Server.Game/GameServer.csMaple2.Server.Game/Manager/DungeonManager.csMaple2.Server.Game/Manager/Field/AgentNavigation.csMaple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.csMaple2.Server.Game/Manager/Field/FieldManager/FieldManager.csMaple2.Server.Game/Manager/Field/FieldManager/IField.csMaple2.Server.Game/Manager/Field/TriggerCollection.csMaple2.Server.Game/Manager/FishingManager.csMaple2.Server.Game/Model/Field/Actor/ActorStateComponent/MovementStateTasks/MovementState.SkillCastTask.csMaple2.Server.Game/Model/Field/Actor/ActorStateComponent/SkillState.csMaple2.Server.Game/Model/Field/Actor/FieldNpc.csMaple2.Server.Game/Model/Field/Entity/FieldSkill.csMaple2.Server.Game/Packets/SkillDamagePacket.csMaple2.Server.Game/Trigger/TriggerContext.Field.csMaple2.Server.Game/Trigger/TriggerContext.Npc.csMaple2.Server.Game/Util/Sync/PlayerInfoStorage.csMaple2.Server.World/Service/WorldService.Migrate.csMaple2.Server.World/WorldServer.csMaple2.Tools/VectorMath/Transform.cs
Comment on lines
+20
to
+28
| writer.WriteInt(Count); | ||
| writer.WriteInt(); | ||
|
|
||
| writer.WriteLong(); | ||
| writer.WriteLong(); | ||
| writer.WriteUnicodeString(); | ||
| writer.WriteUnicodeString(); | ||
| writer.WriteInt(); | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
git ls-files | grep -i "soldugcmarketitem"Repository: MS2Community/Maple2
Length of output: 158
🏁 Script executed:
cat -n Maple2.Model/Game/Market/SoldUgcMarketItem.csRepository: MS2Community/Maple2
Length of output: 1204
🏁 Script executed:
git log --oneline Maple2.Model/Game/Market/SoldUgcMarketItem.cs | head -20Repository: MS2Community/Maple2
Length of output: 80
🏁 Script executed:
git diff HEAD~1 HEAD Maple2.Model/Game/Market/SoldUgcMarketItem.csRepository: MS2Community/Maple2
Length of output: 251
🏁 Script executed:
git show 059fa69 --statRepository: MS2Community/Maple2
Length of output: 50375
🏁 Script executed:
git show 059fa69 -- Maple2.Model/Game/Market/SoldUgcMarketItem.csRepository: MS2Community/Maple2
Length of output: 2821
🏁 Script executed:
find . -type f -name "*.md" -o -name "*schema*" -o -name "*reverse*" | head -20Repository: MS2Community/Maple2
Length of output: 214
🏁 Script executed:
rg -i "reverse.?engineer\|packet.*schema\|client.*expect" --type md | head -30Repository: MS2Community/Maple2
Length of output: 45
🏁 Script executed:
cat -n .claude/skills/decode-packet/skill.md | head -50Repository: MS2Community/Maple2
Length of output: 1976
🏁 Script executed:
rg -i "SoldUgc|Market.*Item" --type cs -A 5 -B 2 | head -100Repository: MS2Community/Maple2
Length of output: 9082
🏁 Script executed:
find . -name "*SoldMeret*" -o -name "*UgcMarket*" | grep -E "\.cs$"Repository: MS2Community/Maple2
Length of output: 708
🏁 Script executed:
cat -n Maple2.Model/Game/Market/UgcMarketItem.csRepository: MS2Community/Maple2
Length of output: 2895
🏁 Script executed:
cat -n Maple2.Model/Game/Market/SoldMeretMarketItem.csRepository: MS2Community/Maple2
Length of output: 142
🏁 Script executed:
cat -n Maple2.Database/Model/Market/SoldUgcMarketItem.csRepository: MS2Community/Maple2
Length of output: 2398
🏁 Script executed:
rg -B 5 -A 5 "WriteInt\(\)\s*$|WriteLong\(\)\s*$|WriteUnicodeString\(\)\s*$" Maple2.Model/Game/Market/ | head -80Repository: MS2Community/Maple2
Length of output: 45
🏁 Script executed:
rg "SoldUgcMarketItem" --type cs | grep -E "(WriteTo|WriteClass|packet)" | head -20Repository: MS2Community/Maple2
Length of output: 167
🏁 Script executed:
rg -B 3 -A 10 "SoldUgcMarketItem" Maple2.Server.Game/Manager/UgcMarketManager.csRepository: MS2Community/Maple2
Length of output: 1513
🏁 Script executed:
rg "WriteTo.*SoldUgc\|SoldUgc.*WriteClass" --type csRepository: MS2Community/Maple2
Length of output: 45
🏁 Script executed:
rg -B 5 -A 15 "WriteClass<SoldUgcMarketItem>" Maple2.Server.Game/Packets/MeretMarketPacket.csRepository: MS2Community/Maple2
Length of output: 773
🏁 Script executed:
cat -n Maple2.Server.Game/Packets/MeretMarketPacket.cs | head -100Repository: MS2Community/Maple2
Length of output: 3832
Ensure the Count property is properly sourced and validated against client packet schema.
The empty Write() calls (lines 17, 21, 23-26) follow the reverse-engineering pattern used throughout the codebase (e.g., UgcMarketItem), so they are not arbitrary padding. However, the Count property is added to the game model but never populated from the database—it always defaults to 1. Verify that Count represents the correct field in the client packet schema and has a legitimate value source, otherwise this misalignment between the database and packet models could cause deserialization errors on the client.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Maple2.Model/Game/Market/SoldUgcMarketItem.cs` around lines 20 - 28,
SoldUgcMarketItem currently writes Count via writer.WriteInt(Count) but Count is
never populated (defaults to 1) and the surrounding empty writer.Write*() calls
follow the client packet schema; fix by locating where SoldUgcMarketItem
instances are constructed or hydrated from the DB (e.g., the ctor, a
FromData/Load method or repository mapping) and map the correct DB column or
source field into the Count property (use the real column name used for
stack/quantity in your market table), then add validation in that same location
to ensure Count > 0 (or clamped to 1 if the client expects minimum 1) before
serialization in Write; also double-check that the sequence of
writer.WriteLong/WriteUnicodeString/WriteInt calls in SoldUgcMarketItem.Write
matches the client packet field order/schema to avoid misalignment.
| public const bool DebugTriggers = false; // Set to true to enable debug triggers. (It'll write triggers to files and load triggers from files instead of DB) | ||
|
|
||
| public const bool AllowUnicodeInNames = false; // Allow Unicode characters in character and guild names | ||
| public const bool AllowUnicodeInNames = true; // Allow Unicode characters in character and guild names |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Where the toggle is used.
rg -n --type=cs -C2 '\bAllowUnicodeInNames\b'
# 2) Name comparison logic (case handling).
rg -n --type=cs -C2 'Name\.ToLower\(\)|Name\.ToUpper\(\)|StringComparison\.(OrdinalIgnoreCase|InvariantCultureIgnoreCase|CurrentCultureIgnoreCase)'
# 3) Unicode normalization/confusable handling in validation paths.
rg -n --type=cs -C2 'Normalize\(|NormalizationForm|Rune|IdnMapping|Confusable|CharacterName|GuildName'Repository: MS2Community/Maple2
Length of output: 50376
🏁 Script executed:
#!/bin/bash
# Get the CharacterNameValidator to see regex patterns and validation logic
cat -n Maple2.Model/Validators/CharacterNameValidator.cs
# Check if names are normalized anywhere before storage or comparison
rg -n --type=cs 'Normalize|NormalizationForm|ToNormalized' | head -20Repository: MS2Community/Maple2
Length of output: 4053
Apply Unicode NFC normalization to all character and guild names at creation, validation, and lookup.
The regex [\p{L}0-9]+ accepts both precomposed and decomposed Unicode forms (e.g., "café" as é vs. e+combining-accent). Without explicit NFC normalization before storage and comparison, the same name can be created twice in different forms, and lookups using .ToLower() without normalization will miss matches.
Database lookups in GameStorage.User.cs (lines 37, 112) compare name.ToLower() == character.Name.ToLower(), which does not normalize combining characters. Additionally, some comparisons use StringComparison.CurrentCultureIgnoreCase (ClubManager, HomeHandler) instead of OrdinalIgnoreCase, creating culture-dependent behavior.
Add string.Normalize(NormalizationForm.FormC) to the ValidName() method, store normalized names in the database, and ensure all name comparisons use consistent ordinal normalization.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Maple2.Model/Metadata/Constants.cs` at line 126, Normalize and consistently
compare/store Unicode names: update ValidName() to call name =
name?.Normalize(NormalizationForm.FormC) (and apply when creating/validating
guild/character names) and ensure the AllowUnicodeInNames flow stores the
normalized string in the database; replace culture-sensitive comparisons (e.g.,
any name.ToLower() == character.Name.ToLower() checks in GameStorage.User
methods and StringComparison.CurrentCultureIgnoreCase uses in ClubManager and
HomeHandler) with normalized ordinal comparisons (use
normalizedName.Equals(otherNormalizedName, StringComparison.OrdinalIgnoreCase)
or compare OrdinalIgnoreCase after Normalize) so all lookups, validations, and
storage use NFC-normalized values and StringComparison.OrdinalIgnoreCase.
Comment on lines
+336
to
+338
| Logger.Warning("Failed to find path from {FromNav} to {ToNav}; fallback: clamped straight-line on navmesh. World(from)={FromWorld} World(to)={ToWorld}", agent.npos, target, DotRecastHelper.FromNavMeshSpace(agent.npos), worldTarget); | ||
| currentPath = [agent.npos, target]; | ||
| return true; |
Contributor
There was a problem hiding this comment.
Fallback mode is never activated for fallback paths.
When fallback paths are created (currentPath = [agent.npos, target]), currentPathIsFallback is never set to true. That means the navmesh re-snap block in Advance (Lines 224-235) will never run.
Suggested fix
Logger.Warning("Failed to find path from {FromNav} to {ToNav}; fallback: clamped straight-line on navmesh. World(from)={FromWorld} World(to)={ToWorld}", agent.npos, target, DotRecastHelper.FromNavMeshSpace(agent.npos), worldTarget);
currentPath = [agent.npos, target];
+currentPathIsFallback = true;
return true; Logger.Warning("Failed to find path from {FromNav} to {ToNav}; fallback: clamped straight-line on navmesh. World(from)={FromWorld} World(to)={ToWorld}", agent.npos, target, DotRecastHelper.FromNavMeshSpace(agent.npos), worldTarget);
currentPath = [agent.npos, target];
+currentPathIsFallback = true;
return true;Also applies to: 407-409
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Maple2.Server.Game/Manager/Field/AgentNavigation.cs` around lines 336 - 338,
The fallback path assignment sets currentPath to [agent.npos, target] but never
marks it as a fallback, so the Advance method's navmesh re-snap logic (Advance)
never runs; update both fallback locations (the Logger.Warning + currentPath
assignment around agent.npos/target and the similar block at the later
occurrence) to also set currentPathIsFallback = true immediately after
constructing currentPath so the Advance(...) navmesh re-snap block can detect
and handle fallback paths.
Comment on lines
122
to
+126
| public FieldNpc? SpawnNpc(NpcMetadata npc, Vector3 position, Vector3 rotation, bool disableAi = false, FieldMobSpawn? owner = null, SpawnPointNPC? spawnPointNpc = null, string spawnAnimation = "") { | ||
|
|
||
| if (spawnPointNpc is not null) { | ||
| deadSpawnPoints.TryRemove(spawnPointNpc.SpawnPointId, out _); | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== All SpawnPointId assignments =="
rg -n -C3 --type cs '\bSpawnPointId\s*='
echo
echo "== SpawnPointNPC SpawnNpc call paths =="
rg -n -C6 --type cs '\bSpawnNpc\s*\(.*SpawnPointNPC'
rg -n -C8 --type cs '\bSpawnOnCreate\s*\('
echo
echo "== IsSpawnPointDead usage sites =="
rg -n -C4 --type cs '\bIsSpawnPointDead\s*\('Repository: MS2Community/Maple2
Length of output: 24092
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== deadSpawnPoints.TryAdd calls =="
rg -n -C5 --type cs 'deadSpawnPoints\.TryAdd'
echo
echo "== Check FieldNpc death/removal flow =="
rg -n -C6 --type cs 'OnDeath|OnRemove|deadSpawnPoints' Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs
echo
echo "== RemoveNpc implementation =="
rg -n -A20 --type cs 'public bool RemoveNpc\(' Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.csRepository: MS2Community/Maple2
Length of output: 4660
Confirm and apply spawn-point ID mismatch fix.
The issue is real: Line 125 clears deadSpawnPoints using spawnPointNpc.SpawnPointId, but Line 161 initializes SpawnPointId to owner?.Value.Id ?? 0. When the convenience overload SpawnNpc(NpcMetadata, SpawnPointNPC) is used, owner is null, so SpawnPointId becomes 0. Later, when the NPC dies, Line 968 attempts to add npc.SpawnPointId (0) to deadSpawnPoints, breaking the key consistency that Line 125 relied on. This breaks IsSpawnPointDead(spawnId) for SpawnPointNPC flows.
Fix
var fieldNpc = new FieldNpc(this, NextLocalId(), agent, new Npc(npc, animation), aiPath, patrolDataUUID: spawnPointNpc?.PatrolData, spawnAnimation: spawnAnimation) {
Owner = owner,
Position = spawnPosition,
Rotation = rotation,
Origin = owner?.Position ?? spawnPosition,
- SpawnPointId = owner?.Value.Id ?? 0,
+ SpawnPointId = spawnPointNpc?.SpawnPointId ?? owner?.Value.Id ?? 0,
};🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs` around
lines 122 - 126, The SpawnNpc flow uses inconsistent SpawnPointId assignment
causing deadSpawnPoints keys to mismatch: update the SpawnNpc method so that
when a SpawnPointNPC argument is provided you set the spawned
FieldNpc.SpawnPointId to spawnPointNpc.SpawnPointId (rather than owner?.Value.Id
?? 0), and ensure any removal
(deadSpawnPoints.TryRemove(spawnPointNpc.SpawnPointId, out _)) and later
death-path additions use the same SpawnPointId source; adjust the assignment in
SpawnNpc (and any overload that calls it) to prefer spawnPointNpc.SpawnPointId
when spawnPointNpc is not null so IsSpawnPointDead(spawnId) remains consistent.
Comment on lines
+68
to
+69
| WriteVector3SSafe(pWriter, record.Position); // Impact | ||
| WriteVector3SSafe(pWriter, record.Direction); |
Contributor
There was a problem hiding this comment.
Keep Command.Damage direction encoded as Vector3 (float3).
Line 69 now serializes record.Direction with the Vector3S helper. That diverges from other direction writes in this packet (Line 46, Line 118, Line 140) and can break client packet parsing for Command.Damage.
Suggested fix
+private static void WriteVector3Safe(ByteWriter w, Vector3 v) {
+ if (!float.IsFinite(v.X) || !float.IsFinite(v.Y) || !float.IsFinite(v.Z)) {
+ v = Vector3.Zero;
+ }
+ w.WriteFloat(v.X);
+ w.WriteFloat(v.Y);
+ w.WriteFloat(v.Z);
+}
+
public static ByteWriter Damage(DamageRecord record) {
var pWriter = Packet.Of(SendOp.SkillDamage);
pWriter.Write<Command>(Command.Damage);
@@
pWriter.WriteByte(record.MotionPoint);
pWriter.WriteByte(record.AttackPoint);
WriteVector3SSafe(pWriter, record.Position); // Impact
- WriteVector3SSafe(pWriter, record.Direction);
+ WriteVector3Safe(pWriter, record.Direction);As per coding guidelines "DO NOT add arbitrary packet fields. Packet structures MUST match the client's exact expectations from reverse engineering."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Maple2.Server.Game/Packets/SkillDamagePacket.cs` around lines 68 - 69, The
packet encodes Command.Damage's direction with WriteVector3SSafe (Vector3S) but
must use the full Vector3 (float3) like the other direction fields; change the
call that writes record.Direction in SkillDamagePacket (the WriteVector3SSafe on
the Command.Damage path) to the Vector3 helper used elsewhere (e.g.,
WriteVector3Safe/WriteVector3) so the direction is serialized as float3 and
matches the writes at lines referenced (the other direction writes at the top
and later in SkillDamagePacket).
Comment on lines
+278
to
281
| // Some data packs are missing Ms2TriggerMesh entries in DB import. | ||
| // Create a lightweight placeholder so the client can still receive updates. | ||
| TriggerObjectMesh mesh = Objects.GetOrAddMesh(triggerId); | ||
| if (mesh.Visible == visible) { |
Contributor
There was a problem hiding this comment.
Force first update for newly created mesh placeholders.
Line 281 can suppress a needed update: GetOrAddMesh() creates placeholders as visible, so an initial visible: true command for missing imports may never be broadcast.
💡 Proposed fix
foreach (int triggerId in triggerIds) {
- // Some data packs are missing Ms2TriggerMesh entries in DB import.
- // Create a lightweight placeholder so the client can still receive updates.
- TriggerObjectMesh mesh = Objects.GetOrAddMesh(triggerId);
- if (mesh.Visible == visible) {
+ bool existed = Objects.Meshes.TryGetValue(triggerId, out TriggerObjectMesh? mesh);
+ // Some data packs are missing Ms2TriggerMesh entries in DB import.
+ // Create a lightweight placeholder so the client can still receive updates.
+ mesh ??= Objects.GetOrAddMesh(triggerId);
+ if (existed && mesh.Visible == visible) {
continue;
}📝 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.
Suggested change
| // Some data packs are missing Ms2TriggerMesh entries in DB import. | |
| // Create a lightweight placeholder so the client can still receive updates. | |
| TriggerObjectMesh mesh = Objects.GetOrAddMesh(triggerId); | |
| if (mesh.Visible == visible) { | |
| bool existed = Objects.Meshes.TryGetValue(triggerId, out TriggerObjectMesh? mesh); | |
| // Some data packs are missing Ms2TriggerMesh entries in DB import. | |
| // Create a lightweight placeholder so the client can still receive updates. | |
| mesh ??= Objects.GetOrAddMesh(triggerId); | |
| if (existed && mesh.Visible == visible) { |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Maple2.Server.Game/Trigger/TriggerContext.Field.cs` around lines 278 - 281,
GetOrAddMesh can create lightweight placeholder meshes already marked visible,
so the current check if (mesh.Visible == visible) can incorrectly skip
broadcasting the initial update; change the logic so newly-created placeholders
always trigger an update: modify Objects.GetOrAddMesh to signal creation (either
by returning a bool/tuple like (mesh, created) or setting a TriggerObjectMesh
property such as IsPlaceholder/WasCreated), then only suppress the update when
mesh.Visible == visible AND the mesh was not just created (e.g., if (!created &&
mesh.Visible == visible) return;), ensuring the first update for placeholders is
always sent.
Comment on lines
+228
to
+234
| if (!matchingMobs.Any()) { | ||
| foreach (int spawnId in spawnIds) { | ||
| if (!Field.IsSpawnPointDead(spawnId)) { | ||
| return false; | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
MonsterDead can return true when only a subset of spawnIds are actually dead.
Line 228 gates fallback on !matchingMobs.Any(), so mixed cases are missed (some spawnIds present+dead, others absent+not-dead-marker). That can prematurely satisfy trigger conditions.
Proposed fix
- IEnumerable<FieldNpc> matchingMobs = Field.Mobs.Values.Where(x => spawnIds.Contains(x.SpawnPointId));
+ List<FieldNpc> matchingMobs = Field.Mobs.Values
+ .Where(x => spawnIds.Contains(x.SpawnPointId))
+ .ToList();
- foreach (FieldNpc mob in matchingMobs) {
- if (!mob.IsDead) {
- return false;
- }
- }
-
- // If we found at least one matching mob, reaching here means all of them are dead.
- // If we found none, then only consider the spawnIds dead if they are marked as dead by the field.
- if (!matchingMobs.Any()) {
- foreach (int spawnId in spawnIds) {
- if (!Field.IsSpawnPointDead(spawnId)) {
- return false;
- }
- }
- }
+ Dictionary<int, List<FieldNpc>> mobsBySpawnId = matchingMobs
+ .GroupBy(m => m.SpawnPointId)
+ .ToDictionary(g => g.Key, g => g.ToList());
+
+ foreach (int spawnId in spawnIds) {
+ if (mobsBySpawnId.TryGetValue(spawnId, out List<FieldNpc>? mobsAtSpawn)) {
+ if (mobsAtSpawn.Any(m => !m.IsDead)) {
+ return false;
+ }
+ continue;
+ }
+
+ if (!Field.IsSpawnPointDead(spawnId)) {
+ return false;
+ }
+ }
return true;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Maple2.Server.Game/Trigger/TriggerContext.Npc.cs` around lines 228 - 234, The
current logic only checks spawnIds for death when no matchingMobs exist at all,
which lets mixed cases pass; change the block to loop every spawnId and for each
one return false if a live mob exists (matchingMobs.Any(m => m.SpawnId ==
spawnId)) or if Field.IsSpawnPointDead(spawnId) is false, ensuring all spawnIds
are confirmed dead (or absent) before considering the trigger satisfied; update
the code that uses matchingMobs, spawnIds and Field.IsSpawnPointDead
accordingly.
Comment on lines
85
to
+102
| public void SendUpdate(PlayerUpdateRequest request) { | ||
| try { | ||
| //PlayerInfoCache | ||
| world.UpdatePlayer(request); | ||
| } catch (RpcException) { /* ignored */ } | ||
| // 对“上线态/频道/地图”更新非常关键，不能静默吞掉失败 | ||
| const int maxRetries = 3; | ||
| for (int i = 0; i < maxRetries; i++) { | ||
| try { | ||
| world.UpdatePlayer(request); | ||
| return; | ||
| } catch (RpcException ex) { | ||
| logger.Warning("SendUpdate(UpdatePlayer) failed attempt {Attempt}/{Max}. Status={Status}", | ||
| i + 1, maxRetries, ex.Status); | ||
|
|
||
| // 小退避，避免瞬间打爆 | ||
| Thread.Sleep(200 * (i + 1)); | ||
| } catch (Exception ex) { | ||
| logger.Warning(ex, "SendUpdate(UpdatePlayer) unexpected failure"); | ||
| Thread.Sleep(200 * (i + 1)); | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Silent failure after retry exhaustion and blocking sleep.
Two concerns with this retry implementation:
-
No final error log or feedback when all retries fail: After 3 failed attempts, the method exits silently. Given the comment states this is critical for online state/channel/map updates, silent failure could perpetuate the very issue this PR aims to fix (players falsely appearing offline). At minimum, log an error after the loop.
-
Thread.Sleepblocks the calling thread: If called from a packet handler or field update context, blocking withThread.Sleepcan degrade throughput. Consider making this methodasyncand usingTask.Delayinstead.
🛠️ Proposed fix
- public void SendUpdate(PlayerUpdateRequest request) {
+ public async Task SendUpdateAsync(PlayerUpdateRequest request) {
// 对"上线态/频道/地图"更新非常关键，不能静默吞掉失败
const int maxRetries = 3;
for (int i = 0; i < maxRetries; i++) {
try {
world.UpdatePlayer(request);
return;
} catch (RpcException ex) {
logger.Warning("SendUpdate(UpdatePlayer) failed attempt {Attempt}/{Max}. Status={Status}",
i + 1, maxRetries, ex.Status);
// 小退避，避免瞬间打爆
- Thread.Sleep(200 * (i + 1));
+ await Task.Delay(200 * (i + 1));
} catch (Exception ex) {
logger.Warning(ex, "SendUpdate(UpdatePlayer) unexpected failure");
- Thread.Sleep(200 * (i + 1));
+ await Task.Delay(200 * (i + 1));
}
}
+
+ logger.Error("SendUpdate(UpdatePlayer) failed after {Max} attempts for CharacterId={CharacterId}",
+ maxRetries, request.CharacterId);
}If async is not feasible for call-site reasons, at minimum add the final error log after the loop to ensure failures are observable.
📝 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.
Suggested change
| public void SendUpdate(PlayerUpdateRequest request) { | |
| try { | |
| //PlayerInfoCache | |
| world.UpdatePlayer(request); | |
| } catch (RpcException) { /* ignored */ } | |
| // 对“上线态/频道/地图”更新非常关键，不能静默吞掉失败 | |
| const int maxRetries = 3; | |
| for (int i = 0; i < maxRetries; i++) { | |
| try { | |
| world.UpdatePlayer(request); | |
| return; | |
| } catch (RpcException ex) { | |
| logger.Warning("SendUpdate(UpdatePlayer) failed attempt {Attempt}/{Max}. Status={Status}", | |
| i + 1, maxRetries, ex.Status); | |
| // 小退避，避免瞬间打爆 | |
| Thread.Sleep(200 * (i + 1)); | |
| } catch (Exception ex) { | |
| logger.Warning(ex, "SendUpdate(UpdatePlayer) unexpected failure"); | |
| Thread.Sleep(200 * (i + 1)); | |
| } | |
| } | |
| public void SendUpdate(PlayerUpdateRequest request) { | |
| // 对"上线态/频道/地图"更新非常关键，不能静默吞掉失败 | |
| const int maxRetries = 3; | |
| for (int i = 0; i < maxRetries; i++) { | |
| try { | |
| world.UpdatePlayer(request); | |
| return; | |
| } catch (RpcException ex) { | |
| logger.Warning("SendUpdate(UpdatePlayer) failed attempt {Attempt}/{Max}. Status={Status}", | |
| i + 1, maxRetries, ex.Status); | |
| // 小退避，避免瞬间打爆 | |
| Thread.Sleep(200 * (i + 1)); | |
| } catch (Exception ex) { | |
| logger.Warning(ex, "SendUpdate(UpdatePlayer) unexpected failure"); | |
| Thread.Sleep(200 * (i + 1)); | |
| } | |
| } | |
| logger.Error("SendUpdate(UpdatePlayer) failed after {Max} attempts for CharacterId={CharacterId}", | |
| maxRetries, request.CharacterId); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Maple2.Server.Game/Util/Sync/PlayerInfoStorage.cs` around lines 85 - 102, The
SendUpdate method’s retry loop currently swallows failures after maxRetries and
blocks the calling thread with Thread.Sleep; change it to an async retry that
uses await Task.Delay instead of Thread.Sleep (e.g., rename to SendUpdateAsync
and make it return Task), replace Thread.Sleep(200*(i+1)) with await
Task.Delay(200*(i+1)), and propagate/await the new async method at callers;
additionally, after the loop finishes without success, emit a final logger.Error
including context and the last exception (or throw a specific exception) so
failures are observable (reference SendUpdate, world.UpdatePlayer,
logger.Warning).
Comment on lines
+127
to
+151
| public void MarkMigrating(long characterId, int seconds = 45) { | ||
| if (characterId == 0) return; | ||
| long until = DateTimeOffset.UtcNow.ToUnixTimeSeconds() + seconds; | ||
| _migratingUntil[characterId] = until; | ||
| } | ||
|
|
||
| public void ClearMigrating(long characterId) { | ||
| if (characterId == 0) return; | ||
| _migratingUntil.TryRemove(characterId, out _); | ||
| } | ||
|
|
||
| private bool IsMigrating(long characterId) { | ||
| if (characterId == 0) return false; | ||
|
|
||
| if (!_migratingUntil.TryGetValue(characterId, out long until)) { | ||
| return false; | ||
| } | ||
|
|
||
| long now = DateTimeOffset.UtcNow.ToUnixTimeSeconds(); | ||
| if (now <= until) return true; | ||
|
|
||
| // expired | ||
| _migratingUntil.TryRemove(characterId, out _); | ||
| return false; | ||
| } |
Contributor
There was a problem hiding this comment.
Migration guard is currently write/clear only, not enforcing behavior.
At Line 127 and Line 138, migration state and TTL cleanup are implemented, but this file still needs to consult that state during heartbeat/offline transitions. Otherwise, migrating players can still be marked offline during handoff, and expired entries rely on paths that may never run after failed migrations.
🔧 Proposed integration point
@@ private void Heartbeat() {
foreach (PlayerInfo playerInfo in playerInfoLookup.GetOnlinePlayerInfos()) {
+ if (IsMigrating(playerInfo.CharacterId)) {
+ // Skip transient checks while character is migrating between services.
+ continue;
+ }
if (playerInfo.CharacterId == 0) {
logger.Information("Player {CharacterId} is online without a character id, setting to offline", playerInfo.CharacterId);
playerInfo.Channel = -1;
continue;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Maple2.Server.World/WorldServer.cs` around lines 127 - 151, The migration
state methods (MarkMigrating, ClearMigrating, IsMigrating) are only
setting/clearing TTL but never consulted; update the heartbeat/offline
transition paths that mark players offline or disconnect (the methods that
currently set offline status or remove sessions) to first call
IsMigrating(characterId) and if true skip or defer the offline/disconnect logic,
and if IsMigrating returns false ensure you call ClearMigrating(characterId)
before proceeding so expired entries are cleaned up; use the exact method names
MarkMigrating, ClearMigrating and IsMigrating to locate and gate the behavior.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
Maple2.Server.Game/Util/ChatUtil.cs (1)
10-17:⚠️ Potential issue | 🟠 MajorScope the item save with the standard unit-of-work pattern.
This helper opens a
GameStorage.Requestand never disposes it. Wrap the save inusing GameStorage.Request db = session.GameStorage.Context();before sending the announcement.As per coding guidelines, "Use storage classes with unit-of-work pattern for all data access: `using GameStorage.Request db = gameStorage.Context() { ... }`".♻️ Minimal fix
- GameStorage.Request db = session.GameStorage.Context(); - db.SaveItems(session.CharacterId, item); + using GameStorage.Request db = session.GameStorage.Context(); + db.SaveItems(session.CharacterId, item);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Server.Game/Util/ChatUtil.cs` around lines 10 - 17, The GameStorage.Request opened in SendEnchantAnnouncement is not disposed; wrap the context acquisition and save in a unit-of-work scope by using the `using GameStorage.Request db = session.GameStorage.Context()` pattern so `db.SaveItems(session.CharacterId, item)` executes inside that using block before proceeding to send the announcement; update SendEnchantAnnouncement to obtain the `GameStorage.Request` via `session.GameStorage.Context()` inside a using and perform the save there to ensure proper disposal.Maple2.Server.World/Containers/PartyManager.cs (1)
286-304:⚠️ Potential issue | 🟠 MajorSerialize vote-expiry callbacks with
mutex.Both timeout tasks read and clear
Party.Voteoutside the lock while reply paths mutate the same object concurrently. That leaves a race whereParty.Votecan becomenullbetween the null-check and the next property access, and it can also broadcastExpiredVoteafter the vote already completed.🔒 Proposed fix
Task.Factory.StartNew(() => { Thread.Sleep(TimeSpan.FromSeconds(partyVoteReadyDurationSeconds)); - if (Party.Vote == null) { - return; - } - int count = Party.Vote.Disapprovals.Count + Party.Vote.Approvals.Count; - if (count >= Party.Vote.Voters.Count) { - Party.Vote = null; - return; - } - - Broadcast(new PartyRequest { - ExpiredVote = new PartyRequest.Types.ExpiredVote { - PartyId = Party.Id, - }, - }); - - Party.Vote = null; + bool expired = false; + lock (mutex) { + PartyVote? vote = Party.Vote; + if (vote == null || vote.Type != PartyVoteType.ReadyCheck) { + return; + } + + int count = vote.Disapprovals.Count + vote.Approvals.Count; + if (count >= vote.Voters.Count) { + Party.Vote = null; + return; + } + + Party.Vote = null; + expired = true; + } + + if (expired) { + Broadcast(new PartyRequest { + ExpiredVote = new PartyRequest.Types.ExpiredVote { + PartyId = Party.Id, + }, + }); + } });Apply the same pattern to the vote-kick timeout path.
Also applies to: 397-411
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Server.World/Containers/PartyManager.cs` around lines 286 - 304, The timeout task for handling vote expiry (the Task.Factory.StartNew block that reads/clears Party.Vote and calls Broadcast with PartyRequest.Types.ExpiredVote) performs null-checks and mutations outside the same mutex used by reply paths; fix it by acquiring the same mutex before reading Party.Vote, computing count, deciding to broadcast, and clearing Party.Vote so the null-check, Broadcast decision and Party.Vote = null are atomic; apply the identical mutex-wrapping change to the vote-kick timeout path (the similar Task.Factory.StartNew at the 397-411 region) to prevent races between timeout and reply handlers.Maple2.Server.Game/Manager/ConfigManager.cs (1)
118-126:⚠️ Potential issue | 🟠 MajorLoaded stat allocations still bypass the new per-stat caps.
This only resets when the total spent points exceed
TotalPoints. If the server-table lowers a single stat cap, an existing save can still load over-cap allocations and keep stats thatAllocateStatPoint()would now reject. Validate each loaded entry againstStatLimit(..., statLimits)before applying it.🧮 Suggested fix
if (load.Allocation != null) { - // Reset points if total points exceed the limit. - if (load.Allocation.Values.Sum() > statAttributes.TotalPoints) { + bool invalidAllocation = load.Allocation.Values.Sum() > statAttributes.TotalPoints + || load.Allocation.Any(entry => entry.Value < 0 + || entry.Value > StatAttributes.PointAllocation.StatLimit(entry.Key, statLimits)); + if (invalidAllocation) { load.Allocation.Clear(); } else { foreach ((BasicAttribute attribute, int amount) in load.Allocation) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Server.Game/Manager/ConfigManager.cs` around lines 118 - 126, When loading saved allocations in ConfigManager (the load.Allocation handling), individual stat values are not validated against per-stat caps, so entries can exceed StatLimit; change the loop that applies entries (the foreach over load.Allocation) to validate each (BasicAttribute attribute, int amount) against StatLimit(attribute, amount, statLimits) (or the existing StatLimit method/signature used elsewhere) and clamp or discard amounts that exceed the per-stat cap before writing into statAttributes.Allocation and calling UpdateStatAttribute; mirror the same validation logic used by AllocateStatPoint so loaded saves cannot bypass statLimits while still enforcing the TotalPoints check.Maple2.Server.Game/Manager/Items/InventoryManager.cs (1)
541-570:⚠️ Potential issue | 🟠 MajorCharge expansion by purchased row count.
expandRowCountcan represent multiple rows, but both the affordability check and the deduction only useInventoryExpandPrice1Rowonce. That lets 2+ row expansions cost the same as a single row.💸 Suggested fix
+ int rowCount = expandRowCount / Constant.InventoryExpandRowCount; + int expandPrice = checked(rowCount * session.ServerTableMetadata.ConstantsTable.InventoryExpandPrice1Row); + - if (session.Currency.Meret < session.ServerTableMetadata.ConstantsTable.InventoryExpandPrice1Row) { + if (session.Currency.Meret < expandPrice) { session.Send(ItemInventoryPacket.Error(s_cannot_charge_merat)); return false; } @@ - session.Currency.Meret -= session.ServerTableMetadata.ConstantsTable.InventoryExpandPrice1Row; + session.Currency.Meret -= expandPrice;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Server.Game/Manager/Items/InventoryManager.cs` around lines 541 - 570, The affordability check and currency deduction in InventoryManager.Expand use InventoryExpandPrice1Row only once, so multi-row expansions are undercharged; update the check and subtraction to multiply the per-row price by expandRowCount (e.g. compute totalPrice = session.ServerTableMetadata.ConstantsTable.InventoryExpandPrice1Row * expandRowCount) and use totalPrice for the Meret comparison and session.Currency.Meret -= totalPrice; locate these changes around the existing references to InventoryExpandPrice1Row inside Expand (the affordability check and the subsequent deduction) and ensure numeric types are handled to avoid overflow.Maple2.Server.Game/Manager/ExperienceManager.cs (1)
214-237:⚠️ Potential issue | 🔴 CriticalPrestige levels are being applied twice.
Line 228 already increments
PrestigeLevelinside the leveling loop.PrestigeLevelUp(PrestigeLevel - startLevel)then adds the same delta again at Line 237, so a single prestige gain can overshoot the intended level while mission/reward bookkeeping still uses the originalamount. Make either the loop orPrestigeLevelUp()own the level mutation, but not both.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Server.Game/Manager/ExperienceManager.cs` around lines 214 - 237, The loop and PrestigeLevelUp both mutate PrestigeLevel, causing double-application; change the loop to accumulate into a local newLevel (start with startLevel) instead of incrementing PrestigeLevel inside the for loop, subtracting PrestigeCurrentExp as you do, and after the loop set PrestigeLevel = Math.Clamp(newLevel, amount, session.ServerTableMetadata.ConstantsTable.AdventureLevelLimit) (or compute delta = newLevel - startLevel and then call PrestigeLevelUp(delta) if PrestigeLevelUp must perform side-effects), so only one place actually updates PrestigeLevel; update references to PrestigeLevel inside this block accordingly and keep PrestigeLevelUp's contract unchanged.
♻️ Duplicate comments (2)
Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs (1)
122-126:⚠️ Potential issue | 🔴 CriticalUse the spawn-point's ID when constructing spawn-point NPCs.
deadSpawnPointsis cleared withspawnPointNpc.SpawnPointIdon Lines 124-125 and repopulated withnpc.SpawnPointIdon Lines 973-975, but Line 163 still initializes spawned NPCs withowner?.Value.Id ?? 0. Spawn-point NPC flows therefore clear one key and later add another, soIsSpawnPointDead(spawnId)remains wrong after despawn.Suggested fix
var fieldNpc = new FieldNpc(this, NextLocalId(), agent, new Npc(npc, animation, ServerTableMetadata.ConstantsTable.NpcLastingSightRadius, ServerTableMetadata.ConstantsTable.NpcLastingSightHeightUp, ServerTableMetadata.ConstantsTable.NpcLastingSightHeightDown), aiPath, patrolDataUUID: spawnPointNpc?.PatrolData, spawnAnimation: spawnAnimation) { Owner = owner, Position = spawnPosition, Rotation = rotation, Origin = owner?.Position ?? spawnPosition, - SpawnPointId = owner?.Value.Id ?? 0, + SpawnPointId = spawnPointNpc?.SpawnPointId ?? owner?.Value.Id ?? 0, };Also applies to: 156-163, 973-975
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs` around lines 122 - 126, SpawnNpc is constructing spawn-point NPCs using owner?.Value.Id ?? 0 which causes deadSpawnPoints to be cleared with spawnPointNpc.SpawnPointId but later repopulated under npc.SpawnPointId (and vice versa), breaking IsSpawnPointDead(spawnId); update the NPC construction logic in SpawnNpc (and the other affected initialization site that currently uses owner?.Value.Id ?? 0) to use spawnPointNpc.SpawnPointId when spawnPointNpc is non-null so the same spawn-point ID is used for both removing and adding entries to deadSpawnPoints (ensure other code that later repopulates deadSpawnPoints uses npc.SpawnPointId consistently).Maple2.Server.World/WorldServer.cs (1)
127-151:⚠️ Potential issue | 🟠 MajorMigration tracking is still inert.
These helpers are never consulted before
Heartbeat()callsSetOffline(). Until that path checksIsMigrating(characterId), players can still be marked offline during handoff and the new TTL map will never prune expired entries in normal operation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Server.World/WorldServer.cs` around lines 127 - 151, The migration helpers (MarkMigrating, ClearMigrating, IsMigrating) are never used when Heartbeat calls SetOffline, so players get marked offline during handoff; update the heartbeat/offline path to consult IsMigrating(characterId) and skip calling SetOffline if it returns true (or have SetOffline itself check IsMigrating at the start) and ensure when a character is finally taken offline you call ClearMigrating(characterId) so the TTL map can be pruned normally; reference Heartbeat(), SetOffline(), IsMigrating(), MarkMigrating(), and ClearMigrating() when making the change.
🧹 Nitpick comments (6)
Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/MovementStateTasks/MovementState.CleanupTask.cs (1)
36-37: Consider caching the TalkableDistance value in a local variable for readability.The same property chain is accessed twice. While functionally correct, extracting it to a local variable improves clarity.
♻️ Suggested refactor
- float maxDistance = player.Session.ServerTableMetadata.ConstantsTable.TalkableDistance * - player.Session.ServerTableMetadata.ConstantsTable.TalkableDistance; + float talkableDistance = player.Session.ServerTableMetadata.ConstantsTable.TalkableDistance; + float maxDistanceSquared = talkableDistance * talkableDistance;Note: Also renamed
maxDistancetomaxDistanceSquaredto clarify it's being compared againstDistanceSquared.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/MovementStateTasks/MovementState.CleanupTask.cs` around lines 36 - 37, Extract player.Session.ServerTableMetadata.ConstantsTable.TalkableDistance into a local variable (e.g., talkableDistance) inside MovementState.CleanupTask, compute maxDistanceSquared = talkableDistance * talkableDistance instead of re-evaluating the property chain twice, and replace the existing maxDistance usage with maxDistanceSquared so the comparison against DistanceSquared is clearer and the property access is only done once.Maple2.Server.Game/PacketHandlers/ClubHandler.cs (1)
67-87: Extract club-name validation into one helper.Create and rename now depend on the same whitespace/length/banned-word/name-validator checks, including the same
ConstantsTablebounds. Centralizing that logic will keep the rules in sync the next time the server-table contract changes.Also applies to: 238-259
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Server.Game/PacketHandlers/ClubHandler.cs` around lines 67 - 87, Extract the repeated club-name checks into a single helper (e.g., bool IsValidClubName(string name, out ClubError error) or ValidateClubName returning a ClubError) and replace the duplicated logic in HandleCreate and the rename handler with a call to that helper; the helper should perform the same checks in order—string.IsNullOrWhiteSpace, length bounds via session.ServerTableMetadata.ConstantsTable.ClubNameLengthMin/Max, BanWordStorage.ContainsBannedWord, and NameValidator.ValidName—and return the same ClubError.s_club_err_name_value (or a specific error) so callers can call session.Send(ClubPacket.Error(error)). Ensure the helper is accessible to both handlers and uses the session metadata for bounds rather than hard-coded values.Maple2.Server.Game/PacketHandlers/GuildHandler.cs (1)
211-218: Consider extractingGuildCreatePriceto a local variable.
GuildCreatePriceis accessed three times (twice in the condition, once for deduction). Extracting to a local variable would improve readability and avoid repeated property access.♻️ Optional refactor
+ long guildCreatePrice = session.ServerTableMetadata.ConstantsTable.GuildCreatePrice; - if (session.Currency.CanAddMeso(-session.ServerTableMetadata.ConstantsTable.GuildCreatePrice) != -session.ServerTableMetadata.ConstantsTable.GuildCreatePrice) { + if (session.Currency.CanAddMeso(-guildCreatePrice) != -guildCreatePrice) { session.Send(GuildPacket.Error(GuildError.s_guild_err_no_money)); return; }And at line 239:
- session.Currency.Meso -= session.ServerTableMetadata.ConstantsTable.GuildCreatePrice; + session.Currency.Meso -= guildCreatePrice;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Server.Game/PacketHandlers/GuildHandler.cs` around lines 211 - 218, Extract session.ServerTableMetadata.ConstantsTable.GuildCreatePrice into a local readonly variable (e.g., guildCreatePrice) at the start of the guild creation flow in GuildHandler.cs, then replace the three uses (the two comparisons that reference GuildCreatePrice and the subsequent currency deduction/CanAddMeso call) with that local variable; ensure you still use the same semantics when comparing via session.Currency.CanAddMeso(-guildCreatePrice) and when deducting currency so behavior is unchanged.Maple2.Server.Game/Model/Field/Actor/FieldPlayer.cs (1)
404-406: Note: Typo in constant nameUserRevivalPaneltyTick.The constant name appears to have a typo ("Panelty" instead of "Penalty"). If this matches the source data naming, it's fine to keep for consistency. Otherwise, consider correcting it in the
ConstantsTabledefinition.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Server.Game/Model/Field/Actor/FieldPlayer.cs` around lines 404 - 406, The code uses a misspelled constant Session.ServerTableMetadata.ConstantsTable.UserRevivalPaneltyTick (typo "Panelty"); fix it by renaming the constant to UserRevivalPenaltyTick in the ConstantsTable definition and update all references (including the call inside the Field.Metadata.Property.DeathPenalty branch that calls Session.Config.UpdateDeathPenalty(...UserRevivalPaneltyTick)) to the corrected name, or if the external source requires the original spelling, add a new correctly-spelled alias UserRevivalPenaltyTick that points to the same value and replace usages to use the correct identifier.Maple2.Server.Game/Model/Field/Tombstone.cs (1)
27-32: Consider extracting repeated constant access for readability.The
hitPerDeadCountconstant is accessed three times in the calculation. While functionally correct, extracting to a local variable would improve readability.♻️ Optional refactor for readability
public Tombstone(FieldPlayer owner, int totalDeaths) { Owner = owner; - TotalHitCount = (byte) Math.Min(totalDeaths * owner.Session.ServerTableMetadata.ConstantsTable.hitPerDeadCount, - owner.Session.ServerTableMetadata.ConstantsTable.hitPerDeadCount * owner.Session.ServerTableMetadata.ConstantsTable.maxDeadCount); + int hitPerDeath = owner.Session.ServerTableMetadata.ConstantsTable.hitPerDeadCount; + int maxDeaths = owner.Session.ServerTableMetadata.ConstantsTable.maxDeadCount; + TotalHitCount = (byte) Math.Min(totalDeaths * hitPerDeath, hitPerDeath * maxDeaths); hitsRemaining = TotalHitCount; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Server.Game/Model/Field/Tombstone.cs` around lines 27 - 32, In the Tombstone constructor, extract owner.Session.ServerTableMetadata.ConstantsTable.hitPerDeadCount (and optionally maxDeadCount) into a local variable to avoid repeated property access and improve readability; then compute TotalHitCount using that local hitPerDeadCount (and maxDeadCount) and assign hitsRemaining as before—update references in the constructor (Tombstone, TotalHitCount, hitsRemaining) to use the new local variables.Maple2.Model/Game/User/StatAttributes.cs (1)
16-18: CopystatLimitsinstead of retaining the caller-owned dictionary.
PointAllocationnow holds a mutable external dictionary by reference. If server metadata is reloaded or patched later, existing characters can see their stat caps change mid-session, and reads here depend on external synchronization. Prefer snapshotting the values and exposing them asIReadOnlyDictionary<string, int>.Suggested refactor
-public StatAttributes(IDictionary<string, int> statLimits) { +public StatAttributes(IReadOnlyDictionary<string, int> statLimits) { Sources = new PointSources(); Allocation = new PointAllocation(statLimits); } ... - private readonly IDictionary<string, int> statLimits; + private readonly IReadOnlyDictionary<string, int> statLimits; ... - public PointAllocation(IDictionary<string, int> statLimits) { + public PointAllocation(IReadOnlyDictionary<string, int> statLimits) { points = new Dictionary<BasicAttribute, int>(); - this.statLimits = statLimits; + this.statLimits = new Dictionary<string, int>(statLimits); }As per coding guidelines, "Ensure thread safety: Packet handlers are stateless singletons; state lives in Session instances. FieldManager runs on dedicated thread per map. Use locks for concurrent database operations."
Also applies to: 55-77
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Model/Game/User/StatAttributes.cs` around lines 16 - 18, StatAttributes currently forwards the caller-owned statLimits dictionary into PointAllocation (via the StatAttributes constructor and the Allocation property), retaining a mutable external reference; instead make a defensive copy (snapshot) of statLimits into an immutable/read-only structure and pass that snapshot to PointAllocation so existing characters cannot observe later server-side dictionary mutations—change the StatAttributes constructor to create a new Dictionary<string,int> (or IReadOnlyDictionary<string,int>) from the statLimits parameter and expose Allocation as/read from an IReadOnlyDictionary to ensure thread-safe, snapshot semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Maple2.Database/Storage/Game/GameStorage.Map.cs`:
- Around line 114-121: GetSoonestPlotFromExpire currently mutates tracked UgcMap
entities in the foreach (adjusting map.ExpiryTime), which can make subsequent
queries and SaveChanges incorrect; instead compute the effective expiry without
mutating by projecting maps into an in-memory sequence of (UgcMap,
effectiveExpiry) via Select/SelectMany (e.g. use maps.Select(map => new { Map =
map, EffectiveExpiry = map.OwnerId == 0 ?
map.ExpiryTime.Add(ugcHomeSaleWaitingTime) : map.ExpiryTime }) and then OrderBy
on EffectiveExpiry and pick the first projection to construct the PlotInfo; do
not call map.ExpiryTime = ... or otherwise modify the UgcMap entities so no
tracked state is changed.
In `@Maple2.File.Ingest/Mapper/ServerTableMapper.cs`:
- Around line 2152-2160: The loop over parser.ParseConstants() currently ignores
unknown keys by continuing when propertyLookup.TryGetValue(trimmedKey, out
PropertyInfo? property) fails; change this to fail fast or emit a diagnostic
instead: when a trimmedKey has no matching property on the ConstantsTable (use
propertyLookup and the constants object as context), either throw a descriptive
exception (including trimmedKey and available property names) or call the
existing ingest diagnostic/logger to record a clear error, then stop processing;
keep the subsequent CleanConstantsInput(...) and SetValue(...) logic unchanged
so known keys are still handled.
- Around line 2165-2185: The CleanConstantsInput method currently risks
IndexOutOfRangeException and contains an unnecessary integer-leading-zero
normalization; at the start of CleanConstantsInput(string input, string
propName, Type type) add a defensive guard like if
(string.IsNullOrWhiteSpace(input)) return input.Trim(); then remove the entire
int-leading-zero branch that checks input[0]; also tighten float handling to
only strip a trailing 'f' (e.g. if (type == typeof(float) &&
(input.EndsWith("f", StringComparison.OrdinalIgnoreCase))) input = input[..^1];)
and keep the bool and TimeSpan (UgcHomeSaleWaitingTime) handling as-is. Ensure
you reference CleanConstantsInput and propName == "UgcHomeSaleWaitingTime" when
making these edits.
In `@Maple2.Model/Game/Cube/Nurturing.cs`:
- Around line 51-60: The Feed method currently mutates state even for
non-positive input; add an early guard at the start of Feed(int
nurturingEatGrowth) that returns immediately when nurturingEatGrowth <= 0 so
Exp, Stage and LastFeedTime are not changed for zero or negative values; keep
the existing checks against NurturingMetadata.RequiredGrowth and Stage after
this guard so only positive growth can update Exp, potentially increment Stage,
and set LastFeedTime.
- Around line 56-59: The code increments Stage only once after adding
nurturingEatGrowth to Exp, which can leave Stage behind if Exp crosses multiple
thresholds; update the logic in Nurturing.cs (the block that updates Exp, Stage,
and reads NurturingMetadata.RequiredGrowth) to loop: after increasing Exp,
repeatedly compare Exp against the required Exp for the current Stage (using
NurturingMetadata.RequiredGrowth.First(x => x.Stage == Stage).Exp) and increment
Stage until Exp is less than the required value or Stage reaches its maximum,
ensuring you guard against going past the highest defined Stage.
In `@Maple2.Model/Game/Mail.cs`:
- Around line 37-50: The parameterless Mail() constructor leaves ExpiryTime at
its default (0), causing newly created mails in AttendanceHandler and
ItemUseHandler to be immediately expired; update the parameterless Mail() to
initialize ExpiryTime to a sane default (e.g.,
DateTimeOffset.UtcNow.AddDays(defaultExpiryDays).ToUnixTimeSeconds()) or
delegate to the existing Mail(int mailExpiryDays) constructor so any code that
uses new Mail() gets a valid expiry; ensure the ExpiryTime field/property in the
Mail class is set during object construction (check the Mail() constructor and
the Mail(int) overload and adjust callers in AttendanceHandler and
ItemUseHandler if they require a different expiry).
In `@Maple2.Model/Metadata/Constants.cs`:
- Line 165: The DummyNpc method currently maps Gender.Female to DummyNpcFemale
and everything else (including Gender.All) to DummyNpcMale; change
DummyNpc(Gender gender) to explicitly handle Gender.All (or throw for unexpected
values) by replacing the ternary with a switch on the Gender enum that returns
DummyNpcFemale for Gender.Female, DummyNpcMale for Gender.Male, and either a
clear mapping for Gender.All or an ArgumentOutOfRangeException/default branch to
fail fast; update references to DummyNpc, DummyNpcFemale, DummyNpcMale and the
Gender enum accordingly.
In `@Maple2.Model/Metadata/ServerTable/ConstantsTable.cs`:
- Around line 673-687: Add a load-time invariant in ConstantsTable to validate
every bagSlotTab*Count array (e.g., bagSlotTabGameCount, bagSlotTabSkinCount,
bagSlotTabSummonCount, bagSlotTabMaterialCount, bagSlotTabMasteryCount,
bagSlotTabLifeCount, bagSlotTabQuestCount, bagSlotTabGemCount,
bagSlotTabPetCount, bagSlotTabActiveSkillCount, bagSlotTabCoinCount,
bagSlotTabBadgeCount, bagSlotTabMiscCount, bagSlotTabLapenShardCount,
bagSlotTabPieceCount) has Length == 2 (base and maxExpand) before exposing the
table; perform this check in the ConstantsTable load/constructor path and fail
fast (throw a clear exception or return a load error) if any array is null or
its Length != 2 so malformed server data cannot propagate — apply the same
validation for the other occurrence noted around line ~949.
In `@Maple2.Server.Game/Commands/PlayerCommand.cs`:
- Around line 130-132: The error branches in PlayerCommand.cs (where level and
prestige input are validated) print errors and return but leave ctx.ExitCode at
success; update both validation failure branches (the one checking level and the
one checking prestige) to set a non-zero exit code (e.g., ctx.ExitCode = 1)
before returning so calling scripts see the command failed; locate the checks
around the existing conditional that writes to ctx.Console.Error.WriteLine and
add the ctx.ExitCode assignment in each rejection path.
In `@Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs`:
- Around line 414-417: The helper that sets nearestRef = 0 and returns false is
logging a Warning on every navmesh miss (logger.Warning(...)), which floods logs
because callers like ValidPosition() call it frequently; change the log level to
logger.Debug (or implement a simple rate-limit/throttle) so routine misses don't
create noise—update the call site using logger.Warning(...) in this helper to
logger.Debug(...) (or add a lightweight rate limiter around that log emission)
and ensure callers like ValidPosition() still get the false return behavior
unchanged.
In `@Maple2.Server.Game/Manager/SkillManager.cs`:
- Around line 124-128: The code assumes
session.ServerTableMetadata.ConstantsTable.SkillBookTreeAddTabFeeMerat is
non-negative; first read it into a local variable (e.g., fee) and validate fee
>= 0 before the affordability check and subtraction: if fee < 0, treat it as an
invalid config and return false (or log and return false) so you never credit
players; then use that validated fee for the existing check
(session.Currency.Meret < fee) and the subtraction (session.Currency.Meret -=
fee).
In `@Maple2.Server.Game/PacketHandlers/MeretMarketHandler.cs`:
- Around line 200-202: The relist path is assigning client-supplied price
directly to item.Price, allowing bypass of server min/max checks; update the
relist logic where item.Price = price to clamp price between
session.ServerTableMetadata.ConstantsTable.UGCShopSellMinPrice and
UGCShopMetadata.ConstantsTable.UGCShopSellMaxPrice (same enforcement used in
HandleListItem), then assign the clamped value to item.Price so relists use the
same limits as new listings.
In `@Maple2.Server.Game/PacketHandlers/TaxiHandler.cs`:
- Around line 139-140: The code in TaxiHandler.cs checks session.Currency.Meret
but sends the meso error StringCode.s_err_lack_meso; change the notification to
the Meret-specific insufficiency error by replacing the MessageBox argument with
the correct Meret error constant (e.g., StringCode.s_err_lack_meret or your
project’s equivalent) so the client shows the proper currency error when
session.Currency.Meret is insufficient.
In `@Maple2.Server.Game/Service/AutoSaveService.cs`:
- Around line 44-51: The loop in AutoSaveService that calls
session.SessionSave() should isolate failures per session: wrap the
SessionSave() invocation inside a try-catch around each iteration (the foreach
over GameSession sessions), so a thrown exception does not exit the loop; log
the exception (including identifying info like session.Player.Id or session.Id
if available) using the service logger, and only increment saved on successful
completion of session.SessionSave(); continue to the next session on error.
---
Outside diff comments:
In `@Maple2.Server.Game/Manager/ConfigManager.cs`:
- Around line 118-126: When loading saved allocations in ConfigManager (the
load.Allocation handling), individual stat values are not validated against
per-stat caps, so entries can exceed StatLimit; change the loop that applies
entries (the foreach over load.Allocation) to validate each (BasicAttribute
attribute, int amount) against StatLimit(attribute, amount, statLimits) (or the
existing StatLimit method/signature used elsewhere) and clamp or discard amounts
that exceed the per-stat cap before writing into statAttributes.Allocation and
calling UpdateStatAttribute; mirror the same validation logic used by
AllocateStatPoint so loaded saves cannot bypass statLimits while still enforcing
the TotalPoints check.
In `@Maple2.Server.Game/Manager/ExperienceManager.cs`:
- Around line 214-237: The loop and PrestigeLevelUp both mutate PrestigeLevel,
causing double-application; change the loop to accumulate into a local newLevel
(start with startLevel) instead of incrementing PrestigeLevel inside the for
loop, subtracting PrestigeCurrentExp as you do, and after the loop set
PrestigeLevel = Math.Clamp(newLevel, amount,
session.ServerTableMetadata.ConstantsTable.AdventureLevelLimit) (or compute
delta = newLevel - startLevel and then call PrestigeLevelUp(delta) if
PrestigeLevelUp must perform side-effects), so only one place actually updates
PrestigeLevel; update references to PrestigeLevel inside this block accordingly
and keep PrestigeLevelUp's contract unchanged.
In `@Maple2.Server.Game/Manager/Items/InventoryManager.cs`:
- Around line 541-570: The affordability check and currency deduction in
InventoryManager.Expand use InventoryExpandPrice1Row only once, so multi-row
expansions are undercharged; update the check and subtraction to multiply the
per-row price by expandRowCount (e.g. compute totalPrice =
session.ServerTableMetadata.ConstantsTable.InventoryExpandPrice1Row *
expandRowCount) and use totalPrice for the Meret comparison and
session.Currency.Meret -= totalPrice; locate these changes around the existing
references to InventoryExpandPrice1Row inside Expand (the affordability check
and the subsequent deduction) and ensure numeric types are handled to avoid
overflow.
In `@Maple2.Server.Game/Util/ChatUtil.cs`:
- Around line 10-17: The GameStorage.Request opened in SendEnchantAnnouncement
is not disposed; wrap the context acquisition and save in a unit-of-work scope
by using the `using GameStorage.Request db = session.GameStorage.Context()`
pattern so `db.SaveItems(session.CharacterId, item)` executes inside that using
block before proceeding to send the announcement; update SendEnchantAnnouncement
to obtain the `GameStorage.Request` via `session.GameStorage.Context()` inside a
using and perform the save there to ensure proper disposal.
In `@Maple2.Server.World/Containers/PartyManager.cs`:
- Around line 286-304: The timeout task for handling vote expiry (the
Task.Factory.StartNew block that reads/clears Party.Vote and calls Broadcast
with PartyRequest.Types.ExpiredVote) performs null-checks and mutations outside
the same mutex used by reply paths; fix it by acquiring the same mutex before
reading Party.Vote, computing count, deciding to broadcast, and clearing
Party.Vote so the null-check, Broadcast decision and Party.Vote = null are
atomic; apply the identical mutex-wrapping change to the vote-kick timeout path
(the similar Task.Factory.StartNew at the 397-411 region) to prevent races
between timeout and reply handlers.
---
Duplicate comments:
In `@Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs`:
- Around line 122-126: SpawnNpc is constructing spawn-point NPCs using
owner?.Value.Id ?? 0 which causes deadSpawnPoints to be cleared with
spawnPointNpc.SpawnPointId but later repopulated under npc.SpawnPointId (and
vice versa), breaking IsSpawnPointDead(spawnId); update the NPC construction
logic in SpawnNpc (and the other affected initialization site that currently
uses owner?.Value.Id ?? 0) to use spawnPointNpc.SpawnPointId when spawnPointNpc
is non-null so the same spawn-point ID is used for both removing and adding
entries to deadSpawnPoints (ensure other code that later repopulates
deadSpawnPoints uses npc.SpawnPointId consistently).
In `@Maple2.Server.World/WorldServer.cs`:
- Around line 127-151: The migration helpers (MarkMigrating, ClearMigrating,
IsMigrating) are never used when Heartbeat calls SetOffline, so players get
marked offline during handoff; update the heartbeat/offline path to consult
IsMigrating(characterId) and skip calling SetOffline if it returns true (or have
SetOffline itself check IsMigrating at the start) and ensure when a character is
finally taken offline you call ClearMigrating(characterId) so the TTL map can be
pruned normally; reference Heartbeat(), SetOffline(), IsMigrating(),
MarkMigrating(), and ClearMigrating() when making the change.
---
Nitpick comments:
In `@Maple2.Model/Game/User/StatAttributes.cs`:
- Around line 16-18: StatAttributes currently forwards the caller-owned
statLimits dictionary into PointAllocation (via the StatAttributes constructor
and the Allocation property), retaining a mutable external reference; instead
make a defensive copy (snapshot) of statLimits into an immutable/read-only
structure and pass that snapshot to PointAllocation so existing characters
cannot observe later server-side dictionary mutations—change the StatAttributes
constructor to create a new Dictionary<string,int> (or
IReadOnlyDictionary<string,int>) from the statLimits parameter and expose
Allocation as/read from an IReadOnlyDictionary to ensure thread-safe, snapshot
semantics.
In
`@Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/MovementStateTasks/MovementState.CleanupTask.cs`:
- Around line 36-37: Extract
player.Session.ServerTableMetadata.ConstantsTable.TalkableDistance into a local
variable (e.g., talkableDistance) inside MovementState.CleanupTask, compute
maxDistanceSquared = talkableDistance * talkableDistance instead of
re-evaluating the property chain twice, and replace the existing maxDistance
usage with maxDistanceSquared so the comparison against DistanceSquared is
clearer and the property access is only done once.
In `@Maple2.Server.Game/Model/Field/Actor/FieldPlayer.cs`:
- Around line 404-406: The code uses a misspelled constant
Session.ServerTableMetadata.ConstantsTable.UserRevivalPaneltyTick (typo
"Panelty"); fix it by renaming the constant to UserRevivalPenaltyTick in the
ConstantsTable definition and update all references (including the call inside
the Field.Metadata.Property.DeathPenalty branch that calls
Session.Config.UpdateDeathPenalty(...UserRevivalPaneltyTick)) to the corrected
name, or if the external source requires the original spelling, add a new
correctly-spelled alias UserRevivalPenaltyTick that points to the same value and
replace usages to use the correct identifier.
In `@Maple2.Server.Game/Model/Field/Tombstone.cs`:
- Around line 27-32: In the Tombstone constructor, extract
owner.Session.ServerTableMetadata.ConstantsTable.hitPerDeadCount (and optionally
maxDeadCount) into a local variable to avoid repeated property access and
improve readability; then compute TotalHitCount using that local hitPerDeadCount
(and maxDeadCount) and assign hitsRemaining as before—update references in the
constructor (Tombstone, TotalHitCount, hitsRemaining) to use the new local
variables.
In `@Maple2.Server.Game/PacketHandlers/ClubHandler.cs`:
- Around line 67-87: Extract the repeated club-name checks into a single helper
(e.g., bool IsValidClubName(string name, out ClubError error) or
ValidateClubName returning a ClubError) and replace the duplicated logic in
HandleCreate and the rename handler with a call to that helper; the helper
should perform the same checks in order—string.IsNullOrWhiteSpace, length bounds
via session.ServerTableMetadata.ConstantsTable.ClubNameLengthMin/Max,
BanWordStorage.ContainsBannedWord, and NameValidator.ValidName—and return the
same ClubError.s_club_err_name_value (or a specific error) so callers can call
session.Send(ClubPacket.Error(error)). Ensure the helper is accessible to both
handlers and uses the session metadata for bounds rather than hard-coded values.
In `@Maple2.Server.Game/PacketHandlers/GuildHandler.cs`:
- Around line 211-218: Extract
session.ServerTableMetadata.ConstantsTable.GuildCreatePrice into a local
readonly variable (e.g., guildCreatePrice) at the start of the guild creation
flow in GuildHandler.cs, then replace the three uses (the two comparisons that
reference GuildCreatePrice and the subsequent currency deduction/CanAddMeso
call) with that local variable; ensure you still use the same semantics when
comparing via session.Currency.CanAddMeso(-guildCreatePrice) and when deducting
currency so behavior is unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b8fe8ef3-e1f3-4af6-9edd-8a8960020ff0
📒 Files selected for processing (59)
Maple2.Database/Model/Mail.csMaple2.Database/Storage/Game/GameStorage.Map.csMaple2.Database/Storage/Metadata/ServerTableMetadataStorage.csMaple2.File.Ingest/Mapper/NpcMapper.csMaple2.File.Ingest/Mapper/ServerTableMapper.csMaple2.Model/Common/ServerTableNames.csMaple2.Model/Game/Cube/Nurturing.csMaple2.Model/Game/Mail.csMaple2.Model/Game/Npc/Npc.csMaple2.Model/Game/User/StatAttributes.csMaple2.Model/Metadata/Constants.csMaple2.Model/Metadata/NpcMetadata.csMaple2.Model/Metadata/ServerTable/ConstantsTable.csMaple2.Model/Metadata/ServerTableMetadata.csMaple2.Server.Game/Commands/PlayerCommand.csMaple2.Server.Game/Manager/BlackMarketManager.csMaple2.Server.Game/Manager/BuddyManager.csMaple2.Server.Game/Manager/BuffManager.csMaple2.Server.Game/Manager/ConfigManager.csMaple2.Server.Game/Manager/CurrencyManager.csMaple2.Server.Game/Manager/ExperienceManager.csMaple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.csMaple2.Server.Game/Manager/Field/FieldManager/FieldManager.csMaple2.Server.Game/Manager/FishingManager.csMaple2.Server.Game/Manager/ItemMergeManager.csMaple2.Server.Game/Manager/Items/InventoryManager.csMaple2.Server.Game/Manager/Items/StorageManager.csMaple2.Server.Game/Manager/SkillManager.csMaple2.Server.Game/Manager/TradeManager.csMaple2.Server.Game/Model/Field/Actor/ActorStateComponent/MovementStateTasks/MovementState.CleanupTask.csMaple2.Server.Game/Model/Field/Actor/FieldNpc.csMaple2.Server.Game/Model/Field/Actor/FieldPet.csMaple2.Server.Game/Model/Field/Actor/FieldPlayer.csMaple2.Server.Game/Model/Field/Tombstone.csMaple2.Server.Game/PacketHandlers/BeautyHandler.csMaple2.Server.Game/PacketHandlers/ClubHandler.csMaple2.Server.Game/PacketHandlers/FunctionCubeHandler.csMaple2.Server.Game/PacketHandlers/GuildHandler.csMaple2.Server.Game/PacketHandlers/HomeBankHandler.csMaple2.Server.Game/PacketHandlers/HomeDoctorHandler.csMaple2.Server.Game/PacketHandlers/ItemLockHandler.csMaple2.Server.Game/PacketHandlers/JobHandler.csMaple2.Server.Game/PacketHandlers/MeretMarketHandler.csMaple2.Server.Game/PacketHandlers/MesoMarketHandler.csMaple2.Server.Game/PacketHandlers/NpcTalkHandler.csMaple2.Server.Game/PacketHandlers/PartyHandler.csMaple2.Server.Game/PacketHandlers/QuestHandler.csMaple2.Server.Game/PacketHandlers/SystemShopHandler.csMaple2.Server.Game/PacketHandlers/TaxiHandler.csMaple2.Server.Game/PacketHandlers/UserChatHandler.csMaple2.Server.Game/Program.csMaple2.Server.Game/Service/AutoSaveService.csMaple2.Server.Game/Session/GameSession.csMaple2.Server.Game/Util/ChatUtil.csMaple2.Server.Login/PacketHandlers/CharacterManagementHandler.csMaple2.Server.World/Containers/PartyLookup.csMaple2.Server.World/Containers/PartyManager.csMaple2.Server.World/Program.csMaple2.Server.World/WorldServer.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- Maple2.Server.Game/Manager/FishingManager.cs
- Maple2.Server.Game/Model/Field/Actor/FieldNpc.cs
Comment on lines
114
to
121
| public PlotInfo? GetSoonestPlotFromExpire(TimeSpan ugcHomeSaleWaitingTime) { | ||
| IQueryable<UgcMap> maps = Context.UgcMap.Where(map => map.ExpiryTime > DateTimeOffset.MinValue && !map.Indoor); | ||
| foreach (UgcMap map in maps) { | ||
| if (map.OwnerId == 0) { | ||
| map.ExpiryTime = map.ExpiryTime.Add(Constant.UgcHomeSaleWaitingTime); | ||
| map.ExpiryTime = map.ExpiryTime.Add(ugcHomeSaleWaitingTime); | ||
| } | ||
| } | ||
| UgcMap? model = maps.OrderBy(map => map.ExpiryTime).FirstOrDefault(); |
Contributor
There was a problem hiding this comment.
Do not mutate tracked UgcMap rows while computing effective expiry.
This loop adjusts tracked entities in memory, but maps.OrderBy(...) reissues a SQL query against the original database values. That can choose the wrong plot, and a later SaveChanges on the same request can persist the artificial delay.
🛠️ Proposed fix
public PlotInfo? GetSoonestPlotFromExpire(TimeSpan ugcHomeSaleWaitingTime) {
- IQueryable<UgcMap> maps = Context.UgcMap.Where(map => map.ExpiryTime > DateTimeOffset.MinValue && !map.Indoor);
- foreach (UgcMap map in maps) {
- if (map.OwnerId == 0) {
- map.ExpiryTime = map.ExpiryTime.Add(ugcHomeSaleWaitingTime);
- }
- }
- UgcMap? model = maps.OrderBy(map => map.ExpiryTime).FirstOrDefault();
+ UgcMap? model = Context.UgcMap
+ .AsNoTracking()
+ .Where(map => map.ExpiryTime > DateTimeOffset.MinValue && !map.Indoor)
+ .AsEnumerable()
+ .OrderBy(map => map.OwnerId == 0
+ ? map.ExpiryTime.Add(ugcHomeSaleWaitingTime)
+ : map.ExpiryTime)
+ .FirstOrDefault();
return model == null ? null : ToPlotInfo(model);
}📝 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.
Suggested change
| public PlotInfo? GetSoonestPlotFromExpire(TimeSpan ugcHomeSaleWaitingTime) { | |
| IQueryable<UgcMap> maps = Context.UgcMap.Where(map => map.ExpiryTime > DateTimeOffset.MinValue && !map.Indoor); | |
| foreach (UgcMap map in maps) { | |
| if (map.OwnerId == 0) { | |
| map.ExpiryTime = map.ExpiryTime.Add(Constant.UgcHomeSaleWaitingTime); | |
| map.ExpiryTime = map.ExpiryTime.Add(ugcHomeSaleWaitingTime); | |
| } | |
| } | |
| UgcMap? model = maps.OrderBy(map => map.ExpiryTime).FirstOrDefault(); | |
| public PlotInfo? GetSoonestPlotFromExpire(TimeSpan ugcHomeSaleWaitingTime) { | |
| UgcMap? model = Context.UgcMap | |
| .AsNoTracking() | |
| .Where(map => map.ExpiryTime > DateTimeOffset.MinValue && !map.Indoor) | |
| .AsEnumerable() | |
| .OrderBy(map => map.OwnerId == 0 | |
| ? map.ExpiryTime.Add(ugcHomeSaleWaitingTime) | |
| : map.ExpiryTime) | |
| .FirstOrDefault(); | |
| return model == null ? null : ToPlotInfo(model); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Maple2.Database/Storage/Game/GameStorage.Map.cs` around lines 114 - 121,
GetSoonestPlotFromExpire currently mutates tracked UgcMap entities in the
foreach (adjusting map.ExpiryTime), which can make subsequent queries and
SaveChanges incorrect; instead compute the effective expiry without mutating by
projecting maps into an in-memory sequence of (UgcMap, effectiveExpiry) via
Select/SelectMany (e.g. use maps.Select(map => new { Map = map, EffectiveExpiry
= map.OwnerId == 0 ? map.ExpiryTime.Add(ugcHomeSaleWaitingTime) : map.ExpiryTime
}) and then OrderBy on EffectiveExpiry and pick the first projection to
construct the PlotInfo; do not call map.ExpiryTime = ... or otherwise modify the
UgcMap entities so no tracked state is changed.
Comment on lines
+2152
to
+2160
| foreach ((string key, Constants.Key constant) in parser.ParseConstants()) { | ||
| string trimmedKey = key.Trim(); | ||
| if (!propertyLookup.TryGetValue(trimmedKey, out PropertyInfo? property)) continue; | ||
| string cleanValue = CleanConstantsInput( | ||
| constant.value.Trim(), | ||
| trimmedKey, | ||
| property.PropertyType | ||
| ); | ||
| SetValue(property, constants, cleanValue); |
Contributor
There was a problem hiding this comment.
Don't silently ignore unknown constants keys.
If a server-table key stops matching a ConstantsTable property, this leaves the property at its default value and the ingest succeeds with bad metadata. That turns table drift into hard-to-diagnose gameplay bugs. Fail fast here or at least emit a diagnostic for the missing key.
Suggested fix
foreach ((string key, Constants.Key constant) in parser.ParseConstants()) {
string trimmedKey = key.Trim();
- if (!propertyLookup.TryGetValue(trimmedKey, out PropertyInfo? property)) continue;
+ if (!propertyLookup.TryGetValue(trimmedKey, out PropertyInfo? property)) {
+ throw new InvalidOperationException($"Unknown constants key '{trimmedKey}'.");
+ }
string cleanValue = CleanConstantsInput(
constant.value.Trim(),
trimmedKey,
property.PropertyType
);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Maple2.File.Ingest/Mapper/ServerTableMapper.cs` around lines 2152 - 2160, The
loop over parser.ParseConstants() currently ignores unknown keys by continuing
when propertyLookup.TryGetValue(trimmedKey, out PropertyInfo? property) fails;
change this to fail fast or emit a diagnostic instead: when a trimmedKey has no
matching property on the ConstantsTable (use propertyLookup and the constants
object as context), either throw a descriptive exception (including trimmedKey
and available property names) or call the existing ingest diagnostic/logger to
record a clear error, then stop processing; keep the subsequent
CleanConstantsInput(...) and SetValue(...) logic unchanged so known keys are
still handled.
Maple2.Model/Game/Cube/Nurturing.cs
Outdated
Comment on lines
51
to
60
| public void Feed(int nurturingEatGrowth) { | ||
| if (Exp >= NurturingMetadata.RequiredGrowth.Last().Exp) { | ||
| return; | ||
| } | ||
|
|
||
| Exp += Constant.NurturingEatGrowth; | ||
| Exp += nurturingEatGrowth; | ||
| if (Exp >= NurturingMetadata.RequiredGrowth.First(x => x.Stage == Stage).Exp) { | ||
| Stage++; | ||
| } | ||
| LastFeedTime = DateTimeOffset.Now; |
Contributor
There was a problem hiding this comment.
Reject non-positive growth before mutating state.
With the new parameterized input, Feed(0) still updates LastFeedTime, and negative values can reduce Exp. Guard this at the method boundary.
Proposed fix
public void Feed(int nurturingEatGrowth) {
+ if (nurturingEatGrowth <= 0) {
+ throw new ArgumentOutOfRangeException(nameof(nurturingEatGrowth));
+ }
+
if (Exp >= NurturingMetadata.RequiredGrowth.Last().Exp) {
return;
}📝 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.
Suggested change
| public void Feed(int nurturingEatGrowth) { | |
| if (Exp >= NurturingMetadata.RequiredGrowth.Last().Exp) { | |
| return; | |
| } | |
| Exp += Constant.NurturingEatGrowth; | |
| Exp += nurturingEatGrowth; | |
| if (Exp >= NurturingMetadata.RequiredGrowth.First(x => x.Stage == Stage).Exp) { | |
| Stage++; | |
| } | |
| LastFeedTime = DateTimeOffset.Now; | |
| public void Feed(int nurturingEatGrowth) { | |
| if (nurturingEatGrowth <= 0) { | |
| throw new ArgumentOutOfRangeException(nameof(nurturingEatGrowth)); | |
| } | |
| if (Exp >= NurturingMetadata.RequiredGrowth.Last().Exp) { | |
| return; | |
| } | |
| Exp += nurturingEatGrowth; | |
| if (Exp >= NurturingMetadata.RequiredGrowth.First(x => x.Stage == Stage).Exp) { | |
| Stage++; | |
| } | |
| LastFeedTime = DateTimeOffset.Now; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Maple2.Model/Game/Cube/Nurturing.cs` around lines 51 - 60, The Feed method
currently mutates state even for non-positive input; add an early guard at the
start of Feed(int nurturingEatGrowth) that returns immediately when
nurturingEatGrowth <= 0 so Exp, Stage and LastFeedTime are not changed for zero
or negative values; keep the existing checks against
NurturingMetadata.RequiredGrowth and Stage after this guard so only positive
growth can update Exp, potentially increment Stage, and set LastFeedTime.
Maple2.Model/Game/Cube/Nurturing.cs
Outdated
Comment on lines
56
to
59
| Exp += nurturingEatGrowth; | ||
| if (Exp >= NurturingMetadata.RequiredGrowth.First(x => x.Stage == Stage).Exp) { | ||
| Stage++; | ||
| } |
Contributor
There was a problem hiding this comment.
Advance Stage until it matches the new EXP.
Line 58 only moves one stage. Now that growth is caller-provided, a single feed/play can cross multiple thresholds and leave Stage behind until a later action.
Proposed fix
Exp += nurturingEatGrowth;
- if (Exp >= NurturingMetadata.RequiredGrowth.First(x => x.Stage == Stage).Exp) {
- Stage++;
- }
+ short maxStage = NurturingMetadata.RequiredGrowth.Last().Stage;
+ while (Stage < maxStage &&
+ Exp >= NurturingMetadata.RequiredGrowth.First(x => x.Stage == Stage).Exp) {
+ Stage++;
+ }
LastFeedTime = DateTimeOffset.Now;📝 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.
Suggested change
| Exp += nurturingEatGrowth; | |
| if (Exp >= NurturingMetadata.RequiredGrowth.First(x => x.Stage == Stage).Exp) { | |
| Stage++; | |
| } | |
| Exp += nurturingEatGrowth; | |
| short maxStage = NurturingMetadata.RequiredGrowth.Last().Stage; | |
| while (Stage < maxStage && | |
| Exp >= NurturingMetadata.RequiredGrowth.First(x => x.Stage == Stage).Exp) { | |
| Stage++; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Maple2.Model/Game/Cube/Nurturing.cs` around lines 56 - 59, The code
increments Stage only once after adding nurturingEatGrowth to Exp, which can
leave Stage behind if Exp crosses multiple thresholds; update the logic in
Nurturing.cs (the block that updates Exp, Stage, and reads
NurturingMetadata.RequiredGrowth) to loop: after increasing Exp, repeatedly
compare Exp against the required Exp for the current Stage (using
NurturingMetadata.RequiredGrowth.First(x => x.Stage == Stage).Exp) and increment
Stage until Exp is less than the required value or Stage reaches its maximum,
ensuring you guard against going past the highest defined Stage.
Comment on lines
+414
to
+417
| logger.Warning("Failed to find nearest poly from position {Source} in field {MapId}", point, MapId); | ||
| nearestRef = 0; | ||
| position = default; | ||
| return false; |
Contributor
There was a problem hiding this comment.
Don't warn on every navmesh miss here.
ValidPosition() and other recovery probes funnel through this helper, so a warning on every total miss will create a lot of log noise and avoidable I/O on busy maps. Please lower this to debug or rate-limit it.
💡 Minimal fix
- logger.Warning("Failed to find nearest poly from position {Source} in field {MapId}", point, MapId);
+ logger.Debug("Failed to find nearest poly from position {Source} in field {MapId}", point, MapId);📝 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.
Suggested change
| logger.Warning("Failed to find nearest poly from position {Source} in field {MapId}", point, MapId); | |
| nearestRef = 0; | |
| position = default; | |
| return false; | |
| logger.Debug("Failed to find nearest poly from position {Source} in field {MapId}", point, MapId); | |
| nearestRef = 0; | |
| position = default; | |
| return false; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.cs` around lines
414 - 417, The helper that sets nearestRef = 0 and returns false is logging a
Warning on every navmesh miss (logger.Warning(...)), which floods logs because
callers like ValidPosition() call it frequently; change the log level to
logger.Debug (or implement a simple rate-limit/throttle) so routine misses don't
create noise—update the call site using logger.Warning(...) in this helper to
logger.Debug(...) (or add a lightweight rate limiter around that log emission)
and ensure callers like ValidPosition() still get the false return behavior
unchanged.
Comment on lines
+124
to
+128
| if (session.Currency.Meret < session.ServerTableMetadata.ConstantsTable.SkillBookTreeAddTabFeeMerat) { | ||
| return false; | ||
| } | ||
|
|
||
| session.Currency.Meret -= Constant.SkillBookTreeAddTabFeeMeret; | ||
| session.Currency.Meret -= session.ServerTableMetadata.ConstantsTable.SkillBookTreeAddTabFeeMerat; |
Contributor
There was a problem hiding this comment.
Reject negative configured tab fees before charging.
This fee is now metadata-driven, but Lines 124 and 128 assume it is non-negative. If SkillBookTreeAddTabFeeMerat is ever misconfigured to a negative value, the affordability check passes for everyone and the subtraction credits Meret instead of charging it.
Suggested fix
+ int addTabFee = session.ServerTableMetadata.ConstantsTable.SkillBookTreeAddTabFeeMerat;
+ if (addTabFee < 0) {
+ logger.Error("Invalid SkillBookTreeAddTabFeeMerat: {Fee}", addTabFee);
+ return false;
+ }
- if (session.Currency.Meret < session.ServerTableMetadata.ConstantsTable.SkillBookTreeAddTabFeeMerat) {
+ if (session.Currency.Meret < addTabFee) {
return false;
}
- session.Currency.Meret -= session.ServerTableMetadata.ConstantsTable.SkillBookTreeAddTabFeeMerat;
+ session.Currency.Meret -= addTabFee;📝 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.
Suggested change
| if (session.Currency.Meret < session.ServerTableMetadata.ConstantsTable.SkillBookTreeAddTabFeeMerat) { | |
| return false; | |
| } | |
| session.Currency.Meret -= Constant.SkillBookTreeAddTabFeeMeret; | |
| session.Currency.Meret -= session.ServerTableMetadata.ConstantsTable.SkillBookTreeAddTabFeeMerat; | |
| int addTabFee = session.ServerTableMetadata.ConstantsTable.SkillBookTreeAddTabFeeMerat; | |
| if (addTabFee < 0) { | |
| logger.Error("Invalid SkillBookTreeAddTabFeeMerat: {Fee}", addTabFee); | |
| return false; | |
| } | |
| if (session.Currency.Meret < addTabFee) { | |
| return false; | |
| } | |
| session.Currency.Meret -= addTabFee; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Maple2.Server.Game/Manager/SkillManager.cs` around lines 124 - 128, The code
assumes session.ServerTableMetadata.ConstantsTable.SkillBookTreeAddTabFeeMerat
is non-negative; first read it into a local variable (e.g., fee) and validate
fee >= 0 before the affordability check and subtraction: if fee < 0, treat it as
an invalid config and return false (or log and return false) so you never credit
players; then use that validated fee for the existing check
(session.Currency.Meret < fee) and the subtraction (session.Currency.Meret -=
fee).
Comment on lines
+200
to
+202
| item.Price = price; | ||
| item.PromotionEndTime = promote ? DateTime.Now.AddHours(Constant.UGCShopAdHour).ToEpochSeconds() : 0; | ||
| item.ListingEndTime = DateTime.Now.AddDays(Constant.UGCShopSaleDay).ToEpochSeconds(); | ||
| item.PromotionEndTime = promote ? DateTime.Now.AddHours(session.ServerTableMetadata.ConstantsTable.UGCShopAdHour).ToEpochSeconds() : 0; | ||
| item.ListingEndTime = DateTime.Now.AddDays(session.ServerTableMetadata.ConstantsTable.UGCShopSaleDay).ToEpochSeconds(); |
Contributor
There was a problem hiding this comment.
Clamp relist prices with the same server limits as initial listings.
Line 200 still trusts the client-supplied price, so relisting can bypass the UGCShopSellMinPrice / UGCShopSellMaxPrice enforcement that HandleListItem applies on Line 157.
Suggested fix
- item.Price = price;
+ item.Price = Math.Clamp(
+ price,
+ session.ServerTableMetadata.ConstantsTable.UGCShopSellMinPrice,
+ session.ServerTableMetadata.ConstantsTable.UGCShopSellMaxPrice
+ );
item.PromotionEndTime = promote ? DateTime.Now.AddHours(session.ServerTableMetadata.ConstantsTable.UGCShopAdHour).ToEpochSeconds() : 0;
item.ListingEndTime = DateTime.Now.AddDays(session.ServerTableMetadata.ConstantsTable.UGCShopSaleDay).ToEpochSeconds();📝 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.
Suggested change
| item.Price = price; | |
| item.PromotionEndTime = promote ? DateTime.Now.AddHours(Constant.UGCShopAdHour).ToEpochSeconds() : 0; | |
| item.ListingEndTime = DateTime.Now.AddDays(Constant.UGCShopSaleDay).ToEpochSeconds(); | |
| item.PromotionEndTime = promote ? DateTime.Now.AddHours(session.ServerTableMetadata.ConstantsTable.UGCShopAdHour).ToEpochSeconds() : 0; | |
| item.ListingEndTime = DateTime.Now.AddDays(session.ServerTableMetadata.ConstantsTable.UGCShopSaleDay).ToEpochSeconds(); | |
| item.Price = Math.Clamp( | |
| price, | |
| session.ServerTableMetadata.ConstantsTable.UGCShopSellMinPrice, | |
| session.ServerTableMetadata.ConstantsTable.UGCShopSellMaxPrice | |
| ); | |
| item.PromotionEndTime = promote ? DateTime.Now.AddHours(session.ServerTableMetadata.ConstantsTable.UGCShopAdHour).ToEpochSeconds() : 0; | |
| item.ListingEndTime = DateTime.Now.AddDays(session.ServerTableMetadata.ConstantsTable.UGCShopSaleDay).ToEpochSeconds(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Maple2.Server.Game/PacketHandlers/MeretMarketHandler.cs` around lines 200 -
202, The relist path is assigning client-supplied price directly to item.Price,
allowing bypass of server min/max checks; update the relist logic where
item.Price = price to clamp price between
session.ServerTableMetadata.ConstantsTable.UGCShopSellMinPrice and
UGCShopMetadata.ConstantsTable.UGCShopSellMaxPrice (same enforcement used in
HandleListItem), then assign the clamped value to item.Price so relists use the
same limits as new listings.
Comment on lines
139
to
140
| if (session.Currency.Meret < session.ServerTableMetadata.ConstantsTable.MeratAirTaxiPrice) { | ||
| session.Send(NoticePacket.MessageBox(StringCode.s_err_lack_meso)); |
Contributor
There was a problem hiding this comment.
Return a Meret-specific insufficiency error here.
This branch checks session.Currency.Meret, but Line 140 still sends s_err_lack_meso, so the client will report the wrong currency when Meret Air Taxi payment fails.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Maple2.Server.Game/PacketHandlers/TaxiHandler.cs` around lines 139 - 140, The
code in TaxiHandler.cs checks session.Currency.Meret but sends the meso error
StringCode.s_err_lack_meso; change the notification to the Meret-specific
insufficiency error by replacing the MessageBox argument with the correct Meret
error constant (e.g., StringCode.s_err_lack_meret or your project’s equivalent)
so the client shows the proper currency error when session.Currency.Meret is
insufficient.
Comment on lines
+44
to
+51
| foreach (GameSession session in sessions) { | ||
| if (stoppingToken.IsCancellationRequested) break; | ||
| if (session.Player == null) continue; | ||
|
|
||
| // SessionSave() is internally locked and already checks for null Player. | ||
| session.SessionSave(); | ||
| saved++; | ||
| } |
Contributor
There was a problem hiding this comment.
Isolate per-session failures to avoid blocking subsequent saves.
If SessionSave() throws for one session, the loop breaks and all remaining sessions in that cycle won't be saved. Wrapping each save in its own try-catch improves resilience—one corrupt/problematic session shouldn't prevent others from being persisted.
🛡️ Proposed fix
foreach (GameSession session in sessions) {
if (stoppingToken.IsCancellationRequested) break;
if (session.Player == null) continue;
- // SessionSave() is internally locked and already checks for null Player.
- session.SessionSave();
- saved++;
+ try {
+ session.SessionSave();
+ saved++;
+ } catch (Exception ex) {
+ logger.LogWarning(ex, "[AutoSave] Failed to save session for player {PlayerId}.", session.Player?.CharacterId);
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Maple2.Server.Game/Service/AutoSaveService.cs` around lines 44 - 51, The loop
in AutoSaveService that calls session.SessionSave() should isolate failures per
session: wrap the SessionSave() invocation inside a try-catch around each
iteration (the foreach over GameSession sessions), so a thrown exception does
not exit the loop; log the exception (including identifying info like
session.Player.Id or session.Id if available) using the service logger, and only
increment saved on successful completion of session.SessionSave(); continue to
the next session on error.
jewelpetmurumo-alt
force-pushed
the
fixfox
branch
from
f576483 to
e92f385
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (3)
Maple2.Server.Game/Util/Sync/PlayerInfoStorage.cs (1)
92-108:⚠️ Potential issue | 🟠 MajorMake retry exhaustion observable and avoid blocking the caller thread.
This still sleeps synchronously and then exits after the third failure with no terminal error/exception. If this runs on a packet or field thread, it can stall processing while still leaving the online/channel/map update failure invisible.
🛠️ Suggested direction
- public void SendUpdate(PlayerUpdateRequest request) { + public async Task SendUpdateAsync(PlayerUpdateRequest request, CancellationToken cancellationToken = default) { const int maxRetries = 3; + Exception? lastException = null; for (int i = 0; i < maxRetries; i++) { try { world.UpdatePlayer(request); return; } catch (RpcException ex) { + lastException = ex; logger.Warning("SendUpdate(UpdatePlayer) failed attempt {Attempt}/{Max}. Status={Status}", i + 1, maxRetries, ex.Status); - Thread.Sleep(200 * (i + 1)); + await Task.Delay(200 * (i + 1), cancellationToken); } catch (Exception ex) { + lastException = ex; logger.Warning(ex, "SendUpdate(UpdatePlayer) unexpected failure"); - Thread.Sleep(200 * (i + 1)); + await Task.Delay(200 * (i + 1), cancellationToken); } } + + logger.Error(lastException, + "SendUpdate(UpdatePlayer) failed after {Max} attempts for CharacterId={CharacterId}", + maxRetries, request.CharacterId); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Server.Game/Util/Sync/PlayerInfoStorage.cs` around lines 92 - 108, The retry loop around world.UpdatePlayer currently blocks the caller thread with Thread.Sleep and silently returns on exhaustion; change it to perform non-blocking retries off the calling thread and make final failure observable: replace the synchronous loop (symbols: maxRetries, for loop, world.UpdatePlayer, Thread.Sleep, logger.Warning) with an async retry mechanism that runs on a background task (e.g., Task.Run) or converts the method to async and uses Task.Delay for backoff, and when all retries fail emit a terminal error (logger.Error/throw an exception or signal failure via a callback/event) so callers or monitoring can detect the persistent failure instead of silently proceeding.Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs (1)
123-126:⚠️ Potential issue | 🔴 CriticalUse the same spawn-point key on spawn and on death.
Line 125 clears
deadSpawnPointswithspawnPointNpc.SpawnPointId, but Line 161 still storesFieldNpc.SpawnPointIdasowner?.Value.Id ?? 0. ForSpawnNpc(npc, SpawnPointNPC), that means the clear happens under the real spawn-point ID while the later death/removal path writes back under0, soIsSpawnPointDead(spawnId)stays inconsistent for spawn-point NPCs.Proposed fix
var fieldNpc = new FieldNpc(this, NextLocalId(), agent, new Npc(npc, animation), aiPath, patrolDataUUID: spawnPointNpc?.PatrolData, spawnAnimation: spawnAnimation) { Owner = owner, Position = spawnPosition, Rotation = rotation, Origin = owner?.Position ?? spawnPosition, - SpawnPointId = owner?.Value.Id ?? 0, + SpawnPointId = spawnPointNpc?.SpawnPointId ?? owner?.Value.Id ?? 0, };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs` around lines 123 - 126, The code clears deadSpawnPoints using spawnPointNpc.SpawnPointId but later records deaths using owner?.Value.Id ?? 0, causing spawn-point NPCs to be re-added under key 0; change the death/removal path to use the same spawn-point key (use spawnPointNpc.SpawnPointId or FieldNpc.SpawnPointId) when writing to deadSpawnPoints so the same ID is used on spawn and on death (update the code paths in the methods handling SpawnNpc and the death/removal where deadSpawnPoints is written, replacing owner?.Value.Id ?? 0 with the spawn-point ID variable/reference).Maple2.Server.World/WorldServer.cs (1)
127-151:⚠️ Potential issue | 🟠 MajorThis migration TTL is still inert.
IsMigrating(...)is private, but nothing inWorldServercalls it beforeHeartbeat()sends players throughSetOffline(...). So the handoff window can still mark migrating characters offline, and expired entries only clear if some future in-file caller ever queries this helper.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Server.World/WorldServer.cs` around lines 127 - 151, The migration TTL is never consulted by the code that marks players offline; update WorldServer.Heartbeat to consult the migration helper before calling SetOffline so migrating characters aren't marked offline: inside the loop where players are processed, call IsMigrating(characterId) (or inline the same logic using _migratingUntil.TryGetValue and current time) and skip SetOffline for true, and ensure expired entries are removed when detected (use _migratingUntil.TryRemove). Keep MarkMigrating and ClearMigrating as-is and reference IsMigrating/_migratingUntil in Heartbeat to enforce the TTL.
🧹 Nitpick comments (1)
Maple2.Server.Game/Model/Field/Actor/FieldNpc.cs (1)
200-207: Consider logging when non-finite position recovery occurs.The defensive check is a good safeguard against serialization crashes. However, silently recovering masks upstream bugs that produced the invalid position. Adding a warning log would help identify and fix the root cause.
Additionally, the indentation on these lines appears inconsistent with the surrounding code (tabs vs. spaces).
🛡️ Proposed improvement with logging
- // Defensive: if any upstream system produced a non-finite position, recover instead of - // crashing during packet serialization (Vector3 -> Vector3S conversion can overflow on NaN). - if (!float.IsFinite(Position.X) || !float.IsFinite(Position.Y) || !float.IsFinite(Position.Z)) { - // Recover instead of crashing during packet serialization. - Position = Origin; - Navigation?.ClearPath(); - SendControl = true; - } + // Defensive: if any upstream system produced a non-finite position, recover instead of + // crashing during packet serialization (Vector3 -> Vector3S conversion can overflow on NaN). + if (!float.IsFinite(Position.X) || !float.IsFinite(Position.Y) || !float.IsFinite(Position.Z)) { + Logger.Warning("Non-finite position detected for NPC {NpcId} ({ObjectId}): {Position}. Resetting to origin.", + Value.Metadata.Id, ObjectId, Position); + Position = Origin; + Navigation?.ClearPath(); + SendControl = true; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Server.Game/Model/Field/Actor/FieldNpc.cs` around lines 200 - 207, The recovery branch in FieldNpc (check of Position.X/Y/Z and reset to Origin, Navigation?.ClearPath(), SendControl = true) should emit a warning log when a non-finite position is detected to surface upstream bugs; add a call to your logger (e.g., processLogger.Warn/Error or the class's logger) including the actor id/state and the invalid Position before resetting, then preserve the existing recovery behavior. Also fix the indentation in the block to match the file's spaces style (convert any tabs to spaces) so formatting is consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Maple2.Database/Storage/Game/GameStorage.Guild.cs`:
- Around line 85-100: The current two-step check-then-insert using
Context.GuildApplication.Any(...) and Context.GuildApplication.Add(...) is racy;
make creation atomic by performing a single transactional upsert or guarded
insert and returning the existing row on conflict. Modify the method handling
guild applications so it either (a) wraps the add+save in a transaction/lock
(e.g., a DB transaction or a process-level lock) and queries again only if
SaveChanges() fails, or (b) relies on a unique DB constraint on (GuildId,
ApplicantId) and catches the insert exception from
SaveChanges()/DbUpdateException to fetch and return the existing
Model.GuildApplication (the existing variable) instead of returning a generic
error. Ensure you update the code paths around Context.GuildApplication, the
created Model.GuildApplication instance, and the SaveChanges() handling to
return a consistent GuildApplication DTO when a duplicate already exists.
In `@Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs`:
- Around line 967-969: The scheduled removal callback currently unconditionally
calls deadSpawnPoints.TryAdd when npc.IsDead and npc.SpawnPointId != 0, which
can re-mark a spawn as dead after a respawn has already cleared the marker;
modify the callback to verify the spawn point is still actually dead before
adding the marker—e.g., confirm the NPC is still dead and there is no live NPC
associated with npc.SpawnPointId (or that the respawn-clearing marker is still
absent) before calling deadSpawnPoints.TryAdd; reference the existing symbols
npc.IsDead, npc.SpawnPointId, deadSpawnPoints.TryAdd and the
IsSpawnPointDead(spawnId)/respawn clearing logic to locate where to add the
extra guard.
In `@Maple2.Server.Game/Model/Field/Actor/Actor.cs`:
- Around line 156-163: The dungeon telemetry updates in Actor.cs (the blocks
that update casterPlayer.Session.Dungeon.UserRecord.AccumulationRecords for
DungeonAccumulationRecordType.TotalDamage, TotalHitCount, TotalCriticalDamage
and the targetPlayer IncomingDamage update) risk overflowing when casting long
damage totals to int; change each update to compute a bounded sum using Math.Min
to cap at int.MaxValue before casting and storing: read the current int value
from AccumulationRecords, compute newValue = Math.Min((long)int.MaxValue,
(long)current + <addedAmount>) where <addedAmount> is positiveDamage or the
Count/Sum expression, then store (int)newValue back into AccumulationRecords;
apply this pattern for TotalDamage, TotalHitCount, TotalCriticalDamage and
IncomingDamage to prevent silent overflow.
In `@Maple2.Server.Game/Model/Field/Buff.cs`:
- Around line 214-216: The current block assumes casterPlayer.Session.Dungeon is
non-null and can throw a NullReferenceException; update the conditional to
explicitly check that casterPlayer.Session.Dungeon != null (and keep the
existing UserRecord != null and record.HpAmount > 0 checks) before accessing
Dungeon.UserRecord and incrementing DungeonAccumulationRecordType.TotalHealing,
i.e., ensure the null check for Dungeon is present around the
Caster/FieldPlayer/session logic so the increment only runs when Dungeon and its
UserRecord exist.
In `@Maple2.Server.Game/PacketHandlers/GuildHandler.cs`:
- Around line 728-738: The current acceptance flow calls CreateGuildMember(...)
then DeleteGuildApplication(...), which can leave the DB in an inconsistent
state and leave other applications intact; refactor by adding a new storage
operation AcceptGuildApplication(RequestContext) (or similar) that, inside
GameStorage.Request db = gameStorage.Context() use a single transaction to
insert the GuildMember and delete all GuildApplication rows for
application.Applicant.CharacterId atomically, return success/failure (or the
created GuildMember), then update the handler in GuildHandler (replace the
CreateGuildMember/DeleteGuildApplication calls) to call
AcceptGuildApplication(...), check its result and only then perform the
in-memory/session updates and notifications.
- Around line 646-676: GetGuild(guildId) returns the LoadGuild shape which does
not populate guild.Members, so the loop in the GuildHandler (after
CreateGuildApplication and before sending ReceiveApplication) iterates an empty
members collection and recruiters are not notified; fix by explicitly loading
members before iterating (e.g., call the method that fully loads a guild with
members or call db.LoadGuildMembers/LoadGuild(guildId, includeMembers:true)
right after Guild? guild = db.GetGuild(guildId) and before foreach), then
continue using guild.Members to find sessions and send
GuildPacket.ReceiveApplication(application).
- Around line 740-745: You are directly mutating session.Guild.Guild.Members and
sending GuildPacket.Joined for the acting session (using newMember and
CloneGuildMember) which bypasses GuildManager.AddMember and therefore skips
important side-effects like BeginListen(member) and subscribing the member's
achievement info (Guild.AchievementInfo += member.Info.AchievementInfo). Replace
the direct Members.TryAdd + session.Send(...) path with a call to
GuildManager.AddMember(...) for the acting session so AddMember performs all
side-effects, and ensure the local session still receives the Joined packet (or
relies on AddMember to notify) rather than sending Joined manually.
In `@Maple2.Server.World/Containers/GuildManager.cs`:
- Around line 103-104: The current pre-insert of the applicant via
Guild.Members.TryAdd(member.CharacterId, member) causes duplicates on the
applicant side and prevents post-join initialization (BeginListen, achievement
aggregation, GuildPacket.Joined) from running; either move the world insertion
so Guild.Members.TryAdd is performed after the fan-out/broadcast, or modify the
applicant-side duplicate path in the local add handler to still run
BeginListen(...) and the post-join aggregation/packet logic when a duplicate is
detected; if you keep TryAdd here, check its boolean return and handle the
duplicate case instead of ignoring it.
---
Duplicate comments:
In `@Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs`:
- Around line 123-126: The code clears deadSpawnPoints using
spawnPointNpc.SpawnPointId but later records deaths using owner?.Value.Id ?? 0,
causing spawn-point NPCs to be re-added under key 0; change the death/removal
path to use the same spawn-point key (use spawnPointNpc.SpawnPointId or
FieldNpc.SpawnPointId) when writing to deadSpawnPoints so the same ID is used on
spawn and on death (update the code paths in the methods handling SpawnNpc and
the death/removal where deadSpawnPoints is written, replacing owner?.Value.Id ??
0 with the spawn-point ID variable/reference).
In `@Maple2.Server.Game/Util/Sync/PlayerInfoStorage.cs`:
- Around line 92-108: The retry loop around world.UpdatePlayer currently blocks
the caller thread with Thread.Sleep and silently returns on exhaustion; change
it to perform non-blocking retries off the calling thread and make final failure
observable: replace the synchronous loop (symbols: maxRetries, for loop,
world.UpdatePlayer, Thread.Sleep, logger.Warning) with an async retry mechanism
that runs on a background task (e.g., Task.Run) or converts the method to async
and uses Task.Delay for backoff, and when all retries fail emit a terminal error
(logger.Error/throw an exception or signal failure via a callback/event) so
callers or monitoring can detect the persistent failure instead of silently
proceeding.
In `@Maple2.Server.World/WorldServer.cs`:
- Around line 127-151: The migration TTL is never consulted by the code that
marks players offline; update WorldServer.Heartbeat to consult the migration
helper before calling SetOffline so migrating characters aren't marked offline:
inside the loop where players are processed, call IsMigrating(characterId) (or
inline the same logic using _migratingUntil.TryGetValue and current time) and
skip SetOffline for true, and ensure expired entries are removed when detected
(use _migratingUntil.TryRemove). Keep MarkMigrating and ClearMigrating as-is and
reference IsMigrating/_migratingUntil in Heartbeat to enforce the TTL.
---
Nitpick comments:
In `@Maple2.Server.Game/Model/Field/Actor/FieldNpc.cs`:
- Around line 200-207: The recovery branch in FieldNpc (check of Position.X/Y/Z
and reset to Origin, Navigation?.ClearPath(), SendControl = true) should emit a
warning log when a non-finite position is detected to surface upstream bugs; add
a call to your logger (e.g., processLogger.Warn/Error or the class's logger)
including the actor id/state and the invalid Position before resetting, then
preserve the existing recovery behavior. Also fix the indentation in the block
to match the file's spaces style (convert any tabs to spaces) so formatting is
consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8bea28de-bf34-443e-b79a-d4127df92b9e
📒 Files selected for processing (16)
Maple2.Database/Storage/Game/GameStorage.Guild.csMaple2.Model/Enum/GameEventUserValueType.csMaple2.Model/Metadata/Constants.csMaple2.Server.Core/Network/Session.csMaple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.csMaple2.Server.Game/Manager/Field/FieldManager/FieldManager.csMaple2.Server.Game/Manager/FishingManager.csMaple2.Server.Game/Model/Field/Actor/Actor.csMaple2.Server.Game/Model/Field/Actor/FieldNpc.csMaple2.Server.Game/Model/Field/Buff.csMaple2.Server.Game/PacketHandlers/GuildHandler.csMaple2.Server.Game/Service/ChannelService.Guild.csMaple2.Server.Game/Util/DamageCalculator.csMaple2.Server.Game/Util/Sync/PlayerInfoStorage.csMaple2.Server.World/Containers/GuildManager.csMaple2.Server.World/WorldServer.cs
🚧 Files skipped from review as they are similar to previous changes (1)
- Maple2.Server.Game/Manager/FishingManager.cs
Comment on lines
+85
to
+100
| if (Context.GuildApplication.Any(app => app.GuildId == guildId && app.ApplicantId == applicantId)) { | ||
| Model.GuildApplication existing = Context.GuildApplication.First(app => app.GuildId == guildId && app.ApplicantId == applicantId); | ||
| return new GuildApplication { | ||
| Id = existing.Id, | ||
| Guild = guild, | ||
| Applicant = applicant, | ||
| CreationTime = existing.CreationTime.ToEpochSeconds(), | ||
| }; | ||
| } | ||
|
|
||
| var app = new Model.GuildApplication { | ||
| GuildId = guildId, | ||
| ApplicantId = applicantId, | ||
| }; | ||
| Context.GuildApplication.Add(app); | ||
| if (!SaveChanges()) { |
Contributor
There was a problem hiding this comment.
Make guild-application creation atomic.
The Any(...) check at Line 85 and the later Add(...) at Line 99 are separate round trips. Concurrent apply requests can both observe “no row” here; without a uniqueness guarantee one inserts a duplicate, and with one the loser still falls back to the generic error path instead of returning the existing application. Collapse this into one atomic path so repeated applies are idempotent.
As per coding guidelines, "Use locks for concurrent database operations."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Maple2.Database/Storage/Game/GameStorage.Guild.cs` around lines 85 - 100, The
current two-step check-then-insert using Context.GuildApplication.Any(...) and
Context.GuildApplication.Add(...) is racy; make creation atomic by performing a
single transactional upsert or guarded insert and returning the existing row on
conflict. Modify the method handling guild applications so it either (a) wraps
the add+save in a transaction/lock (e.g., a DB transaction or a process-level
lock) and queries again only if SaveChanges() fails, or (b) relies on a unique
DB constraint on (GuildId, ApplicantId) and catches the insert exception from
SaveChanges()/DbUpdateException to fetch and return the existing
Model.GuildApplication (the existing variable) instead of returning a generic
error. Ensure you update the code paths around Context.GuildApplication, the
created Model.GuildApplication instance, and the SaveChanges() handling to
return a consistent GuildApplication DTO when a duplicate already exists.
Comment on lines
+967
to
+969
| if (npc.IsDead && npc.SpawnPointId != 0) { | ||
| deadSpawnPoints.TryAdd(npc.SpawnPointId, 1); | ||
| } |
Contributor
There was a problem hiding this comment.
Guard against stale dead markers after delayed removal.
These lines run inside the scheduled removal callback. If the spawn point respawns before that callback fires, the respawn path clears the marker first, then this late TryAdd writes the dead flag back while a live NPC already exists. That will make IsSpawnPointDead(spawnId) return true for an active spawn.
Proposed fix
- if (npc.IsDead && npc.SpawnPointId != 0) {
+ if (npc.IsDead && npc.SpawnPointId != 0 &&
+ !Mobs.Values.Any(mob => mob.SpawnPointId == npc.SpawnPointId) &&
+ !Npcs.Values.Any(otherNpc => otherNpc.SpawnPointId == npc.SpawnPointId)) {
deadSpawnPoints.TryAdd(npc.SpawnPointId, 1);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Maple2.Server.Game/Manager/Field/FieldManager/FieldManager.State.cs` around
lines 967 - 969, The scheduled removal callback currently unconditionally calls
deadSpawnPoints.TryAdd when npc.IsDead and npc.SpawnPointId != 0, which can
re-mark a spawn as dead after a respawn has already cleared the marker; modify
the callback to verify the spawn point is still actually dead before adding the
marker—e.g., confirm the NPC is still dead and there is no live NPC associated
with npc.SpawnPointId (or that the respawn-clearing marker is still absent)
before calling deadSpawnPoints.TryAdd; reference the existing symbols
npc.IsDead, npc.SpawnPointId, deadSpawnPoints.TryAdd and the
IsSpawnPointDead(spawnId)/respawn clearing logic to locate where to add the
extra guard.
Comment on lines
+214
to
+216
| if (Caster is FieldPlayer casterPlayer && casterPlayer.Session.Dungeon.UserRecord != null && record.HpAmount > 0) { | ||
| casterPlayer.Session.Dungeon.UserRecord.AccumulationRecords[DungeonAccumulationRecordType.TotalHealing] += record.HpAmount; | ||
| } |
Contributor
There was a problem hiding this comment.
Potential NullReferenceException: Dungeon may be null.
The code checks casterPlayer.Session.Dungeon.UserRecord != null but accesses Dungeon without first verifying it's non-null. Per the context snippet, GameSession.Dungeon is declared as DungeonManager Dungeon { get; set; } = null!;, which can be null at runtime when the player is not in a dungeon.
Proposed fix
- if (Caster is FieldPlayer casterPlayer && casterPlayer.Session.Dungeon.UserRecord != null && record.HpAmount > 0) {
+ if (Caster is FieldPlayer casterPlayer && casterPlayer.Session.Dungeon?.UserRecord != null && record.HpAmount > 0) {
casterPlayer.Session.Dungeon.UserRecord.AccumulationRecords[DungeonAccumulationRecordType.TotalHealing] += record.HpAmount;
}📝 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.
Suggested change
| if (Caster is FieldPlayer casterPlayer && casterPlayer.Session.Dungeon.UserRecord != null && record.HpAmount > 0) { | |
| casterPlayer.Session.Dungeon.UserRecord.AccumulationRecords[DungeonAccumulationRecordType.TotalHealing] += record.HpAmount; | |
| } | |
| if (Caster is FieldPlayer casterPlayer && casterPlayer.Session.Dungeon?.UserRecord != null && record.HpAmount > 0) { | |
| casterPlayer.Session.Dungeon.UserRecord.AccumulationRecords[DungeonAccumulationRecordType.TotalHealing] += record.HpAmount; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Maple2.Server.Game/Model/Field/Buff.cs` around lines 214 - 216, The current
block assumes casterPlayer.Session.Dungeon is non-null and can throw a
NullReferenceException; update the conditional to explicitly check that
casterPlayer.Session.Dungeon != null (and keep the existing UserRecord != null
and record.HpAmount > 0 checks) before accessing Dungeon.UserRecord and
incrementing DungeonAccumulationRecordType.TotalHealing, i.e., ensure the null
check for Dungeon is present around the Caster/FieldPlayer/session logic so the
increment only runs when Dungeon and its UserRecord exist.
Comment on lines
+728
to
+738
| if (accepted) { | ||
| GuildMember? newMember = db.CreateGuildMember(session.Guild.Id, application.Applicant); | ||
| if (newMember == null) { | ||
| session.Send(GuildPacket.Error(GuildError.s_guild_err_fail_addmember)); | ||
| return; | ||
| } | ||
|
|
||
| if (!db.DeleteGuildApplication(applicationId)) { | ||
| session.Send(GuildPacket.Error(GuildError.s_guild_err_unknown)); | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Make acceptance one atomic storage operation and clear the applicant's other pending applications.
CreateGuildMember(...) commits before DeleteGuildApplication(...). If Lines 735-738 fail, the applicant is already persisted as a guild member, but this handler returns an error and skips the in-memory/session updates. It also removes only the accepted row, leaving the applicant's other pending applications behind so another guild can still accept them later. Move this into one storage method that inserts the member and deletes all applications for application.Applicant.CharacterId in the same transaction.
💡 Suggested direction
- if (accepted) {
- GuildMember? newMember = db.CreateGuildMember(session.Guild.Id, application.Applicant);
- if (newMember == null) {
- session.Send(GuildPacket.Error(GuildError.s_guild_err_fail_addmember));
- return;
- }
-
- if (!db.DeleteGuildApplication(applicationId)) {
- session.Send(GuildPacket.Error(GuildError.s_guild_err_unknown));
- return;
- }
+ if (accepted) {
+ GuildMember? newMember = db.AcceptGuildApplication(session.Guild.Id, application);
+ if (newMember == null) {
+ session.Send(GuildPacket.Error(GuildError.s_guild_err_fail_addmember));
+ return;
+ }AcceptGuildApplication(...) should create the member and delete all pending GuildApplication rows for the applicant under one transaction.
As per coding guidelines, "Use storage classes with unit-of-work pattern for all data access: using GameStorage.Request db = gameStorage.Context() { ... }."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Maple2.Server.Game/PacketHandlers/GuildHandler.cs` around lines 728 - 738,
The current acceptance flow calls CreateGuildMember(...) then
DeleteGuildApplication(...), which can leave the DB in an inconsistent state and
leave other applications intact; refactor by adding a new storage operation
AcceptGuildApplication(RequestContext) (or similar) that, inside
GameStorage.Request db = gameStorage.Context() use a single transaction to
insert the GuildMember and delete all GuildApplication rows for
application.Applicant.CharacterId atomically, return success/failure (or the
created GuildMember), then update the handler in GuildHandler (replace the
CreateGuildMember/DeleteGuildApplication calls) to call
AcceptGuildApplication(...), check its result and only then perform the
in-memory/session updates and notifications.
Comment on lines
+740
to
+745
| GuildMember[] currentMembers = session.Guild.Guild.Members.Values.ToArray(); | ||
| session.Guild.Guild.Members.TryAdd(newMember.CharacterId, newMember); | ||
|
|
||
| // Refresh the guild master's member list immediately even if self session lookup fails. | ||
| session.Send(GuildPacket.Joined(session.PlayerName, CloneGuildMember(newMember))); | ||
|
|
Contributor
There was a problem hiding this comment.
Don't bypass GuildManager.AddMember for the acting session.
At Lines 740-744 you mutate session.Guild.Guild.Members directly and send Joined(...) yourself. That skips GuildManager.AddMember(...)'s other side effects (BeginListen(member) and Guild.AchievementInfo += member.Info.AchievementInfo), so the recruiter session's guild state diverges from everyone else's as soon as an application is accepted.
💡 Suggested fix
GuildMember[] currentMembers = session.Guild.Guild.Members.Values.ToArray();
- session.Guild.Guild.Members.TryAdd(newMember.CharacterId, newMember);
-
- // Refresh the guild master's member list immediately even if self session lookup fails.
- session.Send(GuildPacket.Joined(session.PlayerName, CloneGuildMember(newMember)));
+ session.Guild.AddMember(session.PlayerName, CloneGuildMember(newMember));📝 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.
Suggested change
| GuildMember[] currentMembers = session.Guild.Guild.Members.Values.ToArray(); | |
| session.Guild.Guild.Members.TryAdd(newMember.CharacterId, newMember); | |
| // Refresh the guild master's member list immediately even if self session lookup fails. | |
| session.Send(GuildPacket.Joined(session.PlayerName, CloneGuildMember(newMember))); | |
| GuildMember[] currentMembers = session.Guild.Guild.Members.Values.ToArray(); | |
| session.Guild.AddMember(session.PlayerName, CloneGuildMember(newMember)); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Maple2.Server.Game/PacketHandlers/GuildHandler.cs` around lines 740 - 745,
You are directly mutating session.Guild.Guild.Members and sending
GuildPacket.Joined for the acting session (using newMember and CloneGuildMember)
which bypasses GuildManager.AddMember and therefore skips important side-effects
like BeginListen(member) and subscribing the member's achievement info
(Guild.AchievementInfo += member.Info.AchievementInfo). Replace the direct
Members.TryAdd + session.Send(...) path with a call to
GuildManager.AddMember(...) for the acting session so AddMember performs all
side-effects, and ensure the local session still receives the Joined packet (or
relies on AddMember to notify) rather than sending Joined manually.
Comment on lines
+103
to
+104
| // Add the new member before broadcasting so the applicant also receives the guild member sync. | ||
| Guild.Members.TryAdd(member.CharacterId, member); |
Contributor
There was a problem hiding this comment.
Don’t pre-insert the applicant before the add-member fan-out.
Line 104 breaks the applicant join path. In Maple2.Server.Game/Service/ChannelService.Guild.cs:77-109, the applicant first loads World.GuildInfo when they have no local guild yet, then Maple2.Server.Game/Manager/GuildManager.cs:157-177 calls Guild.Members.TryAdd(...). Because the world snapshot now already contains the new member, that local add becomes a duplicate and returns early before BeginListen(...), achievement aggregation, and GuildPacket.Joined(...) run. Please keep the world insert after broadcasting, or change the applicant-side duplicate path so it still performs the post-join initialization. Also, if TryAdd stays here, its return value should not be ignored.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Maple2.Server.World/Containers/GuildManager.cs` around lines 103 - 104, The
current pre-insert of the applicant via Guild.Members.TryAdd(member.CharacterId,
member) causes duplicates on the applicant side and prevents post-join
initialization (BeginListen, achievement aggregation, GuildPacket.Joined) from
running; either move the world insertion so Guild.Members.TryAdd is performed
after the fan-out/broadcast, or modify the applicant-side duplicate path in the
local add handler to still run BeginListen(...) and the post-join
aggregation/packet logic when a duplicate is detected; if you keep TryAdd here,
check its boolean return and handle the duplicate case instead of ignoring it.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
Maple2.Server.Game/PacketHandlers/GuildHandler.cs (3)
646-676:⚠️ Potential issue | 🟠 Major
GetGuild()still won't give this loop any recruiters to notify.
db.GetGuild(guildId)returns theLoadGuild(...)shape, and that path does not populateguild.Members. Theforeachat Lines 661-676 therefore iterates an empty collection, so online recruiters never receiveReceiveApplication(...)from this handler.Suggested fix
using GameStorage.Request db = session.GameStorage.Context(); Guild? guild = db.GetGuild(guildId); if (guild == null) { session.Send(GuildPacket.Error(GuildError.s_guild_err_null_guild)); return; } +IList<GuildMember> guildMembers = db.GetGuildMembers(session.PlayerInfo, guildId); GuildApplication? application = db.CreateGuildApplication(session.PlayerInfo, guildId, session.CharacterId); if (application == null) { session.Send(GuildPacket.Error(GuildError.s_guild_err_unknown)); return; } session.Send(GuildPacket.SendApplication(application.Id, guild.Name)); -foreach (GuildMember member in guild.Members.Values) { +foreach (GuildMember member in guildMembers) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Server.Game/PacketHandlers/GuildHandler.cs` around lines 646 - 676, GetGuild(guildId) returns the LoadGuild shape which leaves guild.Members empty, so the recruiter-notify foreach loop in the handler never runs; change the retrieval to load the full guild with members (use the method that returns populated members — e.g., call LoadGuildWithMembers or replace db.GetGuild(guildId) with the variant that populates Members) before creating the application (CreateGuildApplication) and iterating members, so the loop that checks member.Rank, rank.Permission and sends GuildPacket.ReceiveApplication actually sees online recruiters; ensure the chosen load function returns Guild.Members and adjust null checks accordingly.
728-738:⚠️ Potential issue | 🔴 CriticalMake application acceptance one atomic storage operation.
Line 729 creates the guild member before Line 735 deletes the application, and only the accepted row is removed. If the delete fails, the applicant is already persisted as a member while this handler returns an error, and any other pending applications for that applicant remain eligible for acceptance by other guilds. This needs to move into one transactional storage method.
As per coding guidelines, "Use storage classes with unit-of-work pattern for all data access:
using GameStorage.Request db = gameStorage.Context() { ... }"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Server.Game/PacketHandlers/GuildHandler.cs` around lines 728 - 738, The guild application acceptance must be atomic: instead of calling db.CreateGuildMember(...) then db.DeleteGuildApplication(...), move this logic into a single transactional storage method (e.g., AcceptGuildApplication or CreateGuildMemberAndRemoveApplications) in the storage layer that creates the GuildMember and deletes the application(s) inside one unit-of-work/transaction; update GuildHandler.cs to call that new method (using the required GameStorage.Request db = gameStorage.Context() pattern) and handle a single success/failure result so you never persist a member without removing the application(s).
740-745:⚠️ Potential issue | 🟠 MajorUse
GuildManager.AddMemberfor the acting session too.The direct
Members.TryAdd(...)plus manualJoined(...)packet skipsGuildManager.AddMember(...)'s side effects (BeginListen(member)andGuild.AchievementInfo += member.Info.AchievementInfo). That leaves the acting session's guild state different from the other online members, who do go throughother.Guild.AddMember(...)at Line 754.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Server.Game/PacketHandlers/GuildHandler.cs` around lines 740 - 745, The code currently uses session.Guild.Guild.Members.TryAdd(...) and a manual session.Send(GuildPacket.Joined(...)) which skips the side effects performed by GuildManager.AddMember (e.g., BeginListen(member) and wiring Guild.AchievementInfo += member.Info.AchievementInfo); replace the direct TryAdd/send path with the same call used for other sessions so the acting session goes through GuildManager.AddMember (or session.Guild.AddMember if that delegates to GuildManager) instead of Members.TryAdd, and remove the manual Joined/send so the standard AddMember flow (as used at other.Guild.AddMember) performs the membership/notification and event hookup for the acting session as well.Maple2.Server.Game/Util/Sync/PlayerInfoStorage.cs (1)
91-108:⚠️ Potential issue | 🟠 MajorDon't let
SendUpdatefail silently after the last retry.Line 103 blocks the caller with
Thread.Sleep, and once Line 108 is reached the method just returns. Since this path is used for online/offline, map, HP, and death-state sync, a transient world failure can still leave cross-server player state stale with no terminal signal or escalation. At minimum, emit a final error after the loop; ideally move the backoff off the caller thread.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Server.Game/Util/Sync/PlayerInfoStorage.cs` around lines 91 - 108, SendUpdate currently swallows failures after maxRetries and blocks the caller with Thread.Sleep; update it so transient failures are escalated and backoff doesn't block the caller: change SendUpdate to a non-blocking retry (either make SendUpdateAsync and use await Task.Delay(...) or offload retries to a background retry queue/task) replacing Thread.Sleep with Task.Delay, and on final failure after the loop call logger.Error (include exception/Status) or throw a specific exception so callers get a terminal signal; refer to SendUpdate, world.UpdatePlayer, logger.Warning, Thread.Sleep and maxRetries when locating the code to change.Maple2.Database/Storage/Game/GameStorage.Guild.cs (1)
85-100:⚠️ Potential issue | 🟠 MajorMake guild-application creation atomic.
Lines 85-86 do a check-then-read before the insert at Lines 95-100. Two concurrent apply requests can both observe "no row" and then both try to insert, so this still depends on timing instead of an atomic write path. Please switch to a transaction/unique-constraint-backed insert-and-requery flow.
As per coding guidelines, "Ensure thread safety: Packet handlers are stateless singletons; state lives in Session instances. FieldManager runs on dedicated thread per map. Use locks for concurrent database operations."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Database/Storage/Game/GameStorage.Guild.cs` around lines 85 - 100, The current check-then-insert in GameStorage.Guild (the block using Context.GuildApplication.Any / First before creating a new Model.GuildApplication and calling Context.GuildApplication.Add + SaveChanges) is racy; instead perform an atomic insert-and-requery within a DB transaction or rely on a unique constraint and handle the conflict: attempt to insert the new Model.GuildApplication inside a transaction (or call SaveChanges and catch the unique-constraint/DbUpdateException), and if the insert fails due to duplicate key then requery Context.GuildApplication.First(...) to return the existing row; ensure you reference Context.GuildApplication, SaveChanges, and the creation path that builds GuildApplication so you don’t do a separate Any/First check, and if your environment requires thread-safety also wrap the DB operation with the appropriate lock per the FieldManager/session guidelines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Maple2.Database/Storage/Game/GameStorage.Guild.cs`:
- Around line 156-177: GetGuildApplicationsByApplicant builds
GuildApplication.Guild via LoadGuild which does not populate Members or
aggregate AchievementInfo, causing GuildPacket.ListAppliedGuilds to report
Members.Count == 0; change GetGuildApplicationsByApplicant to return
fully-hydrated guilds the same way SearchGuilds does (populate guild.Members and
aggregate AchievementInfo) or call the shared helper used by SearchGuilds that
returns a complete guild object, ensuring GuildApplication.Guild has its Members
populated before mapping to GuildApplication.
In `@Maple2.Server.Game/PacketHandlers/GuildHandler.cs`:
- Around line 606-613: The code currently mutates
session.Guild.Guild.Ranks[rankId].Name/Permission before calling db.SaveGuild,
leaving in-memory state inconsistent if SaveGuild fails; to fix, prepare and
modify a separate Guild object or clone the target Rank (e.g., copy
session.Guild.Guild or the specific Rank), apply the new Name/Permission to that
copy, call db.SaveGuild(copy), and only update session.Guild.Guild.Ranks[rankId]
with the new values after SaveGuild returns true (alternatively, store the
original Name/Permission and revert them on db.SaveGuild failure); reference the
session.Guild.Guild.Ranks index access and the db.SaveGuild(...) call to locate
the change.
In `@Maple2.Server.Game/Service/ChannelService.Guild.cs`:
- Around line 94-105: The current flow sets session.Guild via
SetGuild(response.Guild) and immediately calls Load() then unconditionally
AddMember(...), which can duplicate members under concurrent initialization;
change this to a single guarded critical section (lock on session.Guild or a
dedicated session-level sync object) that: 1) checks session.Guild.Guild == null
before SetGuild(response.Guild) and Load(), 2) after ensuring the guild is
loaded, call session.Guild.GetMember(add.RequestorName) and only call
session.Guild.AddMember(...) if GetMember(...) returns null; ensure the same
synchronization/lock is used by other guild-init paths (e.g., the logic mirrored
in GuildHandler.GetMember/AddMember flow) so bootstrapping and member-add are
atomic.
---
Duplicate comments:
In `@Maple2.Database/Storage/Game/GameStorage.Guild.cs`:
- Around line 85-100: The current check-then-insert in GameStorage.Guild (the
block using Context.GuildApplication.Any / First before creating a new
Model.GuildApplication and calling Context.GuildApplication.Add + SaveChanges)
is racy; instead perform an atomic insert-and-requery within a DB transaction or
rely on a unique constraint and handle the conflict: attempt to insert the new
Model.GuildApplication inside a transaction (or call SaveChanges and catch the
unique-constraint/DbUpdateException), and if the insert fails due to duplicate
key then requery Context.GuildApplication.First(...) to return the existing row;
ensure you reference Context.GuildApplication, SaveChanges, and the creation
path that builds GuildApplication so you don’t do a separate Any/First check,
and if your environment requires thread-safety also wrap the DB operation with
the appropriate lock per the FieldManager/session guidelines.
In `@Maple2.Server.Game/PacketHandlers/GuildHandler.cs`:
- Around line 646-676: GetGuild(guildId) returns the LoadGuild shape which
leaves guild.Members empty, so the recruiter-notify foreach loop in the handler
never runs; change the retrieval to load the full guild with members (use the
method that returns populated members — e.g., call LoadGuildWithMembers or
replace db.GetGuild(guildId) with the variant that populates Members) before
creating the application (CreateGuildApplication) and iterating members, so the
loop that checks member.Rank, rank.Permission and sends
GuildPacket.ReceiveApplication actually sees online recruiters; ensure the
chosen load function returns Guild.Members and adjust null checks accordingly.
- Around line 728-738: The guild application acceptance must be atomic: instead
of calling db.CreateGuildMember(...) then db.DeleteGuildApplication(...), move
this logic into a single transactional storage method (e.g.,
AcceptGuildApplication or CreateGuildMemberAndRemoveApplications) in the storage
layer that creates the GuildMember and deletes the application(s) inside one
unit-of-work/transaction; update GuildHandler.cs to call that new method (using
the required GameStorage.Request db = gameStorage.Context() pattern) and handle
a single success/failure result so you never persist a member without removing
the application(s).
- Around line 740-745: The code currently uses
session.Guild.Guild.Members.TryAdd(...) and a manual
session.Send(GuildPacket.Joined(...)) which skips the side effects performed by
GuildManager.AddMember (e.g., BeginListen(member) and wiring
Guild.AchievementInfo += member.Info.AchievementInfo); replace the direct
TryAdd/send path with the same call used for other sessions so the acting
session goes through GuildManager.AddMember (or session.Guild.AddMember if that
delegates to GuildManager) instead of Members.TryAdd, and remove the manual
Joined/send so the standard AddMember flow (as used at other.Guild.AddMember)
performs the membership/notification and event hookup for the acting session as
well.
In `@Maple2.Server.Game/Util/Sync/PlayerInfoStorage.cs`:
- Around line 91-108: SendUpdate currently swallows failures after maxRetries
and blocks the caller with Thread.Sleep; update it so transient failures are
escalated and backoff doesn't block the caller: change SendUpdate to a
non-blocking retry (either make SendUpdateAsync and use await Task.Delay(...) or
offload retries to a background retry queue/task) replacing Thread.Sleep with
Task.Delay, and on final failure after the loop call logger.Error (include
exception/Status) or throw a specific exception so callers get a terminal
signal; refer to SendUpdate, world.UpdatePlayer, logger.Warning, Thread.Sleep
and maxRetries when locating the code to change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 333ed145-7e35-4282-b486-1138691c61ae
📒 Files selected for processing (8)
Maple2.Database/Storage/Game/GameStorage.Guild.csMaple2.Server.Game/Model/Field/Actor/Actor.csMaple2.Server.Game/Model/Field/Buff.csMaple2.Server.Game/PacketHandlers/GuildHandler.csMaple2.Server.Game/Service/ChannelService.Guild.csMaple2.Server.Game/Util/DamageCalculator.csMaple2.Server.Game/Util/Sync/PlayerInfoStorage.csMaple2.Server.World/Containers/GuildManager.cs
🚧 Files skipped from review as they are similar to previous changes (2)
- Maple2.Server.World/Containers/GuildManager.cs
- Maple2.Server.Game/Model/Field/Actor/Actor.cs
Comment on lines
+156
to
+177
| public IList<GuildApplication> GetGuildApplicationsByApplicant(IPlayerInfoProvider provider, long applicantId) { | ||
| List<Model.GuildApplication> applications = Context.GuildApplication.Where(app => app.ApplicantId == applicantId) | ||
| .OrderByDescending(app => app.CreationTime) | ||
| .ToList(); | ||
|
|
||
| return applications | ||
| .Select(app => { | ||
| Guild? guild = LoadGuild(app.GuildId, string.Empty); | ||
| PlayerInfo? applicant = provider.GetPlayerInfo(app.ApplicantId); | ||
| if (guild == null || applicant == null) { | ||
| return null; | ||
| } | ||
|
|
||
| return new GuildApplication { | ||
| Id = app.Id, | ||
| Guild = guild, | ||
| Applicant = applicant, | ||
| CreationTime = app.CreationTime.ToEpochSeconds(), | ||
| }; | ||
| }) | ||
| .WhereNotNull() | ||
| .ToList(); |
Contributor
There was a problem hiding this comment.
ListAppliedGuilds will always serialize an empty member count from this data shape.
GetGuildApplicationsByApplicant(...) builds each GuildApplication.Guild via LoadGuild(...), but that helper does not load members. GuildPacket.ListAppliedGuilds(...) later writes application.Guild.Members.Count, so these results will report 0 members even for populated guilds. Populate members and aggregate AchievementInfo here the same way SearchGuilds(...) does, or reuse a shared helper that returns a fully hydrated guild.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Maple2.Database/Storage/Game/GameStorage.Guild.cs` around lines 156 - 177,
GetGuildApplicationsByApplicant builds GuildApplication.Guild via LoadGuild
which does not populate Members or aggregate AchievementInfo, causing
GuildPacket.ListAppliedGuilds to report Members.Count == 0; change
GetGuildApplicationsByApplicant to return fully-hydrated guilds the same way
SearchGuilds does (populate guild.Members and aggregate AchievementInfo) or call
the shared helper used by SearchGuilds that returns a complete guild object,
ensuring GuildApplication.Guild has its Members populated before mapping to
GuildApplication.
Comment on lines
+606
to
+613
| session.Guild.Guild.Ranks[rankId].Name = rank.Name; | ||
| session.Guild.Guild.Ranks[rankId].Permission = rank.Permission; | ||
|
|
||
| using GameStorage.Request db = session.GameStorage.Context(); | ||
| if (!db.SaveGuild(session.Guild.Guild)) { | ||
| session.Send(GuildPacket.Error(GuildError.s_guild_err_unknown)); | ||
| return; | ||
| } |
Contributor
There was a problem hiding this comment.
Persist the rank change before mutating live guild state.
Lines 606-607 update session.Guild.Guild before db.SaveGuild(...) succeeds. If the save fails, this session keeps the new rank name/permissions in memory even though storage rejected them, so later permission checks run against uncommitted state. Save a copy first, or revert these fields on failure.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Maple2.Server.Game/PacketHandlers/GuildHandler.cs` around lines 606 - 613,
The code currently mutates session.Guild.Guild.Ranks[rankId].Name/Permission
before calling db.SaveGuild, leaving in-memory state inconsistent if SaveGuild
fails; to fix, prepare and modify a separate Guild object or clone the target
Rank (e.g., copy session.Guild.Guild or the specific Rank), apply the new
Name/Permission to that copy, call db.SaveGuild(copy), and only update
session.Guild.Guild.Ranks[rankId] with the new values after SaveGuild returns
true (alternatively, store the original Name/Permission and revert them on
db.SaveGuild failure); reference the session.Guild.Guild.Ranks index access and
the db.SaveGuild(...) call to locate the change.
Comment on lines
+94
to
+105
| if (session.CharacterId == add.CharacterId && session.Guild.Guild == null) { | ||
| WorldGuildInfoResponse response = session.World.GuildInfo(new WorldGuildInfoRequest { | ||
| GuildId = guildId, | ||
| }); | ||
|
|
||
| if (response.Guild != null) { | ||
| session.Guild.SetGuild(response.Guild); | ||
| session.Guild.Load(); | ||
| } | ||
| } | ||
|
|
||
| session.Guild.AddMember(add.RequestorName, member); |
Contributor
There was a problem hiding this comment.
Keep guild bootstrap/add under one guarded flow.
SetGuild(response.Guild) already populates the member list, but this path immediately calls Load() and then unconditionally replays AddMember(...). That diverges from GuildHandler.cs:757-772, which first checks GetMember(...) == null and only then loads. Combined with the unsynchronized session.Guild.Guild == null check, concurrent initialization can produce stale or duplicated local guild state for the joining player.
Suggested shape
if (session.CharacterId == add.CharacterId && session.Guild.Guild == null) {
WorldGuildInfoResponse response = session.World.GuildInfo(new WorldGuildInfoRequest {
GuildId = guildId,
});
if (response.Guild != null) {
session.Guild.SetGuild(response.Guild);
- session.Guild.Load();
}
}
-session.Guild.AddMember(add.RequestorName, member);
+if (session.Guild.Guild != null && session.Guild.GetMember(add.CharacterId) == null) {
+ session.Guild.AddMember(add.RequestorName, member);
+}
+session.Guild.Load();This still needs to run inside a synchronized critical section shared with the other guild-init paths. As per coding guidelines, "Ensure thread safety: Packet handlers are stateless singletons; state lives in Session instances. FieldManager runs on dedicated thread per map. Use locks for concurrent database operations."
📝 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.
Suggested change
| if (session.CharacterId == add.CharacterId && session.Guild.Guild == null) { | |
| WorldGuildInfoResponse response = session.World.GuildInfo(new WorldGuildInfoRequest { | |
| GuildId = guildId, | |
| }); | |
| if (response.Guild != null) { | |
| session.Guild.SetGuild(response.Guild); | |
| session.Guild.Load(); | |
| } | |
| } | |
| session.Guild.AddMember(add.RequestorName, member); | |
| if (session.CharacterId == add.CharacterId && session.Guild.Guild == null) { | |
| WorldGuildInfoResponse response = session.World.GuildInfo(new WorldGuildInfoRequest { | |
| GuildId = guildId, | |
| }); | |
| if (response.Guild != null) { | |
| session.Guild.SetGuild(response.Guild); | |
| } | |
| } | |
| if (session.Guild.Guild != null && session.Guild.GetMember(add.CharacterId) == null) { | |
| session.Guild.AddMember(add.RequestorName, member); | |
| } | |
| session.Guild.Load(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Maple2.Server.Game/Service/ChannelService.Guild.cs` around lines 94 - 105,
The current flow sets session.Guild via SetGuild(response.Guild) and immediately
calls Load() then unconditionally AddMember(...), which can duplicate members
under concurrent initialization; change this to a single guarded critical
section (lock on session.Guild or a dedicated session-level sync object) that:
1) checks session.Guild.Guild == null before SetGuild(response.Guild) and
Load(), 2) after ensuring the guild is loaded, call
session.Guild.GetMember(add.RequestorName) and only call
session.Guild.AddMember(...) if GetMember(...) returns null; ensure the same
synchronization/lock is used by other guild-init paths (e.g., the logic mirrored
in GuildHandler.GetMember/AddMember flow) so bootstrapping and member-add are
atomic.
Fix the management of guild member applications, as well as the management functions of guild permissions and positions
jewelpetmurumo-alt
force-pushed
the
fixfox
branch
from
e92f385 to
6bf3d5e
Compare
Fix damage statistics and calculation
Fixes for merging the main branch
jewelpetmurumo-alt
force-pushed
the
fixfox
branch
from
fee47d8 to
b1d810c
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
After sorting through it, I have roughly fixed the following major bugs, while some minor bugs have not been included
We have made certain fixes to address the existing abnormal issues and BUGs on the server. The fixes are as follows
1.Fixed the issue where monsters would walk through walls when using skills
2.Fixed the issue of monsters facing players when using normal attacks and skills. Now, monsters will not launch attacks or use skills with their backs facing players
3.Fixed the issue where monster skills were directed incorrectly and went through walls
4.Fixed the issue where player-range skills did not have damage numbers and did not interact with hit reactions
5.Fixed the issue where monsters using range skills could not correctly identify player positions, resulting in location errors
6.Fixed the issue where summoned NPCs or monsters would instantly die under certain circumstances, making it impossible to complete some dungeons
7.Fixed the issue where players may falsely go offline (unable to use chat, player status is offline, and the team channel displays 65535) when entering or exiting dungeons or houses
8.Fixed the issue where fishing did not earn character experience points
9.Fixed the issue where all item rewards in the group would be distributed at once when reading dungeon rewards (in cases where it was not due to an XML configuration error)
Summary by CodeRabbit
New Features
Bug Fixes
Chores