-
Notifications
You must be signed in to change notification settings - Fork 59
ravagane fix #1467
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
ravagane fix #1467
Conversation
dc58084 to
c2bff6a
Compare
| curSwingSpeed float64 | ||
| curSwingDuration time.Duration | ||
| enabled bool | ||
| allowed 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.
@Adamrch can you explain why we need the #AllowAutoSwing method and the allowed state versus what exists now?
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 exists now is the player controlled startattack / stopattack. Allowed is a just a flag that causes the swing timer to reset and no auto to go out as seen in game. (the delay at the end of the whirlwind for the first auto is consistant with the swingtimer filling and reseting during the whirlwind)
| core.StartPeriodicAction(sim, core.PeriodicActionOptions{ | ||
| Period: tickPeriod, | ||
| NumTicks: 6, | ||
| Priority: core.ActionPriorityDOT, | ||
| TickImmediately: false, | ||
| OnAction: func(sim *core.Simulation) { | ||
| if aura.IsActive() && aura.TimeActive(sim) >= tickPeriod { // Prevent previous proc from casting on Refresh | ||
| aura.RemoveStack(sim) | ||
| whirlwindSpell.AOEDot().TickOnce(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.
Why do we need to use a separate periodic action here versus just using the AOEDot's aura?
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 can try to implement again using the AOEDot Aura but dot damage was not going away at the right time when bladestorm aura was canceled in my branch. (could you help take a look and replace periodic action with the AoEDot apply() ?) I dont mind changing that if it works but it seemed to have issues in my branch.
| core.StartDelayedAction(sim, core.DelayedActionOptions{ | ||
| DoAt: sim.CurrentTime + time.Millisecond * 50, // Exact amount of time unknown | ||
| OnAction: func(s *core.Simulation) { | ||
| if aura.IsActive() { | ||
| character.AutoAttacks.AllowAutoSwing(sim, false) | ||
| } | ||
| }, | ||
| }) |
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 is a problem if a swing timer happens to fall within this window right?
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.
Possibly, we don't know exactly at what point between 0 and 1.5 the auto attack gets disabled, but it's not disabled immediately after the first hit. Suggestions?
| whirlwindAura := character.RegisterAura(core.Aura{ | ||
| Label: "Ravagane Bladestorm Disable Autos", | ||
| ActionID: core.ActionID{SpellID: 1231547}, | ||
| Duration: time.Second * 9 + core.SpellBatchWindow, // Allows 6th and final tick |
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 actually thinking that we should not expose this aura to the APL and should instead expose the AOEDoT's aura. Then have that aura control the disabling/re-enabling of autos and cancelling the aura in the APL both removes the DoT so that it won't hit, plus it will then go ahead and re-enable autos. I don't really see a need for this aura to exist when we can and should be using that one?
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 couldn't get much of anything to work right without using this aura. A ton of issue popped up for me when try to avoid it. If you can get the original set of auras to work better and fix the issues I pointed out I would gladly go for it. (I can't really explain why I need this but I struggled to fix the issues without it)
clean up ravagane code



Fixes proc mask on whirlwind to suppress Equip effects
Fixes autos going out too soon after whirlwind ends
Allows WS attack to go out immediate after proccing Bladestorm on same swing.
Fixes all cancelaura Issues breaking Ravagane proc when used in APL.
Also fixes auto attacks just being allowed to be re-enabled during whirlwind.