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

Added support for Doom Blast (from Impending Doom) #5530

Merged
merged 8 commits into from
Apr 12, 2023

Conversation

ha107642
Copy link
Contributor

@ha107642 ha107642 commented Jan 3, 2023

  • Now shows DPS instead of average damage
  • You can now pick overlap count (for spell cascade)
  • Support gems no longer get levels from "+# to Level of Active Skill Gems" and similar
  • getTriggerActionTriggerRate() no longer adjusts to tick rate for skills that can store several uses (which Doom Blast can)
  • Added a config checkbox for Vixen's Entrapment to cap Doom Blast trigger rate when cast speed is too high

Description of the problem being solved:

Impending Doom was working incorrectly in several aspects.

Steps taken to verify a working solution:

Moderate ad hoc testing while developing.

Link to a build that showcases this PR:

https://pastebin.com/8ZHzwwGb

Before and after screenshots:

https://imgur.com/a/z9h2U6E

* Now shows DPS instead of average damage
* You can now pick overlap count (for spell cascade)
* Support gems no longer get levels from "+# to Level of Active Skill Gems" and similar
* getTriggerActionTriggerRate() no longer adjusts to tick rate for skills that can store several uses (which Doom Blast can)
* Added a config checkbox for Vixen's Entrapment to cap Doom Blast trigger rate when cast speed is too high
@@ -262,6 +276,7 @@ end
function calcSkillCooldown(skillModList, skillCfg, skillData)
local cooldownOverride = skillModList:Override(skillCfg, "CooldownRecovery")
local cooldown = cooldownOverride or (skillData.cooldown + skillModList:Sum("BASE", skillCfg, "CooldownRecovery")) / m_max(0, calcLib.mod(skillModList, skillCfg, "CooldownRecovery"))
-- TODO: Do not round to tick rate for skills that store multiple uses!
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't know where to fetch this data. Many skills seem to get a property for multiple uses upon extraction, but Doom Blast does not. At least not in sup_int.lua.

Maybe it is possible to fix that somehow, so we can use that property?

Right now, the cooldown displayed for Doom Blast still takes tick rate into account, but the DPS calculations do not.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think the number of cooldown stat that can be found in multiple spots e.g. warcries is manually added I am not sure if that data is supplied. You can add it yourself if you like and use it. Second wind for example is implemented but doesn't really work on much stuff. Multi cooldown traps and flame dash for example aren't implemented.

src/Modules/CalcPerform.lua Outdated Show resolved Hide resolved
@QuickStick123 QuickStick123 added the enhancement New feature, calculation, or mod label Jan 4, 2023
@QuickStick123
Copy link
Contributor

QuickStick123 commented Jan 4, 2023

It was also raised by someone that you can get more than 2 overlaps e.g. the number they suggested some one getting was 4 so you might wish to update this to be a skill stage box / number box instead of skill parts.

* Overlap now instead uses stages so that the user can set any overlap count
* Updated Vixen's Entrapment item text
* "Use Vixen's Entrapment cooldown?" is now automatically checked when equipping the gloves
@ha107642
Copy link
Contributor Author

ha107642 commented Jan 4, 2023

Alright, I made some updates:

Impending Doom: Reverting changes to getTriggerActionTriggerRate() and made it specific to Doom Blast only.

Impending Doom improvements:

  • Overlap now instead uses stages so that the user can set any overlap count
  • Updated Vixen's Entrapment item text
  • "Use Vixen's Entrapment cooldown?" is now automatically checked when equipping the gloves

image

I think that should address all issues. Thank you guys for the help! =)

src/Modules/CalcPerform.lua Outdated Show resolved Hide resolved
src/Modules/CalcPerform.lua Outdated Show resolved Hide resolved
src/Modules/CalcPerform.lua Outdated Show resolved Hide resolved
@Paliak
Copy link
Contributor

Paliak commented Jan 5, 2023

The cooldown hard being hard coded sort of bothers me. You could convert the parsing for the mods on vixens to use the function format like other parsing entries around it and add a BASE mod with a name like vixen's cooldown or something. Put it right under where you put the flag and then just moddb:sum for the cooldown in the calcs.

@ha107642
Copy link
Contributor Author

ha107642 commented Jan 6, 2023

