Make department / job list sorting consistent. (#25486)

* Make department / job list sorting consistent.

This makes late join, crew manifest and character profile all apply consistent sorting for jobs and departments.

We use the already-defined weights for departments (so command, then sec, then station specific, then just sort by prototype ID). Jobs also use weight (so heads are always at the top) then prototype ID, then character name (for manifest).

Removed the crewmanifest.ordering CVar as doing it via prototype weight is just easier, and that CVar was already a mess anyways.

* Fix jittery job icons in lists.

They were set to KeepCentered in TextureRect. This has issues because the allocated space is actually an odd number of pixels, so it tries to position the draw at a half pixel offset.

Now, yes, fixing this in TextureRect would make much more sense, but get off my back. (Ok seriously we need better helper functions for doing that in the engine. Don't wanna deal with that right now and I already have this patch made.)

Instead I'm just gonna fix the issue by using VerticalAlignment in all these places instead which ends up doing exactly the same thing YIPPEE.

Also gave a margin next to the icon on the crew manifest. Margins people!

Content.Client/CrewManifest/UI/CrewManifestListing.cs

@@ -1,18 +1,14 @@
-using Content.Shared.CCVar;
+using Content.Shared.CrewManifest;
-using Content.Shared.CrewManifest;
 using Content.Shared.Roles;
 using Robust.Client.GameObjects;
 using Robust.Client.UserInterface.Controls;
-using Robust.Shared.Configuration;
 using Robust.Shared.Prototypes;
 using Robust.Shared.Utility;
-using System.Linq;
 
 namespace Content.Client.CrewManifest.UI;
 
 public sealed class CrewManifestListing : BoxContainer
 {
-    [Dependency] private readonly IConfigurationManager _configManager = default!;
     [Dependency] private readonly IEntitySystemManager _entitySystem = default!;
     [Dependency] private readonly IPrototypeManager _prototypeManager = default!;
     private readonly SpriteSystem _spriteSystem;
@@ -25,7 +21,7 @@ public sealed class CrewManifestListing : BoxContainer
 
     public void AddCrewManifestEntries(CrewManifestEntries entries)
     {
-        var entryDict = new Dictionary<string, List<CrewManifestEntry>>();
+        var entryDict = new Dictionary<DepartmentPrototype, List<CrewManifestEntry>>();
 
         foreach (var entry in entries.Entries)
         {
@@ -34,37 +30,19 @@ public sealed class CrewManifestListing : BoxContainer
                 // this is a little expensive, and could be better
                 if (department.Roles.Contains(entry.JobPrototype))
                 {
-                    entryDict.GetOrNew(department.ID).Add(entry);
+                    entryDict.GetOrNew(department).Add(entry);
                 }
             }
         }
 
-        var entryList = new List<(string section, List<CrewManifestEntry> entries)>();
+        var entryList = new List<(DepartmentPrototype section, List<CrewManifestEntry> entries)>();
 
         foreach (var (section, listing) in entryDict)
         {
             entryList.Add((section, listing));
         }
 
-        var sortOrder = _configManager.GetCVar(CCVars.CrewManifestOrdering).Split(",").ToList();
+        entryList.Sort((a, b) => DepartmentUIComparer.Instance.Compare(a.section, b.section));
-
-        entryList.Sort((a, b) =>
-        {
-            var ai = sortOrder.IndexOf(a.section);
-            var bi = sortOrder.IndexOf(b.section);
-
-            // this is up here so -1 == -1 occurs first
-            if (ai == bi)
-                return 0;
-
-            if (ai == -1)
-                return -1;
-
-            if (bi == -1)
-                return 1;
-
-            return ai.CompareTo(bi);
-        });
 
         foreach (var item in entryList)
         {

Content.Client/CrewManifest/UI/CrewManifestSection.cs

@@ -4,24 +4,25 @@ using Robust.Client.GameObjects;
 using Robust.Client.UserInterface.Controls;
 using Robust.Shared.Prototypes;
 using System.Numerics;
+using Content.Shared.Roles;
 
 namespace Content.Client.CrewManifest.UI;
 
 public sealed class CrewManifestSection : BoxContainer
 {
-    public CrewManifestSection(IPrototypeManager prototypeManager, SpriteSystem spriteSystem, string sectionTitle,
+    public CrewManifestSection(
+        IPrototypeManager prototypeManager,
+        SpriteSystem spriteSystem,
+        DepartmentPrototype section,
         List<CrewManifestEntry> entries)
     {
         Orientation = LayoutOrientation.Vertical;
         HorizontalExpand = true;
 
-        if (Loc.TryGetString($"department-{sectionTitle}", out var localizedDepart))
-            sectionTitle = localizedDepart;
-
         AddChild(new Label()
         {
             StyleClasses = { "LabelBig" },
-            Text = Loc.GetString(sectionTitle)
+            Text = Loc.GetString($"department-{section.ID}")
         });
 
         var gridContainer = new GridContainer()
@@ -55,8 +56,9 @@ public sealed class CrewManifestSection : BoxContainer
                 var icon = new TextureRect()
                 {
                     TextureScale = new Vector2(2, 2),
-                    Stretch = TextureRect.StretchMode.KeepCentered,
+                    VerticalAlignment = VAlignment.Center,
                     Texture = spriteSystem.Frame0(jobIcon.Icon),
+                    Margin = new Thickness(0, 0, 4, 0)
                 };
 
                 titleContainer.AddChild(icon);

Content.Client/LateJoin/LateJoinGui.cs

@@ -1,3 +1,4 @@
+using System.Linq;
 using System.Numerics;
 using Content.Client.CrewManifest;
 using Content.Client.GameTicking.Managers;
@@ -159,8 +160,10 @@ namespace Content.Client.LateJoin
                 };
 
                 var firstCategory = true;
+                var departments = _prototypeManager.EnumeratePrototypes<DepartmentPrototype>().ToArray();
+                Array.Sort(departments, DepartmentUIComparer.Instance);
 
-                foreach (var department in _prototypeManager.EnumeratePrototypes<DepartmentPrototype>())
+                foreach (var department in departments)
                 {
                     var departmentName = Loc.GetString($"department-{department.ID}");
                     _jobCategories[id] = new Dictionary<string, BoxContainer>();
@@ -176,7 +179,7 @@ namespace Content.Client.LateJoin
                         jobsAvailable.Add(_prototypeManager.Index<JobPrototype>(jobId));
                     }
 
-                    jobsAvailable.Sort((x, y) => -string.Compare(x.LocalizedName, y.LocalizedName, StringComparison.CurrentCultureIgnoreCase));
+                    jobsAvailable.Sort(JobUIComparer.Instance);
 
                     // Do not display departments with no jobs available.
                     if (jobsAvailable.Count == 0)
@@ -231,7 +234,7 @@ namespace Content.Client.LateJoin
                         var icon = new TextureRect
                         {
                             TextureScale = new Vector2(2, 2),
-                            Stretch = TextureRect.StretchMode.KeepCentered
+                            VerticalAlignment = VAlignment.Center
                         };
 
                         var jobIcon = _prototypeManager.Index<StatusIconPrototype>(prototype.Icon);

Content.Client/Medical/CrewMonitoring/CrewMonitoringWindow.xaml.cs

@@ -265,7 +265,7 @@ public sealed partial class CrewMonitoringWindow : FancyWindow
                 var jobIcon = new TextureRect()
                 {
                     TextureScale = new Vector2(2f, 2f),
-                    Stretch = TextureRect.StretchMode.KeepCentered,
+                    VerticalAlignment = VAlignment.Center,
                     Texture = _spriteSystem.Frame0(proto.Icon),
                     Margin = new Thickness(5, 0, 5, 0),
                 };

Content.Client/Preferences/UI/HumanoidProfileEditor.xaml.cs

@@ -539,10 +539,8 @@ namespace Content.Client.Preferences.UI
             _jobCategories.Clear();
             var firstCategory = true;
 
-            var departments = _prototypeManager.EnumeratePrototypes<DepartmentPrototype>()
+            var departments = _prototypeManager.EnumeratePrototypes<DepartmentPrototype>().ToArray();
-                .OrderByDescending(department => department.Weight)
+            Array.Sort(departments, DepartmentUIComparer.Instance);
-                .ThenBy(department => Loc.GetString($"department-{department.ID}"))
-                .ToList();
 
             foreach (var department in departments)
             {
@@ -588,11 +586,8 @@ namespace Content.Client.Preferences.UI
                     _jobList.AddChild(category);
                 }
 
-                var jobs = department.Roles.Select(jobId => _prototypeManager.Index<JobPrototype>(jobId))
+                var jobs = department.Roles.Select(jobId => _prototypeManager.Index<JobPrototype>(jobId)).ToArray();
-                    .Where(job => job.SetPreference)
+                Array.Sort(jobs, JobUIComparer.Instance);
-                    .OrderByDescending(job => job.Weight)
-                    .ThenBy(job => job.LocalizedName)
-                    .ToList();
 
                 foreach (var job in jobs)
                 {
@@ -1315,7 +1310,7 @@ namespace Content.Client.Preferences.UI
                 var icon = new TextureRect
                 {
                     TextureScale = new Vector2(2, 2),
-                    Stretch = TextureRect.StretchMode.KeepCentered
+                    VerticalAlignment = VAlignment.Center
                 };
                 var jobIcon = protoMan.Index<StatusIconPrototype>(proto.Icon);
                 icon.Texture = jobIcon.Icon.Frame0();

Content.Server/CrewManifest/CrewManifestSystem.cs

@@ -9,10 +9,12 @@ using Content.Shared.Administration;
 using Content.Shared.CCVar;
 using Content.Shared.CrewManifest;
 using Content.Shared.GameTicking;
+using Content.Shared.Roles;
 using Content.Shared.StationRecords;
 using Robust.Shared.Configuration;
 using Robust.Shared.Console;
 using Robust.Shared.Player;
+using Robust.Shared.Prototypes;
 using Robust.Shared.Utility;
 
 namespace Content.Server.CrewManifest;
@@ -23,6 +25,7 @@ public sealed class CrewManifestSystem : EntitySystem
     [Dependency] private readonly StationRecordsSystem _recordsSystem = default!;
     [Dependency] private readonly EuiManager _euiManager = default!;
     [Dependency] private readonly IConfigurationManager _configManager = default!;
+    [Dependency] private readonly IPrototypeManager _prototypeManager = default!;
 
     /// <summary>
     ///     Cached crew manifest entries. The alternative is to outright
@@ -223,15 +226,26 @@ public sealed class CrewManifestSystem : EntitySystem
 
         var entries = new CrewManifestEntries();
 
+        var entriesSort = new List<(JobPrototype? job, CrewManifestEntry entry)>();
         foreach (var recordObject in iter)
         {
             var record = recordObject.Item2;
             var entry = new CrewManifestEntry(record.Name, record.JobTitle, record.JobIcon, record.JobPrototype);
 
-            entries.Entries.Add(entry);
+            _prototypeManager.TryIndex(record.JobPrototype, out JobPrototype? job);
+            entriesSort.Add((job, entry));
         }
 
-        entries.Entries = entries.Entries.OrderBy(e => e.JobTitle).ThenBy(e => e.Name).ToList();
+        entriesSort.Sort((a, b) =>
+        {
+            var cmp = JobUIComparer.Instance.Compare(a.job, b.job);
+            if (cmp != 0)
+                return cmp;
+
+            return string.Compare(a.entry.Name, b.entry.Name, StringComparison.CurrentCultureIgnoreCase);
+        });
+
+        entries.Entries = entriesSort.Select(x => x.entry).ToArray();
         _cachedEntries[station] = entries;
     }
 }

Content.Server/StationRecords/Systems/StationRecordsSystem.cs

@@ -129,7 +129,7 @@ public sealed class StationRecordsSystem : SharedStationRecordsSystem
             JobPrototype = jobId,
             Species = species,
             Gender = gender,
-            DisplayPriority = jobPrototype.Weight,
+            DisplayPriority = jobPrototype.RealDisplayWeight,
             Fingerprint = mobFingerprint,
             DNA = dna
         };

Content.Shared/CCVar/CCVars.cs

@@ -1479,15 +1479,6 @@ namespace Content.Shared.CCVar
         public static readonly CVarDef<bool> CrewManifestUnsecure =
             CVarDef.Create("crewmanifest.unsecure", true, CVar.REPLICATED);
 
-        /// <summary>
-        ///     Dictates the order the crew manifest will appear in, in terms of its sections.
-        ///     Sections not in this list will appear at the end of the list, in no
-        ///     specific order.
-        /// </summary>
-        public static readonly CVarDef<string> CrewManifestOrdering =
-            CVarDef.Create("crewmanifest.ordering", "Command,Security,Science,Medical,Engineering,Cargo,Civilian,Unknown",
-                CVar.REPLICATED);
-
         /*
          * Biomass
          */

Content.Shared/CrewManifest/SharedCrewManifestSystem.cs

@@ -1,4 +1,5 @@
 using Content.Shared.Eui;
+using NetSerializer;
 using Robust.Shared.Serialization;
 
 namespace Content.Shared.CrewManifest;
@@ -39,7 +40,7 @@ public sealed class CrewManifestEntries
     ///     Entries in the crew manifest. Goes by department ID.
     /// </summary>
     // public Dictionary<string, List<CrewManifestEntry>> Entries = new();
-    public List<CrewManifestEntry> Entries = new();
+    public CrewManifestEntry[] Entries = Array.Empty<CrewManifestEntry>();
 }
 
 [Serializable, NetSerializable]

Content.Shared/Roles/DepartmentPrototype.cs

@@ -37,3 +37,27 @@ public sealed partial class DepartmentPrototype : IPrototype
     [DataField("weight")]
     public int Weight { get; private set; } = 0;
 }
+
+/// <summary>
+/// Sorts <see cref="DepartmentPrototype"/> appropriately for display in the UI,
+/// respecting their <see cref="DepartmentPrototype.Weight"/>.
+/// </summary>
+public sealed class DepartmentUIComparer : IComparer<DepartmentPrototype>
+{
+    public static readonly DepartmentUIComparer Instance = new();
+
+    public int Compare(DepartmentPrototype? x, DepartmentPrototype? y)
+    {
+        if (ReferenceEquals(x, y))
+            return 0;
+        if (ReferenceEquals(null, y))
+            return 1;
+        if (ReferenceEquals(null, x))
+            return -1;
+
+        var cmp = -x.Weight.CompareTo(y.Weight);
+        if (cmp != 0)
+            return cmp;
+        return string.Compare(x.ID, y.ID, StringComparison.Ordinal);
+    }
+}

Content.Shared/Roles/JobPrototype.cs

@@ -1,5 +1,6 @@
 using Content.Shared.Access;
 using Content.Shared.Players.PlayTimeTracking;
+using Content.Shared.Roles;
 using Content.Shared.StatusIcon;
 using Robust.Shared.Prototypes;
 using Robust.Shared.Serialization.TypeSerializers.Implementations.Custom.Prototype;
@@ -70,6 +71,16 @@ namespace Content.Shared.Roles
         [DataField("weight")]
         public int Weight { get; private set; }
 
+        /// <summary>
+        /// How to sort this job relative to other jobs in the UI.
+        /// Jobs with a higher value with sort before jobs with a lower value.
+        /// If not set, <see cref="Weight"/> is used as a fallback.
+        /// </summary>
+        [DataField]
+        public int? DisplayWeight { get; private set; }
+
+        public int RealDisplayWeight => DisplayWeight ?? Weight;
+
         /// <summary>
         ///     A numerical score for how much easier this job is for antagonists.
         ///     For traitors, reduces starting TC by this amount. Other gamemodes can use it for whatever they find fitting.
@@ -106,4 +117,28 @@ namespace Content.Shared.Roles
         [DataField("extendedAccessGroups", customTypeSerializer: typeof(PrototypeIdListSerializer<AccessGroupPrototype>))]
         public IReadOnlyCollection<string> ExtendedAccessGroups { get; private set; } = Array.Empty<string>();
     }
+
+    /// <summary>
+    /// Sorts <see cref="JobPrototype"/>s appropriately for display in the UI,
+    /// respecting their <see cref="JobPrototype.Weight"/>.
+    /// </summary>
+    public sealed class JobUIComparer : IComparer<JobPrototype>
+    {
+        public static readonly JobUIComparer Instance = new();
+
+        public int Compare(JobPrototype? x, JobPrototype? y)
+        {
+            if (ReferenceEquals(x, y))
+                return 0;
+            if (ReferenceEquals(null, y))
+                return 1;
+            if (ReferenceEquals(null, x))
+                return -1;
+
+            var cmp = -x.RealDisplayWeight.CompareTo(y.RealDisplayWeight);
+            if (cmp != 0)
+                return cmp;
+            return string.Compare(x.ID, y.ID, StringComparison.Ordinal);
+        }
+    }
 }