Authors CurseForge

Ok I have 4 frames and they can be active or inactive. I need a way to check if any of them is active and if it is, set a variable to true (isRunning).
The code below works, kinda, but not like I want it to.

local isRunning
frame1 = {}
frame2 = {}
frame3 = {}
frame4 = {}
frame1.active = false
frame2.active = true
frame3.active = true
frame4.active = false

for i = 1, 4 do
   local frame = _G["frame"..i]
   if frame.active then
      isRunning = true
      break
   else
      isRunning = false
   end
end

print(isRunning)

As you can see it breaks as soon as it encounters the first active frame (frame2), but I need to know if any of the remaining frames are also active, like frame3 in this case. How can I do this?

I wrote a long explanation but it didnt make any sense at all so instead I'm asking you to simplify this code :p

local f = CreateFrame("Frame")
local isRunning

frame1 = {}
frame2 = {}
frame3 = {}
frame4 = {}
frame1.active = false
frame2.active = true
frame3.active = false
frame4.active = true

local lastUpdate = 1
local function OnUpdateFunc(self, elapsed)
   lastUpdate = lastUpdate + elapsed
   for i = 1, 4 do
      local frame = _G["frame"..i]
      if frame.active then
         isRunning = true
         break
      else
         isRunning = false
      end
   end
   
   if isRunning then
      if lastUpdate > 1 then
         for i = 1, 4 do
            local frame = _G["frame"..i]
            if frame.active then
               print("frame"..i.." is active")
               --frame:Show()
            else
               print("frame"..i.." is not active")
               --frame:Hide()
            end
         end
         lastUpdate = 0
      end
   else
      print("no active frames")
      f:SetScript("OnUpdate", nil)
   end
   
end
--      end
f:SetScript("OnUpdate", OnUpdateFunc)

First of all, if possible, instead of making variables frame1, frame2, frame3 and frame4; you'd be better off making a table of frames.

local frames = {};
frames[1] = CreateFrame("Frame"); -- or access a global premade frame
frames[2] = CreateFrame("Frame"); -- You get the idea.

A table lookup in your for loop would be much more efficient than concatenating strings and looking up globals on every update.

If you already have an 'active' variable for each of the four frames that you maintain somewhere, that's great. Otherwise "frames[1]:IsShown()" may do the trick depending on your definition of 'active' is.

I'm trying to follow your logic. On the second code block you posted on post #3, the code makes next to no sense. The first loop to me is an unneeded redundancy. You have a loop checking if any frame is active. Then if any frame is active you have a loop checking which of the frames are active.

Here is some code of what I think you're trying to accomplish. It may not be exactly what you want, but hopefully it'll help you figure out what you need to do.

local lastUpdate = 1
local function OnUpdateFunc(self, elapsed)
   lastUpdate = lastUpdate + elapsed

    isRunning = false; -- first assume no frames are running
    if lastUpdate > 1 then
       for i = 1, 4 do
          local frame = frames[i]; -- This is the frames table I suggested earlier
          if frame.active then
             isRunning = true; -- At least one frame is active, but don't break
             print("frame", i, "is active")
             --frame:Show() -- Also do any other code you need to do.
          else
             print("frame", i, "is not active") -- Note it's better to pass multiple variables to print than to preconcatenate the string yourself.
             --frame:Hide()
          end
       end
       lastUpdate = 0
    end

    if (not isRunning) then -- This would happen if we finished the loop but no frames were active
        print("no active frames")
        f:SetScript("OnUpdate", nil) -- Do any other code you need here for this condition
    end
end

local activeframes = 0

-- whenever you activate a frame
activeframes = activeframes + 1
if activeframes == 1 then
  f:SetScript("OnUpdate",updatefunc)
end

-- whenever you deactivate a frame
activeframes = activeframes -1
if activeframes == 0 then
  f:SetScript("OnUpdate", nil)
end

The above will give you isRunning (activeframes > 0) and be logically consistent.

It's not clear to my why OnUpdate is involved in all of this, but that's a separate question. And if you do need OnUpdate, why not attach scripts to active frames individually? I think you should tell us what actual goal you have, beyond the specifics, because the right answer very much depends upon what you ultimately try to achieve.

