WoWInterface

WoWInterface (https://www.wowinterface.com/forums/index.php)
-   General Authoring Discussion (https://www.wowinterface.com/forums/forumdisplay.php?f=20)
-   -   tComboPoints optimization and feedback (https://www.wowinterface.com/forums/showthread.php?t=55681)

Terenna 08-27-17 08:08 AM

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.

Resike 08-27-17 09:23 AM

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 = ...

Terenna 08-27-17 10:29 AM

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

p3lim 08-29-17 09:24 AM

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.

Terenna 08-29-17 11:20 AM

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?

Seerah 08-29-17 03:19 PM

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?

MunkDev 08-30-17 12:02 AM

Quote:

Originally Posted by Terenna (Post 324848)
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.

Terenna 08-30-17 07:21 AM

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.

p3lim 08-30-17 07:34 AM

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.


All times are GMT -6. The time now is 07:25 PM.

vBulletin © 2024, Jelsoft Enterprises Ltd
© 2004 - 2022 MMOUI