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.

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(
+            this.dalamudAtlas.BuildStepChange += e => e
-                tk =>
+                .OnPostBuild(
-                {
+                    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
                     {
-                        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
+                        // Broadcast to auto-rebuilding instances.
-                    this.AfterBuildFonts?.Invoke();
+                        this.AfterBuildFonts?.Invoke();
-                });
+                    });
         }
 
         // This will wait for scene on its own. We just wait for this.dalamudAtlas.BuildTask in this.InitScene.

Dalamud/Interface/ManagedFontAtlas/FontAtlasBuildStep.cs

@@ -7,32 +7,35 @@ namespace Dalamud.Interface.ManagedFontAtlas;
 /// </summary>
 public enum FontAtlasBuildStep
 {
-    /// <summary>
+    // Note: leave 0 alone; make default(FontAtlasBuildStep) not have a valid value
-    /// An invalid value. This should never be passed through event callbacks.
-    /// </summary>
-    Invalid,
 
     /// <summary>
     /// Called before calling <see cref="ImFontAtlasPtr.Build"/>.<br />
-    /// Expect <see cref="IFontAtlasBuildToolkitPreBuild"/> to be passed.
+    /// Expect <see cref="IFontAtlasBuildToolkitPreBuild"/> to be passed.<br />
+    /// When called from <see cref="IFontAtlas.BuildStepChange"/>, this will be called <b>before</b> the delegates
+    /// passed to <see cref="IFontAtlas.NewDelegateFontHandle"/>.
     /// </summary>
-    PreBuild,
+    PreBuild = 1,
 
     /// <summary>
     /// Called after calling <see cref="ImFontAtlasPtr.Build"/>.<br />
     /// Expect <see cref="IFontAtlasBuildToolkitPostBuild"/> to be passed.<br />
+    /// When called from <see cref="IFontAtlas.BuildStepChange"/>, this will be called <b>after</b> the delegates
+    /// passed to <see cref="IFontAtlas.NewDelegateFontHandle"/>; you can do cross-font operations here.<br />
     /// <br />
     /// This callback is not guaranteed to happen after <see cref="PreBuild"/>,
     /// but it will never happen on its own.
     /// </summary>
-    PostBuild,
+    PostBuild = 2,
 
     /// <summary>
     /// Called after promoting staging font atlas to the actual atlas for <see cref="IFontAtlas"/>.<br />
     /// Expect <see cref="PostBuild"/> to be passed.<br />
+    /// When called from <see cref="IFontAtlas.BuildStepChange"/>, this will be called <b>after</b> the delegates
+    /// passed to <see cref="IFontAtlas.NewDelegateFontHandle"/>; you should not make modifications to fonts.<br />
     /// <br />
     /// This callback is not guaranteed to happen after <see cref="IFontAtlasBuildToolkitPostPromotion"/>,
     /// but it will never happen on its own.
     /// </summary>
-    PostPromotion,
+    PostPromotion = 3,
 }

Dalamud/Interface/ManagedFontAtlas/IFontAtlasBuildToolkit.cs

@@ -80,4 +80,12 @@ public interface IFontAtlasBuildToolkit
     /// </summary>
     /// <param name="action">The action to run on dispose.</param>
     void DisposeWithAtlas(Action action);
+
+    /// <summary>
+    /// Gets the instance of <see cref="ImFontPtr"/> corresponding to <paramref name="fontHandle"/>
+    /// from <see cref="NewImAtlas"/>.
+    /// </summary>
+    /// <param name="fontHandle">The font handle.</param>
+    /// <returns>The corresonding <see cref="ImFontPtr"/>, or default if not found.</returns>
+    ImFontPtr GetFont(IFontHandle fontHandle);
 }

Dalamud/Interface/ManagedFontAtlas/IFontAtlasBuildToolkitPostBuild.cs

@@ -23,4 +23,28 @@ public interface IFontAtlasBuildToolkitPostBuild : IFontAtlasBuildToolkit
     /// <param name="disposeOnError">Dispose the wrap on error.</param>
     /// <returns>The texture index.</returns>
     int StoreTexture(IDalamudTextureWrap textureWrap, bool disposeOnError);
