Reduce hacks and duplication in TAStudio movie loading

Previously, there was no way to load a movie without saving it to a file first. This has been changed, so lots of saves and loads are no longer necessary.
This also partially gets rid of the default.tasproj, although the name is still used as a placeholder for now.

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

@@ -146,8 +146,7 @@ namespace BizHawk.Client.Common
 
 		public bool Load()
 		{
-			var file = new FileInfo(Filename);
-			if (!file.Exists)
+			if (!File.Exists(Filename))
 			{
 				return false;
 			}

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

@@ -61,11 +61,6 @@ namespace BizHawk.Client.Common
 			return new Bk2Controller("", definition ?? MovieController.Definition);
 		}
 
-		public void SetMovieController(ControllerDefinition definition)
-		{
-			MovieController = GenerateMovieController(definition);
-		}
-
 		public void HandleFrameBefore()
 		{
 			if (Movie.NotActive())
@@ -189,72 +184,49 @@ namespace BizHawk.Client.Common
 			return true;
 		}
 
-		/// <exception cref="MoviePlatformMismatchException"><paramref name="record"/> is <see langword="false"/> and <paramref name="movie"/>.<see cref="IBasicMovieInfo.SystemID"/> does not match <paramref name="systemId"/>.<see cref="IEmulator.SystemId"/></exception>
+		/// <exception cref="MoviePlatformMismatchException"><paramref name="movie"/>.<see cref="IBasicMovieInfo.SystemID"/> does not match <paramref name="systemId"/>.<see cref="IEmulator.SystemId"/></exception>
 		public void QueueNewMovie(
 			IMovie movie,
-			bool record,
 			string systemId,
 			string loadedRomHash,
 			PathEntryCollection pathEntries,
 			IDictionary<string, string> preferredCores)
 		{
-			if (movie.IsActive() && movie.Changes)
+			if (movie.SystemID != systemId)
 			{
-				movie.Save();
+				throw new MoviePlatformMismatchException(
+					$"Movie system Id ({movie.SystemID}) does not match the currently loaded platform ({systemId}), unable to load");
 			}
 
-			if (!record) // The semantics of record is that we are starting a new movie, and even wiping a pre-existing movie with the same path, but non-record means we are loading an existing movie into playback mode
+			if (!(string.IsNullOrEmpty(movie.Hash) || loadedRomHash.Equals(movie.Hash, StringComparison.Ordinal))
+				&& movie is TasMovie tasproj)
 			{
-				movie.Load();
-				
-				if (movie.SystemID != systemId)
+				var result = _dialogParent.ModalMessageBox2(
+					caption: "Discard GreenZone?",
+					text: $"The TAStudio project {movie.Filename.MakeRelativeTo(pathEntries.MovieAbsolutePath())} appears to be for a different game than the one that's loaded.\n"
+						+ "Choose \"No\" to continue anyway, which may lead to an invalid savestate being loaded.\n"
+						+ "Choose \"Yes\" to discard the GreenZone (savestate history). This is safer, and at worst you'll only need to watch through the whole movie.");
+				//TODO add abort option
+				if (result)
 				{
-					throw new MoviePlatformMismatchException(
-						$"Movie system Id ({movie.SystemID}) does not match the currently loaded platform ({systemId}), unable to load");
-				}
-
-				if (!(string.IsNullOrEmpty(movie.Hash) || loadedRomHash.Equals(movie.Hash, StringComparison.Ordinal))
-					&& movie is TasMovie tasproj)
-				{
-					var result = _dialogParent.ModalMessageBox2(
-						caption: "Discard GreenZone?",
-						text: $"The TAStudio project {movie.Filename.MakeRelativeTo(pathEntries.MovieAbsolutePath())} appears to be for a different game than the one that's loaded.\n"
-							+ "Choose \"No\" to continue anyway, which may lead to an invalid savestate being loaded.\n"
-							+ "Choose \"Yes\" to discard the GreenZone (savestate history). This is safer, and at worst you'll only need to watch through the whole movie.");
-					//TODO add abort option
-					if (result)
-					{
-						tasproj.TasSession.UpdateValues(frame: 0, currentBranch: tasproj.TasSession.CurrentBranch); // wtf is this API --yoshi
-						tasproj.InvalidateEntireGreenzone();
-					}
+					tasproj.TasSession.UpdateValues(frame: 0, currentBranch: tasproj.TasSession.CurrentBranch); // wtf is this API --yoshi
+					tasproj.InvalidateEntireGreenzone();
 				}
 			}
 
-			if (!record)
+			if (string.IsNullOrWhiteSpace(movie.Core))
 			{
-				if (string.IsNullOrWhiteSpace(movie.Core))
-				{
-					PopupMessage(preferredCores.TryGetValue(systemId, out var coreName)
-						? $"No core specified in the movie file, using the preferred core {coreName} instead."
-						: "No core specified in the movie file, using the default core instead.");
-				}
-				else
-				{
-					var keys = preferredCores.Keys.ToList();
-					foreach (var k in keys)
-					{
-						preferredCores[k] = movie.Core;
-					}
-				}
-			}
-
-			if (record) // This is a hack really, we need to set the movie to its proper state so that it will be considered active later
-			{
-				movie.SwitchToRecord();
+				PopupMessage(preferredCores.TryGetValue(systemId, out var coreName)
+					? $"No core specified in the movie file, using the preferred core {coreName} instead."
+					: "No core specified in the movie file, using the default core instead.");
 			}
 			else
 			{
-				movie.SwitchToPlay();
+				var keys = preferredCores.Keys.ToList();
+				foreach (var k in keys)
+				{
+					preferredCores[k] = movie.Core;
+				}
 			}
 
 			_queuedMovie = movie;
@@ -331,15 +303,17 @@ namespace BizHawk.Client.Common
 			Movie.SwitchToPlay();
 		}
 
