feat(combat): per-weapon momentum — exploding damage dice override (#134) - #135
Merged
Conversation
) Adds an "Exploding damage dice" toggle to the Weapon Bonuses tab. When on, the weapon's damage dice explode regardless of the Shadowdark system's world-wide Momentum Mode setting, which is what building a single custom exploding weapon needs. It covers the weapon's base damage, its configured damage bonuses, and critical extra dice. Stored as flags.shadowdark-extras.weaponBonus.momentum, inside the existing weapon-bonus config, so it inherits the tab's `enabled` master switch and the WEAPON_BONUSES feature gate rather than adding parallel ones. The system is left untouched. Its applyExploding rewrites only the FIRST dice term of a formula (the regex has no `g` flag), which is why the world setting never reaches added bonus dice — a system-side bug, deliberately out of scope here. That bug does shape the design, because SDX's rollFromConfig wrapper runs BEFORE the system's roll(). Two predicates result: - shouldExplodeSystemFormula, for formulas handed to shadowdark.dice.roll, requires Momentum Mode to be OFF. Exploding while it is on hands the system `1d8x`, which its regex rewrites to `1d8xx` — a second explode modifier and inflated damage. When the setting is on the system already explodes, so the override has nothing to add. - shouldExplodeOwnRoll, for Rolls built locally in calculateWeaponBonusDamage, has no such check. Those never reach the system's roll(), so there is no collision and the weapon flag alone governs them. Gating on the setting rather than on the system's current behaviour is also what makes this survive a system-side fix: if applyExploding ever gains its missing `g` flag, the override stays dormant exactly when it should. applyExplodingAll is global and idempotent, so a double pass — or a formula the system already rewrote — never produces `1d8xx`. 17 tests in dev/tests/weapon-momentum.test.mjs, including the `1d8xx + 1d6x` collision the guard prevents and the `1d6max3` case where a naive /x/ modifier check reads the x in `max` as an explode and silently never fires.
Two corrections, no behaviour change. The comments and the test's replica of the system's applyExploding claimed its regex lacks the `g` flag and rewrites only the FIRST dice term. That is wrong. SD 4.0.6 uses `/ig` in both src/dice/dice.mjs:47 and the compiled build Foundry actually loads, so the system explodes every dice term of the formula it is handed — folded-in damage bonuses included. The guard in shouldExplodeSystemFormula is therefore MORE necessary, not less: pre-exploding while the world setting is on yields `1d8xx + 1d6xx`, a second explode modifier on every die rather than just the first. The test that pins the collision now asserts that, and a new assertion pins the system's replica as non-idempotent, which is the property the guard actually exists for. What the world setting genuinely does not reach is a damage bonus rolled separately in calculateWeaponBonusDamage, outside shadowdark.dice.roll. That, not the regex, is why shouldExplodeOwnRoll carries no world-setting check. Also folds the three repeated `explodeOwnRolls ? applyExplodingAll(f) : f` ternaries in calculateWeaponBonusDamage into a single local `withMomentum` helper, so the three roll sites read uniformly.
Retracts the correction in 6bf8698. That commit read a LOCALLY PATCHED copy of the Shadowdark system: this machine's install has dice.mjs.orig and shadowdark-compiled.mjs.orig backups showing stock `/i` edited to `/ig`. Stock SD 4.0.6 does explode only the first dice term, as originally stated. Rather than swap one hard-coded assumption for the other, the comments and tests now assert neither. The rule that actually holds is structural: when Momentum Mode is on the system owns that formula and SDX keeps off it, whichever variant is installed. The guard was already correct for both — nothing about the shipped behaviour has changed across any of these three commits. The test replicates BOTH variants and pins the collision for each: stock yields `1d8xx + 1d6x`, patched yields `1d8xx + 1d6xx`. Neither skips an already exploding term, so the guard cannot be relaxed by assuming a build. Also addresses review findings: - Display formulas were built from the un-exploded source, so a momentum weapon showed "1d6" beside a result of 15. bonusFormula, criticalExtraDiceFormula and criticalFormula now carry what was rolled. - Adds a reroll test: flipping Momentum Mode on between roll and reroll does not double, because the pass is idempotent and rerolls skip the system's own transform. - Corrects the wiki, which claimed SDX "leaves the roll alone" under Momentum Mode without noting that separately rolled bonuses still follow the weapon toggle.
Three review findings, one of them a live damage bug. Modifier detection was a substring search, `/(?:^|[^a-z])x/`, which was wrong in both directions. It read the `x` in `1d6max3` as an explode and never exploded that term, and — the dangerous one — could not see the `x` after `kh` in `2d6khx`, so it appended a second modifier and produced `2d6khxx`. That is a silent damage inflation, not a cosmetic slip. Modifier strings are now walked token by token against Foundry's modifier list, longest-first so `max` is not read as `ma` and `xo` not as `x`. Unrecognised text now reports as already exploding: under-exploding is a visible no-op a user can report, whereas a stray second `x` just quietly inflates damage. The dice-term pattern also matched only numeric faces, so `1d(6+2)` and `4dF` were skipped. Parenthesised faces are now matched and exploded; Fate dice are matched and deliberately NOT exploded, since their maximum face is +1 and exploding would re-roll about a third of the pool. calculateWeaponBonusDamage still runs for an item that kept weaponBonus.enabled when Weapon Bonuses is disabled but Enhanced Damage Cards is not, so shouldExplodeOwnRoll alone let momentum act through a switched-off feature. The local-roll path now checks WEAPON_BONUSES as the system-formula path in roll-patches already did. The damageComponents entry kept the untransformed part.formula while the roll used the exploded one, so buildRollBreakdown reported a bonus rolled as `1d6x` as plain `1d6`. The formula is resolved once per part and reused by the roll, the component and the breakdown. 22 tests, including both modifier directions, Fate dice, parenthesised faces and the fail-closed path for unparseable modifiers.
#134 asks for exploding dice on "any weapon". Requiring Enable Weapon Bonuses first meant a weapon that wants only exploding damage had to switch on the hit, damage and critical bonus machinery it was not using, so weaponHasMomentum no longer reads the `enabled` flag. The checkbox moves out of .sdx-bonus-content and up beside the master switch as its peer. That is not cosmetic: .sdx-bonus-content takes opacity 0.4 and pointer-events: none when the master switch is off, so leaving the control there while making it functional would have produced a toggle that looks disabled and still fires. The WEAPON_BONUSES feature gate still applies — this decouples momentum from the per-item switch, not from the feature that owns the tab. Verified live against a STOCK Shadowdark 4.0.6 (the local /ig patch was reverted first): with the master switch off and momentum on, 1d8 rolls as 1d8x and 1d8 + 1d6 as 1d8x + 1d6x.
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.
Implements the enhancement half of #134: a per-weapon exploding-dice toggle that overrides the system's world-wide Momentum Mode.
The bug half of that issue is deliberately out of scope — it is system-side and was left alone at the owner's direction.
What this adds
An Exploding damage dice toggle on the Weapon → Bonuses tab. When on, that weapon's damage dice explode regardless of the Shadowdark system's Momentum Mode setting — its base damage, its configured damage bonuses, and its critical extra dice.
Stored as
flags.shadowdark-extras.weaponBonus.momentum, inside the existing weapon-bonus config, so it inherits the tab'senabledmaster switch and theWEAPON_BONUSESfeature gate rather than adding parallel ones.The decision that shapes the behaviour
applyExplodingexists in two forms in the wild: stock SD 4.0.6 uses/iand explodes only the first dice term (the bug half of #134); a one-character local patch to/igexplodes all of them, and such installs exist.This module deliberately models neither. The rule it rests on is structural: when Momentum Mode is on, the system owns that formula and SDX keeps off it. Hence two predicates:
shouldExplodeSystemFormulashadowdark.dice.rollshouldExplodeOwnRollcalculateWeaponBonusDamageroll()SDX's
rollFromConfigwrapper runs before the system'sroll(). Neither system variant skips a term that already explodes, so pre-exploding while Momentum Mode is on yields a doubled1d8xxeither way — they differ only in how many terms they double. The test replicates both and pins the collision for each.applyExplodingAllis global and idempotent; neither system variant is. That idempotence is the whole difference and is what makes a double pass harmless.Resulting behaviour
On a stock system the "explodes (system)" cells cover only the first die of the formula. That limitation is the issue's bug half and is not addressed here.
Review fixes
Three rounds of review landed in
6bf8698f,136d7afcandcc49206a:/(?:^|[^a-z])x/search read thexin1d6max3as an explode (never exploding it) and could not see thexafterkhin2d6khx, appending a second modifier for2d6khxx— a silent damage inflation. Modifier strings are now walked token by token against Foundry's modifier list, longest-first. Unrecognised text fails closed.1d(6+2)and4dFwere skipped by a numeric-only faces pattern. Parenthesised faces now explode; Fate dice are matched and deliberately not exploded (max face +1).calculateWeaponBonusDamagestill runs for items retainingweaponBonus.enabledwhen Weapon Bonuses is off but Enhanced Damage Cards is on; the local-roll path now checksWEAPON_BONUSESas the system-formula path already did.bonusFormula,damageComponents[].formula,criticalExtraDiceFormulaandcriticalFormulanow carry the evaluated formula, so a bonus rolled as1d6xno longer reports as1d6beside a result of 15.withMomentumhelper.One finding was refuted: a guard on
config.damageRoll.explodewas requested, but no such path exists in SD 4.0.6.explodappears four times in the compiled build Foundry loads — a doc comment, the function, its single call site, and its namespace export — and that call site is guarded byuseMomentumModealone. There is no explode field intemplate.jsonor the data model for an Active Effect to target.Verification
dev/tests/weapon-momentum.test.mjs)npm run verify→ OK; all four structural gates → OKeslint→ zero output on every touched fileNote on the flag snapshot
dev/snapshots/flag-keys.jsonwas already stale onmainbefore this work. Regenerating swept in informational line-number churn forenhanced-header.mjs(+14) andnpc-feature-macros.mjs(+1), which this change does not touch. Only+"weaponBonus.momentum"and the tworoll-patches.mjsshifts belong to this PR.Not yet done
Not verified in a live world — and note that this machine's Shadowdark install is locally patched to
/ig, so testing here exercises the patched path, not what users on a stock system get.