Reconcile the concept of Edict & Networkable across the codebase

[Kenzzer]

This fixes #1902 and potential crash in other natives.

What is this all about ?

Well, the scope of this PR mainly boils down to
CBaseEdict::GetNetworkable & IServerUnknown::GetNetworkable (CBaseEntity::GetNetworkable)

The former, is the edict that gives a pointer (that might or might not be valid) of the attached entity's networkable
The latter, is the entity/unknown that gives a pointer (that is always valid) of the attached entity's networkable

What are the benefits ?

The PR is mostly a speculative a fix for PR #1902 but everything points towards an unproperly setup Networkable's vtable. Which is strange, because Sourcemod retrives the networkable interface of any entity in a lot of places.
https://github.com/alliedmodders/sourcemod/search?q=GetNetworkable
But the key difference with all of them, and the gamerules native (as well tf2's objective ressource native, and another function). Is that they use the edict_t ptr, to retrieve the networkable interface. While everywhere else in the codebase, we go from edict ptr to unknown ptr (the cbaseentity) to the networkable interface.

This leads me to believe, the engine doesn't properly clean up the networkable ptr on its edict_t(s) which is assigned by the srcds.

Why is a core file modified ?

The goal is to keep the retrieval of the networkable interface consistent across all the codebase. That includes core files.
Furthermore, this also fixes a non logical message of the Set/GetEntProp natives
Edict %d (%d) is not networkable
Edicts are networkable by definition. We should have never gotten that far to begin with, if we started from an edict. But we don't, we start from a IServerUnknown. So we remove this useless check, whose error message never ever showed in sourcemode entire lifetime, I browsed the forums & google never found a post where a plugin maker asks about the error message.

# Why is there some null check removed on networkable ?

IServerUnknown is only ever implemented by CBaseEntity
IServerUnknown::GetNetworkable is implemented like so by CBaseEntity

inline IServerNetworkable *CBaseEntity::GetNetworkable()
{
	return &m_Network;
}

The ptr can never ever be invalid. Its memory is allocated along with CBaseEntity (IServerUnknown)

And it is always initialised.

CBaseEntity::CBaseEntity( bool bServerOnly )
{
	NetworkProp()->Init( this );

This statement is true, all the way back to the 2007 release of the source engine. I haven't/couldn't check older versions. But I'm confident.

Sourcemod itself is "guilty" of not null checking the networkable interface, when retrieving it from IServerUnknown

sourcemod/extensions/tf2/criticals.cpp

Lines 169 to 170 in 57a3863

	IServerNetworkable *pNetWeapon = ((IServerUnknown *)pWeapon)->GetNetworkable();
	ServerClass *pServerClass = pNetWeapon->GetServerClass();

sourcemod/extensions/sdkhooks/natives.cpp

Lines 234 to 237 in 51bba0b

	IServerNetworkable *pNet = pUnk->GetNetworkable();
	if (!UTIL_ContainsDataTable(pNet->GetServerClass()->m_pTable, "DT_BaseCombatWeapon"))
	return pContext->ThrowNativeError("Entity index %d is not a weapon", params[2]);

sourcemod/extensions/sdktools/vnatives.cpp

Lines 1675 to 1676 in 775d4f0

	ServerClass* pClass = ((IServerUnknown*)pEntity)->GetNetworkable()->GetServerClass();
	if (!FindNestedDataTable(pClass->m_pTable, "DT_BaseAnimating"))

sourcemod/extensions/sdktools/vnatives.cpp

Lines 1721 to 1722 in 775d4f0

	ServerClass* pClass = ((IServerUnknown*)pEntity)->GetNetworkable()->GetServerClass();
	if (!FindNestedDataTable(pClass->m_pTable, "DT_BaseAnimating"))

A no check, that I myself justified using the same arguments used in this PR
#1653 (comment)

sourcemod/extensions/cstrike/natives.cpp

Lines 255 to 260 in 536750b

	//Psychonic is awesome for this
	sm_sendprop_info_t spi;
	IServerUnknown *pUnk = (IServerUnknown *)pWeapon;
	IServerNetworkable *pNet = pUnk->GetNetworkable();
	if (!UTIL_FindDataTable(pNet->GetServerClass()->m_pTable, "DT_WeaponCSBase", &spi, 0))

Edited : After discussion, it has been decided null checks should not be removed. Instead null checks have been added where they had been missing.

This should be field tested

I haven't had the chance to try the changes myself. This is all speculative, but I'm confident in the research I've performed.
Attached to this post is a zip file containing the sourcemod installation compiled with the changes of this PR (linux), minus the pgsql extension + spcomp, which gave me troubles to be compiled because of zlib.
sourcemod.zip OUTDATED
sourcemod.zip OUTDATED
sourcemod.zip - Latest

[Kenzzer]

Rebuilt the changes on an older linux debian (buster), this time everything compiled. I also addressed a dumb logic change I made that would crash servers on map load.
sourcemod.zip

[Kenzzer]

So compiling Sourcemod with clang-3.8 didn't please a few extensions, that hack themselves into sourcemod. So I moved up to clang-11, still debian buster
sourcemod.zip
Also fully removed the "networkable" check inside the Set/GetEntProp natives. It never made any sense to begin with.

[KyleSanderson]

So compiling Sourcemod with clang-3.8 didn't please a few extensions, that hack themselves into sourcemod. So I moved up to clang-11, still debian buster sourcemod.zip Also fully removed the "networkable" check inside the Set/GetEntProp natives. It never made any sense to begin with.

Have you reviewed https://bugs.alliedmods.net/show_bug.cgi?id=5297 (and the child bug?).

[Kenzzer]

So compiling Sourcemod with clang-3.8 didn't please a few extensions, that hack themselves into sourcemod. So I moved up to clang-11, still debian buster sourcemod.zip Also fully removed the "networkable" check inside the Set/GetEntProp natives. It never made any sense to begin with.

Have you reviewed https://bugs.alliedmods.net/show_bug.cgi?id=5297 (and the child bug?).

Ty for sharing this. No I hadn't thought of checking this bug tracker.
But this bug ticket seems to corroborate the issue this PR raise.
https://hg.alliedmods.net/sourcemod-experimental/rev/13ea64fe5237

Instead of trusting the edict's networkable ptr, you instead directly go for the CBaseEntity

inline const char *	CBaseEdict::GetClassName() const
{
	if ( !m_pUnk )
		return "";
	return m_pNetworkable->GetClassName();
}

We address the same problem once again. Gamerules natives, retrieve the edict's networkable, but we should be retrieving the serverunknow's(CBaseEntity) networkable.

So I addressed that, and I also took the liberty of eliminating all the null checks against networkable interface. As they're artifacts of a bygone time when the interface was instead found through the edict.

Edit: @psychonic reviewed this through discord, and wants the null checks kept, in the event a new game ever so slightly deviate from the regular network behavior. So I'll add those checks back as well as adding them where they're missing

[Kenzzer]

New field test build ?

Sorry to the people that wish to field test this. I have no SM builds to provide atm, I'll upload one later today or tomorrow.

Null checks update

I've re-added the removed null checks, and I added new ones where they had been previously missing.
I've also added null checks around IServerNetworkable::GetServerClass because a few parts of SM already null checked it. I believe this is in scope of the PR still, as the ServerClass pointer is currently only retrieved through IServerNetworkable which this PR brings consistency for.

Examples of where ServerClass is currently null checked on master

sourcemod/extensions/tf2/criticals.cpp

Lines 170 to 175 in 250dc1b

	ServerClass *pServerClass = pNetWeapon->GetServerClass();
	if (!pServerClass)
	{
	g_pSM->LogError(myself, "Invalid server class on weapon.");
	RETURN_META_VALUE(MRES_IGNORED, false);
	}

Streamline code further ?

I also thought it might be a good idea to overload IGameHelpers::FindServerClass

sourcemod/public/IGameHelpers.h

Line 102 in 250dc1b

virtual ServerClass *FindServerClass(const char *classname) =0;

And introduce SeverClass* IGameHelpers::FindServerClass(CBaseEntity*), so that in the event a mod doesn't have a networkable interface for its entities, we can just create a ifdef SE_GAME in this function to add support. Without having to do that for all the natives in SourceMod that currently do

IServerNetworkable *pNet = pUnk->GetNetworkable();
if (!pNet)
{
	return;
}

ServerClass *pClass = pNet->GetServerClass();
if (!pClass)
{
	return;
}

to

ServerClass *pClass = gamehelpers/halflife2->GetServerClass(pUnk/pEntity);
if (!pClass)
{
	return;
}

The null status of IServerNetworkable, open discussion

I also wish to bring something new to this discussion. While rebrowsing source engine code, I remembered that IServerNetworkable was a return value of IEntityFactory, I therefore want to ask again. Do we want to continue staying on the side of safety and null check IServerUnknown::GetNetworkable still ?

abstract_class IEntityFactory
{
public:
	virtual IServerNetworkable *Create( const char *pClassName ) = 0;
	virtual void Destroy( IServerNetworkable *pNetworkable ) = 0;
	virtual size_t GetEntitySize() = 0;
};

[Kenzzer]

Decided to roll with the idea of overloading IGameHelpers::FindServerClass (let me know if I bumped the interface version correctly, first time I'm doing this).

In my opinion this does streamline the code much further, and overall improves understandability of the code in certain places. The retrieval of the IServerNetworkable was only ever used to ultimately call GetServerClass, so let's hide that away in the event a game breaks away from this "convention", it will be less code to maintain overall.

[Kenzzer]

The gamerules fix has been uploaded to my server for a week now, and the crash hasn't re-surfaced. Every other changes are either a merely extra null check, or the use of the new vtable entry of IGameHelpers.

I haven't noticed any regression or new bugs following those changes. Attached to this post is a build of the latest commit for those that wish to try it privately (tf2 only).
sourcemod_1.12_3e2520418a4c375c2a9040df4d63aa4d2bbfe152.tar.gz

[psychonic]

This isn't safe to add like this since it won't actually get added to the end of the vtable. (It will be next to the other overload). An extension compiled against v11 is going to have a bad time if they access this version. At minimum, IsVersionCompatible would need to be overridden to return false if a lower version is requested. If not wanting to break compatibility, that could be taken one step further to add an additional interface registration with a shim providing the previous version. Another alternative is just using a different name.

I'm not sure how much any of that is worth the addition.

[asherkin]

Just giving it a different name so it's simple compat seems like the way to go.

[psychonic]

That'd be fine, too.