-
Notifications
You must be signed in to change notification settings - Fork 45
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
Prot Paladin: Righteous fury + Hand of Reckoning #1064
Conversation
paladin.OnSpellRegistered(func(spell *core.Spell) { | ||
if spell.SpellSchool.Matches(core.SpellSchoolHoly) { | ||
spell.ThreatMultiplier *= 1.0 + rfThreatMultiplier | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't the bonus threat only apply if RF is active?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Or are we just assuming it's always active for prot I guess?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The function starts with a check to see if RF was enabled via the button in the UI spec options. Since Rets won't have the option on their UI, the OnSpellRegistered
won't get ever get evaluated for them (it will default to false in the options struct). For prot, it also works correctly if you turn it off in the UI.
Also note: In game, the passives for Hand of Reckoning are only active when you have Righteous fury on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative here is I could have the OnSpellRegistered
just append each spell to an array (e.g. paladin.holySpells
) and have the aura's OnGain
and OnExpire
apply/remove the threat multiplier on the list of spells. The disadvantage to that is you're doing a multiply/divide for each spell at the beginning and end of each iteration versus doing it once during the build phase. Putting the threat mod onto the aura itself would only be advantageous if some other future item or aura for paladins modified threat for specific Holy spells, in which case I think we can cross that bridge if we ever get there.
sim/paladin/righteous_fury.go
Outdated
} | ||
}) | ||
|
||
if !hasHoR { // This is just a visual/UI indicator when we don't have HoR rune. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What do you think about instead of using an if/else here, you can go ahead and create the righteous fury aura with one setup. Then if you don't have the HoR rune just return. Anything past that means you have HoR, so do the rest of the setup there.
paladin.RighteousFuryAura = core.MakePermanent(paladin.RegisterAura(core.Aura{
ActionID: actionID
Label: "Righteous Fury",
}))
if !hasHoR {
return
}
// Passive effects granted by Hand of Reckoning rune.
// Damage which takes you below 35% health is reduced by 20% (DR component of WotLK's Ardent Defender)
rfDamageReduction := 0.2
handler := func(sim *core.Simulation, spell *core.Spell, result *core.SpellResult) {
incomingDamage := result.Damage
if (paladin.CurrentHealth()-incomingDamage)/paladin.MaxHealth() <= 0.35 {
result.Damage -= (paladin.MaxHealth()*0.35 - (paladin.CurrentHealth() - incomingDamage)) * rfDamageReduction
if sim.Log != nil {
paladin.Log(sim, "Righteous Fury absorbs %d damage", int32(incomingDamage-result.Damage))
}
}
}
paladin.AddDynamicDamageTakenModifier(handler)
paladin.RighteousFuryAura.OnHealTaken = func(aura *core.Aura, sim *core.Simulation, spell *core.Spell, result *core.SpellResult) {
if spell.IsOtherAction(proto.OtherAction_OtherActionHealingModel) {
manaGained := result.Damage * 0.25
paladin.AddMana(sim, manaGained, horManaMetrics)
}
}
sorry for the formatting it's really annoying copying/pasting and dealing with spacing in GitHub 😅 let me know if this makes sense
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, I refactored this to use a single Aura struct and moved the RegisterAura
call to the end.
Implements:
This touches parts of core (healing model in
sim/core/health.go
so feedback on whether my solution is sound greatly appreciated.