From f47c58f5506c71f01aba35d488296316ff18debd Mon Sep 17 00:00:00 2001
From: Moritz Bender <35152647+Morilli@users.noreply.github.com>
Date: Sat, 7 Sep 2024 15:27:20 +0200
Subject: [PATCH] Fix movie header values being potentially outdated upon
starting a movie record (#4010)
* Rename StartNewMovie parameter
* Move PopulateWithDefaultHeaderValues to MainForm.Movie, make private
Author still needs to be set locally
* Make PopulateWithDefaultHeaderValues non-static
* Remove now-unnecessary firmware parameter in RecordMovie
* add TODO comment
* add Prepopulate function before movie load
Now, the main header population happens after the core is reloaded, ensuring all values are in the correct state.
A few movie values still need to be set beforehand though to ensure the core reboot works correctly.
* Allow recording movies even with outstanding reboot
Should be safe now with the previous commit.
---
.../movie/MovieConversionExtensions.cs | 132 ----------------
.../IMainFormForTools.cs | 3 +-
src/BizHawk.Client.EmuHawk/MainForm.Events.cs | 6 +-
src/BizHawk.Client.EmuHawk/MainForm.Movie.cs | 149 +++++++++++++++++-
.../movie/RecordMovie.cs | 12 +-
.../tools/TAStudio/TAStudio.cs | 8 +-
6 files changed, 153 insertions(+), 157 deletions(-)
diff --git a/src/BizHawk.Client.Common/movie/MovieConversionExtensions.cs b/src/BizHawk.Client.Common/movie/MovieConversionExtensions.cs
index 8ec5bf7fdf..85330e9081 100644
--- a/src/BizHawk.Client.Common/movie/MovieConversionExtensions.cs
+++ b/src/BizHawk.Client.Common/movie/MovieConversionExtensions.cs
@@ -4,17 +4,6 @@ using System.Linq;
using BizHawk.Common;
using BizHawk.Common.PathExtensions;
-using BizHawk.Emulation.Common;
-using BizHawk.Emulation.Cores.Arcades.MAME;
-using BizHawk.Emulation.Cores.Atari.Jaguar;
-using BizHawk.Emulation.Cores.Consoles.Nintendo.Ares64;
-using BizHawk.Emulation.Cores.Consoles.Nintendo.Gameboy;
-using BizHawk.Emulation.Cores.Consoles.Nintendo.NDS;
-using BizHawk.Emulation.Cores.Consoles.Sega.gpgx;
-using BizHawk.Emulation.Cores.Consoles.Sega.PicoDrive;
-using BizHawk.Emulation.Cores.Nintendo.NES;
-using BizHawk.Emulation.Cores.Nintendo.SubNESHawk;
-using BizHawk.Emulation.Cores.Sega.MasterSystem;
namespace BizHawk.Client.Common
{
@@ -183,127 +172,6 @@ namespace BizHawk.Client.Common
return tas;
}
- // TODO: This doesn't really belong here, but not sure where to put it
- public static void PopulateWithDefaultHeaderValues(
- this IMovie movie,
- IEmulator emulator,
- ISettingsAdapter settable,
- IGameInfo game,
- FirmwareManager firmwareManager,
- string author)
- {
- movie.Author = author;
- movie.EmulatorVersion = VersionInfo.GetEmuVersion();
- movie.OriginalEmulatorVersion = VersionInfo.GetEmuVersion();
- movie.SystemID = emulator.SystemId;
-
- if (settable.HasSyncSettings)
- {
- movie.SyncSettingsJson = ConfigService.SaveWithType(settable.GetSyncSettings());
- }
-
- movie.GameName = game.FilesystemSafeName();
- movie.Hash = game.Hash;
- if (game.FirmwareHash != null)
- {
- movie.FirmwareHash = game.FirmwareHash;
- }
-
- if (emulator.HasBoardInfo())
- {
- movie.BoardName = emulator.AsBoardInfo().BoardName;
- }
-
- if (emulator.HasRegions())
- {
- var region = emulator.AsRegionable().Region;
- if (region == DisplayType.PAL)
- {
- movie.HeaderEntries.Add(HeaderKeys.Pal, "1");
- }
- }
-
- if (firmwareManager.RecentlyServed.Count != 0)
- {
- foreach (var firmware in firmwareManager.RecentlyServed)
- {
- var key = firmware.ID.MovieHeaderKey;
- if (!movie.HeaderEntries.ContainsKey(key))
- {
- movie.HeaderEntries.Add(key, firmware.Hash);
- }
- }
- }
-
- if (emulator is NDS nds && nds.IsDSi)
- {
- movie.HeaderEntries.Add("IsDSi", "1");
-
- if (nds.IsDSiWare)
- {
- movie.HeaderEntries.Add("IsDSiWare", "1");
- }
- }
-
- if ((emulator is NES nes && nes.IsVS)
- || (emulator is SubNESHawk subnes && subnes.IsVs))
- {
- movie.HeaderEntries.Add("IsVS", "1");
- }
-
- if (emulator is IGameboyCommon gb)
- {
- //TODO doesn't IsCGBDMGMode imply IsCGBMode?
- if (gb.IsCGBMode) movie.HeaderEntries.Add(gb.IsCGBDMGMode ? "IsCGBDMGMode" : "IsCGBMode", "1");
- }
-
- if (emulator is SMS sms)
- {
- if (sms.IsSG1000)
- {
- movie.HeaderEntries.Add("IsSGMode", "1");
- }
-
- if (sms.IsGameGear)
- {
- movie.HeaderEntries.Add("IsGGMode", "1");
- }
- }
-
- if (emulator is GPGX gpgx && gpgx.IsMegaCD)
- {
- movie.HeaderEntries.Add("IsSegaCDMode", "1");
- }
-
- if (emulator is PicoDrive pico && pico.Is32XActive)
- {
- movie.HeaderEntries.Add("Is32X", "1");
- }
-
- if (emulator is VirtualJaguar jag && jag.IsJaguarCD)
- {
- movie.HeaderEntries.Add("IsJaguarCD", "1");
- }
-
- if (emulator is Ares64 ares && ares.IsDD)
- {
- movie.HeaderEntries.Add("IsDD", "1");
- }
-
- if (emulator is MAME mame)
- {
- movie.HeaderEntries.Add(HeaderKeys.VsyncAttoseconds, mame.VsyncAttoseconds.ToString());
- }
-
- if (emulator.HasCycleTiming())
- {
- movie.HeaderEntries.Add(HeaderKeys.CycleCount, "0");
- movie.HeaderEntries.Add(HeaderKeys.ClockRate, "0");
- }
-
- movie.Core = emulator.Attributes().CoreName;
- }
-
internal static string ConvertFileNameToTasMovie(string oldFileName)
{
if (oldFileName is null) return null;
diff --git a/src/BizHawk.Client.EmuHawk/IMainFormForTools.cs b/src/BizHawk.Client.EmuHawk/IMainFormForTools.cs
index d2ba2bd5ed..fec81f1e89 100644
--- a/src/BizHawk.Client.EmuHawk/IMainFormForTools.cs
+++ b/src/BizHawk.Client.EmuHawk/IMainFormForTools.cs
@@ -17,6 +17,7 @@ namespace BizHawk.Client.EmuHawk
bool EmulatorPaused { get; }
/// <remarks>only referenced from <see cref="TAStudio"/></remarks>
+ // TODO: remove? or does anything ever need access to the FirmwareManager
FirmwareManager FirmwareManager { get; }
/// <remarks>only referenced from <see cref="TAStudio"/></remarks>
@@ -81,7 +82,7 @@ namespace BizHawk.Client.EmuHawk
void SetMainformMovieInfo();
- bool StartNewMovie(IMovie movie, bool record);
+ bool StartNewMovie(IMovie movie, bool newMovie);
/// <remarks>only referenced from <see cref="BasicBot"/></remarks>
void Throttle();
diff --git a/src/BizHawk.Client.EmuHawk/MainForm.Events.cs b/src/BizHawk.Client.EmuHawk/MainForm.Events.cs
index 5fb9121a17..5bea05ba63 100644
--- a/src/BizHawk.Client.EmuHawk/MainForm.Events.cs
+++ b/src/BizHawk.Client.EmuHawk/MainForm.Events.cs
@@ -173,10 +173,6 @@ namespace BizHawk.Client.EmuHawk
= RecentMovieSubMenu.Enabled
= !Emulator.IsNull() && !Tools.IsLoaded<TAStudio>();
- // Record movie dialog should not be opened while in need of a reboot,
- // Otherwise the wrong sync settings could be set for the recording movie and cause crashes
- RecordMovieMenuItem.Enabled &= !RebootStatusBarIcon.Visible;
-
PlayFromBeginningMenuItem.Enabled = MovieSession.Movie.IsActive() && !Tools.IsLoaded<TAStudio>();
}
@@ -368,7 +364,7 @@ namespace BizHawk.Client.EmuHawk
// Nag user to user a more accurate core, but let them continue anyway
EnsureCoreIsAccurate();
- using var form = new RecordMovie(this, Config, Game, Emulator, MovieSession, FirmwareManager);
+ using var form = new RecordMovie(this, Config, Game, Emulator, MovieSession);
this.ShowDialogWithTempMute(form);
}
diff --git a/src/BizHawk.Client.EmuHawk/MainForm.Movie.cs b/src/BizHawk.Client.EmuHawk/MainForm.Movie.cs
index a4e8564e7b..e3bbcd687d 100644
--- a/src/BizHawk.Client.EmuHawk/MainForm.Movie.cs
+++ b/src/BizHawk.Client.EmuHawk/MainForm.Movie.cs
@@ -2,13 +2,24 @@ using System.Collections.Generic;
using System.Windows.Forms;
using BizHawk.Client.Common;
+using BizHawk.Common;
using BizHawk.Emulation.Common;
+using BizHawk.Emulation.Cores.Arcades.MAME;
+using BizHawk.Emulation.Cores.Atari.Jaguar;
+using BizHawk.Emulation.Cores.Consoles.Nintendo.Ares64;
+using BizHawk.Emulation.Cores.Consoles.Nintendo.Gameboy;
+using BizHawk.Emulation.Cores.Consoles.Nintendo.NDS;
+using BizHawk.Emulation.Cores.Consoles.Sega.gpgx;
+using BizHawk.Emulation.Cores.Consoles.Sega.PicoDrive;
+using BizHawk.Emulation.Cores.Nintendo.NES;
+using BizHawk.Emulation.Cores.Nintendo.SubNESHawk;
+using BizHawk.Emulation.Cores.Sega.MasterSystem;
namespace BizHawk.Client.EmuHawk
{
public partial class MainForm
{
- public bool StartNewMovie(IMovie movie, bool record)
+ public bool StartNewMovie(IMovie movie, bool newMovie)
{
if (movie is null) throw new ArgumentNullException(paramName: nameof(movie));
@@ -24,6 +35,7 @@ namespace BizHawk.Client.EmuHawk
var oldPreferredCores = new Dictionary<string, string>(Config.PreferredCores);
try
{
+ if (newMovie) PrepopulateMovieHeaderValues(movie);
try
{
MovieSession.QueueNewMovie(
@@ -48,7 +60,13 @@ namespace BizHawk.Client.EmuHawk
Config.RecentMovies.Add(movie.Filename);
- MovieSession.RunQueuedMovie(record, Emulator);
+ MovieSession.RunQueuedMovie(newMovie, Emulator);
+ if (newMovie)
+ {
+ PopulateWithDefaultHeaderValues(movie);
+ if (movie is ITasMovie tasMovie)
+ tasMovie.ClearChanges();
+ }
}
finally
{
@@ -138,5 +156,132 @@ namespace BizHawk.Client.EmuHawk
}
}
}
+
+ /// <summary>
+ /// Sets necessary movie values in order to be able to start this <paramref name="movie"/>
+ /// Only required when creating a new movie from scratch.
+ /// </summary>
+ /// <param name="movie">The movie to fill with values</param>
+ private void PrepopulateMovieHeaderValues(IMovie movie)
+ {
+ movie.Core = Emulator.Attributes().CoreName;
+ movie.SystemID = Emulator.SystemId; // TODO: I feel like setting this shouldn't be necessary, but it is currently
+
+ var settable = GetSettingsAdapterForLoadedCoreUntyped();
+ if (settable.HasSyncSettings)
+ {
+ movie.SyncSettingsJson = ConfigService.SaveWithType(settable.GetSyncSettings());
+ }
+ }
+
+ /// <summary>
+ /// Sets default header values for the given <paramref name="movie"/>. Notably needs to be done after loading the core
+ /// to make sure all values are in the correct state, see https://github.com/TASEmulators/BizHawk/issues/3980
+ /// </summary>
+ /// <param name="movie">The movie to fill with values</param>
+ private void PopulateWithDefaultHeaderValues(IMovie movie)
+ {
+ movie.EmulatorVersion = VersionInfo.GetEmuVersion();
+ movie.OriginalEmulatorVersion = VersionInfo.GetEmuVersion();
+
+ movie.GameName = Game.FilesystemSafeName();
+ movie.Hash = Game.Hash;
+ if (Game.FirmwareHash != null)
+ {
+ movie.FirmwareHash = Game.FirmwareHash;
+ }
+
+ if (Emulator.HasBoardInfo())
+ {
+ movie.BoardName = Emulator.AsBoardInfo().BoardName;
+ }
+
+ if (Emulator.HasRegions())
+ {
+ var region = Emulator.AsRegionable().Region;
+ if (region == DisplayType.PAL)
+ {
+ movie.HeaderEntries.Add(HeaderKeys.Pal, "1");
+ }
+ }
+
+ if (FirmwareManager.RecentlyServed.Count != 0)
+ {
+ foreach (var firmware in FirmwareManager.RecentlyServed)
+ {
+ var key = firmware.ID.MovieHeaderKey;
+ if (!movie.HeaderEntries.ContainsKey(key))
+ {
+ movie.HeaderEntries.Add(key, firmware.Hash);
+ }
+ }
+ }
+
+ if (Emulator is NDS nds && nds.IsDSi)
+ {
+ movie.HeaderEntries.Add("IsDSi", "1");
+
+ if (nds.IsDSiWare)
+ {
+ movie.HeaderEntries.Add("IsDSiWare", "1");
+ }
+ }
+
+ if ((Emulator is NES nes && nes.IsVS)
+ || (Emulator is SubNESHawk subnes && subnes.IsVs))
+ {
+ movie.HeaderEntries.Add("IsVS", "1");
+ }
+
+ if (Emulator is IGameboyCommon gb)
+ {
+ //TODO doesn't IsCGBDMGMode imply IsCGBMode?
+ if (gb.IsCGBMode) movie.HeaderEntries.Add(gb.IsCGBDMGMode ? "IsCGBDMGMode" : "IsCGBMode", "1");
+ }
+
+ if (Emulator is SMS sms)
+ {
+ if (sms.IsSG1000)
+ {
+ movie.HeaderEntries.Add("IsSGMode", "1");
+ }
+
+ if (sms.IsGameGear)
+ {
+ movie.HeaderEntries.Add("IsGGMode", "1");
+ }
+ }
+
+ if (Emulator is GPGX gpgx && gpgx.IsMegaCD)
+ {
+ movie.HeaderEntries.Add("IsSegaCDMode", "1");
+ }
+
+ if (Emulator is PicoDrive pico && pico.Is32XActive)
+ {
+ movie.HeaderEntries.Add("Is32X", "1");
+ }
+
+ if (Emulator is VirtualJaguar jag && jag.IsJaguarCD)
+ {
+ movie.HeaderEntries.Add("IsJaguarCD", "1");
+ }
+
+ if (Emulator is Ares64 ares && ares.IsDD)
+ {
+ movie.HeaderEntries.Add("IsDD", "1");
+ }
+
+ if (Emulator is MAME mame)
+ {
+ movie.HeaderEntries.Add(HeaderKeys.VsyncAttoseconds, mame.VsyncAttoseconds.ToString());
+ }
+
+ if (Emulator.HasCycleTiming())
+ {
+ movie.HeaderEntries.Add(HeaderKeys.CycleCount, "0");
+ movie.HeaderEntries.Add(HeaderKeys.ClockRate, "0");
+ }
+ }
}
}
diff --git a/src/BizHawk.Client.EmuHawk/movie/RecordMovie.cs b/src/BizHawk.Client.EmuHawk/movie/RecordMovie.cs
index 439772bb13..6fcf09c699 100644
--- a/src/BizHawk.Client.EmuHawk/movie/RecordMovie.cs
+++ b/src/BizHawk.Client.EmuHawk/movie/RecordMovie.cs
@@ -24,7 +24,6 @@ namespace BizHawk.Client.EmuHawk
private readonly GameInfo _game;
private readonly IEmulator _emulator;
private readonly IMovieSession _movieSession;
- private readonly FirmwareManager _firmwareManager;
private readonly TextBox AuthorBox;
@@ -41,8 +40,7 @@ namespace BizHawk.Client.EmuHawk
Config config,
GameInfo game,
IEmulator core,
- IMovieSession movieSession,
- FirmwareManager firmwareManager)
+ IMovieSession movieSession)
{
if (game.IsNullInstance()) throw new InvalidOperationException("how is the traditional Record dialog open with no game loaded? please report this including as much detail as possible");
@@ -51,7 +49,6 @@ namespace BizHawk.Client.EmuHawk
_game = game;
_emulator = core;
_movieSession = movieSession;
- _firmwareManager = firmwareManager;
SuspendLayout();
@@ -222,6 +219,7 @@ namespace BizHawk.Client.EmuHawk
}
var movieToRecord = _movieSession.Get(path);
+ movieToRecord.Author = AuthorBox.Text ?? _config.DefaultAuthor;
var fileInfo = new FileInfo(path);
if (!fileInfo.Exists)
@@ -260,12 +258,6 @@ namespace BizHawk.Client.EmuHawk
movieToRecord.SaveRam = core.CloneSaveRam();
}
- movieToRecord.PopulateWithDefaultHeaderValues(
- _emulator,
- ((MainForm) _mainForm).GetSettingsAdapterForLoadedCoreUntyped(), //HACK
- _game,
- _firmwareManager,
- AuthorBox.Text ?? _config.DefaultAuthor);
_mainForm.StartNewMovie(movieToRecord, true);
_config.UseDefaultAuthor = DefaultAuthorCheckBox.Checked;
diff --git a/src/BizHawk.Client.EmuHawk/tools/TAStudio/TAStudio.cs b/src/BizHawk.Client.EmuHawk/tools/TAStudio/TAStudio.cs
index e9773faee5..684d75d523 100644
--- a/src/BizHawk.Client.EmuHawk/tools/TAStudio/TAStudio.cs
+++ b/src/BizHawk.Client.EmuHawk/tools/TAStudio/TAStudio.cs
@@ -580,18 +580,12 @@ namespace BizHawk.Client.EmuHawk
var filename = DefaultTasProjName(); // TODO don't do this, take over any mainform actions that can crash without a filename
var tasMovie = (ITasMovie)MovieSession.Get(filename);
+ tasMovie.Author = Config.DefaultAuthor;
tasMovie.BindMarkersToInput = Settings.BindMarkersToInput;
tasMovie.GreenzoneInvalidated = GreenzoneInvalidated;
tasMovie.PropertyChanged += TasMovie_OnPropertyChanged;
- tasMovie.PopulateWithDefaultHeaderValues(
- Emulator,
- ((MainForm) MainForm).GetSettingsAdapterForLoadedCoreUntyped(), //HACK
- Game,
- MainForm.FirmwareManager,
- Config.DefaultAuthor);
-
_ = StartNewMovieWrapper(tasMovie, isNew: true);
// clear all selections