Quantcast oUF_Layout optimization - WoWInterface
Thread Tools Display Modes
12-11-14, 02:07 AM   #1
sirann
A Flamescale Wyrmkin
Join Date: Mar 2007
Posts: 142
oUF_Layout optimization

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!
  Reply With Quote
12-11-14, 10:07 AM   #2
Dawn
A Molten Giant
 
Dawn's Avatar
AddOn Author - Click to view addons
Join Date: May 2006
Posts: 918
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
__________________
Rock: "We're sub-standard DPS. Nerf Paper, Scissors are fine."
Paper: "OMG, WTF, Scissors!"
Scissors: "Rock is OP and Paper are QQers. We need PvP buffs."

"neeh the game wont be remembered as the game who made blizz the most money, it will be remembered as the game who had the most QQ'ers that just couldnt quit the game for some reason..."


Last edited by Dawn : 12-11-14 at 10:14 AM.
  Reply With Quote
12-11-14, 11:56 AM   #3
Phanx
Cat.
 
Phanx's Avatar
AddOn Author - Click to view addons
Join Date: Mar 2006
Posts: 5,617
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.
__________________
Author/maintainer of Grid, PhanxChat, oUF_Phanx, and many more.
Troubleshoot an addon Turn any code into an addon More addon resources
Need help with your code? Post all of your actual code! Attach or paste your files.
Please dont PM me about addon bugs or code questions. Post a comment or forum thread instead!

Last edited by Phanx : 12-11-14 at 12:01 PM.
  Reply With Quote
12-11-14, 12:21 PM   #4
sirann
A Flamescale Wyrmkin
Join Date: Mar 2007
Posts: 142
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?

Last edited by sirann : 12-11-14 at 01:52 PM.
  Reply With Quote
12-11-14, 04:31 PM   #5
Phanx
Cat.
 
Phanx's Avatar
AddOn Author - Click to view addons
Join Date: Mar 2006
Posts: 5,617
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 View Post
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.
__________________
Author/maintainer of Grid, PhanxChat, oUF_Phanx, and many more.
Troubleshoot an addon Turn any code into an addon More addon resources
Need help with your code? Post all of your actual code! Attach or paste your files.
Please dont PM me about addon bugs or code questions. Post a comment or forum thread instead!
  Reply With Quote
12-11-14, 06:50 PM   #6
sirann
A Flamescale Wyrmkin
Join Date: Mar 2007
Posts: 142
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.
  Reply With Quote
12-11-14, 10:39 PM   #7
Phanx
Cat.
 
Phanx's Avatar
AddOn Author - Click to view addons
Join Date: Mar 2006
Posts: 5,617
Tags:
Code:
return format('|cff%02x%02x%02x%s', r, g, 0, healthcurrent)
Very minor, but there's no need to spend CPU time turning that 0 into 00 over and over; just hardcode the 00:
Code:
return format('|cff%02x%02x00%s', r, g, healthcurrent)
You could even take that further and avoid that second format call entirely:
Code:
        local color
        if healthcurrent < healthmax then
                color = '|cffb40000'
        else
                color = '|cff00b400'
        end
        if healthcurrent >= 1000000 then
                healthcurrent = format('%.1fm', healthcurrent/1000000)
        elseif healthcurrent >= 1000 then
                healthcurrent = format('%.0fk', healthcurrent/1000)
        end
        return color .. healthcurrent
================================
Code:
powertype = select(2,UnitPowerType(u))
select is a complete waste of CPU time in 99.9999999999999999% of situations, including this one. Use a throwaway variable instead:
Code:
local powercurrent, powertype, pr, pg, pb, _
_, powertype = UnitPowerType(u)
...or just the less intuitive but equally effective method of overwriting the variable with itself:
Code:
powertype, powertype = UnitPowerType(u)
================================
Code:
                elseif powertype == 'FOCUS' then
                        pr, pg, pb = 255, 128, 65
                elseif powertype == 'ENERGY' then
                        pr, pg, pb = 255, 255, 0
                elseif powertype == 'RUNIC_POWER' then
                        pr, pg, pb = 0, 209, 255
                else
                        pr, pg, pb = 255, 0, 0
                end
In all these cases, those are the default colors for those power types, so rather than a list of "elseif" conditions and hardcoded values, just fetch the default colors:
Code:
                else
                        local c = PowerBarColor[powertype]
                        pr, pg, pb = c.r * 255, c*g * 255, c*b * 255
                end
__________________
Author/maintainer of Grid, PhanxChat, oUF_Phanx, and many more.
Troubleshoot an addon Turn any code into an addon More addon resources
Need help with your code? Post all of your actual code! Attach or paste your files.
Please dont PM me about addon bugs or code questions. Post a comment or forum thread instead!
  Reply With Quote
12-11-14, 10:53 PM   #8
Phanx
Cat.
 
Phanx's Avatar
AddOn Author - Click to view addons
Join Date: Mar 2006
Posts: 5,617
Other:
De-duplication looking much better overall.
Code:
                self.Health:SetHeight(29)
Having this in the unit specific function with the frame height set in the shared function will present difficulties when making changes later, since you'll have to change one height value in one place, and another in another place, and it's very likely you'll just change it in one place and then have to waste time debugging and remembering where the other value is.

