Need expert advice and The Klaxxi faction ID

[skarie]

Halo,

As I am new to wow programming, I need some expert advice on how to improve this code snippet below. I am concerned about the performance issue. Is there a better (more efficient way) to achieve the same goal. I am trying to track reputations. So far, it runs fine except for The Klaxxi faction. Only the name ("The Klaxxi") is displayed and nothing else.

--faciton ids
local repTable = {1341, 1269, 1270, 1337, 1359}

local fEvent = {}
local repFrame = {}
local rName = {}

for k,v in pairs(repTable) do 
	fEvent[k] = CreateFrame("Frame",nil, UIParent)
	fEvent[k]:SetSize(140,15)
	fEvent[k]:SetFrameStrata("LOW")

	if ( k == 1) then 
		fEvent[k]:SetPoint("TOP",Minimap,"BOTTOM",0,-25)
	else
		fEvent[k]:SetPoint("TOP",fEvent[k-1],"BOTTOM",0,-2)
	end

        --CreateBD creates back drop with with black background and alpha .7
	F.CreateBD(fEvent[k],.7)
	
	repFrame[k] = CreateFrame("StatusBar",nil,self)
	repFrame[k]:SetFrameStrata("BACKGROUND")
	repFrame[k]:SetAllPoints(fEvent[k])
	repFrame[k]:SetStatusBarTexture(C.media.texture)
	repFrame[k]:SetStatusBarColor(1, 1, 1, 1)
	repFrame[k]:SetReverseFill(false)
	repFrame[k]:RegisterEvent("UPDATE_FACTION")
	
	rName[k] = F.CreateFS(fEvent[k], 8,"LEFT")
	rName[k]:SetPoint("LEFT",fEvent[k],"LEFT",2,0)
	
	repFrame[k]:SetScript("OnEvent", function(self,event,...)
		local name, _, standing, barMin, _, barValue, _, _, _, _, _, _, _ = GetFactionInfoByID(v)
		local  curr
		curr = barValue - barMin
		
		if( standing == 8) then
                        --when exalted, change text color
			rName[k]:SetText("|cFFFF9955"..name)
		else
			rName[k]:SetText("|r"..name)
			repFrame[k]:SetMinMaxValues(0,barMin)
			repFrame[k]:SetValue(barMin - curr)
		end
	end)
 end

What is the The Klaxxi faction id? I use faction id provided by wowhead.com. But that doesnt seem to work.

[Phanx]

If it's showing the name ("The Klaxxi") then you are getting the name via the API call, which means you are using the correct faction ID.

The only thing that jumps out at me is that you are using pairs instead of ipairs. pairs returns key/value pairs in undefined order, so you won't get 1/1341 and then 2/1269 and so on; you could get 2/1269 and then 5/1359 and then 3/1270, for example. Since you are using the indices (keys) inside your for-loop in a way that clearly expects the key/value pairs to be processed in sequential order, this is likely causing your problem, and you are almost certainly getting a Lua error. Simply switching to ipairs may solve your problem.

However, if you do not already have BugSack installed and enabled, go get it. The built-in errror display is (a) disabled by default, (b) annoyingly intrusive, and (c) cannot show you errors that occur during the initial loading process, which is when the majority of errors will occur while you are developing an addon.

[skarie]

Hi Phanx,

Thanks for taking the time to look at my codes.

I changed pairs to ipairs and installed bugstack. No error is generated. However, that doesn't do the trick. The results are still the same. Its so strange that all the other reps i am tracking show up correctly, except for The Klaxxi. Maybe I should use other API method to get the reps.

[Phanx]

After taking another look, I see several places where you're calling functions like F.CreateBD and F.CreateFS that are not defined anywhere in your code, which means that either:

(a) the code you posted is not your real code, or not all of your real code, in which case, please post your entire, actual code if you want help with it, or

(b) you based your code on something you copied and pasted from somewhere, but didn't remove the references to parts you didn't keep, and you forgot to restart WoW after installing BugSack, because attempting to call a function that is not defined anywhere will always trigger a Lua error, without exception, in which case, try removing those references and restarting WoW if you haven't yet so that BugSack can actually catch the errors that are occurring.

[skarie]

