Seperate skill cooldown from trigger cooldown and fix multiple related issues.

[Paliak]

trafficstars

Fixes https://github.com/PathOfBuildingCommunity/PathOfBuilding/issues/2444 , #4107 , #4210 , #3558, #4959, #4679

Description of the problems being solved:

Separation of Skill cooldown and trigger cooldown in CalcPerform. Correct calculation of triggers for Kitava's Thirst and Craft trigger with improved source selection for the latter. Change Kitava's Thirst to use the in game trigger mod and add parsing for it. Improved handling of hard skill cooldown overrides. Improved handling of a gem being supported by more than one trigger. Adds isTriger property to skills that can trigger other skills. Adds triggeredBy property to skills triggered. Handling of helmet focus trigger (focus trigger now assumes the player recasts focus when the cooldown ends). Correctly account for the fact that some skills have no cooldown in calcMultiSpellRotationImpact. Attack rate calculation for counter attack skills. Fix handling of hard skill cooldown overrides (Discharge). Add support for more unique triggers. Refactoring of trigger code. Improved breakdowns for skills where average trigger rate is affected by accuracy and or crit chance.

The changes are extensive and there may be more edge cases that i was not able to find.

Steps taken to verify a working solution:

• Check builds from poe.ninja
• Test build: https://pobb.in/GM-W5q3HkJq7
• Test build2 https://pobb.in/9ADoZbUn-lQK

Live vs pr side by side pictures:

[Paliak]

~~Found issue. When there's no triggering skill the triggered skill is assumed to be cast by the trigger not self cast. Fixing.~~

Fixed by: #5b6ebd9e143a8b7540dd2fb95aa454ca24d81ba7

[Paliak]

~~Cast on melee kill and cast on stun not working correctly. Should behave closer to CWDT. Cast on Damage Taken currently doesn't take skill rotation into account. Fixing.~~

Should be Fixed.

Basic support for global triggers added.

[QuickStick123]

This skill isn't really implemented. This skill is supposed to trigger Tawhoa's Chosen which summon's a Mirage Chieftain that attacks once with the given slam that triggered it with no echo/repeat. https://github.com/PathOfBuildingCommunity/PathOfBuilding/pull/4732 Will fix this crash by dropping support.

[QuickStick123]

There seems to be large differences in simulated impact of spells between master and this I asumme this is intended? https://pastebin.com/gsBUpSN1

[Paliak]