Crap.. now I'm confused.:p Ok I'm gonna try to explain what my idea is.
The frames are supposed to be buffs/cooldowns and when they are active (frame.active) they will be updated through an OnUpdate function. This OnUpdate function will be controlled with the isRunning variable. So I can turn it completely off when no frames are active.. I don't know if that makes any sense ugh.

The code I posted in my last post does exactly what I want it to do, but it's probably beyond crap for efficiency.:(

I can't explain it any better, I'll try to break it down in an example:

1. frame2.active = true
2. run OnUpdate
3. OnUpdate is updating frame2 and checking for more active frames
4. frame2 finished, frame2.active = false
5. OnUpdate set to nil since no more frames are found and frame2 is inactive

well, to answer the very first question... the issue is your break. rather than explicitly setting isRunning to false, it should start off being false (outside the loop) and then if any frame is active set it to true and don't do an else. then you can eliminate the break.

what does your OnUpdate script actually do tho? is it where you draw your buffs with some kind of timer notification? or do you have something else you're updating on each frame?

Quote from Elsia

It's not clear to my why OnUpdate is involved in all of this, but that's a separate question. And if you do need OnUpdate, why not attach scripts to active frames individually? I think you should tell us what actual goal you have, beyond the specifics, because the right answer very much depends upon what you ultimately try to achieve.

Having one OnUpdate is more efficient than many, thus why Ace_Timer-3.0 exists.

Like everyone else has said more information on the actual intent of the code is needed. Asking us to improve that particular piece of code could easily result in something completely useless to you.

Things that would be nice to know:

Will it always be those same 4 frames or is that just something you made up for an example?

Will the frames have OnUpdate handlers and if so will they all have different ones or share just one?

Will isRunning be used outside of what we have seen?

How is the field "active" determined for frames outside of what you've shown, how frequently is it expected to change, and is it used by other code?

How is the OnUpdate code shown started again once it shuts itself off?

Are you more interested in the code being fast or in using little memory?

The answers to those questions will determine what approach would yield the best results for what you want. That said, I still have no idea what it is that you are trying to accomplish.

I thought that it would be easier if I took out part of the code and made it into an example but it was a bad idea. Anyway here is the full code, fully working but kinda crap coded :p
http://pastebin.com/shbcWxi9

i think if this is the limit of your mod, then you're probably better off using your main frame to collect the cooldown info sort of as you're doing, then having each subframe have its own update script that handles turning itself off when the cooldown expires.

it'd actually be the same update script, only placed individually on each frame.

you're actually misstepping in your definition of OnUpdateFunc. it shouldn't be addon:OnUpdateFunc. it happens to work because "addon:OnUpdateFunc(elapsed)" equates to "OnUpdateFunc(addon, elapsed)". in reality, OnUpdateFunc() is being called as "OnUpdateFunc(addon.buffframe, elapsed)". but since you're ignoring the first arg, you don't notice.

you should make this a local function that accepts a frame as the first argument. this frame is the frame that got the OnUpdate event. if you attach this same func to each of your individual buff frames, then you just treat them inside the OnUpdateFunc function as generic frames each with their own .cd timer.

local function OnUpdateFunc(frame,elapsed)
    frame.cd = frame.cd - elapsed
 
    if frame.cd < 0 then
        frame:Hide()
    end
end

addon.buff1:SetScript("OnUpdate",OnUpdateFunc)

that's the basics right there. you could throttle if neccesary (but keep in mind the throttle needs to be attached to the frame so each frame has its own throttle).

when you detect your buff, set your cd and :Show() your frame.

Oh ok, but what if I want to add more buttons later on? Say if I had 20 buttons wouldnt that be a lot of individual scripts?

Ah hmm.. interesting..
So I don't have to monitor and shut down the OnUpdate function since just hiding the frame will do the same thing.
By the way why can't I use a throttle in the update function from the start?

function addon.SetCD(unit, spell, cd)
   print("recieved: " .. unit, spell, tostring(cd))
   
   for i = 1, 4 do
      local frame = addon["buff"..i]
      if not frame.active then 
         print("frame " .. i .. " is not active")
         frame.spell = spell
         frame.active = true
         frame.cd = cd
         print("cd: " .. tostring(frame.cd))
         _G[frame:GetName().."Icon"]:SetTexture(GetSpellTexture(spell))
         addon.lastUpdate = 1
         frame:Show()
         frame:SetScript("OnUpdate", addon.OnUpdateFunc)
         break
      else
         if frame.spell == spell then
            frame.cd = cd
            print(spell .. " is already activated and running, resetting timer")
            break
         else
            print("frame " .. i.. " is already set")
         end
      end
   end
