Best way to determine who killed me

[gmarco]

Hi all,
I'd like to improve my ganklist addon to offer the possibility to show a frame with the name of the char that killed you to easily add to the db.

So I begin to study a little bit the combat log.

The best way I found to determine who killed me is something like this pseudo sample code:

--- other code above 
local playerName = UnitName("player");
local type = ""
local typeMask = 0x00F
 
local function eventHandler_cleu(self, event, ...)
    local timeStamp, event, hideCaster, sourceGUID, sourceName, sourceFlags, sourceRaidFlags, destGUID, destName, destFlags, destRaidFlags, prefixParam1, prefixParam2, dummyparam, suffixParam1, suffixParam2 = ...
        
        -- bit.band(tonumber(sourceGUID:sub(1,5)),typeMask) = 0 Player, 3-4 NPC
 
        if destName == playerName and bit.band(tonumber(sourceGUID:sub(1,5)),typeMask) == 0 and (event == "SPELL_DAMAGE" or event == "SPELL_PERIODIC_DAMAGE" or event == "RANGE_DAMAGE") and suffixParam2 > 0  then
            print(type .. " ["..sourceName.."] killed ["..destName.."] with "..suffixParam1.." damage of ".. GetSpellLink(prefixParam1) .. " overkill " .. suffixParam2)    
        elseif destName == playerName and bit.band(tonumber(sourceGUID:sub(1,5)),typeMask) == 0 and event == "SWING_DAMAGE" and prefixParam2 > 0 then
            print(type .." ["..sourceName.."] killed ["..destName.."] with "..prefixParam1.." Melee, overkill " .. suffixParam2 )
        end         
        
        -- a sample line when an enemy player killed me
        -- [name] killed [me] with 65762 damage of [Combustione dell'Ombra] overkill 48472
    end
 
 
local frame_cleu = CreateFrame("FRAME", "remgankframe_cleu")
frame_cleu:RegisterEvent("COMBAT_LOG_EVENT_UNFILTERED")
frame_cleu:SetScript("OnEvent", eventHandler_cleu) 
 
--- other code not show...

Some questions now:

1) As noted in another thread the UNIT_DIED subevent not contain all the information about the src of the damage so I think I can't use it. So is this the best way ( check the overkill ) ?

2) May I use the CLE not CLEU ? I have not understand so much the differences but if in the first there are only events related to me probably is more efficent than the second and for what I need is the same.

3) I am a little bit scared about the so many conditions to parsing of the die event ... if it is my name related (destName), if I am not in a bg/arena, if the srcName is a Player, if this player is an Enemy (this could be omitted I think), if the there is an overkill so I am died by spell or it was a melee ... etc :-) . But I think I can't summarize them all :-)

Thanks all for attention.

[Phanx]

You could use COMBAT_LOG_EVENT instead of COMBAT_LOG_EVENT_UNFILTERED, but only if your combat log is currently set to display the events you need. This means that if you switch to a combat log filter that only shows your actions, for example, your addon would stop working. You also can't guarantee that other people will have their filters set correctly. For these reasons (mainly the second) you should always use CLEU over CLE for addons you plan to release to the public.

Also, there's no need to use string.sub and tonumber on the GUID; just use bit.band on the whole GUID and compare it to the COMBATLOG_OBJECT_CONTROL_PLAYER global (0x00000100). This will save you 2 function calls and one string creation.

Here's a drycoded example with a few other minor changes:

-- For general use, I wouldn't recommend obsessively upvaluing every
-- global but it is worth doing for situations like CLEU or OnUpdate
-- where you are accessing them extremely often.
local bit_band = bit.band
local COMBATLOG_OBJECT_CONTROL_PLAYER = COMBATLOG_OBJECT_CONTROL_PLAYER

-- GUIDs are not available when the main chunk executes, so you have to
-- wait and fill this in once the combat log starts running.
local playerGUID

local frame_cleu = CreateFrame("FRAME", "remgankframe_cleu")
frame_cleu:RegisterEvent("COMBAT_LOG_EVENT_UNFILTERED")