-		public IMovie Get(string path)
+		public IMovie Get(string path, bool loadMovie)
 		{
 			// TODO: change IMovies to take HawkFiles only and not path
-			if (Path.GetExtension(path)?.EndsWithOrdinal("tasproj") ?? false)
-			{
-				return new TasMovie(this, path);
-			}
+			IMovie movie = Path.GetExtension(path)?.EndsWithOrdinal("tasproj") is true
+				? new TasMovie(this, path)
+				: new Bk2Movie(this, path);
 
-			return new Bk2Movie(this, path);
+			if (loadMovie)
+				movie.Load();
+
+			return movie;
 		}
 
 		public void PopupMessage(string message) => _dialogParent.ModalMessageBox(message, "Warning", EMsgBoxIcon.Warning);

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

@@ -54,17 +54,6 @@ namespace BizHawk.Client.Common
 		/// </summary>
 		IMovieController GenerateMovieController(ControllerDefinition definition = null);
 
-		/// <summary>
-		/// Hack only used for TAStudio when starting a new movie
-		/// This is due to needing to save a "dummy" default.tasproj
-		/// This dummy file's initial save bypasses the normal queue/run
-		/// new movie code (which normally sets the controller), although
-		/// once it saves it goes through the normal queue/run code anyway.
-		/// TODO: Stop relying on this dummy file so we do not need this ugly hack
-		/// </summary>
-		/// <param name="definition">current IEmulator ControllerDefinition</param>
-		void SetMovieController(ControllerDefinition definition);
-
 		void HandleFrameBefore();
 		void HandleFrameAfter();
 		void HandleSaveState(TextWriter writer);
@@ -79,7 +68,6 @@ namespace BizHawk.Client.Common
 		/// </summary>
 		void QueueNewMovie(
 			IMovie movie,
-			bool record,
 			string systemId,
 			string loadedRomHash,
 			PathEntryCollection pathEntries,
@@ -100,7 +88,11 @@ namespace BizHawk.Client.Common
 		/// </summary>
 		void ConvertToTasProj();
 
-		IMovie Get(string path);
+		/// <summary>
+		/// Create a new (Tas)Movie with the given path as filename. If <paramref name="loadMovie"/> is true,
+		/// will also attempt to load an existing movie from <paramref name="path"/>.
+		/// </summary>
+		IMovie Get(string path, bool loadMovie = false);
 
 		string BackupDirectory { get; set; }
 

src/BizHawk.Client.EmuHawk/MainForm.FileLoader.cs

@@ -95,7 +95,7 @@ namespace BizHawk.Client.EmuHawk
 			}
 			return Tools.IsLoaded<TAStudio>()
 				? Tools.TAStudio.LoadMovieFile(filename)
