Hekili Priority Helper

Hekili Priority Helper

44M Downloads

[REC] Judgment debuff isn't triggered by Divine Resonance

cremor opened this issue ยท 12 comments

commented

Before You Begin

  • I confirm that I have downloaded the latest version of the addon.
  • I am not playing on a private server.
  • I checked for an existing, open ticket for this issue and was not able to find one.
  • I edited the title of this issue (above) so that it describes the issue I am reporting.
  • I am reporting an issue with the default priority included with the specialization (imported or edited priorities are not supported).

Describe the Issue

It seems like there is an issue with the feature that you added for #1398. The Judgment debuff isn't simulated when it should be triggered by a priodic Divine Resonance Judgment.

How to Reproduce

  1. Enter game as a Kyrian Retribution Paladin.
  2. Equip the Divine Resonance or Unity legendary.
  3. Use Divine Toll.
  4. Use Templars Verdict to consume the Judgment debuff.
  5. Now check the recommendations when the periodic Divine Resonance Judgment comes up.

Snapshot (Link)

https://pastebin.com/QsZ4kPRu

Raidbots Sim Report (Link)

https://www.raidbots.com/simbot/report/9r18p9fqASPHkmdSJreGPA

Additional Information

If I read the snapshot correctly, the following happens:

  1. The snapshot starts without a Judgment debuff.
  2. On line 801 the periodic Divine Resonance event is simulated.
  3. Now debuff.judgment.up should be true, but still isn't. All conditions between line 801 and 1067 show debuff.judgment.up[false].
  4. On line 1067 Judgment (the ability) is recommended. From here on debuff.judgment.up is true, so only now it is recognized.
  5. Starting with line 1374 debuff.judgment.up will be false again, because it is consumed by Templars Verdict. That's fine.

The result of that is that recommendation 2 is Judgment, which it wouldn't be if the Divine Resonance Judgment debuff would have been simulated.

Contact Information

No response

commented

Thanks, good diagnosis on this one.

When Divine Resonance procs, should the Judgment cast also generate Holy Power? I genuinely don't recall. It looks like it wouldn't right in the addon's model.

commented

Yes, it does generate Holy Power. But that already works. You fixed that in 72f400f

The snapshot also shows this working. holy_power is increased to 4 on line 806 (after the periodic Divine Resonance event).

commented

Oh, now I see what you mean. If the change in 89a96bb would have worked, a periodic Divine Resonance tick would now simulate 2 Holy Power, right? One from the change from 72f400f and one from judgment.handler.

commented

I noticed another bug in 89a96bb:
You seem to call judgment.handler for both Ret and Prot. But Divine Resonance only casts Judgment for Ret. For Prot it casts Avenger's Shield (so the same spell as Divine Toll casts).

commented

I noticed another bug in 89a96bb:
You seem to call judgment.handler for both Ret and Prot. But Divine Resonance only casts Judgment for Ret. For Prot it casts Avenger's Shield (so the same spell as Divine Toll casts).

Technically, the bug is that it doesn't cast anything on tick for either. Because the event processor wasn't running the function for those events. It'll be fixed soon. I just couldn't work on it last night.

commented

Ok, I'll be posting 1.0.13-alpha3 that should address this for both Ret and Prot. The change can impact other specializations, so I want to look closely at it.

commented

The change to address it isn't actually in the Ret or Prot file, it's in State, but I can see it didn't make it into the alpha for some reason. I'll add it now.

commented

I just tested v9.2.0-1.0.13-alpha3 and it seems like I can still reproduce the bug.

Here is a new snapshot: https://pastebin.com/m43B8tJ5
A Divine Resonance tick is on line 804. The next recommendation after that is Judgment. But it shouldn't have been. debuff.judgment.up is false on line 1062, but should be true here.

I've also quickly checked the commits and while I could find the spell change for Prot, I couldn't find any fix for Ret (or a general fix that would affect Ret). Are you sure that the change is in v9.2.0-1.0.13-alpha3?

commented

Thanks! I'll test it in a few hours when I get home.

But just from reading the code, wouldn't it now simulate 2 Holy Power per Divine Resonance tick? Because as far as I see it, we now have 3 places where Divine Resonance is handled:

  • spec:RegisterResource: This only simulates Holy Power gain, right? If we indeed have 2 Holy Power per tick now, this could be removed.
  • spec:RegisterHook( "reset_precast": This simulates Judgment for Ret and Avengers Shield for Prot, which also includes the Holy Power gain. Seems fine.
  • divine_toll.handler: This seems to be more or less the same as point 2, just for the initial cast? And it seems like this is ran for both Ret and Prot? If that's the case, Prot is still incorrectly triggering Judgment there.
commented

Yeah, if I want to implement Divine Resonance this way, I need to consistently put the Holy Power gain in the handler for those abilities (or not). It's a problem that it's inconsistent.

commented

I've just tested v9.2.0-1.0.13-alpha4 plus the changes from 0899358. Looks good now, thanks!

commented

Thanks!