WoWInterface - How could this be written neater?

Page 1 of 2

Show 60 post(s) from this thread on one page

WoWInterface (https://www.wowinterface.com/forums/index.php)

- Lua/XML Help (https://www.wowinterface.com/forums/forumdisplay.php?f=16)

- - How could this be written neater? (https://www.wowinterface.com/forums/showthread.php?t=55376)

Layback_

05-07-17 09:41 AM

How could this be written neater?

Hi all,

I've tried to make an aura tracker addon and..... it is working fine, but the code looks really disgusting.

Lua Code:

-- Upvalue variables
local _G = getfenv(0);
 
local GetSpecialization = _G.GetSpecialization;
local GetSpellInfo = _G.GetSpellInfo;
local ReloadUI = _G.ReloadUI;
 
-- Local variables
local size = 40;
local point = {"CENTER", UIParent, "CENTER", 0, -110};
local gap = 2;
local max = 6;
 
local buffIDList = {
    ["WARLOCK"] = {
        [3] = { -- Destruction
            235156,
            215165,
            119899,
        },
    },
};
 
-- Local functions
local InitObjects;
 
-- AuraTrack
local AuraTrack = CreateFrame("Frame");
AuraTrack:RegisterEvent("ADDON_LOADED");
AuraTrack:RegisterEvent("PLAYER_ENTERING_WORLD");
AuraTrack:SetScript("OnEvent", function(self, event, ...)
    self[event](self, ...);
end);
 
function AuraTrack:ADDON_LOADED(...)
    if ... == "MyAuraTracker" then
        InitObjects();
 
        self:UnregisterEvent("ADDON_LOADED");
    end
end
 
function AuraTrack:PLAYER_ENTERING_WORLD()
    local holder = self.holder;
 
    holder:RegisterUnitEvent("UNIT_AURA", "player");
    holder:SetScript("OnEvent", function(self, event, ...)
        AuraTrack[event](AuraTrack, ...);
    end);
 
    self:UnregisterEvent("PLAYER_ENTERING_WORLD");
end
 
function AuraTrack:UNIT_AURA(...)
    local holder = self.holder;
 
    -- Get player's class and spec to access buffIDList table
    local _, class = UnitClass(...);
    local spec = GetSpecialization();
 
    -- For each button, check if they are already assigned with aura
    for i = 1, #holder.button do
        local button = holder.button[i];
 
        -- If the button is assigned with aura and that aura is on-going, update its duration and so on
        -- Otherwise, remove id from button and hide
        if button.spellID then
            local spellName = GetSpellInfo(button.spellID);
 
            local _, _, icon, count, _, duration, expires = UnitAura(..., spellName);
 
            if duration and duration > 0 then
                button.cd:SetCooldown(expires - duration, duration);
 
                button.icon:SetTexture(icon);
 
                button.count:SetText((count and count > 0) and count or "");
 
                button:Show();
            else
                button.spellID = nil;
 
                button:Hide();
            end
        end
    end
 
    -- For each buff, check if they are activated
    for i = 1, #buffIDList[class][spec] do
        local spellName = GetSpellInfo(buffIDList[class][spec][i]);
 
        local _, _, icon, count, _, duration, expires = UnitAura(..., spellName);
 
        -- If the buff is activated, see if that buff is already assigned to a button
        if duration and duration > 0 then
            local assigned = false;
 
            for j = 1, #holder.button do
                if holder.button[j].spellID == buffIDList[class][spec][i] then
                    assigned = true;
                end
            end
 
            -- If buff is not assigned to any of buttons,
            -- assign the id and update its duration and so on
            if not assigned then
                for j = 1, #holder.button do
                    local button = holder.button[j];
 
                    if not button.spellID then
                        button.spellID = buffIDList[class][spec][i];
 
                        button.cd:SetCooldown(expires - duration, duration);
 
                        button.icon:SetTexture(icon);
 
                        button.count:SetText((count and count > 0) and count or "");
 
                        button:Show();
                        
                        break;
                    end
                end
            end
        end
    end
end
 
function InitObjects()
    local holder = CreateFrame("Frame", "AuraTrackerHolder", UIParent);
    holder:SetPoint(unpack(point));
    holder:SetSize((size + gap) * max - gap, size);
 
    holder.button = {};
 
    for i = 1, max do
        local button = CreateFrame("Frame", "$parentButton" .. i, holder);
        button:Hide();
 
        if i == 1 then
            button:SetPoint("LEFT");
        else
            button:SetPoint("LEFT", holder.button[i - 1], "RIGHT", gap, 0);
        end
 
        button:SetSize(size, size);
 
        local cd = CreateFrame("Cooldown", "$parentCooldown", button, "CooldownFrameTemplate");
        cd:SetAllPoints(button);
 
        local icon = button:CreateTexture(nil, "BORDER");
        icon:SetAllPoints(button);
 
        local count = button:CreateFontString(nil, "OVERLAY");
        count:SetFontObject(NumberFontNormal);
        count:SetPoint("TOP", button, "BOTTOM", 0, -2);
 
        holder.button[i] = button;
 
        button.cd = cd;
        button.icon = icon;
        button.count = count;
    end
 
    AuraTrack.holder = holder;
end

I am pretty happy with the rest of parts, but :UNIT_AURA() function is... meh.......

Even if I've left some comments there, pretty sure I would forget what I have done there with all those for loops.

Could I ask some helps?!

Thank you.

Kakjens

05-07-17 12:38 PM

For me it seems like AuraTrack:UNIT_AURA(...) can be split into two functions.

MunkDev

05-07-17 05:13 PM

(1) You're doing checkups on tables that may not exist. If you don't intend to add tables for every class and spec, anyone else using your addon that is not a destruction warlock will get errors.

(2) What's the point in your holder frame and not just using the AuraTrack frame you already created to show your auras?

(3) You don't need to upvalue the global environment just to grab a few functions. This process is quick and painless and only happens once on login anyway.

(4) Unless there's a specific reason why you're using a set max of 6 auras, you can use a frame factory / frame pool to spawn your trackers instead of creating them on load. I'll show you an example of how this works with the native implementation already in the UI.

Lua Code:

-- Upvalue variables
local GetSpellInfo = GetSpellInfo;
local UnitAura = UnitAura;
 
-- Local variables
local size = 40;
local point = {"CENTER", UIParent, "CENTER", 0, -110};
local gap = 2;
 
local buffIDList = {
    ["WARLOCK"] = {
        [3] = { -- Destruction
            235156,
            215165,
            119899,
        },
    },
};
 
-- AuraTrack
local AuraTrack = CreateFrame("Frame", "AuraTrack", UIParent);
AuraTrack.numTotalObjects = 0;
AuraTrack:SetPoint(unpack(point));
AuraTrack:RegisterEvent("ACTIVE_TALENT_GROUP_CHANGED");
AuraTrack:RegisterEvent("PLAYER_LOGIN");
AuraTrack:RegisterEvent("ADDON_LOADED");
AuraTrack:SetScript("OnEvent", function(self, event, ...)
    self[event](self, ...);
end);
 
 
function AuraTrack:UNIT_AURA(...)
    -- Release all the currently active objects
    -- See the :ResetTracker function for what it does to individual objects.
    self:ReleaseAll();
 
    -- Get the buffs to track for the current spec
    -- Assert a table exists for the player's class and spec
    local buffIDs = self.buffIDList;
 
    if buffIDs then
 
        -- Iterate over buff IDs
        local prevButton;
        for i, buffID in pairs(buffIDs) do
            local spellName = GetSpellInfo(buffID);
            local _, _, icon, count, _, duration, expires = UnitAura(..., spellName);
            if duration and duration > 0 then
                -- Acquire a button from the object pool
                local button = self:Acquire();
 
                -- Assign your stuff
                button.spellID = buffID; -- not sure this is necessary with this approach.
                button.cd:SetCooldown(expires - duration, duration);
                button.icon:SetTexture(icon);
                button.count:SetText((count and count > 0) and count or "");
                
                -- Show and set the point for the button
                button:Show();
                if prevButton then
                    button:SetPoint("LEFT", prevButton, "RIGHT", gap, 0);
                else
                    button:SetPoint("LEFT");
                end
 
                -- Reference this button so the next one knows where to anchor
                prevButton = button;
            end
        end
    end
 
    -- Update width to match active objects.
    -- numActiveObjects is provided by the object pool mixin.
    self:SetSize(((size + gap) * self.numActiveObjects) - gap, size);
end
 
-- Update spec and the buff table when the spec actually changes
function AuraTrack:ACTIVE_TALENT_GROUP_CHANGED()
    local _, class = UnitClass("player");
    local spec = GetSpecialization();
    self.buffIDList = buffIDList[class] and buffIDList[class][spec];
end
 
function AuraTrack:ADDON_LOADED(...)
    if ... == "MyAuraTracker" then
        self.ADDON_LOADED = nil;
        self:UnregisterEvent("ADDON_LOADED");
        self:RegisterUnitEvent("UNIT_AURA", "player");
    end
end
 
function AuraTrack:PLAYER_LOGIN()
    -- Run the spec check on login
    self:ACTIVE_TALENT_GROUP_CHANGED();
    self:UnregisterEvent("PLAYER_LOGIN");
    self.PLAYER_LOGIN = nil;
end
 
-- Related to creating and recycling active trackers
function AuraTrack:CreateTracker()
    -- This counter is only here to generate non-garbage frame names
    self.numTotalObjects = self.numTotalObjects + 1;
 
    local id = self.numTotalObjects;
    local button = CreateFrame("Frame", "$parentButton"..id, self);
 
    button:Hide();
    button:SetSize(size, size);
 
    local cd = CreateFrame("Cooldown", "$parentCooldown", button, "CooldownFrameTemplate");
    cd:SetAllPoints(button);
 
    local icon = button:CreateTexture(nil, "BORDER");
    icon:SetAllPoints(button);
 
    local count = button:CreateFontString(nil, "OVERLAY");
    count:SetFontObject(NumberFontNormal);
    count:SetPoint("TOP", button, "BOTTOM", 0, -2);
 
    button.cd = cd;
    button.icon = icon;
    button.count = count;
 
    return button;
end
 
function AuraTrack:ResetTracker(button)
    button.spellID = nil;
    button:ClearAllPoints();
    button:Hide();
end
 
-- Mix in the object pool functions
Mixin(AuraTrack, ObjectPoolMixin);
-- Give the object pool creation and resetting functions
AuraTrack:OnLoad(AuraTrack.CreateTracker, AuraTrack.ResetTracker);

Lombra

05-07-17 05:35 PM

Quote:

Originally Posted by MunkDev (Post 323255)

(3) You don't need to upvalue the global environment just to grab a few functions. This process is quick and painless and only happens once on login anyway.

Pretty sure it's a net loss, even.

Layback_

05-07-17 07:08 PM

Hi MunkDev,

Your example is awesome :banana:!!!!

Quote:

Originally Posted by MunkDev (Post 323255)

(1) You're doing checkups on tables that may not exist. If you don't intend to add tables for every class and spec, anyone else using your addon that is not a destruction warlock will get errors.

You are right! It was to show my intention of this addon and id for each class and spec are not listed there, yet :)

If possible, I am planning to create an interface options panel to add the id for each class and spec.

Quote:

Originally Posted by MunkDev (Post 323255)

(2) What's the point in your holder frame and not just using the AuraTrack frame you already created to show your auras?

Sorry for the lacking explanation.

The actual project uses Ace3.0 and AuraTrack in that case is actually an addon of Ace3.0 (not frame).

I totally forgot to clarify regarding that :(

Quote:

Originally Posted by MunkDev (Post 323255)

(3) You don't need to upvalue the global environment just to grab a few functions. This process is quick and painless and only happens once on login anyway.

:banana:

Quote:

Originally Posted by MunkDev (Post 323255)

(4) Unless there's a specific reason why you're using a set max of 6 auras, you can use a frame factory / frame pool to spawn your trackers instead of creating them on load. I'll show you an example of how this works with the native implementation already in the UI.

There was no specific reason to lock max frame to 6, but I was just trying to match its width to one of my center bar :p

In the future, if I want to limit the max frame to 6, I guess I could just put if statement at the top of :UNIT_AURA() function, am I right?

Code:

function AuraTrack:UNIT_AURA(...)

        if self.numTotalObjects >= 6 then

                return;

        end



        -- rest of your code

end

I have skimmed through the APIs and there are new terms (and functions) in your example that I am not familiar with.

Mixin
ObjectPoolMixin
:OnLoad()
:ReleaseAll()
:Acquire()

So, you are inheriting ObjectPoolMixin's functions to AuraTracker via Mixin which are OnLoad, ReleaseAll and Acquire, right?

Learning new stuff is always exciting :banana:

*EDIT: Guess I found a reference here

*EDIT2: Are there any other good Mixin that is good to know with?

MunkDev

05-07-17 08:07 PM

Mixin literally stands for "mix in", as in mixing in stuff from another table into your own table. It's a good way to preserve memory, because it utilises the same basic functions that are useful in many different cases. It's essentially an implementation of inheritance and object-oriented programming, albeit less bloated.

There's also a function called CreateFramePool, which returns a table that can be used to recycle objects that are displayed dynamically. A typical case for that is when you don't know how many objects you need. The downside is that it expects an XML template for the type of frame you want to create a pool for.

In the example I posted, I used a simpler solution, which is to only use the downscaled ObjectPoolMixin. This is the basis of a frame pool, but you supply the functions to create and reset your objects yourself. This is useful for when you're creating your objects in pure Lua.

Generally, you probably shouldn't go looking for mixins unless you have a distinct need for them. If you do insist, however, you can use this script to list all existing mixins in your chat:

Lua Code:

/run for k in pairs(_G) do if k:match("Mixin") then print(k) end end

Layback_

05-07-17 08:26 PM

Quote:

Originally Posted by MunkDev (Post 323259)

Generally, you probably shouldn't go looking for mixins unless you have a distinct need for them. If you do insist, however, you can use this script to list all existing mixins in your chat:

Lua Code:

/run for k in pairs(_G) do if k:match("Mixin") then print(k) end end

Wow... there are tons of Mixins haha!!

Thank you so much for your detailed explanation!

I really appreciate your help :banana:

Kkthnx

05-08-17 06:32 AM

Quote:

Originally Posted by MunkDev (Post 323255)

(3) You don't need to upvalue the global environment just to grab a few functions. This process is quick and painless and only happens once on login anyway.

Quote:

Originally Posted by Lombra (Post 323256)

Pretty sure it's a net loss, even.

I'd love to see more info on this. I have learned that is it is faster.

For example

Lua Code:

local _G = _G

then follow suit

Lua Code:

local GetCVar = _G.GetCVar

Is this bad practice? If so I'd love more info on this. Everyone has their ways.

So telling him he doesn't need to do this is right or wrong here or are you teaching him what you have learned.

Lua Code:

-- WoW Lua
local _G = _G
 
-- Wow API
local GetSpecialization = _G.GetSpecialization
local GetSpellInfo = _G.GetSpellInfo

lightspark

05-08-17 06:54 AM

The only reason why I do local _G = getfenv(0) is my paranoia o_O

I've seen addons that do some weird manipulations w/ environments, so _G may end up being something else, and a check like _G == getfenv(0) will return false.

IMHO, if you have no paranoia and don't call global functions from inside OnUpdate and prob OnEvent funcs, only if you register for REALLY spammy events, e.g., CLEU, personally, I consider UNIT_POWER_FREQUENT spammy too, there's no point in creating upvalues for pretty much anything.

Another reason might be to please one's Lua linter :D

Kkthnx

05-08-17 07:00 AM

Quote:

Originally Posted by lightspark (Post 323264)

The only reason why I do local _G = getfenv(0) is my paranoia o_O

I've seen addons that do some weird manipulations w/ environments, so _G may end up being something else, and a check like _G == getfenv(0) will return false.

IMHO, if you have no paranoia and don't call global functions from inside OnUpdate and prob OnEvent funcs, only if you register for REALLY spammy events, e.g., CLEU, there's no point in creating upvalues for pretty much anything.

Another reason might be to please one's Lua linter :D

Lmao @ please one's Lua linter! This is so true and has become a thing for me. I use Atom text editor and https://atom.io/packages/linter-lua-findglobals so I guess I am just so used to pleasing it :eek:

Also never thought of the whole _G = getfenv(0) deal. Incoming paranoia? :confused:

lightspark

05-08-17 07:05 AM

Quote:

Originally Posted by Kkthnx (Post 323265)

I call it _G-fetish :D

But in most cases upvalues and/or accessing globals via _G are a personal preference or even a style, people like weird things o_O

MunkDev

05-08-17 08:40 AM

Quote:

Originally Posted by Kkthnx (Post 323263)

Is this bad practice? If so I'd love more info on this. Everyone has their ways.

So telling him he doesn't need to do this is right or wrong here or are you teaching him what you have learned.

Lua Code:

-- WoW Lua
local _G = _G
 
-- Wow API
local GetSpecialization = _G.GetSpecialization
local GetSpellInfo = _G.GetSpellInfo

I'm not saying it's bad practice, I'm saying it's pointless. Lua in WoW assumes global environment access if the variable name you're looking for doesn't exist in your current scope. Localising those functions only entails two instances of table access. Why store a pointer to the global environment for the remainder of your game session when you're not even using it for anything?

The performance gain is negligible at best. The only recurring functions that are used in loops in response to a frequent event are the two functions I listed at the top. If you're doing a ton of lookups very frequently, it might be wise to upvalue functions or entire tables in that case.

Personally, I don't upvalue the global environment because it produces uglier code.

Ketho

05-08-17 10:38 AM

There were those old threads about _G upvaluing if that helps

Lombra

05-08-17 12:39 PM

Quote:

Originally Posted by Kkthnx (Post 323263)

I'd love to see more info on this. I have learned that is it is faster.

I'm not saying you did, but it's important not to take my quote out of context. I referred to this code sample, specifically. The call to getfenv() will take "much" longer than what you gained from upvaluing _G. _G = _G is better, in that case. Still very pointless.

Quote:

Originally Posted by lightspark (Post 323264)

I mean, sure you can be careful, but there is such a thing as being too careful to the point of it just being foolish. "No offence". If addons are reassigning the global _G, at that point they're practically malware.

People need to understand what kind of gains we are talking about here. Some results were presented in one of the threads Ketho linked. I'd like to provide some more.

I tested the following cases using debugprofilestop(), which is very precise.

Over a hundred thousand iterations, calling UnitAura without any upvalues takes around 52 milliseconds.

Code:

for i = 1, 1e5 do

  local a = UnitAura("player", 1)

end

Doing the same with an upvalued UnitAura takes just under 51 ms.

Code:

local UnitAura = UnitAura

for i = 1, 1e5 do

  local a = UnitAura("player", 1)

end

As for assigning values, we're upping the iteration count to one million for increased accuracy. Upvaluing without using _G takes 15.5 ms.

Code:

for i = 1, 1e6 do

  local a = UnitAura

end

Upvaluing using _G, but without first having upvalued _G it self takes 26 ms, which is significantly slower. There's never any reason to do this thinking it's faster. (that's not the net loss I was talking about, though)

Code:

for i = 1, 1e6 do

  local a = _G.UnitAura

end

Doing the same with an upvalued _G is... actually still slower, for some reason. 15.7 ms. When I tested on PTR last night it was slightly faster. Possibly related to size of _G? Though I would've thought doing a global lookup would be affect by the size of _G in the same way.

Code:

local _G = _G

for i = 1, 1e6 do

  local a = _G.UnitAura

end

This is what I was referring to. Upvaluing _G to upvalue a single value through that is a net loss due to having perform a global lookup to get _G, and then a table lookup in _G to get UnitAura, as opposed to just doing a global lookup for UnitAura. This took 26 ms. Of course, the gain should be greater the more values you're upvaluing, though the above seems to imply the opposite... I'm not sure if there is some underlying caching going on that's skewing the results.

Code:

for i = 1, 1e6 do

  local _G = _G

  local a = _G.UnitAura

end

For reference, replaing _G with getfenv(0) in the above code puts the result at 123 ms.

Do with this info as you will.
Here is the exact test suite I'm using. Feel free to try to produce your own results.

Code:

local t = debugprofilestop



local ct = t()



for i = 1, 1e6 do

  local _G = getfenv(0)

  local a = _G.UnitAura

end



ct = t() - ct



print(format("Execution time: %s ms.", ct))

lightspark

05-08-17 01:05 PM

Quote:

Originally Posted by Lombra (Post 323275)

That's why I said what I said, I do it because of my paranoia, and I have it because I've seen shit. Being paranoid is kinda foolish.

I agree w/ you and I also said that there's almost no need to create any upvalues in 99.99% of cases. Basically, my comment was "do as I say, not as I do" :D

P.S. I prob should start fighting _G-paranoia though....

Lombra

05-08-17 02:09 PM

Fair enough. :)

MunkDev

05-08-17 03:51 PM

Quote:

Originally Posted by lightspark (Post 323276)

That's why I said what I said, I do it because of my paranoia, and I have it because I've seen shit. Being paranoid is kinda foolish.

You're also not building software for NASA's rocket computers. If someone produces shitty, incompatible code in their add-ons that completely dismantle the entire interface, they'll fail automatically because people will not download them.

Kanegasi

05-08-17 03:53 PM

Quote:

Originally Posted by lightspark (Post 323276)

I can fully understand code paranoia. During a testing phase of Decliner, the friend helping me said it didn't work. After some investigation, having them use /dump on my main player check function returned nil, but it returned a function for me. I double-checked they were using the exact same /dump string and that my addon was loaded.

My answer was to move the check function to a local and then make a global dummy function that returns the check function. The worst part was the integration into a timer loop I use to check the global dummy every five seconds and reinitialize it if it's nil, while trying to out the addon deleting it with ADDON_LOADED. I recently removed that part of the timer loop actually, but I still hide the main check function locally, while the global dummy is just an API thing I keep.

Paranoia aside, I still learned that I shouldn't have important functions outside my addon's scope.

lightspark

05-09-17 01:35 AM

Quote:

Originally Posted by MunkDev (Post 323278)

You're also not building software for NASA's rocket computers.

Sure. I fully agree :p

Quote:

Originally Posted by MunkDev (Post 323278)

If someone produces shitty, incompatible code in their add-ons that completely dismantle the entire interface, they'll fail automatically because people will not download them.

Oh, people do download a lot of shit, they download and use the worst addons ever quite happily, and then they report "bugs" to other addons' devs, and then those devs waste their time trying to reproduce said "bugs", whereas they're caused by other addons that do some mind-boggling stuff w/ API and/or environments :D:mad:

I once wasted a lot of time doing so...

lightspark

05-09-17 01:37 AM

Quote:

Originally Posted by Kanegasi (Post 323279)

Been there, done that T_T Some devs are... Ugh :mad:

All times are GMT -6. The time now is 06:00 AM.

Page 1 of 2

Show 60 post(s) from this thread on one page