Skin Fixer: Switch to a passive approach.

Do not load skin.shpk for ourselves as it causes a race condition.
Instead, inspect the materials' ShPk names.

Penumbra/Communication/CreatedCharacterBase.cs

@@ -16,9 +16,6 @@ public sealed class CreatedCharacterBase : EventWrapper<Action<nint, ModCollecti
     {
         /// <seealso cref="PenumbraApi.CreatedCharacterBase"/>
         Api = int.MinValue,
-
-        /// <seealso cref="Interop.Services.SkinFixer.OnCharacterBaseCreated"/>
-        SkinFixer = 0,
     }
 
     public CreatedCharacterBase()

Penumbra/Communication/MtrlShpkLoaded.cs

@@ -0,0 +1,24 @@
+using System;
+using OtterGui.Classes;
+
+namespace Penumbra.Communication;
+
+/// <summary> <list type="number">
+///     <item>Parameter is the material resource handle for which the shader package has been loaded. </item>
+///     <item>Parameter is the associated game object. </item>
+/// </list> </summary>
+public sealed class MtrlShpkLoaded : EventWrapper<Action<nint, nint>, MtrlShpkLoaded.Priority>
+{
+    public enum Priority
+    {
+        /// <seealso cref="Interop.Services.SkinFixer.OnMtrlShpkLoaded"/>
+        SkinFixer = 0,
+    }
+
+    public MtrlShpkLoaded()
+        : base(nameof(MtrlShpkLoaded))
+    { }
+
+    public void Invoke(nint mtrlResourceHandle, nint gameObject)
+        => Invoke(this, mtrlResourceHandle, gameObject);
+}

Penumbra/Interop/PathResolving/SubfileHelper.cs

@@ -11,6 +11,7 @@ using Penumbra.GameData.Enums;
 using Penumbra.Interop.ResourceLoading;
 using Penumbra.Interop.Services;
 using Penumbra.Interop.Structs;
+using Penumbra.Services;
 using Penumbra.String;
 using Penumbra.String.Classes;
 using Penumbra.Util;
