TellMeWhen

TellMeWhen

24M Downloads

[CF 1635] Doesn't work check for pet spells in range

tmw-issue-import opened this issue ยท 14 comments

commented

I want to check DK's pet Jump spell for in range condition. I tried "Icon" tab with Spell Cooldown type and "Conditions" tab with spell in range, nothing don't work. I searched some info and found that IsSpellInRange function has stop work since 7.2 patch, seems like truth coz I can't receive through /dump true statement but I found that action bar slot still works for that. Is any way to make check pet spell for in range?


Posted by CurseForge user cordankos | Imported from CurseForge issue #1635 | Raw

commented

First:  from your Spellbook's Pet tab, drag "Leap" (from here on out I'm assuming that's what you mean by Jump spell) to one of your action bars.

 

Import the following custom lua snippet:

 

^1^T^SName^SLeapInRance ^SCode^Sfunction~`GetActionSlot(id)~J ~`~`~`~`local~`id~`=~`id~J ~`~`~`~`for~`i=1,~`72~`do~J ~`~`~`~`~`~`~`~`if~`select(2,~`GetActionInfo(i))~`==~`id~`then~J ~`~`~`~`~`~`~`~`~`~`~`~`return~`i~J ~`~`~`~`~`~`~`~`end~J ~`~`~`~`end~J end~J ~J leapslot~`=~`GetActionSlot(47482)~J if~`not~`leapslot~`then~J ~`~`~`~`leapslot~`=~`GetActionSlot(91809)~J end~J ~J ~J function~`IsLeapInRange(unit)~J ~`~`~`~`if~`not~`leapslot~`then~J ~`~`~`~`~`~`~`~`return~`false~J ~`~`~`~`end~J ~`~`~`~`if~`not~`unit~`then~J ~`~`~`~`~`~`~`~`unit~`=~`"target"~J ~`~`~`~`end~J ~`~`~`~`return~`UnitExists(unit)~`and~`IsActionInRange(leapslot,unit)~J end~J ~J ~J ~J ~J ^t^N85702^S~`~| ^Scodesnippet^^

 

 

For your icon's condition, create a Miscellaneous > "Lua (advanced)" condition and put IsLeapInRange() in the textbox.

Here's an example group with such an icon you can import and look at:

^1^T^SGUID^STMW:group:1S63h0liB99r ^SScale^F6380114325536771 ^f-51^SPoint ^T^Sy^F8469066696097792 ^f-47^Spoint ^SBOTTOM^SrelativePoint ^SBOTTOM^Sx ^F-5339729237517724^f-46 ^t^SColumns^N1 ^SIcons^T ^N1^T ^SType^Sconditionicon ^SConditions^T ^N1^T ^SType^SLUA ^SName^SIsLeapInRange() ^t^Sn^N1 ^t^SSettingsPerView^T ^Sicon^T ^STextLayout^Sicon1 ^STexts^T ^N1^SLeap ^N2^S ^t^t^t^SCustomTex^S47482 ^SStates^T ^N1^T ^t^N3^T ^t^N4^T ^t^t^SEnabled^B ^t^N2^T ^SStates^T ^N1^T ^t^N3^T ^t^N4^T ^t^t^t^N3^T ^SStates^T ^N1^T ^t^N3^T ^t^N4^T ^t^t^t^N4^T ^SStates^T ^N1^T ^t^N3^T ^t^N4^T ^t^t^t^t^t^N85702^S~`~| ^Sgroup^N1 ^^

 


Edited Dec 17, 2018

Posted by CurseForge user canofbutter

commented

Thanks for reply, actually its heavy load cpu with huge fps drop to query each slot so I did it by cache through events UNIT_PET and ACTIONBAR_SLOT_CHANGED, idea is same, so sadly that I saw this too late. 


Posted by CurseForge user cordankos

commented

It doesn't check every cycle.  It only has to check every slot once when the snippet first loads.  (the downside to having done that means if you move Leap to a different slot, you'll have to rerun the snippet or /reload)  Read the code again - you'll see that's how it works.


Edited Dec 18, 2018

Posted by CurseForge user canofbutter

commented

Here you go - I spent a little more time on this for you so you can freely move Leap around on your action bars without reloading:

 

^1^T^SName^SLeapInRange ^SCode^Sfunction~`GetActionSlot(id)~J ~`~`~`~`local~`id~`=~`id~J ~`~`~`~`for~`i=1,~`72~`do~J ~`~`~`~`~`~`~`~`if~`select(2,~`GetActionInfo(i))~`==~`id~`then~J ~`~`~`~`~`~`~`~`~`~`~`~`return~`i~J ~`~`~`~`~`~`~`~`end~J ~`~`~`~`end~J end~J ~J local~`leapslot~`=~`GetActionSlot(47482)~J if~`not~`leapslot~`then~J ~`~`~`~`leapslot~`=~`GetActionSlot(91809)~J end~J ~J ~J local~`leapf~`=~`CreateFrame("FRAME");~J ~J leapf:RegisterEvent("ACTIONBAR_SLOT_CHANGED");~J ~J function~`LeapButtonHandler(self,~`event,~`...)~J ~`~`~`~`leapslot~`=~`GetActionSlot(47482)~J ~`~`~`~`if~`not~`leapslot~`then~J ~`~`~`~`~`~`~`~`leapslot~`=~`GetActionSlot(91809)~J ~`~`~`~`end~J end~J ~J leapf:SetScript("OnEvent",~`LeapButtonHandler);~J ~J ~J ~J function~`IsLeapInRange(unit)~J ~`~`~`~`if~`not~`leapslot~`then~J ~`~`~`~`~`~`~`~`return~`false~J ~`~`~`~`end~J ~`~`~`~`if~`not~`unit~`then~J ~`~`~`~`~`~`~`~`unit~`=~`"target"~J ~`~`~`~`end~J ~`~`~`~`return~`UnitExists(unit)~`and~`IsActionInRange(leapslot,unit)~J end~J ~J ~J ~J ~J ~J ~J ^t^N85702^S~`~| ^Scodesnippet^^

 

For convenience of reviewing it, here's the full code of the snippet without the import string encoding:

function GetActionSlot(id)
    local id = id
    for i=1, 72 do
        if select(2, GetActionInfo(i)) == id then
            return i
        end
    end
end

local leapslot = GetActionSlot(47482)
if not leapslot then
    leapslot = GetActionSlot(91809)
end


local leapf = CreateFrame("FRAME");

leapf:RegisterEvent("ACTIONBAR_SLOT_CHANGED");

function LeapButtonHandler(self, event, ...)
    leapslot = GetActionSlot(47482)
    if not leapslot then
        leapslot = GetActionSlot(91809)
    end
end

leapf:SetScript("OnEvent", LeapButtonHandler);



function IsLeapInRange(unit)
    if not leapslot then
        return false
    end
    if not unit then
        unit = "target"
    end
    return UnitExists(unit) and IsActionInRange(leapslot,unit)
end

 

As you can see, the function that scans all action bar slots is only called on load and on the action bar event - not every update.  IsLeapInRange uses the already scanned for "leapslot" variable in calling IsActionInRange; it does not continuously call GetActionSlot.


Posted by CurseForge user canofbutter

commented

I have around same code. In your code you still have weak in cpu performance because ACTIONBAR_SLOT_CHANGED trigers on anything, if you have macro with modifiers then it will triger event and again call GetActionSlot several times while you playing since slot changing by macro. Also more better will be add time stamp to stop triger event several times at same time rate. And even if you triger ACTIONBAR_SLOT_CHANGED then in arg1 it return slot which has been changed, so you can compare it with leapslot variable as well which make you more performance. UnitExists(unit) in IsLeapInRange can be removed since IsActionInRange always return nil if target is not valid, better "return IsActionInRange(leapslot,unit) or false". Wow has 120 max buttons to check, I don't here a lot why need use 72 if possible make all 120. True ID always is 47482 for Jump, no need use 91809, or it can be checked by locale name compare through GetSpellInfo. 


Posted by CurseForge user cordankos

commented

https://wow.gamepedia.com/Action_slot

Really only makes sense to check 72 at most.

 

The operation to scan them is very fast and not heavy on the CPU. Such a small, short, simple loop is likely not slower than any cache lookups - also I wasn't aware of any action bar caching; what do you mean by this?

No real reason for time stamping or the like - even with heavy macro usage it won't fire all that frequently (no more so than maybe once or twice per modifier keypress at most).  I messed around with this a bit and in the worst case got it to fire maybe 3 times per second if I was absolutely mad about smashing modifiers.

UnitExists is a leftover from other checks I was doing (that I removed for this) in addition to IsActionInRange that actually needed it - feel free to remove it.

 

If you feel you have a better answer to your own question, I don't see why you're not sharing it.  Other people might benefit that are trying to do the same/similar things...


Posted by CurseForge user canofbutter

commented
function GetActionSlot(id)
    local id = id
    for i=1, 72 do
        actionType, actionid, subType = GetActionInfo(i)
        if subType == "pet" and actionid == id then
            return i
        end
    end
   message("Leap is not found")
end

local leapslot, timestamp = GetActionSlot(47482), TMW.time

local leapf = CreateFrame("FRAME")
leapf:RegisterEvent("ACTIONBAR_SLOT_CHANGED")
function LeapButtonHandler(self, event, ...)
    if PetHasActionBar() and TMW.time ~= timestamp and (not leapslot or leapslot == ...)  then
        timestamp = TMW.time
        leapslot = GetActionSlot(47482)
    end
