rework tilesheet loading to improve errors, allow future validation, and drop support for legacy content files

3 files changed, 80 insertions, 70 deletions

diff --git a/src/SMAPI/Framework/ContentCoordinator.cs b/src/SMAPI/Framework/ContentCoordinator.cs
index 2fd31263..0b1ccc3c 100644
--- a/src/SMAPI/Framework/ContentCoordinator.cs
+++ b/src/SMAPI/Framework/ContentCoordinator.cs
@@ -112,9 +112,10 @@ namespace StardewModdingAPI.Framework
 
         /// <summary>Get a new content manager which handles reading files from a SMAPI mod folder with support for unpacked files.</summary>
         /// <param name="name">A name for the mod manager. Not guaranteed to be unique.</param>
+        /// <param name="modName">The mod display name to show in errors.</param>
         /// <param name="rootDirectory">The root directory to search for content (or <c>null</c> for the default).</param>
         /// <param name="gameContentManager">The game content manager used for map tilesheets not provided by the mod.</param>
-        public ModContentManager CreateModContentManager(string name, string rootDirectory, IContentManager gameContentManager)
+        public ModContentManager CreateModContentManager(string name, string modName, string rootDirectory, IContentManager gameContentManager)
         {
             return this.ContentManagerLock.InWriteLock(() =>
             {
@@ -123,6 +124,7 @@ namespace StardewModdingAPI.Framework
                     gameContentManager: gameContentManager,
                     serviceProvider: this.MainContentManager.ServiceProvider,
                     rootDirectory: rootDirectory,
+                    modName: modName,
                     currentCulture: this.MainContentManager.CurrentCulture,
                     coordinator: this,
                     monitor: this.Monitor,
diff --git a/src/SMAPI/Framework/ContentManagers/ModContentManager.cs b/src/SMAPI/Framework/ContentManagers/ModContentManager.cs
index 0a526fc8..6b463424 100644
--- a/src/SMAPI/Framework/ContentManagers/ModContentManager.cs
+++ b/src/SMAPI/Framework/ContentManagers/ModContentManager.cs
@@ -26,6 +26,9 @@ namespace StardewModdingAPI.Framework.ContentManagers
         /// <summary>Encapsulates SMAPI's JSON file parsing.</summary>
         private readonly JsonHelper JsonHelper;
 
+        /// <summary>The mod display name to show in errors.</summary>
+        private readonly string ModName;
+
         /// <summary>The game content manager used for map tilesheets not provided by the mod.</summary>
         private readonly IContentManager GameContentManager;
 
@@ -40,6 +43,7 @@ namespace StardewModdingAPI.Framework.ContentManagers
         /// <param name="name">A name for the mod manager. Not guaranteed to be unique.</param>
         /// <param name="gameContentManager">The game content manager used for map tilesheets not provided by the mod.</param>
         /// <param name="serviceProvider">The service provider to use to locate services.</param>
+        /// <param name="modName">The mod display name to show in errors.</param>
         /// <param name="rootDirectory">The root directory to search for content.</param>
         /// <param name="currentCulture">The current culture for which to localize content.</param>
         /// <param name="coordinator">The central coordinator which manages content managers.</param>
@@ -47,11 +51,12 @@ namespace StardewModdingAPI.Framework.ContentManagers
         /// <param name="reflection">Simplifies access to private code.</param>
         /// <param name="jsonHelper">Encapsulates SMAPI's JSON file parsing.</param>
         /// <param name="onDisposing">A callback to invoke when the content manager is being disposed.</param>
-        public ModContentManager(string name, IContentManager gameContentManager, IServiceProvider serviceProvider, string rootDirectory, CultureInfo currentCulture, ContentCoordinator coordinator, IMonitor monitor, Reflector reflection, JsonHelper jsonHelper, Action<BaseContentManager> onDisposing)
+        public ModContentManager(string name, IContentManager gameContentManager, IServiceProvider serviceProvider, string modName, string rootDirectory, CultureInfo currentCulture, ContentCoordinator coordinator, IMonitor monitor, Reflector reflection, JsonHelper jsonHelper, Action<BaseContentManager> onDisposing)
             : base(name, serviceProvider, rootDirectory, currentCulture, coordinator, monitor, reflection, onDisposing, isNamespaced: true)
         {
             this.GameContentManager = gameContentManager;
             this.JsonHelper = jsonHelper;
+            this.ModName = modName;
         }
 
         /// <summary>Load an asset that has been processed by the content pipeline.</summary>
@@ -297,98 +302,99 @@ namespace StardewModdingAPI.Framework.ContentManagers
             foreach (TileSheet tilesheet in map.TileSheets)
             {
                 string imageSource = tilesheet.ImageSource;
+                string errorPrefix = $"{this.ModName} loaded map '{relativeMapPath}' with invalid tilesheet path '{imageSource}'.";
 
                 // validate tilesheet path
                 if (Path.IsPathRooted(imageSource) || PathUtilities.GetSegments(imageSource).Contains(".."))
-                    throw new ContentLoadException($"The '{imageSource}' tilesheet couldn't be loaded. Tilesheet paths must be a relative path without directory climbing (../).");
-
-                // get seasonal name (if applicable)
-                string seasonalImageSource = null;
-                if (isOutdoors && Context.IsSaveLoaded && Game1.currentSeason != null)
-                {
-                    string filename = Path.GetFileName(imageSource) ?? throw new InvalidOperationException($"The '{imageSource}' tilesheet couldn't be loaded: filename is unexpectedly null.");
-                    bool hasSeasonalPrefix =
-                        filename.StartsWith("spring_", StringComparison.CurrentCultureIgnoreCase)
-                        || filename.StartsWith("summer_", StringComparison.CurrentCultureIgnoreCase)
-                        || filename.StartsWith("fall_", StringComparison.CurrentCultureIgnoreCase)
-                        || filename.StartsWith("winter_", StringComparison.CurrentCultureIgnoreCase);
-                    if (hasSeasonalPrefix && !filename.StartsWith(Game1.currentSeason + "_"))
-                    {
-                        string dirPath = imageSource.Substring(0, imageSource.LastIndexOf(filename, StringComparison.CurrentCultureIgnoreCase));
-                        seasonalImageSource = $"{dirPath}{Game1.currentSeason}_{filename.Substring(filename.IndexOf("_", StringComparison.CurrentCultureIgnoreCase) + 1)}";
-                    }
-                }
+                    throw new SContentLoadException($"{errorPrefix} Tilesheet paths must be a relative path without directory climbing (../).");
 
                 // load best match
                 try
                 {
-                    string key =
-                        this.GetTilesheetAssetName(relativeMapFolder, seasonalImageSource)
-                        ?? this.GetTilesheetAssetName(relativeMapFolder, imageSource);
-                    if (key != null)
-                    {
-                        tilesheet.ImageSource = key;
-                        continue;
-                    }
+                    if (!this.TryGetTilesheetAssetName(relativeMapFolder, imageSource, isOutdoors, out string assetName, out string error))
+                        throw new SContentLoadException($"{errorPrefix} {error}");
+
+                    tilesheet.ImageSource = assetName;
                 }
-                catch (Exception ex)
+                catch (Exception ex) when (!(ex is SContentLoadException))
                 {
-                    throw new ContentLoadException($"The '{imageSource}' tilesheet couldn't be loaded relative to either map file or the game's content folder.", ex);
+                    throw new SContentLoadException($"{errorPrefix} The tilesheet couldn't be loaded.", ex);
                 }
-
-                // none found
-                throw new ContentLoadException($"The '{imageSource}' tilesheet couldn't be loaded relative to either map file or the game's content folder.");
             }
         }
 
         /// <summary>Get the actual asset name for a tilesheet.</summary>
         /// <param name="modRelativeMapFolder">The folder path containing the map, relative to the mod folder.</param>
-        /// <param name="imageSource">The tilesheet image source to load.</param>
-        /// <returns>Returns the asset name.</returns>
+        /// <param name="originalPath">The tilesheet path to load.</param>
+        /// <param name="willSeasonalize">Whether the game will apply seasonal logic to the tilesheet.</param>
+        /// <param name="assetName">The found asset name.</param>
+        /// <param name="error">A message indicating why the file couldn't be loaded.</param>
+        /// <returns>Returns whether the asset name was found.</returns>
         /// <remarks>See remarks on <see cref="FixCustomTilesheetPaths"/>.</remarks>
-        private string GetTilesheetAssetName(string modRelativeMapFolder, string imageSource)
+        private bool TryGetTilesheetAssetName(string modRelativeMapFolder, string originalPath, bool willSeasonalize, out string assetName, out string error)
         {
-            if (imageSource == null)
-                return null;
+            assetName = null;
+            error = null;
+
+            // nothing to do
+            if (string.IsNullOrWhiteSpace(originalPath))
+            {
+                assetName = originalPath;
+                return true;
+            }
 
-            // check relative to map file
+            // parse path
+            string filename = Path.GetFileName(originalPath);
+            bool isSeasonal = filename.StartsWith("spring_", StringComparison.CurrentCultureIgnoreCase)
+                || filename.StartsWith("summer_", StringComparison.CurrentCultureIgnoreCase)
+                || filename.StartsWith("fall_", StringComparison.CurrentCultureIgnoreCase)
+                || filename.StartsWith("winter_", StringComparison.CurrentCultureIgnoreCase);
+            string relativePath = originalPath;
+            if (willSeasonalize && isSeasonal)
             {
-                string localKey = Path.Combine(modRelativeMapFolder, imageSource);
-                FileInfo localFile = this.GetModFile(localKey);
-                if (localFile.Exists)
-                    return this.GetInternalAssetKey(localKey);
+                string dirPath = Path.GetDirectoryName(originalPath);
+                relativePath = Path.Combine(dirPath, $"{Game1.currentSeason}_{filename.Substring(filename.IndexOf("_", StringComparison.CurrentCultureIgnoreCase) + 1)}");
             }
 
-            // check relative to content folder
+            // get relative to map file
             {
-                foreach (string candidateKey in new[] { imageSource, Path.Combine("Maps", imageSource) })
+                string localKey = Path.Combine(modRelativeMapFolder, relativePath);
+                if (this.GetModFile(localKey).Exists)
+                {
+                    assetName = this.GetInternalAssetKey(localKey);
+                    return true;
+                }
+            }
+
+            // get from game assets
+            {
+                string contentKey = Path.Combine("Maps", relativePath);
+                if (contentKey.EndsWith(".png"))
+                    contentKey = contentKey.Substring(0, contentKey.Length - 4);
+
+                try
+                {
+                    this.GameContentManager.Load<Texture2D>(contentKey, this.Language, useCache: true); // no need to bypass cache here, since we're not storing the asset
+                    assetName = contentKey;
+                    return true;
+                }
+                catch
                 {
-                    string contentKey = candidateKey.EndsWith(".png")
-                        ? candidateKey.Substring(0, candidateKey.Length - 4)
-                        : candidateKey;
-
-                    try
-                    {
-                        this.GameContentManager.Load<Texture2D>(contentKey, this.Language, useCache: true); // no need to bypass cache here, since we're not storing the asset
-                        return contentKey;
-                    }
-                    catch
-                    {
-                        // ignore file-not-found errors
-                        // TODO: while it's useful to suppress an asset-not-found error here to avoid
-                        // confusion, this is a pretty naive approach. Even if the file doesn't exist,
-                        // the file may have been loaded through an IAssetLoader which failed. So even
-                        // if the content file doesn't exist, that doesn't mean the error here is a
-                        // content-not-found error. Unfortunately XNA doesn't provide a good way to
-                        // detect the error type.
-                        if (this.GetContentFolderFileExists(contentKey))
-                            throw;
-                    }
+                    // ignore file-not-found errors
+                    // TODO: while it's useful to suppress an asset-not-found error here to avoid
+                    // confusion, this is a pretty naive approach. Even if the file doesn't exist,
+                    // the file may have been loaded through an IAssetLoader which failed. So even
+                    // if the content file doesn't exist, that doesn't mean the error here is a
+                    // content-not-found error. Unfortunately XNA doesn't provide a good way to
+                    // detect the error type.
+                    if (this.GetContentFolderFileExists(contentKey))
+                        throw;
                 }
             }
 
             // not found
-            return null;
+            error = "The tilesheet couldn't be found relative to either map file or the game's content folder.";
+            return false;
         }
 
         /// <summary>Get whether a file from the game's content folder exists.</summary>
diff --git a/src/SMAPI/Framework/ModHelpers/ContentHelper.cs b/src/SMAPI/Framework/ModHelpers/ContentHelper.cs
index 043ae376..e9b70845 100644
--- a/src/SMAPI/Framework/ModHelpers/ContentHelper.cs
+++ b/src/SMAPI/Framework/ModHelpers/ContentHelper.cs
@@ -32,7 +32,7 @@ namespace StardewModdingAPI.Framework.ModHelpers
         /// <summary>The friendly mod name for use in errors.</summary>
         private readonly string ModName;
 
-        /// <summary>Encapsulates monitoring and logging for a given module.</summary>
+        /// <summary>Encapsulates monitoring and logging.</summary>
         private readonly IMonitor Monitor;
 
 
@@ -70,9 +70,11 @@ namespace StardewModdingAPI.Framework.ModHelpers
         public ContentHelper(ContentCoordinator contentCore, string modFolderPath, string modID, string modName, IMonitor monitor)
             : base(modID)
         {
+            string managedAssetPrefix = contentCore.GetManagedAssetPrefix(modID);
+
             this.ContentCore = contentCore;
-            this.GameContentManager = contentCore.CreateGameContentManager(this.ContentCore.GetManagedAssetPrefix(modID) + ".content");
-            this.ModContentManager = contentCore.CreateModContentManager(this.ContentCore.GetManagedAssetPrefix(modID), modFolderPath, this.GameContentManager);
+            this.GameContentManager = contentCore.CreateGameContentManager(managedAssetPrefix + ".content");
+            this.ModContentManager = contentCore.CreateModContentManager(managedAssetPrefix, modName, modFolderPath, this.GameContentManager);
             this.ModName = modName;
             this.Monitor = monitor;
         }