From fb8beb9370865652cf96fb57975d85885a954dbe Mon Sep 17 00:00:00 2001 From: Soreepeong Date: Tue, 23 Jan 2024 22:09:47 +0900 Subject: [PATCH] Move PostPromotion modification functions to PostBuild These changes are done to ensure that `IFontHandle.Lock` will be guaranteed to obtain a fully built font that will not be modified any further (unless `PostPromotion` is being used for modifying fonts, which should not be done by clients.) * Moved `CopyGlyphsAcrossFonts` and `BuildLookupTable` from `PostPromotion` to `PostBuild` build toolkit. * `IFontAtlasBuildToolkit`: Added `GetFont` to enable retrieving font corresponding to a handle being built. * `InterfaceManager`: Use `OnPostBuild` for copying glyphs from Mono to Default. * `FontAtlasBuildStep`: * Removed `Invalid` to prevent an unnecessary switch-case warnings. * Added contracts on when `IFontAtlas.BuildStepChanged` will be called. --- .../Interface/Internal/InterfaceManager.cs | 40 +++---- .../ManagedFontAtlas/FontAtlasBuildStep.cs | 19 +-- .../IFontAtlasBuildToolkit.cs | 8 ++ .../IFontAtlasBuildToolkitPostBuild.cs | 24 ++++ .../IFontAtlasBuildToolkitPostPromotion.cs | 26 +--- .../FontAtlasFactory.BuildToolkit.cs | 112 +++++++++++------- .../FontAtlasFactory.Implementation.cs | 85 ++++++------- Dalamud/Interface/UiBuilder.cs | 2 + 8 files changed, 180 insertions(+), 136 deletions(-) diff --git a/Dalamud/Interface/Internal/InterfaceManager.cs b/Dalamud/Interface/Internal/InterfaceManager.cs index 25baa5e29..93050d67a 100644 --- a/Dalamud/Interface/Internal/InterfaceManager.cs +++ b/Dalamud/Interface/Internal/InterfaceManager.cs @@ -717,28 +717,28 @@ internal class InterfaceManager : IDisposable, IServiceType tk => tk.AddDalamudAssetFont( DalamudAsset.InconsolataRegular, new() { SizePx = DefaultFontSizePx }))); - this.dalamudAtlas.BuildStepChange += e => e.OnPostPromotion( - tk => - { - // Note: the first call of this function is done outside the main thread; this is expected. - // Do not use DefaultFont, IconFont, and MonoFont. - // Use font handles directly. - - using var defaultFont = this.DefaultFontHandle.Lock(); - using var monoFont = this.MonoFontHandle.Lock(); - - // Fill missing glyphs in MonoFont from DefaultFont - tk.CopyGlyphsAcrossFonts(defaultFont, monoFont, true); - - // Update default font - unsafe + this.dalamudAtlas.BuildStepChange += e => e + .OnPostBuild( + tk => { - ImGui.GetIO().NativePtr->FontDefault = defaultFont; - } + // Fill missing glyphs in MonoFont from DefaultFont. + tk.CopyGlyphsAcrossFonts( + tk.GetFont(this.DefaultFontHandle), + tk.GetFont(this.MonoFontHandle), + missingOnly: true); + }) + .OnPostPromotion( + tk => + { + // Update the ImGui default font. + unsafe + { + ImGui.GetIO().NativePtr->FontDefault = tk.GetFont(this.DefaultFontHandle); + } - // Broadcast to auto-rebuilding instances - this.AfterBuildFonts?.Invoke(); - }); + // Broadcast to auto-rebuilding instances. + this.AfterBuildFonts?.Invoke(); + }); } // This will wait for scene on its own. We just wait for this.dalamudAtlas.BuildTask in this.InitScene. diff --git a/Dalamud/Interface/ManagedFontAtlas/FontAtlasBuildStep.cs b/Dalamud/Interface/ManagedFontAtlas/FontAtlasBuildStep.cs index 345ab729d..ba94db435 100644 --- a/Dalamud/Interface/ManagedFontAtlas/FontAtlasBuildStep.cs +++ b/Dalamud/Interface/ManagedFontAtlas/FontAtlasBuildStep.cs @@ -7,32 +7,35 @@ namespace Dalamud.Interface.ManagedFontAtlas; /// public enum FontAtlasBuildStep { - /// - /// An invalid value. This should never be passed through event callbacks. - /// - Invalid, + // Note: leave 0 alone; make default(FontAtlasBuildStep) not have a valid value /// /// Called before calling .
- /// Expect to be passed. + /// Expect to be passed.
+ /// When called from , this will be called before the delegates + /// passed to . /// - PreBuild, + PreBuild = 1, /// /// Called after calling .
/// Expect to be passed.
+ /// When called from , this will be called after the delegates + /// passed to ; you can do cross-font operations here.
///
/// This callback is not guaranteed to happen after , /// but it will never happen on its own. /// - PostBuild, + PostBuild = 2, /// /// Called after promoting staging font atlas to the actual atlas for .
/// Expect to be passed.
+ /// When called from , this will be called after the delegates + /// passed to ; you should not make modifications to fonts.
///
/// This callback is not guaranteed to happen after , /// but it will never happen on its own. /// - PostPromotion, + PostPromotion = 3, } diff --git a/Dalamud/Interface/ManagedFontAtlas/IFontAtlasBuildToolkit.cs b/Dalamud/Interface/ManagedFontAtlas/IFontAtlasBuildToolkit.cs index a997c48c1..f75ed4686 100644 --- a/Dalamud/Interface/ManagedFontAtlas/IFontAtlasBuildToolkit.cs +++ b/Dalamud/Interface/ManagedFontAtlas/IFontAtlasBuildToolkit.cs @@ -80,4 +80,12 @@ public interface IFontAtlasBuildToolkit /// /// The action to run on dispose. void DisposeWithAtlas(Action action); + + /// + /// Gets the instance of corresponding to + /// from . + /// + /// The font handle. + /// The corresonding , or default if not found. + ImFontPtr GetFont(IFontHandle fontHandle); } diff --git a/Dalamud/Interface/ManagedFontAtlas/IFontAtlasBuildToolkitPostBuild.cs b/Dalamud/Interface/ManagedFontAtlas/IFontAtlasBuildToolkitPostBuild.cs index 3c14197e0..eb7c7e08c 100644 --- a/Dalamud/Interface/ManagedFontAtlas/IFontAtlasBuildToolkitPostBuild.cs +++ b/Dalamud/Interface/ManagedFontAtlas/IFontAtlasBuildToolkitPostBuild.cs @@ -23,4 +23,28 @@ public interface IFontAtlasBuildToolkitPostBuild : IFontAtlasBuildToolkit /// Dispose the wrap on error. /// The texture index. int StoreTexture(IDalamudTextureWrap textureWrap, bool disposeOnError); + + /// + /// Copies glyphs across fonts, in a safer way.
+ /// If the font does not belong to the current atlas, this function is a no-op. + /// + /// Source font. + /// Target font. + /// Whether to copy missing glyphs only. + /// Whether to call target.BuildLookupTable(). + /// Low codepoint range to copy. + /// High codepoing range to copy. + void CopyGlyphsAcrossFonts( + ImFontPtr source, + ImFontPtr target, + bool missingOnly, + bool rebuildLookupTable = true, + char rangeLow = ' ', + char rangeHigh = '\uFFFE'); + + /// + /// Calls , with some fixups. + /// + /// The font. + void BuildLookupTable(ImFontPtr font); } diff --git a/Dalamud/Interface/ManagedFontAtlas/IFontAtlasBuildToolkitPostPromotion.cs b/Dalamud/Interface/ManagedFontAtlas/IFontAtlasBuildToolkitPostPromotion.cs index 8c3c91624..930851fc7 100644 --- a/Dalamud/Interface/ManagedFontAtlas/IFontAtlasBuildToolkitPostPromotion.cs +++ b/Dalamud/Interface/ManagedFontAtlas/IFontAtlasBuildToolkitPostPromotion.cs @@ -1,5 +1,3 @@ -using ImGuiNET; - namespace Dalamud.Interface.ManagedFontAtlas; /// @@ -7,27 +5,5 @@ namespace Dalamud.Interface.ManagedFontAtlas; /// public interface IFontAtlasBuildToolkitPostPromotion : IFontAtlasBuildToolkit { - /// - /// Copies glyphs across fonts, in a safer way.
- /// If the font does not belong to the current atlas, this function is a no-op. - /// - /// Source font. - /// Target font. - /// Whether to copy missing glyphs only. - /// Whether to call target.BuildLookupTable(). - /// Low codepoint range to copy. - /// High codepoing range to copy. - void CopyGlyphsAcrossFonts( - ImFontPtr source, - ImFontPtr target, - bool missingOnly, - bool rebuildLookupTable = true, - char rangeLow = ' ', - char rangeHigh = '\uFFFE'); - - /// - /// Calls , with some fixups. - /// - /// The font. - void BuildLookupTable(ImFontPtr font); + // empty } diff --git a/Dalamud/Interface/ManagedFontAtlas/Internals/FontAtlasFactory.BuildToolkit.cs b/Dalamud/Interface/ManagedFontAtlas/Internals/FontAtlasFactory.BuildToolkit.cs index fde115c9e..3addfabe8 100644 --- a/Dalamud/Interface/ManagedFontAtlas/Internals/FontAtlasFactory.BuildToolkit.cs +++ b/Dalamud/Interface/ManagedFontAtlas/Internals/FontAtlasFactory.BuildToolkit.cs @@ -135,6 +135,19 @@ internal sealed partial class FontAtlasFactory } } + /// + public ImFontPtr GetFont(IFontHandle fontHandle) + { + foreach (var s in this.data.Substances) + { + var f = s.GetFontPtr(fontHandle); + if (!f.IsNull()) + return f; + } + + return default; + } + /// public ImFontPtr IgnoreGlobalScale(ImFontPtr fontPtr) { @@ -608,49 +621,6 @@ internal sealed partial class FontAtlasFactory ArrayPool.Shared.Return(buf); } } - } - - /// - /// Implementations for . - /// - private class BuildToolkitPostPromotion : IFontAtlasBuildToolkitPostPromotion - { - private readonly FontAtlasBuiltData builtData; - - /// - /// Initializes a new instance of the class. - /// - /// The built data. - public BuildToolkitPostPromotion(FontAtlasBuiltData builtData) => this.builtData = builtData; - - /// - public ImFontPtr Font { get; set; } - - /// - public float Scale => this.builtData.Scale; - - /// - public bool IsAsyncBuildOperation => true; - - /// - public FontAtlasBuildStep BuildStep => FontAtlasBuildStep.PostPromotion; - - /// - public ImFontAtlasPtr NewImAtlas => this.builtData.Atlas; - - /// - public unsafe ImVectorWrapper Fonts => new( - &this.NewImAtlas.NativePtr->Fonts, - x => ImGuiNative.ImFont_destroy(x->NativePtr)); - - /// - public T DisposeWithAtlas(T disposable) where T : IDisposable => this.builtData.Garbage.Add(disposable); - - /// - public GCHandle DisposeWithAtlas(GCHandle gcHandle) => this.builtData.Garbage.Add(gcHandle); - - /// - public void DisposeWithAtlas(Action action) => this.builtData.Garbage.Add(action); /// public unsafe void CopyGlyphsAcrossFonts( @@ -707,4 +677,60 @@ internal sealed partial class FontAtlasFactory } } } + + /// + /// Implementations for . + /// + private class BuildToolkitPostPromotion : IFontAtlasBuildToolkitPostPromotion + { + private readonly FontAtlasBuiltData builtData; + + /// + /// Initializes a new instance of the class. + /// + /// The built data. + public BuildToolkitPostPromotion(FontAtlasBuiltData builtData) => this.builtData = builtData; + + /// + public ImFontPtr Font { get; set; } + + /// + public float Scale => this.builtData.Scale; + + /// + public bool IsAsyncBuildOperation => true; + + /// + public FontAtlasBuildStep BuildStep => FontAtlasBuildStep.PostPromotion; + + /// + public ImFontAtlasPtr NewImAtlas => this.builtData.Atlas; + + /// + public unsafe ImVectorWrapper Fonts => new( + &this.NewImAtlas.NativePtr->Fonts, + x => ImGuiNative.ImFont_destroy(x->NativePtr)); + + /// + public T DisposeWithAtlas(T disposable) where T : IDisposable => this.builtData.Garbage.Add(disposable); + + /// + public GCHandle DisposeWithAtlas(GCHandle gcHandle) => this.builtData.Garbage.Add(gcHandle); + + /// + public void DisposeWithAtlas(Action action) => this.builtData.Garbage.Add(action); + + /// + public ImFontPtr GetFont(IFontHandle fontHandle) + { + foreach (var s in this.builtData.Substances) + { + var f = s.GetFontPtr(fontHandle); + if (!f.IsNull()) + return f; + } + + return default; + } + } } diff --git a/Dalamud/Interface/ManagedFontAtlas/Internals/FontAtlasFactory.Implementation.cs b/Dalamud/Interface/ManagedFontAtlas/Internals/FontAtlasFactory.Implementation.cs index 99ce8dab9..85f7219b2 100644 --- a/Dalamud/Interface/ManagedFontAtlas/Internals/FontAtlasFactory.Implementation.cs +++ b/Dalamud/Interface/ManagedFontAtlas/Internals/FontAtlasFactory.Implementation.cs @@ -4,7 +4,6 @@ using System.Collections.Generic; using System.Diagnostics; using System.Linq; using System.Reactive.Disposables; -using System.Threading; using System.Threading.Tasks; using Dalamud.Interface.GameFonts; @@ -168,7 +167,7 @@ internal sealed partial class FontAtlasFactory _ => throw new InvalidOperationException(), }; - public unsafe int Release() + public int Release() { switch (IRefCountable.AlterRefCount(-1, ref this.refCount, out var newRefCount)) { @@ -176,22 +175,35 @@ internal sealed partial class FontAtlasFactory return newRefCount; case IRefCountable.RefCountResult.FinalRelease: - if (this.IsBuildInProgress) - { - Log.Error( - "[{name}] 0x{ptr:X}: Trying to dispose while build is in progress; waiting for build.\n" + - "Stack:\n{trace}", - this.Owner?.Name ?? "", - (nint)this.Atlas.NativePtr, - new StackTrace()); - while (this.IsBuildInProgress) - Thread.Sleep(100); - } - #if VeryVerboseLog Log.Verbose("[{name}] 0x{ptr:X}: Disposing", this.Owner?.Name ?? "", (nint)this.Atlas.NativePtr); #endif - this.Garbage.Dispose(); + + if (this.IsBuildInProgress) + { + unsafe + { + Log.Error( + "[{name}] 0x{ptr:X}: Trying to dispose while build is in progress; disposing later.\n" + + "Stack:\n{trace}", + this.Owner?.Name ?? "", + (nint)this.Atlas.NativePtr, + new StackTrace()); + } + + Task.Run( + async () => + { + while (this.IsBuildInProgress) + await Task.Delay(100); + this.Garbage.Dispose(); + }); + } + else + { + this.Garbage.Dispose(); + } + return newRefCount; case IRefCountable.RefCountResult.AlreadyDisposed: @@ -549,20 +561,10 @@ internal sealed partial class FontAtlasFactory return; } - var toolkit = new BuildToolkitPostPromotion(data); + foreach (var substance in data.Substances) + substance.Manager.InvokeFontHandleImFontChanged(); - try - { - this.BuildStepChange?.Invoke(toolkit); - } - catch (Exception e) - { - Log.Error( - e, - "[{name}] {delegateName} PostPromotion error", - this.Name, - nameof(FontAtlasBuildStepDelegate)); - } + var toolkit = new BuildToolkitPostPromotion(data); foreach (var substance in data.Substances) { @@ -580,20 +582,18 @@ internal sealed partial class FontAtlasFactory } } - foreach (var font in toolkit.Fonts) + try { - try - { - toolkit.BuildLookupTable(font); - } - catch (Exception e) - { - Log.Error(e, "[{name}] BuildLookupTable error", this.Name); - } + this.BuildStepChange?.Invoke(toolkit); + } + catch (Exception e) + { + Log.Error( + e, + "[{name}] {delegateName} PostPromotion error", + this.Name, + nameof(FontAtlasBuildStepDelegate)); } - - foreach (var substance in data.Substances) - substance.Manager.InvokeFontHandleImFontChanged(); #if VeryVerboseLog Log.Verbose("[{name}] Built from {source}.", this.Name, source); @@ -709,6 +709,9 @@ internal sealed partial class FontAtlasFactory toolkit.PostBuildSubstances(); this.BuildStepChange?.Invoke(toolkit); + foreach (var font in toolkit.Fonts) + toolkit.BuildLookupTable(font); + if (this.factory.SceneTask is { IsCompleted: false } sceneTask) { Log.Verbose( @@ -754,6 +757,8 @@ internal sealed partial class FontAtlasFactory } finally { + // RS is being dumb + // ReSharper disable once ConstantConditionalAccessQualifier toolkit?.Dispose(); this.buildQueued = false; } diff --git a/Dalamud/Interface/UiBuilder.cs b/Dalamud/Interface/UiBuilder.cs index ea3803f35..af4cc39c2 100644 --- a/Dalamud/Interface/UiBuilder.cs +++ b/Dalamud/Interface/UiBuilder.cs @@ -700,6 +700,8 @@ public sealed class UiBuilder : IDisposable if (e.IsAsyncBuildOperation) return; + ThreadSafety.AssertMainThread(); + if (this.BuildFonts is not null) { e.OnPreBuild(