OnCastComplete rework #2247
OnCastComplete rework #2247
Conversation
Tharre
force-pushed
the
oncastcomplete_rework
branch
4 times, most recently
from
8e48bef
to
ecf64ba
Compare
| dps: 7355.53047 | ||
| tps: 7194.44067 | ||
| dps: 7320.79478 | ||
| tps: 7159.70498 | ||
| } | ||
| } | ||
| dps_results: { | ||
| key: "TestBalance-Average-Default" |
There was a problem hiding this comment.
Looks like balance is losing 34 DPS in the average test. Is this expected?
There was a problem hiding this comment.
If you look at the Remaining Differences commit, balance damage doesn't change at all there, so it should be fully explained by the caster trinket change.
| @@ -314,14 +314,15 @@ func (druid *Druid) applyOmenOfClarity() { | |||
| spell.CostMultiplier += 1 | |||
| } | |||
| }, | |||
| OnCastComplete: func(aura *core.Aura, sim *core.Simulation, spell *core.Spell) { | |||
There was a problem hiding this comment.
Instead of deleting this, I think we could have a local variable var proccedAt time.Duration which is set on proc, and then only remove the aura if proccedAt != sim.CurrentTime.
Then you'll be able to remove the hard-coded handling of clearcasting from all the spells.
There was a problem hiding this comment.
I don't think that would work. The issue here why we have to change anything at all is that we have a CC proc -> we cast something for free -> OnSpellHitDealt is called and may proc another CC -> OnCastComplete is called and removes the CC proc. Weirdly even when moving the check into ApplyEffects the DPS is still different, I have no clue yet why that is.
Thinking about it, maybe we can check in OnCastComplete if the aura was applied at the exact same time as sim.CurrentTime and only remove it if they differ.
| @@ -9,6 +9,8 @@ import ( | |||
|
|
|||
| func (paladin *Paladin) registerShieldOfRighteousnessSpell() { | |||
| baseCost := paladin.BaseMana * 0.06 | |||
| has4pT8 := paladin.HasSetBonus(ItemSetAegisPlate, 4) | |||
| procAura := paladin.NewTemporaryStatsAura("Aegis", core.ActionID{SpellID: 64883}, stats.Stats{stats.BlockValue: 225}, time.Second*6) | |||
There was a problem hiding this comment.
Could we refactor this slightly like so?
var aegisPlateProcAura *core.Aura
if paladin.HasSetBonus(ItemSetAegisPlate, 4) {
aegisPlateProcAura = paladin.NewTemporaryStatsAura("Aegis", core.ActionID{SpellID: 64883}, stats.Stats{stats.BlockValue: 225}, time.Second*6)
}
...
...
if aegisPlateProcAura != nil {
aegisPlateProcAura.Activate(sim)
}
The advantage is that the aura won't be registered unless the set bonus is equipped, so in most cases we'll have 1 less aura attached to the unit.
There was a problem hiding this comment.
Yeah that makes more sense.
sim/priest/items.go
Outdated
| @@ -89,22 +89,22 @@ var ItemSetSanctificationRegalia = core.NewItemSet(core.ItemSet{ | |||
| 2: func(agent core.Agent) { | |||
| }, | |||
| 4: func(agent core.Agent) { | |||
| priest := agent.(PriestAgent).GetPriest() | |||
There was a problem hiding this comment.
I think you can revert this change entirely. It seems likely that activating after the PWS cast is correct behavior.
sim/warlock/lifetap.go
Outdated
| @@ -46,6 +47,14 @@ func (warlock *Warlock) registerLifeTapSpell() { | |||
| if hasGlyph { | |||
| warlock.GlyphOfLifeTapAura.Activate(sim) | |||
| } | |||
|
|
|||
| if has4PT7 { | |||
| if warlock.SpiritsoftheDamnedAura.IsActive() { | |||
There was a problem hiding this comment.
aura.Activate() already checks for IsActive() and does the refresh. You can simplify this code here, which will also cause the PPM metrics for the aura to be correct.
Tharre
force-pushed
the
oncastcomplete_rework
branch
from
ecf64ba
to
049862e
Compare
Tharre
force-pushed
the
oncastcomplete_rework
branch
from
049862e
to
dfe44b6
Compare
Tharre
force-pushed
the
oncastcomplete_rework
branch
from
dfe44b6
to
0f44286
Compare
No description provided.