search

SFX-WoW / Masque

A button-skinning engine for World of Warcraft.
Other
48 stars 11 forks source link

Feature Request: Static ChargeCooldowns #58

Closed ZinohJoe closed 5 years ago

ZinohJoe commented 5 years ago

https://github.com/Zinoh/Masque/commit/09482effd86b29b4443bdb9f998d1c3d9dca75c5#diff-29640bbe49e3344125235d5c78a95054L155

Simple fix.

Also, is it intended that the chargeCooldown region only be usable by Action buttons? Due to the line above:

155: if bType ~= "Action" then return end

In my local copy I simply moved that line to below the chargeCooldown related stuff to test it out in my local copy of TellMeWhen (which hasn't updated to the new parameters yet so using the Legacy setting). Wasn't sure of your intention though.

StormFX commented 5 years ago

ChargeCooldowns are recycled, thus they're created and [re]assigned dynamically. That particular line of code exists only to catch any CCDs that are assigned to buttons when they're skinned. So the existing code is correct.

As far as the placement of the "Action" check, I've no problem with moving it down. I was unaware that any add-ons were using them outside the context of Action buttons.

ZinohJoe commented 5 years ago

Ahh okay. TellMeWhen uses a seperate frame for chargeCooldowns. I wasn't able to get it skinned without changing that line though.

StormFX commented 5 years ago

You're referring to the "Action" check? That's the only thing that's changed.

ZinohJoe commented 5 years ago

The change from Button -> Regions

StormFX commented 5 years ago

The current implementation is how it's always been, so if you had to change it to get it to work, that means it's never worked.

ZinohJoe commented 5 years ago

You're right. I didn't realize it wasn't intended for Masque to skin deliberately created static charge cooldown frames the same as it does for regular cooldown frames. Assumed it was an oversight.

I have a custom circular skin I made that I use in TMW. I wanted Masque to skin TMW's charge frame so it would follow the circularedge and utilize custom edge textures if desired. It only took the change I initially mentioned here (along with moving the Action check line) and adding the following line to TMW to allow Masque to skin its charge frame.

https://github.com/Zinoh/TellMeWhen/commit/eed6e2fc27a55120c283305a5242969b8a975a08#diff-d5ef1ed81d47d0ddfff60601cf13eeb1

Edit: I only use Masque to skin WeakAuras and TellMeWhen so I wasn't aware of how the charge cooldown skinning is implemented in regular action buttons. I see that now.

ZinohJoe commented 5 years ago

I realize now that an addon like TMW should utilize the UpdateCharge API already in Masque to skin its charge frame. I'll see how to implement that in TMW and submit a ticket there. Apologies if I caused any confusion :).

StormFX commented 5 years ago

A ChargeCooldown is just a Cooldown with its SetDrawSwipe set to false, so there's really no reason to distinguish between the two, aside from that. Even if an author uses both the edge and swipe, Masque should catch it.

If TMW uses a static ChargeCooldown, then I've no problems adding a check for that in the Regions table as that demonstrates a use case. Note that the key will be ChargeCooldown. It seems TMW actually skips that particular region anyhow, and only passes Icon, HotKey , Count and Cooldown. So you'd have to submit a request to TMW to have that region added.

I've yet to notify the author of the changes to :AddButton(), so that will need to be updated as well. As far as I'm aware, TMW doesn't provide actual clickable buttons but they have some regions that an Action button has so the logical course would be to pass "Action" as the type and then set Strict to true.

Edit: In regards to your follow-up post, if the ChargeCooldown is static (never changes its parent), there's no reason to call :UpdateCharge() unless TMW changes something that Masque has done.

ZinohJoe commented 5 years ago

That makes sense. I believe TMW utilizes a seperate frame for charges so that GCD swipes can be shown independently without interrupting the re-charge animation (which utilizes an edge w/ no swipe). As far as I can tell the charge frame does remain static upon icon creation. It does seem then in that case adding a check in the Regions table would be necessary.

Right, until now TMW didn't pass the charge frame as a skinnable frame to Masque. I was hoping to submit a ticket to change that and demonstrate it working. That sent me down the rabbit hole of making the changes that I ended up making, heh. Thanks for the advice.

StormFX commented 5 years ago

No problem. I'm currently trying to go through my skins, but I'll make the appropriate changes that should be in the next alpha.