fix: prevent OverflowException in rune skill power for high-stat casters (PLD-1390)#3293
Open
ipdae wants to merge 5 commits into
Open
fix: prevent OverflowException in rune skill power for high-stat casters (PLD-1390)#3293ipdae wants to merge 5 commits into
ipdae wants to merge 5 commits into
Conversation
…casters Rune skills with a Caster stat reference computed their power as `(int)Math.Round(stat * SkillValue)`. Because a C# `decimal -> int` cast always performs an overflow check (regardless of checked/unchecked context), any character whose referenced stat is large enough that `stat * SkillValue` exceeds int.MaxValue threw System.OverflowException, which aborted every battle action that calls SetRuneSkills (HackAndSlash, Sweep, Raid, AdventureBoss, InfiniteTower) and blocked gameplay. Widen `power` to `long` and use NumberConversionHelper.SafeDecimalToInt64, matching the rest of the pipeline which is already long-based (SkillFactory.GetV1(long power), Skill.Power, SkillCustomField.BuffValue, and the AttackSkill/ArenaAttackSkill damage calc via SafeDecimalToInt64). Reproduced from the real failing tx 1ab27944f05a195c1d5a1906513bd15e7c2e97253ba077e9bd0272965abf556a (ATK 629,550,973 + rune 10003 lv200, SkillValue 4.47 -> (int)Math.Round(2,814,092,849) overflow). Added RuneSkillOverflowTest as a regression test. The obsolete Player.SetRuneV1 (used by StageSimulatorV3/RaidSimulatorV2 for legacy action versions) has the same cast but is intentionally left unchanged to preserve historical replay determinism. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Deploying lib9c with
|
| Latest commit: |
32a4e22
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://10dc6c88.lib9c.pages.dev |
| Branch Preview URL: | https://bugfix-pld-1390-rune-skill-o.lib9c.pages.dev |
Document the constructor and the two test methods so the PR does not increase the no-docs count enforced by the count-no-docs lint gate. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ArenaCharacter.SetRuneSkills clamped rune skill power to int.MaxValue via SafeDecimalToInt32, while the PvE Player.SetRuneSkills fix widened it to long. For a high-stat caster this made PvE and arena compute different rune skill power (PvE full value, arena capped at ~2.1B). Widen the active arena SetRuneSkills to long/SafeDecimalToInt64 as well so both paths agree and match the long-based damage pipeline. The obsolete ArenaCharacter.SetRuneV1 is left unchanged for historical determinism. Add an arena regression test alongside the PvE one, and drop the per-member XML doc comments on the test (no longer needed now that the count-no-docs gate excludes test projects, #3295). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
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.
Summary
Fixes PLD-1390. A Caster-stat-referencing rune skill computed its power as
(int)Math.Round(stat * SkillValue)inPlayer.SetRuneSkills. In C# adecimal -> intcast always performs an overflow check (regardless ofchecked/unchecked), so any character whose referenced stat is large enough thatstat * SkillValueexceedsint.MaxValue(~2.1B) threwSystem.OverflowException. That aborted every battle action callingSetRuneSkills(HackAndSlash, Sweep, Raid, AdventureBoss, InfiniteTower) and blocked gameplay for the affected avatar.The whole downstream pipeline is already
long-based —SkillFactory.GetV1(long power),Skill.Power,SkillCustomField.BuffValue, and theAttackSkill/ArenaAttackSkilldamage calc (SafeDecimalToInt64). Theintlocal inSetRuneSkillswas the only narrowing point. This widenspowertolongviaNumberConversionHelper.SafeDecimalToInt64.Root cause
Lib9c/Model/Character/Player.cs(SetRuneSkills):Reproduction
Real failing tx:
1ab27944f05a195c1d5a1906513bd15e7c2e97253ba077e9bd0272965abf556aSkillValue = 4.47, ATK% Caster(int)Math.Round(629,550,973 × 4.47)=2,814,092,849>int.MaxValue→OverflowExceptionRuneSkillOverflowTestreproduces this against the real values (localRuneOptionSheet.csvalready contains rune 10003 lv1–300, so noPatchTableSheetneeded) and asserts the power is now the fulllongvalue.Notes / out of scope
Player.SetRuneV1(used byStageSimulatorV3/RaidSimulatorV2for legacy action versions) has the same cast but is intentionally left unchanged to preserve historical replay determinism.ArenaCharacter.SetRuneSkillsalready clamps withSafeDecimalToInt32(no crash); it is unchanged here, so PvE now yields the fulllongpower while arena still caps atint.MaxValue. Whether to unify the two (and the relatedSafeDecimalToInt32clamps in the damage/DoT/vampiric paths) is a separate balance/consensus decision tracked in PLD-1390.Test
🤖 Generated with Claude Code