InCombatLockdown() versus Events and Bool

[Olvar]

What would be more efficient to check if the player is in combat, in your opinion?

1. Calling InCombatLockdown().

2. Registering events PLAYER_REGEN_DISABLED and PLAYER_REGEN_ENABLED and remembering the status in a local bool variable.

[VagrantEsha]

Would you be registering any other events in the mod?

[Olvar]

I already have PLAYER_REGEN_DISABLED and COMBAT_LOG_EVENT_UNFILTERED registered, which is why I think that changing some InCombat bool to false in a PLAYER_REGEN_ENABLED event might be faster than using InCombatLockdown().

But I wanted to know what other authors with more knowledge of the whole system think about it.

From what I understand, using something local is always faster. Thus working with a local bool variable should be more efficient than calling a global function.

[Taffu]

I've always found PLAYER_REGEN_ENABLED and PLAYER_REGEN_DISABLED to be more reliable means for combat association, and using a local.

[Akryn]

Originally Posted by Taffu I've always found PLAYER_REGEN_ENABLED and PLAYER_REGEN_DISABLED to be more reliable means for combat association, and using a local.

the other advantage to the events is that they fire when the player is not locked down; so if you want to perform some last-second actions before things get locked, you can do that.

[Duugu]

COMBAT_LOG_UNFILTERED means there are a LOT calls oft your event handler - at least in combat or with other people in range. Or not?
And how often will you call InCombatLockdown() in your code?

I don't think that there are mentionable performance differences between both ways ... at least not within common scenarios.

Furthermore you could make InCombatLockdown() local, or not?

local InCombatLockdown = InCombatLockdown

Shouldn't this boost the performance?

[Shirik]

Originally Posted by Olvar What would be more efficient to check if the player is in combat, in your opinion? 1. Calling InCombatLockdown(). 2. Registering events PLAYER_REGEN_DISABLED and PLAYER_REGEN_ENABLED and remembering the status in a local bool variable.

There is a difference, though the difference is slight.

Firstly, before I say anything, accessing locals is free. However, you probably don't mean local, you probably mean upvalue. But if it were truly a local, it would be a free lookup (and the test is extremely fast).

Calling InCombatLockdown() can be a global call unless you retain a local copy of it, and even then it will require a call. This means this will always be slower.


HOWEVER: The two things you are saying are not the same:

local isInLockdown = false;
local frame = CreateFrame "FRAME"
frame:RegisterEvent("PLAYER_REGEN_ENABLED");
frame:RegisterEvent("PLAYER_REGEN_DISABLED");
frame:SetScript("OnEvent", function(self, event, ...)
   if event == "PLAYER_REGEN_DISABLED"
      isInLockdown = true;
   elseif event == "PLAYER_REGEN_ENABLED"
      isInLockdown = false;
   end
end);

This will work for most purposes, but let's say that we had a little change:

local isInLockdown = false;
local frame = CreateFrame "FRAME"
frame:RegisterEvent("PLAYER_REGEN_ENABLED");
frame:RegisterEvent("PLAYER_REGEN_DISABLED");
frame:SetScript("OnEvent", function(self, event, ...)
   if event == "PLAYER_REGEN_DISABLED"
      isInLockdown = true;
      test();
   elseif event == "PLAYER_REGEN_ENABLED"
      isInLockdown = false;
      test();
   end
end);

function test()
   assert(isInLockdown == not not InCombatLockdown());
end

You will find assertion failures popping up all over the place. In particular, you go into lockdown at some time shortly after PLAYER_REGEN_DISABLED. At PLAYER_REGEN_DISABLED, you are still not in lockdown. This may be a relevant difference for some addons. Keep this in mind.

[Tekkub]

The real question is how often, on average, are you calling InCombatLockdown()? If it's more than twice per combat session, the events would be the faster route. You can, of course, hold a local of the function to avoid that global lookup Shirik mentions, but there's not much point to that if you're only calling once or twice.

[Olvar]

First of all, thanks for taking part in the discussion, guys.

In case you are wondering, what this is for, it's part of my Pull Waring addon:
http://www.wowinterface.com/download...llWarning.html

I know that COMBAT_LOG_EVENT_UNFILTERED means a lot of events, which is why I want to make the test as fast as possible. Unfortunately Faerie Fire is still not part of the filtered combat log, so I'm forced to use the unfiltered events.

The only advantage (for my purposes) of InCombatLockdown() I can think of right now, is that the check is correct even when the addon is enabled after you are already in combat. But I can check that when I'm registering the events and simply set my local bool accordingly.

I'll have to do an update of my addon anway, because I found a very peculiar bug during a raid yesterday. Try the following:

/rw ** Body Pull **
(Leads to the pull warning sound and the message ** Body Pull ** being displayed)

/rw *** Body Pull ***
(You can hear the pull warning sound, but there will be no message!)

According to my tests, a raid warning starting with at least three asterisks won't be displayed, but the sound effect is generated. Very strange indeed.

[Broken Hope]

Originally Posted by Olvar I'll have to do an update of my addon anway, because I found a very peculiar bug during a raid yesterday. Try the following: /rw ** Body Pull ** (Leads to the pull warning sound and the message ** Body Pull ** being displayed) /rw *** Body Pull *** (You can hear the pull warning sound, but there will be no message!) According to my tests, a raid warning starting with at least three asterisks won't be displayed, but the sound effect is generated. Very strange indeed.

That's not a bug as far as I know, it's due to mods like Bigwigs and Deadly Boss Mods, they have Bossblock features which block RW spam from other mods, these RW messages have 3 asterisks at the start.

This is taken from the BossBlock module of Bigwigs

function plugin:IsSpam(text)
	if type(text) ~= "string" then return end
	if fnd(text, "%*%*%*") then return true end
end

[Tekkub]

Stupid question, but are you wanting to only listen to COMBAT_LOG_EVENT_UNFILTERED when in combat?

[Olvar]

Originally Posted by Broken Hope That's not a bug as far as I know, it's due to mods like Bigwigs and Deadly Boss Mods, they have Bossblock features which block RW spam from other mods, these RW messages have 3 asterisks at the start.

Thanks for clearing this up. That explains a lot. I knew that BigWigs had some filter, but I didn't know how it actually worked. I've changed my output accordingly.

Originally Posted by Tekkub Stupid question, but are you wanting to only listen to COMBAT_LOG_EVENT_UNFILTERED when in combat?

Since Pull Warning shall display a warning as soon as an enemy is pulled but otherwise keep quite during combat, I need the event just before combat is entered. Of course this could be done with PLAYER_REGEN_DISABLED only, but the event for an instant cast fires a little bit earlier and I also like the fact that I'm able to display the pullers name and the enemy.

But your question wasn't stupid at all, because it reminded me that I could actually unregister COMBAT_LOG_EVENT_UNFILTERED during combat. I'm not sure how efficient UnregisterEvent() is, but for a longer combat and especially in a raid it will be much better than catching all those combat events and ignoring them.

The redesigned code seems to work fine solo and in a group. I should have a chance to test it in a raid this evening. Unless I notice any problems I'll probably update the addon tomorrow.

Thank again for the discussion.