Would like to get some general advice

[Layback_]

Hi all,

For this time, I am here to ask some general advice or feedback regarding my Outlaw Rogue's Roll the Dice assist addon.

Until now, I have been playing around with those awesome libraries or modules like oUF, LibSharedMedia, LibStub, CallbackHandler and so on, and I know that there are already couple of fancy Roll the Dice assist addons exist. However, creating one by myself would be a good practice to test my skills, revise the things that I am misunderstanding and even learn new features.

SO, here's a super simple addon that just displays duration, icon and tooltip for each Roll the Dice buffs that the player currently got.
(Pretty sure even a person who just started learning lua can make this and that's how simple this addon is haha...)

if select(2, UnitClass("player")) ~= "ROGUE" then
    return;
end
 
local RtheDsList = {
    199603, -- Jolly Roger
    193358, -- Grand Melee
    193357, -- Shark Infested Waters
    193359, -- True Bearing
    199600, -- Buried Treasure
    193356, -- Broadsides
};
 
local LSM = LibStub("LibSharedMedia-3.0");
local font = LSM:Fetch("font", "koverwatch");
 
local OutlawFrame = CreateFrame("Frame", "CA_Outlaw", UIParent);
OutlawFrame:RegisterEvent("PLAYER_LOGIN");
OutlawFrame:RegisterEvent("PLAYER_ENTERING_WORLD");
OutlawFrame:RegisterEvent("UNIT_AURA");
OutlawFrame:RegisterEvent("PLAYER_SPECIALIZATION_CHANGED");
OutlawFrame:SetSize(200, 200);
OutlawFrame:SetPoint("CENTER", 0, 250);
 
function OutlawFrame:CreateRtheDs()
    local RtheDs = CreateFrame("Frame", "$parentRollTheDice", self);
    RtheDs:SetAllPoints(true);
 
    for i = 1, #RtheDsList do
        local RtheD = CreateFrame("Frame", "$parentBuff" .. i, RtheDs);
        RtheD:SetSize(32, 32);
        RtheD:SetPoint("CENTER", math.cos(math.pi / 3 * (i - 1)) * 64, math.sin(math.pi / 3 * (i - 1)) * 64);
 
        RtheD.sID = RtheDsList[i];
 
        local cd = CreateFrame("Cooldown", "$parentCooldown", RtheD, "CooldownFrameTemplate");
        cd:SetPoint("CENTER");
        cd:GetRegions():SetSize(RtheD:GetWidth() + 2, 15);
        cd:GetRegions():SetFont(font, 15, "OUTLINE");
        cd:GetRegions():SetJustifyV("TOP");
        cd:GetRegions():SetJustifyH("CENTER");
 
        local name, _, icon = GetSpellInfo(RtheD.sID);
 
        local texture = RtheD:CreateTexture("$parentIcon", "Overlay");
        texture:SetTexture(icon);
        texture:SetAllPoints(true);
 
        RtheD.UpdateTooltip = function(self)
            GameTooltip:SetSpellByID(self.sID);
        end;
        RtheD:SetScript("OnEnter", function(self)
            if not self:IsVisible() then
                return;
            end
 
            GameTooltip:SetOwner(self, "ANCHOR_BOTTOMRIGHT");
 
            self:UpdateTooltip();
        end);
        RtheD:SetScript("OnLeave", function()
            GameTooltip:Hide();
        end);
 
        RtheD.cd = cd;
        RtheD.texture = texture;
 
        RtheDs[i] = RtheD;
    end
 
    self.RtheDs = RtheDs;
end
 
function OutlawFrame:OnEvent(event, ...)
    if event == "PLAYER_LOGIN" then
        self:CreateRtheDs();
 
        if select(1, GetSpecializationInfo(GetSpecialization())) ~= 260 then
            self:Hide();
        else
            for i = 1, #(self.RtheDs) do
                self.RtheDs[i]:SetAlpha(0.4);
            end
        end
    elseif event == "PLAYER_ENTERING_WORLD" or event == "UNIT_AURA" then
        for i = 1, #(self.RtheDs) do
            local name = GetSpellInfo(self.RtheDs[i].sID);
 
            local _, _, _, _, _, duration, expirationTime = UnitAura("player", name);
 
            if duration and duration > 0 then
                self.RtheDs[i]:SetAlpha(1);
                self.RtheDs[i].cd:SetCooldown(expirationTime - duration, duration);
                self.RtheDs[i].cd:Show();
            else
                self.RtheDs[i]:SetAlpha(0.4);
                self.RtheDs[i].cd:Hide();
            end
        end
    elseif event == "PLAYER_SPECIALIZATION_CHANGED" then
        if select(1, GetSpecializationInfo(GetSpecialization())) == 260 then
            self:Show();
        else
            self:Hide();
        end
    end
end
 
OutlawFrame:SetScript("OnEvent", OutlawFrame.OnEvent);

I would appreciate any feedback, advice as well as criticism

Thank you!

[myrroddin]

(Re)creating frames every time those functions get called, or the for loop runs gives me cold shivers. Create all those frames outside your functions and loops; then alter the parameters when the fuctions and loops get called.

[Layback_]

Originally Posted by myrroddin (Re)creating frames every time those functions get called, or the for loop runs gives me cold shivers. Create all those frames outside your functions and loops; then alter the parameters when the fuctions and loops get called.

So, you are saying that I would better create those 6 frames outside OutlawFrame:CreateRtheDs() function and for loop, and call a function that textures, set tooltip and so on with different parameters (which would possibly be spell id)?

like:

RtheDs[1] = CreateFrame("Frame", "$parentBuff" .. 1, RtheDs);
RtheDs[1]:CreateArtWork(param);
RtheDs[2] = CreateFrame("Frame", "$parentBuff" .. 2, RtheDs);
RtheDs[2]:CreateArtWork(param);
RtheDs[3] = CreateFrame("Frame", "$parentBuff" .. 3, RtheDs);
RtheDs[3]:CreateArtWork(param);
RtheDs[4] = CreateFrame("Frame", "$parentBuff" .. 4, RtheDs);
RtheDs[4]:CreateArtWork(param);
RtheDs[5] = CreateFrame("Frame", "$parentBuff" .. 5, RtheDs);
RtheDs[5]:CreateArtWork(param);
RtheDs[6] = CreateFrame("Frame", "$parentBuff" .. 6, RtheDs);
RtheDs[6]:CreateArtWork(param);

or

for i = 1, 6 do
    RtheDs[i] = CreateFrame("Frame", "$parentBuff" .. i, RtheDs);
    RtheDs[i]:CreateArtWork(param);
end

[Fizzlemizz]

The bottom line is, once a frame is created, it is never destroyed, de-initialised, gotten rid of, garbage collected until you logout,/reload.

The same goes for any child of the frame (textures, fontstrings, subframes....).

Create once and re-use as required.

[Layback_]

Originally Posted by Fizzlemizz The bottom line is, once a frame is created, it is never destroyed, de-initialised, gotten rid of, garbage collected until you logout,/reload. The same goes for any child of the frame (textures, fontstrings, subframes....). Create once and re-use as required.

Hi Fizzlemizz,

So, you mean by that this is inefficient design in terms of memory management as frames are created every single time when "PLAYER_LOGIN" event is fired rather than create once and re-use as you said and that's why myrroddin also said so?

Am I getting it correct?

[Fizzlemizz]

PLAYER_LOGIN only fires once (in terms of a game session when a character logs in). Ideal time FOR YOU to create YOUR frames that you are going to use/reuse for a session.

In reality, it doesn't matter when you create a frame, you need a mechanism to not keep re-creating it over and over.

[Layback_]

Originally Posted by Fizzlemizz PLAYER_LOGIN only fires once (in terms of a game session when a character logs in). Ideal time FOR YOU to create YOUR frames that you are going to use/reuse for a session. In reality, it doesn't matter when you create a frame, you need a mechanism to not keep re-creating it over and over.

So... the simplest mechanism to prevent frames being created over and over again would be using if statement like...

for i = 1, #RtheDsList do
    if not RtheDs[i] then
        ...
    end
end

is it....?


EDIT: Does such mechanism necessary even if I know that the frames will be created only once? I mean I know that having one is better than none, but still ~_~

[Lombra]

If you're already creating the frames on PLAYER_LOGIN (I don't know if you editted that in afterwards), you're fine.

