fix(engine): accuracy fixes + /phd debug diagnostic (#44)#45
Merged
Conversation
The WoW API GetSpellCritChance(school) and GetCritChance() already include all passive talents that grant school-wide spell crit, global all-spell crit, weapon-conditional melee crit (when the weapon is wielded), and form-conditional crit (when in form). The TalentMap was re-applying these on top of the API value, causing a 1-3% overestimate per learned rank. Removed 17 entries entirely and stripped the CRIT_BONUS effect from 2 multi-effect entries (Arcane Instability, Force of Will - DAMAGE_MULTIPLIER effects retained). 16 KEEP entries (single-spell or per-spell-list crit) remain because the per-school API scalar cannot encode them. Each removal carries an explanatory tombstone comment referencing plan 44, Bug A. New tests/test_crit_double_count.lua enumerates all 17 removed keys and asserts the 2 partial-keep entries no longer carry CRIT_BONUS effects. 11 existing test files updated with recomputed combined-build crit math.
…44) Previously StateCollector wrote `state.auras[scope][spellID] = true` and ModifierCalc applied the AuraMap value at full strength on the first stack, ignoring auraData.applications. This caused overestimates against Shadow Vulnerability, Shadow Weaving, Fire Vulnerability, and Healing Way at sub-max stacks. StateCollector now stores auraData.applications (defaulting to 1). ApplyAuraEntry takes an applications param and computes stackFactor = stacks / maxStacks, threading it through ApplyEffect for both regular and countField effects. Type coercion at the boundary preserves boolean back-compat for non-stack-aware entries (factor 1.0). AuraMap data: - Shadow Vulnerability (17800/17803): maxStacks=5, value=0.20 - Shadow Weaving (15258, Warlock + Priest): maxStacks=5, value=0.10 - Fire Vulnerability (22959, new Mage entry for Improved Scorch debuff): maxStacks=5, value=0.15, school=Fire - Healing Way (29203): corrected from value=0.06 flat to value=0.18 maxStacks=3 (its own comment said +6%/stack to 3 stacks) 22-case tests/test_stack_scaling.lua covers linear scaling, max-stack clamp, legacy boolean back-compat, school filter, no-maxStacks baseline, and end-to-end Pipeline integration.
Surfaces why a populated action slot might lack a damage overlay - particularly the Hellfire-style failure where a spell is on the bar but its base spellID is missing from ns.SpellData. Default `/phd debug` dumps slots 1-12; `/phd debug N` dumps a single slot (1-120). Output shows action type, raw and base spellIDs, SpellData presence, Pipeline result or error. Defensive pcall around Pipeline.Calculate and GetMacroSpell so a single bad slot doesn't abort the dump. No engine or data changes.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (13)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a roadmap and implements fixes: talent crit double-count removal, stack-aware aura scaling end-to-end, a Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
.plans/44-accuracy-and-diagnostic.md (1)
216-217: Add an explicit in-game spellID verification step for AuraMap edits.Since this phase adds/updates stacking auras (including Improved Scorch), include a required verification step using
/dump GetPlayerAuraBySpellID(id)for each touched spellID before declaring verification complete.Based on learnings: Applies to Data/AuraMap_*.lua : Verify aura spellIDs in-game using
/dump GetPlayerAuraBySpellID(id)command.Also applies to: 238-242
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.plans/44-accuracy-and-diagnostic.md around lines 216 - 217, Add an explicit in-game spellID verification step for any AuraMap edits: when changing Data/AuraMap_*.lua (e.g., entries for Improved Scorch and other stacking auras) run and record the `/dump GetPlayerAuraBySpellID(id)` output for each modified spellID and include those dumps in the PR description; update your audit checklist and the PR checklist (referenced near the stack-scaling tests in tests/test_stack_scaling.lua and the Improved Scorch entry) to require this verification before marking verification complete.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.plans/44-accuracy-and-diagnostic.md:
- Around line 48-73: The plan is inconsistent: the summary "18 REMOVE" doesn't
match the 19 rows in the REMOVE table and the global instruction at the end
("delete whole entry blocks for all REMOVE rows") would erase partially-kept
effects like CRIT_BONUS vs DAMAGE_MULTIPLIER; fix by updating the REMOVE summary
count to match the table (or remove one row from the table), and change the
blanket-delete instruction so it only removes the CRIT_BONUS effects listed for
talents such as Arcane Instability and Force of Will while preserving their
DAMAGE_MULTIPLIER effects and any other partial-keeps (e.g., Sanctified Seals
should drop the entry body but preserve non-crit effects per note); also ensure
entries referencing weapon-specific auras (Dagger Specialization, Fist Weapon
Specialization, Poleaxe Specialization) remove their obsolete "weaponType
filter" TODO comments rather than deleting the whole talent, and keep references
to methods that consume these effects (GetCritChance, GetRangedCritChance,
GetSpellCritChance) consistent with the adjusted removals.
In `@tests/test_rogue_auras.lua`:
- Around line 550-556: The test currently uses a loose check
(assert.is_true(result.critChance >= 1.0)) which won't fail if critChance
exceeds the intended cap; change the assertion in the "Cold Blood alone caps at
1.00" test to assert the clamped value exactly (e.g. assert.are.equal(1.0,
result.critChance) or assert.is_true(result.critChance == 1.0)) so that
Pipeline.Calculate's critChance result is verified to be exactly 1.0 for the
given state (makeRogueState + aura 14177).
---
Nitpick comments:
In @.plans/44-accuracy-and-diagnostic.md:
- Around line 216-217: Add an explicit in-game spellID verification step for any
AuraMap edits: when changing Data/AuraMap_*.lua (e.g., entries for Improved
Scorch and other stacking auras) run and record the `/dump
GetPlayerAuraBySpellID(id)` output for each modified spellID and include those
dumps in the PR description; update your audit checklist and the PR checklist
(referenced near the stack-scaling tests in tests/test_stack_scaling.lua and the
Improved Scorch entry) to require this verification before marking verification
complete.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ccd77ca4-cb9c-4953-bc17-74af1ccad5ef
📒 Files selected for processing (33)
.plans/44-accuracy-and-diagnostic.mdCore/Init.luaCore/StateCollector.luaData/AuraMap_Mage.luaData/AuraMap_Priest.luaData/AuraMap_Shaman.luaData/AuraMap_Warlock.luaData/TalentMap_Druid.luaData/TalentMap_Hunter.luaData/TalentMap_Mage.luaData/TalentMap_Paladin.luaData/TalentMap_Priest.luaData/TalentMap_Rogue.luaData/TalentMap_Warlock.luaData/TalentMap_Warrior.luaEngine/ModifierCalc.luaPresentation/Diagnostics.luatests/test_crit_double_count.luatests/test_critcalc.luatests/test_druid_talents.luatests/test_hunter.luatests/test_mage_auras.luatests/test_mage_talents.luatests/test_modifiercalc.luatests/test_new_talents.luatests/test_paladin_talents.luatests/test_priest_auras.luatests/test_priest_talents.luatests/test_rogue_auras.luatests/test_rogue_talents.luatests/test_shaman_auras.luatests/test_stack_scaling.luatests/test_warrior_talents.lua
Comment on lines
+48
to
+73
| **Audit (18 REMOVE, 17 KEEP, 0 INCONCLUSIVE):** | ||
|
|
||
| REMOVE rows: | ||
|
|
||
| | File | Talent | Per-rank | Rationale | | ||
| |---|---|---|---| | ||
| | `Data/TalentMap_Druid.lua` | Sharpened Claws | +2% | Form aura active in Cat/Bear; in `GetCritChance()` while in form | | ||
| | `Data/TalentMap_Druid.lua` | Natural Perfection | +1% | All spells universal; in `GetSpellCritChance()` for every school | | ||
| | `Data/TalentMap_Hunter.lua` | Lethal Shots | +1% | Ranged-wide; in `GetRangedCritChance()` | | ||
| | `Data/TalentMap_Hunter.lua` | Survival Instincts | +2% | Melee+ranged; in both `GetCritChance()` and `GetRangedCritChance()` | | ||
| | `Data/TalentMap_Mage.lua` | Arcane Instability | +1% | **CRIT_BONUS effect only.** DAMAGE_MULTIPLIER effect on the same talent stays. | | ||
| | `Data/TalentMap_Mage.lua` | Critical Mass | +2% | Whole Fire school; in `GetSpellCritChance(SCHOOL_FIRE)` | | ||
| | `Data/TalentMap_Mage.lua` | Pyromaniac | +1% | Whole Fire school | | ||
| | `Data/TalentMap_Paladin.lua` | Holy Power | +1% | Whole Holy school; in `GetSpellCritChance(SCHOOL_HOLY)` | | ||
| | `Data/TalentMap_Paladin.lua` | Combat Expertise | (none) | **Data bug.** TBC tooltip grants Expertise + Stamina, NOT crit. Crit was added in Wrath 3.1.0. Delete entry entirely. | | ||
| | `Data/TalentMap_Paladin.lua` | Sanctified Seals (3:21) | +1% | Universal +crit per TBC tooltip (Wowhead spell=35395/35396/35397). Reflected in every `GetSpellCritChance(school)` and `GetCritChance()`. **Coder must verify the comment "Retribution 3:21" against PhDamage's tab numbering convention** - Wowpedia places this talent in Holy Tier 7. The `tab:index` key stays as `"3:21"` per existing source; only the entry body is removed. | | ||
| | `Data/TalentMap_Priest.lua` | Holy Specialization | +1% | Whole Holy school | | ||
| | `Data/TalentMap_Priest.lua` | Force of Will | +1% | **CRIT_BONUS effect only.** DAMAGE_MULTIPLIER effect stays. | | ||
| | `Data/TalentMap_Rogue.lua` | Malice | +1% | Melee-wide; in `GetCritChance()` | | ||
| | `Data/TalentMap_Rogue.lua` | Dagger Specialization | +1% | Weapon aura while wielding daggers; in `GetCritChance()`. Delete the obsolete `weaponType filter` TODO comment. | | ||
| | `Data/TalentMap_Rogue.lua` | Fist Weapon Specialization | +1% | Same as Dagger Spec. Delete obsolete TODO. | | ||
| | `Data/TalentMap_Warlock.lua` | Demonic Tactics | +1% | All spells + melee universal | | ||
| | `Data/TalentMap_Warlock.lua` | Backlash | +1% | All spells universal per TBC tooltip ("with all spells"). Existing "no school filter" comment confirms. | | ||
| | `Data/TalentMap_Warrior.lua` | Poleaxe Specialization | +1% | Weapon aura while wielding axe/polearm; in `GetCritChance()`. Delete obsolete TODO. | | ||
| | `Data/TalentMap_Warrior.lua` | Cruelty | +1% | Melee-wide; in `GetCritChance()` | | ||
|
|
There was a problem hiding this comment.
Fix contradictory execution guidance for Bug A before implementation.
The plan currently conflicts in two places:
- Line 48 says 18 REMOVE, but the REMOVE table has 19 rows (Line 54-Line 73).
- Line 58 and Line 65 mark partial-keep talents (remove only
CRIT_BONUS), but Line 207 instructs deleting whole entry blocks for all REMOVE rows. That would drop intendedDAMAGE_MULTIPLIEReffects.
Proposed patch to make the plan unambiguous
-**Audit (18 REMOVE, 17 KEEP, 0 INCONCLUSIVE):**
+**Audit (17 FULL REMOVE, 2 CRIT_BONUS-only edits, 17 KEEP, 0 INCONCLUSIVE):**
-REMOVE rows:
+Rows requiring action (17 full removals + 2 partial CRIT_BONUS removals):
-### Phase 1: Bug A — crit double-count [PENDING]
+### Phase 1: Bug A — crit double-count [PENDING]
-- 1.1 Touch each TalentMap_*.lua file per the REMOVE table; delete the offending entry blocks (the table-assignment to `TalentMap["tab:index"]` and the comment line above it). Where a TODO comment for weapon-type filtering accompanied an entry (Rogue Dagger/Fist Spec, Warrior Poleaxe Spec), delete the TODO too. For Sanctified Seals (`TalentMap_Paladin.lua` `["3:21"]`), additionally verify and correct the tab-name comment on L121/L135 if PhDamage's tab numbering disagrees with Wowpedia (Holy = Tier 7 per Wowpedia; comment currently says "Retribution"). KEEP entries are not modified. ← CURRENT
+- 1.1 Touch each TalentMap_*.lua file per the action table.
+ - Full-remove rows: delete the offending talent entry blocks.
+ - Partial rows (Arcane Instability, Force of Will): remove only `MOD.CRIT_BONUS`; keep `DAMAGE_MULTIPLIER`.
+ - Remove obsolete weapon-type TODO comments where they belong to deleted rows.
+ - For Sanctified Seals (`TalentMap_Paladin.lua` `["3:21"]`), verify/correct tab-name comment vs PhDamage tab numbering.
+ KEEP rows are not modified.Also applies to: 205-209
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.plans/44-accuracy-and-diagnostic.md around lines 48 - 73, The plan is
inconsistent: the summary "18 REMOVE" doesn't match the 19 rows in the REMOVE
table and the global instruction at the end ("delete whole entry blocks for all
REMOVE rows") would erase partially-kept effects like CRIT_BONUS vs
DAMAGE_MULTIPLIER; fix by updating the REMOVE summary count to match the table
(or remove one row from the table), and change the blanket-delete instruction so
it only removes the CRIT_BONUS effects listed for talents such as Arcane
Instability and Force of Will while preserving their DAMAGE_MULTIPLIER effects
and any other partial-keeps (e.g., Sanctified Seals should drop the entry body
but preserve non-crit effects per note); also ensure entries referencing
weapon-specific auras (Dagger Specialization, Fist Weapon Specialization,
Poleaxe Specialization) remove their obsolete "weaponType filter" TODO comments
rather than deleting the whole talent, and keep references to methods that
consume these effects (GetCritChance, GetRangedCritChance, GetSpellCritChance)
consistent with the adjusted removals.
Comment on lines
+550
to
556
| it("Cold Blood alone caps at 1.00", function() | ||
| local state = makeRogueState() | ||
| state.auras.player[14177] = true -- Cold Blood +100% | ||
| state.talents["1:3"] = 5 -- Malice 5/5 (+5%) | ||
| local result = Pipeline.Calculate(1752, state) | ||
| -- 0.25 + 1.00 + 0.05 = 1.30 → capped at 1.00 | ||
| -- 0.25 + 1.00 = 1.25 -> capped at 1.00 | ||
| assert.is_true(result.critChance >= 1.0, | ||
| "Expected critChance >= 1.0, got " .. tostring(result.critChance)) |
There was a problem hiding this comment.
Tighten the cap assertion.
The new test name/comment says this verifies capping, but >= 1.0 still passes if critChance regresses above the clamp. Please assert the clamped value directly so this test catches the failure mode.
Proposed fix
- assert.is_true(result.critChance >= 1.0,
- "Expected critChance >= 1.0, got " .. tostring(result.critChance))
+ assert.is_near(1.0, result.critChance, 0.001,
+ "Expected critChance to clamp to 1.0, got " .. tostring(result.critChance))📝 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
| it("Cold Blood alone caps at 1.00", function() | |
| local state = makeRogueState() | |
| state.auras.player[14177] = true -- Cold Blood +100% | |
| state.talents["1:3"] = 5 -- Malice 5/5 (+5%) | |
| local result = Pipeline.Calculate(1752, state) | |
| -- 0.25 + 1.00 + 0.05 = 1.30 → capped at 1.00 | |
| -- 0.25 + 1.00 = 1.25 -> capped at 1.00 | |
| assert.is_true(result.critChance >= 1.0, | |
| "Expected critChance >= 1.0, got " .. tostring(result.critChance)) | |
| it("Cold Blood alone caps at 1.00", function() | |
| local state = makeRogueState() | |
| state.auras.player[14177] = true -- Cold Blood +100% | |
| local result = Pipeline.Calculate(1752, state) | |
| -- 0.25 + 1.00 = 1.25 -> capped at 1.00 | |
| assert.is_near(1.0, result.critChance, 0.001, | |
| "Expected critChance to clamp to 1.0, got " .. tostring(result.critChance)) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/test_rogue_auras.lua` around lines 550 - 556, The test currently uses a
loose check (assert.is_true(result.critChance >= 1.0)) which won't fail if
critChance exceeds the intended cap; change the assertion in the "Cold Blood
alone caps at 1.00" test to assert the clamped value exactly (e.g.
assert.are.equal(1.0, result.critChance) or assert.is_true(result.critChance ==
1.0)) so that Pipeline.Calculate's critChance result is verified to be exactly
1.0 for the given state (makeRogueState + aura 14177).
…44) Phase 5 of #44 - resolves the bug surfaced by /phd debug: spellID 27213 (Hellfire R4 channel-cast) was unresolvable because PhDamage stored 27214 (the per-tick effect spell) in the rank.spellID slot. TBC channel spells using "Aura #23 Periodic Trigger Spell" have two IDs per rank: the channel-cast (returned by GetActionInfo) and the per-tick effect (the proc damage spell). The new optional rank.effectID field captures both. Changes: - New ns.SpellResolver consolidates three duplicated reverse-map builders (ActionBar, Tooltip, Diagnostics). Single lookup table maps any rank or effect ID to (baseID, rankIndex). Pure-Lua, no WoW APIs. - Hellfire ranks 1-4: add effectID (5857/11681/11682/27214) and restore R4 spellID to 27213 (channel-cast). - ActionBar/Tooltip/Diagnostics now delegate ID resolution to ns.SpellResolver. - New tests/test_spell_resolver.lua covers Hellfire''s cast/effect/base ID resolution + type guards + Rebuild. Engine and combat-log paths untouched (PhDamage has zero combat-log listeners; only action-bar overlay and tooltip needed updates).
Cross-class audit covered all TBC channeled AoE spells using "Aura #23 Periodic Trigger Spell": - Rain of Fire (Warlock 5740): 5 ranks - Blizzard (Mage 10): 7 ranks - Volley (Hunter 1510): 4 ranks - Hurricane (Druid 16914): 4 ranks All 20 ranks already had the channel-cast spellID stored correctly (Hellfire R4 was uniquely MIXED). This commit adds only the effectID field per rank (effect IDs in the 42xxx range, introduced patch 2.4.0). Now SpellResolver covers any of these spells dragged from spellbook to action bar. Other channeled spells (Drain Life/Soul, Health Funnel, Mind Flay, Arcane Missiles, Tranquility) use different mechanics (Periodic Drain, Trigger Missile) - single ID, no fix needed. Source: TBC-Twinhead per-rank pages, cross-validated against the verified Hellfire R1/R4 entries. Also adds: - ns.SpellResolver row in AGENTS.md namespace table - Cross-class describe block in test_spell_resolver.lua (8 cases covering one rank cast+effect ID per audited spell)
Hellfire R1-R4 had incorrect totalDmg values (255/510/825/1395) and R1-R3 had incorrect levels (12/28/44 vs the correct 30/42/54). R4 level was already correct at 68. Verified against TBC-Twinhead authoritative tooltips: - R1 (1949): per-tick 83, level 30 -> totalDmg = 1245 - R2 (11683): per-tick 139, level 42 -> totalDmg = 2085 - R3 (11684): per-tick 208, level 54 -> totalDmg = 3120 - R4 (27213): per-tick 306, level 68 -> totalDmg = 4590 PhDamage convention for channel spells (confirmed by reading Engine/SpellCalc.lua ComputeChannel/ComputeDot/ComputeHybrid): totalDmg = perTickDamage * numTicks. Hellfire is a 15-tick channel (numTicks=15, tickInterval=1, duration=15) - parent fields unchanged. Cross-checked Drain Life R8 (108/tick * 5 ticks = 540, matches stored 540 exactly) confirming convention is correctly applied elsewhere. Hellfire is an isolated data error, not class-wide. Surfaced during /phd debug investigation of issue #44.
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.
Closes #44.
Summary
Fixes the consistent ~5-10% expected-damage overestimate reported in #44 (two independent root causes) and adds a new
/phd debugdiagnostic to investigate the related missing-overlay symptom (e.g. Hellfire).Bug A: crit double-count (data fix)
Core/StateCollector.luareadsGetSpellCritChance(school)andGetCritChance()per-school. The WoW API value already reflects passive talents that grant school-wide spell crit, global all-spell crit, weapon-conditional melee crit (when the weapon is wielded), and form-conditional crit (when in form).Engine/ModifierCalc.luawas then addingmods.critBonus(sourced from TalentMapMOD.CRIT_BONUSentries) on top, double-counting the same talents.The audit (per-talent verbatim TBC tooltip lookups via the wow-addon agent) classified 35 candidate entries:
MOD.CRIT_BONUSeffect was removed from Arcane Instability and Force of Will. TheirMOD.DAMAGE_MULTIPLIEReffects remain (no per-school spell-damage API to subsume them).GetSpellCritChance(school)), Improved Overpower, plus the surviving DAMAGE_MULTIPLIER effects on the two partials.Each removal carries an inline tombstone comment citing this PR. New
tests/test_crit_double_count.luaenumerates the removed keys and asserts the partial-keep entries no longer carryCRIT_BONUSeffects. 11 existing test files were updated with recomputed combined-build crit math.Bug B: stack-aware aura modifiers
StateCollectorpreviously wrotestate.auras[scope][spellID] = true, discardingauraData.applications.ModifierCalcthen applied the AuraMap value at full strength even for a single stack of a 5-stack aura. Producers and consumers were out of sync on what "present" means.Fixed end-to-end:
Core/StateCollector.lua: storesauraData.applications or 1in all three aura-write sites.Engine/ModifierCalc.lua:ApplyAuraEntryacceptsapplicationsand computesstackFactor = stacks / maxStacks, threading it throughApplyEffect(for both regular andcountFieldeffects). Type coercion at the boundary preserves boolean back-compat: entries withoutmaxStackssee factor 1.0; stack-aware entries treattrueas 1 stack.Data/AuraMap_Warlock.lua,Data/AuraMap_Priest.lua: Shadow Vulnerability (17800/17803) and Shadow Weaving (15258) getmaxStacks = 5with at-max values.Data/AuraMap_Mage.lua: new Fire Vulnerability entry (spellID 22959, the debuff applied by Improved Scorch) -maxStacks = 5,value = 0.15, school filter Fire.Data/AuraMap_Shaman.lua: corrected pre-existing Healing Way bug (29203) - wasvalue = 0.06flat which under-applied at 2-3 stacks; nowvalue = 0.18, maxStacks = 3so stack 1 = +6%, stack 3 = +18% per its own comment.New
tests/test_stack_scaling.lua(22 cases) covers linear scaling, max-stack clamping, legacy boolean back-compat, school filtering, no-maxStacks baseline (Curse of Elements 27228), and end-to-end viaPipeline.Calculate.Bug C (diagnostic only):
/phd debug [slot]To investigate the user-reported missing-overlay symptom on Hellfire (root cause unknown - the spell data is complete and the engine has no skip path), this PR ships a chat-print diagnostic instead of guessing at a fix.
/phd debugdumps slots 1-12;/phd debug Ndumps a single slot (1-120). Output shows action type, raw and base spellIDs,ns.SpellDatapresence, and Pipeline result or error. The Hellfire failure mode (action present, base spellID unresolved or absent fromns.SpellData) is surfaced asbase spellID: no resolution | SpellData: absent. Once a user runs this in-game on a non-overlaying slot, the root cause should be obvious and a follow-up issue can be filed.Defensive
pcallaround bothPipeline.CalculateandGetMacroSpellso one bad slot doesn''t abort the bar dump. No engine or data changes; onlyPresentation/Diagnostics.luaandCore/Init.lua(slash dispatch + help line).Non-goals (deferred)
/phd debugreveals what is actually missing. Filing a follow-up issue once a user reports the diagnostic output.Verification
luacheck . --exclude-files Libs/: 0 warnings, 0 errors in 80 files.lua tests/bootstrap.lua: 117 spells, 110 talents, 55 auras, 4 engine modules.busted --verbose: not runnable locally (MinGW/luasystem missing); CI is the gate. New regression tests:tests/test_crit_double_count.lua(Bug A),tests/test_stack_scaling.lua(Bug B, 22 cases). All previously-passing tests updated to match the new ground truth where the removed crit talents and stack-aware auras were asserted.Plan
.plans/44-accuracy-and-diagnostic.mdis included in this PR for reviewer reference.Summary by CodeRabbit
New Features
/phd debug [slot]diagnostic to inspect action-bar slot spell resolution and pipeline outputs.Bug Fixes
Documentation
Tests
Bug D: Channel-spell dual-ID resolution (Phase 5, added after #45 opened)
User ran
/phd debug 6(the new diagnostic from Phase 3) and got:The diagnostic surfaced a real data bug: TBC channeled AoE spells using "Aura #23 Periodic Trigger Spell" have TWO spell IDs per rank. The channel-cast ID is what
GetActionInforeturns (player-facing). The effect ID is the per-tick damage proc.Hellfire R4 was uniquely MIXED: PhDamage stored effect ID
27214in the rank''s spellID slot instead of channel-cast27213. When the user dragged Hellfire from the spellbook (where it sits as the rank-4 cast spell), the action bar resolved to27213and PhDamage''s reverse-map didn''t know about it.Architectural fix
Three duplicated reverse-map builders (in
Presentation/ActionBar.lua,Presentation/Tooltip.lua,Presentation/Diagnostics.lua) consolidated into one new modulens.SpellResolver. Pure-Lua, no WoW APIs, lazy-built lookup. Maps any base/rank-cast/effect ID to(baseID, rankIndex).Schema change
Data/SpellData_*.luarank entries gain an optionaleffectIDfield. Purely additive - no existing engine reader cares. SpellResolver indexes bothspellIDandeffectIDinto the same lookup table.Cross-class audit
All four TBC channeled AoE spells using the same Aura #23 mechanic were audited:
24 effectID entries total. Source: TBC-Twinhead per-rank pages.
Out of scope
These channeled spells use different mechanics (Periodic Drain, Trigger Missile) - single ID per rank, no fix needed: Drain Life, Drain Soul, Health Funnel (Warlock), Mind Flay (Priest), Arcane Missiles (Mage), Tranquility (Druid).
Test coverage
tests/test_spell_resolver.lua(21 cases): Hellfire base/cast/effect resolution for all 4 ranks, type guards, Rebuild idempotence, plus cross-class verification of one rank cast+effect ID per audited spell.Files
Core/SpellResolver.lua,tests/test_spell_resolver.luaPhDamage.toc,Presentation/{ActionBar,Tooltip,Diagnostics}.lua,Data/SpellData_{Warlock,Mage,Hunter,Druid}.lua,tests/bootstrap.lua,AGENTS.mdHellfire data correction (added with the dual-ID resolver fix)
While verifying Hellfire's spell IDs against TBC-Twinhead, the lookup also surfaced incorrect
totalDmgandlevelvalues for ranks 1-4. Corrected per authoritative tooltips:Convention
totalDmg = perTick * numTicksconfirmed by readingEngine/SpellCalc.lua(channel/DoT/hybrid paths all divide bynumTicks). Cross-checked Drain Life R8 (108/tick * 5 ticks = 540, stored 540) confirming the convention is correctly applied elsewhere - Hellfire is an isolated data error, not class-wide.