BigWigs icon indicating copy to clipboard operation
BigWigs copied to clipboard

Core/BossPrototype: Add Throttle function

Open Oppzippy opened this issue 6 years ago • 2 comments

The following pattern occurs very frequently for throttling messages from spells that are all cast in rapid succession.

do
	local prev = 0
	function mod:AnAbility(args)
		local t = args.time
		if t-prev > 2 then
			prev = t
			-- do stuff
		end
	end
end

This could be rewritten as:

function mod:AnAbility(args)
	if self:Throttle(args.spellId, 2, args.time) then
	-- or this: if not self:Throttle(args.spellId, 2, args.time) then return end
		-- do stuff
	end
end

Oppzippy avatar Jan 10 '19 12:01 Oppzippy

While this might arguably be a bit prettier, it performs worse. It requires an unneeded table lookup and the scope of the variable gets bigger.

So for me it's a nope - performance is the highest priority in a throttle function.

elvador avatar Jan 10 '19 17:01 elvador

Fair enough. Didn't think performance would really matter for something run so infrequently but you are right. A quick test shows this to take 300% as long to run as the current solution.

Edit: At the same time though, it is common to use args.spellId when there is only one possible value for it anyway. It's an extra table lookup every time that's done instead of hardcoding the spell ID instead. There is value to readability and ease of writing code. I'm not saying it's okay because it's done elsewhere, just that it is done for a reason that can apply to this as well.

Oppzippy avatar Jan 10 '19 18:01 Oppzippy