Thread Tools Display Modes
07-21-13, 09:33 PM   #1
Niketa
A Wyrmkin Dreamwalker
 
Niketa's Avatar
AddOn Author - Click to view addons
Join Date: Jul 2013
Posts: 54
Checking if raid group is in combat with a specific boss...

Hi. I'm new to LUA and AddOn development, so this might be a pretty simple question but I'm having problems finding an efficient way to check if a raid group is in combat with a boss.

I currently have it so that when it registers that I enter combat it checks each group member's target (cycles through based on "raid1" "raid2" etc.) and gets its NPC ID then compares that to values in a table.

This sometimes work but it's very inefficient and unreliable for several reasons, but mainly being that if people don't have the boss targeted right away when their turn comes it won't register it and thus if no one does it may miss a correct ID.

I tried looking over DBM's codes but since I've only started learning this this past week, it's way too advanced for me to get an idea of what they did.


Any help is greatly appreciated!
  Reply With Quote
07-22-13, 12:11 AM   #2
semlar
A Pyroguard Emberseer
 
semlar's Avatar
AddOn Author - Click to view addons
Join Date: Sep 2007
Posts: 1,060
This is not a simple question, and the answer depends on what bosses you're trying to track.

Typically, a fight against a raid boss from newer expansions (I think starting with wrath?) create a unit frame for the boss, and therefor have corresponding events which make it relatively easy to detect when you're in combat with them.

If the boss in question does not spawn a unit frame when engaged it becomes more complicated, as some fights don't even involve any sort of combat between you and the boss itself.

DBM and BigWigs treat each boss independently and have modules that describe how each fight is detected.

Most bosses (or other mobs) can be detected through general combat log events though, so unless the fight you're trying to track is really unusual like Valithria in Icecrown where the objective is to actually heal the boss then it probably won't be too hard to figure out.

Odds are you'll be tracking it through either the boss frame event (INSTANCE_ENCOUNTER_ENGAGE_UNIT), the combat log, or some emote that gets sent to the chat when the fight starts.
  Reply With Quote
07-22-13, 01:39 AM   #3
Malsomnus
A Cobalt Mageweaver
AddOn Author - Click to view addons
Join Date: Apr 2013
Posts: 203
It really depends a lot on what you're actually trying to do.

In my add-on, for example, I have a list of unit names that correspond to boss encounters, and as soon as one of them appears in the log I know this encounter is in progress. For example, Sul the Sand Crawler is from the (Elder Council, was it?) encounter, and Lu'lin is from the Twin Consorts encounter. This way I don't depend on any frames or events that may or may not work on specific encounters, but on the other hand I have to do extra work for specific bosses.
__________________
SanityCheck - If you've ever said the words "Sorry, I forgot" then you need this add-on.

Remember, every time you post a comment on an add-on, a kitten gets its wings!
  Reply With Quote
07-22-13, 02:49 AM   #4
Phanx
Cat.
 
Phanx's Avatar
AddOn Author - Click to view addons
Join Date: Mar 2006
Posts: 5,617
Originally Posted by Malsomnus View Post
In my add-on, for example, I have a list of unit names ...
Using NPC names is a really bad way to do it, as they are not locale-independent, and require hardcoded translations for every language. Instead, you should use mobIDs, and check them against the unit's GUID. To find a mobID, look it up on Wowhead and copy the number out of the URL. For example, the mobID for Horridon is 68476. And to get the mobID from a GUID inside your CLEU handler:

Code:
local mobID = tonumber(strsub(sourceGUID, 6, 10), 16)
If you're checking all CLEU "damage dealt" events, for example, you should also use a cache table to avoid repeatedly calling the same functions to get the same mobID from the same GUID:

Code:
-- outside CLEU handler:
local mobIDs = setmetatable({}, { __index = function(t, guid)
   local mobID = tonumber(strsub(guid, 6, 10), 16)
   t[guid] = mobID
   return mobID
end })

-- inside CLEU handler:
local mobID = mobIDs[sourceGUID]
The first time you see a particular GUID, it will extract the mobID and add it to the cache; after that, it will return the same mobID via simple table lookup.
__________________
Retired author of too many addons.
Message me if you're interested in taking over one of my addons.
Don’t message me about addon bugs or programming questions.
  Reply With Quote
07-22-13, 03:20 AM   #5
myrroddin
A Pyroguard Emberseer
 
myrroddin's Avatar
AddOn Author - Click to view addons
Join Date: Oct 2008
Posts: 1,237
If you are checking for a specific boss, or a list of bosses, could you use INSTANCE_ENCOUNTER_ENGAGE_UNIT? AFAIK, it only works during encounters where you would see one or more boss frames. Having checked both wowprogramming.com and wowpedia.org, it doesn't appear to pass any arguments, but the documentation might not be current.
  Reply With Quote
