fix antag selection being evil (#28197)

* fix antag selection being evil

* fix test

* untroll the other tests

* remove role timer troll

* Allow tests to modify antag preferences

* Fix antag selection

* Misc test fixes

* Add AntagPreferenceTest

* Fix lazy mistakes

* Test cleanup

* Try stop players in lobbies from being assigned mid-round antags

* ranting

* I am going insane

---------

Co-authored-by: deltanedas <@deltanedas:kde.org>
Co-authored-by: ElectroJr <leonsfriedrich@gmail.com>

Content.IntegrationTests/Pair/TestPair.Helpers.cs

@@ -2,6 +2,9 @@
 using System.Collections.Generic;
 using System.Diagnostics.CodeAnalysis;
 using System.Linq;
+using Content.Server.Preferences.Managers;
+using Content.Shared.Preferences;
+using Content.Shared.Roles;
 using Robust.Shared.GameObjects;
 using Robust.Shared.Map;
 using Robust.Shared.Prototypes;
@@ -128,4 +131,29 @@ public sealed partial class TestPair
 
         return list;
     }
+
+    /// <summary>
+    /// Helper method for enabling or disabling a antag role
+    /// </summary>
+    public async Task SetAntagPref(ProtoId<AntagPrototype> id, bool value)
+    {
+        var prefMan = Server.ResolveDependency<IServerPreferencesManager>();
+
+        var prefs = prefMan.GetPreferences(Client.User!.Value);
+        // what even is the point of ICharacterProfile if we always cast it to HumanoidCharacterProfile to make it usable?
+        var profile = (HumanoidCharacterProfile) prefs.SelectedCharacter;
+
+        Assert.That(profile.AntagPreferences.Contains(id), Is.EqualTo(!value));
+        var newProfile = profile.WithAntagPreference(id, value);
+
+        await Server.WaitPost(() =>
+        {
+            prefMan.SetProfile(Client.User.Value, prefs.SelectedCharacterIndex, newProfile).Wait();
+        });
+
+        // And why the fuck does it always create a new preference and profile object instead of just reusing them?
+        var newPrefs = prefMan.GetPreferences(Client.User.Value);
+        var newProf = (HumanoidCharacterProfile) newPrefs.SelectedCharacter;
+        Assert.That(newProf.AntagPreferences.Contains(id), Is.EqualTo(value));
+    }
 }

Content.IntegrationTests/PoolManager.cs