Originally Posted by Phanx After taking another look, I see several places where you're calling functions like F.CreateBD and F.CreateFS that are not defined anywhere in your code, which means that either: (a) the code you posted is not your real code, or not all of your real code, in which case, please post your entire, actual code if you want help with it, or (b) you based your code on something you copied and pasted from somewhere, but didn't remove the references to parts you didn't keep, and you forgot to restart WoW after installing BugSack, because attempting to call a function that is not defined anywhere will always trigger a Lua error, without exception, in which case, try removing those references and restarting WoW if you haven't yet so that BugSack can actually catch the errors that are occurring.

You're correct..CreateDB and CreateFS is defined some place else...but here I modified the lua to include both functions in same file.

--local F, C, L = unpack(select(2, ...))

local CreateBD = function(f, a)
	f:SetBackdrop({
		--bgFile = C.media.backdrop,
		bgFile = "",
		--edgeFile = C.media.backdrop,
		edgeFile = "",
		edgeSize = 1,
	})
	f:SetBackdropColor(0, 0, 0, a or .5)
	--f:SetBackdropColor(0.2, 0.36, 0.45, a or .5)
	f:SetBackdropBorderColor(0, 0, 0)
end

local CreateFS = function(parent, size, justify)
    local f = parent:CreateFontString(nil, "OVERLAY")
    --f:SetFont(C.media.font, size, "OUTLINEMONOCHROME")
	f:SetFont(STANDARD_TEXT_FONT, size, "OUTLINEMONOCHROME")
	f:SetShadowColor(0, 0, 0, 0)
    if(justify) then f:SetJustifyH(justify) end
    return f
end

local repTable = {1341, 1269, 1270, 1337, 1359}

local fEvent = {}
local repFrame = {}
local rName = {}

for k,v in ipairs(repTable) do 
	fEvent[k] = CreateFrame("Frame",nil, UIParent)
	fEvent[k]:SetSize(140,15)
	fEvent[k]:SetFrameStrata("LOW")

	if ( k == 1) then 
		fEvent[k]:SetPoint("TOP",Minimap,"BOTTOM",0,-25)
	else
		fEvent[k]:SetPoint("TOP",fEvent[k-1],"BOTTOM",0,-2)
	end
	--F.CreateBD(fEvent[k],.7)
	CreateBD(fEvent[k],.7)
	
	repFrame[k] = CreateFrame("StatusBar",nil,self)
	repFrame[k]:SetFrameStrata("BACKGROUND")
	repFrame[k]:SetAllPoints(fEvent[k])
	repFrame[k]:SetStatusBarTexture(C.media.texture)
	repFrame[k]:SetStatusBarColor(1, 1, 1, 1)
	repFrame[k]:SetReverseFill(true)
	repFrame[k]:RegisterEvent("UPDATE_FACTION")
	
	rName[k] = CreateFS(fEvent[k], 8,"LEFT")
	rName[k]:SetPoint("LEFT",fEvent[k],"LEFT",2,0)
	
	repFrame[k]:SetScript("OnEvent", function(self,event,...)
		local name, _, standing, barMin, _, barValue, _, _, _, _, _, _, _ = GetFactionInfoByID(v)
		local  curr
		curr = barValue - barMin
		
		if( standing == 8) then
			rName[k]:SetText("|cFFFF9955"..name)
		else
			rName[k]:SetText("|r"..name)
			repFrame[k]:SetMinMaxValues(0,barMin)
			repFrame[k]:SetValue(barMin - curr)
		end
	end)
 end

[Phanx]

I don't know what, specifically, was wrong with your code, but I ended up just rewriting it from scratch, and this works flawlessly. I included some comments to indicate where to insert calls to your backdrop and border functions if you want.

local factions = { 1341, 1269, 1270, 1337, 1359 }

local bars = {}

-- Feel free to remove this and replace it with your backdrop function:
local BACKDROP = {
	bgFile = [[Interface\Tooltips\UI-Tooltip-Background]], tile = true, tileSize = 16,
	edgeFile = [[Interface\Tooltips\UI-Tooltip-Border]], edgeSize = 16,
	insets = { left = 5, right = 5, top = 5, right = 5 },
}

local function OnEvent(self, event, ...)
	local name, _, standing, barMin, barMax, barValue = GetFactionInfoByID(self.factionID)
	self:SetMinMaxValues(barMin, barMax)
	self:SetValue(barValue)

	local color = FACTION_BAR_COLORS[standing]
	self:SetStatusBarColor(color.r, color.g, color.b)

	self.text:SetText(name)
	if standing == 8 then
		self.text:SetTextColor(1, 0.6, 0.33)
	else
		self.text:SetTextColor(1, 1, 1)
	end
end

-- Feel free to remove this function if you don't want mouse interactivity:
local function OnEnter(self)
	local _, _, _, barMin, barMax, barValue = GetFactionInfoByID(self.factionID)
	self.text:SetFormattedText("%d/%d", barValue - barMin, barMax - barMin)
end

-- Feel free to remove this function if you don't want mouse interactivity:
local function OnLeave(self)
	local name = GetFactionInfoByID(self.factionID)
	self.text:SetText(name)
end

for i, factionID in ipairs(factions) do
	local bar = CreateFrame("StatusBar", nil, UIParent)
	bar:SetPoint("TOP", i>1 and bars[i-1] or Minimap, "BOTTOM", 0, i>1 and -8 or -24)
	bar:SetSize(162, 14)
	bars[i] = bar

	-- Feel free to remove this section and replace it with a call to your backdrop function:
	local bg = bar:CreateTexture(nil, "BACKGROUND")
	bg:SetAllPoints(true)
	bg:SetTexture(0, 0, 0, 0.7)
	bar.bg = bg

	bar:SetStatusBarTexture([[Interface/TargetingFrame/UI-StatusBar]])
	bar:GetStatusBarTexture():SetDrawLayer("BORDER")

	-- Feel free to remove this section and replace it with a call to your border function
	local left = bar:CreateTexture(nil, "ARTWORK")
	left:SetTexture([[Interface\AchievementFrame\UI-Achievement-ProgressBar-Border]])
	left:SetTexCoord(0, 0.0625, 0, 0.75)
	left:SetPoint("TOPLEFT", -6, 5)
	left:SetPoint("BOTTOMLEFT", -6, -5)
	left:SetWidth(16)
	bar.left = left
	local right = bar:CreateTexture(nil, "ARTWORK")
	right:SetTexture([[Interface\AchievementFrame\UI-Achievement-ProgressBar-Border]])
	right:SetTexCoord(0.812, 0.8745, 0, 0.75)
	right:SetPoint("TOPRIGHT", 6, 5)
	right:SetPoint("BOTTOMRIGHT", 6, -5)
	right:SetWidth(16)
	bar.right = right
	local center = bar:CreateTexture(nil, "ARTWORK")
	center:SetTexture([[Interface\AchievementFrame\UI-Achievement-ProgressBar-Border]])
	center:SetTexCoord(0.0625, 0.812, 0, 0.75)
	center:SetPoint("TOPLEFT", left, "TOPRIGHT")
	center:SetPoint("BOTTOMRIGHT", right, "BOTTOMLEFT")
	center:SetWidth(16)
	bar.center = center
	-- End of border section

	local text = bar:CreateFontString(nil, "OVERLAY", "GameFontHighlightSmall")
	text:SetPoint("CENTER")
	bar.text = text

	bar.factionID = factionID
	bar:SetScript("OnEvent", OnEvent)
	bar:RegisterEvent("UPDATE_FACTION")

	-- Feel free to remove this section if you don't want mouse interactivity:
	bar:EnableMouse(true)
	bar:SetScript("OnEnter", OnEnter)
	bar:SetScript("OnLeave", OnLeave)
end

[skarie]

Originally Posted by Phanx I don't know what, specifically, was wrong with your code, but I ended up just rewriting it from scratch, and this works flawlessly. I included some comments to indicate where to insert calls to your backdrop and border functions if you want.

You are so awesome !! I am gonna take a good look and learn from your codes. Thanks again.

Did you try to run my codes and get the same result (my result that is)? I am still very new to wow programming and certainly appreciate greatly all the help I get. :-)

Cheer ! This beer on me.

[skarie]

Completed and working codes

 
local function OnEvent(self, event, ...)
	local name, _, standing, barMin, barMax, barValue = GetFactionInfoByID(self.factionID)
	self:SetMinMaxValues(barMin, barMax)
	self:SetValue(barValue)
	self.text:SetText(name)
	
	local color = FACTION_BAR_COLORS[standing]
	self:SetStatusBarColor(color.r, color.g, color.b)
	
	if standing == 8 then
		self.text:SetTextColor(1, 0.6, 0.33)
	else
		self.text:SetTextColor(1, 1, 1)
	end
	
end
local repTable = { 1341, 1269, 1270, 1337, 1359 }

local repframes = {}
local bgs = {}

for k,v in ipairs(repTable) do 
	
	local bg = CreateFrame("Frame",nil,UIParent)
	bg:SetSize(140,15)
	bgs[k] = bg
	
	local repframe = CreateFrame("StatusBar",nil,self)
	repframes[k] = repframe

	repframe:SetAllPoints(bg)
	F.CreateBD(bg)

	bg:SetPoint("TOP", k > 1 and bgs[k-1] or Minimap,"BOTTOM", 0, k > 1 and -2 or -25)
	

	repframe:SetStatusBarTexture(C.media.texture)
	
	local text = F.CreateFS(bg,8)
	text:SetPoint("CENTER")
	
	repframe.text = text
	
	repframe.factionID = v
	repframe:SetScript("OnEvent", OnEvent)
	repframe:RegisterEvent("UPDATE_FACTION")
	
 end

bg:SetPoint("TOP", k > 1 and bgs[k-1] or Minimap,"BOTTOM", 0, k > 1 and -2 or -25) <--- this makes me feel so cool !!

[Phanx]

A couple thoughts on your final code:

1) There is no reason to create a separate frame for the backdrop vs. the bar. You can apply backdrops to a StatusBar object, as well as add ordinary textures to it, register events on it, set scripts on it, etc. StatusBar objects are just Frame objects with some extra features; they don't lose any features.

2) Even if you do want to create separate frames for some reason, you don't need to store them in separate tables; you should attach the child to the parent (eg. frames[i] = frame and then frame.bar = bar, instead of frames[i] = frame and bars[i] = bar).

3) You're parenting your bar to "self" but "self" is not defined in that scope, so your bars are actually not being parented to anything, which means they are not properly attached to the UI, and won't get hidden with Ctrl+Z. Again, if you really want separate frames for some reason, the bar frame should be parented to the regular frame.

4) For ease of maintenance, I'd suggest putting all of your variables/constants (like repframes and repTable) at the top of your file, so you can easily find them without having to scroll through a bunch of functions. It'll also make it easier to add more factions.

On a side note, the reason the OnEvent function is defined outside of the for-loop is because you only need one copy of it, and can attach the same function to multiple frames; defining the function inside the for-loop would create multiple copies of the function, taking up -- in your case -- 5 times as much memory but doing the same thing.

On another side note, while it doesn't affect functionality, it is conventional to use k/v only with pairs; when you're using ipairs, you use i/v instead. It just helps improve code readability, so it's more obvious that you're inside an ipairs loop (indexed values processed in sequential order) instead of a pairs loop (values processed in undefined order). It may not seem important now, but if you come back in a month or 6 months and want to change something, little helpers like this will add up and make it much easier to remember what everything does.

Revised version:

local repTable = { 1341, 1269, 1270, 1337, 1359 }

local repframes = {}

local function OnEvent(self, event, ...)
	local name, _, standing, barMin, barMax, barValue = GetFactionInfoByID(self.factionID)
	self:SetMinMaxValues(barMin, barMax)
	self:SetValue(barValue)
	self.text:SetText(name)

	local color = FACTION_BAR_COLORS[standing]
	self:SetStatusBarColor(color.r, color.g, color.b)

	if standing == 8 then
		self.text:SetTextColor(1, 0.6, 0.33)
	else
		self.text:SetTextColor(1, 1, 1)
	end
end

for i, v in ipairs(repTable) do
	local repframe = CreateFrame("StatusBar", nil, UIParent)
	repframe:SetPoint("TOP", i > 1 and repframe[i-1] or Minimap,"BOTTOM", 0, i > 1 and -2 or -25)
	repframe:SetSize(140,15)
	repframe:SetStatusBarTexture(C.media.texture)
	repframes[i] = repframe

	F.CreateBD(bg)

	local text = F.CreateFS(repframe, 8)
	text:SetPoint("CENTER")
	repframe.text = text

	repframe.factionID = v
	repframe:SetScript("OnEvent", OnEvent)
	repframe:RegisterEvent("UPDATE_FACTION")
end

[skarie]