+
+    /// <summary>
+    /// Copies glyphs across fonts, in a safer way.<br />
+    /// If the font does not belong to the current atlas, this function is a no-op.
+    /// </summary>
+    /// <param name="source">Source font.</param>
+    /// <param name="target">Target font.</param>
+    /// <param name="missingOnly">Whether to copy missing glyphs only.</param>
+    /// <param name="rebuildLookupTable">Whether to call target.BuildLookupTable().</param>
+    /// <param name="rangeLow">Low codepoint range to copy.</param>
+    /// <param name="rangeHigh">High codepoing range to copy.</param>
+    void CopyGlyphsAcrossFonts(
+        ImFontPtr source,
+        ImFontPtr target,
+        bool missingOnly,
+        bool rebuildLookupTable = true,
+        char rangeLow = ' ',
+        char rangeHigh = '\uFFFE');
+
+    /// <summary>
+    /// Calls <see cref="ImFontPtr.BuildLookupTable"/>, with some fixups.
+    /// </summary>
+    /// <param name="font">The font.</param>
+    void BuildLookupTable(ImFontPtr font);
 }

Dalamud/Interface/ManagedFontAtlas/IFontAtlasBuildToolkitPostPromotion.cs

@@ -1,5 +1,3 @@
-using ImGuiNET;
-
 namespace Dalamud.Interface.ManagedFontAtlas;
 
 /// <summary>
@@ -7,27 +5,5 @@ namespace Dalamud.Interface.ManagedFontAtlas;
 /// </summary>
 public interface IFontAtlasBuildToolkitPostPromotion : IFontAtlasBuildToolkit
 {
-    /// <summary>
+    // empty
-    /// Copies glyphs across fonts, in a safer way.<br />
-    /// If the font does not belong to the current atlas, this function is a no-op.
-    /// </summary>
-    /// <param name="source">Source font.</param>
-    /// <param name="target">Target font.</param>
-    /// <param name="missingOnly">Whether to copy missing glyphs only.</param>
-    /// <param name="rebuildLookupTable">Whether to call target.BuildLookupTable().</param>
-    /// <param name="rangeLow">Low codepoint range to copy.</param>
-    /// <param name="rangeHigh">High codepoing range to copy.</param>
-    void CopyGlyphsAcrossFonts(
-        ImFontPtr source,
-        ImFontPtr target,
-        bool missingOnly,
-        bool rebuildLookupTable = true,
-        char rangeLow = ' ',
-        char rangeHigh = '\uFFFE');
-
-    /// <summary>
-    /// Calls <see cref="ImFontPtr.BuildLookupTable"/>, with some fixups.
-    /// </summary>
-    /// <param name="font">The font.</param>
-    void BuildLookupTable(ImFontPtr font);
 }

Dalamud/Interface/ManagedFontAtlas/Internals/FontAtlasFactory.BuildToolkit.cs

@@ -135,6 +135,19 @@ internal sealed partial class FontAtlasFactory
             }
         }
 
+        /// <inheritdoc/>
+        public ImFontPtr GetFont(IFontHandle fontHandle)
+        {
+            foreach (var s in this.data.Substances)
+            {
+                var f = s.GetFontPtr(fontHandle);
+                if (!f.IsNull())
+                    return f;
+            }
+
+            return default;
+        }
+
         /// <inheritdoc/>
         public ImFontPtr IgnoreGlobalScale(ImFontPtr fontPtr)
         {
@@ -608,49 +621,6 @@ internal sealed partial class FontAtlasFactory
                 ArrayPool<byte>.Shared.Return(buf);
             }
         }