-- There's no need to define your event handler function separately and
-- then set it as the OnEvent script. You can just define the function
-- right in the SetScript call.
frame_cleu:SetScript("OnEvent", function(self, event, timeStamp, event, hideCaster, sourceGUID, sourceName, sourceFlags, sourceRaidFlags, destGUID, destName, destFlags, destRaidFlags, prefixParam1, prefixParam2, dummyparam, suffixParam1, suffixParam2)
	-- Fill in the playerGUID variable here if needed.
	if not playerGUID then
		playerGUID = UnitGUID("player")
	end

	-- Since these first two conditions apply no matter which event is
	-- firing, check them first.
	if destGUID == playerGUID and bit_band(sourceGUID, COMBATLOG_OBJECT_CONTROL_PLAYER) == COMBATLOG_OBJECT_CONTROL_PLAYER then
		if event == "SWING_DAMAGE" then
			-- Nest checks inside each other instead of using and, so
			-- that if any check matches (eg. it is this event, but not
			-- the right prefix) the code doesn't keep checking other
			-- things that obviously can never match.
			if prefixParam2 > 0 then
				print(string_format("[%s] killed [%s] with %s Melee overkill %s", sourceName, destName, prefixParam1, suffixParam2))
			end
		elseif event == "SPELL_DAMAGE" or event == "SPELL_PERIODIC_DAMAGE" or event == "RANGE_DAMAGE" then
			if suffixParam2 > 0 then
				print(string_format("[%s] killed [%s] with %s damage of %s overkill %s", sourceName, destName, suffixParam1, GetSpellLink(prefixParam1), suffixParam2))
			end
		end
	end

	-- a sample line when an enemy player killed me
	-- [name] killed [me] with 65762 damage of [Combustione dell'Ombra] overkill 48472
end)

I also don't know what you were using the type variable for (since it was always set to an empty string) and it's generally a bad idea to use local variables with the same names as global functions or variables (the type function tells you whether a value is a string, number, table, etc.) so I just deleted it.

[gmarco]

Originally Posted by Phanx Also, there's no need to use string.sub and tonumber on the GUID; just use bit.band on the whole GUID and compare it to the COMBATLOG_OBJECT_CONTROL_PLAYER global (0x00000100). This will save you 2 function calls and one string creation.

Yes, every day I'll learn something new :-)
Probably I also seen these globals during googling/wowiki/wowpedia and the book but I was unable how to use them with proficency.

I also don't know what you were using the type variable for (since it was always set to an empty string) and it's generally a bad idea to use local variables with the same names as global functions or variables (the type function tells you whether a value is a string, number, table, etc.) so I just deleted it.

this was a "read and rewrite" from a book :-) and it was used to define type = NPC, Player etc etc ... I didn't think to the global impact of the thing and used the same variable name I found. Now I'll take more care in the future for the variable name.

A thing which I didn't understand so well in your code is the defining as local of:

-- For general use, I wouldn't recommend obsessively upvaluing every
-- global but it is worth doing for situations like CLEU or OnUpdate
-- where you are accessing them extremely often.
local bit_band = bit.band
local COMBATLOG_OBJECT_CONTROL_PLAYER = COMBATLOG_OBJECT_CONTROL_PLAYER

1) Defining a lua function (bit.band) as local is better than use it as is ?
2) Defining local to this addon the global constant for the controller type. Is it better and faster than using as is ?


Last question:

frame_cleu:SetScript("OnEvent", function(self, event, 
 
--- etc etc

could be evaluated depending of a condition or a variable name ?

Something like (I am writing on the fly so be kind about my errors in the replies :-) :

if not UnitInBattleground("player") or not IsActiveBattlefieldArena() then 
        
         frame_cleu:SetScript("OnEvent", function(self, event, 
 
         --- etc etc 
 
end

Thanks again for everything.

[Phanx]

Originally Posted by gmarco 1) Defining a lua function (bit.band) as local is better than use it as is ?

local bit_band = bit.band

... simply creates a local shortcut named "bit_band" that points to the global "bit.band" function. It doesn't replace or change the function in any way. It just makes it faster to access it.

Imagine the global namespace as a book, and yourself as a reader with absolutely no short-term memory. Every time you ask for "bit.band" you have to go through the whole book to find it. Creating a local shortcut -- this is called an "upvalue" -- is like putting a sticky note on the page, so instead of having to look through the thousands of pages in the book (at the moment there are 49,080 objects in my global namespace) you only have to look at the few sticky notes sticking out of the side. Much faster.

Plus, "bit.band" is actually two lookups -- one global lookup to find "bit", and then a table lookup inside "bit" to find "band". So creating the "bit_band" upvalue is even faster in this case.

Originally Posted by gmarco 2) Defining local to this addon the global constant for the controller type. Is it better and faster than using as is ?

As far as the game is concerned, looking up a function in the global namespace is exactly the same as looking up a string, table, or other kind of value. It doesn't know what kind of value it is until it's found it. Going back to the book analogy, you know you're looking for a page entitled "COMBATLOG_OBJECT_CONTROL_PLAYER" but until you find the page, you have no idea what information is on that page, or even what kind of information is on that page.

Now, despite all of this, I wouldn't suggest creating local upvalues for every global thing you access in your addon. Only do it for things you access very frequently, like in an OnUpdate script, or in response to events like CLEU that fire very often. For infrequently accessed globals, the speed doesn't matter, so creating upvalues just adds clutter to your code. Also, upvalues will always point to the original value, so if someone comes along and overwrites the global "bit.band" with a new function, your upvalue will still point to the original function, not the new one. Obviously nobody is (or should be) overwriting "bit.band" but this is another argument against upvaluing everything all the time. Also, there is a limit on the number of locals that can be in scope at once; I don't remember what it is, and you probably won't ever hit it in normal use, but you might if you go wild with upvalues.

Originally Posted by gmarco if not UnitInBattleground("player") or not IsActiveBattlefieldArena() then frame_cleu:SetScript("OnEvent", function(self, event, --- etc etc end

That is valid Lua code, but it won't work the way you're imagining. That code will cause your frame to only respond to events if you are outside of a battleground or arena when the addon first loads, and it won't detect when you enter or leave one and change the script.

If you want to ignore events that occur inside a battleground or arena, you should register for the event that fires when you change zones, and register/unregister CLEU as needed depending on what kind of zone you're in:

-- Don't register CLEU here!
frame_cleu:RegisterEvent("ZONE_CHANGED_NEW_AREA")

frame_cleu:SetScript("OnEvent", function(self, event, ...)
     if event == "ZONE_CHANGED_NEW_AREA" then
          local _, instanceType = IsInInstance()
          if instanceType == "arena" or instanceType == "pvp" or (instanceType == "none" and GetZonePVPInfo() == "combat") then
               -- arena is, obviously, an arena.
               -- pvp is a battleground.
               -- none with GetZonePVPInfo() == "combat" is an outdoor PvP zone like Wintergrasp.
               self:UnregisterEvent("COMBAT_LOG_EVENT_UNFILTERED")
          else
               -- If not in a pvp zone, register CLEU:
               self:RegisterEvent("COMBAT_LOG_EVENT_UNFILTERED")
          end
          return
     end

     local timeStamp, event, hideCaster, sourceGUID, sourceName, sourceFlags, sourceRaidFlags, destGUID, destName, destFlags, destRaidFlags, prefixParam1, prefixParam2, dummyparam, suffixParam1, suffixParam2 = ...

     -- proceed with CLEU handling
end)

[gmarco]

Originally Posted by Phanx local bit_band = bit.band ... simply creates a local shortcut named "bit_band" that points to the global "bit.band" function. It doesn't replace or change the function in any way. It just makes it faster to access it. Imagine the global namespace as a book, and yourself as a reader with absolutely no short-term memory. Every time you ask for "bit.band" you have to go through the whole book to find it. Creating a local shortcut -- this is called an "upvalue" -- is like putting a sticky note on the page, so instead of having to look through the thousands of pages in the book (at the moment there are 49,080 objects in my global namespace) you only have to look at the few sticky notes sticking out of the side. Much faster. Plus, "bit.band" is actually two lookups -- one global lookup to find "bit", and then a table lookup inside "bit" to find "band". So creating the "bit_band" upvalue is even faster in this case. As far as the game is concerned, looking up a function in the global namespace is exactly the same as looking up a string, table, or other kind of value. It doesn't know what kind of value it is until it's found it. Going back to the book analogy, you know you're looking for a page entitled "COMBATLOG_OBJECT_CONTROL_PLAYER" but until you find the page, you have no idea what information is on that page, or even what kind of information is on that page. Now, despite all of this, I wouldn't suggest creating local upvalues for every global thing you access in your addon. Only do it for things you access very frequently, like in an OnUpdate script, or in response to events like CLEU that fire very often. For infrequently accessed globals, the speed doesn't matter, so creating upvalues just adds clutter to your code. Also, upvalues will always point to the original value, so if someone comes along and overwrites the global "bit.band" with a new function, your upvalue will still point to the original function, not the new one. Obviously nobody is (or should be) overwriting "bit.band" but this is another argument against upvaluing everything all the time. Also, there is a limit on the number of locals that can be in scope at once; I don't remember what it is, and you probably won't ever hit it in normal use, but you might if you go wild with upvalues. That is valid Lua code, but it won't work the way you're imagining. That code will cause your frame to only respond to events if you are outside of a battleground or arena when the addon first loads, and it won't detect when you enter or leave one and change the script. If you want to ignore events that occur inside a battleground or arena, you should register for the event that fires when you change zones, and register/unregister CLEU as needed depending on what kind of zone you're in: -- Don't register CLEU here! frame_cleu:RegisterEvent("ZONE_CHANGED_NEW_AREA") frame_cleu:SetScript("OnEvent", function(self, event, ...) if event == "ZONE_CHANGED_NEW_AREA" then local _, instanceType = IsInInstance() if instanceType == "arena" or instanceType == "pvp" or (instanceType == "none" and GetZonePVPInfo() == "combat") then -- arena is, obviously, an arena. -- pvp is a battleground. -- none with GetZonePVPInfo() == "combat" is an outdoor PvP zone like Wintergrasp. self:UnregisterEvent("COMBAT_LOG_EVENT_UNFILTERED") else -- If not in a pvp zone, register CLEU: self:RegisterEvent("COMBAT_LOG_EVENT_UNFILTERED") end return end local timeStamp, event, hideCaster, sourceGUID, sourceName, sourceFlags, sourceRaidFlags, destGUID, destName, destFlags, destRaidFlags, prefixParam1, prefixParam2, dummyparam, suffixParam1, suffixParam2 = ... -- proceed with CLEU handling end)

Hi Phanx,
thanks so much for your kind reply.

I always don't like to quote everything in a reply but I think your reply is a masterpice and should be sticky somewhere in the LUA forum.

Finally I begin to understand how things move inside addons and the game itself.

Thanks again.

[Phanx]

There is some more discussion on the topic here:

http://www.wowinterface.com/forums/s...ad.php?t=43658

(More book analogies for you. )