@@ -65,11 +65,11 @@ public static partial class PoolManager
 
         options.BeforeStart += () =>
         {
+            // Server-only systems (i.e., systems that subscribe to events with server-only components)
             var entSysMan = IoCManager.Resolve<IEntitySystemManager>();
-            entSysMan.LoadExtraSystemType<ResettingEntitySystemTests.TestRoundRestartCleanupEvent>();
-            entSysMan.LoadExtraSystemType<InteractionSystemTests.TestInteractionSystem>();
             entSysMan.LoadExtraSystemType<DeviceNetworkTestSystem>();
             entSysMan.LoadExtraSystemType<TestDestructibleListenerSystem>();
+
             IoCManager.Resolve<ILogManager>().GetSawmill("loc").Level = LogLevel.Error;
             IoCManager.Resolve<IConfigurationManager>()
                 .OnValueChanged(RTCVars.FailureLogLevel, value => logHandler.FailureLevel = value, true);

Content.IntegrationTests/Tests/GameRules/AntagPreferenceTest.cs

@@ -0,0 +1,76 @@
+#nullable enable
+using System.Collections.Generic;
+using System.Linq;
+using Content.Server.Antag;
+using Content.Server.Antag.Components;
+using Content.Server.GameTicking;
+using Content.Shared.GameTicking;
+using Robust.Shared.GameObjects;
+using Robust.Shared.Player;
+using Robust.Shared.Random;
+
+namespace Content.IntegrationTests.Tests.GameRules;
+
+// Once upon a time, players in the lobby weren't ever considered eligible for antag roles.
+// Lets not let that happen again.
+[TestFixture]
+public sealed class AntagPreferenceTest
+{
+    [Test]
+    public async Task TestLobbyPlayersValid()
+    {
+        await using var pair = await PoolManager.GetServerClient(new PoolSettings
+        {
+            DummyTicker = false,
+            Connected = true,
+            InLobby = true
+        });
+
+        var server = pair.Server;
+        var client = pair.Client;
+        var ticker = server.System<GameTicker>();
+        var sys = server.System<AntagSelectionSystem>();
+
+        // Initially in the lobby
+        Assert.That(ticker.RunLevel, Is.EqualTo(GameRunLevel.PreRoundLobby));
+        Assert.That(client.AttachedEntity, Is.Null);
+        Assert.That(ticker.PlayerGameStatuses[client.User!.Value], Is.EqualTo(PlayerGameStatus.NotReadyToPlay));
+
+        EntityUid uid = default;
+        await server.WaitPost(() => uid = server.EntMan.Spawn("Traitor"));
+        var rule = new Entity<AntagSelectionComponent>(uid, server.EntMan.GetComponent<AntagSelectionComponent>(uid));
+        var def = rule.Comp.Definitions.Single();
+
+        // IsSessionValid & IsEntityValid are preference agnostic and should always be true for players in the lobby.
+        // Though maybe that will change in the future, but then GetPlayerPool() needs to be updated to reflect that.
+        Assert.That(sys.IsSessionValid(rule, pair.Player, def), Is.True);
+        Assert.That(sys.IsEntityValid(client.AttachedEntity, def), Is.True);
+
+        // By default, traitor/antag preferences are disabled, so the pool should be empty.
+        var sessions = new List<ICommonSession>{pair.Player!};
+        var pool = sys.GetPlayerPool(rule, sessions, def);
+        Assert.That(pool.Count, Is.EqualTo(0));
+
+        // Opt into the traitor role.
+        await pair.SetAntagPref("Traitor", true);
+
+        Assert.That(sys.IsSessionValid(rule, pair.Player, def), Is.True);
+        Assert.That(sys.IsEntityValid(client.AttachedEntity, def), Is.True);
+        pool = sys.GetPlayerPool(rule, sessions, def);
+        Assert.That(pool.Count, Is.EqualTo(1));
+        pool.TryPickAndTake(pair.Server.ResolveDependency<IRobustRandom>(), out var picked);
+        Assert.That(picked, Is.EqualTo(pair.Player));
+        Assert.That(sessions.Count, Is.EqualTo(1));
+
+        // opt back out
+        await pair.SetAntagPref("Traitor", false);
+
+        Assert.That(sys.IsSessionValid(rule, pair.Player, def), Is.True);
+        Assert.That(sys.IsEntityValid(client.AttachedEntity, def), Is.True);
+        pool = sys.GetPlayerPool(rule, sessions, def);
+        Assert.That(pool.Count, Is.EqualTo(0));
+
+        await server.WaitPost(() => server.EntMan.DeleteEntity(uid));
+        await pair.CleanReturnAsync();
+    }
+}

Content.IntegrationTests/Tests/GameRules/NukeOpsTest.cs

@@ -58,6 +58,9 @@ public sealed class NukeOpsTest
         Assert.That(client.AttachedEntity, Is.Null);
         Assert.That(ticker.PlayerGameStatuses[client.User!.Value], Is.EqualTo(PlayerGameStatus.NotReadyToPlay));
 
+        // Opt into the nukies role.
+        await pair.SetAntagPref("NukeopsCommander", true);
+
         // There are no grids or maps
         Assert.That(entMan.Count<MapComponent>(), Is.Zero);
         Assert.That(entMan.Count<MapGridComponent>(), Is.Zero);
@@ -198,6 +201,7 @@ public sealed class NukeOpsTest
 
         ticker.SetGamePreset((GamePresetPrototype?)null);
         server.CfgMan.SetCVar(CCVars.GridFill, false);
+        await pair.SetAntagPref("NukeopsCommander", false);
         await pair.CleanReturnAsync();
     }
 }

