Refactor script hook system

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 @@ public:
 
 	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 @@ public:
 
 	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 @@ public:
 
 	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 @@ public:
 
 	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))
+	if ( !cached )
-		cached = hook.CanRunInScope(m_ScriptScope);
+	{
+		if ( hook.CanRunInScope( m_ScriptScope ) )
+		{
+			cached = hook.m_hFunc;
+		}
+	}
 
 	if (cached)
 	{

sp/src/public/vscript/ivscript.h

@@ -885,9 +885,8 @@ public:
 	//--------------------------------------------------------
 	// Hooks
 	//--------------------------------------------------------
-	virtual bool ScopeIsHooked( HSCRIPT hScope, const char *pszEventName ) = 0;
+	virtual HSCRIPT LookupHookFunction( const char *pszEventName, HSCRIPT hScope ) = 0;
-	virtual HSCRIPT LookupHookFunction( const char *pszEventName, HSCRIPT hScope, bool &bLegacy ) = 0;
+	virtual ScriptStatus_t ExecuteHookFunction( HSCRIPT hFunction, const char *pszEventName, ScriptVariant_t *pArgs, int nArgs, ScriptVariant_t *pReturn, HSCRIPT hScope, bool bWait ) = 0;
-	virtual ScriptStatus_t ExecuteHookFunction( const char *pszEventName, HSCRIPT hFunction, 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,51 +1608,72 @@ 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
+	ScriptHook_t() :
-	HSCRIPT CanRunInScope( HSCRIPT hScope )
+		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
+#ifdef _DEBUG
-	bool CheckFuncValid( HSCRIPT hFunc )
+	//
+	// 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?
+		Assert( hScope.IsInitialized() );
-		if (hFunc)
+		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;
+			// Legacy support if the new system is not being used
-			return true;
+			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
+		// Call() should not be called without CanRunInScope() check first, it caches m_hFunc for legacy support
-		if (!m_hFunc && !CanRunInScope(hScope))
+		Assert( CanRunInScope( hScope ) );
-			return false;
+
 		// 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++)
@@ -1672,19 +1681,19 @@ struct ScriptHook_t
 				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 );
+			ScriptStatus_t status = g_pScriptVM->ExecuteHookFunction( m_hFunc, m_desc.m_pszScriptName, pArgs, m_desc.m_Parameters.Count(), pReturn, hScope, true );
-			if (bRelease)
+
+			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 @@ public:
 	//--------------------------------------------------------
 	// Hooks
 	//--------------------------------------------------------
-	virtual bool ScopeIsHooked( HSCRIPT hScope, const char *pszEventName ) override;
+	virtual HSCRIPT LookupHookFunction( const char *pszEventName, HSCRIPT hScope ) override;
-	virtual HSCRIPT LookupHookFunction( const char *pszEventName, HSCRIPT hScope, bool &bLegacy ) override;
+	virtual ScriptStatus_t ExecuteHookFunction( HSCRIPT hFunction, const char *pszEventName, ScriptVariant_t *pArgs, int nArgs, ScriptVariant_t *pReturn, HSCRIPT hScope, bool bWait ) override;
-	virtual ScriptStatus_t ExecuteHookFunction( const char *pszEventName, HSCRIPT hFunction, ScriptVariant_t *pArgs, int nArgs, ScriptVariant_t *pReturn, HSCRIPT hScope, bool bWait ) override;
 
 	//--------------------------------------------------------
 	// External functions
@@ -2348,55 +2347,38 @@ 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)
 {
+	SquirrelSafeCheck safeCheck(vm_);
+
 	// For now, assume null scope (which is used for global hooks) is always hooked
-	if (!hScope)
+	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)))
 	{
-		sq_pop(vm_, 3);
+		Assert( hScope != INVALID_HSCRIPT );
-		return 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);
+
+		SQBool val;
+		if (SQ_FAILED(sq_getbool(vm_, -1, &val)))
+		{
+			sq_pop(vm_, 3);
+			return nullptr;
+		}
+
+		sq_pop(vm_, 4);
+
+		if (!val)
+			return nullptr;
 	}
 
-	sq_pop(vm_, 4);
-	return val ? true : false;
-}
-
-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;
-	}
-
-	if (!ScopeIsHooked(hScope, pszEventName))
-		return nullptr;
-
-	SquirrelSafeCheck safeCheck(vm_);
-
 	sq_pushroottable(vm_);
 	sq_pushstring(vm_, "Hooks", -1);
 	sq_get(vm_, -2);
@@ -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)
+	SquirrelSafeCheck safeCheck(vm_);
-		return SCRIPT_ERROR;
 
 	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]);

sp/src/vscript/vscript_squirrel.nut