07-22-13, 04:29 AM   #6
semlar
A Pyroguard Emberseer
 
semlar's Avatar
AddOn Author - Click to view addons
Join Date: Sep 2007
Posts: 1,060
Originally Posted by myrroddin View Post
If you are checking for a specific boss, or a list of bosses, could you use INSTANCE_ENCOUNTER_ENGAGE_UNIT?
I'm not sure if you're suggesting this or asking how, but basically what you would do is on that event make a for loop from 1 to MAX_BOSS_FRAMES, check if UnitGUID('boss'..i) exists, and if it does extract the npc ID and compare it to the one in your list.

I don't think it's necessary to call UnitExists first since you need the GUID anyway and UnitGUID returns nil for an invalid unit ID.
  Reply With Quote
07-22-13, 05:45 AM   #7
Malsomnus
A Cobalt Mageweaver
AddOn Author - Click to view addons
Join Date: Apr 2013
Posts: 203
Originally Posted by Phanx View Post
Using NPC names is a really bad way to do it, as they are not locale-independent, and require hardcoded translations for every language.
I think I've seen the GUID method that you mention used in DBM, and while the advantages are clear, I felt that getting my version 1.00 up and running was a higher priority than making sure the add-on would work for users that may or may not exist. It's just a quick and dirty solution. Which is why I said it depends on what you're trying to do at the moment
__________________
SanityCheck - If you've ever said the words "Sorry, I forgot" then you need this add-on.

Remember, every time you post a comment on an add-on, a kitten gets its wings!
  Reply With Quote
07-22-13, 01:55 PM   #8
Phanx
Cat.
 
Phanx's Avatar
AddOn Author - Click to view addons
Join Date: Mar 2006
Posts: 5,617
Originally Posted by semlar View Post
I'm not sure if you're suggesting this or asking how, but basically what you would do is on that event make a for loop from 1 to MAX_BOSS_FRAMES, check if UnitGUID('boss'..i) exists, and if it does extract the npc ID and compare it to the one in your list.
Code:
local BossesToCheck = {
   [68476] = true, -- Horridon
   [69017] = true, -- Primordius
}

local MyAddon = CreateFrame("Frame")
MyAddon:RegisterEvent("INSTANCE_ENCOUNTER_ENGAGE_UNIT")
MyAddon:SetScript("OnEvent", function(self, event, ...)
   for i = 1, MAX_BOSS_FRAMES do
      local guid = UnitGUID("boss"..i)
      if guid then
         local mobid = tonumber(strsub(guid, 6, 10), 16)
         if BossesToCheck[mobid] then
            -- You are engaged with one of the bosses on your list.
            return
         end
      end
   end
   -- If you didn't return out by now, you're not engaged with any of the listed bosses.
end)
__________________
Retired author of too many addons.
Message me if you're interested in taking over one of my addons.
Don’t message me about addon bugs or programming questions.
  Reply With Quote
07-22-13, 04:23 PM   #9
Niketa
A Wyrmkin Dreamwalker
 
Niketa's Avatar
AddOn Author - Click to view addons
Join Date: Jul 2013
Posts: 54
Just an update what I came up with so far is this:


Code:
local timestamp, eventType, sourceGUID, sourceName, sourceFlags, destGUID, destName = ...
enemy = NPCID(sourceName)
	
if event == "COMBAT_LOG_EVENT_UNFILTERED" then
		for k,v in pairs(ml_bosses) do
			if v == enemy and methodtrigger ~= 1 then
				method, threshold = GetLootMethod(), GetLootThreshold()
				print("|cff00ff00MountLooter: Master Looter set! Will revert back to \""..method.."\" loot method and threshold "..threshold.." when boss is killed.")
				SetLootMethod("master",UnitName("player"))
				methodtrigger = 1 -- prevents spam
				break
			end
		end
	end
Just a couple notes on that... I've defined my variables outside of this as local already so no worries about what looks like names that would likely conflict with other addons and stuff. Also this is obviously inside my f:SetScript.

ml_bosses is the table where I have the NPCIDs of the bosses I want stored. The NPCID() function is just a function I made with the conversion from GUID to NPCID. methodtrigger prevents spam since it will set the value to 1 the first time a correct NPC is found in the combat log and I have the code if statement dependent of this being ~= 1.

Further on in the "LOOT_OPENED" event I'll set methodtrigger = nil so it can rinse and repeat for other bosses.


I have only been able to test it so far on Onyxia and some mobs outside because I'm trying to finish up the rest of the coding before I check others. For now I just need a place to stay and test. So far it's working.

