Skip to content
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

Implement explosive trap +-30% base tertiary radius, fix overlap chance for transfigured version #7384

Merged

Conversation

Edvinas-Smita
Copy link
Contributor

Fixes #6909 and #7260 .
Supersedes #7321 .

Description of the problem being solved:

Explosive Trap of Shrapnel was using wrong variables for it's overlap calculations and producing incorrect values.
The +- 30% base radius on both explosive traps was not yet implemented leading to overestimating the damage output.

Steps taken to verify a working solution:

  • The "Average explosions hitting" setting produces a bit lower overlap chance for normal version and not 100% for trans version

Link to a build that showcases this PR:

https://pobb.in/HFrrt77bxIjn

Before screenshots:

image
image
image
image

After screenshots:

image
image
image
image

@Paliak Paliak added the bug: calculation Numerical differences label Mar 11, 2024
@Puddlestomper
Copy link
Contributor

I'll just note here that there's a slight inaccuracy in the calculations.

The PR first finds the average radius and then uses that to calculate the overlap chance, but actually you should calculate the overlap chance for each possible tertiary radius and then take the average of those chances (weighted by the chance for that tertiary radius to occur). These are not mathematically the same.

I ran some python experiments to see what difference it makes. It seems to sometimes overestimate and sometimes underestimate depending on increased AoE and boss size. It makes up to a 10% difference in overlap chance (this is with no AoE modifiers and boss radius 3 or 5). I don't want to make the post too long so I won't include the code but I can send it if asked.

@LocalIdentity LocalIdentity merged commit bb1c825 into PathOfBuildingCommunity:dev Mar 22, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug: calculation Numerical differences
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Explosive Trap is calculated inaccurately.
4 participants