Thread Tools Display Modes
08-27-17, 08:08 AM   #1
Terenna
A Flamescale Wyrmkin
AddOn Author - Click to view addons
Join Date: Jun 2016
Posts: 105
tComboPoints optimization and feedback

Good afternoon everyone. I finally feel comfortable with the ability of my addon to be bug free and work exactly as intended. I have published it for others to use, and am now looking for feedback on my coding for future addon endeavors.

This addon is a simple combo point tracker with support for various rogue talents as well as a subtlety specific function which generates combo points when you successfully auto attack a number of times.

The code may be found here: https://pastebin.com/MDRmMJeg

I welcome all criticism of styling and coding efforts. Please feel free to include areas I could optimize memory/cpu usage or I have strangely styled something.

Thank you.
  Reply With Quote
08-27-17, 09:23 AM   #2
Resike
A Pyroguard Emberseer
AddOn Author - Click to view addons
Join Date: Mar 2010
Posts: 1,290
These calls will not work on a non-english clients:

Lua Code:
  1. strsub(arg13, 1, 12) == 'Shadow Blade'
  2. arg13 == 'Shadow Techniques'

You gonna have to use spellIDs, these are the the proper arguments for the 3 event type you need:

Lua Code:
  1. -- SPELL_DAMAGE
  2. local timestamp, eventType, hideCaster, sourceGUID, sourceName, sourceFlags, sourceRaidFlags, destGUID, destName, destFlags, destRaidFlags, spellID, spellName, spellSchool, amount, overkill, school, resisted, blocked, absorbed, critical, glancing, crushing, isOffHand = ...
  3.  
  4. -- SPELL_ENERGIZE
  5. local timestamp, eventType, hideCaster, sourceGUID, sourceName, sourceFlags, sourceRaidFlags, destGUID, destName, destFlags, destRaidFlags, spellID, spellName, spellSchool, amount, powerType = ...
  6.  
  7. -- SWING_DAMAGE
  8. local timestamp, eventType, hideCaster, sourceGUID, sourceName, sourceFlags, sourceRaidFlags, destGUID, destName, destFlags, destRaidFlags, amount, overkill, school, resisted, blocked, absorbed, critical, glancing, crushing, isOffHand = ...
  Reply With Quote
08-27-17, 10:29 AM   #3
Terenna
A Flamescale Wyrmkin
AddOn Author - Click to view addons
Join Date: Jun 2016
Posts: 105
Thank you, good point.

I've updated it to:
Lua Code:
  1. local _, arg2, _, arg4, _, _, _, _, _, _, _, arg12, _, _, arg15 = ...
  2.         if arg4 == playerGUID then
  3.             if (arg2 == 'SWING_DAMAGE' and type(arg12) == 'number') or (arg2 == 'SPELL_DAMAGE' and (arg12 == 121473 or arg12 == 121474) and type(arg15) == 'number') then --this behemoth ensures that our white swing or shadow blade swing landed and wasnt dodged blocked parried missed etc
  4.                 shadowtechniques = shadowtechniques + 1
  5.                 UpdateSTFText()
  6.             elseif (arg2 == 'SPELL_ENERGIZE' and arg12 == 196911) then --shadow techniques procced 1-2 combo points, reset the counter
  7.                 shadowtechniques = 0
  8.                 UpdateSTFText()
  9.             end
  10.         end
  Reply With Quote
08-29-17, 09:24 AM   #4
p3lim
A Pyroguard Emberseer
 
p3lim's Avatar
AddOn Author - Click to view addons
Join Date: Feb 2007
Posts: 1,710
I would advice you to start using descriptive variable names, like the ones Resike suggested, as they explain exactly what you are checking for, and it will help you in the long term, as well as anyone evaluating your code.
  Reply With Quote
08-29-17, 11:20 AM   #5
Terenna
A Flamescale Wyrmkin
AddOn Author - Click to view addons
Join Date: Jun 2016
Posts: 105
p3lim,

Thank you for your feedback. Huge fan of your addons and your code. When I saw the difference in readability between arg2 and spellID or what not I saw the importance of naming variables more descriptively. One possible problem with this is that the 12th argument for a swing_damage event is a an amount of damage or a string of miss, block, parry, dodge, etc, whereas the 12th argument for spell_damage is a spellID. To assuage this problem, I could have the first 4 arguments set as they are always the same, and then perform an if check on the 2nd argument to determine type of CLEU event. At that point I could assign variables that would be more accurately and descriptively named, but I would be utilizing extra CPU cycles to perform the check, overwrite the 2nd and 4th arguments unnecessarily and also have extra variables taking up (albeit minuscule) amounts of memory.

Outside of the CLEU variables, are shorthand such as STF for shadowtechniquesframe acceptable? Or do authors generally give longer names?
  Reply With Quote
08-29-17, 03:19 PM   #6
Seerah
Fishing Trainer
 
