Controller backend improvements (#4552)

* more ControllerDefinition docs

* controller: rm old buggy 1-player special case

* rename PlayerCount -> NumControllerGroups

more accurately reflects usage and hopefully prevents future confusion

* avoid recalculating NumControllerGroups

* revert and fix "avoid recalculating NumControllerGroups"

* Add LogEntry+LogKey tests

* fix logic to pass new tests

* remove unused using

* add comments and rename for clarity

---------

Co-authored-by: Morilli <35152647+Morilli@users.noreply.github.com>

Author: RetroEdit

src/BizHawk.Client.Common/movie/bk2/Bk2Controller.cs

@@ -100,10 +100,10 @@ private class Bk2ControllerDefinition : ControllerDefinition
 			public Bk2ControllerDefinition(ControllerDefinition sourceDefinition, string logKey)
 				: base(sourceDefinition)
 			{
-				var groups = logKey.Split(new[] { "#" }, StringSplitOptions.RemoveEmptyEntries);
+				var groups = logKey.Split('#');
 
-				_controlsFromLogKey = groups
-					.Select(group => group.Split(new[] { "|" }, StringSplitOptions.RemoveEmptyEntries)
+				_controlsFromLogKey = groups.Skip(1) // ignore everything before the first '#'
+					.Select(group => group.Split('|', StringSplitOptions.RemoveEmptyEntries)
 						.Select(buttonname => (buttonname, sourceDefinition.Axes.TryGetValue(buttonname, out var axisSpec) ? axisSpec : (AxisSpec?)null))
 						.ToArray())
 					.ToArray();

src/BizHawk.Client.Common/movie/bk2/Bk2LogEntryGenerator.cs

@@ -1,4 +1,3 @@
-using System.Linq;
 using System.Text;
 
 using BizHawk.Emulation.Common;
@@ -27,7 +26,7 @@ public static string GenerateLogKey(ControllerDefinition definition)
 		{
 			var sb = new StringBuilder();
 
-			foreach (var group in definition.ControlsOrdered.Where(static c => c.Count is not 0))
+			foreach (var group in definition.ControlsOrdered)
 			{
 				sb.Append('#');
 				foreach ((string buttonName, _) in group)

src/BizHawk.Client.Common/movie/import/LsmvImport.cs

@@ -87,7 +87,7 @@ protected override void RunImport()
 			controllerDefinition.BuildMnemonicsCache(VSystemID.Raw.SNES);
 			_emptyController = new SimpleController(controllerDefinition);
 			_controller = new SimpleController(controllerDefinition);
-			_playerCount = controllerDefinition.PlayerCount;
+			_playerCount = controllerDefinition.ControlsOrdered.Count - 1;
 
 			Result.Movie.LogKey = Bk2LogEntryGenerator.GenerateLogKey(controllerDefinition);
 

src/BizHawk.Client.Common/movie/import/bkm/BkmControllerDefinition.cs

@@ -9,7 +9,7 @@ internal class BkmControllerDefinition(string name) : ControllerDefinition(name)
 		// same as ControllerDefinition.GenOrderedControls, just with Axes after BoolButtons
 		protected override IReadOnlyList<IReadOnlyList<(string Name, AxisSpec? AxisSpec)>> GenOrderedControls()
 		{
-			var ret = new List<(string, AxisSpec?)>[PlayerCount + 1];
+			var ret = new List<(string, AxisSpec?)>[NumControllerGroups];
 			for (var i = 0; i < ret.Length; i++) ret[i] = new();
 			foreach (var btn in BoolButtons) ret[PlayerNumber(btn)].Add((btn, null));
 			foreach ((string buttonName, var axisSpec) in Axes) ret[PlayerNumber(buttonName)].Add((buttonName, axisSpec));

src/BizHawk.Client.Common/tools/TAStudio/MovieZone.cs

@@ -159,15 +159,15 @@ private void PlaceMacroInternal(IMovie movie, int start)
 		public FileWriteResult Save(string fileName)
 		{
 			// Save the controller definition/LogKey
-			// Save the controller name and player count. (Only for the user.)
+			// Save the controller name and controller groups count. (Only for the user)
 			// Save whether or not the macro should use overlay input, and/or replace
 
 			return FileWriter.Write(fileName, (fs) =>
 			{
 				using var writer = new StreamWriter(fs);
 				writer.WriteLine(InputKey);
 				writer.WriteLine(_movieDefinition.Name);
-				writer.WriteLine(_movieDefinition.PlayerCount.ToString());
+				writer.WriteLine(_movieDefinition.ControlsOrdered.Count.ToString());
 				writer.WriteLine($"{Overlay},{Replace}");
 
 				foreach (string line in _log)
@@ -197,7 +197,7 @@ public FileWriteResult Save(string fileName)
 			{
 				if (!emuKeys.Contains(macroKey))
 				{
-					dialogController.ShowMessageBox($"The selected macro is not compatible with the current emulator core.\nMacro controller: {readText[1]}\nMacro player count: {readText[2]}", "Error");
+					dialogController.ShowMessageBox($"The selected macro is not compatible with the current emulator core.\nMacro controller: {readText[1]}\nMacro controller groups (\"players\" + 1) count: {readText[2]}", "Error");
 					return null;
 				}
 			}

src/BizHawk.Client.EmuHawk/tools/TAStudio/TAStudio.MenuItems.cs

@@ -734,7 +734,7 @@ private void SetUpToolStripColumns()
 				keysMenus[i] = new ToolStripMenuItem();
 			}
 
-			var playerMenus = new ToolStripMenuItem[Emulator.ControllerDefinition.PlayerCount + 1];
+			var playerMenus = new ToolStripMenuItem[Emulator.ControllerDefinition.ControlsOrdered.Count];
 			playerMenus[0] = ColumnsSubMenu;
 
 			for (int i = 1; i < playerMenus.Length; i++)

src/BizHawk.Emulation.Common/Base Implementations/ControllerDefinition.cs

@@ -4,7 +4,6 @@
 using System.Text.RegularExpressions;
 
 using BizHawk.Common.CollectionExtensions;
-using BizHawk.Common.StringExtensions;
 
 namespace BizHawk.Emulation.Common
 {
@@ -20,7 +19,7 @@ public class ControllerDefinition
 
 		private IReadOnlyList<IReadOnlyList<(string, AxisSpec?)>>? _orderedControls;
 
-		/// <summary>starts with console buttons, then each player's buttons individually</summary>
+		/// <summary>Starts with console buttons, then each player's buttons individually</summary>
 		public IReadOnlyList<IReadOnlyList<(string Name, AxisSpec? AxisSpec)>> ControlsOrdered
 		{
 			get
@@ -35,6 +34,11 @@ public class ControllerDefinition
 		public readonly string Name;
 
 		private Dictionary<string, char>? _mnemonicsCache;
+
+		/// <summary>
+		/// A mapping between buttons names and their Bk2 mnemonics.
+		/// (it's only relevant for buttons, not axes)
+		/// </summary>
 		public IReadOnlyDictionary<string, char>? MnemonicsCache => _mnemonicsCache;
 
 		/// <remarks>
@@ -113,17 +117,17 @@ private void AssertMutable()
 			if (!_mutable) throw new InvalidOperationException(ERR_MSG);
 		}
 
-		/// <remarks>implementors should include empty lists for empty players, including "player 0", to match this base implementation</remarks>
+		/// <remarks>Implementors should include empty lists for empty players, including "player 0" (console buttons), to match this base implementation</remarks>
 		protected virtual IReadOnlyList<IReadOnlyList<(string Name, AxisSpec? AxisSpec)>> GenOrderedControls()
 		{
-			var ret = new List<(string, AxisSpec?)>[PlayerCount + 1];
+			var ret = new List<(string, AxisSpec?)>[NumControllerGroups];
 			for (var i = 0; i < ret.Length; i++) ret[i] = new();
 			foreach ((string buttonName, var axisSpec) in Axes) ret[PlayerNumber(buttonName)].Add((buttonName, axisSpec));
 			foreach (var btn in BoolButtons) ret[PlayerNumber(btn)].Add((btn, null));
 			return ret;
 		}
 
-		/// <summary>permanently disables the ability to mutate this instance; returns this reference</summary>
+		/// <summary>Permanently disables the ability to mutate this instance; returns this reference</summary>
 		public ControllerDefinition MakeImmutable()
 		{
 			BoolButtons = BoolButtons.ToImmutableList();
@@ -134,6 +138,12 @@ public ControllerDefinition MakeImmutable()
 			return this;
 		}
 
+		/// <summary>
+		/// Get the player number associated with a control (button, analog axis, etc.).
+		/// Returns 0 for general console buttons not associated with a particular player's control port.
+		/// (for example, returns 0 for the Power button on NES)
+		/// For some consoles like (non-linked) Game Boy, this always returns 0.
+		/// </summary>
 		public static int PlayerNumber(string buttonName)
 		{
 			var match = PlayerRegex.Match(buttonName);
@@ -144,7 +154,14 @@ public static int PlayerNumber(string buttonName)
 
 		private static readonly Regex PlayerRegex = new Regex("^P(\\d+) ");
 
-		public int PlayerCount
+		/// <summary>
+		/// Returns the number of controller groups.
+		/// Usually is number of players + 1.
+		/// Returns 1 for both consoles where all control ports are empty and
+		/// consoles where system buttons aren't separate from "player" buttons,
+		/// like (non-linked) Game Boy
+		/// </summary>
+		public int NumControllerGroups
 		{
 			get
 			{
@@ -153,14 +170,7 @@ public int PlayerCount
 					.Select(PlayerNumber)
 					.DefaultIfEmpty(0)
 					.Max();
-
-				if (player > 0)
-				{
-					return player;
-				}
-
-				// Hack for things like gameboy/ti-83 as opposed to genesis with no controllers plugged in
-				return allNames.Exists(static b => b.StartsWithOrdinal("Up")) ? 1 : 0;
+				return player + 1;
 			}
 		}
 

src/BizHawk.Tests.Client.Common/Movie/LogGeneratorTests.cs

@@ -55,6 +55,60 @@ public void GenerateLogEntry_Floats()
 			var actual = Bk2LogEntryGenerator.GenerateLogEntry(_axisController);
 			Assert.AreEqual("|  100,  100,|", actual);
 		}
+
+		[TestMethod]
+		public void GenerateLogEntry_NoStupidHack()
+		{
+			var upController = new SimpleController(new ControllerDefinition("Dummy Gamepad") { BoolButtons = { "Up" } }.MakeImmutable());
+			upController.Definition.BuildMnemonicsCache(VSystemID.Raw.NES);
+
+			var logEntry = Bk2LogEntryGenerator.GenerateLogEntry(upController);
+			Assert.AreEqual("|.|", logEntry);
+		}
+
+		[TestMethod]
+		public void GenerateLogEntry_EmptyPlayerGroups()
+		{
+			var upController = new SimpleController(new ControllerDefinition("Dummy Gamepad") { BoolButtons = { "P2 Up" } }.MakeImmutable());
+			upController.Definition.BuildMnemonicsCache(VSystemID.Raw.NES);
+
+			var logEntry = Bk2LogEntryGenerator.GenerateLogEntry(upController);
+			Assert.AreEqual("|||.|", logEntry);
+		}
+
+		[TestMethod]
+		public void GenerateLogKey_EmptyPlayerGroups()
+		{
+			var upControllerDefinition = new ControllerDefinition("Dummy Gamepad") { BoolButtons = { "P2 Up" } }.MakeImmutable();
+			upControllerDefinition.BuildMnemonicsCache(VSystemID.Raw.NES);
+
+			var logKey = Bk2LogEntryGenerator.GenerateLogKey(upControllerDefinition);
+			Assert.AreEqual("###P2 Up|", logKey);
+		}
+
+		[TestMethod]
+		public void GenerateLogEntry_Bk2Controller()
+		{
+			var simpleController = new SimpleController(new ControllerDefinition("Dummy Gamepad") { BoolButtons = { "P1 Up", "P3 A" } }.MakeImmutable());
+			simpleController.Definition.BuildMnemonicsCache(VSystemID.Raw.NES);
+
+			var originalLogEntry = Bk2LogEntryGenerator.GenerateLogEntry(simpleController);
+			var originalLogKey = Bk2LogEntryGenerator.GenerateLogKey(simpleController.Definition);
+
+			// just for safety, should be covered by the above tests already
+			Assert.AreEqual("||.||.|", originalLogEntry);
+			Assert.AreEqual("##P1 Up|##P3 A|", originalLogKey);
+
+			// ensure a Bk2Controller constructed with ControllerDefinition and LogKey
+			// generates the exact same outputs as the original SimpleController
+			Bk2Controller bk2Controller = new Bk2Controller(simpleController.Definition, originalLogKey);
+
+			var newLogEntry = Bk2LogEntryGenerator.GenerateLogEntry(bk2Controller);
+			Assert.AreEqual(originalLogEntry, newLogEntry);
+
+			var newLogKey = Bk2LogEntryGenerator.GenerateLogKey(bk2Controller.Definition);
+			Assert.AreEqual(originalLogKey, newLogKey);
+		}
 #pragma warning restore BHI1600
 	}
 }