[Layback_]

Originally Posted by Lombra If you're already creating the frames on PLAYER_LOGIN (I don't know if you editted that in afterwards), you're fine.

Hi Lombra,

Yeah, frames are only created via "PLAYER_LOGIN" event and won't be modified at all

Thanks for letting me know!

[Fizzlemizz]

Originally Posted by Layback_ EDIT: Does such mechanism necessary even if I know that the frames will be created only once? I mean I know that having one is better than none, but still ~_~

Creating them only at PLAYER_LOGIN is essentialy the mechanism to ensure they're created once.

[Phanx]

local font = LSM:Fetch("font", "koverwatch");

This line is problematic "in the wild", because a third-party font may not be registered with LSM yet when your addon's main chunk executes. You should wait for PLAYER_LOGIN to apply LSM media.

OutlawFrame:RegisterEvent("UNIT_AURA");

You should use RegisterUnitEvent with "player" as the second argument, instead of RegisterEvent, to limit your frame to receive UNIT_AURA events only for yourself. Otherwise you're going to run your event handler every time anyone's auras change, which can be thousands of times per second in a raid.

	local RtheDs = CreateFrame("Frame", "$parentRollTheDice", self);
	RtheDs:SetAllPoints(true);

What's the point of this intermediary frame? Just parent the children directly to your main frame.

		RtheD.sID = RtheDsList[i];

Since this is an addon, not a macro, and space is unlimited, you should avoid using cryptic variable names like "RtheD" and "sID" that may or may not make sense when you come back to your code after 6 months or a year, or to other people reading your code. There's no reason to not write out "RollTheDice" and "spellID" here.

		local texture = RtheD:CreateTexture("$parentIcon", "Overlay");

"OVERLAY" should be in all-caps here. Frame stratas, draw layers, fontstring flags, and named points are written in all-caps; unit tokens are in all-lowercase; etc. While in most (if not all) cases the API will auto-correct it for you, you should get in the habit of using the correct casing in your code, not only because it's correct, but also because if you ever need to compare against such a value returned by the API (eg. GetPoint) then your comparison will fail if you're using incorrect casing.

		RtheD:SetScript("OnEnter", function(self)
			if not self:IsVisible() then
				return;
			end

I don't think there's any way for a hidden frame to recieve an OnEnter event, so you don't need that check.

		if select(1, GetSpecializationInfo(GetSpecialization())) ~= 260 then

There is absolutely no reason to use "select(1, ...)", under any circumstances, ever. It does literally the exact same thing as just "...", but adds the overhead of an extra function call, which is the slowest single thing you can do in Lua.

"select" in general should generally be avoided. Instead of writing "local x = select(2, GetValues())" just write "local _, x = GetValues()". The only place where "select" is not a total waste of CPU time in an addon is using "select('#')" to count the values in a vararg. In all other cases you should just assign out all the variables. Using an underscore as the variable name is a common paradigm for making it obvious that the variable's value isn't used, but the underscore doesn't actually have any special behavior; it's just a variable name like any other.

(In macros, "select" may be useful to save space, but as already mentioned, there's no limit on space in an addon, so that should never be a consideration.)

		for i = 1, #(self.RtheDs) do
			local name = GetSpellInfo(self.RtheDs[i].sID);

1. You don't need parentheses around "self.RtheDs" (or semicolons at the end of lines) in Lua. If you want to type extra stuff, they won't hurt anything, but they're not necessary either, so feel free to leave them out.

2. Rather than writing "self.RtheDs[i]" over and over in this section (and others like it) I'd recommend assigning that to a local variable, and referring to the variable instead:

		for i = 1, #(self.RtheDs) do
			local object = self.RtheDs[i]
			local name = GetSpellInfo(object.sID);

[Banknorris]

Originally Posted by Phanx There is absolutely no reason to use "select(1, ...)", under any circumstances, ever. It does literally the exact same thing as just "...", but adds the overhead of an extra function call, which is the slowest single thing you can do in Lua. "select" in general should generally be avoided. Instead of writing "local x = select(2, GetValues())" just write "local _, x = GetValues()". The only place where "select" is not a total waste of CPU time in an addon is using "select('#')" to count the values in a vararg. In all other cases you should just assign out all the variables. Using an underscore as the variable name is a common paradigm for making it obvious that the variable's value isn't used, but the underscore doesn't actually have any special behavior; it's just a variable name like any other.

According to these tests, select is not that bad
http://www.wowinterface.com/forums/s...4&postcount=23
http://www.wowinterface.com/forums/s...1&postcount=26

[Layback_]

Originally Posted by Fizzlemizz Creating them only at PLAYER_LOGIN is essentialy the mechanism to ensure they're created once.

So, I did make such mechanism without my being aware of it!

YEAY!!

Originally Posted by Phanx 2. Rather than writing "self.RtheDs[i]" over and over in this section (and others like it) I'd recommend assigning that to a local variable, and referring to the variable instead: for i = 1, #(self.RtheDs) do local object = self.RtheDs[i] local name = GetSpellInfo(object.sID);

Thank you so much for such a detailed feedback.

I made all modifications based on your advice.

Just a quick last question regarding the second point of last feedback.

If I am understanding properly, lua actually copies the value of other variable via assignment rather than copying the address of the original variable.

Would there be any necessity to copy the value (or object) to another variable called object rather than directly accessing it like what I have originally done?

Originally Posted by Banknorris According to these tests, select is not that bad http://www.wowinterface.com/forums/s...4&postcount=23 http://www.wowinterface.com/forums/s...1&postcount=26

Hmmmmm...

I might have to do some further research by myself

[Phanx]

Originally Posted by Layback_ If I am understanding properly, lua actually copies the value of other variable via assignment rather than copying the address of the original variable.

Tables and functions are copied by reference, and as far as Lua is concerned, a frame is just a table with a userdata value at index 0. If you define 50000 variables pointing to a frame or function, they all point to the same instance of the frame or function in memory.

You are correct for other data types, although strings in Lua are interned, so even if you write somevar = "foo" 20 times there's still only one copy of the string "blabla" in memory, and all the variables point to it. Strings are also immutable in Lua, so somevar = somevar .. "bar" doesn't modify the original string "foo" -- it creates a new string "foobar", and redirects the "somevar" variable to point to the new string; any other variables that were pointing to the string "foo" are not affected at all.

Originally Posted by Layback_ Would there be any necessity to copy the value (or object) to another variable called object rather than directly accessing it like what I have originally done?

No. Creating a variable pointing to a value otherwise repeatedly accessed through a chain of table lookups does save you a bunch of table lookups, but the main reason to do it is because it's easier to type and easier to read.

[Phanx]

Originally Posted by Banknorris According to these tests, select is not that bad http://www.wowinterface.com/forums/s...4&postcount=23 http://www.wowinterface.com/forums/s...1&postcount=26

Whenever you're benchmarking anything, the relative results between similar tests is generally what's important, and the absolute results between unrelated tests doesn't matter at all. Those tests are all about string manipulation, which is already slow and expensive, so the results aren't particularly relevant to whether or not using select is more expensive than not using select (hint: it is).

Sure, you can say "well, it only takes 300ms to run this code 1 million times, but in actual use it's only running once, so it's pretty fast and I don't need to worry about it", and you're not wrong, but if you can rewrite that code so that it only takes 150ms to run 1m iterations -- and becomes more straightforward and readable as a bonus -- that's obviously better, and you should do that.

In fact, making the code more straightforward and readable is just as important as, if not more important than, making it faster.

Compare:

for i = 1, 1000000 do
    local specID = GetSpecializationInfo(GetSpecialization())
end

for i = 1, 1000000 do
    local specID = select(1, GetSpecializationInfo(GetSpecialization()))
end

The first version is faster and more readable. The second version is slower, less readable, and functionally identical, so there's absolutely no reason to ever use it.

I'm currently at work, but I'll do an actual benchmark on this later and update this post with the results.

Edit: Average time on my machine for the first version (1 million iterations per test, did 10 tests and averaged the results) is 906ms, vs 1077ms for the second version. That's an 18% performance boost just from removing an extra function call that doesn't do anything anyway. Results are about the same for using select to grab returns other than the first, eg.

for i = 1, 1000000 do
    local _, specName = GetSpecializationInfo(GetSpecialization())
end

for i = 1, 1000000 do
    local specName= select(2, GetSpecializationInfo(GetSpecialization()))
end

There's just no good argument in favor of using select for dealing with simple "call a function and assign its return values to variables" tasks when using it is universally slower than not using it.

[SDPhantom]

If you really want to single out the impact of using select(), why not isolate it using the least operations possible?



For example:

local x=1;

versus

local x=select(1,1);

On another note, I read an 8ms difference in comparison. Both snips of code used the same operations on the same string. the only difference was running the output through select() or singling out the value's position manually.

In the end, all you're doing isolating out common operations is trimming the variance in the Lua engine running the code.