@@ -152,114 +152,144 @@ Hooks <-
 	// table, string, closure, string
 	function Add( scope, event, callback, context )
 	{
+		switch ( typeof scope )
+		{
+			case "table":
+			case "instance":
+			case "class":
+				break;
+			default:
+				throw "invalid scope param";
+		}
+
+		if ( typeof event != "string" )
+			throw "invalid event param";
+
 		if ( typeof callback != "function" )
-			throw "invalid callback param"
+			throw "invalid callback param";
 
-		if ( !(scope in s_List) )
+		if ( typeof context != "string" )
-			s_List[scope] <- {}
+			throw "invalid context param";
 
-		local t = s_List[scope]
+		if ( !(event in s_List) )
+			s_List[event] <- {};
 
-		if ( !(event in t) )
+		local t = s_List[event];
-			t[event] <- {}
 
-		t[event][context] <- callback
+		if ( !(scope in t) )
+			t[scope] <- {};
+
+		t[scope][context] <- callback;
 	}
 
-	function Remove( context, event = null )
+	function Remove( event, context )
 	{
+		local rem;
+
 		if ( event )
 		{
-			foreach( k,scope in s_List )
+			if ( event in s_List )
 			{
-				if ( event in scope )
+				foreach ( scope, ctx in s_List[event] )
 				{
-					local t = scope[event]
+					if ( context in ctx )
-					if ( context in t )
 					{
-						delete t[context]
+						delete ctx[context];
 					}
 
-					// cleanup?
+					if ( !ctx.len() )
-					if ( !t.len() )
+					{
-						delete scope[event]
+						if ( !rem )
+							rem = [];
+						rem.append( event );
+						rem.append( scope );
+					}
 				}
-
-				// cleanup?
-				if ( !scope.len() )
-					delete s_List[k]
 			}
 		}
 		else
 		{
-			foreach( k,scope in s_List )
+			foreach ( ev, t in s_List )
 			{
-				foreach( kk,ev in scope )
+				foreach ( scope, ctx in t )
 				{
-					if ( context in ev )
+					if ( context in ctx )
 					{
-						delete ev[context]
+						delete ctx[context];
 					}
 
-					// cleanup?
+					if ( !ctx.len() )
-					if ( !ev.len() )
-						delete scope[kk]
-				}
-
-				// cleanup?
-				if ( !scope.len() )
-					delete s_List[k]
-			}
-		}
-	}
-
-	function Call( scope, event, ... )
-	{
-		local firstReturn
-
-		// global hook; call all scopes
-		if ( !scope )
-		{
-			vargv.insert( 0, null )
-			foreach( sc,t in s_List )
-			{
-				if ( event in t )
-				{
-					vargv[0] = sc
-					foreach( context, callback in t[event] )
 					{
-						//printf( "(%.4f) Calling hook '%s' of context '%s' in static iteration\n", Time(), event, context )
+						if ( !rem )
-
+							rem = [];
-						local curReturn = callback.acall(vargv)
+						rem.append( ev );
-						if (firstReturn == null)
+						rem.append( scope );
-							firstReturn = curReturn
 					}
 				}
 			}
 		}
-		else if ( scope in s_List )
-		{
-			local t = s_List[scope]
-			if ( event in t )
-			{
-				vargv.insert( 0, scope )
-				foreach( context, callback in t[event] )
-				{
-					//printf( "(%.4f) Calling hook '%s' of context '%s'\n", Time(), event, context )
 
-					local curReturn = callback.acall(vargv)
+		if ( rem )
-					if (firstReturn == null)
+		{
-						firstReturn = curReturn
+			local c = rem.len() - 1;
+			for ( local i = 0; i < c; i += 2 )
+			{
+				local ev = rem[i];
+				local scope = rem[i+1];
+
+				if ( !s_List[ev][scope].len() )
+					delete s_List[ev][scope];
+
+				if ( !s_List[ev].len() )
+					delete s_List[ev];
+			}
+		}
+	}
+
+	function Call( event, scope, ... )
+	{
+		local firstReturn;
+
+		if ( event in s_List )
+		{
+			vargv.insert( 0, scope );
+
+			local t = s_List[event];
+			if ( scope in t )
+			{
+				foreach ( fn in t[scope] )
+				{
+					//printf( "(%.4f) Calling hook %s:%s\n", Time(), event, context );
+
+					local r = fn.acall( vargv );
+					if ( firstReturn == null )
+						firstReturn = r;
+				}
+			}
+			else if ( !scope ) // global hook
+			{
+				foreach ( sc, ctx in t )
+				{
+					vargv[0] = sc;
+
+					foreach ( context, fn in ctx )
+					{
+						//printf( "(%.4f) Calling hook (g) %s:%s\n", Time(), event, context );
+
+						local r = fn.acall( vargv );
+						if ( firstReturn == null )
+							firstReturn = r;
+					}
 				}
 			}
 		}
 
-		return firstReturn
+		return firstReturn;
 	}
 
-	function ScopeHookedToEvent( scope, event )
+	function IsEventHookedInScope( event, scope )
 	{
-		return ( scope in s_List ) && ( event in s_List[scope] )
+		return ( event in s_List ) && ( scope in s_List[event] )
 	}
 }