Hekili Priority Helper

Hekili Priority Helper

44M Downloads

cooldown.x.remains incorrect value

ShmooDude opened this issue ยท 5 comments

commented

Describe the Bug/Issue
cooldown.x.remains is always reporting the maximum cooldown when used for an ability that is shown in a different display

To Reproduce
I have Convoke the Spirits as a cooldown toggle.
The below snapshot was generated with both Tiger's Fury and Convoke the Spirits on cooldown.
The cooldown values are different depending on which display they are in.

Expected behavior
The cooldown should be valid even for abilities that are toggled to show in other displays.

Cooldown snapshot - Full Snapshot https://pastebin.com/5jC67Nxi
12. heart_of_the_wild ( owlweave - 3 )
The action (heart_of_the_wild) is usable at (0.00 + 0.00).
- the action is ready before the current recommendation (at +0.00 vs. +60.00).
List ( owlweave ) called from ( ShmooDude Feral:default:2 ) would PASS at 0.00.
druid.owlweave_cat[true]
- this entry's criteria PASSES: energy.current[15.00] < 30 & dot.rip.remains[25.52] > 4.5 & ( cooldown.tigers_fury.remains[30.00] >= 6.5 | runeforge.cateye_curio.enabled[false] ) & buff.clearcasting.stack[0.00] < 1 & ! buff.apex_predators_craving.up[false] & ! buff.bloodlust.up[false] & ( buff.bs_inc.remains[0.00] > 5 | ! buff.bs_inc.up[false] ) & ( cooldown.convoke_the_spirits.remains[105.06] > 6.5 | ! covenant.night_fae[true] )
Action chosen: heart_of_the_wild at 0.00!
Returned from list (owlweave), current recommendation is heart_of_the_wild (+0.00).
- owlweave
The recommended action (heart_of_the_wild) is ready in less than 0.2s; exiting list (default).

Primary snapshot - Full Snapshot: https://pastebin.com/pV7UU4f1
13. moonkin_form ( owlweave - 4 )
The action (moonkin_form) is usable at (0.00 + 0.00).
- the action is ready before the current recommendation (at +0.00 vs. +60.00).
List ( owlweave ) called from ( ShmooDude Feral:default:2 ) would PASS at 0.00.
druid.owlweave_cat[true]
- this entry's criteria FAILS: energy.current[15.00] < 30 & dot.rip.remains[25.52] > 4.5 & ( cooldown.tigers_fury.remains[1.31] >= 6.5 | runeforge.cateye_curio.enabled[false] ) & buff.clearcasting.stack[0.00] < 1 & ! buff.apex_predators_craving.up[false] & ! buff.bloodlust.up[false] & ( buff.bs_inc.remains[0.00] > 5 | ! buff.bs_inc.up[false] ) & ( cooldown.convoke_the_spirits.remains[120.00] > 6.5 | ! covenant.night_fae[true] )
Excluded 0.00 recheck time as it is outside our constraints ( 0.00 - 15.00 ).
Excluded 0.00 recheck time as it is outside our constraints ( 0.00 - 15.00 ).
Excluded 0.00 recheck time as it is outside our constraints ( 0.00 - 15.00 ).
There were no recheck events to check.
Time spent on this action: 0.44ms
TimeData:ShmooDude Feral-owlweave-4:moonkin_form:0.44:Post-TTR and Essential:0.02:Post Cycle:0.00:Post Usable:0.01:Post Ready/Clash:0.01:Post Stack:0.02:Pre-Script:0.00:Post-Script:0.02:Pre-Recheck:0.32:Post-Recheck Times:0.03:Post Recheck:0.00
Returned from list (owlweave), current recommendation is NO ACTION (+60.00).
- owlweave

Thoughts:
I'm unsure if this was a design decision in order to prevent abilities in other displays from blocking (or not blocking) abilities in the current display but in this case it causes the suggestions to be incorrect.

commented

This is a necessity to allow for toggles at all. Otherwise, quite often, you'll have abilities toggled off that lead into a feedback loop of prep work for when you'd press the button -- but you're telling the addon never to recommend that button.

