MAJOR cleanup of Spectator set/unset

- G_AddPlayer now contains CL_ClearPlayer, G_DestroyParty, and playeringame set
- Instead of a nasty, complicated block in P_SpawnPlayer, externalise it into G_SpectatePlayerOnJoin
- All mid-game human player-to-spectator transitions are handled by P_SetPlayerSpectator, instead of lots of `spectator = true` and associated boilerplate
    - Simplifies Got_Teamchange MASSIVELY
        - Of course this is helped by also stripping back team change
    - This is called by P_KillPlayer, too
- P_KillPlayer no longer eats DMG_SPECTATOR when lightsnaking or exiting
- G_GametypeHasSpectators condition tidied

src/d_clisrv.c

@@ -3875,22 +3875,14 @@ static void Got_AddPlayer(UINT8 **p, INT32 playernum)
 
 	CONS_Debug(DBG_NETPLAY, "addplayer: %d %d\n", node, newplayernum);
 
-	{
-		// Clear player before joining, lest some things get set incorrectly
-		CL_ClearPlayer(newplayernum);
-		G_DestroyParty(newplayernum);
+	G_AddPlayer(newplayernum);
+	//G_SpectatePlayerOnJoin(newplayernum); -- caused desyncs in this spot :(
 
-		playeringame[newplayernum] = true;
-		G_AddPlayer(newplayernum);
-
-		if (newplayernum+1 > doomcom->numslots)
-			doomcom->numslots = (INT16)(newplayernum+1);
-	}
+	if (newplayernum+1 > doomcom->numslots)
+		doomcom->numslots = (INT16)(newplayernum+1);
 
 	newplayer = &players[newplayernum];
 
-	newplayer->jointime = 0;
-
 	READSTRINGN(*p, player_names[newplayernum], MAXPLAYERNAME);
 	READMEM(*p, players[newplayernum].public_key, PUBKEYLENGTH);
 	READMEM(*p, clientpowerlevels[newplayernum], sizeof(((serverplayer_t *)0)->powerlevels));

src/d_netcmd.c

@@ -1189,14 +1189,13 @@ static void Got_NameAndColor(UINT8 **cp, INT32 playernum)
 				&& LUA_HookTeamSwitch(&players[playernum], 0, false, false, false)) // fiiiine, lua can except it
 			{
 				P_DamageMobj(players[playernum].mo, NULL, NULL, 1, DMG_SPECTATOR);
-				players[playernum].playerstate = PST_REBORN;
 
-				players[playernum].pflags &= ~PF_WANTSTOJOIN;
-				players[playernum].spectator = true;
+				if (players[i].spectator)
+				{
+					HU_AddChatText(va("\x82*%s became a spectator.", player_names[playernum]), false);
 
-				HU_AddChatText(va("\x82*%s became a spectator.", player_names[playernum]), false);
-
-				FinalisePlaystateChange(playernum);
+					FinalisePlaystateChange(playernum);
+				}
 			}
 		}
 	}
@@ -3448,6 +3447,22 @@ static void Command_ServerTeamChange_f(void)
 	SendNetXCmd(XD_TEAMCHANGE, &usvalue, sizeof(usvalue));
 }
 
