Roll the Bones addon for Rogue

[Layback_]

Hi all,

I am currently trying to create Roll the Bones indicator for Outlaw Rogue and here is a draft for what I have done so far.
(Pretty sure I made something similar to this before, but w/e...)

if select(2, UnitClass("player")) ~= "ROGUE" then
    return;
end
 
-----------------------------------------------------
-- Variables
-----------------------------------------------------
local RolltheBonesID = {
    193356, -- Broadsides
    193357, -- Shark Infested Waters
    193358, -- Grand Melee
    199603, -- Jolly Roger
    199600, -- Buried Treasure
    193359, -- True Bearing
}
 
local UpdateRolltheBonesBuff, CheckPlayerSpec;
 
-----------------------------------------------------
-- Frames
-----------------------------------------------------
local RolltheBones = CreateFrame("Frame", "RolltheBones", UIParent);
RolltheBones:RegisterEvent("PLAYER_ENTERING_WORLD");
RolltheBones:RegisterEvent("PLAYER_SPECIALIZATION_CHANGED");
RolltheBones:RegisterUnitEvent("UNIT_AURA", "player");
RolltheBones:SetSize(180, 180);
RolltheBones:SetPoint("CENTER", 0, -200);
 
RolltheBones.Dice = {};
for i = 1, 6 do
    local Dice = CreateFrame("Button", "$parentDice" .. i, RolltheBones);
    Dice:SetSize(40, 40);
    Dice:SetPoint("CENTER", 60 * math.cos(i * math.pi / 3), 60 * math.sin(i * math.pi / 3));
 
    local Cooldown = CreateFrame("Cooldown", "$parentCooldown", Dice, "CooldownFrameTemplate");
    Cooldown:SetAllPoints(true);
 
    local Icon = Dice:CreateTexture("$parentIcon", "OVERLAY");
    Icon:SetAllPoints(true);
    Icon:SetTexture(GetSpellTexture(RolltheBonesID[i]));
 
    Dice.Cooldown = Cooldown;
    Dice.Icon = Icon;
 
    RolltheBones.Dice[i] = Dice;
end
 
-----------------------------------------------------
-- Functions
-----------------------------------------------------
-- Update the buff frame based on its status
function UpdateRolltheBonesBuff()
    for i = 1, #RolltheBonesID do
        local spellName = GetSpellInfo(RolltheBonesID[i]);
        local _, _, _, _, _, duration, expires = UnitAura("player", spellName);
 
        -- TO-DO: Condition check needed to prevent :SetCooldown being called on UNIT_AURA event by irrelevant aura
        if duration then
            RolltheBones.Dice[i].Cooldown:SetCooldown(expires - duration, duration);
        else
            -- Resets the duration to 0 when the buff is removed before it is expired
            -- (when 'Roll the bones' is fired again)
            RolltheBones.Dice[i].Cooldown:SetCooldown(0, 0);
        end
    end
end
 
-- Check player's spec and returns bool value
function CheckPlayerSpec()
    local playerSpecID = GetSpecializationInfo(GetSpecialization());
 
    if playerSpecID ~= 260 then
        RolltheBones:Hide();
        RolltheBones:SetScript("OnEvent", nil);
        RolltheBones:UnregisterEvent("UNIT_AURA");
 
        return false;
    else
        RolltheBones:Show();
        RolltheBones:SetScript("OnEvent", RolltheBones.OnEvent);
        RolltheBones:RegisterEvent("UNIT_AURA");
 
        return true;
    end
end
 
function RolltheBones:OnEvent(event, ...)
    self[event](self);
end
 
function RolltheBones:PLAYER_ENTERING_WORLD()
    if CheckPlayerSpec() then
        UpdateRolltheBonesBuff();
    end
 
    self:UnregisterEvent("PLAYER_ENTERING_WORLD");
end
 