-				: StartNewMovie(MovieSession.Get(filename), false);
+				: StartNewMovie(MovieSession.Get(filename, true), false);
 		}
 
 		private bool LoadRom(string filename, string archive = null)

src/BizHawk.Client.EmuHawk/MainForm.Movie.cs

@@ -29,7 +29,6 @@ namespace BizHawk.Client.EmuHawk
 				{
 					MovieSession.QueueNewMovie(
 						movie,
-						record: record,
 						systemId: Emulator.SystemId,
 						loadedRomHash: Game.Hash,
 						Config.PathEntries,

src/BizHawk.Client.EmuHawk/MainForm.cs

@@ -668,7 +668,7 @@ namespace BizHawk.Client.EmuHawk
 				// If user picked a game, then do the commandline logic
 				if (!Game.IsNullInstance())
 				{
-					var movie = MovieSession.Get(_argParser.cmdMovie);
+					var movie = MovieSession.Get(_argParser.cmdMovie, true);
 					MovieSession.ReadOnly = true;
 
 					// if user is dumping and didn't supply dump length, make it as long as the loaded movie
@@ -703,7 +703,7 @@ namespace BizHawk.Client.EmuHawk
 				{
 					if (File.Exists(Config.RecentMovies.MostRecent))
 					{
-						StartNewMovie(MovieSession.Get(Config.RecentMovies.MostRecent), false);
+						StartNewMovie(MovieSession.Get(Config.RecentMovies.MostRecent, true), false);
 					}
 					else
 					{
@@ -2284,7 +2284,7 @@ namespace BizHawk.Client.EmuHawk
 		{
 			if (File.Exists(path))
 			{
-				var movie = MovieSession.Get(path);
+				var movie = MovieSession.Get(path, true);
 				MovieSession.ReadOnly = true;
 				StartNewMovie(movie, false);
 			}

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

@@ -102,7 +102,7 @@ namespace BizHawk.Client.EmuHawk
 			var indices = MovieView.SelectedIndices;
 			if (indices.Count > 0) // Import file if necessary
 			{
-				var movie = _movieSession.Get(_movieList[MovieView.SelectedIndices[0]].Filename);
+				var movie = _movieSession.Get(_movieList[MovieView.SelectedIndices[0]].Filename, true);
 				_mainForm.StartNewMovie(movie, false);
 			}
 		}
@@ -523,8 +523,7 @@ namespace BizHawk.Client.EmuHawk
 			if (indices.Count > 0)
 			{
 				// TODO this will allocate unnecessary memory when this movie is a TasMovie due to TasStateManager
-				var movie = _movieSession.Get(_movieList[MovieView.SelectedIndices[0]].Filename);
-				movie.Load();
+				var movie = _movieSession.Get(_movieList[MovieView.SelectedIndices[0]].Filename, true);
 				var form = new EditCommentsForm(movie, readOnly: false, disposeOnClose: true);
 				form.Show();
 			}
@@ -536,8 +535,7 @@ namespace BizHawk.Client.EmuHawk
 			if (indices.Count > 0)
 			{
 				// TODO this will allocate unnecessary memory when this movie is a TasMovie due to TasStateManager
-				var movie = _movieSession.Get(_movieList[MovieView.SelectedIndices[0]].Filename);
-				movie.Load();
+				var movie = _movieSession.Get(_movieList[MovieView.SelectedIndices[0]].Filename, true);
 				var form = new EditSubtitlesForm(DialogController, movie, _config.PathEntries, readOnly: false, disposeOnClose: true);
 				form.Show();
 			}

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

@@ -267,7 +267,6 @@ namespace BizHawk.Client.EmuHawk
 					_game,
 					_firmwareManager,
 					AuthorBox.Text ?? _config.DefaultAuthor);
-				movieToRecord.Save();
 				_mainForm.StartNewMovie(movieToRecord, true);
 
 				_config.UseDefaultAuthor = DefaultAuthorCheckBox.Checked;

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

@@ -50,7 +50,7 @@ namespace BizHawk.Client.EmuHawk
 					Emulator.Frame, StatableEmulator.CloneSavestate());
 
 				MainForm.PauseEmulator();
-				LoadFile(new FileInfo(newProject.Filename), true);
+				LoadMovie(newProject, true);
 			}
 		}
 
@@ -62,7 +62,7 @@ namespace BizHawk.Client.EmuHawk
 				GoToFrame(TasView.AnyRowsSelected ? TasView.FirstSelectedRowIndex : 0);
 				var newProject = CurrentTasMovie.ConvertToSaveRamAnchoredMovie(saveRam);
 				MainForm.PauseEmulator();
-				LoadFile(new FileInfo(newProject.Filename), true);
+				LoadMovie(newProject, true);
 			}
 		}
 
