I'm not a Wow addon specialist, far from it, I've never written one before. So my knowledge with the Wow APi is limited. I'm also not fluent in Lua, I can read/understand (usually) a piece of lua presented to me, but i'm not going to be spewing lines of lua code without constantly looking things up... I am however a professional software developper and have been for many years, so I do at least know how to write something slightly more complex than a "hello world" application :p
I went over the SHIT.lua source to try and figure out an issue I was having and ended up going way beyond that. Here's some stuff I noticed that may (or may not) help make this an even better mod than it already is.
1) the PLAYER_ALIVE event is being registered to the eventFrame, but all this event ever does is unregister itself. So unless this is a placeholder for a planned future feature. it can be removed entirely.
2) The PLAYER_REGEN_DISABLED and the COMBAT_LOG_EVENT_UNFILTERED handler do exactly the same. So could both be delegated to a single helper function.
the COMBAT_LOG_EVENT_UNFILTERED event seems a weird choice to me here (but then I don't know the wow api that well) but it's my understanding this is fired each and every time a line gets added to the combatlog, isn't this a bit "extreme" ? I'd imagine updates don't need to happen anywhere near that frequent (?).
2b) the PLAYER_REGEN_DISABLED function (and thus the combat event one) can be written smaller and more optimal (less overhead). Note that unless an error (or a purposed attempt at sabotage by manually changing the savedvariable file) is made, SHITdb.showon cannot have any other value than 1, 2 or 3. This assumption means we can rewrite the entire function to:
function SHIT.events.PLAYER_REGEN_DISABLED(...)
if (GetSpellInfo(SHIT.L["Crusader Strike"]) == null then
SHIT.displayFrame_last:Hide()
SHIT.displayFrame_current:Hide()
SHIT.displayFrame_next:Hide()
elseif (UnitName("target") == nil or
(SHITdb.showon == 1 and (UnitIsFriend("player","target") ~= nil or UnitHealth("target") == 0)) or
(SHITdb.showon == 2 and InCombatLockdown() == nil) or
(SHITdb.showon == 3 and UnitClassification("target") ~= "worldboss"))
SHIT.displayFrame_last:Hide()
SHIT.displayFrame_current:Hide()
SHIT.displayFrame_next:Hide()
SHIT.eventFrame:UnregisterEvent("COMBAT_LOG_EVENT_UNFILTERED")
else
SHIT.displayFrame_last:Show()
SHIT.displayFrame_current:Show()
SHIT.displayFrame_next:Show()
SHIT:DecideSpells()
end
end
3) The same logic as 2b can be applied to the PLAYER_TARGET_CHANGED() event handler.
function SHIT.events.PLAYER_TARGET_CHANGED(...)
-- target changed, set last target, update current target, will be nil if no target
SHIT.lastTarget = SHIT.currentTarget
SHIT.currentTarget = UnitGUID("target")
if GetSpellInfo(SHIT.L["Crusader Strike"]) == null then
SHIT.displayFrame_last:Hide()
SHIT.displayFrame_current:Hide()
SHIT.displayFrame_next:Hide()
elsif (UnitName("target") == nil or
(SHITdb.showon == 1 and (UnitIsFriend("player","target") ~= nil or UnitHealth("target") == 0)) or
(SHITdb.showon == 2 and InCombatLockdown() == nil) or
(SHITdb.showon == 3 and UnitClassification("target") ~= "worldboss")) then
SHIT.displayFrame_last:Hide()
SHIT.displayFrame_current:Hide()
SHIT.displayFrame_next:Hide()
else
SHIT.displayFrame_last:Show()
SHIT.displayFrame_current:Show()
SHIT.displayFrame_next:Show()
SHIT.eventFrame:RegisterEvent("COMBAT_LOG_EVENT_UNFILTERED")
SHIT:DecideSpells()
end
end
4) SHIT: DecideSpells() is now always being called in a situation where prior testing has been performed to determing that indeed something needs to be displayed.
Yet, in the start of DecideSpells() function this same set of tests (player has Crusader strike, testing the ShowOn conditions etc) is being done again to have an 'early out' type test.
The tests seem to be not needed.
The one exception is in the showon=1 case where some extra stuff happens. If this is needed the start of DecideSpells() can be changed to just:
if (SHITdb.showon==1)
local guid = UnitGUID("target")
if guid == nil then
SHIT.textureList["last"]:SetTexture(nil)
SHIT.textureList["current"]:SetTexture(nil)
SHIT.textureList["next"]:SetTexture(nil)
return
end
end
My guess is it isn't needed. DecideSpells() is now always being called when UnitName("target") has not returned nil. I'd guess/expect that when you have a targetname you will always also have a targetGUID (?).
5) SHIT: DecideSpells()
Is using a hardcoded 1.5sec (GCD) towards calculating the spell to use after the current one.
This is correct except for Consecration, Exorcism, Divine Plea and Sacred Shield as those are on hasted GCD.
Is there a way to calculate actual GCD ? maybe you could use the CastTime for FOL. since that's a 1.5sec spell and has the same haste benefit as the GCD on spells.
I'm not sure if our GCD's on the melee abilities get hasted from Heroism, but that would also mean the "next spell" might be off with the actual spell that will be suggested as new current one after casting the current one.
The next spell also doesn't account for the change in mana for casting the current ability. So next spell could be suggested to be Consecration, but after casting your current ability not have enough mana left to meet the Consecration mana requirement. Or other way around, if you just casted judgement, the mana returned could mean you suddenly have enough mana for Consecration where you previously had not.
Next spell also assumes the consecration glyph (hardcoding the cooldown in nextspell to 10 seconds), and also assumes 2/2 Imp Judgement setting judgement at 8sec cd, while we probably "all" will have this, there was the debate about whether or not you'd ever judge faster than every 9 seconds, so only 1/2 imp judgements would be needed.
A minor issue, but... don't trust too much on the "next" ability to actually going to be the next one being suggested
6) SHIT:NextSpell()
The long chain of if-tests to test each possible ability in each possible priority seems like a prime candidate for a better alternative.
for a start the pr1, pr2, pr3, pr4... can be changed to an array...
-> pr[1], pr[2], pr[3], pr[4]...
you can then already cut the chain of if's down to a for-loop testing each priority (by index) for the various ability numbers.
That'll cut down in lua code size, but I'm not sure how that change would affect performance.