Improve and fix health and power update logic
Ultis opened this issue ยท 7 comments
Currently there is a correctness and a performance issue with the health and the power elements.
The correctness issue is that in frequent update mode, the bar is updated only if the current health/power change; however, it should also be updated if the maximum health/power changes without the current value also changing (e.g. Rallying Cry possibly), or if the power type changes without the values changing (e.g. bear->cat switch without Furor).
The performance issue is that while in frequent update mode there is a check that avoids updating if values are unchanged, there is no such thing for the automatic updates that non-group unit frames do on all elements.
This proposed patch changes the code so that the Path function checks the values in all cases and does not call the update function at all if nothing changed.
Note that "min" is an inappopriate name for the current health/power (should be "cur"), but I kept that to avoid slightly breaking the API.
I tested this patch, although haven't really used in gameplay yet, so there might be some issues although hopefully not.
Apparently I was using an older version, here is a patch updated to current trunk.
In addition, to the patch above, I also remove the "(unit == 'player' or unit == 'pet')" check in power.lua that doesn't seem to make any sense and breaks frequent updates for "target" and other units.
Also, restored the self.unit ~= unit checks, which now happen in Path before calling any WoW API function.
--- oUF\elements\health.lua.orig 2011-08-21 08:56:32 +0200
+++ oUF\elements\health.lua 2011-09-14 14:14:01 +0200
@@ -1,16 +1,15 @@
local parent, ns = ...
local oUF = ns.oUF
+local UnitHealth, UnitHealthMax, UnitIsConnected = UnitHealth, UnitHealthMax, UnitIsConnected
oUF.colors.health = {49/255, 207/255, 37/255}
local Update = function(self, event, unit, powerType)
- if(self.unit ~= unit) then return end
local health = self.Health
if(health.PreUpdate) then health:PreUpdate(unit) end
- local min, max = UnitHealth(unit), UnitHealthMax(unit)
- local disconnected = not UnitIsConnected(unit)
+ local min, max, disconnected = health.min, health.max, health.disconnected
health:SetMinMaxValues(0, max)
if(disconnected) then
@@ -19,8 +18,6 @@
health:SetValue(min)
end
- health.disconnected = disconnected
-
local r, g, b, t
if(health.colorTapping and UnitIsTapped(unit) and not UnitIsTappedByPlayer(unit)) then
t = self.colors.tapped
@@ -57,8 +54,19 @@
end
end
-local Path = function(self, ...)
- return (self.Health.Override or Update) (self, ...)
+local Path = function(self, event, unit, ...)
+ if(self.unit ~= unit) then return end
+
+ local health = self.Health
+ local min, max, disconnected = UnitHealth(unit), UnitHealthMax(unit), not UnitIsConnected(unit)
+
+ if(min ~= health.min or max ~= health.max or disconnected ~= health.disconnected) then
+ health.min = min
+ health.max = max
+ health.disconnected = disconnected
+
+ return (self.Health.Override or Update) (self, event, unit, ...)
+ end
end
local ForceUpdate = function(element)
--- oUF\elements\power.lua.orig 2011-08-21 08:56:32 +0200
+++ oUF\elements\power.lua 2011-09-14 15:12:52 +0200
@@ -1,5 +1,6 @@
local parent, ns = ...
local oUF = ns.oUF
+local UnitPower, UnitPowerMax, UnitPowerType, UnitIsConnected = UnitPower, UnitPowerMax, UnitPowerType, UnitIsConnected
oUF.colors.power = {}
for power, color in next, PowerBarColor do
@@ -18,14 +19,12 @@
end
local Update = function(self, event, unit)
- if(self.unit ~= unit) then return end
local power = self.Power
if(power.PreUpdate) then power:PreUpdate(unit) end
local displayType = GetDisplayPower(power, unit)
- local min, max = UnitPower(unit, displayType), UnitPowerMax(unit, displayType)
- local disconnected = not UnitIsConnected(unit)
+ local min, max, disconnected = power.min, power.max, power.disconnected
power:SetMinMaxValues(0, max)
if(disconnected) then
@@ -34,8 +33,6 @@
power:SetValue(min)
end
- power.disconnected = disconnected
-
local r, g, b, t
if(power.colorTapping and UnitIsTapped(unit) and not UnitIsTappedByPlayer(unit)) then
t = self.colors.tapped
@@ -78,8 +75,21 @@
end
end
-local Path = function(self, ...)
- return (self.Power.Override or Update) (self, ...)
+local Path = function(self, event, unit, ...)
+ if(self.unit ~= unit) then return end
+
+ local power = self.Power
+ local type = GetDisplayPower(self, unit)
+ local min, max, disconnected = UnitPower(unit, type), UnitPowerMax(unit, type), not UnitIsConnected(unit)
+
+ if(min ~= power.min or max ~= power.max or type ~= power.type or disconnected ~= power.disconnected) then
+ power.min = min
+ power.max = max
+ power.type = type
+ power.disconnected = disconnected
+
+ return (self.Power.Override or Update) (self, event, unit, ...)
+ end
end
local ForceUpdate = function(element)
@@ -91,14 +101,8 @@
local UnitPower = UnitPower
OnPowerUpdate = function(self)
if(self.disconnected) then return end
- local unit = self.__owner.unit
- local power = UnitPower(unit, GetDisplayPower(self, unit))
- if(power ~= self.min) then
- self.min = power
-
- return Path(self.__owner, 'OnPowerUpdate', unit)
- end
+ return Path(self.__owner, 'OnPowerUpdate', self.__owner.unit)
end
end
@@ -108,7 +112,7 @@
power.__owner = self
power.ForceUpdate = ForceUpdate
- if(power.frequentUpdates and (unit == 'player' or unit == 'pet')) then
+ if(power.frequentUpdates) then
power:SetScript("OnUpdate", OnPowerUpdate)
else
self:RegisterEvent('UNIT_POWER', Path)Patch here.
--- oUF\elements\health.lua.original 2011-09-07 19:48:27 +0200
+++ oUF\elements\health.lua 2011-09-14 13:37:27 +0200
@@ -1,16 +1,15 @@
local parent, ns = ...
local oUF = ns.oUF
+local UnitHealth, UnitHealthMax, UnitIsConnected = UnitHealth, UnitHealthMax, UnitIsConnected
oUF.colors.health = {49/255, 207/255, 37/255}
local Update = function(self, event, unit, powerType)
- if(self.unit ~= unit or (event == 'UNIT_POWER' and powerType ~= 'HAPPINESS')) then return end
local health = self.Health
if(health.PreUpdate) then health:PreUpdate(unit) end
- local min, max = UnitHealth(unit), UnitHealthMax(unit)
- local disconnected = not UnitIsConnected(unit)
+ local min, max, disconnected = health.min, health.max, health.disconnected
health:SetMinMaxValues(0, max)
if(disconnected) then
@@ -19,8 +18,6 @@
health:SetValue(min)
end
- health.disconnected = disconnected
-
local r, g, b, t
if(health.colorTapping and UnitIsTapped(unit) and not UnitIsTappedByPlayer(unit)) then
t = self.colors.tapped
@@ -59,28 +56,27 @@
end
end
-local Path = function(self, ...)
- return (self.Health.Override or Update) (self, ...)
+local Path = function(self, event, unit, ...)
+ local health = self.Health
+ local min, max, disconnected = UnitHealth(unit), UnitHealthMax(unit), not UnitIsConnected(unit)
+
+ if(min ~= health.min or max ~= health.max or disconnected ~= health.disconnected) then
+ health.min = min
+ health.max = max
+ health.disconnected = disconnected
+
+ return (self.Health.Override or Update) (self, event, unit, ...)
+ end
end
local ForceUpdate = function(element)
return Path(element.__owner, 'ForceUpdate', element.__owner.unit)
end
-local OnHealthUpdate
-do
- local UnitHealth = UnitHealth
- OnHealthUpdate = function(self)
- if(self.disconnected) then return end
- local unit = self.__owner.unit
- local health = UnitHealth(unit)
-
- if(health ~= self.min) then
- self.min = health
+local OnHealthUpdate = function(self)
+ if(self.disconnected) then return end
- return Path(self.__owner, "OnHealthUpdate", unit)
- end
- end
+ return Path(self.__owner, "OnHealthUpdate", self.__owner.unit)
end
local Enable = function(self, unit)
--- oUF\elements\power.lua.original 2010-11-28 21:37:40 +0100
+++ oUF\elements\power.lua 2011-09-14 13:39:23 +0200
@@ -1,5 +1,6 @@
local parent, ns = ...
local oUF = ns.oUF
+local UnitPower, UnitPowerMax, UnitPowerType, UnitIsConnected = UnitPower, UnitPowerMax, UnitPowerType, UnitIsConnected
oUF.colors.power = {}
for power, color in next, PowerBarColor do
@@ -9,13 +10,11 @@
end
local Update = function(self, event, unit)
- if(self.unit ~= unit) then return end
local power = self.Power
if(power.PreUpdate) then power:PreUpdate(unit) end
- local min, max = UnitPower(unit), UnitPowerMax(unit)
- local disconnected = not UnitIsConnected(unit)
+ local min, max, disconnected = power.min, power.max, power.disconnected
power:SetMinMaxValues(0, max)
if(disconnected) then
@@ -24,8 +23,6 @@
power:SetValue(min)
end
- power.disconnected = disconnected
-
local r, g, b, t
if(power.colorTapping and UnitIsTapped(unit) and not UnitIsTappedByPlayer(unit)) then
t = self.colors.tapped
@@ -70,28 +67,28 @@
end
end
-local Path = function(self, ...)
- return (self.Power.Override or Update) (self, ...)
+local Path = function(self, event, unit, ...)
+ local power = self.Power
+ local min, max, type, disconnected = UnitPower(unit), UnitPowerMax(unit), select(2, UnitPowerType(unit)), not UnitIsConnected(unit)
+
+ if(min ~= power.min or max ~= power.max or type ~= power.type or disconnected ~= power.disconnected) then
+ power.min = min
+ power.max = max
+ power.type = type
+ power.disconnected = disconnected
+
+ return (self.Power.Override or Update) (self, event, unit, ...)
+ end
end
local ForceUpdate = function(element)
return Path(element.__owner, 'ForceUpdate', element.__owner.unit)
end
-local OnPowerUpdate
-do
- local UnitPower = UnitPower
- OnPowerUpdate = function(self)
- if(self.disconnected) then return end
- local unit = self.__owner.unit
- local power = UnitPower(unit)
-
- if(power ~= self.min) then
- self.min = power
+local OnPowerUpdate = function(self)
+ if(self.disconnected) then return end
- return Path(self.__owner, 'OnPowerUpdate', unit)
- end
- end
+ return Path(self.__owner, 'OnPowerUpdate', self.__owner.unit)
end
local Enable = function(self, unit)
The correctness issue is that in frequent update mode, the bar is updated only if the current health/power change; however, it should also be updated if the maximum health/power changes without the current value also changing (e.g. Rallying Cry possibly), or if the power type changes without the values changing (e.g. bear->cat switch without Furor).
Both health and power register events outside of the periodic check to work around this.
The performance issue is that while in frequent update mode there is a check that avoids updating if values are unchanged, there is no such thing for the automatic updates that non-group unit frames do on all elements.
There isn't a check for unnecessary updates on non-frequent updated health/power bars, as they are driven by events, which contain the unitid. Are there really cases where the events fire, and nothing change?
This proposed patch changes the code so that the Path function checks the values in all cases and does not call the update function at all if nothing changed.
You would have to cache all return values of all functions used to have this work correctly. Due to that it would most likely make it less performance friendly than it currently is.
I would say you are currently trying to fix this issue in the wrong spot. The main problem with oUF right now is excessive full unit updates in raids. The fix for this would be to track unit guid and tag frames for full updates, instead of just running it regardless.
In addition, to the patch above, I also remove the "(unit == 'player' or unit == 'pet')" check in power.lua that doesn't seem to make any sense and breaks frequent updates for "target" and other units.
I added the check because I couldn't understand why anyone would care about having more up to date power for other units. I can understand why a healer would want it for health, but not why any class would want it for mana. I was unsure if I should let the target unitid in on the fun, but I decided against it as UNIT_POWER fires pretty frequently anyway.
On Thu, Sep 15, 2011 at 7:56 AM, Adrian L Lange
[email protected]
wrote:
It would be nice to have frequent updates on target power on certain raid encounters, majordomo comes to mind
I don't mind adding target to the list. The main goal of the check is
to block people from adding it on every single unit they create. As
mentioned earlier, I don't see why anyone would want frequent updates
on all of the raid/party units. For now I'd suggest extending the
whitelist to focus and target as well.
Reply to this email directly or view it on GitHub:
#99 (comment)
It would be nice to have frequent updates on target power on certain raid encounters, majordomo comes to mind
Both health and power register events outside of the periodic check to work around this.
Yes, right, I guess it might indeed be faster to just check the current health during the frequent updates and rely on events otherwise, as it is currently done.
There isn't a check for unnecessary updates on non-frequent updated health/power bars, as they are driven
by events, which contain the unitid. Are there really cases where the events fire, and nothing change?
If I'm not mistaken, units like "boss1target" for instance are not driven by events, since those are not fired, but rather by the oUF framework updating all elements.
This by default happens every 0.5 seconds, but I guess at least some people lower the threshold to update it every frame.
In this scenario, the whole update logic would be run every frame, as far as I can tell.
So my idea is to put a check in Path that prevents it running if nothing changed.
Now that said, there are issues with the other modules: for example, updating Aura elements every frame will have significant overhead.
So overall it might be best to handle COMBAT_LOG_EVENT_UNFILTERED in a single central place, dispatch "fake" UNIT_HEALTH, UNIT_AURA, etc. based on that and then drop both the frequent updates logic, and the periodic updating from enableTargetUpdate.
This would require central tracking of GUIDs, as you said.
Perhaps enableTargetUpdate could be kept as an alternative option with lower overhead in some cases for those who don't care about having everything up-to-date every frame.
Anyway, yes, don't apply the patch as-is, except perhaps for the part allowing power frequent updates on all units.
Yes, units which don't have any unitid associated with them can (if you set frenquentUpdates to 0) be updated on every frame. I don't think anyone lowers this below 0.05 however. On top of this execution time slows down the OnUpdate handler somewhat.
I would be a lot more concerned about auras (as you mention) and tags in this scenario. For auras one could centralize the updating to cache return values for all currently active unitid's and expose this through a backwards compatible UnitAura function. Alternatively one could go a step further and expose the raw cache data. It would probably lessen the footprint of that element by quite a bit.
So my idea is to put a check in Path that prevents it running if nothing changed.
I believe this won't really help much for power and health. There are too many values that you need to cache and I don't think the update logic is the bottleneck, but rather the function calls to the WoW API.
So overall it might be best to handle COMBAT_LOG_EVENT_UNFILTERED in a single central place, dispatch "fake" UNIT_HEALTH, UNIT_AURA, etc. based on that and then drop both the frequent updates logic, and the periodic updating from enableTargetUpdate.
I'm not sure if handling CLEU is beneficial, it isn't exactly straight forward to convert the data into what the events provide. It also is a very active function, especially in raids.
Perhaps enableTargetUpdate could be kept as an alternative option with lower overhead in some cases for those who don't care about having everything up-to-date every frame.
Eventless frames can already do this through onUpdateFrequency. Adding custom paths in the elements for eventless frames would most likely only be beneficial to auras.
Anyway, yes, don't apply the patch as-is, except perhaps for the part allowing power frequent updates on all units.
Create a git format-patch and I'll commit it in your name. If you don't care, then a unified diff is fine. :)