Originally Posted by Phanx A couple thoughts on your final code: 1) There is no reason to create a separate frame for the backdrop vs. the bar. You can apply backdrops to a StatusBar object, as well as add ordinary textures to it, register events on it, set scripts on it, etc. StatusBar objects are just Frame objects with some extra features; they don't lose any features. 2) Even if you do want to create separate frames for some reason, you don't need to store them in separate tables; you should attach the child to the parent (eg. frames[i] = frame and then frame.bar = bar, instead of frames[i] = frame and bars[i] = bar). 3) You're parenting your bar to "self" but "self" is not defined in that scope, so your bars are actually not being parented to anything, which means they are not properly attached to the UI, and won't get hidden with Ctrl+Z. Again, if you really want separate frames for some reason, the bar frame should be parented to the regular frame. 4) For ease of maintenance, I'd suggest putting all of your variables/constants (like repframes and repTable) at the top of your file, so you can easily find them without having to scroll through a bunch of functions. It'll also make it easier to add more factions. On a side note, the reason the OnEvent function is defined outside of the for-loop is because you only need one copy of it, and can attach the same function to multiple frames; defining the function inside the for-loop would create multiple copies of the function, taking up -- in your case -- 5 times as much memory but doing the same thing. On another side note, while it doesn't affect functionality, it is conventional to use k/v only with pairs; when you're using ipairs, you use i/v instead. It just helps improve code readability, so it's more obvious that you're inside an ipairs loop (indexed values processed in sequential order) instead of a pairs loop (values processed in undefined order). It may not seem important now, but if you come back in a month or 6 months and want to change something, little helpers like this will add up and make it much easier to remember what everything does. Revised version:

1) This is unknown to me. This is certainly helpful and will cut down lots of codes and run-times.
2) Nice idea. I didnt know a frame can have a frame as a child. This is similar to a frame carrying a text field as a child ?
3) yeah, that was a blunder...

F.CreateBD(bg) should be F.CreateBD(repframe) .. but i get the idea.

Questions for you (whenever you have time):
Can you have two separate OnEvent funtions defined in the same lua file ?

Lets say. I wanna track valor pts and honor pts together with reputation

 local function OnEvent(self, event, ...)
	local name, amount, texture, earnedThisWeek, weeklyMax, totalMax, isDiscovered = GetCurrencyInfo(self.currID)
	self:SetMinMaxValues(0, self.max)
	self:SetValue(amount)
	self.text:SetText(name)
		
end

---for i,v ....codes intentionally omitted....
	currframe:SetScript("OnEvent",OnEvent)
	currframe:RegisterEvent("CURRENCY_DISPLAY_UPDATE")
---

Is it ok to have two onEvent functions; one is tracking faction update and the other is tracking currency update ?

[Ordrosh]

if I recall my little lua knowledge correctly, no, at least not with identical names. And I even think you wont need to, just check what event triggered your onEvent function and have it do what you want it to do
Edit:
see http://www.wowpedia.org/Handling_events for some hints

[Phanx]

Your question is a bit unclear, but yes, you can have multiple event handlers defined in the same file.

If you want to set different event handler scripts on different frames, just define two functions. Function names are just variables, so they follow all the same rules as other variables; in particular, giving a variable a name that's already assigned to another variable overwrites the first variable, so you would want to give your functions unique names. Example:

local function RepBar_OnEvent(self, event, ...)
	...		
end

for i, v in ipairs(repTable) do
	...
	currframe:SetScript("OnEvent", RepBar_OnEvent)
	currframe:RegisterEvent("CURRENCY_DISPLAY_UPDATE")
	...
end

local function SomeOtherFrame_OnEvent(self, event, ...)
	...
end

otherframe:SetScript("OnEvent", SomeOtherFrame_OnEvent)
otherframe:RegisterEvent("SOME_EVENT")
otherframe:RegisterEvent("SOME_OTHER_EVENT")

If you want a single function on a single frame to process different events differently, you'd do something like this:

local function RepBar_OnEvent(self, event, ...)
	if event == "CURRENCY_DISPLAY_UPDATE" then
		-- do something
	else
		-- assume it's the other event, and do something else
	end
end

for i, v in ipairs(repTable) do
	...
	currframe:SetScript("OnEvent", RepBar_OnEvent)
	currframe:RegisterEvent("CURRENCY_DISPLAY_UPDATE")
	currframe:RegisterEvent("SOME_OTHER_EVENT")
	...
end

You could, of course, handle more than two events in more than two ways, by adding to the if/else logic.

If you're trying to add a bar that displays honor (or any other currency) I'd suggest Method #1, and creating that bar separately (ie. not in your existing ipairs loop through repTable), since you don't need a currency bar to respond to rep events, or vice versa.

[skarie]

Yep Phanx,

This is exactly what I am looking for. Thank again.