Allow Apparatus of Khaz'goroth to be used at any stack size #918
Conversation
sim/common/cata/other_effects.go
Outdated
| buffAuraOnGain := func(aura *core.Aura, sim *core.Simulation) { | ||
| aura.SetStacks(sim, stacks) | ||
| character.AddStatsDynamic(sim, buffs) | ||
| } | ||
| buffAuraOnExpire := func(aura *core.Aura, sim *core.Simulation) { | ||
| character.AddStatsDynamic(sim, buffs.Invert()) | ||
| } | ||
|
|
There was a problem hiding this comment.
I think it will be cleaner to create a shared utility similar to NewTemporaryStatsAura(), which we can name something like NewTemporaryStatBuffWithStacks(). This helper could take in the label, spellID, max stacks, stat type, and stat bonus per stack, and then automatically register an OnStacksChange callback to perform the multiplication on the bonusPerStack and call AddStatDynamic() with the stat type. Then you simply register the three types of buff auras below with the helper, and you only need to call SetStacks() after Activate() within the trinket activation spell, without needing to do all the multiplications here.
Your version works fine, but making it reusable is cleaner in case there are other such trinkets in the future. Doing it this way also means you don't need to rely on outer scope variables for stacks and buffs, since everything you need will be contained within the Aura structs.
There was a problem hiding this comment.
In fact, there's already a MakeStackingAura() helper in sim/core/aura_helpers.go that you can take advantage of when creating the NewTemporaryStatBuffWithStacks() helper.
There was a problem hiding this comment.
Something like 4b011af ?
There was a problem hiding this comment.
Something like 4b011af ?
Sorry for the delayed review, just came back from vacation. Yes this looks much cleaner now, thanks! Feel free to merge it into master now, or alternatively I can merge it into the stats_refactor branch and fix the conflicts to ship the two changes together once the refactor is ready. Whichever way you prefer is fine, I'll have to fix merge conflicts on my end either way so doesn't matter to me.
There was a problem hiding this comment.
Seeing as the refactor is ready for review and the trinket still is "unusable" for rets until that one is merged, you can go ahead and merge it into stats_refactor and ship both 👍
There was a problem hiding this comment.
I've merged in the stats refactor today, so I'll now fix this PR and ship it either later today or tomorrow.
There was a problem hiding this comment.
I created a PR on your fork for merging in stats_refactor, once you approve that then I believe this current PR should be merge-able.
hillerstorm
force-pushed
the
remove_restrictions
branch
from
ceefca1
to
8db9ff8
Compare
hillerstorm
force-pushed
the
remove_restrictions
branch
from
8db9ff8
to
4b011af
Compare
On branch remove_restrictions Changes to be committed: modified: sim/common/cata/other_effects.go modified: sim/death_knight/frost/TestFrost.results modified: sim/death_knight/unholy/TestUnholy.results modified: sim/paladin/retribution/TestRetribution.results modified: sim/rogue/assassination/TestAssassination.results modified: sim/rogue/combat/TestCombat.results modified: sim/rogue/subtlety/TestSubtlety.results modified: sim/shaman/enhancement/TestEnhancement.results
Stats refactor
No description provided.