Need help for troubleshooting my addon

Sylen · 08-01-14, 11:49 AM

Hey there, i would appreciate some help in troubleshooting my addon. It's the first addon i have ever written and now i am unable to find my mistake.
Basicly this error message pops up everytime i log into the game. Nevertheless it won't show up again if i /reload ui. So it only occurs at my first login.
My theory: some of the data i call is not available dircetly after login (?)

The error message:

Code:

Message: Interface\AddOns\Shot Indicator\Shot Indicator.lua:68: attempt to perform arithmetic on upvalue 'mainSpellCost' (a nil value)
Time: 08/01/14 19:45:40
Count: 1
Stack: Interface\AddOns\Shot Indicator\Shot Indicator.lua:68: in function `?'
Interface\AddOns\Shot Indicator\Shot Indicator.lua:36: in function <Interface\AddOns\Shot Indicator\Shot Indicator.lua:36>

Locals: self = <unnamed> {
 0 = <userdata>
 ADDON_LOADED = <function> defined @Interface\AddOns\Shot Indicator\Shot Indicator.lua:39
 PLAYER_ENTERING_WORLD = <function> defined @Interface\AddOns\Shot Indicator\Shot Indicator.lua:58
 PLAYER_SPECIALIZATION_CHANGED = <function> defined @Interface\AddOns\Shot Indicator\Shot Indicator.lua:65
 texture = <unnamed> {
 }
}
(*temporary) = <function> defined =[C]:-1
(*temporary) = <unnamed> {
 0 = <userdata>
 ADDON_LOADED = <function> defined @Interface\AddOns\Shot Indicator\Shot Indicator.lua:39
 PLAYER_ENTERING_WORLD = <function> defined @Interface\AddOns\Shot Indicator\Shot Indicator.lua:58
 PLAYER_SPECIALIZATION_CHANGED = <function> defined @Interface\AddOns\Shot Indicator\Shot Indicator.lua:65
 texture = <unnamed> {
 }
}
(*temporary) = "LEFT"
(*temporary) = PlayerFrameManaBar {
 0 = <userdata>
 currValue = 0
 cvarLabel = "STATUS_TEXT_PLAYER"
 prefix = "Mana"
 textLockable = 1
 disconnected = true
 unit = "player"
 cvar = "playerStatusText"
 capNumericDisplay = true
 TextString = PlayerFrameManaBarText {
 }
 powerType = 0
 frequentUpdates = true
 lockShow = 0
}
(*temporary) = <function> defined =[C]:-1
(*temporary) = 119
(*temporary) = 119
(*temporary) = nil
(*temporary) = nil
(*temporary) = "attempt to perform arithmetic on upvalue 'mainSpellCost' (a nil value)"
f = <unnamed> {
 0 = <userdata>
 ADDON_LOADED = <function> defined @Interface\AddOns\Shot Indicator\Shot Indicator.lua:39
 PLAYER_ENTERING_WORLD = <function> defined @Interface\AddOns\Shot Indicator\Shot Indicator.lua:58
 PLAYER_SPECIALIZATION_CHANGED = <function> defined @Interface\AddOns\Shot Indicator\Shot Indicator.lua:65
 texture = <unnamed> {
 }
}
w = 119
mainSpellCost = nil

My addon code:

Code:

--get locales
local playerClass = select(2, UnitClass("player"))	
local w = PlayerFrameManaBar:GetWidth()
local h = PlayerFrameManaBar:GetHeight()
local mainSpells = {[253] = 34026, [254] = 53209, [255] = 53301}	--kill command, chimera shot, explosive shot
local mainSpellCost = 0

--update spellcost of main spells
function updateSpellCost()	
	local currentSpecId = select(1, GetSpecializationInfo(GetSpecialization()))	
	local currentSpellCost = select(4, GetSpellInfo(mainSpells[currentSpecId]))	
	if (currentSpellCost ~= 0) then		
		mainSpellCost = currentSpellCost
	end
	return mainSpellCost
end

--update max power
function updateMaxPower()
	local currentMaxPower = UnitPowerMax("player")
	if (currentMaxPower ~= 0) then		
		maxPower = currentMaxPower
	end
	return maxPower
end

--create frame
local f = CreateFrame("Frame") 
	f:SetSize(2,h)	
	f:SetPoint("LEFT", PlayerFrameManaBar, 0, 0)	
	f:RegisterEvent("PLAYER_ENTERING_WORLD")
	f:RegisterEvent("ADDON_LOADED")	
	f:RegisterEvent("PLAYER_SPECIALIZATION_CHANGED")
	--f:RegisterEvent("ACTIVE_TALENT_GROUP_CHANGED")		
	
f:SetScript("OnEvent", function(self, event, ...) self[event](self, ...) end)

--should addon be loaded?
function f:ADDON_LOADED(AddOnName)
	if (AddOnName == "Shot Indicator") and (playerClass ~= "HUNTER") then
		DisableAddOn("Shot Indicator")
	else
		print("Shot Indicator succesfully loaded!")
	end
	f:UnregisterEvent("ADDON_LOADED") 
end

--texture stuff	
f.texture = f:CreateTexture(nil, "OVERLAY")
	f.texture:ClearAllPoints()
	f.texture:SetAllPoints(f)
	f.texture:SetTexture("Interface\\CastingBar\\UI-CastingBar-Spark")
	f.texture:SetTexture(0, 130, 245, 1)
	f.texture:SetBlendMode("ADD")
	f.texture:Hide()

--show the indicator
function f:PLAYER_ENTERING_WORLD()
	updateMaxPower()
	updateSpellCost()
	f:SetPoint("LEFT", PlayerFrameManaBar, math.floor((w-w*(1-mainSpellCost/maxPower))), 0)		
	f.texture:Show()
end
	
function f:PLAYER_SPECIALIZATION_CHANGED()
	updateMaxPower()
	updateSpellCost()
	f:SetPoint("LEFT", PlayerFrameManaBar, math.floor((w-w*(1-mainSpellCost/maxPower))), 0)		
	f.texture:Show()
end
--[[
function f:ACTIVE_TALENT_GROUP_CHANGED()
	updateMaxPower()
	updateSpellCost()
	f:SetPoint("LEFT", PlayerFrameManaBar, math.floor((w-w*(1-mainSpellCost/maxPower))), 0)		
	f.texture:Show()
end
]]

Nynaeve · 08-01-14, 12:41 PM

I'm no expert at reading error codes, but it seems to not like your maths. Sub line 58 (your entering world call), it seems to believe you have done something untoward with your mainSpellCost maths. That's what I got from the error code, anyway? Did you have multiple error codes? Seems odd that it would only call out line 68 and not 61 and 75.

(Like I said, not an expert on reading error codes, so someone correct me if I'm wrong. Also, this is just from reading the error. I'm not entirely certain what your addon is supposed to do. Track spell costs and focus?)

**Seerah** · 08-01-14, 01:15 PM

Well, it's not erroring for line 75 because that block is commented out.

Lua error messages are quite nice 90% of the time, and tell you exactly what's wrong. The stack trace you provided also lists the values of your variables at the time of the error.

Message: Interface\AddOns\Shot Indicator\Shot Indicator.lua:68: attempt to perform arithmetic on upvalue 'mainSpellCost' (a nil value)
.
.
.
mainSpellCost = nil

Let's break this down...

First the error provides the file where the error occurred and the specific line number of the offending bit of code.
Next, it's telling you that the error had to do with the arithmetic portion of that line.
The problem was with the upvalue 'mainSpellCost' (it's an upvalue, because you declared it at the top of your file, outside the scope of the function).
The reason why the arithmetic failed is because 'mainSpellCost' is nil.
At the bottom of the stack trace, you find that 'mainSpellCost' does indeed have the value of nil assigned to it.

Since, when you declared this upvalue, you specifically gave it a value of 0, then your issue lies where you reassign a different value to the variable.

Lua Code:

if (currentSpellCost ~= 0) then     
    mainSpellCost = currentSpellCost
end

Since the error is occuring in the function for the PLAYER_SPECIALIZATION_CHANGED event and not in PLAYER_ENTERING_WORLD, perhaps this event is firing before PEW. It does fire for party and raid members also.

Also, talent info may not be available at PLAYER_ENTERING_WORLD.

If you read here, you will find:

Most information about the game world should now be available to the UI. If this is an interface reload rather than a fresh log in, talent information should also be available.

So the error would likely occur in that section of the code as well. (Perhaps it did, and you really have two errors? In that case, PLAYER_SPECIALIZATION_CHANGED would be firing after PEW.)

Try changing your code to this:

Lua Code:

if currentSpellCost ~= 0 and currentSpellCost ~= nil then       
    mainSpellCost = currentSpellCost
end

Oh! Also, make your two functions, updateSpellCost and updateMaxPower, local so that you're not polluting the global namespace.

SDPhantom · 08-01-14, 02:07 PM

It looks like it's all coming from GetSpellInfo() in updateSpellCost(). The first time you use GetSpellInfo() in a play session and the spell you're querying hasn't been seen by the game client in that session before, it will return nil. Every query for the same spell afterward (even after a UI reload) will provide the necessary information. The reason the error doesn't persist through a UI reload is because WoW only restarts the Lua interpreter and reloads all the addon data. It doesn't go through and clear its internal data caches.

Also...

Originally Posted by Sylen

lua Code:

--should addon be loaded?
function f:ADDON_LOADED(AddOnName)
    if (AddOnName == "Shot Indicator") and (playerClass ~= "HUNTER") then
        DisableAddOn("Shot Indicator")
    else
        print("Shot Indicator succesfully loaded!")
    end
    f:UnregisterEvent("ADDON_LOADED") 
end

This will make it so if a player logs on a non-Hunter, it'll never load again, even if the player were to log on a Hunter later. DisableAddOn() goes into the addon manager and tells it to stop loading your addon for future play sessions. This is essentially unchecking your addon in the addon window at the character select screen.

To make sure your addon only loads on hunters, you need to have your addon check before running ANY other code and stop itself if needed. To do this, you put this line at the top of your addon.

Code:

if select(2,UnitClass("player"))~="HUNTER" then return; end

Note, calling return in the main chunk of a Lua file will cause the interpreter to stop processing further statements. This is because Lua runs every file as the body of its own function.

semlar · 08-01-14, 02:31 PM

Originally Posted by SDPhantom

The first time you use GetSpellInfo() in a play session and the spell you're querying hasn't been seen by the game client in that session before, it will return nil.

I don't think spells need to be cached, I'm pretty sure this error is just from the function being called before the player's spec is available.

Spell data is all stored in the client, so it doesn't need to request the information from the server.

SDPhantom · 08-01-14, 06:08 PM

It seems PLAYER_SPECIALIZATION_CHANGED is the culprit in this as it kept firing before PLAYER_LOGIN even though specialization data wasn't available. GetSpecialization() returns the correct value while GetSpecializationInfo() returns nil. For some reason, nil is now allowed to propagate through table indexing as a key and caused the whole chain reaction.

The following code has some other fixes and optimizations.

lua Code:

if select(2,UnitClass("player"))~="HUNTER" then return; end
 
local w,h=PlayerFrameManaBar:GetSize();
local mainSpells={
    [253] = 34026;  -- kill command
    [254] = 53209;  -- chimera shot
    [255] = 53301;  -- explosive shot
};
 
--create frame
local f=CreateFrame("Frame");
f:SetSize(2,h);
f:SetPoint("LEFT",PlayerFrameManaBar);
f:RegisterEvent("PLAYER_LOGIN");
f:RegisterEvent("PLAYER_SPECIALIZATION_CHANGED");
 
--texture stuff
f.texture=f:CreateTexture(nil,"OVERLAY");
f.texture:SetAllPoints(f);
--f.texture:SetTexture("Interface\\CastingBar\\UI-CastingBar-Spark");
f.texture:SetTexture(0,0.51,0.96,1);
f.texture:SetBlendMode("ADD");
 
f:SetScript("OnEvent",function(self,event,...)
    if self.dataAvailable or event=="PLAYER_LOGIN" then
        local max=UnitPowerMax("player");
        local _,_,_,cost=GetSpellInfo(mainSpells[GetSpecializationInfo(GetSpecialization())]);
 
        self:SetPoint("LEFT",PlayerFrameManaBar,w*cost/max,0);
        self.dataAvailable=true;
    end
end)

Note setting a texture, then using texture:SetTexture() to set a color will overwrite the texture with a solid box of the specified color. If you wish to use a texture and change the color of it, you need to use texture:SetVertexColor(). Also, color values are percentage ranges between 0 and 1.

EDIT: Removed the texture hide and show calls. These shouldn't be needed since the texture is being created, hidden, then immediately and permanently shown.

Phanx · 08-01-14, 07:14 PM

Originally Posted by SDPhantom

This will make it so if a player logs on a non-Hunter, it'll never load again, even if the player were to log on a Hunter later. DisableAddOn() goes into the addon manager and tells it to stop loading your addon for future play sessions. This is essentially unchecking your addon in the addon window at the character select screen.

Not quite correct. When you call DisableAddOn while in-game, it only affects the addon's state for the currently logged-in character. It's the same as if you'd selected the character in the dropdown list at the top of the addon list at the character screen before checking or unchecking the box next to the addon. Calling DisableAddOn for non-hunter characters is perfectly safe; I've done this in several of my own addons since WoW 1.x without issue.

However, this is still weird:

Code:

function f:ADDON_LOADED(AddOnName)
    if (AddOnName == "Shot Indicator") and (playerClass ~= "HUNTER") then
        DisableAddOn("Shot Indicator")
    else
        print("Shot Indicator succesfully loaded!")
    end
    f:UnregisterEvent("ADDON_LOADED") 
end

1) Avoid using spaces in your addon folder/TOC name. Use "ShotIndicator" or "Shot_Indicator" instead.

2) Currently you're saying "if this event is for this addon and the player isn't a hunter, disable this addon, otherwise -- no matter which addon this event is for -- spam the chat frame with a message saying the addon was loaded". Unless you specifically want to detect and respond to other addons' loading events, check the addon first, and only proceed with anything else if it's yours:

Code:

function f:ADDON_LOADED(AddOnName)
    if (AddOnName ~= "Shot Indicator") then return end

    if (playerClass ~= "HUNTER") then
        DisableAddOn("Shot Indicator")
    else
        print("Shot Indicator succesfully loaded!")
    end
    f:UnregisterEvent("ADDON_LOADED") 
end

But, as SDPhantom mentioned, you can do the class check at the very top of your file, and avoid creating any frames, registering for any events, or running any other code, since you already know at that point that your addon isn't relevant to the current charater:

Code:

local ADDON_NAME, privateTable = ...
local _, playerClass = UnitClass("player")
if playerClass ~= "HUNTER" then
    return DisableAddOn(ADDON_NAME)
end

-- if you get to this point, go ahead and create your frame, register events, etc.

Then, all you need to do in ADDON_LOADED is initialize your saved variables, if you have any; if not, you can just remove everything related to that event.

Finally, once you get to the point where you're ready to publish your addon for others to use, please do not print "addon loaded" messages to the user's chat frame every time the addon loads. The user knows your addon is loaded -- they installed it, and made sure it was enabled, so of course it's loaded. They don't need to be reminded of this every time they log into the game. While you may not think it's a big deal, imagine what happens if you're using 100 addons, and they all print an "addon loaded" message on login -- that's a lot of spam.

Sylen · 08-02-14, 07:45 AM

Oh! Also, make your two functions, updateSpellCost and updateMaxPower, local so that you're not polluting the global namespace.

I had it local before. Changed it so i could run the functions manually ingame and use print(...) to print some variables and other stuff, so i could see if its working. Totally forgot to change it back

Note setting a texture, then using texture:SetTexture() to set a color will overwrite the texture with a solid box of the specified color. If you wish to use a texture and change the color of it, you need to use texture:SetVertexColor(). Also, color values are percentage ranges between 0 and 1.

I will test this out. If i remember correctly i used to overwrote it because visuality was better, but i will try your suggestion out anyway.

How do i get the percentage ranges for the color values? I got my information from here: http://www.colorschemer.com/online.html

1) Avoid using spaces in your addon folder/TOC name. Use "ShotIndicator" or "Shot_Indicator" instead.

Yes, Sir.

Finally, once you get to the point where you're ready to publish your addon for others to use, please do not print "addon loaded" messages to the user's chat frame every time the addon loads.

Yes, Sir.

My changes:

Simplified locales w,h
Removed the space in the AddOn Name
Changed the way of checking "Should the addon be loaded"
Changed functions to local functions again
Changed the way the texture is set/colored
Changed r,g,b values to percentage values
Commented out PLAYER_SPECIALIZATION_CHANGED, now using ACTIVE_TALENT_GROUP_CHANGED; due to PLAYER_SPECIALIZATION_CHANGED fires when group/raid member also changes spec -> unnecesary information tracked
Changed the way Coords were calculated. Still dont know why i used this horrible way of calculating it lol
Moved some code

First test results:
SDPhantom's Code works just fine after trying it out. But i will still fiddle around with my own one and trying to get it to work for learning effects.

My fine working code:

Code:

--get locales
local ShotIndicator, privateTable = ...
local playerClass = select(2, UnitClass("player"))	
local w, h = PlayerFrameManaBar:GetSize()
local mainSpellCost = 0
local maxPower = 0
local mainSpells = {
	[253] = 34026,	--kill command
	[254] = 53209,	--chimera shot
	[255] = 53301	--explosive shot
}	

--should addon be loaded?
if (playerClass ~= "HUNTER") then
	return DisableAddOn(ShotIndicator)
end

--update spellcost of main spells
local function updateSpellCost()	
	--local currentSpecId = select(1, GetSpecializationInfo(GetSpecialization()))	
	local currentSpellCost = select(4, GetSpellInfo(mainSpells[GetSpecializationInfo(GetSpecialization())]))	
	if (currentSpellCost ~= 0 and currentSpellCost ~= nil) then		
		mainSpellCost = currentSpellCost
	end
	return mainSpellCost
end

--update max power
local function updateMaxPower()
	local currentMaxPower = UnitPowerMax("player")
	if (currentMaxPower ~= 0 and currentMaxPower ~= nil) then		
		maxPower = currentMaxPower
	end
	return maxPower
end

--create frame
local f = CreateFrame("Frame") 
	f:SetSize(2,h)	
	f:SetPoint("LEFT", PlayerFrameManaBar, 0, 0)	
	f:RegisterEvent("PLAYER_LOGIN")
	f:RegisterEvent("ACTIVE_TALENT_GROUP_CHANGED")		

--texture stuff	
f.texture = f:CreateTexture(nil, "OVERLAY")
	--f.texture:ClearAllPoints()
	f.texture:SetAllPoints(f)
	--f.texture:SetTexture("Interface\\CastingBar\\UI-CastingBar-Spark")
	f.texture:SetTexture(0,0.51,0.96,1)	--was told r,g,b values are not cool enough(0, 130, 245, 1)
	f.texture:SetBlendMode("ADD")
	f.texture:Hide()

f:SetScript("OnEvent", function(self, event, ...) self[event](self, ...) end)

--show the indicator
function f:PLAYER_LOGIN()
	updateMaxPower()
	updateSpellCost()
	f:SetPoint("LEFT", PlayerFrameManaBar, w*mainSpellCost/maxPower, 0)		
	f.texture:Show()
end

function f:ACTIVE_TALENT_GROUP_CHANGED()
	updateMaxPower()
	updateSpellCost()
	f:SetPoint("LEFT", PlayerFrameManaBar, w*mainSpellCost/maxPower, 0)		
	f.texture:Show()
end

Stuff i dont get:

local ADDON_NAME, privateTable = ...

Whats the "privateTable" part for?

SDPhantom can i actually use your code? It's shorter, more efficient and overall looks much smoother then mine one does.

**Seerah** · 08-02-14, 09:00 AM

The decimal value is just out of 255. You could just as easily use 245/255, but then you're doing some math you don't need to.

As for the private table, Blizzard passes through two arguments to every Lua file: the name of the addon the file belongs to, and a table that is locally accessible to every file in that addon. (This is so that if you need something declared in one file to be accessible in another, you don't need to make it global.)

Sylen · 08-02-14, 09:21 AM

Ah i see now. This is basically needed if you want to make a separate config file.

SDPhantom · 08-02-14, 11:29 AM

Originally Posted by Sylen

SDPhantom can i actually use your code? It's shorter, more efficient and overall looks much smoother then mine one does.

Sure.

Among the changes I made:

I did shorten w and h. It made more sense to make one call to populate both variables than two.
I completely removed the playerClass variable, its only use was to check whether or not the addon should run.
All of the event handlers were basicly the same code, so I replaced the callback system with the event code itself.
Since there was only one event handler now, the external functions were unnecessary and honestly, there was no real need to store spell cost and max power in upvalues.
I wasn't sure what you were trying to do by using two texture:SetTexture() calls, so I commented out the first one in case you wanted to use it in another way.
I trimmed the line involving select(1,GetSpecializationInfo(GetSpecialization())). Using select() with the index of 1 does nothing and is a waste of CPU power.
local _,_,_,cost=GetSpellInfo(mainSpells[GetSpecializationInfo(GetSpecialization())]) Besides shortening the calls into a single line and removing unneeded variables, this is a popular method of getting around using a select() call. Essentially, the underscore character has no real meaning to Lua and is considered just another variable name. Many programmers use it as a garbage variable to get to the return values that are needed. In this example, its written to three times in the line, but it's still much faster to process than a select() call.

For future reference, if you have a choice between saving memory and saving CPU cycles, saving CPU cycles is the best way to go. The less time the CPU spends processing your code, the more time it has to render the game. This isn't an excuse to get sloppy with memory usage. Good code is finding ways to be efficient with both.

PS: I made a small change to the code I posted above and removed a couple calls that were unnecessary.