Fix duplicate key errors when a character enters the world

src/GameLogic/Attributes/ItemAwareAttributeSystem.cs

@@ -20,7 +20,7 @@ public sealed class ItemAwareAttributeSystem : AttributeSystem, IDisposable
     /// <param name="gameConfiguration">The game configuration with global attributes.</param>
     public ItemAwareAttributeSystem(Account account, Character character, GameConfiguration gameConfiguration)
         : base(
-            character.Attributes.Concat(account.Attributes),
+            GetStatAttributes(account, character),
             character.CharacterClass!.BaseAttributeValues.Concat(gameConfiguration.GlobalBaseAttributeValues),
             character.CharacterClass.AttributeCombinations.Concat(gameConfiguration.GlobalAttributeCombinations))
     {
@@ -114,6 +114,23 @@ protected override void OnAttributeRemoved(IAttribute attribute)
         base.OnAttributeRemoved(attribute);
     }
 
+    /// <summary>
+    /// Gets the stat attributes of the character and its account, without duplicates.
+    /// The attribute system holds exactly one attribute per <see cref="AttributeDefinition"/>, so a
+    /// duplicated definition would fail the construction of the whole system - and with it the login
+    /// of the character. Duplicates can exist in the stored data, e.g. when a data update added a stat
+    /// attribute to a character class which already had it, and every character of that class then got
+    /// the attribute twice. The character's own attribute wins over the one of its account.
+    /// </summary>
+    /// <param name="account">The account.</param>
+    /// <param name="character">The character.</param>
+    /// <returns>The distinct stat attributes of the character and its account.</returns>
+    private static IEnumerable<IAttribute> GetStatAttributes(Account account, Character character)
+    {
+        return character.Attributes.Concat(account.Attributes)
+            .DistinctBy(attribute => attribute.Definition);
+    }
+
     /// <summary>
     /// Called when an attribute value changed. Forwards the event to <see cref="AttributeValueChanged"/>.
     /// </summary>

src/GameLogic/Bots/BotGenerator.cs

@@ -392,7 +392,11 @@ private void CreateCharacter(IPlayerContext context, Account account, string nam
         character.CreateDate = DateTime.UtcNow;
         character.KeyConfiguration = CreateDefaultKeyConfiguration();
 
-        foreach (var attribute in characterClass.StatAttributes.Select(a => context.CreateNew<StatAttribute>(a.Attribute, a.BaseValue)))
+        // Distinct, because a character class may define the same stat attribute more than once (data
+        // which got duplicated by an update); a character must never hold an attribute twice.
+        foreach (var attribute in characterClass.StatAttributes
+                     .DistinctBy(a => a.Attribute)
+                     .Select(a => context.CreateNew<StatAttribute>(a.Attribute, a.BaseValue)))
         {
             character.Attributes.Add(attribute);
         }

src/GameLogic/Player.cs

@@ -1651,12 +1651,46 @@ private void AddMissingStatAttributes()
             throw new InvalidOperationException($"The character {this.SelectedCharacter} has no assigned character class.");
         }
 
-        var missingStats = characterClass.StatAttributes.Where(a => this.SelectedCharacter.Attributes.All(c => c.Definition != a.Attribute));
+        this.RemoveDuplicateStatAttributes(character);
+
+        // The character class itself may define a stat attribute more than once (a data update which
+        // added an attribute the class already had), so the missing ones are taken distinctly - otherwise
+        // we would create the duplicates we just removed all over again.
+        var missingStats = characterClass.StatAttributes
+            .DistinctBy(a => a.Attribute)
+            .Where(a => character.Attributes.All(c => c.Definition != a.Attribute));
 
         var attributes = missingStats.Select(a => this.PersistenceContext.CreateNew<StatAttribute>(a.Attribute, a.BaseValue)).ToList();
         attributes.ForEach(character.Attributes.Add);
     }
 
