Change ShouldCollide to a posthook

[Kenzzer]

Fixes #1650

As pointed by @asherkin over discord the use of META_RESULT_OVERRIDE_RET or META_RESULT_ORIG_RET within a non post hook is wrong. This PR fixes that.

I'm not super familiar with sourcehook, so my fix could be horribly wrong. If I'm wrong, at the very least this PR will kick off a discussion? I was also tempted to replace the META_RESULT_ORIG_RET line with an SH_CALL instead, but this would end to essentially turning the function into a post hook.

Smallest PR in the small west ?

[asherkin]

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

[Kenzzer]

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.

[Mikusch]

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

[KyleSanderson]

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