refactor(core): simplify FreeVIP flow by Rushaway · Pull Request #20 · srcdslab/sm-plugin-VIP_FreeVIPGiveaway

Rushaway · 2026-02-14T12:42:38Z

This PR refactors the Free VIP Giveaway plugin to improve readability and reduce duplicated logic, without changing intended behavior.

What changed

Extracted duplicated player counting logic into a shared helper (GetRealPlayersOnServer) to exclude SourceTV consistently.
Refactored Event_RoundStart with early returns for clearer control flow.
Simplified Command_FreeVIP flow and aligned status checks/messages with current logic.
Minor style cleanup for clearer decrement/readability (playersOnServer--).

Why

Reduce duplicated code.
Make control flow easier to follow and maintain.
Keep behavior consistent between round event handling and command status output.

Risk / Impact

Low risk: refactor-focused changes, no new feature added.

Validation

Verified no SourcePawn diagnostics/errors after refactor.
Manually reviewed enabled/disabled for:
- FreeVIP off
- Not enough players
- Enough players

Rushaway · 2026-02-14T12:43:27Z

Also fix: #1

Copilot

Pull request overview

This PR claims to be a "refactor-focused" change to improve readability and reduce duplicated logic without changing behavior. However, the changes introduce multiple critical bugs and significant behavior changes that contradict the stated goals.

Changes:

Removed cstrike include and extracted player counting logic to GetRealPlayersOnServer() helper
Replaced manual client loop in OnPluginStart with two new timers (CheckALLVIPPlayers and Timer_ReloadVIPs)
Refactored Event_RoundStart with early returns and removed spectator/team checking
Changed VIP_OnClientLoaded signature to add unused isVIP parameter
Added global variable caching for ConVars with change hooks
Bumped version from 2.1 to 3.0.0

addons/sourcemod/scripting/VIP_FreeVIPGiveaway.sp

Copilot · 2026-02-14T12:50:34Z

addons/sourcemod/scripting/VIP_FreeVIPGiveaway.sp

+	if (g_iFreeVIPStart > iTime || g_iFreeVIPEnd < iTime || g_iMinPlayers < 0)
 		return false;


The IsFreeVIPOn function now returns false when g_iMinPlayers is 0, but this fundamentally changes the plugin's behavior. Previously, when min_players was 0, the old code would skip round-based checking (line "if(minPlayers <= 0) return;") and rely on OnClientPostAdminCheck. Now IsFreeVIPOn returns true when g_iMinPlayers is 0, but Event_RoundStart will execute and potentially give VIP even with 0 players. The old logic was: "0 = always on via callback, >= 1 = round-based checking". The new logic treats 0 differently. This is a behavior change, not just a refactor.

Copilot · 2026-02-14T12:50:34Z

addons/sourcemod/scripting/VIP_FreeVIPGiveaway.sp

