Update the ISaveRam interface for clarity. We no longer constrain the sram data length, because it already wasn't true for all cores. Cores that require a specific size should throw if they get the wrong size.

Author: SuuperW

src/BizHawk.Client.Common/movie/MovieConversionExtensions.cs

@@ -133,7 +133,6 @@ public static ITasMovie ConvertToSaveRamAnchoredMovie(this ITasMovie old, byte[]
 
 			foreach (var (k, v) in old.HeaderEntries) tas.HeaderEntries[k] = v;
 
-			tas.StartsFromSaveRam = true;
 			tas.SyncSettingsJson = old.SyncSettingsJson;
 
 			foreach (string comment in old.Comments)

src/BizHawk.Client.Common/movie/bk2/Bk2Movie.HeaderApi.cs

@@ -51,26 +51,7 @@ public virtual bool StartsFromSavestate
 			}
 		}
 
-		public bool StartsFromSaveRam
-		{
-			// ReSharper disable SimplifyConditionalTernaryExpression
-			get => Header.TryGetValue(HeaderKeys.StartsFromSaveram, out var s) ? bool.Parse(s) : false;
-			// ReSharper restore SimplifyConditionalTernaryExpression
-			set
-			{
-				if (value)
-				{
-					if (!Header.ContainsKey(HeaderKeys.StartsFromSaveram))
-					{
-						Header.Add(HeaderKeys.StartsFromSaveram, "True");
-					}
-				}
-				else
-				{
-					Header.Remove(HeaderKeys.StartsFromSaveram);
-				}
-			}
-		}
+		public bool StartsFromSaveRam => SaveRam != null;
 
 		public override string GameName
 		{

src/BizHawk.Client.Common/movie/interfaces/IMovie.cs

@@ -67,7 +67,7 @@ public interface IMovie : IBasicMovieInfo
 		byte[] SaveRam { get; set; }
 
 		bool StartsFromSavestate { get; set; }
-		bool StartsFromSaveRam { get; set; }
+		bool StartsFromSaveRam { get; }
 
 		string LogKey { get; set; }
 

src/BizHawk.Client.EmuHawk/MainForm.cs

@@ -27,12 +27,8 @@
 
 using BizHawk.Emulation.Common;
 using BizHawk.Emulation.Cores;
-using BizHawk.Emulation.Cores.Computers.AppleII;
-using BizHawk.Emulation.Cores.Computers.Commodore64;
 using BizHawk.Emulation.Cores.Computers.DOS;
 using BizHawk.Emulation.Cores.Consoles.Nintendo.QuickNES;
-using BizHawk.Emulation.Cores.Consoles.SNK;
-using BizHawk.Emulation.Cores.Nintendo.GBA;
 using BizHawk.Emulation.Cores.Nintendo.NES;
 using BizHawk.Emulation.Cores.Nintendo.SNES;
 
@@ -1921,36 +1917,24 @@ private void LoadSaveRam()
 					return;
 				}
 
+				byte[] sram = null;
 				try
 				{
-					byte[] sram;
-
-					// some cores might not know how big the saveram ought to be, so just send it the whole file
-					if (Emulator is AppleII or C64 or DOSBox or MGBAHawk or NeoGeoPort or NES { BoardName: "FDS" })
-					{
-						sram = File.ReadAllBytes(saveramToLoad.FullName);
-					}
-					else
-					{
-						var oldRam = Emulator.AsSaveRam().CloneSaveRam();
-						if (oldRam is null)
-						{
-							// we have a SaveRAM file, but the current core does not have save ram.
-							// just skip loading the saveram file in that case
-							return;
-						}
-
-						// why do we silently truncate\pad here instead of warning\erroring?
-						sram = new byte[oldRam.Length];
-						using var fs = saveramToLoad.OpenRead();
-						_ = fs.Read(sram, 0, sram.Length);
-					}
+					sram = File.ReadAllBytes(saveramToLoad.FullName);
+				}
+				catch (Exception e)
+				{
+					AddOnScreenMessage("An IO error occurred while loading Sram");
+					Console.Error.WriteLine(e);
+				}
 
-					Emulator.AsSaveRam().StoreSaveRam(sram);
+				try
+				{
+					if (sram != null) Emulator.AsSaveRam().StoreSaveRam(sram);
 				}
-				catch (IOException e)
+				catch (Exception e)
 				{
-					AddOnScreenMessage("An error occurred while loading Sram");
+					AddOnScreenMessage("The core threw an error while loading Sram");
 					Console.Error.WriteLine(e);
 				}
 			}
@@ -1976,9 +1960,7 @@ public bool FlushSaveRAM(bool autosave = false)
 				var backupPath = $"{path}.bak";
 				var backupFile = new FileInfo(backupPath);
 