+    /// <summary>
+    /// Removes stat attributes which are assigned to the character more than once, keeping the one with
+    /// the highest value. An attribute system holds exactly one attribute per definition, so a duplicate
+    /// would make the character unable to enter the game at all.
+    /// </summary>
+    /// <param name="character">The character.</param>
+    private void RemoveDuplicateStatAttributes(Character character)
+    {
+        var duplicateGroups = character.Attributes
+            .GroupBy(a => a.Definition)
+            .Where(group => group.Count() > 1)
+            .ToList();
+
+        foreach (var duplicates in duplicateGroups)
+        {
+            // The highest value is kept, so a character never loses points that were invested into a stat.
+            var obsolete = duplicates.OrderByDescending(a => a.Value).Skip(1).ToList();
+            obsolete.ForEach(attribute => character.Attributes.Remove(attribute));
+
+            this.Logger.LogWarning(
+                "Removed {Count} duplicate stat attribute(s) '{Attribute}' of character '{Character}'.",
+                obsolete.Count,
+                duplicates.Key,
+                character.Name);
+        }
+    }
+
     private async ValueTask OnPlayerEnteredWorldAsync()
     {
         if (this.SelectedCharacter is not { } selectedCharacter)

src/GameLogic/PlayerActions/Character/CreateCharacterAction.cs

@@ -103,7 +103,13 @@ private static byte[] CreateDefaultKeyConfiguration()
         character.CharacterSlot = freeSlot.Value;
         character.CreateDate = DateTime.UtcNow;
         character.KeyConfiguration = CreateDefaultKeyConfiguration();
-        var attributes = character.CharacterClass.StatAttributes.Select(a => player.PersistenceContext.CreateNew<StatAttribute>(a.Attribute, a.BaseValue)).ToList();
+
+        // Distinct, because a character class may define the same stat attribute more than once (data
+        // which got duplicated by an update); a character must never hold an attribute twice.
+        var attributes = character.CharacterClass.StatAttributes
+            .DistinctBy(a => a.Attribute)
+            .Select(a => player.PersistenceContext.CreateNew<StatAttribute>(a.Attribute, a.BaseValue))
+            .ToList();
         attributes.ForEach(character.Attributes.Add);
         character.CurrentMap = characterClass.HomeMap;
         var randomSpawnGate = character.CurrentMap!.ExitGates.Where(g => g.IsSpawnGate).SelectRandom();

src/GameLogic/SkillList.cs

@@ -5,6 +5,7 @@
 namespace MUnique.OpenMU.GameLogic;
 
 using System.ComponentModel;
+using Microsoft.Extensions.Logging;
 using MUnique.OpenMU.AttributeSystem;
 using MUnique.OpenMU.GameLogic.Attributes;
 using MUnique.OpenMU.GameLogic.Views.Character;
@@ -61,7 +62,7 @@ public SkillList(Player player)
         this._learnedSkills = this._player.SelectedCharacter.LearnedSkills ?? new List<SkillEntry>();
         this._learnedSkills.Where(entry => entry.Skill is null).ForEach(entry => throw Error.NotInitializedProperty(entry, nameof(entry.Skill)));
 
-        this._availableSkills = this._learnedSkills.ToDictionary(skillEntry => skillEntry.Skill!.Number.ToUnsigned());
+        this._availableSkills = this.CreateAvailableSkills();
         this._itemSkills = new List<SkillEntry>();
         this._player.Inventory.EquippedItems
             .Where(item => item.HasSkill)
@@ -114,17 +115,23 @@ public async ValueTask AddLearnedSkillAsync(Skill skill)
     /// <inheritdoc/>
     public async ValueTask<bool> RemoveItemSkillAsync(ushort skillId)
     {
-        this._availableSkills.TryGetValue(skillId, out var skillEntry);
+        // The entry is looked up in the item skills, not in the available skills: when the same skill
+        // is also learned by the character, the available skills hold the learned entry - removing that
+        // one would take a learned skill away just because an item was taken off.
+        var skillEntry = this._itemSkills.FirstOrDefault(s => s.Skill!.Number.ToUnsigned() == skillId);
         if (skillEntry is null)
         {
             return false;
         }
 
-        // We need to take into account that we there might be multiple items equipped with the same skill
-        var skillRemoved = this._itemSkills.Remove(skillEntry);
-        if (skillRemoved && this._itemSkills.All(s => s.Skill!.Number != skillId))
+        this._itemSkills.Remove(skillEntry);
+
+        // We need to take into account that there might be multiple items equipped with the same skill
+        if (this._itemSkills.All(s => s.Skill!.Number.ToUnsigned() != skillId)
+            && this._availableSkills.TryGetValue(skillId, out var availableSkill)
+            && !this._learnedSkills.Contains(availableSkill))
         {
-            await this._player.InvokeViewPlugInAsync<ISkillListViewPlugIn>(p => p.RemoveSkillAsync(skillEntry.Skill!)).ConfigureAwait(false);
+            await this._player.InvokeViewPlugInAsync<ISkillListViewPlugIn>(p => p.RemoveSkillAsync(availableSkill.Skill!)).ConfigureAwait(false);
             this._availableSkills.Remove(skillId);
         }
 
@@ -137,6 +144,41 @@ public bool ContainsSkill(ushort skillId)
         return this._availableSkills.ContainsKey(skillId);
     }
 
+    /// <summary>
+    /// Creates the dictionary of the available skills from the learned skills of the character.
+    /// A character may have the same skill in its stored skill list more than once - it's data which
+    /// shouldn't exist, but it must never keep the character from entering the game. Such duplicates
+    /// are removed from the character, keeping the entry with the highest level.
+    /// </summary>
+    /// <returns>The dictionary of the available skills.</returns>
+    private Dictionary<ushort, SkillEntry> CreateAvailableSkills()
+    {
+        var availableSkills = new Dictionary<ushort, SkillEntry>();
+        foreach (var skillEntry in this._learnedSkills.ToList())
+        {
+            var skillId = skillEntry.Skill!.Number.ToUnsigned();
+            if (availableSkills.TryAdd(skillId, skillEntry))
+            {
+                continue;
+            }
+
+            var previousEntry = availableSkills[skillId];
+            var (keptEntry, obsoleteEntry) = skillEntry.Level > previousEntry.Level
+                ? (skillEntry, previousEntry)
+                : (previousEntry, skillEntry);
+            availableSkills[skillId] = keptEntry;
+            this._learnedSkills.Remove(obsoleteEntry);
+
+            this._player.Logger.LogWarning(
+                "Removed a duplicate learned skill '{Skill}' (number {Number}) of character '{Character}'.",
+                skillEntry.Skill.Name,
+                skillEntry.Skill.Number,
+                this._player.SelectedCharacter?.Name);
+        }
+
+        return availableSkills;
+    }
+
     private async ValueTask AddItemSkillAsync(Skill skill)
     {
         // If character is not selected (e.g., during disconnect cleanup), skill list doesn't need to be updated
@@ -167,7 +209,10 @@ private async ValueTask AddItemSkillAsync(Skill skill)
 
     private async ValueTask AddLearnedSkillAsync(SkillEntry skill)
     {
-        this._availableSkills.Add(skill.Skill!.Number.ToUnsigned(), skill);
+        // A learned skill replaces the entry of an equipped item which grants the same skill: an item
+        // skill is always level 0, while the learned one keeps its own level. The item skill entry stays
+        // in the item skill list, so unequipping the item doesn't take the learned skill away.
+        this._availableSkills[skill.Skill!.Number.ToUnsigned()] = skill;
         this._learnedSkills.Add(skill);
 
         if (skill.Skill.SkillType == SkillType.PassiveBoost || this._castedSkillsWithPassiveBoost.Contains(skill.Skill.Number))

src/Persistence/Initialization/PlugIns/CharacterCreated/AddInitialSkillPlugInBase.cs

@@ -52,6 +52,16 @@ public void CharacterCreated(Player player, Character createdCharacter)
             return;
         }
 
+        if (createdCharacter.LearnedSkills.Any(entry => entry.Skill?.Number == skillDefinition.Number))
+        {
+            // This plug-in is not only called when a character is created, but also for characters which
+            // were created outside the game (e.g. on the database or with the admin panel) and are missing
+            // their inventory. Adding the skill again would give the character the same skill twice, which
+            // its skill list can't handle.
+            player.Logger.LogDebug("Skill {0} is already learned by character {1}.", skillDefinition.Name, createdCharacter.Name);
+            return;
+        }
+
         var skillEntry = player.PersistenceContext.CreateNew<SkillEntry>();
         skillEntry.Skill = skillDefinition;
         createdCharacter.LearnedSkills.Add(skillEntry);

src/Persistence/Initialization/Updates/RegenerationsRefactorPlugInBase.cs

@@ -39,9 +39,11 @@ public abstract class RegenerationsRefactorPlugInBase : UpdatePlugInBase
     /// <inheritdoc />
     protected override async ValueTask ApplyAsync(IContext context, GameConfiguration gameConfiguration)
     {
-        // Create new Stats
-        var isResting = context.CreateNew<AttributeDefinition>(Stats.IsResting.Id, Stats.IsResting.Designation, Stats.IsResting.Description);
-        gameConfiguration.Attributes.Add(isResting);
+        // Create new Stats. Only when they don't exist yet - a configuration which was initialized after
+        // these attributes were introduced already contains them, and adding them again would create
+        // duplicated attribute definitions.
+        this.AddStatIfNotExists(context, gameConfiguration, Stats.IsResting);
+        var isResting = Stats.IsResting.GetPersistent(gameConfiguration);
 
         var areTwoWeaponsEquipped = Stats.AreTwoWeaponsEquipped.GetPersistent(gameConfiguration);
         var attackSpeedByWeapon = Stats.AttackSpeedByWeapon.GetPersistent(gameConfiguration);
@@ -57,6 +59,14 @@ protected override async ValueTask ApplyAsync(IContext context, GameConfiguratio
 
         gameConfiguration.CharacterClasses.ForEach(charClass =>
         {
+            void AddStatAttributeIfNotExists(AttributeDefinition attribute)
+            {
+                if (charClass.StatAttributes.All(sa => sa.Attribute != attribute))
+                {
+                    charClass.StatAttributes.Add(context.CreateNew<StatAttributeDefinition>(attribute, 0, false));
+                }
+            }
+
             var attrCombos = charClass.AttributeCombinations;
 
             // Remove temp attack speed combos
@@ -161,9 +171,11 @@ protected override async ValueTask ApplyAsync(IContext context, GameConfiguratio
 
             charClass.BaseAttributeValues.Add(context.CreateNew<ConstValueAttribute>(0.037f, manaRecoveryMultiplier, AggregateType.AddRaw));
 
-            // Create new StatAttributDefinitions
-            charClass.StatAttributes.Add(context.CreateNew<StatAttributeDefinition>(isResting, 0, false));
-            charClass.StatAttributes.Add(context.CreateNew<StatAttributeDefinition>(nearbyPartyMemberCount, 0, false));
+            // Create new StatAttributDefinitions, if the class doesn't have them already. A class which was
+            // initialized after these stats were introduced already has them, and a character of a class
+            // holding the same stat attribute twice can't enter the game at all.
+            AddStatAttributeIfNotExists(isResting);
+            AddStatAttributeIfNotExists(nearbyPartyMemberCount);
 
             // Change base ability recovery multiplier
             if (charClass.Number != 4 && charClass.Number != 6 && charClass.Number != 7) // DK classes

src/Persistence/Initialization/Updates/RegenerationsRefactorPlugInSeason6.cs

@@ -40,14 +40,15 @@ protected override async ValueTask ApplyAsync(IContext context, GameConfiguratio
     {
         await base.ApplyAsync(context, gameConfiguration).ConfigureAwait(false);
 
-        // Create new Stats
-        var isShieldRecoveryActive = context.CreateNew<AttributeDefinition>(Stats.IsShieldRecoveryActive.Id, Stats.IsShieldRecoveryActive.Designation, Stats.IsShieldRecoveryActive.Description);
-        gameConfiguration.Attributes.Add(isShieldRecoveryActive);
-        var shieldRecoveryHiatus = context.CreateNew<AttributeDefinition>(Stats.ShieldRecoveryHiatus.Id, Stats.ShieldRecoveryHiatus.Designation, Stats.ShieldRecoveryHiatus.Description);
-        gameConfiguration.Attributes.Add(shieldRecoveryHiatus);
-        var shieldRecoveryRampFactor = context.CreateNew<AttributeDefinition>(Stats.ShieldRecoveryRampFactor.Id, Stats.ShieldRecoveryRampFactor.Designation, Stats.ShieldRecoveryRampFactor.Description);
+        // Create new Stats, but only when the configuration doesn't contain them already - it would
+        // create duplicated attribute definitions otherwise.
+        this.AddStatIfNotExists(context, gameConfiguration, Stats.IsShieldRecoveryActive);
+        var isShieldRecoveryActive = Stats.IsShieldRecoveryActive.GetPersistent(gameConfiguration);
+        this.AddStatIfNotExists(context, gameConfiguration, Stats.ShieldRecoveryHiatus);
+        var shieldRecoveryHiatus = Stats.ShieldRecoveryHiatus.GetPersistent(gameConfiguration);
+        this.AddStatIfNotExists(context, gameConfiguration, Stats.ShieldRecoveryRampFactor);
+        var shieldRecoveryRampFactor = Stats.ShieldRecoveryRampFactor.GetPersistent(gameConfiguration);
         shieldRecoveryRampFactor.MaximumValue = 3;
-        gameConfiguration.Attributes.Add(shieldRecoveryRampFactor);
 
         var isInSafezone = Stats.IsInSafezone.GetPersistent(gameConfiguration);
         var shieldRecoveryMultiplier = Stats.ShieldRecoveryMultiplier.GetPersistent(gameConfiguration);

src/Persistence/Initialization/Updates/RemoveDuplicateStatAttributesPlugIn075.cs

@@ -0,0 +1,23 @@
+// <copyright file="RemoveDuplicateStatAttributesPlugIn075.cs" company="MUnique">
+// Licensed under the MIT License. See LICENSE file in the project root for full license information.
+// </copyright>
+
+namespace MUnique.OpenMU.Persistence.Initialization.Updates;
+
+using System.Runtime.InteropServices;
+using MUnique.OpenMU.PlugIns;
+
+/// <summary>
+/// This update removes stat attributes which are defined more than once for a character class.
+/// </summary>
+[PlugIn]
+[Display(Name = PlugInName, Description = PlugInDescription)]
+[Guid("6B0A9D2C-6E4F-4C1B-9B5A-2E7F8D4C0A31")]
+public class RemoveDuplicateStatAttributesPlugIn075 : RemoveDuplicateStatAttributesPlugInBase
+{
+    /// <inheritdoc />
+    public override UpdateVersion Version => UpdateVersion.RemoveDuplicateStatAttributes075;
+
+    /// <inheritdoc />
+    public override string DataInitializationKey => Version075.DataInitialization.Id;
+}

src/Persistence/Initialization/Updates/RemoveDuplicateStatAttributesPlugIn095d.cs

@@ -0,0 +1,23 @@
+// <copyright file="RemoveDuplicateStatAttributesPlugIn095d.cs" company="MUnique">
+// Licensed under the MIT License. See LICENSE file in the project root for full license information.
+// </copyright>
+
+namespace MUnique.OpenMU.Persistence.Initialization.Updates;
+
+using System.Runtime.InteropServices;
+using MUnique.OpenMU.PlugIns;
+
+/// <summary>
+/// This update removes stat attributes which are defined more than once for a character class.
+/// </summary>
+[PlugIn]
+[Display(Name = PlugInName, Description = PlugInDescription)]
+[Guid("D1F4C7A8-3B62-4E05-8A9C-5C1B6E3F7D24")]
+public class RemoveDuplicateStatAttributesPlugIn095D : RemoveDuplicateStatAttributesPlugInBase
+{
+    /// <inheritdoc />
+    public override UpdateVersion Version => UpdateVersion.RemoveDuplicateStatAttributes095d;
+
+    /// <inheritdoc />
+    public override string DataInitializationKey => Version095d.DataInitialization.Id;
+}