-
Notifications
You must be signed in to change notification settings - Fork 34
Mistweaver implementation #380
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
base: feature/monk-mistweaver
Are you sure you want to change the base?
Mistweaver implementation #380
Conversation
sim/monk/mistweaver/soothing_mist.go
Outdated
| ClassMask: monk.MonkSpellSurgingMist, | ||
| }) | ||
|
|
||
| surgingMistChannelMod := mw.AddDynamicMod(core.SpellModConfig{ |
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.
Soothing mist currently enables and disables two aura's to allow surging mist and enveloping mist to be cast while channeling (also another two aura's to reduce both spells cast time to zero). I am unsure if this is the ideal solution or if there is a better way to make this happen?
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.
You can combine these into one pair of mods:
mistCastTimeMod := mw.AddDynamicMod(core.SpellModConfig{
Kind: core.SpellMod_CastTime_Pct,
FloatValue: -1,
ClassMask: monk.MonkSpellSurgingMist | monk.MonkSpellEnvelopingMist,
})
mistChannelMod := mw.AddDynamicMod(core.SpellModConfig{
Kind: core.SpellMod_AllowCastWhileChanneling,
ClassMask: monk.MonkSpellSurgingMist | monk.MonkSpellEnvelopingMist,
})
NerdEgghead
left a comment
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.
Also make sure to add a unit test configuration for the spec.
|
|
||
| if apl.unit.ChanneledDot != nil { | ||
| //Probably not the best solution, added so apl evaluates if a spell can be cast while channeling during runtime rather than on reset | ||
| apl.allowCastWhileChanneling = slices.ContainsFunc(apl.unit.Spellbook, func(spell *Spell) bool { | ||
| return spell.Flags.Matches(SpellFlagCastWhileChanneling) | ||
| }) | ||
|
|
||
| if apl.unit.ChanneledDot != nil && !apl.allowCastWhileChanneling { | ||
| return | ||
| } |
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.
I think there's a simpler solution for this: you can permanently add SpellFlagCastWhileChanneling to Soothing Mist, then all you have to check is
if (apl.unit.ChanneledDot != nil) && !apl.unit.ChanneledDot.Spell.Matches(SpellFlagCastWhileChanneling) {
return
}
sim/core/apl.go
Outdated
| //Checking for cast-while-channeling spells to allow the APL to not evaluate during channels unless absolutely necessary | ||
| allowCastWhileChanneling bool |
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.
This field could be removed if you go with my previous suggestion.
sim/core/spell.go
Outdated
|
|
||
| // While casting or channeling, no other action is possible | ||
| if spell.Unit.Hardcast.Expires > sim.CurrentTime { | ||
| if (spell.Unit.Hardcast.Expires > sim.CurrentTime || spell.Unit.IsChanneling(sim)) && !spell.Flags.Matches(SpellFlagCastWhileChanneling) { |
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.
You should add the same correction to the CanQueue() method within sim/core/spell_queueing.go.
sim/core/unit.go
Outdated
| func (unit *Unit) IsChanneling(sim *Simulation) bool { | ||
| return unit.ChanneledDot != nil | ||
| } |
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 sim argument here is unused and can be omitted.
sim/monk/jab.go
Outdated
| BaseCostPercent: core.TernaryFloat64(monk.StanceMatches(WiseSerpent), 8, 0), | ||
| //This is wrong? But works? | ||
| BaseCostPercent: core.TernaryFloat64(monk.StanceMatches(WiseSerpent), 0, 8), |
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.
Reminder to figure out what the underlying bug is here and fix it
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.
I'm a bit confused myself, the intention behind the original I assume was to have the mana cost be set if the player is in wise serpent stance. However when running the sim the mana cost of jab was zero. Replacing it with the above now had it properly costing mana or simply replacing it with the mana cost sans TernaryFloat64 also works and doesn't seem to disrupt the other monk sims from what I can tell.
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.
I would leave in the costs, and then if needed based on the active Stance aura enable/disable a SpellMod that reduces the costs
sim/monk/mistweaver/soothing_mist.go
Outdated
| if envelopingActive { | ||
| multiplier += +0.3 | ||
| } |
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.
Are you sure this shouldn't be multiplier *= 1.3?
sim/monk/mistweaver/soothing_mist.go
Outdated
| outcome := sim.Roll(1, 10) | ||
| if outcome > 7 { | ||
| mw.AddChi(sim, soothingMist, 1, chiMetrics) | ||
| } |
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.
This will simulate a 33.33% proc chance rather than a 30% proc chance. You can fix it and also simplify the code by calling
if sim.Proc(0.3, "Soothing Mist Chi") {
mw.AddChi(sim, dot.Spell, 1, chiMetrics)
}
Note that using dot.Spell here also eliminates the need for the dangling soothingMist pointer.
sim/monk/mistweaver/surging_mist.go
Outdated
| baseHealing := 19630 + spellCoeff*spell.HealingPower(target) | ||
| //Hardcoded to heal the player for now | ||
| spell.CalcAndDealHealing(sim, &mw.Unit, baseHealing, spell.OutcomeHealingCrit) |
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.
If you set the BonusCoefficient field on the SpellConfig to 1.8, then you can simplify this to just
spell.CalcAndDealHealing(sim, target, 19630, spell.OutcomeHealingCrit)
and the Spell Power contribution will be handled automatically.
sim/monk/mistweaver/uplift.go
Outdated
| baseHealing := 0 + spellCoeff*spell.HealingPower(target) | ||
| spell.CalcAndDealHealing(sim, player, baseHealing, spell.OutcomeHealingCrit) |
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.
Same here - if you set the BonusCoefficient field to 0.68, then you can just call
spell.CalcAndDealHealing(sim, player, 0, spell.OutcomeHealingCrit)
|
@Patrick-Hogeveen Can you merge |
I have synced my fork with master and resolved all conflicts. I assumed that simply accepting the incoming changes resulting from the new casting while channelling changes was the correct thing to do. |
NerdEgghead
left a comment
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.
Also make sure to add a unit test configuration for the spec so that the spells you added will be tested.
| //rot.allowCastWhileChanneling = slices.ContainsFunc(rot.unit.Spellbook, func(spell *Spell) bool { | ||
| // return spell.Flags.Matches(SpellFlagCastWhileChanneling) | ||
| //}) |
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.
General comment: delete any unused commented code like this for cleanliness once you are done debugging.
| ProcMask: core.ProcMaskSpellHealing, | ||
| Flags: core.SpellFlagHelpful | core.SpellFlagAPL, // | core.SpellFlagCastWhileChanneling, |
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.
Did you intend to comment out the extra flag here?
|
|
||
| ManaCost: core.ManaCostOptions{BaseCostPercent: 0}, |
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.
You can just omit this field if the cost is 0.
| //ClassSpellMask: monk.MonkSpellEnvelopingMist, | ||
|
|
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.
Did you intend to comment out the spell mask here?
| if target.Type == core.EnemyUnit { | ||
| fmt.Printf("Attemping to cast Enveloping mist on enemy: %v\n", target.Label) | ||
| return | ||
| } |
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.
I think it would be better to panic here rather than printing a message and returning. Forcing a crash is more likely to alert the user that their APL was set up incorrectly.
| if envelopingActive { | ||
| dot.SnapshotAttackerMultiplier = 1.3 | ||
| } else { | ||
| dot.SnapshotAttackerMultiplier = 1 | ||
| } |
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.
Should these be *= rather than =? Otherwise you lose the original result of the CasterHealingMultiplier() call.
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.
I'm not sure? The way it is now is currently working. Changing it to *= causes the healing to increase each time it ticks while enveloping mist is active where it should be a 30% increase in healing while enveloping mist is active and normal when it is not. Perhaps an aura would work better than my current implementation?
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 you could (and most of us) do is:
multiplier := core.TernaryFloat(mw.envelopingMist.RelatedDotSpell.Hot(target).IsActive(), 1.3, 1.0)
dot.SnapshotAttackerMultiplier *= multiplier
dot.CalcAndDealPeriodicSnapshotHealing(sim, target, dot.OutcomeTick)
dot.SnapshotAttackerMultiplier /= multiplier
Basically adding > casting > removing the multiplier to not get infinite 30% gain.
| ApplyEffects: func(sim *core.Simulation, target *core.Unit, spell *core.Spell) { | ||
| //Currently target mistweaver only, will need to fix this |
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.
This comment is no longer accurate.
| if target.Type == core.EnemyUnit { | ||
| fmt.Printf("Attemping to cast Enveloping mist on enemy: %v\n", target.Label) | ||
| return | ||
| } |
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.
Same comment, panic here so you fail loudly rather than silently.
| chiGain := int32(1) | ||
| mw.AddChi(sim, spell, chiGain, chiMetrics) |
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.
You don't need to define a new variable for single-use parameters like this, you can just call
mw.AddChi(sim, spell, 1, chiMetrics)
| if monk.MuscleMemoryAura.IsActive() { | ||
| result.Damage += result.Damage * 1.5 |
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.
You can just do
result.Damage *= 2.5
| if hot.IsActive() { | ||
|
|
||
| spell.CalcAndDealHealing(sim, player, 0, spell.OutcomeHealingCrit) | ||
| success = true |
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.
Can heals on a target ever fail? or does this spell in-game not cost Chi if you had 0 targets healed?
otherwise this spell should always just SpendChi
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.
I believe that you can not cast it at all if there's no renewing mist active
| } | ||
|
|
||
| if monk.MuscleMemoryAura.IsActive() { | ||
| result.Damage += result.Damage * 1.5 |
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.
This should probably become a 0.5 DamageDonePct SpellMod on the Aura itself, because right now you are actually not going a 150% mod, but 250%. (100% of the original result + 150% of the original result * 1.5)
| manaPerTick := 0.0 | ||
| //numerOFTicks := 6 | ||
|
|
||
| mw.Monk.RegisterOnChiSpent(func(sim *core.Simulation, chiSpent int32) { |
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.
mw.RegisterOnChiSpent should work
| MaxStacks: 20, | ||
| }) | ||
|
|
||
| mw.Monk.RegisterOnNewBrewStacks(func(sim *core.Simulation, stacksToAdd int32) { |
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.
mw.RegisterOnNewBrewStacks should work
|
|
||
| dmgDone = result.Damage | ||
| //Should be a smart heal | ||
| serpentZealHeal.Cast(sim, &mw.Unit) |
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.
Does it need to be smart here? Or in the ApplyEffects part of this heal? There you can loop over all friendlies and find the lowest HP Unit
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.
It definitely could be done in ApplyEffects, I just left that there as a reminder to myself since I wasn't sure if the raid member target dummies technically had health pools so I hadn't thought up how to implement it as a smart heal yet.
| } | ||
| aura.Deactivate(sim) | ||
| }, | ||
| }) |
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.
Here you should add the 1.5 .AttachSpellMod for the 2 BCK and TP
| chiMetrics := mw.NewChiMetrics(actionID) | ||
| spellCoeff := 0.19665 //Will have to verify this | ||
| //targets := mw.Env.Raid.GetFirstNPlayersOrPets(int32(mw.Env.Raid.NumTargetDummies)) | ||
| charges := 3 |
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.
Probably better to keep this in a "Dummy Aura" that has stacks. Then the timeline will show how many charges the Aura had.
|
|
||
| Hot: core.DotConfig{ | ||
| Aura: core.Aura{ | ||
| Label: "Renewing Mist", |
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.
I think you can actually add MaxStacks here and use this Aura for the "charges"
| } | ||
|
|
||
| if monk.MuscleMemoryAura.IsActive() { | ||
| result.Damage += result.Damage * 1.5 |
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.
Would be nice to also move this to a 0.5 SpellMod, same as BCK. And it's not 250% but 150%, so add 50% of the original damage
First draft code for a number of mistweaver abilities and passives