From 8a2ba70cc3d4bce55e625a7ecad58ab1c312e9d5 Mon Sep 17 00:00:00 2001 From: goat Date: Tue, 14 Nov 2023 20:23:02 +0100 Subject: [PATCH 01/17] chore: upgrade projects to net8 --- Dalamud.Common/Dalamud.Common.csproj | 2 +- Dalamud.CorePlugin/Dalamud.CorePlugin.csproj | 2 +- Dalamud.Injector/Dalamud.Injector.csproj | 2 +- Dalamud.Test/Dalamud.Test.csproj | 2 +- Dalamud/Dalamud.csproj | 2 +- global.json | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/Dalamud.Common/Dalamud.Common.csproj b/Dalamud.Common/Dalamud.Common.csproj index ac5d3fdba..594e09021 100644 --- a/Dalamud.Common/Dalamud.Common.csproj +++ b/Dalamud.Common/Dalamud.Common.csproj @@ -1,7 +1,7 @@ - net7.0 + net8.0 enable enable diff --git a/Dalamud.CorePlugin/Dalamud.CorePlugin.csproj b/Dalamud.CorePlugin/Dalamud.CorePlugin.csproj index 67ca26dee..1e87fcf9b 100644 --- a/Dalamud.CorePlugin/Dalamud.CorePlugin.csproj +++ b/Dalamud.CorePlugin/Dalamud.CorePlugin.csproj @@ -1,7 +1,7 @@ Dalamud.CorePlugin - net7.0-windows + net8.0-windows x64 10.0 true diff --git a/Dalamud.Injector/Dalamud.Injector.csproj b/Dalamud.Injector/Dalamud.Injector.csproj index d8a74e58d..1ff29ea66 100644 --- a/Dalamud.Injector/Dalamud.Injector.csproj +++ b/Dalamud.Injector/Dalamud.Injector.csproj @@ -1,7 +1,7 @@ - net7.0 + net8.0 win-x64 x64 x64;AnyCPU diff --git a/Dalamud.Test/Dalamud.Test.csproj b/Dalamud.Test/Dalamud.Test.csproj index 8f4ccf0dd..8dcdf0889 100644 --- a/Dalamud.Test/Dalamud.Test.csproj +++ b/Dalamud.Test/Dalamud.Test.csproj @@ -1,7 +1,7 @@ - net7.0-windows + net8.0-windows win-x64 x64 x64;AnyCPU diff --git a/Dalamud/Dalamud.csproj b/Dalamud/Dalamud.csproj index 5c971489a..095a29f4d 100644 --- a/Dalamud/Dalamud.csproj +++ b/Dalamud/Dalamud.csproj @@ -1,7 +1,7 @@ - net7.0-windows + net8.0-windows x64 x64;AnyCPU 11.0 diff --git a/global.json b/global.json index 3d9090158..c65c9eac9 100644 --- a/global.json +++ b/global.json @@ -1,6 +1,6 @@ { "sdk": { - "version": "7.0.0", + "version": "8.0.0", "rollForward": "latestMinor", "allowPrerelease": true } From f8bd6d20ef2603a5b361c2c3e50d9c0c4240f41a Mon Sep 17 00:00:00 2001 From: goat Date: Tue, 14 Nov 2023 20:28:36 +0100 Subject: [PATCH 02/17] chore: remove all MonoMod hooks for now --- Dalamud/Dalamud.csproj | 9 --------- Dalamud/Logging/Internal/TaskTracker.cs | 9 ++++++--- Dalamud/Plugin/Internal/PluginManager.cs | 20 ++++++++++++++------ Dalamud/Plugin/Internal/Types/LocalPlugin.cs | 3 ++- 4 files changed, 22 insertions(+), 19 deletions(-) diff --git a/Dalamud/Dalamud.csproj b/Dalamud/Dalamud.csproj index 095a29f4d..f786b864c 100644 --- a/Dalamud/Dalamud.csproj +++ b/Dalamud/Dalamud.csproj @@ -74,7 +74,6 @@ all - @@ -121,14 +120,6 @@ - - - - monomod - - - - $(OutputPath)TEMP_gitver.txt diff --git a/Dalamud/Logging/Internal/TaskTracker.cs b/Dalamud/Logging/Internal/TaskTracker.cs index b65f0efa7..da4007570 100644 --- a/Dalamud/Logging/Internal/TaskTracker.cs +++ b/Dalamud/Logging/Internal/TaskTracker.cs @@ -23,7 +23,8 @@ internal class TaskTracker : IDisposable, IServiceType [ServiceManager.ServiceDependency] private readonly Framework framework = Service.Get(); - private MonoMod.RuntimeDetour.Hook? scheduleAndStartHook; + // NET8 CHORE + // private MonoMod.RuntimeDetour.Hook? scheduleAndStartHook; private bool enabled = false; [ServiceManager.ServiceConstructor] @@ -121,7 +122,8 @@ internal class TaskTracker : IDisposable, IServiceType /// public void Dispose() { - this.scheduleAndStartHook?.Dispose(); + // NET8 CHORE + // this.scheduleAndStartHook?.Dispose(); this.framework.Update -= this.FrameworkOnUpdate; } @@ -170,7 +172,8 @@ internal class TaskTracker : IDisposable, IServiceType return; } - this.scheduleAndStartHook = new MonoMod.RuntimeDetour.Hook(targetMethod, patchMethod); + // NET8 CHORE + // this.scheduleAndStartHook = new MonoMod.RuntimeDetour.Hook(targetMethod, patchMethod); Log.Information("AddToActiveTasks Hooked!"); } diff --git a/Dalamud/Plugin/Internal/PluginManager.cs b/Dalamud/Plugin/Internal/PluginManager.cs index ff6b045be..9b5fa0285 100644 --- a/Dalamud/Plugin/Internal/PluginManager.cs +++ b/Dalamud/Plugin/Internal/PluginManager.cs @@ -136,7 +136,8 @@ internal partial class PluginManager : IDisposable, IServiceType this.configuration.PluginTestingOptIns ??= new List(); - this.ApplyPatches(); + // NET8 CHORE + //this.ApplyPatches(); } /// @@ -393,8 +394,9 @@ internal partial class PluginManager : IDisposable, IServiceType plugin.ExplicitDisposeIgnoreExceptions($"Error disposing {plugin.Name}", Log); } - this.assemblyLocationMonoHook?.Dispose(); - this.assemblyCodeBaseMonoHook?.Dispose(); + // NET8 CHORE + // this.assemblyLocationMonoHook?.Dispose(); + // this.assemblyCodeBaseMonoHook?.Dispose(); } /// @@ -804,7 +806,8 @@ internal partial class PluginManager : IDisposable, IServiceType this.installedPluginsList.Remove(plugin); } - PluginLocations.Remove(plugin.AssemblyName?.FullName ?? string.Empty, out _); + // NET8 CHORE + // PluginLocations.Remove(plugin.AssemblyName?.FullName ?? string.Empty, out _); this.NotifyinstalledPluginsListChanged(); this.NotifyAvailablePluginsChanged(); @@ -1443,7 +1446,8 @@ internal partial class PluginManager : IDisposable, IServiceType } catch (InvalidPluginException) { - PluginLocations.Remove(plugin.AssemblyName?.FullName ?? string.Empty, out _); + // NET8 CHORE + // PluginLocations.Remove(plugin.AssemblyName?.FullName ?? string.Empty, out _); throw; } catch (BannedPluginException) @@ -1489,7 +1493,8 @@ internal partial class PluginManager : IDisposable, IServiceType } else { - PluginLocations.Remove(plugin.AssemblyName?.FullName ?? string.Empty, out _); + // NET8 CHORE + // PluginLocations.Remove(plugin.AssemblyName?.FullName ?? string.Empty, out _); throw; } } @@ -1580,6 +1585,8 @@ internal partial class PluginManager : IDisposable, IServiceType } } +// NET8 CHORE +/* /// /// Class responsible for loading and unloading plugins. /// This contains the assembly patching functionality to resolve assembly locations. @@ -1687,3 +1694,4 @@ internal partial class PluginManager this.assemblyCodeBaseMonoHook = new MonoMod.RuntimeDetour.Hook(codebaseTarget, codebasePatch); } } +*/ diff --git a/Dalamud/Plugin/Internal/Types/LocalPlugin.cs b/Dalamud/Plugin/Internal/Types/LocalPlugin.cs index 5d132fd9c..81903d970 100644 --- a/Dalamud/Plugin/Internal/Types/LocalPlugin.cs +++ b/Dalamud/Plugin/Internal/Types/LocalPlugin.cs @@ -408,7 +408,8 @@ internal class LocalPlugin : IDisposable } // Update the location for the Location and CodeBase patches - PluginManager.PluginLocations[this.pluginType.Assembly.FullName] = new PluginPatchData(this.DllFile); + // NET8 CHORE + // PluginManager.PluginLocations[this.pluginType.Assembly.FullName] = new PluginPatchData(this.DllFile); this.DalamudInterface = new DalamudPluginInterface(this, reason); From 0c0cfbca823c18484937c70d34e0bfca33cef2b3 Mon Sep 17 00:00:00 2001 From: goat <16760685+goaaats@users.noreply.github.com> Date: Tue, 14 Nov 2023 20:31:24 +0100 Subject: [PATCH 03/17] ci: add setup-dotnet 8.0.0 --- .github/workflows/main.yml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 7ada48e50..09b7a86a8 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -16,6 +16,9 @@ jobs: fetch-depth: 0 - name: Setup MSBuild uses: microsoft/setup-msbuild@v1.0.2 + - uses: actions/setup-dotnet@v3 + with: + dotnet-version: '8.0.0' - name: Define VERSION run: | $env:COMMIT = $env:GITHUB_SHA.Substring(0, 7) From a6bce462d7c3d7b97b5be3ebf1e7ea6c81cd197a Mon Sep 17 00:00:00 2001 From: goat <16760685+goaaats@users.noreply.github.com> Date: Tue, 14 Nov 2023 20:39:13 +0100 Subject: [PATCH 04/17] it's actually 8.0.100 --- .github/workflows/main.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 09b7a86a8..4157c5f41 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -18,7 +18,7 @@ jobs: uses: microsoft/setup-msbuild@v1.0.2 - uses: actions/setup-dotnet@v3 with: - dotnet-version: '8.0.0' + dotnet-version: '8.0.100' - name: Define VERSION run: | $env:COMMIT = $env:GITHUB_SHA.Substring(0, 7) From fc837f723b1e5daf130125558cb0c1317e46319a Mon Sep 17 00:00:00 2001 From: Ottermandias <70807659+Ottermandias@users.noreply.github.com> Date: Sat, 25 Nov 2023 21:24:58 +0100 Subject: [PATCH 05/17] Check for function pointers in SignatureHelper. (#1540) --- Dalamud/Utility/Signatures/SignatureHelper.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Dalamud/Utility/Signatures/SignatureHelper.cs b/Dalamud/Utility/Signatures/SignatureHelper.cs index 51f59bba2..e34d8ef69 100755 --- a/Dalamud/Utility/Signatures/SignatureHelper.cs +++ b/Dalamud/Utility/Signatures/SignatureHelper.cs @@ -90,7 +90,7 @@ internal static class SignatureHelper switch (sig.UseFlags) { - case SignatureUseFlags.Auto when actualType == typeof(IntPtr) || actualType.IsPointer || actualType.IsAssignableTo(typeof(Delegate)): + case SignatureUseFlags.Auto when actualType == typeof(IntPtr) || actualType.IsFunctionPointer || actualType.IsUnmanagedFunctionPointer || actualType.IsPointer || actualType.IsAssignableTo(typeof(Delegate)): case SignatureUseFlags.Pointer: { if (actualType.IsAssignableTo(typeof(Delegate))) From 324acbf937629ebd0c52b9ed8f84651e57947fd9 Mon Sep 17 00:00:00 2001 From: goat <16760685+goaaats@users.noreply.github.com> Date: Sat, 16 Dec 2023 21:18:01 +0100 Subject: [PATCH 06/17] buildfix: use c#12 --- Dalamud/Dalamud.csproj | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Dalamud/Dalamud.csproj b/Dalamud/Dalamud.csproj index 92b468dde..26889ad95 100644 --- a/Dalamud/Dalamud.csproj +++ b/Dalamud/Dalamud.csproj @@ -4,7 +4,7 @@ net8.0-windows x64 x64;AnyCPU - 11.0 + 12.0 From b5c689c0ba32a7581de9955d22d021dfdbd72c9b Mon Sep 17 00:00:00 2001 From: marzent Date: Thu, 18 Jan 2024 11:27:37 +0100 Subject: [PATCH 07/17] fix bad a0f4baf merge --- Dalamud/Plugin/Internal/PluginManager.cs | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/Dalamud/Plugin/Internal/PluginManager.cs b/Dalamud/Plugin/Internal/PluginManager.cs index e88dc3f8e..a3a4fb7e4 100644 --- a/Dalamud/Plugin/Internal/PluginManager.cs +++ b/Dalamud/Plugin/Internal/PluginManager.cs @@ -147,6 +147,14 @@ internal partial class PluginManager : IDisposable, IServiceType // NET8 CHORE //this.ApplyPatches(); + + registerStartupBlocker( + Task.Run(this.LoadAndStartLoadSyncPlugins), + "Waiting for plugins that asked to be loaded before the game."); + + registerUnloadAfter( + ResolvePossiblePluginDependencyServices(), + "See the attached comment for the called function."); } /// From 6f2ebdc7a785e302164c0b9c866201b626d3a219 Mon Sep 17 00:00:00 2001 From: Kaz Wolfe Date: Mon, 11 Mar 2024 09:15:45 -0700 Subject: [PATCH 08/17] Upgrade ClientStructs to .NET 8 --- lib/FFXIVClientStructs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/FFXIVClientStructs b/lib/FFXIVClientStructs index ac2ced26f..7a1c1dd80 160000 --- a/lib/FFXIVClientStructs +++ b/lib/FFXIVClientStructs @@ -1 +1 @@ -Subproject commit ac2ced26fc98153c65f5b8f0eaf0f464258ff683 +Subproject commit 7a1c1dd8035d326dfc307412462d3e9aeb7fbb6d From 31227016c18cbbd1569e9becdbc30ac9deeb3663 Mon Sep 17 00:00:00 2001 From: rootdarkarchon Date: Wed, 13 Mar 2024 22:13:29 +0100 Subject: [PATCH 09/17] Add Object Table Cache (#1708) Proposed improvement to object table access speeds; prevents creating objects for every plugin iterating the object table. --------- Co-authored-by: rootdarkarchon Co-authored-by: Soreepeong --- .../Game/ClientState/Objects/ObjectTable.cs | 230 ++++++++++++++++-- .../ClientState/Objects/Types/GameObject.cs | 2 +- Dalamud/Game/Framework.cs | 39 +-- 3 files changed, 230 insertions(+), 41 deletions(-) diff --git a/Dalamud/Game/ClientState/Objects/ObjectTable.cs b/Dalamud/Game/ClientState/Objects/ObjectTable.cs index 278c0772f..b643abedb 100644 --- a/Dalamud/Game/ClientState/Objects/ObjectTable.cs +++ b/Dalamud/Game/ClientState/Objects/ObjectTable.cs @@ -1,13 +1,18 @@ -using System; using System.Collections; using System.Collections.Generic; +using System.Runtime.CompilerServices; using Dalamud.Game.ClientState.Objects.Enums; using Dalamud.Game.ClientState.Objects.SubKinds; using Dalamud.Game.ClientState.Objects.Types; using Dalamud.IoC; using Dalamud.IoC.Internal; +using Dalamud.Plugin.Internal; using Dalamud.Plugin.Services; +using Dalamud.Utility; + +using Microsoft.Extensions.ObjectPool; + using Serilog; namespace Dalamud.Game.ClientState.Objects; @@ -21,22 +26,45 @@ namespace Dalamud.Game.ClientState.Objects; #pragma warning disable SA1015 [ResolveVia] #pragma warning restore SA1015 -internal sealed partial class ObjectTable : IServiceType, IObjectTable +internal sealed partial class ObjectTable : IServiceType, IObjectTable, IDisposable { private const int ObjectTableLength = 599; - private readonly ClientStateAddressResolver address; + private readonly ClientState clientState; + private readonly Framework framework; + private readonly CachedEntry[] cachedObjectTable = new CachedEntry[ObjectTableLength]; + + private readonly ObjectPool multiThreadedEnumerators = + new DefaultObjectPoolProvider().Create(); + + private readonly Enumerator?[] frameworkThreadEnumerators = new Enumerator?[64]; + + private long nextMultithreadedUsageWarnTime; [ServiceManager.ServiceConstructor] - private ObjectTable(ClientState clientState) + private ObjectTable(ClientState clientState, Framework framework) { - this.address = clientState.AddressResolver; + this.clientState = clientState; + this.framework = framework; + foreach (ref var e in this.cachedObjectTable.AsSpan()) + e = CachedEntry.CreateNew(); + for (var i = 0; i < this.frameworkThreadEnumerators.Length; i++) + this.frameworkThreadEnumerators[i] = new(this, i); - Log.Verbose($"Object table address 0x{this.address.ObjectTable.ToInt64():X}"); + framework.BeforeUpdate += this.FrameworkOnBeforeUpdate; + Log.Verbose($"Object table address 0x{this.clientState.AddressResolver.ObjectTable.ToInt64():X}"); } /// - public IntPtr Address => this.address.ObjectTable; + public nint Address + { + get + { + _ = this.WarnMultithreadedUsage(); + + return this.clientState.AddressResolver.ObjectTable; + } + } /// public int Length => ObjectTableLength; @@ -46,14 +74,17 @@ internal sealed partial class ObjectTable : IServiceType, IObjectTable { get { - var address = this.GetObjectAddress(index); - return this.CreateObjectReference(address); + _ = this.WarnMultithreadedUsage(); + + return index is >= ObjectTableLength or < 0 ? null : this.cachedObjectTable[index].ActiveObject; } } /// public GameObject? SearchById(ulong objectId) { + _ = this.WarnMultithreadedUsage(); + if (objectId is GameObject.InvalidGameObjectId or 0) return null; @@ -70,23 +101,22 @@ internal sealed partial class ObjectTable : IServiceType, IObjectTable } /// - public unsafe IntPtr GetObjectAddress(int index) + public nint GetObjectAddress(int index) { - if (index < 0 || index >= ObjectTableLength) - return IntPtr.Zero; + _ = this.WarnMultithreadedUsage(); - return *(IntPtr*)(this.address.ObjectTable + (8 * index)); + return index is < 0 or >= ObjectTableLength ? nint.Zero : this.GetObjectAddressUnsafe(index); } /// - public unsafe GameObject? CreateObjectReference(IntPtr address) + public unsafe GameObject? CreateObjectReference(nint address) { - var clientState = Service.GetNullable(); + _ = this.WarnMultithreadedUsage(); - if (clientState == null || clientState.LocalContentId == 0) + if (this.clientState.LocalContentId == 0) return null; - if (address == IntPtr.Zero) + if (address == nint.Zero) return null; var obj = (FFXIVClientStructs.FFXIV.Client.Game.Object.GameObject*)address; @@ -104,6 +134,88 @@ internal sealed partial class ObjectTable : IServiceType, IObjectTable _ => new GameObject(address), }; } + + /// + public void Dispose() + { + this.framework.BeforeUpdate -= this.FrameworkOnBeforeUpdate; + } + + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private bool WarnMultithreadedUsage() + { + if (ThreadSafety.IsMainThread) + return false; + + var n = Environment.TickCount64; + if (this.nextMultithreadedUsageWarnTime < n) + { + this.nextMultithreadedUsageWarnTime = n + 30000; + + Log.Warning( + "{plugin} is accessing {objectTable} outside the main thread. This is deprecated.", + Service.Get().FindCallingPlugin()?.Name ?? "", + nameof(ObjectTable)); + } + + return true; + } + + private void FrameworkOnBeforeUpdate(IFramework unused) + { + for (var i = 0; i < ObjectTableLength; i++) + this.cachedObjectTable[i].Update(this.GetObjectAddressUnsafe(i)); + } + + [MethodImpl(MethodImplOptions.AggressiveInlining)] + private unsafe nint GetObjectAddressUnsafe(int index) => + *(nint*)(this.clientState.AddressResolver.ObjectTable + (8 * index)); + + private struct CachedEntry + { + public GameObject? ActiveObject; + public PlayerCharacter PlayerCharacter; + public BattleNpc BattleNpc; + public Npc Npc; + public EventObj EventObj; + public GameObject GameObject; + + public static CachedEntry CreateNew() => + new() + { + PlayerCharacter = new(nint.Zero), + BattleNpc = new(nint.Zero), + Npc = new(nint.Zero), + EventObj = new(nint.Zero), + GameObject = new(nint.Zero), + }; + + public unsafe void Update(nint address) + { + if (address == nint.Zero) + { + this.ActiveObject = null; + return; + } + + var obj = (FFXIVClientStructs.FFXIV.Client.Game.Object.GameObject*)address; + var objKind = (ObjectKind)obj->ObjectKind; + var activeObject = objKind switch + { + ObjectKind.Player => this.PlayerCharacter, + ObjectKind.BattleNpc => this.BattleNpc, + ObjectKind.EventNpc => this.Npc, + ObjectKind.Retainer => this.Npc, + ObjectKind.EventObj => this.EventObj, + ObjectKind.Companion => this.Npc, + ObjectKind.MountType => this.Npc, + ObjectKind.Ornament => this.Npc, + _ => this.GameObject, + }; + activeObject.Address = address; + this.ActiveObject = activeObject; + } + } } /// @@ -117,17 +229,87 @@ internal sealed partial class ObjectTable /// public IEnumerator GetEnumerator() { - for (var i = 0; i < ObjectTableLength; i++) + if (this.WarnMultithreadedUsage()) { - var obj = this[i]; - - if (obj == null) - continue; - - yield return obj; + // let's not + var e = this.multiThreadedEnumerators.Get(); + e.InitializeForPooledObjects(this); + return e; } + + foreach (ref var x in this.frameworkThreadEnumerators.AsSpan()) + { + if (x is not null) + { + var t = x; + x = null; + t.Reset(); + return t; + } + } + + return new Enumerator(this, -1); } /// IEnumerator IEnumerable.GetEnumerator() => this.GetEnumerator(); + + private sealed class Enumerator : IEnumerator, IResettable + { + private readonly int slotId; + private ObjectTable? owner; + + private int index = -1; + + public Enumerator() => this.slotId = -1; + + public Enumerator(ObjectTable owner, int slotId) + { + this.owner = owner; + this.slotId = slotId; + } + + public GameObject Current { get; private set; } = null!; + + object IEnumerator.Current => this.Current; + + public bool MoveNext() + { + if (this.index == ObjectTableLength) + return false; + + var cache = this.owner!.cachedObjectTable.AsSpan(); + for (this.index++; this.index < ObjectTableLength; this.index++) + { + if (cache[this.index].ActiveObject is { } ao) + { + this.Current = ao; + return true; + } + } + + return false; + } + + public void InitializeForPooledObjects(ObjectTable ot) => this.owner = ot; + + public void Reset() => this.index = -1; + + public void Dispose() + { + if (this.owner is not { } o) + return; + + if (this.index == -1) + o.multiThreadedEnumerators.Return(this); + else + o.frameworkThreadEnumerators[this.slotId] = this; + } + + public bool TryReset() + { + this.Reset(); + return true; + } + } } diff --git a/Dalamud/Game/ClientState/Objects/Types/GameObject.cs b/Dalamud/Game/ClientState/Objects/Types/GameObject.cs index 292430b27..3d5b4c288 100644 --- a/Dalamud/Game/ClientState/Objects/Types/GameObject.cs +++ b/Dalamud/Game/ClientState/Objects/Types/GameObject.cs @@ -29,7 +29,7 @@ public unsafe partial class GameObject : IEquatable /// /// Gets the address of the game object in memory. /// - public IntPtr Address { get; } + public IntPtr Address { get; internal set; } /// /// Gets the Dalamud instance. diff --git a/Dalamud/Game/Framework.cs b/Dalamud/Game/Framework.cs index ce34f2c06..d05177208 100644 --- a/Dalamud/Game/Framework.cs +++ b/Dalamud/Game/Framework.cs @@ -25,9 +25,9 @@ namespace Dalamud.Game; internal sealed class Framework : IDisposable, IServiceType, IFramework { private static readonly ModuleLog Log = new("Framework"); - + private static readonly Stopwatch StatsStopwatch = new(); - + private readonly GameLifecycle lifecycle; private readonly Stopwatch updateStopwatch = new(); @@ -76,6 +76,11 @@ internal sealed class Framework : IDisposable, IServiceType, IFramework /// public event IFramework.OnUpdateDelegate? Update; + /// + /// Executes during FrameworkUpdate before all delegates. + /// + internal event IFramework.OnUpdateDelegate BeforeUpdate; + /// /// Gets or sets a value indicating whether the collection of stats is enabled. /// @@ -280,7 +285,7 @@ internal sealed class Framework : IDisposable, IServiceType, IFramework this.updateStopwatch.Reset(); StatsStopwatch.Reset(); } - + /// /// Adds a update time to the stats history. /// @@ -307,7 +312,7 @@ internal sealed class Framework : IDisposable, IServiceType, IFramework internal void ProfileAndInvoke(IFramework.OnUpdateDelegate? eventDelegate, IFramework frameworkInstance) { if (eventDelegate is null) return; - + var invokeList = eventDelegate.GetInvocationList(); // Individually invoke OnUpdate handlers and time them. @@ -353,6 +358,8 @@ internal sealed class Framework : IDisposable, IServiceType, IFramework ThreadSafety.MarkMainThread(); + this.BeforeUpdate?.InvokeSafely(this); + this.hitchDetector.Start(); try @@ -425,7 +432,7 @@ internal sealed class Framework : IDisposable, IServiceType, IFramework this.hitchDetector.Stop(); - original: + original: return this.updateHook.OriginalDisposeSafe(framework); } @@ -558,19 +565,19 @@ internal class FrameworkPluginScoped : IDisposable, IServiceType, IFramework /// public DateTime LastUpdate => this.frameworkService.LastUpdate; - + /// public DateTime LastUpdateUTC => this.frameworkService.LastUpdateUTC; - + /// public TimeSpan UpdateDelta => this.frameworkService.UpdateDelta; - + /// public bool IsInFrameworkUpdateThread => this.frameworkService.IsInFrameworkUpdateThread; - + /// public bool IsFrameworkUnloading => this.frameworkService.IsFrameworkUnloading; - + /// public void Dispose() { @@ -582,27 +589,27 @@ internal class FrameworkPluginScoped : IDisposable, IServiceType, IFramework /// public Task RunOnFrameworkThread(Func func) => this.frameworkService.RunOnFrameworkThread(func); - + /// public Task RunOnFrameworkThread(Action action) => this.frameworkService.RunOnFrameworkThread(action); - + /// public Task RunOnFrameworkThread(Func> func) => this.frameworkService.RunOnFrameworkThread(func); - + /// public Task RunOnFrameworkThread(Func func) => this.frameworkService.RunOnFrameworkThread(func); - + /// public Task RunOnTick(Func func, TimeSpan delay = default, int delayTicks = default, CancellationToken cancellationToken = default) => this.frameworkService.RunOnTick(func, delay, delayTicks, cancellationToken); - + /// public Task RunOnTick(Action action, TimeSpan delay = default, int delayTicks = default, CancellationToken cancellationToken = default) => this.frameworkService.RunOnTick(action, delay, delayTicks, cancellationToken); - + /// public Task RunOnTick(Func> func, TimeSpan delay = default, int delayTicks = default, CancellationToken cancellationToken = default) => this.frameworkService.RunOnTick(func, delay, delayTicks, cancellationToken); From 66a04cb45dbbfed17ec4cde76a29829b562452ad Mon Sep 17 00:00:00 2001 From: karashiiro <49822414+karashiiro@users.noreply.github.com> Date: Wed, 13 Mar 2024 18:23:27 -0700 Subject: [PATCH 10/17] Fix edge case in GameVersion and refactor - Test coverage has been added for the entire class, and verbose/redundant code has been refactored - Fixes JSON serialization: JsonConstructor requires that the ctor parameters match fields/properties of the target class. Previously, this meant that the JSON constructor would always throw an ArgumentNullException, as `Input` was not a class property. --- Dalamud.Common/Game/GameVersion.cs | 134 +++++----------- Dalamud.Test/Dalamud.Test.csproj | 2 +- Dalamud.Test/Game/GameVersionTests.cs | 212 +++++++++++++++++++++++++- 3 files changed, 250 insertions(+), 98 deletions(-) diff --git a/Dalamud.Common/Game/GameVersion.cs b/Dalamud.Common/Game/GameVersion.cs index 26ff0e48f..2112a43ea 100644 --- a/Dalamud.Common/Game/GameVersion.cs +++ b/Dalamud.Common/Game/GameVersion.cs @@ -23,7 +23,6 @@ public sealed class GameVersion : ICloneable, IComparable, IComparable class. /// /// Version string to parse. - [JsonConstructor] public GameVersion(string version) { var ver = Parse(version); @@ -42,20 +41,9 @@ public sealed class GameVersion : ICloneable, IComparable, IComparableThe day. /// The major version. /// The minor version. - public GameVersion(int year, int month, int day, int major, int minor) + [JsonConstructor] + public GameVersion(int year, int month, int day, int major, int minor) : this(year, month, day, major) { - if ((this.Year = year) < 0) - throw new ArgumentOutOfRangeException(nameof(year)); - - if ((this.Month = month) < 0) - throw new ArgumentOutOfRangeException(nameof(month)); - - if ((this.Day = day) < 0) - throw new ArgumentOutOfRangeException(nameof(day)); - - if ((this.Major = major) < 0) - throw new ArgumentOutOfRangeException(nameof(major)); - if ((this.Minor = minor) < 0) throw new ArgumentOutOfRangeException(nameof(minor)); } @@ -67,17 +55,8 @@ public sealed class GameVersion : ICloneable, IComparable, IComparableThe month. /// The day. /// The major version. - public GameVersion(int year, int month, int day, int major) + public GameVersion(int year, int month, int day, int major) : this(year, month, day) { - if ((this.Year = year) < 0) - throw new ArgumentOutOfRangeException(nameof(year)); - - if ((this.Month = month) < 0) - throw new ArgumentOutOfRangeException(nameof(month)); - - if ((this.Day = day) < 0) - throw new ArgumentOutOfRangeException(nameof(day)); - if ((this.Major = major) < 0) throw new ArgumentOutOfRangeException(nameof(major)); } @@ -88,14 +67,8 @@ public sealed class GameVersion : ICloneable, IComparable, IComparableThe year. /// The month. /// The day. - public GameVersion(int year, int month, int day) + public GameVersion(int year, int month, int day) : this(year, month) { - if ((this.Year = year) < 0) - throw new ArgumentOutOfRangeException(nameof(year)); - - if ((this.Month = month) < 0) - throw new ArgumentOutOfRangeException(nameof(month)); - if ((this.Day = day) < 0) throw new ArgumentOutOfRangeException(nameof(day)); } @@ -105,11 +78,8 @@ public sealed class GameVersion : ICloneable, IComparable, IComparable /// The year. /// The month. - public GameVersion(int year, int month) + public GameVersion(int year, int month) : this(year) { - if ((this.Year = year) < 0) - throw new ArgumentOutOfRangeException(nameof(year)); - if ((this.Month = month) < 0) throw new ArgumentOutOfRangeException(nameof(month)); } @@ -183,17 +153,13 @@ public sealed class GameVersion : ICloneable, IComparable, IComparableGameVersion object. public static GameVersion Parse(string input) { - if (input == null) - throw new ArgumentNullException(nameof(input)); + ArgumentNullException.ThrowIfNull(input); if (input.ToLower(CultureInfo.InvariantCulture) == "any") - return new GameVersion(); + return Any; var parts = input.Split('.'); - var tplParts = parts.Select(p => - { - var result = int.TryParse(p, out var value); - return (result, value); - }).ToArray(); + var tplParts = parts.Select( + p => + { + var result = int.TryParse(p, out var value); + return (result, value); + }).ToArray(); if (tplParts.Any(t => !t.result)) throw new FormatException("Bad formatting"); @@ -259,18 +223,15 @@ public sealed class GameVersion : ICloneable, IComparable, IComparable t.value).ToArray(); var len = intParts.Length; - if (len == 1) - return new GameVersion(intParts[0]); - else if (len == 2) - return new GameVersion(intParts[0], intParts[1]); - else if (len == 3) - return new GameVersion(intParts[0], intParts[1], intParts[2]); - else if (len == 4) - return new GameVersion(intParts[0], intParts[1], intParts[2], intParts[3]); - else if (len == 5) - return new GameVersion(intParts[0], intParts[1], intParts[2], intParts[3], intParts[4]); - else - throw new ArgumentException("Too many parts"); + return len switch + { + 1 => new GameVersion(intParts[0]), + 2 => new GameVersion(intParts[0], intParts[1]), + 3 => new GameVersion(intParts[0], intParts[1], intParts[2]), + 4 => new GameVersion(intParts[0], intParts[1], intParts[2], intParts[3]), + 5 => new GameVersion(intParts[0], intParts[1], intParts[2], intParts[3], intParts[4]), + _ => throw new ArgumentException("Too many parts"), + }; } /// @@ -299,17 +260,12 @@ public sealed class GameVersion : ICloneable, IComparable, IComparable public int CompareTo(object? obj) { - if (obj == null) - return 1; - - if (obj is GameVersion value) + return obj switch { - return this.CompareTo(value); - } - else - { - throw new ArgumentException("Argument must be a GameVersion"); - } + null => 1, + GameVersion value => this.CompareTo(value), + _ => throw new ArgumentException("Argument must be a GameVersion", nameof(obj)), + }; } /// @@ -342,16 +298,14 @@ public sealed class GameVersion : ICloneable, IComparable, IComparable value.Minor ? 1 : -1; + // This should never happen return 0; } /// public override bool Equals(object? obj) { - if (obj is not GameVersion value) - return false; - - return this.Equals(value); + return obj is GameVersion value && this.Equals(value); } /// @@ -373,16 +327,8 @@ public sealed class GameVersion : ICloneable, IComparable, IComparable public override int GetHashCode() { - var accumulator = 0; - - // This might be horribly wrong, but it isn't used heavily. - accumulator |= this.Year.GetHashCode(); - accumulator |= this.Month.GetHashCode(); - accumulator |= this.Day.GetHashCode(); - accumulator |= this.Major.GetHashCode(); - accumulator |= this.Minor.GetHashCode(); - - return accumulator; + // https://learn.microsoft.com/en-us/dotnet/api/system.object.gethashcode?view=net-8.0#notes-to-inheritors + return HashCode.Combine(this.Year, this.Month, this.Day, this.Major, this.Minor); } /// @@ -396,11 +342,11 @@ public sealed class GameVersion : ICloneable, IComparable, IComparablewin-x64 x64 x64;AnyCPU - 9.0 + 11.0 diff --git a/Dalamud.Test/Game/GameVersionTests.cs b/Dalamud.Test/Game/GameVersionTests.cs index dcace4279..b45d55c4b 100644 --- a/Dalamud.Test/Game/GameVersionTests.cs +++ b/Dalamud.Test/Game/GameVersionTests.cs @@ -1,10 +1,71 @@ +using System; + using Dalamud.Common.Game; + +using Newtonsoft.Json; + using Xunit; namespace Dalamud.Test.Game { public class GameVersionTests { + [Fact] + public void VersionComparisons() + { + var v1 = GameVersion.Parse("2021.01.01.0000.0000"); + var v2 = GameVersion.Parse("2021.01.01.0000.0000"); + Assert.True(v1 == v2); + Assert.False(v1 != v2); + Assert.False(v1 < v2); + Assert.True(v1 <= v2); + Assert.False(v1 > v2); + Assert.True(v1 >= v2); + } + + [Fact] + public void VersionAddition() + { + var v1 = GameVersion.Parse("2021.01.01.0000.0000"); + var v2 = GameVersion.Parse("2021.01.05.0000.0000"); + Assert.Equal(v2, v1 + TimeSpan.FromDays(4)); + } + + [Fact] + public void VersionAdditionAny() + { + Assert.Equal(GameVersion.Any, GameVersion.Any + TimeSpan.FromDays(4)); + } + + [Fact] + public void VersionSubtraction() + { + var v1 = GameVersion.Parse("2021.01.05.0000.0000"); + var v2 = GameVersion.Parse("2021.01.01.0000.0000"); + Assert.Equal(v2, v1 - TimeSpan.FromDays(4)); + } + + [Fact] + public void VersionSubtractionAny() + { + Assert.Equal(GameVersion.Any, GameVersion.Any - TimeSpan.FromDays(4)); + } + + [Fact] + public void VersionClone() + { + var v1 = GameVersion.Parse("2021.01.01.0000.0000"); + var v2 = v1.Clone(); + Assert.NotSame(v1, v2); + } + + [Fact] + public void VersionCast() + { + var v = GameVersion.Parse("2021.01.01.0000.0000"); + Assert.Equal("2021.01.01.0000.0000", v); + } + [Theory] [InlineData("any", "any")] [InlineData("2021.01.01.0000.0000", "2021.01.01.0000.0000")] @@ -14,6 +75,18 @@ namespace Dalamud.Test.Game var v2 = GameVersion.Parse(ver2); Assert.Equal(v1, v2); + Assert.Equal(0, v1.CompareTo(v2)); + Assert.Equal(v1.GetHashCode(), v2.GetHashCode()); + } + + [Fact] + public void VersionNullEquality() + { + // Tests `Equals(GameVersion? value)` + Assert.False(GameVersion.Parse("2021.01.01.0000.0000").Equals(null)); + + // Tests `Equals(object? value)` + Assert.False(GameVersion.Parse("2021.01.01.0000.0000").Equals((object)null)); } [Theory] @@ -31,6 +104,67 @@ namespace Dalamud.Test.Game Assert.True(v1.CompareTo(v2) < 0); } + [Theory] + [InlineData("any", "2020.06.15.0000.0000")] + public void VersionComparisonInverse(string ver1, string ver2) + { + var v1 = GameVersion.Parse(ver1); + var v2 = GameVersion.Parse(ver2); + + Assert.True(v1.CompareTo(v2) > 0); + } + + [Fact] + public void VersionComparisonNull() + { + var v = GameVersion.Parse("2020.06.15.0000.0000"); + + // Tests `CompareTo(GameVersion? value)` + Assert.True(v.CompareTo(null) > 0); + + // Tests `CompareTo(object? value)` + Assert.True(v.CompareTo((object)null) > 0); + } + + [Fact] + public void VersionComparisonBoxed() + { + var v1 = GameVersion.Parse("2020.06.15.0000.0000"); + var v2 = GameVersion.Parse("2020.06.15.0000.0000"); + Assert.Equal(0, v1.CompareTo((object)v2)); + } + + [Fact] + public void VersionComparisonBoxedInvalid() + { + var v = GameVersion.Parse("2020.06.15.0000.0000"); + Assert.Throws(() => v.CompareTo(42)); + } + + [Theory] + [InlineData("2020.06.15.0000.0000")] + [InlineData("2021.01.01.0000")] + [InlineData("2021.01.01")] + [InlineData("2021.01")] + [InlineData("2021")] + public void VersionParse(string ver) + { + var v = GameVersion.Parse(ver); + Assert.NotNull(v); + } + + [Theory] + [InlineData("2020.06.15.0000.0000")] + [InlineData("2021.01.01.0000")] + [InlineData("2021.01.01")] + [InlineData("2021.01")] + [InlineData("2021")] + public void VersionTryParse(string ver) + { + Assert.True(GameVersion.TryParse(ver, out var v)); + Assert.NotNull(v); + } + [Theory] [InlineData("2020.06.15.0000.0000")] [InlineData("2021.01.01.0000")] @@ -39,9 +173,8 @@ namespace Dalamud.Test.Game [InlineData("2021")] public void VersionConstructor(string ver) { - var v = GameVersion.Parse(ver); - - Assert.True(v != null); + var v = new GameVersion(ver); + Assert.NotNull(v); } [Theory] @@ -54,5 +187,78 @@ namespace Dalamud.Test.Game Assert.False(result); Assert.Null(v); } + + [Theory] + [InlineData("any", "any")] + [InlineData("2020.06.15.0000.0000", "2020.06.15.0000.0000")] + [InlineData("2021.01.01.0000", "2021.01.01.0000.0000")] + [InlineData("2021.01.01", "2021.01.01.0000.0000")] + [InlineData("2021.01", "2021.01.00.0000.0000")] + [InlineData("2021", "2021.00.00.0000.0000")] + public void VersionToString(string ver1, string ver2) + { + var v1 = GameVersion.Parse(ver1); + Assert.Equal(ver2, v1.ToString()); + } + + [Fact] + public void VersionIsSerializationSafe() + { + var v = GameVersion.Parse("2020.06.15.0000.0000"); + var serialized = JsonConvert.SerializeObject(v); + var deserialized = JsonConvert.DeserializeObject(serialized); + Assert.Equal(v, deserialized); + } + + [Fact] + public void VersionInvalidDeserialization() + { + var serialized = """ + { + "Year": -1, + "Month": -1, + "Day": -1, + "Major": -1, + "Minor": -1, + } + """; + Assert.Throws(() => JsonConvert.DeserializeObject(serialized)); + } + + [Fact] + public void VersionConstructorNegativeYear() + { + Assert.Throws(() => new GameVersion(-2024)); + } + + [Fact] + public void VersionConstructorNegativeMonth() + { + Assert.Throws(() => new GameVersion(2024, -3)); + } + + [Fact] + public void VersionConstructorNegativeDay() + { + Assert.Throws(() => new GameVersion(2024, 3, -13)); + } + + [Fact] + public void VersionConstructorNegativeMajor() + { + Assert.Throws(() => new GameVersion(2024, 3, 13, -1)); + } + + [Fact] + public void VersionConstructorNegativeMinor() + { + Assert.Throws(() => new GameVersion(2024, 3, 13, 0, -1)); + } + + [Fact] + public void VersionParseNull() + { + Assert.Throws(() => GameVersion.Parse(null!)); + } } } From 2721e2df167fe344916b4973199e766083caf04a Mon Sep 17 00:00:00 2001 From: rootdarkarchon Date: Thu, 14 Mar 2024 05:41:02 +0100 Subject: [PATCH 11/17] Resolve stale pointer issues (#1711) * Resolve stale pointer issues * remove unncessary FrameworkOnBeforeUpdate --------- Co-authored-by: rootdarkarchon --- .../Game/ClientState/Objects/ObjectTable.cs | 26 ++++++------------- 1 file changed, 8 insertions(+), 18 deletions(-) diff --git a/Dalamud/Game/ClientState/Objects/ObjectTable.cs b/Dalamud/Game/ClientState/Objects/ObjectTable.cs index b643abedb..16bf5432f 100644 --- a/Dalamud/Game/ClientState/Objects/ObjectTable.cs +++ b/Dalamud/Game/ClientState/Objects/ObjectTable.cs @@ -26,12 +26,11 @@ namespace Dalamud.Game.ClientState.Objects; #pragma warning disable SA1015 [ResolveVia] #pragma warning restore SA1015 -internal sealed partial class ObjectTable : IServiceType, IObjectTable, IDisposable +internal sealed partial class ObjectTable : IServiceType, IObjectTable { private const int ObjectTableLength = 599; private readonly ClientState clientState; - private readonly Framework framework; private readonly CachedEntry[] cachedObjectTable = new CachedEntry[ObjectTableLength]; private readonly ObjectPool multiThreadedEnumerators = @@ -42,16 +41,14 @@ internal sealed partial class ObjectTable : IServiceType, IObjectTable, IDisposa private long nextMultithreadedUsageWarnTime; [ServiceManager.ServiceConstructor] - private ObjectTable(ClientState clientState, Framework framework) + private ObjectTable(ClientState clientState) { this.clientState = clientState; - this.framework = framework; foreach (ref var e in this.cachedObjectTable.AsSpan()) e = CachedEntry.CreateNew(); for (var i = 0; i < this.frameworkThreadEnumerators.Length; i++) this.frameworkThreadEnumerators[i] = new(this, i); - framework.BeforeUpdate += this.FrameworkOnBeforeUpdate; Log.Verbose($"Object table address 0x{this.clientState.AddressResolver.ObjectTable.ToInt64():X}"); } @@ -76,7 +73,9 @@ internal sealed partial class ObjectTable : IServiceType, IObjectTable, IDisposa { _ = this.WarnMultithreadedUsage(); - return index is >= ObjectTableLength or < 0 ? null : this.cachedObjectTable[index].ActiveObject; + if (index is >= ObjectTableLength or < 0) return null; + this.cachedObjectTable[index].Update(this.GetObjectAddressUnsafe(index)); + return this.cachedObjectTable[index].ActiveObject; } } @@ -135,12 +134,6 @@ internal sealed partial class ObjectTable : IServiceType, IObjectTable, IDisposa }; } - /// - public void Dispose() - { - this.framework.BeforeUpdate -= this.FrameworkOnBeforeUpdate; - } - [MethodImpl(MethodImplOptions.AggressiveInlining)] private bool WarnMultithreadedUsage() { @@ -161,12 +154,6 @@ internal sealed partial class ObjectTable : IServiceType, IObjectTable, IDisposa return true; } - private void FrameworkOnBeforeUpdate(IFramework unused) - { - for (var i = 0; i < ObjectTableLength; i++) - this.cachedObjectTable[i].Update(this.GetObjectAddressUnsafe(i)); - } - [MethodImpl(MethodImplOptions.AggressiveInlining)] private unsafe nint GetObjectAddressUnsafe(int index) => *(nint*)(this.clientState.AddressResolver.ObjectTable + (8 * index)); @@ -192,6 +179,9 @@ internal sealed partial class ObjectTable : IServiceType, IObjectTable, IDisposa public unsafe void Update(nint address) { + if (this.ActiveObject != null && address == this.ActiveObject.Address) + return; + if (address == nint.Zero) { this.ActiveObject = null; From 0d7a036ff148edd11bbe589a85f60e2d265b55bc Mon Sep 17 00:00:00 2001 From: Kaz Wolfe Date: Wed, 13 Mar 2024 21:47:47 -0700 Subject: [PATCH 12/17] fix: OT caching fix until Kizer can PR a better version --- Dalamud/Game/ClientState/Objects/ObjectTable.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Dalamud/Game/ClientState/Objects/ObjectTable.cs b/Dalamud/Game/ClientState/Objects/ObjectTable.cs index 16bf5432f..d3ca884c0 100644 --- a/Dalamud/Game/ClientState/Objects/ObjectTable.cs +++ b/Dalamud/Game/ClientState/Objects/ObjectTable.cs @@ -271,6 +271,8 @@ internal sealed partial class ObjectTable var cache = this.owner!.cachedObjectTable.AsSpan(); for (this.index++; this.index < ObjectTableLength; this.index++) { + this.owner!.cachedObjectTable[this.index].Update(this.owner!.GetObjectAddressUnsafe(this.index)); + if (cache[this.index].ActiveObject is { } ao) { this.Current = ao; From 710fff118de4ac1ccae9157846ce3e6d39a60b09 Mon Sep 17 00:00:00 2001 From: srkizer Date: Fri, 15 Mar 2024 06:37:52 +0900 Subject: [PATCH 13/17] Remove unnecessary stuff from ObjectTable (#1713) * Remove unnecessary stuff from ObjectTable * Remove unused * fix --- .../Game/ClientState/Objects/ObjectTable.cs | 130 +++++++++--------- Dalamud/Game/Framework.cs | 3 + Dalamud/Plugin/Services/IObjectTable.cs | 11 +- 3 files changed, 77 insertions(+), 67 deletions(-) diff --git a/Dalamud/Game/ClientState/Objects/ObjectTable.cs b/Dalamud/Game/ClientState/Objects/ObjectTable.cs index d3ca884c0..9b42e1024 100644 --- a/Dalamud/Game/ClientState/Objects/ObjectTable.cs +++ b/Dalamud/Game/ClientState/Objects/ObjectTable.cs @@ -15,6 +15,8 @@ using Microsoft.Extensions.ObjectPool; using Serilog; +using CSGameObject = FFXIVClientStructs.FFXIV.Client.Game.Object.GameObject; + namespace Dalamud.Game.ClientState.Objects; /// @@ -36,16 +38,19 @@ internal sealed partial class ObjectTable : IServiceType, IObjectTable private readonly ObjectPool multiThreadedEnumerators = new DefaultObjectPoolProvider().Create(); - private readonly Enumerator?[] frameworkThreadEnumerators = new Enumerator?[64]; + private readonly Enumerator?[] frameworkThreadEnumerators = new Enumerator?[4]; private long nextMultithreadedUsageWarnTime; [ServiceManager.ServiceConstructor] - private ObjectTable(ClientState clientState) + private unsafe ObjectTable(ClientState clientState) { this.clientState = clientState; - foreach (ref var e in this.cachedObjectTable.AsSpan()) - e = CachedEntry.CreateNew(); + + var nativeObjectTableAddress = (CSGameObject**)this.clientState.AddressResolver.ObjectTable; + for (var i = 0; i < this.cachedObjectTable.Length; i++) + this.cachedObjectTable[i] = new(nativeObjectTableAddress, i); + for (var i = 0; i < this.frameworkThreadEnumerators.Length; i++) this.frameworkThreadEnumerators[i] = new(this, i); @@ -73,9 +78,7 @@ internal sealed partial class ObjectTable : IServiceType, IObjectTable { _ = this.WarnMultithreadedUsage(); - if (index is >= ObjectTableLength or < 0) return null; - this.cachedObjectTable[index].Update(this.GetObjectAddressUnsafe(index)); - return this.cachedObjectTable[index].ActiveObject; + return index is >= ObjectTableLength or < 0 ? null : this.cachedObjectTable[index].Update(); } } @@ -87,24 +90,21 @@ internal sealed partial class ObjectTable : IServiceType, IObjectTable if (objectId is GameObject.InvalidGameObjectId or 0) return null; - foreach (var obj in this) + foreach (var e in this.cachedObjectTable) { - if (obj == null) - continue; - - if (obj.ObjectId == objectId) - return obj; + if (e.Update() is { } o && o.ObjectId == objectId) + return o; } return null; } /// - public nint GetObjectAddress(int index) + public unsafe nint GetObjectAddress(int index) { _ = this.WarnMultithreadedUsage(); - return index is < 0 or >= ObjectTableLength ? nint.Zero : this.GetObjectAddressUnsafe(index); + return index is < 0 or >= ObjectTableLength ? nint.Zero : (nint)this.cachedObjectTable[index].Address; } /// @@ -118,7 +118,7 @@ internal sealed partial class ObjectTable : IServiceType, IObjectTable if (address == nint.Zero) return null; - var obj = (FFXIVClientStructs.FFXIV.Client.Game.Object.GameObject*)address; + var obj = (CSGameObject*)address; var objKind = (ObjectKind)obj->ObjectKind; return objKind switch { @@ -134,6 +134,7 @@ internal sealed partial class ObjectTable : IServiceType, IObjectTable }; } + [Api10ToDo("Use ThreadSafety.AssertMainThread() instead of this.")] [MethodImpl(MethodImplOptions.AggressiveInlining)] private bool WarnMultithreadedUsage() { @@ -154,56 +155,58 @@ internal sealed partial class ObjectTable : IServiceType, IObjectTable return true; } - [MethodImpl(MethodImplOptions.AggressiveInlining)] - private unsafe nint GetObjectAddressUnsafe(int index) => - *(nint*)(this.clientState.AddressResolver.ObjectTable + (8 * index)); - - private struct CachedEntry + /// Stores an object table entry, with preallocated concrete types. + internal readonly unsafe struct CachedEntry { - public GameObject? ActiveObject; - public PlayerCharacter PlayerCharacter; - public BattleNpc BattleNpc; - public Npc Npc; - public EventObj EventObj; - public GameObject GameObject; + private readonly CSGameObject** gameObjectPtrPtr; + private readonly PlayerCharacter playerCharacter; + private readonly BattleNpc battleNpc; + private readonly Npc npc; + private readonly EventObj eventObj; + private readonly GameObject gameObject; - public static CachedEntry CreateNew() => - new() - { - PlayerCharacter = new(nint.Zero), - BattleNpc = new(nint.Zero), - Npc = new(nint.Zero), - EventObj = new(nint.Zero), - GameObject = new(nint.Zero), - }; - - public unsafe void Update(nint address) + /// Initializes a new instance of the struct. + /// The object table that this entry should be pointing to. + /// The slot index inside the table. + public CachedEntry(CSGameObject** ownerTable, int slot) { - if (this.ActiveObject != null && address == this.ActiveObject.Address) - return; + this.gameObjectPtrPtr = ownerTable + slot; + this.playerCharacter = new(nint.Zero); + this.battleNpc = new(nint.Zero); + this.npc = new(nint.Zero); + this.eventObj = new(nint.Zero); + this.gameObject = new(nint.Zero); + } - if (address == nint.Zero) - { - this.ActiveObject = null; - return; - } + /// Gets the address of the underlying native object. May be null. + public CSGameObject* Address + { + [MethodImpl(MethodImplOptions.AggressiveInlining)] + get => *this.gameObjectPtrPtr; + } - var obj = (FFXIVClientStructs.FFXIV.Client.Game.Object.GameObject*)address; - var objKind = (ObjectKind)obj->ObjectKind; - var activeObject = objKind switch + /// Updates and gets the wrapped game object pointed by this struct. + /// The pointed object, or null if no object exists at that slot. + public GameObject? Update() + { + var address = this.Address; + if (address is null) + return null; + + var activeObject = (ObjectKind)address->ObjectKind switch { - ObjectKind.Player => this.PlayerCharacter, - ObjectKind.BattleNpc => this.BattleNpc, - ObjectKind.EventNpc => this.Npc, - ObjectKind.Retainer => this.Npc, - ObjectKind.EventObj => this.EventObj, - ObjectKind.Companion => this.Npc, - ObjectKind.MountType => this.Npc, - ObjectKind.Ornament => this.Npc, - _ => this.GameObject, + ObjectKind.Player => this.playerCharacter, + ObjectKind.BattleNpc => this.battleNpc, + ObjectKind.EventNpc => this.npc, + ObjectKind.Retainer => this.npc, + ObjectKind.EventObj => this.eventObj, + ObjectKind.Companion => this.npc, + ObjectKind.MountType => this.npc, + ObjectKind.Ornament => this.npc, + _ => this.gameObject, }; - activeObject.Address = address; - this.ActiveObject = activeObject; + activeObject.Address = (nint)address; + return activeObject; } } } @@ -219,6 +222,7 @@ internal sealed partial class ObjectTable /// public IEnumerator GetEnumerator() { + // If something's trying to enumerate outside the framework thread, we use the ObjectPool. if (this.WarnMultithreadedUsage()) { // let's not @@ -227,6 +231,7 @@ internal sealed partial class ObjectTable return e; } + // If we're on the framework thread, see if there's an already allocated enumerator available for use. foreach (ref var x in this.frameworkThreadEnumerators.AsSpan()) { if (x is not null) @@ -238,6 +243,7 @@ internal sealed partial class ObjectTable } } + // No reusable enumerator is available; allocate a new temporary one. return new Enumerator(this, -1); } @@ -271,9 +277,7 @@ internal sealed partial class ObjectTable var cache = this.owner!.cachedObjectTable.AsSpan(); for (this.index++; this.index < ObjectTableLength; this.index++) { - this.owner!.cachedObjectTable[this.index].Update(this.owner!.GetObjectAddressUnsafe(this.index)); - - if (cache[this.index].ActiveObject is { } ao) + if (cache[this.index].Update() is { } ao) { this.Current = ao; return true; @@ -292,7 +296,7 @@ internal sealed partial class ObjectTable if (this.owner is not { } o) return; - if (this.index == -1) + if (this.slotId == -1) o.multiThreadedEnumerators.Return(this); else o.frameworkThreadEnumerators[this.slotId] = this; diff --git a/Dalamud/Game/Framework.cs b/Dalamud/Game/Framework.cs index 4aaf15bee..606bf03da 100644 --- a/Dalamud/Game/Framework.cs +++ b/Dalamud/Game/Framework.cs @@ -498,6 +498,9 @@ internal class FrameworkPluginScoped : IDisposable, IServiceType, IFramework /// public DateTime LastUpdateUTC => this.frameworkService.LastUpdateUTC; + /// + public TaskFactory FrameworkThreadTaskFactory => this.frameworkService.FrameworkThreadTaskFactory; + /// public TimeSpan UpdateDelta => this.frameworkService.UpdateDelta; diff --git a/Dalamud/Plugin/Services/IObjectTable.cs b/Dalamud/Plugin/Services/IObjectTable.cs index d029045fa..e0f671b3c 100644 --- a/Dalamud/Plugin/Services/IObjectTable.cs +++ b/Dalamud/Plugin/Services/IObjectTable.cs @@ -1,24 +1,27 @@ using System.Collections.Generic; using Dalamud.Game.ClientState.Objects.Types; +using Dalamud.Utility; namespace Dalamud.Plugin.Services; /// /// This collection represents the currently spawned FFXIV game objects. /// +[Api10ToDo( + "Make it an IEnumerable instead. Skipping null objects make IReadOnlyCollection.Count yield incorrect values.")] public interface IObjectTable : IReadOnlyCollection { /// /// Gets the address of the object table. /// public nint Address { get; } - + /// /// Gets the length of the object table. /// public int Length { get; } - + /// /// Get an object at the specified spawn index. /// @@ -32,14 +35,14 @@ public interface IObjectTable : IReadOnlyCollection /// Object ID to find. /// A game object or null. public GameObject? SearchById(ulong objectId); - + /// /// Gets the address of the game object at the specified index of the object table. /// /// The index of the object. /// The memory address of the object. public nint GetObjectAddress(int index); - + /// /// Create a reference to an FFXIV game object. /// From 44e847a5d6cc974b1118b183bcd6c1a3d40ebbf3 Mon Sep 17 00:00:00 2001 From: goat <16760685+goaaats@users.noreply.github.com> Date: Sat, 16 Mar 2024 16:41:44 +0100 Subject: [PATCH 14/17] Dalamud.Plugin.targets TFM => net8.0-windows --- targets/Dalamud.Plugin.targets | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/targets/Dalamud.Plugin.targets b/targets/Dalamud.Plugin.targets index 37c0940d7..513180854 100644 --- a/targets/Dalamud.Plugin.targets +++ b/targets/Dalamud.Plugin.targets @@ -1,7 +1,7 @@ - net7.0-windows + net8.0-windows x64 enable latest From 1e8fb2e3634343ab55649363f6f59e89ecdf9bb4 Mon Sep 17 00:00:00 2001 From: Kara <49822414+karashiiro@users.noreply.github.com> Date: Sat, 16 Mar 2024 08:43:01 -0700 Subject: [PATCH 15/17] Add tests for ReliableFileStorage (#1718) --- .../Storage/ReliableFileStorageTests.cs | 386 ++++++++++++++++++ 1 file changed, 386 insertions(+) create mode 100644 Dalamud.Test/Storage/ReliableFileStorageTests.cs diff --git a/Dalamud.Test/Storage/ReliableFileStorageTests.cs b/Dalamud.Test/Storage/ReliableFileStorageTests.cs new file mode 100644 index 000000000..6cea81aec --- /dev/null +++ b/Dalamud.Test/Storage/ReliableFileStorageTests.cs @@ -0,0 +1,386 @@ +using System; +using System.IO; +using System.Linq; +using System.Threading.Tasks; + +using Dalamud.Storage; + +using Xunit; + +namespace Dalamud.Test.Storage; + +public class ReliableFileStorageTests +{ + private const string DbFileName = "dalamudVfs.db"; + private const string TestFileName = "file.txt"; + private const string TestFileContent1 = "hello from señor dalamundo"; + private const string TestFileContent2 = "rewritten"; + + [Fact] + public async Task IsConcurrencySafe() + { + var dbDir = CreateTempDir(); + using var rfs = new ReliableFileStorage(dbDir); + + var tempFile = Path.Combine(CreateTempDir(), TestFileName); + + // Do reads/writes/deletes on the same file on many threads at once and + // see if anything throws + await Task.WhenAll( + Enumerable.Range(1, 6) + .Select( + i => Parallel.ForEachAsync( + Enumerable.Range(1, 100), + (j, _) => + { + if (i % 2 == 0) + { + // ReSharper disable once AccessToDisposedClosure + rfs.WriteAllText(tempFile, j.ToString()); + } + else if (i % 3 == 0) + { + try + { + // ReSharper disable once AccessToDisposedClosure + rfs.ReadAllText(tempFile); + } + catch (FileNotFoundException) + { + // this is fine + } + } + else + { + File.Delete(tempFile); + } + + return ValueTask.CompletedTask; + }))); + } + + [Fact] + public void Constructor_Dispose_Works() + { + var dbDir = CreateTempDir(); + var dbPath = Path.Combine(dbDir, DbFileName); + using var rfs = new ReliableFileStorage(dbDir); + + Assert.True(File.Exists(dbPath)); + } + + [Fact] + public void Exists_ThrowsIfPathIsEmpty() + { + using var rfs = CreateRfs(); + Assert.Throws(() => rfs.Exists("")); + } + + [Fact] + public void Exists_ThrowsIfPathIsNull() + { + using var rfs = CreateRfs(); + Assert.Throws(() => rfs.Exists(null!)); + } + + [Fact] + public void Exists_WhenFileMissing_ReturnsFalse() + { + var tempFile = Path.Combine(CreateTempDir(), TestFileName); + using var rfs = CreateRfs(); + + Assert.False(rfs.Exists(tempFile)); + } + + [Fact] + public void Exists_WhenFileMissing_WhenDbFailed_ReturnsFalse() + { + var tempFile = Path.Combine(CreateTempDir(), TestFileName); + using var rfs = CreateFailedRfs(); + + Assert.False(rfs.Exists(tempFile)); + } + + [Fact] + public async Task Exists_WhenFileOnDisk_ReturnsTrue() + { + var tempFile = Path.Combine(CreateTempDir(), TestFileName); + await File.WriteAllTextAsync(tempFile, TestFileContent1); + using var rfs = CreateRfs(); + + Assert.True(rfs.Exists(tempFile)); + } + + [Fact] + public void Exists_WhenFileInBackup_ReturnsTrue() + { + var tempFile = Path.Combine(CreateTempDir(), TestFileName); + using var rfs = CreateRfs(); + + rfs.WriteAllText(tempFile, TestFileContent1); + + File.Delete(tempFile); + Assert.True(rfs.Exists(tempFile)); + } + + [Fact] + public void Exists_WhenFileInBackup_WithDifferentContainerId_ReturnsFalse() + { + var tempFile = Path.Combine(CreateTempDir(), TestFileName); + using var rfs = CreateRfs(); + + rfs.WriteAllText(tempFile, TestFileContent1); + + File.Delete(tempFile); + Assert.False(rfs.Exists(tempFile, Guid.NewGuid())); + } + + [Fact] + public void WriteAllText_ThrowsIfPathIsEmpty() + { + using var rfs = CreateRfs(); + Assert.Throws(() => rfs.WriteAllText("", TestFileContent1)); + } + + [Fact] + public void WriteAllText_ThrowsIfPathIsNull() + { + using var rfs = CreateRfs(); + Assert.Throws(() => rfs.WriteAllText(null!, TestFileContent1)); + } + + [Fact] + public async Task WriteAllText_WritesToDbAndDisk() + { + var tempFile = Path.Combine(CreateTempDir(), TestFileName); + using var rfs = CreateRfs(); + + rfs.WriteAllText(tempFile, TestFileContent1); + + Assert.True(File.Exists(tempFile)); + Assert.Equal(TestFileContent1, rfs.ReadAllText(tempFile, forceBackup: true)); + Assert.Equal(TestFileContent1, await File.ReadAllTextAsync(tempFile)); + } + + [Fact] + public void WriteAllText_SeparatesContainers() + { + var tempFile = Path.Combine(CreateTempDir(), TestFileName); + var containerId = Guid.NewGuid(); + + using var rfs = CreateRfs(); + rfs.WriteAllText(tempFile, TestFileContent1); + rfs.WriteAllText(tempFile, TestFileContent2, containerId); + File.Delete(tempFile); + + Assert.Equal(TestFileContent1, rfs.ReadAllText(tempFile, forceBackup: true)); + Assert.Equal(TestFileContent2, rfs.ReadAllText(tempFile, forceBackup: true, containerId)); + } + + [Fact] + public async Task WriteAllText_WhenDbFailed_WritesToDisk() + { + var tempFile = Path.Combine(CreateTempDir(), TestFileName); + using var rfs = CreateFailedRfs(); + + rfs.WriteAllText(tempFile, TestFileContent1); + + Assert.True(File.Exists(tempFile)); + Assert.Equal(TestFileContent1, await File.ReadAllTextAsync(tempFile)); + } + + [Fact] + public async Task WriteAllText_CanUpdateExistingFile() + { + var tempFile = Path.Combine(CreateTempDir(), TestFileName); + using var rfs = CreateRfs(); + + rfs.WriteAllText(tempFile, TestFileContent1); + rfs.WriteAllText(tempFile, TestFileContent2); + + Assert.True(File.Exists(tempFile)); + Assert.Equal(TestFileContent2, rfs.ReadAllText(tempFile, forceBackup: true)); + Assert.Equal(TestFileContent2, await File.ReadAllTextAsync(tempFile)); + } + + [Fact] + public void WriteAllText_SupportsNullContent() + { + var tempFile = Path.Combine(CreateTempDir(), TestFileName); + using var rfs = CreateRfs(); + + rfs.WriteAllText(tempFile, null); + + Assert.True(File.Exists(tempFile)); + Assert.Equal("", rfs.ReadAllText(tempFile)); + } + + [Fact] + public void ReadAllText_ThrowsIfPathIsEmpty() + { + using var rfs = CreateRfs(); + Assert.Throws(() => rfs.ReadAllText("")); + } + + [Fact] + public void ReadAllText_ThrowsIfPathIsNull() + { + using var rfs = CreateRfs(); + Assert.Throws(() => rfs.ReadAllText(null!)); + } + + [Fact] + public async Task ReadAllText_WhenFileOnDisk_ReturnsContent() + { + var tempFile = Path.Combine(CreateTempDir(), TestFileName); + await File.WriteAllTextAsync(tempFile, TestFileContent1); + using var rfs = CreateRfs(); + + Assert.Equal(TestFileContent1, rfs.ReadAllText(tempFile)); + } + + [Fact] + public void ReadAllText_WhenFileMissingWithBackup_ReturnsContent() + { + var tempFile = Path.Combine(CreateTempDir(), TestFileName); + using var rfs = CreateRfs(); + + rfs.WriteAllText(tempFile, TestFileContent1); + File.Delete(tempFile); + + Assert.Equal(TestFileContent1, rfs.ReadAllText(tempFile)); + } + + [Fact] + public void ReadAllText_WhenFileMissingWithBackup_ThrowsWithDifferentContainerId() + { + var tempFile = Path.Combine(CreateTempDir(), TestFileName); + var containerId = Guid.NewGuid(); + using var rfs = CreateRfs(); + + rfs.WriteAllText(tempFile, TestFileContent1); + File.Delete(tempFile); + + Assert.Throws(() => rfs.ReadAllText(tempFile, containerId: containerId)); + } + + [Fact] + public void ReadAllText_WhenFileMissing_ThrowsIfDbFailed() + { + var tempFile = Path.Combine(CreateTempDir(), TestFileName); + using var rfs = CreateFailedRfs(); + Assert.Throws(() => rfs.ReadAllText(tempFile)); + } + + [Fact] + public async Task ReadAllText_WithReader_WhenFileOnDisk_ReadsContent() + { + var tempFile = Path.Combine(CreateTempDir(), TestFileName); + await File.WriteAllTextAsync(tempFile, TestFileContent1); + using var rfs = CreateRfs(); + rfs.ReadAllText(tempFile, text => Assert.Equal(TestFileContent1, text)); + } + + [Fact] + public async Task ReadAllText_WithReader_WhenReaderThrows_ThrowsIfBackupMissing() + { + var tempFile = Path.Combine(CreateTempDir(), TestFileName); + await File.WriteAllTextAsync(tempFile, TestFileContent1); + + var readerCalledOnce = false; + + using var rfs = CreateRfs(); + Assert.Throws(() => rfs.ReadAllText(tempFile, Reader)); + + return; + + void Reader(string text) + { + var wasReaderCalledOnce = readerCalledOnce; + readerCalledOnce = true; + if (!wasReaderCalledOnce) throw new Exception(); + } + } + + [Fact] + public void ReadAllText_WithReader_WhenReaderThrows_ReadsContentFromBackup() + { + var tempFile = Path.Combine(CreateTempDir(), TestFileName); + + var readerCalledOnce = false; + var assertionCalled = false; + + using var rfs = CreateRfs(); + rfs.WriteAllText(tempFile, TestFileContent1); + File.Delete(tempFile); + + rfs.ReadAllText(tempFile, Reader); + Assert.True(assertionCalled); + + return; + + void Reader(string text) + { + var wasReaderCalledOnce = readerCalledOnce; + readerCalledOnce = true; + if (!wasReaderCalledOnce) throw new Exception(); + Assert.Equal(TestFileContent1, text); + assertionCalled = true; + } + } + + [Fact] + public async Task ReadAllText_WithReader_RethrowsFileNotFoundException() + { + var tempFile = Path.Combine(CreateTempDir(), TestFileName); + await File.WriteAllTextAsync(tempFile, TestFileContent1); + using var rfs = CreateRfs(); + Assert.Throws(() => rfs.ReadAllText(tempFile, _ => throw new FileNotFoundException())); + } + + [Theory] + [InlineData(true)] + [InlineData(false)] + public void ReadAllText_WhenFileDoesNotExist_Throws(bool forceBackup) + { + var tempFile = Path.Combine(CreateTempDir(), TestFileName); + using var rfs = CreateRfs(); + Assert.Throws(() => rfs.ReadAllText(tempFile, forceBackup)); + } + + private static ReliableFileStorage CreateRfs() + { + var dbDir = CreateTempDir(); + return new ReliableFileStorage(dbDir); + } + + private static ReliableFileStorage CreateFailedRfs() + { + var dbDir = CreateTempDir(); + var dbPath = Path.Combine(dbDir, DbFileName); + + // Create a corrupt DB deliberately, and hold its handle until + // the end of the scope + using var f = File.Open(dbPath, FileMode.CreateNew); + f.Write("broken"u8); + + // Throws an SQLiteException initially, and then throws an + // IOException when attempting to delete the file because + // there's already an active handle associated with it + return new ReliableFileStorage(dbDir); + } + + private static string CreateTempDir() + { + string tempDir; + do + { + // Generate temp directories until we get a new one (usually happens on the first try) + tempDir = Path.Combine(Path.GetTempPath(), Path.GetRandomFileName()); + } + while (File.Exists(tempDir)); + + Directory.CreateDirectory(tempDir); + return tempDir; + } +} From d393fa64b6442f69f787596d6056e3b78560ed4a Mon Sep 17 00:00:00 2001 From: Kara <49822414+karashiiro@users.noreply.github.com> Date: Mon, 18 Mar 2024 20:12:34 -0700 Subject: [PATCH 16/17] Add tests for GameVersionConverter and fix edge case (#1726) - Adds tests for GameVersionConverter - Refactors GameVersionConverter to reduce nesting - Fixes an edge case in GameVersion deserialization in which the JsonConstructor will be invoked even if no properties match - Adds a test for the GameVersion deserialization edge case --- Dalamud.Common/Game/GameVersion.cs | 5 + Dalamud.Common/Game/GameVersionConverter.cs | 39 +++-- .../Game/GameVersionConverterTests.cs | 138 ++++++++++++++++++ Dalamud.Test/Game/GameVersionTests.cs | 11 ++ 4 files changed, 171 insertions(+), 22 deletions(-) create mode 100644 Dalamud.Test/Game/GameVersionConverterTests.cs diff --git a/Dalamud.Common/Game/GameVersion.cs b/Dalamud.Common/Game/GameVersion.cs index 2112a43ea..8bbcf891d 100644 --- a/Dalamud.Common/Game/GameVersion.cs +++ b/Dalamud.Common/Game/GameVersion.cs @@ -109,26 +109,31 @@ public sealed class GameVersion : ICloneable, IComparable, IComparable /// Gets the year component. /// + [JsonRequired] public int Year { get; } = -1; /// /// Gets the month component. /// + [JsonRequired] public int Month { get; } = -1; /// /// Gets the day component. /// + [JsonRequired] public int Day { get; } = -1; /// /// Gets the major version component. /// + [JsonRequired] public int Major { get; } = -1; /// /// Gets the minor version component. /// + [JsonRequired] public int Minor { get; } = -1; public static implicit operator GameVersion(string ver) diff --git a/Dalamud.Common/Game/GameVersionConverter.cs b/Dalamud.Common/Game/GameVersionConverter.cs index a1876869a..2a988b7ef 100644 --- a/Dalamud.Common/Game/GameVersionConverter.cs +++ b/Dalamud.Common/Game/GameVersionConverter.cs @@ -15,17 +15,16 @@ public sealed class GameVersionConverter : JsonConverter /// The calling serializer. public override void WriteJson(JsonWriter writer, object? value, JsonSerializer serializer) { - if (value == null) + switch (value) { - writer.WriteNull(); - } - else if (value is GameVersion) - { - writer.WriteValue(value.ToString()); - } - else - { - throw new JsonSerializationException("Expected GameVersion object value"); + case null: + writer.WriteNull(); + break; + case GameVersion: + writer.WriteValue(value.ToString()); + break; + default: + throw new JsonSerializationException("Expected GameVersion object value"); } } @@ -43,24 +42,20 @@ public sealed class GameVersionConverter : JsonConverter { return null; } - else + + if (reader.TokenType == JsonToken.String) { - if (reader.TokenType == JsonToken.String) + try { - try - { - return new GameVersion((string)reader.Value!); - } - catch (Exception ex) - { - throw new JsonSerializationException($"Error parsing GameVersion string: {reader.Value}", ex); - } + return new GameVersion((string)reader.Value!); } - else + catch (Exception ex) { - throw new JsonSerializationException($"Unexpected token or value when parsing GameVersion. Token: {reader.TokenType}, Value: {reader.Value}"); + throw new JsonSerializationException($"Error parsing GameVersion string: {reader.Value}", ex); } } + + throw new JsonSerializationException($"Unexpected token or value when parsing GameVersion. Token: {reader.TokenType}, Value: {reader.Value}"); } /// diff --git a/Dalamud.Test/Game/GameVersionConverterTests.cs b/Dalamud.Test/Game/GameVersionConverterTests.cs new file mode 100644 index 000000000..ac8c4c17d --- /dev/null +++ b/Dalamud.Test/Game/GameVersionConverterTests.cs @@ -0,0 +1,138 @@ +using Dalamud.Common.Game; + +using JetBrains.Annotations; + +using Newtonsoft.Json; + +using Xunit; + +namespace Dalamud.Test.Game; + +public class GameVersionConverterTests +{ + [Fact] + public void ReadJson_ConvertsFromString() + { + var serialized = """ + { + "Version": "2020.06.15.0000.0000" + } + """; + var deserialized = JsonConvert.DeserializeObject(serialized); + + Assert.NotNull(deserialized); + Assert.Equal(GameVersion.Parse("2020.06.15.0000.0000"), deserialized.Version); + } + + + [Fact] + public void ReadJson_ConvertsFromNull() + { + var serialized = """ + { + "Version": null + } + """; + var deserialized = JsonConvert.DeserializeObject(serialized); + + Assert.NotNull(deserialized); + Assert.Null(deserialized.Version); + } + + [Fact] + public void ReadJson_WhenInvalidType_Throws() + { + var serialized = """ + { + "Version": 2 + } + """; + Assert.Throws( + () => JsonConvert.DeserializeObject(serialized)); + } + + [Fact] + public void ReadJson_WhenInvalidVersion_Throws() + { + var serialized = """ + { + "Version": "junk" + } + """; + Assert.Throws( + () => JsonConvert.DeserializeObject(serialized)); + } + + [Fact] + public void WriteJson_ConvertsToString() + { + var deserialized = new TestSerializationClass + { + Version = GameVersion.Parse("2020.06.15.0000.0000"), + }; + var serialized = JsonConvert.SerializeObject(deserialized); + + Assert.Equal("""{"Version":"2020.06.15.0000.0000"}""", RemoveWhitespace(serialized)); + } + + [Fact] + public void WriteJson_ConvertsToNull() + { + var deserialized = new TestSerializationClass + { + Version = null, + }; + var serialized = JsonConvert.SerializeObject(deserialized); + + Assert.Equal("""{"Version":null}""", RemoveWhitespace(serialized)); + } + + [Fact] + public void WriteJson_WhenInvalidVersion_Throws() + { + var deserialized = new TestWrongTypeSerializationClass + { + Version = 42, + }; + Assert.Throws(() => JsonConvert.SerializeObject(deserialized)); + } + + [Fact] + public void CanConvert_WhenGameVersion_ReturnsTrue() + { + var converter = new GameVersionConverter(); + Assert.True(converter.CanConvert(typeof(GameVersion))); + } + + [Fact] + public void CanConvert_WhenNotGameVersion_ReturnsFalse() + { + var converter = new GameVersionConverter(); + Assert.False(converter.CanConvert(typeof(int))); + } + + [Fact] + public void CanConvert_WhenNull_ReturnsFalse() + { + var converter = new GameVersionConverter(); + Assert.False(converter.CanConvert(null!)); + } + + private static string RemoveWhitespace(string input) + { + return input.Replace(" ", "").Replace("\r", "").Replace("\n", ""); + } + + private class TestSerializationClass + { + [JsonConverter(typeof(GameVersionConverter))] + [CanBeNull] + public GameVersion Version { get; init; } + } + + private class TestWrongTypeSerializationClass + { + [JsonConverter(typeof(GameVersionConverter))] + public int Version { get; init; } + } +} diff --git a/Dalamud.Test/Game/GameVersionTests.cs b/Dalamud.Test/Game/GameVersionTests.cs index b45d55c4b..2a21350b4 100644 --- a/Dalamud.Test/Game/GameVersionTests.cs +++ b/Dalamud.Test/Game/GameVersionTests.cs @@ -225,6 +225,17 @@ namespace Dalamud.Test.Game Assert.Throws(() => JsonConvert.DeserializeObject(serialized)); } + [Fact] + public void VersionInvalidTypeDeserialization() + { + var serialized = """ + { + "Value": "Hello" + } + """; + Assert.Throws(() => JsonConvert.DeserializeObject(serialized)); + } + [Fact] public void VersionConstructorNegativeYear() { From a372476340d822cd90a6abc6ec3ceedc0947e602 Mon Sep 17 00:00:00 2001 From: Kaz Wolfe Date: Tue, 19 Mar 2024 08:20:14 -0700 Subject: [PATCH 17/17] chore: bump clientstructs --- lib/FFXIVClientStructs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/FFXIVClientStructs b/lib/FFXIVClientStructs index 7a1c1dd80..2c885a35e 160000 --- a/lib/FFXIVClientStructs +++ b/lib/FFXIVClientStructs @@ -1 +1 @@ -Subproject commit 7a1c1dd8035d326dfc307412462d3e9aeb7fbb6d +Subproject commit 2c885a35e0edf8ab7a335e3296f06642837afbec