Seerah's Avatar
WoWInterface Super Mod
Featured
Join Date: Oct 2006
Posts: 10,860
You can name them whatever you want. If you will remember in a year what STF is for, then go ahead and name it that. The purpose for descriptive variable names is so that you will remember when you come back later to maintain your code. It also makes it easier for anyone else reading your code. You don't *have* to do it, it's just nice.

For args that can be different types of values... Aren't you going to be doing different things based on what CLEU event it is anyway?
__________________
"You'd be surprised how many people violate this simple principle every day of their lives and try to fit square pegs into round holes, ignoring the clear reality that Things Are As They Are." -Benjamin Hoff, The Tao of Pooh

  Reply With Quote
08-30-17, 12:02 AM   #7
MunkDev
A Scalebane Royal Guard
 
MunkDev's Avatar
AddOn Author - Click to view addons
Join Date: Mar 2015
Posts: 431
Originally Posted by Terenna View Post
When I saw the difference in readability between arg2 and spellID or what not I saw the importance of naming variables more descriptively. One possible problem with this is that the 12th argument for a swing_damage event is a an amount of damage or a string of miss, block, parry, dodge, etc, whereas the 12th argument for spell_damage is a spellID. To assuage this problem, I could have the first 4 arguments set as they are always the same, and then perform an if check on the 2nd argument to determine type of CLEU event. At that point I could assign variables that would be more accurately and descriptively named, but I would be utilizing extra CPU cycles to perform the check, overwrite the 2nd and 4th arguments unnecessarily and also have extra variables taking up (albeit minuscule) amounts of memory.
It's very common to use event-specific functions stored in your frame table and then run them from your event handler. This helps to compartmentalise the code and effectively removes the problem of choosing whether to name your arguments descriptively or not.

Lua Code:
  1. frame:SetScript("OnEvent", function(self, event, ...)
  2.     if self[event] then
  3.         self[event](self, ...)
  4.     end
  5. end)
  6.  
  7. function frame:PLAYER_ENTERING_WORLD(...)
  8.     -- code
  9. end
  10.  
  11. function frame:PLAYER_REGEN_DISABLED(...)
  12.     -- code
  13. end
  14.  
  15. function frame:COMBAT_LOG_EVENT_UNFILTERED(...)
  16.     -- code
  17. end

Any code that is general and applicable to every event can be written directly in the event handler. In the case of processing combat events, you can apply the same principle. In terms of optimisation, there are a couple of things you can do, such as upvalueing the sub table in your color picker instead of doing the table lookup 3 times (one for each color value), but it will hardly make any difference with such a tiny code snippet.
__________________

Last edited by MunkDev : 08-30-17 at 12:47 AM.
  Reply With Quote
08-30-17, 07:21 AM   #8
Terenna
A Flamescale Wyrmkin
AddOn Author - Click to view addons
Join Date: Jun 2016
Posts: 105
Munk,

Thank you for the feedback. I've seen others use
Lua Code:
  1. frame:SetScript("OnEvent", function(self, event, ...)
  2.     if self[event] then
  3.         self[event](self, ...)
  4.     end
  5. end)

And I get the concept of how it works, but I don't truly understand the syntax. My understanding is this: we see if the event is a table key of our function table with
Lua Code:
  1. if self[event] then
. If this passes, then it runs a function that has the same name as the event.

Later in the code you name the function like:
Lua Code:
  1. function frame:PLAYER_ENTERING_WORLD(...)
  2.     -- code
  3. end

but wouldn't this create a global function as there's no local before the function? Or is it still localized because that function is bound to (in this case) the STF frame from the initial SetScript?

Additionally, I don't understand how you would upvalue a table value. I understand that an upvalue look up is faster than a table look up, especially three table look ups, I just don't fully grasp how you would do it.

Thank you for everyone's comments and feedback. It has been very helpful.
  Reply With Quote
08-30-17, 07:34 AM   #9
p3lim
A Pyroguard Emberseer
 
p3lim's Avatar
AddOn Author - Click to view addons
Join Date: Feb 2007
Posts: 1,710
With the API "CreateFrame", what that really does is create a table and inject it with methods (in essence).
When you create a function "frame:PLAYER_LOGIN" you are basically assigning that to the table "frame".

Lua Code:
  1. local t = {
  2.     PLAYER_LOGIN = function() end
  3. }
is the same as
Lua Code:
  1. local t = {}
  2. function t:PLAYER_LOGIN()
  3. end

So in SetScript in that example, you're calling the function by its table key in the current table (or frame rather), which in the context of SetScript is "self".
You could just as well (in the SetScript block) call it by "frame[event]" (just in case the "self" variable confuses you).
And the value for that key would be the function.

It's never global, but you can access it globally if the frame itself is global (either by variable or name, the 2nd argument in CreateFrame), by using the key (in this case PLAYER_LOGIN or any other event).

The reason you check for "self[event]" is to make sure you don't get errors when the function doesn't exist in the table.
  Reply With Quote

WoWInterface » Developer Discussions » General Authoring Discussion » tComboPoints optimization and feedback

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