Summary
Tracker for six NOTE-severity findings from the engine accuracy audit. Most are documentation additions confirming correct-but-non-obvious code. N6 is a small math refinement (boss crit suppression) when target-level signal becomes available.
Findings
N1 - Pipeline.CalculateAll only iterates highest ranks
Location: Engine/Pipeline.lua:71-93
Issue: pairs(ns.SpellData) calls Calculate(spellID, state, nil) → GetCurrentRank always picks highest rank. Action-bar UI never exercises lower ranks. Masked the downranking BLOCKER from earlier QA passes.
Fix: When downranking lands (separate BLOCKER issue), add regression test calling Calculate(spellID, state, lowerRankIndex) for L70. Optionally extend CalculateAll to iterate per-rank for headline diagnostics.
N2 - GetSpellBonusDamage cross-school overlap
Location: StateCollector.lua:188-193
Status: Already correct. Per-school query already returns max applicable item bonus per school.
Fix: No code change. Add a comment so maintainers do not "fix" by summing across schools.
N3 - DoT/channel crit math is per-tick-independent EV
Location: Engine/CritCalc.lua:240-242,89-92
Status: Already correct. expectedDamage = totalDmg * (1 + critChance * (critMult - 1)) collapses to 1.0 when canCrit=false. Spot-checked: SW:P, Corruption, Mind Flay, CoA, UA, CoD, Drain Life, Hellfire, Rain of Fire, Insect Swarm, Hurricane, Blizzard, Consecration, Volley, Serpent Sting, Immolation Trap (all canCrit=false). Arcane Missiles canCrit=true correctly.
Fix: Add unit test asserting collapse for canCrit=false spells.
N4 - Soul Fire 1.15 coefficient intentional (post-2.4 patch)
Location: Data/SpellData_Warlock.lua:73-87
Status: Already correct. Naive 6.0/3.5 = 1.714 capped at 1.0; patch 2.4 raised it explicitly to 1.15.
Fix: No code change. Add inline citation comment referencing patch 2.4 notes.
N5 - CoD and Drain Soul coefficient 2.0 cap
Location: Data/SpellData_Warlock.lua:289-304 (CoD), 378-396 (Drain Soul)
Status: Already correct. Both 2.0 = canonical TBC cap for very-long DoTs/channels. Linear formula would give 4.0+ uncapped.
Fix: No code change. Document the 2.0 cap convention in SpellData header comments.
N6 - Boss-level crit suppression (~0.6%) unmodeled
Location: Engine/CritCalc.lua:82-88
Issue: L70 vs L73 (boss) target gets ~0.6% absolute crit reduction beyond the +3-level miss/dodge/parry penalties already modeled.
Status: Polish-tier accuracy gap.
Fix: When target-level signal becomes available (currently no UI input), subtract 0.006 from resolved crit chance in the boss-level branch.
Sequencing recommendation
- Documentation-only (N2, N4, N5) - batch into one PR adding citation comments.
- N3 unit test - add to
tests/test_critcalc.lua or similar.
- N1 - resolves naturally when the downranking BLOCKER lands.
- N6 - deferred until target-level signal exists in playerState/UI.
Dependencies
- N1 closes when the downranking penalty BLOCKER ships.
- N6 depends on UI/state exposing a
targetLevel field, which is a separate scope.
Summary
Tracker for six NOTE-severity findings from the engine accuracy audit. Most are documentation additions confirming correct-but-non-obvious code. N6 is a small math refinement (boss crit suppression) when target-level signal becomes available.
Findings
N1 -
Pipeline.CalculateAllonly iterates highest ranksLocation:
Engine/Pipeline.lua:71-93Issue:
pairs(ns.SpellData)callsCalculate(spellID, state, nil)→GetCurrentRankalways picks highest rank. Action-bar UI never exercises lower ranks. Masked the downranking BLOCKER from earlier QA passes.Fix: When downranking lands (separate BLOCKER issue), add regression test calling
Calculate(spellID, state, lowerRankIndex)for L70. Optionally extendCalculateAllto iterate per-rank for headline diagnostics.N2 -
GetSpellBonusDamagecross-school overlapLocation:
StateCollector.lua:188-193Status: Already correct. Per-school query already returns max applicable item bonus per school.
Fix: No code change. Add a comment so maintainers do not "fix" by summing across schools.
N3 - DoT/channel crit math is per-tick-independent EV
Location:
Engine/CritCalc.lua:240-242,89-92Status: Already correct.
expectedDamage = totalDmg * (1 + critChance * (critMult - 1))collapses to 1.0 when canCrit=false. Spot-checked: SW:P, Corruption, Mind Flay, CoA, UA, CoD, Drain Life, Hellfire, Rain of Fire, Insect Swarm, Hurricane, Blizzard, Consecration, Volley, Serpent Sting, Immolation Trap (all canCrit=false). Arcane Missiles canCrit=true correctly.Fix: Add unit test asserting collapse for canCrit=false spells.
N4 - Soul Fire 1.15 coefficient intentional (post-2.4 patch)
Location:
Data/SpellData_Warlock.lua:73-87Status: Already correct. Naive
6.0/3.5 = 1.714capped at 1.0; patch 2.4 raised it explicitly to 1.15.Fix: No code change. Add inline citation comment referencing patch 2.4 notes.
N5 - CoD and Drain Soul coefficient 2.0 cap
Location:
Data/SpellData_Warlock.lua:289-304 (CoD), 378-396 (Drain Soul)Status: Already correct. Both 2.0 = canonical TBC cap for very-long DoTs/channels. Linear formula would give 4.0+ uncapped.
Fix: No code change. Document the 2.0 cap convention in SpellData header comments.
N6 - Boss-level crit suppression (~0.6%) unmodeled
Location:
Engine/CritCalc.lua:82-88Issue: L70 vs L73 (boss) target gets ~0.6% absolute crit reduction beyond the +3-level miss/dodge/parry penalties already modeled.
Status: Polish-tier accuracy gap.
Fix: When target-level signal becomes available (currently no UI input), subtract 0.006 from resolved crit chance in the boss-level branch.
Sequencing recommendation
tests/test_critcalc.luaor similar.Dependencies
targetLevelfield, which is a separate scope.