+void P_SetPlayerSpectator(INT32 playernum)
+{
+	//Make sure you're in the right gametype.
+	if (!G_GametypeHasTeams() && !G_GametypeHasSpectators())
+		return;
+
+	// Don't duplicate efforts.
+	if (players[playernum].spectator)
+		return;
+
+	players[playernum].spectator = true;
+	players[playernum].pflags &= ~PF_WANTSTOJOIN;
+
+	players[playernum].playerstate = PST_REBORN;
+}
+
 //todo: This and the other teamchange functions are getting too long and messy. Needs cleaning.
 static void Got_Teamchange(UINT8 **cp, INT32 playernum)
 {
@@ -3519,55 +3534,38 @@ static void Got_Teamchange(UINT8 **cp, INT32 playernum)
 	{
 		CONS_Alert(CONS_WARNING, M_GetText("Illegal team change received from player %s\n"), player_names[playernum]);
 		SendKick(playernum, KICK_MSG_CON_FAIL);
+		return;
 	}
 
 	//Safety first!
 	// (not respawning spectators here...)
 	wasspectator = (players[playernum].spectator == true);
-		
-	if (!wasspectator && gamestate == GS_LEVEL)
-	{
-		if (players[playernum].mo)
-		{
-			P_DamageMobj(players[playernum].mo, NULL, NULL, 1,
-				(NetPacket.packet.newteam ? DMG_INSTAKILL : DMG_SPECTATOR));
-		}
-		//else
-		if (!NetPacket.packet.newteam)
-		{
-			players[playernum].playerstate = PST_REBORN;
-		}
-	}
 
-	players[playernum].pflags &= ~PF_WANTSTOJOIN;
+	if (!wasspectator)
+	{
+		if (gamestate == GS_LEVEL && players[playernum].mo)
+		{
+			// The following will call P_SetPlayerSpectator if successful
+			P_DamageMobj(players[playernum].mo, NULL, NULL, 1, DMG_SPECTATOR);
+		}
+
+		//...but because the above could return early under some contexts, we try again here
+		P_SetPlayerSpectator(playernum);
+	}
 
 	//Now that we've done our error checking and killed the player
 	//if necessary, put the player on the correct team/status.
-	boolean nochangeoccourred = false;
+
+	if (NetPacket.packet.newteam != 0)
+	{
+		// This serves us in both teamchange contexts.
+		players[playernum].pflags |= PF_WANTSTOJOIN;
+	}
 
 	if (G_GametypeHasTeams())
 	{
-		if (!NetPacket.packet.newteam)
-		{
-			players[playernum].ctfteam = 0;
-			players[playernum].spectator = true;
-		}
-		else
-		{
-			players[playernum].ctfteam = NetPacket.packet.newteam;
-			players[playernum].pflags |= PF_WANTSTOJOIN; //players[playernum].spectator = false;
-			nochangeoccourred = true;
-		}
-	}
-	else if (G_GametypeHasSpectators())
-	{
-		if (!NetPacket.packet.newteam)
-			players[playernum].spectator = true;
-		else
-		{
-			players[playernum].pflags |= PF_WANTSTOJOIN; //players[playernum].spectator = false;
-			nochangeoccourred = true;
-		}
+		// This one is, of course, specific.
+		players[playernum].ctfteam = NetPacket.packet.newteam;
 	}
 
 	if (NetPacket.packet.autobalance)
@@ -3595,20 +3593,7 @@ static void Got_Teamchange(UINT8 **cp, INT32 playernum)
 	else if (NetPacket.packet.newteam == 0 && !wasspectator)
 		HU_AddChatText(va("\x82*%s became a spectator.", player_names[playernum]), false); // "entered the game" text was moved to P_SpectatorJoinGame
 
-	/*if (G_GametypeHasTeams())
-	{
-		if (NetPacket.packet.newteam)
-		{
-			UINT8 i;
-			for (i = 0; i <= splitscreen; i++)
-			{
-				if (playernum == g_localplayers[i]) //CTF and Team Match colors.
-					CV_SetValue(&cv_playercolor[i], NetPacket.packet.newteam + 5); - -this calculation is totally wrong
-			}
-		}
-	}*/
-
-	if (gamestate != GS_LEVEL || nochangeoccourred == true)
+	if (gamestate != GS_LEVEL || wasspectator == true)
 		return;
 
 	FinalisePlaystateChange(playernum);

src/d_netcmd.h

@@ -245,6 +245,7 @@ void D_SetupVote(void);
 void D_ModifyClientVote(UINT8 player, SINT8 voted);
 void D_PickVote(void);
 void ObjectPlace_OnChange(void);
+void P_SetPlayerSpectator(INT32 playernum);
 boolean IsPlayerAdmin(INT32 playernum);
 void SetAdminPlayer(INT32 playernum);
 void ClearAdminPlayers(void);

src/g_demo.c

@@ -228,10 +228,7 @@ void G_ReadDemoExtraData(void)
 		{
 			if (!playeringame[p])
 			{
-				CL_ClearPlayer(p);
-				playeringame[p] = true;
 				G_AddPlayer(p);
-				players[p].spectator = true;
 			}
 
 			for (i = 0; i < MAXAVAILABILITY; i++)
@@ -253,28 +250,34 @@ void G_ReadDemoExtraData(void)
 
 			switch (i) {
 			case DXD_PST_PLAYING:
-				if (players[p].bot)
+				if (players[p].spectator == true)
 				{
-					players[p].spectator = false;
-				}
-				else
-				{
-					players[p].pflags |= PF_WANTSTOJOIN;
+					if (players[p].bot)
+					{
+						players[p].spectator = false;
+					}
+					else
+					{
+						players[p].pflags |= PF_WANTSTOJOIN;
+					}
 				}
 				//CONS_Printf("player %s is despectating on tic %d\n", player_names[p], leveltime);
 				break;
 
 			case DXD_PST_SPECTATING:
-				players[p].pflags &= ~PF_WANTSTOJOIN; // double-fuck you
-				if (players[p].spectator != true)
+				if (players[p].spectator)
 				{
-					//CONS_Printf("player %s is spectating on tic %d\n", player_names[p], leveltime);
-					players[p].spectator = true;
-					if (players[p].mo)
-						P_DamageMobj(players[p].mo, NULL, NULL, 1, DMG_INSTAKILL);
-					else
-						players[p].playerstate = PST_REBORN;
+					players[p].pflags &= ~PF_WANTSTOJOIN;
 				}
+				else
+				{
+					if (players[p].mo)
+					{
+						P_DamageMobj(players[p].mo, NULL, NULL, 1, DMG_SPECTATOR);
+					}
+					P_SetPlayerSpectator(p);
+				}
+
 				break;
 
 			case DXD_PST_LEFT:
@@ -3420,7 +3423,7 @@ void G_DoPlayDemo(const char *defdemoname)
 		if (!playeringame[displayplayers[0]] || players[displayplayers[0]].spectator)
 			displayplayers[0] = consoleplayer = serverplayer = p;
 
-		playeringame[p] = true;
+		G_AddPlayer(p);
 		players[p].spectator = spectator;
 
 		if (flags & DEMO_KICKSTART)

src/g_demo.h

@@ -127,6 +127,8 @@ extern UINT8 demo_writerng;
 #define DXD_COLOR      0x10 // color changed
 #define DXD_FOLLOWER   0x20 // follower was changed
 
+#define DXD_ADDPLAYER (DXD_JOINDATA|DXD_PLAYSTATE|DXD_COLOR|DXD_NAME|DXD_SKIN|DXD_FOLLOWER)
+
 #define DXD_WEAPONPREF 0x80 // netsynced playsim settings were changed
 
 #define DXD_PST_PLAYING    0x01

src/g_game.c

@@ -2822,11 +2822,53 @@ void G_DoReborn(INT32 playernum)
 	}
 }
 
+// These are the barest esentials.
+// This func probably doesn't even need to know if the player is a bot.
 void G_AddPlayer(INT32 playernum)
 {
-	player_t *p = &players[playernum];
-	p->playerstate = PST_REBORN;
-	demo_extradata[playernum] |= DXD_JOINDATA|DXD_PLAYSTATE|DXD_COLOR|DXD_NAME|DXD_SKIN|DXD_FOLLOWER; // Set everything
+	CL_ClearPlayer(playernum);
+	G_DestroyParty(playernum);
+
+	playeringame[playernum] = true;
+
+	player_t *newplayer = &players[playernum];
+
+	newplayer->playerstate = PST_REBORN;
+	newplayer->jointime = 0;
+
+	demo_extradata[playernum] |= DXD_ADDPLAYER;
+}
+
+void G_SpectatePlayerOnJoin(INT32 playernum)
+{
+	// This is only ever called shortly after the above.
+	// That calls CL_ClearPlayer, so spectator is false by default
+
+	if (!netgame && !G_GametypeHasTeams() && !G_GametypeHasSpectators())
+		return;
+
+	// These are handled automatically elsewhere
+	if (demo.playback || players[playernum].bot)
+		return;
+
+	UINT8 i;
+	for (i = 0; i < MAXPLAYERS; i++)
+	{
+		if (!playeringame[i])
+			continue;
+
+		// Spectators are of no consequence
+		if (players[i].spectator)
+			continue;
+
+		// Prevent splitscreen hosters/joiners from only adding 1 player at a time in empty servers (this will also catch yourself)
+		if (!players[i].jointime)
+			continue;
+
+		// A ha! An established player! It's time to spectate
+		players[playernum].spectator = true;
+		break;
+	}
 }
 
 void G_BeginLevelExit(void)
@@ -3245,7 +3287,7 @@ boolean G_GametypeHasSpectators(void)
 #ifdef DEVELOP
 	return true;
 #endif
-	return (netgame || (multiplayer && demo.netgame));
+	return (netgame || (demo.playback && demo.netgame));
 }
 
 //

