The Adventurer's skill list, and the rows a class change leaves behind

src/NosCore.GameObject/Ecs/Extensions/PlayerBundleExtensions.cs

@@ -945,7 +945,7 @@ public static async Task GenerateMailAsync(this ClientSession session, IEnumerab
     public static async Task ChangeClassAsync(this ClientSession session, CharacterClassType classType,
         IOptions<WorldConfiguration> worldConfiguration,
         IExperienceService experienceService, IJobExperienceService jobExperienceService, IHeroExperienceService heroExperienceService,
-        IItemGenerationService itemProvider)
+        IItemGenerationService itemProvider, Services.SkillService.ISkillService skillService)
     {
         var character = session.Character;
         var inventoryService = character.InventoryService;
@@ -983,6 +983,18 @@ await session.SendPacketAsync(new SayiPacket
         character.Hp = character.MaxHp;
         character.Mp = character.MaxMp;
 
+        // The old class's skills are no longer usable and the new one's are not there yet:
+        // without this you change job and keep the previous bar, full of icons the client
+        // refuses to cast because they do not belong to the class.
+        character.Skills.Clear();
+
+        // Emptying the list is not enough - the rows behind it survive, and the next login loads
+        // them straight back on top of the new class's. LearnClassSkillsAsync then grants what
+        // the job level allows, and the change has just put that back to 1, so it starts from
+        // the first skill.
+        await skillService.ForgetSkillsOfOtherClassesAsync(character).ConfigureAwait(false);
+        await skillService.LearnClassSkillsAsync(character).ConfigureAwait(false);
+
         var itemsToAdd = worldConfiguration.Value.BasicEquipments.TryGetValue(classType.ToString(), out var byOrigin)
             && byOrigin.TryGetValue(StarterOrigin.CreateAndUpgrade, out var pack)
             ? pack

src/NosCore.GameObject/Messaging/Handlers/Nrun/ChangeClassHandler.cs

@@ -33,7 +33,8 @@ public sealed class ChangeClassHandler(
         IExperienceService experienceService,
         IJobExperienceService jobExperienceService,
         IHeroExperienceService heroExperienceService,
-        NosCore.GameObject.Services.ItemGenerationService.IItemGenerationService itemProvider) : INrunEventHandler
+        NosCore.GameObject.Services.ItemGenerationService.IItemGenerationService itemProvider,
+        Services.SkillService.ISkillService skillService) : INrunEventHandler
     {
         public NrunRunnerType Runner => NrunRunnerType.ChangeClass;
 
@@ -86,7 +87,7 @@ await session.SendPacketAsync(new SayiPacket
             }
 
             await session.ChangeClassAsync(classType, worldConfiguration, experienceService,
-                jobExperienceService, heroExperienceService, itemProvider);
+                jobExperienceService, heroExperienceService, itemProvider, skillService);
         }
     }
 }

src/NosCore.GameObject/Services/SkillService/ISkillService.cs

@@ -8,5 +8,15 @@ public interface ISkillService
         Task LoadSkill(ICharacterEntity character);
 
         Task<bool> LearnClassSkillsAsync(ICharacterEntity character);
+
+        /// <summary>
+        /// Deletes the skills the character can no longer learn, from memory and from the
+        /// database both.
+        /// </summary>
+        /// <remarks>
+        /// A class change already emptied the in-memory list; the rows behind it stayed, and
+        /// came back on the next login. See the implementation for what that did.
+        /// </remarks>
+        Task ForgetSkillsOfOtherClassesAsync(ICharacterEntity character);
     }
 }

src/NosCore.GameObject/Services/SkillService/SkillService.cs

