From 5a817db0692e2713f9ae7d573b0c27e15ff22530 Mon Sep 17 00:00:00 2001 From: Ottermandias Date: Mon, 3 Apr 2023 12:14:43 +0200 Subject: [PATCH] Make Individual Collection lookup thread-safe by locking. --- Penumbra/Api/PenumbraApi.cs | 14 +- Penumbra/Api/TempCollectionManager.cs | 2 +- .../Collections/CollectionManager.Active.cs | 2 +- Penumbra/Collections/IndividualCollections.cs | 232 ++++++++++-------- 4 files changed, 134 insertions(+), 116 deletions(-) diff --git a/Penumbra/Api/PenumbraApi.cs b/Penumbra/Api/PenumbraApi.cs index 56251935..af3fec75 100644 --- a/Penumbra/Api/PenumbraApi.cs +++ b/Penumbra/Api/PenumbraApi.cs @@ -459,7 +459,7 @@ public class PenumbraApi : IDisposable, IPenumbraApi if (!id.IsValid) return (false, false, _collectionManager.Default.Name); - if (_collectionManager.Individuals.Individuals.TryGetValue(id, out var collection)) + if (_collectionManager.Individuals.TryGetValue(id, out var collection)) return (true, true, collection.Name); AssociatedCollection(gameObjectIdx, out collection); @@ -474,7 +474,7 @@ public class PenumbraApi : IDisposable, IPenumbraApi if (!id.IsValid) return (PenumbraApiEc.InvalidIdentifier, _collectionManager.Default.Name); - var oldCollection = _collectionManager.Individuals.Individuals.TryGetValue(id, out var c) ? c.Name : string.Empty; + var oldCollection = _collectionManager.Individuals.TryGetValue(id, out var c) ? c.Name : string.Empty; if (collectionName.Length == 0) { @@ -809,8 +809,8 @@ public class PenumbraApi : IDisposable, IPenumbraApi if (!identifier.IsValid) return (PenumbraApiEc.InvalidArgument, string.Empty); - if (!forceOverwriteCharacter && _collectionManager.Individuals.Individuals.ContainsKey(identifier) - || _tempCollections.Collections.Individuals.ContainsKey(identifier)) + if (!forceOverwriteCharacter && _collectionManager.Individuals.ContainsKey(identifier) + || _tempCollections.Collections.ContainsKey(identifier)) return (PenumbraApiEc.CharacterCollectionExists, string.Empty); var name = $"{tag}_{character}"; @@ -855,11 +855,11 @@ public class PenumbraApi : IDisposable, IPenumbraApi if (forceAssignment) { - if (_tempCollections.Collections.Individuals.ContainsKey(identifier) && !_tempCollections.Collections.Delete(identifier)) + if (_tempCollections.Collections.ContainsKey(identifier) && !_tempCollections.Collections.Delete(identifier)) return PenumbraApiEc.AssignmentDeletionFailed; } - else if (_tempCollections.Collections.Individuals.ContainsKey(identifier) - || _collectionManager.Individuals.Individuals.ContainsKey(identifier)) + else if (_tempCollections.Collections.ContainsKey(identifier) + || _collectionManager.Individuals.ContainsKey(identifier)) { return PenumbraApiEc.CharacterCollectionExists; } diff --git a/Penumbra/Api/TempCollectionManager.cs b/Penumbra/Api/TempCollectionManager.cs index 7ed67f53..455751c6 100644 --- a/Penumbra/Api/TempCollectionManager.cs +++ b/Penumbra/Api/TempCollectionManager.cs @@ -114,6 +114,6 @@ public class TempCollectionManager : IDisposable return false; var identifier = Penumbra.Actors.CreatePlayer(byteString, worldId); - return Collections.Individuals.TryGetValue(identifier, out var collection) && RemoveTemporaryCollection(collection.Name); + return Collections.TryGetValue(identifier, out var collection) && RemoveTemporaryCollection(collection.Name); } } diff --git a/Penumbra/Collections/CollectionManager.Active.cs b/Penumbra/Collections/CollectionManager.Active.cs index f685e886..50bbefea 100644 --- a/Penumbra/Collections/CollectionManager.Active.cs +++ b/Penumbra/Collections/CollectionManager.Active.cs @@ -58,7 +58,7 @@ public sealed partial class CollectionManager : ISavable CollectionType.Default => Default, CollectionType.Interface => Interface, CollectionType.Current => Current, - CollectionType.Individual => identifier.IsValid && Individuals.Individuals.TryGetValue(identifier, out var c) ? c : null, + CollectionType.Individual => identifier.IsValid && Individuals.TryGetValue(identifier, out var c) ? c : null, _ => null, }; } diff --git a/Penumbra/Collections/IndividualCollections.cs b/Penumbra/Collections/IndividualCollections.cs index d52fd69f..e5de838a 100644 --- a/Penumbra/Collections/IndividualCollections.cs +++ b/Penumbra/Collections/IndividualCollections.cs @@ -1,5 +1,6 @@ using System; using System.Collections.Generic; +using System.Diagnostics.CodeAnalysis; using System.Linq; using Dalamud.Game.ClientState.Objects.Enums; using OtterGui.Filesystem; @@ -11,18 +12,15 @@ namespace Penumbra.Collections; public sealed partial class IndividualCollections { - private readonly ActorManager _actorManager; - private readonly List< (string DisplayName, IReadOnlyList< ActorIdentifier > Identifiers, ModCollection Collection) > _assignments = new(); - private readonly Dictionary< ActorIdentifier, ModCollection > _individuals = new(); + private readonly ActorManager _actorManager; + private readonly List<(string DisplayName, IReadOnlyList Identifiers, ModCollection Collection)> _assignments = new(); + private readonly Dictionary _individuals = new(); - public IReadOnlyList< (string DisplayName, IReadOnlyList< ActorIdentifier > Identifiers, ModCollection Collection) > Assignments + public IReadOnlyList<(string DisplayName, IReadOnlyList Identifiers, ModCollection Collection)> Assignments => _assignments; - public IReadOnlyDictionary< ActorIdentifier, ModCollection > Individuals - => _individuals; - // TODO - public IndividualCollections( ActorService actorManager ) + public IndividualCollections(ActorService actorManager) => _actorManager = actorManager.AwaitedService; public IndividualCollections(ActorManager actorManager) @@ -35,196 +33,216 @@ public sealed partial class IndividualCollections Invalid, } - public AddResult CanAdd( params ActorIdentifier[] identifiers ) + public bool TryGetValue(ActorIdentifier identifier, [NotNullWhen(true)] out ModCollection? collection) { - if( identifiers.Length == 0 ) + lock (_individuals) { - return AddResult.Invalid; + return _individuals.TryGetValue(identifier, out collection); } - - if( identifiers.Any( i => !i.IsValid ) ) - { - return AddResult.Invalid; - } - - if( identifiers.Any( Individuals.ContainsKey ) ) - { - return AddResult.AlreadySet; - } - - return AddResult.Valid; } - public AddResult CanAdd( IdentifierType type, string name, ushort homeWorld, ObjectKind kind, IEnumerable< uint > dataIds, out ActorIdentifier[] identifiers ) + public bool ContainsKey(ActorIdentifier identifier) { - identifiers = Array.Empty< ActorIdentifier >(); + lock (_individuals) + { + return _individuals.ContainsKey(identifier); + } + } - switch( type ) + public AddResult CanAdd(params ActorIdentifier[] identifiers) + { + if (identifiers.Length == 0) + return AddResult.Invalid; + + if (identifiers.Any(i => !i.IsValid)) + return AddResult.Invalid; + + bool set; + lock (_individuals) + { + set = identifiers.Any(_individuals.ContainsKey); + } + + return set ? AddResult.AlreadySet : AddResult.Valid; + } + + public AddResult CanAdd(IdentifierType type, string name, ushort homeWorld, ObjectKind kind, IEnumerable dataIds, + out ActorIdentifier[] identifiers) + { + identifiers = Array.Empty(); + + switch (type) { case IdentifierType.Player: - if( !ByteString.FromString( name, out var playerName ) ) - { + if (!ByteString.FromString(name, out var playerName)) return AddResult.Invalid; - } - identifiers = new[] { _actorManager.CreatePlayer( playerName, homeWorld ) }; + identifiers = new[] + { + _actorManager.CreatePlayer(playerName, homeWorld), + }; break; case IdentifierType.Retainer: - if( !ByteString.FromString( name, out var retainerName ) ) - { + if (!ByteString.FromString(name, out var retainerName)) return AddResult.Invalid; - } - identifiers = new[] { _actorManager.CreateRetainer( retainerName, 0 ) }; + identifiers = new[] + { + _actorManager.CreateRetainer(retainerName, 0), + }; break; case IdentifierType.Owned: - if( !ByteString.FromString( name, out var ownerName ) ) - { + if (!ByteString.FromString(name, out var ownerName)) return AddResult.Invalid; - } - identifiers = dataIds.Select( id => _actorManager.CreateOwned( ownerName, homeWorld, kind, id ) ).ToArray(); + identifiers = dataIds.Select(id => _actorManager.CreateOwned(ownerName, homeWorld, kind, id)).ToArray(); break; case IdentifierType.Npc: - identifiers = dataIds.Select( id => _actorManager.CreateIndividual( IdentifierType.Npc, ByteString.Empty, ushort.MaxValue, kind, id ) ).ToArray(); + identifiers = dataIds + .Select(id => _actorManager.CreateIndividual(IdentifierType.Npc, ByteString.Empty, ushort.MaxValue, kind, id)).ToArray(); break; default: - identifiers = Array.Empty< ActorIdentifier >(); + identifiers = Array.Empty(); break; } - return CanAdd( identifiers ); + return CanAdd(identifiers); } - public ActorIdentifier[] GetGroup( ActorIdentifier identifier ) + public ActorIdentifier[] GetGroup(ActorIdentifier identifier) { - if( !identifier.IsValid ) - { - return Array.Empty< ActorIdentifier >(); - } + if (!identifier.IsValid) + return Array.Empty(); - static ActorIdentifier[] CreateNpcs( ActorManager manager, ActorIdentifier identifier ) + static ActorIdentifier[] CreateNpcs(ActorManager manager, ActorIdentifier identifier) { - var name = manager.Data.ToName( identifier.Kind, identifier.DataId ); + var name = manager.Data.ToName(identifier.Kind, identifier.DataId); var table = identifier.Kind switch { ObjectKind.BattleNpc => manager.Data.BNpcs, ObjectKind.EventNpc => manager.Data.ENpcs, ObjectKind.Companion => manager.Data.Companions, ObjectKind.MountType => manager.Data.Mounts, - ( ObjectKind )15 => manager.Data.Ornaments, + (ObjectKind)15 => manager.Data.Ornaments, _ => throw new NotImplementedException(), }; - return table.Where( kvp => kvp.Value == name ) - .Select( kvp => manager.CreateIndividualUnchecked( identifier.Type, identifier.PlayerName, identifier.HomeWorld, identifier.Kind, kvp.Key ) ).ToArray(); + return table.Where(kvp => kvp.Value == name) + .Select(kvp => manager.CreateIndividualUnchecked(identifier.Type, identifier.PlayerName, identifier.HomeWorld, identifier.Kind, + kvp.Key)).ToArray(); } return identifier.Type switch { - IdentifierType.Player => new[] { identifier.CreatePermanent() }, - IdentifierType.Special => new[] { identifier }, - IdentifierType.Retainer => new[] { identifier.CreatePermanent() }, - IdentifierType.Owned => CreateNpcs( _actorManager, identifier.CreatePermanent() ), - IdentifierType.Npc => CreateNpcs( _actorManager, identifier ), - _ => Array.Empty< ActorIdentifier >(), + IdentifierType.Player => new[] + { + identifier.CreatePermanent(), + }, + IdentifierType.Special => new[] + { + identifier, + }, + IdentifierType.Retainer => new[] + { + identifier.CreatePermanent(), + }, + IdentifierType.Owned => CreateNpcs(_actorManager, identifier.CreatePermanent()), + IdentifierType.Npc => CreateNpcs(_actorManager, identifier), + _ => Array.Empty(), }; } - internal bool Add( ActorIdentifier[] identifiers, ModCollection collection ) + internal bool Add(ActorIdentifier[] identifiers, ModCollection collection) { - if( identifiers.Length == 0 || !identifiers[ 0 ].IsValid ) - { + if (identifiers.Length == 0 || !identifiers[0].IsValid) return false; - } - var name = DisplayString( identifiers[ 0 ] ); - return Add( name, identifiers, collection ); + var name = DisplayString(identifiers[0]); + return Add(name, identifiers, collection); } - private bool Add( string displayName, ActorIdentifier[] identifiers, ModCollection collection ) + private bool Add(string displayName, ActorIdentifier[] identifiers, ModCollection collection) { - if( CanAdd( identifiers ) != AddResult.Valid - || displayName.Length == 0 - || _assignments.Any( a => a.DisplayName.Equals( displayName, StringComparison.OrdinalIgnoreCase ) ) ) - { + if (CanAdd(identifiers) != AddResult.Valid + || displayName.Length == 0 + || _assignments.Any(a => a.DisplayName.Equals(displayName, StringComparison.OrdinalIgnoreCase))) return false; - } - for( var i = 0; i < identifiers.Length; ++i ) + for (var i = 0; i < identifiers.Length; ++i) { - identifiers[ i ] = identifiers[ i ].CreatePermanent(); - _individuals.Add( identifiers[ i ], collection ); + identifiers[i] = identifiers[i].CreatePermanent(); + lock (_individuals) + { + _individuals.Add(identifiers[i], collection); + } } - _assignments.Add( ( displayName, identifiers, collection ) ); + _assignments.Add((displayName, identifiers, collection)); return true; } - internal bool ChangeCollection( ActorIdentifier identifier, ModCollection newCollection ) - => ChangeCollection( DisplayString( identifier ), newCollection ); + internal bool ChangeCollection(ActorIdentifier identifier, ModCollection newCollection) + => ChangeCollection(DisplayString(identifier), newCollection); - internal bool ChangeCollection( string displayName, ModCollection newCollection ) - => ChangeCollection( _assignments.FindIndex( t => t.DisplayName.Equals( displayName, StringComparison.OrdinalIgnoreCase ) ), newCollection ); + internal bool ChangeCollection(string displayName, ModCollection newCollection) + => ChangeCollection(_assignments.FindIndex(t => t.DisplayName.Equals(displayName, StringComparison.OrdinalIgnoreCase)), newCollection); - internal bool ChangeCollection( int displayIndex, ModCollection newCollection ) + internal bool ChangeCollection(int displayIndex, ModCollection newCollection) { - if( displayIndex < 0 || displayIndex >= _assignments.Count || _assignments[ displayIndex ].Collection == newCollection ) - { + if (displayIndex < 0 || displayIndex >= _assignments.Count || _assignments[displayIndex].Collection == newCollection) return false; - } - _assignments[ displayIndex ] = _assignments[ displayIndex ] with { Collection = newCollection }; - foreach( var identifier in _assignments[ displayIndex ].Identifiers ) + _assignments[displayIndex] = _assignments[displayIndex] with { Collection = newCollection }; + lock (_individuals) { - _individuals[ identifier ] = newCollection; + foreach (var identifier in _assignments[displayIndex].Identifiers) + _individuals[identifier] = newCollection; } return true; } - internal bool Delete( ActorIdentifier identifier ) - => Delete( Index( identifier ) ); + internal bool Delete(ActorIdentifier identifier) + => Delete(Index(identifier)); - internal bool Delete( string displayName ) - => Delete( Index( displayName ) ); + internal bool Delete(string displayName) + => Delete(Index(displayName)); - internal bool Delete( int displayIndex ) + internal bool Delete(int displayIndex) { - if( displayIndex < 0 || displayIndex >= _assignments.Count ) - { + if (displayIndex < 0 || displayIndex >= _assignments.Count) return false; - } - var (name, identifiers, _) = _assignments[ displayIndex ]; - _assignments.RemoveAt( displayIndex ); - foreach( var identifier in identifiers ) + var (name, identifiers, _) = _assignments[displayIndex]; + _assignments.RemoveAt(displayIndex); + lock (_individuals) { - _individuals.Remove( identifier ); + foreach (var identifier in identifiers) + _individuals.Remove(identifier); } return true; } - internal bool Move( int from, int to ) - => _assignments.Move( from, to ); + internal bool Move(int from, int to) + => _assignments.Move(from, to); - internal int Index( string displayName ) - => _assignments.FindIndex( t => t.DisplayName.Equals( displayName, StringComparison.OrdinalIgnoreCase ) ); + internal int Index(string displayName) + => _assignments.FindIndex(t => t.DisplayName.Equals(displayName, StringComparison.OrdinalIgnoreCase)); - internal int Index( ActorIdentifier identifier ) - => identifier.IsValid ? Index( DisplayString( identifier ) ) : -1; + internal int Index(ActorIdentifier identifier) + => identifier.IsValid ? Index(DisplayString(identifier)) : -1; - private string DisplayString( ActorIdentifier identifier ) + private string DisplayString(ActorIdentifier identifier) { return identifier.Type switch { - IdentifierType.Player => $"{identifier.PlayerName} ({_actorManager.Data.ToWorldName( identifier.HomeWorld )})", + IdentifierType.Player => $"{identifier.PlayerName} ({_actorManager.Data.ToWorldName(identifier.HomeWorld)})", IdentifierType.Retainer => $"{identifier.PlayerName} (Retainer)", IdentifierType.Owned => - $"{identifier.PlayerName} ({_actorManager.Data.ToWorldName( identifier.HomeWorld )})'s {_actorManager.Data.ToName( identifier.Kind, identifier.DataId )}", - IdentifierType.Npc => $"{_actorManager.Data.ToName( identifier.Kind, identifier.DataId )} ({identifier.Kind.ToName()})", + $"{identifier.PlayerName} ({_actorManager.Data.ToWorldName(identifier.HomeWorld)})'s {_actorManager.Data.ToName(identifier.Kind, identifier.DataId)}", + IdentifierType.Npc => $"{_actorManager.Data.ToName(identifier.Kind, identifier.DataId)} ({identifier.Kind.ToName()})", _ => string.Empty, }; } -} \ No newline at end of file +}