function RolltheBones:PLAYER_SPECIALIZATION_CHANGED()
    if CheckPlayerSpec() then
        UpdateRolltheBonesBuff();
    end
end
 
function RolltheBones:UNIT_AURA(unitID)
    UpdateRolltheBonesBuff();
end
 
RolltheBones:SetScript("OnEvent", RolltheBones.OnEvent);

There are two things that I am unhappy with.

1. Structure of local functions, CheckPlayerSpec & UpdateRolltheBonesBuff

2. Their usage

I don't know why, but they just don't feel right to me and both their structure and usage seems soooooooooooooooooooo awkward to me.

Could I get some advice, please?

Also, any recommendation or criticism are welcomed !!

Thank you.

[pas06]

You don't have to do frame:SetScript("OnEvent", xxxxx) more than once. You set that Script once and whenever a event thats registered to that frame fires the function(xxxxx) gets called.

local RolltheBonesID = {
    193356, -- Broadsides
    193357, -- Shark Infested Waters
    193358, -- Grand Melee
    199603, -- Jolly Roger
    199600, -- Buried Treasure
    193359, -- True Bearing
}

what about saving the spellnames like this:

local RolltheBonesBuffNames = {
    GetSpellInfo(193356), -- Broadsides
    GetSpellInfo(193357), -- Shark Infested Waters
    GetSpellInfo(193358), -- Grand Melee
    GetSpellInfo(199603), -- Jolly Roger
    GetSpellInfo(199600), -- Buried Treasure
    GetSpellInfo(193359), -- True Bearing
}

Then you don't have to run the function GetSpellInfo that often, you don't have to access the spellID anyways in the current state.

Another problem is that you use SetCooldown a lot. Maybe something like this would be better:

function RolltheBones:UNIT_AURA(unitID)
    for i, #RolltheBonesBuffNames do
        local name, _, _, _, _, duration, expires = UnitBuff(unitID, RolltheBonesBuffNames[i])
        if name then
            if not HasBuff[i] then --player just got the buff
                HasBuff[i] = true
                RolltheBones.Dice[i].Cooldown:SetCooldown(expires - duration, duration)
            end
        elseif HasBuff[i] thenH
            HasBuff[i] = false
            RolltheBones.Dice[i].Cooldown:SetCooldown(0, 0)
        end
    end
end

But i am not sure what happens if you reroll the bones and replace buff 1 with buff 1. The Code should work if it works the following: reroll > UNIT_AURA fires > buff disappears for a short peroid of time(gets replaced by the same buff) > UNIT_AURA fires > only new buff exists. Feel free to test it.

The whole code:

if select(2, UnitClass("player")) ~= "ROGUE" then
    return
end
 
-----------------------------------------------------
-- Variables
-----------------------------------------------------
local RolltheBonesBuffNames = {
    GetSpellInfo(193356), -- Broadsides
    GetSpellInfo(193357), -- Shark Infested Waters
    GetSpellInfo(193358), -- Grand Melee
    GetSpellInfo(199603), -- Jolly Roger
    GetSpellInfo(199600), -- Buried Treasure
    GetSpellInfo(193359), -- True Bearing
}
local HasBuff = {}
-----------------------------------------------------
-- Frames
-----------------------------------------------------
local RolltheBones = CreateFrame("Frame", "RolltheBones", UIParent)
RolltheBones:SetScript("OnEvent", function(self, event, ...) self[event](self, ...))
RolltheBones:RegisterEvent("PLAYER_ENTERING_WORLD")
RolltheBones:RegisterEvent("PLAYER_SPECIALIZATION_CHANGED")
RolltheBones:SetSize(180, 180)
RolltheBones:SetPoint("CENTER", 0, -200)
 