-				var saveram = Emulator.AsSaveRam().CloneSaveRam();
-				if (saveram == null)
-					return true;
+				var saveram = Emulator.AsSaveRam().CloneSaveRam()!;
 
 				try
 				{

src/BizHawk.Client.EmuHawk/movie/RecordMovie.cs

@@ -103,7 +103,7 @@ public RecordMovie(
 				MaxDropDownItems = 32,
 				Size = new(152, 21),
 			};
-			if (_emulator.HasSaveRam() && _emulator.AsSaveRam().CloneSaveRam(clearDirty: false) is not null) StartFromCombo.Items.Add(START_FROM_SAVERAM);
+			if (_emulator.HasSaveRam()) StartFromCombo.Items.Add(START_FROM_SAVERAM);
 			if (_emulator.HasSavestates()) StartFromCombo.Items.Add(START_FROM_SAVESTATE);
 
 			DefaultAuthorCheckBox = new()
@@ -242,7 +242,6 @@ private void Ok_Click(object sender, EventArgs e)
 				else if (selectedStartFromValue is START_FROM_SAVERAM && _emulator.HasSaveRam())
 				{
 					var core = _emulator.AsSaveRam();
-					movieToRecord.StartsFromSaveRam = true;
 					movieToRecord.SaveRam = core.CloneSaveRam(clearDirty: false);
 				}
 

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

@@ -55,7 +55,7 @@ private void StartANewProjectFromSaveRamMenuItem_Click(object sender, EventArgs
 		{
 			if (AskSaveChanges())
 			{
-				var saveRam = SaveRamEmulator?.CloneSaveRam(clearDirty: false) ?? throw new Exception("No SaveRam");
+				var saveRam = SaveRamEmulator?.CloneSaveRam(clearDirty: false) ?? throw new Exception("No SaveRam; this button should have been disabled.");
 				GoToFrame(TasView.AnyRowsSelected ? TasView.FirstSelectedRowIndex : 0);
 				var newProject = CurrentTasMovie.ConvertToSaveRamAnchoredMovie(saveRam);
 				MainForm.PauseEmulator();

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

@@ -23,7 +23,7 @@ private bool LinkedSaveRamModified()
 		{
 			for (int i = 0; i < _numCores; i++)
 			{
-				if (_linkedCores[i].AsSaveRam().SaveRamModified)
+				if (_linkedCores[i].AsSaveRam()?.SaveRamModified == true)
 				{
 					return true;
 				}
@@ -37,7 +37,7 @@ public byte[] CloneSaveRam(bool clearDirty)
 			int len = 0;
 			for (int i = 0; i < _numCores; i++)
 			{
-				linkedBuffers.Add(_linkedCores[i].AsSaveRam().CloneSaveRam(clearDirty) ?? Array.Empty<byte>());
+				linkedBuffers.Add(_linkedCores[i].AsSaveRam()?.CloneSaveRam(clearDirty) ?? Array.Empty<byte>());
 				len += linkedBuffers[i].Length;
 			}
 			byte[] ret = new byte[len];
@@ -55,13 +55,15 @@ public void StoreSaveRam(byte[] data)
 			int pos = 0;
 			for (int i = 0; i < _numCores; i++)
 			{
-				var toCopy = _linkedCores[i].AsSaveRam().CloneSaveRam(); // wait CloneSaveRam is already a copy, why are we copying it again
-				if (toCopy is null) continue;
-				var b = new byte[toCopy.Length];
+				var numberBytesToCopy = _linkedCores[i].AsSaveRam()?.CloneSaveRam().Length;
+				if (numberBytesToCopy is null) continue;
+				var b = new byte[numberBytesToCopy.Value];
 				Buffer.BlockCopy(data, pos, b, 0, b.Length);
 				pos += b.Length;
 				_linkedCores[i].AsSaveRam().StoreSaveRam(b);
 			}
+
+			if (data.Length != pos) throw new InvalidOperationException("Incorrect sram size.");
 		}
 	}
 }

src/BizHawk.Emulation.Common/Interfaces/Services/ISaveRam.cs

@@ -10,16 +10,14 @@ public interface ISaveRam : IEmulatorService
 	{
 		/// <summary>
 		/// Returns a copy of the SaveRAM. Editing it won't do you any good unless you later call StoreSaveRam()
-		/// This IS allowed to return null.
-		/// Unfortunately, the core may think differently of a nonexisting (null) saveram vs a 0 size saveram.
-		/// Frontend users of the ISaveRam should treat null as nonexisting (and thus not even write the file, so that the "does not exist" condition can be roundtripped and not confused with an empty file)
 		/// </summary>
 		/// <param name="clearDirty">Whether the saveram should be considered in a clean state after this call for purposes of <see cref="SaveRamModified"/></param>
-		byte[]? CloneSaveRam(bool clearDirty = true);
+		byte[] CloneSaveRam(bool clearDirty = true);
 
 		/// <summary>
-		/// store new SaveRAM to the emu core. the data should be the same size as the return from ReadSaveRam()
+		/// Store new SaveRAM to the emu core.
 		/// </summary>
+		/// <exception cref="Exception">The core may throw an exception if the given data is invalid.</exception>
 		void StoreSaveRam(byte[] data);
 
 		/// <summary>

src/BizHawk.Emulation.Cores/Arcades/MAME/MAME.IEmulator.cs

@@ -8,7 +8,10 @@ public partial class MAME : IEmulator
 		public string SystemId => VSystemID.Raw.Arcade;
 		public bool DeterministicEmulation { get; }
 		public int Frame { get; private set; }
-		public IEmulatorServiceProvider ServiceProvider { get; }
+
+		private BasicServiceProvider _serviceProvider;
+		public IEmulatorServiceProvider ServiceProvider => _serviceProvider;
+
 		public ControllerDefinition ControllerDefinition => MAMEController;
 
 		/// <summary>

src/BizHawk.Emulation.Cores/Arcades/MAME/MAME.ISaveRam.cs

@@ -22,7 +22,7 @@ public byte[] CloneSaveRam(bool clearDirty)
 		{
 			if (_nvramFilenames.Count == 0)
 			{
-				return null;
+				throw new InvalidOperationException("Core currently has no SRAM and should not be providing ISaveRam service.");
 			}
 
 			for (int i = 0; i < _nvramFilenames.Count; i++)
@@ -53,7 +53,7 @@ public void StoreSaveRam(byte[] data)
 		{
 			if (_nvramFilenames.Count == 0)
 			{
-				return;
+				throw new InvalidOperationException("Core currently has no SRAM and should not be providing ISaveRam service.");
 			}
 
 			using var ms = new MemoryStream(data, false);