Content.IntegrationTests/Tests/Interaction/Click/InteractionSystemTests.cs

@@ -407,7 +407,6 @@ namespace Content.IntegrationTests.Tests.Interaction.Click
             await pair.CleanReturnAsync();
         }
 
-        [Reflect(false)]
         public sealed class TestInteractionSystem : EntitySystem
         {
             public EntityEventHandler<InteractUsingEvent>? InteractUsingEvent;

Content.IntegrationTests/Tests/ResettingEntitySystemTests.cs

@@ -9,7 +9,6 @@ namespace Content.IntegrationTests.Tests
     [TestOf(typeof(RoundRestartCleanupEvent))]
     public sealed class ResettingEntitySystemTests
     {
-        [Reflect(false)]
         public sealed class TestRoundRestartCleanupEvent : EntitySystem
         {
             public bool HasBeenReset { get; set; }
@@ -49,8 +48,6 @@ namespace Content.IntegrationTests.Tests
 
                 system.HasBeenReset = false;
 
-                Assert.That(system.HasBeenReset, Is.False);
-
                 gameTicker.RestartRound();
 
                 Assert.That(system.HasBeenReset);

Content.Server/Antag/AntagSelectionSystem.API.cs

@@ -27,6 +27,11 @@ public sealed partial class AntagSelectionSystem
         if (mindCount >= totalTargetCount)
             return false;
 
+        // TODO ANTAG fix this
+        // If here are two definitions with 1/10 and 10/10 slots filled, this will always return the second definition
+        // even though it has already met its target
+        // AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA I fucking hate game ticker code.
+        // It needs to track selected minds for each definition independently.
         foreach (var def in ent.Comp.Definitions)
         {
             var target = GetTargetAntagCount(ent, null, def);
@@ -64,6 +69,10 @@ public sealed partial class AntagSelectionSystem
     /// </summary>
     public int GetTargetAntagCount(Entity<AntagSelectionComponent> ent, AntagSelectionPlayerPool? pool, AntagSelectionDefinition def)
     {
+        // TODO ANTAG
+        // make pool non-nullable
+        // Review uses and ensure that people are INTENTIONALLY including players in the lobby if this is a mid-round
+        // antag selection.
         var poolSize = pool?.Count ?? _playerManager.Sessions
             .Count(s => s.State.Status is not SessionStatus.Disconnected and not SessionStatus.Zombie);
 

Content.Server/Antag/AntagSelectionSystem.cs

@@ -14,6 +14,7 @@ using Content.Server.Roles.Jobs;
 using Content.Server.Shuttles.Components;
 using Content.Server.Station.Systems;
 using Content.Shared.Antag;
+using Content.Shared.GameTicking;
 using Content.Shared.Ghost;
 using Content.Shared.Humanoid;
 using Content.Shared.Players;
@@ -25,6 +26,7 @@ using Robust.Shared.Enums;
 using Robust.Shared.Map;
 using Robust.Shared.Player;
 using Robust.Shared.Random;
+using Robust.Shared.Utility;
 
 namespace Content.Server.Antag;
 
@@ -85,10 +87,9 @@ public sealed partial class AntagSelectionSystem : GameRuleSystem<AntagSelection
                 continue;
 
             if (comp.SelectionsComplete)
-                return;
+                continue;
 
             ChooseAntags((uid, comp), pool);
-            comp.SelectionsComplete = true;
 
             foreach (var session in comp.SelectedSessions)
             {
@@ -106,11 +107,7 @@ public sealed partial class AntagSelectionSystem : GameRuleSystem<AntagSelection
             if (comp.SelectionTime != AntagSelectionTime.PostPlayerSpawn)
                 continue;
 
-            if (comp.SelectionsComplete)
+            ChooseAntags((uid, comp), args.Players);
-                continue;
-
-            ChooseAntags((uid, comp));
-            comp.SelectionsComplete = true;
         }
     }
 
@@ -126,12 +123,18 @@ public sealed partial class AntagSelectionSystem : GameRuleSystem<AntagSelection
         var query = QueryActiveRules();
         while (query.MoveNext(out var uid, out _, out var antag, out _))
         {
+            // TODO ANTAG
+            // what why aasdiuhasdopiuasdfhksad
+            // stop this insanity please
+            // probability of antag assignment shouldn't depend on the order in which rules are returned by the query.
             if (!RobustRandom.Prob(LateJoinRandomChance))
                 continue;
 
             if (!antag.Definitions.Any(p => p.LateJoinAdditional))
                 continue;
 
+            DebugTools.AssertEqual(antag.SelectionTime, AntagSelectionTime.PostPlayerSpawn);
+
             if (!TryGetNextAvailableDefinition((uid, antag), out var def))
                 continue;
 
@@ -164,43 +167,40 @@ public sealed partial class AntagSelectionSystem : GameRuleSystem<AntagSelection
     {
         base.Started(uid, component, gameRule, args);
 
-        if (component.SelectionsComplete)
+        // If the round has not yet started, we defer antag selection until roundstart
-            return;
-
         if (GameTicker.RunLevel != GameRunLevel.InRound)
             return;
 
-        if (GameTicker.RunLevel == GameRunLevel.InRound && component.SelectionTime == AntagSelectionTime.PrePlayerSpawn)
+        if (component.SelectionsComplete)
             return;
 
-        ChooseAntags((uid, component));
+        var players = _playerManager.Sessions
-        component.SelectionsComplete = true;
+            .Where(x => GameTicker.PlayerGameStatuses[x.UserId] == PlayerGameStatus.JoinedGame)
-    }
+            .ToList();
 
-    /// <summary>
+        ChooseAntags((uid, component), players);
-    /// Chooses antagonists from the current selection of players
-    /// </summary>
-    public void ChooseAntags(Entity<AntagSelectionComponent> ent)
-    {
-        var sessions = _playerManager.Sessions.ToList();
-        ChooseAntags(ent, sessions);
     }
 
     /// <summary>
     /// Chooses antagonists from the given selection of players
     /// </summary>
-    public void ChooseAntags(Entity<AntagSelectionComponent> ent, List<ICommonSession> pool)
+    public void ChooseAntags(Entity<AntagSelectionComponent> ent, IList<ICommonSession> pool)
     {
+        if (ent.Comp.SelectionsComplete)
+            return;
+
         foreach (var def in ent.Comp.Definitions)
         {
             ChooseAntags(ent, pool, def);
         }
+
+        ent.Comp.SelectionsComplete = true;
     }
 
     /// <summary>
     /// Chooses antagonists from the given selection of players for the given antag definition.
     /// </summary>
-    public void ChooseAntags(Entity<AntagSelectionComponent> ent, List<ICommonSession> pool, AntagSelectionDefinition def)
+    public void ChooseAntags(Entity<AntagSelectionComponent> ent, IList<ICommonSession> pool, AntagSelectionDefinition def)
     {
         var playerPool = GetPlayerPool(ent, pool, def);
         var count = GetTargetAntagCount(ent, playerPool, def);
@@ -324,20 +324,15 @@ public sealed partial class AntagSelectionSystem : GameRuleSystem<AntagSelection
     /// <summary>
     /// Gets an ordered player pool based on player preferences and the antagonist definition.
     /// </summary>
-    public AntagSelectionPlayerPool GetPlayerPool(Entity<AntagSelectionComponent> ent, List<ICommonSession> sessions, AntagSelectionDefinition def)
+    public AntagSelectionPlayerPool GetPlayerPool(Entity<AntagSelectionComponent> ent, IList<ICommonSession> sessions, AntagSelectionDefinition def)
     {
         var preferredList = new List<ICommonSession>();
         var fallbackList = new List<ICommonSession>();
-        var unwantedList = new List<ICommonSession>();
-        var invalidList = new List<ICommonSession>();
         foreach (var session in sessions)
         {
             if (!IsSessionValid(ent, session, def) ||
                 !IsEntityValid(session.AttachedEntity, def))
-            {
-                invalidList.Add(session);
                 continue;
-            }
 
             var pref = (HumanoidCharacterProfile) _pref.GetPreferences(session.UserId).SelectedCharacter;
             if (def.PrefRoles.Count != 0 && pref.AntagPreferences.Any(p => def.PrefRoles.Contains(p)))
@@ -348,13 +343,9 @@ public sealed partial class AntagSelectionSystem : GameRuleSystem<AntagSelection
             {
                 fallbackList.Add(session);
             }
-            else
-            {
-                unwantedList.Add(session);
-            }
         }
 
-        return new AntagSelectionPlayerPool(new() { preferredList, fallbackList, unwantedList, invalidList });
+        return new AntagSelectionPlayerPool(new() { preferredList, fallbackList });
     }
 
     /// <summary>
@@ -365,14 +356,18 @@ public sealed partial class AntagSelectionSystem : GameRuleSystem<AntagSelection
         if (session == null)
             return true;
 
-        mind ??= session.GetMind();
-
         if (session.Status is SessionStatus.Disconnected or SessionStatus.Zombie)
             return false;
 
         if (ent.Comp.SelectedSessions.Contains(session))
             return false;
 
+        mind ??= session.GetMind();
+
+        // If the player has not spawned in as any entity (e.g., in the lobby), they can be given an antag role/entity.
+        if (mind == null)
+            return true;
+
         //todo: we need some way to check that we're not getting the same role twice. (double picking thieves or zombies through midrounds)
 
         switch (def.MultiAntagSetting)
@@ -401,10 +396,11 @@ public sealed partial class AntagSelectionSystem : GameRuleSystem<AntagSelection
     /// <summary>
     /// Checks if a given entity (mind/session not included) is valid for a given antagonist.
     /// </summary>
-    private bool IsEntityValid(EntityUid? entity, AntagSelectionDefinition def)
+    public bool IsEntityValid(EntityUid? entity, AntagSelectionDefinition def)
     {
+        // If the player has not spawned in as any entity (e.g., in the lobby), they can be given an antag role/entity.
         if (entity == null)
-            return false;
+            return true;
 
         if (HasComp<PendingClockInComponent>(entity))
             return false;

Content.Server/GameTicking/GameTicker.RoundFlow.cs

@@ -795,7 +795,7 @@ namespace Content.Server.GameTicking
     }
 
     /// <summary>
-    ///     Event raised after players were assigned jobs by the GameTicker.
+    ///     Event raised after players were assigned jobs by the GameTicker and have been spawned in.
     ///     You can give on-station people special roles by listening to this event.
     /// </summary>
     public sealed class RulePlayerJobsAssignedEvent

Content.Server/Preferences/Managers/IServerPreferencesManager.cs

@@ -20,5 +20,7 @@ namespace Content.Server.Preferences.Managers
         PlayerPreferences? GetPreferencesOrNull(NetUserId? userId);
         IEnumerable<KeyValuePair<NetUserId, ICharacterProfile>> GetSelectedProfilesForPlayers(List<NetUserId> userIds);
         bool HavePreferencesLoaded(ICommonSession session);
+
+        Task SetProfile(NetUserId userId, int slot, ICharacterProfile profile);
     }
 }

Content.Server/Preferences/Managers/ServerPreferencesManager.cs

@@ -29,11 +29,14 @@ namespace Content.Server.Preferences.Managers
         [Dependency] private readonly IServerDbManager _db = default!;
         [Dependency] private readonly IPlayerManager _playerManager = default!;
         [Dependency] private readonly IDependencyCollection _dependencies = default!;
+        [Dependency] private readonly ILogManager _log = default!;
 
         // Cache player prefs on the server so we don't need as much async hell related to them.
         private readonly Dictionary<NetUserId, PlayerPrefData> _cachedPlayerPrefs =
             new();
 
+        private ISawmill _sawmill = default!;
+
         private int MaxCharacterSlots => _cfg.GetCVar(CCVars.GameMaxCharacterSlots);
 
         public void Init()
@@ -42,6 +45,7 @@ namespace Content.Server.Preferences.Managers
             _netManager.RegisterNetMessage<MsgSelectCharacter>(HandleSelectCharacterMessage);
             _netManager.RegisterNetMessage<MsgUpdateCharacter>(HandleUpdateCharacterMessage);
             _netManager.RegisterNetMessage<MsgDeleteCharacter>(HandleDeleteCharacterMessage);
+            _sawmill = _log.GetSawmill("prefs");
         }
 
         private async void HandleSelectCharacterMessage(MsgSelectCharacter message)
@@ -78,27 +82,25 @@ namespace Content.Server.Preferences.Managers
 
         private async void HandleUpdateCharacterMessage(MsgUpdateCharacter message)
         {
-            var slot = message.Slot;
-            var profile = message.Profile;
             var userId = message.MsgChannel.UserId;
 
-            if (profile == null)
+            // ReSharper disable once ConditionIsAlwaysTrueOrFalseAccordingToNullableAPIContract
-            {
+            if (message.Profile == null)
-                Logger.WarningS("prefs",
+                _sawmill.Error($"User {userId} sent a {nameof(MsgUpdateCharacter)} with a null profile in slot {message.Slot}.");
-                    $"User {userId} sent a {nameof(MsgUpdateCharacter)} with a null profile in slot {slot}.");
+            else
-                return;
+                await SetProfile(userId, message.Slot, message.Profile);
-            }
+        }
 
+        public async Task SetProfile(NetUserId userId, int slot, ICharacterProfile profile)
+        {
             if (!_cachedPlayerPrefs.TryGetValue(userId, out var prefsData) || !prefsData.PrefsLoaded)
             {
-                Logger.WarningS("prefs", $"User {userId} tried to modify preferences before they loaded.");
+                _sawmill.Error($"Tried to modify user {userId} preferences before they loaded.");
                 return;
             }
 
             if (slot < 0 || slot >= MaxCharacterSlots)
-            {
                 return;
-            }
 
             var curPrefs = prefsData.Prefs!;
             var session = _playerManager.GetSessionById(userId);
@@ -112,10 +114,8 @@ namespace Content.Server.Preferences.Managers
 
             prefsData.Prefs = new PlayerPreferences(profiles, slot, curPrefs.AdminOOCColor);
 
-            if (ShouldStorePrefs(message.MsgChannel.AuthType))
+            if (ShouldStorePrefs(session.Channel.AuthType))
-            {
+                await _db.SaveCharacterSlotAsync(userId, profile, slot);
-                await _db.SaveCharacterSlotAsync(message.MsgChannel.UserId, message.Profile, message.Slot);
-            }
         }
 
         private async void HandleDeleteCharacterMessage(MsgDeleteCharacter message)

Content.Shared/Antag/AntagAcceptability.cs

@@ -22,6 +22,14 @@ public enum AntagAcceptability
 
 public enum AntagSelectionTime : byte
 {
+    /// <summary>
+    /// Antag roles are assigned before players are assigned jobs and spawned in.
+    /// This prevents antag selection from happening if the round is on-going.
+    /// </summary>
     PrePlayerSpawn,
+
+    /// <summary>
+    /// Antag roles get assigned after players have been assigned jobs and have spawned in.
+    /// </summary>
     PostPlayerSpawn
 }

Content.Shared/Roles/Jobs/SharedJobSystem.cs

@@ -146,8 +146,10 @@ public abstract class SharedJobSystem : EntitySystem
 
     public bool CanBeAntag(ICommonSession player)
     {
+        // If the player does not have any mind associated with them (e.g., has not spawned in or is in the lobby), then
+        // they are eligible to be given an antag role/entity.
         if (_playerSystem.ContentData(player) is not { Mind: { } mindId })
-            return false;
+            return true;
 
         if (!MindTryGetJob(mindId, out _, out var prototype))
             return true;