Damage-model grouping: 40 misplaced modifiers, plus burn and two wrong numerators - #156
Open
shaumik wants to merge 4 commits into
Open
Damage-model grouping: 40 misplaced modifiers, plus burn and two wrong numerators#156shaumik wants to merge 4 commits into
shaumik wants to merge 4 commits into
Conversation
Showdown applies damage modifiers in three groups and the group decides where
the number truncates. This engine exposed every ability and item damage
influence as one lumped multiplier per side, so all of them landed in the final
group. Twenty-nine of the forty-eight modelled modifiers register `onBasePower`
upstream and belong before the formula instead. This moves those.
Two things made that a wrong number rather than a wrong shape. The final group
multiplies a figure that already contains the `+2` from `base + 2`, which canon
never scales; and base power stopped being truncated to a whole number before
entering the formula, so the error compounded with type effectiveness instead
of staying flat. Everything misplaced this way was systematically a little high.
What moved: the whole eighteen-item type-booster family (one constructor, one
edit), Muscle Band, Wise Glasses, Punching Glove, and the eight abilities
Technician, Rivalry, Reckless, Iron Fist, Analytic, Sheer Force, Sand Force and
Dry Skin — the last of those defender-side, as canon's `onSourceBasePower`.
`basePowerMod` in damage.go is the new group. It collects every handler on
either side, chains them in canon's `on*BasePowerPriority` order and applies the
result once. The priorities are named constants rather than a fixed sequence
because chainMod rounds at every pairing, so the order is observable. Charge
moved into the chain too: it is a priority-9 handler upstream, not a doubling of
raw power, and while the two are provably identical to the bit it is what
settles what Technician's threshold reads.
Tests first, and they were watched failing. `damage_grouping_test.go`
transcribes Showdown's chain from the reference rather than factoring it out of
computeDamage, and asserts the exact sixteen-roll spread per modifier — a
relative assertion ("with the item it should be bigger") passes in every group,
which is how forty of these sat here unnoticed. It opens with a control case
that must pass unchanged, so a mistranscribed reference cannot masquerade as a
forty-instance bug. All fourteen base-power cases failed before the change, each
with the engine reading high, and pass after.
Three existing tests asserted a ratio with a point of slack and went red on the
corrected engine. They were not loosened: a base-power boost lands unevenly on
the finished figure because the formula divides by the defence and by 50, so a
Mystic Water Surf correctly goes 24 → 30. They now compare against the same move
arriving with its base power already modified, which is exact and states the
group instead of a ratio that only holds where truncation is kind.
Numerators are now spelled the way upstream carries them. Muscle Band and Wise
Glasses are 4505/4096 and `toMod(1.1)` gives 4506; Reckless and Iron Fist are
4915/4096, not a clean 1.2.
The audit in docs/royale-followups.md was re-derived against a fresh upstream
checkout before anything moved. The list of forty is confirmed. Its Technician
side-finding is not: it claimed canon reads the base power after earlier
modifiers, but `comparePriority` sorts high-to-low and Technician's 30 is the
highest `onBasePower` priority in the gen-9 dataset, so it reads the raw power
and this engine was already right. Upstream's own test file pins both sides —
the boost is refused after a gen-7 Battery and granted after a gen-9 Steely
Spirit, because the gen-7 mod overrides Technician down to 19. Withdrawn in the
doc, with the reasoning recorded at the registry entry so it is not "fixed"
later. Three further findings filed as items 9-11 while doing this: two
final-group modifiers with off-by-one numerators, Reckless missing crash-damage
moves, and four tests that drifted back into pinning the RNG in #154.
Zero of the 147 golden fixtures moved, which the handoff predicted and which is
verified rather than assumed: no corpus roster carries a base-power-group
mechanic. The Showdown port is unchanged at 401 pass / 465 gaps — these are
rounding corrections, not missing mechanics.
`go test ./... -count=1`, `make test-showdown` and `make lint` are green, and
the Part II perturbation audit adds no new failures.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EeCvat5B8L1ymDhE5hsRcw
The second half of the damage-grouping fix. Eleven modifiers register
`onModify{Atk,SpA}` or the defender's `onSourceModify*` upstream and were
lumped into the final group along with everything else: Choice Band, Choice
Specs, Flash Fire, Hustle, Solar Power, Guts, Thick Fat and the four pinch
abilities. They now apply to the stat the formula reads.
The stat group is its own shape, not just its own position. calculateStat
floors the stage against the raw stat and hands over a whole number; handlers
run highest priority first; a *direct* handler replaces that number where it
stands while the chained ones accumulate; finalModify applies the accumulated
chain exactly once. Multiplying it all together as floats and truncating once —
which is what this did — agrees on a bare stat and drifts as soon as two
modifiers meet. `applyStatGroup` in damage.go is that shape, with canon's
`on*Modify*Priority` values as named constants.
Hustle is the one handler that does not chain, and upstream says so in a
comment before writing `this.modify(atk, 1.5)`. Thick Fat's Atk hook is
priority 6 rather than 5 for the same reason: it must have chained before
Hustle truncates.
Three things came out differently from what the handoff expected, and the
doc now records each.
**Burn belonged to this bug and was not on the list.** It is a condition, not
an ability or item, so the audit never looked at it — but canon applies it in
modifyDamage after type effectiveness, as a modifier on the damage, and skips
it outright for a Guts holder. This engine halved the Attack stat. Guts could
not move into the stat group without settling that: its old implementation was
Atk ×1.5 *plus* ×2 on the finished damage to cancel a halving applied to a
different number, three multipliers standing in for canon's one and landing on
neither of canon's figures. Guts is now a plain ×1.5 and burn is a damage
modifier that does not fire for it.
**The weather stat boosts were keyed on the wrong thing**, also not on the
list. Sandstorm's Rock Sp. Def boost is `onModifySpD` and snow's Ice Defense
boost is `onModifyDef`, and canon keys the defensive stat event on the move's
override — unlike the offensive one, which it re-keys to the category. So a
Psyshock runs ModifyDef and a Rock-type in sand gets nothing from it, where
reading the category handed the boost out anyway. Fixed here because it is the
same dispatch, and pinned by two cases that differ only in which stat the move
reads.
**The offensive event is re-keyed to the category and the raw read is not.**
`attackStat = (category === 'Physical' ? 'atk' : 'spa')` runs after
`calculateStat(attackStat...)` has already read the override, so a Body Press
reads Defense and still runs ModifyAtk — a Choice Band and a Thick Club boost
it. Reading the override slug for the item lookup silently dropped that.
**110 of 147 golden fixtures re-recorded, attributed by measurement rather than
assumed.** With burn left where it was, the stat modifiers alone move 99;
relocating burn adds the other 11. The handoff said "five of these are on
corpus teams", which is exactly right and easy to misread as five fixtures —
Choice Band, Choice Specs, Guts, Solar Power and Thick Fat, on four of the six
archetypes, so 18 of the 21 pairings and 126 of the 147 games are in scope
before a single roll is considered. The weather-stat fix moves none of them:
the corpus never pairs a Psyshock with a Rock-type in sand.
`docs/benchmark.md`'s spread-impact table is re-derived from
`go run ./cmd/spread-impact`, as Section 8 requires, along with the two
sentences downstream of it that quote its figures. Bastion's games now run ~33%
longer than with spreads stripped rather than ~18%. The expectimax sweep is
being re-measured too — depths 1 and 2 are in, depth 3 is still running — and
lands as its own commit rather than holding this one.
Four ratio assertions went red and none was loosened. "×1.5 within a point" and
"≈ half" describe a final-group modifier; once the modifier is on the stat the
finished damage is not in that ratio at all, because the boosted stat still has
to survive the division by the defense and by 50. Each now compares against the
same matchup with the raw stat pre-modified, which is exact and states the
group. One of them — Thick Fat in TestAbilityBattleIntegration — was passing
only on its seed and went red under Part II's perturbation audit; that is the
audit doing its job, and with it fixed the audit adds no new failures beyond
the six that predate this work.
`go test ./... -count=1`, `make test-showdown` and `make lint` are green. The
Showdown port is unchanged at 401 pass / 465 gaps.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EeCvat5B8L1ymDhE5hsRcw
Not a grouping bug — both were already final-group `onModifyDamage` handlers.
The number was wrong. Upstream carries its modifiers as a fraction over 4096,
and writing the decimal instead and rounding it into 4096ths lands somewhere
else for two of them:
Life Orb canon [5324, 4096] toMod(1.3) = 5325
Metronome, 3 repeats canon [6553, 4096] toMod(1.6) = 6554
Metronome, 4 repeats canon [7372, 4096] toMod(1.8) = 7373
Found while re-deriving item 5's audit, filed as item 9 in
docs/royale-followups.md, and held back from item 5's two commits so their
fixture movement stayed attributable to grouping alone.
The two items are not equally serious, and measuring rather than assuming is
what separates them. A one-point difference in numerator only changes the answer
once the figure being modified is large enough for the two products to straddle
a multiple of 4096. For Life Orb that starts at 5, and holds for about nine
values in ten above it — so nearly every Life Orb hit in the game was a point
off. For Metronome it starts at 686 and 512, and a single hit at level 50 does
not produce a pre-modifier figure anywhere near either.
So they are tested differently, deliberately. Life Orb is pinned as an exact
sixteen-roll spread in damage_grouping_test.go and was watched failing there.
Metronome is pinned at the multiplier against upstream's `dmgMod` table,
because a damage-spread case for it would pass with or without the fix and
would read as coverage it is not. The comment says so, so nobody later
"strengthens" it into a spread that proves nothing.
Metronome now carries the table verbatim rather than computing `1 + 0.2n`;
metronomeStep and metronomeMax are gone, since a table has no step. Two existing
tests asserted the decimals as setup and now assert the numerators — neither was
about the multiplier's value, and both read better for naming it exactly.
17 of the 147 golden fixtures re-recorded, all of them Life Orb's: three corpus
picks carry it and no roster runs Metronome. docs/benchmark.md needs no
re-derivation — the benchmark library carries neither item, verified rather than
assumed.
`go test ./... -count=1`, `make test-showdown` and `make lint` are green.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EeCvat5B8L1ymDhE5hsRcw
Both published tables in docs/benchmark.md are measurements, and Section 8 says
they must be re-derivable rather than transcribed. Item 5 moved every damage
number in the format, so both were re-run.
**Expectimax depth sweep** (240 games per depth, `go run ./cmd/bench -agents
heuristic,expectimax -depth N -games 20`). The table gains a column rather than
overwriting one — the pre-grouping figures are kept beside the new ones, the
same way the rounding fix kept its predecessor:
depth 1 48.1% → 49.2% [42.9%, 55.5%]
depth 2 36.7% → 37.5% [31.6%, 43.8%]
depth 3 42.1% → 41.2% [35.2%, 47.6%]
Every new point estimate sits inside the previous engine's interval, which is
what a fix that moves damage by a point here and there should do — it changes
numbers, not the shape of the game.
The finding the section rests on survived a third time, and is now better
supported than it was. The d1→d2 drop measures **11.7** points here against
**11.4** before the grouping fix and **11.5** before the rounding one: three
runs, spanning two engine changes that each moved every damage roll in the
format, agreeing inside half a point. The prose that quoted the two-run
agreement now quotes the three-run one.
The tail claim is restated rather than re-asserted. d2 and d3 separated on the
rounding-fix engine and stay separated here, but the d1→d3 slope has now
measured 12.3, 6.0 and 8.0 points across the three runs — so the note saying the
*slope* is not the stable part, only the sign and the first step, is kept and
strengthened rather than being quietly dropped now that a third number exists.
**Spread-impact table** re-derived from `go run ./cmd/spread-impact`, along with
the two sentences downstream that quote it: Bastion's games now run ~33% longer
than with spreads stripped rather than ~18%, and the worst case across 720 games
is 94 turns rather than 92.
Two things fixed in passing. The ruleset table still said 538 moves; the dataset
has been 560 since #154, which corrected the same figure in
docs/showdown-port.md and missed this one. And docs/engine-findings.md, which is
a closed record of the rounding pass, now says so where it quotes figures the
grouping fix has since superseded — its numbers were true of that pass and are
kept as its record rather than rewritten, with a pointer to the current ones.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01EeCvat5B8L1ymDhE5hsRcw
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
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 item 5 in
docs/royale-followups.md.Showdown applies damage modifiers in three groups, and the group decides where the number truncates: base power before the formula, the stat the formula reads, and the finished figure. This engine exposed every ability and item damage influence as a single lumped multiplier per side, so all of them landed in the final group. Forty of the forty-eight modelled modifiers were in the wrong one.
Two things followed, and they are why this was a wrong number rather than a wrong shape. The final group multiplies a figure that already contains the
+2frombase + 2, which canon never scales; and base power stopped being truncated to a whole number before entering the formula, so the error compounded with type effectiveness instead of staying flat. Everything misplaced this way was systematically a little high.Commits
416b111b57a7ed1e2cc8c8f7b246Split so the risky commit is eleven changes with the mechanism already proven by the first one, rather than forty landing together with a fixture re-record.
The audit was re-derived first, and confirmed
The doc says not to trust its own table. Re-derived against a fresh upstream checkout before any code moved: 48 modelled modifiers — 29 base-power, 11 stat, 8 already correct. Unchanged from what was filed.
Two notes for whoever reads it next. The engine models 66 damage-influencing registry entries, not 48; the other 18 are the resist berries, which are
onSourceModifyDamageand correctly final — "58 of 66" is the same finding with the berries counted in. And several canon modifiers are not the decimal they look like: Muscle Band is[4505, 4096]wheretoMod(1.1)gives 4506, Reckless and Iron Fist are the type boosters' 4915 rather than a clean 1.2. The moved handlers spell the numerator.Three things the handoff had wrong or missing
Technician needed no fix, and none was made. The doc claimed canon reads the base power after earlier modifiers, so the engine's raw
m.Power <= 60was half a bug. It isn't:Battle.comparePrioritysorts handlers priority high to low, and Technician'sonBasePowerPriority: 30is the highest in the entire gen-9 dataset — nothing runs before it, sothis.event.modifieris 1 andmodify(bp, 1)isbp. Upstream's owntest/sim/abilities/technician.jspins both sides: the boost is refused after a gen-7 Battery (22) and granted after a gen-9 Steely Spirit (22), becausedata/mods/gen7/abilities.tsoverrides Technician down to 19. Withdrawn in the doc, with the reasoning recorded at the registry entry so it does not get "fixed" later.Burn belonged to this bug and was not on the list. It is a condition rather than an ability or item, so the audit never looked at it — but canon applies it in
modifyDamageafter type effectiveness, as a modifier on the damage, and skips it outright for a Guts holder. This engine halved the Attack stat. Guts could not move into the stat group without settling that: its old implementation was Atk ×1.5 plus ×2 on the finished damage to cancel a halving applied to a different number — three multipliers standing in for canon's one, landing on neither of canon's figures.The stat half moved 99 fixtures, not a handful. The doc said "five of these are on corpus teams", which is exactly right — Choice Band, Choice Specs, Guts, Solar Power, Thick Fat. It is easy to read that as five fixtures. Four of six archetypes carry one, so 18 of 21 pairings do, which is 126 of the 147 games before a single roll is considered. Attributed by measurement rather than assumed: with burn left where it was, the stat modifiers alone move 99; relocating burn adds the other 11.
Two further defects, fixed with their group
Both are the same dispatch and were found by reading upstream rather than reasoning from the mechanism's name.
onModifySpDand snow's Ice Defense boost isonModifyDef, and canon keys the defensive stat event on the move's override — unlike the offensive one, which it re-keys to the category. A Psyshock therefore runsModifyDef, and a Rock-type in sand gets nothing from it, where reading the category handed the boost out anyway. Pinned by two cases differing only in which stat the move reads.attackStat = (category === 'Physical' ? 'atk' : 'spa')runs aftercalculateStat(attackStat…)has already read the override, so a Body Press reads Defense and still runsModifyAtk— a Choice Band and a Thick Club boost it. Reading the override slug for the item lookup silently dropped that.Tests, written first and watched failing
internal/engine/damage_grouping_test.gotranscribes Showdown's chain from the reference rather than factoring it out ofcomputeDamage, and asserts the exact sixteen-roll spread per modifier. A relative assertion ("with the item it should be bigger") passes in every group, which is how forty of these sat here unnoticed.It opens with a control that must pass unchanged, so a mistranscribed reference cannot masquerade as a forty-instance bug. All 25 cases failed before their commit, each in the predicted direction, and pass after. Each group also gets a whole-battle test through
ResolveTurn— the number proves the formula, a played turn proves the wiring reaches it.Four ratio assertions went red and none was loosened. "×1.5 within a point" and "≈ half" describe a final-group modifier; once the modifier is on the stat the finished damage is not in that ratio at all, because the boosted stat still has to survive the division by the defense and by 50. A Mystic Water Surf correctly goes 24 → 30, a 25% jump from a 19.995% modifier. Each now compares against the same matchup with the raw stat or base power pre-modified, which is exact and states the group. One of them — Thick Fat in
TestAbilityBattleIntegration— was passing only on its seed and went red under Part II's perturbation audit.Verification
go test ./... -count=1,make test-showdownandmake lintare green.gapBugstays 0.main.docs/benchmark.mdrequires. The expectimax sweep's finding survived a third time: the d1→d2 drop measured 11.7 points against 11.4 and 11.5 on the two previous engines, and every new point estimate landed inside the previous engine's interval. The spread-impact table came fromgo run ./cmd/spread-impact.Also filed, not fixed
move.recoil || move.hasCrashDamage). High Jump Kick and Jump Kick are both in the dataset carrying the flag; it needs a field throughcmd/data-sync, which is the same omission that cost Sonic Boom itsdamagefield.docs/engine-findings.mdis a closed record of the rounding pass; where it quotes figures this work supersedes, it now says so and points at the current ones rather than being rewritten.Generated by Claude Code