Skip to content

Fix Scorching Ray exposure not applying when using calculate maximum sustainable stages option#5164

Merged
Wires77 merged 1 commit intoPathOfBuildingCommunity:devfrom
Paliak:modOrderingIssue
Dec 9, 2022
Merged

Fix Scorching Ray exposure not applying when using calculate maximum sustainable stages option#5164
Wires77 merged 1 commit intoPathOfBuildingCommunity:devfrom
Paliak:modOrderingIssue

Conversation

@Paliak
Copy link
Contributor

@Paliak Paliak commented Nov 2, 2022

Fixes #5162 .

Description of the problem being solved:

Scorching ray applies exposure when at maximum stages. 222a1c5 moved the logic that calculates maximum sustainable stages for a few skills to after buffs/debuffs are applied to take arcane surge cast speed buff into consideration causing mods that use stage count as thresholds to not be added when they should.

mod("FireExposure", "BASE", nil, 0, 0, { type = "GlobalEffect", effectType = "Debuff", effectName = "Fire Exposure", effectCond = "ScorchingRayMaxStages" }),

Before screenshot:

obraz

After screenshot:

obraz

@QuickStick123 QuickStick123 added bug Something isn't working bug: calculation Numerical differences and removed bug Something isn't working labels Nov 2, 2022
@Wires77
Copy link
Member

Wires77 commented Dec 9, 2022

Is there a reason we didn't reuse the buff calculation logic from ~line 1818 for this? Seems like we could've popped that into a function and recalculated for those certain skills.

Edit: Discussion in Discord answered this:

[7:59 AM]Paliak: Couldn't think of a better way without rewriting a good chunk of CalcPerform.
[7:59 AM]Paliak: lots of mods get processed sequentially and that sometimes breaks things.
[7:59 AM]Paliak: kinda like with the gems thing that i made a pr for some time ago.
[7:59 AM]Paliak: where due to the order of processing them a flag was not being applied
[8:00 AM]QuickStick123: Why do you need to merge the buff into those list as well? 
[8:00 AM]QuickStick123: Or is it just newly added ones.
[8:00 AM]Paliak: it's just new ones
[8:00 AM]Paliak: it's in case something needs them
[8:01 AM]QuickStick123: Fair enough it seems to work.
[8:01 AM]Paliak: Yeah i know it's ugly but we'd need some sort of callback based system for processing this kind of stuff to make it nicer.

@Wires77 Wires77 merged commit 8e72eec into PathOfBuildingCommunity:dev Dec 9, 2022
@Wires77 Wires77 changed the title Fix Scorching Ray exposure not applying when using calculate maximum sustainable stages option. Fix Scorching Ray exposure not applying when using calculate maximum sustainable stages option Dec 9, 2022
@Paliak Paliak deleted the modOrderingIssue branch January 20, 2024 15:25
Paliak added a commit to Paliak/PathOfBuilding that referenced this pull request Jul 13, 2024
Essentially an improved version of
PathOfBuildingCommunity#5164 that
utilizes skill cache functionality.
LocalIdentity pushed a commit that referenced this pull request Jul 21, 2024
…ds (#7678)

* FIX: maximum sustainable stacks calculations ignoring certain buffs/mods

Essentially an improved version of
#5164 that
utilizes skill cache functionality.

* FIX: spelling

* TEST: add tests for schorching ray and blight interactions

* FIX: formatting

* DOCS: add fancy annotation for new function

* FIX: calculate the correct skill

* FIX: annotation

* FIX: use activation frequency for penance brand
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.

Scorching ray has incorrect behavior for the 'maximum sustainable stages' dropdown option

3 participants