-    }
-
-    /// <summary>
-    /// Implementations for <see cref="IFontAtlasBuildToolkitPostPromotion"/>.
-    /// </summary>
-    private class BuildToolkitPostPromotion : IFontAtlasBuildToolkitPostPromotion
-    {
-        private readonly FontAtlasBuiltData builtData;
-
-        /// <summary>
-        /// Initializes a new instance of the <see cref="BuildToolkitPostPromotion"/> class.
-        /// </summary>
-        /// <param name="builtData">The built data.</param>
-        public BuildToolkitPostPromotion(FontAtlasBuiltData builtData) => this.builtData = builtData;
-
-        /// <inheritdoc/>
-        public ImFontPtr Font { get; set; }
-
-        /// <inheritdoc/>
-        public float Scale => this.builtData.Scale;
-
-        /// <inheritdoc/>
-        public bool IsAsyncBuildOperation => true;
-
-        /// <inheritdoc/>
-        public FontAtlasBuildStep BuildStep => FontAtlasBuildStep.PostPromotion;
-
-        /// <inheritdoc/>
-        public ImFontAtlasPtr NewImAtlas => this.builtData.Atlas;
-
-        /// <inheritdoc/>
-        public unsafe ImVectorWrapper<ImFontPtr> Fonts => new(
-            &this.NewImAtlas.NativePtr->Fonts,
-            x => ImGuiNative.ImFont_destroy(x->NativePtr));
-
-        /// <inheritdoc/>
-        public T DisposeWithAtlas<T>(T disposable) where T : IDisposable => this.builtData.Garbage.Add(disposable);
-
-        /// <inheritdoc/>
-        public GCHandle DisposeWithAtlas(GCHandle gcHandle) => this.builtData.Garbage.Add(gcHandle);
-
-        /// <inheritdoc/>
-        public void DisposeWithAtlas(Action action) => this.builtData.Garbage.Add(action);
 
         /// <inheritdoc/>
         public unsafe void CopyGlyphsAcrossFonts(
@@ -707,4 +677,60 @@ internal sealed partial class FontAtlasFactory
             }
         }
     }
+
+    /// <summary>
+    /// Implementations for <see cref="IFontAtlasBuildToolkitPostPromotion"/>.
+    /// </summary>
+    private class BuildToolkitPostPromotion : IFontAtlasBuildToolkitPostPromotion
+    {
+        private readonly FontAtlasBuiltData builtData;
+
+        /// <summary>
+        /// Initializes a new instance of the <see cref="BuildToolkitPostPromotion"/> class.
+        /// </summary>
+        /// <param name="builtData">The built data.</param>
+        public BuildToolkitPostPromotion(FontAtlasBuiltData builtData) => this.builtData = builtData;
+
+        /// <inheritdoc/>
+        public ImFontPtr Font { get; set; }
+
+        /// <inheritdoc/>
+        public float Scale => this.builtData.Scale;
+
+        /// <inheritdoc/>
+        public bool IsAsyncBuildOperation => true;
+
+        /// <inheritdoc/>
+        public FontAtlasBuildStep BuildStep => FontAtlasBuildStep.PostPromotion;
+
+        /// <inheritdoc/>
+        public ImFontAtlasPtr NewImAtlas => this.builtData.Atlas;
+
+        /// <inheritdoc/>
+        public unsafe ImVectorWrapper<ImFontPtr> Fonts => new(
+            &this.NewImAtlas.NativePtr->Fonts,
+            x => ImGuiNative.ImFont_destroy(x->NativePtr));
+
+        /// <inheritdoc/>
+        public T DisposeWithAtlas<T>(T disposable) where T : IDisposable => this.builtData.Garbage.Add(disposable);
+
+        /// <inheritdoc/>
+        public GCHandle DisposeWithAtlas(GCHandle gcHandle) => this.builtData.Garbage.Add(gcHandle);
+
+        /// <inheritdoc/>
+        public void DisposeWithAtlas(Action action) => this.builtData.Garbage.Add(action);
+
+        /// <inheritdoc/>
+        public ImFontPtr GetFont(IFontHandle fontHandle)
+        {
+            foreach (var s in this.builtData.Substances)
+            {
+                var f = s.GetFontPtr(fontHandle);
+                if (!f.IsNull())
+                    return f;
+            }
+
+            return default;
+        }
+    }
 }

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
+                var toolkit = new BuildToolkitPostPromotion(data);
-                {
-                    this.BuildStepChange?.Invoke(toolkit);
-                }
-                catch (Exception e)
-                {
-                    Log.Error(
-                        e,
-                        "[{name}] {delegateName} PostPromotion error",
-                        this.Name,
-                        nameof(FontAtlasBuildStepDelegate));
-                }
 
                 foreach (var substance in data.Substances)
                 {
@@ -580,20 +582,18 @@ internal sealed partial class FontAtlasFactory
                     }
                 }
 
-                foreach (var font in toolkit.Fonts)
+                try
                 {
-                    try
+                    this.BuildStepChange?.Invoke(toolkit);
-                    {
+                }
-                        toolkit.BuildLookupTable(font);
+                catch (Exception e)
-                    }
+                {
-                    catch (Exception e)
+                    Log.Error(
-                    {
+                        e,
-                        Log.Error(e, "[{name}] BuildLookupTable error", this.Name);
+                        "[{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;
             }

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(