Fix NPC skill hitbox detection and add debug tooling#646
Merged
AngeloTadeucci merged 10 commits intomasterfrom Feb 18, 2026
Merged
Fix NPC skill hitbox detection and add debug tooling#646AngeloTadeucci merged 10 commits intomasterfrom
AngeloTadeucci merged 10 commits intomasterfrom
Conversation
Polygon.Intersects() was only testing the polygon's own edge normals when the other shape was also a Polygon. When intersecting with a Circle, those axes were never tested, causing all Circle vs Polygon checks to incorrectly return true regardless of position.
- GetPrism: use Trapezoid instead of Rectangle for Box type so the hitbox projects forward from the caster instead of being centered. Apply RotateZDegree, RangeOffset, and +180 to match the NPC's facing convention - SkillState: always resolve targets using the attack range prism rather than falling back to the client-provided target list - Actor.TargetAttack: fall back to caster position when ImpactPosition is unset (NPC casts never set it from a client packet)
Allows spawning NPCs without AI so they stand still, useful for testing skill hitboxes without the NPC moving or attacking.
Allows forcing a spawned NPC to cast a specific skill at a chosen attack point, making it easier to test individual hitboxes in isolation.
Renders the current attack point's prism wireframe in orange for all actors with active skills. Toggleable via the Visualization Controls window. NPC skills are sourced from MovementState.CastTask; player skills from ActiveSkills when in a Skill animation. Supporting changes: - Pass skill metadata to TryPlaySequence so AnimationRecord tracks which skill is active for NPCs - Add SkillQueue.GetMostRecent() for player skill lookup
The NoRevivalHere check was also gating on RevivalReturnId == 0, preventing revival in maps that have no return point configured but should still allow in-place revive.
SkillMetadataRange.Width was incorrectly set to region.height, causing every skill's hitbox width to use the height value instead. Requires re-ingesting skill data.
Scale circle projection by axis length to fix SAT calculations: Circle.AxisProjection now multiplies Radius by axis.Length(). Add comprehensive collision unit tests: PolygonIntersectTests (polygon vs polygon/circle SAT cases) and PrismTests (3D prism height+polygon overlap). Adjust existing CircleTests and HoleCircleTest coordinates to match corrected geometry expectations. Minor code/cleanup: simplify Prism type reference in SkillState and update how the attack prism is passed to GetTargets; clarify Box behavior comment in SkillUtils.
Contributor
|
Warning Rate limit exceeded
⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughThis PR extends skill mechanics with debug visualization, adds commands for mob attacks and AI-controlled NPC spawning, refines skill target resolution to consistently use spatial queries, improves skill metadata geometry calculations, and expands collision detection testing for prism-based intersections. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
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 |
Zintixx
approved these changes
Feb 18, 2026
![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/ActorStateComponent/SkillState.cs (1)
70-86:⚠️ Potential issue | 🟠 MajorFix redundant target limit handling and ensure minimum query limit.
When
attack.TargetCountis 0 or negative,GetTargetsat line 73 passes this value aslimittoFilterin SkillUtils.cs. TheFiltermethod immediately yields break whenlimit <= 0, returning no targets. However, lines 78–79 contain fallback logic that attempts to handleTargetCount == 0by using 1 instead — this code is unreachable whenTargetCountis 0 becauseresolvedTargetsis empty.Calculate the minimum target limit before querying:
Proposed fix
- Tools.Collision.Prism attackPrism = attack.Range.GetPrism(actor.Position, actor.Rotation.Z); + int targetLimit = attack.TargetCount > 0 ? attack.TargetCount : 1; + Tools.Collision.Prism attackPrism = attack.Range.GetPrism(actor.Position, actor.Rotation.Z); var resolvedTargets = new List<IActor>(); - foreach (IActor target in actor.Field.GetTargets(actor, [attackPrism], attack.Range.ApplyTarget, attack.TargetCount)) { + foreach (IActor target in actor.Field.GetTargets(actor, [attackPrism], attack.Range.ApplyTarget, targetLimit)) { resolvedTargets.Add(target); } if (resolvedTargets.Count > 0) { - int limit = attack.TargetCount > 0 ? attack.TargetCount : 1; - for (int i = 0; i < resolvedTargets.Count && i < limit; i++) { + for (int i = 0; i < resolvedTargets.Count && i < targetLimit; i++) {🤖 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/SkillState.cs` around lines 70 - 86, The query is using attack.TargetCount directly which, when <=0, causes SkillUtils.Filter to return no targets; compute a safe minimum limit before calling actor.Field.GetTargets and pass that limit instead of attack.TargetCount (e.g., int queryLimit = attack.TargetCount > 0 ? attack.TargetCount : 1) so GetTargets returns at least one candidate; then keep the existing loop that enforces the final send limit (int limit = attack.TargetCount > 0 ? attack.TargetCount : 1) and call actor.TargetAttack(cast) as before, referencing attack.TargetCount, attack.Range.GetPrism, actor.Field.GetTargets, resolvedTargets, cast.Targets.TryAdd, and actor.TargetAttack to locate the affected code.
🧹 Nitpick comments (3)
Maple2.Server.Game/Commands/MobAttackCommand.cs (1)
56-65: Consider validating that the mob actually owns the requested skill.Line 58 validates that the skill exists in field metadata, but doesn't check whether the mob's
Skill.Entriesinclude thisskillId. A user could pass a valid skill that the mob can't actually cast, which may produce confusing behavior. For a debug command this is arguably fine (and even useful for testing arbitrary skills), but a brief note in the output would help:Optional: warn if skill isn't in mob's skill list
// Validate skill exists in metadata if (!session.Field.SkillMetadata.TryGet(skillId.Value, skillLevel, out SkillMetadata? _)) { ctx.Console.Error.WriteLine($"Skill {skillId.Value} level {skillLevel} not found in metadata."); return; } + if (!entries.Any(e => e.Id == skillId.Value)) { + ctx.Console.Out.WriteLine($"Warning: Skill {skillId.Value} is not in this mob's skill list. Casting anyway."); + } + // Cast the skill facing the player🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Server.Game/Commands/MobAttackCommand.cs` around lines 56 - 65, Before casting, check whether the target mob actually has the requested skill in its own skill list (e.g., inspect mob.Skill.Entries or equivalent) after the existing session.Field.SkillMetadata.TryGet validation; if the mob does not own the skill, write a clear warning to ctx.Console.Out or ctx.Console.Error (including mob.Value.Metadata.Name and the skillId/skillLevel) and then either return or proceed based on intended debug behavior (for a debug command, emitting the warning and continuing to call mob.CastAiSkill(skillId.Value, skillLevel, faceTarget: 1, facePos: session.Player.Position) is acceptable).Maple2.Server.DebugGame/Graphics/DebugFieldRenderer.cs (2)
1292-1319: Trapezoid visualization uses an averaged-width box approximation.This renders a uniform-width wireframe cube instead of a true trapezoid shape (which has different start/end widths). For debug visualization purposes this is reasonable, but worth noting the visual won't precisely match the actual collision geometry. A brief inline comment would help future maintainers understand this is intentional.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Server.DebugGame/Graphics/DebugFieldRenderer.cs` around lines 1292 - 1319, The Trapezoid branch is using an averaged width (avgWidth = (range.Width + range.EndWidth) * 0.5f) to render a uniform wireframe cube, which does not reflect the true trapezoidal collision shape; update the Trapezoid handling in DebugFieldRenderer (the block that computes center2D/center, avgWidth, distance, height, rotation and sets instanceBuffer.Transformation, then calls UpdateWireframeInstance and WireCube.Draw) to include a brief inline comment stating that this is an intentional visual approximation for debug purposes (uniform-width box using avgWidth) so future maintainers know the loss of start/end width fidelity is expected and not a bug.
62-62: Consider defaultingShowSkillHitboxestofalse.All other "advanced" visualization flags like
ShowMeshColliders,ShowSpawnPoints, andShowVibrateObjectsdefault tofalse, while only foundational ones likeShowBoxColliders,ShowPortals, andShowActorsdefault totrue. Skill hitboxes are transient and potentially noisy — defaulting totruemay clutter the view for users who aren't actively debugging skills.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Maple2.Server.DebugGame/Graphics/DebugFieldRenderer.cs` at line 62, The field ShowSkillHitboxes is defaulting to true, which is inconsistent with other advanced visualization flags; change its initializer to false so ShowSkillHitboxes = false; and update the inline comment if needed to reflect it’s off by default; ensure there are no other places that assume it starts true (search for usages of ShowSkillHitboxes in DebugFieldRenderer and related rendering code) and run tests/verify visually that skill hitboxes remain off unless explicitly enabled.
🤖 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.DebugGame/Graphics/DebugFieldRenderer.cs`:
- Around line 1320-1332: The fallback uses a hardcoded center Vector3(0,0,...)
so the box always renders at world origin; update the fallback in
DebugFieldRenderer (the else branch that sets variable center and
instanceBuffer.Transformation) to compute the X/Y from the polygon or actor
instead of 0: derive center.X/center.Y from the polygon's bounding box or vertex
average (use IPolygon vertices/bounds if available) or accept and use the actor
position passed in from RenderActorSkillHitbox, then set
instanceBuffer.Transformation the same way and call
UpdateWireframeInstance(window) and WireCube.Draw() so the fallback box appears
at the correct location.
In `@Maple2.Server.Tests/Tools/Collision/PolygonIntersectTests.cs`:
- Around line 13-15: The class comment in PolygonIntersectTests.cs is stale
because Circle.AxisProjection now multiplies Radius by axis.Length(), making
edge-touching projections scale-consistent; update or remove the note that warns
about "barely-touching-edge cases" being unreliable. Locate the comment block
above the test class (mentions Circle.AxisProjection and Radius) and either
reword it to reflect that Circle projections are now scaled by axis.Length() and
comparable to polygon projections, or delete the caveat entirely so tests no
longer contain misleading guidance.
---
Outside diff comments:
In `@Maple2.Server.Game/Model/Field/Actor/ActorStateComponent/SkillState.cs`:
- Around line 70-86: The query is using attack.TargetCount directly which, when
<=0, causes SkillUtils.Filter to return no targets; compute a safe minimum limit
before calling actor.Field.GetTargets and pass that limit instead of
attack.TargetCount (e.g., int queryLimit = attack.TargetCount > 0 ?
attack.TargetCount : 1) so GetTargets returns at least one candidate; then keep
the existing loop that enforces the final send limit (int limit =
attack.TargetCount > 0 ? attack.TargetCount : 1) and call
actor.TargetAttack(cast) as before, referencing attack.TargetCount,
attack.Range.GetPrism, actor.Field.GetTargets, resolvedTargets,
cast.Targets.TryAdd, and actor.TargetAttack to locate the affected code.
---
Nitpick comments:
In `@Maple2.Server.DebugGame/Graphics/DebugFieldRenderer.cs`:
- Around line 1292-1319: The Trapezoid branch is using an averaged width
(avgWidth = (range.Width + range.EndWidth) * 0.5f) to render a uniform wireframe
cube, which does not reflect the true trapezoidal collision shape; update the
Trapezoid handling in DebugFieldRenderer (the block that computes
center2D/center, avgWidth, distance, height, rotation and sets
instanceBuffer.Transformation, then calls UpdateWireframeInstance and
WireCube.Draw) to include a brief inline comment stating that this is an
intentional visual approximation for debug purposes (uniform-width box using
avgWidth) so future maintainers know the loss of start/end width fidelity is
expected and not a bug.
- Line 62: The field ShowSkillHitboxes is defaulting to true, which is
inconsistent with other advanced visualization flags; change its initializer to
false so ShowSkillHitboxes = false; and update the inline comment if needed to
reflect it’s off by default; ensure there are no other places that assume it
starts true (search for usages of ShowSkillHitboxes in DebugFieldRenderer and
related rendering code) and run tests/verify visually that skill hitboxes remain
off unless explicitly enabled.
In `@Maple2.Server.Game/Commands/MobAttackCommand.cs`:
- Around line 56-65: Before casting, check whether the target mob actually has
the requested skill in its own skill list (e.g., inspect mob.Skill.Entries or
equivalent) after the existing session.Field.SkillMetadata.TryGet validation; if
the mob does not own the skill, write a clear warning to ctx.Console.Out or
ctx.Console.Error (including mob.Value.Metadata.Name and the skillId/skillLevel)
and then either return or proceed based on intended debug behavior (for a debug
command, emitting the warning and continuing to call
mob.CastAiSkill(skillId.Value, skillLevel, faceTarget: 1, facePos:
session.Player.Position) is acceptable).
Change debug renderer defaults and visuals: disable skill hitbox rendering by default, add comments about visual approximation for trapezoids, and compute 2D center from polygon vertices for fallback prism rendering. Add a warning in the MobAttackCommand when a mob casts a skill not present in its skill list. Fix SkillState target resolution by ensuring the field query uses a minimum query limit (uses attack.TargetCount if >0, otherwise 1) when resolving targets. Also update a test comment to clarify how circle axis projections are scaled.
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
heightinstead ofwidthfor skill range Width (all skill widths in DB were wrong)mob-attackcommand and--no-aiflag for NPC spawn to test skill hitboxesSummary by CodeRabbit
New Features
--no-aioption and randomized spawn positionsBug Fixes
Tests