Issue with COMBAT_LOG_EVENT sub-event.

[gamerfreak]

Hey there. First time addon programmer here, and I'm having an issue pulling the sub-event argument from COMBAT_LOG_EVENT_UNFILTERED.

Here is my code:

function events:COMBAT_LOG_EVENT_UNFILTERED(self, event, ...)
    local eventType = select(2,...);
    local destGUID = select(7,...);
    print (eventType); -- for debugging
    if (eventType == "UNIT_DIED") then
        print("UNIT_DIED");
    end
end


-----------------
--EVENT HANDLER--
-----------------
frame:SetScript("OnEvent", function(self, event, ...)
events[event](self, event, ...);
end);
for k, v in pairs(events) do
    frame:RegisterEvent(k);
end

eventType is coming back with a true/false value, not the sub-event string.

Any ideas what I'm doing wrong? Thanks.

[Dridzt]

I can't get in-game now but try print(tostringall(...)) to see what's coming in your vararg.

[gamerfreak]

Originally Posted by Dridzt I can't get in-game now but try print(tostringall(...)) to see what's coming in your vararg.

SPELL_CAST_SUCCESS false 0x07000000054E9410 Gamerfreak 1297 0 0x07000000054E9410 Gamerfreak 1297 0 1126 Mark of the Wild 8

[gamerfreak]

Originally Posted by gamerfreak SPELL_CAST_SUCCESS false 0x07000000054E9410 Gamerfreak 1297 0 0x07000000054E9410 Gamerfreak 1297 0 1126 Mark of the Wild 8

Changed local eventType = select(2,...); to local eventType = select(1,...); and its working! Thanks. All the examples I saw had the sub-event string as the second arg.

[semlar]

It may be working, but it's not correct. The first argument for COMBAT_LOG_EVENT is the timestamp, and since you have the sub-event first it means your arguments are all off by one.

This is because "self" is implied when you use a colon to define a function. You could change it to events:COMBAT_LOG_EVENT_UNFILTERED(event, ...) but the entire point of writing an event handler like this is so you can define your variables directly in the function arguments.

I would personally remove the event name from the arguments since it's already the name of the function itself and I can't think of any reason you would need it. Something like this..

function events:COMBAT_LOG_EVENT_UNFILTERED(timestamp, event, _, srcGUID, srcName, srcFlags, srcRaidFlags, dstGUID, dstName, dstFlags, dstRaidFlags, ...)
    print(event) -- for debugging
    if (event == "UNIT_DIED") then
        print("UNIT_DIED")
    end
end
 
-----------------
--EVENT HANDLER--
-----------------
frame:SetScript("OnEvent", function(self, event, ...)
    events[event](self, ...)
end)
 
for k, v in pairs(events) do
    frame:RegisterEvent(k)
end

Also semi-colons are essentially just for looks, so unless you really like them the only time I would bother is to prevent ambiguity.

One more thing, NEVER do var = select(1, ...), it's literally the same thing as var = ... except with the overhead of a function call.

[gamerfreak]

Originally Posted by semlar It may be working, but it's not correct. The first argument for COMBAT_LOG_EVENT is the timestamp, and since you have the sub-event first it means your arguments are all off by one. This is because "self" is implied when you use a colon to define a function. You could change it to events:COMBAT_LOG_EVENT_UNFILTERED(event, ...) but the entire point of writing an event handler like this is so you can define your variables directly in the function arguments. I would personally remove the event name from the arguments since it's already the name of the function itself and I can't think of any reason you would need it. Something like this..

Thanks, super helpful post. Like I said I'm new to addons/lua, so these kind of mistakes are inevitable. Great explanation.

Originally Posted by semlar Also semi-colons are essentially just for looks, so unless you really like them the only time I would bother is to prevent ambiguity.

I'm coming from a C++/Java background, so they're really just habit.

[Phanx]

Also, do not do this:

function events:COMBAT_LOG_EVENT_UNFILTERED(self, event, ...)
    local eventType = select(2,...);
    local destGUID = select(7,...);

Calling functions is really, really slow (relatively speaking) and should be avoided whenever possible. In this case, you're doing two calls to select, which is almost always a completely useless function, this being no exception. Instead, assign all the variables (or at least the 1st-nth, where n is the highest number you care about) and just ignore the ones you're not using. A common convention in Lua is to use an underscore as the name of any variable you don't care about:

function events:COMBAT_LOG_EVENT_UNFILTERED(self, event, ...)
    local _, eventType, _, _, _, _, destGUID = ...

Alternatively, you can just name the arguments in the function definition itself:

function events:COMBAT_LOG_EVENT_UNFILTERED(self, event, _, eventType, _, _, _, _, destGUID, ...)

^ Here I've left the vararg at the end to contain all the unnamed variables, but if you know your code will never use anything past destGUID you can omit it.

Finally, a common practice for handling CLEU is to name all the variables that are the same for all sub-events, and use the vararg only for the event-specific variables:

function events:COMBAT_LOG_EVENT_UNFILTERED(self, event, timestamp, eventType, hideCaster, sourceGUID, sourceName, sourceFlags, sourceRaidFlags, destGUID, destName, destFlags, destRaidFlags, ...)