Thanks for the pointers and further tips are still appreciated, I will be checking back in (especially because I haven't been able to test everything yet, so I might still have problems!).


Also because I don't want to spam a bunch of posts in this forum, I do have another question.

I created two frames that I was using to handle my events. One I made as a sort of welcome frame that defines appropriate variables when entering the world and prints a message depending on whether or not the player is in an instance and raid leader. I set it so that if these criterion were met if would register events to the second frame to enable the functionality of the addon (so that it wouldn't register the events frame two was watching if, for example, you weren't even raid leader, etc).

I registered the COMBAT_LOG_EVENT_UNFILTERED to frame two and it works just fine. However when I try to register "LOOT_OPENED" to the same frame it doesn't work. The event fires but it won't execute the if statement I set. For debugging purposes I registered it to my welcome frame instead and it worked.

So, does COMBAT_LOG_EVENT_UNFILTERED have to have its own frame or is there another issue going on here?

Last edited by Niketa : 07-22-13 at 04:45 PM.
  Reply With Quote
07-22-13, 08:53 PM   #10
Phanx
Cat.
 
Phanx's Avatar
AddOn Author - Click to view addons
Join Date: Mar 2006
Posts: 5,617
You don't need a separate frame for each event, but you do need to handle your events properly. In the code you posted, you are defining CLEU-specific variables before checking that you're actually handling the CLEU event. Check the event first, and then assign the provided arguments to variables:

Code:
MyFrame:SetScript("OnEvent", function(self, event, ...)
   if event == "COMBAT_LOG_EVENT_UNFILTERED" then
      local timestamp, eventType, sourceGUID, sourceName, sourceFlags, destGUID, destName = ...
      -- do stuff here

   elseif event == "LOOT_OPENED" then
      local autoLoot = ...
      -- do stuff here
   end
end)
Also, that CLEU handling code itself is pretty horrifyingly inefficient. If you just want to detect when you enter a boss fight, use INSTANCE_ENCOUNTER_ENGAGE_UNIT, not COMBAT_LOG_EVENT_UNFILTERED. At the very least, implement some kind of sub-event filtering and/or basic throttling so you're not running that pairs loop 1000000 times per second in raid combat.

Finally, there is no reason to define your event-handler-specific variables as locals in any scope other than the scope of your event handler function. Always limit your variables to the narrowest scope necessary. Defining, for instance, local enemy outside of your CLEU handler just makes your code look badly written (eg. leaked globals) to anyone else who glances at it, and makes it harder for anyone (yourself included) to determine the variable's scope, but doesn't actually confer any performance benefits.
__________________
Retired author of too many addons.
Message me if you're interested in taking over one of my addons.
Don’t message me about addon bugs or programming questions.
  Reply With Quote
07-22-13, 09:55 PM   #11
Niketa
A Wyrmkin Dreamwalker
 
Niketa's Avatar
AddOn Author - Click to view addons
Join Date: Jul 2013
Posts: 54
@Phanx

The reason why I didn't use INSTANCE_ENCOUNTER_ENGAGE_UNIT is because from what I understand it only works on more recent bosses that have the boss frames. For the purpose of the addon I'm working on, I will need to be able to check bosses from older raids as well.


Also as for the local variables being defined out of the handler functions, I've only defined those outside when I need to use that variable in a different handler. So I set it as a local variable with a nil value and then when an event fires the value is changed appropriately. I'm then able to use that same variable in other events without defining it as global, which I've been trying to avoid as much as possible so I don't have any conflicts with other addons using the same variables and without having to use ridiculously long variable names. What would be a more efficient method of passing variables around?


With the CLEU variables defined in the wrong space, that just came from a lot of confusion. All of my programming work is self taught through trial and error and as much google/forum research as I can. Definitely still at a beginner phase. At the time of writing that I didn't quite understand the arguments and I still only have a hazy idea of how they work (but I understand how I messed up because these values would be different for each CLEU).
  Reply With Quote
07-23-13, 07:50 AM   #12
myrroddin
A Pyroguard Emberseer
 
myrroddin's Avatar
AddOn Author - Click to view addons
Join Date: Oct 2008
Posts: 1,237
@Semlar: I was making the suggestion to use the event.

@Phanx: And that is how I would have used the suggestion.

@Niketa: Well, now I see my suggestion does not help. Darn older content!
  Reply With Quote
07-23-13, 11:09 AM   #13
semlar
A Pyroguard Emberseer
 
semlar's Avatar
AddOn Author - Click to view addons
Join Date: Sep 2007
Posts: 1,060
Originally Posted by myrroddin View Post
@Semlar: I was making the suggestion to use the event.
Oh, I was confused because I had already brought it up in my first post.

@Niketa

If you're trying to automatically set the master looter when you get into combat with specific bosses then I would make a table like Phanx had with the npc IDs as the keys so you don't need to loop over the entire table just to see if the boss is in there.

If you're going to use the combat log, make sure to only monitor events that actually put you in combat with the unit, for example SPELL_AURA_APPLIED fires for a lot of spells that don't (like hunter's mark) and should probably be ignored.

You also want to make sure you don't accidentally set the master looter again after the boss has died due to residual effects that could still be going off like void zones or dots.
  Reply With Quote
07-23-13, 11:21 AM   #14
Niketa
A Wyrmkin Dreamwalker
 
Niketa's Avatar
AddOn Author - Click to view addons
Join Date: Jul 2013
Posts: 54
I was thinking to reduce some of the variables could I register the LOOT_OPENED event to a new frame if the CLEU finds a match like I do to enable the functionality for only raid leaders anyway? Or is having too many different event handler frames bad practice?

Sorry if that's confusing, on my phone and it's hard to type. I can explain what I mean more later when I can type if necessary.



Semlar I have my CLEU if statement dependent on UnitAffectingCombat("player") too. I had something else to say but... I should wait til I'm home. Lol

Last edited by Niketa : 07-23-13 at 11:27 AM.
  Reply With Quote
07-23-13, 02:52 PM   #15
Phanx
Cat.
 
Phanx's Avatar
AddOn Author - Click to view addons
Join Date: Mar 2006
Posts: 5,617
Originally Posted by Niketa View Post
I was thinking to reduce some of the variables could I register the LOOT_OPENED event to a new frame if the CLEU finds a match like I do to enable the functionality for only raid leaders anyway? Or is having too many different event handler frames bad practice?
There's no reason to create multiple frames to handle events. It's trivial to handle multiple events on the same frame. If you can explain better why you think this would be needed or useful, or why you think your current code can't handle this, then we can be more specific, but in general, it's not needed or useful.

Originally Posted by Niketa View Post
I have my CLEU if statement dependent on UnitAffectingCombat("player") too.
You should really just post your entire, actual code. Posting only random snippets out of context just leads to confusion, frustration, and wasted time for everyone.

That said, checking UnitAffectingCombat every time CLEU fires is really wasteful. If you only want to respond to CLEU events while you're in combat, then you should only register for the CLEU event while you're in combat. Instead of registering for CLEU right away, register for PLAYER_REGEN_DISABLED and PLAYER_REGEN_ENABLED instead. When PLD fires, that means you entered combat, so register CLEU. When PRE fires, that means you left combat, so unregister CLEU. This way, you frame will simply not receive CLEU events when you don't want them.
__________________
Retired author of too many addons.
Message me if you're interested in taking over one of my addons.
Don’t message me about addon bugs or programming questions.
  Reply With Quote
07-23-13, 07:04 PM   #16
Niketa
A Wyrmkin Dreamwalker
 
Niketa's Avatar
AddOn Author - Click to view addons
Join Date: Jul 2013
Posts: 54
The code I currently have is all over the place and will give you a headache. It's currently functional in my test environment (have not been able to test all bosses yet since I don't lead my raids until the weekend) but I'm starting over from scratch to try do it right.

This is what I have so far. Obviously it's not done, but this is the new direction I'm heading based on the feedback you guys have given.

Lua Code:
  1. local bossNPCID = {
  2.     [55294] = true, -- Ultraxion
  3.    
  4.     -- Madness of Deathwing... check combat log next time to see if there's any NPC that shows up regardless of what platform we start at.--
  5.     [56846] = true, -- Madness of Deathwing: Arm Tentacle
  6.     [56167] = true, -- Madness of Deathwing: Arm Tentacle
  7.     [56168] = true, -- Madness of Deathwing: Wing Tentacle (Need to check if there's a second NPCID during raid, only one listed on WoWhead.)
  8.     [56173] = true, -- Madness of Deathwing: Deathwing
  9.     ----------------------------------------------------------------------------------------------------------------------------------------
  10.    
  11.     [52530] = true, -- Alysrazor
  12.     [52409] = true, -- Ragnaros
  13.     [46753] = true, -- Al'akir
  14.     [33136] = true, -- Yogg-Saron: Guardian of Yogg-Saron
  15.     [60410] = true, -- Elegon
  16.     [36597] = true, -- The Lich King
  17.     [10184] = true, -- Onyxia
  18.     [16152] = true, -- Attumen the Huntsman
  19.     [19622] = true, -- Kael'thas Sunstrider
  20.     [28859] = true, -- Malygos
  21.     [28860] = true, -- Sartharion
  22.     [69712] = true, -- Ji-Kun
  23.    
  24.     --TEST NPCIDS
  25.     [12129] = true,
  26.     --[[
  27.     [] = true,
  28.     [] = true,
  29.     [] = true,
  30.     [] = true,
  31.     [] = true,
  32.     [] = true,
  33.     [] = true,
  34.     [] = true,
  35.     ]]
  36. }
  37.  
  38. local events = CreateFrame("Frame")
  39.  
  40. events:RegisterEvent("PLAYER_ENTERING_WORLD")
  41. events:RegisterEvent("PARTY_LEADER_CHANGED")
  42.  
  43. events:SetScript("OnEvent",function(self,event,...)
  44.     if event == "PLAYER_ENTERING_WORLD" or event == "PARTY_LEADER_CHANGED" then
  45.         local ininstance, instancetype = IsInInstance("player") -- check if player is in a raid and in an instance
  46.         local isgroupleader = UnitIsGroupLeader("player") -- check if player is raid leader
  47.        
  48.         if ininstance == 1 and instancetype == "raid" and isgroupleader then -- if player is group leader in a raid and currently in an instance enable functionality
  49.             print("|cff00ff00Niketa's Mount Looter enabled!")
  50.             events:RegisterEvent("PLAYER_REGEN_DISABLED")
  51.             events:RegisterEvent("PARTY_LOOT_METHOD_CHANGED") --!!!!!!!!!!!!!!!!!! DON'T FORGET TO DISABLE THIS WHEN REVERTING LOOT SETTINGS!!!!!!!!!!!!!!
  52.         elseif ininstance == 1 and instancetype == "raid" and not isgroupleader then -- if player is in a raid and in instance but not group leader, "error" message
  53.             print("|cff00ff00Niketa's Mount Looter disabled: You need to be raid leader to use this addon.")
  54.         end
  55.     end
  56.    
  57.     if event == "PLAYER_REGEN_DISABLED" then -- on entering combat, start looking at CLEU
  58.         events:RegisterEvent("COMBAT_LOG_EVENT_UNFILTERED")
  59.        
  60.     end
  61.    
  62.     if event == "COMBAT_LOG_EVENT_UNFILTERED" and eventType ~= "SPELL_AURA_APPLIED" then
  63.         local _, _, _, sourceGUID, sourceName, _, _, destGUID, destName, _, _ = ...
  64.         local enemyNPCIDa = tonumber(strsub(sourceGUID, 6, 10), 16)
  65.         local enemyNPCIDb = tonumber(strsub(destGUID, 6, 10), 16)
  66.        
  67.         if bossNPCID[enemyNPCIDa] or bossNPCID[enemyNPCIDb] then -- if enemy is in table then set loot and unregister CLEU
  68.             events:UnregisterEvent("COMBAT_LOG_EVENT_UNFILTERED")
  69.            
  70.             nmlmethod, nmlthreshold = GetLootMethod(), GetLootThreshold()
  71.            
  72.             if nmlmethod == "master" and nmlthreshold ~= 4 then -- only set threshold if already master
  73.                 print("no thresh")
  74.                
  75.                 SetLootThreshold(4)
  76.             elseif nmlmethod ~= "master" and nmlthreshold == 4 then -- only set method if already epic
  77.                 print("no method")
  78.                
  79.                 SetLootMethod("master",UnitName("player"))
  80.             elseif nmlmethod ~= "master" and nmlthreshold ~= 4 then -- set both
  81.                 print("no thresh or method")
  82.                
  83.                 nmlnodead = 1 -- set nil when reverting settings
  84.                 nmlsetthreshold = 1 -- trigger PARTY_LOOT_METHOD_CHANGED to set to epic
  85.                
  86.                 SetLootMethod("master",UnitName("player"))
  87.          -- else do nothing
  88.             end
  89.         end
  90.     end
  91.    
  92.     if event == "PARTY_LOOT_METHOD_CHANGED" and nmlsetthreshold == 1 then
  93.         nmlsetthreshold = nil
  94.        
  95.         SetLootThreshold(4)
  96.     end
  97. end)


Is this a step in the right direction or am I still way off?

(* I just realized I forgot to add in your suggestion about caching the NPCID so I don't keep calling local enemyNPCID. I don't really understand your code for that [as far as metatables go]; would you mind explaining so I actually understand what I'm writing [we don't want another issue like my local variables for CLEU, lol]? *)

Last edited by Niketa : 07-23-13 at 08:12 PM.
  Reply With Quote
07-23-13, 11:13 PM   #17
semlar
A Pyroguard Emberseer
 
semlar's Avatar
AddOn Author - Click to view addons
Join Date: Sep 2007
Posts: 1,060
This is a considerable improvement.

One change you should make is use 'elseif' for your event checks so that it doesn't have to evaluate the other conditions after one succeeds.

The other thing is you didn't localize nmlmethod or nmlthreshold so they become global variables which you generally want to avoid on the off-chance you overwrite something.

You can also consolidate your conditions a little bit, for example..

Lua Code:
  1. if ininstance == 1 and instancetype == "raid" then
  2.     if isgroupleader then -- if player is group leader in a raid and currently in an instance enable functionality
  3.         print("|cff00ff00Niketa's Mount Looter enabled!")
  4.         events:RegisterEvent("PLAYER_REGEN_DISABLED")
  5.         events:RegisterEvent("PARTY_LOOT_METHOD_CHANGED") --!!!!!!!!!!!!!!!!!! DON'T FORGET TO DISABLE THIS WHEN REVERTING LOOT SETTINGS!!!!!!!!!!!!!!
  6.     else -- if player is in a raid and in instance but not group leader, "error" message
  7.         print("|cff00ff00Niketa's Mount Looter disabled: You need to be raid leader to use this addon.")
  8.     end
  9. end

Lua Code:
  1. if nmlmethod ~= "master" then
  2.     if nmlthreshold ~= 4 then
  3.         print("no thresh or method")
  4.         nmlnodead = 1 -- set nil when reverting settings
  5.         nmlsetthreshold = 1 -- trigger PARTY_LOOT_METHOD_CHANGED to set to epic
  6.     else
  7.         print("no method")
  8.     end
  9.     SetLootMethod("master", UnitName("player"))
  10. elseif nmlthreshold ~= 4 then -- only set threshold if already master
  11.     print("no thresh")
  12.     SetLootThreshold(4)
  13. end

Last edited by semlar : 07-23-13 at 11:22 PM.
  Reply With Quote
07-23-13, 11:21 PM   #18
Phanx
Cat.
 
Phanx's Avatar
AddOn Author - Click to view addons
Join Date: Mar 2006
Posts: 5,617
Originally Posted by Niketa View Post
Is this a step in the right direction or am I still way off?
That's looking a lot better. Just a couple suggestions:

1. Your event handler should be an if-else chain, not a bunch of separate if blocks. For example, right now, when PLAYER_ENTERING_WORLD fires, the first if block in your function runs -- but then all the other blocks have to be checked, too, even though it's obvious to you as a human that they'll never match that event. Alternatively, add a return inside each if block, so that if the block is run, it will exit the function afterwards instead of going through and checking all the other blocks.

Code:
if event == "PLAYER_ENTERING_WORLD" or event == "PARTY_LEADER_CHANGED" then
   -- do something
-- no "end" here
elseif event == "PLAYER_REGEN_DISABLED" then
   -- do something for entering combat
...
2. Instead of checking if event == X and something_else == Y then, you should just check the event first, and then the other condition. Otherwise, even if the event is X, your code is still running through all the other possibilities, when you actually want it to just stop executing the function if the event is X but the other condition doesn't match.

Don't:
Code:
if event == "COMBAT_LOG_EVENT_UNFILTERED" and eventType ~= "SPELL_AURA_APPLIED" then
   -- do something
end
Do:
Code:
if event == "COMBAT_LOG_EVENT_UNFILTERED" then
   if eventType == "SPELL_AURA_APPLIED" then
      -- don't want this, quit.
      return
   end
   -- do something
end
3. Once you start handling more than 2-3 events, or each event's handling code is more than 5-10 lines, you may want to consider splitting each event's code out into its own function. An easy way to do this is to make all the event handler functions methods on your event handler frame, with the same name as the event they handle, and then just call the right method for whichever event is firing:

Code:
events:RegisterEvent("PLAYER_ENTERING_WORLD")
events:RegisterEvent("PARTY_LEADER_CHANGED")

events:SetScript("OnEvent", function(self, event, ...)
   -- automatically call the method for this event, if it exists
   return self[event] and self[event](self, event, ...)
end)

function events:PLAYER_ENTERING_WORLD(event)
   -- this function gets called when PEW fires
end

function events:PARTY_LEADER_CHANGED(event)
   -- this function gets called when PLC fires
end
For cases where multiple events should run the same code, you don't need to duplicate the function; just create an alias:

Code:
function events:PLAYER_ENTERING_WORLD(event)
   -- this function gets called when PEW *or* PLC fires
end

events.PARTY_LEADER_CHANGED = events.PLAYER_ENTERING_WORLD

Originally Posted by Niketa View Post
(* I just realized I forgot to add in your suggestion about caching the NPCID so I don't keep calling local enemyNPCID. I don't really understand your code for that [as far as metatables go]; would you mind explaining so I actually understand what I'm writing [we don't want another issue like my local variables for CLEU, lol]? *)
Add this in the main chunk of your file, outside of any function, and before you set your OnEvent script:

Code:
local mobIDFromGUID = setmetatable({}, { __index = function(t, guid)
   local mobID = tonumber(strsub(guid, 6, 10), 16)
   t[guid] = mobID
   return mobID
end })
Replace these lines in your CLEU handling code:

Code:
        local enemyNPCIDa = tonumber(strsub(sourceGUID, 6, 10), 16)
        local enemyNPCIDb = tonumber(strsub(destGUID, 6, 10), 16)
With this:

Code:
        local enemyNPCIDa = mobIDFromGUID[sourceGUID]
        local enemyNPCIDb = mobIDFromGUID[destGUID]
How it works:

Code:
local mobIDFromGUID = setmetatable({}, { __index = function(t, guid)
   local mobID = tonumber(strsub(guid, 6, 10), 16)
   t[guid] = mobID
   return mobID
end })
At this point, this is equivalent to:

Code:
local mobIDFromGUID = {}
Then we attach a metatable, which is an additional table that can define special behaviors for the original table to use under certain circumstances:

Code:
setmetatable(mobIDFromGUID, {})
Right now, the metatable is empty, so the table doesn't have any special behaviors. If you try to look up a value that's not in the table, you'll just get nil.

But, we want the table to do something special when we look up a value that isn't actually contained in it, so we'll add an __index key to the metatable, whose value is the function to run when this happens:

Code:
setmetatable(mobIDFromGUID, { __index = function(t, k)
   print("GUID", k, "isn't in the table yet!")
end })
The first argument passed to the __index function is a reference to the mobIDFromGUID table itself. The second argument is the key that was looked up. Now let's make it do something actually useful:

Code:
setmetatable(mobIDFromGUID, { __index = function(t, guid)
   -- Extract the mob ID from the looked-up GUID:
   local mobID = tonumber(strsub(guid, 6, 10), 16)
   -- Store the guid->mobID pair in the table so it can be accessed faster
   -- the next time the same GUID gets looked up:
   t[guid] = mobID
   -- Finally, return the mobID to whatever did the lookup.
   -- As far as the lookup code (eg. mobIDFromGUID(sourceGUID)) is concerned,
   -- the only thing that happened is a regular table lookup.
   return mobID
end })
__________________
Retired author of too many addons.
Message me if you're interested in taking over one of my addons.
Don’t message me about addon bugs or programming questions.
  Reply With Quote
07-24-13, 12:20 AM   #19
Niketa
A Wyrmkin Dreamwalker
 
Niketa's Avatar
AddOn Author - Click to view addons
Join Date: Jul 2013
Posts: 54
Originally Posted by semlar View Post
The other thing is you didn't localize nmlmethod or nmlthreshold so they become global variables which you generally want to avoid on the off-chance you overwrite something.
I set nmlmethod and nmlthreshold as global so I could access them for different events (because I need to be able to retrieve these values on LOOT_OPENED to revert the settings). What would a better approach be?


Originally Posted by Phanx View Post
Code:
events:SetScript("OnEvent", function(self, event, ...)
   -- automatically call the method for this event, if it exists
   return self[event] and self[event](self, event, ...)
end)
I don't understand the code here that you've used to call the function. I'm assuming "self[event]" would refer to events and whatever the event is and "(self, event, ...)" is just what it is?

Originally Posted by Phanx View Post
Add this in the main chunk of your file, outside of any function, and before you set your OnEvent script:

Code:
local mobIDFromGUID = setmetatable({}, { __index = function(t, guid)
   local mobID = tonumber(strsub(guid, 6, 10), 16)
   t[guid] = mobID
   return mobID
end })
I'm not sure where the "t" comes from in the code. Is it just an alias for the table where "function(t, guid)" is essentially "function(self,parameter)" or something along those lines?


I'm still reworking the code with your suggestions so I won't post quite yet, but I will either before I go to bed or after work tomorrow.

Last edited by Niketa : 07-24-13 at 01:21 AM.
  Reply With Quote
07-24-13, 01:37 AM   #20
Phanx
Cat.
 
Phanx's Avatar
AddOn Author - Click to view addons
Join Date: Mar 2006
Posts: 5,617
Originally Posted by Niketa View Post
I don't understand the code here that you've used to call the function. I'm assuming "self[event]" would refer to events and whatever the event is and "(self, event, ...)" is just what it is?
Okay, let's break that down:

