Game shouldn't crash when playing a deathmatch map with bots

[AdamTadeusz]

Caused by bots joining TEAM_UNASSIGNED which messes up the actions somehow.

To reproduce:

set sv_neo_gamemode_enforcement to 1
launch a map with some bots (I picked ntre_bullet_tdm specifically)

What should happen:

The game doesn't crash

What happens:

The game crashes

To fix:

Remove the IsTeamplay() check in CNEOBotManager::MaintainBotQuota() (or better yet the entirety of line 346)

[Rainyan]

I can repro this with the steps, and it produces the following call stack for the exception:

>	server.dll!Action<CNEOBot>::IsOutOfScope() Line 1006	C++
 	server.dll!Action<CNEOBot>::InvokeUpdate(CNEOBot * me, Behavior<CNEOBot> * behavior, float interval) Line 1357	C++
 	server.dll!Behavior<CNEOBot>::Update(CNEOBot * me, float interval) Line 258	C++
 	server.dll!CNEOBotIntention::Update() Line 2528	C++
 	server.dll!INextBot::Update() Line 133	C++
 	server.dll!NextBotPlayer<CNEO_Player>::Update() Line 976	C++
 	server.dll!CNEOBot::Update() Line 817	C++
 	server.dll!NextBotPlayer<CNEO_Player>::PhysicsSimulate() Line 743	C++
 	server.dll!CNEOBot::PhysicsSimulate() Line 683	C++
 	server.dll!Physics_SimulateEntity(CBaseEntity * pEntity) Line 2019	C++
 	server.dll!Physics_RunThinkFunctions(bool simulating) Line 2075	C++
 	server.dll!CServerGameDLL::GameFrame(bool simulating) Line 1281	C++
 	[External Code]	

Incidentally, this call stack turns out to be identical to the one in issue comments of #1355

Specifically around this comment: #1355 (comment)

Maybe these two issues are in fact symptoms of the same underlying problem? Although I'm not quite sure what that is. It appears like the action "list" somehow ends up dangling, which causes the memory violation.

[Rainyan]

I could also raise the exception with call stack (same as the alternative crash at #1355):

 	0000000000000000()	Unknown
>	server.dll!Action<CNEOBot>::ApplyResult(CNEOBot * me, Behavior<CNEOBot> * behavior, ActionResult<CNEOBot> result) Line 1569	C++
 	server.dll!Behavior<CNEOBot>::Update(CNEOBot * me, float interval) Line 258	C++
 	server.dll!CNEOBotIntention::Update() Line 2528	C++
 	server.dll!INextBot::Update() Line 133	C++
 	server.dll!NextBotPlayer<CNEO_Player>::Update() Line 976	C++
 	server.dll!CNEOBot::Update() Line 817	C++
 	server.dll!NextBotPlayer<CNEO_Player>::PhysicsSimulate() Line 743	C++
 	server.dll!CNEOBot::PhysicsSimulate() Line 683	C++
 	server.dll!Physics_SimulateEntity(CBaseEntity * pEntity) Line 2019	C++
 	server.dll!Physics_RunThinkFunctions(bool simulating) Line 2075	C++
 	server.dll!CServerGameDLL::GameFrame(bool simulating) Line 1281	C++
 	[External Code]	

by trying these repro steps with ntre_dawn_ctg (although I imagine the actual map chosen is irrelevant, but I'm including it here just in case).

I'll go ahead and close #1355 as a duplicate of this one, since this is the more generalized version and has a repro case that is easy to trigger.

[AdamTadeusz]

I wonder if the functionality of the JGR gamemode that cause players to become observers when they're prevented from respawning could somehow be accidentaly making a bot an observer. In the deathmatch gamemode the issue is that bots join TEAM_UNASSIGNED, but I imagine TEAM_SPECTATOR would cause this issue as well, and perhaps being on a game team but still being an observer would too. I would think there would be a check in the bot update function to skip the update if the bot is an observer but perhaps that's something we could add if it's not there

[Rainyan]

Removing the IsTeamplay check as proposed, at:

neo/src/game/server/neo/bot/neo_bot_manager.cpp

Line 346 in cd925bd

if ( NEORules()->IsTeamplay() && iTeam == TEAM_UNASSIGNED )

and joining spectator team causes all of the bots to spawn at the spectator camera:

[AdamTadeusz]

thats just the case of the map not being made for deathmatch and there not being a deathmatch spawns loc file

Theres a command for creating some spawns at the local players current position and another to save them after which the bots will spawn at those locations and walk around the map just fine, but I figured that wasn't important for reproducing this bug specifically

---

(edit by Rain: I'm hiding this comment together with my comment above, to which this is responding, to keep the issue clean. Since it turns out my original comment was off-topic.)

[Rainyan]

I wonder if the functionality of the JGR gamemode that cause players to become observers when they're prevented from respawning could somehow be accidentaly making a bot an observer. In the deathmatch gamemode the issue is that bots join TEAM_UNASSIGNED, but I imagine TEAM_SPECTATOR would cause this issue as well, and perhaps being on a game team but still being an observer would too. I would think there would be a check in the bot update function to skip the update if the bot is an observer but perhaps that's something we could add if it's not there

That sounds plausible. I wonder if we could somehow detect this, and cleanup the Action<Actor>* "list" of m_buriedUnderMe etc with proper nullptr values (or update the values to link the correct valid ptr if relevant), to prevent the crash at its root cause.

So that the calls which throw the memory violation:

neo/src/game/server/NextBot/NextBotBehavior.h

Lines 1004 to 1006 in cd925bd

	for( Action< Actor > *under = GetActionBuriedUnderMe(); under; under = under->GetActionBuriedUnderMe() )
	{
	if ( under->m_eventResult.m_type == CHANGE_TO ||

where under is dangling (or uninitialized/invalid? unsure), and:

neo/src/game/server/NextBot/NextBotBehavior.h

Lines 1539 to 1540 in cd925bd

	template < typename Actor >
	Action< Actor > *Action< Actor >::ApplyResult( Actor *me, Behavior< Actor > *behavior, ActionResult< Actor > result )

where me is dangling/bad, could be revolved by them properly returning a nullptr instead of a stale value (which would incorrectly pass the if(value) check, causing the mem violation). Or such that they would return the correct ptr if there is a case where it shouldn't return a nullptr.

[sunzenshen]

Experimental branch #1553 seems to address this crash, but it would be a good idea to sanity check if it doesn't affect other gamemodes or that it doesn't have unintended side effects.