Feature: Only Fade if Extended

[Garretonzo]

Problem: I like to mash whatever button is shown, but I want to follow the recommendations perfectly. This includes ability delays. When using the Extend Spiral option under the Delays tab, it is hard to tell when an ability is being delayed by some additional amount of time vs the regular gcd. Currently, when enabling the Fade as Unusable option, the ability will fade even if there was no extra delay added. I thought combining these options would be very helpful.

Solution: This adds a feature to improve the clarity of when an ability's delay is extended. New behavior - If "Only Fade if Extended" is enabled (requires both Extend Spiral and Fade as Unusable options to be enabled), it will only fade if the ability was extended beyond the regular duration.

Please review, would love to see this feature make it into the addon.

[Garretonzo]

Added an explicit option to only fade the ability when both Fade and Extend are checked, if the ability delay was extended.

This maintains previous functionality while providing an option to enable this logic.

[Gyudon47]

Wow! This is actually what I'm looking for!

[Hekili]

It looks like you updated it to make it function per-display rather than globally, so I don't have to follow-up and ask for that.

I'll toy with this later and see about including it.

[Garretonzo]

Cool! Please do let me know if you'd like to see something about it changed, I had fun figuring this much out. I love this addon and would be very proud to be a contributor, albeit through a very small, niche addition.

[johnnylam88]

Is Extended a good description of what's happening? Maybe Wait instead, as in wait past GCD?

[Garretonzo]

I'm impartial to the semantics So I'd name it whatever the heck That said, I worded things after the already existing options "Fade as Unusable" and "Extend Spiral"

[Garretonzo]

It looks like you updated it to make it function per-display rather than globally, so I don't have to follow-up and ask for that.

I'll toy with this later and see about including it.

Hey @Hekili did you have a chance to check this out further?

[johnnylam88]

I've taken a stab at another way to implement this. It doesn't need another toggle, and instead just tries to resurrect the commented-out code:

https://github.com/Hekili/hekili/compare/thewarwithin...johnnylam88:hekili:feat/fade-on-wait

I think this does what you want: it only fades as unusable if the ability can't be spam-cast because you need to wait past the end of the current GCD or spellcast. If it can be spam-cast, then it should show at full alpha as usable.

[Garretonzo]

I've taken a stab at another way to implement this. It doesn't need another toggle, and instead just tries to resurrect the commented-out code:

thewarwithin...johnnylam88:hekili:feat/fade-on-wait

I think this does what you want: it only fades as unusable if the ability can't be spam-cast because you need to wait past the end of the current GCD or spellcast. If it can be spam-cast, then it should show at full alpha as usable.

I would have appreciated if you gave me chance to address that comment instead of taking my idea and making your own MR. Is there something fundamentally wrong with my approach that a new MR was necessary? The toggle was a feature so as to not remove existing functionality.

[johnnylam88]

It's just code and it was easier to share what I was thinking instead of describing it?

[Garretonzo]

It's just code and it was easier to share what I was thinking instead of describing it?

Yeah you're right sorry. I was just passionate about contributing to this project. Sorry mate

[johnnylam88]

No worries 👍 I hadn't known about the fade feature until I saw this pull request, and I think fading if you need to wait would be perfect. Judging from the commented-out code, I think that what you want is actually what's intended, so a separate setting is likely not needed.

I think your approach is actually correct, now that I've read through the OnUpdate method for the display. We do want to leave the potentially expensive API calls to GetItemCooldown and GetSpellCooldown only inside RefreshCooldowns instead of possibly invoking them twice. However, instead of just a flag, I think it would be better to store the actual earliest time the ability can be cast so we can directly inspect it and make decisions in the OnUpdate.

[johnnylam88]

Something like this patch series. I decided to re-use the existing cooldown calculations in the delay check and stashing that earliest time the ability can be cast in the first button in the display, then using that value in the range check that I moved to later in OnUpdate. This change is computationally neutral.

https://github.com/Hekili/hekili/compare/thewarwithin...johnnylam88:hekili:feat/fade-on-wait

Do you happen to have a test-case profile we can use to check that everything is working?

[Garretonzo]

I gotta be honest, I am having a hard time following what you are doing in your branch. I can look closer at it this evening. If this is something tag-team-able with merging our stuff together, that would be awesome. Is that what you are proposing? Or if your code makes mine obsolete, I understand.

I wish I did have a test-case profile but I have just been testing with my Arms warrior on training dummies.

[johnnylam88]

I've been testing on my Arms warrior, too :-) I'll see if I can create a simple test case.

[johnnylam88]

For the rest, I think we just need for @Hekili to check out the approaches to see what's best.

[Garretonzo]

cool, also I addressed the point @Hekili made in my latest commit