Code:
events:SetScript("OnEvent", function(self, event, ...)
end)
The above assigns a function to be run when an event your frame ("events") was registered for happens. Functions assigned as OnEvent scripts receive at least two arguments -- the first being a reference to the frame itself, the second being a string value containing the name of the event being handled -- plus any and all arguments specific to the event being handled.

Typically, the first two arguments are named "self" and "event" but variable names do not actually affect the variables, so you can could name them "panda" and "polkadot" if you really wanted, though I'd recommend you stick with the established conventions to keep your code readable, both for yourself and for anyone else who looks at it.

Code:
events:SetScript("OnEvent", function(self, event, ...)
    local method = self[event]
end)
Now we perform a table lookup on the frame ("self") to find the value assigned to the key with the same name as the event. So, if your frame is currently handling the "PLAYER_LOGIN" event, the "method" variable above would contain whatever value is found at self["PLAYER_LOGIN"]. Since self["PLAYER_LOGIN"] is just another way of writing self.PLAYER_LOGIN, if you correctly defined your function:

Code:
function events:PLAYER_LOGIN(event)
   print("I am now logged in!")
end
... then "method" refers to that function.

Code:
events:SetScript("OnEvent", function(self, event, ...)
    local method = self[event]
    if method then
        method(self, event, ...)
    end
end)
Now we check that "method" actually contains a value -- if we don't do this check, and it doesn't contain a value, then we'll get an error when we try to call it as a function. If you wanted to be really safe, you could check if type(method) == "function" then instead of just if method then, but realistically you will never be assigning any value to events.PLAYER_LOGIN other than the function you want to run for the PLAYER_LOGIN event, so we skip the extra function call and just assume it's a function.

