Skip to content

Core/Aura: allow all dots to crit#460

Merged
Ovahlord merged 1 commit intoThe-Cataclysm-Preservation-Project:masterfrom
gonzo1247:All-Dots-Crit
Feb 9, 2026
Merged

Core/Aura: allow all dots to crit#460
Ovahlord merged 1 commit intoThe-Cataclysm-Preservation-Project:masterfrom
gonzo1247:All-Dots-Crit

Conversation

@gonzo1247
Copy link
Contributor

@gonzo1247 gonzo1247 commented Feb 8, 2026

Changes proposed:

Previously, the attribute SPELL_ATTR8_PERIODIC_CAN_CRIT was used to check whether a periodic spell could critically hit or not.
However, investigations have shown that there are only 38 spells in 4.3.4 Spell.dbcs with the SPELL_ATTR8_PERIODIC_CAN_CRIT attribute.

Spells such as Sunfire, Insect Swarm and Black Arrow do not have the SPELL_ATTR8_PERIODIC_CAN_CRIT attribute, but can still critically hit.
At https://classic.warcraftlogs.com/reports/V8xQmCYqJdfzrcyb?fight=1&type=damage-done&source=2, you can see that in Cataclysm Classic, Sunfire and Insect Swarm could critically hit.
I was also able to verify this for the Black Arrow spell using sniffs from Cataclysm Classic.

Creatures that are not under the player's control cannot cause critical hits with periodic spells.

I examined Cataclysm Classic sniffs from BoT, BwD, BH, FL, and DS and could not find a single critical hit from periodic spells, regardless of whether the spells used had SPELL_ATTR2_CANT_CRIT or not.

In the core, it is implemented by

 if (GetTypeId() == TYPEID_UNIT && !GetSpellModOwner())
        return 0.0f;

in the function

float Unit::SpellCritChanceDone(SpellInfo const* spellInfo, SpellSchoolMask schoolMask, WeaponAttackType attackType /*= BASE_ATTACK*/, bool isPeriodic /*= false*/) const

Here is a sniff from the Black Arrow spell

TargetGUID: Full: 0x0000000000000000000000000000000 Creature/0 R4459/S10773 Map: 967 (Dragon Soul) Entry: 55265 (Morchok) Low: 00000
CasterGUID: Full: 0x0000000000000000000000000000000 Player/0 R4442/S0 Map: 0 (Eastern Kingdoms) Low: 000000000
SpellID: 3674 ((Serverside/Non-DB2) zzOldBlack Arrow)
PeriodicAuraLogEffectCount: 1
HasLogData: False
(PeriodicAuraLogEffectData) [0] Effect: 3
(PeriodicAuraLogEffectData) [0] Amount: 9900
(PeriodicAuraLogEffectData) [0] OriginalDamage: 3987
(PeriodicAuraLogEffectData) [0] OverHealOrKill: -1
(PeriodicAuraLogEffectData) [0] SchoolMaskOrPower: 32
(PeriodicAuraLogEffectData) [0] AbsorbedOrAmplitude: 0
(PeriodicAuraLogEffectData) [0] Resisted: 0
(PeriodicAuraLogEffectData) [0] SupportInfosCount: 0
(PeriodicAuraLogEffectData) [0] Crit: True
(PeriodicAuraLogEffectData) [0] HasDebugInfo: False
(PeriodicAuraLogEffectData) [0] HasContentTuning: False

Tests performed: (Does it build, tested in-game, etc)
Build and works

@Ovahlord Ovahlord merged commit b9bee7e into The-Cataclysm-Preservation-Project:master Feb 9, 2026
0 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants