oUF_Layout optimization

sirann · 12-11-14, 02:07 AM

Hey all,

I've finally gotten around to writing my own personal oUF. I am nearly complete; completed modules work as desired. Party, pet, boss, and raid still to go, but wanted some feedback on better ways to optimize my layout. I've written this in a rather verbose manner, a lot of similar looking code for the sake of lots of customization on a unit to unit basis.

The pastebin can be found here: http://pastebin.com/bB3MFkeM

Thanks!

Dawn · 12-11-14, 10:07 AM

Just took a glance at your code. I noticed that you can easily optimize by reusing similar code.

For instance, create health and power bar code just once and insert it for each unit.

You could do this with using a "Shared" function, that contains code used for every unit and/or by creating single functions. Somewhat like this:

Code:

local createPower = function(self) 	-- Power bar

.......

end

and insert it into each unit that you want to have a power bar, like:

Code:

UnitSpecific.player = function(self, ...)
	Shared(self, ...)                    -- only if you use a shared function, obviously
	createPower(self)                 

end

You can also do this with textures, like borders, background and so on ... Basically if you reuse code at least once on another frame, it makes sense to do it this way.

€: Just to add to the confusion, let's say you create a "createPower" function; you don't want to put the SetPoint stuff in there. Unless every unit you use is exactly the same, ofc. Only put the CreateFrame, background, PostUpdate, color and stuff like that in there.

Set your Points for each unit, after you inserted the createPower(self) function.

like

Code:

	UnitSpecific.player = function(self, ...)
		createPower(self)
		self.Power:SetPoint("BOTTOMLEFT", self, "BOTTOMLEFT", 38, 20)

		createRaidIcon(self)
		self.RaidIcon:SetPoint("TOPLEFT", self, "TOPLEFT", 4, 0)		
	end

	UnitSpecific.target = function(self, ...)
		createPower(self)
		self.Power:SetPoint("BOTTOMRIGHT", self, "BOTTOMRIGHT", -38, 20)

		createRaidIcon(self)
		self.RaidIcon:SetPoint("TOPRIGHT", self, "TOPRIGHT", -4, 0)		
	end

Phanx · 12-11-14, 11:56 AM

Personally I've never seen the point in splitting each part of the spawn function out into a "make a health bar" function, a "make a power bar" function, a "do stuff for the player unit" and "do stuff for the target unit" functions, etc. You're only running that code once per frame created, so just have it all in the same place.

Code:

local health = CreateFrame("StatusBar", nil, self)
self.Health = health

if unit == "player" then
   -- do stuff with the health bar for the player frame
elseif unit == "target" then
   -- do stuff with the health bar for the target frame
end

Putting each "make a _____" function in its own file, as I've seen some layouts do, is especially wasteful, since loading files from disk is a significant factor in how much time you spend staring at the loading screen while logging in.

----------

This is also pretty inefficient:

Code:

                        BuffBars.customFilter = function(_, _, _, _, _, _, _, _, _, _, _, _, spellID, _, _, _, _, _)
                                if spellID == 97341 or          --Guild Champion
                                        spellID == 93339 or             --Champion
                                        spellID == 93828 or             --Silvermoon Champion
                                        spellID == 93341 or             --Hyjal Champion
                                        spellID == 93347 or             --Therazane Champion
                                        spellID == 97340 or     --Guild Champion
                                        spellID == 93811 or             --Exodar Champion
                                        spellID == 93816 or             --Gilnean Champion
                                        spellID == 72968 or             --Precious's Ribbon
                                        spellID == 93806 or             --Darnassus Champion
                                        spellID == 93368 or             --Wildhammer Champion
                                        spellID == 93337 or             --Ramkahen Champion
                                        spellID == 126434 or    --Tushui Champion
                                        spellID == 57819 or             --Argent Chamption
                                        spellID == 57822 or     --Wyrmrest Champion
                                        spellID == 93821 or             --Gnomergan Champion
                                        spellID == 142234 or    --Brawling Champion (toot toot)
                                        spellID == 142239 or    --Brawling Champion (gorgeous)
                                        spellID == 142242 or    --Brawling Champion (south seas)
                                        spellID == 142243 or    --Brawling Champion (splat)
                                        spellID == 142244 or    --Brawling Champion (bruce)
                                        spellID == 142246 or    --Brawling Champion (rockpaperscissors)
                                        spellID == 142247 or    --Brawling Champion (blingtrong)
                                        spellID == 143625 or    --Brawling Champion (rank 10)
                                        spellID == 93805 or             --Ironforge Champion
                                        spellID == 25780 or     --Righteous Fury
                                        spellID == 17619 or             --Alchemist Stone
                                        spellID == 93795                --Stormwind Champion
                                then
                                        return false
                                else
                                        return true
                                end
                        end

Rather than running a long "if A or B or C or ..." chain you should put all those spell IDs in a table:

Code:

local blacklist = {
     [93795] = true, -- Stormwind Champion
     -- more spells here
}

Then the contents of your filter function can just be:

Code:

     return not blacklist[spellID]

Also:

Code:

BuffBars.customFilter = function(_, _, _, _, _, _, _, _, _, _, _, _, spellID, _, _, _, _, _)

As with assigning values returned by a function, you can just stop the list at the last value you want to catch. You don't need to assign every value. In this case, just stop your list with "spellID" and don't bother naming any of the additional values with underscores:

Code:

BuffBars.customFilter = function(_, _, _, _, _, _, _, _, _, _, _, _, spellID)

----------

Your focus frame is ... I don't even. Why are you creating a bunch of extra frames and listening for events yourself, instead of just using oUF elements and/or tags?

----------

Other than that, your #1 priority should be removing all that duplication.

sirann · 12-11-14, 12:21 PM

Thank you both for the replies. My initial thought was to have all that repeat code because as far as I know, and correct me if I'm wrong, no less CPU/men efficient than making a shared function that goes through all the frames and does the same code anyway. It's certainly more unwieldy to look at that, and for that it will get changed. I just wanted to easily be able to go to a certain line of code for a certain frame and make tweaks before I became satisfied.

In regards to the aura if checks, I'll change that as well as the unnecessary _ variables.

As for the reason I didn't make my own tags in ouf. That code has to be put somewhere in the ouf folder, be it a tags file in my layout folder, or editing the ouf tags. while it would look cleaner if I just had tags in my layout, rather than functions and frame creations, it would ultimately be messy somewhere.

Tl;Dr is what I've done, minus the buff checking, any less CPU/men efficient, or just unsightly?

Phanx · 12-11-14, 04:31 PM

The biggest problem with duplication is that it makes things harder to debug and maintain in the long run. If you decide to change anything about your health bar, for example, you have to make the same change 4 times in 4 different places, which is boring, a waste of time, and easy to screw up. If you only have to make the change in one place, it's faster and less error-prone.

Also just noticed this:

Code:

self.HealthBG = HealthBG

You should stick with oUF convention and set this as self.Health.bg instead of self.HealthBG. It won't make any functional difference, but it will help other oUF layout authors looking at your code, and it will help you if you're looking at other people's layout code, since it'll be easier to recognize what's what.

Originally Posted by sirann

As for the reason I didn't make my own tags in ouf. That code has to be put somewhere in the ouf folder, be it a tags file in my layout folder, or editing the ouf tags. while it would look cleaner if I just had tags in my layout, rather than functions and frame creations, it would ultimately be messy somewhere.

But it would be a lot less messy than creating frames and setting event handlers in your code, especially when you consider that you're currently duplicating event handlers 4 times. For example, here are tags for your health value and percent strings:

Code:

oUF.Tags.Events.healthtext = 'UNIT_HEALTH'
oUF.Tags.Methods.healthtext = function(unit)
	if UnitIsGhost(unit) then
		return '|cffb40000Ghost'
	elseif UnitIsDead(unit) then
		return '|cffb40000Dead'
	end
	local r, g = 0, 180
	local v, vmax = UnitHealth(unit), UnitHealthMax(unit)
	if v < vmax then
		r, g = g, r
	end
	if v > 1000000 then
		return format('|cff%02x%02x00%.1fm', r, g, v / 1000000)
	elseif v > 1000 then
		return format('|cff%02x%02x00%.0fk', r, g, v / 1000)
	else
		return format('|cff%02x%02x00%d', r, g, v)
	end
end

oUF.Tags.Events.healthpercent = 'UNIT_HEALTH'
oUF.Tags.Methods.healthpercent = function(unit)
	if UnitIsDead(unit) then
		return
	end
	local r, g = 0, 180
	local v, vmax = UnitHealth(unit), UnitHealthMax(unit)
	if v < vmax then
		r, g = g, r
	end
	return format('|cff%02x%02x00%.0f%%', r, g, 100 * (v / vmax))
end

Then you can just do this (example for the player frame):

Code:

local HealthText = Health:CreateFontString(nil, 'OVERLAY')
HealthText:SetPoint('RIGHT', 5, -10)
HealthText:SetFont(FONT, 10, 'OUTLINE')
HealthText:SetJustifyH('LEFT')
Health.Text = HealthText

self:Tag(HealthText, '[healthvalue]')

local HealthPercent = playerhealthtextframe:CreateFontString(nil, 'OVERLAY')
HealthPercent:SetPoint('LEFT', -5, -10)
HealthPercent:SetFont(FONT, 10, 'OUTLINE')
HealthPercent:SetJustifyH('RIGHT')
Health.Percent = HealthPercent

self:Tag(HealthPercent, '[healthpercent]')

Then you can reuse those tags on the other frames. You don't need to specifically listen for vehicle changes or PLAYER_ENTERING_WORLD since oUF automatically updates all elements and tagged fontstrings when those events occur, and passes the correct unit to the tag method.

sirann · 12-11-14, 06:50 PM

As an update I did this: http://pastebin.com/FcVTCRA4

Obviously it's only the player, I have to raid or I'd finish the other units. Relevant tags I made can be found here: http://pastebin.com/KGfZFXEA

I appreciate both of your initial, and hopefully ongoing, feedback.

sirann · 12-12-14, 12:53 PM

1.

Code:

self.HealthBG = HealthBG

Noted.
-----------------------------------------------------------------------------

2.

Code:

self.Health:SetHeight(29)

Noted. Will add self.Health:SetPoint('BOTTOM', 0, 3)
-----------------------------------------------------------------------------

3. Several problems in here:
A. I'm assuming global name is PlayerLeaderAssistFrame and PlayerLeaderAssistTexture, will remove
B. Will use oUF elements and change texture path
C. The texture paths do work. If they're not supposed to, IDK why, but I can assure you they do.
D. That is bad copypasta. But it did work, so I didn't catch that.
-----------------------------------------------------------------------------

4. oUF also already provides a combat indicator element and a resting indicator element

Noted. Will use those and change textures again if needed like assist frame.
-----------------------------------------------------------------------------

5. However, oUF does not provide any elements named "BuffBars" or "DebuffBars"

This is from an element you rewrote for me, oUF_AuraBars. I still use it, although I think I'm literally the only person to do so. I just couldn't deal with elkbuffbars another god damn day.
-----------------------------------------------------------------------------

6.

Code:

oUF:RegisterStyle('Terenna', Shared)
oUF:RegisterStyle('oUF_Terenna_Player', UnitSpecific.player)
oUF:SetActiveStyle('oUF_Terenna_Player')

so inside of my shared function put:

oUF:RegisterStyle('oUF_Terenna_Player', UnitSpecific.player)
as well as other units with the UnitSpecific.insertunitnamehere ?

And then just have oUF:SetActiveStyle('oUF_Terenna') at the bottom of my code outside of all functions?