The best way to avoid this would be to use relative positioning. Instead of anchoring the health bar by the TOPLEFT and TOPRIGHT corners and giving it an explicit height, add a third anchor to the BOTTOM point with a vertical offset to leave space for the power bar -- and then anchor the TOP of the power bar to the BOTTOM of the health bar. That way the only value you have to change is the frame height, and the health bar will automatically get bigger or smaller accordingly.

========================

Several problems in here:
Code:
                local leaderassistframe = CreateFrame('Button', 'PlayerLeaderAssistFrame', self)
                        leaderassistframe:SetSize(3,8)
                        leaderassistframe:SetPoint('TOPLEFT', self, 'TOPLEFT', 2, 0)
                        leaderassistframe:RegisterEvent('GROUP_ROSTER_UPDATE')
                        leaderassistframe:RegisterEvent('PARTY_LEADER_CHANGED')
                        leaderassistframe:RegisterEvent('PLAYER_ENTERING_WORLD')
                        
                local leaderassisttexture = leaderassistframe:CreateTexture('PlayerLeaderAssistTexture', 'OVERLAY')
                        --leaderassisttexture:SetTexture('Interface\AddOns\oUF_Terenna\Status_Texture', 'OVERLAY')
                        leaderassisttexture = leaderassistframe:CreateTexture('Interface\AddOns\oUF_P3lim\F1_StatusBox_Bar', 'OVERLAY')
                        leaderassisttexture:SetTexture(1, 1, 0)
                        leaderassisttexture:SetAllPoints(true)
                        
                leaderassistframe:SetScript('OnEvent', function(self, event, unit)
                        if UnitIsGroupLeader('player') or UnitIsRaidOfficer('player') then
                                leaderassisttexture:Show()
                        else
                                leaderassisttexture:Hide()
                        end
                end)
1. These objects do not need global names. Get rid of the global names.

2. These objects don't need to exist at all. Get rid of them and use oUF's native Assistant and Leader elements; you can set them both to use the same texture if you really don't want to know the difference.

3. Those file paths are invalid; you need to either escape your backslashes by doubling them, or use [[ literal string notation ]] instead of "double quotes" around the path.

4. This appears to be a bad copypasta:
Code:
leaderassisttexture = leaderassistframe:CreateTexture('Interface\AddOns\oUF_P3lim\F1_StatusBox_Bar', 'OVERLAY')
a. You already created a texture object and assigned it to the "leaderassisttexture" variable.
b. This usage of CreateTexture will actually produce a new texture object whose global name is that (malformed) file path. You should delete this line and uncomment the SetTexture line directly above it.

========================

oUF also already provides a combat indicator element and a resting indicator element; you don't need to create your own frames and manage them yourself with event handlers. Just use the elements and give them the textures, dimensions, etc. that you want.

========================

However, oUF does not provide any elements named "BuffBars" or "DebuffBars" so if you want those objects to do anything, you either need to use the proper names -- "Buffs" and "Debuffs" -- or install/write a third-party element/plugin to handle them.

========================

Code:
oUF:RegisterStyle('Terenna', Shared)
oUF:RegisterStyle('oUF_Terenna_Player', UnitSpecific.player)
oUF:SetActiveStyle('oUF_Terenna_Player')
While this works (though the first line isn't necessary) I'd suggest you're actually doing things a bit backward. You should call the UnitSpecific function from inside the Shared function, rather than the other way around, so you only need to register one style and spawn all your frames with the same style.
__________________
Author/maintainer of Grid, PhanxChat, oUF_Phanx, and many more.
Troubleshoot an addon Turn any code into an addon More addon resources
Need help with your code? Post all of your actual code! Attach or paste your files.
Please dont PM me about addon bugs or code questions. Post a comment or forum thread instead!
  Reply With Quote
12-12-14, 12:53 PM   #9
sirann
A Flamescale Wyrmkin
Join Date: Mar 2007
Posts: 142
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?

Last edited by sirann : 12-12-14 at 01:10 PM.
  Reply With Quote
12-13-14, 02:57 AM   #10
Phanx
Cat.
 
Phanx's Avatar
AddOn Author - Click to view addons
Join Date: Mar 2006
Posts: 5,617
Originally Posted by sirann View Post
so inside of my shared function put:

oUF:RegisterStyle('oUF_Terenna_Player', UnitSpecific.player)
as well as other units with the UnitSpecific.insertunitnamehere ?
No, that won't do anything there; you need to actually call the function:

Code:
local func = UnitSpecific[unit]
if func then
   func()
end
Also, that would need to be at the end of the function, so all the objects are created before the unit specific function tries to do stuff with them.

Originally Posted by sirann View Post
And then just have oUF:SetActiveStyle('oUF_Terenna') at the bottom of my code outside of all functions?
Yes.
__________________
Author/maintainer of Grid, PhanxChat, oUF_Phanx, and many more.
Troubleshoot an addon Turn any code into an addon More addon resources
Need help with your code? Post all of your actual code! Attach or paste your files.
Please dont PM me about addon bugs or code questions. Post a comment or forum thread instead!
  Reply With Quote

WoWInterface » Featured Projects » oUF (Otravi Unit Frames) » oUF_Layout optimization

Thread Tools
Display Modes

Posting Rules
You may not post new threads
You may not post replies
You may not post attachments
You may not edit your posts

vB code is On
Smilies are On
[IMG] code is On
HTML code is Off