[Core] Cooldown, Rounding for Cast Time and GCD

[kayla-glick]

Reverts #745

Re-opening #723 so that we can fix issues with it @Polynomix

[Polynomix]

i can fix the shaman bug by adding a test to the shaman code without changing the core, but for the lava surge proc to reset lava burst cooldown when they happnes at the same time, i need the rounding that's causing issue to other specs

[NerdEgghead]

i can fix the shaman bug by adding a test to the shaman code without changing the core, but for the lava surge proc to reset lava burst cooldown when they happnes at the same time, i need the rounding that's causing issue to other specs

Can you explain why this is a rounding issue that causes the current incorrect behavior for Lava Surge? If I understand better what's happening, I might be able to suggest a less intrusive fix to make it work properly.

[Polynomix]

When you flame shock into lavaburst, if no haste procs in between, the first tick of flame shock is going to happen at the same time as the end of the lava burst cast. So if that ticks procs lava surge, the cd of lava burst is going to be reseted. Now that dots ticks are rounded, in the sim they do not coincide anymore, the dot tick can happen before the end of the LvB cast.

[NerdEgghead]

When you flame shock into lavaburst, if no haste procs in between, the first tick of flame shock is going to happen at the same time as the end of the lava burst cast. So if that ticks procs lava surge, the cd of lava burst is going to be reseted. Now that dots ticks are rounded, in the sim they do not coincide anymore, the dot tick can happen before the end of the LvB cast.

Got it. So the issue with the Guardian sim wasn't related to cast time rounding, but due to the fact that you moved where the CD is actually set, which breaks any spell code that resets the CD within ApplyEffects(). So if you can find a way to implement the rounding without changing when the CD is triggered, that would fix the Guardian issue at least.

[rosenrusinov]

So the issue with the Guardian sim wasn't related to cast time rounding, but due to the fact that you moved where the CD is actually set, which breaks any spell code that resets the CD within ApplyEffects()

This can still be fixed easily even with what he already did. You just need to move the cd set a couple of lines up before the ApplyEffects. Setting the CD on the start of the cast instead of the finish is definitely wrong and I don't know how we've had it like this without any other issues.

[Polynomix]

You just need to move the cd set a couple of lines up before the ApplyEffects.

i'll try again tomorrow when i have more time. Also how does this revert PR works ? do i have to make a new one or pushing to my old branch will update this ?

[NerdEgghead]

You just need to move the cd set a couple of lines up before the ApplyEffects.

i'll try again tomorrow when i have more time. Also how does this revert PR works ? do i have to make a new one or pushing to my old branch will update this ?

The easiest way would be to pull down the revert-745-revert/pr-723 branch used for this current PR, commit any desired changes + push those, which should automatically update this PR.

If you don't have perms to push to this branch directly, then what you can do is merge this branch into your old branch locally, make changes in your old branch and push those, and then open another PR to merge your changes into this branch (rather than into master like we typically do). You can see an example of this type of a "PR into a PR" here, which got incorporated into this PR before it was merged.

[NerdEgghead]

@Polynomix @rosenrusinov @kayla-glick I've fixed this PR now to produce the desired behavior for Lava Surge without breaking other sims. By selectively reverting parts of the original PR, I determined that the actual rounding itself was causing zero issues for any other sims. Instead, several melee sims were exhibiting DPS losses due to the placement of the CD reset (discussed above), while several caster sims were exhibiting DPS losses due to changing the priority of the Hardcast PendingAction. I fixed the first issue by simply moving the CD reset code up a couple lines like @rosenrusinov suggested. The second issue is hard to diagnose, so I just reverted the priority to the prior default, and instead added custom handling for Lava Surge procs to allow for back-to-back Lava Burst casts to be possible. With these adjustments, the average DPS across all tests is now within noise of master for all other sims, while the Elemental test averages match the values from the original PR that was reverted.

[Tharre]

LGTM