World Boss Corpse Handling#655
Merged
AngeloTadeucci merged 14 commits intoMS2Community:masterfrom Feb 22, 2026
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds hittable NPC corpse support across metadata, ingest, model records, actor logic, packets, drop distribution, field streaming, and server-side targeting so corpses persist, can be hit, and produce corpse-specific drops. Changes
Sequence Diagram(s)sequenceDiagram
participant Mapper as Data Mapper
participant Meta as NpcMetadata
participant Spawner as FieldNpc (Spawner)
participant DropMgr as ItemDropManager
participant Net as Network / Session
participant Player as Player Client
Mapper->>Meta: Map corpse fields into NpcMetadata.Corpse
Meta->>Spawner: Spawn NPC with Corpse metadata
Spawner->>Spawner: OnDeath -> set IsCorpse if hittable
Spawner->>Net: Broadcast Dead packet (corpse state)
Net->>Player: Player receives corpse NPC
Player->>Spawner: Attack / Skill hits corpse
Spawner->>Spawner: ApplyDamage override -> record firstAttacker
Spawner->>DropMgr: DropCorpseLoot / DropHitLoot (assign ownership)
Spawner->>Net: Broadcast CorpseHit packet
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Maple2.Server.Game/Model/Field/Actor/FieldNpc.cs (1)
326-333:⚠️ Potential issue | 🔴 CriticalCorpse mechanic will not receive damage because dead actors are filtered before
ApplyDamageis called.When an NPC dies,
IsDeadbecomestrueandIsCorpseis set totrueinOnDeath. However, inFieldManager.State.cs, skill targeting usesSkillUtils.Filter()which skips all actors whereIsDead == true(lines 82–84 of SkillUtils.cs). This means dead NPCs are excluded fromrecord.TargetsbeforeApplyDamageis invoked, so theIsCorpsecheck insideApplyDamage(lines 360–366) never executes. Corpse loot will not drop, and attacks on corpses will silently fail.🤖 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 326 - 333, SkillUtils.Filter currently removes all actors with IsDead == true, which prevents corpses from reaching ApplyDamage and running the IsCorpse logic set in FieldNpc.OnDeath; update the filtering so corpses are preserved: change SkillUtils.Filter (or its call from FieldManager.State) to exclude dead actors only when actor.IsCorpse == false (i.e., allow actors where IsCorpse == true to remain in record.Targets), so ApplyDamage/FieldNpc.ApplyDamage and OnDeath/IsCorpse logic can execute and corpses can receive damage/loot.
🧹 Nitpick comments (2)
Maple2.Server.Game/Model/Field/Actor/FieldNpc.cs (1)
396-396:DropCorpseLootshould beprivate.It is only called from
ApplyDamagewithin this class; there is no need for public visibility.♻️ Proposed fix
- public void DropCorpseLoot(FieldPlayer player) { + private void DropCorpseLoot(FieldPlayer player) {🤖 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` at line 396, The method DropCorpseLoot is declared public but only used internally from ApplyDamage inside the FieldNpc class; change its accessibility to private by updating the DropCorpseLoot method declaration to private so it is encapsulated within FieldNpc and ensure no external callers exist (verify uses of DropCorpseLoot in the repo to avoid breaking callers).Maple2.File.Ingest/Mapper/NpcMapper.cs (1)
117-125:NpcMetadataCorpse.HitAbleis alwaystrue— consider removing it.The field
HitAble: trueis hardcoded unconditionally because the record is only ever constructed whendata.corpse.hitAble == 1. As a result, non-nullCorpsealready impliesHitAble == true, making the field redundant. The checkValue.Metadata.Corpse?.HitAble == trueinFieldNpc.OnDeathis semantically identical toValue.Metadata.Corpse != null.♻️ Proposed simplification
Remove
HitAblefromNpcMetadataCorpse(inNpcMetadata.cs):public record NpcMetadataCorpse( float Width, float Height, float Depth, float Added, float OffsetNametag, string CorpseEffect, - bool HitAble, Vector3 Rotation);Update the mapper:
- Corpse: data.corpse.hitAble == 1 ? new NpcMetadataCorpse( + Corpse: data.corpse.hitAble == 1 ? new NpcMetadataCorpse( Width: data.corpse.width, Height: data.corpse.height, Depth: data.corpse.depth, Added: data.corpse.added, OffsetNametag: data.corpse.offsetNametag, CorpseEffect: data.corpse.corpseEffect, - HitAble: true, Rotation: data.corpse.rotation) : null,Update
FieldNpc.OnDeath:- if (Value.Metadata.Corpse?.HitAble == true) { + if (Value.Metadata.Corpse != null) { IsCorpse = true; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.File.Ingest/Mapper/NpcMapper.cs` around lines 117 - 125, Remove the redundant HitAble property from the NpcMetadataCorpse record (NpcMetadata.cs), update the Mapper code in NpcMapper.cs to stop setting HitAble (remove the HitAble: true assignment in the Corpse construction), and update all consumers (e.g. FieldNpc.OnDeath) to test for Value.Metadata.Corpse != null instead of Value.Metadata.Corpse?.HitAble == true; ensure any other references to the HitAble symbol are removed or replaced and rebuild to catch compiler errors so you can fix any remaining call sites.
🤖 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.Server.Game/Model/Field/Actor/FieldNpc.cs`:
- Around line 379-381: DeadGlobalDropBoxIds are being processed inside the
per-dealer drop loop (via Field.ItemDrop.GetGlobalDropItems in DropLoot),
causing global drops to be generated once per DamageDealer and bound to
firstPlayer; fix this by moving the generation of global drops out of the
per-dealer path so they run exactly once when the NPC dies (e.g., handle
DeadGlobalDropBoxIds in the death/aggregated handler that calls DropLoot or add
a guard in DropLoot to only process DeadGlobalDropBoxIds the first time),
ensuring Field.ItemDrop.GetGlobalDropItems is invoked once and the resulting
items are not re-bound to firstPlayer for each dealer.
- Around line 359-367: ApplyDamage currently calls DropCorpseLoot on every hit
when IsCorpse is true, allowing unlimited duplicate drops; add deduplication by
introducing per-corpse tracking (e.g., a bool CorpseLooted for global drops and
a HashSet/ConcurrentSet of player IDs like LootClaimants for per-player claims)
inside FieldNpc, update DropCorpseLoot to check and atomically record claims
(thread-safe) before emitting drops, and ensure ApplyDamage uses DropCorpseLoot
only when the claim check passes (keep SequenceCounter and
Field.Broadcast(NpcControlPacket.CorpseHit(this)) behavior intact).
---
Outside diff comments:
In `@Maple2.Server.Game/Model/Field/Actor/FieldNpc.cs`:
- Around line 326-333: SkillUtils.Filter currently removes all actors with
IsDead == true, which prevents corpses from reaching ApplyDamage and running the
IsCorpse logic set in FieldNpc.OnDeath; update the filtering so corpses are
preserved: change SkillUtils.Filter (or its call from FieldManager.State) to
exclude dead actors only when actor.IsCorpse == false (i.e., allow actors where
IsCorpse == true to remain in record.Targets), so
ApplyDamage/FieldNpc.ApplyDamage and OnDeath/IsCorpse logic can execute and
corpses can receive damage/loot.
---
Duplicate comments:
In `@Maple2.Model/Metadata/NpcMetadata.cs`:
- Around line 109-117: The HitAble field on the NpcMetadataCorpse record is
always true per the mapper note; update the record and the mapping code so
HitAble reflects the actual corpse flag: rename HitAble to IsHitAble (or keep
name) and ensure the code that constructs NpcMetadataCorpse (the corpse mapping
routine in your NPC metadata mapper) reads the correct corpse flag (e.g.,
corpseFlags.HasFlag(CorpseFlag.HitAble) or equivalent) and assigns that boolean
instead of leaving it implicitly true or hardcoded.
---
Nitpick comments:
In `@Maple2.File.Ingest/Mapper/NpcMapper.cs`:
- Around line 117-125: Remove the redundant HitAble property from the
NpcMetadataCorpse record (NpcMetadata.cs), update the Mapper code in
NpcMapper.cs to stop setting HitAble (remove the HitAble: true assignment in the
Corpse construction), and update all consumers (e.g. FieldNpc.OnDeath) to test
for Value.Metadata.Corpse != null instead of Value.Metadata.Corpse?.HitAble ==
true; ensure any other references to the HitAble symbol are removed or replaced
and rebuild to catch compiler errors so you can fix any remaining call sites.
In `@Maple2.Server.Game/Model/Field/Actor/FieldNpc.cs`:
- Line 396: The method DropCorpseLoot is declared public but only used
internally from ApplyDamage inside the FieldNpc class; change its accessibility
to private by updating the DropCorpseLoot method declaration to private so it is
encapsulated within FieldNpc and ensure no external callers exist (verify uses
of DropCorpseLoot in the repo to avoid breaking callers).
- Correct drop box trigger points: GlobalHitDropBoxIds fires on hit while alive (OnDamageReceived), DeadGlobalDropBoxIds fires on corpse hit, GlobalDropBoxIds fires on death only. - Add DropHitLoot method handling GlobalHitDropBoxIds and IndividualHitDropBoxIds per-player, triggered from OnDamageReceived. - Fix DropCorpseLoot to use DeadGlobalDropBoxIds (not GlobalHitDropBoxIds) and instance drops per-player via characterId. - Fix individual drop level check to use player level instead of NPC level, so level-gated items (e.g. Shinjo's Feather at MinLevel 30) drop correctly. - Document drop box semantics and the globalDropItemBox vs globalDropItemSet naming distinction as comments. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
Maple2.Server.Game/PacketHandlers/SkillHandler.cs (1)
187-222: Server-side corpse-targeting fallback — logic is sound, but live mobs also pass through the prism filter.The
attackPrism.Filter(session.Field.Mobs.Values, ...)call at line 190 returns both live mobs and corpses (per the updatedSkillUtils.Filter). The innerif (npc.IsCorpse)guard at line 191 correctly narrows to corpses only, so functionality is correct.One minor point: you're paying the cost of intersecting live mobs against the prism just to discard them. If the mob population is large and this path is hot, consider a dedicated corpse collection or pre-filtering
Where(n => n.IsCorpse)before the prism check.Also, the non-directional fallback (lines 196-216) intentionally widens the search but could target corpses behind the player. If that's acceptable for gameplay, this is fine.
💡 Optional: pre-filter to corpses before prism intersection
- foreach (FieldNpc npc in attackPrism.Filter(session.Field.Mobs.Values, record.Attack.TargetCount)) { - if (npc.IsCorpse) { - record.Targets.TryAdd(npc.ObjectId, npc); - } - } + foreach (FieldNpc npc in attackPrism.Filter( + session.Field.Mobs.Values.Where(n => n.IsCorpse), record.Attack.TargetCount)) { + record.Targets.TryAdd(npc.ObjectId, npc); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Server.Game/PacketHandlers/SkillHandler.cs` around lines 187 - 222, The prism intersection is currently done against all mobs (attackPrism.Filter(session.Field.Mobs.Values,...)) then discards non-corpses, which wastes work; change it to pre-filter the mob collection to corpses before calling attackPrism.Filter (e.g., use session.Field.Mobs.Values.Where(n => n.IsCorpse) or build a small List<FieldNpc> of corpses) while keeping the existing npc.IsCorpse guard as a safety check; refer to attackPrism, attackPrism.Filter(...), session.Field.Mobs, FieldNpc and record.Targets.TryAdd when locating where to update the call.Maple2.Server.Game/Model/Field/Actor/FieldNpc.cs (1)
92-95: Good TODO — aggro-reset clearing offirstAttackerObjectIdandDamageDealers.The comment correctly identifies a missing mechanic. Just noting that until this is implemented, if a mob heals to full after losing aggro, the original attacker retains drop ownership even though they may have left the area.
Would you like me to open an issue to track the aggro-reset loot ownership clearing?
🤖 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 92 - 95, The field mob currently keeps drop ownership indefinitely because firstAttackerObjectId and the DamageDealers collection are never cleared when the mob loses aggro; implement an aggro-reset path in the actor's aggro/targeting or tick logic (e.g., in methods that handle OnTargetLost, ClearAggro, or when DetectNoAttackers runs) to check when there are no players in aggro list, restore HP to full if required, and clear firstAttackerObjectId and DamageDealers so the next attacker becomes owner; reference and update the fields firstAttackerObjectId and DamageDealers and any relevant methods (e.g., HealToFull, ClearAggro, OnAggroTimeout) to perform this reset.
🤖 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.Server.Game/Model/Field/Actor/FieldNpc.cs`:
- Around line 410-423: The current implementation in DropGlobalLoot (and
similarly in DropHitLoot, DropIndividualLoot, DropCorpseLoot) repeatedly uses
.Concat(...).ToList() inside the loop which causes O(n²) allocations; replace
the accumulation pattern with a mutable List<Item> (e.g., declare globalDrops as
new List<Item>()) and call
globalDrops.AddRange(Field.ItemDrop.GetGlobalDropItems(globalDropId,
Value.Metadata.Basic.Level)) inside the loop (or iterate returned items and
Add), then iterate that final list to call Field.DropItem; apply the same change
to the corresponding local collections in DropHitLoot, DropIndividualLoot and
DropCorpseLoot to avoid repeated list copying.
- Around line 148-155: Update currently rebroadcasts NpcControlPacket.Dead every
tick in FieldNpc.Update causing excessive network traffic; instead, remove the
per-tick Field.Broadcast(NpcControlPacket.Dead(this)) call and simply increment
SequenceCounter and return for dead corpses in Update, since OnDeath already
sends the initial Dead packet (see methods Update, OnDeath, SequenceCounter,
Field.Broadcast, NpcControlPacket.Dead); if periodic corpse sync is required,
implement a low-frequency throttle (e.g., timer or tick modulus) rather than
broadcasting every tick.
In `@Maple2.Server.Game/PacketHandlers/SkillHandler.cs`:
- Around line 1-2: Remove the redundant using directive "using System.Linq" from
the top of the file (leave "using System.Numerics"), since ImplicitUsings for
net8.0 already supplies System.Linq; simply delete the explicit "using
System.Linq" line in SkillHandler.cs.
---
Duplicate comments:
In `@Maple2.Server.Game/Model/Field/Actor/FieldNpc.cs`:
- Around line 384-394: When IsCorpse is true the current ApplyDamage
unconditionally calls DropCorpseLoot allowing repeated loot; add a HashSet<long>
(e.g., claimedCorpseLooters) on FieldNpc to track character IDs that have
already claimed corpse loot and check it inside ApplyDamage before calling
DropCorpseLoot (only call DropCorpseLoot when the attacking player's Id is not
in claimedCorpseLooters, then add the Id after awarding loot), and ensure access
is thread-safe (lock or ConcurrentHashSet) and still increment SequenceCounter
and broadcast NpcControlPacket.CorpseHit(this) as before.
---
Nitpick comments:
In `@Maple2.Server.Game/Model/Field/Actor/FieldNpc.cs`:
- Around line 92-95: The field mob currently keeps drop ownership indefinitely
because firstAttackerObjectId and the DamageDealers collection are never cleared
when the mob loses aggro; implement an aggro-reset path in the actor's
aggro/targeting or tick logic (e.g., in methods that handle OnTargetLost,
ClearAggro, or when DetectNoAttackers runs) to check when there are no players
in aggro list, restore HP to full if required, and clear firstAttackerObjectId
and DamageDealers so the next attacker becomes owner; reference and update the
fields firstAttackerObjectId and DamageDealers and any relevant methods (e.g.,
HealToFull, ClearAggro, OnAggroTimeout) to perform this reset.
In `@Maple2.Server.Game/PacketHandlers/SkillHandler.cs`:
- Around line 187-222: The prism intersection is currently done against all mobs
(attackPrism.Filter(session.Field.Mobs.Values,...)) then discards non-corpses,
which wastes work; change it to pre-filter the mob collection to corpses before
calling attackPrism.Filter (e.g., use session.Field.Mobs.Values.Where(n =>
n.IsCorpse) or build a small List<FieldNpc> of corpses) while keeping the
existing npc.IsCorpse guard as a safety check; refer to attackPrism,
attackPrism.Filter(...), session.Field.Mobs, FieldNpc and record.Targets.TryAdd
when locating where to update the call.
Comment on lines
1
to
2
| using System.Linq; | ||
| using System.Numerics; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the Server.Game csproj file and check for ImplicitUsings
find . -name "*.csproj" | grep -i "game" | head -20Repository: MS2Community/Maple2
Length of output: 166
🏁 Script executed:
#!/bin/bash
# Check for ImplicitUsings in csproj files
fd -e csproj | xargs grep -l "ImplicitUsings" 2>/dev/nullRepository: MS2Community/Maple2
Length of output: 430
🏁 Script executed:
#!/bin/bash
# Also check the specific SkillHandler.cs file to see if System.Linq is actually used
fd "SkillHandler.cs" --exec cat {} \;Repository: MS2Community/Maple2
Length of output: 19968
🏁 Script executed:
cat Maple2.Server.Game/Maple2.Server.Game.csprojRepository: MS2Community/Maple2
Length of output: 2649
Remove the redundant using System.Linq directive.
The project has ImplicitUsings enabled in its net8.0 target framework, which automatically includes System.Linq in the global implicit usings. The explicit directive at line 1 is redundant and can be safely removed without affecting functionality.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Maple2.Server.Game/PacketHandlers/SkillHandler.cs` around lines 1 - 2, Remove
the redundant using directive "using System.Linq" from the top of the file
(leave "using System.Numerics"), since ImplicitUsings for net8.0 already
supplies System.Linq; simply delete the explicit "using System.Linq" line in
SkillHandler.cs.
Contributor
There was a problem hiding this comment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Maple2.Server.Game/Model/Field/Actor/FieldNpc.cs`:
- Around line 148-155: The Update method is re-broadcasting
NpcControlPacket.Dead every tick for corpses; remove the
Field.Broadcast(NpcControlPacket.Dead(this)) call from FieldNpc.Update so that
when IsDead && IsCorpse you only increment SequenceCounter and return, relying
on the existing broadcast in OnDeath (which already sends the Dead packet);
update FieldNpc.Update to stop sending Dead packets each tick and keep the
single-time broadcast in OnDeath.
- Around line 384-394: In ApplyDamage (FieldNpc.ApplyDamage) the corpse loot
path calls DropCorpseLoot every hit when IsCorpse is true; add a deduplication
guard (e.g., a private bool flag like HasDroppedCorpseLoot or reuse/set
IsCorpse) and perform an atomic check-and-set before calling DropCorpseLoot so
only the first hit by a player triggers DropCorpseLoot (use
Interlocked.CompareExchange or a lock to be thread-safe), and ensure the flag is
set immediately after scheduling the drop so subsequent ApplyDamage calls skip
DropCorpseLoot while still incrementing SequenceCounter and broadcasting the
CorpseHit packet.
- Around line 413-416: The loop repeatedly recreates the accumulated collection
by calling Concat(...).ToList(), causing O(n²) allocations; change the
accumulator to a concrete List<Item> and call AddRange(...) inside the loop
instead (e.g., replace "ICollection<Item> globalDrops = new List<Item>();
foreach(...){ globalDrops = globalDrops.Concat(...).ToList(); }" with a
List<Item> and globalDrops.AddRange(Field.ItemDrop.GetGlobalDropItems(...)));
apply this fix in the methods DropGlobalLoot, DropHitLoot, DropIndividualLoot
and DropCorpseLoot where the same Concat(...).ToList() pattern is used.
In `@Maple2.Server.Game/PacketHandlers/SkillHandler.cs`:
- Around line 1-2: The file SkillHandler.cs contains an explicit using directive
"using System.Linq" which is redundant because net8.0 ImplicitUsings already
provides System.Linq; remove the "using System.Linq" line from the top of
SkillHandler.cs so the file only includes necessary usings (e.g., keep "using
System.Numerics" and any others) and ensure the project still compiles without
that explicit directive.
- Replace Concat().ToList() loops in drop methods with List.AddRange() to avoid O(n²) allocations. - Remove redundant 'using System.Linq' from SkillHandler.cs (covered by ImplicitUsings in net8.0). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Broadcasting NpcControlPacket.Dead every update tick (~67/s) per corpse was excessive. Throttle to once per second with lastCorpseBroadcastTick. Also fix OnAddPlayer to immediately send the Dead packet for any NPCs already in corpse state when a player joins the field, so new joiners see the corpse correctly without relying on the periodic broadcast. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
Maple2.Server.Game/Manager/Items/ItemDropManager.cs (1)
19-21: Consider renaminglevel→npcLevelinGetGlobalDropItemsfor symmetry.The new comment correctly distinguishes NPC level from player level, but the parameter itself is still named
levelwhile the parallel method usesplayerLevel. Renaming it would make the distinction self-documenting without requiring a reader to check the comment.♻️ Proposed rename
- // level is the NPC's level. It gates both group-level and item-level min/max level checks within - // globalDropItemBox/globalDropItemSet (e.g. meso amounts in group 103 scale with NPC level). - public ICollection<Item> GetGlobalDropItems(int globalDropBoxId, int level = 0) { + // npcLevel gates both group-level and item-level min/max level checks within + // globalDropItemBox/globalDropItemSet (e.g. meso amounts in group 103 scale with NPC level). + public ICollection<Item> GetGlobalDropItems(int globalDropBoxId, int npcLevel = 0) {And update all usages of
levelinside the method body tonpcLevel.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Server.Game/Manager/Items/ItemDropManager.cs` around lines 19 - 21, Rename the parameter named level to npcLevel in the GetGlobalDropItems method signature and update every internal reference within ItemDropManager.GetGlobalDropItems to use npcLevel (this keeps symmetry with the parallel method that uses playerLevel); then update all call sites that pass or reference that parameter to use the new parameter name where applicable (or adjust named-argument usages) and run a build to ensure no remaining references to level remain.Maple2.Server.Game/PacketHandlers/SkillHandler.cs (1)
195-214: Fallback always picks at most one corpse, ignoringTargetCount.The prism-based path (line 189) correctly limits results to
record.Attack.TargetCount, but the fallback nearest-corpse search always picks at most 1 target. For multi-target skills, this means the fallback path is strictly weaker and may cause inconsistent behavior depending on which code path runs.If this is intentional (fallback is a best-effort single-target recovery), a brief comment would help. Otherwise, consider collecting up to
TargetCountnearest corpses:Sketch: multi-target fallback
- FieldNpc? nearestCorpse = null; - float nearestDist = float.MaxValue; + var candidateCorpses = new SortedList<float, FieldNpc>(); foreach (FieldNpc npc in session.Field.Mobs.Values) { if (!npc.IsCorpse) continue; float dist = Vector2.Distance(new Vector2(record.Position.X, record.Position.Y), new Vector2(npc.Position.X, npc.Position.Y)); float heightDelta = MathF.Abs(npc.Position.Z - record.Position.Z); - if (dist <= maxDist && heightDelta <= maxHeight && dist < nearestDist) { - nearestDist = dist; - nearestCorpse = npc; + if (dist <= maxDist && heightDelta <= maxHeight) { + candidateCorpses.TryAdd(dist, npc); } } - if (nearestCorpse != null) { - record.Targets.TryAdd(nearestCorpse.ObjectId, nearestCorpse); + foreach (FieldNpc corpse in candidateCorpses.Values.Take(record.Attack.TargetCount)) { + record.Targets.TryAdd(corpse.ObjectId, corpse); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Server.Game/PacketHandlers/SkillHandler.cs` around lines 195 - 214, The fallback currently adds at most one corpse to record.Targets, ignoring multi-target skills; update the fallback in SkillHandler.cs to collect up to record.Attack.TargetCount nearest corpses from session.Field.Mobs (filter IsCorpse, within maxDist/maxHeight), sort or select by distance, then add the first N unique corpses to record.Targets (respecting existing entries), or if single-target fallback was deliberate add a clarifying comment near the fallback explaining it is intentionally best-effort single-target.Maple2.Server.Game/Model/Field/Actor/FieldNpc.cs (2)
386-400:DropCorpseLootispublic— inconsistent with the other private loot helpers.
DropGlobalLoot,DropHitLoot, andDropIndividualLootare allprivate.DropCorpseLoothas no visible external callers; its sole call site isApplyDamageon the same class.♻️ Proposed fix
- public void DropCorpseLoot(FieldPlayer player) { + private void DropCorpseLoot(FieldPlayer player) {🤖 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 386 - 400, The DropCorpseLoot method is public but used only internally by FieldNpc.ApplyDamage; change its access modifier to private to match other internal helpers (DropGlobalLoot, DropHitLoot, DropIndividualLoot) and prevent unintended external usage—locate the DropCorpseLoot method in the FieldNpc class and update its declaration to private, leaving the ApplyDamage call unchanged.
93-96: Tracked TODO:firstAttackerObjectIdis not cleared on aggro reset.The comment acknowledges that if a mob loses aggro it should reset
firstAttackerObjectId(andDamageDealers), but this is not yet implemented. The mob would incorrectly retain the original tagger's drop ownership across a full heal cycle.Would you like me to open a tracking issue for the aggro-loss reset path?
🤖 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 93 - 96, When a mob loses aggro you must clear its ownership state: in the FieldNpc class add logic on the aggro-reset/heal-to-full path (e.g., in whatever method handles full heal or "lost all aggro" events) to reset firstAttackerObjectId to 0 (or sentinel), clear the DamageDealers collection, and reset any tag/state that marks a first attacker; update any methods that call full heal (HealToFull / ResetAggro / OnAllPlayersLostAggro) to invoke this reset so drop ownership does not persist across the full-heal cycle.
🤖 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.Server.Game/Manager/Items/ItemDropManager.cs`:
- Around line 100-112: The call in HousingManager.cs that invokes
GetIndividualDropItems is passing decorationScore instead of the player's level;
update that call so the second argument is the player's level
(session.Player.Value.Character.Level or player.Value.Character.Level as
available) to match the method signature (playerLevel) and other call sites
(ItemBoxManager.cs, FieldNpc.cs), ensuring GetIndividualDropItems(...,
playerLevel, ...) receives the character level not decorationScore.
In `@Maple2.Server.Game/Model/Field/Actor/FieldNpc.cs`:
- Around line 149-157: On death we broadcast Dead and flip IsCorpse but never
seed lastCorpseBroadcastTick, so Update immediately re-broadcasts; modify the
OnDeath method to set lastCorpseBroadcastTick to the current tick used for the
initial Dead broadcast (e.g. the tickCount parameter or TickCount64) right after
setting IsCorpse = true (and after the
Field.Broadcast(NpcControlPacket.Dead(this)) call) so the 1000 ms throttle in
Update (which checks lastCorpseBroadcastTick) works as intended.
In `@Maple2.Server.Game/PacketHandlers/SkillHandler.cs`:
- Around line 216-220: The OnSkillCastEnd trigger is being skipped for corpse
attacks because the continue after handling record.Targets prevents the
TriggerEvent(..., EventConditionType.OnSkillCastEnd, ...) call from running;
update the block that handles record.Targets (where
session.Player.TargetAttack(record) and record.Targets.Clear() are called) to
either invoke the same TriggerEvent(... EventConditionType.OnSkillCastEnd ...)
immediately before the continue or move the existing TriggerEvent call above the
continue so that OnSkillCastEnd always fires for corpse hits as well.
- Around line 186-193: Prism.Filter is being given all mobs
(session.Field.Mobs.Values) so live mobs consume record.Attack.TargetCount even
though only corpses are used; restrict the input to corpses before calling
Prism.Filter (e.g., pass session.Field.Mobs.Values filtered by FieldNpc.IsCorpse
using LINQ Where) so attackPrism.Filter only counts corpse candidates, then add
matches to record.Targets via record.Targets.TryAdd as before.
---
Nitpick comments:
In `@Maple2.Server.Game/Manager/Items/ItemDropManager.cs`:
- Around line 19-21: Rename the parameter named level to npcLevel in the
GetGlobalDropItems method signature and update every internal reference within
ItemDropManager.GetGlobalDropItems to use npcLevel (this keeps symmetry with the
parallel method that uses playerLevel); then update all call sites that pass or
reference that parameter to use the new parameter name where applicable (or
adjust named-argument usages) and run a build to ensure no remaining references
to level remain.
In `@Maple2.Server.Game/Model/Field/Actor/FieldNpc.cs`:
- Around line 386-400: The DropCorpseLoot method is public but used only
internally by FieldNpc.ApplyDamage; change its access modifier to private to
match other internal helpers (DropGlobalLoot, DropHitLoot, DropIndividualLoot)
and prevent unintended external usage—locate the DropCorpseLoot method in the
FieldNpc class and update its declaration to private, leaving the ApplyDamage
call unchanged.
- Around line 93-96: When a mob loses aggro you must clear its ownership state:
in the FieldNpc class add logic on the aggro-reset/heal-to-full path (e.g., in
whatever method handles full heal or "lost all aggro" events) to reset
firstAttackerObjectId to 0 (or sentinel), clear the DamageDealers collection,
and reset any tag/state that marks a first attacker; update any methods that
call full heal (HealToFull / ResetAggro / OnAllPlayersLostAggro) to invoke this
reset so drop ownership does not persist across the full-heal cycle.
In `@Maple2.Server.Game/PacketHandlers/SkillHandler.cs`:
- Around line 195-214: The fallback currently adds at most one corpse to
record.Targets, ignoring multi-target skills; update the fallback in
SkillHandler.cs to collect up to record.Attack.TargetCount nearest corpses from
session.Field.Mobs (filter IsCorpse, within maxDist/maxHeight), sort or select
by distance, then add the first N unique corpses to record.Targets (respecting
existing entries), or if single-target fallback was deliberate add a clarifying
comment near the fallback explaining it is intentionally best-effort
single-target.
- Seed lastCorpseBroadcastTick in OnDeath so the 1-second throttle in Update takes effect immediately after death (previously started at 0, causing an immediate re-broadcast on the next tick). - Pre-filter Mobs to corpses before passing to Prism.Filter so live mobs don't consume the target count and prevent corpses from being found. - Fire OnSkillCastEnd TriggerEvent for corpse hits (was being skipped by the continue that handles the corpse attack path). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The parameter isn't always a player level — HousingManager passes a decoration score to gate reward groups. Rename to the generic 'level' and update the comment to reflect both use cases. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
Maple2.Server.Game/Model/Field/Actor/FieldNpc.cs (2)
340-342:DropHitLootis invoked on every player hit regardless of whether the NPC has any hit drop boxes.
DropHitLootallocates twoList<Item>instances and iteratesGlobalHitDropBoxIds/IndividualHitDropBoxIdson each call. For the overwhelming majority of mobs those lists are empty, making this pure overhead on a hot path. A cheap early-out prevents the unnecessary allocations:♻️ Optional early-out
if (caster is FieldPlayer hitPlayer) { + NpcMetadataDropInfo dropInfo = Value.Metadata.DropInfo; + if (dropInfo.GlobalHitDropBoxIds.Count > 0 || dropInfo.IndividualHitDropBoxIds.Count > 0) { DropHitLoot(hitPlayer); + } }🤖 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 340 - 342, The call to DropHitLoot in FieldNpc should be avoided when there are no hit drop boxes to prevent unnecessary allocations; before invoking DropHitLoot in the hit handling branch (the code that checks "if (caster is FieldPlayer hitPlayer)"), check whether GlobalHitDropBoxIds or IndividualHitDropBoxIds contain any entries (e.g., both are null or both Count/Length == 0) and only call DropHitLoot(hitPlayer) when at least one collection has items; update the condition around the existing call rather than changing DropHitLoot itself to keep the hot path cheap.
467-467:DropCorpseLootshould beprivate— it is only called fromApplyDamagewithin the same class.Making it private prevents accidental external invocations and tightens encapsulation.
🤖 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` at line 467, The method DropCorpseLoot on class FieldNpc is currently public but only invoked from ApplyDamage within the same class; change its accessibility to private to tighten encapsulation by updating the method signature from public void DropCorpseLoot(...) to private void DropCorpseLoot(...). Ensure no external callers exist (the only caller should be ApplyDamage) and run a build/compile to catch any visibility-related issues.Maple2.Server.Game/PacketHandlers/SkillHandler.cs (1)
193-213: Non-directional fallback may select a corpse outside the intended attack arc.The prism search at line 189 correctly respects attack direction. The fallback on lines 193–213 uses a flat XY-distance check without regard for arc/facing, so for directional skills (Box/Frustum) it can resolve to a corpse that is strictly behind or beside the caster — notably if multiple corpse bosses are present in the same field.
This is fine for the single world-boss scenario, but if multiple hittable corpses can coexist, consider restricting the fallback to corpses inside the attack half-space (dot-product sign check on
record.Direction).💡 Optional directional guard
foreach (FieldNpc npc in session.Field.Mobs.Values) { if (!npc.IsCorpse) continue; + // For directional range types, reject targets behind the caster. + if (record.Attack.Range.Type is SkillRegion.Box or SkillRegion.Frustum) { + var toNpc = new Vector2(npc.Position.X - record.Position.X, npc.Position.Y - record.Position.Y); + var dir2d = new Vector2(record.Direction.X, record.Direction.Y); + if (Vector2.Dot(toNpc, dir2d) < 0) continue; + } float dist = Vector2.Distance(...);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Server.Game/PacketHandlers/SkillHandler.cs` around lines 193 - 213, The fallback nearest-corpse search in the block using record.Targets, record.Attack.Range, record.Position and session.Field.Mobs ignores attack facing and can pick corpses outside the attack arc for directional skills; update the loop that computes nearestCorpse (in the same block that checks npc.IsCorpse and computes dist/heightDelta) to also check the facing half-space by computing the 2D dot product between record.Direction (or its normalized Vector2) and (npc.Position - record.Position) and only consider candidates with a non-negative (or >0) dot product for directional attack types (Box/Frustum) before assigning nearestCorpse, leaving non-directional skills unchanged.
🤖 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.Server.Game/Manager/Items/ItemDropManager.cs`:
- Around line 19-20: The header comment in ItemDropManager.cs states that the
variable named level refers to the NPC's level, but the existing inline comment
inside the drop-check logic (in the method handling
globalDropItemBox/globalDropItemSet within class ItemDropManager) incorrectly
says "Check if player meets level requirements."; update that inline comment to
reflect that MinLevel/MaxLevel checks are against the NPC's level (e.g., change
to "Check if NPC meets level requirements (level is NPC level)" or equivalent)
so both comments consistently state that level is the NPC's level.
---
Duplicate comments:
In `@Maple2.Server.Game/Manager/Items/ItemDropManager.cs`:
- Around line 100-102: The comment reveals a semantic bug: ItemDropManager's
filtering uses IndividualDropItemTable.MinLevel as a player level requirement,
but HousingManager calls the drop routine passing decorationScore (so items get
incorrectly filtered). Fix by changing the HousingManager call to pass an
appropriate player level (or the character's level) when invoking
ItemDropManager methods that expect MinLevel, or add an explicit
overload/parameter to ItemDropManager (e.g., a boolean flag or a separate
method) to indicate the caller is using decoration score vs player level and
implement separate gating logic; update the callsite in HousingManager and the
method signatures in ItemDropManager (and any helper like the drop-evaluation
function that references MinLevel) to make the intent explicit and correct.
---
Nitpick comments:
In `@Maple2.Server.Game/Model/Field/Actor/FieldNpc.cs`:
- Around line 340-342: The call to DropHitLoot in FieldNpc should be avoided
when there are no hit drop boxes to prevent unnecessary allocations; before
invoking DropHitLoot in the hit handling branch (the code that checks "if
(caster is FieldPlayer hitPlayer)"), check whether GlobalHitDropBoxIds or
IndividualHitDropBoxIds contain any entries (e.g., both are null or both
Count/Length == 0) and only call DropHitLoot(hitPlayer) when at least one
collection has items; update the condition around the existing call rather than
changing DropHitLoot itself to keep the hot path cheap.
- Line 467: The method DropCorpseLoot on class FieldNpc is currently public but
only invoked from ApplyDamage within the same class; change its accessibility to
private to tighten encapsulation by updating the method signature from public
void DropCorpseLoot(...) to private void DropCorpseLoot(...). Ensure no external
callers exist (the only caller should be ApplyDamage) and run a build/compile to
catch any visibility-related issues.
In `@Maple2.Server.Game/PacketHandlers/SkillHandler.cs`:
- Around line 193-213: The fallback nearest-corpse search in the block using
record.Targets, record.Attack.Range, record.Position and session.Field.Mobs
ignores attack facing and can pick corpses outside the attack arc for
directional skills; update the loop that computes nearestCorpse (in the same
block that checks npc.IsCorpse and computes dist/heightDelta) to also check the
facing half-space by computing the 2D dot product between record.Direction (or
its normalized Vector2) and (npc.Position - record.Position) and only consider
candidates with a non-negative (or >0) dot product for directional attack types
(Box/Frustum) before assigning nearestCorpse, leaving non-directional skills
unchanged.
'level' is the NPC's level, not the player's. Update inline comment to match the method header comment. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Extract WriteNpcEntry as a shared parameterized helper covering all varying fields (flags, velocity, animSpeed, bossTargetId, state, seqId). Dead and CorpseHit become single expression-bodied calls through SingleNpcPacket. NpcBuffer is now a thin wrapper that reads live NPC values and delegates to WriteNpcEntry, keeping the SequenceId mutation isolated to the live-NPC path. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
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.
Summary by CodeRabbit
New Features
Bug Fixes
Chores