Other cases of collision callback checks with no state restriction (and others)

Recently I reported this issue. Where Rooks could destroy on collision, so long as they had enough velocity, even if idling. I mentioned how I wasn't sure about disabling and enabling the collision callbacks when relevant, but he game already does actually do that.

alterguardian_phase1 is specifically set up to enable physics collision callback during certain states, and disable them otherwise. It should probably be done like this more often than not, to alleviate on the amount of physics collision callback checks + checks inside the custom collision callback functions.

Here's a list of entities that make use of collisions callbacks, none of them with state restrictions:

• archive_centipede (has velocity check)
• bearger and deerclops (has extremely low velocity check)
• deer (has velocity check)
• klaus (no velocity check)
• minotaur (has velocity check)
• rook (already reported in the linked report)
• stalker (no velocity check)
• stalker_minion (no velocity check, and questionable, as the stategraph already handles destruction of nearby things)
• toadstool (no velocity check, and questionable, as the stategraph already handles destruction of nearby things)

The last two are the most questionable ones, as they already have some stategraph logic to handle destruction of nearby things. Not only that, but the code in their stategraph and their collision callback doesn't even match in functionality or some of the checks. I feel like the collisions callback for them could be removed entirely as they seem redundant and a waste of performance.

The rest? I guess it depends and it's up to you after going through them and deciding. I do think that for most, if not all of them, should probably only use collision callbacks during certain states. There's no reason for many of them to run these things in weird non-moving states like idle, sleeping, frozen, etc.

Slightly related but I also feel like some of the collision callbacks could be checking for their work actions in a vein like this:

local COLLAPSIBLE_WORK_ACTIONS =
{
    CHOP = true,
    DIG = true,
    HAMMER = true,
    MINE = true,
}

-- example
(work_action == nil and v:HasTag("NPC_workable")) or
(v.components.workable:CanBeWorked() and work_action ~= nil and COLLAPSIBLE_WORK_ACTIONS[work_action.id])

Rather than:

other.components.workable ~= nil and
other.components.workable:CanBeWorked() and
other.components.workable.action ~= ACTIONS.DIG and
other.components.workable.action ~= ACTIONS.NET and

But I also feel like in general this could be further improved (for stategraphs, collision callback checks, etc) by consolidating a bunch of this behavior thrown around everywhere into the workable component itself. Something like:

-- allowed_actions should be a table set as a local in a file like a stategraph or prefab, or so
-- could also be set on an inst and sent here when doing the call, or even checked through here as a fallback


-- for NPC_workable capability, it could also be set as part of the allowed_actions table
-- instead of using the can_npc_work parameter, would need to adapt a bunch of what's shown in the example though

-- also, you could even add functionality for checking for tough work here, to see if allowed to break tough things at all
function Workable:CanBeWorkedBy(worker, can_npc_work, allowed_actions)
	if worker == nil then
		return false
	end

	-- nil action while having the component means we check for NPC_workable
		-- NOTE: this seems to intentionally ignore self.workable and self.workleft
		-- when checked in places in vanilla code, but if not intended, correct accordingly

	if self.action == nil then
		return can_npc_work and self.inst:HasTag("NPC_workable")
	end

	-- we have an action set, so we need to be in a workable state
	if not (self.workable and self.workleft > 0) then
		return false
	end

	-- up to you if you want to support checking thing that aren't a table
	-- I feel it'd probably be cleaner to handle things like this to make them simple and consistent to update
	if type(allowed_actions) ~= "table" or not allowed_actions[self.action.id] then
		return false
	end

    return true
end

But this is more a suggestion for cleanup, the physics collision callback stuff is way more important as it can affect performance and mechanics.