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

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

Refactors the Free VIP Giveaway plugin and fixes the bugs found along the way. The default mode (sm_freevip_min_players 0) behaves as before. The round-based mode (>= 1) has intended behavior changes, listed below. master is merged in (#31).

Changes

sm_freevip_min_players: -1 disables Free VIP, 0 = no minimum (given on connect), N = minimum players (checked on round start and on connect).
IsFreeVIPOn() / FreeVIP_IsFreeVIPOn(): true for the whole event window unless disabled (-1), no longer only in mode 0. sm_freevip and the hostname prefix follow it.
ConVar values cached in globals and kept up to date with change hooks.
Player count moved to GetRealPlayersOnServer(): humans in game only (SourceTV and bots excluded).
Event_RoundStart and Command_FreeVIP use early returns and the same status messages.
VIP_OnClientLoaded(int client, bool isVIP) now matches the VIP Core forward declaration.
Late load: players already in game are checked in OnConfigsExecuted(), once the plugin config is loaded. master did it in OnPluginStart() with the default timestamps, which reset the "extended" cookie of every VIP in game (allowing a second extension).
Removed: the cstrike include and the manual ADMFLAG_CUSTOM1. VIP_SourcemodFlags already sets that flag when a VIP is added and clears it on removal; the manual one was never cleared.

Fixes

Temporary VIP was never removed (round start below the minimum, disconnect): VIP_GetClientVIPGroup() writes the client's group into the buffer and returns true for any VIP, so the "test group" check skipped every player. HasFreeVIP() now matches temporary VIP (VIP_GetClientID() == -1) of the Free VIP group. Test VIP and VIP given by other plugins are kept, and the sm_vip_test_group lookup is gone (VIP_Test is optional, FindConVar returned null without it).
Cookie access is CookieAccess_Protected: with Public, players could reset freevip_cookie through sm_cookies and get their VIP extended again.
Hostname prefix no longer added when Free VIP is disabled.
VIP group buffer is 64 chars, like VIP Core (was 16).

Behavior changes in round-based mode (`min_players >= 1`)

Spectators are no longer excluded: they get Free VIP at round start and keep it (intended).
Players joining while the minimum is met get Free VIP right away instead of at the next round.
Existing VIPs get the remaining event time added to their VIP, once per event (cookie), as in mode 0. Before, this only happened in mode 0.
Bots no longer count toward the minimum.
FreeVIP_IsFreeVIPOn() returns true during the event window in this mode too.

About the Copilot review

VIP_OnClientLoaded signature: VIP Core declares forward void VIP_OnClientLoaded(int iClient, bool bIsVIP);, so two parameters is correct.
Cookie access: Protected is kept on purpose (see Fixes).
ADMFLAG_CUSTOM1: handled by VIP_SourcemodFlags.
Spectator checks: removed on purpose.
Timers, sm_reloadvips: both removed.
GiveVIP() / SetVIP() without IsFreeVIPOn(): private helpers, only called from VIP_OnClientLoaded() after that check.

Validation

Builds with spcomp 1.12 (git7259), no warnings.
To test on a server:
- Mode 0: a non-VIP gets Free VIP on connect, a VIP gets extended once.
- Mode N: below / above the minimum at round start; a Test VIP is kept below the minimum.
- -1: nothing given, no hostname prefix, sm_freevip says disabled.
- Late load (sm plugins reload) during an event.

Version 2.2.0.

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

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

+	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

+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

 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


-	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);

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

+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.

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

 		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

 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.

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


-		// 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);

Timer_ReloadVIPs ran a full VIP DB reload 20.5s after plugin start, right after CheckALLVIPPlayers grants temporary (non-DB) Free VIP to already-connected players at 2s. The reload could wipe those in-memory grants. Late-load sync is already handled by CheckALLVIPPlayers, so the extra reload timer served no remaining purpose.

- The removal of temporary VIP never ran: VIP_GetClientVIPGroup() writes the client's group into the buffer and returns true for every VIP, so the test group check always skipped the player (and overwrote the cached test group). HasFreeVIP() now matches temporary VIP of the Free VIP group: Test VIP and VIP given by other plugins are left alone, and sm_vip_test_group (VIP_Test, optional: FindConVar could return null) is no longer needed. - With a minimum set, temporary VIP is only given on connect when enough players are in game; otherwise round start removed it right away. - Player count: humans in game only, bots no longer count. - Late load: check players in OnConfigsExecuted instead of a 2 s timer, once the config values are loaded. - No hostname prefix when Free VIP is disabled (-1). - ConVar methodmaps like the rest of the file, group buffer of 64 like VIP Core, clearer sm_freevip_min_players description, Copilot instructions updated.

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

Rushaway and others added 2 commits February 14, 2026 15:42

Stop VIP timers after execution

2c5c750

Rushaway added the enhancement New feature or request label Oct 30, 2026

Rushaway added 2 commits October 30, 2026 16:09

Merge branch 'master' into refactor

fe42179

Rushaway changed the title ~~refactor(core): simplify FreeVIP flow~~ refactor(core): simplify FreeVIP flow, fix temporary VIP removal Oct 30, 2026

		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 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changes

Fixes

Behavior changes in round-based mode (min_players >= 1)

About the Copilot review

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

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Rushaway commented Feb 14, 2026 •

edited

Loading

Behavior changes in round-based mode (`min_players >= 1`)