Refactor script hook system

Author: samisalreadytaken

sp/src/game/client/c_baseentity.cpp

@@ -1353,7 +1353,7 @@ void C_BaseEntity::Term()
 	if ( m_hScriptInstance )
 	{
 #ifdef MAPBASE_VSCRIPT
-		if ( m_ScriptScope.IsInitialized() )
+		if ( m_ScriptScope.IsInitialized() && g_Hook_UpdateOnRemove.CanRunInScope( m_ScriptScope ) )
 		{
 			g_Hook_UpdateOnRemove.Call( m_ScriptScope, NULL, NULL );
 		}

sp/src/game/server/baseentity.cpp

@@ -2646,7 +2646,7 @@ void CBaseEntity::UpdateOnRemove( void )
 	if ( m_hScriptInstance )
 	{
 #ifdef MAPBASE_VSCRIPT
-		if (m_ScriptScope.IsInitialized())
+		if ( m_ScriptScope.IsInitialized() && g_Hook_UpdateOnRemove.CanRunInScope( m_ScriptScope ) )
 		{
 			g_Hook_UpdateOnRemove.Call( m_ScriptScope, NULL, NULL );
 		}

sp/src/game/server/filters.cpp

@@ -2157,7 +2157,7 @@ class CFilterScript : public CBaseFilter
 public:
 	bool PassesFilterImpl( CBaseEntity *pCaller, CBaseEntity *pEntity )
 	{
-		if (m_ScriptScope.IsInitialized())
+		if ( m_ScriptScope.IsInitialized() && g_Hook_PassesFilter.CanRunInScope( m_ScriptScope ) )
 		{
 			// caller, activator
 			ScriptVariant_t functionReturn;
@@ -2176,7 +2176,7 @@ class CFilterScript : public CBaseFilter
 
 	bool PassesDamageFilterImpl( CBaseEntity *pCaller, const CTakeDamageInfo &info )
 	{
-		if (m_ScriptScope.IsInitialized())
+		if ( m_ScriptScope.IsInitialized() && g_Hook_PassesDamageFilter.CanRunInScope( m_ScriptScope ) )
 		{
 			HSCRIPT pInfo = g_pScriptVM->RegisterInstance( const_cast<CTakeDamageInfo*>(&info) );
 
@@ -2201,7 +2201,7 @@ class CFilterScript : public CBaseFilter
 
 	bool PassesFinalDamageFilter( CBaseEntity *pCaller, const CTakeDamageInfo &info )
 	{
-		if (m_ScriptScope.IsInitialized())
+		if ( m_ScriptScope.IsInitialized() && g_Hook_PassesFinalDamageFilter.CanRunInScope( m_ScriptScope ) )
 		{
 			HSCRIPT pInfo = g_pScriptVM->RegisterInstance( const_cast<CTakeDamageInfo*>(&info) );
 
@@ -2225,7 +2225,7 @@ class CFilterScript : public CBaseFilter
 
 	bool BloodAllowed( CBaseEntity *pCaller, const CTakeDamageInfo &info )
 	{
-		if (m_ScriptScope.IsInitialized())
+		if ( m_ScriptScope.IsInitialized() && g_Hook_BloodAllowed.CanRunInScope( m_ScriptScope ) )
 		{
 			HSCRIPT pInfo = g_pScriptVM->RegisterInstance( const_cast<CTakeDamageInfo*>(&info) );
 
@@ -2249,7 +2249,7 @@ class CFilterScript : public CBaseFilter
 
 	bool DamageMod( CBaseEntity *pCaller, CTakeDamageInfo &info )
 	{
-		if (m_ScriptScope.IsInitialized())
+		if ( m_ScriptScope.IsInitialized() && g_Hook_DamageMod.CanRunInScope( m_ScriptScope ) )
 		{
 			HSCRIPT pInfo = g_pScriptVM->RegisterInstance( &info );
 

sp/src/game/shared/baseentity_shared.cpp

@@ -1613,7 +1613,7 @@ typedef CTraceFilterSimpleList CBulletsTraceFilter;
 void CBaseEntity::FireBullets( const FireBulletsInfo_t &info )
 {
 #if defined(MAPBASE_VSCRIPT) && defined(GAME_DLL)
-	if (m_ScriptScope.IsInitialized())
+	if ( m_ScriptScope.IsInitialized() && g_Hook_FireBullets.CanRunInScope( m_ScriptScope ) )
 	{
 		HSCRIPT hInfo = g_pScriptVM->RegisterInstance( const_cast<FireBulletsInfo_t*>(&info) );
 

sp/src/game/shared/mapbase/weapon_custom_scripted.cpp

@@ -177,8 +177,13 @@ CWeaponCustomScripted::CWeaponCustomScripted()
 
 bool CWeaponCustomScripted::RunWeaponHook( ScriptHook_t &hook, HSCRIPT &cached, ScriptVariant_t *retVal, ScriptVariant_t *pArgs )
 {
-	if (!hook.CheckFuncValid(cached))
-		cached = hook.CanRunInScope(m_ScriptScope);
+	if ( !cached )
+	{
+		if ( hook.CanRunInScope( m_ScriptScope ) )
+		{
+			cached = hook.m_hFunc;
+		}
+	}
 
 	if (cached)
 	{

sp/src/public/vscript/ivscript.h

@@ -885,9 +885,8 @@ class IScriptVM
 	//--------------------------------------------------------
 	// Hooks
 	//--------------------------------------------------------
-	virtual bool ScopeIsHooked( HSCRIPT hScope, const char *pszEventName ) = 0;
-	virtual HSCRIPT LookupHookFunction( const char *pszEventName, HSCRIPT hScope, bool &bLegacy ) = 0;
-	virtual ScriptStatus_t ExecuteHookFunction( const char *pszEventName, HSCRIPT hFunction, ScriptVariant_t *pArgs, int nArgs, ScriptVariant_t *pReturn, HSCRIPT hScope, bool bWait ) = 0;
+	virtual HSCRIPT LookupHookFunction( const char *pszEventName, HSCRIPT hScope ) = 0;
+	virtual ScriptStatus_t ExecuteHookFunction( HSCRIPT hFunction, const char *pszEventName, ScriptVariant_t *pArgs, int nArgs, ScriptVariant_t *pReturn, HSCRIPT hScope, bool bWait ) = 0;
 #endif
 
 	//--------------------------------------------------------
@@ -1593,17 +1592,6 @@ typedef CScriptScopeT<> CScriptScope;
 // 
 // This was previously done with raw function lookups, but Mapbase adds more and
 // it's hard to keep track of them without proper standards or documentation.
-// 
-// At the moment, this simply plugs hook documentation into VScript and maintains
-// the same function lookup method on the inside, but it's intended to be open for
-// more complex hook mechanisms with proper parameters in the future.
-// 
-// For example:
-// 
-//	if (m_hFunc)
-//	{
-//		g_pScriptVM->ExecuteFunction( m_Func, pArgs, m_desc.m_Parameters.Count(), pReturn, m_ScriptScope, true );
-//	}
 //-----------------------------------------------------------------------------
 struct ScriptHook_t
 {
@@ -1620,71 +1608,92 @@ struct ScriptHook_t
 
 	// -----------------------------------------------------------------
 
-	// Cached for when CanRunInScope() is called before Call()
+	// Only valid between CanRunInScope() and Call()
 	HSCRIPT m_hFunc;
 	bool m_bLegacy;
 
-	// Checks if there's a function of this name which would run in this scope
-	HSCRIPT CanRunInScope( HSCRIPT hScope )
+	ScriptHook_t() :
+		m_bLegacy(false),
+		m_hFunc(NULL)
 	{
-		extern IScriptVM *g_pScriptVM;
-		m_hFunc = g_pScriptVM->LookupHookFunction( m_desc.m_pszScriptName, hScope, m_bLegacy );
-		return m_hFunc;
 	}
 
-	// Checks if an existing func can be used
-	bool CheckFuncValid( HSCRIPT hFunc )
+#ifdef _DEBUG
+	//
+	// An uninitialised script scope will pass as null scope which is considered a valid hook scope (global hook)
+	// This should catch CanRunInScope() calls without CScriptScope::IsInitalised() checks first.
+	//
+	bool CanRunInScope( CScriptScope &hScope )
 	{
-		// TODO: Better crtieria for this?
-		if (hFunc)
+		Assert( hScope.IsInitialized() );
+		return hScope.IsInitialized() && CanRunInScope( (HSCRIPT)hScope );
+	}
+#endif
+
+	// Checks if there's a function of this name which would run in this scope
+	bool CanRunInScope( HSCRIPT hScope )
+	{
+		extern IScriptVM *g_pScriptVM;
+
+		// Check the hook system first to make sure an unintended function in the script scope does not cancel out all script hooks.
+		m_hFunc = g_pScriptVM->LookupHookFunction( m_desc.m_pszScriptName, hScope );
+		if ( !m_hFunc )
 		{
-			m_hFunc = hFunc;
-			return true;
+			// Legacy support if the new system is not being used
+			m_hFunc = g_pScriptVM->LookupFunction( m_desc.m_pszScriptName, hScope );
+			m_bLegacy = true;
 		}
-		return false;
+		else
+		{
+			m_bLegacy = false;
+		}
+
+		return !!m_hFunc;
 	}
 
 	// Call the function
+	// NOTE: `bRelease` only exists for weapon_custom_scripted legacy script func caching
 	bool Call( HSCRIPT hScope, ScriptVariant_t *pReturn, ScriptVariant_t *pArgs, bool bRelease = true )
 	{
 		extern IScriptVM *g_pScriptVM;
 
-		// Make sure we have a function in this scope
-		if (!m_hFunc && !CanRunInScope(hScope))
-			return false;
+		// Call() should not be called without CanRunInScope() check first, it caches m_hFunc for legacy support
+		Assert( CanRunInScope( hScope ) );
+
 		// Legacy
-		else if (m_bLegacy)
+		if ( m_bLegacy )
 		{
+			Assert( m_hFunc );
+
 			for (int i = 0; i < m_desc.m_Parameters.Count(); i++)
 			{
 				g_pScriptVM->SetValue( m_pszParameterNames[i], pArgs[i] );
 			}
 
-			g_pScriptVM->ExecuteFunction( m_hFunc, NULL, 0, pReturn, hScope, true );
+			ScriptStatus_t status = g_pScriptVM->ExecuteFunction( m_hFunc, NULL, 0, pReturn, hScope, true );
 
-			if (bRelease)
+			if ( bRelease )
 				g_pScriptVM->ReleaseFunction( m_hFunc );
-
 			m_hFunc = NULL;
 
 			for (int i = 0; i < m_desc.m_Parameters.Count(); i++)
 			{
 				g_pScriptVM->ClearValue( m_pszParameterNames[i] );
 			}
 
-			return true;
+			return status == SCRIPT_DONE;
 		}
 		// New Hook System
 		else
 		{
-			g_pScriptVM->ExecuteHookFunction( m_desc.m_pszScriptName, m_hFunc, pArgs, m_desc.m_Parameters.Count(), pReturn, hScope, true );
-			if (bRelease)
+			ScriptStatus_t status = g_pScriptVM->ExecuteHookFunction( m_hFunc, m_desc.m_pszScriptName, pArgs, m_desc.m_Parameters.Count(), pReturn, hScope, true );
+
+			if ( bRelease )
 				g_pScriptVM->ReleaseFunction( m_hFunc );
 			m_hFunc = NULL;
-			return true;
-		}
 
-		return false;
+			return status == SCRIPT_DONE;
+		}
 	}
 };
 #endif

sp/src/vscript/vscript_squirrel.cpp

@@ -188,9 +188,8 @@ class SquirrelVM : public IScriptVM
 	//--------------------------------------------------------
 	// Hooks
 	//--------------------------------------------------------
-	virtual bool ScopeIsHooked( HSCRIPT hScope, const char *pszEventName ) override;
-	virtual HSCRIPT LookupHookFunction( const char *pszEventName, HSCRIPT hScope, bool &bLegacy ) override;
-	virtual ScriptStatus_t ExecuteHookFunction( const char *pszEventName, HSCRIPT hFunction, ScriptVariant_t *pArgs, int nArgs, ScriptVariant_t *pReturn, HSCRIPT hScope, bool bWait ) override;
+	virtual HSCRIPT LookupHookFunction( const char *pszEventName, HSCRIPT hScope ) override;
+	virtual ScriptStatus_t ExecuteHookFunction( HSCRIPT hFunction, const char *pszEventName, ScriptVariant_t *pArgs, int nArgs, ScriptVariant_t *pReturn, HSCRIPT hScope, bool bWait ) override;
 
 	//--------------------------------------------------------
 	// External functions
@@ -2348,54 +2347,37 @@ ScriptStatus_t SquirrelVM::ExecuteFunction(HSCRIPT hFunction, ScriptVariant_t* p
 	return SCRIPT_DONE;
 }
 
-bool SquirrelVM::ScopeIsHooked( HSCRIPT hScope, const char *pszEventName )
+HSCRIPT SquirrelVM::LookupHookFunction(const char *pszEventName, HSCRIPT hScope)
 {
-	// For now, assume null scope (which is used for global hooks) is always hooked
-	if (!hScope)
-		return true;
-
 	SquirrelSafeCheck safeCheck(vm_);
 
-	Assert(hScope != INVALID_HSCRIPT);
-
-	sq_pushroottable(vm_);
-	sq_pushstring(vm_, "Hooks", -1);
-	sq_get(vm_, -2);
-	sq_pushstring(vm_, "ScopeHookedToEvent", -1);
-	sq_get(vm_, -2);
-	sq_push(vm_, -2);
-	sq_pushobject(vm_, *((HSQOBJECT*)hScope));
-	sq_pushstring(vm_, pszEventName, -1);
-	sq_call(vm_, 3, SQTrue, SQTrue);
-
-	SQBool val;
-	if (SQ_FAILED(sq_getbool(vm_, -1, &val)))
+	// For now, assume null scope (which is used for global hooks) is always hooked
+	if ( hScope )
 	{
-		sq_pop(vm_, 3);
-		return false;
-	}
+		Assert( hScope != INVALID_HSCRIPT );
 
-	sq_pop(vm_, 4);
-	return val ? true : false;
-}
+		sq_pushroottable(vm_);
+		sq_pushstring(vm_, "Hooks", -1);
+		sq_get(vm_, -2);
+		sq_pushstring(vm_, "IsEventHookedInScope", -1);
+		sq_get(vm_, -2);
+		sq_push(vm_, -2);
+		sq_pushstring(vm_, pszEventName, -1);
+		sq_pushobject(vm_, *((HSQOBJECT*)hScope));
+		sq_call(vm_, 3, SQTrue, SQTrue);
 
-HSCRIPT SquirrelVM::LookupHookFunction(const char *pszEventName, HSCRIPT hScope, bool &bLegacy)
-{
-	HSCRIPT hFunc = hScope ? LookupFunction( pszEventName, hScope ) : nullptr;
-	if (hFunc)
-	{
-		bLegacy = true;
-		return hFunc;
-	}
-	else
-	{
-		bLegacy = false;
-	}
+		SQBool val;
+		if (SQ_FAILED(sq_getbool(vm_, -1, &val)))
+		{
+			sq_pop(vm_, 3);
+			return nullptr;
+		}
 
-	if (!ScopeIsHooked(hScope, pszEventName))
-		return nullptr;
+		sq_pop(vm_, 4);
 
-	SquirrelSafeCheck safeCheck(vm_);
+		if (!val)
+			return nullptr;
+	}
 
 	sq_pushroottable(vm_);
 	sq_pushstring(vm_, "Hooks", -1);
@@ -2411,31 +2393,31 @@ HSCRIPT SquirrelVM::LookupHookFunction(const char *pszEventName, HSCRIPT hScope,
 
 	HSQOBJECT* pObj = new HSQOBJECT;
 	*pObj = obj;
+
 	return (HSCRIPT)pObj;
 }
 
-ScriptStatus_t SquirrelVM::ExecuteHookFunction(const char *pszEventName, HSCRIPT hFunction, ScriptVariant_t* pArgs, int nArgs, ScriptVariant_t* pReturn, HSCRIPT hScope, bool bWait)
+ScriptStatus_t SquirrelVM::ExecuteHookFunction(HSCRIPT hFunction, const char *pszEventName, ScriptVariant_t* pArgs, int nArgs, ScriptVariant_t* pReturn, HSCRIPT hScope, bool bWait)
 {
-	SquirrelSafeCheck safeCheck(vm_);
-	if (!hFunction)
+	if ( !hFunction )
 		return SCRIPT_ERROR;
 
-	if (hFunction == INVALID_HSCRIPT)
-		return SCRIPT_ERROR;
+	SquirrelSafeCheck safeCheck(vm_);
 
 	HSQOBJECT* pFunc = (HSQOBJECT*)hFunction;
 	sq_pushobject(vm_, *pFunc);
 
-	// TODO: Run in hook scope
+	// The call environment of the Hooks::Call function does not matter
+	// as the function does not access any member variables.
 	sq_pushroottable(vm_);
 
+	sq_pushstring(vm_, pszEventName, -1);
+
 	if (hScope)
 		sq_pushobject(vm_, *((HSQOBJECT*)hScope));
 	else
 		sq_pushnull(vm_); // global hook
 
-	sq_pushstring(vm_, pszEventName, -1);
-
 	for (int i = 0; i < nArgs; ++i)
 	{
 		PushVariant(vm_, pArgs[i]);