Reply
Thread Tools Display Modes
Unread 03-15-13, 09:49 PM   #1
Clamsoda
A Cobalt Mageweaver
Join Date: Nov 2011
Posts: 244
Thoughts on an AddOn using "COMBAT_LOG_EVENT_UNFILTERED"

Good Evening,

I was approached by the Death Knight tank in my guild, and was asked to write a stand-alone AddOn that provided death strike heal prediction. At first I wasn't sure I could write such an AddOn because I've never tried to interpret information from the combat log before, but I decided I'd give it a go.

I'd like some feedback and insight on the code thus far, keeping in mind the following notes:
  • The code is VERY far from complete. I wanted to throw a "proof of concept" together.
  • Death Strike heals you for the amount of damage you have sustained from non players within the last 5 seconds, with some additional modifiers that I chose to ignore for this test.
  • The table that holds the key-value pairs of time stamps and damage values populates to an unlimited size. I intend to remove all non-relevant entries.
  • The code is really sloppy, and not well commented at all, as this was more of a flow-of-thought.
My approach was to isolate all damage(JUST Swing damage for this test) that has an NPC as a source, and the player as the destination. Those values are then added to a table with the timeStamp as the key, and the damage amount as the value. A for-loop sorts through the table and adds together any damage values that have happened within the last 5 seconds.

Just keep in mind that for now, it is quite sloppy and not optimized. I hope this doesn't deter anyone from having a glance over it.

I'd really appreciate insight on the methods and processes, and not an entire written AddOn to replace what I have come up with. This has been a really hands-on learning process for me, and I'd like to continue that.

Thanks in advance for any opinions or suggestions.

http://pastebin.com/rgZ9ZwqH

Last edited by Clamsoda : 03-15-13 at 09:53 PM.
Clamsoda is offline   Reply With Quote
Unread 03-15-13, 11:54 PM   #2
Torhal
A Pyroguard Emberseer
 
