Asking for help

Popej · 05-22-14, 12:13 PM

Hi, Let me start by saying I am not a programmer. I have made an addon that I have been testing for a while and it has a some bugs. The addon is a PVP flag expiration timer. Here are the known bugs.
1: The frame does not hide properly. Example, if you join a BG while the timer is counting down the frame stays visible, and stops counting.
2: When the timer reaches 1 minute it shows 0 until it hits 59 seconds.

I used these two resources in writing the addon and looked at many more since.
http://www.wowwiki.com/API_GetPVPTimer
http://wowprogramming.com/snippets/C...ing_OnUpdate_3

Perhaps there are better ways to do this, and all help is appreciated.
I have attached the file.
Thanks.

Had uploaded a bad file, this has now been corrected.

Phanx · 05-22-14, 07:16 PM

Starting at the top:

Code:

local FlagTimerFrame = CreateFrame ("FRAME", "FlagTimerFrame", UIParent, nil)

That should be "Frame", not "FRAME"; the API will auto-correct it for you, but it's better to be in the habit of using the correct format.

"FlagTimerFrame" isn't a good choice for your frame's global name. Global object names, like all global variables, should be unique (so they won't conflict with other addons or the default UI) and should clearly identify which addon owns them.

You don't need to specify nil there; just end the list of arguments with UIParent.

Code:

 FlagTimerFrame:RegisterEvent("ADDON_LOADED")
FlagTimerFrame:RegisterEvent("VARIABLES_LOADED")
FlagTimerFrame:RegisterEvent("PLAYER_ENTERING_WORLD")
FlagTimerFrame:RegisterEvent("PLAYER_LOGIN")

Registering these events currently does nothing, because you don't set an OnEvent script for your frame, so it doesn't have anything to do when an event fires. Also, based on the list of events, it looks like you want to do a one-time initialization thing. If that's the case, just use PLAYER_LOGIN and get rid of the others.

In particular, VARIABLES_LOADED has not been an appropriate event for addon loading procedures in many years, as it fires many times throughout the loading process in response to the game client receiving synced settings, macros, keybinds, and other data from the server.

However, it doesn't look like anything in your code needs to be initialized on login, so you can probably just remove all these event registrations.

On the other hand, you do need to register for the events that fire when your PVP flag changes, and hide/show the frame appropriately. That doesn't happen on its own.

Code:

FlagTimerFrame:SetScale(1.0, 1.0)

SetScale only accepts one value, and the default scale of any object is 1, so there is no need to set the scale to 1 unless you have previously set it to something else. Just delete this line.

Code:

FlagTimerFrame:SetPoint("CENTER",ofsx,ofsy)

You don't define the ofsx and ofsy variables anywhere, so you are looking for these in the global namespace, where it is very likely they don't exist, or if they do exist, they were defined by another addon and may not even be numbers. Just pass "CENTER" to place the frame in the middle of the screen, or use actual numbers to offset the position.

Code:

FlagTimerFrame:Hide(true)

The Hide method does not accept any arguments. There's no need to pass true here.

Code:

FlagTimerFrame:RegisterForDrag("RightButton")

There's nothing specifically wrong with this line, but there's no point in registering your frame to be dragged if you're not going to use drag-specific scripts. Either change OnMouseUp/OnMouseDown to OnDragStart/OnDragStop, or don't bother registering any buttons for dragging.

Also, your motion scripts are way more complicated than they need to be; setting and checking that isMoving key doesn't actually do anything, so there's no reason to do it.

Code:

color = RAID_CLASS_COLORS[select(2, UnitClass("player"))]

You are creating a global color variable here, which is extremely likely to collide with leaked globals from other addons (or even the default UI). There is no reason to make this global. Add a local keyword in front of it.

Code:

local Time = FlagTimerFrame:CreateFontString("Time", "OVERLAY", FontInstance)

"Time" is a horrible global name for anything. Font strings do not need global names, so get rid of this (use nil) or at least give it a unique name that won't collide with everything under the sun.

You never defined any variable called FontInstance so you are effectively passing nil there, unless some other addon defined such a variable in the global scope, in which case it's pretty unlikely that it's actually pointing to a valid font object template name. Get rid of this.

Code:

Time:SetFont("Fonts\\Neuropol.ttf", 13, nil)

This is not a valid font path. While you can add fonts to the top-level Fonts folder on your local machine, if you share this addon with someone else, they will get a "font not set" error. Put the font file in your addon's folder and adjust the path accordingly, eg. "Interface\\AddOns\\MyAddon\\Neuropol.ttf".

And again, there's no need to pass nil at the end of a list of arguments. Just stop the list with the last non-nil value.

Code:

local function On_Update()
	PVPTime, e = (GetPVPTimer()/1000)
	PVPStringFormat()
end

You are putting both of the variables on the first line in the global scope, and again, these are some of the worst global names imaginable. Since you don't actually use these variables anywhere, or call their containing function from anywhere, just get rid of the whole thing.

Code:

local f = CreateFrame("frame")

You don't need to create a second frame to run scripts on, and you're currently creating two extra frames, only one of which you actually do anything with.

Code:

	      IsPVPTimerRunning()
	        if IsPVPTimerRunning() then

The first function call here does absolutely nothing except waste CPU time. The second call doesn't use the value returned by the first call -- it just calls the function a second time.

Code:

PVPTimeMes = (" "..(sec>60 and math.floor(sec/60)..":" or "")..(sec%60).."  ")

Again, you are putting this in the global namespace, which is bad.

Code:

function PVPStringFormat()
  Time:SetText(PVPTimeMes)
end

Another global. Also, since you only call this function in one place, there's no reason for it to be a function at all. Just set the text directly in the place where you currently call this function.

---------------------------------------------------------------------------

Not tested, but this should work better, and is much cleaner:

Code:

local frame = CreateFrame ("Frame", "MyPVPFlagTimer", UIParent)
frame:SetPoint("CENTER")
frame:SetWidth(47)
frame:SetHeight(17)
frame:SetAlpha(0.8)
frame:Hide()

frame:SetClampedToScreen(true)
frame:SetMovable(true)
frame:SetUserPlaced(true)
frame:EnableMouse(true)
frame:RegisterForDrag("RightButton")
frame:SetScript("OnDragStart", function(self, button)
	if button == "RightButton" then
		self:StartMoving()
	end
end)
frame:SetScript("OnDragStop", frame.StopMovingOrSizing)
frame:SetScript("OnHide", frame.StopMovingOrSizing)

frame:SetBackdrop({
	bgFile = "Interface\\Buttons\\WHITE8x8", tile = true, tileSize = 0,
	edgeFile = "Interface\\Buttons\\WHITE8x8", edgeSize = 5,
	insets = { left = 2, right = 2, top = 2, bottom = 2 }
})
frame:SetBackdropColor(0,0,0,1)
frame:SetBackdropBorderColor(0,0,0,0.8)

local text = frame:CreateFontString(nil, "OVERLAY")
text:SetPoint("CENTER", 2, 0)
text:SetFont("Interface\\AddOns\\MyAddpn\\Neuropol.ttf", 13) -- Change "MyAddon" to the name of your addon's folder, and move the font file.
do
	local _, class = UnitClass("player")
	local color = (CUSTOM_CLASS_COLORS or RAID_CLASS_COLORS)[class]
	text:SetTextColor(color.r, color.g, color.b)
end

local UPDATE_INTERVAL, updateTime = 1, 1

local function OnUpdate(self, elapsed)
	updateTime = updateTime - elapsed
	if updateTime < 0 then
		local t = floor(GetPVPTimer() / 1000)
		if t > 60 then
			text:SetFormattedText("%d:%d", floor(t / 60), t % 60)
		else
			text:SetText(t)
		end
		updateTime = UPDATE_INTERVAL
	end
end

frame:RegisterUnitEvent("UNIT_FACTION", "player")
frame:SetScript("OnEvent", function(self, event, unit)
	if UnitIsPVPFreeForAll("player") or UnitIsPVP("player") then
		local t = GetPVPTimer()
		if t > 0 and t < 301000 then
			updateTime = UPDATE_INTERVAL
			self:SetScript("OnUpdate", OnUpdate)
		else
			self:SetScript("OnUpdate", nil)
			text:SetText("PVP")
		end
		self:Show()
	else
		self:Hide()
	end
end)

humfras · 05-23-14, 05:21 AM

May I just say: This is a wonderfull thread.

Clear questions and information by the OP with the respective file attached.

Great and detailed answer by Phanx (as usual).

Why can't all help request threads be like this?

Popej · 05-23-14, 12:55 PM

Thank you Phanx for your response. I appreciate the time you took to explain how my code was working, and why things should be done differently. Your rewrite of the code is much neater, and I am still going through your reply to better understand what is going on. I will test this when I get home. Once again thank you.

Ketho · 05-23-14, 03:49 PM

Threads like this make me wish I joined the Seal Cub Clubbing Club

Popej · 05-23-14, 05:37 PM

Tested and got this error; 41: attempt to perform arithmetic on global "total" (a nil value).

Phanx · 05-23-14, 06:12 PM

Change "total" to "updateTime".

Popej · 05-26-14, 01:47 PM

Changing "total" to "updateTime" that worked, many thanks. I changed line 38 (the timer would appear to freeze)
from; local UPDATE_INTERVAL, updateTime = 1, 1
to; local UPDATE_INTERVAL, updateTime = 0, 1
I also changed line 62 (the frame would populate "PVP"
from; text:SetText("PVP")
to; text:SetText(" ")
There are still a couple rough spots, added on line 56, " if IsPVPTimerRunning("player") then" , this fixed the frame showing when joining a bg while the timer is not running.
1: It takes a /reload to get the frame to show after the timer is running. This was happening before editing the code.
2: After the timer has expired the frame still shows "5:1". Pretty sure these two are related as a /reload works.

Code:

local UPDATE_INTERVAL, updateTime = 0, 1

Code:

frame:RegisterUnitEvent("UNIT_FACTION", "player")
frame:SetScript("OnEvent", function(self, event, unit)
	if UnitIsPVPFreeForAll("player") or UnitIsPVP("player") then
        if IsPVPTimerRunning("player") then
		local t = GetPVPTimer()
		if t > 0 and t < 301000 then
			updateTime = UPDATE_INTERVAL
			self:SetScript("OnUpdate", OnUpdate)
		else
			self:SetScript("OnUpdate", nil)
			text:SetText(" ")
		end
		self:Show()
	else
		self:Hide()
	end
	end
end)

Phanx · 05-26-14, 06:23 PM

Originally Posted by Popej

I changed line 38 (the timer would appear to freeze)
from; local UPDATE_INTERVAL, updateTime = 1, 1
to; local UPDATE_INTERVAL, updateTime = 0, 1

That defeats the whole point of creating and using those variables in the first place. If you really want to run your update routine on every frame draw (instead of only once per second) then just get rid of those variables and every line that does anything with them.

If you would rather solve the problem, please be more specific about what "appear to freeze" means, and when this problem happens.

Originally Posted by Popej

I also changed line 62 (the frame would populate "PVP"
from; text:SetText("PVP")
to; text:SetText(" ")

If you don't want to know when your PVP flag is permanently enabled (eg. in a PVP free-for-all area) then replace the entire contents of the OnEvent function with this:

Code:

	local t = GetPVPTimer()
	if t and t > 0 and t < 301000 then
		updateTime = UPDATE_INTERVAL
		self:Show()
	else
		self:Hide()
	end

... and add this at the end of the file:

Code:

frame:SetScript("OnUpdate", OnUpdate)

Originally Posted by Popej

... this fixed the frame showing when joining a bg while the timer is not running.

I haven't done a battleground or arena in at least 5 years, so you're going to have to be a lot more specific about what you're doing, what you expect to be happening, and what actually is happening...

Popej · 05-27-14, 12:27 PM

Phanx, thanks for taking the time to help me.
This is what I was trying to do.
A pvp flag expiration timer that shows a frame with a visible timer counting down the time until the pvp flag falls off.
I only want the frame and text visible when the timer is counting down.
Having the frame show when your PVP flag is permanently enabled is a great feature (and a good idea) if your unit frame do not have that feature.
Sorry for not being real clear on this earlier when I said the addon was a PVP flag expiration timer.

I Renamed the Cache, Interface, and WTF folders to start fresh.
Tested with only these addons installed.
MyPVPTimer (addon being tested)
BugSack
!BugGrabber
I reverted all the changes that I made to the addon

This is what I observed.
Once PVP was toggled on, PVP would show in the frame, and it would take a /reload to show the time in the frame.

After the /reload this is what the frame looks like, the text would remain as it is without counting down (thus appearing to freeze).

These actions would reset the text in the frame to PVP and a /reload would show the time in the frame (not counting down).
Using hearth stone,
Taking a portal from a major city to another city,
Going through the Dark Portal in the Blasted Lands.

What I observed earlier after I made the following change.

local UPDATE_INTERVAL, updateTime = 0, 1 The frame would show the text in the frame counting down.
This may not solve the problem, but it appeared to work.

What works well after you have done a /reload
Going between crossrealm zones, it resets to 5 minutes.
After the PVP timer expires the frame and text is not visible.

This is pretty much what is actually happening.

Thanks again.