RolltheBones.Dice = {}
for i = 1, 6 do
    local Dice = CreateFrame("Button", "$parentDice" .. i, RolltheBones)
    Dice:SetSize(40, 40)
    Dice:SetPoint("CENTER", 60 * math.cos(i * math.pi / 3), 60 * math.sin(i * math.pi / 3))
 
    local Cooldown = CreateFrame("Cooldown", "$parentCooldown", Dice, "CooldownFrameTemplate")
    Cooldown:SetAllPoints(true)
 
    local Icon = Dice:CreateTexture("$parentIcon", "OVERLAY")
    Icon:SetAllPoints(true)
    Icon:SetTexture(GetSpellTexture(RolltheBonesID[i]))
 
    Dice.Cooldown = Cooldown
    Dice.Icon = Icon
 
    RolltheBones.Dice[i] = Dice
end
 
-----------------------------------------------------
-- Functions
-----------------------------------------------------
 
-- Check player's spec and returns bool value
function RolltheBones:CheckPlayerSpec()
    local playerSpecID = GetSpecializationInfo(GetSpecialization())
 
    if playerSpecID ~= 260 then
        self:Hide()
        self:UnregisterEvent("UNIT_AURA")
    else
        self:Show()
        self:RegisterUnitEvent("UNIT_AURA", "player")
    end
end
 
function RolltheBones:PLAYER_ENTERING_WORLD()
    self:CheckPlayerSpec()
    self:UnregisterEvent("PLAYER_ENTERING_WORLD")
end
 
function RolltheBones:PLAYER_SPECIALIZATION_CHANGED()
    self:CheckPlayerSpec()
end
 
function RolltheBones:UNIT_AURA(unitID)
    for i, #RolltheBonesBuffNames do
        local name, _, _, _, _, duration, expires = UnitBuff(unitID, RolltheBonesBuffNames[i])
        if name then
            if not HasBuff[i] then --player just got the buff
                HasBuff[i] = true
                RolltheBones.Dice[i].Cooldown:SetCooldown(expires - duration, duration)
            end
        elseif HasBuff[i] thenH
            HasBuff[i] = false
            RolltheBones.Dice[i].Cooldown:SetCooldown(0, 0)
        end
    end
end

Feel free to test it. i am pretty sure that someone comes up with a better idea afterwards

[Layback_]

Hi pas06,

Thank you for your reply.

1. For :SetScript("OnEvent", ...), I was trying to minimize the memory usage when it is not needed, but I definitely should have called RegisterUnitEvent, not RegisterEvent for UNIT_AURA

2. I also tried that approach for Roll the Bones id table, but the way you've done it will insert the rest of GetSpellInfo(193359) data into the table (like rank, icon, castingTime, minRange, maxRange, spellID) as well.

3. Same for UNIT_AURA function. I've tried a same approach and as you've guessed, there was a small taint which I really didn't like

[Layback_]

Here's a fix for UpdateRolltheBonesBuff function.

function UpdateRolltheBonesBuff(i)
    local spellName = GetSpellInfo(RolltheBonesID[i]);
    local _, _, _, _, _, duration, expires = UnitAura("player", spellName);
 
    if duration then
        -- Prevents from redundant SetCooldown function called on UNIT_AURA event
        if not RolltheBones.Dice[i].IsRunning then
            RolltheBones.Dice[i].Cooldown:SetCooldown(expires - duration, duration);
 
            RolltheBones.Dice[i].IsRunning = true;
        else
            local startTime = RolltheBones.Dice[i].Cooldown:GetCooldownTimes();
 
            if startTime / 1000 < expires - duration then
                RolltheBones.Dice[i].Cooldown:SetCooldown(expires - duration, duration);
            end
        end
    else
        -- Resets the duration to 0 when the buff is removed before it is expired
        -- e.g. When Roll the dice is fired again
        RolltheBones.Dice[i].Cooldown:SetCooldown(0, 0);
 
        RolltheBones.Dice[i].IsRunning = false;
    end
end

Currently it is working as I expected

[myrroddin]

Your function UpdateRolltheBonesBuff should be local.

[Layback_]

Yeap, it is declared as local on variables section (L#17) !!