@@ -24,22 +25,24 @@ namespace Penumbra.Interop.PathResolving;
 /// </summary>
 public unsafe class SubfileHelper : IDisposable, IReadOnlyCollection<KeyValuePair<nint, ResolveData>>
 {
-    private readonly PerformanceTracker _performance;
+    private readonly PerformanceTracker  _performance;
-    private readonly ResourceLoader     _loader;
+    private readonly ResourceLoader      _loader;
-    private readonly GameEventManager   _events;
+    private readonly GameEventManager    _events;
+    private readonly CommunicatorService _communicator;
 
     private readonly ThreadLocal<ResolveData> _mtrlData = new(() => ResolveData.Invalid);
     private readonly ThreadLocal<ResolveData> _avfxData = new(() => ResolveData.Invalid);
 
     private readonly ConcurrentDictionary<nint, ResolveData> _subFileCollection = new();
 
-    public SubfileHelper(PerformanceTracker performance, ResourceLoader loader, GameEventManager events)
+    public SubfileHelper(PerformanceTracker performance, ResourceLoader loader, GameEventManager events, CommunicatorService communicator)
     {
         SignatureHelper.Initialise(this);
 
-        _performance = performance;
+        _performance  = performance;
-        _loader      = loader;
+        _loader       = loader;
-        _events      = events;
+        _events       = events;
+        _communicator = communicator;
 
         _loadMtrlShpkHook.Enable();
         _loadMtrlTexHook.Enable();
@@ -150,10 +153,12 @@ public unsafe class SubfileHelper : IDisposable, IReadOnlyCollection<KeyValuePai
     private byte LoadMtrlShpkDetour(nint mtrlResourceHandle)
     {
         using var performance = _performance.Measure(PerformanceType.LoadShaders);
-        var       last        = _mtrlData.Value;
+        var       last        = _mtrlData.Value;
-        _mtrlData.Value = LoadFileHelper(mtrlResourceHandle);
+        var mtrlData = LoadFileHelper(mtrlResourceHandle);
+        _mtrlData.Value = mtrlData;
         var ret = _loadMtrlShpkHook.Original(mtrlResourceHandle);
-        _mtrlData.Value = last;
+        _mtrlData.Value = last;
+        _communicator.MtrlShpkLoaded.Invoke(mtrlResourceHandle, mtrlData.AssociatedGameObject);
         return ret;
     }
 

Penumbra/Interop/Services/SkinFixer.cs

@@ -1,30 +1,22 @@
 using System;
 using System.Collections.Concurrent;
-using System.Collections.Generic;
 using System.Runtime.InteropServices;
 using System.Threading;
-using System.Threading.Tasks;
 using Dalamud.Hooking;
 using Dalamud.Utility.Signatures;
 using FFXIVClientStructs.FFXIV.Client.Graphics.Render;
-using FFXIVClientStructs.FFXIV.Client.Graphics.Scene;
-using FFXIVClientStructs.FFXIV.Client.System.Resource;
-using FFXIVClientStructs.FFXIV.Client.System.Resource.Handle;
-using Penumbra.Collections;
 using Penumbra.Communication;
 using Penumbra.GameData;
-using Penumbra.GameData.Enums;
 using Penumbra.Interop.ResourceLoading;
-using Penumbra.Interop.SafeHandles;
 using Penumbra.Services;
-using Penumbra.String.Classes;
+using Penumbra.Util;
-
+
 namespace Penumbra.Interop.Services;
 
 public sealed unsafe class SkinFixer : IDisposable
 {
-    public static readonly Utf8GamePath SkinShpkPath =
+    public static ReadOnlySpan<byte> SkinShpkName
-        Utf8GamePath.FromSpan("shader/sm5/shpk/skin.shpk"u8, out var p) ? p : Utf8GamePath.Empty;
+        => "skin.shpk"u8;
 
     [Signature(Sigs.HumanVTable, ScanType = ScanType.StaticAddress)]
     private readonly nint* _humanVTable = null!;
@@ -48,11 +40,12 @@ public sealed unsafe class SkinFixer : IDisposable
     private readonly ResourceLoader      _resources;
     private readonly CharacterUtility    _utility;
 
-    // CharacterBase to ShpkHandle
+    // MaterialResourceHandle set
-    private readonly ConcurrentDictionary<nint, SafeResourceHandle> _skinShpks = new();
+    private readonly ConcurrentDictionary<nint, Unit> _moddedSkinShpkMaterials = new();
 
     private readonly object _lock = new();
-
+
+    // ConcurrentDictionary.Count uses a lock in its current implementation.
     private int   _moddedSkinShpkCount = 0;
     private ulong _slowPathCallDelta   = 0;
 
@@ -68,83 +61,65 @@ public sealed unsafe class SkinFixer : IDisposable
         _resources            = resources;
         _utility              = utility;
         _communicator    = communicator;
-        _onRenderMaterialHook = Hook<OnRenderMaterialDelegate>.FromAddress(_humanVTable[62], OnRenderHumanMaterial);
+        _onRenderMaterialHook = Hook<OnRenderMaterialDelegate>.FromAddress(_humanVTable[62], OnRenderHumanMaterial);
-        _communicator.CreatedCharacterBase.Subscribe(OnCharacterBaseCreated, CreatedCharacterBase.Priority.SkinFixer);
+        _communicator.MtrlShpkLoaded.Subscribe(OnMtrlShpkLoaded, MtrlShpkLoaded.Priority.SkinFixer);
-        _gameEvents.CharacterBaseDestructor += OnCharacterBaseDestructor;
+        _gameEvents.ResourceHandleDestructor += OnResourceHandleDestructor;
         _onRenderMaterialHook.Enable();
-    }
+    }
-
+
     public void Dispose()
     {
-        _onRenderMaterialHook.Dispose();
+        _onRenderMaterialHook.Dispose();
-        _communicator.CreatedCharacterBase.Unsubscribe(OnCharacterBaseCreated);
+        _communicator.MtrlShpkLoaded.Unsubscribe(OnMtrlShpkLoaded);
-        _gameEvents.CharacterBaseDestructor -= OnCharacterBaseDestructor;
+        _gameEvents.ResourceHandleDestructor -= OnResourceHandleDestructor;
-        foreach (var skinShpk in _skinShpks.Values)
+        _moddedSkinShpkMaterials.Clear();
-            skinShpk.Dispose();
-        _skinShpks.Clear();
         _moddedSkinShpkCount = 0;
-    }
+    }
-
+
     public ulong GetAndResetSlowPathCallDelta()