end

leapf:SetScript("OnEvent", LeapButtonHandler)

function IsLeapInRange()
    return (leapslot and IsActionInRange(leapslot)) or false
end

 

 

 

I don't have a lot of powerfull pc and I see game spikes when query slots through ACTIONBAR_SLOT_CHANGED. Function IsActionInRange has only one argument to use, its slot without unit, you can check slot in range for "focus" if there exist macro with @Focus.


Edited Dec 18, 2018

Posted by CurseForge user cordankos

commented

Default TMW must have this feature. Would be interest in theory somehow create hidden button to use it for query action.


Posted by CurseForge user cordankos

commented

Still not seeing anything one would call "caching" related to the action bar.  Also, I don't think you intended to assign the slot location to "timestamp", that wouldn't really mean anything.

 

If you're using a ton of macro modifiers it would fire a lot, I suppose.  For a test I filled all 6 of my action bars with macros that had modifiers.  Even using my code without the timestamp check, it scanned them all repeatedly like that in around 1/8000th of a second.  Try as I might, I just can't see your frame drops coming from that, but there's no reason not to do the timestamp check as you did, so that's fine.

 

As for your change in GetActionSlot - you don't need to check the subtype for pet - spell ids are unique

IsActionInRange can indeed take a unit - API documentation you're finding must be out of date.  I rely on this a lot when checking units by nameplate.

 

Regardless, if you're really trying to squeeze out everything you can performance wise, skip all the event handling entirely and use the original I posted - it'll only ever scan when you load in or /reload - can't get better than that and it's what I use for all pet range checking - here's that code block:

function GetActionSlot(id)
    for i=1, 72 do
        if select(2, GetActionInfo(i)) == id then
            return i
        end
    end
end
-- this only runs once on load/reload, so do a /reload if you move the button
local leapslot = GetActionSlot(47482)

function IsLeapInRange(unit)
    if not leapslot then
        return false
    end
    if not unit then
        unit = "target"
    end
    return IsActionInRange(leapslot,unit) or false
end


As for any sort of hidden buttons, no not really:  IsActionInRange accepts action bar slots from the base game only.  Any new, hidden action buttons you create could not be passed to it.


Edited Dec 18, 2018

Posted by CurseForge user canofbutter

commented

I remade your code which you sent first time because as you noticed in last message  

If you're using a ton of macro modifiers it would fire a lot, I suppose

function LeapButtonHandler(self, event, ...)
    leapslot = GetActionSlot(47482) -- here is each Alt Ctrl Shift will fire this event and query slots which will make for me game spikes
    if not leapslot then
        leapslot = GetActionSlot(91809)
    end
end


So better use:

if PetHasActionBar() and TMW.time ~= timestamp and (not leapslot or leapslot == ...) then  

Cache also means that, don't query slots if we know that Leap already here or saved LeapSlot has been changed in current fired slot.

 

OMG IsActionInRange second argument for me is big suprise, because I checked wowpedia and wowwiki, they are both telling only 1 argument. Where you looking actual API for that?


Edited Dec 18, 2018

Posted by CurseForge user cordankos

commented

https://wow.gamepedia.com/Patch_2.0.1/API_changes#API_Changes:_Information

Been available since 2.0.1 - works 100% - use it all the time

Also, no, the code that I sent the first time (the import string) didn't have any event handling at all - it was a single run on load.  We're over complicating this to handle what is really just an edge case:  moving the button around without relying on a /reload.  That's just unnecessary and the original snippet import string (not the readable code block - my very first reply had import strings) is more then sufficient.   I never should have tried adding that bit, as it just seems to be leading to confusion and possible performance issues.


Posted by CurseForge user canofbutter

commented

Oh, and the cache thing you're mentioning:  that's EXACTLY what the first reply's import snippet did.  You just with apparent misunderstanding about how TMW's snippets worked (they don't run constantly - they're run once).  Using a LUA condition and actually calling IsLeapInRange() is the only thing that will run repeatedly, and that's using the action bar location of your pet's ability that was found on the snippets initial first run and never gets rescanned by that function.


Posted by CurseForge user canofbutter

commented

Oh yea, it was 3rd msg. I guess for people who will search any solution will fair enough everything here. Well this thing is very usefully for range class with pet though nameplates. Thanks canofbutter 


Posted by CurseForge user cordankos

commented

It does seem that IsSpellInRange does indeed not work for pet spells anymore. I don't plan on implementing any workarounds in TMW to do actionbar scanning - Blizzards needs to get their stuff together and fix their API. Unforunately, as a general rule of thumb, Blizzard couldn't care less about any of their API as long as it isn't broken in a way that affects the default UI.


Posted by CurseForge user Cybeloras