From 8527e035f11afbd963706232512b18876348700f Mon Sep 17 00:00:00 2001 From: goat Date: Sat, 23 Sep 2023 17:37:41 +0200 Subject: [PATCH 1/2] chore: remove refcounting, keepalive logic from TextureManager, remove scoped service Makes this whole thing a lot simpler to use and easier to understand. --- Dalamud/Interface/Internal/TextureManager.cs | 193 +++---------------- Dalamud/Plugin/Services/ITextureProvider.cs | 6 +- 2 files changed, 25 insertions(+), 174 deletions(-) diff --git a/Dalamud/Interface/Internal/TextureManager.cs b/Dalamud/Interface/Internal/TextureManager.cs index ce08e6cc7..542299656 100644 --- a/Dalamud/Interface/Internal/TextureManager.cs +++ b/Dalamud/Interface/Internal/TextureManager.cs @@ -1,8 +1,6 @@ -using System.Collections.Concurrent; -using System.Collections.Generic; +using System.Collections.Generic; using System.Diagnostics; using System.IO; -using System.Linq; using System.Numerics; using Dalamud.Data; @@ -11,13 +9,14 @@ using Dalamud.IoC; using Dalamud.IoC.Internal; using Dalamud.Logging.Internal; using Dalamud.Plugin.Services; -using ImGuiScene; using Lumina.Data.Files; using Lumina.Data.Parsing.Tex.Buffers; using SharpDX.DXGI; namespace Dalamud.Interface.Internal; +// TODO API10: Remove keepAlive from public APIs + /// /// Service responsible for loading and disposing ImGui texture wraps. /// @@ -25,9 +24,10 @@ namespace Dalamud.Interface.Internal; [InterfaceVersion("1.0")] [ServiceManager.BlockingEarlyLoadedService] #pragma warning disable SA1015 +[ResolveVia] [ResolveVia] #pragma warning restore SA1015 -internal class TextureManager : IDisposable, IServiceType, ITextureSubstitutionProvider +internal class TextureManager : IDisposable, IServiceType, ITextureProvider, ITextureSubstitutionProvider { private const string IconFileFormat = "ui/icon/{0:D3}000/{1}{2:D6}.tex"; private const string HighResolutionIconFileFormat = "ui/icon/{0:D3}000/{1}{2:D6}_hr1.tex"; @@ -78,16 +78,16 @@ internal class TextureManager : IDisposable, IServiceType, ITextureSubstitutionP /// If null, default to the game's current language. /// /// - /// Prevent Dalamud from automatically unloading this icon to save memory. Usually does not need to be set. + /// Not used. This parameter is ignored. /// /// /// Null, if the icon does not exist in the specified configuration, or a texture wrap that can be used /// to render the icon. /// - public TextureManagerTextureWrap? GetIcon(uint iconId, ITextureProvider.IconFlags flags = ITextureProvider.IconFlags.HiRes, ClientLanguage? language = null, bool keepAlive = false) + public IDalamudTextureWrap? GetIcon(uint iconId, ITextureProvider.IconFlags flags = ITextureProvider.IconFlags.HiRes, ClientLanguage? language = null, bool keepAlive = false) { var path = this.GetIconPath(iconId, flags, language); - return path == null ? null : this.CreateWrap(path, keepAlive); + return path == null ? null : this.CreateWrap(path); } /// @@ -171,16 +171,16 @@ internal class TextureManager : IDisposable, IServiceType, ITextureSubstitutionP /// You may only specify paths in the game's VFS. /// /// The path to the texture in the game's VFS. - /// Prevent Dalamud from automatically unloading this texture to save memory. Usually does not need to be set. + /// Not used. This parameter is ignored. /// Null, if the icon does not exist, or a texture wrap that can be used to render the texture. - public TextureManagerTextureWrap? GetTextureFromGame(string path, bool keepAlive = false) + public IDalamudTextureWrap? GetTextureFromGame(string path, bool keepAlive = false) { ArgumentException.ThrowIfNullOrEmpty(path); if (Path.IsPathRooted(path)) throw new ArgumentException("Use GetTextureFromFile() to load textures directly from a file.", nameof(path)); - return !this.dataManager.FileExists(path) ? null : this.CreateWrap(path, keepAlive); + return !this.dataManager.FileExists(path) ? null : this.CreateWrap(path); } /// @@ -190,12 +190,12 @@ internal class TextureManager : IDisposable, IServiceType, ITextureSubstitutionP /// This API can load .png and .tex files. /// /// The FileInfo describing the image or texture file. - /// Prevent Dalamud from automatically unloading this texture to save memory. Usually does not need to be set. + /// Not used. This parameter is ignored. /// Null, if the file does not exist, or a texture wrap that can be used to render the texture. - public TextureManagerTextureWrap? GetTextureFromFile(FileInfo file, bool keepAlive = false) + public IDalamudTextureWrap? GetTextureFromFile(FileInfo file, bool keepAlive = false) { ArgumentNullException.ThrowIfNull(file); - return !file.Exists ? null : this.CreateWrap(file.FullName, keepAlive); + return !file.Exists ? null : this.CreateWrap(file.FullName); } /// @@ -307,8 +307,7 @@ internal class TextureManager : IDisposable, IServiceType, ITextureSubstitutionP throw new Exception("null info in activeTextures"); } - if (info.KeepAliveCount == 0) - info.LastAccess = DateTime.UtcNow; + info.LastAccess = DateTime.UtcNow; if (info is { Wrap: not null }) return info; @@ -384,33 +383,6 @@ internal class TextureManager : IDisposable, IServiceType, ITextureSubstitutionP return info; } - /// - /// Notify the system about an instance of a texture wrap being disposed. - /// If required conditions are met, the texture will be unloaded at the next update. - /// - /// The path to the texture. - /// Whether or not this handle was created in keep-alive mode. - internal void NotifyTextureDisposed(string path, bool keepAlive) - { - lock (this.activeTextures) - { - if (!this.activeTextures.TryGetValue(path, out var info)) - { - Log.Warning("Disposing texture that didn't exist: {Path}", path); - return; - } - - info.RefCount--; - - if (keepAlive) - info.KeepAliveCount--; - - // Clean it up by the next update. If it's re-requested in-between, we don't reload it. - if (info.RefCount <= 0) - info.LastAccess = default; - } - } - private static string FormatIconPath(uint iconId, string? type, bool highResolution) { var format = highResolution ? HighResolutionIconFileFormat : IconFileFormat; @@ -422,23 +394,15 @@ internal class TextureManager : IDisposable, IServiceType, ITextureSubstitutionP return string.Format(format, iconId / 1000, type, iconId); } - private TextureManagerTextureWrap? CreateWrap(string path, bool keepAlive) + private TextureManagerTextureWrap? CreateWrap(string path) { lock (this.activeTextures) { // This will create the texture. // That's fine, it's probably used immediately and this will let the plugin catch load errors. var info = this.GetInfo(path, rethrow: true); - - // We need to increase the refcounts here while locking the collection! - // Otherwise, if this is loaded from a task, cleanup might already try to delete it - // before it can be increased. - info.RefCount++; - - if (keepAlive) - info.KeepAliveCount++; - return new TextureManagerTextureWrap(path, info.Extents, keepAlive, this); + return new TextureManagerTextureWrap(path, info.Extents, this); } } @@ -450,19 +414,7 @@ internal class TextureManager : IDisposable, IServiceType, ITextureSubstitutionP foreach (var texInfo in this.activeTextures) { - if (texInfo.Value.RefCount == 0) - { - Log.Verbose("Evicting {Path} since no refs", texInfo.Key); - - Debug.Assert(texInfo.Value.KeepAliveCount == 0, "texInfo.Value.KeepAliveCount == 0"); - - texInfo.Value.Wrap?.Dispose(); - texInfo.Value.Wrap = null; - toRemove.Add(texInfo.Key); - continue; - } - - if (texInfo.Value.KeepAliveCount > 0 || texInfo.Value.Wrap == null) + if (texInfo.Value.Wrap == null) continue; if (DateTime.UtcNow - texInfo.Value.LastAccess > TimeSpan.FromMilliseconds(MillisecondsEvictionTime)) @@ -470,6 +422,7 @@ internal class TextureManager : IDisposable, IServiceType, ITextureSubstitutionP Log.Verbose("Evicting {Path} since too old", texInfo.Key); texInfo.Value.Wrap.Dispose(); texInfo.Value.Wrap = null; + toRemove.Add(texInfo.Key); } } @@ -501,16 +454,6 @@ internal class TextureManager : IDisposable, IServiceType, ITextureSubstitutionP /// Gets or sets the time the texture was last accessed. /// public DateTime LastAccess { get; set; } - - /// - /// Gets or sets the number of active holders of this texture. - /// - public uint RefCount { get; set; } - - /// - /// Gets or sets the number of active holders that want this texture to stay alive forever. - /// - public uint KeepAliveCount { get; set; } /// /// Gets or sets the extents of the texture. @@ -519,90 +462,6 @@ internal class TextureManager : IDisposable, IServiceType, ITextureSubstitutionP } } -/// -/// Plugin-scoped version of a texture manager. -/// -[PluginInterface] -[InterfaceVersion("1.0")] -[ServiceManager.ScopedService] -#pragma warning disable SA1015 -[ResolveVia] -#pragma warning restore SA1015 -internal class TextureProviderPluginScoped : ITextureProvider, IServiceType, IDisposable -{ - private readonly TextureManager textureManager; - - private readonly ConcurrentBag trackedTextures = new(); - - /// - /// Initializes a new instance of the class. - /// - /// TextureManager instance. - public TextureProviderPluginScoped(TextureManager textureManager) - { - this.textureManager = textureManager; - } - - /// - public IDalamudTextureWrap? GetIcon( - uint iconId, - ITextureProvider.IconFlags flags = ITextureProvider.IconFlags.ItemHighQuality, - ClientLanguage? language = null, - bool keepAlive = false) - { - var wrap = this.textureManager.GetIcon(iconId, flags, language, keepAlive); - if (wrap == null) - return null; - - this.trackedTextures.Add(wrap); - return wrap; - } - - /// - public string? GetIconPath(uint iconId, ITextureProvider.IconFlags flags = ITextureProvider.IconFlags.HiRes, ClientLanguage? language = null) - => this.textureManager.GetIconPath(iconId, flags, language); - - /// - public IDalamudTextureWrap? GetTextureFromGame(string path, bool keepAlive = false) - { - ArgumentException.ThrowIfNullOrEmpty(path); - - var wrap = this.textureManager.GetTextureFromGame(path, keepAlive); - if (wrap == null) - return null; - - this.trackedTextures.Add(wrap); - return wrap; - } - - /// - public IDalamudTextureWrap? GetTextureFromFile(FileInfo file, bool keepAlive) - { - ArgumentNullException.ThrowIfNull(file); - - var wrap = this.textureManager.GetTextureFromFile(file, keepAlive); - if (wrap == null) - return null; - - this.trackedTextures.Add(wrap); - return wrap; - } - - /// - public IDalamudTextureWrap? GetTexture(TexFile file) - => this.textureManager.GetTexture(file); - - /// - public void Dispose() - { - // Dispose all leaked textures - foreach (var textureWrap in this.trackedTextures.Where(x => !x.IsDisposed)) - { - textureWrap.Dispose(); - } - } -} - /// /// Wrap. /// @@ -610,19 +469,16 @@ internal class TextureManagerTextureWrap : IDalamudTextureWrap { private readonly TextureManager manager; private readonly string path; - private readonly bool keepAlive; /// /// Initializes a new instance of the class. /// /// The path to the texture. /// The extents of the texture. - /// Keep alive or not. /// Manager that we obtained this from. - internal TextureManagerTextureWrap(string path, Vector2 extents, bool keepAlive, TextureManager manager) + internal TextureManagerTextureWrap(string path, Vector2 extents, TextureManager manager) { this.path = path; - this.keepAlive = keepAlive; this.manager = manager; this.Width = (int)extents.X; this.Height = (int)extents.Y; @@ -648,12 +504,7 @@ internal class TextureManagerTextureWrap : IDalamudTextureWrap /// public void Dispose() { - lock (this) - { - if (!this.IsDisposed) - this.manager.NotifyTextureDisposed(this.path, this.keepAlive); - - this.IsDisposed = true; - } + this.IsDisposed = true; + // This is a no-op. The manager cleans up textures that are not being drawn. } } diff --git a/Dalamud/Plugin/Services/ITextureProvider.cs b/Dalamud/Plugin/Services/ITextureProvider.cs index 091b2ed67..f91d4ee8e 100644 --- a/Dalamud/Plugin/Services/ITextureProvider.cs +++ b/Dalamud/Plugin/Services/ITextureProvider.cs @@ -44,7 +44,7 @@ public interface ITextureProvider /// If null, default to the game's current language. /// /// - /// Prevent Dalamud from automatically unloading this icon to save memory. Usually does not need to be set. + /// Not used. This parameter is ignored. /// /// /// Null, if the icon does not exist in the specified configuration, or a texture wrap that can be used @@ -72,7 +72,7 @@ public interface ITextureProvider /// You may only specify paths in the game's VFS. /// /// The path to the texture in the game's VFS. - /// Prevent Dalamud from automatically unloading this texture to save memory. Usually does not need to be set. + /// Not used. This parameter is ignored. /// Null, if the icon does not exist, or a texture wrap that can be used to render the texture. public IDalamudTextureWrap? GetTextureFromGame(string path, bool keepAlive = false); @@ -83,7 +83,7 @@ public interface ITextureProvider /// This API can load .png and .tex files. /// /// The FileInfo describing the image or texture file. - /// Prevent Dalamud from automatically unloading this texture to save memory. Usually does not need to be set. + /// Not used. This parameter is ignored. /// Null, if the file does not exist, or a texture wrap that can be used to render the texture. public IDalamudTextureWrap? GetTextureFromFile(FileInfo file, bool keepAlive = false); From acb81deb9c073829ff22c5c895096c3d1f079750 Mon Sep 17 00:00:00 2001 From: goat Date: Sat, 23 Sep 2023 17:45:50 +0200 Subject: [PATCH 2/2] make sure that access is completely atomic --- Dalamud/Interface/Internal/TextureManager.cs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/Dalamud/Interface/Internal/TextureManager.cs b/Dalamud/Interface/Internal/TextureManager.cs index 542299656..7c773bd36 100644 --- a/Dalamud/Interface/Internal/TextureManager.cs +++ b/Dalamud/Interface/Internal/TextureManager.cs @@ -305,12 +305,12 @@ internal class TextureManager : IDisposable, IServiceType, ITextureProvider, ITe if (info == null) throw new Exception("null info in activeTextures"); - } - - info.LastAccess = DateTime.UtcNow; + + info.LastAccess = DateTime.UtcNow; - if (info is { Wrap: not null }) - return info; + if (info is { Wrap: not null }) + return info; + } if (!this.im.IsReady) throw new InvalidOperationException("Cannot create textures before scene is ready");