Skip to content

Support "+X% to Fire Spell Critical Hit Chance" affix#2246

Open
luther-rotmg wants to merge 2 commits into
PathOfBuildingCommunity:devfrom
luther-rotmg:fix/2226-fire-spell-crit-chance
Open

Support "+X% to Fire Spell Critical Hit Chance" affix#2246
luther-rotmg wants to merge 2 commits into
PathOfBuildingCommunity:devfrom
luther-rotmg:fix/2226-fire-spell-crit-chance

Conversation

@luther-rotmg

Copy link
Copy Markdown
Contributor

Closes #2226.

Problem

The "of Xoph" suffix — +X% to Fire Spell Critical Hit Chance (mod group FireSpellBaseCriticalChance) — had no modNameList entry, so the fire spell qualifier was left unparsed and the affix granted nothing.

Fix

Adds a modNameList mapping to CritChance (BASE) restricted to fire spells via ModFlag.Spell + KeywordFlag.Fire, matching the existing fire spells convention already used in preFlagList.

Testing

Adds a parse regression test in spec/System/TestItemParse_spec.lua: +5% to Fire Spell Critical Hit Chance grants +5 base crit to fire spells, and 0 to attacks / 0 to cold spells. The full test suite and the ModCache check pass.

The "of Xoph" suffix ("+X% to Fire Spell Critical Hit Chance", mod group
FireSpellBaseCriticalChance) had no modNameList entry, so the "fire spell"
qualifier was left unparsed and the affix did nothing.

Add a modNameList mapping to CritChance (BASE) restricted to fire spells via
ModFlag.Spell + KeywordFlag.Fire, matching the codebase's existing "fire spells"
convention in preFlagList. Add a parse regression test.

Closes PathOfBuildingCommunity#2226
@Blitz54

Blitz54 commented Jun 21, 2026

Copy link
Copy Markdown
Contributor

This should be done differently. It should be under -- Skill Types as just fire spells so that it works for other mods like crit damage bonus. Worth adding similar lines for cold, lightning, and chaos spell for future proofing.

@luther-rotmg

Copy link
Copy Markdown
Contributor Author

This should be done differently. It should be under -- Skill Types as just fire spells so that it works for other mods like crit damage bonus. Worth adding similar lines for cold, lightning, and chaos spell for future proofing.

Gotcha, give me a chance to rework it.

… entry

Per review feedback: rather than a single modNameList entry for "fire spell
critical hit chance", add fire/cold/lightning/chaos spell to modFlagList's
Skill types section. These compose with any stat (critical hit chance, critical
damage bonus, ...) through the normal modName parse, and future-proof the other
elements. Update the regression test to assert the tag composes with both
CritChance and CritMultiplier and works per element.
@luther-rotmg

Copy link
Copy Markdown
Contributor Author

Thanks for the review — agreed, that's a cleaner approach. Pushed an update:

  • Dropped the single modNameList entry and instead added fire / cold / lightning / chaos spell to modFlagList's -- Skill types section. Since modFlagList is scanned after the mod name, the element+spell qualifier now composes with any stat through the normal parse — so it handles Critical Damage Bonus as well as Critical Hit Chance — and the other elements are future-proofed.
  • Updated the regression test to assert the tag composes with both CritChance and CritMultiplier, and works per-element (cold).

Tests + ModCache check are green.

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.

support for "+X% to Fire Spell Critical Hit Chance" affix

3 participants