The cooldown hard being hard coded sort of bothers me. You could convert the parsing for the mods on vixens to use the function format like other parsing entries around it and add a BASE mod with a name like vixen's cooldown or something. Put it right under where you put the flag and then just moddb:sum for the cooldown in the calcs.

Yeah, you are right. I was being lazy with that since I did not know how to add it. Luckily, it was easy. =)
I updated the code!

@Paliak
Copy link
Contributor

Paliak commented Jan 7, 2023

Looks good but i'd change the name TriggerCurseOnCurseCooldown => VixensCurseOnCurseCooldown since it's specific to Vixens.

Paliak added a commit to Paliak/PathOfBuilding that referenced this pull request Jan 7, 2023
@ha107642
Copy link
Contributor Author

ha107642 commented Jan 8, 2023

Alright, I updated it now. :)

Though I'm not sure I agree, since the line could technically appear on other equipment in the future. But then again, the flag is named Condition:HaveVixensEntrapment.

@Paliak
Copy link
Contributor

Paliak commented Jan 11, 2023

The code overall mentions vixens too much to worry about making it general for now.

I've implemented proper handling for vixens and tried to copy your pr to add support for Doom Blast for #4599 so it be great if you could take a look and check if the calculations work as expected.

@QuickStick123
Copy link
Contributor

#5537 has been merged which adds support for skill uses if you wanted to try a more general approach.

return cooldown
-- If a skill can store extra uses and has a cooldown, it doesn't round the cooldown value to server ticks
local rounded = false
if (skillData.storedUses and skillData.storedUses > 1) or (skillData.VaalStoredUses and skillData.VaalStoredUses > 1) or skillModList:Sum("BASE", skillCfg, "AdditionalCooldownUses") > 0 then
Copy link
Contributor

Choose a reason for hiding this comment

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

You could probably move the stored uses part before this so you could just have.

Suggested change
if (skillData.storedUses and skillData.storedUses > 1) or (skillData.VaalStoredUses and skillData.VaalStoredUses > 1) or skillModList:Sum("BASE", skillCfg, "AdditionalCooldownUses") > 0 then
if (skillData.storedUses and skillData.storedUses > 1) or (skillData.VaalStoredUses and skillData.VaalStoredUses > 1) then

@Nightblade
Copy link
Contributor