A good example would be Unholy and Apocalypse. If you toggled off Apocalypse or moved it to the CDs display, the Primary display would tell you to build up wounds and then you'd be stuck without recommendations: Apocalypse is off, you don't generate wounds because you'd overcap, you don't spend because you want to spend those wounds on Apocalypse.

So this is a downside of having a Cooldowns display that functions this way. Interrupts and Defensives are less controversial, because they are not generally interdependent with the actual DPS priority. The AOE display doesn't involve enabling or disabling any particular abilities.

Note that, even with this functionality disabled, you'd likely create a lot of weirdness by moving some abilities to the Cooldowns display. Let's say you move Tiger's Fury. It has pretty limited criteria, so it'll show quite a lot. Now, in the Primary display, Tiger's Fury isn't shown, and you have nothing to indicate whether TF would be first, second, third, or fourth in queue.

Perhaps that means that the Cooldowns display shouldn't be populated as its own queue at all. Maybe when recommendations are generated for the Primary display, any cooldown abilities should get queued up for the CDs display (this would provide offset information, so it could clarify that it means 'use Tiger's Fury in 3 seconds, after the next few recommendations'). That could cost more CPU, because time spent generating a recommendation for each icon generally increases a bit over the previous (barring the CPU cost of the initial reset).

Custom priorities can be written to bypass this (cooldown.X.true_remains bypasses the toggle/disabled check), but I agree that it needs to be a bit clearer.

So the remaining questions are:

  1. Can this be addressed without creating a regression in the whole toggle system?
  2. Do we need a way to specify this behavior with a user option? Per ability? (Note: I don't like adding options that will just cause confusion and lead to a higher maintenance burden.)
  3. Do we need to steer back toward the "tray" idea which, in theory, wouldn't treat abilities as enabled or disabled, just provide whether the addons criteria are met for the trayed ability in that moment?
  4. Is there another solution? Just providing the actual cooldown would break too many specializations.
commented

cooldown.X.true_remains will do what I need for now. Thanks for that.

I know back when I was using Ovale, it would only display something in the cooldown icon if it were next in the priority list by checking to make sure all the abilities above it in the priority list were not shown. I guess Hekili just flat out ignores abilities that aren't in its current display. It had been very CPU intensive in the past, but what the author did was to save nodes in memory so when the same node came up, it would simply reference what was already calculated. May not be relevant for Hekili but might spark an idea.

Tiger's Fury I don't think is a good example since if you move it to the cooldowns, it only shows up at the proper time (when you're >40 energy.deficit or the other criteria). But I do see the point you were trying to make as more complicated cooldowns this wouldn't necessarily be the case and could easily show up earlier than expected. I think you're right in that "the Cooldowns display shouldn't be populated as its own queue at all."


I think maybe the solution might be that cooldown.x.remains always returns the actual value for abilities that are displayed in the primary display (ie anything not toggled off or shown separately). The idea being that anything in the Primary display is expected to be used, but spells in the other displays won't necessarily be used (might be held).

For example, after thinking about it, owlweaving should have actual cooldown for tigers_fury but not for convoke_the_spirits since I would want to owlweave while I'm holding onto convoke for later.

commented

So, a couple of ideas worth testing:

  1. What if the feigned CD logic only applied to Primary / AOE?

This would mean your CDs display would use real cooldown times; your primary display would not.

  1. What if the ancillary (CDs, Interrupts, Defensives) displays were populated all at once rather than in their own separate set of predictions?

This would also mean that abilities popped off into separate displays could still be timed in relation to the buttons you've pressed. This could result in some CPU savings, but could also feel kinda weird. Depending on how your displays are laid out, it might seem really misleading as to the order in which you might want to do things.

commented

If you want to test the first point, instead of using true_remains edit State.lua:2635 as follows...

local raw = state.display ~= "Primary" and state.display ~= "AOE"

If you test it, let me know how you feel about the effect on recommendations in the CD display and the Primary display.

commented

Going to push my testfix to live (where the feigned CD only applies to Primary and AOE).