-        => Interlocked.Exchange(ref _slowPathCallDelta, 0);
+        => Interlocked.Exchange(ref _slowPathCallDelta, 0);
-
+
-    private void OnCharacterBaseCreated(nint gameObject, ModCollection collection, nint drawObject)
+    private static bool IsSkinMaterial(Structs.MtrlResource* mtrlResource)
-    {
+    {
-        if (((CharacterBase*)drawObject)->GetModelType() != CharacterBase.ModelType.Human)
+        if (mtrlResource == null)
-            return;
+            return false;
-
+        
-        Task.Run(() =>
+        var shpkName = MemoryMarshal.CreateReadOnlySpanFromNullTerminated(mtrlResource->ShpkString);
-        {
+        return SkinShpkName.SequenceEqual(shpkName);
-            var skinShpk = SafeResourceHandle.CreateInvalid();
+    }
-            try
+
-            {
+    private void OnMtrlShpkLoaded(nint mtrlResourceHandle, nint gameObject)
-                var data = collection.ToResolveData(gameObject);
+    {
-                if (data.Valid)
+        var mtrl = (Structs.MtrlResource*)mtrlResourceHandle;
-                {
+        var shpk = mtrl->ShpkResourceHandle;
-                    var loadedShpk = _resources.LoadResolvedResource(ResourceCategory.Shader, ResourceType.Shpk, SkinShpkPath.Path, data);
+        if (shpk == null)
-                    skinShpk = new SafeResourceHandle((ResourceHandle*)loadedShpk, false);
+            return;
-                }
+
-            }
+        if (!IsSkinMaterial(mtrl))
-            catch (Exception e)
+            return;
-            {
+
-                Penumbra.Log.Error($"Error while resolving skin.shpk for human {drawObject:X}: {e}");
+        if ((nint)shpk != _utility.DefaultSkinShpkResource)
-            }
+        {
-
+            if (_moddedSkinShpkMaterials.TryAdd(mtrlResourceHandle, Unit.Instance))
-            if (!skinShpk.IsInvalid)
+                Interlocked.Increment(ref _moddedSkinShpkCount);
-            {
+        }
-                if (_skinShpks.TryAdd(drawObject, skinShpk))
+    }
-                {
+
-                    if ((nint)skinShpk.ResourceHandle != _utility.DefaultSkinShpkResource)
+    private void OnResourceHandleDestructor(Structs.ResourceHandle* handle)
-                        Interlocked.Increment(ref _moddedSkinShpkCount);
+    {
-                }
+        if (_moddedSkinShpkMaterials.TryRemove((nint)handle, out _))
-                else
+            Interlocked.Decrement(ref _moddedSkinShpkCount);
-                {
-                    skinShpk.Dispose();
-                }
-            }
-        });
-    }
-
-    private void OnCharacterBaseDestructor(nint characterBase)
-    {
-        if (!_skinShpks.Remove(characterBase, out var skinShpk))
-            return;
-
-        var handle = skinShpk.ResourceHandle;
-        skinShpk.Dispose();
-        if ((nint)handle != _utility.DefaultSkinShpkResource)
-            Interlocked.Decrement(ref _moddedSkinShpkCount);
     }
 
     private nint OnRenderHumanMaterial(nint human, OnRenderMaterialParams* param)
     {
         // If we don't have any on-screen instances of modded skin.shpk, we don't need the slow path at all.
-        if (!Enabled || _moddedSkinShpkCount == 0 || !_skinShpks.TryGetValue(human, out var skinShpk) || skinShpk.IsInvalid)
+        if (!Enabled || _moddedSkinShpkCount == 0)
             return _onRenderMaterialHook!.Original(human, param);
 
-        var material     = param->Model->Materials[param->MaterialIndex];
+        var material     = param->Model->Materials[param->MaterialIndex];
-        var shpkResource = ((Structs.MtrlResource*)material->MaterialResourceHandle)->ShpkResourceHandle;
+        var mtrlResource = (Structs.MtrlResource*)material->MaterialResourceHandle;
-        if ((nint)shpkResource != (nint)skinShpk.ResourceHandle)
+        if (!IsSkinMaterial(mtrlResource))
             return _onRenderMaterialHook!.Original(human, param);
 
         Interlocked.Increment(ref _slowPathCallDelta);
@@ -158,7 +133,7 @@ public sealed unsafe class SkinFixer : IDisposable
         {
             try
             {
-                _utility.Address->SkinShpkResource = (Structs.ResourceHandle*)skinShpk.ResourceHandle;
+                _utility.Address->SkinShpkResource = (Structs.ResourceHandle*)mtrlResource->ShpkResourceHandle;
                 return _onRenderMaterialHook!.Original(human, param);
             }
             finally

Penumbra/Services/CommunicatorService.cs

@@ -24,6 +24,9 @@ public class CommunicatorService : IDisposable
     /// <inheritdoc cref="Communication.CreatedCharacterBase"/>
     public readonly CreatedCharacterBase CreatedCharacterBase = new();
 
+    /// <inheritdoc cref="Communication.MtrlShpkLoaded"/>
+    public readonly MtrlShpkLoaded MtrlShpkLoaded = new();
+
     /// <inheritdoc cref="Communication.ModDataChanged"/>
     public readonly ModDataChanged ModDataChanged = new();
 
@@ -75,6 +78,7 @@ public class CommunicatorService : IDisposable
         TemporaryGlobalModChange.Dispose();
         CreatingCharacterBase.Dispose();
         CreatedCharacterBase.Dispose();
+        MtrlShpkLoaded.Dispose();
         ModDataChanged.Dispose();
         ModOptionChanged.Dispose();
         ModDiscoveryStarted.Dispose();

Penumbra/UI/Tabs/DebugTab.cs

@@ -603,7 +603,7 @@ public class DebugTab : Window, ITab
             ImGui.SameLine();
             ImGui.Dummy(ImGuiHelpers.ScaledVector2(20, 0));
             ImGui.SameLine();
-            ImGui.TextUnformatted($"Draw Objects with Modded skin.shpk: {_skinFixer.ModdedSkinShpkCount}");
+            ImGui.TextUnformatted($"Materials with Modded skin.shpk: {_skinFixer.ModdedSkinShpkCount}");
         }
 
         using var table = Table("##CharacterUtility", 7, ImGuiTableFlags.RowBg | ImGuiTableFlags.SizingFixedFit,

Penumbra/Util/Unit.cs

@@ -0,0 +1,26 @@
+using System;
+
+namespace Penumbra.Util;
+
+/// <summary>
+/// An empty structure. Can be used as value of a concurrent dictionary, to use it as a set.
+/// </summary>
+public readonly struct Unit : IEquatable<Unit>
+{
+    public static readonly Unit Instance = new();
+
+    public bool Equals(Unit other)
+        => true;
+
+    public override bool Equals(object? obj)
+        => obj is Unit;
+
+    public override int GetHashCode()
+        => 0;
+
+    public static bool operator ==(Unit left, Unit right)
+        => true;
+
+    public static bool operator !=(Unit left, Unit right)
+        => false;
+}