src/g_game.h

@@ -239,6 +239,7 @@ void G_ResetView(UINT8 viewnum, INT32 playernum, boolean onlyactive);
 void G_AdjustView(UINT8 viewnum, INT32 offset, boolean onlyactive);
 
 void G_AddPlayer(INT32 playernum);
+void G_SpectatePlayerOnJoin(INT32 playernum);
 
 void G_SetExitGameFlag(void);
 void G_ClearExitGameFlag(void);

src/k_bot.c

@@ -47,12 +47,8 @@ void K_SetBot(UINT8 newplayernum, UINT8 skinnum, UINT8 difficulty, botStyle_e st
 {
 	CONS_Debug(DBG_NETPLAY, "addbot: %d\n", newplayernum);
 
-	// Clear player before joining, lest some things get set incorrectly
-	CL_ClearPlayer(newplayernum);
-	G_DestroyParty(newplayernum);
-
-	playeringame[newplayernum] = true;
 	G_AddPlayer(newplayernum);
+
 	if (newplayernum+1 > doomcom->numslots)
 		doomcom->numslots = (INT16)(newplayernum+1);
 

src/k_grandprix.c

@@ -201,7 +201,7 @@ void K_InitGrandPrixBots(void)
 			}
 			else
 			{
-				players[i].spectator = true; // force spectate for all other players, if they happen to exist?
+				P_SetPlayerSpectator(i); // force spectate for all other players, if they happen to exist?
 			}
 		}
 	}

