PathOfBuilding icon indicating copy to clipboard operation
PathOfBuilding copied to clipboard

Fix numerous issues in bleed calculations

Open CelestiaTheDryad opened this issue 1 year ago • 3 comments

  • Chance to bleed was not applied
  • Bleed stacks and stack potential was overwritten with incorrect values at times
  • Bleed stack potential was incorrectly applied to per-stack bleed damage
  • Multiple damage formulas were used simultaneously
  • Output from multiple formulas did not align with numbers displayed to user

Fixes #7575,

Description of the problem being solved:

I noticed that chance to bleed was not being applied, which was greatly inflating the worth of the Crimson Dance keystone.

While implementing the fix for that issue, I saw several other issues in the bleed damage calculation, so I fixed those as well.

Steps taken to verify a working solution:

  • Generated several builds with a variety of bleed-affecting stats and checked the calculation output was sensible
  • Sanity checked 0% bleed, 1% bleed, and 100% bleed calculations via debugger
  • Checked that increases to bleed chance resulted in DPS gains

Link to a build that showcases this PR:

https://pobb.in/aDMlMEJAyZyc

Before screenshot:

image image

After screenshot:

image image

CelestiaTheDryad avatar Jul 22 '24 03:07 CelestiaTheDryad

Do you think you'd be able to add a couple of tests around this code? Would be great to make sure we don't regress if someone touches it in the future.

Wires77 avatar Jul 22 '24 03:07 Wires77

I would like to, but I don't know how to do that. This is the first time I've written lua code or worked on PoB. As well the bleed calculations are buried in a 5000 line function that clearly takes a lot of setup to run.

CelestiaTheDryad avatar Jul 22 '24 04:07 CelestiaTheDryad

I would like to, but I don't know how to do that. This is the first time I've written lua code or worked on PoB. As well the bleed calculations are buried in a 5000 line function that clearly takes a lot of setup to run.

The tests we run are more like integration tests, where you can set up the build and just see if the output looks correct. If you want to take a crack at it take a look at some of these files for examples: https://github.com/PathOfBuildingCommunity/PathOfBuilding/blob/dev/spec/System/TestAttacks_spec.lua

Wires77 avatar Jul 22 '24 04:07 Wires77

I rebased to get the changes from the recent updates that caused a conflict. I also did another pass on the code changes, trying to minimize changes from earlier code. I was a bit overzealous on restyling the first time.

I created one test, which I hope works. I was having difficulty getting luarocks and busted to work so I can't run it locally.

Another good test to write would be to check that allocating crimson dance gives sensible results with <8 active bleeds. I couldn't find examples about how to manipulate the skill tree.

CelestiaTheDryad avatar Jul 23 '24 06:07 CelestiaTheDryad

Yeah, busted can be annoying to set up on windows to say the least. There is a docker command if you have that installed, but otherwise I can run the tests later.

I'll also look into allocating nodes on the tree, but you might also just be able to add the text from crimson same to the custom mod box in the config tab. There are examples of that

Wires77 avatar Jul 23 '24 11:07 Wires77

Tree allocation tests would also be useful for scion pathing tests. Shouldn't be too hard to get it working. https://github.com/PathOfBuildingCommunity/PathOfBuilding/blob/87b8ddccc2d648ada5642a440d56ed489c85b0ac/src/Classes/PassiveSpec.lua#L663-L666

Paliak avatar Jul 23 '24 12:07 Paliak

Fixed the test, I think it should run as part of CI on this PR now if you wanted to add more, but I'll fully review it later in either case.

Wires77 avatar Jul 23 '24 13:07 Wires77

I was having a ton of issues with bleed so I googled and found this which explained a lot of them, but after merging the code locally I still have a lot of other issues, and I'm curious if it's something I'm missing or another issue with the proposed changes.

The issues I'm having seem to be related to Perfect Agony also calculating bleed chance strangely -- if I apply Perfect Agony while having this fix in, my bleed chance/damage drops to 0% despite having crit chance.

This could absolutely be a Me Problem since my build could just be bad, but I just wanted to let you know that you should doublecheck if PA is working correctly with your changes 👍

Edit: Ah, I see the comment up above now..! Oop.

Jewelots avatar Jul 24 '24 13:07 Jewelots

I fixed the issue with Perfect agony and applied the same fixes to poison and ignite (less egregious errors there, but still).

After doing so, I have three questions that should be resolved before this PR is merged.

  1. Is dot cap applied per-dot-type, or is it applied per-dot-stack? The code seems to apply dot cap both ways and it should be unified.
  2. There's a custom mod being added for SinglePoison and SingleBleed. But not one for ignite. I assumed this was a marker that's an output of the calcs tab, but want to make sure there's not an affect in the game which limits the stacks of ailments applied to an enemy.
  3. Poison stacks are handled weirdly, in that they are applied to the Full Poison DPS in the sidebar, but not to the Poison DPS in the calcs pane. I left this alone for now, but I think stacks should be integrated in the calcs pane. Stacks are applied here: https://github.com/PathOfBuildingCommunity/PathOfBuilding/blob/8bea7bd77822ac8bbab876d49e1a8181e217581c/src/Modules/Calcs.lua#L321

CelestiaTheDryad avatar Jul 25 '24 06:07 CelestiaTheDryad

  1. the dot cap should be applied at every stage, it applies to each individual dot, to the sum of all dots of that type (eg all bleeds), and to the sum of all dot damage in the build,

I assumed this was a marker that's an output of the calcs tab

~~yes that is the case, however related I think you moving where the poison stacks are capped to after where base damage is calculated does mess up the poison calcs for low tolerance (the 300% inc damage if they have no poisons on them) which the config was added for in the first place, but havnt tested~~ edit never mind, its the condition thats used for that, and works fine

  1. its also on the sidebar contained within total dot dps, (not just for full dps), I agree it would be a good idea to have it on the calcs tab as well for easy reference

Regisle avatar Jul 25 '24 06:07 Regisle

~~actually wait question, isnt chance is factored in here https://github.com/PathOfBuildingCommunity/PathOfBuilding/blob/dev/src/Modules/CalcOffence.lua#L4952 + https://github.com/PathOfBuildingCommunity/PathOfBuilding/blob/dev/src/Modules/CalcOffence.lua#L1846-L1906 ?~~ edit: again me jumping ahead, its not becouse its using the chance calculated within the section you are

Regisle avatar Jul 25 '24 06:07 Regisle

s/Seperator/Separator/g

(why do i remember vi still?)

Nightblade avatar Jul 25 '24 09:07 Nightblade

s/Seperator/Separator/g

(why do i remember vi still?)

Clearly you don't remember it well enough! It's :%s/... :)

Rybadour avatar Jul 25 '24 23:07 Rybadour

This PR effectively redoes #7954. The fix is applied to all 3 dot types and is integrated with the breakdown text.

CelestiaTheDryad avatar Jul 26 '24 05:07 CelestiaTheDryad