Finally, if the function exists, we call it, passing along all the arguments received by the OnEvent script.

Code:
events:SetScript("OnEvent", function(self, event, ...)
    return self[event] and self[event](self, event, ...)
end)
This is just a shorter (and slightly more efficient) way to write the previous example, using ternary syntax ("x = a and b or c" vs "if a then x = b else x = c end") and taking advantage of tail calls. If you find the shorter syntax too confusing for now, just use the longer version from above.

Originally Posted by Niketa View Post
Also, I'm having issues getting that to work for me. Is that the code as it should be or are there things I need to change? Sorry, that might be a stupid question but I don't really understand what's going on well enough.
You need to remove your old SetScript("OnEvent", function ... end) line, but first, you need to copy your actual code into the event handlers. So, for example, for your first block (also, I took the liberty of reformatting your code to clean up some redundancy and avoid some unnecessary function calls):

Code:
function events:PLAYER_ENTERING_WORLD(event)
	local isInInstance, instanceType = IsisInInstance("player")
	if not isInInstance or instanceType ~= "raid" then
		-- Quit:
		return
	end

	if not UnitIsGroupLeader("player") then
		-- Stop listening to events:
		self:UnregisterEvent("PLAYER_REGEN_DISABLED")
		self:UnregisterEvent("PLAYER_REGEN_ENABLED")
		self:UnregisterEvent("PARTY_LOOT_METHOD_CHANGED")

		-- Tell the player:
		print("|cff00ff00Niketa's Mount Looter disabled: You need to be raid leader to use this addon.")

		-- Quit:
		return
	end

	-- If you've gotten to this point, you are in a raid instance,
	-- and you are the group leader.

	-- Start listening to events:
	self:RegisterEvent("PLAYER_REGEN_DISABLED")
	self:RegisterEvent("PLAYER_REGEN_ENABLED")
	self:RegisterEvent("PARTY_LOOT_METHOD_CHANGED")

	-- Tell the player:
	print("|cff00ff00Niketa's Mount Looter enabled!")
end

events.PARTY_LEADER_CHANGED = events.PLAYER_ENTERING_WORLD
Do the same for the code you want to run for other events, putting each block of code into an appropriately named function.

Originally Posted by Niketa View Post
I'm not sure where the "t" comes from in the code. Is it just an alias for the table where "function(t, guid)" is essentially "function(self,parameter)" or something along those lines?
Yes. As explained above, you can name variables whatever you want. Since most parts of your code will be using "self" to refer to your addon object (in your case, your frame, so "self" and "events" will usually mean the same thing) I used a different name here ("t" for "table", but you can't use "table" because that's already a Lua global) to avoid confusion.

The only time you are required to use a specific variable name is when you are using method notation, in which case the variable "self" is automatically created and assigned.

This is method notation:

Code:
function MyAddon:DoSomething()
    print(type(self))
end
If you call this function -- eg. by typing "/run MyAddon:DoSomething()" -- you will see "table" printed, even though you did not actually define the variable "self" anywhere. Using a colon is a shortcut for this:

Code:
function MyAddon.DoSomething(self)
    print(type(self))
end
No matter which way you define your function (method notation with a colon, or regular notation with a period) you can call it via either notation, even if it's not the same notation used when you defined your function.

Code:
MyAddon:DoSomething()
and

Code:
MyAddon.DoSomething(MyAddon)
are functionally identical. Personally, I always use method notation (colon) since it's less code to type, but whichever you choose to use, use it consistently to keep your code readable.

This is also why inside your OnEvent function, you can just refer to "self" instead of "events", though both variables mean the same thing in that particular scope.
__________________
Retired author of too many addons.
Message me if you're interested in taking over one of my addons.
Don’t message me about addon bugs or programming questions.

Last edited by Phanx : 07-24-13 at 01:44 AM.
  Reply With Quote

WoWInterface » Developer Discussions » Lua/XML Help » Checking if raid group is in combat with a specific boss...

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