Add SetCollisionGroup to API (#1461)

[ashort96]

Addresses issue #1461 and adds SetCollisionGroup to the API. Only CS:S and TF2 signatures are added - requesting feedback of where the CS:GO signature should go.

[ashort96]

Added CS:GO signature (linux only).

[KyleSanderson]

I'm going to sleep on this one. It's likely the right thing to do (years late, but still the right thing) but there has to be a better way than the signatures.

[asherkin]

but there has to be a better way than the signatures.

CollisionRulesChanged (which is the important bit of this that SetCollisionGroup calls internally) is not completely trivial to re-implement and neither of them are virtual - this seems like the best option. This has been in the queue for almost a decade.

[KyleSanderson]

but there has to be a better way than the signatures.

CollisionRulesChanged (which is the important bit of this that SetCollisionGroup calls internally) is not completely trivial to re-implement and neither of them are virtual - this seems like the best option. This has been in the queue for almost a decade.

There's a better option, which is PhysEnableEntityCollisions / PhysDisableEntityCollisions - no? Ultimately that's what people actually want out of this, changing collision groups themselves no one particularly cares about. I could be misunderstanding this function though but if we're sigscanning already perhaps it isn't optimized out.

[ashort96]

but there has to be a better way than the signatures.

CollisionRulesChanged (which is the important bit of this that SetCollisionGroup calls internally) is not completely trivial to re-implement and neither of them are virtual - this seems like the best option. This has been in the queue for almost a decade.

There's a better option, which is PhysEnableEntityCollisions / PhysDisableEntityCollisions - no? Ultimately that's what people actually want out of this, changing collision groups themselves no one particularly cares about. I could be misunderstanding this function though but if we're sigscanning already perhaps it isn't optimized out.

The most common use of changing an entity's collision group I've seen is setting the collision group to to either COLLISION_GROUP_DEBRIS_TRIGGER (noblock) or COLLISION_GROUP_PLAYER (block), although I've seen people in the AM Discord mention using other collision groups, such as COLLISION_GROUP_WEAPON. I took a quick peak at the two methods you mentioned, and it doesn't look like it gives the ability to specify the collision group you want - I didn't the RemoveObjectPair() method, so maybe there was something more in there I missed. I do think that while the majority will probably only use the first two collision groups for players, there might be some other uses cases that could be supported by adding this.

[asherkin]

Nice, this all looks good to me.

[einyux]

Maybe too late, but shouldn't the native named for the sake of clarity something like "SetEntityCollisionGroup" (To be consistent with the other entity natives) ?

[psychonic]

Maybe too late, but shouldn't the native named for the sake of clarity something like "SetEntityCollisionGroup" (To be consistent with the other entity natives) ?

I agree.

[ashort96]

The native's first param is entity, but being consistent with other natives makes sense - I can make a follow on PR. Should this enum also be added? That would make a transition from anyone using my plugin seamless (just delete #include <SetCollisionGroup> and remove the running plugin).

[Kenzzer]

Another late comment on this PR. But now that SetCollisionGroup is added, odds are people will want SetOwnerEntity exposed as it also calls CollisionRulesChanged to fix some physics issues.

Much agreeing with @KyleSanderson , depending on a signature seems a bit too much, especially if this native is going to be wanted on more games than just TF2/CS.

Therefore I suggest the following alternative:
CBaseEntity::SetOwnerEntity is virtual, and its vtable offset is quite low (Linux:18 Win:17 on TF2, L4D2, HL2:DM, Insurgency, Days Of Defeat, and CSS, I'm willing to bet it's also the same on CSGO, unfortunately I lack binaries before the symbol strip to confirm this).

The function is implemented as follow:

void CBaseEntity::SetOwnerEntity( CBaseEntity* pOwner )
{
	if ( m_hOwnerEntity.Get() != pOwner )
	{
		m_hOwnerEntity = pOwner;

		CollisionRulesChanged();
	}
}

Considering m_hOwnerEntity is a netprop, to obtain a dirty access to CollisionRulesChanged. One simple way would be to retrieve whatever entity is inside m_hOwnerEntity.
If the entity is null, we force set m_hOwnerEntity to some random entity (worldspawn/first entity)
If the retrieved entity is not null, we force set m_hOwnerEntity to null.
Then we call the function, with as parameter whatever was previously in m_hOwnerEntity.

This effectively only calls CollisionRulesChanged which is what we want.

With that dirty hack done, we can then reimplement SetCollisionGroup ourselves, by changing the netprop collision group value and then calling SetOwnerEntity with the twist I just mentioned.
The added bonus, is we can also offer plugins a new native to call CollisionRulesChanged and SetOwnerEntity.

If I wasn't clear in my explanations, I could always make a PR.

Extra note: it is possible that this virtual function gets overriden, although I'm not aware of any classes that do this. But should it be overridden, we could parse the vtable on worldspawn entity to retrieve CBaseEntity::SetOwnerEntity as it isn't overidden there.

[einyux]

The native's first param is entity, but being consistent with other natives makes sense - I can make a follow on PR. Should this enum also be added? That would make a transition from anyone using my plugin seamless (just delete #include <SetCollisionGroup> and remove the running plugin).

About the enum Collision_Group, I don't know if all games supported by SM have the exact same list (At least PORTAL2 doesn't, but it isn't supported so I guess it's ok). If the enum is added, it would be nice to have the GetEntityCollisionGroup native to avoid a view_as<> cast everytime.

