busybox42 / PapaInterrupt

World of Warcraft addon for dynamic interrupt messaging.
0 stars 0 forks source link

Small Improvements #1

Closed Road-block closed 2 weeks ago

Road-block commented 3 months ago

The saved variables initialization works "incidentally" at the moment and runs more often than you'd intend.

ADDON_LOADED fires for every addon the player has so currently you're running your initialization code for every addon loading after PapaInterrupt.

The wow client passes every lua file loaded by the addon's .toc a vararg with 2 arguments, the addon name (as a string) and an empty table scoped to the addon. The second is useful for storing and retrieving any variables or functions you want to share between files in your addon without polluting the global namespace all addons share.

In this case you can use the first argument to only run your initialization for your addon, not every addon loading.

local addonName, addonTable = ...

at the top of PapaInterrupt.lua

Then change PapaInterrupt.lua#L53 to

if event == "ADDON_LOADED" and ... == addonName then

and your init will only run for your addon.

Second improvement is performance oriented. Function calls have the largest performance hit in the wow Lua environment. You don't want to be running CombatLogGetCurrentEventInfo OR select when you don't need to.

It's best to run CombatLogGetCurrentEventInfo once, store all the returns to local variables and reference those when you need them. Using select() just to discard the timestamp return is wasteful.

Something like this is better. Replace PapaInterrupt.lua#L56 with

local timestamp, eventType, hideCaster, sourceGUID, sourceName, sourceFlags, sourceRaidFlags, destGUID, destName, destFlags, destRaidFlags, arg12, arg13, arg14, arg15, arg16, arg17, arg18, arg19, arg20, arg21, arg22, arg23, arg24 = CombatLogGetCurrentEventInfo()

UnitGUID("player") should not be called repeatedly everytime CLEU fires, it won't change. You should have

local playerGUID = UnitGUID("player")

outside of your function (above your event handler function for example) and just check against that.

if (eventType == "SPELL_INTERRUPT" and playerGUID == sourceGUID and PapaInterruptSettings.enabled) then
  local spellId, extraSpellID  = arg12, arg15

You get the idea 😃

Road-block commented 3 months ago

I would go as far as saying since you're not doing anything if PapaInterruptSettings.enabled is not true to have that check as the first line in the C_L_E_U handler and bail immediately before calling anything else.

elseif (event == "COMBAT_LOG_EVENT_UNFILTERED") then
  if not PapaInterruptSettings.enabled then return end
  local timestamp, eventType, hideCaster, sourceGUID, sourceName, sourceFlags, sourceRaidFlags, destGUID, destName, destFlags, destRaidFlags, arg12, arg13, arg14, arg15, arg16, arg17, arg18, arg19, arg20, arg21, arg22, arg23, arg24 = CombatLogGetCurrentEventInfo()
  if (eventType == "SPELL_INTERRUPT" and sourceGUID == playerGUID) then
    local spellId, extraSpellID  = arg12, arg15

etc

busybox42 commented 2 weeks ago

Thanks for the feedback! I've implemented your suggestions in the release v0.5.4.