(AnnointOnly and AtlasnodeGroup added to spellchecker's ignore dictionary)

@LocalIdentity LocalIdentity merged commit 0d3e614 into PathOfBuildingCommunity:dev Apr 12, 2023
LocalIdentity pushed a commit that referenced this pull request Jun 2, 2023
…d issues. (#4599)

* FIX: Change the Kitava's Thirst trigger mod description to one that is used in game and add parsing for it.

* FIX/FEAT: Correct trigger calculations for Kitava's thirst and Craft trigger. Improve handling of skill cooldown overrride. Add handing for multipler triggers supporting one skill

* FIX: Readd a commit lost during rebase that fixes cooldown override.

* FIX: Handling of Focus triggers. Triggers assume perfect focus reuse as self casting the focus skill is the only way to obtain focus currently.

* FIX: Fix cooldown recovery not applying when using cooldownoverride.

* FIX: Fix crashing due to incorrect handling of minion trigger rate calculations.

* FIX: Correct calculations for cwdt. Account for possible issues resulting from skillData.cooldown working differently.

* FIX: Correct potentially wrong values for "SummonRigwaldsPack"

* FEAT: Improve source skill selection for Craft trigger

* FIX: Fixing typos and formatting

* FIX: Specific handling for Cast When Damage Taken

* FIX: Fix max duration not being displayed properly for cwdt

* FIX: Handle Cast on Melee kill

* FEAT: Add basic support for global triggers

* FEAT: Add basic support for spellslinger

* FEAT: Add support for Mark on hit

* FIX: Improve trigger source selection requirements for a variety of trigger skills

* FEAT: Add basic handling for counter attack skills with internal triggers

* FEAT: Add breakdown for arcanist brand casts per second calcualtion. Simplify arcanist brand related math. Remove redundant s_format calls

* FEAT: Some minor refactoring

* FEAT: Refactoring and add support for more unique triggers

* FIX: Change skill stat for cwdt

* FIX/FEAT: Add support for more uniques and hextouch. Minor refactoring. Fix crash when uniqueTriggerName is nil.

* FEAT: Add support for Tawhoa's_Chosen

* FIX: minor cleanup

* FEAT/FIX: Small improvements to error messages and breakdowns. Fix edge cases with in Tawhoa's Chosen implementation that caused crashes.

* FEAT/FIX: Implement Replica Maloney's mechanism. Add extra checks for minion trigger source selection.

* FIX: Cleanup and refactoring

* FIX: Restrict Tawhoa's Chosen to attack slams not just any slams

* FIX: fix trigger source location finding not working for global triggers

* FEAT: Add parsing for chance to trigger. Minor cleanup. Skill re export.

* FIX: fix multistrike crash caused by bad export

* FIX/FEAT: Add recently merged to dev handling for a new flag. Fix some uniques not working due to last skills export.

* FEAT: Improve breakdowns when SpellCastTimeAddedToCooldownIfTriggered is set

* FEAT: More improvements to breakdowns

* FIX: if cast time is added to trigger cooldown it should account for cast time modifiers

* FIX: disable  unseen strike when not phasing

* FEAT: Add proper support for CWC and make it work with lightning conduit

* FIX/FEAT: Add better support for addsCastTime to other triggers. Minor cleanup

* FIX: more consistent variable names.

* FIX: minor cleanup

* FIX: fix crash fix arakali's fang using old var

* FIX: fix unleash trigger source handling

* FIX: remove some redundant code.

* FIX: arcanist brand used cooldown for speed.

* FIX: arcanist brand breakdown

* FIX: Apply QuickStick's arcanist brand formula.

* FIX: handling of arcanist brand

Now should correctly calculate the impact of multiple linked skills

* FIX: minor breakdown formatting fix

* FIX: stop tawoah's chosen from using totems

* FIX: Use the modlist of the actual brand gem.

Arcanist brand has a hidden support gem that supports linked gems and
causes them to trigger but most the of important stats are in the mod
list of the actual main gem os use that instead.

* FIX: better support TriggerDamage mod.

Should properly handle TriggerDamage mods such as the ones on arcanist
brand. Also fixed arcanist brand config option requiring hit flag.

* FIX: trigger damage mods not working for cwc

* FIX: fix missing trigger name for holy relic

* WIP: major renaming.Changed order of operations.

* WIP: more work on reordering application of mods

* WIP: Minor fixes for self triggers

* WIP: fixes to Tawhoa's Chosen

* FIX: prevent the saviour from using totem skills

* FIX: icdr not being considered for alignment

* FIX: update Cwc/Focus to new format

* FIX: misspelled word

* FIX: fix kitava's thirst calcs

* FIX: add src rate is effective switch

Used for Atziri's rule

* FIX: breakdown ordering issue

* FIX: remove dead code

* FIX: self trigger calcs

* FIX: hextouch support

* FIX: minor fixes to unique various triggers

* FIX: spelling issues

* FIX: arcanist brand improvements

* WIP: implement faster skill rotation simulator

Based on:
#5428

* WIP: remove unnecessary for loop

* FIX: lower sim resolution, implement ceil func with base

* FIX: fix Tawhoa's Chosen

* FIX: off by one error due to different indexing

* FIX: fixed to new simulator

* FIX: minor formatting. Comments.

* FIX: remove helper func. Minor tweaks.

* FIX: spelling issues.

* FIX: change func name.Awaiting Dict update.

* FIX: adapt Cwc to work with new simulation.

* FIX: breakdown clarity. icdr fixes.

* FIX: use ceil_b and floor_b.

* FIX: crash due to no socketGroup

* FIX: hexes applied with hextouch have no mana cost

* FIX: hextouch should have no cost

* FIX: code formatting

* FEAT: implement vixens trigger.Minor fixes

* FIX:no sim for self triggers.Smarter breakdowns

* WIP: implement Impending doom.

Based on #5530

* FIX: fixes to impeding doom breakdown

* FIX: Tawhoa's Chosen simulation arguments

* FIX: triggered minion skills not using icdr.

If minions skills such as summon skeletons were triggered they would not
use player icdr mods due to using the wrong actor.

Additionally renamed some variables.

* WIP: initial work on CWDT loops support

* FIX: spelling

* FIX: global trigger not working as sources.

Use actor variable more often to improve minion skill support

* FIX: cd overrides should override trigger cd too.

* FIX: ignore dualwield impact if source is global

* FIX: limiting scope to self damage only.

CWDT loops while a really cool feature are hard to support correctly in
POB. Due the amount of assumptions the involved code has to make
regarding the state of the loop, the numbers aren't always accurate.
Staring with this commit the scope is being limited to only add support for
self hit damage as i think it'd be a waste to throw all that code away
and that part should work perfectly fine.

* FIX: arcanist brand being affected by tickrate

* FIX: uuid collision and remove confusing breakdown

* DOCS: add coomment.

* WIP: mixed group setups and two part skills fix

Currently there's an issue with two part skills (active and support)
such as Arcanist Brand where the support part will not be applied to
skills socketed into the same item but in different groups. An example
would be to two groups both socketed into Weapon 1. One having the
Arcane Brand skill gem and the other any other skill. The other skill
will not be supported by Arcane Surge even though they are socketed into
the same item and should be considered as linked.

* FIX: spelling

* FIX: add handling for shotgunning skills

* FIX: null skillpart and copy skill limit recursion

* FIX: hide trigger supports if not compatible

* FEAT: impl Battlemage's cry trigger

* FEAT: impl trigger rate for Combust

* FIX: skills from items not counting as sources

* FIX: remove unused flag

* FIX: unique triggers broken by wrong condition

* FEAT: basic support for skills with charges

* Add support for Prismatic Burst

* FEAT: hook up prismatic burst to trigger logic

* FIX: rename kitava's trigger and add threshold

* FEAT: comparer func + documenting comments

* FIX: spelling

* FEAT: hook up shockwave to trigger logic

* FIX: avoid scoping wierdness and remove debug log

* WIP: manaforged support

* FEAT: finish implementaion of manaforged

* FIX: red text on imepending doom

* FIX: merge issue

* FIX: minor issues with spellslinger

* FIX: mirager archer crash caused by merge

* FIX: small cleanup

* FIX:remove remenants of old handling for self trig

* FEAT: impl triggerbots

* FIX: better tooltip

* FIX: cleanup kitava's thirst cond

* FIX: spelling

* FIX: remove non trigger related changes

* WIP: inital work on moving trigger code

* WIP: fix arcanist brand crash

The defualt trigger handler still needs to be modified to work with the
new config format

* FIX: variety of new config compatibility issues

* FIX: trigger info issues

* FIX: variable names, refactoring

* FIX: missing variables

* FIX: mark mirage archer as a trigger

* FIX: missing actor variable

* FIX: CWC and added cast time breakdowns

* FIX: remove redundant limited processing check

* FIX: tweaks to Queen's Demand

* FIX: better feedback for COMK

* FIX: assumingEveryHitKills is not in config

* FIX: Awakened triggers not finding configs

* FIX: mark as not a triger when no config found

* FIX: ignoresTickRate being overwritten

* FIX: self trigger display name crash

* FIX/FEAT:impl vengance, use cap rate for self trig

* FIX: prevent battlemeage's cry self trigger

* FIX: hextouch shoulnd not be a global trigger

* FIX: misc fixes

---------

Co-authored-by: Wires77 <Wires77@users.noreply.github.com>
Dullson pushed a commit to Dullson/PathOfBuilding that referenced this pull request Dec 6, 2023
…munity#5530)

* Added support for Doom Blast (from Impending Doom)
* Now shows DPS instead of average damage
* You can now pick overlap count (for spell cascade)
* Support gems no longer get levels from "+# to Level of Active Skill Gems" and similar
* getTriggerActionTriggerRate() no longer adjusts to tick rate for skills that can store several uses (which Doom Blast can)
* Added a config checkbox for Vixen's Entrapment to cap Doom Blast trigger rate when cast speed is too high

* Impending Doom improvements:
* Overlap now instead uses stages so that the user can set any overlap count
* Updated Vixen's Entrapment item text
* "Use Vixen's Entrapment cooldown?" is now automatically checked when equipping the gloves

* Impending Doom: Reverting changes to getTriggerActionTriggerRate() and made it specific to Doom Blast only.

* Impending Doom: some cleanup.

* Parse the cooldown on Vixen's Entrapment rather than hard code it.

* TriggerCurseOnCurseCooldown -> VixensCurseOnCurseCooldown

* Stop rounding of cooldown to server ticks if skill has extra uses

---------

Co-authored-by: ha107642 (Henrik A) <>
Co-authored-by: LocalIdentity <localidentity2@gmail.com>
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.

5 participants