Merge branch 'develop' into stable

5 files changed, 154 insertions, 53 deletions

diff --git a/src/SMAPI/Patches/DialogueErrorPatch.cs b/src/SMAPI/Patches/DialogueErrorPatch.cs
index f1c25c05..24f97259 100644
--- a/src/SMAPI/Patches/DialogueErrorPatch.cs
+++ b/src/SMAPI/Patches/DialogueErrorPatch.cs
@@ -10,6 +10,9 @@ using StardewValley;
 namespace StardewModdingAPI.Patches
 {
     /// <summary>A Harmony patch for the <see cref="Dialogue"/> constructor which intercepts invalid dialogue lines and logs an error instead of crashing.</summary>
+    /// <remarks>Patch methods must be static for Harmony to work correctly. See the Harmony documentation before renaming patch arguments.</remarks>
+    [SuppressMessage("ReSharper", "InconsistentNaming", Justification = "Argument names are defined by Harmony and methods are named for clarity.")]
+    [SuppressMessage("ReSharper", "IdentifierTypo", Justification = "Argument names are defined by Harmony and methods are named for clarity.")]
     internal class DialogueErrorPatch : IHarmonyPatch
     {
         /*********
@@ -29,7 +32,7 @@ namespace StardewModdingAPI.Patches
         ** Accessors
         *********/
         /// <summary>A unique name for this patch.</summary>
-        public string Name => $"{nameof(DialogueErrorPatch)}";
+        public string Name => nameof(DialogueErrorPatch);
 
 
         /*********
@@ -68,8 +71,6 @@ namespace StardewModdingAPI.Patches
         /// <param name="masterDialogue">The dialogue being parsed.</param>
         /// <param name="speaker">The NPC for which the dialogue is being parsed.</param>
         /// <returns>Returns whether to execute the original method.</returns>
-        /// <remarks>This method must be static for Harmony to work correctly. See the Harmony documentation before renaming arguments.</remarks>
-        [SuppressMessage("ReSharper", "InconsistentNaming", Justification = "Argument names are defined by Harmony.")]
         private static bool Before_Dialogue_Constructor(Dialogue __instance, string masterDialogue, NPC speaker)
         {
             // get private members
@@ -109,8 +110,6 @@ namespace StardewModdingAPI.Patches
         /// <param name="__result">The return value of the original method.</param>
         /// <param name="__originalMethod">The method being wrapped.</param>
         /// <returns>Returns whether to execute the original method.</returns>
-        /// <remarks>This method must be static for Harmony to work correctly. See the Harmony documentation before renaming arguments.</remarks>
-        [SuppressMessage("ReSharper", "InconsistentNaming", Justification = "Argument names are defined by Harmony.")]
         private static bool Before_NPC_CurrentDialogue(NPC __instance, ref Stack<Dialogue> __result, MethodInfo __originalMethod)
         {
             if (DialogueErrorPatch.IsInterceptingCurrentDialogue)
diff --git a/src/SMAPI/Patches/EventErrorPatch.cs b/src/SMAPI/Patches/EventErrorPatch.cs
index cd530616..1dc7e8c3 100644
--- a/src/SMAPI/Patches/EventErrorPatch.cs
+++ b/src/SMAPI/Patches/EventErrorPatch.cs
@@ -7,6 +7,9 @@ using StardewValley;
 namespace StardewModdingAPI.Patches
 {
     /// <summary>A Harmony patch for the <see cref="Dialogue"/> constructor which intercepts invalid dialogue lines and logs an error instead of crashing.</summary>
+    /// <remarks>Patch methods must be static for Harmony to work correctly. See the Harmony documentation before renaming patch arguments.</remarks>
+    [SuppressMessage("ReSharper", "InconsistentNaming", Justification = "Argument names are defined by Harmony and methods are named for clarity.")]
+    [SuppressMessage("ReSharper", "IdentifierTypo", Justification = "Argument names are defined by Harmony and methods are named for clarity.")]
     internal class EventErrorPatch : IHarmonyPatch
     {
         /*********
@@ -23,7 +26,7 @@ namespace StardewModdingAPI.Patches
         ** Accessors
         *********/
         /// <summary>A unique name for this patch.</summary>
-        public string Name => $"{nameof(EventErrorPatch)}";
+        public string Name => nameof(EventErrorPatch);
 
 
         /*********
@@ -56,8 +59,6 @@ namespace StardewModdingAPI.Patches
         /// <param name="precondition">The precondition to be parsed.</param>
         /// <param name="__originalMethod">The method being wrapped.</param>
         /// <returns>Returns whether to execute the original method.</returns>
-        /// <remarks>This method must be static for Harmony to work correctly. See the Harmony documentation before renaming arguments.</remarks>
-        [SuppressMessage("ReSharper", "InconsistentNaming", Justification = "Argument names are defined by Harmony.")]
         private static bool Before_GameLocation_CheckEventPrecondition(GameLocation __instance, ref int __result, string precondition, MethodInfo __originalMethod)
         {
             if (EventErrorPatch.IsIntercepted)
diff --git a/src/SMAPI/Patches/LoadContextPatch.cs b/src/SMAPI/Patches/LoadContextPatch.cs
index 3f86c9a9..0cc8c8eb 100644
--- a/src/SMAPI/Patches/LoadContextPatch.cs
+++ b/src/SMAPI/Patches/LoadContextPatch.cs
@@ -1,17 +1,19 @@
 using System;
-using System.Collections.ObjectModel;
-using System.Collections.Specialized;
+using System.Diagnostics.CodeAnalysis;
 using Harmony;
 using StardewModdingAPI.Enums;
 using StardewModdingAPI.Framework.Patching;
 using StardewModdingAPI.Framework.Reflection;
 using StardewValley;
 using StardewValley.Menus;
+using StardewValley.Minigames;
 
 namespace StardewModdingAPI.Patches
 {
-    /// <summary>A Harmony patch for <see cref="Game1.loadForNewGame"/> which notifies SMAPI for save creation load stages.</summary>
-    /// <remarks>This patch hooks into <see cref="Game1.loadForNewGame"/>, checks if <c>TitleMenu.transitioningCharacterCreationMenu</c> is true (which means the player is creating a new save file), then raises <see cref="LoadStage.CreatedBasicInfo"/> after the location list is cleared twice (the second clear happens right before locations are created), and <see cref="LoadStage.CreatedLocations"/> when the method ends.</remarks>
+    /// <summary>Harmony patches which notify SMAPI for save creation load stages.</summary>
+    /// <remarks>Patch methods must be static for Harmony to work correctly. See the Harmony documentation before renaming patch arguments.</remarks>
+    [SuppressMessage("ReSharper", "InconsistentNaming", Justification = "Argument names are defined by Harmony and methods are named for clarity.")]
+    [SuppressMessage("ReSharper", "IdentifierTypo", Justification = "Argument names are defined by Harmony and methods are named for clarity.")]
     internal class LoadContextPatch : IHarmonyPatch
     {
         /*********
@@ -23,18 +25,12 @@ namespace StardewModdingAPI.Patches
         /// <summary>A callback to invoke when the load stage changes.</summary>
         private static Action<LoadStage> OnStageChanged;
 
-        /// <summary>Whether <see cref="Game1.loadForNewGame"/> was called as part of save creation.</summary>
-        private static bool IsCreating;
-
-        /// <summary>The number of times that <see cref="Game1.locations"/> has been cleared since <see cref="Game1.loadForNewGame"/> started.</summary>
-        private static int TimesLocationsCleared;
-
 
         /*********
         ** Accessors
         *********/
         /// <summary>A unique name for this patch.</summary>
-        public string Name => $"{nameof(LoadContextPatch)}";
+        public string Name => nameof(LoadContextPatch);
 
 
         /*********
@@ -53,9 +49,15 @@ namespace StardewModdingAPI.Patches
         /// <param name="harmony">The Harmony instance.</param>
         public void Apply(HarmonyInstance harmony)
         {
+            // detect CreatedBasicInfo
+            harmony.Patch(
+                original: AccessTools.Method(typeof(TitleMenu), nameof(TitleMenu.createdNewCharacter)),
+                prefix: new HarmonyMethod(this.GetType(), nameof(LoadContextPatch.Before_TitleMenu_CreatedNewCharacter))
+            );
+
+            // detect CreatedLocations
             harmony.Patch(
                 original: AccessTools.Method(typeof(Game1), nameof(Game1.loadForNewGame)),
-                prefix: new HarmonyMethod(this.GetType(), nameof(LoadContextPatch.Before_Game1_LoadForNewGame)),
                 postfix: new HarmonyMethod(this.GetType(), nameof(LoadContextPatch.After_Game1_LoadForNewGame))
             );
         }
@@ -64,45 +66,25 @@ namespace StardewModdingAPI.Patches
         /*********
         ** Private methods
         *********/
-        /// <summary>The method to call instead of <see cref="Game1.loadForNewGame"/>.</summary>
+        /// <summary>Called before <see cref="TitleMenu.createdNewCharacter"/>.</summary>
         /// <returns>Returns whether to execute the original method.</returns>
         /// <remarks>This method must be static for Harmony to work correctly. See the Harmony documentation before renaming arguments.</remarks>
-        private static bool Before_Game1_LoadForNewGame()
+        private static bool Before_TitleMenu_CreatedNewCharacter()
         {
-            LoadContextPatch.IsCreating = Game1.activeClickableMenu is TitleMenu menu && LoadContextPatch.Reflection.GetField<bool>(menu, "transitioningCharacterCreationMenu").GetValue();
-            LoadContextPatch.TimesLocationsCleared = 0;
-            if (LoadContextPatch.IsCreating)
-            {
-                // raise CreatedBasicInfo after locations are cleared twice
-                ObservableCollection<GameLocation> locations = (ObservableCollection<GameLocation>)Game1.locations;
-                locations.CollectionChanged += LoadContextPatch.OnLocationListChanged;
-            }
-
+            LoadContextPatch.OnStageChanged(LoadStage.CreatedBasicInfo);
             return true;
         }
 
-        /// <summary>The method to call instead after <see cref="Game1.loadForNewGame"/>.</summary>
+        /// <summary>Called after <see cref="Game1.loadForNewGame"/>.</summary>
         /// <remarks>This method must be static for Harmony to work correctly. See the Harmony documentation before renaming arguments.</remarks>
         private static void After_Game1_LoadForNewGame()
         {
-            if (LoadContextPatch.IsCreating)
-            {
-                // clean up
-                ObservableCollection<GameLocation> locations = (ObservableCollection<GameLocation>)Game1.locations;
-                locations.CollectionChanged -= LoadContextPatch.OnLocationListChanged;
+            bool creating =
+                (Game1.currentMinigame is Intro) // creating save with intro
+                || (Game1.activeClickableMenu is TitleMenu menu && LoadContextPatch.Reflection.GetField<bool>(menu, "transitioningCharacterCreationMenu").GetValue()); // creating save, skipped intro
 
-                // raise stage changed
+            if (creating)
                 LoadContextPatch.OnStageChanged(LoadStage.CreatedLocations);
-            }
-        }
-
-        /// <summary>Raised when <see cref="Game1.locations"/> changes.</summary>
-        /// <param name="sender">The event sender.</param>
-        /// <param name="e">The event arguments.</param>
-        private static void OnLocationListChanged(object sender, NotifyCollectionChangedEventArgs e)
-        {
-            if (++LoadContextPatch.TimesLocationsCleared == 2)
-                LoadContextPatch.OnStageChanged(LoadStage.CreatedBasicInfo);
         }
     }
 }
