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

Fix Explosive Arrow Full DPS #5432

Merged
merged 1 commit into from
Aug 13, 2023
Merged

Conversation

AnSq
Copy link
Contributor

@AnSq AnSq commented Dec 22, 2022

Fixes #2486

Description of the problem being solved:

When "Include in Full DPS" is checked for Explosive Arrow, currently PoB doesn't give it any special treatment and simply takes the "Average Damage" (of an explosion) times Attack Rate (the rate that you apply fuses) to get DPS. The correct calculation is Average Damage times Explosion Rate. This change implements a calculation for Explosion Rate, shown as "Hit Rate" in PoB.

The time between explosions (HitTime) is either the skill duration, or the time it take to reach the fuse limit, whichever is lower. The time to max fuses is:

timeToMaxFuses = fuseLimit / (hitChance * attackSpeed * totemLimit * barrageProjectiles)

The calculation is done in explosiveArrowFunc(), which lives on the skill itself and kinda looks like a preFunc, but it needs access to information that's not available to the preFuncs. I also pulled the Maximum Sustainable Fuses calculation into this new function.

Screenshots

dps

Link to a build that showcases this PR:

This is the build used in the above screenshots: https://pobb.in/m4qGXNAjPT89

@AnSq
Copy link
Contributor Author

AnSq commented Dec 22, 2022

This relates to #2486 (though I'm still unclear exactly how).

@QuickStick123 QuickStick123 added the bug: calculation Numerical differences label Dec 23, 2022
@QuickStick123
Copy link
Contributor

QuickStick123 commented Dec 23, 2022

I think explosiveArrowFunc() would be a more reasonable name no need for weirdness

@AnSq
Copy link
Contributor Author

AnSq commented Jan 3, 2023

I think explosiveArrowFunc() would be a more reasonable name no need for weirdness

Good call.

This should also work for Barrage Support now, which fixes #2486.

@AnSq AnSq marked this pull request as ready for review January 3, 2023 23:46
@QuickStick123
Copy link
Contributor

You have some conflict you need to fix @AnSq.


local barrageProjectiles = nil
if skillModList:Flag(nil, "SequentialProjectiles") and not skillModList:Flag(nil, "OneShotProj") and not skillModList:Flag(nil,"NoAdditionalProjectiles") and not skillModList:Flag(nil, "TriggeredBySnipe") then
barrageProjectiles = skillModList:Sum("BASE", skillCfg, "ProjectileCount")
Copy link
Contributor

@Lilylicious Lilylicious Aug 13, 2023

Choose a reason for hiding this comment

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

Suggested change
barrageProjectiles = skillModList:Sum("BASE", skillCfg, "ProjectileCount")
barrageProjectiles = skillModList:Sum("BASE", activeSkill.skillCfg, "ProjectileCount")

Without specifying the config properly, you're missing some projectiles.

@LocalIdentity LocalIdentity merged commit d04fadc into PathOfBuildingCommunity:dev Aug 13, 2023
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.

Full DPS for explosive arrow might be incorrect
4 participants