@@ -100,8 +100,7 @@ namespace BizHawk.Client.EmuHawk
 			if (askToSave && !AskSaveChanges()) return false;
 			if (filename.EndsWithOrdinal(MovieService.TasMovieExtension))
 			{
-				LoadFileWithFallback(filename);
-				return true; //TODO should this return false if it fell back to a new project?
+				return LoadFileWithFallback(filename);
 			}
 			if (filename.EndsWithOrdinal(MovieService.StandardMovieExtension))
 			{
@@ -113,18 +112,8 @@ namespace BizHawk.Client.EmuHawk
 				{
 					return false;
 				}
-				_initializing = true; // Starting a new movie causes a core reboot
-				WantsToControlReboot = false;
-				_engaged = false;
-				if (!MainForm.StartNewMovie(MovieSession.Get(filename), record: false)) return false;
-				ConvertCurrentMovieToTasproj();
-				_initializing = false;
-				var success = StartNewMovieWrapper(CurrentTasMovie);
-				_engaged = true;
-				WantsToControlReboot = true;
-				SetUpColumns();
-				UpdateWindowTitle();
-				return success; //TODO is this correct?
+
+				return LoadFileWithFallback(filename);
 			}
 			DialogController.ShowMessageBox(
 				caption: "Movie load error",

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

@@ -204,11 +204,7 @@ namespace BizHawk.Client.EmuHawk
 
 		private void LoadMostRecentOrStartNew()
 		{
-			if (!LoadFile(new(Settings.RecentTas.MostRecent)))
-			{
-				TasView.AllColumns.Clear();
-				StartNewTasMovie();
-			}
+			LoadFileWithFallback(Settings.RecentTas.MostRecent);
 		}
 
 		private bool Engage()
@@ -255,7 +251,7 @@ namespace BizHawk.Client.EmuHawk
 			// Start Scenario 2: A tasproj is already active
 			else if (MovieSession.Movie.IsActive() && MovieSession.Movie is ITasMovie)
 			{
-				bool result = LoadFile(new FileInfo(CurrentTasMovie.Filename), gotoFrame: Emulator.Frame);
+				bool result = LoadMovie(CurrentTasMovie, gotoFrame: Emulator.Frame);
 				if (!result)
 				{
 					TasView.AllColumns.Clear();
@@ -520,26 +516,18 @@ namespace BizHawk.Client.EmuHawk
 			CurrentTasMovie.PropertyChanged += TasMovie_OnPropertyChanged;
 		}
 
-		private bool LoadFile(FileInfo file, bool startsFromSavestate = false, int gotoFrame = 0)
+		private bool LoadMovie(ITasMovie tasMovie, bool startsFromSavestate = false, int gotoFrame = 0)
 		{
-			if (!file.Exists)
-			{
-				Settings.RecentTas.HandleLoadError(MainForm, file.FullName);
-				return false;
-			}
-
 			_engaged = false;
-			var newMovie = (ITasMovie)MovieSession.Get(file.FullName);
-			newMovie.BindMarkersToInput = Settings.BindMarkersToInput;
-			newMovie.GreenzoneInvalidated = GreenzoneInvalidated;
+			tasMovie.BindMarkersToInput = Settings.BindMarkersToInput;
+			tasMovie.GreenzoneInvalidated = GreenzoneInvalidated;
 
-			if (!HandleMovieLoadStuff(newMovie))
+			if (!HandleMovieLoadStuff(tasMovie))
 			{
 				return false;
 			}
 
 			_engaged = true;
-			Settings.RecentTas.Add(newMovie.Filename); // only add if it did load
 
 			if (startsFromSavestate)
 			{
@@ -602,14 +590,8 @@ namespace BizHawk.Client.EmuHawk
 				Config.DefaultAuthor);
 
 			SetTasMovieCallbacks(tasMovie);
-			MovieSession.SetMovieController(Emulator.ControllerDefinition); // hack, see interface comment
-			tasMovie.ClearChanges(); // Don't ask to save changes here.
-			tasMovie.Save();
+			tasMovie.ClearChanges();
 			_ = HandleMovieLoadStuff(tasMovie);
-			// let's not keep this longer than we actually need
-			// the user will be prompted to enter a proper name
-			// when they want to save
-			File.Delete(tasMovie.Filename);
 
 			// clear all selections
 			TasView.DeselectAll();
@@ -623,7 +605,6 @@ namespace BizHawk.Client.EmuHawk
 		private bool HandleMovieLoadStuff(ITasMovie movie)
 		{
 			WantsToControlStopMovie = false;
-			WantsToControlReboot = false;
 			var result = StartNewMovieWrapper(movie);
 
 			if (!result)
@@ -632,10 +613,8 @@ namespace BizHawk.Client.EmuHawk
 			}
 
 			WantsToControlStopMovie = true;
-			WantsToControlReboot = true;
 
 			CurrentTasMovie.ChangeLog.Clear();
-			CurrentTasMovie.ClearChanges();
 
 			UpdateWindowTitle();
 			MessageStatusLabel.Text = $"{Path.GetFileName(CurrentTasMovie.Filename)} loaded.";
@@ -650,7 +629,9 @@ namespace BizHawk.Client.EmuHawk
 			SetTasMovieCallbacks(movie);
 
 			SuspendLayout();
+			WantsToControlReboot = false;
 			bool result = MainForm.StartNewMovie(movie, false);
+			WantsToControlReboot = true;
 			ResumeLayout();
 			if (result)
 			{
@@ -672,16 +653,31 @@ namespace BizHawk.Client.EmuHawk
 			}
 		}
 
-		private void LoadFileWithFallback(string path)
+		private bool LoadFileWithFallback(string path)
 		{
-			var result = LoadFile(new FileInfo(path));
-			if (!result)
+			bool movieLoadSucceeded = false;
+
+			if (!File.Exists(path))
+			{
+				Settings.RecentTas.HandleLoadError(MainForm, path);
+			}
+			else
+			{
+				var movie = MovieSession.Get(path, true);
+
+				var tasMovie = movie as ITasMovie ?? movie.ToTasMovie();
+				movieLoadSucceeded = LoadMovie(tasMovie);
+			}
+
+			if (!movieLoadSucceeded)
 			{
 				TasView.AllColumns.Clear();
 				WantsToControlReboot = false;
 				StartNewTasMovie();
 				_engaged = true;
 			}
+
+			return movieLoadSucceeded;
 		}
 
 		private void DummyLoadMacro(string path)
@@ -939,7 +935,7 @@ namespace BizHawk.Client.EmuHawk
 			if (fromLua)
 			{
 				bool wasPaused = MainForm.EmulatorPaused;
-				
+
 				// why not use this? because I'm not letting the form freely run. it all has to be under this loop.
 				// i could use this and then poll StepRunLoop_Core() repeatedly, but.. that's basically what I'm doing
 				// PauseOnFrame = frame;
@@ -1095,7 +1091,7 @@ namespace BizHawk.Client.EmuHawk
 				{
 					CurrentTasMovie.ClearFrame(i);
 				}
-				
+
 				if (needsToRollback)
 				{
 					GoToLastEmulatedFrameIfNecessary(beginningFrame);