Thoughts on an AddOn using "COMBAT_LOG_EVENT_UNFILTERED"

[Clamsoda]

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

[Torhal]

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:

                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:

                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.

[Clamsoda]

Exactly the kind of criticism I was looking for!

I'll make the change for sure. Thank you.

[Phanx]

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:

        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:

        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:

local function onUpdate(self, elapsed)
        -- stuff happening here
end
frame:SetScript("OnUpdate", onUpdate)

You can just define the function anonymously in the SetScript call:

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.

                        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

[Clamsoda]

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!

[bsmorgan]

You might also look at the addon Blood Shield Tracker (http://www.curse.com/addons/wow/blood-shield-tracker).

[Clamsoda]

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:

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.

[Phanx]

Originally Posted by Clamsoda 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:

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.

[Clamsoda]

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

[Phanx]

You can also use the vararg (...) only for the arguments that change based on the event type:

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.

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

[Clamsoda]

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 ;])

-- OnEvent function to handle all of the registered events firing
    addonFrame:SetScript("OnEvent", function(self, event, _, eventType, _, sourceGUID, _, sourceFlags, _, destGUID, _, _, _, ...)
        if event == "PLAYER_LOGIN" then
            -- Get and set the player GUID for later use
            playerGUID = UnitGUID("player")
            -- Unregister PLAYER_LOGIN as it is no longer needed
            addonFrame:UnregisterEvent("PLAYER_LOGIN")
        elseif event == "COMBAT_LOG_EVENT_UNFILTERED" then
            eventIndex = eventIndex + 1
            local currentTime = time()
            if (bit_band(sourceFlags, COMBATLOG_OBJECT_TYPE_NPC) > 0) and (destGUID == playerGUID) then
                if eventType == "SWING_DAMAGE" then
                    local amount = ...
                    damageTable[eventIndex] = {currentTime, amount}
                elseif eventType == "SPELL_DAMAGE" or "SPELL_PERIODIC_DAMAGE" or "RANGE_DAMAGE" then
                    local _, _, _, amount = ...
                    damageTable[eventIndex] = {currentTime, amount}
                end -- if eventType
            end -- if bit.band
        end -- if event
    end)

[Phanx]

Originally Posted by Clamsoda 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 ... 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:

-- 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:

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:

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

[Clamsoda]

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]

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?

[Phanx]

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.

[Clamsoda]

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!