+public Action Timer_ReloadVIPs(Handle timer)
+{
+	ServerCommand("sm_reloadvips");
+	return Plugin_Continue;


Timer_ReloadVIPs executes a server command "sm_reloadvips" 20.5 seconds after plugin start. This appears to be debugging code that was accidentally left in. There's no indication in the PR description or code comments why this timer is needed. If this is intentional, it should be documented; otherwise, it should be removed.

Copilot · 2026-02-14T12:50:35Z

addons/sourcemod/scripting/VIP_FreeVIPGiveaway.sp

 void GiveVIP(int client)
 {
-	if(!IsFreeVIPOn())
-		return;
-
-	if(client < 1)
+	if (client < 1)
 		return;

-	if(VIP_IsClientVIP(client))
+	if (VIP_IsClientVIP(client))
 	{
 		SetVIP(client);
 		return;
 	}
-
-	char vipGroup[16];
-	g_Cvar_VIPGroup.GetString(vipGroup, sizeof(vipGroup));
-	VIP_GiveClientVIP(_, client, 0, vipGroup, false);
-	/* Give client the vip flags */
-	int flags = GetUserFlagBits(client);
-	if(!(flags & ADMFLAG_CUSTOM1))
-		SetUserFlagBits(client, flags | ADMFLAG_CUSTOM1);
+
+	VIP_GiveClientVIP(_, client, 0, g_sVIPGroup, false);
 }


The GiveVIP and SetVIP functions had their IsFreeVIPOn checks removed. Previously these functions would early-return if FreeVIP was not active. Now they will execute regardless of the FreeVIP status. While VIP_OnClientLoaded checks IsFreeVIPOn before calling GiveVIP, if these functions are called from elsewhere or in the future, they could grant VIP when the feature is disabled.

Copilot · 2026-02-14T12:50:35Z

addons/sourcemod/scripting/VIP_FreeVIPGiveaway.sp


-	g_hCookie = new Cookie("freevip_cookie", "Cookie to know who got his vip extended", CookieAccess_Public);
-	
+	g_hCookie = new Cookie("freevip_cookie", "Cookie to know who got his vip extended", CookieAccess_Protected);


The cookie access level was changed from CookieAccess_Public to CookieAccess_Protected without explanation in the PR description. This changes who can access the cookie data and could break external integrations or admin tools that previously relied on public access. If this change is intentional, it should be documented in the PR.

Suggested change

g_hCookie = new Cookie("freevip_cookie", "Cookie to know who got his vip extended", CookieAccess_Protected);

g_hCookie = new Cookie("freevip_cookie", "Cookie to know who got his vip extended", CookieAccess_Public);

addons/sourcemod/scripting/VIP_FreeVIPGiveaway.sp

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 11 comments.

Comments suppressed due to low confidence (1)

addons/sourcemod/scripting/VIP_FreeVIPGiveaway.sp:249

Similar to GiveVIP, the SetVIP function removed its IsFreeVIPOn() check at the start. This means it relies on the caller to verify that FreeVIP is enabled before calling. While the current caller (GiveVIP) is only called from VIP_OnClientLoaded which does check IsFreeVIPOn(), removing this defensive check reduces the function's robustness.

void SetVIP(int client)
{
	if (client < 1)
		return;

	if (!AreClientCookiesCached(client))
		return;

	bool canGetVIP;
	char cookieValue[6];
	g_hCookie.Get(client, cookieValue, sizeof(cookieValue));
	if (StrEqual(cookieValue, "true"))
		canGetVIP = false;
	else
		canGetVIP = true;

	if (!VIP_IsClientVIP(client) || VIP_GetClientID(client) == -1 || VIP_GetClientAccessTime(client) == 0)
		return;

	if (!canGetVIP)
		return;

	int seconds = (g_iFreeVIPEnd - GetTime());
	int originalExpireTimeStamp = VIP_GetClientAccessTime(client);
	int newExpireTimeStamp = (originalExpireTimeStamp + seconds);
	VIP_SetClientAccessTime(client, newExpireTimeStamp, true);
	g_hCookie.Set(client, "true");
	return;
}

Copilot · 2026-02-14T13:44:31Z

addons/sourcemod/scripting/VIP_FreeVIPGiveaway.sp

+stock int GetRealPlayersOnServer()
+{
+	int playersOnServer = GetClientCount();
+	for (int i = 1; i <= MaxClients; i++)
+	{
+		if (!IsClientConnected(i))
+			continue;
+
+		if (IsClientSourceTV(i))
+			playersOnServer--;
+	}
+
+	return playersOnServer;
+}


The GetRealPlayersOnServer function uses GetClientCount() which returns the number of connected clients, but then only checks IsClientConnected(i) in the loop. This is redundant - if GetClientCount() already gives connected clients, the IsClientConnected check inside the loop should always pass for valid client indices. Consider using IsClientInGame(i) instead to count only active players, or clarify the intent of this counting logic.

addons/sourcemod/scripting/VIP_FreeVIPGiveaway.sp

Copilot · 2026-02-14T13:44:31Z

addons/sourcemod/scripting/VIP_FreeVIPGiveaway.sp

 		return;

-	if(freeVipEnd < GetTime())
+	if (g_iMinPlayers <= 0)


The IsFreeVIPOn() function now checks if g_iMinPlayers < 0 to determine if FreeVIP is disabled. However, Event_RoundStart has a separate check at line 118 that returns early if g_iMinPlayers <= 0. This creates inconsistent behavior: IsFreeVIPOn() will return true when g_iMinPlayers == 0, but Event_RoundStart will not process VIP assignments in that case. The original code's logic for minPlayers == 0 was documented in the ConVar description as "0 = OnClientPostAdminCheck" (meaning VIP should be given on client connection, not in rounds), but this behavior appears to be broken now.

Suggested change

if (g_iMinPlayers <= 0)

if (g_iMinPlayers < 0)

Copilot · 2026-02-14T13:44:31Z

addons/sourcemod/scripting/VIP_FreeVIPGiveaway.sp

 Action Command_FreeVIP(int client, int args)
 {
-	int playersOnServer = GetClientCount();
-	for(int i = 1; i <= MaxClients; i++)
-	{
-		if(i <= 0 || i > MaxClients || !IsClientConnected(i))
-			continue;
-
-		if (IsClientSourceTV(i))
-			playersOnServer = playersOnServer  - 1; // -1 cuz of sourcetv
-	}
-	int minPlayers = g_Cvar_MinPlayers.IntValue;
-	int freeVIPStart = g_Cvar_FreeVIPStart.IntValue;
-	int freeVipEnd = g_Cvar_FreeVIPEnd.IntValue;
-
-	if(freeVIPStart > GetTime() || freeVipEnd < GetTime())
+	if (!IsFreeVIPOn())
 	{
 		CReplyToCommand(client, "{green}[SM] {pink}Free VIP Giveaway {default}is {red}disabled{default}.");
 		return Plugin_Handled;
 	}

-	if (playersOnServer >= minPlayers)
+	int playersOnServer = GetRealPlayersOnServer();
+	if (playersOnServer < g_iMinPlayers)
 	{
-		CReplyToCommand(client, "{green}[SM] {pink}Free VIP Giveaway {default}is {green}enabled{default}.");
+		CReplyToCommand(client, "{green}[SM] {pink}Free VIP Giveaway {default}is {red}disabled{default}.");
+		CReplyToCommand(client, "{pink}Players on: {green}%d {pink}| Players required: {green}%d {pink}| Players needed: {red}+%d", playersOnServer, g_iMinPlayers, (g_iMinPlayers - playersOnServer));
 		return Plugin_Handled;
 	}
-	else
-	{
-		int playersNeeded = minPlayers - playersOnServer;

-		CReplyToCommand(client, "{green}[SM] {pink}Free VIP Giveaway {default}is {red}disabled{default}.\nPlayers on: {green}%d {default}| Players required: {green}%d {default}| Players needed: {green}+%d", playersOnServer, minPlayers, playersNeeded);
-		return Plugin_Handled;
-	}
+	CReplyToCommand(client, "{green}[SM] {pink}Free VIP Giveaway is {green}enabled{pink}. {green}Players got Free VIP.");
+	return Plugin_Handled;


The Command_FreeVIP logic appears inverted. When g_iMinPlayers is 0 (which according to old documentation meant "always on via OnClientPostAdminCheck"), the command will now show "disabled" because IsFreeVIPOn() returns true but then line 118 check in Event_RoundStart prevents VIP from being granted. This creates confusing user-facing behavior where the command reports one status but the actual functionality behaves differently.

addons/sourcemod/scripting/VIP_FreeVIPGiveaway.sp

Copilot · 2026-02-14T13:44:32Z

addons/sourcemod/scripting/VIP_FreeVIPGiveaway.sp


-		// push chat message
-		CPrintToChatAll("{green}[SM] {pink}Free VIP Giveaway {default}is {red}disabled{default}.\nPlayers on: {green}%d {default}| Players required: {green}%d {default}| Players needed: {green}+%d", playersOnServer, minPlayers, playersNeeded);
+		VIP_OnClientLoaded(i, (VIP_IsClientVIP(i)) ? true : false);


The isVIP parameter passed to VIP_OnClientLoaded at line 179 is computed but never used within the function. The parameter appears to be redundant since the function internally calls VIP_IsClientVIP(client) in GiveVIP anyway.

Suggested change

VIP_OnClientLoaded(i, (VIP_IsClientVIP(i)) ? true : false);

VIP_OnClientLoaded(i, false);

addons/sourcemod/scripting/VIP_FreeVIPGiveaway.sp

refactor(core): simplify FreeVIP flow

8cc9632

Copilot AI review requested due to automatic review settings February 14, 2026 12:42

Copilot started reviewing on behalf of Rushaway February 14, 2026 12:43 View session

Copilot AI reviewed Feb 14, 2026

View reviewed changes

minor adjustements

28a8b80

Rushaway requested a review from Copilot February 14, 2026 13:38

Copilot started reviewing on behalf of Rushaway February 14, 2026 13:38 View session

Copilot AI reviewed Feb 14, 2026

View reviewed changes

Stop VIP timers after execution

2c5c750

		if (g_iFreeVIPStart > iTime \|\| g_iFreeVIPEnd < iTime \|\| g_iMinPlayers < 0)
		return false;


		g_hCookie = new Cookie("freevip_cookie", "Cookie to know who got his vip extended", CookieAccess_Public);

		g_hCookie = new Cookie("freevip_cookie", "Cookie to know who got his vip extended", CookieAccess_Protected);

	VIP_OnClientLoaded(i, (VIP_IsClientVIP(i)) ? true : false);
	VIP_OnClientLoaded(i, false);

Conversation

Rushaway commented Feb 14, 2026

This PR refactors the Free VIP Giveaway plugin to improve readability and reduce duplicated logic, without changing intended behavior.

What changed

Why

Risk / Impact

Validation

Uh oh!

Rushaway commented Feb 14, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant