Skills with a CELL pattern hit their pattern, not a single target - #2289
Open
denislauri1999 wants to merge 3 commits into
Open
Skills with a CELL pattern hit their pattern, not a single target#2289denislauri1999 wants to merge 3 commits into
denislauri1999 wants to merge 3 commits into
Conversation
…arget Sixty-eight skills in Skill.dat carry a CELL section: an explicit list of the cells they hit, relative to the caster and drawn facing north. Sixty-seven of them are declared as area (HitType 3) with an area RADIUS OF ZERO, because the area is the pattern and not the radius. TargetResolver only read the radius, so all sixty-seven hit a single target. No exception, no log - just less damage than the file says, on skills like the archer's 244 "Piercing Shot" (a row of eight cells, an arrow that pierces) and 367 "Fire Breath" (a rectangle of thirty). The pattern is authored facing north and cast towards the target, so it is rotated by the angle between the two rather than snapped to eight directions: on the four cardinals the result is exact (sine and cosine are 0 or +-1), on the diagonals it is the nearest approximation. An approximation worth stating, since it is visible in play: rotating by an arbitrary angle and rounding can turn a row one cell wide into a staircase with the odd gap at its side, and somebody half a cell off the line can slip through. On the cardinals - the overwhelming majority of casts, since the caster lines up with the target - it does not arise. The table is generated from the client's Skill.dat and checked in rather than put in a column: it is static client data, the same for every server, and it reads straight out of the file that ships with the game. Eight tests, from the file rather than from another implementation: the two example patterns, the four cardinals, a diagonal, and the two cases that fail in silence - a skill with no pattern, and caster and target on the same cell (where normalising divides by zero, every cell becomes NaN and then zero, and the whole pattern stacks on the caster without a word). Zero warnings, all tests pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Included review availability: Your plan provides up to 2 included reviews per hour; 1 remains after this review. WalkthroughThe change adds generated skill cell patterns, resolves relative coordinates toward a target, and applies exact pattern membership during area target selection. Skills without patterns retain rectangular range checks. Tests cover geometry, direction, boundaries, and missing patterns. ChangesSkill cell targeting
Estimated code review effort: 3 (Moderate) | ~25 minutes Merge Risk: ⚪ Minimal · up to The PR enables skills with authored cell patterns to affect their intended areas rather than a single target, with focused tests covering the documented cases; no actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant TargetResolver
participant SkillCells
participant SkillCellTable
participant BattlefieldTargets
TargetResolver->>SkillCells: resolve skill pattern toward primary target
SkillCells->>SkillCellTable: look up skill ID
SkillCellTable-->>SkillCells: return relative coordinate pairs
SkillCells-->>TargetResolver: return absolute battlefield cells
TargetResolver->>BattlefieldTargets: test target positions with IsHit
BattlefieldTargets-->>TargetResolver: include matching monsters and players
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
62 comment lines down to 35, and the long ones down to a line or two. What is left is what a reader would otherwise get wrong: the radius being zero because the area is the pattern, the authored basis, and the zero-distance guard. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The CELL section holds two header fields plus thirty triplets (dx, dy, continues) and stops -
there is no room for a thirty-first. Patterns that need more continue in the unused tail of
COST, whose thirty-three fields are three real ones (CP, price, and a third nothing reads)
followed by ten more triplets in the same format.
Ten skills use it, and the table clipped every one of them at thirty. Fireblast lost the last
six squares of one column of its 3x12 rectangle; Armour Piercing Round lost the tip of its
beam. Nothing raised and nothing logged - they simply covered less ground than the client
draws.
How the continuation was established, since a wrong reading here is silent:
- the tail is non-zero only on the skills whose CELL is full;
- it holds triplets in the same format and orientation;
- it never repeats a cell CELL already gave, across all ten;
- Fireblast and Dragon's Breath close into exact full rectangles - 3x12 and 3x13 - and the
cells the tail supplies are precisely the corner that was missing.
The eleventh skill that fills CELL, Reaper's Scythe, has an empty tail and a closed thirty-cell
figure: a "continues" flag on the last triplet CELL can hold does not mean there is more, it
means you cannot tell from there. So the tail is read rather than assumed, and there is a test
pinning Reaper's Scythe at thirty - it is the skill that assuming would give cells it has not
got.
Fireblast 36, Ice Chain 39, Dragon's Breath 39, Armour Piercing Round 40, Triple Bolt 37,
Triple Arrow and Lucky Wideshot 32, and three more at 34 to 39. Sixty-seven cells in all.
The test that asserted thirty for Fireblast asserted the old reading, so it now asserts the
rectangle instead, and two guards join it: no pattern lists a cell twice, and none exceeds what
the two sections can hold.
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
1 participant
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.
Sixty-eight skills in
Skill.datcarry aCELLsection: an explicit list of the cells they hit, relative to the caster and drawn facing north. Sixty-seven of them are declared as area (HitType 3) with an area radius of ZERO — because the area is the pattern, not the radius.TargetResolveronly read the radius, so all sixty-seven hit a single target. No exception, no log, just less damage than the file says:The rotation
The pattern is authored facing north and cast towards the target, so it is rotated by the angle between the two rather than snapped to eight directions. On the four cardinals the result is exact (sine and cosine are 0 or ±1); on the diagonals it is the nearest approximation.
An approximation worth stating, since it is visible in play: rotating by an arbitrary angle and rounding can turn a row one cell wide into a staircase with the odd gap at its side, and somebody half a cell off the line can slip through. On the cardinals — the overwhelming majority of casts, since the caster lines up with the target — it does not arise.
The table
Generated from the client’s
Skill.datand checked in rather than put in a column: it is static client data, identical for every server, and it reads straight out of the file that ships with the game. 68 skills, 1002 cells.Tests
Eight, with the expected numbers taken from the file rather than from another implementation: the two example patterns, the four cardinals, a diagonal, and the two cases that fail in silence — a skill with no pattern, and caster and target on the same cell (where normalising divides by zero, every cell becomes
NaNand then zero, and the whole pattern stacks on the caster without a word).Zero warnings, all tests pass. This is the first slice of the BCard/combat work from the roadmap’s W 1.0.7 — the rest is larger and will come separately so each piece stays reviewable.
Summary by CodeRabbit
New Features
Bug Fixes
Tests