src/p_inter.c

@@ -2056,30 +2056,29 @@ static boolean P_PlayerHitsPlayer(mobj_t *target, mobj_t *inflictor, mobj_t *sou
 
 static boolean P_KillPlayer(player_t *player, mobj_t *inflictor, mobj_t *source, UINT8 type)
 {
-	if (player->respawn.state != RESPAWNST_NONE)
+	if (type == DMG_SPECTATOR && (G_GametypeHasTeams() || G_GametypeHasSpectators()))
 	{
-		K_DoInstashield(player);
-		return false;
+		P_SetPlayerSpectator(player-players);
 	}
-
-	if (!player->exiting && (specialstageinfo.valid == true || modeattacking & ATTACKING_SPB))
+	else
 	{
-		// TODO: this would make a great debug feature for release
-#ifdef DEVELOP
-		if (type != DMG_SPECTATOR)
+		if (player->respawn.state != RESPAWNST_NONE)
+		{
+			K_DoInstashield(player);
+			return false;
+		}
+
+		if (player->exiting)
+		{
+			player->mo->destscale = 1;
+			player->mo->flags |= MF_NOCLIPTHING;
+			return false;
+		}
+
+		if (specialstageinfo.valid == true || (modeattacking & ATTACKING_SPB))
 		{
 			P_DoPlayerExit(player, PF_NOCONTEST);
 		}
-#else
-		P_DoPlayerExit(player, PF_NOCONTEST);
-#endif
-	}
-
-	if (player->exiting)
-	{
-		player->mo->destscale = 1;
-		player->mo->flags |= MF_NOCLIPTHING;
-		return false;
 	}
 
 	switch (type)
@@ -2164,12 +2163,6 @@ static boolean P_KillPlayer(player_t *player, mobj_t *inflictor, mobj_t *source,
 		player->pflags |= PF_ELIMINATED;
 	}
 
-	if (type == DMG_SPECTATOR)
-	{
-		// Set it here so K_CheckBumpers knows about it later.
-		player->spectator = true;
-	}
-
 	return true;
 }
 

src/p_mobj.c

@@ -11770,61 +11770,24 @@ void P_RespawnSpecials(void)
 //
 void P_SpawnPlayer(INT32 playernum)
 {
-	UINT8 i, pcount = 0; // MAXPLAYERS if exiting
+	UINT8 i;
 	player_t *p = &players[playernum];
 	mobj_t *mobj;
 
+	boolean justjoined = (p->jointime <= 1);
+
 	if (p->playerstate == PST_REBORN)
 	{
-		G_PlayerReborn(playernum, (p->jointime <= 1));
+		G_PlayerReborn(playernum, justjoined);
 	}
 
-	for (i = 0; i < MAXPLAYERS; i++)
-	{
-		if (i == playernum)
-			continue;
-		if (!playeringame[i] || players[i].spectator)
-			continue;
-		if (players[i].exiting)
-		{
-			pcount = MAXPLAYERS;
-			break;
-		}
-		if (players[i].jointime <= 1) // Prevent splitscreen hosters/joiners from only adding 1 player at a time in empty servers
-			continue;
-		pcount++;
-	}
+	if (justjoined)
+		G_SpectatePlayerOnJoin(playernum);
 
-	// spawn as spectator determination
-	if (multiplayer && demo.playback)
-	{
-		; // Don't mess with spectator values since the demo setup handles them already.
-	}
-	else if (p->bot)
-	{
-		if (K_PodiumSequence() == true)
-			; // This is too late to correct spectator status. Whatever state we're in at this point, our (dog) bed is made.
-		else if (!(gametyperules & GTR_BOTS)
-		|| (grandprixinfo.gp == true
-			&& grandprixinfo.eventmode != GPEVENT_NONE))
-		{
-			// Bots aren't supposed to be here.
-			p->spectator = true;
-		}
-		else
-		{
-			// No point in a spectating bot!
-			p->spectator = false;
-		}
-	}
-	else if (netgame && p->jointime <= 1 && pcount)
-	{
-		p->spectator = true;
-	}
-	else if (multiplayer && !netgame)
+	if (G_GametypeHasTeams())
 	{
 		// If you're in a team game and you don't have a team assigned yet...
-		if (G_GametypeHasTeams() && p->ctfteam == 0)
+		if (!p->spectator && p->ctfteam == 0)
 		{
 			changeteam_union NetPacket;
 			UINT16 usvalue;
@@ -11834,9 +11797,6 @@ void P_SpawnPlayer(INT32 playernum)
 			// yes even in splitscreen mode
 			p->spectator = true;
 
-			if (playernum&1) p->skincolor = skincolor_redteam;
-			else             p->skincolor = skincolor_blueteam;
-
 			// but immediately send a team change packet.
 			NetPacket.packet.playernum = playernum;
 			NetPacket.packet.verification = true;
@@ -11845,22 +11805,6 @@ void P_SpawnPlayer(INT32 playernum)
 			usvalue = SHORT(NetPacket.value.l|NetPacket.value.b);
 			SendNetXCmd(XD_TEAMCHANGE, &usvalue, sizeof(usvalue));
 		}
-		else // Otherwise, never spectator.
-		{
-			// TODO: this would make a great debug feature for release
-#ifndef DEVELOP
-			p->spectator = false;
-#endif
-		}
-	}
-
-	if (G_GametypeHasTeams())
-	{
-		// Fix stupid non spectator spectators.
-		if (!p->spectator && !p->ctfteam)
-		{
-			p->spectator = true;
-		}
 
 		// Fix team colors.
 		// This code isn't being done right somewhere else. Oh well.
@@ -11959,8 +11903,7 @@ void P_SpawnPlayer(INT32 playernum)
 			S_StartSound(body, sfx_s1af);
 	}
 
-	// I'm not refactoring the loop at the top of this file.
-	pcount = 0;
+	UINT8 pcount = 0;
 
 	for (i = 0; i < MAXPLAYERS; ++i)
 	{

src/p_user.c

@@ -3504,7 +3504,7 @@ boolean P_SpectatorJoinGame(player_t *player)
 	// Pressing fire assigns you to a team that needs players if allowed.
 	// Partial code reproduction from p_tick.c autobalance code.
 	// a surprise tool that will help us later...
-	if (G_GametypeHasTeams())
+	if (G_GametypeHasTeams() && player->ctfteam == 0)
 	{
 		INT32 z, numplayersred = 0, numplayersblue = 0;