Skip to content

Add support for Forking Critical Strikes #1100

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 7 commits into from
May 6, 2025

Conversation

Antaresque
Copy link
Contributor

@Antaresque Antaresque commented May 4, 2025

Fixes #1081 .

Description of the problem being solved:

As Tangletongue is one of most used uniques in current patch, it should be added as its addition would be very useful to most of players using PoB.
Forking crits, according to in-game descriptions and players' observation:

  • rolls Critical Strike twice (similar to Lucky, but both works alongside each other),
  • if only one of them rolls, then its normal crit,
  • if both roll, then critical bonus is doubled.

Implementation adds flag CritFork from modifier, which have to support:

Double roll on Critical Chance

(same as already implemented Lucky critical, but after Lucky happens)
50% base chance -> 75% odds of hitting single critical strike with Forking
50% base chance -> 75% of hitting with Lucky -> 93,75% of hitting with Fork/Lucky
Lucky should be first as each Fork has separate Lucky roll if both modifiers are together.

Multiplying Crit Multplier on forking Crits

if both rolls succeed, then multiplier is doubled:
e.g. 50% odds for fork -> half of crits are 300 multi, other half is 200 multi (assuming default multiplier)

Implementation calculates it using average Crit Multiplier (for example above, it would be 250):
(critchance before fork) * (critchance before fork) / (critchance before accuracy penalty) -> average MORE multiplier provided by forking, e.g. 50% base -> 25% for fork hit/25% more crit multiplier

Correction for accuracy must be made due to calculating only for critical hits (other dont have crit multiplier, so they're irrelevant for average), so avg. crit multplier stays same as intended, despite hit chance changing (and, due to it, effective crit chance).

Unresolved

  • it may be useful to have displayed chance for double-criting in skill stats on left (as its in-game)
  • unsure if critical multiplier should have "average" clause for clarity when fork flag is appearing
  • implementation most likely could be cleaner, as currently there is both before-fork and before-accuracy critchance in output
  • still need to test if everything works properly as I am not that familar with codebase at the moment

Link to a build that showcases this PR:

https://maxroll.gg/poe2/pob/4f59g0on

After screenshot:

image
image

Antaresque and others added 7 commits April 29, 2025 00:09
The forked effective crit multi was using the preHitCheckCritChance value for some reason which was skewing the values from being correct
e.g. With 50% crit chance you have a 25% chance for your crit to hit to be a forking crit not 33.33% chance that it was calculating
The game uses the effective crit chance when calculating the chance for lucky or forked crits so we need to incorporate the effect from accuracy earlier
@LocalIdentity LocalIdentity marked this pull request as ready for review May 6, 2025 19:45
@LocalIdentity LocalIdentity added the enhancement New feature, calculation, or mod label May 6, 2025
@LocalIdentity LocalIdentity changed the title Add initial implemention for forking Critical Strikes Add support for Forking Critical Strikes May 6, 2025
Copy link
Contributor

@LocalIdentity LocalIdentity left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made quite a few changes to the PR to fix some issues surrounding hit chance. I also added a breakdown for the chance to get a forked crit

@LocalIdentity LocalIdentity merged commit bd68476 into PathOfBuildingCommunity:dev May 6, 2025
1 of 2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature, calculation, or mod
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add Tangletongue crit “forking” to damage calculations
2 participants