@@ -18,6 +18,11 @@ public class SkillService(IDao<CharacterSkillDto, Guid> characterSkillDao, List<
     {
         public async Task LoadSkill(ICharacterEntity character)
         {
+            // Characters who changed class before the deletion below existed are still carrying
+            // the old rows. Clearing them at login, and not only at the next class change, means
+            // those characters heal themselves instead of staying broken for ever.
+            await ForgetSkillsOfOtherClassesAsync(character).ConfigureAwait(false);
+
             var characterSkills = characterSkillDao.Where(x => x.CharacterId == character.VisualId).Adapt<List<CharacterSkill>>() ?? new List<CharacterSkill>();
             var skillToUse = skills.Where(x => characterSkills.Select(s => s.SkillVNum).Contains(x.SkillVNum));
             character.Skills.Clear();
@@ -48,22 +53,79 @@ await character.SendPacketAsync(new SkiPacket
             }).ConfigureAwait(false);
         }
 
+        /// <summary>
+        /// The Adventurer's skills, listed one by one.
+        ///
+        /// <b>They cannot be selected by class like every other one</b>, and that is this table's
+        /// trap: class 0 does not mean "Adventurer", it is the scrap container where 193 entries
+        /// end up - passives, monster skills, things with no cost and no cast. Filtering by class
+        /// 0 gave an Adventurer <i>all</i> of them, and the bar filled with icons the client will
+        /// not cast: the symptom is "the skills arrive but they do not work".
+        ///
+        /// These are the real numbers, from the original game. 209 does not exist.
+        /// </summary>
+        private static readonly short[] AdventurerSkills =
+            { 200, 201, 202, 203, 204, 205, 206, 207, 208, 210 };
+
+        /// <summary>The skills this class can hold.</summary>
+        private IEnumerable<SkillDto> Learnable(ICharacterEntity character) =>
+            character.Class == CharacterClassType.Adventurer
+                ? skills.Where(s => AdventurerSkills.Contains(s.SkillVNum))
+                : skills.Where(s => s.Class == (byte)character.Class);
+
+        /// <summary>
+        /// A class change was only half done. The change empties the in-memory list and learns the
+        /// new class's skills, but nothing ever deleted the rows behind the old ones - so the next
+        /// login loaded both sets back.
+        ///
+        /// That is not a cosmetic leftover. Cast ids are numbered per class and start at zero, so
+        /// an Archer who used to be an Adventurer ended up knowing two skills answering to cast 0:
+        /// Swing (melee) and Archery (ranged). Which one the resolver returned came down to
+        /// dictionary order.
+        ///
+        /// The visible symptom was a basic attack computed off the <b>wrong weapon</b>: Swing is a
+        /// melee skill, a melee skill selects the secondary-weapon profile on an Archer, and the
+        /// bow in the main hand counted for nothing.
+        /// </summary>
+        public async Task ForgetSkillsOfOtherClassesAsync(ICharacterEntity character)
+        {
+            var keep = Learnable(character).Select(s => s.SkillVNum).ToHashSet();
+            var characterId = character.VisualId;
+
+            foreach (var stale in characterSkillDao.Where(x => x.CharacterId == characterId)?
+                         .Where(x => !keep.Contains(x.SkillVNum)).ToList() ?? [])
+            {
+                await characterSkillDao.TryDeleteAsync(stale.Id).ConfigureAwait(false);
+                character.Skills.TryRemove(stale.SkillVNum, out _);
+            }
+        }
+
         public async Task<bool> LearnClassSkillsAsync(ICharacterEntity character)
         {
             var classByte = (byte)character.Class;
             var learned = false;
-            foreach (var skill in skills.Where(s => s.Class == classByte && s.LevelMinimum <= character.JobLevel))
+            foreach (var skill in Learnable(character).Where(s => s.LevelMinimum <= character.JobLevel))
             {
                 if (character.Skills.ContainsKey(skill.SkillVNum))
                 {
                     continue;
                 }
 
+                // The row's existing database id is reused when there is one. With a fresh Guid
+                // every time, each call inserted one more row for the same skill: in memory it did
+                // not show, because the dictionary is keyed by skill number and collapses them,
+                // but the rows piled up behind it.
+                var characterId = character.VisualId;
+                var skillVNum = skill.SkillVNum;
+                var existing = await characterSkillDao
+                    .FirstOrDefaultAsync(x => x.CharacterId == characterId && x.SkillVNum == skillVNum)
+                    .ConfigureAwait(false);
+
                 var entry = new CharacterSkill
                 {
-                    Id = Guid.NewGuid(),
-                    CharacterId = character.VisualId,
-                    SkillVNum = skill.SkillVNum,
+                    Id = existing?.Id ?? Guid.NewGuid(),
+                    CharacterId = characterId,
+                    SkillVNum = skillVNum,
                     Skill = skill,
                 };
                 if (character.Skills.TryAdd(skill.SkillVNum, entry))

src/NosCore.PacketHandlers/Command/ChangeClassPacketHandler.cs

@@ -28,14 +28,15 @@ namespace NosCore.PacketHandlers.Command
     public class ChangeClassPacketHandler(IPubSubHub pubSubHub,
         IOptions<WorldConfiguration> worldConfiguration, IExperienceService experienceService,
         IJobExperienceService jobExperienceService, IHeroExperienceService heroExperienceService,
-        IItemGenerationService itemProvider)
+        IItemGenerationService itemProvider,
+        NosCore.GameObject.Services.SkillService.ISkillService skillService)
         : PacketHandler<ChangeClassPacket>, IWorldPacketHandler
     {
         public override async Task ExecuteAsync(ChangeClassPacket changeClassPacket, ClientSession session)
         {
             if ((changeClassPacket.Name == session.Character.Name) || string.IsNullOrEmpty(changeClassPacket.Name))
             {
-                await session.ChangeClassAsync(changeClassPacket.ClassType, worldConfiguration, experienceService, jobExperienceService, heroExperienceService, itemProvider);
+                await session.ChangeClassAsync(changeClassPacket.ClassType, worldConfiguration, experienceService, jobExperienceService, heroExperienceService, itemProvider, skillService);
                 return;
             }
 

test/NosCore.GameObject.Tests/Services/SkillService/ClassSkillLearningTests.cs

@@ -0,0 +1,131 @@
+//  __  _  __    __   ___ __  ___ ___
+// |  \| |/__\ /' _/ / _//__\| _ \ __|
+// | | ' | \/ |`._`.| \_| \/ | v / _|
+// |_|\__|\__/ |___/ \__/\__/|_|_\___|
+//
+
+using System;
+using System.Collections.Generic;
+using System.Linq;
+using System.Threading.Tasks;
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+using Moq;
+using NosCore.Dao.Interfaces;
+using NosCore.Data.Dto;
+using NosCore.Data.StaticEntities;
+using NosCore.GameObject.Networking.ClientSession;
+using NosCore.Shared.Enumerations;
+using NosCore.Tests.Shared;
+
+namespace NosCore.GameObject.Tests.Services.SkillService
+{
+    // This table's trap: **class 0 does not mean "Adventurer"**. It is the scrap
+    // container where 193 entries end up - passives, monster skills, things with no cost and no cast.
+    //
+    // Filtering by class as is done for the other jobs, an Adventurer got all of them:
+    // the bar filled with icons the client will not cast. Trying it with the client in hand the
+    // symptom was "the skills arrive but do not work", and it was exactly this.
+    [TestClass]
+    public class ClassSkillLearningTests
+    {
+        private NosCore.GameObject.Services.SkillService.SkillService _service = null!;
+        private ClientSession _session = null!;
+
+        private static readonly List<SkillDto> Catalog = new()
+        {
+            // the Adventurer's real skills, listed one by one from the original game
+            new SkillDto { SkillVNum = 200, Class = 0, LevelMinimum = 0, CastId = 0 },
+            new SkillDto { SkillVNum = 201, Class = 0, LevelMinimum = 0, CastId = 1 },
+            new SkillDto { SkillVNum = 208, Class = 0, LevelMinimum = 0, CastId = 8 },
+            new SkillDto { SkillVNum = 210, Class = 0, LevelMinimum = 0, CastId = 10 },
+            // 209 does NOT exist as an Adventurer skill
+            new SkillDto { SkillVNum = 209, Class = 0, LevelMinimum = 0, CastId = 9 },
+            // scrap: the same class 0, but they are passives and monster skills
+            new SkillDto { SkillVNum = 1, Class = 0, LevelMinimum = 0, CastId = 4, SkillType = 3 },
+            new SkillDto { SkillVNum = 17, Class = 0, LevelMinimum = 0, CastId = 0, SkillType = 3 },
+            new SkillDto { SkillVNum = 999, Class = 0, LevelMinimum = 0, CastId = 4, SkillType = 3 },
+            // a real job's skill
+            new SkillDto { SkillVNum = 220, Class = 1, LevelMinimum = 0, CastId = 0 },
+            new SkillDto { SkillVNum = 221, Class = 1, LevelMinimum = 5, CastId = 1 },
+            new SkillDto { SkillVNum = 222, Class = 1, LevelMinimum = 50, CastId = 2 },
+        };
+
+        [TestInitialize]
+        public async Task SetupAsync()
+        {
+            await TestHelpers.ResetAsync();
+            _session = await TestHelpers.Instance.GenerateSessionAsync();
+            _service = new NosCore.GameObject.Services.SkillService.SkillService(
+                new Mock<IDao<CharacterSkillDto, Guid>>().Object, Catalog);
+        }
+
+        [TestMethod]
+        public async Task AdventurerGetsOnlyItsOwnSkills()
+        {
+            _session.Character.Class = CharacterClassType.Adventurer;
+            _session.Character.JobLevel = 20;
+
+            await _service.LearnClassSkillsAsync(_session.Character);
+
+            var learned = _session.Character.Skills.Keys.ToList();
+            CollectionAssert.Contains(learned, (short)200);
+            CollectionAssert.Contains(learned, (short)210);
+        }
+
+        [TestMethod]
+        public async Task AdventurerDoesNotGetTheJunkBucket()
+        {
+            // It is the heart of the defect: 193 scrap entries share class 0.
+            _session.Character.Class = CharacterClassType.Adventurer;
+            _session.Character.JobLevel = 20;
+
+            await _service.LearnClassSkillsAsync(_session.Character);
+
+            var learned = _session.Character.Skills.Keys.ToList();
+            CollectionAssert.DoesNotContain(learned, (short)1);
+            CollectionAssert.DoesNotContain(learned, (short)17);
+            CollectionAssert.DoesNotContain(learned, (short)999);
+        }
+
+        [TestMethod]
+        public async Task TwoHundredAndNineIsNotAnAdventurerSkill()
+        {
+            // A hole in the numbering that the original game skips explicitly. The
+            // test character had it, because the class filter did not exclude it.
+            _session.Character.Class = CharacterClassType.Adventurer;
+            _session.Character.JobLevel = 20;
+
+            await _service.LearnClassSkillsAsync(_session.Character);
+
+            CollectionAssert.DoesNotContain(_session.Character.Skills.Keys.ToList(), (short)209);
+        }
+
+        [TestMethod]
+        public async Task OtherClassesStillFilterByClass()
+        {
+            _session.Character.Class = CharacterClassType.Swordsman;
+            _session.Character.JobLevel = 10;
+
+            await _service.LearnClassSkillsAsync(_session.Character);
+
+            var learned = _session.Character.Skills.Keys.ToList();
+            CollectionAssert.Contains(learned, (short)220);
+            CollectionAssert.Contains(learned, (short)221);
+            CollectionAssert.DoesNotContain(learned, (short)222);   // oltre il livello di lavoro
+            CollectionAssert.DoesNotContain(learned, (short)200);   // e niente roba di classe 0
+        }
+
+        [TestMethod]
+        public async Task LearningTwiceDoesNotDuplicate()
+        {
+            _session.Character.Class = CharacterClassType.Swordsman;
+            _session.Character.JobLevel = 10;
+
+            await _service.LearnClassSkillsAsync(_session.Character);
+            var afterFirst = _session.Character.Skills.Count;
+            await _service.LearnClassSkillsAsync(_session.Character);
+
+            Assert.AreEqual(afterFirst, _session.Character.Skills.Count);
+        }
+    }
+}

test/NosCore.PacketHandlers.Tests/Command/ChangeClassPacketHandlerTests.cs

@@ -47,7 +47,8 @@ public async Task SetupAsync()
 
             Handler = new ChangeClassPacketHandler(PubSubHub.Object,
                 TestHelpers.Instance.WorldConfiguration, new ExperienceService(), new JobExperienceService(), new HeroExperienceService(),
-                TestHelpers.Instance.GenerateItemProvider());
+                TestHelpers.Instance.GenerateItemProvider(),
+                new Mock<NosCore.GameObject.Services.SkillService.ISkillService>().Object);
         }
 
         [TestMethod]