This fix looks correct to me, but I'd like it to get some testing around the behaviour of ShouldCollide before merging.

Will try to test this weekend then. Though by behavior what exactly are you looking into being tested ?

All I can think right now, is retrieving the address of CBaseEntity::ShouldCollide make an SDKCall of it. (no virtual call)
Then write a plugin that makes use of SDKHook_ShouldCollide, and verify that the boolean parameter in the callback, matches what I get from the SDKCall. Then testing this plugin on current version of SM, then again but with this PR's change.
And we should get at least one random mismatch without the fix, and the other no mismatch.

I believe this would satisfy testing the behaviour ?

Edit : Something like

public bool Test_SDKHook_ShouldCollide(int entity, int collisiongroup, int contentsmask, bool originalResult)
{
	if (originalResult != SDKCall(sigCallShouldCollide, collisiongroup, contentsmask))
	{
		SetFailState("SDKHook_ShouldCollide mismatch!");
	}
	return originalResult;
}

With this plugin and setting the hook up on entities that don't override the virtual function. We should after a while, get a mismatch and the plugin unloaded. And with the PR hopefully no mismatch ever.

I've tested this on TF2 Windows.

Without the changes in this PR, a couple entities report a mismatch between SDKHook's originalResult and the return value of the SDKCall.
The only thing all of these entities have in common is that their collision group is set to COLLISION_GROUP_DEBRIS, but none of them actually override CBaseEntity::ShouldCollide.

With the changes in this PR, the only entities that report a mismatch are the ones that override CBaseEntity::ShouldCollide.

TF2 Windows sig in case someone else would like to test: \x55\x8B\xEC\x83\xB9\xF0\x01\x00\x00\x01

I've tested this on TF2 Windows.

Without the changes in this PR, a couple entities report a mismatch between SDKHook's originalResult and the return value of the SDKCall. The only thing all of these entities have in common is that their collision group is set to COLLISION_GROUP_DEBRIS, but none of them actually override CBaseEntity::ShouldCollide.

With the changes in this PR, the only entities that report a mismatch are the ones that override CBaseEntity::ShouldCollide.

TF2 Windows sig in case someone else would like to test: \x55\x8B\xEC\x83\xB9\xF0\x01\x00\x00\x01

Thank you very much for your feedback