Torhal's Avatar
AddOn Author - Click to view addons
Join Date: Aug 2008
Posts: 1,056
I just looked over it briefly - one thing I spotted (and I know you said this isn't optimized...) is that you should change this:

Code:
                if destGUID == UnitGUID("player") and knownTypes[maskedTypeBit] == "NPC" then
                        if eventType == "SWING_DAMAGE" then
                                local amount = select(12, ...)
                                damageTable[timeStamp] = amount
                        end -- if event
                end -- if destGUID
to this:

Code:
                if eventType == "SWING_DAMAGE" and destGUID == UnitGUID("player") and knownTypes[maskedTypeBit] == "NPC" then
                            local amount = select(12, ...)
                            damageTable[timeStamp] = amount
                end -- if destGUID

...and avoid using select() here since SWING_DAMAGE could come up several times per second.
__________________
Whenever someone says "pls" because it's shorter than "please", I say "no" because it's shorter than "yes".

Author of Revelation, Spamalyzer, TravelAgent, Volumizer, and many other AddOns.
Torhal is online now   Reply With Quote
Unread 03-16-13, 12:15 AM   #3
Clamsoda
A Cobalt Mageweaver
Join Date: Nov 2011
Posts: 244
Exactly the kind of criticism I was looking for!

I'll make the change for sure. Thank you.
Clamsoda is offline   Reply With Quote
Unread 03-16-13, 02:09 AM   #4
Phanx
A Pyroguard Emberseer
 
Phanx's Avatar
AddOn Author - Click to view addons
Join Date: Mar 2006
Posts: 4,391
You can make that even more efficient by storing the player's GUID in a variable, and referring to that, instead of looking up the same value over and over again every time a combat log event occurs. The GUID isn't available before PLAYER_LOGIN on a fresh login, so you'll need to wait until then to populate the variable.

Similarly, in your OnUpdate function, you have this:
Code:
        local function onUpdate(self, elapsed)
                totalTime = totalTime + elapsed
                if totalTime >= updateInterval then
                        for k, v in pairs(damageTable) do
                                if k >= time() - 5 then
You could improve efficiency by looking up time() and subtracting 5 once per OnUpdate, instead of once per table entry per OnUpdate:
Code:
        local function onUpdate(self, elapsed)
                totalTime = totalTime + elapsed
                if totalTime >= updateInterval then
                        local t = time() - 5
                        for k, v in pairs(damageTable) do
                                if k >= t then
Also, it's kind of pointless to define your OnUpdate function, and then set it separately:

Code:
local function onUpdate(self, elapsed)
        -- stuff happening here
end
frame:SetScript("OnUpdate", onUpdate)
You can just define the function anonymously in the SetScript call:

Code:
frame:SetScript("OnUpdate", function(self, elapsed)
        -- stuff happening here
end)
You might also consider clearing out old table entries; otherwise after a few hours of play you'll have a pretty large table.

Code:
                        local t = time() - 5
                        for k, v in pairs(damageTable) do
                                if k < t then
                                        -- old, get rid of it
                                        damageTable[k] = nil
                                else
                                        -- within the window, use it
                                        totalDamage = totalDamage + v
                                end
                        end
__________________
Author/maintainer of Grid, PhanxChat, ShieldsUp, and many more.
Troubleshoot an addonTurn any code into an addonMore addon resources
Need help with your code? Post all of your actual code! Attach or paste your files.
Please don’t PM me about addon bugs or code questions. Post a comment or forum thread instead!

Last edited by Phanx : 03-16-13 at 02:17 AM.
Phanx is offline   Reply With Quote
Unread 03-16-13, 02:21 AM   #5
Clamsoda
A Cobalt Mageweaver
Join Date: Nov 2011
Posts: 244
Thanks for the response Phanx.

I was already planning on removing all entries older than 5 seconds from the table, just never got around to writing the code in.

I am glad to know there aren't any glaring issues with my approach. I'm looking forward to completing it!
Clamsoda is offline   Reply With Quote
Unread 03-16-13, 01:12 PM   #6
bsmorgan
A Theradrim Guardian
AddOn Author - Click to view addons
Join Date: Mar 2005
Posts: 62
You might also look at the addon Blood Shield Tracker (http://www.curse.com/addons/wow/blood-shield-tracker).
bsmorgan is offline   Reply With Quote
Unread 03-16-13, 09:01 PM   #7
Clamsoda
A Cobalt Mageweaver
Join Date: Nov 2011
Posts: 244
Thanks for the response bsmorgan. I have taken a look at it, but the over 5,000 lines of code make it quite difficult to sift through relevant parts of the code. I may fall back on it if I have some specific concerns further down the line.

Edit: In response to Torhal's suggestion of not using select(), should I just upsource the select() function, or use something similar to:

Lua Code:
  1. local _, _, _, _, _, _, _, _, _, _, _, amount = ...

Edit 2: It is taking ~15 seconds for my loop to decide that the timestamps are too old. Is there a disparity between what time() and timestamp returns? I was under the impression that they were both in seconds, and that timestamp just returns with miliseconds as well, which shouldn't be contributing to this issue. Does "time() - 5" not reflect the last 5 seconds?

Edit 3: I had my SWING_DAMAGE function print the timeStamp, and time(), and there is a 12 second disparity between them. Is this something that will ALWAYS be the same? Is it some kind of bug?

Edit 4: For now, I will just use time() as the key, rather than relying on timeStamp. Seems to be working well.

Last edited by Clamsoda : 03-17-13 at 12:03 AM.
Clamsoda is offline   Reply With Quote
Unread 03-17-13, 12:51 AM   #8
Phanx
A Pyroguard Emberseer
 
Phanx's Avatar
AddOn Author - Click to view addons
Join Date: Mar 2006
Posts: 4,391
Originally Posted by Clamsoda View Post
Edit: In response to Torhal's suggestion of not using select(), should I just upsource the select() function, or use something similar to: <code>
If those are the only options, then you'd use the code you posted. Calling functions is sloooooow, even if they're upvalued.

However, since your code is only handling one event, you can just name the arguments as they're passed to your handler:

Code:
frame:SetScript("OnEvent", function(self, event, timestamp, combatEvent, _, sourceGUID, _, _, _, destGUID, _, _, _, amount)
     if combatEvent == "SWING_DAMAGE" and destGUID == PLAYER_GUID and knownTypes[tonumber(strsub(sourceGUID, 5, 5), 16) % 8] == "NPC" then
          damageTable[time()] = amount
     end
end)
On a side note, strsub(<string>, 5, 5) is faster than <string>:sub(5, 5) even if you don't upvalue strsub, and way faster if you do.
__________________
Author/maintainer of Grid, PhanxChat, ShieldsUp, and many more.
Troubleshoot an addonTurn any code into an addonMore addon resources
Need help with your code? Post all of your actual code! Attach or paste your files.
Please don’t PM me about addon bugs or code questions. Post a comment or forum thread instead!

Last edited by Phanx : 03-17-13 at 12:58 AM.
Phanx is offline   Reply With Quote
Unread 03-17-13, 01:05 AM   #9
Clamsoda
A Cobalt Mageweaver
Join Date: Nov 2011
Posts: 244
My code needs to be able to handle SWING_DAMAGE, SPELL_DAMAGE, and SPELL_PERIODIC_DAMAGE. Only SWING_DAMAGE has "amount" seated at the 12th spot of ..., SPELL_DAMAGE and SPELL_PERIODIC_DAMAGE have "amount" seated at the 15th spot of ..., so I think I need to keep it the way it is. Let me know what you think.

Also, I'll definitely substitute strsub in!

Edit: I guess I could pass it as:

(self, event, _, eventType, _, sourceGUID, _, _, _, destGUID, _, _, _, swingAmount, _, _, spellAmount)

The first 11 never change, and swingAmount or spellAmount would be correct so long as they are called for the right eventType. I don't know how much of an impact this would have, just an idea.

This is what I've got now. I've had to make some changes to the way information is stored. Events are stored into the table with an index that increments for each time the event fires, and a nested table that holds the time and amount, that way if two events happen at the EXACT same time, one isn't written in, then overwritten.

Commenting has sort of gone to hell...=/

http://pastebin.com/ccbqiL4A

Last edited by Clamsoda : 03-17-13 at 01:31 AM.
Clamsoda is offline   Reply With Quote
Unread 03-17-13, 04:07 AM   #10
Phanx
A Pyroguard Emberseer
 
Phanx's Avatar
AddOn Author - Click to view addons
Join Date: Mar 2006
Posts: 4,391
You can also use the vararg (...) only for the arguments that change based on the event type:

Code:
f:SetScript("OnEvent", function(self, event, _, eventType, _, sourceGUID, _, sourceFlags, _, destGUID, _, destFlags, _, ...)
     local maskedTypeBit = tonumber(strsub(sourceGUID, 5, 5), 16) % 8
     if maskedTypeBit == "NPC" then
          if eventType == "SWING_DAMAGE" then
               if destGUID == playerGUID and 
                    local amount = ...
                    damageTable[time()] = amount
               end
          elseif eventType == "SOME_EVENT" then
               -- etc
          end
     end
end)
Also, the new damageTable format you're using is going to generate a large amount of garbage, since creating new tables is relatively costly in terms of memory. Since there is no advantage in your case to using an indexed table (sequential integer keys) over a hash table (keys can be anything) -- especially since you are nil-ing out keys, which puts holes in the table and prevents the use of any index-related functions like ipairs or table.sort on the table -- you should just use your original damageTable[time()] = amount format instead.

Also, the way you're detecting NPCs seems rather inefficient, with two function calls, a string creation, and a modulus operation. Why not use a single bit.band comparison on the sourceFlags argument instead? It'll be much faster, especially if you upvalue bit.band and the specific flag value constants you're interested in.

Code:
if bit.band(sourceFlags, COMBATLOG_OBJECT_TYPE_NPC) > 0 then
    -- source is an NPC
end
More info about unit flags here:
http://www.wowpedia.org/UnitFlag
__________________
Author/maintainer of Grid, PhanxChat, ShieldsUp, and many more.
Troubleshoot an addonTurn any code into an addonMore addon resources
Need help with your code? Post all of your actual code! Attach or paste your files.
Please don’t PM me about addon bugs or code questions. Post a comment or forum thread instead!
Phanx is offline   Reply With Quote
Unread 03-17-13, 02:46 PM   #11
Clamsoda
A Cobalt Mageweaver
Join Date: Nov 2011
Posts: 244
The answer to your "why not..." is because I have no idea what bit.band is >.<.

I'll certainly look into every single one of your suggestions!

Also, the reason I had to change the way I am storing information, is because if two evens happen at the exact same time, they'll return the same integer from time(), so the first event will be written in, and then overwritten with the second event, seeing as they have the same key value.

I chose to just increment some integer to use as the key just to ensure that there would always be a unique key, no matter what the values were.

Is that making sense? That's what was happening in my testing at least. I was able to find a pack of wolves in Vale of Eternal Blossoms that ALL cast the SAME spell at the exact same time, and it would print(debugging I had setup) all of their spells, but it would only recognize one in the table(the damage varied by ~3,000). I assumed this was because they key was the same for all of them, and that the value just took on the last one to register.

Thank you so much for your help!

Here is what I have gotten my combat log parsing function to. I've tried to heed all suggestions, seems a lot cleaner than when I first started.

I upsourced bit.band as bit_band (was throwing an error about having a "." when I tried up upsource as local bit.band = bit.band, but I found some post you made where it was bit_band ;])

Lua Code:
  1. -- OnEvent function to handle all of the registered events firing
  2.     addonFrame:SetScript("OnEvent", function(self, event, _, eventType, _, sourceGUID, _, sourceFlags, _, destGUID, _, _, _, ...)
  3.         if event == "PLAYER_LOGIN" then
  4.             -- Get and set the player GUID for later use
  5.             playerGUID = UnitGUID("player")
  6.             -- Unregister PLAYER_LOGIN as it is no longer needed
  7.             addonFrame:UnregisterEvent("PLAYER_LOGIN")
  8.         elseif event == "COMBAT_LOG_EVENT_UNFILTERED" then
  9.             eventIndex = eventIndex + 1
  10.             local currentTime = time()
  11.             if (bit_band(sourceFlags, COMBATLOG_OBJECT_TYPE_NPC) > 0) and (destGUID == playerGUID) then
  12.                 if eventType == "SWING_DAMAGE" then
  13.                     local amount = ...
  14.                     damageTable[eventIndex] = {currentTime, amount}
  15.                 elseif eventType == "SPELL_DAMAGE" or "SPELL_PERIODIC_DAMAGE" or "RANGE_DAMAGE" then
  16.                     local _, _, _, amount = ...
  17.                     damageTable[eventIndex] = {currentTime, amount}
  18.                 end -- if eventType
  19.             end -- if bit.band
  20.         end -- if event
  21.     end)

Last edited by Clamsoda : 03-17-13 at 05:18 PM.
Clamsoda is offline   Reply With Quote
Unread 03-17-13, 06:15 PM   #12
Phanx
A Pyroguard Emberseer
 
Phanx's Avatar
AddOn Author - Click to view addons
Join Date: Mar 2006
Posts: 4,391
Originally Posted by Clamsoda View Post
The answer to your "why not..." is because I have no idea what bit.band is >.<.
Bitwise operators allow lots of information to be stored in a single bitflag. For example, the same flag (it's a hexadecimal number, and looks like 0x12A3B4C5D6E7F89 or something) shown in the combat log for a unit can tell you whether the unit is a player, pet, totem, guardian, NPC, or object; whether it's controlled by a player or not; how it reacts to the player; whether it's in the player's group or not; etc.

In simple terms, bit.bor combines bitflags (for example, you'd use it to create a bitflag describing a server-controlled NPC that's hostile to you) and bit.band lets you determine whether a bitflag includes another bitflag (in your case, the NPC-controlled flag).

Blizzard uses these operators in their combat log code.

Originally Posted by Clamsoda View Post
... if two evens happen at the exact same time, they'll return the same integer from time(), so the first event will be written in, and then overwritten with the second event, seeing as they have the same key value.
You could save the value in ms instead, and just add 1 if it's a duplicate:
Code:
-- Add an event:
local now = time() * 1000
if damageTable[now] then
    now = now + 1
end
damageTable[now] = amount

-- Loop over the events:
local earliest = (time() - 5) * 1000
...
Alternatively, you could use two flat indexed tables:
Code:
local damageTimes, damageAmounts = {}, {}

-- Add an event:
damageTimes[#damageTimes+1] = time()
damageAmounts[#damageAmounts+1] = amount

-- Loop over the times, add up relevant values, remove irrelevant ones:
local i, total, earliest = 1, 0, time() - 5
while i <= #damageTimes do
    if damageTimes[i] >= earliest then
        -- It's within 5 seconds; add it to the total.
        total = total + damageAmounts[i]
        -- Move on to the next record.
        i = i + 1
    else
        -- It's old; remove it from both tables.
        tremove(damageTimes, i)
        tremove(damageAmounts, i)
        -- Don't increment the index; the next record just moved up into the current index.
    end
end
As a third alternative, you could use subtables as you are now, but recycle them so you're not constantly creating new tables:
Code:
local GetTable, ReleaseTable
do
    local pool = {}

    function newTable()
        local t = next(pool)
        if t then
            pool[t] = nil
            return t
        end
        return {}
    end

    function delTable(t)
        -- This next line is specifically tailored to your code; if you're reusing
        -- this function in some other code, change it to wipe(t) or something.
        t[1], t[2] = nil, nil
        pool[t] = true
    end
end

-- Add an event:
local t = newTable()
t[1] = time()
t[2] = amount
damageTable[#damageTable+1] = t

-- Loop over the events, add up relevant values, remove irrelevant ones:
local total, earliest = 0, time() - 5
for i = #damageTable, 1, -1 do
     local t = damageTable[i]
     if t[1] >= earliest then
          total = total + t[2]
     else
          -- Remove it from the event table and put it in the pool:
          delTable(tremove(damageTable, i))
     end
end
Originally Posted by Clamsoda View Post
I upsourced bit.band as bit_band (was throwing an error about having a "." when I tried up upsource as local bit.band = bit.band, but I found some post you made where it was bit_band ;])
Yep; variable names can only contain letters, numbers, and underscores. Replacing spaces and periods with underscores is generally the common practice, like using an underscore to represent a "junk" variable in a list of returns.

Originally Posted by Clamsoda View Post
Here is what I have gotten my combat log parsing function to.
I'd suggest registering CLEU only after PLAYER_LOGIN fires. I'm not sure when CLEU starts firing, but if it does fire before PLAYER_LOGIN it's just wasting CPU cycles since you don't know the player's GUID yet and the player isn't doing anything at that point either.
__________________
Author/maintainer of Grid, PhanxChat, ShieldsUp, and many more.
Troubleshoot an addonTurn any code into an addonMore addon resources
Need help with your code? Post all of your actual code! Attach or paste your files.
Please don’t PM me about addon bugs or code questions. Post a comment or forum thread instead!
Phanx is offline   Reply With Quote
Unread 03-17-13, 06:41 PM   #13
Clamsoda
A Cobalt Mageweaver
Join Date: Nov 2011
Posts: 244
Good lord. Your responses are nearly overwhelming! I appreciate your help so so much.

I like the idea of saving the event times in miliseconds, and then just adding 1 milisecond to it. It won't affect accuracy and it has the least amount of additional logic.

I'll make sure to register CLEU when PLAYER_LOGIN fires!

On a side note, I factored in all the specifics of the death strike heal, and it predicted the exact amount that it healed for, so, that was a nice success.
Clamsoda is offline   Reply With Quote
Unread 03-18-13, 05:06 AM   #14
Clamsoda
A Cobalt Mageweaver
Join Date: Nov 2011
Posts: 244
In advance: sorry for the double post

I wanted to inquire as to whether or not there is already a library that I can pass an amount to, and it will calculate any healing modification relevant to said player (Vampiric Blood, Mortal Strike, Guardian Spirit etc.), and it will return the value once any spell has modified it.

I'd rather not re-invent the wheel here. I can, and already have accounted for Scent of Blood, so that isn't necessary. Also, I was curious if anyone is aware as to what the priority for healing modifications is.

Edit: Just thinking, perhaps there is some way to spoof UnitGetIncomingHeals(), and let WoW do the work for me? Or possibly the UNIT_HEAL_PREDICTION event?

Last edited by Clamsoda : 03-18-13 at 06:46 AM.
Clamsoda is offline   Reply With Quote
Unread 03-18-13, 06:11 PM   #15
Phanx
A Pyroguard Emberseer
 
Phanx's Avatar
AddOn Author - Click to view addons
Join Date: Mar 2006
Posts: 4,391
No, because UNIT_HEAL_PREDICTION and UnitGetIncomingHeals only tell you about heals that are already in progress -- eg. spells in the process of being cast, or active HoT effects that are ticking. The healing from Death Strike happens instantly when you use the ability. There's no cast time and no ongoing effect, and no way for the game to predict when you're about to use Death Strike.

I'm also not aware of any current library for calculating the healing someone will receive... LibResComm did this prior to Cataclysm when Blizzard added the incoming heals API, but at this point it's so out of date it would be more work to update than just to write your own code tailored to your own specific needs (tracking the player's own self-healing).

If you want a list of current debuffs that reduce/prevent healing received, check my addon GridStatusHealingReduced. I can also send you a spreadsheet with more detailed info about the debuffs it tracks, such as the actual amount of the reduction, if you want. I don't have a list of buffs that increase healing received, though.

On the other hand, I'm not really sure it's worthwhile to spend all this extra time tracking all these auras and doing all this math for such a simple addon.
__________________
Author/maintainer of Grid, PhanxChat, ShieldsUp, and many more.
Troubleshoot an addonTurn any code into an addonMore addon resources
Need help with your code? Post all of your actual code! Attach or paste your files.
Please don’t PM me about addon bugs or code questions. Post a comment or forum thread instead!
Phanx is offline   Reply With Quote
Unread 03-18-13, 07:41 PM   #16
Clamsoda
A Cobalt Mageweaver
Join Date: Nov 2011
Posts: 244
On the other hand, I'm not really sure it's worthwhile to spend all this extra time tracking all these auras and doing all this math for such a simple addon.
You may have a point there, and the AddOn's relevance only reflects that of Throne of Thunder, with our raid comp (for now that is). I was more so interested in a library to do the heavy hauling, rather than writing my own or something like it.

For now, Scent of Blood and Vampiric Blood (glyphed) are factored in. I'll come up with something for Hard Stare and Horridon's debuff, but other than those the AddOn works perfectly, and consistently predicts the amount of healing with very low occurrences of error, and those errors tend to be 1,000 or 2,000.

I ended up going with two indexed tables, by the way. The "milisecond + 1" approach only worked if there were two events at the same time, if there was a third event, it would over write the second event and so forth. I am pretty happy with it for now, it was an awesome learning process.

Thanks for all of the help, replies, and code ideas!
Clamsoda is offline   Reply With Quote
Reply

Go BackWoWInterface » Developer Discussions » Lua/XML Help » Thoughts on an AddOn using "COMBAT_LOG_EVENT_UNFILTERED"

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