Fix dual wield skills with "both weapons hit..." combining into a single hit#2006
Open
majochem wants to merge 13 commits into
Open
Fix dual wield skills with "both weapons hit..." combining into a single hit#2006majochem wants to merge 13 commits into
majochem wants to merge 13 commits into
Conversation
In PoE2 `doubleHitsWhenDualWielding` does not actually cause a combined hit from both weapons, but instead causes two separate near simultaneous hits. In order to still be able to reuse the old logic, I changed all checks for `doublenHitsWhenDualWielding` to `combinesHitsWhenDualWielding` instead. Support for the old flag will be in separate commit.
Now correctly reflects behavior of the three possible dual wield hitRate scenarios: 1. One combined hit from both weapons (Harmonic Mean) 2. Two separate simultaneous hits from each weapon (2x Harmonic Mean) 3. Alternating between Mainhand and Offhand (Harmonic Mean)
This is still not ideal imo, but it's mostly due to the fact that we don't have a good breakdown for "DPS" mods
Found this bug when expanding `PasteSocketGroup()`, which probably wasn't a problem until now because it could only occur with `testInput`
Also added emmylua annotations for easier understanding of the `testInput` structure
Since `skillData.dpsMultiplier` was multiplied with itself in each pass, any modifiers would be applied twice for dual wield skills. Instead it is now added to `output.DpsMultiplier` and combined with `combineStat()`
Previously got a failed test because DPS value were not equal after 10 decimals ...
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.
Description of the problem being solved:
PoB2 until now assumed that the
doubleHitsWhenDualWieldingbehaved similar to PoE1, where both weapons do a single combined hit. However, as confirmed by Viperesque from GGG and in-game testing, the skills actually perform two separate near simultaneous hits. As a result, Average Damage, Attack Rate and the Effective Crit Chance are all currently wrong (combined hit makes the hit count as critical, as long as one of the weapons crits)This PR:
doubleHitsWhenDualWieldingto instead perform two hitscombinesHitsWhenDualWielding. *(Note: There's currently no skill in the game that has such behavior, but it is likely to be included with future weapon types, so we will still need the logic)Steps taken to verify a working solution:
TotalDPSvalues are correct for all dual wield variationsLink to a build that showcases this PR:
Build with test maces
Limitations:
"DPS"i.e. hit rate multiplier mods. The current one shows that it's multiplying the "Average Damage", but it's not very visible imo. Changing DPS breakdown should be a separate PR though.Before screenshot:
Average hit wrongly combines damage
No DPS Multiplier
After screenshot:
Average hit corrected
New DPS Multiplier