[ashort96]

It is very well possible that things like CBaseEntity::SetMoveType() could break physics as well - maybe it would just be more beneficial to do what @Kenzzer suggested with SetOwnerEntity() OR change the signature dependency to rely on CollisionRulesChanges() - the method is very easy to find in the binary (easily searchable string), so any updates that might break it should be easily fixable.

Anyone who might call something that would break physics would just have to do something like this

SetEntProp(client, Prop_Data, "m_CollisionGroup", COLLISION_GROUP_DEBRIS_TRIGGER, 4);
CollisionRulesChanged();

[einyux]

Same issue for CBaseEntity::SetSolid & CBaseEntity::SetSolidFlags.
For CS:GO, these functions look more complicated than CBaseEntity::SetOwnerEntity.

---

CBaseEntity::SetSolid (Wrapper to CCollisionProperty::SetSolid):
https://git.byr.ac.cn/Gaojianli/cstrike15_src/-/blob/ef809f5382e4264413aaff3aec266b6ac937c3ab/game/shared/collisionproperty.cpp#L531
CBaseEntity::SetSolidFlags (Wrapper to CCollisionProperty::SetSolidFlags) : https://git.byr.ac.cn/Gaojianli/cstrike15_src/-/blob/ef809f5382e4264413aaff3aec266b6ac937c3ab/game/shared/collisionproperty.cpp#L595.
CBaseEntity::SetMoveType: https://git.byr.ac.cn/Gaojianli/cstrike15_src/-/blob/ef809f5382e4264413aaff3aec266b6ac937c3ab/game/server/baseentity.cpp#L3969

---

To be perfect and avoid any problem with "CollisionRulesChanged", we would need to call directly these virtual functions, but that would mean maintaining 5 signatures:
CBaseEntity::SetCollisionGroup
CBaseEntity::SetSolidFlags
CBaseEntity::SetSolid
CBaseEntity::SetMoveType
CBaseEntity::SetOwnerEntity

With their associated enums:
enum CollisionGroup
enum SolidFlag
enum SolidType
enum MoveType
enum MoveCollide

[Kenzzer]

Not necessarily applying my method. Only the vtable offset (not signature) for CBaseEntity::SetOwnerEntity would need to be maintained. And the signature for CBaseEntity::SetCollisionGroup no longer required.

Secondly, if we need a better method for SetMoveType than the current SM's implementation, we can fallback on the IServerTools interface. Which is already available through Sourcemod.
See valve sdk2013
CServerTools::SetMoveType calls CBaseEntity::SetMoveType

So we would need to maintain 2 signatures (not 5)
CBaseEntity::SetSolid
CBaseEntity::SetSolidFlags
And 1 vtable offset
CBaseEntity::SetOwnerEntity

CBaseEntity::SetCollisionGroup & CBaseEntity::SetMoveType can be remade in code. We don't need to maintain signatures for those.

Finally, I don't know if plugins really need access to CBaseEntity::SetSolidFlags & CBaseEntity::SetSolid, do you have examples where it could come handy?

[einyux]

No, I was just tracking where CollisionRulesChanged() was called, and found SetSolid & SetSolidFlags. Don't know if issues have been detected with these ones, but looking at the last topic comment (https://forums.alliedmods.net/showpost.php?p=2247647&postcount=173), it looks like there's something with SetSolid.

By the way, I don't know if we should continue to talk here.

@ashort96 if possible, just rename the native to "SetEntityCollisionGroup(int entity, int collisiongroup)", and then let's discuss everything on an alliedmodders topic.

EDIT : Small mistake from me, but I said signatures while SetSolid & SetSolidFlags may be called with offset (Need to check the vtable list).

[KyleSanderson]

@Kenzzer if you're up for making a PR for the virtual solution I'm up for reviewing and/or co-authoring to help it get in. I agree that's the much better way, and we already do this with other functions by grabbing the function from 0 so there's precedence.

IServerTools is not available on all games, so while it's handy for new stuff, old games will still need a classic fallback.

[Kenzzer]

Alright, in that case I'll open a PR in the next couple of days!