end

function addon.OnUpdateFunc(frame, elapsed)
   frame.cd = frame.cd - elapsed
   addon.lastUpdate = addon.lastUpdate + elapsed
   if addon.lastUpdate > 1 then
      print("Updating : " ..frame:GetName(), frame.cd)
      if frame.cd < 0 then
         frame:Hide()
         frame.active = false
      end
      addon.lastUpdate = 0
   end
end

Is this looking any better? :p

Quote from airscape

Oh ok, but what if I want to add more buttons later on? Say if I had 20 buttons wouldnt that be a lot of individual scripts?

as others pointed out, it's only one script that is being run on a generic "frame" element multiple times. since all your frames share the same data, the update script doesn't really care which buff frame is being operated on. it just knows that frame.cd is the cooldown timer and it needs to be adjusted and the frame turned off when it reaches 0 (or less).

now it is a reasonable concern to consider that you might have 20 buttons that all want to run a script every frame. but is 20 different scripts any more intensive that 1 script that uses lua to loop thru the 20 potential buttons? kinda depends on the overhead of calling an OnUpdate script. i'm sure the loop blizz uses to go over each frame's script is at least as efficient as a lua loop. so unless there's a serious overhead involved in calling an lua function from C (which there might be) i think it's at least as efficient to have 20 buttons with their own scripts. but it's definitely a consideration to be made.

sinec you're still learning the ropes, i'd opt for an elegant solution over an efficient one. once you get the hang of things a bit more, then you can work on squeezing more out of the system. $0.02

Quote from airscape

Ah hmm.. interesting..
So I don't have to monitor and shut down the OnUpdate function since just hiding the frame will do the same thing.
By the way why can't I use a throttle in the update function from the start?

function addon.SetCD(unit, spell, cd)
   print("recieved: " .. unit, spell, tostring(cd))
   
   for i = 1, 4 do
      local frame = addon["buff"..i]
      if not frame.active then 
         print("frame " .. i .. " is not active")
         frame.spell = spell
         frame.active = true
         frame.cd = cd
         print("cd: " .. tostring(frame.cd))
         _G[frame:GetName().."Icon"]:SetTexture(GetSpellTexture(spell))
         addon.lastUpdate = 1
         frame:Show()
         frame:SetScript("OnUpdate", addon.OnUpdateFunc)
         break
      else
         if frame.spell == spell then
            frame.cd = cd
            print(spell .. " is already activated and running, resetting timer")
            break
         else
            print("frame " .. i.. " is already set")
         end
      end
   end
end

function addon.OnUpdateFunc(frame, elapsed)
   frame.cd = frame.cd - elapsed
   addon.lastUpdate = addon.lastUpdate + elapsed
   if addon.lastUpdate > 1 then
      print("Updating : " ..frame:GetName(), frame.cd)
      if frame.cd < 0 then
         frame:Hide()
         frame.active = false
      end
      addon.lastUpdate = 0
   end
end

Is this looking any better? :p

not sure why you feel the need to insert the OnUpdateFunc into the addon table, but it's acceptable. i'd just do "local function OnUpdateFunc" but it's not any different functionally.

you don't need ".active" anymore. and the throttle is going to cause you problems because it's going to only update one frame every second since your throttle isn't tied to a specific frame. use "frame.lastUpdate" if you want to throttle (and of course, define it when you set the buff).

also, put your frames in a table instead of referring to them by their global. i personally try to avoid creating frames with names whenever i can since it tend to pollute the global namespace unnecessarily.

edit: nvm i see you've got them in a table, but still use a string concat to identify them. lose the string concat -- it's not needed. just use the index. something like "addon.buffFrame[i]"

Ok so rather than creating local frame = addon["buff"..i]
i'll just use addon["buff"..i] ? I just thought it would make it easier to work with if I made a local copy with shorter name.
Also I was thinking abit about the OnUpdate script, would it be a good idea to start it on every frame and then hide them until they are ready to be shown?
Something like this

for i = 1, numFrames do
    local frame = addon["buff"..i] -- :P
    frame:SetScript("OnUpdate", OnUpdateFunc)
    frame:Hide()
end

function addon:OnSomeEvent()
    frame:Show()
end