From 32608ea45b1c51b8ebaf08d21fbbe2831a183022 Mon Sep 17 00:00:00 2001 From: Exter-N Date: Tue, 5 Sep 2023 12:53:53 +0200 Subject: [PATCH 1/2] 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. --- .../Communication/CreatedCharacterBase.cs | 3 - Penumbra/Communication/MtrlShpkLoaded.cs | 24 +++ .../Interop/PathResolving/SubfileHelper.cs | 25 ++-- Penumbra/Interop/Services/SkinFixer.cs | 137 +++++++----------- Penumbra/Services/CommunicatorService.cs | 4 + Penumbra/UI/Tabs/DebugTab.cs | 2 +- Penumbra/Util/Unit.cs | 26 ++++ 7 files changed, 126 insertions(+), 95 deletions(-) create mode 100644 Penumbra/Communication/MtrlShpkLoaded.cs create mode 100644 Penumbra/Util/Unit.cs diff --git a/Penumbra/Communication/CreatedCharacterBase.cs b/Penumbra/Communication/CreatedCharacterBase.cs index 48ba86a5..69d84ce2 100644 --- a/Penumbra/Communication/CreatedCharacterBase.cs +++ b/Penumbra/Communication/CreatedCharacterBase.cs @@ -16,9 +16,6 @@ public sealed class CreatedCharacterBase : EventWrapper Api = int.MinValue, - - /// - SkinFixer = 0, } public CreatedCharacterBase() diff --git a/Penumbra/Communication/MtrlShpkLoaded.cs b/Penumbra/Communication/MtrlShpkLoaded.cs new file mode 100644 index 00000000..4b5600c9 --- /dev/null +++ b/Penumbra/Communication/MtrlShpkLoaded.cs @@ -0,0 +1,24 @@ +using System; +using OtterGui.Classes; + +namespace Penumbra.Communication; + +/// +/// Parameter is the material resource handle for which the shader package has been loaded. +/// Parameter is the associated game object. +/// +public sealed class MtrlShpkLoaded : EventWrapper, MtrlShpkLoaded.Priority> +{ + public enum Priority + { + /// + SkinFixer = 0, + } + + public MtrlShpkLoaded() + : base(nameof(MtrlShpkLoaded)) + { } + + public void Invoke(nint mtrlResourceHandle, nint gameObject) + => Invoke(this, mtrlResourceHandle, gameObject); +} diff --git a/Penumbra/Interop/PathResolving/SubfileHelper.cs b/Penumbra/Interop/PathResolving/SubfileHelper.cs index 05f42220..ffa3ac24 100644 --- a/Penumbra/Interop/PathResolving/SubfileHelper.cs +++ b/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; /// public unsafe class SubfileHelper : IDisposable, IReadOnlyCollection> { - private readonly PerformanceTracker _performance; - private readonly ResourceLoader _loader; - private readonly GameEventManager _events; + private readonly PerformanceTracker _performance; + private readonly ResourceLoader _loader; + private readonly GameEventManager _events; + private readonly CommunicatorService _communicator; private readonly ThreadLocal _mtrlData = new(() => ResolveData.Invalid); private readonly ThreadLocal _avfxData = new(() => ResolveData.Invalid); private readonly ConcurrentDictionary _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; - _loader = loader; - _events = events; + _performance = performance; + _loader = loader; + _events = events; + _communicator = communicator; _loadMtrlShpkHook.Enable(); _loadMtrlTexHook.Enable(); @@ -150,10 +153,12 @@ public unsafe class SubfileHelper : IDisposable, IReadOnlyCollection SkinShpkName + => "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 - private readonly ConcurrentDictionary _skinShpks = new(); + // MaterialResourceHandle set + private readonly ConcurrentDictionary _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.FromAddress(_humanVTable[62], OnRenderHumanMaterial); - _communicator.CreatedCharacterBase.Subscribe(OnCharacterBaseCreated, CreatedCharacterBase.Priority.SkinFixer); - _gameEvents.CharacterBaseDestructor += OnCharacterBaseDestructor; + _onRenderMaterialHook = Hook.FromAddress(_humanVTable[62], OnRenderHumanMaterial); + _communicator.MtrlShpkLoaded.Subscribe(OnMtrlShpkLoaded, MtrlShpkLoaded.Priority.SkinFixer); + _gameEvents.ResourceHandleDestructor += OnResourceHandleDestructor; _onRenderMaterialHook.Enable(); - } - + } + public void Dispose() { - _onRenderMaterialHook.Dispose(); - _communicator.CreatedCharacterBase.Unsubscribe(OnCharacterBaseCreated); - _gameEvents.CharacterBaseDestructor -= OnCharacterBaseDestructor; - foreach (var skinShpk in _skinShpks.Values) - skinShpk.Dispose(); - _skinShpks.Clear(); + _onRenderMaterialHook.Dispose(); + _communicator.MtrlShpkLoaded.Unsubscribe(OnMtrlShpkLoaded); + _gameEvents.ResourceHandleDestructor -= OnResourceHandleDestructor; + _moddedSkinShpkMaterials.Clear(); _moddedSkinShpkCount = 0; - } - + } + public ulong GetAndResetSlowPathCallDelta() - => Interlocked.Exchange(ref _slowPathCallDelta, 0); - - private void OnCharacterBaseCreated(nint gameObject, ModCollection collection, nint drawObject) - { - if (((CharacterBase*)drawObject)->GetModelType() != CharacterBase.ModelType.Human) - return; - - Task.Run(() => - { - var skinShpk = SafeResourceHandle.CreateInvalid(); - try - { - var data = collection.ToResolveData(gameObject); - if (data.Valid) - { - var loadedShpk = _resources.LoadResolvedResource(ResourceCategory.Shader, ResourceType.Shpk, SkinShpkPath.Path, data); - skinShpk = new SafeResourceHandle((ResourceHandle*)loadedShpk, false); - } - } - catch (Exception e) - { - Penumbra.Log.Error($"Error while resolving skin.shpk for human {drawObject:X}: {e}"); - } - - if (!skinShpk.IsInvalid) - { - if (_skinShpks.TryAdd(drawObject, skinShpk)) - { - if ((nint)skinShpk.ResourceHandle != _utility.DefaultSkinShpkResource) - Interlocked.Increment(ref _moddedSkinShpkCount); - } - else - { - 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); + => Interlocked.Exchange(ref _slowPathCallDelta, 0); + + private static bool IsSkinMaterial(Structs.MtrlResource* mtrlResource) + { + if (mtrlResource == null) + return false; + + var shpkName = MemoryMarshal.CreateReadOnlySpanFromNullTerminated(mtrlResource->ShpkString); + return SkinShpkName.SequenceEqual(shpkName); + } + + private void OnMtrlShpkLoaded(nint mtrlResourceHandle, nint gameObject) + { + var mtrl = (Structs.MtrlResource*)mtrlResourceHandle; + var shpk = mtrl->ShpkResourceHandle; + if (shpk == null) + return; + + if (!IsSkinMaterial(mtrl)) + return; + + if ((nint)shpk != _utility.DefaultSkinShpkResource) + { + if (_moddedSkinShpkMaterials.TryAdd(mtrlResourceHandle, Unit.Instance)) + Interlocked.Increment(ref _moddedSkinShpkCount); + } + } + + private void OnResourceHandleDestructor(Structs.ResourceHandle* handle) + { + if (_moddedSkinShpkMaterials.TryRemove((nint)handle, out _)) + 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 shpkResource = ((Structs.MtrlResource*)material->MaterialResourceHandle)->ShpkResourceHandle; - if ((nint)shpkResource != (nint)skinShpk.ResourceHandle) + var material = param->Model->Materials[param->MaterialIndex]; + var mtrlResource = (Structs.MtrlResource*)material->MaterialResourceHandle; + 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 diff --git a/Penumbra/Services/CommunicatorService.cs b/Penumbra/Services/CommunicatorService.cs index 728b391c..97340f6b 100644 --- a/Penumbra/Services/CommunicatorService.cs +++ b/Penumbra/Services/CommunicatorService.cs @@ -24,6 +24,9 @@ public class CommunicatorService : IDisposable /// public readonly CreatedCharacterBase CreatedCharacterBase = new(); + /// + public readonly MtrlShpkLoaded MtrlShpkLoaded = new(); + /// 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(); diff --git a/Penumbra/UI/Tabs/DebugTab.cs b/Penumbra/UI/Tabs/DebugTab.cs index c24d64fa..d02da883 100644 --- a/Penumbra/UI/Tabs/DebugTab.cs +++ b/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, diff --git a/Penumbra/Util/Unit.cs b/Penumbra/Util/Unit.cs new file mode 100644 index 00000000..9b8f4b1e --- /dev/null +++ b/Penumbra/Util/Unit.cs @@ -0,0 +1,26 @@ +using System; + +namespace Penumbra.Util; + +/// +/// An empty structure. Can be used as value of a concurrent dictionary, to use it as a set. +/// +public readonly struct Unit : IEquatable +{ + 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; +} From 0e0733dab02b86db7e68cdec0612098fc3982df3 Mon Sep 17 00:00:00 2001 From: Ottermandias Date: Tue, 5 Sep 2023 14:48:06 +0200 Subject: [PATCH 2/2] Some formatting, use ConcurrentSet explicitly for clarity. --- OtterGui | 2 +- .../Interop/PathResolving/SubfileHelper.cs | 14 +-- Penumbra/Interop/Services/SkinFixer.cs | 113 ++++++++---------- Penumbra/Util/Unit.cs | 26 ---- 4 files changed, 61 insertions(+), 94 deletions(-) delete mode 100644 Penumbra/Util/Unit.cs diff --git a/OtterGui b/OtterGui index 8c7a309d..86ec4d72 160000 --- a/OtterGui +++ b/OtterGui @@ -1 +1 @@ -Subproject commit 8c7a309d039fdf008c85cf51923b4eac51b32428 +Subproject commit 86ec4d72c9c9ed57aa7be4a7d0c81069c5b94ad7 diff --git a/Penumbra/Interop/PathResolving/SubfileHelper.cs b/Penumbra/Interop/PathResolving/SubfileHelper.cs index ffa3ac24..c0b8c5e3 100644 --- a/Penumbra/Interop/PathResolving/SubfileHelper.cs +++ b/Penumbra/Interop/PathResolving/SubfileHelper.cs @@ -11,7 +11,7 @@ using Penumbra.GameData.Enums; using Penumbra.Interop.ResourceLoading; using Penumbra.Interop.Services; using Penumbra.Interop.Structs; -using Penumbra.Services; +using Penumbra.Services; using Penumbra.String; using Penumbra.String.Classes; using Penumbra.Util; @@ -27,7 +27,7 @@ public unsafe class SubfileHelper : IDisposable, IReadOnlyCollection _mtrlData = new(() => ResolveData.Invalid); @@ -41,7 +41,7 @@ public unsafe class SubfileHelper : IDisposable, IReadOnlyCollectionFileType) { case ResourceType.Mtrl: - case ResourceType.Avfx: + case ResourceType.Avfx: if (handle->FileSize == 0) _subFileCollection[(nint)handle] = resolveData; @@ -153,11 +153,11 @@ public unsafe class SubfileHelper : IDisposable, IReadOnlyCollection SkinShpkName + public static ReadOnlySpan SkinShpkName => "skin.shpk"u8; [Signature(Sigs.HumanVTable, ScanType = ScanType.StaticAddress)] @@ -37,90 +35,85 @@ public sealed unsafe class SkinFixer : IDisposable private readonly GameEventManager _gameEvents; private readonly CommunicatorService _communicator; - private readonly ResourceLoader _resources; private readonly CharacterUtility _utility; - - // MaterialResourceHandle set - private readonly ConcurrentDictionary _moddedSkinShpkMaterials = new(); + + // MaterialResourceHandle set + private readonly ConcurrentSet _moddedSkinShpkMaterials = new(); private readonly object _lock = new(); - + // ConcurrentDictionary.Count uses a lock in its current implementation. - private int _moddedSkinShpkCount = 0; - private ulong _slowPathCallDelta = 0; + private int _moddedSkinShpkCount; + private ulong _slowPathCallDelta; public bool Enabled { get; internal set; } = true; public int ModdedSkinShpkCount => _moddedSkinShpkCount; - public SkinFixer(GameEventManager gameEvents, ResourceLoader resources, CharacterUtility utility, CommunicatorService communicator) + public SkinFixer(GameEventManager gameEvents, CharacterUtility utility, CommunicatorService communicator) { SignatureHelper.Initialise(this); _gameEvents = gameEvents; - _resources = resources; _utility = utility; - _communicator = communicator; - _onRenderMaterialHook = Hook.FromAddress(_humanVTable[62], OnRenderHumanMaterial); + _communicator = communicator; + _onRenderMaterialHook = Hook.FromAddress(_humanVTable[62], OnRenderHumanMaterial); _communicator.MtrlShpkLoaded.Subscribe(OnMtrlShpkLoaded, MtrlShpkLoaded.Priority.SkinFixer); _gameEvents.ResourceHandleDestructor += OnResourceHandleDestructor; _onRenderMaterialHook.Enable(); - } - + } + public void Dispose() { - _onRenderMaterialHook.Dispose(); - _communicator.MtrlShpkLoaded.Unsubscribe(OnMtrlShpkLoaded); + _onRenderMaterialHook.Dispose(); + _communicator.MtrlShpkLoaded.Unsubscribe(OnMtrlShpkLoaded); _gameEvents.ResourceHandleDestructor -= OnResourceHandleDestructor; _moddedSkinShpkMaterials.Clear(); _moddedSkinShpkCount = 0; - } - + } + public ulong GetAndResetSlowPathCallDelta() - => Interlocked.Exchange(ref _slowPathCallDelta, 0); - - private static bool IsSkinMaterial(Structs.MtrlResource* mtrlResource) - { - if (mtrlResource == null) - return false; - - var shpkName = MemoryMarshal.CreateReadOnlySpanFromNullTerminated(mtrlResource->ShpkString); - return SkinShpkName.SequenceEqual(shpkName); - } - - private void OnMtrlShpkLoaded(nint mtrlResourceHandle, nint gameObject) - { - var mtrl = (Structs.MtrlResource*)mtrlResourceHandle; - var shpk = mtrl->ShpkResourceHandle; - if (shpk == null) - return; - - if (!IsSkinMaterial(mtrl)) - return; - - if ((nint)shpk != _utility.DefaultSkinShpkResource) - { - if (_moddedSkinShpkMaterials.TryAdd(mtrlResourceHandle, Unit.Instance)) - Interlocked.Increment(ref _moddedSkinShpkCount); - } - } - - private void OnResourceHandleDestructor(Structs.ResourceHandle* handle) - { - if (_moddedSkinShpkMaterials.TryRemove((nint)handle, out _)) - Interlocked.Decrement(ref _moddedSkinShpkCount); + => Interlocked.Exchange(ref _slowPathCallDelta, 0); + + private static bool IsSkinMaterial(Structs.MtrlResource* mtrlResource) + { + if (mtrlResource == null) + return false; + + var shpkName = MemoryMarshal.CreateReadOnlySpanFromNullTerminated(mtrlResource->ShpkString); + return SkinShpkName.SequenceEqual(shpkName); + } + + private void OnMtrlShpkLoaded(nint mtrlResourceHandle, nint gameObject) + { + var mtrl = (Structs.MtrlResource*)mtrlResourceHandle; + var shpk = mtrl->ShpkResourceHandle; + if (shpk == null) + return; + + if (!IsSkinMaterial(mtrl) || (nint)shpk == _utility.DefaultSkinShpkResource) + return; + + if (_moddedSkinShpkMaterials.TryAdd(mtrlResourceHandle)) + Interlocked.Increment(ref _moddedSkinShpkCount); + } + + private void OnResourceHandleDestructor(Structs.ResourceHandle* handle) + { + if (_moddedSkinShpkMaterials.TryRemove((nint)handle)) + 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) - return _onRenderMaterialHook!.Original(human, param); + return _onRenderMaterialHook.Original(human, param); - var material = param->Model->Materials[param->MaterialIndex]; - var mtrlResource = (Structs.MtrlResource*)material->MaterialResourceHandle; - if (!IsSkinMaterial(mtrlResource)) - return _onRenderMaterialHook!.Original(human, param); + var material = param->Model->Materials[param->MaterialIndex]; + var mtrlResource = (Structs.MtrlResource*)material->MaterialResourceHandle; + if (!IsSkinMaterial(mtrlResource)) + return _onRenderMaterialHook.Original(human, param); Interlocked.Increment(ref _slowPathCallDelta); @@ -134,7 +127,7 @@ public sealed unsafe class SkinFixer : IDisposable try { _utility.Address->SkinShpkResource = (Structs.ResourceHandle*)mtrlResource->ShpkResourceHandle; - return _onRenderMaterialHook!.Original(human, param); + return _onRenderMaterialHook.Original(human, param); } finally { diff --git a/Penumbra/Util/Unit.cs b/Penumbra/Util/Unit.cs deleted file mode 100644 index 9b8f4b1e..00000000 --- a/Penumbra/Util/Unit.cs +++ /dev/null @@ -1,26 +0,0 @@ -using System; - -namespace Penumbra.Util; - -/// -/// An empty structure. Can be used as value of a concurrent dictionary, to use it as a set. -/// -public readonly struct Unit : IEquatable -{ - 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; -}