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 @@ public virtual string FirmwareHash
 
 		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 @@ public IMovieController GenerateMovieController(ControllerDefinition definition
 			return new Bk2Controller("", definition ?? MovieController.Definition);
 		}
 
-		public void SetMovieController(ControllerDefinition definition)
-		{
-			MovieController = GenerateMovieController(definition);
-		}
-
 		public void HandleFrameBefore()
 		{
 			if (Movie.NotActive())
@@ -189,72 +184,49 @@ public bool HandleLoadState(TextReader reader)
 			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)
-			{
-				movie.Save();
-			}
-
-			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 (movie.SystemID != systemId)
 			{
-				movie.Load();
-
-				if (movie.SystemID != systemId)
-				{
-					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();
-					}
-				}
+				throw new MoviePlatformMismatchException(
+					$"Movie system Id ({movie.SystemID}) does not match the currently loaded platform ({systemId}), unable to load");
 			}
 
-			if (!record)
+			if (!(string.IsNullOrEmpty(movie.Hash) || loadedRomHash.Equals(movie.Hash, StringComparison.Ordinal))
+				&& movie is TasMovie tasproj)
 			{
-				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 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)
 				{
-					var keys = preferredCores.Keys.ToList();
-					foreach (var k in keys)
-					{
-						preferredCores[k] = movie.Core;
-					}
+					tasproj.TasSession.UpdateValues(frame: 0, currentBranch: tasproj.TasSession.CurrentBranch); // wtf is this API --yoshi
+					tasproj.InvalidateEntireGreenzone();
 				}
 			}
 
-			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
+			if (string.IsNullOrWhiteSpace(movie.Core))
 			{
-				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 @@ public void ConvertToTasProj()
 			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);
+
+			if (loadMovie)
+				movie.Load();
 
-			return new Bk2Movie(this, path);
+			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 @@ public interface IMovieSession
 		/// </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 @@ public interface IMovieSession
 		/// </summary>
 		void QueueNewMovie(
 			IMovie movie,
-			bool record,
 			string systemId,
 			string loadedRomHash,
 			PathEntryCollection pathEntries,
@@ -100,7 +88,11 @@ void QueueNewMovie(
 		/// </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 @@ public bool LoadMovie(string filename, string archive = null)
 			}
 			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 @@ public bool StartNewMovie(IMovie movie, bool record)
 				{
 					MovieSession.QueueNewMovie(
 						movie,
-						record: record,
 						systemId: Emulator.SystemId,
 						loadedRomHash: Game.Hash,
 						Config.PathEntries,

src/BizHawk.Client.EmuHawk/MainForm.cs

@@ -668,7 +668,7 @@ _argParser.SocketAddress is var (socketIP, socketPort)
 				// 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 @@ _argParser.SocketAddress is var (socketIP, socketPort)
 				{
 					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 @@ private void LoadMoviesFromRecent(string path)
 		{
 			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 @@ private void Run()
 			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 @@ private void CommentsBtn_Click(object sender, EventArgs e)
 			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 @@ private void SubtitlesBtn_Click(object sender, EventArgs e)
 			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 @@ private void Ok_Click(object sender, EventArgs e)
 					_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 @@ private void StartNewProjectFromNowMenuItem_Click(object sender, EventArgs e)
 					Emulator.Frame, StatableEmulator.CloneSavestate());
 
 				MainForm.PauseEmulator();
-				LoadFile(new FileInfo(newProject.Filename), true);
+				LoadMovie(newProject, true);
 			}
 		}
 
@@ -62,7 +62,7 @@ private void StartANewProjectFromSaveRamMenuItem_Click(object sender, EventArgs
 				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 @@ public bool LoadMovieFile(string filename, bool askToSave = true)
 			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 @@ public bool LoadMovieFile(string filename, bool askToSave = true)
 				{
 					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",