The simulation incorrectly used the cooldown of the trigger for simulation as the cooldown of the triggered skill (main point of this pr is to separate the values). So basically currently in release the simulator thinks that eye of winter has a cooldown of 0.15s (same as awakened cast on crit it's linked to) in the build you linked, which afaik is incorrect, thus the difference.

[Lilylicious]

Currently cast while channeling does not apply the less damage on the gem to the triggered skill.

[Lilylicious]

I can't find anything else obviously wrong, so tentative LGTM.

[Paliak]

While writing the rundown i did some more testing and i think i've failed to address an issue that exists in current release version of pob. Modifiers to effective source attack speed are applied after skill rotation is taken into consideration. This causes slight differences in effective trigger rate as the simulator assumes that all attacks cause the skills to trigger; Which changes the timing. I will try to fix this as soon as possible.

If this is to be merged i think it'd be best to merge it into beta for now. That way i could get more feedback from users and it won't break things for the majority of users getting ready for the new league.

[Nightblade]

(Not sure what's going on with some of those old spelling errors... you may need to freshen your dev snapshot on this branch?)

[Paliak]

The new calcMultiSpellRotationImpact still seems to produce wrong values. ~~It's likely still caused by lua being 1 indexed because the order in which gems appear in a group changes the values returned.~~

~~it seems the trigger rate of skills with cooldown is correct before division by skillcount.~~ ~~Arc: [1]:9.5904559155197~~ ~~Frost Bomb [2]:0.39016777214202~~ Fixed. Was an indexing error.

@ProphetLamb I think i found a case where it's hitting ~20% inaccuracy

Frost bomb is within margin but the difference of 1.0/s on Ice Spear is a bit much. (aps around 5.3)

I'm starting to wonder whether the original simulation is accurate.

Given 5.3 aps and .15 trigger cooldown not all attacks will cause the trigger to trigger since it will be on cooldown some of the time. Then if the triggger rate of the trigger is < 5.3/s then the trigger will trigger each skill < (5.3 / spellcount). Some skills will cap out on their trigger rate due to their cooldown but the rest will use up all of it.

Though this is also a different value than the python version produces so honestly not sure at this point.

Also note that the original simulation was never meant to work with skills that have cd of 0. It was designed with the assumption that cooldown is inherited. So effectively skill.cd = max(skill.cd, trigger.cd). (Looking at the python code again this seems to be accounted for.)

[Paliak]

Fixed the indexing issues. The lua implementation should now behave like the python one. The 20% error is still concerning though. Would be great if you could take a look @ProphetLamb

[ProphetLamb]

Fixed the indexing issues. The lua implementation should now behave like the python one. The 20% error is still concerning though. Would be great if you could take a look @ProphetLamb

The reduction in triggerrate of the fast skill (without cd) is because of missed triggers, where the triggerrate doesn't align (exhibits resonance) with the skill. In those cases the activation is delayed until the next attack.

The effect of this phenomenon on the slow skill (with cd) is about equal in penalty, but because of the lower triggerrate virtually invisible.

In the original simulation this is not considered. Therefore the trigger rate is higher and without the resonant peaks.

Iirc this interaction has not previously been handled anywhere in PoB, but I could be mistaken.

The source of these missed triggers is that upon activation (e.g. by attacking) the trigger always executes the next skill in order, regardless of cooldown or cast speed etc.. if that skill is on cooldown it will be triggered, but not activate. Despite this the trigger moves onto the next skill. The trigger itself is not on cooldown, since the activation failed.

From the perspective of the trigger rate this causes a delay untill the trigger is ready again, which is dependent on the attackspeed.

This delay can never occur when skills have no cooldowntime (which virtually all meta-builds have), so we never noticed that the triggerrate in PoB was to high.

[QuickStick123]

Arcanist Brand doesn't seem to work on item granted skills. https://github.com/PathOfBuildingCommunity/PathOfBuilding/issues/1065

[Paliak]

@QuickStick123 The issue seems to be with all two part skills (Arcanist brand has support part and active part). Putting Arcanist Brand into one group and any other active spell into another group and socketing both groups into the same item results in the support part of the two part skill not being applied to the active skill in the other group even though it's socketed into the same item. I've fixed it in 1f88b70 but i'm really not sure if that's how it should be done and whether or not it breaks some other interaction.

[QuickStick123]

Seems like a pretty small change might be nice to put it in a separate PR so it gets merged quicker?

[QuickStick123]

Fixes https://github.com/PathOfBuildingCommunity/PathOfBuilding/issues/3815 But name is too long.

[QuickStick123]

https://github.com/PathOfBuildingCommunity/PathOfBuilding/issues/3035 https://github.com/PathOfBuildingCommunity/PathOfBuilding/issues/2558 https://github.com/PathOfBuildingCommunity/PathOfBuilding/issues/2424 https://github.com/PathOfBuildingCommunity/PathOfBuilding/issues/1757 Issues related to triggers I saw if you wanted to look at any of them.

[QuickStick123]

Any Trigger Skills on items that come from a skill gem (such as Rain of Arrows from Lioneye's Paws) now share the same cooldown group, so using the gem version of the skill should put the triggered version on cooldown or vice versa.

Not sure if this affects anything 3.21.

[Paliak]

I think this pr already does this but i'll have to double check.

[Paliak]

The simulation may not be correctly handling cool downs for multiple instances of the same skill in the trigger group. Need to do some more digging into that.

[QuickStick123]

There is also https://github.com/PathOfBuildingCommunity/PathOfBuilding/issues/5648 as well if you ever wanted to try and get that working. The data is stored now as that PR was merged.

[Paliak]

Should be easy to do as i'm already handling that for some skills.

[Paliak]

Few things to note.

Cool downs across groups are not supported for the same reason CWDT loops are not supported (too many assumptions).

Some triggers require all skills to be in the same group due to the way cross group supports are handled currently. There does not seem to be an easy way of fixing it without throwing graph theory at the problem.

Current simulation of skills in the same group seems to be correct, at least according to my current understanding of trigger skills.

[QuickStick123]

Is this in a state where it can be added to the beta branch for testing or not due to WIP state?

[Paliak]

@QuickStick123 I've kept it as a draft as i keep adding things but most of recent changes could've been separate prs if this were merged to beta.

The only thing left to implement really are trigger bots but that again could be a separate pr (but would require code from here to work so getting this merged to beta would be nice).

This being merged would also mean that there's less duplicate code to work on since both me and Nostrademous basically have to implement the same thing twice.

[Nostrademous]

@Paliak since the Manaforge Arrows PR was merged into dev, do you need to make any changes to this PR to not break functionality?

[Paliak]

@Nostrademous I re-implemented it here. https://github.com/PathOfBuildingCommunity/PathOfBuilding/pull/4599/commits/c6a2f122dc479d51c07d490471654b5979fa8538

This pr replaces all current trigger logic. You can test to make sure it behaves how you'd expect it to.

[Paliak]

This pr is ready to go into beta at 5a16766 but it has been asked of me to refactor trigger code into a seperate CalcTriggers.lua file.

TODO:

• [x] Move all trigger related code into a separate file
• [x] Refactor current else if tree into a hash map with a trigger config structure
• [x] Instead of building on top of existing code, restructure into something that is cleaner and easier to expand.

[Paliak]

I've moved trigger related code to a new file CalcTriggers.lua.

Some things are likely still broken but i've tested everything that makes sense to implement from https://www.poewiki.net/wiki/Trigger and everything should behave correctly.