diff --git a/src/SMAPI/Patches/LoadErrorPatch.cs b/src/SMAPI/Patches/LoadErrorPatch.cs
new file mode 100644
index 00000000..eedb4164
--- /dev/null
+++ b/src/SMAPI/Patches/LoadErrorPatch.cs
@@ -0,0 +1,120 @@
+using System;
+using System.Collections.Generic;
+using System.Diagnostics.CodeAnalysis;
+using System.Linq;
+using Harmony;
+using StardewModdingAPI.Framework.Patching;
+using StardewValley;
+using StardewValley.Locations;
+
+namespace StardewModdingAPI.Patches
+{
+    /// <summary>A Harmony patch for <see cref="SaveGame"/> which prevents some errors due to broken save data.</summary>
+    /// <remarks>Patch methods must be static for Harmony to work correctly. See the Harmony documentation before renaming patch arguments.</remarks>
+    [SuppressMessage("ReSharper", "InconsistentNaming", Justification = "Argument names are defined by Harmony and methods are named for clarity.")]
+    [SuppressMessage("ReSharper", "IdentifierTypo", Justification = "Argument names are defined by Harmony and methods are named for clarity.")]
+    internal class LoadErrorPatch : IHarmonyPatch
+    {
+        /*********
+        ** Fields
+        *********/
+        /// <summary>Writes messages to the console and log file.</summary>
+        private static IMonitor Monitor;
+
+        /// <summary>A callback invoked when custom content is removed from the save data to avoid a crash.</summary>
+        private static Action OnContentRemoved;
+
+
+        /*********
+        ** Accessors
+        *********/
+        /// <summary>A unique name for this patch.</summary>
+        public string Name => nameof(LoadErrorPatch);
+
+
+        /*********
+        ** Public methods
+        *********/
+        /// <summary>Construct an instance.</summary>
+        /// <param name="monitor">Writes messages to the console and log file.</param>
+        /// <param name="onContentRemoved">A callback invoked when custom content is removed from the save data to avoid a crash.</param>
+        public LoadErrorPatch(IMonitor monitor, Action onContentRemoved)
+        {
+            LoadErrorPatch.Monitor = monitor;
+            LoadErrorPatch.OnContentRemoved = onContentRemoved;
+        }
+
+
+        /// <summary>Apply the Harmony patch.</summary>
+        /// <param name="harmony">The Harmony instance.</param>
+        public void Apply(HarmonyInstance harmony)
+        {
+            harmony.Patch(
+                original: AccessTools.Method(typeof(SaveGame), nameof(SaveGame.loadDataToLocations)),
+                prefix: new HarmonyMethod(this.GetType(), nameof(LoadErrorPatch.Before_SaveGame_LoadDataToLocations))
+            );
+        }
+
+
+        /*********
+        ** Private methods
+        *********/
+        /// <summary>The method to call instead of <see cref="SaveGame.loadDataToLocations"/>.</summary>
+        /// <param name="gamelocations">The game locations being loaded.</param>
+        /// <returns>Returns whether to execute the original method.</returns>
+        private static bool Before_SaveGame_LoadDataToLocations(List<GameLocation> gamelocations)
+        {
+            bool removedAny = false;
+
+            // remove invalid locations
+            foreach (GameLocation location in gamelocations.ToArray())
+            {
+                if (location is Cellar)
+                    continue; // missing cellars will be added by the game code
+
+                if (Game1.getLocationFromName(location.name) == null)
+                {
+                    LoadErrorPatch.Monitor.Log($"Removed invalid location '{location.Name}' to avoid a crash when loading save '{Constants.SaveFolderName}'. (Did you remove a custom location mod?)", LogLevel.Warn);
+                    gamelocations.Remove(location);
+                    removedAny = true;
+                }
+            }
+
+            // get building interiors
+            var interiors =
+                (
+                    from location in gamelocations.OfType<BuildableGameLocation>()
+                    from building in location.buildings
+                    where building.indoors.Value != null
+                    select building.indoors.Value
+                );
+
+            // remove custom NPCs which no longer exist
+            IDictionary<string, string> data = Game1.content.Load<Dictionary<string, string>>("Data\\NPCDispositions");
+            foreach (GameLocation location in gamelocations.Concat(interiors))
+            {
+                foreach (NPC npc in location.characters.ToArray())
+                {
+                    if (npc.isVillager() && !data.ContainsKey(npc.Name))
+                    {
+                        try
+                        {
+                            npc.reloadSprite(); // this won't crash for special villagers like Bouncer
+                        }
+                        catch
+                        {
+                            LoadErrorPatch.Monitor.Log($"Removed invalid villager '{npc.Name}' to avoid a crash when loading save '{Constants.SaveFolderName}'. (Did you remove a custom NPC mod?)", LogLevel.Warn);
+                            location.characters.Remove(npc);
+                            removedAny = true;
+                        }
+                    }
+                }
+            }
+
+            if (removedAny)
+                LoadErrorPatch.OnContentRemoved();
+
+            return true;
+        }
+    }
+}
diff --git a/src/SMAPI/Patches/ObjectErrorPatch.cs b/src/SMAPI/Patches/ObjectErrorPatch.cs
index 5b918d39..d716b29b 100644
--- a/src/SMAPI/Patches/ObjectErrorPatch.cs
+++ b/src/SMAPI/Patches/ObjectErrorPatch.cs
@@ -8,13 +8,16 @@ using SObject = StardewValley.Object;
 namespace StardewModdingAPI.Patches
 {
     /// <summary>A Harmony patch for <see cref="SObject.getDescription"/> which intercepts crashes due to the item no longer existing.</summary>
+    /// <remarks>Patch methods must be static for Harmony to work correctly. See the Harmony documentation before renaming patch arguments.</remarks>
+    [SuppressMessage("ReSharper", "InconsistentNaming", Justification = "Argument names are defined by Harmony and methods are named for clarity.")]
+    [SuppressMessage("ReSharper", "IdentifierTypo", Justification = "Argument names are defined by Harmony and methods are named for clarity.")]
     internal class ObjectErrorPatch : IHarmonyPatch
     {
         /*********
         ** Accessors
         *********/
         /// <summary>A unique name for this patch.</summary>
-        public string Name => $"{nameof(ObjectErrorPatch)}";
+        public string Name => nameof(ObjectErrorPatch);
 
 
         /*********
@@ -45,8 +48,6 @@ namespace StardewModdingAPI.Patches
         /// <param name="__instance">The instance being patched.</param>
         /// <param name="__result">The patched method's return value.</param>
         /// <returns>Returns whether to execute the original method.</returns>
-        /// <remarks>This method must be static for Harmony to work correctly. See the Harmony documentation before renaming arguments.</remarks>
-        [SuppressMessage("ReSharper", "InconsistentNaming", Justification = "Argument names are defined by Harmony.")]
         private static bool Before_Object_GetDescription(SObject __instance, ref string __result)
         {
             // invalid bigcraftables crash instead of showing '???' like invalid non-bigcraftables
@@ -63,8 +64,6 @@ namespace StardewModdingAPI.Patches
         /// <param name="__instance">The instance being patched.</param>
         /// <param name="hoveredItem">The item for which to draw a tooltip.</param>
         /// <returns>Returns whether to execute the original method.</returns>
-        /// <remarks>This method must be static for Harmony to work correctly. See the Harmony documentation before renaming arguments.</remarks>
-        [SuppressMessage("ReSharper", "InconsistentNaming", Justification = "Argument names are defined by Harmony.")]
         private static bool Before_IClickableMenu_DrawTooltip(IClickableMenu __instance, Item hoveredItem)
         {
             // invalid edible item cause crash when drawing tooltips