Bug with __project_source when resolving callbacks

[Lokean]

I have been pestering Shibari about a bug I uncovered while working on a class addon, and think I have enough to report properly now.

Sometimes, the __project_source and resolveSource architecture fails to result in delayedLogDamage aggregating lines correctly.

An example of the behaviour can be reproduced in vanilla ToME by damaging yourself while affected by Reality Smearing. Instead of the expected line:

Code: Select all

Player hits Player for (45 converted), 106 arcane (106 total damage)

there are two separate entries in the log, which are not necessarily adjacent:

Code: Select all

Player hits Player for 106 arcane damage
Reality Smearing hits player for (45 converted) damage

I thought this was probably restricted to the callback architecture, and have traced the specific behaviour in my addon, and in vanilla with reality smearing, to the replacement of the existing __project_source in fireTalentCheck (/modules/tome/class/Actor.lua#5328‑5341 in the current repository). This results in the called-back sustain being treated as the source when it should still resolve to the self in the case of damage mitigation from self-damage.

[Lokean]

I hacked together a poor solution, which I threw at Shibari, but think there is a small tweak required compared to the merge request he posted:

Code: Select all

function _M:fireTalentCheck(event, ...)
 	if self[store] then upgradeStore(self[store], store) end
 	if self[store] and next(self[store].__priorities) then
 		local old_ps = self.__project_source
-- Changes to code here
		local src_check, keep_old -- we need these to be available outside the if
		if event == "callbackOnHit" then
			src_check = (select(2,...)) -- if this is a callbackonHit, the second parameter is the source of the hit, I think this is fairly awful coding practice, but I am not super-familiar with Lua
			keep_old = (src_check.resolveSource and src_check:resolveSource() or src_check) == self --we shouldn't overwrite our __project_source if we're the source of the damage (we are more concerned with why we hurt ourselves than what the callback is)
-- original hackfix had this line:
--			old_ps = old_ps or self
-- This leaves us with an instantiated __project_source at the end of the function call, though, even if it wasn't at the start, so it may be better to leave this as nil and accept more logic overhead in the ternaries
		end
 		for _, info in ipairs(self[store].__sorted) do
 			local priority, kind, stringId, tid = unpack(info)
 			if kind == "effect" then
				self.__project_source = keep_old and (old_ps or self) or self.tmp[tid] -- change from MR, to account for old_ps == nil
 				ret = self:callEffect(tid, event, ...) or ret
 			elseif kind == "object" then
				self.__project_source = keep_old and (old_ps or self) or tid -- change from MR, to account for old_ps == nil
 				ret = tid:check(event, self, ...) or ret
 			else
				self.__project_source = keep_old and (old_ps or self) or self.sustain_talents[tid] -- change from MR, to account for old_ps == nil
 				ret = self:callTalent(tid, event, ...) or ret
 			end
 			self.__project_source = old_ps
 		end
 	end
 	return ret
end

This fixes the bug I encountered, but isn't perfect. A similar change might need to be made to _M:iterCallbacks so that the same tests are applied inside the wrapper functions. In this case, it would be essential that the more complex ternary be used, to avoid messing up the __project_source as you iterate over the callbacks.

Potential bugs exist in any other callback where there are both a source and a target entity passed and where both could resolve to the same source (callbackOnDealDamage, callbackOnKill, callbackOnSummonDeath) but only callbackOnDealDamage is likely to actually result in anything odd being printed to the log.